refactor: allow undo redo action with font #1751
Merged
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.
👍 Looks good to me! Reviewed everything up to 108784d in 1 minute and 55 seconds
More details
- Looked at
243lines of code in4files - Skipped
0files when reviewing. - Skipped posting
12drafted comments based on config settings.
1. apps/studio/electron/main/assets/fonts/font.ts:133
- Draft comment:
Returning a CodeDiff without writing the formatted content to file. Ensure that the consumer applies the diff properly. - 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 the consumer applies the diff properly, which violates the rule against asking the author to ensure behavior is intended or tested. It doesn't provide a specific code suggestion or point out a specific issue with the code.
2. apps/studio/electron/main/assets/fonts/font.ts:292
- Draft comment:
The block that deletes font files is commented out to support undo/redo. Verify that this rollback behavior is fully tested. - 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 that the rollback behavior is fully tested, which violates the rule against asking for confirmation or verification of testing. It does not provide a specific suggestion or point out a specific issue with the code.
3. apps/studio/electron/main/assets/fonts/watcher.ts:112
- Draft comment:
Font change events are emitted only when added or removed fonts exist. Confirm that missing updates (if only modified fonts) are handled elsewhere. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
4. apps/studio/src/lib/editor/engine/font/index.ts:147
- Draft comment:
After adding a font, the code pushes a WriteCodeAction; ensure that the file system update occurs and UI is refreshed accordingly without an explicit scan. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that a specific behavior occurs, which violates the rule against asking the author to ensure behavior. It doesn't provide a specific suggestion or point out a specific issue with the code.
5. apps/studio/src/lib/editor/engine/font/index.ts:213
- Draft comment:
Using setTimeout to delay getDefaultFont might be fragile. Consider awaiting getDefaultFont or using a promise chain to ensure proper timing. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
The setTimeout might be intentional to allow some async operation to complete before getting the default font. Without more context about why the delay was added, we can't be certain that using await would be correct. The author may have had a specific reason for the delay. The current code works, even if not ideal.
I might be too hesitant - timing issues can cause real bugs and setTimeout is often a code smell. The 300ms delay seems arbitrary.
While setTimeout isn't ideal, we don't have enough context to know if await would actually fix the underlying timing issue. The author likely added this delay for a specific reason.
Without understanding why the delay was added, suggesting to remove it could cause problems. The current code, while not perfect, is likely intentional.
6. packages/models/src/constants/ipc.ts:172
- Draft comment:
New channel 'FONTS_CHANGED' added. Verify that all front-end listeners and back-end emitters are in sync with this new channel name. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
7. apps/studio/electron/main/assets/fonts/font.ts:133
- Draft comment:
Returning a CodeDiff instead of writing the file directly. Ensure that downstream processing handles file writing correctly for undo/redo. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that downstream processing handles file writing correctly, which is against the rules. It doesn't provide a specific suggestion or ask for a specific test to be written. It seems to be more of a reminder or a caution, which is not allowed.
8. apps/studio/electron/main/assets/fonts/font.ts:292
- Draft comment:
File deletion logic has been commented out. Confirm that preserving font files for undo/redo is the intended behavior and file cleanup is managed elsewhere. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to confirm their intention regarding the commented-out file deletion logic. This violates the rule against asking the PR author to confirm their intention or ensure behavior is intended. The comment should be removed.
9. apps/studio/electron/main/assets/fonts/watcher.ts:112
- Draft comment:
Broadcasting the FONTS_CHANGED event is a good idea for state updates. Ensure the renderer’s listener is properly updating its state on these events. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
10. apps/studio/src/lib/editor/engine/font/index.ts:213
- Draft comment:
Using a timeout to update the default font may be unreliable; consider a more robust mechanism to trigger getDefaultFont when the update completes. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
11. packages/models/src/constants/ipc.ts:172
- Draft comment:
New FONTS_CHANGED channel added. Verify that the naming and usage are consistent across all modules. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
12. packages/models/src/constants/ipc.ts:54
- Draft comment:
The enum value for CLEAN_CODE_KEYS is set to 'clean-move-keys', which appears to be a typographical error. To maintain consistency with its name, consider changing it to 'clean-code-keys'. - 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_Vj4TgEPf1gNfIRTy
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Title: Font not applied to certain elements (e.g., Button) Summary: When changing the font of some elements (like Button), the font does not get applied, while other elements (like Text) are updated correctly. Steps to Reproduce:
Actual Result:
Expected Result: |
|
Title: "Set Default" action not undoable Summary: When setting a font (or style) as default and performing Undo, the "Set Default" action is not reverted. Steps to Reproduce:
Actual Result: Expected Result: |
Actually I think for now we should skip this since we have a technical problem with this. |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on b0bde8e in 1 minute and 58 seconds
More details
- Looked at
244lines of code in3files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. apps/studio/electron/main/assets/fonts/layout.ts:293
- Draft comment:
Refactored updateFontInLayout now returns a CodeDiff (or null) instead of writing to file. Ensure callers properly apply the diff. - 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/layout.ts:409
- Draft comment:
setDefaultFont now returns the diff from updateFontInLayout. Confirm that the caller uses this diff to update the file as needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm that the caller uses the diff returned by setDefaultFont to update the file. This falls under asking the author to confirm their intention, which is against the rules.
3. apps/studio/src/lib/editor/engine/font/index.ts:240
- Draft comment:
The _lastDefaultFont check prevents redundant state updates. Verify that it properly handles initial null/empty states. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to verify the behavior of a specific check in the code. It doesn't provide a specific suggestion or point out a clear issue, which violates the rule against asking the author to confirm or verify intentions.
4. apps/studio/electron/main/assets/fonts/layout.ts:329
- Draft comment:
Consider aggregating CodeDiff generation after traverseClassName completes to avoid potential overwrites when multiple className attributes are updated. - 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 current implementation is actually correct. The updatedAst flag ensures we only generate a diff when changes were made. The original content is safely captured once at the start. Multiple className updates in the same traversal would correctly accumulate in the AST before the final code generation. There's no risk of overwrites since we're working with the same AST instance.
I could be missing a subtle race condition or memory efficiency concern with generating code during traversal. There might be performance benefits to generating code only once at the end.
The performance impact would be negligible since code generation only happens once when updatedAst is true. The current approach is simpler and just as correct.
The comment suggests a change that would not provide any meaningful benefit and the current implementation is already correct and efficient.
5. apps/studio/electron/main/assets/fonts/layout.ts:410
- Draft comment:
Propagate the CodeDiff return value for undo/redo support; ensure the caller handles a possible null return appropriately. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. apps/studio/electron/main/assets/fonts/watcher.ts:61
- Draft comment:
Good use of the selfModified flag to prevent processing of self-triggered file changes. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. apps/studio/electron/main/assets/fonts/watcher.ts:115
- Draft comment:
Consider caching routerConfig outside the loop to reduce redundant async calls during font sync. - 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/src/lib/editor/engine/font/index.ts:222
- Draft comment:
Avoid hardcoding a 300ms delay; consider using a configurable timeout or an event-driven update mechanism for getDefaultFont. - 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:240
- Draft comment:
Good use of _lastDefaultFont to update the default font only when it actually changes. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
Workflow ID: wflow_4ym7riLHcAcF4OVi
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.
| @@ -55,6 +74,18 @@ export class FontFileWatcher { | |||
| console.error('Error syncing fonts with configs:', error); | |||
| } | |||
| } | |||
| if ( | |||
| eventPath === layoutPath || | |||
There was a problem hiding this comment.
Review event type condition: layoutPath check lacks an explicit event type (update/create). Ensure this is intentional or add a similar guard as for appPath.
Suggested change
| eventPath === layoutPath || | |
| (eventPath === layoutPath && (event.type === 'update' || event.type === 'create')) || |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 7f520a8 in 1 minute and 57 seconds
More details
- Looked at
71lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. apps/studio/electron/main/assets/fonts/watcher.ts:77
- Draft comment:
Changed condition: now both layoutPath and appPath events require update/create. Previously, layoutPath events were handled regardless of type. Verify this change is intentional. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/FontFamily.tsx:70
- Draft comment:
Removed inline fontFamily style from tooltip span; relying on inheritance from parent. Confirm that the intended styling is maintained. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. apps/studio/electron/main/assets/fonts/watcher.ts:77
- Draft comment:
Refactored condition now applies the (update|create) event type check for both layout and app paths. Confirm that layout events not matching these types (which previously were handled unconditionally) is the desired behavior. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/FontFamily.tsx:55
- Draft comment:
The container now applies an inline fontFamily style and the fontFamily style on the TooltipTrigger span is removed. Verify that inheriting the fontFamily from the parent works consistently in all usage contexts. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_38t82iACsN7C21AJ
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.
apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/FontFamily.tsx
Show resolved
Hide resolved
Kitenite
reviewed
Apr 12, 2025
Kitenite
reviewed
Apr 12, 2025
Kitenite
reviewed
Apr 12, 2025
Kitenite
reviewed
Apr 12, 2025
packages/models/src/constants/ipc.ts
Outdated
| @@ -168,6 +168,9 @@ export enum MainChannels { | |||
|
|
|||
| // Trainloop | |||
| SAVE_APPLY_RESULT = 'save-apply-result', | |||
|
|
|||
| // New channel | |||
There was a problem hiding this comment.
Need to clean up comments. Also fonts group exists already.
Kitenite
reviewed
Apr 12, 2025
|
|
||
| export class FontFileWatcher { | ||
| private subscription: AsyncSubscription | null = null; | ||
| private previousFonts: Font[] = []; | ||
| private selfModified: Set<string> = new Set(); |
There was a problem hiding this comment.
I think eventually we want a generic file watcher that any service can subscribe to instead of having the same pattern duplicated in multiple places. Out of scope for now.
|
Generated with ❤️ by ellipsis.dev |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 83be89c in 1 minute and 0 seconds
More details
- Looked at
3035lines of code in30files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. apps/web/template/.next/app-build-manifest.json:1
- Draft comment:
Build manifest files (and similarly generated files in .next) should not be committed. Ensure these auto‐generated artifacts are added to .gitignore. - 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/web/template/.next/types/app/layout.ts:1
- Draft comment:
Type-check scaffolding for page/layout appears fully removed; confirm these removals are auto-generated and won't impact type safety or expected Next.js behavior. - 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/web/template/.next/app-build-manifest.json:1
- Draft comment:
Build artifacts (e.g. app-build-manifest.json) are auto-generated and should not be committed. Consider adding .next/ to .gitignore. - 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/web/template/.next/types/app/layout.ts:1
- Draft comment:
Type definitions under .next/ are generated and should not be tracked. Ensure these files are excluded from version control. - 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. General:1
- Draft comment:
The PR title and issue description mention features like 'undo redo action with font' and 'better onboarding docs', but the diff only removes generated build files. Please confirm if the intended feature/code/documentation changes are included. - 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_2dvgSCowGkQjZl5C
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 41921e9 in 34 seconds
More details
- Looked at
9lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. apps/web/template/next-env.d.ts:1
- Draft comment:
Unexpected deletion: next-env.d.ts is auto-generated and should not be removed. - 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/web/template/next-env.d.ts:1
- Draft comment:
next-env.d.ts is auto‐generated by Next.js (see the comment in the file). Removing it can break TypeScript type resolution. Confirm if this deletion is intentional. - 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/web/template/next-env.d.ts:5
- Draft comment:
The PR title and description mention undo/redo functionality and documentation improvements, but this diff only removes an unrelated file. Please ensure the intended changes are included. - 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_3Qqt7F1YWzHndmED
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Kitenite
approved these changes
Apr 12, 2025
There was a problem hiding this comment.
❌ Changes requested. Incremental review on de9d51e in 1 minute and 21 seconds
More details
- Looked at
36lines of code in1files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. apps/studio/electron/main/assets/fonts/layout.ts:37
- Draft comment:
Good addition of error logging. However, the PR title mentions undo/redo functionality but the diff only adds logging; please ensure related undo/redo changes are handled elsewhere. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. apps/studio/electron/main/assets/fonts/layout.ts:129
- Draft comment:
Added error logging for readFile failure in addFontVariableToElement. Consider if additional error handling (like throwing) is needed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. apps/studio/electron/main/assets/fonts/layout.ts:241
- Draft comment:
Added error logging for readFile failure in removeFontVariableFromLayout. Ensure consistency with other error logs. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. apps/studio/electron/main/assets/fonts/layout.ts:307
- Draft comment:
Added error logging for readFile failure in updateFontInLayout. Consider if further error propagation is required. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. apps/studio/electron/main/assets/fonts/layout.ts:37
- Draft comment:
Added error logging for when file content is missing. However, the PR title mentions undo/redo functionality and onboarding docs, which are not present in this diff. Please update the PR description or include the intended features. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to update the PR description, which violates the rule against asking for PR description updates. The comment also mentions the PR title, which is not relevant to the code changes themselves. Therefore, this comment should be removed.
6. apps/studio/electron/main/assets/fonts/layout.ts:241
- Draft comment:
Similar error logging for a failed file read is added here. Extracting a common error-handling utility could improve maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. apps/studio/electron/main/assets/fonts/layout.ts:307
- Draft comment:
Consistent error logging is applied here as well. Consider centralizing file read error handling to reduce code duplication. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_6d6nxp095SLBNYK5
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.
| @@ -33,6 +34,7 @@ export async function traverseClassName( | |||
|
|
|||
| const content = await readFile(filePath); | |||
| if (!content) { | |||
| console.error(`Failed to read file: ${filePath}`); | |||
There was a problem hiding this comment.
Error logging for a missing file read is duplicated here. Consider refactoring the file reading logic into a helper to avoid repetition.
ml-delaurier
pushed a commit
to ml-delaurier/nolook
that referenced
this pull request
Apr 23, 2025
* refactor: allow undo redo action with font * add listener * allow undo redo default font * add font weight * add log error when can not read file content
t1c1
pushed a commit
to t1c1/onlookbotcodes
that referenced
this pull request
Jun 5, 2025
* refactor: allow undo redo action with font * add listener * allow undo redo default font * add font weight * add log error when can not read file content
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
Enhances font management with undo/redo functionality, updates UI components, and modifies inter-process communication for font changes.
font.tsandlayout.tsby returningCodeDiffobjects instead of writing directly to files.FontFileWatcherinwatcher.tsto handle font changes and notify the main window.FontManagerinindex.tsto pushWriteCodeActionto history for undo/redo.FontFamilyinFontFamily.tsxto include a toggle for setting default fonts and a button for adding fonts.FONTS_CHANGEDtoMainChannelsinipc.tsfor inter-process communication.This description was created by
for de9d51e. It will automatically update as commits are pushed.