refactor(input): make pane key forwarding protocol-complete - #2578
refactor(input): make pane key forwarding protocol-complete#2578ogulcancelik wants to merge 19 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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR unifies keyboard parsing and forwarding around state-aware Kitty and ChangesUnified keyboard forwarding
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ClientInput
participant RawInputFramer
participant ClientKeySource
participant PaneTerminal
participant GhosttyEncoder
ClientInput->>RawInputFramer: frame and decode keyboard bytes
RawInputFramer->>ClientKeySource: preserve VT metadata and text-commit state
ClientKeySource->>PaneTerminal: restore terminal key state
PaneTerminal->>GhosttyEncoder: encode using live protocol state
GhosttyEncoder-->>PaneTerminal: return encoded, suppressed, or unavailable result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
kangal-bot
added
the
ai-review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
src/input/parse.rs (1)
1030-1036: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing the shared fixture parsers for the variant corpora.
This test now uses
crate::input::test_support. The local helpersdecode_hex,parse_fixture_key_code,parse_fixture_modifiers, andparse_fixture_kindremain in this file and duplicate the shared versions. Only the column layout differs. Move the column-count handling intotest_supportand delete the local copies to keep one parser per concept.src/protocol/wire.rs (1)
313-344: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving the commit flag once instead of per branch.
All three branches apply the same
with_text_commit()decision with a different predicate. Compute the predicate first, then apply the source and the commit flag once. This also makes the asymmetry explicit: theVtbranch trusts the transmittedtext_commit, while the other two infer it fromgenerated_text.Note one coupling to keep in mind:
with_text_commit()recomputesgenerated_textfromcoderather than keeping the transmitted text. No current producer sends a multi-codepoint commit withtext_commit: true, so nothing breaks today. If that changes, the receiver would silently truncate the text.vendor/libghostty-vt/src/input/key_mods.zig (1)
114-115: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a bit-position test for
hyperandmeta.
binding()now propagates the two new flags. The existing self-check test only pins bit 0 (shift). Add assertions for the new bit positions so a future field reorder cannot silently shift the ABI that the C header mirrors.Run the targeted test with
zig build test-lib-vt -Dtest-filter=<filter>.As per coding guidelines: "For libghostty-vt changes, prefer
zig build test-lib-vt -Dtest-filter=<filter>for targeted tests."♻️ Proposed test addition
test { const testing = std.testing; try testing.expectEqual(`@as`(Backing, `@bitCast`(Mods{})), `@as`(Backing, 0b0)); try testing.expectEqual( `@as`(Backing, `@bitCast`(Mods{ .shift = true })), `@as`(Backing, 0b0000_0001), ); + try testing.expectEqual( + `@as`(Backing, `@bitCast`(Mods{ .hyper = true })), + `@as`(Backing, 0b0000_0100_0000_0000), + ); + try testing.expectEqual( + `@as`(Backing, `@bitCast`(Mods{ .meta = true })), + `@as`(Backing, 0b0000_1000_0000_0000), + ); }Source: Coding guidelines
vendor/libghostty-vt/src/input/function_keys.zig (1)
310-326: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSet an explicit eval branch quota in
pcStyleWithImplicitMods.
pcStylecalls@setEvalBranchQuota(500_000)inside its owncomptimeblock.pcStyleWithImplicitModsdoes not. The helper runs 13 times during thekeyscomptime block and relies on the quota thatpcStylehappens to raise first. Set the quota explicitly so the helper does not depend on call ordering.♻️ Proposed fix
fn pcStyleWithImplicitMods(comptime fmt: []const u8, comptime implicit: key.Mods) []Entry { comptime { + `@setEvalBranchQuota`(500_000); var entries: [modifiers.len]Entry = undefined;src/pane/input.rs (1)
77-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the consumed-Shift predicate for readability.
ghostty_consumed_modspacks four disjunctive conditions into amatches!guard nested inside a boolean expression. The rule is hard to read and hard to extend. The behavior is correct; only the shape is a concern.Split the character-specific decision into a named helper.
♻️ Proposed refactor
fn ghostty_consumed_mods(key: &crate::input::TerminalKey) -> u16 { - let only_shift = key.modifiers == crossterm::event::KeyModifiers::SHIFT; - let has_generated_text = key - .generated_text - .as_ref() - .is_some_and(|text| !text.is_empty()); - let shift_generated_text = key - .modifiers - .contains(crossterm::event::KeyModifiers::SHIFT) - && matches!(key.code, crossterm::event::KeyCode::Char(c) if - has_generated_text - || key.shifted_codepoint.and_then(char::from_u32).is_some() - || c.is_ascii_uppercase() - || (only_shift - && (c.is_ascii_alphabetic() - || ghostty_unshifted_ascii_pair(c).is_some()))); - if shift_generated_text { + if !key + .modifiers + .contains(crossterm::event::KeyModifiers::SHIFT) + { + return 0; + } + let crossterm::event::KeyCode::Char(c) = key.code else { + return 0; + }; + if shift_produced_the_character(key, c) { crate::ghostty::MOD_SHIFT } else { 0 } } + +/// True when Shift was used to produce the character rather than acting as a +/// separate binding modifier. +fn shift_produced_the_character(key: &crate::input::TerminalKey, c: char) -> bool { + let has_generated_text = key + .generated_text + .as_ref() + .is_some_and(|text| !text.is_empty()); + let only_shift = key.modifiers == crossterm::event::KeyModifiers::SHIFT; + has_generated_text + || key.shifted_codepoint.and_then(char::from_u32).is_some() + || c.is_ascii_uppercase() + || (only_shift && (c.is_ascii_alphabetic() || ghostty_unshifted_ascii_pair(c).is_some())) +}src/pane/terminal.rs (1)
2186-2207: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe one-shot latches hide recurring encoder failures.
EVENT_ALLOCATION_LOGGED,ENCODER_LOCK_LOGGED, andENCODER_ERROR_LOGGEDare process-wideAtomicBoolvalues that are never reset. After the first failure of each category, every later failure is silent for the lifetime of the process. A pane whose encoder lock is poisoned drops every key with no further signal.Keep the noise suppression but preserve visibility. Count the suppressed failures and report the total periodically, or emit a metric alongside the first log.
♻️ Proposed refactor
fn log_key_encoding_unavailable(reason: KeyEncodingUnavailable) { - use std::sync::atomic::{AtomicBool, Ordering}; + use std::sync::atomic::{AtomicU64, Ordering}; - static EVENT_ALLOCATION_LOGGED: AtomicBool = AtomicBool::new(false); - static ENCODER_LOCK_LOGGED: AtomicBool = AtomicBool::new(false); - static ENCODER_ERROR_LOGGED: AtomicBool = AtomicBool::new(false); + // Log the first failure, then every 1024th, so recurring faults stay visible. + const LOG_INTERVAL: u64 = 1024; + static EVENT_ALLOCATION_FAILURES: AtomicU64 = AtomicU64::new(0); + static ENCODER_LOCK_FAILURES: AtomicU64 = AtomicU64::new(0); + static ENCODER_ERROR_FAILURES: AtomicU64 = AtomicU64::new(0); - let first_failure = match reason { + let failures = match reason { KeyEncodingUnavailable::Adapter(GhosttyKeyEventAdapterError::UnsupportedKey) => { debug!(?reason, "Ghostty key encoding unavailable; suppressing key"); return; } KeyEncodingUnavailable::Adapter(GhosttyKeyEventAdapterError::EventAllocation) => { - &EVENT_ALLOCATION_LOGGED + &EVENT_ALLOCATION_FAILURES } - KeyEncodingUnavailable::EncoderLockPoisoned => &ENCODER_LOCK_LOGGED, - KeyEncodingUnavailable::EncoderError => &ENCODER_ERROR_LOGGED, + KeyEncodingUnavailable::EncoderLockPoisoned => &ENCODER_LOCK_FAILURES, + KeyEncodingUnavailable::EncoderError => &ENCODER_ERROR_FAILURES, }; - if !first_failure.swap(true, Ordering::Relaxed) { - error!(?reason, "Ghostty key encoding failed; suppressing key"); + let count = failures.fetch_add(1, Ordering::Relaxed) + 1; + if count == 1 || count.is_multiple_of(LOG_INTERVAL) { + error!( + ?reason, + count, "Ghostty key encoding failed; suppressing key" + ); } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cc10e144-35ef-4225-89d5-33bd66710878
⛔ Files ignored due to path filters (1)
tests/fixtures/keyboard_protocol_corpus.tsvis excluded by!**/*.tsv
📒 Files selected for processing (37)
docs/next/CHANGELOG.mdsrc/app/api/panes.rssrc/app/input/mouse.rssrc/app/mod.rssrc/client/input.rssrc/client/input/windows_vti.rssrc/ghostty/bindings.rssrc/ghostty/mod.rssrc/input/encode.rssrc/input/mod.rssrc/input/model.rssrc/input/parse.rssrc/input/test_support.rssrc/pane.rssrc/pane/input.rssrc/pane/kitty_keyboard.rssrc/pane/terminal.rssrc/protocol/wire.rssrc/raw_input.rssrc/server/client_transport.rssrc/server/headless.rstests/live_handoff.rsvendor/libghostty-vt.patches.mdvendor/libghostty-vt/include/ghostty/vt/key/event.hvendor/libghostty-vt/src/input/function_keys.zigvendor/libghostty-vt/src/input/key.zigvendor/libghostty-vt/src/input/key_encode.zigvendor/libghostty-vt/src/input/key_mods.zigvendor/libghostty-vt/src/lib_vt.zigvendor/libghostty-vt/src/terminal/c/key_event.zigvendor/libghostty-vt/src/terminal/c/main.zigvendor/patches/libghostty-vt/0002-proxied-kitty-key-metadata.patchvendor/patches/libghostty-vt/0003-report-kitty-repeat-events.patchvendor/patches/libghostty-vt/0004-encode-extended-function-keys.patchvendor/patches/libghostty-vt/0005-preserve-legacy-ctrl-tab.patchvendor/patches/libghostty-vt/0006-honor-consumed-shift-in-legacy-control-keys.patchvendor/patches/libghostty-vt/0007-preserve-proxy-key-compatibility.patch
Greptile SummaryThe PR replaces duplicate pane-key encoding with a state-synchronized Ghostty encoder and expands typed input metadata, incremental framing, keyboard lifecycle handling, and handoff state preservation.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains in the eligible follow-up review scope. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/raw_input.rs | Reworks raw input decoding into an incremental state machine covering fragmented UTF-8, CSI, control strings, malformed input, and timeout recovery. |
| src/pane/input.rs | Adapts normalized TerminalKey values and source metadata into semantic Ghostty key events. |
| src/pane/terminal.rs | Makes the pane’s state-synchronized Ghostty encoder authoritative and restores exact keyboard state during handoff. |
| src/protocol/wire.rs | Extends structured VT key-source metadata and updates protocol-version and frozen-wire coverage. |
| src/ghostty/mod.rs | Extends the Ghostty bindings wrapper with retained UTF-8 storage, alternate codepoints, consumed modifiers, and proxy-event encoding. |
| src/pane/kitty_keyboard.rs | Tracks exact modifyOtherKeys modes alongside Kitty keyboard protocol state. |
| vendor/libghostty-vt/src/input/key_encode.zig | Adds proxy-event behavior and compatibility handling for lifecycle, modifiers, Alt text, and extended function keys. |
| tests/fixtures/keyboard_protocol_corpus.tsv | Expands end-to-end compatibility cases across framing, transport, pane state, and final PTY bytes. |
Sequence Diagram
sequenceDiagram
participant T as Host terminal
participant F as Incremental input framer
participant W as Typed wire protocol
participant P as Pane input adapter
participant G as State-synchronized Ghostty encoder
participant C as PTY child
T->>F: Raw keyboard bytes/events
F->>W: Typed key + lifecycle/source metadata
W->>P: Reconstructed TerminalKey
P->>G: Semantic proxy key event
G->>G: Apply pane Kitty/modifyOtherKeys state
G->>C: Encoded terminal bytes
Reviews (3): Last reviewed commit: "fix(input): encode no-text proxy alt eve..." | Re-trigger Greptile
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vendor/libghostty-vt/src/input/key_encode.zig (1)
426-431: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winComplete proxy Alt handling for events without generated text.
Proxy events must not use host-specific macOS Option behavior. The current no-text path still does so, and the vendor patch does not correct it.
vendor/libghostty-vt/src/input/key_encode.zig#L426-L431: add a proxy-specific ESC-prefix path for ASCIIunshifted_codepointvalues before callinglegacyAltPrefix.vendor/patches/libghostty-vt/0005-proxy-key-encoding.patch#L83-L93: add the matching no-text-path hunk and regression test so patch replay preserves the correction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bf1a040c-420f-4325-8a83-51dcbf5b053b
⛔ Files ignored due to path filters (1)
tests/fixtures/keyboard_protocol_corpus.tsvis excluded by!**/*.tsv
📒 Files selected for processing (18)
scripts/test_vendor_libghostty_vt.pyscripts/vendor_libghostty_vt.pysrc/ghostty/bindings.rssrc/ghostty/mod.rssrc/input/model.rssrc/pane/input.rssrc/pane/terminal.rsvendor/libghostty-vt.patches.mdvendor/libghostty-vt/include/ghostty/vt/key/encoder.hvendor/libghostty-vt/include/ghostty/vt/key/event.hvendor/libghostty-vt/src/input/function_keys.zigvendor/libghostty-vt/src/input/key.zigvendor/libghostty-vt/src/input/key_encode.zigvendor/libghostty-vt/src/input/kitty.zigvendor/libghostty-vt/src/terminal/c/key_encode.zigvendor/patches/libghostty-vt/0005-proxy-key-encoding.patchvendor/patches/libghostty-vt/0006-extended-function-keys-f35.patchvendor/patches/libghostty-vt/series
🚧 Files skipped from review as they are similar to previous changes (3)
- src/ghostty/mod.rs
- src/input/model.rs
- src/pane/terminal.rs
|
Addressed CodeRabbit’s no-text proxy Alt finding in 0690d0a. Proxy mode now prefixes ASCII unshifted codepoints before any host-specific macOS Option policy, with a vendored Zig regression test and refreshed replayable patch. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Summary
modifyOtherKeysacross handoff and close legacy compatibility gaps for Alt UTF-8, Ctrl aliases, Ctrl-Tab, and extended function keysRefs #2514
Validation
just check— 3312 Rust tests passed, Windows-target clippy passed, maintenance and integration suites passedcd vendor/libghostty-vt && zig build test-lib-vt -Dsimd=truemodifyOtherKeyssequencesThe Windows VM was unreachable from this workstation (
No route to host); Windows-target compilation and clippy passed locally.