-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: add keyboard shortcut for "Add to Context" action #7908
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code because apparently I trust no one, not even myself.
src/activate/registerCodeActions.ts
Outdated
| await provider.postMessageToWebview({ type: "action", action: "focusInput" }) | ||
| } | ||
| }, 100) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new focus input functionality lacks test coverage. Since I'm the one who wrote this, I should probably add tests to verify the focus behavior works correctly - but here we are.
src/activate/registerCodeActions.ts
Outdated
| await vscode.commands.executeCommand("roo-cline.SidebarProvider.focus") | ||
| } | ||
| // Send focus input message after a short delay to ensure the view is ready | ||
| setTimeout(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is using setTimeout with a fixed 100ms delay intentional? This could be unreliable on slower systems or when the webview takes longer to initialize. Consider using a more robust approach like waiting for a confirmation message from the webview or implementing a promise-based mechanism.
src/activate/registerCodeActions.ts
Outdated
| setTimeout(async () => { | ||
| const provider = ClineProvider.getVisibleInstance() | ||
| if (provider) { | ||
| await provider.postMessageToWebview({ type: "action", action: "focusInput" }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The postMessageToWebview call here could throw if something goes wrong. Consider wrapping this in a try-catch to handle potential promise rejections gracefully:
| await provider.postMessageToWebview({ type: "action", action: "focusInput" }) | |
| try { | |
| await provider.postMessageToWebview({ type: "action", action: "focusInput" }) | |
| } catch (error) { | |
| // Log error but don't break the flow | |
| console.error("Failed to focus input:", error) | |
| } |
src/package.json
Outdated
| "keybindings": [ | ||
| { | ||
| "command": "roo-cline.addToContext", | ||
| "key": "cmd+'", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment explaining why Cmd/Ctrl+' was chosen as the keyboard shortcut. This helps future maintainers understand the decision and ensures consistency with common IDE patterns.
mrubens
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things on this:
- There's a lot of logic around waiting - could use something like pWaitFor instead of rolling our own?
- Cline uses the same keyboard shortcut. How much does this conflict?
- Can we add some indication in the UI that this exists? Probably showing the keyboard shortcut in the context menu.
… only) - Added keyboard shortcut configuration in package.json: - macOS: Cmd+' - Windows/Linux: Ctrl+' - Shortcut is only active when editor has text selection - Removed all code implementation per review feedback
4dce67a to
395e42e
Compare
This PR addresses Issue #7907 by adding a keyboard shortcut to quickly add selected code to context.
Changes
package.json:Cmd+'Ctrl+'addToContextcommand to also focus the input field after adding contenteditorTextFocus && editorHasSelection)Benefits
Testing
Closes #7907
Important
Adds a keyboard shortcut for the 'Add to Context' action, enhancing workflow by focusing the input field post-action and ensuring shortcut activation only when text is selected.
addToContextcommand inpackage.json.Cmd+'for macOS,Ctrl+'for Windows/Linux.editorTextFocus && editorHasSelection.addToContextto focus input field after action.This description was created by
for be146d8. You can customize this summary. It will automatically update as commits are pushed.