Block context menu refactor#687
Conversation
|
Warning Review limit reached
More reviews will be available in 34 minutes and 2 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR moves Blockly context menu, clipboard, paste, and floating block toolbar logic into ChangesBlockly workspace UI behavior
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ui/contextmenu.js`:
- Around line 645-647: The pointerdown event listener is attached to
injectionDiv, but on mobile the blocklyinit.js shim stops propagation before the
event reaches injectionDiv, causing the toolbar scheduling to fail on first
selection. Move the addEventListener call from injectionDiv to document to
capture the pointerdown event before propagation is stopped, and update the
handler to check if any hidden toolbar exists on the currently selected block
and treat it as openable. Apply the same fix to the similar code block
referenced at lines 745-759.
- Around line 584-600: The translate() function returns the key string when a
translation is missing, so patterns like translate('duplicate_button') ||
'Duplicate' never reach the English fallback. Replace all toolbar button
aria-label assignments (duplicateBtn, deleteBtn, detachBtn, and others
referenced in lines 609-632 and 681-693) with a consistent translation pattern
that properly provides English fallbacks. Instead of using the || operator after
translate(), pass the English fallback text as a parameter to the translate
function or use an alternative pattern that actually falls back when
translations are missing, ensuring all toolbar labels use the same approach
throughout the file.
- Around line 168-170: The empty catch block in the registry.unregister call is
triggering ESLint no-empty and no-unused-vars errors. Fix this by adding an
eslint-disable comment above the try-catch statement to suppress both the
no-empty and no-unused-vars rules, or alternatively add a comment inside the
empty catch block such as // ignore error to satisfy the no-empty rule while
keeping the catch block empty.
- Around line 843-849: The delete flow in the context menu calls
`Blockly.Toast.show` unconditionally after `block.checkAndDelete()`, unlike the
rest of `ui/contextmenu.js` where `Blockly.Toast?.show` is used. Update the
delete handler to guard the toast invocation with optional chaining in the same
way as the other `Blockly.Toast` usages, keeping the `checkAndDelete()` behavior
unchanged and preventing runtime errors when Toast is unavailable.
- Around line 285-292: In the blockPaste context menu callback (lines 285-292),
replace the use of globally selected item with scope.block to ensure paste
attaches to the right-clicked block. Additionally, add a check to validate that
the selected item is actually a Block type before passing it to
pasteAsChildOrHere, since Blockly selectables can include workspace comments
which will crash when accessing inputList. Apply the same validation logic to
the keyboard paste handler in the mentioned section (lines 558-564) to prevent
passing non-block selectables to pasteAsChildOrHere.
- Around line 470-541: Strategies 1 and 4 in the block pasting logic are missing
occupancy checks before connecting pasted blocks. In strategy 1, before calling
targetBlock.nextConnection.connect(pb.previousConnection), verify that
targetBlock.nextConnection is free (either by checking
!targetBlock.nextConnection.targetBlock() or by disconnecting any existing
connection first). Similarly in strategy 4, before calling
targetBlock.previousConnection.connect(pb.nextConnection), verify that
targetBlock.previousConnection is free using the same pattern. Follow the
approach used in strategy 2b which explicitly calls disconnect() before
connecting to ensure existing chains are properly preserved when pasting onto
blocks in the middle of a stack.
In `@ui/gizmos.js`:
- Around line 467-471: The issue is that after calling
`restoreFreeCameraFromOrbit()`, the code unconditionally calls
`disconnectOrbitView()` even when the restore operation failed to deactivate the
orbit camera. When restoration fails and the orbit camera remains active,
calling `disconnectOrbitView()` clears the flag without actually disconnecting,
allowing a new orbit camera to be attached on top. The fix is to check if the
orbit camera is still active after attempting restoration, and if it is still
active, return without calling `disconnectOrbitView()` to prevent the
fall-through behavior that would attach another orbit camera on top of the
existing one. This applies to both the section at lines 467-471 and the similar
code at lines 690-694.
🪄 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: 661806ef-5553-480c-92e3-e139f5933ccd
📒 Files selected for processing (3)
main/blocklyinit.jsui/contextmenu.jsui/gizmos.js
|
@tracygardner There is a regression bug where sometimes the context menu doesn't show up on a block. I haven't got time to fix it today and Claude is also down, but I've noted it for next time. I can also implement any design changes then once you've had chance to have a look. |
1 participant
Summary
Refactor the 'right click' and mobile context menus into a separate file, and enable what was previously the mobile context menu to appear in desktop view too for mouse users.
AI usage
Claude Sonnet 4.6 and Opus 4.8 used interactively to find and make updates. Edits approved individually with modifications to approach.
Summary by CodeRabbit
New Features
Improvements