Gizmo kb controls#539
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 28 minutes and 21 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis pull request consolidates keyboard placement and color-picking modes from multiple files into a shared Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/gizmos.js (1)
140-168:⚠️ Potential issue | 🟠 MajorTear down the window click picker on every exit path.
onPickMeshonly unregisters itself when the user clicks outside the canvas. If Escape ordisableGizmos()ends color-picking, thiswindowlistener survives, so later clicks can still repaint meshes after the mode was cancelled. Re-entering the mode will also stack another listener.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/gizmos.js` around lines 140 - 168, The window click listener added in pickMeshFromCanvas (onPickMesh) is only removed when clicking outside the canvas, causing leaked listeners when color-picking is cancelled via Escape or disableGizmos(); fix by centralizing cleanup: create a cleanup function that removes window.removeEventListener("click", onPickMesh), clears any pending setTimeout handle, calls stopCanvasKeyboardMode(), and restores cursors, then ensure this cleanup is invoked both from onPickMesh's outside-canvas branch and passed into startCanvasKeyboardMode's cancel/escape handler (and also called whenever disableGizmos() cancels the mode); also remove any pre-existing onPickMesh listener before adding to avoid stacking.
🧹 Nitpick comments (1)
ui/canvas-utils.js (1)
3-4: Trim the narration comments in this module.Most of these comments just restate the next line and make a small utility file harder to scan. Please keep only the ones that add non-obvious context.
As per coding guidelines, "Comments should be infrequent" and "Comments should be genuinely noteworthy".
Also applies to: 11-11, 16-16, 24-24, 28-28, 33-33, 42-42, 49-49, 72-74, 81-81, 88-88, 107-107, 120-120, 124-124, 131-131, 167-168, 184-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/canvas-utils.js` around lines 3 - 4, Remove trivial "narration" comments that merely restate the next line and retain only comments that add non-obvious context or API usage notes; specifically remove or shorten the duplicate/descriptive comments (e.g., the "Create yellow circle for canvas position indicator" style lines and the other occurrences called out at lines 11,16,24,28,33,42,49,72-74,81,88,107,120,124,131,167-168,184) so the helper file is easier to scan, but keep concise JSDoc or explanatory notes for exported functions and any comments that explain non-obvious behavior or important tradeoffs in functions in this module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/addmenu.js`:
- Around line 819-838: The call to startCanvasKeyboardMode uses
keyboardNavigationActive which has already been reset by callers (so
showCircleImmediately is effectively false); capture the pre-clear state into a
local variable (e.g. wasKeyboardNavigationActive) before
removeKeyboardNavigation() is called, and pass that variable as the second
argument to startCanvasKeyboardMode instead of reading keyboardNavigationActive
directly; update references in startPlacementKeyboardMode to use that saved
value so the visible circle is shown immediately when placement was started via
keyboard.
In `@ui/gizmos.js`:
- Around line 25-30: Remove the unused imports getCanvasCircle and
destroyCanvasCircle from the import list in the top-level import statement
(leave only startCanvasKeyboardMode and stopCanvasKeyboardMode), so ESLint no
longer flags them as dead code; alternatively, if those functions should be
used, add the missing references where appropriate, but the minimal fix is to
delete the two unused symbols from the import.
---
Outside diff comments:
In `@ui/gizmos.js`:
- Around line 140-168: The window click listener added in pickMeshFromCanvas
(onPickMesh) is only removed when clicking outside the canvas, causing leaked
listeners when color-picking is cancelled via Escape or disableGizmos(); fix by
centralizing cleanup: create a cleanup function that removes
window.removeEventListener("click", onPickMesh), clears any pending setTimeout
handle, calls stopCanvasKeyboardMode(), and restores cursors, then ensure this
cleanup is invoked both from onPickMesh's outside-canvas branch and passed into
startCanvasKeyboardMode's cancel/escape handler (and also called whenever
disableGizmos() cancels the mode); also remove any pre-existing onPickMesh
listener before adding to avoid stacking.
---
Nitpick comments:
In `@ui/canvas-utils.js`:
- Around line 3-4: Remove trivial "narration" comments that merely restate the
next line and retain only comments that add non-obvious context or API usage
notes; specifically remove or shorten the duplicate/descriptive comments (e.g.,
the "Create yellow circle for canvas position indicator" style lines and the
other occurrences called out at lines
11,16,24,28,33,42,49,72-74,81,88,107,120,124,131,167-168,184) so the helper file
is easier to scan, but keep concise JSDoc or explanatory notes for exported
functions and any comments that explain non-obvious behavior or important
tradeoffs in functions in this module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75064fba-6811-4bc5-80a3-4d785c555e45
📒 Files selected for processing (4)
style.cssui/addmenu.jsui/canvas-utils.jsui/gizmos.js
…nto gizmo-kb-controls
2 participants
Summary
A yellow circle 'cursor' (
canvasCircle) is implemented to be controlled by the keyboard within the canvas in place of the mouse cursor, for placing objects, selecting objects to colour etc. This code has been moved to its own module so that it can be reused whenever acanvasCircleis needed. There can only ever be one canvasCircle active at one time.This will be helpful in the future for other gizmos which need to use the canvasCircle.
Claude Sonnet 4.6 was used as a pair programmer collaborator, but all decisions and design were made by a human.
Summary by CodeRabbit
New Features
Refactor