Repository navigation
fix(terminal): take the pty back from the phone's input line and key bar - #964
Merged
Merged
Conversation
In the terminal lens, a phone ignores another device's pane-geometry (it drives the grid itself), but only a press or key inside its xterm element reclaimed the size. The input line, its Send and Enter, and key-bar taps are outside that element, so after a desktop took the pty to its own width the phone kept sending input to a pty drawn for another width. Factor the size check out of reclaim into reassertIfStale (the shared grid differs from this grid, the tab is in use and holds its pane -> refit) and call it from sendTerminalLine, pressEnter, pressKey and the input line's focus, before anything is sent. 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 20 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (2 skipped: 2… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the primary change: reclaiming the terminal pty size from phone input-line and key-bar interactions. Description check The description explains the problem, cause, fix, test coverage, results, and known limitations. It uses headings that differ from the template, but it provides the required Change and Validation info… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 2
-
📝 Generate docstrings 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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>
This was referenced Oct 14, 2026
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
A phone shows a pane in the terminal lens while a desktop tab of the same pane takes the pty back to its own size (a click or key in the desktop terminal, or the desktop window coming back to the front). The phone's xterm stays at phone width while herdr streams rows drawn for the desktop's width, so lines wrap in the wrong place and agent TUIs look scrambled. Typing in the input line, Send, Enter and key-bar taps did not fix it. Only a touch on the grid itself, a focus change or a reload did.
Cause
In the terminal lens, an interact client stores
pane-geometryinsharedGridRefand then ignores it, because it drives the grid itself. The only thing that took the size back while the window kept its focus wasreclaim, and that runs only for a pointerdown or keydown whose target is insideterm.element. The phone's usual input paths are all outside that element:sendTerminalLinesubmits from theTerminalInputtextarea.pressEntersends\r/keys from the same line.pressKeycallsterm.input()from the key bar, with no DOM event on the grid.Fix
reclaimintoreassertIfStale(). When the shared grid differs from this grid, the tab is in use and has not released its pane, it callsrefit()(force resize).reclaimkeeps its event filters (target inside the grid, no IME/repeat key, primary button) and then calls the helper.sendTerminalLine,pressEnterandpressKeycall it before they send, so the resize frame goes out before the input on the same socket.TerminalInputgets an optionalonFocus, wired to the same helper. Tapping into the input line counts as the user being back, the same way a tap on the grid does.Tests
scripts/chat-size-regression.tscheckFocusedWindowReclaimsTerminalSize(already inui-contexts-checks.ts, so it runs in the browser lane) now continues with two clients after its existing steps. A desktop at 1280x800 and a phone at 390x844 in line mode take turns:Each phone step comes after a desktop key has taken the pty back. Each step asserts that the phone's own-size
resizeframe goes out before itssubmit/inputframe, and that the pane's shell (stty size) reports the phone's size again.CHAT_SIZE_CASE=reclaim bun scripts/chat-size-regression.tsnow runs that check on its own.Without the
src/change the new step fails ("a tap into the input line takes the pane back to the phone's grid"). With it,bun run check run --build bun scripts/chat-size-regression.ts(withCHAT_SIZE_CASE=reclaim) passes.bun run typecheckandbun run test:unit(3069 pass, 0 fail) pass.check full/concurrentwas not run.Notes
reclaim,sharedGridRef,sendTerminalLine,pressEnterorpressKey. feat(chat): port t3code chat rendering, composer and settings features #951, feat(chat): Enter twice sends a queued message now #950, feat(terminal): hide the on-screen keyboard, with built-in Korean input #946, feat(appearance): add a phone-only terminal scrollbar toggle #945, feat(dictation): add optional self-hosted Speaches transcription #936, feat(composer): opt-in keyboard autocorrect in the message box #935, feat(chat): steer running agents directly from composer submissions #932, feat(hermes): discover native skill commands and render skill invocations #910, fix(chat): type a message sent during a turn into the agent at once #894, feat(hermes): add safe Chat interaction #859 and feat(keyboard): configure chat autofocus and focus pane input #822 edit other parts ofPaneTerminal.tsx. feat(dictation): add optional self-hosted Speaches transcription #936 also editsTerminalInput.tsx(dictation), so a small textual conflict is possible there.🤖 Generated with Claude Code