Keyboard controls for Select gizmo#543
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 31 minutes and 36 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. 📝 WalkthroughWalkthroughRefactored Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
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)
903-943:⚠️ Potential issue | 🔴 CriticalBug:
blockKeyreferences the previously selected mesh, not the newly picked one.The
applySelectionfunction uses the outer-scopedblockKey(line 922) to look up and highlight the associated block. However,blockKeyis set fromgizmoManager.attachedMesh(the old selection) in both callers beforeapplySelectionis invoked. This causes the wrong block to be highlighted when selecting a new mesh.The highlighted block will be from the previous selection, while the gizmo attaches to the new mesh.
🐛 Proposed fix: derive blockKey from the picked mesh
function applySelection(pickedMesh, pickedPoint) { if (pickedMesh && pickedMesh.name !== "ground") { const position = pickedMesh.getAbsolutePosition(); const roundedPosition = roundVectorToFixed(position, 2); flock.printText({ text: translate("position_readout").replace( "{position}", String(roundedPosition), ), duration: 30, color: "black", }); if (flock.meshDebug) console.log(pickedMesh.parent); if (pickedMesh.parent) { pickedMesh = getRootMesh(pickedMesh.parent); if (flock.meshDebug) console.log(pickedMesh.visibility); pickedMesh.visibility = 0.001; if (flock.meshDebug) console.log(pickedMesh.visibility); } - const block = meshMap[blockKey]; + const newBlockKey = findParentWithBlockId(pickedMesh)?.metadata?.blockKey; + const block = meshMap[newBlockKey]; highlightBlockById(Blockly.getMainWorkspace(), block); gizmoManager.attachToMesh(pickedMesh); pickedMesh.showBoundingBox = true; } else {With this fix, the outer
blockKeyvariable and its assignments in the callers (lines 952-953, 974-975) become unnecessary and can be removed for cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/gizmos.js` around lines 903 - 943, applySelection is using an outer-scoped blockKey (based on the previous selection) instead of deriving the block key from the newly picked mesh; change applySelection to compute blockKey from the picked mesh (use getRootMesh(pickedMesh or pickedMesh.parent) as needed), look up the block with meshMap[derivedBlockKey], and pass that block into highlightBlockById(Blockly.getMainWorkspace(), block) before attaching the gizmo; also remove the now-unnecessary outer-scope blockKey assignments that were set from gizmoManager.attachedMesh in the callers so selection highlighting is always derived from the current picked mesh.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ui/gizmos.js`:
- Around line 903-943: applySelection is using an outer-scoped blockKey (based
on the previous selection) instead of deriving the block key from the newly
picked mesh; change applySelection to compute blockKey from the picked mesh (use
getRootMesh(pickedMesh or pickedMesh.parent) as needed), look up the block with
meshMap[derivedBlockKey], and pass that block into
highlightBlockById(Blockly.getMainWorkspace(), block) before attaching the
gizmo; also remove the now-unnecessary outer-scope blockKey assignments that
were set from gizmoManager.attachedMesh in the callers so selection highlighting
is always derived from the current picked mesh.
2 participants
Summary
Things this does NOT do
Claude Sonnet 4.6 wrote the
applySelectionfunction within gizmos.js but this reuses most of the existing code. It's mainly there so that both mouse and keyboard controls can use the same code.Part of #360
Summary by CodeRabbit
New Features
Improvements