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. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| '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!
|
Hi @waleedlatif1 @TheodoreSpeaks , are u guys open for contributions ?? |
1 participant
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