Repository navigation
fix: preserve shift on ctrl+shift+letter for legacy panes - #4597
Conversation
Panes whose app has not enabled the kitty keyboard protocol received ctrl+shift+letter as the same C0 byte as ctrl+letter. Encode the chord as CSI-u instead, matching Ghostty's legacy encoder. refs herdrdev#4581
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughLegacy encoding now uses CSI-u for non-release Super-modified character keys and ASCII letters with Control and Shift. Other Control+Shift characters, including punctuation, retain legacy encoding. Tests cover lowercase and uppercase letters, Alt-modified letters, Kitty-parsed host input, punctuation, and a terminal test input change. Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The intended legacy key encoding appears to reach pane input, with no actionable merge-blocking issue identified. Merge after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| && key.kind != crossterm::event::KeyEventKind::Release | ||
| && matches!(key.code, KeyCode::Char(_)) | ||
| && key.modifiers.contains(KeyModifiers::SUPER) | ||
| && legacy_chord_needs_csi_u(&key) |
There was a problem hiding this comment.
Legacy. This branch sends Ctrl+Shift+letter as CSI-u, but an app using the negotiated mode expects a CSI 27;...~ sequence and can miss the chord. Ghostty’s level-2 encoder, for example, sends CSI 27;6;72~ for Ctrl+Shift+H. This path needs to respect the pane’s modifyOtherKeys mode.
There was a problem hiding this comment.
fair point, but this isn't a regression. on master those panes get a plain 03 for ctrl+shift+c. now they get CSI 99;6u, which modifyOtherKeys readers like vim and neovim also accept. herdr's encoder already ignores modifyOtherKeys for all letter keys, plain ctrl+letter included, so that's a separate follow-up.
There was a problem hiding this comment.
That’s fair. Since encode_terminal_key already treats all character chords—including plain Ctrl+letters—as legacy/CSI-u behavior without consulting modifyOtherKeys, this change does not introduce a new modifyOtherKeys-specific regression; it only changes Ctrl+Shift+ASCII letters from the old collapsed C0 byte to CSI-u. If vim/neovim’s readers accept that sequence, the concern I raised is a separate consistency/follow-up issue rather than a blocker for this PR. I’m withdrawing the finding.
Merges 46 upstream commits spanning stable releases v0.9.2 and v0.9.3: multiple prefix keys (herdrdev#4653), self-reported agent resume commands (herdrdev#4687), Go To workspace-grouped navigator rows (herdrdev#4384), clear_pane keybinding (herdrdev#4383), non-interactive machine status/reconnect with SSH MFA (herdrdev#3763), interactive machine add session picker (herdrdev#4204, herdrdev#4678), mouse events on terminal session control (herdrdev#4685), staggered agent restarts (herdrdev#4102), SSH compression (herdrdev#4340), row-shift scrolling (herdrdev#4711), TERM_PROGRAM identity (herdrdev#4104), PowerShell 7 default on Windows (herdrdev#4297), tab-close confirmation (herdrdev#4379, herdrdev#4409), full event subscription bursts (herdrdev#4178, herdrdev#4225), SIGWINCH color refresh (herdrdev#4349), saved-layout shutdown survival (herdrdev#4320), stray-Escape click fix (herdrdev#3480), first-task-done detection (herdrdev#3338, herdrdev#3990, herdrdev#3916), CPU/perf work across navigator/splits/named-targets/event-batching (herdrdev#4506, herdrdev#4546, herdrdev#4426, herdrdev#4669, herdrdev#4670, herdrdev#4671, herdrdev#4672), synchronized-frame tearing and cursor flicker fixes (herdrdev#2968, herdrdev#4303), API socket transient-error resilience (herdrdev#4601), async worktree git lookups (herdrdev#4492), worktree-open workspace-takeover fix (herdrdev#4293), SSH agent forwarding after reconnect (herdrdev#1931, herdrdev#4252), OpenCode V1 subagent status (herdrdev#1362, herdrdev#4240), Codex idle/mention/interrupt-key detection (herdrdev#4507, herdrdev#4196), Kiro/Grok detection fixes (herdrdev#4203, herdrdev#4372, herdrdev#4333), Droid scrollback clears (herdrdev#4432), copy-mode-stays-active fix (herdrdev#3812, herdrdev#4281), Ctrl+Shift+letter fix (herdrdev#4581, herdrdev#4597), split mouse-report fix (herdrdev#4630), several Windows paste/mouse-capture/clipboard/detection fixes (herdrdev#4251, herdrdev#4284, herdrdev#4470, herdrdev#4314, herdrdev#4313, herdrdev#4579, herdrdev#4309), navigator search/highlight fixes (herdrdev#4300, herdrdev#4408, herdrdev#4273, herdrdev#4535, herdrdev#4355), session-attach-without-terminal error (herdrdev#4393), socket error request-ID preservation (herdrdev#4344), plugin install option ordering (herdrdev#4446), Windows host color detection (herdrdev#1530, herdrdev#4369), post-update saved-machine summary (herdrdev#4722), a hotfix for Escape-prefixed terminal key bindings (herdrdev#4759) and pinned Windows ConPTY bundling (herdrdev#4760), Windows same-user cross-elevation attach (herdrdev#4777), and the libghostty-vt binding extraction into a ghostty-vt workspace crate (herdrdev#4661). Also fixes two merge seam breaks: overlays.rs combines the fork's rounded_borders threading with upstream's per-client prefix-label onboarding overlay, and the fork's command_palette test file is updated for the new Vec<KeyCombo> prefix representation and the BindingConfig::one constructor introduced by the multi-prefix-key feature. refs upstream herdrdev/herdr@331775c3
) Panes whose app has not enabled the kitty keyboard protocol received ctrl+shift+letter as the same C0 byte as ctrl+letter. Encode the chord as CSI-u instead, matching Ghostty's legacy encoder. refs herdrdev#4581 Co-authored-by: ain3sh <ainesh.chatterjee@gmail.com>
Description
What
Ctrl+Shift+letter now reaches pane apps with Shift intact when the app has not enabled the kitty keyboard protocol. Herdr sends
CSI 99;6ufor Ctrl+Shift+C instead of0x03, which is the same bytes bare Ghostty sends. Verified live: stable 0.9.1 delivers03 03for Ctrl+Shift+C then Ctrl+C; this branch delivers1b 5b 39 39 3b 36 75 03.Why
Every
Charkey goes through herdr's own encoder (ghostty_prefers_herdr_text_encoding). Its legacy path turned any Ctrl+letter into a C0 byte and dropped Shift, so apps could not tell Ctrl+Shift+C from Ctrl+C and treated it as an interrupt. Droid 0.226.2 is one such app: in a herdr pane it never pushes kitty flags or modifyOtherKeys, so it always gets legacy encoding.How
In legacy mode, Ctrl+Shift+ASCII letter (with or without Alt) now goes through the same CSI-u branch that already preserves Super chords. This mirrors libghostty-vt's legacy encoder (
ctrlSeqdeclines Ctrl+Shift+letter, and the fixterms branch emits CSI-u). Plain Ctrl+letter and Ctrl+Shift+punctuation keep their C0 bytes.Repro Recipe
\x1b[>7uwrite removed).herdr pane send-keys <pane> ctrl+shift+c ctrl+cGOT: 03 03. After:GOT: 1b 5b 39 39 3b 36 75 03.Related Issue
refs #4581
Reviewer Guide
Review depth: Standard. One predicate in the encoder plus tests.
Read order:
src/input/encode.rs:legacy_chord_needs_csi_uand its call site inencode_terminal_key.src/input/encode.rstests: Ctrl+Shift+C (either case), Ctrl+Alt+Shift+C, the kitty host sequence from the issue, and Ctrl+Shift+_keeping0x1f.src/pane/terminal.rs:ghostty_char_keys_still_use_herdr_encodingnow uses Ctrl+A. Ctrl+Shift+A no longer tells herdr's encoder and Ghostty's encoder apart, because both now emitCSI 97;6u. Ctrl+A still does (0x01vsCSI 97;5u).Risk & Impact
A legacy-mode app that relied on Ctrl+Shift+letter arriving as the plain Ctrl+letter byte will now receive a CSI-u sequence instead. Bare Ghostty already behaves this way, so apps that work in Ghostty outside herdr are unaffected. Kitty-mode panes, non-letter keys, and the wire protocol are unchanged. Reverting the single commit restores the old behavior.
Verification
Behavior verified @
0e8c5926: Disposable herdr sessions on stable 0.9.1 and on this branch, byte probe in a non-kitty pane,send-keys ctrl+shift+c ctrl+c:03 03before,CSI 99;6u+03after.Regression coverage:
input::encode::tests::legacy_ctrl_shift_letter_*fail on master with[3]and pass here.Not tested: A physical Ghostty keypress into Droid. The probe drives the same
encode_terminal_keypath through the API.Standard validators:
just cipasses (fmt, clippy-D warnings, 3802 nextest tests, maintenance tests).