Web version pt.7 #1775
Merged
Web version pt.7 #1775
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 499eafc in 1 minute and 24 seconds
More details
- Looked at
56lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsx:13
- Draft comment:
Refactor note: The component now accepts a direct 'webFrame' prop instead of a ref. Ensure that the parent always provides a non-null webFrame (the removed null-check is now handled by conditional rendering). - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx:18
- Draft comment:
Consider revising the useEffect dependency: using 'webFrameRef.current' in the dependency array may be unreliable. Using an empty array or a callback ref could ensure the webFrame is captured once it’s attached. - Reason this comment was not posted:
Marked as duplicate.
3. apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx:31
- Draft comment:
The PR description mentions adding better onboarding docs (issue 123), but these changes only affect web frame gesture handling. Confirm that documentation updates are included elsewhere or in a future PR. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_28W94gPe5vUawIyu
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.
| @@ -13,6 +13,12 @@ export const FrameView = observer( | |||
| frame: Frame; | |||
| }) => { | |||
| const webFrameRef = useRef<WebFrameView>(null); | |||
| const [webFrame, setWebFrame] = useState<WebFrameView | null>(null); | |||
|
|
|||
| useEffect(() => { | |||
There was a problem hiding this comment.
Dependency on webFrameRef.current in useEffect may not update reliably. Consider using a callback ref or an empty dependency array if the ref won't change.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 887979a in 1 minute and 18 seconds
More details
- Looked at
80lines of code in3files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. apps/web/client/src/components/store/editor/engine/element/index.ts:30
- Draft comment:
Good use of optional chaining for hoveredElement. Ensure all cases where hoveredElement might be undefined or missing domId are handled. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/web/client/src/components/store/editor/engine/element/index.ts:55
- Draft comment:
Removal of debug logs from click method is appropriate for production code. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. apps/web/client/src/components/store/editor/engine/overlay/index.ts:29
- Draft comment:
Replacing updateHoverRect(null) with removeHoverRect improves API clarity for clearing hover state. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. apps/web/client/src/components/store/editor/engine/overlay/index.ts:50
- Draft comment:
Uncommenting the loop to add click rects ensures proper UI updates. Confirm that updating the text editor when editing is expected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. apps/web/client/src/components/store/editor/engine/overlay/state.ts:41
- Draft comment:
The updateHoverRect method now requires a non-null rect. Verify that all consumers of updateHoverRect have been updated to use removeHoverRect to clear hover state. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. apps/web/client/src/components/store/editor/engine/element/index.ts:30
- Draft comment:
Good use of optional chaining in the hovered-element check. Note that the removal of the debug log may impact troubleshooting—ensure logging is handled elsewhere if needed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. apps/web/client/src/components/store/editor/engine/overlay/index.ts:29
- Draft comment:
Using removeHoverRect instead of updateHoverRect(null) improves semantic clarity. Make sure all callers correctly use removeHoverRect when clearing the hover state. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
8. apps/web/client/src/components/store/editor/engine/overlay/index.ts:50
- Draft comment:
When text editing is active, updateTextEditor is called for each click rect in a loop. Verify that multiple calls (potentially overwriting previous text editor state) are intentional. - 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 comment asks to "verify" the behavior is intentional, which violates our rules about not asking for verification. It's speculative - we don't know if multiple calls are actually problematic. Without seeing the implementation of updateTextEditor, we can't know if multiple calls are an issue. The author likely uncommented this code intentionally.
Maybe multiple updateTextEditor calls could cause performance issues or visual glitches if they conflict with each other?
Without seeing updateTextEditor's implementation or evidence of actual problems, this is purely speculative. The code was explicitly uncommented, suggesting this behavior is intended.
Delete this comment as it violates rules against asking for verification and making speculative comments without clear evidence of problems.
9. apps/web/client/src/components/store/editor/engine/overlay/state.ts:41
- Draft comment:
Separating updateHoverRect (for setting) from removeHoverRect (for clearing) is a clear design choice. Ensure that all clients update their calls accordingly, as updateHoverRect now no longer accepts null. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
Workflow ID: wflow_daZR9KY337JEtYv5
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 4877c7e in 1 minute and 50 seconds
More details
- Looked at
1502lines of code in43files - Skipped
0files when reviewing. - Skipped posting
25drafted comments based on config settings.
1. apps/web/preload/script/api/dom.ts:8
- Draft comment:
Good replacement from getWebviewId to getFrameId. Consider adding documentation for the new frameId concept if not already documented. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/web/preload/script/api/elements/helpers.ts:36
- Draft comment:
Using getFrameId() in parentDomElement is correct. No issues found. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. packages/models/src/actions/target.ts:8
- Draft comment:
Renaming webviewId to frameId is consistent; ensure that all consumers update accordingly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. packages/models/src/element/layers.ts:9
- Draft comment:
FrameId added to the layer schema is correct. No issues found. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx:55
- Draft comment:
Expose Penpal remote methods and call remote.setFrameId(frame.id) correctly. Consider stronger typing for the remote connection if possible. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
6. apps/web/client/src/components/store/editor/engine/style/index.ts:127
- Draft comment:
Using Array.prototype.toSorted() in onSelectedElementsChanged may require a polyfill for older browsers. Confirm browser support or provide fallback. - 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/web/client/src/components/store/editor/engine/chat/context.ts:125
- Draft comment:
Logic in getScreenshotContext is clear. Verify that calling takeWebviewScreenshot with selected element's frameId consistently returns the expected image. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
8. apps/web/client/src/components/store/editor/engine/copy/index.ts:29
- Draft comment:
Reading frameId from selectedEl.frameId is correct. Ensure consistency with other modules. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
9. apps/web/client/src/components/store/editor/engine/group/index.ts:50
- Draft comment:
Replacing webviewId with frameId in getGroupParentId and group actions looks consistent. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
10. apps/web/client/src/components/store/editor/engine/image/index.ts:142
- Draft comment:
Using element.frameId within getTargets() is correct. No issues noticed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
11. apps/web/client/src/components/store/editor/engine/insert/index.ts:198
- Draft comment:
InsertManager correctly constructs the insert action with frameId from webview.id. Good use of createDomId and createOid. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
12. apps/web/client/src/components/store/editor/engine/move/index.ts:56
- Draft comment:
MoveManager uses dragTarget.frameId to get the webview – the pattern is consistent. Consider handling the case when the offset parent is not found more gracefully. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
13. apps/web/client/src/components/store/editor/engine/overlay/index.ts:16
- Draft comment:
Using optional chaining for canvas position and scale is good. Ensure that state removal methods (removeHoverRect, removeClickRects) are consistently implemented in OverlayState. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
14. apps/web/client/src/components/store/editor/engine/style/index.ts:110
- Draft comment:
The merging logic for style actions is clear. Good use of Object.fromEntries. No issues found. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
15. apps/web/client/src/components/store/editor/engine/text/index.ts:21
- Draft comment:
TextEditingManager’s logic for starting, editing, and ending text editing looks solid. Consider handling the case when window.api returns undefined more robustly. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
16. apps/web/preload/script/api/elements/dom/remove.ts:30
- Draft comment:
The remove action construction now uses frameId. Looks correct. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
17. apps/web/preload/script/api/elements/helpers.ts:33
- Draft comment:
getDomElement function correctly retrieves the frameId using getFrameId. Code is clear. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
18. apps/web/preload/script/api/index.ts:26
- Draft comment:
API methods expose setFrameId and element helpers correctly. No issues found. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
19. apps/web/preload/script/api/state.ts:1
- Draft comment:
setFrameId and getFrameId are straightforward. Consider logging a specific debug message if frameId is missing. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
20. packages/models/src/actions/target.ts:8
- Draft comment:
Target interface now includes frameId instead of webviewId. This is consistent. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
21. packages/models/src/element/layers.ts:9
- Draft comment:
LayerNode schema correctly adds the frameId field. Looks good. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
22. apps/studio/electron/preload/webview/elements/helpers.ts:29
- Draft comment:
Minor: The variable 'nested_shadow' in the function 'getDeepElement' uses snake_case while the rest of the code uses camelCase. Consider renaming it to 'nestedShadow' 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.
23. apps/web/preload/script/api/dom.ts:11
- Draft comment:
Typographical Error: The console warning on line 11 still refers to 'Webview id' even though the code now usesgetFrameId()resulting in aframeId. It would be clearer and more consistent to update the message to 'Frame id not found, skipping dom processing'. - Reason this comment was not posted:
Comment was on unchanged code.
24. apps/web/preload/script/api/dom.ts:36
- Draft comment:
Typographical Error: The warning on line 36 still mentions 'Webview id not found' even though we're now usinggetFrameId()to get the frame identifier. Consider changing the message to 'Frame id not found, skipping dom processing' for consistency. - Reason this comment was not posted:
Comment was on unchanged code.
25. apps/web/preload/script/api/elements/helpers.ts:29
- Draft comment:
Minor naming inconsistency: the variable 'nested_shadow' should be renamed to 'nestedShadow' to match the camelCase naming convention used throughout the file. - 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_tRiPSnhmuyiSDYIq
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on ae4d22c in 2 minutes and 33 seconds
More details
- Looked at
617lines of code in11files - Skipped
0files when reviewing. - Skipped posting
16drafted comments based on config settings.
1. apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsx:186
- Draft comment:
Good: using removeHoverRect() instead of updating with null to clear hover state. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx:60
- Draft comment:
Ensure that remote.setFrameId and processDom return promises; using await guarantees sequential initialization of penpal connection. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. apps/web/client/src/components/store/editor/engine/ast/index.ts:150
- Draft comment:
Warning: 'invokeMainChannel' is used but its import is commented out. Confirm if this 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.
4. apps/web/client/src/components/store/editor/engine/group/index.ts:60
- Draft comment:
Renaming from webviewId to frameId is consistent; ensure all references are properly updated. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. apps/web/client/src/components/store/editor/engine/insert/index.ts:218
- Draft comment:
Updating insertDroppedImage, insertImageElement, and updateElementBackgroundAction to use frame instead of webview is clear and consistent. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. apps/web/client/src/components/store/editor/engine/overlay/index.ts:18
- Draft comment:
The new refresh loop using requestAnimationFrame and the needsRefresh flag looks like a good improvement for performance. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. apps/web/preload/package.json:23
- Draft comment:
Dependency change: replacing 'uuid' with 'nanoid' is fine as long as usage is updated. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
8. apps/web/preload/script/api/elements/dom/remove.ts:8
- Draft comment:
Parameter renaming from webviewId to frameId is consistent; no issues detected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
9. apps/web/preload/script/helpers/ids.ts:2
- Draft comment:
Switching from uuid to nanoid is implemented correctly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
10. apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsx:186
- Draft comment:
Change from updateHoverRect(null) to removeHoverRect() looks intentional. Confirm that removeHoverRect() fully clears the hover state as expected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
11. apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx:60
- Draft comment:
Using await for both setFrameId and processDom ensures proper connection initialization. Verify that processDom() correctly processes the DOM on the remote side. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
12. apps/web/client/src/components/store/editor/engine/insert/index.ts:218
- Draft comment:
InsertManager now uses FrameData instead of Electron.WebviewTag. Ensure that the FrameData.view methods (getInsertLocation, getElementAtLoc, getActionElementByDomId) are correctly implemented and return expected results. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
13. apps/web/client/src/components/store/editor/engine/overlay/index.ts:41
- Draft comment:
Consider adding error handling (e.g., try/catch) inside the performRefresh async function to avoid unhandled errors during the requestAnimationFrame loop. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
14. apps/web/client/src/components/store/editor/engine/ast/index.ts:172
- Draft comment:
Consider updating the warning message in getElementFromDomId (line 172) to match the function name (e.g., change 'Failed to getNodeFromDomId: Document not found' to 'Failed to getElementFromDomId: Document not found') for consistency. - Reason this comment was not posted:
Comment was on unchanged code.
15. apps/web/preload/script/api/elements/index.ts:43
- Draft comment:
Typo in error message: the function is namedupdateElementInstance, but the error message logs 'Failed to updateElementInstanceId: Element not found'. Consider updating the error message 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.
16. apps/web/preload/script/api/elements/index.ts:36
- Draft comment:
Inconsistent naming style: the variable 'nested_shadow' uses snake_case whereas the rest of the code uses camelCase. Consider renaming it to 'nestedShadow' 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_EGPPmhBWl6tp5WR3
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.
| @@ -1,7 +1,7 @@ | |||
| import { invokeMainChannel } from '@/lib/utils'; | |||
| // import { invokeMainChannel } from '@/lib/utils'; | |||
There was a problem hiding this comment.
The function invokeMainChannel is still used (e.g., in getTemplateNodeById) but its import is commented out. This will lead to runtime errors; please restore or replace the import.
Suggested change
| // import { invokeMainChannel } from '@/lib/utils'; | |
| import { invokeMainChannel } from '@/lib/utils'; |
ml-delaurier
pushed a commit
to ml-delaurier/nolook
that referenced
this pull request
Apr 23, 2025
* Fix webframe * Working click element * Working hover rect * Set frame id * Working refresh on scroll
t1c1
pushed a commit
to t1c1/onlookbotcodes
that referenced
this pull request
Jun 5, 2025
* Fix webframe * Working click element * Working hover rect * Set frame id * Working refresh on scroll
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
Refactor codebase to replace
webviewIdwithframeId, update frame management, overlays, and element handling accordingly.webviewIdwithframeIdacross multiple files, includingdom.ts,remove.ts,helpers.ts,index.ts, andstate.ts.target.ts,element.ts, andlayers.tsto useframeId.FramesManagerinframes/index.tsto manage frame data and operations.GestureScreen,FrameView, andWebFrameComponentto useframeId.OverlayManagerandElementsManagerto handle frames usingframeId.overlay/utils.tsandoverlay/state.tsfor frame-based operations.ActionManagerandStyleManagerto useframeIdfor targeting elements.insert/index.tsandmove/index.tsfor frame-based element manipulation.history.test.tsto reflect changes in action targets.package.jsonand other utility files for consistency.This description was created by
for ae4d22c. It will automatically update as commits are pushed.