fix(app): move message navigation off cmd+arrow#18728
Conversation
|
@adamdotdevin is the new keybind fine? |
atharvau
left a comment
There was a problem hiding this comment.
Code Review Comments
✅ Smart UX Improvement
This change fixes a common keyboard navigation conflict on macOS.
🔍 Analysis
Problem: cmd+up/down conflicts with native macOS cursor movement to beginning/end of document in text inputs, breaking user expectations.
Solution: Move message navigation to cmd+opt+[ / cmd+opt+] which follows common IDE/editor conventions for block navigation.
✅ What's Great
-
Follows Platform Conventions:
- Preserves native macOS
cmd+up/downbehavior cmd+opt+[]is familiar pattern from Xcode, VS Code, etc.
- Preserves native macOS
-
Good Test Coverage: Added specific tests for bracket key matching
-
Non-Breaking: Only changes keyboard shortcuts, no functional changes
-
Clear Intent: The bracket symbols
[]visually suggest "previous/next" navigation
🔵 Minor Observations
-
Accessibility: Consider if the new shortcuts are discoverable - brackets require shift key on some keyboards
-
Cross-Platform: While this solves macOS conflict, verify the new shortcuts work well on Windows/Linux
-
Documentation: Users may need to learn the new shortcuts - consider release notes/help text
💡 Suggestions
-
Consider adding tooltip/help text mentioning the shortcut change for discoverability
-
Verify bracket key availability across different keyboard layouts (QWERTY vs AZERTY, etc.)
🎯 Verdict
APPROVE - This is a thoughtful fix that respects platform conventions while providing intuitive alternative shortcuts. The bracket notation is semantically appropriate for navigation.
The change improves UX for macOS users without breaking functionality. Test coverage is adequate for the keybind parsing logic.
atharvau
left a comment
There was a problem hiding this comment.
Code Review: Fix Message Navigation Shortcuts
Summary
This PR changes message navigation shortcuts from mod+arrow to mod+alt+[/mod+alt+] to avoid conflicts with common text editing operations like moving by word boundaries.
✅ Positives
- UX improvement: Prevents conflicts with standard text editor shortcuts (Cmd+Arrow for word navigation)
- Logical keybind choice: Using bracket keys [
[/]] is intuitive for previous/next navigation - Proper test coverage: Added comprehensive tests for bracket key matching in keybind system
- Consistent implementation: Updated both keybind definitions and corresponding tests
🔍 Analysis
Problem Solved
- Before:
mod+arrowup/mod+arrowdownconflicted with standard word-by-word navigation in text editors - After:
mod+alt+[/mod+alt+]provides dedicated navigation without conflicts
Technical Implementation
- Keybind changes: Modified
use-session-commands.tsxto use new key combinations - Test coverage: Added specific test case for bracket key matching in
command-keybind.test.ts - Cross-platform support: Uses
modprefix for proper Cmd/Ctrl handling across platforms
✅ Security & Performance
- No security implications
- No performance impact
- Backward compatibility: Users will need to learn new shortcuts, but old ones won't break functionality
✅ Code Style
- Follows existing keybind definition patterns
- Clean test implementation with proper assertions
- Consistent with codebase conventions
✅ Tests Pass
- Keybind tests pass successfully ✓
- New bracket key test validates the functionality
💭 Considerations
User Communication
This is a breaking change for existing users who have muscle memory for the old shortcuts. Consider:
- Adding a note in release notes/changelog
- Potentially showing a brief notification about the shortcut change
Alternative Considerations
The chosen shortcuts (mod+alt+[/mod+alt+]) are:
- ✅ Intuitive (brackets suggest previous/next)
- ✅ Unlikely to conflict with other shortcuts
- ✅ Easy to type on most keyboard layouts
Documentation
Consider updating any keyboard shortcut documentation or help text to reflect the new keybinds.
Verdict: LGTM ✅
This is a sensible UX improvement that resolves a real conflict issue while providing intuitive alternative shortcuts.
atharvau
left a comment
There was a problem hiding this comment.
Code Review - PR #18728
Summary
This PR changes message navigation keybinds from cmd+arrow to cmd+alt+bracket keys to avoid conflicts.
Positive Findings
- Conflict resolution: cmd+arrow keys are commonly used for text navigation in editors
- Logical alternative: Using brackets [ ] for prev/next is intuitive and matches some editors
- Added test coverage: New test specifically covers bracket key parsing and matching
- Consistent implementation: Both commands updated together
Code Quality
- Good test coverage: Comprehensive test for the new bracket key functionality
- Clear intent: The keybind change addresses a common conflict
UX Considerations
- Discoverability: cmd+alt+[ might be less discoverable than cmd+arrow
- Muscle memory: Users may need to retrain their shortcuts
- Alternative access: Users can still access via command palette
- Platform consistency: Some apps use similar bracket navigation (VS Code tabs)
Technical Analysis
- Proper parsing: The bracket keys are correctly handled in the keybind parser
- Event handling: Test shows proper KeyboardEvent matching for bracket characters
Potential Issues
- Breaking change: This changes existing user workflows
- Documentation: Should update any keyboard shortcut documentation
- Migration: Consider showing a tooltip about the change initially
Security & Performance
- No security concerns
- No performance impact
Recommendations
- Consider adding migration notice or tooltip for existing users
- Update documentation/help text about keyboard shortcuts
- Could consider making this configurable in user preferences
Recommendation: APPROVE
This is a reasonable fix for a keybind conflict. The bracket alternative is logical and well-tested. Consider user communication about the change.
atharvau
left a comment
There was a problem hiding this comment.
✅ APPROVED - Good keybind conflict resolution with proper testing.
Strengths:
- Resolves conflict between message navigation and cursor movement
- Uses common bracket navigation pattern (similar to IDE tab switching)
- Includes comprehensive test coverage for bracket key combinations
- Tests validate both
[and]key handling with modifiers - Clean implementation with proper event handling
Assessment:
mod+alt+[andmod+alt+]are intuitive for navigation- Avoids interfering with text editing workflows
- Test coverage demonstrates thorough validation
The keybind choice is well-reasoned and properly tested.
atharvau
left a comment
There was a problem hiding this comment.
✅ Smart Keybinding Conflict Resolution
This change resolves conflicts with common text editing shortcuts.
Benefits:
- Avoids conflicts: is standard for word/line navigation in text editors
- Intuitive replacement: follows bracket-based navigation patterns
- Proper testing: Added comprehensive test coverage for bracket key events
Implementation Quality:
- Clean keybinding parsing logic
- Good test coverage for edge cases (bracket characters)
- Maintains consistent navigation metaphors
Suggestion: Consider documenting these shortcuts in user-facing help/docs since they're less discoverable than arrow keys.
✅ Approved - Resolves real usability conflicts and uses sensible alternatives.
atharvau
left a comment
There was a problem hiding this comment.
Smart Keybinding Conflict Resolution
This resolves conflicts with standard text editing shortcuts. Moving from Cmd+Arrow to Cmd+Alt+bracket follows intuitive navigation patterns and includes comprehensive test coverage.
Implementation quality is good with clean keybinding parsing and proper edge case testing.
Suggestion: Document these shortcuts in user-facing help since brackets are less discoverable than arrows.
Approved - resolves real usability conflicts with sensible alternatives.
atharvau
left a comment
There was a problem hiding this comment.
Code Review Summary
✅ Overall Assessment: APPROVED - This PR successfully moves message navigation off Cmd+Arrow keys to avoid conflicts.
🟢 Strengths:
- Conflict resolution: New keybinds ( and ) avoid conflicts with standard navigation.
- Test coverage: Comprehensive test cases validate bracket key handling.
- Semantic keybinds: Bracket keys are intuitive for previous/next navigation.
- Cross-platform: key ensures compatibility across macOS and Windows.
📝 Technical Implementation:
- Proper keybind parsing for bracket characters
- Test coverage includes platform-specific modifiers (ctrl/meta)
- Clean migration from arrow-based to bracket-based navigation
🟡 UX Consideration:
Users will need to learn new keybinds, but the bracket keys are more semantically appropriate for message navigation than arrow keys.
No technical issues found. The implementation is clean and well-tested.
atharvau
left a comment
There was a problem hiding this comment.
Code Review Summary
Overall Assessment: APPROVED - This PR successfully moves message navigation off Cmd+Arrow keys to avoid conflicts.
Strengths:
- Conflict resolution: New keybinds avoid conflicts with standard navigation.
- Test coverage: Comprehensive test cases validate bracket key handling.
- Semantic keybinds: Bracket keys are intuitive for previous/next navigation.
- Cross-platform: mod key ensures compatibility across macOS and Windows.
Technical Implementation:
- Proper keybind parsing for bracket characters
- Test coverage includes platform-specific modifiers
- Clean migration from arrow-based to bracket-based navigation
UX Consideration:
Users will need to learn new keybinds, but the bracket keys are more semantically appropriate for message navigation than arrow keys.
No technical issues found. The implementation is clean and well-tested.
atharvau
left a comment
There was a problem hiding this comment.
Code Review - PR #18728: Move message navigation off cmd+arrow
✅ LOOKS GOOD - Smart keybind change
Review Summary
This PR changes message navigation from cmd+arrow to cmd+alt+[ and cmd+alt+], resolving conflicts with standard text editing shortcuts.
✅ Strengths
- UX Improvement: Avoids conflicts with standard cursor movement keybinds
- Bracket Logic: Uses bracket keys which semantically represent prev/next navigation
- Test Coverage: Includes comprehensive test for bracket key matching
- Consistent: Updates both command definitions and tests
Technical Quality
- ✅ Test covers both bracket keys with proper modifier key handling
- ✅
parseKeybindandmatchKeybindfunctions handle bracket keys correctly - ✅ No breaking changes to non-conflicting functionality
No Issues Found
- ✅ No bugs, security, or performance concerns
- ✅ Test coverage is appropriate and thorough
Recommendation: Ready to merge! This resolves a real keybind conflict issue.
atharvau
left a comment
There was a problem hiding this comment.
Code Review: fix(app): move message navigation off cmd+arrow
✅ APPROVED
This is a well-designed UX improvement addressing a common keybind conflict.
Problem: cmd+arrowup/down conflicts with system navigation shortcuts (text cursor movement, window switching, etc.)
Solution: Changed to cmd+alt+[/] which are:
- Less likely to conflict with system shortcuts
- Semantically appropriate (brackets suggest navigation boundaries)
- Follow macOS conventions for secondary navigation
Quality Analysis:
- ✅ Added comprehensive tests for bracket key parsing
- ✅ Updated both command definitions and keybind parsing
- ✅ No functional logic changes, only keybind remapping
- ✅ Maintains existing
disabledstate logic correctly - ✅ Clear test coverage for the new bracket keys
Technical Implementation:
- Keybind parsing already supported bracket characters
- Tests verify
ctrl/metakey detection works properly - Format functions handle the new keys appropriately
This change improves the user experience without introducing any risks. The bracket keys are intuitive for navigation and much less likely to cause conflicts. Ready to merge! ⌨️
atharvau
left a comment
There was a problem hiding this comment.
Code Review: fix(app): move message navigation off cmd+arrow
✅ APPROVED - Good UX Improvement
This PR addresses a keyboard shortcut conflict by moving message navigation from to , preventing interference with standard text editing shortcuts.
🎯 Problem Solved
- Conflict Resolution: is commonly used for word-by-word navigation in text editors
- Better UX: New shortcuts don't interfere with text editing workflows
- Intuitive: Bracket keys () naturally suggest previous/next navigation
✅ Implementation Quality
1. Consistent Changes
- Updates both keybind definitions and test cases
- Uses standard pattern (cross-platform compatible)
2. Proper Testing
- Adds comprehensive test coverage for bracket key navigation
- Tests both and keys with proper modifier handling
- Uses dynamic / key detection for cross-platform support
3. Clean Code
- Maintains existing command structure
- No functional changes beyond keybind reassignment
📋 Technical Analysis
Keybind Changes:
Test Coverage:
- Validates bracket key detection
- Confirms modifier key combinations work correctly
- Ensures cross-platform compatibility
✅ Quality Checks
- No Breaking Changes: Only changes keyboard shortcuts, not functionality
- No Security Issues: UI-only change with no security implications
- No Performance Impact: Minimal change to event handling
- Accessibility: New shortcuts are still easily discoverable via command palette
💡 Minor Suggestion
Consider updating any user documentation that references the old shortcuts to mention the new shortcuts.
This is a thoughtful UX improvement that solves a real workflow conflict. Ready to merge! 🎯
summary
cmd+up/cmd+downtocmd+opt+[/cmd+opt+]