fix(app): prefer cmd+k for command palette#18731
Conversation
atharvau
left a comment
There was a problem hiding this comment.
Code Review - PR #18731
Summary
This PR changes the command palette keybind from to as primary, with as fallback.
✅ Positive Findings
- Good UX improvement: is more standard for command palettes (VS Code, GitHub, etc.)
- Backward compatibility: Keeps as secondary keybind
- Test coverage: Added E2E test for both keybinds
- Consistent implementation: Updates both keybind logic and tests
🔍 Code Quality
- Clean changes: Minimal, focused modifications
- Good test coverage: Both E2E and unit tests updated
- Proper parameterization: Made function more flexible
⚠️ Minor Observations
- The new test logic assumes prefers the first combo - this behavior should be documented or made more explicit in the implementation
- Consider adding a comment explaining the keybind precedence logic
✅ Security & Performance
- No security concerns
- No performance impact
- Changes are UI-only
Recommendation: APPROVE
This is a straightforward UX improvement with proper testing. The code is clean and maintains backward compatibility.
atharvau
left a comment
There was a problem hiding this comment.
Code Review - PR #18731
Summary
This PR changes the command palette keybind from cmd+p to cmd+k as primary, with cmd+p as fallback.
Positive Findings
- Good UX improvement: cmd+k is more standard for command palettes (VS Code, GitHub, etc.)
- Backward compatibility: Keeps cmd+p as secondary keybind
- Test coverage: Added E2E test for both keybinds
- Consistent implementation: Updates both keybind logic and tests
Code Quality
- Clean changes: Minimal, focused modifications
- Good test coverage: Both E2E and unit tests updated
- Proper parameterization: Made openPalette function more flexible
Minor Observations
- The new test logic assumes formatKeybind prefers the first combo - this behavior should be documented
- Consider adding a comment explaining the keybind precedence logic
Security & Performance
- No security concerns
- No performance impact
- Changes are UI-only
Recommendation: APPROVE
This is a straightforward UX improvement with proper testing. The code is clean and maintains backward compatibility.
atharvau
left a comment
There was a problem hiding this comment.
✅ APPROVED - Good UX improvement with proper testing.
Strengths:
- Follows macOS conventions (cmd+k is common for command palettes)
- Maintains backward compatibility with cmd+p as secondary binding
- Includes proper E2E test coverage
- Updates test helpers appropriately
- Adds test for formatKeybind preference behavior
Minor Suggestion:
- Consider documenting this keybind change in migration notes if users are accustomed to cmd+p
The implementation is solid and well-tested.
atharvau
left a comment
There was a problem hiding this comment.
✅ Sensible Keybinding Update
Good change to move the file open dialog to a more intuitive keybinding.
Improvements:
- Better UX: is more common for command palettes across editors
- Maintains compatibility: still works as fallback
- Proper testing: Added E2E test for the fallback
- Smart display: shows primary shortcut (Cmd+K) in UI
Code Quality:
- Clean implementation with proper test coverage
- Follows existing patterns in the codebase
- Good separation between primary and secondary shortcuts
Minor note: The test name "search palette also opens with cmd+p" could be clearer as "search palette opens with secondary cmd+p shortcut"
✅ Approved - This improves discoverability and follows common conventions.
atharvau
left a comment
There was a problem hiding this comment.
Sensible Keybinding Update
Good change moving file open dialog to more intuitive Cmd+K while maintaining Cmd+P fallback. Includes proper E2E testing and smart display logic showing primary shortcut in UI.
Implementation is clean with good test coverage following existing codebase patterns.
Minor note: Test name could be clearer about being the secondary shortcut.
Approved - improves discoverability following common conventions.
(cherry picked from commit 0f5626d)
summary
Cmd+Kby making it the primary keybind for the command paletteCmd+Pworking as a secondary shortcut so existing muscle memory still workstesting