feat: font panel config #1666
Open
feat: font panel config #1666
+24,571
−193
Conversation
This file contains 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
| * 2. Updating Tailwind config with the new font family | ||
| * 3. Adding the font variable to the appropriate layout file | ||
| */ | ||
| export async function addFont(projectRoot: string, font: Font) { |
There was a problem hiding this comment.
This really should be using babel and AST instead of regex. Regex is very error-prone. See how we apply changes to ts files:
https://github.com/onlook-dev/onlook/blob/main/apps/studio/electron/main/code/diff/transform.ts#L13-L14
Will need to create a parsing module for ts file instead of tsx https://github.com/onlook-dev/onlook/blob/main/apps/studio/electron/main/code/helpers.ts#L3-L4
|
|
||
| newContent += fontConfig; | ||
|
|
||
| fs.writeFileSync(fontPath, newContent); |
There was a problem hiding this comment.
@spartan-vutrannguyen , writeFileSync is discouraged. Please change to the promisified writeFile
| * 1. Saving the font files to the fonts folder | ||
| * 2. Adding the font configuration to fonts.ts using next/font/local | ||
| */ | ||
| export async function addLocalFont( |
There was a problem hiding this comment.
Same here, should use Babel/AST which you are using for removeFont
|
|
||
| // 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); |
| async scanFonts() { | ||
| const projectRoot = this.projectsManager.project?.folderPath; | ||
| if (!projectRoot) { | ||
| return; |
There was a problem hiding this comment.
Should console.error here
| async addFont(font: Font) { | ||
| const projectRoot = this.projectsManager.project?.folderPath; | ||
| if (!projectRoot) { | ||
| return; |
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 |
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
Introduce a comprehensive font management system with backend logic, UI components, and tests for handling font operations in the application.
addFont,removeFont,addLocalFont,scanFonts,setDefaultFont,getDefaultFontinfont.tsfor managing fonts.FontFileWatcherinwatcher.tsto monitor font file changes and sync configurations.tailwind.tsto modify Tailwind config for font additions/removals.FontManagerclass infont/index.tsfor managing font state in the editor.FontPanel,FontFamily,SystemFont, andUploadModalcomponents to support font operations.extractFontPartsandgetFontFileNameinfont.ts.babel.test.tsandfonts.test.tsto verify font utilities and class name manipulations.code.tsto handle IPC channels for font operations.constantsandmodelsto include font-related configurations and types.This description was created by
for 09770d7. It will automatically update as commits are pushed.