Repository navigation
fix(terminal): keep the key bar live while the input line composes - #962
Merged
Merged
Conversation
The input line's compositionstart/compositionend drove the same composingRef and composing state that guard xterm's deferred IME commit, so a word in progress (most of the time with Gboard, and every syllable with a Korean or Japanese IME) greyed out the whole key bar and made pressKey, toggleModifier and toggleDirect return early. The input line's text never passes through xterm, so there is nothing for a key-bar key to overtake. Only the xterm host listeners now set the guard; the input line keeps its own Send guard. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 41 minutes. View limit details
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 |
…poses Review follow-up: the input line's composition no longer greys out the key bar, so the direct-typing toggle became live mid-word and could unmount the line with an IME composition still open. Track the line's composition separately, block toggleDirect on it and disable only the direct key. The phone regression now taps that key mid-composition and checks the line, its text and the pane are untouched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # src/components/PaneTerminal.tsx
devswha
added a commit
that referenced
this pull request
Oct 13, 2026
… ping (#967) ## Problem A WebSocket can still report `OPEN` after its TCP path is gone, for example when a phone moves from Wi-Fi to cellular or wakes on a changed network. No close event arrives for tens of seconds to minutes. In that time: - the terminal stops updating while the header still says connected, and no reconnecting banner shows; - keys go to `rawSend` on the dead socket and `sendInput()` returns true, so PaneTerminal never keeps them as a held draft and they are lost; - a composer send waits the full 90 s submit timeout; - `wake()` (visibilitychange / pageshow / online) leaves an `OPEN` socket alone. ## Cause `HerdrSocket` treats `readyState === OPEN` as proof that the connection is alive. The protocol has no app-level ping. Bun's own pings and idle timeout only tell the server about a dead peer, and the browser never sees them. ## Fix - **Protocol** (`shared/protocol.ts`): new `ClientMessage` `ping {id}`, `ServerMessage` `pong {id}` and `ServerFeature` `"ping"`. The doc comments on both frames are new. - **Server** (`server/index.ts`): answers `ping` with `pong` for any role and lists `"ping"` in its snapshot. A malformed id gets an `invalid_ping` error frame, not a pong. - **Remote relay:** no code change. `MachineRelay` already forwards frames as they are, so the remote PC's own snapshot decides whether the client pings. The contract test proves the pass-through. - **Demo** (`site/demo/transport.ts`): lists `"ping"` and answers it the way the server does. - **Client** (`src/lib/ws.ts`): - Tracks `lastFrameAt`. On a bridge whose last snapshot listed `"ping"`, it probes an `OPEN` socket on wake (visible, pageshow, online) and after 20 s without a frame while the page is visible (checked every 5 s). The flag is remembered across reconnects, so a socket whose snapshot never arrives (a relay path that dies right after the upgrade, a failed snapshot) is probed too. - Only the `pong` of a ping in flight ends a probe. Any frame still postpones the idle probe, but a frame the browser buffered before a sleep, or a pong with another id, does not prove the path is alive. The probe ends before handlers run, so `canInput()` already reads true for them. - No pong within 3 s sends a second ping; no pong to that drops the socket on the client side: `socket = null`, the disconnect handlers run, submits settle as `disconnected`, `close()` is called, and `connect()` runs at once with retries reset. The open handler then replays the role and attaches. Each probe that ends in a drop doubles the per-ping deadline (cap 12 s) until a pong comes back, so a slow relayed link is not dropped over and over. - A dropped socket is kept in `endedEarly` (now a `WeakSet`, shared with `wake()`), so a 4008 close that arrives after the drop still stops output and closes the replacement. A socket that is `CLOSING` when the deadline hits is left to its close event (same rule as #954's `wake()`). - Hiding the page cancels a probe; showing it starts a fresh one. A deadline that fires more than 2 s late (the page was frozen and the timer ran before the queued pong) pings again instead of dropping. - While a probe waits, `canInput()` refuses, so typing goes to the held draft. `submit()`, `pendingAction()` and `sendSecret()` wait the probe out after their snapshot wait and right before sending; if the socket is dropped they settle as `disconnected` having sent nothing (the secret is still cleared). The composer no longer reports offline during a routine probe. - New explicit `ConnectionState` (`idle | connecting | open | probing | waiting | stopped | closed`), exposed as `state`. `connected` still reads the socket itself, so the banner does not flash on every tab switch. - **Held draft** (`src/components/PaneTerminal.tsx`): once input is held for a pane, all later typing joins the draft until Send or Discard, also after the terminal is ready again. Before this, keys refused during a probe (or a disconnect) were held while the next keys after the pong went straight to the pty: `echo ` held and `rm -rf build⏎` run. Text, pastes, Backspace and anything with a line break join the draft; Enter is left out of it, a sticky-modifier chord is refused with a note (new string, translated in ko/ja/zh/tr), and the input line's Enter is refused while a draft is held. Other control keys (Ctrl+C, Escape from the Stop button, Tab, arrows) still reach the program, so an interrupt never waits on the draft. - `src/AGENTS.md`: the WS client row records the probe rules, and a new row records the held-draft rule. ## Tests - `src/lib/ws.test.ts` (fake socket and fake timers): - an `OPEN` socket with no pong gets a second ping at 3 s and is dropped at 6 s. The disconnect handler runs once, a pending submit settles as `disconnected`, a new socket starts with no backoff and its open replays role and attach at the saved geometry, and the late close of the old socket changes nothing; - during a probe, `sendInput` and `sendKeys` are refused; a buffered `pane-status` frame and a pong with another id leave it probing; the matching pong restores input; a second wake sends no second ping; - a socket that only delivers stale frames during a probe is dropped; - `submit`, `pendingAction(steer)` and `sendSecret` started during a probe send nothing until the pong, then each sends once; - a submit already waiting on its snapshot when `wake()` runs, and a secret, settle as `disconnected` with nothing sent when the probe drops the socket; - a 4008 close after a probe drop still stops output and closes the replacement; - a deadline that runs out while the page is frozen pings again, and the pre-freeze pong no longer counts; - hiding the page cancels the probe (no drop in 60 s), showing it starts a fresh one; - a reconnected socket with no snapshot is probed when the previous snapshot listed `ping`; - a probe drop doubles the next deadline, and a pong resets it; - plus the earlier idle-ping, no-flag and `CLOSING` tests. - The 10 new or changed tests fail with the previous `ws.ts` of this branch (checked by restoring it). - `scripts/terminal-input-regression.ts`: after the terminal becomes ready with `a` held, Enter and `b` are typed: the draft reads `ab` and nothing reaches the pane; Escape still reaches the pane with the draft kept; Send types `ab` once with no Enter. It fails without the PaneTerminal change (checked by restoring it and rebuilding). - `server/ping.contract.test.ts` (integration, real herdr): the snapshot lists `ping`; ping gets a pong with the same id, also for an observe role; a malformed id gets `invalid_ping`; ping and pong pass through `MachineRelay`. Not rerun for the review fixes (no server change). - `bun run typecheck`: pass. - `HERDR_TEST_MODE=unit bun test ./src/lib/ws.test.ts`: 49 pass. - `bun run test:unit`: 3088 pass, 0 fail, 8 skip (includes the #949 convention guards). - `bun run check run --build bun scripts/terminal-input-regression.ts`: pass. - `scripts/ui-contexts-regression.ts` passed on the first revision; not rerun after the review fixes. - No browser test of a real half-open socket: Playwright cannot hold a socket half-open, so the unit tests cover that part. ## Notes - **Stacked on #954; retarget to main after it merges.** It builds on #954's connect deadline, `wake()` rules and `SOCKET_CLOSING` handling in `ws.ts`. - **Release:** `shared/protocol.ts` and `server/index.ts` are inputs to the remote bundle, so the release must raise `REMOTE_BUNDLE_VERSION`. The release guard from #948 (`scripts/release-guards.ts`) refuses to tag otherwise. It is not bumped here. Until a remote PC updates, its snapshot does not list `ping`, so it is never probed and behaves as before. - **Trade-off:** a key typed within one round trip of a probe start lands in the held draft with Send/Discard rather than being sent, and later typing joins it until the user picks one. Sending it later on its own would break the never-queue rule. - **Left out on purpose:** - In-flight submits still settle as `disconnected` on a probe drop rather than waiting for the close event: a half-open socket may not close for minutes, and the server never types a message more than 45 s after it arrived. - No "checking connection" UI and no state-change event for probe start and end. `inputReady` drives the "Waiting for terminal input…" banner and the key bar, so a sub-second probe on a healthy link would flash both on every wake. The held-draft banner already shows the case the user can act on, and the composer no longer reports offline. The remaining gap: PaneTerminal recomputes `inputReady` only when a message arrives. - The `ping` flag survives a reconnect: if a PC is downgraded to a bridge without `ping`, the first probe after the reconnect costs one drop before the new snapshot clears the flag. - `resume()` in PaneTerminal still resets the screen before the attach answers (render-scroll-1's second half). - **Overlaps:** - #956 edits the held-draft code (`applyToDraft` Backspace and line breaks) and the same lines of `scripts/terminal-input-regression.ts`: a textual conflict, and the test expectation changes with it: after #956 an Enter in the draft becomes a space plus a notice, so the held and sent text become `a b` instead of `ab`. Never an Enter either way. - #932 edits the same `submit()` guard area in `ws.ts`, plus `protocol.ts` and the demo transport. - #951, #936 and #859 add to the protocol unions, the demo switch and `server/index.ts`'s message switch. These are textual merges with no conflict in meaning. - #962, #964 and #966 touch PaneTerminal's key bar and input line, not the onData draft path. - #952 owns `server/machine-relay.ts`. Nothing changed there. - From a mobile-terminal stability audit: connection-1, render-scroll-1, lifecycle-1, history-1, moshi-1. ## Merge-session review Fixed (each with a regression test that fails without the fix): - **Hiding the page answered the probe** (`ws.ts`): a submit, pending action or secret waiting on a probe was sent on an unchecked socket once the page hid. A hidden page now pauses the probe (`probePaused`): input stays refused, waiters keep waiting, and showing the page pings again. Test: `keeps a submit and a secret waiting when the page hides during a probe...`; the existing hide test now expects `probing` and refused input while hidden. - **Replacing a CLOSING socket left its probe unresolved** (`ws.ts`): `connect()` now ends a non-null previous socket (CLOSING, or CLOSED before its close event) first, so its probe and waiting submits settle, the disconnect handlers run, and a late 4008 from it still stops output (`endedEarly`). Test: `settles a submit waiting on a probe when a CLOSING socket is replaced...`. - **Ctrl+Enter under modifyOtherKeys got past the held-draft guard** (`PaneTerminal.tsx`): a modified Enter is decided on before its rewrite to `CSI 27;5;13~` and refused like a chord, with the held-input note. Test: `terminal-input-regression` turns on modifyOtherKeys and presses Ctrl+Enter beside a held draft. - **A paste with a tab went to the pty and a multi-line paste was dropped in silence** (`PaneTerminal.tsx`): while a draft is held, a paste the draft cannot take is refused with the held-input note. Test: paste `a\tb` and `x\ny` beside a held draft; nothing reaches the pane, and the multi-line paste is told or held, never dropped in silence (written so it still holds once #956 folds line breaks into the draft). - **The input line said "disconnected" and its non-empty line got past the draft rule** (`TerminalInput.tsx`, `PaneTerminal.tsx`): `onEnter` returns `boolean | string` and the held-input reason is shown; `sendTerminalLine` refuses beside a held draft too and keeps the text. Test: empty Enter and `ls` + Enter on the input line beside a held draft show the held-input note and send nothing. - `src/AGENTS.md` rows and the changelog fragment updated to match. Deferred (unchanged from the builder's notes, including its review finding 8): the items under "Left out on purpose" above, such as PaneTerminal recomputing `inputReady` only when a message arrives. Notes for the merge step: - #968 is stacked on this branch and does not carry these two commits; it picks them up when restacked onto main after this PR squash-merges. - #956 overlap: the `terminal-input-regression` Send step now checks the draft's own text, so it holds whether #956 shows `ab` or `a b`. Checks: `bun run check fast` pass; `scripts/ui-regression.ts` (terminal input, Ctrl+Enter, held-draft lanes) pass; `scripts/sticky-modifiers-regression.ts` pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
devswha
added a commit
that referenced
this pull request
Oct 13, 2026
Conflicts: - src/components/TerminalInput.tsx: send() keeps reading the draft store and takes main's onEnter string result (#967 held-input reason). - scripts/terminal-input-regression.ts: the PR's boolean socket refusal is renamed refuseConnections/currentSocket beside main's refuse() submit-result callback and toPage; the PR block reuses main's note locator. - docs/terminal-input.md: main's key-bar paragraph (#962) plus the Send-during-composition sentences. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On a phone in input-line mode (the touch default), the whole key bar goes grey and ignores taps while a word is still composing in the input line. With Gboard that is most of the time, because every word composes until a space; with a Korean or Japanese IME it lasts until the syllable or phrase commits. Esc and ^C cannot stop an agent, an arrow cannot answer a menu, Ctrl/Alt/Shift cannot be armed, and the keyboard-mode toggle is dead.
Cause
PaneTerminalpassedonComposing={setComposing}toTerminalInput, so the input line'scompositionstart/compositionendwrote the samecomposingRefandcomposingstate that guard xterm's deferred IME commit. That flag disables<KeyBar>and short-circuitspressKey,toggleModifierandtoggleDirect. The guard exists so a key-bar key cannot overtake xterm's pending commit, but the input line's text never goes through xterm oronData: it is submitted separately withsocket.submit. There was nothing to protect, only taps to drop.Fix
PaneTerminal.tsx: stop wiringTerminalInput's composition intosetComposing. The xterm hostcompositionstart/compositionendlisteners (and the pane reset) stay the only writers ofcomposingRef/compositionCommitPendingRef, so the xterm deferred-commit gate is unchanged.TerminalInput.tsx: remove the now unusedonComposingprop and its unmount cleanup. The line keeps its own localcomposingref that guards its Send button and Enter.docs/terminal-input.md: the key bar waits only for a direct-typing (xterm) composition; an input-line composition leaves it live.TerminalInputreports its composition throughonComposingagain.PaneTerminaltracks it apart from the xterm composition,toggleDirectreturns early on it, andKeyBargetsdirectDisabled, which greys out only that key.Tests
scripts/terminal-input-regression.ts(390x844, touch, line mode): fill the line, dispatchcompositionstarton.terminal-input-text, assert Esc, ^C and Ctrl are enabled, tap Esc and ^C, expect exactly twoinputframes\x1band\x03, and the line's value unchanged.bun run check run --build bun scripts/terminal-input-regression.ts: PASS (all three blocks). With the two source files reverted the new block fails on "Esc stays live while the input line composes".bun run typecheck: clean.bun run test:unit: 3069 pass, 0 fail, 8 skip.scripts/changelog-fold.test.ts,scripts/pages-gate.test.ts,scripts/conventions.test.ts: pass.scripts/ui-regression.tsas a whole (it callscheckTerminalInput, so CI runs the new block there too),check full/concurrent.[data-key="direct"]is disabled during the composition and that a forced tap on it leaves the line, its text and the pane untouched. Before the follow-up fix, that check failed with "the direct-typing toggle waits for the line's composition".Notes
onComposing/setComposing). feat(dictation): add optional self-hosted Speaches transcription #936 editsTerminalInput.tsxand fix(terminal): key bar off the pane's edge; home-indicator room only on the bottom bar #943/feat(terminal): hide the on-screen keyboard, with built-in Korean input #946/feat(chat): Enter twice sends a queued message now #950/feat(chat): port t3code chat rendering, composer and settings features #951 editPaneTerminal.tsx/KeyBar.tsx, so a textual conflict is possible depending on merge order, but none duplicates this fix.\rto the pane, not the line's text, just as it does after a commit. The direct-typing toggle is gated (see the follow-up in Fix).🤖 Generated with Claude Code