-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: Added file watcher & auto update modified opened files in Dev Tab #1802
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…d files in dev tab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to 59d81bb in 3 minutes and 49 seconds. Click for details.
- Reviewed
761
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
12
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/studio/electron/main/code/fileWatcher.ts:78
- Draft comment:
Typo in comment: change 'This prevent multiple notifications' to 'This prevents multiple notifications'. - 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 grammar, fixing comment typos is a very minor issue that doesn't affect code functionality or readability. The meaning is perfectly clear even with the typo. According to the rules, we should not make comments that are obvious or unimportant. The typo could be seen as reducing code quality and professionalism. Some teams have strict standards about documentation quality. While documentation quality matters, this is an extremely minor grammatical issue that doesn't impact understanding. The rules specifically say not to make unimportant comments. This comment should be deleted as it addresses an extremely minor documentation issue that doesn't meaningfully impact code quality or understanding.
2. apps/studio/electron/main/code/fileWatcher.ts:35
- Draft comment:
Consider using path.resolve() instead of normalize() if an absolute path is needed for robust comparisons. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
3. apps/studio/electron/main/events/code.ts:200
- Draft comment:
New file watcher IPC handlers look correct; consider adding type validations if needed. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. apps/studio/src/routes/editor/EditPanel/DevTab/index.tsx:80
- Draft comment:
Consider wrapping syncEditorViewWithState in useCallback to avoid re-creation on each render. - 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% While technically correct that the function is recreated on each render, several factors make this optimization less important: 1) The function is not passed as a prop to child components, 2) It's only called in response to user/system events, not on every render, 3) The function's performance is dominated by the DOM operations it performs, not its creation cost, 4) Using useCallback would require listing openedFiles in dependencies, which would still cause recreation when files change. The comment correctly identifies a potential optimization point. In a highly performance-sensitive application, minimizing function recreations could be beneficial. However, in this case the optimization would add complexity without meaningful performance benefits since the function is not passed as a prop and its performance is dominated by DOM operations. The comment should be removed as the suggested optimization would add unnecessary complexity without meaningful benefits in this specific case.
5. apps/studio/src/routes/editor/EditPanel/DevTab/index.tsx:591
- Draft comment:
Dependencies of the useEffect for file change events include entire openedFiles array; ensure this is intentional to avoid excess re-subscriptions. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
6. packages/models/src/constants/ipc.ts:178
- Draft comment:
New file watcher IPC channel constants look correctly added. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
7. apps/studio/electron/main/code/fileWatcher.ts:15
- Draft comment:
Consider using asynchronous fs methods instead of fs.existsSync to avoid blocking the main process. - 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% While async fs methods are generally better for performance, in this case: 1) The operations are very lightweight (just checking existence) 2) This is in Electron's main process where blocking isn't as critical 3) The checks are used as guards before more expensive async operations 4) The sync checks make the code simpler and safer 5) The performance impact would be negligible The comment raises a valid point about blocking operations. In high-scale scenarios, many simultaneous file checks could cause noticeable delays. Given this is an Electron app dealing with individual file watches, the scale and frequency of these existence checks will be low enough that the sync version is an acceptable tradeoff for simpler, safer code. The comment should be removed. The use of existsSync is appropriate here given the context and tradeoffs.
8. apps/studio/electron/main/code/fileWatcher.ts:56
- Draft comment:
Consider handling other event types (e.g., 'delete' or 'rename') to clean up watchers if the file is removed or renamed. - 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 mechanisms to handle deleted files: notifyFileChanged checks fs.existsSync and returns if the file is gone. The unwatchFile method exists to clean up watchers. The code watches the directory, not individual files, so renames would still be caught as create/update events. The current implementation seems intentionally designed this way. I could be wrong about how renames are handled by the watcher library. There might be edge cases where explicit rename handling would be beneficial. The existing code already handles the key scenarios robustly through directory watching and existence checks. Adding explicit rename/delete handling would add complexity without clear benefits. The comment should be removed as the code already handles file deletion and renaming scenarios through other mechanisms.
9. apps/studio/src/routes/editor/EditPanel/DevTab/index.tsx:359
- Draft comment:
Consider awaiting or handling errors from window.api.invoke for WATCH_FILE to surface any potential failures in setting up the file watcher. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% Looking at the context, this is part of a loadFile function that already has error handling via try/catch. The WATCH_FILE call is inside that try block, so any errors would be caught by the outer error handler. The function already logs errors and shows a toast notification to users. Adding another error handler specifically for WATCH_FILE seems redundant. I could be wrong about how the error propagation works - maybe WATCH_FILE errors need special handling that the general catch block doesn't cover. The comment might be identifying a real issue. Looking at the full context again, the existing error handling is comprehensive - it catches all errors in the function, logs them, and shows user-friendly messages. Adding another catch specifically for WATCH_FILE would be redundant and add unnecessary complexity. The comment should be deleted because the code already has appropriate error handling via the outer try/catch block that covers this API call.
10. apps/studio/src/routes/editor/EditPanel/DevTab/index.tsx:589
- Draft comment:
Consider memoizing the file change handler or using refs for openedFiles to prevent unnecessary re-subscriptions in the useEffect dependency array. - 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% While the suggestion has some merit since re-subscribing to events can be inefficient, the actual impact here is likely minimal since event subscriptions are lightweight operations, the cleanup function properly removes listeners, the handler needs access to current state, and using refs would make the code more complex. The comment identifies a real performance consideration. Re-subscribing on every openedFiles change could be unnecessary. However, the complexity cost of memoizing or using refs likely outweighs the minimal performance benefit. The current implementation is cleaner and safer. The comment should be deleted as it suggests a premature optimization that would make the code more complex for minimal benefit.
11. apps/studio/src/routes/editor/EditPanel/DevTab/index.tsx:80
- Draft comment:
Wrap syncEditorViewWithState in useCallback to ensure stable function identity and avoid unnecessary re-renders. - 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% While the function does reference openedFiles from closure, it's only used for a find() operation. editorViewsRef is a useRef so it's stable. The function is used in effects and handlers that already have their own dependencies. useCallback would require listing openedFiles as a dependency anyway, so it wouldn't prevent re-creation when openedFiles changes. The function does capture openedFiles from closure which could theoretically cause stale closure issues. And technically useCallback could memoize the function reference between openedFiles changes. The stale closure risk is minimal since openedFiles is only used to look up the current file state. And since openedFiles would need to be a dependency, useCallback wouldn't prevent most re-renders anyway. The suggestion to use useCallback here would add complexity without providing meaningful benefits. The current implementation is fine.
12. apps/studio/electron/main/code/fileWatcher.ts:78
- Draft comment:
Minor typographical error: The comment on line 78 currently reads 'This prevent multiple notifications for a single save event'. Consider changing it to 'This prevents multiple notifications for a single save event'. - 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 technically correct, this is an extremely minor grammatical fix that doesn't impact code functionality or readability in any meaningful way. The meaning is perfectly clear either way. Comments about pure grammar/spelling in comments, unless severely impacting readability, fall into the category of "not important enough" to warrant a PR comment. The comment is technically correct, and some might argue that maintaining perfect grammar everywhere is important for professionalism. The benefit of fixing this minor grammatical issue is far outweighed by the cognitive overhead of processing another PR comment. It doesn't meaningfully improve code quality or understanding. Delete this comment as it addresses an extremely minor grammatical issue that doesn't meaningfully impact code quality or understanding.
Workflow ID: wflow_aZmnGiPbMBXdwTMM
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full blown IDE in the browser. This is awesome!
* 'main' of https://github.com/onlook-dev/onlook: use DomElementStyles (onlook-dev#1810) feat: Added file watcher & auto update modified opened files in Dev Tab (onlook-dev#1802) fix: border thickness hidden when setting border color from color picker (onlook-dev#1805) move overflow input (onlook-dev#1800) Added overflow option in the style panel (onlook-dev#1793) refactor: update images manager (onlook-dev#1784) chore: error nesting button (onlook-dev#1790) refactor: update naming for brand color (onlook-dev#1785) Tys toolbar v4 (onlook-dev#1786)
…ab (onlook-dev#1802) * fix dev tab scroll bug & added file watcher functionalities for opened files in dev tab
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Introduces file watcher for auto-updating modified files in Dev Tab, adds conflict notifications, and fixes scroll issue.
FileWatcher
class infileWatcher.ts
to watch file changes and notify UI.code.ts
forWATCH_FILE
,UNWATCH_FILE
, andMARK_FILE_MODIFIED
.FileConflictAlert
component inFileConflictAlert.tsx
for conflict notifications.DevTab/index.tsx
to handle file changes, conflicts, and sync editor state.EditPanel/index.tsx
for Dev Tab.ipc.ts
to include new file-related channels.This description was created by
for 59d81bb. You can customize this summary. It will automatically update as commits are pushed.