fix:shortcut for changing hand and cursor mode#5131
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview
Reviewed by Cursor Bugbot for commit 08248c1. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3b8f2b8. Configure here.
Greptile SummaryThis PR adds
Confidence Score: 3/5The new shortcut commands are wired up correctly at the handler level, but the shortcut key strings in the registry require Shift while every user-visible label omits it, so the advertised shortcuts will not work as shown. The core behaviour of the feature is broken as shipped: shortcuts are registered as Shift+P / Shift+M but the tooltip and popover both display bare P / M, meaning users following the UI hint will never trigger the command. The fix is a one-line change in either the registry or the labels. Both changed files need attention: commands-utils.ts to decide whether the shortcut should be bare P/M or Shift+P/Shift+M, and workflow-controls.tsx to update the displayed keys to match whatever is chosen. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant GlobalCommandsProvider
participant CanvasModeStore
User->>GlobalCommandsProvider: keydown P (as shown in UI)
GlobalCommandsProvider->>GlobalCommandsProvider: "matchesShortcut() requires shiftKey===true"
GlobalCommandsProvider-->>User: No match, command ignored
User->>GlobalCommandsProvider: keydown Shift+P (actual registered)
GlobalCommandsProvider->>CanvasModeStore: setMode('cursor')
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant GlobalCommandsProvider
participant CanvasModeStore
User->>GlobalCommandsProvider: keydown P (as shown in UI)
GlobalCommandsProvider->>GlobalCommandsProvider: "matchesShortcut() requires shiftKey===true"
GlobalCommandsProvider-->>User: No match, command ignored
User->>GlobalCommandsProvider: keydown Shift+P (actual registered)
GlobalCommandsProvider->>CanvasModeStore: setMode('cursor')
Reviews (1): Last reviewed commit: "fix:shortcut for changing hand and curso..." | Re-trigger Greptile |
| 'set-canvas-mode-pointer': { | ||
| id: 'set-canvas-mode-pointer', | ||
| shortcut: 'Shift+P', | ||
| allowInEditable: false, | ||
| }, | ||
| 'set-canvas-mode-mover': { | ||
| id: 'set-canvas-mode-mover', | ||
| shortcut: 'Shift+M', | ||
| allowInEditable: false, | ||
| }, |
There was a problem hiding this comment.
Shift+P / Shift+M) does not match what the UI displays (P / M). The matchesShortcut function enforces an exact match on e.shiftKey, so pressing bare P or M alone will never fire these commands. Either drop the Shift+ prefix here to match the displayed label, or update the tooltip and popover labels to show ⇧P / ⇧M.
| 'set-canvas-mode-pointer': { | |
| id: 'set-canvas-mode-pointer', | |
| shortcut: 'Shift+P', | |
| allowInEditable: false, | |
| }, | |
| 'set-canvas-mode-mover': { | |
| id: 'set-canvas-mode-mover', | |
| shortcut: 'Shift+M', | |
| allowInEditable: false, | |
| }, | |
| 'set-canvas-mode-pointer': { | |
| id: 'set-canvas-mode-pointer', | |
| shortcut: 'P', | |
| allowInEditable: false, | |
| }, | |
| 'set-canvas-mode-mover': { | |
| id: 'set-canvas-mode-mover', | |
| shortcut: 'M', | |
| allowInEditable: false, | |
| }, |
| @@ -137,7 +152,10 @@ export const WorkflowControls = memo(function WorkflowControls() { | |||
| }} | |||
| > | |||
| <Cursor className='size-3' /> | |||
| <span>Pointer</span> | |||
| <div className=' flex items-center gap-2 '> | |||
There was a problem hiding this comment.
className strings have stray whitespace — the first has a trailing space and the second has both a leading and trailing space. These are harmless but inconsistent with the surrounding code style.
| <div className='flex items-center gap-2'> | |
| <span>Mover</span> | |
| <span className='opacity-70'>M</span> | |
| </div> | |
| </PopoverItem> | |
| <PopoverItem | |
| onClick={() => { | |
| setMode('cursor') | |
| setIsCanvasModeOpen(false) | |
| }} | |
| > | |
| <Cursor className='size-3' /> | |
| <div className='flex items-center gap-2'> |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Adds keyboard shortcuts for switching canvas modes and updates the matching UI labels.
P→ switch to Pointer (cursor) modeM→ switch to Mover (hand) modeType of Change
Testing
COMMAND_DEFINITIONS.workflow-controls.tsxcallssetMode('cursor')/setMode('hand')from the handlers.allowInEditable: false, so they won't fire while typing in inputs/textareas.Checklist
Screenshots/Videos
before :
https://github.com/user-attachments/assets/dc64c564-56ad-49d8-b3bc-43757b3ae633
after :
after.mp4