fix(input): leave navigate mode on ctrl+[ - #2340
Conversation
kangal-bot
added
the
ai-review
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughNavigate mode now treats unbound Suggested reviewers: Merge Risk: ⚪ Minimal · up to Ctrl+[ now exits Navigate mode while preserving existing bindings and Ctrl+Shift+[ behavior. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 1 files. (1 skipped: 1 unsupported.)
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 makes Ctrl+[ leave Navigate mode when reported as a distinct Kitty keyboard-protocol key, while preserving configured binding precedence.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/app/input/navigate.rs | Adds the narrowly scoped Ctrl+[ Navigate-mode fallback after binding dispatch and tests the intended precedence and modifier behavior. |
| docs/next/CHANGELOG.md | Records the Navigate-mode Ctrl+[ compatibility fix and configured-binding precedence. |
Reviews (6): Last reviewed commit: "docs: note ctrl+[ navigate mode fix in c..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/input/navigate.rs (1)
1312-1312: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover both navigation handlers.
The new tests call
App::handle_navigate_keyonly. Line 1312 changes the separatehandle_navigate_key(&mut AppState, KeyEvent)handler. Add equivalentCtrl+[andCtrl+Shift+[assertions through that handler so the two paths cannot diverge.Suggested additional coverage
+ #[test] + fn state_navigate_mode_cancel_keys_match_app_handler() { + let mut state = state_with_workspaces(&["one", "two"]); + state.mode = Mode::Navigate; + handle_navigate_key( + &mut state, + KeyEvent::new(KeyCode::Char('['), KeyModifiers::CONTROL), + ); + assert_eq!(state.mode, Mode::Terminal); + + state.mode = Mode::Navigate; + handle_navigate_key( + &mut state, + KeyEvent::new( + KeyCode::Char('['), + KeyModifiers::CONTROL | KeyModifiers::SHIFT, + ), + ); + assert_eq!(state.mode, Mode::Navigate); + }Also applies to: 2907-2932
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ac4deea-63fe-4cd6-bc83-4e32e490b329
📒 Files selected for processing (1)
src/app/input/navigate.rs
ogulcancelik
left a comment
ogulcancelik
left a comment
There was a problem hiding this comment.
requesting changes because this currently overrides valid user configuration.
ctrl+[ is checked before navigate-mode keybinding dispatch, so any existing ctrl+[ binding is ignored while navigate mode is open. please treat it as a fallback cancel key only when no configured binding matched.
please cover both paths: unbound ctrl+[ cancels navigate mode, and a configured ctrl+[ binding still runs.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/input/navigate.rs (1)
1317-1337: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the state-only navigation handler.
handle_navigate_key(&mut AppState, KeyEvent)has a separate conversion and dispatch path. The new precedence test at Lines [2931-2943] covers onlyApp::handle_navigate_key(TerminalKey). Add equivalent tests through theKeyEvententry point for unboundCtrl+[, a configuredCtrl+[binding, andCtrl+Shift+[. This prevents the two handlers from diverging.As per coding guidelines, keep unit tests next to the Rust code in
#[cfg(test)] mod tests.Also applies to: 2931-2943
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 75966db5-3886-40ce-8eb5-694f825749b0
📒 Files selected for processing (1)
src/app/input/navigate.rs
|
Good catch,
Both paths are covered:
The second test fails if the check is moved back above dispatch, so it pins the ordering rather than just passing. |
|
@qaz74107410 the revised ordering now preserves configured ctrl+[ bindings, so my original blocker is resolved. please rebase onto current master, add the user-facing entry to |
7251638 to
deaf073
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. |
|
Thanks for the review. Rebased onto current master and added the changelog entry under Unreleased / Fixed in The rebase only touched Checks and both review bots have rerun on the new head and are green. Please let me know if anything else would help. |
|
first time using herdr and I noticed this issue right away (using alacritty - herdr @qaz74107410 could you rebase once more? @ogulcancelik the PR is ready, ci and bots are green. may we can get this to next release |
deaf073 to
c44de1e
Compare
|
Rebased onto current master. Only the changelog conflicted this time, the fix itself replayed unchanged, and checks are rerunning on the new head. Thanks for testing it and for the report. |
4 participants
Ctrl+[ is the terminal-level equivalent of Esc, but navigate mode only checked
KeyCode::Esc, so the mode stayed open.On a legacy terminal Ctrl+[ sends 0x1b and already arrives as
KeyCode::Esc, which is why this works for some people. Under the kitty keyboard protocol, which Herdr negotiates, the modified key is reported on its own and reaches the handler asChar('[')with CONTROL, so the Esc check missed it. The report came from foot, which speaks that protocol.Adds
is_navigate_cancel_keyand uses it where navigate mode tested for Esc. Ctrl+Shift+[ keeps its own meaning because the match requires CONTROL alone.Prefix mode needs no matching change. It already falls through to
leave_command_modefor any unbound key, so Ctrl+[ cancels it today, and adding the same check there would shadow a custom Ctrl+[ binding under Prefix dispatch.No doc change looks needed: the keyboard docs do not currently mention Esc for navigate mode. Happy to add one if you want the alias documented.
Checks on 1.96.1:
cargo fmt --checkandcargo clippy --all-targets --locked -- -D warningsare clean.cargo nextest run --lockedis 3179/3181; the two failures,live_handoff::live_server_holds_one_pty_master_fd_per_paneandterminal::state::metadata::tests::metadata_clear_only_without_ttl_does_not_extend_old_ttl, fail the same way on unmodified master in my environment. I ran this in a Linux container because zig 0.15.2 cannot build the vendored libghostty-vt on macOS 26, sojust windows-lintis untested here; the change is not platform-gated.refs #1431