Repository navigation
fix(tui): negotiate keyboard enhancement so Shift+Enter inserts a newline - #914
Conversation
…line The composer's own key handling was already correct: `handle_key` inserts a newline for `KeyCode::Enter if shift || alt` (textarea.rs). The bug was one layer up. Most terminals never report a modifier on Enter over the legacy protocol unless the app explicitly opts into the Kitty keyboard enhancement protocol (`PushKeyboardEnhancementFlags`); Astra never called it, so on any terminal that needs the opt-in, Shift+Enter and Enter were byte-for-byte identical before crossterm ever produced a KeyEvent, and the already-correct branch in textarea.rs never had a chance to fire. Detect support during the existing one-shot startup query window (`StartupTerminal::begin`, alongside the color/sixel queries, same quiet period before the interactive event loop starts reading) and thread the result into `TerminalGuard`, which pushes/pops `DISAMBIGUATE_ESCAPE_CODES` around raw mode — deliberately not the fuller Kitty flag set (no `REPORT_EVENT_TYPES`/`REPORT_ALTERNATE_KEYS`), since the rest of the input pipeline does not expect key-release events or shifted-character codepoints. Push/Pop is gated on the detected capability; Pop is best-effort everywhere Astra already does best-effort `DisableBracketedPaste` cleanup (Drop, panic hook, the early init guard), consistent with this file's existing style. No behavior changes for terminals that don't support the protocol. Testing: - `cargo fmt -p astra-cli -- --check` - `cargo clippy -p astra-cli --lib --tests -- -D warnings` - `cargo test -p astra-cli --lib tui::terminal::` and `tui::bottom_pane::textarea::` — all passing - Full-workspace `make check`/`make lint` not run (time cost); the change is isolated to astra-cli's TUI terminal module with no shared-crate edits. - Did not add a new PTY regression test: the existing `terminal_startup_pty_tests.rs` harness would need to simulate the Kitty keyboard-enhancement handshake inside its existing DA1/color query timing budget, which felt like disproportionate risk/effort for this fix versus relying on crossterm's own tested `supports_keyboard_enhancement`/Push/Pop implementation (its own example uses the identical pattern). Did not verify interactively against a real terminal — no TTY available in this environment; only reasoned through the code path and crossterm's documented protocol. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tartup query /open-code-review:delegate-review against the first commit on this branch found a real regression: crossterm's own supports_keyboard_enhancement()/query_keyboard_enhancement_flags() write their own independent `CSI ?u CSI c` pair with a ~2s internal timeout. That both exceeds this file's 300ms QUERY_TIMEOUT budget for every other startup query, and sends a second, competing DA1 query alongside the one query_startup_attributes() already sends for color/sixel detection -- caught concretely by `pty_late_da1_is_unknown_until_reply`, which now failed after the first commit (verified this is not pre-existing: reverting to that commit alone reproduces the same failure). Fold keyboard-enhancement detection into the existing single DA1-anchored round trip instead of adding a second one. query_startup_attributes() now also writes `CSI ?u` right before its existing `CSI c` and returns any KeyboardEnhancementFlags reply alongside foreground/background/DA1; DA1 arriving is already the correct completion sentinel per the Kitty keyboard-protocol spec's own recommended detection method (a supporting terminal answers `?u` at or before DA1), so no new timeout or retry logic is needed. astra-cli reads that field directly and no longer calls crossterm's own detection functions during startup. Documented in ASTRA-PATCH.md. Testing: - `cargo fmt -p astra-cli -- --check`, `cargo clippy -p astra-cli --lib --tests -- -D warnings` -- clean. - `cargo test -p astra-cli --lib tui::terminal::` (4) and `tui::bottom_pane::textarea::` (3) -- passing. - `cargo test --locked -p astra-cli --lib tui::terminal_startup -- --test-threads=2` -- 15 passed, including `pty_late_da1_is_unknown_until_reply`. - `cargo test --locked -p astra-cli --lib --features crossterm/use-dev-tty tui::terminal_startup -- --test-threads=2` -- 15 passed (the alternate reader backend ASTRA-PATCH.md calls out for query/framing/deadline changes). - `cargo test --locked --manifest-path vendor/crossterm/Cargo.toml --lib --features event-stream` -- 107 passed, 0 failed (the vendored patch's own suite). Still not done interactively against a real terminal in this environment, same limitation as the first commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed both commits through 8cf887f against base 62dc136.
The final detection design is appropriate: extend the existing bounded startup query, retain the shared crossterm reader and DA1 completion contract, and request only DISAMBIGUATE_ESCAPE_CODES. The second commit removes the competing query introduced by the first; I found no remaining second detection path or additional query timeout. The parser/event adapter preserves Enter modifiers and the existing composer branch consumes Shift+Enter as a newline.
Request changes for one P2 in keyboard-mode ownership/cleanup, detailed inline. Terminal support and an outstanding push are different facts. The current cleanup treats stack pop like an idempotent mode reset, which is unsafe across panic/unwind and temporary terminal handoff. Please keep the fix in TerminalGuard's existing lifecycle rather than adding another terminal manager. Also establish rollback ownership before the new push: currently EnableBracketedPaste can fail after a successful push but before RawModeGuard exists, leaving that keyboard entry unbalanced.
Code-growth review: +107/-9, net +98: +84 in implementation files (including comments), +12 documentation, and +2 in the PTY test file. This is a proportionate extension of the existing owners, with no new dependency, polling loop, or duplicate capability query. However, the test-file change only adapts the init signature; it adds no behavioral coverage. Every existing PTY fixture omits CSI ?u, so none exercises the newly enabled push branch. A supporting-terminal fixture belongs in this fix: return CSI ?0u before DA1, assert one query and the push, check Shift+Enter versus Enter through the input/composer boundary, and verify balanced restoration on normal exit, external-editor handoff, and panic/unwind with a pre-existing parent keyboard mode.
Validation: Test Suite and Static Checks are successful for this exact head. Inspected the actual Linux default-reader and macOS dev-tty PTY logs: 15 tests pass in each. Local git diff --check passes and the worktree is unchanged. No Rust/Cargo toolchain is available locally, so the ownership finding is based on source control flow and the Kitty protocol contract, not a claimed runtime reproduction.
Addresses XuPeng-SH's P2 in review matrixorigin#914 (review). Push/Pop keyboard enhancement is a stack, not an idempotent toggle like DisableBracketedPaste. A panic while TerminalGuard is alive runs the global panic hook first (synchronously, before unwinding starts), then TerminalGuard::drop during unwind -- both called PopKeyboardEnhancementFlags unconditionally, so every such panic popped twice. On a terminal where a parent program (tmux, an outer shell) already had its own entry pushed, the second pop would remove that entry instead of being a no-op; the review correctly identified this as unsafe, not merely redundant. init() had a second, narrower gap: if the push succeeded but the very next fallible write (EnableBracketedPaste) did not, the function returned before RawModeGuard existed, leaving that push permanently unpaired. Track ownership of the currently-outstanding push separately from whether the terminal supports the protocol at all, in one process-wide AtomicBool (TerminalGuard is a process-wide singleton already, matching the existing PANIC_HOOK_INSTALLED Once in this same file). push_keyboard_enhancement() only marks ownership after the write actually succeeds; pop_keyboard_enhancement_if_owned(_fallible) consumes it with one atomic swap, so whichever cleanup path runs first -- the panic hook, the early RawModeGuard, with_restored, or the final Drop -- is the only one that actually writes Pop. Also reordered init() to construct RawModeGuard before attempting the push, closing the unpaired-push gap on the push-succeeds-but-bracketed-paste-fails path. Added the two regression tests the review asked for: - crates/astra-cli/src/tui/bottom_pane/textarea.rs: shift_enter_inserts_newline_but_plain_enter_submits locks in the composer-side half of the Shift+Enter contract (unchanged by this PR, but previously uncovered) at the unit level. - crates/astra-cli/src/tui/terminal_startup_pty_tests.rs: a new "keyboard_enhancement_panic" PTY case simulates a terminal that answers CSI ?u (flags=0, still Some(..) not None) before DA1, panics right after a successful TerminalGuard::init(), and pty_keyboard_enhancement_push_and_pop_survive_a_panic_exactly_once asserts exactly one CSI ?u query, one push (CSI >1u), and one pop (CSI <1u) in the captured byte stream. Verified the test actually catches the bug: with the ownership check temporarily reverted to an unconditional pop, this test fails left:2 right:1, with two literal `\x1b[<1u` sequences visible in the captured output (one from the panic hook, one from Drop); restored the fix immediately after confirming that. Did not build the fuller scenario the review also described (pre-existing parent keyboard mode, external-editor handoff apply-then-verify, Shift+Enter through the full input/composer boundary in a live TUI). The PTY harness simulates raw terminal replies, not actual Kitty-protocol stack state held by a real parent process, so "was the parent's entry preserved" isn't observable through it; the closest faithful signal this harness can give is the exactly-once push/pop byte count above, which is what the new test asserts. Composer-level Shift+Enter-vs-Enter is now covered by the new textarea.rs unit test; a full TUI event-loop-level assertion of it felt like a separate, larger addition to the existing PTY suite's scope, which currently stops at the crossterm KeyEvent boundary (see `pty_escape_poll_stream_and_key_sequences` and friends) rather than driving the composer. Testing: - `cargo fmt -p astra-cli -- --check`, `cargo clippy -p astra-cli --lib --tests -- -D warnings` -- clean. - `cargo test -p astra-cli --lib tui::terminal_startup -- --test-threads=2` -- 16 passed (15 previous + the new case), on both the default reader and `--features crossterm/use-dev-tty`. - `cargo test -p astra-cli --lib tui::terminal::` (4) and `tui::bottom_pane::textarea::` (4, including the new one) -- passing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed the complete PR through d63d3ad, including the delta from my previous review at 8cf887f. This approval supersedes my earlier request-changes decision for the double-pop finding.
The original P2 is addressed: support detection is separate from outstanding-push ownership; panic cleanup and subsequent Drop consume the same ownership bit; temporary handoff consumes it before re-entry; and the early rollback guard now exists before the push and bracketed-paste writes. The new supporting-terminal PTY fixture exercises the real startup/init/panic/unwind path and asserts one query, one push and one pop. Counting the emitted protocol operations is sufficient regression evidence for the reported double-pop issue; a complete terminal emulator or parent process is not required to establish that property.
Rechecked the combined query/filter/deadline, unsupported-terminal fallback, modifier parsing/event forwarding, composer dispatch, and normal external-editor/shell handoff and re-entry. The final branch still has one startup query owner and one terminal lifecycle owner, requests only DISAMBIGUATE_ESCAPE_CODES, and introduces no second reader, recurring query, dependency, or new keyboard-event mode. No remaining merge-blocking finding from this pass.
Code-growth review: +246/-11, net +235, consisting of +120 in implementation files (including comments), +103 in test code and +12 documentation. The ownership state and panic regression have concrete value. One non-blocking simplification is inline: the additional textarea test repeats existing direct Enter/Shift+Enter coverage and should be consolidated rather than maintaining another copy. The PTY regression covers negotiation and cleanup; it does not feed a Shift+Enter escape sequence through a live composer, so comments should describe that scope accurately.
Validation: the exact-head Test Suite and Static Checks workflows passed. Inspected Linux default-reader and macOS dev-tty logs: all 16 startup tests pass in each, including the new panic regression. The CLI log also confirms the new textarea test and the existing Shift+Enter test pass. git diff --check is clean. Local Rust/Cargo execution and manual real-terminal testing were unavailable; I have not claimed either.
…a tests Addresses XuPeng-SH's non-blocking P3 in review matrixorigin#914 (comment). crates/astra-cli/src/tui/tests.rs already has enter_submits and shift_enter_inserts_newline exercising this exact contract; my earlier claim that it was previously uncovered was wrong -- I had only grepped textarea.rs's own inline test module and missed the separate external one. The third commit's new test duplicated most of that existing coverage. Removed it and folded its two additional assertions into the existing tests instead of keeping a second copy: enter_submits now also checks Submit does not consume/alter the draft, and shift_enter_inserts_newline now also checks the Shift+Enter handle_key call itself returns TextAreaAction::Changed (it previously only checked the resulting text after a following keystroke). To be precise about scope, since the review also flagged this: the PTY fixture added in the third commit (pty_keyboard_enhancement_push_and_pop_survive_a_panic_exactly_once) verifies capability negotiation and panic/unwind cleanup ownership by counting protocol escape sequences in the captured byte stream. It does not send a Shift+Enter key sequence through a live composer -- that half of the contract is exactly what enter_submits/shift_enter_inserts_newline (now with the added assertions) cover, at the TextArea::handle_key level. Testing: - `cargo fmt -p astra-cli -- --check`, `cargo clippy -p astra-cli --lib --tests -- -D warnings` -- clean. - `cargo test -p astra-cli --lib tui::tests::textarea_tests::` -- 18 passed (all existing cases plus the two new assertions). - `cargo test -p astra-cli --lib tui::bottom_pane::textarea::` -- 3 passed (the duplicate removed, the other three unaffected). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Merge Queue Status
This pull request spent 10 seconds in the queue, including 1 second running CI. Required conditions to merge
|
2 participants
Summary
Shift+Enter in the composer behaved the same as plain Enter (submitted instead of inserting a newline). The composer's own key handling (
textarea.rs) was already correct — it inserts a newline forEnterwhen theSHIFT(orALT) modifier is present, and this already had direct test coverage (enter_submits/shift_enter_inserts_newlineincrates/astra-cli/src/tui/tests.rs). The bug was one layer up: over the legacy terminal protocol, most terminals never report a modifier on Enter at all unless the application explicitly opts into the Kitty keyboard enhancement protocol (PushKeyboardEnhancementFlags). Astra never called it, so on any terminal that needs that opt-in, Shift+Enter and plain Enter arrived as byte-for-byte identical input before crossterm ever produced aKeyEvent— the already-correct, already-tested branch intextarea.rsnever had a chance to fire.Detection approach:
query_startup_attributes()(Astra's existing vendored-crossterm extension for the one bounded, DA1-anchored startup round trip already used for color/sixel detection) now also writesCSI ?uimmediately before its existingCSI cand returns anyKeyboardEnhancementFlagsreply alongside the others. DA1 arriving is already the correct completion sentinel per the Kitty keyboard-protocol spec's own recommended detection method (a supporting terminal answers?uat or before DA1), so this needed no new timeout or retry logic, and stays inside the existing 300msQUERY_TIMEOUTbudget with no second query sent to the terminal.TerminalGuardthen pushes/popsDISAMBIGUATE_ESCAPE_CODESaround raw mode based on that one result, with ownership of the currently-outstanding push tracked separately from terminal support (see commit 3) so cleanup consumes it exactly once no matter which path runs first.History (four commits, two from review feedback)
supports_keyboard_enhancement()directly./open-code-review:delegate-reviewbefore considering commit 1 done. That function writes its own independentCSI ?u CSI cpair with a ~2s internal timeout — exceeding this file's 300ms budget, and sending a second, competing DA1 query alongside the onequery_startup_attributes()already sends. The existing PTY regressionpty_late_da1_is_unknown_until_replyfailed as a result (verified not pre-existing: reverting to just commit 1 reproduces it). Fixed at the root by folding detection into the existing single round trip.TerminalGuardis alive ran both the global panic hook andTerminalGuard::dropduring unwind, each popping unconditionally — on a terminal where a parent program already had its own entry pushed, the second pop would have removed that entry instead of being a no-op. Tracked ownership of the currently-outstanding push in one process-wideAtomicBool, consumed by exactly one cleanup path via an atomic swap. Also reorderedinit()to construct the rollback guard before attempting the push, closing a narrower leak where a push-succeeds-then-EnableBracketedPaste-fails window left the push permanently unpaired. Added a PTY fixture that answersCSI ?ubefore DA1 and panics right after a successfulinit(), asserting exactly one query/push/pop byte sequence in the captured output — verified it actually catches the regression (temporarily reverted the fix, watched it failleft:2 right:1with two literal\x1b[<1usequences visible).textarea.rsunit test claiming Shift+Enter had no direct regression coverage. That was wrong — I'd only greppedtextarea.rs's own inline test module and missedcrates/astra-cli/src/tui/tests.rs, which already hadenter_submits/shift_enter_inserts_newlinecovering this. Removed the duplicate and folded its two extra assertions (theChangedreturn value, and thatSubmitdoesn't alter the draft) into the existing tests instead.Related issue
No related issue — reported directly.
Change type
User and compatibility impact
On terminals that support the Kitty keyboard enhancement protocol (most modern terminal emulators: kitty, WezTerm, iTerm2 3.5+, Ghostty, foot, contour, etc.), Shift+Enter now inserts a newline in the composer as documented (
ARCHITECTURE.md, the in-app help text, andslash_dispatch.rsall already claimed this behavior). On terminals that don't support it, behavior is unchanged (no regression, capability-gated). No configuration or API changes.Architecture and complexity delta
StartupTerminal::begin()(crates/astra-cli/src/tui/terminal_startup.rs) already owns the one bounded, DA1-anchored startup query; extended to also carry keyboard-enhancement detection.TerminalGuard(crates/astra-cli/src/tui/terminal.rs) already owns raw-mode and bracketed-paste negotiation; extended to also own keyboard-enhancement push/pop and its ownership bookkeeping.PushKeyboardEnhancementFlags/KeyboardEnhancementFlagswere previously unused anywhere in Astra's own code. Confirmed crossterm's ownsupports_keyboard_enhancement/query_keyboard_enhancement_flagsare not suitable for use during Astra's startup window (see commit 2) and are not called anywhere in the final diff. Confirmedcrates/astra-cli/src/tui/tests.rsalready owned direct Enter/Shift+Enter coverage before adding to it (see commit 4).textarea.rstest (removed in commit 4).TerminalGuarddeliberately requests onlyDISAMBIGUATE_ESCAPE_CODES, not the fuller Kitty flag set (REPORT_EVENT_TYPES,REPORT_ALTERNATE_KEYS,REPORT_ALL_KEYS_AS_ESCAPE_CODES): those would also start delivering key-release events and alternate/shifted-character codepoints the rest of the input pipeline does not currently expect, out of scope for this fix.Verification
cargo fmt -p astra-cli -- --check,cargo clippy -p astra-cli --lib --tests -- -D warnings— clean.cargo test -p astra-cli --lib tui::terminal::— 4 passed.cargo test -p astra-cli --lib tui::bottom_pane::textarea::— 3 passed.cargo test -p astra-cli --lib tui::tests::textarea_tests::— 18 passed (all pre-existing cases plus the two new assertions from commit 4).cargo test --locked -p astra-cli --lib tui::terminal_startup -- --test-threads=2— 16 passed (15 pre-existing + the commit-3 panic/cleanup regression), on both the default reader and--features crossterm/use-dev-tty.cargo test --locked --manifest-path vendor/crossterm/Cargo.toml --lib --features event-stream— 107 passed, 0 failed (the vendored patch's own suite, per ASTRA-PATCH.md).make check/make lintwas not run (time cost); the change is isolated toastra-cli's TUI terminal module plus the already-Astra-ownedvendor/crosstermextension, with no other shared-crate edits.TextArea::handle_keyunit tests, not by hand end-to-end.KeyboardEnhancementFlagsreply arrives before DA1) — push is never attempted, gated on the detected capability. A panic while a push is outstanding, exercised by the new PTY fixture, no longer double-pops.Precise scope, since a prior version of this PR description overstated it: the PTY suite verifies capability negotiation and cleanup ownership by counting protocol escape sequences in the captured byte stream — it does not drive a live composer with a Shift+Enter key sequence. That half of the contract (a
Shift+EnterKeyEventreachingTextArea::handle_keyand producing a newline) is covered separately, at the unit level, byenter_submits/shift_enter_inserts_newlineincrates/astra-cli/src/tui/tests.rs.Final checklist
ASTRA-PATCH.mdupdated for thevendor/crosstermextension;ARCHITECTURE.md/help text already documented the intended Shift+Enter behavior — this makes the implementation match it).🤖 Generated with Claude Code