Web version pt.6 #1767
Merged
Web version pt.6 #1767
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 cdbebce in 1 minute and 30 seconds
More details
- Looked at
17lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. apps/web/preload/package.json:7
- Draft comment:
Verify 'serve' now uses 'server/index.ts' while 'build' uses 'src/index.ts'. Ensure this inconsistency is intended. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. apps/web/preload/package.json:7
- Draft comment:
The 'serve' script now uses 'server/index.ts' instead of 'src/index.ts', but the build commands still reference 'src/index.ts'. Verify that this inconsistency is intentional and update documentation if needed. - 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 violates several rules: 1) It asks for verification ("Verify that...") 2) It's speculative - it doesn't definitively show there's a problem 3) It asks for documentation updates 4) Without seeing the actual files and structure, we can't know if this is actually an issue - the paths could be intentionally different.
Maybe this really is a bug - having different paths could indicate a real problem in the build setup.
Even if it might be a bug, we don't have enough context to be certain. The different paths could be intentional - serve might need a different entry point than the build.
Delete the comment as it violates multiple rules - it's speculative, asks for verification, and we lack context to confirm if it's actually an issue.
Workflow ID: wflow_UYqyqolRjeKuBSyh
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 8030ce2 in 2 minutes and 19 seconds
More details
- Looked at
2287lines of code in28files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. apps/web/preload/src/api/elements/index.ts:8
- Draft comment:
The getDeepElement function is reimplemented here, duplicating similar logic from other modules. Consider centralizing this logic to a shared helper for consistency and maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. apps/web/preload/src/api/elements/move/drag.ts:17
- Draft comment:
Incorrect arithmetic on style.width/height: concatenating a CSS string with 1 yields an invalid value. - Reason this comment was not posted:
Marked as duplicate.
3. apps/web/preload/src/api/dom.ts:7
- Draft comment:
The debounced processDom result is ignored; verify that calling processDebounced without handling its return value is intentional. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. apps/web/preload/src/index.ts:1
- Draft comment:
PR description mentions onboarding docs, but no documentation changes are visible in the diff. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to update the PR description or to ensure that documentation changes are included, which violates the rules. It does not provide a specific code suggestion or ask for a specific test to be written.
5. apps/web/preload/package.json:25
- Draft comment:
There's no newline at the end of the file. While this is a trivial style issue and generally doesn't affect functionality, it's good practice to include a newline at the end of the file to adhere to common formatting conventions. - Reason this comment was not posted:
Comment was on unchanged code.
6. apps/web/preload/src/api/elements/index.ts:43
- Draft comment:
Typographical error: The console warning message 'Failed to updateElementInstanceId: Element not found' on line 43 seems to have an extra 'Id' which can be confusing since the function is named 'updateElementInstance'. Consider updating the message to: 'Failed to updateElementInstance: Element not found'. - 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 issue about the wording of a debug message. It doesn't affect functionality, and debug messages are primarily for developers. The message is still clear and understandable despite the minor inconsistency. According to the rules, we should not make purely informative comments or comments about obvious/unimportant issues.
The inconsistency could potentially cause confusion when searching through logs or debugging. Maybe it's worth maintaining consistency in error messages.
The benefit of fixing this is minimal - both versions of the message clearly convey the same meaning. This falls under "obvious or unimportant" in our rules.
This comment should be deleted as it addresses a trivial inconsistency in a debug message that doesn't impact functionality or code quality in any meaningful way.
7. apps/web/preload/src/api/elements/move/index.ts:29
- Draft comment:
Typographical Error: 'htmlElments' seems to be a misspelling. Consider correcting it to 'htmlElements' for clarity. - 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 about the typo, this is a very minor issue. Variable naming is important for code readability, but this particular typo is obvious and doesn't impact understanding. The variable is only used locally within the function. We should avoid nitpicky comments that don't materially improve code quality.
The typo could theoretically impact code searchability and consistency with naming conventions in other files. Maybe there's a team standard about variable naming that I'm not aware of.
Even considering those points, this is still too minor of an issue to warrant a PR comment. The benefit of fixing it doesn't outweigh the overhead of the review comment.
Delete this comment as it's too minor and doesn't materially impact code quality or understanding.
8. apps/web/preload/src/api/state.ts:9
- Draft comment:
Consider capitalizing 'id' to 'ID' in the warning message (line 9) to be consistent with the use of 'webviewId' in the code. This minor typographical adjustment improves readability. - 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%
This is an extremely minor stylistic change to a debug message that doesn't affect functionality. While there's some merit to consistency, this feels too trivial to warrant a PR comment. Debug messages are primarily for developers and don't need to follow strict naming conventions. This falls under the "do not make comments that are obvious or unimportant" rule.
The comment does point out an actual inconsistency in naming convention. Maybe consistency in logging messages could be important for searchability?
While consistency is good, this is far too minor of an issue to warrant a PR comment. Console messages are for debugging and don't need this level of scrutiny.
Delete this comment as it's too trivial and doesn't meaningfully improve the code quality.
Workflow ID: wflow_0NdzL42LspU822xg
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.
|
|
||
| el.style.left = `${left}px`; | ||
| el.style.top = `${top}px`; | ||
| el.style.width = styles.width + 1; |
There was a problem hiding this comment.
Avoid concatenating a number to a CSS size string (e.g. '100px' + 1 becomes '100px1'). Parse the numeric value and add before appending the unit.
Suggested change
| el.style.width = styles.width + 1; | |
| el.style.width = `${parseInt(styles.width) + 1}px`; |
| type: 'SelectorList', | ||
| children: [ | ||
| { | ||
| type: 'Selector', |
There was a problem hiding this comment.
Using a TypeSelector for an attribute-based selector in addRule may not match as intended.
| } | ||
| }; | ||
|
|
||
| const nested_shadow = crawlShadows(el); |
There was a problem hiding this comment.
Typographical note: The variable name nested_shadow uses snake_case, which is inconsistent with the camelCase convention used elsewhere in the file (e.g., getDeepElement, getDomElement). Consider renaming it to nestedShadow for better consistency and readability.
apps/web/preload/src/api/ready.ts
Outdated
| try { | ||
| handleBodyReady(); | ||
| } catch (err) { | ||
| console.log('Error in documentBodyInit:', err); |
There was a problem hiding this comment.
In the catch block starting at line 46, the error message logs 'Error in documentBodyInit:' even though the function invoked is handleBodyReady(). This naming inconsistency could confuse developers. Consider changing it to 'Error in handleBodyReady:' or updating the naming to be consistent.
Suggested change
| console.log('Error in documentBodyInit:', err); | |
| console.log('Error in handleBodyReady:', err); |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 3c54c3c in 1 minute and 0 seconds
More details
- Looked at
33lines of code in2files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. apps/web/preload/package.json:3
- Draft comment:
Ensure folder structure matches new module entry 'script/index.ts'. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/web/preload/package.json:8
- Draft comment:
Verify build commands now use 'script/index.ts' consistently. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. apps/web/preload/script/api/elements/style.ts:2
- Draft comment:
Confirm relative import '../../helpers' is correct after file relocation. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. apps/web/preload/package.json:3
- Draft comment:
Ensure new entry point 'script/index.ts' exists and aligns with the updated project structure. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. apps/web/preload/package.json:8
- Draft comment:
Verify build scripts now correctly compile 'script/index.ts' instead of 'src/index.ts'. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. apps/web/preload/script/api/elements/style.ts:2
- Draft comment:
Changed import path to a relative path; confirm that the 'helpers' file is now located at 'apps/web/preload/script/helpers'. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. apps/web/preload/script/api/elements/style.ts:33
- Draft comment:
Consider renaming the local getComputedStyle function to avoid shadowing the global function. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
8. apps/web/preload/script/api/elements/style.ts:58
- Draft comment:
Typographical issue: The console.warn message on line 58 uses 'css rules'. It might be better to use 'CSS rules' (in all capitals) to maintain consistency and 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_ukoaFZe42V17CXRu
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 06b78b0 in 2 minutes and 1 seconds
More details
- Looked at
390lines of code in7files - Skipped
0files when reviewing. - Skipped posting
11drafted comments based on config settings.
1. apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsx:18
- Draft comment:
Potential null dereference: 'webframe.current' could be null. Add a null-check before calling getElementAtLoc. - 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%
- React refs are guaranteed to be available after mount and during event handlers
- The component is already using webframe.current without null checks in multiple places
- If webframe.current was null, it would likely break in getRelativeMousePosition first
- The code is undergoing active refactoring so basic patterns should be consistent
- Adding null checks everywhere would add noise without meaningful safety
The ref could theoretically be null if the component is unmounting or if there's a race condition. The existing code's lack of null checks doesn't prove they're unnecessary.
While technically true, React guarantees ref stability during normal component lifecycle. Adding defensive null checks everywhere would be overly cautious and inconsistent with React patterns.
The comment should be removed. The suggested null check, while technically defensive, would be inconsistent with React patterns and the rest of the codebase without providing meaningful safety.
2. apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx:72
- Draft comment:
Dependency on 'iframeRemote' in useCallback for handleIframeLoad may trigger reinitialization on every change. Consider removing it from the dependency array to prevent potential infinite loops. - 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/preload/script/index.ts:10
- Draft comment:
Using '*' for allowedOrigins in the preload script may pose security risks. Ensure that this is acceptable or restrict origins accordingly. - Reason this comment was not posted:
Marked as duplicate.
4. apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsx:18
- Draft comment:
Ensure webframe.current is non-null before use to avoid runtime errors. Consider adding a null-check before calling methods like getElementAtLoc. - 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 webframe.current could be null, this appears to be a React component where the ref is expected to be available during event handlers. The component seems to handle the ref consistently throughout. Adding optional chaining would make the code more defensive but could mask real issues if the ref is unexpectedly missing. The existing error would help catch issues earlier.
The comment does point out a potential runtime error source. Refs can be null before mounting or after unmounting.
In React components, refs are expected to be available during event handlers after mount. Adding null checks everywhere would add noise and potentially hide real issues.
This appears to be a standard React ref usage pattern where the ref is expected to be available. The comment suggests being more defensive but doesn't point out a clear issue requiring changes.
5. apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx:52
- Draft comment:
Using '*' for allowedOrigins can introduce security risks in production; consider specifying explicit origins. - Reason this comment was not posted:
Marked as duplicate.
6. apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx:72
- Draft comment:
Including 'iframeRemote' in the dependency array for handleIframeLoad may trigger unnecessary reinitializations. Review if this dependency is needed. - 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/app/project/[id]/_components/canvas/frame/web-frame.tsx:125
- Draft comment:
Verify that 'iframeRemote' is defined before calling getElementAtLoc to avoid unexpected undefined results. - Reason this comment was not posted:
Comment looked like it was already resolved.
8. apps/web/preload/script/index.ts:10
- Draft comment:
AllowedOrigins is set to '*' here, which can be a security risk. Consider restricting to specific origins in production. - Reason this comment was not posted:
Marked as duplicate.
9. apps/web/client/src/components/store/editor/engine/overlay/utils.ts:84
- Draft comment:
There is a minor typo in the documentation comment for getRelativeMousePositionToWebview (line 84). It currently reads "Get the relative mouse position a webview element inside the canvas container." Consider changing it to "Get the relative mouse position of a webview element inside the canvas container." - 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/web/preload/script/api/elements/index.ts:35
- Draft comment:
Consider renaming 'nested_shadow' to 'nestedShadow' for consistency with the camelCase naming convention used in the rest of 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.
11. apps/web/preload/script/api/elements/index.ts:42
- Draft comment:
The console warning message on line 42 says 'Failed to updateElementInstanceId: Element not found'. Since the function is named 'updateElementInstance', consider changing the message to 'Failed to updateElementInstance: Element not found' 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_TjFB38gx3DXjJTFG
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.
| const messenger = new WindowMessenger({ | ||
| remoteWindow: iframe.contentWindow, | ||
| // TODO: Use a proper origin | ||
| allowedOrigins: ['*'], |
There was a problem hiding this comment.
Using '*' for allowedOrigins is overly permissive. Ensure this is safe for production or consider restricting to specific origins.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 55cab0d in 1 minute and 2 seconds
More details
- Looked at
78lines of code in5files - Skipped
0files when reviewing. - Skipped posting
14drafted comments based on config settings.
1. apps/web/client/src/components/store/editor/engine/canvas/index.ts:24
- Draft comment:
Changed hardcoded frame URL to localhost:8084. Confirm this value is appropriate for your environment. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/web/client/src/components/store/editor/engine/canvas/index.ts:32
- Draft comment:
Ensure consistent frame URL update for webFrame1: check that using http://localhost:8084 is intentional. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. apps/web/docker-compose.yml:48
- Draft comment:
Port mapping updated to 8084:8084; verify this mapping aligns with your service expectations. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. apps/web/package.json:7
- Draft comment:
Updated 'dev' script filters include web-preload and web-template. Confirm these new packages are ready and documented. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. apps/web/package.json:10
- Draft comment:
Added 'dev:preload' script. Check that this consistent command is tested and properly documented. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
6. apps/web/preload/package.json:2
- Draft comment:
Renamed package from '@onlook/preload' to '@onlook/web-preload' for consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. apps/web/template/package.json:2
- Draft comment:
Package name changed to '@onlook/web-template'; ensure references in documentation and code are updated accordingly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
8. apps/web/template/package.json:6
- Draft comment:
Setting PORT=8084 in the dev command ensures alignment with the docker configuration. Verify Next.js listens correctly on this port. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
9. apps/web/client/src/components/store/editor/engine/canvas/index.ts:24
- Draft comment:
Consider parameterizing the frame URL rather than hardcoding 'http://localhost:8084'. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
10. apps/web/client/src/components/store/editor/engine/canvas/index.ts:32
- Draft comment:
Both frames now use the same URL. Confirm if this is intended or if unique endpoints are needed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
11. apps/web/docker-compose.yml:48
- Draft comment:
Ensure that the container port mapping change ('8084:8084') aligns with the internal service configuration (PORT=8084 in the template package). - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
12. apps/web/package.json:7
- Draft comment:
Verify that the added filters for '@onlook/web-preload' and '@onlook/web-template' in the 'dev' script are intended and consistent with the new naming conventions. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
13. apps/web/preload/package.json:2
- Draft comment:
The package name has been updated to '@onlook/web-preload'; ensure all references to this package are updated accordingly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
14. apps/web/template/package.json:6
- Draft comment:
The 'dev' script now uses 'PORT=8084 next dev'. Confirm that the template app is configured to listen on port 8084 internally. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_7melfRig3WZ7Cnxc
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 e9fa93b in 2 minutes and 23 seconds
More details
- Looked at
71lines of code in3files - 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:16
- Draft comment:
Consider memoizing getRelativeMousePosition with useCallback to avoid recreating the function on each render, as it’s used in other hooks. - 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:26
- Draft comment:
Rendering conditionally on webFrameRef.current may not update reliably since useRef changes don't trigger 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 = 10% vs. threshold = 50%
While useRef changes don't trigger re-renders, this pattern is actually common and correct in React. The WebFrameComponent will set the ref when mounted, and since the parent component is an observer, any mobx state changes will trigger re-renders anyway. The ref check here appears to be used correctly as a guard to ensure the webFrame exists before rendering GestureScreen.
The comment raises a valid React principle about refs not triggering re-renders. Maybe there's a specific edge case where this could cause issues?
The pattern used here is actually a common and safe way to ensure child components have mounted before rendering dependent components. The observer wrapper ensures proper updates.
The comment should be deleted as it raises concerns about a pattern that is actually correct and commonly used in React.
3. apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsx:16
- Draft comment:
Consider memoizing getRelativeMousePosition with useCallback to avoid creating a new function on every render, which affects the stability of its use in useCallback for handleMouseEvent and consequently in throttledMouseMove. - 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 a new function is created on each render, this function is very simple and the cost of creating it is likely negligible. The function only depends on webFrame which is a prop and won't change often. The performance impact would be minimal at best. Additionally, adding useCallback would make the code more complex for questionable benefit.
The comment is technically accurate about function recreation. Perhaps in a high-performance scenario with many renders, this could matter.
The potential performance benefit is likely too small to justify the added complexity. React's documentation suggests avoiding premature optimization with useCallback unless there's a measurable performance issue.
While technically correct, this suggestion falls into the category of premature optimization and would add unnecessary complexity for minimal benefit.
Workflow ID: wflow_xmg6wRFw2Xz1f9CZ
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.
| {frame.type === FrameType.WEB && <WebFrameComponent frame={frame as WebFrame} />} | ||
| <GestureScreen /> | ||
| {frame.type === FrameType.WEB && <WebFrameComponent frame={frame as WebFrame} ref={webFrameRef} />} | ||
| {webFrameRef.current && <GestureScreen frame={frame as WebFrame} webFrame={webFrameRef.current} />} |
There was a problem hiding this comment.
Using webFrameRef.current in a conditional render may prevent GestureScreen from mounting if the ref is initially null, since ref updates do not trigger a re-render. Consider adopting a state-based approach or always rendering GestureScreen and handling a null webFrame inside it.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 3369db2 in 2 minutes and 41 seconds
More details
- Looked at
291lines of code in1files - Skipped
0files when reviewing. - Skipped posting
15drafted comments based on config settings.
1. bun.lock:178
- Draft comment:
Renaming package '@onlook/preload' to '@onlook/web-preload'. Ensure corresponding package.json files 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 = 0% vs. threshold = 50%
This comment is about dependency changes and package renaming, which falls under the rule "Do NOT comment on dependency changes, library versions that you don't recognize, or anything else related to dependencies." Package renames and corresponding package.json updates are part of dependency management.
Could this package rename cause breaking changes that need attention? Could missing package.json updates cause build failures?
Even if there are potential issues, they would be caught by the build system automatically. The rules explicitly state not to comment on dependency changes or things that would be caught by the build.
The comment should be deleted as it violates the rule about not commenting on dependency changes. Any issues would be caught by the build system.
2. bun.lock:216
- Draft comment:
Renamed '@onlook/test-project' to '@onlook/web-template'. Confirm that all references to the old name are updated consistently. - 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 all references to the old name are updated consistently. This falls under the rule of not asking the author to double-check things or ensure behavior is intended. Therefore, this comment should be removed.
3. bun.lock:2395
- Draft comment:
Inconsistent Next.js versions: 'next' is 14.2.25 in the web-template app, yet overall entry upgrades to 15.3.0. Verify intended version alignment. - 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%
Version mismatches in dependencies can cause real problems, but this is a lockfile - its entire purpose is to lock down exact versions. The fact that different parts of the monorepo use different versions is likely intentional, as upgrading major versions (14 to 15) often requires coordination and testing. The comment is asking for verification rather than pointing out a clear problem.
The version mismatch could potentially cause issues if these apps need to share Next.js-specific code or components. Major version differences (14 vs 15) are significant.
However, the comment is phrased as a request for verification rather than pointing out a specific issue. If there were actual compatibility problems, they would be caught by the build system. The lockfile's purpose is precisely to allow such version differences when needed.
The comment should be deleted as it's asking for verification rather than pointing out a clear issue that needs fixing. Version differences in a lockfile are often intentional.
4. bun.lock:2225
- Draft comment:
Upgraded 'lucide-react' from 0.438.0 to 0.486.0. Confirm that peer dependency ranges remain compatible with consuming packages. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is related to a dependency change, specifically an upgrade of thelucide-reactpackage. The comment asks the PR author to confirm compatibility with peer dependencies, which falls under the rule of not asking the author to confirm or ensure things. Therefore, this comment should be removed.
5. bun.lock:364
- Draft comment:
Removal of duplicate 'lodash.debounce' entries—verify that runtime functionality isn’t affected by this cleanup. - 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 runtime functionality isn't affected by the removal of duplicate entries. This falls under asking the author to ensure behavior is intended or to double-check things, which is against the rules.
6. bun.lock:3791
- Draft comment:
PR description mentions onboarding docs for frameworks, but changes are limited to dependency lockfile updates. Confirm that documentation changes are included elsewhere. - 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 if documentation changes are included elsewhere, which violates the rule against asking the author to confirm their intention or to update the PR description. The comment does not provide a specific code suggestion or request for tests, and it is related to dependency changes, which should not be commented on.
7. bun.lock:178
- Draft comment:
Renaming '@onlook/preload' to '@onlook/web-preload' is reflected here. Please ensure all code imports are updated accordingly. - 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 all code imports are updated accordingly after a rename. This falls under asking the author to double-check things, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue, making it not useful according to the guidelines.
8. bun.lock:219
- Draft comment:
Renaming '@onlook/test-project' to '@onlook/web-template' in this block. Verify that all project configurations reference the new name. - 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 all project configurations reference the new name. This is a request for confirmation and verification, which violates the rule against asking the author to confirm or ensure things. Therefore, this comment should be removed.
9. bun.lock:364
- Draft comment:
Removal of 'lodash.debounce' dependencies here (and at line 416) – confirm that this library is no longer needed by any code. - 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 a dependency is no longer needed, which violates the rule against asking for confirmation or verification of intention. The comment does not provide a specific code suggestion or ask for a test to be written. Therefore, it should be removed.
10. bun.lock:799
- Draft comment:
Workspace mappings now use '@onlook/web-preload' and '@onlook/web-template'. Verify that all consumer modules in the monorepo reference these updated keys. - 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 all consumer modules reference the updated keys. This falls under the rule of not asking the author to double-check things or ensure behavior is intended. It doesn't provide a specific code suggestion or ask for a specific test to be written.
11. bun.lock:2395
- Draft comment:
Global 'next' package is updated to 15.3.0. Please double‐check that this upgrade is compatible with your codebase. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to double-check the compatibility of a dependency upgrade, which violates the rule against asking for confirmation or verification of intentions. It also relates to a dependency change, which we should not comment on unless it's a specific version issue that we recognize.
12. bun.lock:1025
- Draft comment:
Updated '@swc/helpers' from 0.5.5 to 0.5.15 with changes in dependencies. Verify there are no breaking changes in helper usage. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is related to a dependency update and asks the author to verify for breaking changes, which violates the rules. It doesn't provide a specific suggestion or point out a specific issue.
13. bun.lock:3237
- Draft comment:
The lucide‐react version in '@onlook/web-template' remains at 0.438.0, while the global lock entry is updated to 0.486.0. Please confirm if these should be aligned. - 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%
While the comment identifies a real version mismatch, it's asking for confirmation rather than stating a clear problem. The comment is about dependency versions, which according to the rules should not be commented on. The rules specifically state "Do NOT comment on dependency changes, library versions that you don't recognize, or anything else related to dependencies."
The version mismatch could potentially cause issues if the versions are incompatible. Maybe this deserves attention despite the rules?
No, the rules are clear about not commenting on dependency versions. Package managers and build systems will catch any real compatibility issues.
Delete the comment as it violates the rule about not commenting on dependency versions.
14. bun.lock:3225
- Draft comment:
ESLint-related entries have been renamed from '@onlook/test-project' to '@onlook/web-template'. Ensure that the ESLint configuration files reference these updated keys. - Reason this comment was not posted:
Comment was on unchanged code.
15. bun.lock:3791
- Draft comment:
PR description mentions onboarding docs improvements, but the changes here are solely dependency updates. Please confirm that documentation changes are included in this PR or in an accompanying commit. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to confirm whether documentation changes are included, which violates the rule against asking for confirmation or updates to the PR description. It does not provide a specific code suggestion or point out a specific issue with the code itself.
Workflow ID: wflow_vJOKm1I58XD08rN2
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.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on d31b050 in 1 minute and 48 seconds
More details
- Looked at
34lines of code in2files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsx:17
- Draft comment:
Check handling when webFrame is null. Instead of only logging, consider a fallback mechanism (or error boundary) to better handle missing ref in production. - 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 handling (return null) is actually a reasonable fallback - it prevents the component from crashing and provides developer feedback. An error boundary would be overkill since this isn't an error case, just an initialization state. The component gracefully handles the null case already. The suggestion seems like overengineering.
Maybe I'm underestimating the impact of a missing webFrame - could this cause issues for parent components or user experience?
The current null handling is defensive and follows React patterns. Parent components can handle the null return appropriately, and the log helps debugging. More complex error handling isn't clearly beneficial.
The comment should be deleted as the current null handling is appropriate and the suggested changes would add unnecessary complexity.
2. apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx:26
- Draft comment:
GestureScreen is now rendered unconditionally since it handles a null webFrame. Verify tests cover cases where webFrameRef.current is null. - 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 tests cover specific cases, which violates the rule against asking the author to ensure that their change is tested. The comment does not provide a specific suggestion or point out a specific issue with the code.
3. apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsx:15
- Draft comment:
Using webFrameRef.current directly may not update the component when the ref becomes non-null since ref changes don’t trigger re-renders. Consider using state or an effect to update when the web frame is ready. - 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 React pattern concern about ref updates. However, since this component is using MobX with observer(), it will re-render when observable state changes. The webFrameRef is likely set during initial mount and the observer pattern will handle subsequent updates. The current implementation seems intentional and working within the MobX paradigm.
I might be making assumptions about when the webFrameRef gets set. There could be edge cases where the ref updates after initial mount that this pattern would miss.
While edge cases are possible, the code's use of MobX observer pattern suggests this is a deliberate design choice that works within their architecture. The current pattern is clean and straightforward.
The comment should be removed. The current implementation appears intentional and works within the MobX reactive pattern. The suggested change would add complexity without clear benefit.
4. apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsx:17
- Draft comment:
Consider using console.error (or another error-reporting mechanism) instead of console.log when no web frame is found. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsx:14
- Draft comment:
The 'isResizing' constant is always false and not updated. Remove it or add a comment explaining its intended future use. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx:1
- Draft comment:
The PR description mentions onboarding docs updates for frameworks, but no documentation changes are visible in this diff. Please ensure the intended documentation updates 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_C9N5vvdho7blU2rL
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.
ml-delaurier
pushed a commit
to ml-delaurier/nolook
that referenced
this pull request
Apr 23, 2025
* Add build script * Add iframe polyfill * Add preload apis * Established connection * More iframe connection * Connect with mousedown * Better port management * Fix frame * Fix next dep * Update render
t1c1
pushed a commit
to t1c1/onlookbotcodes
that referenced
this pull request
Jun 5, 2025
* Add build script * Add iframe polyfill * Add preload apis * Established connection * More iframe connection * Connect with mousedown * Better port management * Fix frame * Fix next dep * Update render
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 gesture handling and web frame components to use Penpal for iframe communication, update mouse event handling, and adjust configurations for local development.
GestureScreeningesture.tsxto handle mouse events usingwebFrameinstead ofwebview.getRelativeMousePositionandhandleMouseEventfunctions to manage mouse actions.WebFrameComponentinweb-frame.tsxto usePenpalfor communication with iframes.WebFrameViewtype and refactor methods for iframe interactions.urlincanvas/index.tstohttp://localhost:8084.docker-compose.ymlto expose port8084.package.jsonscripts and dependencies to include@onlook/web-preloadand@onlook/web-template.This description was created by
for d31b050. It will automatically update as commits are pushed.