feat: font panel config #1666
Merged
feat: font panel config #1666
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 35f660f in 2 minutes and 48 seconds
More details
- Looked at
785lines of code in10files - Skipped
1files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/FontFamily.tsx:40
- Draft comment:
The prop 'showAddButton' defaults to true but the comment says 'Default to false'. Ensure the default value matches documentation. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. apps/studio/electron/main/assets/fonts.ts:217
- Draft comment:
Use async fs.writeFile instead of fs.writeFileSync to avoid blocking the main thread. - Reason this comment was not posted:
Marked as duplicate.
3. apps/studio/electron/main/assets/fonts.ts:206
- Draft comment:
Ensure weight and style values are correctly formatted (e.g., quoted if needed) as they are inserted without quotes. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The parsing code explicitly handles both quoted strings and numeric literals for weights. The Next.js font API likely accepts both formats. The code is already handling the conversion correctly in the parser. The suggestion to force quotes could actually make things worse by double-quoting numbers that should be numeric.
The comment raises a valid concern about data formatting consistency. There could be edge cases where the weight values need to be strings rather than numbers.
The existing code already handles both string and numeric weights appropriately during parsing, and the Next.js API accepts both formats, so forcing quotes is unnecessary and could be problematic.
The comment should be deleted as it suggests a change that could introduce problems while the current code already handles the formatting correctly.
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/FontFamily.tsx:40
- Draft comment:
The comment for the default value of showAddButton is inconsistent with its actual default (true). Update the comment or default value. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/FontFamily.tsx:40
- Draft comment:
Typographical issue: The comment for 'showAddButton' on line 40 states 'Default to false', but the property is actually defaulting to true. Please update the comment to reflect the correct default value. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. packages/models/src/constants/editor.ts:48
- Draft comment:
Typo: 'Potrait' should be corrected to 'Portrait' in the Orientation enum for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_150QmB2Ugwgemt6Q
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
| newContent += fontConfig; | ||
|
|
||
| await fs.writeFileSync(fontPath, newContent); |
There was a problem hiding this comment.
Avoid using await fs.writeFileSync in an async function. Use fs.promises.writeFile or the async fs.writeFile instead.
Suggested change
| await fs.writeFileSync(fontPath, newContent); | |
| await fs.promises.writeFile(fontPath, newContent); |
| // If we found and removed the font, generate new code | ||
| if (removedFont) { | ||
| const { code } = generate(ast); | ||
| await fs.writeFileSync(fontPath, code); |
There was a problem hiding this comment.
Avoid using fs.writeFileSync in removeFont; switch to an asynchronous write method.
Suggested change
| await fs.writeFileSync(fontPath, code); | |
| await fs.promises.writeFile(fontPath, code); |
apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/index.tsx
Outdated
Show resolved
Hide resolved
spartan-vutrannguyen
force-pushed
the
feat/font-panel-config
branch
from
2da2c48 to
bede7d5
Compare
Kitenite
reviewed
Mar 27, 2025
| @@ -0,0 +1,1012 @@ | |||
| import * as fs from 'fs'; | |||
There was a problem hiding this comment.
This should probably be refactored. The file is too large should be split into multiple files based on related functionalities.
There was a problem hiding this comment.
So instead of one file, make it a folder with multiple files. For example, helpers, remove, etc. can be in their own files. Will help with organization, and readability
There was a problem hiding this comment.
Sure, I will split them into smaller chunks
apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/FontFamily.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 2142737 in 3 minutes and 17 seconds
More details
- Looked at
1807lines of code in15files - Skipped
1files when reviewing. - Skipped posting
19drafted comments based on config settings.
1. apps/studio/electron/main/assets/fonts.ts:338
- Draft comment:
Regex used to match font imports may lead to false positives if import order or spacing changes. Consider using more robust parsing rather than regex. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment raises a valid concern - regex parsing of imports can be fragile. However, the codebase already uses proper AST parsing via @babel/parser in many places. The regex is only used as a quick check before doing more complex AST manipulation. The regex approach here is actually reasonable since it's just a preliminary check before the real work is done with the AST.
The comment identifies a real potential issue with regex parsing. AST parsing would be more robust. But is this really a significant enough issue to warrant a code change?
While AST parsing would be more robust, the current regex usage is limited and conservative. The code already uses AST parsing for the complex manipulations. The regex is just an optimization to avoid unnecessary AST work.
The comment should be deleted. While technically correct, the current implementation is reasonable and the suggested change would add complexity for minimal benefit.
2. apps/studio/electron/main/assets/fonts.ts:463
- Draft comment:
In updateFileWithImport, the replacement of import statements via regex could be error prone if the file's structure varies. Consider refining or adding comments for maintainability. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. apps/studio/electron/main/assets/fonts.ts:855
- Draft comment:
The traverseClassName function’s callback signature is loosely typed. Consider adding proper TypeScript types for its callback parameter for clarity and type safety. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. apps/studio/electron/main/assets/fonts.ts:900
- Draft comment:
The modifyTailwindConfig visitor function parameter is untyped. Adding explicit types for the path parameter may improve clarity and prevent future issues. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
5. apps/studio/src/lib/editor/engine/font/index.ts:50
- Draft comment:
Consider adding error handling or user feedback when invokeMainChannel fails, so that errors are propagated or logged appropriately. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%
None
6. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/FontFamily.tsx:47
- Draft comment:
Local state 'expanded' is initialized to false. If the UI needs to remember expansion state across renders or sessions, consider lifting the state up or persisting it. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/index.tsx:100
- Draft comment:
Deduplication of fonts based on search query uses filter with findIndex. Ensure that this logic covers all edge cases (e.g. varying casing) and is well tested. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%
None
8. packages/models/src/assets/index.ts:55
- Draft comment:
The Font interface seems appropriate. Ensure that any additional font properties required in the UI are added and documented. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
9. packages/models/src/constants/editor.ts:76
- Draft comment:
Font folder path is defined as 'app/fonts.ts'. Confirm this location works across various project structures and consider adding tests to validate configuration paths. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%
None
10. apps/studio/electron/main/assets/fonts.ts:186
- Draft comment:
Consider using asynchronous file reading (e.g. readFile) instead of fs.readFileSync to avoid blocking the main thread. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
11. apps/studio/electron/main/assets/fonts.ts:338
- Draft comment:
Regex checks for existing import/export (e.g. exportRegex) may be brittle; consider using AST-based inspection for robustness. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
12. apps/studio/electron/main/assets/fonts.ts:516
- Draft comment:
Using async callbacks inside Babel traverse may not work as expected since Babel's traverse doesn't await async callbacks. Confirm that intended updates occur reliably. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment raises a valid technical concern about async behavior in Babel traverse. However, looking at the code more carefully:
- The async callback is not used directly in Babel's traverse - it's wrapped in our own traverseClassName function
- The traverseClassName function properly handles the async operations by awaiting them
- The code appears to be working as intended, with proper error handling
- The comment is speculative ("may not work") rather than pointing to a concrete issue
The comment identifies a real technical concern about async operations in AST traversal. If there was a real issue here, it could cause subtle bugs that are hard to track down.
While the concern is valid in general, this specific implementation handles async operations correctly through proper wrapping and error handling. The comment is speculative rather than pointing to an actual problem.
The comment should be deleted because it is speculative ("may not work") rather than pointing to a concrete issue, and the code actually handles async operations correctly.
13. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/FontFamily.tsx:50
- Draft comment:
Good use of conditional props for actions. Ensure onSetFont action properly integrates global state changes beyond UI toggle. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
14. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/SystemFont.tsx:11
- Draft comment:
Ensure that invoking scanFonts and getDefaultFont on mount covers edge cases when fonts update dynamically. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
15. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/index.tsx:99
- Draft comment:
The filtering and deduplication logic for fonts looks sound; verify edge cases where font names may differ in case or spacing. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
16. packages/models/src/constants/editor.ts:76
- Draft comment:
Ensure FONT_FOLDER setting ('app/fonts.ts') accurately reflects the project's font config file path and is consistently used across modules. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
17. apps/studio/src/lib/editor/engine/font/index.ts:115
- Draft comment:
Typographical consistency: In the catch block of getDefaultFont, the error message says 'Error getting current font:' on line 115. Consider changing it to 'Error getting default font:' to match the method's purpose. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While the comment is technically correct about the inconsistency, error messages in catch blocks are primarily for debugging purposes. The inconsistency doesn't affect functionality and is a very minor issue. According to the rules, we should not make purely informative comments or comments about obvious/unimportant issues.
The inconsistency could potentially cause confusion during debugging. Error message consistency can be important for log parsing and troubleshooting.
While consistency in error messages has value, this is too minor of an issue to warrant a PR comment. The meaning is clear enough for debugging purposes.
Delete this comment as it addresses a trivial wording inconsistency that doesn't impact functionality or code quality significantly.
18. packages/models/src/constants/editor.ts:48
- Draft comment:
Typographical error: The enum value 'Potrait' in the Orientation enum appears to be misspelled. Consider correcting both the key and value to 'Portrait' to ensure clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
19. packages/models/src/constants/ipc.ts:64
- Draft comment:
Typo: In the 'AST' section, the enum constant GET_TEMPLATE_NODE_CLASS has a string value of 'get-template-node-classes'. The constant name uses singular 'CLASS' while the value uses plural 'classes'. Consider aligning these for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_dGy3MezHVNuLNsVI
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| font, | ||
| }); | ||
|
|
||
| setTimeout(() => { |
There was a problem hiding this comment.
Using setTimeout to trigger scanFonts after add/remove operations may be unreliable. Consider awaiting the result of the font update or chaining promises.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 22b0500 in 2 minutes and 16 seconds
More details
- Looked at
2539lines of code in11files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. apps/studio/electron/main/assets/fonts/font.ts:177
- Draft comment:
Avoid using 'await' with writeFileSync in removeFont; these calls are synchronous. - Reason this comment was not posted:
Marked as duplicate.
2. apps/studio/src/lib/editor/engine/font/index.ts:91
- Draft comment:
Consider replacing setTimeout with a direct call to await getDefaultFont; using setTimeout delays can hide errors. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. apps/studio/electron/main/assets/fonts/font.ts:89
- Draft comment:
Avoid using 'await' with fs.writeFileSync since it is synchronous. Consider using an asynchronous writeFile (e.g. fs.promises.writeFile) to avoid blocking the event loop. - Reason this comment was not posted:
Marked as duplicate.
4. apps/studio/electron/main/assets/fonts/utils.ts:68
- Draft comment:
When constructing dynamic regex patterns using font IDs, ensure to escape special regex characters to prevent unintended matches or injection issues. - Reason this comment was not posted:
Comment looked like it was already resolved.
5. apps/studio/src/lib/editor/engine/font/index.ts:91
- Draft comment:
Using setTimeout to delay getDefaultFont() is a code smell; consider awaiting getDefaultFont() directly to ensure proper sequencing without arbitrary delays. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_IJ2AHFjp2Wn8noSW
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
| newContent += fontConfig; | ||
|
|
||
| await fs.writeFileSync(fontPath, newContent); |
There was a problem hiding this comment.
Avoid using await with synchronous functions like writeFileSync. Consider using fs.writeFile (async) or remove await.
Suggested change
| await fs.writeFileSync(fontPath, newContent); | |
| fs.writeFileSync(fontPath, newContent); |
|
|
||
| if (updatedAst) { | ||
| const { code } = generate(ast); | ||
| await fs.writeFileSync(layoutPath, code); |
There was a problem hiding this comment.
Avoid using await with writeFileSync in scanner.ts as well; writeFileSync is synchronous.
Suggested change
| await fs.writeFileSync(layoutPath, code); | |
| fs.writeFileSync(layoutPath, code); |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on f363c9f in 2 minutes and 8 seconds
More details
- Looked at
871lines of code in13files - Skipped
0files when reviewing. - Skipped posting
11drafted comments based on config settings.
1. apps/studio/electron/main/assets/fonts/font.ts:90
- Draft comment:
Avoid using 'await' with the synchronous fs.writeFileSync; use an asynchronous fs.writeFile instead to prevent blocking the event loop. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. apps/studio/electron/main/assets/fonts/font.ts:242
- Draft comment:
Guard against empty fontFiles before accessing fontFiles[0] for baseFontName extraction. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment points out a valid edge case - if fontFiles is empty, accessing fontFiles[0] would cause an error. However, this function is clearly meant to process font files, and an empty array would make no sense in this context. The function signature and usage implies that fontFiles should contain at least one font file. TypeScript would also catch this at compile time if fontFiles was empty.
The comment identifies a real potential runtime error. Even if it's not expected, defensive programming would suggest checking for empty arrays.
While technically correct, this is an implementation detail that should be handled by the caller. The function's purpose is to process font files - if there are no files, it shouldn't be called at all.
The comment should be deleted as it suggests handling an edge case that shouldn't occur in normal operation and would be better handled by the caller.
3. apps/studio/electron/main/assets/fonts/scanner.ts:387
- Draft comment:
Avoid using synchronous fs.writeFileSync in the scanner; consider using asynchronous file operations to avoid blocking. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/UploadModal.tsx:24
- Draft comment:
Good use of asynchronous file processing for uploads; consider handling errors if file.arrayBuffer() fails. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
5. packages/utility/src/font.ts:71
- Draft comment:
The regex-based removal and camelCase conversion look robust; ensure that edge cases (e.g., unusual file name patterns) are covered by tests. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
6. apps/studio/src/lib/editor/engine/font/index.ts:116
- Draft comment:
Consider error handling for projectRoot absence in uploadFonts; currently, the function returns silently if projectRoot is missing. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%
None
7. apps/studio/electron/main/assets/fonts/font.ts:90
- Draft comment:
Avoid using 'await' with fs.writeFileSync as it blocks the event loop. Consider using asynchronous methods (e.g. fs.promises.writeFile) instead. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. apps/studio/electron/main/assets/fonts/scanner.ts:387
- Draft comment:
Avoid using 'await fs.writeFileSync' in async functions; switch to an asynchronous file write to prevent blocking the event loop. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. apps/studio/src/lib/editor/engine/font/index.ts:92
- Draft comment:
Using setTimeout with a fixed 100ms delay to update the default font may lead to race conditions. Consider refactoring to await the state update or use a more robust mechanism. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. packages/utility/src/font.ts:53
- Draft comment:
The regex pattern for extracting font names is comprehensive. If this function is called frequently, consider pre-compiling the regex outside the function for potential performance improvements. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
11. packages/models/src/constants/editor.ts:48
- Draft comment:
Typo: The enum value 'Potrait' appears to be misspelled. Consider renaming it to 'Portrait' for clarity and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_RF4e0qS8Xu11HMXk
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
| // Save the file | ||
| const buffer = Buffer.from(fontFile.file.buffer); | ||
| fs.writeFileSync(filePath, buffer); |
There was a problem hiding this comment.
Synchronous file write (fs.writeFileSync) is used here in addLocalFont. Using an async method would be preferable to avoid blocking.
Suggested change
| fs.writeFileSync(filePath, buffer); | |
| await fs.promises.writeFile(filePath, buffer); |
There was a problem hiding this comment.
Same here, should await writeFile instead
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 24bc6bf in 1 minute and 50 seconds
More details
- Looked at
302lines of code in3files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. apps/studio/electron/main/assets/fonts/font.ts:91
- Draft comment:
Using 'await' with fs.writeFileSync was removed correctly since the sync version doesn't return a promise. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/studio/electron/main/assets/fonts/font.ts:250
- Draft comment:
New deletion of font files is added; ensure computed file paths are correctly validated to avoid accidental file removals. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to ensure that file paths are correctly validated to avoid accidental deletions. This falls under the category of asking the author to double-check or ensure something, which is against the rules.
3. apps/studio/electron/main/assets/fonts/font.ts:352
- Draft comment:
Prefixing font file paths with '../' in the local font configuration appears intentional; double-check relative paths for consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. packages/utility/src/font.ts:6
- Draft comment:
Enhancements in extractFontName (filtering out combined weight-style terms and numeric weights) improve name extraction; verify that edge cases are covered. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/SystemFont.tsx:18
- Draft comment:
Rendering a placeholder when no fonts are added improves UX. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. apps/studio/electron/main/assets/fonts/font.ts:205
- Draft comment:
The code removes the '../' prefix from the font file path (line 205–208). Verify that this assumption about the directory structure holds in all cases. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify an assumption about the directory structure, which violates the rule against asking for confirmation or verification. It does not provide a specific suggestion or point out a clear issue.
7. apps/studio/electron/main/assets/fonts/font.ts:352
- Draft comment:
The local font configuration now prepends '../' to the font file path. Confirm that this relative path adjustment correctly references the font files from the consuming module. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
8. packages/utility/src/font.ts:110
- Draft comment:
In getFontFileName, baseName has spaces removed for the filename. Ensure that this normalization does not inadvertently affect font display names elsewhere in the application. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
9. packages/utility/src/font.ts:41
- Draft comment:
The early return in extractFontName for names containing spaces (without hyphens/underscores) bypasses further normalization. Confirm this behavior aligns with all expected font naming conventions. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_vtvmyQwnvBjlMD7Q
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
| newContent += fontConfig; | ||
|
|
||
| fs.writeFileSync(fontPath, newContent); |
There was a problem hiding this comment.
Using fs.writeFileSync in an async function may block the event loop. Consider using the asynchronous fs.promises.writeFile method for non‐blocking I/O.
Suggested change
| fs.writeFileSync(fontPath, newContent); | |
| await fs.promises.writeFile(fontPath, newContent); |
There was a problem hiding this comment.
@spartan-vutrannguyen , writeFileSync is discouraged. Please change to the promisified writeFile
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4e4a9c5 in 1 minute and 40 seconds
More details
- Looked at
56lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. apps/studio/electron/main/assets/fonts/font.ts:112
- Draft comment:
Good documentation update: Added step 6 for removing unused localFont import. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/studio/electron/main/assets/fonts/font.ts:137
- Draft comment:
Clear use of the hasRemainingLocalFonts flag to track remaining localFont declarations. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. apps/studio/electron/main/assets/fonts/font.ts:166
- Draft comment:
Nice check for localFont in declarations; consider refactoring to use array methods (e.g. some()) for clarity, though current implementation is correct. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. apps/studio/electron/main/assets/fonts/font.ts:245
- Draft comment:
Consider removing 'await' from fs.writeFileSync as it is a synchronous operation. - Reason this comment was not posted:
Comment was on unchanged code.
5. apps/studio/electron/main/assets/fonts/font.ts:163
- Draft comment:
Tracking remaining localFont declarations is clear. Consider ensuring that declarations outside ExportNamedDeclaration (if any) are also covered. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. apps/studio/electron/main/assets/fonts/font.ts:240
- Draft comment:
Consider using AST-based removal for the localFont import instead of regex replacement for increased robustness. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_PpPdQ6MQNJo1jU3I
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Some feedback on just the normal google font experience, not the custom fonts experience: 1. For a project I already had a Google font loaded into, it seems like it wasn't detected by the side panel. I wonder if it's because when I asked Onlook to add the font, it used next/font/google to bring it in. I think Next.js may do something special for fonts. `'use client'; import { useState, useEffect } from 'react'; const figtree = Figtree({ const ptSerif = PT_Serif({ 
Here are screenshots of the files: 
I know this can be a bit of an edge-case situation, but ideally the installed fonts also show up in the "Added Fonts" and can be controlled from the left panel. If this explodes the scope of this PR too much, I'm happy to push it to another PR. 2. I loaded in a font from Google Fonts (Andada Pro), and it made it in... 
When I set Inter as the default, it looks like this: 
But when I set Andada Pro as the default, it looks like this: The code seems to still include Inter. It seems like a few things are happening....
I used Cursor Tab to make these suggestions. When I used Cursor tab, it correctly imported the font, and now it looks like this (correct): Ok feedback on the custom font experience: 3. Seems to work well just loading the font in. We'll likely need to design how selecting an italic for a font would work, but otherwise good 4. Small style improvement 5. Font panel breaks for long text 
Let's make the width fixed for the labels, so in this case it would be... customCassannetPlusW.. (Default) [...] 
Then when the user hovers over the name, I'd like a tooltip off-to-the-side with the full-name, just like the layers. You can borrow the max-width styling and such from there. 
6. Suggest font weights from font-names For example, where the fonts say "medium" it should automatically choose the medium weight. For Medium Italic, we need to do a bit of design work on what the best experience is there, so don't worry about that just yet. 7. Refresh / update on font switch |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 3959ad9 in 2 minutes and 35 seconds
More details
- Looked at
1377lines of code in8files - Skipped
0files when reviewing. - Skipped posting
11drafted comments based on config settings.
1. apps/studio/electron/main/assets/fonts/font.ts:84
- Draft comment:
Consider using asynchronous file writes (fs.promises.writeFile) rather than sync methods to avoid blocking the main process. - Reason this comment was not posted:
Comment was on unchanged code.
2. apps/studio/electron/main/assets/fonts/scanner.ts:112
- Draft comment:
Error handling in the nested try/catch is good, but consider having consistent return types (avoid returning 'existedFonts || []' in multiple catch branches). - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%
None
3. apps/studio/electron/main/assets/fonts/utils.ts:151
- Draft comment:
The complex regex handling in removeFontsFromClassName is functional but consider extracting utility functions or adding more inline comments for better maintainability. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
4. apps/studio/electron/main/assets/fonts/watcher.ts:43
- Draft comment:
Ensure subscription cleanup and error logging provide enough context; consider logging the projectRoot with errors. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
5. apps/studio/electron/main/events/code.ts:190
- Draft comment:
The addition of the WATCH_FONT_FILE handler is clear; ensure that front-end UI triggers and handles potential asynchronous errors. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
6. apps/studio/src/lib/editor/engine/font/index.ts:54
- Draft comment:
Good use of MobX reaction to watch project changes. Consider adding disposal in case the project changes frequently to avoid memory leaks. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
7. packages/models/src/constants/ipc.ts:164
- Draft comment:
Ensure that the updated MainChannels for fonts (e.g. WATCH_FONT_FILE, ADD_FONT, etc.) are consistently used across front and back ends. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
8. apps/studio/electron/main/assets/fonts/font.ts:84
- Draft comment:
Consider using asynchronous file write methods (e.g. fs.promises.writeFile) instead of writeFileSync to avoid blocking the main thread. - Reason this comment was not posted:
Comment was on unchanged code.
9. apps/studio/electron/main/assets/fonts/layout.ts:88
- Draft comment:
The regex-based import update in updateFileWithImport may be brittle; consider using full AST manipulation for robust import handling. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. apps/studio/electron/main/assets/fonts/watcher.ts:52
- Draft comment:
The async call to syncFontsWithConfigs inside the event loop is not awaited. If errors occur, they might not be caught. Consider awaiting this call or using proper error handling. - Reason this comment was not posted:
Based on historical feedback, this comment is too similar to comments previously marked by users as bad.
11. apps/studio/src/lib/editor/engine/font/index.ts:23
- Draft comment:
The reaction that sets up the font watcher on project folder changes may create multiple watchers if not cleaned up properly. Ensure that prior subscriptions are disposed to avoid duplicate watchers. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_R6UfUw7QLPV345mf
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| newContent = fontImport + '\n' + newContent; | ||
| } | ||
|
|
||
| fs.writeFileSync(filePath, newContent); |
There was a problem hiding this comment.
Using fs.writeFileSync in updateFileWithImport may block the event loop; consider using an asynchronous approach.
Suggested change
| fs.writeFileSync(filePath, newContent); | |
| await fs.promises.writeFile(filePath, newContent); |
Kitenite
reviewed
Mar 31, 2025
packages/models/src/constants/ipc.ts
Outdated
| @@ -82,6 +82,9 @@ export enum MainChannels { | |||
| REPLACE_APP_STATE = 'replace-app-state', | |||
| UPDATE_PROJECTS = 'update-projects', | |||
|
|
|||
| // Fonts | |||
| WATCH_FONT_FILE = 'watch-font-file', | |||
There was a problem hiding this comment.
There's already a fonts group on line 163
|
|
||
| newContent += fontConfig; | ||
|
|
||
| fs.writeFileSync(fontPath, newContent); |
There was a problem hiding this comment.
@spartan-vutrannguyen , writeFileSync is discouraged. Please change to the promisified writeFile
|
|
||
| // Save the file | ||
| const buffer = Buffer.from(fontFile.file.buffer); | ||
| fs.writeFileSync(filePath, buffer); |
There was a problem hiding this comment.
Same here, should await writeFile instead
| private async watchFontFile(projectRoot: string) { | ||
| console.log('Setting up font file watcher for path:', projectRoot); | ||
| if (!projectRoot) { | ||
| console.log('No project root provided, skipping font file watcher setup'); |
| } | ||
|
|
||
| try { | ||
| console.log('Invoking WATCH_FONT_FILE with project root:', projectRoot); |
spartan-vutrannguyen
force-pushed
the
feat/font-panel-config
branch
from
3959ad9 to
d760b28
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d760b28 in 1 minute and 13 seconds
More details
- Looked at
252lines of code in3files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. apps/studio/electron/main/assets/fonts/font.ts:112
- Draft comment:
Consider merging the second AST traversal (for updating the import) with the initial traversal to avoid iterating the AST twice. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
2. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/FontFamily.tsx:44
- Draft comment:
The inline comment says 'Default to false' but the default value is set to true. Please update the comment or default to ensure they match. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. apps/studio/electron/main/assets/fonts/font.ts:112
- Draft comment:
Consider merging the second AST traversal (updating the import specifier) into the initial traversal to avoid an extra pass. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
4. apps/studio/electron/main/assets/fonts/font.ts:274
- Draft comment:
Consider using an AST-based approach for removing the localFont import instead of regex manipulation for consistency. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
5. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/FontFamily.tsx:44
- Draft comment:
The inline comment for 'showAddButton' default value is contradictory (stating 'Default to false' while default is set to true); please update for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/FontFamily.tsx:44
- Draft comment:
There is a mismatch in the default value setting for 'showAddButton': the parameter is set to 'true' but the comment indicates it should default to false. Please update the comment or the default value to ensure clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_4fYh0SAmXsDbLCkx
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 09770d7 in 2 minutes and 4 seconds
More details
- Looked at
713lines of code in9files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. packages/utility/src/font.ts:76
- Draft comment:
In the nested loops for checking combined weight and style (lines 76-84), consider breaking out once a match is found to avoid unnecessary iterations. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. packages/utility/src/font.ts:104
- Draft comment:
While the weight conversion logic works, consider normalizing the style value similarly for consistency, e.g. converting to lowercase or mapping to a canonical form. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. packages/utility/src/font.ts:3
- Draft comment:
Update the function's jsdoc comment to reflect that it now returns an object with font parts (family, weight, style) instead of just the font name. - Reason this comment was not posted:
Comment was on unchanged code.
4. packages/utility/src/font.ts:76
- Draft comment:
Consider refactoring the nested loops for combined term detection into a helper function to improve readability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. packages/utility/src/font.ts:103
- Draft comment:
Review the fallback behavior when weight normalization fails; ensure that returning the original weight string is the intended behavior. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/FontFamily.tsx:45
- Draft comment:
Typo: The comment for 'showAddButton' is misleading. It states 'Default to false' although the default is actually set to true. Please update the comment to reflect the correct default value. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/FontFamily.tsx:77
- Draft comment:
Minor Formatting: Consider removing the extraneous whitespace ('{' '}) on line 77 in the TooltipContent block to keep the code cleaner. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While the whitespace is unnecessary, this is an extremely minor issue. It doesn't affect functionality or readability significantly. The comment is technically correct but deals with an extremely trivial formatting issue. We should focus on more substantial code quality issues.
The whitespace might be intentional for some styling purpose I'm not aware of. Also, we were told not to comment on pure UI/styling changes.
The whitespace is clearly not functional as it's inside a JSX expression, and even if it was for styling, we're instructed not to comment on UI/styling changes.
While technically correct, this comment is too trivial and deals with UI/styling which we're instructed to ignore. It should be removed.
Workflow ID: wflow_3uEw3nlq94m2yNvU
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Great suggestions, @drfarrell ! I appreciate the insights and I’ve incorporated them into the latest commit. Thanks for taking the time to review!
One thing that Im working on is detect new font which has been added manually to the app Screen.Recording.2025-04-01.at.21.09.57.mov |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 2da75e8 in 3 minutes and 19 seconds
More details
- Looked at
171lines of code in2files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. apps/studio/electron/main/assets/fonts/font.ts:353
- Draft comment:
Good initialization of 'existingFontNode'. Consider adding a comment explaining its role to aid future maintenance. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/studio/electron/main/assets/fonts/font.ts:407
- Draft comment:
When merging config (pushing to 'src' array), check if duplicate entries might be added. Consider de-duplicating srcArrayElements. - Reason this comment was not posted:
Marked as duplicate.
3. apps/studio/electron/main/assets/fonts/font.ts:463
- Draft comment:
Ensure that adding the localFont import only once is sufficient if merging an existing font. Consider a comment to explain why the import isn't updated in merge case. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. apps/studio/src/lib/editor/engine/font/index.ts:48
- Draft comment:
Now logging an error when there is no project root. Consider standardizing error messages across all methods for consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. apps/studio/src/lib/editor/engine/font/index.ts:77
- Draft comment:
In methods like addFont, removeFont, setDefaultFont, and getDefaultFont the console error message mentions 'skipping font file watcher setup' even if the operation isn't about watching fonts. A more generic error message may be more appropriate. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. apps/studio/electron/main/assets/fonts/font.ts:354
- Draft comment:
Good use of 'existingFontNode' to track an existing export for merging. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. apps/studio/electron/main/assets/fonts/font.ts:463
- Draft comment:
The conditional import addition for 'localFont' is handled correctly when creating a new font configuration. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
8. apps/studio/src/lib/editor/engine/font/index.ts:64
- Draft comment:
Several methods log the same error message for a missing projectRoot. Consider customizing these messages to more accurately reflect the operation being skipped. - Reason this comment was not posted:
Comment was on unchanged code.
9. apps/studio/src/lib/editor/engine/font/index.ts:124
- Draft comment:
Using setTimeout in setDefaultFont to delay fetching the default font may be brittle. Consider refactoring to await the completion of the font setting process instead. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_8E6k3KiyZCtxmVgz
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| t.isObjectProperty(srcProp) && | ||
| t.isArrayExpression(srcProp.value) | ||
| ) { | ||
| srcProp.value.elements.push(...srcArrayElements); |
There was a problem hiding this comment.
Consider adding deduplication to avoid duplicate entries when merging new src elements into the existing font config.
Suggested change
| srcProp.value.elements.push(...srcArrayElements); | |
| srcProp.value.elements.push(...srcArrayElements.filter(el => !srcProp.value.elements.some(existingEl => existingEl.properties[0].value.value === el.properties[0].value.value))); |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 24b2554 in 2 minutes and 4 seconds
More details
- Looked at
330lines of code in8files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. apps/studio/src/lib/editor/engine/style/index.ts:44
- Draft comment:
updateFontFamily uses font.id for action but then applies font.family for update. Ensure this behavior is intentional to separate stored value from display. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
2. apps/studio/src/lib/editor/styles/group.ts:245
- Draft comment:
TextGroup now includes a fontFamily style. Verify its default value is handled correctly in the UI. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
3. apps/studio/src/routes/editor/EditPanel/StylesTab/index.tsx:50
- Draft comment:
FontInput is rendered when style type is Font. Confirm that the mapping aligns with the new Font style implementation. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
4. apps/studio/src/routes/editor/EditPanel/StylesTab/single/FontInput.tsx:15
- Draft comment:
convertFontString function strips '__' and hashes. Verify it consistently processes font values across the app. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
5. apps/studio/src/routes/editor/EditPanel/StylesTab/single/FontInput.tsx:53
- Draft comment:
In handleValueChange, setValue(newValue.id) is used but useEffect uses convertFontString. Consider normalizing the new value before setting state for UI consistency. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
6. apps/studio/src/routes/editor/EditPanel/StylesTab/single/FontInput.tsx:15
- Draft comment:
Consider strengthening the parsing logic in convertFontString to handle unexpected font string formats, as the current replacement rules (removing '__' and trailing hashes) might not cover all edge cases. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The function seems to handle the documented format well. The comment is speculative - it doesn't point to any specific issues or edge cases. Without seeing the actual font string formats used in the codebase, we can't know if there are real edge cases being missed. The function has basic error handling and follows a clear pattern.
I might be overlooking real edge cases that could cause issues in production. The function could potentially break with malformed input.
While edge cases are possible, the comment doesn't identify any specific issues. The function handles the documented format and has basic error handling. Speculative hardening without concrete use cases would be premature optimization.
Delete this comment as it's speculative and doesn't point to specific issues that need fixing. The current implementation appears sufficient for its documented use case.
7. apps/studio/src/routes/editor/EditPanel/StylesTab/single/FontInput.tsx:76
- Draft comment:
The add new font handler is currently a placeholder. Please implement the add font functionality or clearly mark it as pending to avoid confusion. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The code already has a clear TODO comment marking this as unimplemented. The comment doesn't add any new information beyond what's already in the code. The TODO comment is a standard way to mark pending work. The PR author is likely aware this is incomplete since they added the TODO.
Maybe having an unimplemented UI button could confuse users who try to click it. The comment raises a valid concern about user experience.
While user experience is important, the TODO comment already makes it clear this is pending work. The comment doesn't add actionable value beyond the existing TODO.
Delete the comment since the code already clearly marks this as pending work with a TODO comment. The review comment doesn't add new information.
8. apps/studio/src/routes/editor/EditPanel/StylesTab/single/FontInput.tsx:84
- Draft comment:
For better accessibility, consider adding an aria-label to the PopoverTrigger button so screen readers can announce its purpose. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The button already has visible text content that screen readers can use - it shows either the selected font name or "Select font". Adding an aria-label would be redundant since the button's purpose is already clear from its text content. The visible text is actually better than a static aria-label since it dynamically updates to show the current selection.
I could be wrong about accessibility best practices. Maybe there's a reason why an explicit aria-label would be better than relying on the button text.
Even if an aria-label could provide slightly better accessibility, the current implementation with visible text content is already accessible. The suggestion is not fixing a real accessibility issue.
The comment should be removed since the button is already accessible through its visible text content, and adding an aria-label would be redundant.
9. apps/studio/common/helpers/twTranslator/index.ts:475
- Draft comment:
Typo: In the 'hue-rotate' function within the 'backdrop-filter' mapping, the code incorrectly accessescustomTheme['backdrop-grayscale']. It should be updated to usecustomTheme['backdrop-hue-rotate']. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_7VnrvtzdrRfKDBSa
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c0166b7 in 2 minutes and 49 seconds
More details
- Looked at
62lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. packages/utility/src/font.ts:55
- Draft comment:
Consider setting default values for weight and style in the early return block (L55-L58) for consistency with the rest of the function. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. apps/studio/src/lib/editor/engine/font/index.ts:107
- Draft comment:
Good improvement: clearing _defaultFont when the removed font is the current default. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. apps/studio/src/lib/editor/engine/font/index.ts:127
- Draft comment:
Replacing setTimeout with await getDefaultFont() is cleaner and more reliable. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. packages/utility/src/font.ts:142
- Draft comment:
Default assignments for style and weight are added here; consider applying these defaults even when returning early (line 55) for consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_gdGrF2p5BbCjrDk5
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
This was referenced Apr 2, 2025
spartan-vutrannguyen
force-pushed
the
feat/font-panel-config
branch
from
c0166b7 to
e2e80f9
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e2e80f9 in 2 minutes and 59 seconds
More details
- Looked at
74lines of code in4files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. packages/utility/src/font.ts:142
- Draft comment:
Default font style is applied if missing. Verify that using DEFAULT_FONT_STYLE here aligns with all font files. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. packages/utility/src/font.ts:146
- Draft comment:
Default font weight is applied if missing. Ensure DEFAULT_FONT_WEIGHT is the desired fallback. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. apps/studio/common/helpers/twTranslator/index.ts:3423
- Draft comment:
Good removal of the debug log; eliminating console.log helps keep production logs clean. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. apps/studio/src/lib/editor/engine/font/index.ts:105
- Draft comment:
Resetting _defaultFont when the removed font is the default ensures state consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. apps/studio/src/lib/editor/engine/font/index.ts:127
- Draft comment:
Replacing setTimeout with await getDefaultFont() improves reliability; ensure that getDefaultFont returns a promise. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. packages/models/src/constants/fonts.ts:1
- Draft comment:
Added DEFAULT_FONT_STYLE and DEFAULT_FONT_WEIGHT constants to centralize default font settings – a clear improvement. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. packages/utility/src/font.ts:142
- Draft comment:
Default assignments for style and weight are applied here; note that the early exit (line 55) returns empty values. Confirm this difference is intentional for space-separated names. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
8. apps/studio/common/helpers/twTranslator/index.ts:477
- Draft comment:
Typo error: In the backdropFilterValConfig for the 'hue-rotate' property, the code usescustomTheme['backdrop-grayscale']instead ofcustomTheme['backdrop-hue-rotate']. Please correct the theme key. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. apps/studio/common/helpers/twTranslator/index.ts:1115
- Draft comment:
Typo error: In the filterValConfig for the 'hue-rotate' property, the code mistakenly usescustomTheme['grayscale']instead ofcustomTheme['hue-rotate']. Please update the property lookup to the correct key. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_7Q62ENu9ssp2gtT0
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
spartan-vutrannguyen
force-pushed
the
feat/font-panel-config
branch
from
e2e80f9 to
7431438
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 7431438 in 1 minute and 55 seconds
More details
- Looked at
62lines of code in3files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. apps/studio/src/lib/editor/engine/font/index.ts:123
- Draft comment:
Using await getDefaultFont removes the delay from setTimeout; ensure that any timing dependency (e.g. waiting for file system updates) is handled appropriately. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
2. packages/utility/src/font.ts:142
- Draft comment:
Ensure the early return for names with spaces (line 55) is intentional since it bypasses applying default weight/style; consider applying defaults for consistency. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. apps/studio/src/lib/editor/engine/font/index.ts:107
- Draft comment:
Good update: clears _defaultFont when the removed font is the default, ensuring later operations re-fetch the default correctly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. apps/studio/src/lib/editor/engine/font/index.ts:127
- Draft comment:
Replacing setTimeout with an awaited call to getDefaultFont ensures the default font is updated reliably without relying on arbitrary delays. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. packages/models/src/constants/fonts.ts:1
- Draft comment:
Default font constants added; this improves consistency when defaults are needed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. packages/utility/src/font.ts:1
- Draft comment:
Updated import to include DEFAULT_FONT_STYLE and DEFAULT_FONT_WEIGHT for consistent default handling. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. packages/utility/src/font.ts:139
- Draft comment:
Inserted default assignments ensure that if no style or weight is extracted, the defaults are applied, enhancing consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
Workflow ID: wflow_BfVNOSBg3P7PdYdh
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Kitenite
approved these changes
Apr 3, 2025
zongkelong
pushed a commit
to zongkelong/coolook
that referenced
this pull request
Apr 3, 2025
…slation_zh * 'main' of https://github.com/onlook-dev/onlook: fix: add validation when create new color group (onlook-dev#1703) Disable web when running dev (onlook-dev#1718) feat: font panel config (onlook-dev#1666) Add web app (onlook-dev#1702)
ml-delaurier
pushed a commit
to ml-delaurier/nolook
that referenced
this pull request
Apr 23, 2025
* feat: read add remove font from config * write to tailwind * write variable to root layout * add default font * remove default class when remove font variable * Show both brand sections * split into smaller functions * feat: upload custom fonts * delete font files if remove font * remove local font import * refactor: add watcher to file * using writeFile instead * use babel instead of regex * extract font weight from title * merge with existed font * add default font value
t1c1
pushed a commit
to t1c1/onlookbotcodes
that referenced
this pull request
Jun 5, 2025
* feat: read add remove font from config * write to tailwind * write variable to root layout * add default font * remove default class when remove font variable * Show both brand sections * split into smaller functions * feat: upload custom fonts * delete font files if remove font * remove local font import * refactor: add watcher to file * using writeFile instead * use babel instead of regex * extract font weight from title * merge with existed font * add default font value
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Add comprehensive font management system with UI components, backend logic, and tests for handling fonts in the application.
fonts.ts.addFont(),removeFont(), andsetDefaultFont()infont.ts.FontFileWatcherinwatcher.ts.FontFamilycomponent updated to support adding, removing, and setting default fonts.UploadModalcomponent for uploading custom fonts.FontPanelandSystemFontcomponents for managing and displaying fonts.extractFontParts()andgetFontFileName()infont.tsfor parsing font file names.babel.test.tsandfonts.test.tsfor font utilities and class name manipulation.MainChannelsinipc.tsfor font-related operations.updateTailwindFontConfig()andremoveFontFromTailwindConfig()intailwind.ts.This description was created by
for 7431438. It will automatically update as commits are pushed.