fix(input): allow ctrl+n & ctrl+p keybinds in session navigator modal - #2271
fix(input): allow ctrl+n & ctrl+p keybinds in session navigator modal#2271jplew wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change adds configurable session navigator up and down bindings with ChangesSession navigator bindings
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This localized change adds configurable navigator movement bindings while preserving existing behavior, and no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Configuration
participant NavigatorInput
participant KeybindHelp
Configuration->>NavigatorInput: provide navigator up/down bindings
NavigatorInput->>NavigatorInput: match configured input and move selection
Configuration->>KeybindHelp: provide effective bindings
KeybindHelp-->>KeybindHelp: display session navigator selection shortcuts
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 6 files. (2 skipped: 2 unsupported.) ✨ 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 |
Greptile SummaryThe PR adds independently configurable Session Navigator movement bindings, defaults them to Ctrl+P/Ctrl+N, and exposes them through runtime dispatch, help, generated configuration, and documentation.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported printable-binding issue is rejected by current validation, and navigator text commits cannot be preempted by keybinding matching.
|
| Filename | Overview |
|---|---|
| src/app/input/modal.rs | Dispatches configured navigator movement in both normal and search-focused views while retaining built-in movement and search commands. |
| src/config/keybinds.rs | Adds an independent navigator binding registry and rejects character bindings capable of intercepting searchable text. |
| src/config/model.rs | Adds the two serialized keybinding fields, overlay handling, effective-value copying, and Ctrl+P/Ctrl+N defaults. |
| src/ui/keybind_help.rs | Displays the resolved Session Navigator movement bindings in the navigation help group. |
| docs/next/website/src/data/config-reference.json | Documents both new configuration keys with defaults consistent with the runtime model. |
Reviews (3): Last reviewed commit: "docs(changelog): add session navigator m..." | Re-trigger Greptile
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/input/modal.rs (1)
1897-1931: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest a non-default navigator binding.
Both tests use only the default bindings. A hardcoded Ctrl+N/Ctrl+P implementation would pass them.
src/app/input/modal.rs#L1897-L1931: setstate.keybinds.navigator.upandstate.keybinds.navigator.downto non-default direct bindings. Test both normal and search-focused navigator states.src/ui.rs#L1440-L1473: set the same non-default bindings. Assert that keybinding help displays their labels.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c4d677b9-0d6c-46c8-a8c3-acec20449f24
📒 Files selected for processing (7)
docs/next/website/src/data/config-reference.jsonsrc/app/input/modal.rssrc/config/keybinds.rssrc/config/model.rssrc/main.rssrc/ui.rssrc/ui/keybind_help.rs
7fb7692 to
9fffd06
Compare
|
Addressed the automated review feedback in 9fffd06: printable search-input bindings are now rejected, and tests exercise non-default |
|
@jplew the implementation review is clean. please rebase onto current master, add the user-facing entry to |
9fffd06 to
dd17fbb
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rebased onto current master and added the user-facing entry under Unreleased → Added in Validation on the new head before pushing: CI and both bot reviews are re-running on dd17fbb; will confirm once they're complete. |
Summary
It pains me that I can't rebind up/down motion in the Navigator modal (prefix+g) to Vim/Emacs-style
ctrl+nandctrl+p.This PR fixes Session Navigator movement so
ctrl+n/ctrl+pare configurable and work both before and after focusing search. The default bindings preserve the existing search-focused behavior while extending it to the normal navigator view.keys.navigate_navigator_upandkeys.navigate_navigator_downj/kand arrow-key movement plus navigator commandsVerification
ZIG=/home/ubuntu/Sites/.tools/zig-x86_64-linux-0.15.2/zig just checkDocs
Updated the unreleased config reference and generated default-config comments for the new bindings.