feat(ui): add Ctrl+N/P navigation in command palette#2549
feat(ui): add Ctrl+N/P navigation in command palette#2549jcalixte wants to merge 6 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdded Ctrl+N and Ctrl+P keyboard shortcuts to the command palette so they behave like ArrowDown/ArrowUp while the palette is open; updated the English instruction string to mention these shortcuts. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ 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 |
|
Hello! Thank you for opening your first PR to npmx, @jcalixte! 🚀 Here’s what will happen next:
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
![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)
app/components/CommandPalette.client.vue (1)
176-185:⚠️ Potential issue | 🟡 MinorConstrain Ctrl+N/P to the exact shortcut combo.
Lines 176 and 185 currently match any
Ctrl+N/Pvariant, including when combined withAlt,Meta, orShift. This can unintentionally trigger navigation for unrelated keyboard shortcuts whilst the palette is open. The codebase establishes the pattern of explicitly guarding against unwanted modifiers (see lines 160–164 for the toggle shortcut and lines 199–205 for the escape handler), so these handlers should follow the same convention.Suggested patch
- if (event.key === 'ArrowDown' || (event.ctrlKey && event.key.toLowerCase() === 'n')) { + if ( + event.key === 'ArrowDown' || + (event.ctrlKey && + !event.altKey && + !event.metaKey && + !event.shiftKey && + event.key.toLowerCase() === 'n') + ) { @@ - if (event.key === 'ArrowUp' || (event.ctrlKey && event.key.toLowerCase() === 'p')) { + if ( + event.key === 'ArrowUp' || + (event.ctrlKey && + !event.altKey && + !event.metaKey && + !event.shiftKey && + event.key.toLowerCase() === 'p') + ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/CommandPalette.client.vue` around lines 176 - 185, The Ctrl+N/P handlers currently match any combination that includes Ctrl (e.g., Ctrl+Alt+N) because they only check event.ctrlKey and key; update both keyboard handlers (the ArrowDown block that uses getCommandElements, flatCommands.value and focusCommand, and the ArrowUp block) to require exactly Ctrl with no other modifiers by adding checks !event.altKey && !event.metaKey && !event.shiftKey in the condition (keep the event.key.toLowerCase() === 'n' / 'p' checks) so navigation only triggers for the precise Ctrl+N and Ctrl+P shortcuts.
🤖 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 `@app/components/CommandPalette.client.vue`:
- Around line 176-185: The Ctrl+N/P handlers currently match any combination
that includes Ctrl (e.g., Ctrl+Alt+N) because they only check event.ctrlKey and
key; update both keyboard handlers (the ArrowDown block that uses
getCommandElements, flatCommands.value and focusCommand, and the ArrowUp block)
to require exactly Ctrl with no other modifiers by adding checks !event.altKey
&& !event.metaKey && !event.shiftKey in the condition (keep the
event.key.toLowerCase() === 'n' / 'p' checks) so navigation only triggers for
the precise Ctrl+N and Ctrl+P shortcuts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 995477e0-7fdd-4113-af74-7f79efe65f8d
📒 Files selected for processing (2)
app/components/CommandPalette.client.vuei18n/locales/en.json
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@jcalixte I agree that the arrow-keys are awkward, but tab works well for me at least. Any reason this doesn't work for you? I'd personally be a bit hesitant about adding something emacs specific (I'm not an emacs user, but I understand To be clear, not saying to do one or the other, just questioning whether adding emacs specific modifiers as a default is too opinionated. |
|
Hi @jhroemer, thanks for taking time for answering me! I completely forgot that the tab / shift+tab was a possibility as I'm learning new ways to develop others than in VS Code, I may have been in a rabbit hole these past months 🫣 I agree with you that this feature may be too opinionated. If you agree, I'll close this PR then. Thank you! |
2 participants
Hi, I propose the shortcuts in the Command Palette
ctrl+n/pfor list navigation that can be a good fit for keyboard-heavy users for list navigation without affecting anyone else.Disclaimer : I've primarly used Claude Code to generate this Pull Request.
Summary
Ctrl+N(next) andCtrl+P(previous) as alternative navigation shortcuts in the command palette, alongside the existingArrowDown/ArrowUpkeyscommand_palette.instructionsi18n string to mentionCtrl+N/PImplementation
The two existing
ArrowDown/ArrowUpconditions inhandleGlobalKeydownwere extended with an||clause — minimal change with no logic duplication. I reused the existinggetCommandElements(),focusCommand(), andfocusInput()helpers.