Web version pt.4 #1730
Merged
Web version pt.4 #1730
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 973706c in 1 minute and 35 seconds
More details
- Looked at
315lines of code in3files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. apps/web/src/app/project/[id]/_components/canvas/frames/frame.tsx:16
- Draft comment:
Consider refactoring dimensions state to use a more trackable structure (e.g., via Yjs) as noted in the TODO comment. This might improve state sync. - 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 is about changed code (new dimensions state). However, it's essentially just agreeing with and restating an existing TODO comment. It doesn't provide any concrete actionable advice beyond what's already documented. It's more of an observation than a required change.
The comment does point out a potential architectural improvement. Maybe having this in both TODO and PR comment format could emphasize its importance?
The TODO already captures this concern, and our rules specifically state not to make purely informative comments. The comment doesn't provide additional actionable information.
Delete the comment as it's redundant with the TODO and doesn't provide specific actionable guidance beyond what's already documented in the code.
2. apps/web/src/app/project/[id]/_components/canvas/frames/resize-handles.tsx:146
- Draft comment:
Multiple elements share the same ref (resizeHandleRef). If individual refs are needed for each handle, consider using separate refs. - 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/src/app/project/[id]/_components/canvas/frames/frame.tsx:16
- Draft comment:
Consider elaborating on the TODO: clarify how Yjs integration should work and whether the dimensions state should sync with external changes. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. apps/web/src/app/project/[id]/_components/canvas/frames/gesture.tsx:192
- Draft comment:
Avoid inline useCallback in JSX props; define the onMouseOut handler outside the JSX for better referential stability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. apps/web/src/app/project/[id]/_components/canvas/frames/resize-handles.tsx:146
- Draft comment:
Avoid assigning the same ref to multiple elements. Use separate refs (or remove if not needed) to ensure each element is referenced correctly. - 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_x9eMLfBeeWAh7GgF
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.
| className={gestureScreenClassName} | ||
| onClick={handleClick} | ||
| onMouseOver={() => setHovered(true)} | ||
| onMouseOut={useCallback(() => { |
There was a problem hiding this comment.
Inline useCallback in JSX (onMouseOut) is re-created on each render. Consider extracting it outside the JSX for performance.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 3a31fb4 in 1 minute and 57 seconds
More details
- Looked at
1555lines of code in18files - Skipped
0files when reviewing. - Skipped posting
25drafted comments based on config settings.
1. apps/studio/src/routes/editor/LayersPanel/LayersTab.tsx:17
- Draft comment:
Consider adding 'handleSelectChange' to the dependency array or memoizing it via useCallback. - 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/src/app/project/[id]/_components/canvas/frames/gesture.tsx:15
- Draft comment:
The getWebview function is entirely commented out; if it’s not used, consider removing unused code. - 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 appears to be part of a larger refactoring where much of the file's functionality is commented out. The getWebview function is still referenced by active code, so removing it would break things. The real issue is that it's returning undefined while being used. This suggests the code is in an intermediate state during refactoring.
Maybe this is intentionally left as a stub during refactoring? The fact that so much code is commented out suggests this is work in progress.
Even if it's a work in progress, having a function that's called but returns undefined is dangerous. This needs attention.
The comment is correct that there's an issue, but suggesting removal is wrong since the function is still being used. The real issue is that it needs to be properly implemented.
3. apps/web/src/app/project/[id]/_components/canvas/frames/resize-handles.tsx:94
- Draft comment:
Casting event listeners using 'as unknown as EventListener' circumvents strict type checks. Consider using proper types for event handlers. - 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 point about type safety. The code is using a type assertion to bypass TypeScript's type checking, which could hide potential type mismatches. However, looking at the code, the handlers are properly typed MouseEvent handlers that match the expected signature. The casting seems to be needed due to DOM API typing limitations rather than actual type safety issues.
The type casting might be necessary due to DOM API limitations in TypeScript, and there may not be a better way to handle this without the type assertion. The comment might be leading to unnecessary complexity.
While type safety is important, this pattern is common when working with DOM events in TypeScript, and the underlying types are actually compatible. The casting isn't hiding real type safety issues.
The comment should be removed. The type casting, while not ideal, appears to be a necessary workaround for TypeScript DOM event handling, and the actual types are compatible.
4. apps/web/src/app/project/[id]/_components/canvas/frames/right-click.tsx:161
- Draft comment:
The updateMenuItems function is completely commented out; ensure that this is intentional as it prevents menu items from updating. - 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 appears to be a new file with intentionally disabled functionality. The pattern of commented out code and empty action handlers (action: () => {}) throughout suggests this is scaffolding or work in progress. The author likely knows the function is commented out since they wrote it that way. The comment isn't providing new information or suggesting a clear improvement.
The comment could be valid if this was an accidental commenting out that breaks functionality. But the systematic pattern of disabled features suggests this is intentional.
Even if this was accidental, the comment merely points out what's obvious from the code without suggesting how to fix it or identifying a specific problem.
Delete the comment. It points out something that's obvious from reading the code, and the systematic pattern of disabled functionality suggests the commenting out was intentional.
5. apps/web/src/app/project/[id]/_components/canvas/index.tsx:117
- Draft comment:
Consider including editorEngine in the dependency arrays for middleMouseButtonDown/up useCallbacks if editorEngine may change over time. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
6. apps/studio/src/routes/editor/LayersPanel/LayersTab.tsx:7
- Draft comment:
Good use of a named export (RightClickMenu) for consistency. Ensure all consumers are updated accordingly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
7. apps/studio/src/routes/editor/RightClickMenu/index.tsx:227
- Draft comment:
Removed default export; verify that all imports use the named export. This improves clarity and consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
8. apps/web/src/app/project/[id]/_components/canvas/frames/frame.tsx:1
- Draft comment:
The old Frame component has been removed. Ensure that all functionality is migrated to the new FrameView in index.tsx. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
9. apps/web/src/app/project/[id]/_components/canvas/frames/frame/index.tsx:31
- Draft comment:
FrameView now renders GestureScreen without props. Confirm that GestureScreen no longer needs webviewRef or setHovered. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
10. apps/web/src/app/project/[id]/_components/canvas/frames/frame/web.tsx:1
- Draft comment:
WebFrameComponent cleanly renders an iframe from WebFrame data. No issues noted. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
11. apps/web/src/app/project/[id]/_components/canvas/frames/gesture.tsx:11
- Draft comment:
GestureScreen has large sections of code commented out and uses a dummy setHovered function. Remove unused code or document the intent. - Reason this comment was not posted:
Marked as duplicate.
12. apps/web/src/app/project/[id]/_components/canvas/frames/index.tsx:8
- Draft comment:
Frames container maps over frames and renders FrameView correctly. No issues noted. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
13. apps/web/src/app/project/[id]/_components/canvas/frames/resize-handles.tsx:20
- Draft comment:
The resizing handles are implemented well. Since 'lockedPreset' is always false, consider removing it if not used. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
14. apps/web/src/app/project/[id]/_components/canvas/frames/right-click.tsx:142
- Draft comment:
Right-click menu includes several placeholder actions (commented out). Confirm whether these should be implemented or removed to avoid confusion. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
15. apps/web/src/app/project/[id]/_components/canvas/index.tsx:35
- Draft comment:
Canvas component properly handles zoom and pan. Verify that dependencies in useCallback and useMemo cover all necessary properties from editorEngine. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
16. apps/web/src/app/project/[id]/_components/canvas/overlay/elements/chat.tsx:60
- Draft comment:
OverlayChat looks well implemented. Consider caching document lookups for elementId if it appears multiple times to improve performance. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
17. apps/web/src/components/ide.ts:6
- Draft comment:
IDE class is implemented well. Consider adding inline documentation for its methods (e.g., getCodeCommand) to clarify usage. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
18. apps/web/src/components/store/editor/engine/canvas/frame.ts:1
- Draft comment:
FrameImpl and WebFrameImpl correctly implement observable state. Consider adding additional type annotations if needed for clarity. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
19. apps/web/src/components/store/editor/engine/canvas/index.ts:107
- Draft comment:
CanvasManager contains commented-out logic for saving settings and applying settings. Ensure that persistence is handled appropriately or remove dead code if no longer needed. - 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 comment points out a real issue - there's a lot of commented code and inconsistent handling of settings persistence. Some methods still call saveSettings() while others have it commented out. This could lead to bugs. However, the comment is phrased as a question ("ensure that...") rather than making a clear recommendation.
The comment violates the rule about not asking authors to "ensure" things. Additionally, maybe this commented code is temporary during a refactor and the author is aware of the persistence changes.
While the phrasing could be better, this is pointing out a concrete code quality issue that could cause bugs - inconsistent persistence behavior and large blocks of dead code.
The comment should be rephrased to be more direct, but it identifies a real code quality issue that should be addressed.
20. apps/web/src/components/store/editor/engine/overlay/index.ts:14
- Draft comment:
OverlayManager's reaction callback is commented out. Confirm if overlay refreshing on canvas scale changes is intentionally disabled. - 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 violates the rule about not asking for confirmations or intentions. It's asking "Confirm if... is intentionally disabled" which is exactly the kind of comment we should avoid. The PR author clearly made these changes deliberately by commenting out the code. If it was a mistake, it would be caught in testing.
Maybe the commented-out code represents a serious regression in functionality that should be flagged?
While the change might be significant, asking for confirmation of intention isn't helpful. If there's a concrete issue, it should be stated directly instead of asking if it was intentional.
Delete the comment as it violates the rule about not asking for confirmations of intent. If there's a concrete issue with the change, it should be stated directly.
21. packages/models/src/frame/index.ts:1
- Draft comment:
Frame and WebFrame models are defined correctly. No issues found. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
22. packages/models/src/index.ts:1
- Draft comment:
Aggregated exports in models index file are correctly set up. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
23. apps/web/src/app/project/[id]/_components/canvas/frames/frame/web.tsx:25
- Draft comment:
It looks like there's no newline at the end of the file. For consistency and to avoid potential issues with some tools, please add a newline at the end. - 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 missing newlines at end of file can cause issues with some tools, this is a very minor issue that would likely be caught by linting or formatting tools. It's not a logic issue or code quality concern that requires manual review. The comment is technically correct but not worth human attention.
The comment is factually correct and suggests a concrete fix. Some version control systems and Unix tools do work better with newlines at end of files.
While true, this is exactly the kind of minor formatting issue that should be handled by automated tools rather than manual review comments. It doesn't affect functionality.
Delete this comment as it addresses a minor formatting issue that should be handled by automated tools rather than manual review.
24. apps/web/src/app/project/[id]/_components/canvas/frames/frame/web.tsx:13
- Draft comment:
In the commented code, 'shouldShowDomFailed' might be a minor typographical error. Consider reviewing/correcting it if the comment is intended to be used, e.g., 'shouldShowDOMFailed' or 'shouldShowDomFail' for clarity. - Reason this comment was not posted:
Comment was on unchanged code.
25. apps/web/src/app/project/[id]/_components/canvas/index.tsx:47
- Draft comment:
Typo: The variable 'lintedScale' (line 47) appears to be misnamed. Consider renaming it to 'clampedScale' for clarity, because it holds a value returned by clampZoom. - 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_0m5j8Cx3j83JkJW1
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.
| editorEngine.overlay.state.updateHoverRect(null); | ||
| } | ||
|
|
||
| const setHovered = (hovered: boolean) => { |
There was a problem hiding this comment.
The local setHovered function is a no-op and overrides the prop setter; this is likely a bug.
| </ContextMenuContent> | ||
| </ContextMenu> | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Please add a newline at the end of the file to ensure consistent formatting across different tools.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on ed334a9 in 1 minute and 39 seconds
More details
- Looked at
136lines of code in3files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. apps/web/src/app/project/[id]/_components/canvas/frame/gesture.tsx:21
- Draft comment:
getWebview is defined but does not return a value. If it’s intentionally left unimplemented, add a comment explaining why. - 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/src/app/project/[id]/_components/canvas/frame/gesture.tsx:186
- Draft comment:
Consider cleaning up unused commented-out code to improve readability. - 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/src/app/project/[id]/_components/canvas/frame/gesture.tsx:186
- Draft comment:
Removed the onMouseOver callback that used setHovered. Ensure that hover state is now correctly managed via editorEngine or elsewhere. - 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 removedsetHoveredfunction was just a no-op that did nothing. TheonMouseOverhandler called this no-op function. Removing these has no functional impact. The real hover state management is clearly handled by editorEngine as seen in handleMouseOut. The comment is asking for verification of something that doesn't need verification since we can see the actual hover management code.
I could be wrong if there are hover-related side effects I'm not seeing in other files. The editorEngine's hover management might be incomplete.
The removed code was definitively a no-op, and we can see the actual hover management in the same file. Cross-file issues are out of scope per the rules.
Delete this comment. It asks for verification of hover state management when removing a no-op function, which adds no value.
4. apps/web/src/app/project/[id]/_components/canvas/frame/gesture.tsx:15
- Draft comment:
The getWebview function is declared with a return type but not implemented. Either implement it or remove it if not 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.
5. apps/web/src/app/project/[id]/_components/canvas/frame/index.tsx:19
- Draft comment:
TopBar is rendered with the 'frame' prop but no children. If no extra content is needed inside TopBar, consider removing the children wrapper. - Reason this comment was not posted:
Comment looked like it was already resolved.
6. apps/web/src/app/project/[id]/_components/canvas/frame/index.tsx:23
- Draft comment:
The type assertion (frame as WebFrame) is used when rendering WebFrameComponent. Ensure that the frame is always a WebFrame when frame.type === FrameType.WEB. - 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 condition is met, which violates the rule against asking the author to ensure behavior is intended. It doesn't provide a specific suggestion or point out a specific issue with the code.
7. apps/web/src/app/project/[id]/_components/canvas/frame/top-bar.tsx:51
- Draft comment:
Using 1 / editorEngine.canvas.scale in the style may risk division by zero. Consider adding a defensive check for canvas.scale. - 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 concern about division by zero is mathematically valid. However, canvas scale is likely initialized with a non-zero value and controlled by the editor engine. The same scale is used for calculations in handleMove() and other style properties without checks. This suggests the scale is managed elsewhere and guaranteed to be valid. Without seeing the editor engine code, we can't be certain this is a real issue.
I might be assuming too much about the editor engine's guarantees. A defensive check could prevent runtime errors if there are edge cases we don't know about.
While defensive programming is good practice, this seems like an implementation detail that should be handled by the editor engine itself, not at the component level. The consistent usage throughout the code suggests this isn't a practical concern.
The comment should be removed as it makes assumptions about the codebase without strong evidence that this is actually an issue. The scale management should be handled by the editor engine.
8. apps/web/src/app/project/[id]/_components/canvas/frame/top-bar.tsx:60
- Draft comment:
It appears that there is no newline at the end of the file. Adding a newline at the end of the file is a common style convention that can help prevent issues with some tools. - 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%
Missing newlines at end of file are typically handled automatically by code formatters and linters. This is a very minor issue that doesn't affect functionality. The comment doesn't point out any significant problems that require manual intervention. This kind of formatting issue should be handled by automated tools rather than PR comments.
The comment is technically correct - it is a common convention to have newlines at end of files. Some tools do require this.
While technically correct, this is exactly the kind of minor formatting issue that should be handled by automated tools rather than manual review comments. It doesn't warrant human attention.
Delete this comment as it points out a minor formatting issue that should be handled by automated tools rather than manual review.
Workflow ID: wflow_g4xc47bGfhPnQOkP
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.
|
|
||
| window.removeEventListener('mousemove', handleMove); | ||
| window.removeEventListener('mouseup', endMove); | ||
| }; |
There was a problem hiding this comment.
Ensure window event listeners in startMove are always removed, even if mouseup isn’t triggered (e.g., when mouse leaves window).
There was a problem hiding this comment.
❌ Changes requested. Incremental review on e8bd6f6 in 2 minutes and 17 seconds
More details
- Looked at
433lines of code in6files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. apps/web/src/app/project/[id]/_components/right-panel/chat/controls.tsx:25
- Draft comment:
The button is always disabled due to|| true. Remove or adjust this to conditionally disable based oneditorEngine.chat?.isWaitingonly. - Reason this comment was not posted:
Marked as duplicate.
2. apps/web/src/app/project/[id]/_components/right-panel/chat/panel-dropdown.tsx:117
- Draft comment:
The functionsetIsChatHistoryOpenis used but not defined in this component. Ensure it's passed as a prop or defined locally. - Reason this comment was not posted:
Marked as duplicate.
3. apps/web/src/app/project/[id]/_components/right-panel/chat/panel-dropdown.tsx:75
- Draft comment:
Minor typographical error: Consider changing "Auto - apply results" to "Auto-apply results" for improved 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%
While this is technically correct - compound modifiers should be hyphenated without spaces - this is an extremely minor UI text change. The rules state not to comment on pure frontend UI changes. Even though this is a text label, it's still primarily a UI concern and doesn't affect functionality. The meaning is clear either way.
The hyphenation could potentially affect readability and professional appearance of the UI. Some might argue that maintaining consistent typography is important for code quality.
While consistency is good, this is too minor of a UI issue to warrant a PR comment. The rules explicitly state not to comment on UI changes.
Delete this comment as it violates the rule about not commenting on pure UI/styling changes.
4. apps/web/src/app/project/[id]/_components/right-panel/index.tsx:124
- Draft comment:
There are several instances where JSX tags are preceded by an extra space (e.g.,< Icons.ChevronDown,< TabsTrigger,< TabsContent). While this is a minor cosmetic issue, cleaning up the extra whitespace will improve readability and consistency across the file. - 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, it's pointing out a very minor code style issue. The code will work perfectly fine with or without these spaces. This seems like something that could be better handled by a linter or formatter rather than a PR comment. The comment also doesn't indicate any actual functional problems.
The spaces are a real issue and could indicate inconsistent code formatting practices. Having consistent formatting does make code more maintainable.
While code consistency is important, this is too minor of an issue to warrant a PR comment. It would be better handled by automated tooling like Prettier or ESLint.
Delete this comment as it points out a very minor formatting issue that doesn't affect functionality and would be better handled by automated tooling.
5. apps/web/src/app/project/[id]/page.tsx:18
- Draft comment:
In the className on line 18, the CSS calc expression is written ascalc(100%-2.60rem). For better clarity and to ensure it works correctly, consider adding spaces:calc(100% - 2.60rem). This minor spacing fix will improve code 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%
While technically correct that calc() expressions work better with spaces, this is a minor formatting issue. The current syntax will actually still work in CSS - browsers are quite forgiving with calc() spacing. The change is purely cosmetic and doesn't affect functionality. We should avoid nitpicking on such minor formatting details.
The lack of spaces could potentially cause issues in some edge cases or with certain CSS preprocessors. Also, it does violate common CSS best practices.
While it's true this isn't perfectly formatted CSS, our rules state we should not make purely informative comments or point out obvious/unimportant issues. This is too minor to warrant a comment.
Delete this comment as it's too minor of an issue and doesn't affect functionality.
6. apps/web/src/app/project/[id]/page.tsx:22
- Draft comment:
In the className on line 22, the CSS calc expression is written ascalc(100%-2.60rem). Adding spaces to becomecalc(100% - 2.60rem)improves clarity and ensures proper parsing. Please adjust. - 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 that calc() expressions work better with spaces, this is a very minor stylistic issue. The current syntax will still work in all modern browsers. This feels more like a personal preference than a real issue requiring a code change. We should focus on more substantial problems.
The spaces could potentially improve readability and maintainability. Some CSS parsers might be more strict about calc() syntax.
While spaces might be slightly better, this is too minor of an issue to warrant a PR comment. The code works fine as-is.
Delete this comment as it's too minor and stylistic to be worth addressing in a PR review.
Workflow ID: wflow_nWJyVddYPXvBGRlw
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.
| size={'icon'} | ||
| className="p-2 w-fit h-fit hover:bg-background-onlook" | ||
| onClick={handleNewChat} | ||
| disabled={editorEngine.chat?.isWaiting || true} |
There was a problem hiding this comment.
The disabled prop always evaluates to true (using '|| true'). This prevents button activation. Remove or adjust the constant.
Suggested change
| disabled={editorEngine.chat?.isWaiting || true} | |
| disabled={editorEngine.chat?.isWaiting} |
| const chatSettings = userManager.settings.settings?.chat ?? DefaultSettings.CHAT_SETTINGS; | ||
| const selectedTab = editorEngine.state.editorPanelTab; | ||
|
|
||
| const updateChatSettings = (e: React.MouseEvent, settings: Partial<ChatSettings>) => { |
There was a problem hiding this comment.
The type ChatSettings is not defined or imported. Import or define it to avoid type errors.
| </DropdownMenuItem> | ||
| < DropdownMenuSeparator /> | ||
| <DropdownMenuItem | ||
| onClick={() => setIsChatHistoryOpen(!isChatHistoryOpen)} |
There was a problem hiding this comment.
'setIsChatHistoryOpen' is used but not defined. Define it or remove this call.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 368be20 in 1 minute and 56 seconds
More details
- Looked at
39lines of code in3files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. apps/web/src/app/project/[id]/_components/right-panel/chat/controls.tsx:25
- Draft comment:
Ensure 'disabled' reflects desired state. The change from '|| true' to '|| false' seems correct if you want the button enabled when 'isWaiting' is false. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. apps/web/src/app/project/[id]/_components/right-panel/chat/panel-dropdown.tsx:117
- Draft comment:
Usage of 'setIsChatHistoryOpen' is undefined in this file. Either remove or pass it as a prop from the parent component. - 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/src/app/project/[id]/_components/right-panel/chat/panel-dropdown.tsx:23
- Draft comment:
Avoid leaving commented out functional code. Either remove it or explain why it's kept. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. apps/web/src/app/project/[id]/_components/right-panel/chat/controls.tsx:25
- Draft comment:
Fixed a logic bug: the disabled prop now correctly defaults to false instead of always disabling the button. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, explaining a change that was made. It does not provide a suggestion, ask for confirmation, or point out a potential issue. It violates the rule against making purely informative comments.
5. apps/web/src/app/project/[id]/_components/right-panel/chat/panel-dropdown.tsx:117
- Draft comment:
setIsChatHistoryOpen is referenced but not defined in this component, which may lead to runtime errors. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. apps/web/src/app/project/[id]/_components/right-panel/index.tsx:37
- Draft comment:
Removal of the chat history state (isChatHistoryOpen) here may break the functionality in ChatPanelDropdown that relies on it. - Reason this comment was not posted:
Comment looked like it was already resolved.
7. apps/web/src/app/project/[id]/_components/right-panel/chat/panel-dropdown.tsx:75
- Draft comment:
Typographical issue: The option label 'Auto - apply results' has extra spaces around the hyphen. Consider changing it to 'Auto-apply results' 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_RXY3kRGv46FjrDz0
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 updateChatSettings = (e: React.MouseEvent, settings: Partial<ChatSettings>) => { | ||
| e.preventDefault(); | ||
| // userManager.settings.updateChat(settings); |
There was a problem hiding this comment.
The updateChatSettings call is commented out. If this is intentional, consider removing the unused parameter; otherwise, re-enable the update to allow settings changes.
Suggested change
| // userManager.settings.updateChat(settings); | |
| userManager.settings.updateChat(settings); |
ml-delaurier
pushed a commit
to ml-delaurier/nolook
that referenced
this pull request
Apr 23, 2025
* Working resize * Save * Update frame state * Working frame * Add right panel
t1c1
pushed a commit
to t1c1/onlookbotcodes
that referenced
this pull request
Jun 5, 2025
* Working resize * Save * Update frame state * Working frame * Add right panel
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
Introduces new web version features with enhanced frame handling, context menus, and resizable panels, alongside refactoring and new models for improved editor functionality.
FrameViewinframe/index.tsxto handle frame rendering withGestureScreen,ResizeHandles, andWebFrameComponent.RightClickMenuinright-click.tsxfor context menu functionality.ResizablePanelinresizable.tsxfor adjustable UI panels.FrameandWebFrameinterfaces inframe/index.ts.FrameImplandWebFrameImplclasses incanvas/frame.tsfor frame management.TopBarintop-bar.tsxfor frame movement.ChatControlsandChatPanelDropdowninright-panel/chatfor chat management.CanvasManagerincanvas/index.tsto manage frames and scaling.OverlayManagerinoverlay/index.tsto handle overlay state changes.ide.tsfor IDE command generation.This description was created by
for 368be20. It will automatically update as commits are pushed.