From abf1d1f1ca3e382775fa60d2ae16be984c3aae5d Mon Sep 17 00:00:00 2001 From: Rounak Datta Date: Tue, 22 Sep 2026 13:07:33 +0530 Subject: [PATCH] fix(terminal): the PTY and the browser terminal must never disagree about size MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #464, "text gets muffled sometimes, in both TUI default and fullscreen". The screenshot is not a dropped frame or a frozen renderer — it is arithmetic. Claude Code's TUI wraps its frame at the width the PTY reported and erases the previous frame by walking the cursor up the rows it believes that frame took. A browser terminal of a different width makes each logical line occupy more physical rows than Ink counted, so `eraseLines(n)` clears too few and the new frame paints over rows nothing erased: doubled lines, and short tool summaries sitting inside longer prose rows with the prose's tail still visible. Reproduced against this repo's own xterm before changing anything — a 120-column PTY against a 62-column terminal renders every wrapped line twice. `test/ terminal-pty-geometry.test.ts` pins that, and pins the clean render at matching widths beside it, so the assertion cannot be satisfied by code that fixes nothing. Four ways the two drifted apart, none of them observable from either end: 1. `fitAddon.fit()` resizes xterm to `proposeDimensions()` RAW while every server-facing path reported those floored at 40x10. Measured in Chrome at 430px: font size 44 proposed 13 columns, the server was told 40, and xterm stayed at 13. Three call sites each did their own fit-then-floor, and two re-read the proposal after the fit — `_shrinkPaddingToFit()` runs exactly there, so the container had moved. 2. `throttledResize` (keyboard up) and `sendResize` (session detached into its own window) reflowed locally and withheld only the SIGWINCH. That is the one combination that cannot be right: a reflow nothing is rendering for buys nothing and costs correctness. Both now withhold everything, and the keyboard's settle timer still sends the one resize that stops the PTY going stale. 3. `setFontSize`/`setFontFamily`/`setFontWeight` move the cell size — a geometry change — and told the server nothing at all, so raising the font on a phone left the CLI wrapping at the old column count. 4. `Session.resize` DECLINES a small-viewport request while a desktop connection holds an active sizing claim, and said nothing, because resize was write-only. `syncTerminalGeometry()` is now the one function that may change the terminal's size: it fits, floors and applies as a single step, so the numbers xterm holds are the numbers the server is told. A test sweeps every module for a bare `fit()` on the main terminal, and finds exactly one — the owner's own. For (4) the client cannot win, so it is told the truth instead: both transports answer a resize with `session.ptyCols`/`ptyRows` (`{"t":"zc"}` on the socket, the body of the resize POST) and `_onPtyGeometryReport` adopts them. A terminal that keeps a shape the PTY refused does not render "too narrow", it renders garbled. Adopting can leave the pane wider than the screen and the container is `overflow: hidden`, so `.pty-oversized` grants horizontal reach for exactly as long as the mismatch lasts: correct-and-reachable beats correct-and-clipped beats garbled. That rule sets both overflow axes and its own `touch-action` because mobile.css loads later and sets `.terminal-container { overflow: visible; touch-action: none }` — a bare `overflow-x` would leave overflow-y computing to `auto` and hand the browser a vertical scroll container the terminal's touch handler knows nothing about. Verified in Chrome at 430px against a live server, with a desktop client holding the claim: the phone adopts 198x43, gets `overflow-x: auto` / `overflow-y: hidden` / `touch-action: pan-x`, 758px of reach to the right, and keeps its own vertical scrolling. The pre-fix build was measured in the same harness for the control. Two things this deliberately does not do. It does not change who owns the pane size — the desktop still wins, and `_startMobileResizeRetry` still takes it back once that goes idle. And `throttledResize` still holds the PTY's shape for the whole keyboard animation rather than sending a SIGWINCH per step; that decision predates this and was not re-tested here. Also in this commit, Ark0N's third-pass review items on #431: - The response viewer's byte-buffer fallback and `_onSessionClearTerminal` both used the no-param `/terminal` form, capped only by `terminalBufferMaxBytes` (32MB) — the largest body the frontend asks for anywhere. One carried no deadline at all and the other got the 15s tail budget. Both now take the full-history budget. - A `?full=1` capture that outruns its deadline falls back to the bounded tail. The pane is blanked before that fetch, so an abort used to leave a black rectangle, discard the queued live output and never reach `_connectWs`. A failed load now still opens the socket, says one dim line where the content would have been, and clears the tab's spinner — which nothing did, so a failed select left `aria-busy="true"` set forever. - `_wsOutputGapSession` is cleared at the repaint that settles it, not in a `finally` that also ran on the catch. A reconcile that threw, or hit the new deadline — the flaky link the marker exists for — dropped the gap with nothing to retry it. `ws.onopen` no longer clears it up front either. - The replay-clear invariant is pinned in the gate, which is the drift this PR exists to fix: `_resetTerminalForReplay` must be a queued write and nothing else, and no module may blank the terminal with a `clear()+reset()` pair. - `DIAG_ENTRY_MAX_CHARS` replaces the hardcoded 300, bound through a local first: `CodemanDiag?.x` still throws a ReferenceError when the identifier was never declared, and that is the one function in the app that must not throw. - panels-ui's two kill-all clears route through the same helper, and the xterm-version guard's comment says "resolved lockfile version" rather than "dependency RANGE", which is what it has pinned since the last round. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/pty-geometry-464.md | 13 + CLAUDE.md | 4 +- src/session.ts | 20 ++ src/web/public/app.js | 140 ++++++--- src/web/public/constants.js | 70 +++++ src/web/public/mobile-handlers.js | 49 ++-- src/web/public/notification-manager.js | 6 +- src/web/public/panels-ui.js | 9 +- src/web/public/ralph-panel.js | 7 +- src/web/public/settings-ui.js | 2 +- src/web/public/styles.css | 36 +++ src/web/public/tab-rail-resize.js | 2 +- src/web/public/terminal-ui.js | 252 ++++++++++++---- src/web/routes/session-routes.ts | 7 +- src/web/routes/ws-routes.ts | 13 + test/terminal-pty-geometry.test.ts | 384 +++++++++++++++++++++++++ test/terminal-resilience.test.ts | 44 +++ 17 files changed, 934 insertions(+), 124 deletions(-) create mode 100644 .changeset/pty-geometry-464.md create mode 100644 test/terminal-pty-geometry.test.ts diff --git a/.changeset/pty-geometry-464.md b/.changeset/pty-geometry-464.md new file mode 100644 index 00000000..ecc5a047 --- /dev/null +++ b/.changeset/pty-geometry-464.md @@ -0,0 +1,13 @@ +--- +"aicodeman": patch +--- + +fix(terminal): the PTY and the browser terminal must never disagree about size (#464) + +"Text gets muffled sometimes" was arithmetic, not a dropped frame. Claude Code's TUI wraps its frame at the width the PTY reported and erases the previous frame by walking the cursor up the rows it believes that frame took, so a browser terminal of a different width makes the erase come out short and each repaint paints over rows nothing cleared — the doubled lines and half-overwritten prose in the report. + +Four ways the two drifted apart, all silent: `fitAddon.fit()` resized xterm to the raw proposal while every server-facing path reported it floored at 40x10 (measured in Chrome at 430px — font size 44 proposed 13 columns, the server was told 40, xterm stayed at 13); `throttledResize` and `sendResize` reflowed locally while deliberately withholding the SIGWINCH; the font setters moved the cell size and told the server nothing; and `Session.resize` declined small-viewport requests under an active desktop sizing claim without telling the asking client, because resize was write-only. + +`syncTerminalGeometry()` is now the one function that changes the terminal's size — it fits, floors and applies as a single step, and a test sweeps every module for a bare `fit()`. Both transports answer a resize with the geometry the PTY actually holds, and the client adopts it; a pane wider than the screen earns horizontal reach for as long as the mismatch lasts, because correct-and-reachable beats correct-and-clipped beats garbled. + +Also in this release: the response viewer's and clear-terminal's uncapped captures now carry the full-history deadline rather than the tail one; a `?full=1` capture that outruns its deadline falls back to the bounded tail instead of leaving a blank pane, a dead socket and a tab stuck reporting `aria-busy`; the output-gap marker is cleared at the repaint that settles it rather than in a `finally` that ran on failure too; and the replay-clear invariant is pinned in the gate. diff --git a/CLAUDE.md b/CLAUDE.md index 3c7dac44..f1294b57 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -346,9 +346,11 @@ Frontend JS modules have `@fileoverview` with `@dependency`/`@loadorder` tags. L **Mobile prompt composer** (PR #444, the first slice of #359, `keyboard-accessory.js`): the agent bars' Paste key is now **Compose**, a dialog with a native multiline textarea (autocorrect, autocapitalize, spellcheck) where Enter adds a line and only **Send** submits; the shell bar keeps the direct Paste dialog, since shell input is not an agent prompt. Opening it ADOPTS the whole editable terminal prompt (`_takePendingLocalEcho`): the local-echo overlay's pending text has never reached the PTY, but the flushed prefix has, so that prefix is erased with backspaces counted in CODE POINTS (`Array.from(text).length`; measured on Claude Code 2.1.278, `a` + emoji + `b` takes three, and the UTF-16 count sent four and ate the neighbour; `clearTerminalInput()` in terminal-ui.js moved with it). ⚠️ **Drafts are per-session and in memory only** (`_composerDrafts`, never persisted: prompts routinely carry secrets, and persisting them would need the 0600 treatment the intent store gets). Every non-Send exit (Cancel, backdrop, Escape, "Use terminal keyboard") leaves the taken text ONLY in the draft, with the dot on the key (`has-draft`, kept in step by `_syncComposerDraftIndicator`) as the signal that the terminal prompt is empty on purpose, and `_cleanupSessionData` discards the draft with the session. ⚠️ **Delivery is a hand-built bracketed-paste frame** (`\x1b[200~` + the text with newlines mapped to `\r` + `\x1b[201~`, byte-identical to what `terminal.paste()` would emit) through `_sendInputAsync` WITHOUT `{ useMux: true }`, plus a SEPARATE Enter 120 ms later WITH it (codex drops keys that share a PTY read with a bracketed paste). Not `terminal.paste()`, for two reasons: xterm's `bracketedPasteMode` mirror is false for every session after a tab switch or reload (`terminal.reset()` in the replay re-clones the DEC modes, the tmux capture carries no `?2004h`, and tmux never forwards the pane's DECSET to a client after attach), so a paste through xterm would go out unbracketed and the CLI would submit at the first `\r`; and xterm's onData is where the local-echo paste branch flushes pending overlay text AHEAD of the block, text the composer has already taken and erased from the PTY, so the frame goes straight to the wire with the composer as the prompt's only owner. The unconditional frame is safe for a CLI that never enabled DECSET 2004 because tmux does the gating (measured against a live pane: markers stripped for a `cat -v` pane, forwarded intact to a process that had emitted `?2004h`). ⚠️ The frame must never take the mux fallback: `TmuxManager.sendInput()` strips every `\r` and `\n`, which welds the lines together and submits them. ⚠️ The size guard sits on the `MAX_INPUT_LENGTH` boundary (64 KiB of UTF-16 code units): `ws-routes.ts` drops a longer frame WITHOUT an ACK, which would wedge the durable queue, so `_composerMaxLength` is derived from that limit minus both markers and an oversized prompt stays a draft with a toast. ⚠️ `.prompt-composer-overlay` is a `.paste-overlay` with a gutter of its own (a `padding` shorthand whose bottom is 12px plus the safe area), and the unconditional `.paste-overlay` fold rule at the end of styles.css is a later longhand at the same specificity, so it ERASED that gutter (measured at 393x852: `padding-bottom: 0px` flat, and the hinge strip REPLACING the gutter with the fold variables set): the composer has its own restatement after the fold rules, the dialog's `max-height` subtracts `--fold-block-end`, and `test/foldable-layout.test.ts` lists the composer in `ELEMENTS` by hand, because its derived overlay list keys on rules that declare `position: fixed; inset: 0` themselves. Tests: `test/mobile-prompt-composer.test.ts` (in the CI gate, deliberately not under `test/mobile/**`). +**The PTY and the browser terminal must never disagree about size** (issue #464, `syncTerminalGeometry()` in terminal-ui.js): Claude Code's TUI wraps its frame at the width the PTY reported and erases the previous frame by walking the cursor up the number of rows it BELIEVES that frame occupied. A browser terminal of a different width makes each logical line take more physical rows than Ink counted, so `eraseLines(n)` clears too few and the new frame paints over rows nothing erased — the doubled lines and half-overwritten prose in #464, **measured** against this repo's xterm (a 120-column PTY against a 62-column terminal renders every wrapped line twice; `test/terminal-pty-geometry.test.ts` pins it, and pins the clean render at matching widths so the assertion cannot pass against code that fixes nothing). ⚠️ **`fitAddon.fit()` is NOT the way to resize this terminal.** It resizes xterm to `proposeDimensions()` RAW while every server-facing path reports those floored at 40x10, so whenever the floor bit the two diverged silently — **measured in Chrome at 430px**: font size 44 proposed 13 columns, the server was told 40, and xterm stayed at 13. `syncTerminalGeometry()` fits, floors and applies as one step and is the ONE function that may change the size; `test/terminal-pty-geometry.test.ts` sweeps every module for a bare fit. ⚠️ **Withhold the fit wherever you withhold the SIGWINCH.** `throttledResize` (virtual keyboard up) and `sendResize` (session detached into its own window) used to reflow locally and skip only the server write, which is the one combination that cannot be right. ⚠️ **A font change is a geometry change**: `setFontSize`/`setFontFamily`/`setFontWeight` move the cell size and told the server nothing, so raising the font on a phone left the CLI wrapping at the old column count (`_refitAfterCellSizeChange`). ⚠️ **Resize is no longer write-only.** `Session.resize` DECLINES a small-viewport request while a desktop connection holds an active sizing claim and says nothing, so both transports now answer with `session.ptyCols`/`ptyRows` (`{"t":"zc"}` on the socket, the body of the resize POST) and `_onPtyGeometryReport` adopts them — a terminal that keeps a shape the PTY refused renders GARBLED, not merely wrong-sized. Adopting can leave the pane wider than the screen, and `.terminal-container` is `overflow: hidden`, so `.pty-oversized` grants horizontal reach for exactly as long as the mismatch lasts. ⚠️ That rule sets **both** overflow axes and its own `touch-action`: mobile.css loads later and sets `.terminal-container { overflow: visible; touch-action: none }`, and a bare `overflow-x` would leave overflow-y computing to `auto`, handing the browser a vertical scroll container the terminal's touch handler does not know about. **Verified in Chrome at 430px** against a live server with a desktop client holding the claim: the phone adopts 198x43, `overflow-x: auto` / `overflow-y: hidden` / `touch-action: pan-x`, 758px reachable to the right, and the terminal's own vertical scroll still works. + **Terminal resilience: replay clears, renderer liveness, fetch deadlines**: three rules that each close a way the terminal silently stops being correct. ⚠️ **A replay clear MUST be in-stream, never `reset()`/`clear()`.** xterm's `write()` is asynchronously queued while `Terminal.reset()` is synchronous and, per upstream, "does not clear input buffers and does not reset the parser" — so bytes queued just before a reset are parsed AFTER it and fuse into the snapshot written next. **Measured** against the real xterm in this repo: `write('p8'); reset(); write('rmissions')` renders `p8rmissions`; the queued `\x1bc` renders `rmissions` and clears scrollback. `_resetTerminalForReplay()` (app.js) is the ONE clear, a single queued `\x1bc` (RIS), and all three replay paths go through it; RIS rather than `\x1b[3J\x1b[H\x1b[2J` because the erase leaves modes, charsets, scroll regions and SGR state alone. Callers may still chunk the content — ordering in the queue is what matters, not writing it in one call. ⚠️ **The renderer watchdog reads xterm privates and CANNOT be covered by the gate.** `_kickRenderer()` (terminal-ui.js) cancels a stale `_core._renderService._renderDebouncer._animationFrame` and forces a repaint. **Verified against xterm 6.0.0** (jsdom, after `open()`): the field path resolves, a forced stale handle genuinely makes `refreshRows` a no-op, and the kick schedules a fresh frame. **Reasoned, not reproduced here**: the premise that iOS discards scheduled rAF callbacks when a PWA backgrounds, which is what leaves the handle stale — that half wants a real-device pass. Codeman has exactly ONE xterm for the whole page load, so one backgrounding would wedge it until a reload. `_renderService` only exists after `open()`, which needs a real DOM, and the gate runs in node — so `test/xterm-private-api.test.ts` pins the RESOLVED lockfile version (not the `^6.0.0` range, which a real upgrade slips through) and a bump means re-verifying by hand. Every access is optional-chained on purpose: a renamed field must degrade to a no-op, never throw on a 2s timer. ⚠️ **Every terminal capture carries a deadline, and the helper reads the BODY** (`_fetchTerminalCapture`, app.js). `await fetch()` settles on response HEADERS, so clearing the timer there leaves the body — the multi-megabyte `?full=1` capture this exists for — unbounded: **measured** at 4026ms under a 1000ms deadline before the fix. The helper therefore returns `{json, headers, headersAt}` rather than a `Response`, and `_terminalCaptureInflight` is scoped the same way so a body still streaming counts toward a capture starting beside it. It degrades to a plain fetch where `AbortController` is missing — the deadline is a safety net, not a dependency. Tests: `test/terminal-resilience.test.ts` (pure decisions), `test/xterm-private-api.test.ts`. -**WebSocket output-gap reconcile** (`_wsOutputGapSession`, app.js): terminal OUTPUT frames carry no sequence number (input frames do — `seq`+`cid`, at-most-once, ACKed), so a dropped socket leaves a hole nothing replays. ⚠️ **The gap is narrower than "the device went offline"**: if the network drops, SSE drops with it and `handleInit`'s keepTerminal branch already calls `_onSessionNeedsRefresh`. The uncovered case is the WS dying while SSE stays up (half-open socket, proxy idle-timeout, ping timeout), because `_onSSETerminal` discards every SSE terminal frame while `_wsReady` is true and `_wsReady` only flips in `ws.onclose`. Reaching `onclose` at all means the drop was unintentional (`_disconnectWs` nulls the handler first), so the session is marked and the next successful open reconciles. ⚠️ **The marker must be cleared by EVERY path that repaints that session's buffer** — `_markTerminalBufferReconciled()` is called from `_onSessionNeedsRefresh`'s finally, from `selectSession` after its load, and from `_cleanupSessionData`. `selectSession` loads the buffer and only THEN calls `_connectWs`, so without that clear the socket opening afterwards replays the whole buffer a second time on top of the one just written. Sequencing the output frames is the real fix and is not done. This is reasoned from the code path, not observed on a device. +**WebSocket output-gap reconcile** (`_wsOutputGapSession`, app.js): terminal OUTPUT frames carry no sequence number (input frames do — `seq`+`cid`, at-most-once, ACKed), so a dropped socket leaves a hole nothing replays. ⚠️ **The gap is narrower than "the device went offline"**: if the network drops, SSE drops with it and `handleInit`'s keepTerminal branch already calls `_onSessionNeedsRefresh`. The uncovered case is the WS dying while SSE stays up (half-open socket, proxy idle-timeout, ping timeout), because `_onSSETerminal` discards every SSE terminal frame while `_wsReady` is true and `_wsReady` only flips in `ws.onclose`. Reaching `onclose` at all means the drop was unintentional (`_disconnectWs` nulls the handler first), so the session is marked and the next successful open reconciles. ⚠️ **The marker must be cleared by every path that repaints that session's buffer, and ONLY once one actually has.** `_markTerminalBufferReconciled()` is called from `selectSession` after its load, from `_cleanupSessionData`, and from `_onSessionNeedsRefresh` **at the repaint itself, not in its `finally`** — clearing on every exit meant a reconcile that threw, or hit the fetch deadline (the flaky link the marker exists for), dropped the gap with nothing to retry it. `ws.onopen` no longer clears it up front either, so a reconcile that is skipped or fails is tried again on the next open; re-entry is safe because `_terminalRefreshOwner` makes a second reconcile for the same session a no-op. `selectSession` loads the buffer and only THEN calls `_connectWs`, so without that clear the socket opening afterwards replays the whole buffer a second time on top of the one just written. Sequencing the output frames is the real fix and is not done. This is reasoned from the code path, not observed on a device. **Service worker: precache and cache key are BUILD-GENERATED** (`sw.js` + `scripts/build.mjs`): the build content-hashes assets and rewrites two exact declarations in `sw.js` — `const BUILD_ID = 'dev';` and `const HASHED_ASSETS = [];`. ⚠️ **Each must appear exactly once or the build THROWS**, which is deliberate: the list used to be hand-maintained with PRE-hash names, so every entry 404'd in production and `cache.add().catch(() => {})` hid it (15 of 23 verified failing against a running instance). The dev literals are valid on their own, so dev serves an unrewritten worker with an empty precache. ⚠️ **`caches.match` must pass `ignoreSearch: true`**: `renderIndexHtml` runs `cacheBustAssets`, which appends `?v=` to every same-origin `.js`/`.css` reference INCLUDING content-hashed names, so the page requests `/app..js?v=` while the cache holds `/app..js`. Without it no precached entry is reachable and the install downloads ~1.3MB that can never be served — once per deploy, since `CACHE_NAME` now carries the build id. That per-build key is what makes `activate`'s cleanup actually delete anything; it used to be the constant `'codeman-v1'`, so assets from every past release accumulated forever. Contract pinned by `test/sw-precache-manifest.test.ts`, which PARSES the `HASHABLE` list out of `build.mjs` rather than copying it. diff --git a/src/session.ts b/src/session.ts index fdad22f8..2b448c92 100644 --- a/src/session.ts +++ b/src/session.ts @@ -3779,6 +3779,22 @@ export class Session extends EventEmitter { private _ptyCols = 120; private _ptyRows = 40; + /** + * The geometry the CLI is actually drawing for. + * + * Exposed because `resize()` can decline a request outright (arbitration + * below) and the asking client has no other way to find out: a browser + * terminal that keeps a shape the PTY refused renders garbled output rather + * than wrong-sized output, because Claude Code's repaints are computed from + * the width it was told (issue #464). Both transports report these back. + */ + get ptyCols(): number { + return this._ptyCols; + } + get ptyRows(): number { + return this._ptyRows; + } + /** * Live WebSocket connections that have announced a desktop viewport for this * session. While at least one is registered, small-viewport (mobile/tablet) @@ -3864,6 +3880,10 @@ export class Session extends EventEmitter { } if (isSmallViewport && this._desktopSizeClaims.size > 0) { if (Date.now() - this._lastDesktopActivityAt < Session.DESKTOP_CLAIM_IDLE_MS) { + // Declined. The caller is told nothing here on purpose — the decision + // belongs to the session, not the socket — but the caller MUST report + // `ptyCols`/`ptyRows` back afterwards so the asking client can adopt + // the shape it did not get. Both transports do; see issue #464. return; } this._mobileSizeOverride = true; diff --git a/src/web/public/app.js b/src/web/public/app.js index 8da86ff5..09b9e58c 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -73,10 +73,15 @@ const _crashDiag = { // the storage quota and silently kill every later breadcrumb. Flatten and // cap. CodemanDiag is loaded before app.js, but guard anyway — a // diagnostic that can throw is worse than no diagnostic. - const flat = - typeof CodemanDiag !== 'undefined' && CodemanDiag.sanitizeDiagEntry - ? CodemanDiag.sanitizeDiagEntry(msg) - : String(msg == null ? '' : msg).replace(/[\r\n\u2028\u2029]+/g, ' ').slice(0, 300); + // Bound to a local FIRST: `CodemanDiag?.x` still throws a ReferenceError + // when the identifier was never declared, and this is the one function in + // the app that must never throw. + const diag = typeof CodemanDiag !== 'undefined' ? CodemanDiag : null; + const flat = diag?.sanitizeDiagEntry + ? diag.sanitizeDiagEntry(msg) + : String(msg == null ? '' : msg) + .replace(/[\r\n\u2028\u2029]+/g, ' ') + .slice(0, diag?.DIAG_ENTRY_MAX_CHARS ?? 300); const entry = `${new Date().toISOString().slice(11,23)} ${flat}`; this._entries.push(entry); if (this._entries.length > this._maxEntries) this._entries.shift(); @@ -2451,8 +2456,14 @@ class CodemanApp { // placeholder beats a messy screen dump there. const sessionMode = this.sessions.get(this.activeSessionId)?.mode || 'claude'; if (!lastResponse && (sessionMode === 'claude' || sessionMode === 'shell')) { - const termRes = await fetch(`/api/sessions/${this.activeSessionId}/terminal`); - const termData = (await termRes.json())?.data ?? {}; + // The no-param form is capped only by `terminalBufferMaxBytes` (32MB by + // default), so it is the largest body the frontend asks for anywhere — + // it gets the full-history budget, not the tail one. + const termCapture = await this._fetchTerminalCapture( + `/api/sessions/${this.activeSessionId}/terminal`, + { full: true } + ); + const termData = termCapture.json?.data ?? {}; if (termData.terminalBuffer) { lastResponse = this._cleanTerminalBuffer(termData.terminalBuffer); } @@ -2711,6 +2722,13 @@ class CodemanApp { // terminal sat at the bottom of a just-rewritten buffer, so the next // flush would scroll back down and undo the restore above. this._syncStickyScrollBaseline(); + // ⚠️ HERE, not in the `finally`. The marker means "this session lost + // output", and only a repaint that actually happened settles it. Clearing + // on every exit meant a reconcile that threw — or hit the new fetch + // deadline, which is the flaky-link case the marker exists for — dropped + // the gap silently, and nothing ever retried it. Left set, the next + // ws.onopen has another go. + this._markTerminalBufferReconciled(sessionId); // Re-position local echo overlay at new prompt location this._localEchoOverlay?.rerender(); // Resize PTY to match actual browser dimensions (critical for OpenCode @@ -2723,11 +2741,6 @@ class CodemanApp { console.error('needsRefresh reload failed:', err); } finally { if (this._terminalRefreshOwner === refreshOwner) this._terminalRefreshOwner = null; - // Any completed reload for this session IS the reconcile, whoever asked - // for it — handleInit's SSE-reconnect branch and selectSession both land - // here or do the same work. Leaving the marker set would make the next - // ws.onopen replay the whole buffer a second time. - this._markTerminalBufferReconciled(sessionId); } } @@ -2752,7 +2765,10 @@ class CodemanApp { // Fetch buffer, clear terminal, write buffer, resize (no Ctrl+L needed) try { - const capture = await this._fetchTerminalCapture(`/api/sessions/${data.id}/terminal`); + // No-param capture: `terminalBufferMaxBytes` (32MB) is its only ceiling, + // so it needs the full-history budget. Defaulting to the tail budget + // gave the largest payload the smallest deadline. + const capture = await this._fetchTerminalCapture(`/api/sessions/${data.id}/terminal`, { full: true }); const headersReceivedAt = capture.headersAt; const termData = capture.json?.data ?? {}; @@ -3087,7 +3103,11 @@ class CodemanApp { // Only after an unintentional close — a first connect has no gap, and // refetching there would duplicate the buffer selectSession just wrote. if (this._wsOutputGapSession === sessionId) { - this._wsOutputGapSession = null; + // NOT cleared here. `_onSessionNeedsRefresh` clears it once it has + // actually repainted; a reconcile that fails or is skipped (a buffer + // load already in flight, a tab switch) leaves the marker set so the + // next open retries. Re-entry is safe: `_terminalRefreshOwner` makes + // a second reconcile for the same session a no-op. _crashDiag.log(`WS REOPEN: reconciling output gap for ${sessionId}`); // Fire-and-forget: this is recovery, and a failure here must not stop // the socket coming up. _onSessionNeedsRefresh already guards against @@ -3116,6 +3136,10 @@ class CodemanApp { // Input ACK — the server applied (or deduped) this seq; drop it from // the durable queue so it can never be re-delivered/lost. this._onWsInputAck(msg.seq, msg); + } else if (msg.t === 'zc') { + // Resize confirm — the geometry the PTY actually holds, which is not + // always the one this client asked for (issue #464). + this._onPtyGeometryReport(sessionId, msg.c, msg.r); } } catch { // Ignore malformed messages @@ -6482,11 +6506,14 @@ class CodemanApp { // For that just-created-session case we flush (not discard) queued SSE events. let bufferWasEmpty = false; let cacheResetAndParseMs = 0; + // Hoisted out of the try: the catch needs to know whether the pane was + // blanked before the fetch, because only then is there nothing on screen. + let clearedBeforeFresh = false; try { // Fit terminal to container BEFORE writing any buffer data. // If the browser was resized while viewing another session, the terminal // canvas may be at stale dimensions — content would render at wrong width. - if (this.fitAddon) this.fitAddon.fit(); + this.syncTerminalGeometry(); // Also push the new dimensions to the PTY. Without this, codex/codeman // sees the size that was set the last time the throttled resize handler @@ -6563,7 +6590,8 @@ class CodemanApp { // blank and rewrites with fresh data. Skip the cache and write the fresh // buffer once for a single clean transition. const cachedBuffer = this.terminalBufferCache.get(sessionId); - let clearedBeforeFresh = false; + // `clearedBeforeFresh` is declared above the try, because the catch reads + // it — re-declaring it here would shadow that and silently break it. if (cachedBuffer && !sessionIsBusy && !restoredSnapshot && session?.mode !== 'shell') { _crashDiag.log(`CACHE_WRITE: ${(cachedBuffer.length/1024).toFixed(0)}KB`); this._setTerminalLoadState(sessionId, selectGen, 'replaying'); @@ -6612,12 +6640,26 @@ class CodemanApp { const useFullHistory = session?.mode !== 'shell' && !this._fullHistoryLoaded.has(sessionId); if (useFullHistory) this._fullHistoryLoaded.add(sessionId); const fetchStartedAt = performance.now(); - const capture = await this._fetchTerminalCapture( - useFullHistory - ? `/api/sessions/${sessionId}/terminal?full=1` - : `/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}`, - { full: useFullHistory } - ); + const tailUrl = `/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}`; + let capture; + try { + capture = await this._fetchTerminalCapture( + useFullHistory ? `/api/sessions/${sessionId}/terminal?full=1` : tailUrl, + { full: useFullHistory } + ); + } catch (err) { + // The deadline made a slow link reachable for the first time, and the + // pane was already blanked above — so an abort here used to leave a + // black rectangle, discard the queued live output, and never reach + // `_connectWs`. Degrade to the bounded tail instead: less history, but + // a working tab. Only for the full-history pull; the tail has nothing + // smaller to fall back to, and a second failure is the honest floor. + if (err?.name !== 'AbortError' || !useFullHistory) throw err; + _crashDiag.log('FULL CAPTURE ABORTED → tail'); + // It never loaded, so the next select must be allowed to try again. + this._fullHistoryLoaded.delete(sessionId); + capture = await this._fetchTerminalCapture(tailUrl); + } const headersReceivedAt = capture.headersAt; if (this._isStaleSelect(selectGen)) { this._clearTerminalLoadState(sessionId, selectGen); @@ -6819,17 +6861,28 @@ class CodemanApp { // and, because it goes through `forceReload`, a dropped and reopened // WebSocket plus a deleted xterm snapshot. // - // That equality is the signature of a CLAMP rather than a race. - // `getTerminalDimensions()` floors at 40x10 while `fitAddon.fit()` does - // not, so a terminal narrower than 40 columns or shorter than 10 rows - // reports a pane permanently bigger than itself, and every select would - // retry without ever converging. A race never produces this equality: its - // whole premise is that the pane was still at the size we asked it to - // leave. The other non-converging case, `Session.resize` declining a - // small viewport while a desktop claim is live, does not produce it - // either — that pane sits at the DESKTOP's size — so it still costs the - // one capped attempt, and stopping it needs the pane-ownership policy - // this does not touch. + // A race never produces this equality: its whole premise is that the pane + // was still at the size we asked it to leave. So the equality means the + // pane already IS what we asked for and a retry would capture the same + // frame twice. + // + // ⚠️ This used to also be the signature of a CLAMP, and that is now fixed + // at the source rather than worked around here (issue #464). + // `getTerminalDimensions()` floors at 40x10 while `fitAddon.fit()` did + // not, so a terminal under 40 columns or 10 rows reported a pane + // permanently bigger than itself and every select retried without ever + // converging. `syncTerminalGeometry()` now applies that floor to xterm as + // well, so the browser terminal IS the size it reports and the clamp can + // no longer manufacture a mismatch — which also means the repair below is + // reached only by cases it can actually repair. + // + // The other non-converging case, `Session.resize` declining a small + // viewport while a desktop claim is live, does not produce this equality + // either — that pane sits at the DESKTOP's size. It no longer needs + // repairing from here: the server reports the geometry the PTY actually + // holds ({"t":"zc"} / the resize response) and `_onPtyGeometryReport` + // adopts it, so the terminal matches the pane that is being drawn instead + // of replaying against one that never existed. const captureMatchesRequestedSize = !!dimsAfterLoad && data.captureCols === dimsAfterLoad.cols && data.captureRows === dimsAfterLoad.rows; @@ -6996,8 +7049,29 @@ class CodemanApp { } catch (err) { if (this._isLoadingBuffer) this._finishBufferLoad(bufferLoadOwner); this._restoringFlushedState = false; - this._setTerminalLoadState(sessionId, selectGen, 'failed'); console.error('Failed to load session terminal:', err); + if (this._isStaleSelect(selectGen)) { + this._clearTerminalLoadState(sessionId, selectGen); + return; + } + // The history did not load. That is not a reason to leave the tab dead: + // ⚠️ the socket is what carries LIVE output, and it is opened at the end + // of the happy path, so bailing here left the session mute until the user + // switched away and back. + this._connectWs(sessionId); + // Only when the pane was blanked for a replay that never came. A pane + // still holding its previous content is stale, not empty, and stacking a + // notice on top of readable output is worse than the staleness. + if (clearedBeforeFresh && this.terminal) { + this.terminal.write( + '\r\n\x1b[2m Could not load this session\u2019s history. Live output continues below.\x1b[0m\r\n' + ); + } + // ⚠️ CLEAR, not 'failed'. `_setTerminalLoadState` only marks the TAB, and + // nothing ever cleared it on this path — so the tab kept its spinner and + // `aria-busy="true"` forever, telling every reader and every screen reader + // that a load was still running when it had already given up. + this._clearTerminalLoadState(sessionId, selectGen); } } diff --git a/src/web/public/constants.js b/src/web/public/constants.js index 75d389bd..1ddf0a46 100644 --- a/src/web/public/constants.js +++ b/src/web/public/constants.js @@ -1673,6 +1673,69 @@ function sanitizeDiagEntry(msg) { .slice(0, DIAG_ENTRY_MAX_CHARS); } +// ── Terminal geometry: xterm and the PTY must never disagree ─────────────── +// +// Issue #464 ("text gets muffled"). Claude Code's TUI repaints by wrapping its +// frame at the width the PTY reported and walking the cursor up that many +// ROWS. So a browser terminal whose width differs from the PTY's makes every +// repaint arithmetic wrong: a logical line occupies more physical rows than +// Ink counted, `eraseLines(n)` clears too few of them, and the new frame paints +// over rows that were never erased. Measured against a real xterm — a PTY +// believing 120 columns against a 62-column terminal renders each wrapped line +// twice, and a shorter replacement line leaves the tail of the old one behind. +// That is exactly the doubled rows and half-overwritten prose in the report. +// +// The floor exists because a PTY a handful of columns wide makes any CLI wrap +// every word; it is NOT a display preference, so the browser terminal has to +// honour it too. Three separate call sites used to fit xterm to the RAW +// proposal and report the CLAMPED one, which is how the two drifted apart with +// nothing to notice: resize is write-only, so nobody could see the disagreement. +const TERMINAL_MIN_COLS = 40; +const TERMINAL_MIN_ROWS = 10; + +/** + * The geometry to apply AND report — there is only ever one answer to both. + * @param {{cols: number, rows: number}|null|undefined} proposed + * @returns {{cols: number, rows: number}|null} + */ +function clampTerminalDimensions(proposed) { + if (!proposed || !Number.isFinite(proposed.cols) || !Number.isFinite(proposed.rows)) return null; + return { + cols: Math.max(Math.trunc(proposed.cols), TERMINAL_MIN_COLS), + rows: Math.max(Math.trunc(proposed.rows), TERMINAL_MIN_ROWS), + }; +} + +/** Whether two geometries are the same screen. Either being absent is a mismatch. */ +function terminalGeometryAgrees(a, b) { + return !!a && !!b && a.cols === b.cols && a.rows === b.rows; +} + +/** + * What to do when the server reports the PTY's real geometry. + * + * The server is the authority: it owns the PTY the CLI is drawing for, and it + * can refuse a resize outright (`Session.resize` ignores small-viewport + * requests while a desktop connection holds an active sizing claim) without + * the asking client ever being told. A terminal that keeps its own shape after + * such a refusal renders garbage; one that adopts the PTY's shape renders the + * truth, and may simply be wider than the screen can show. + * + * Correct-and-reachable beats correct-and-clipped beats garbled, so a pane + * wider than the viewport also earns horizontal reach — see `.pty-oversized`. + * + * @param {{cols: number, rows: number}|null} local - what xterm currently holds + * @param {{cols: number, rows: number}|null} pty - what the server just reported + * @returns {{adopt: boolean, oversized: boolean}} + */ +function reconcilePtyGeometry(local, pty) { + if (!pty || !Number.isFinite(pty.cols) || !Number.isFinite(pty.rows)) { + return { adopt: false, oversized: false }; + } + if (terminalGeometryAgrees(local, pty)) return { adopt: false, oversized: false }; + return { adopt: true, oversized: !!local && pty.cols > local.cols }; +} + if (typeof window !== 'undefined') { window.CodemanHistoryFormat = { formatHistoryBytes, computeHistoryTruncationNotice, computeRewriteScrollLine }; window.CodemanFilePaths = { absoluteFilePathPattern, previewsInFileViewer, FILE_PREVIEW_EXTENSIONS }; @@ -1690,4 +1753,11 @@ if (typeof window !== 'undefined') { FETCH_DEADLINE_MAX_MS, }; window.CodemanDiag = { sanitizeDiagEntry, DIAG_ENTRY_MAX_CHARS }; + window.CodemanTerminalGeometry = { + clampTerminalDimensions, + terminalGeometryAgrees, + reconcilePtyGeometry, + TERMINAL_MIN_COLS, + TERMINAL_MIN_ROWS, + }; } diff --git a/src/web/public/mobile-handlers.js b/src/web/public/mobile-handlers.js index 7d815f70..e06e4c60 100644 --- a/src/web/public/mobile-handlers.js +++ b/src/web/public/mobile-handlers.js @@ -581,11 +581,10 @@ const KeyboardHandler = { this._settleRestoreScroll = false; if (typeof app !== 'undefined' && app.terminal) { - if (app.fitAddon) { - try { - app.fitAddon.fit(); - } catch {} - } + // Floored fit, not a bare fitAddon.fit(): _shrinkPaddingToFit measures + // the leftover gap under the LAST row, so it has to run against the + // geometry xterm will actually keep (issue #464). + app.syncTerminalGeometry?.(); if (this.keyboardVisible) this._shrinkPaddingToFit(); // Following live output → bottom, as before. Reading history → back to // the pre-reflow anchor instead of being yanked down (#259). @@ -601,25 +600,23 @@ const KeyboardHandler = { }, this.VIEWPORT_SETTLE_MS); }, - /** Send current terminal dimensions to the server (one-shot, for keyboard open/close) */ + /** + * Send the settled terminal dimensions to the server (one-shot, for keyboard + * open/close — `throttledResize` deliberately holds the PTY's shape for the + * whole animation, so this is what stops it going stale). + * + * ⚠️ Delegates rather than computing its own numbers. This used to re-read + * `proposeDimensions()` and floor only what it POSTed, so on a phone with the + * keyboard up — where the proposal is routinely under ten rows — the PTY was + * told ten and xterm kept six, which is the #464 divergence. Worse, it read + * the proposal AFTER `_shrinkPaddingToFit()` had moved the container, so even + * unfloored its answer could differ from the fit above it. `sendResize` fits, + * floors and applies in one step, and additionally gets the WS fast path and + * the detached-session yield this hand-rolled POST never had. + */ _sendTerminalResize() { - if (typeof app === 'undefined' || !app.activeSessionId || !app.fitAddon) return; - try { - const dims = app.fitAddon.proposeDimensions(); - if (dims) { - const cols = Math.max(dims.cols, 40); - const rows = Math.max(dims.rows, 10); - app._lastResizeDims = { cols, rows }; - // Declare the viewport type so resize arbitration can ignore this - // while a desktop connection is sizing the same session. - const viewportType = MobileDetection.getDeviceType ? MobileDetection.getDeviceType() : 'mobile'; - fetch(`/api/sessions/${app.activeSessionId}/resize`, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ cols, rows, viewportType }), - }).catch(() => {}); - } - } catch {} + if (typeof app === 'undefined' || !app.activeSessionId) return; + app.sendResize?.(app.activeSessionId)?.catch?.(() => {}); }, /** @@ -676,10 +673,8 @@ const KeyboardHandler = { const currentPadding = parseInt(main.style.paddingBottom) || 0; const floor = Math.min(currentPadding, this._fixedBottomBarsHeight()); main.style.paddingBottom = Math.max(floor, currentPadding - gap) + 'px'; - if (app.fitAddon) - try { - app.fitAddon.fit(); - } catch {} + // Floored, like every other fit of the main terminal (#464). + app.syncTerminalGeometry?.(); } } catch {} }, diff --git a/src/web/public/notification-manager.js b/src/web/public/notification-manager.js index c1f7f665..d13f02a0 100644 --- a/src/web/public/notification-manager.js +++ b/src/web/public/notification-manager.js @@ -512,8 +512,10 @@ class NotificationManager { } // Re-fit terminal and send resize to PTY so this client's dimensions win. // Fixes broken layout when switching between desktop and mobile on the same session. - if (this.app?.fitAddon && this.app?.activeSessionId) { - this.app.fitAddon.fit(); + // sendResize fits (floored) as its first synchronous step, so the bare + // fit that used to precede it was both redundant and a chance to leave + // xterm at the unfloored proposal (#464). + if (this.app?.activeSessionId) { this.app.sendResize(this.app.activeSessionId); } } diff --git a/src/web/public/panels-ui.js b/src/web/public/panels-ui.js index 85f0a61e..e3666fb9 100644 --- a/src/web/public/panels-ui.js +++ b/src/web/public/panels-ui.js @@ -5276,8 +5276,10 @@ Object.assign(CodemanApp.prototype, { try { localStorage.removeItem('codeman-active-session'); } catch {} this.renderSessionTabs(); this.renderMuxSessions(); - this.terminal.clear(); - this.terminal.reset(); + // Not a replay path, so the ordering hazard does not apply here — but + // there is one way to clear this terminal and this is it, so a future + // caller cannot copy a clear()+reset() pair out of here into one. + this._resetTerminalForReplay(); this.toast('All sessions and tmux killed', 'success'); } } else { @@ -5286,8 +5288,7 @@ Object.assign(CodemanApp.prototype, { this.activeSessionId = null; try { localStorage.removeItem('codeman-active-session'); } catch {} this.renderSessionTabs(); - this.terminal.clear(); - this.terminal.reset(); + this._resetTerminalForReplay(); this.toast('All tabs removed, tmux still running', 'info'); } } catch (err) { diff --git a/src/web/public/ralph-panel.js b/src/web/public/ralph-panel.js index 5dc8c630..85f9a5d9 100644 --- a/src/web/public/ralph-panel.js +++ b/src/web/public/ralph-panel.js @@ -155,10 +155,9 @@ Object.assign(CodemanApp.prototype, { if (xtermViewport && scrollTop !== undefined) { xtermViewport.scrollTop = scrollTop; } - // Refit terminal to new container size - if (this.terminal && this.fitAddon) { - this.fitAddon.fit(); - } + // Refit terminal to new container size. Through the one owner so the + // floor that is reported to the PTY is also the one xterm holds (#464). + this.syncTerminalGeometry?.(); }); }, diff --git a/src/web/public/settings-ui.js b/src/web/public/settings-ui.js index 975efa77..af2120f0 100644 --- a/src/web/public/settings-ui.js +++ b/src/web/public/settings-ui.js @@ -3097,7 +3097,7 @@ Object.assign(CodemanApp.prototype, { const changed = orientationChanged || previousDetail !== detail || previousSort !== sort; if (orientationChanged) { this.updateTabOverflowMode?.(); - if (!settleRailWidth) this.fitAddon?.fit(); + if (!settleRailWidth) this.syncTerminalGeometry?.(); } // applyTabWrapSettings() is the ONE owner of tabs-show-folder and is // rail-aware, so it has to run AFTER the two attributes above — the diff --git a/src/web/public/styles.css b/src/web/public/styles.css index e0cc8e4a..44627912 100644 --- a/src/web/public/styles.css +++ b/src/web/public/styles.css @@ -3817,6 +3817,42 @@ body.solo-mode .btn-lifecycle-log { background: transparent !important; } +/* A PTY wider than this screen (issue #464). Another device holds the session's + sizing claim, so the browser terminal has adopted the PTY's width: the text is + rendered CORRECTLY, it simply does not fit. Without horizontal reach the right + columns sit behind .terminal-container's overflow:hidden with no gesture that + can get to them — correct-but-unreachable is no better than garbled. + Present only while the mismatch is; _setPtyOversized() owns the class. */ +.terminal-container.pty-oversized { + /* ⚠️ BOTH axes, explicitly, and touch-action here rather than only on the + .touch-device variant below. mobile.css loads after this file and sets + `.terminal-container { overflow: visible; touch-action: none }` — a bare + `overflow-x` would then leave overflow-y computing to `auto` (CSS promotes + a `visible` paired with a non-visible axis), handing the browser a vertical + scroll container the terminal's own touch handler does not know about. */ + overflow-x: auto; + overflow-y: hidden; + /* pan-x ONLY: the terminal's touchmove handler still owns vertical scrolling. */ + touch-action: pan-x; +} +/* xterm's own element is width:100% above, so the container would see no + overflow to scroll even though .xterm-screen is wider than both. */ +.terminal-container.pty-oversized .xterm { + width: max-content; + min-width: 100%; +} +/* The inner elements carry touch-action: none of their own (both here and in + mobile.css), so the container's pan-x is not enough on its own. */ +.touch-device .terminal-container.pty-oversized, +.touch-device .terminal-container.pty-oversized .xterm, +.touch-device .terminal-container.pty-oversized .xterm-viewport, +.touch-device .terminal-container.pty-oversized .xterm-screen, +.terminal-container.pty-oversized .xterm, +.terminal-container.pty-oversized .xterm-viewport, +.terminal-container.pty-oversized .xterm-screen { + touch-action: pan-x; +} + /* Touch devices: prevent browser from claiming the touch gesture before our JS touchmove handler fires. Without this, the browser starts native scrolling during the first few px of finger travel and ignores our diff --git a/src/web/public/tab-rail-resize.js b/src/web/public/tab-rail-resize.js index ab2e54ab..17e99ea5 100644 --- a/src/web/public/tab-rail-resize.js +++ b/src/web/public/tab-rail-resize.js @@ -154,7 +154,7 @@ Object.assign(CodemanApp.prototype, { this._persistTabRailWidth(preferred); try { if (this.activeSessionId && this.sendResize) await this.sendResize(this.activeSessionId); - else this.fitAddon?.fit(); + else this.syncTerminalGeometry?.(); this._updateConnectionLinesImmediate?.(); } catch (error) { console.warn('Failed to resize terminal after rail resize:', error); diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index 3f8a2f8b..ef008af2 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -587,12 +587,12 @@ Object.assign(CodemanApp.prototype, { if (isMobileSafari) { // Wait for layout, then fit multiple times to ensure proper sizing requestAnimationFrame(() => { - this.fitAddon.fit(); + this.syncTerminalGeometry(); // Double-check after another frame - requestAnimationFrame(() => this.fitAddon.fit()); + requestAnimationFrame(() => this.syncTerminalGeometry()); }); } else { - this.fitAddon.fit(); + this.syncTerminalGeometry(); } // Whenever that first fit runs — on this line, or a frame or two later on // the mobile-Safari branch above — it measures whatever font the browser has @@ -999,10 +999,6 @@ Object.assign(CodemanApp.prototype, { this._resizeTimeout = null; this._lastResizeDims = null; - // Minimum terminal dimensions to prevent vertical text wrapping - const MIN_COLS = 40; - const MIN_ROWS = 10; - const throttledResize = () => { if (this._tabRailResizeOwnsObserver) return; // Trailing-edge debounce: ALL resize work (fit + clear + SIGWINCH) happens @@ -1022,10 +1018,6 @@ Object.assign(CodemanApp.prototype, { } this._resizeTimeout = setTimeout(() => { this._resizeTimeout = null; - // Fit xterm.js to final container dimensions - if (this.fitAddon) { - this.fitAddon.fit(); - } // Flush any stale flicker buffer before clearing viewport if (this.flickerFilterBuffer) { if (this.flickerFilterTimeout) { @@ -1034,24 +1026,35 @@ Object.assign(CodemanApp.prototype, { } this.flushFlickerBuffer(); } - // Skip server resize while mobile keyboard is visible — sending SIGWINCH - // causes Ink to re-render at the new row count, garbling terminal output. - // Local fit() still runs so xterm knows the viewport size for scrolling. + // Hold the PTY's shape while the virtual keyboard is up: a SIGWINCH per + // step of the OS animation makes Ink re-render at a row count that is + // about to change again, and shifts the accessory toolbar mid-typing. + // KeyboardHandler's settle timer sends ONE resize once the animation + // stops (`_sendTerminalResize`), so the PTY is not left stale. const keyboardUp = typeof KeyboardHandler !== 'undefined' && KeyboardHandler.keyboardVisible; // Same yield as sendResize: never resize a PTY whose session is showing // in its own window. Dragging the dashboard's border must not reshape it. const detachedElsewhere = !this.isSoloWindow && this.detachedSessions?.has(this.activeSessionId); - if (this.activeSessionId && !keyboardUp && !detachedElsewhere) { - const dims = this.fitAddon.proposeDimensions(); - // Enforce minimum dimensions to prevent layout issues - const cols = dims ? Math.max(dims.cols, MIN_COLS) : MIN_COLS; - const rows = dims ? Math.max(dims.rows, MIN_ROWS) : MIN_ROWS; + // ⚠️ Whether to fit is the SAME question as whether to send (issue #464). + // This block used to fit unconditionally and skip only the SIGWINCH, + // which is the one combination that cannot be right: it moves xterm to + // a shape the PTY is never told about, and Claude Code computes its + // repaints from the shape it was told. Withhold both, or neither — + // a reflow nothing is rendering for buys nothing and costs correctness. + const dims = this.activeSessionId && !keyboardUp && !detachedElsewhere ? this.syncTerminalGeometry() : null; + // ⚠️ A null measurement is NOT a reason to report the floor. It used to + // fall back to a bare 40x10, which tells the PTY a shape nothing measured + // and xterm does not hold — the write-only guess this whole change exists + // to remove. An unmeasurable terminal has nothing to say; the next + // resize event says it. + if (dims) { + const { cols, rows } = dims; // Only send resize if dimensions actually changed if (!this._lastResizeDims || cols !== this._lastResizeDims.cols || rows !== this._lastResizeDims.rows) { // Clear viewport + scrollback ONLY when dimensions actually change. - // fitAddon.fit() reflows content: lines at old width may wrap to more rows, - // pushing overflow into scrollback. Ink's cursor-up count is based on the - // pre-reflow line count, so ghost renders accumulate in scrollback. + // syncTerminalGeometry() reflowed content: lines at old width may wrap to + // more rows, pushing overflow into scrollback. Ink's cursor-up count is + // based on the pre-reflow line count, so ghost renders accumulate there. // Fix: \x1b[3J (Erase Saved Lines) clears scrollback reflow debris, // then \x1b[H\x1b[2J clears the viewport for a clean Ink redraw. // IMPORTANT: Only clear when we're actually sending SIGWINCH (dims changed). @@ -3380,11 +3383,13 @@ Object.assign(CodemanApp.prototype, { * is validated against xterm 6.x and CANNOT be covered by the CI gate: * `_renderService` is only constructed by `Terminal.open()`, which needs a * real DOM, and the gate runs in node. `test/xterm-private-api.test.ts` pins - * the dependency RANGE instead, so a major bump fails there and sends someone - * to re-check this by hand; `test/terminal-resilience.test.ts` covers the - * decision half. If the path ever goes stale the watchdog silently stops - * healing — that is the failure mode to watch for, and why the range guard - * exists at all. + * the RESOLVED lockfile version instead, so ANY bump fails there — not only a + * major — and sends someone to re-check this by hand; the declared `^6.0.0` + * range was the wrong assertion in both directions, since 6.4.0 could rename a + * private field while resolving inside it. `test/terminal-resilience.test.ts` + * covers the decision half. If the path ever goes stale the watchdog silently + * stops healing — that is the failure mode to watch for, and why the version + * guard exists at all. */ _startRenderLivenessWatchdog() { this._stopRenderLivenessWatchdog(); @@ -5232,7 +5237,7 @@ Object.assign(CodemanApp.prototype, { setFontSize(size) { this.terminal.options.fontSize = size; document.getElementById('fontSizeDisplay').textContent = size; - this.fitAddon.fit(); + this._refitAfterCellSizeChange(); localStorage.setItem('codeman-font-size', size); // Update overlay font cache and re-render at new cell dimensions this._localEchoOverlay?.refreshFont(); @@ -5261,9 +5266,9 @@ Object.assign(CodemanApp.prototype, { // without needing a tab switch. The fit below still runs, so the terminal // is never left unfitted if the wait is slow. this._terminalFontReady = this._awaitTerminalFont().then(() => { - if (this.terminal?.options?.fontFamily === resolved) this.fitAddon?.fit(); + if (this.terminal?.options?.fontFamily === resolved) this._refitAfterCellSizeChange(); }); - this.fitAddon?.fit(); + this._refitAfterCellSizeChange(); this._localEchoOverlay?.refreshFont(); this._predictiveEcho?.refreshFont(); if (this._splitPane?.terminal) { @@ -5306,9 +5311,9 @@ Object.assign(CodemanApp.prototype, { // rasterized yet. Re-arm the wait and fit again once it settles; the fit // below still runs, so the terminal is never left unfitted. this._terminalFontReady = this._awaitTerminalFont().then(() => { - if (this.terminal?.options?.fontWeight === fontWeight) this.fitAddon?.fit(); + if (this.terminal?.options?.fontWeight === fontWeight) this._refitAfterCellSizeChange(); }); - this.fitAddon?.fit(); + this._refitAfterCellSizeChange(); this._localEchoOverlay?.refreshFont(); this._predictiveEcho?.refreshFont(); for (const [, entry] of this.teammateTerminals || []) { @@ -5399,19 +5404,95 @@ Object.assign(CodemanApp.prototype, { }, /** - * Get terminal dimensions with minimum enforcement. - * Prevents extremely narrow terminals that cause vertical text wrapping. + * The geometry this terminal would report right now, floors applied. + * Reads only — `syncTerminalGeometry()` is what makes it true of xterm. * @returns {{cols: number, rows: number}|null} */ getTerminalDimensions() { - const MIN_COLS = 40; - const MIN_ROWS = 10; - const dims = this.fitAddon?.proposeDimensions(); + // Never throws. `proposeDimensions()` reads a rendered element and throws + // on a terminal that has been disposed or detached mid-resize, which is an + // ordinary outcome on a tab switch — and this is called from the settle + // timer and the resize observer, where an exception takes the rest of the + // callback (the padding fit, the scroll restore, the SIGWINCH) with it. + try { + return window.CodemanTerminalGeometry.clampTerminalDimensions(this.fitAddon?.proposeDimensions()); + } catch { + return null; + } + }, + + /** + * Fit xterm to its container and return the geometry that was APPLIED. + * + * ⚠️ THE ONLY function that may change the terminal's size, and the only + * source of the numbers sent to the server. `fitAddon.fit()` on its own is + * not enough and the gap is issue #464: fit() resizes xterm to + * `proposeDimensions()` RAW, while every server-facing path reported those + * dimensions floored at 40x10. Whenever the floor bit — a phone with the + * keyboard up routinely proposes under ten rows — the PTY was told one shape + * and xterm held another, and Claude Code then computed every repaint for a + * screen that did not exist. See the note in constants.js for what that + * renders as, and why the floor is not negotiable at either end. + * + * Three call sites each used to do their own fit-then-clamp + * (`throttledResize`, `sendResize`, KeyboardHandler's one-shot), which is + * three chances to disagree; two of them also re-read `proposeDimensions()` + * after the fit, so a container that moved in between — `_shrinkPaddingToFit` + * runs exactly there — changed the answer without touching xterm. + * + * The second resize only happens when the floor actually bites, so the + * ordinary path still reflows once, as before. + * + * @returns {{cols: number, rows: number}|null} null when the terminal cannot be measured + */ + syncTerminalGeometry() { + if (!this.fitAddon || !this.terminal) return null; + try { + this.fitAddon.fit(); + } catch { + /* a disposed or unattached terminal cannot be fitted; fall through to the read */ + } + const dims = this.getTerminalDimensions(); if (!dims) return null; - return { - cols: Math.max(dims.cols, MIN_COLS), - rows: Math.max(dims.rows, MIN_ROWS), - }; + return this._resizeTerminalTo(dims) ? dims : null; + }, + + /** + * Re-measure after something changed the CELL size, and tell the server. + * + * ⚠️ A font change is a geometry change. Bigger glyphs mean fewer columns in + * the same box, and the PTY is drawing for a column count nobody updated: + * `setFontSize`, `setFontFamily` and `setFontWeight` all refitted the terminal + * and sent NOTHING, so raising the font on a phone could drop the browser + * below the columns the CLI was still wrapping at until some unrelated resize + * event happened along. That is issue #464 reached through the font menu. + * + * With no session there is no PTY to tell, and a session detached into its own + * window is not this terminal's to resize — `sendResize` makes that call, and + * fits as its first synchronous step, so this never fits twice. + */ + _refitAfterCellSizeChange() { + if (this.activeSessionId) { + this.sendResize(this.activeSessionId)?.catch?.(() => {}); + return; + } + this.syncTerminalGeometry(); + }, + + /** + * Make xterm exactly `dims`. Idempotent, and never throws at a caller — a + * terminal disposed mid-resize is an ordinary outcome on a tab switch. + * @returns {{cols: number, rows: number}|null} the applied geometry + */ + _resizeTerminalTo(dims) { + if (!this.terminal || !dims) return null; + if (this.terminal.cols === dims.cols && this.terminal.rows === dims.rows) return dims; + try { + this.terminal.resize(dims.cols, dims.rows); + return dims; + } catch { + return null; + } }, /** @@ -5421,21 +5502,25 @@ Object.assign(CodemanApp.prototype, { * @returns {Promise} Whether dimensions changed from the last send */ async sendResize(sessionId, options = {}) { - // Fit terminal to container before reading dimensions — ensures local - // terminal size matches what we report to the server PTY. - if (this.fitAddon) this.fitAddon.fit(); // One PTY cannot hold two sizes. A detached session is owned by its own // window, and the dashboard's terminal is narrower than that window because // the session rail takes width the popup does not have — so both sizing it // makes the CLI draw frames that fit neither, which garbles the popup. The // dashboard yields; the solo window sizes what it alone displays. // (_maybeRefetchFullHistory already stands aside for the same reason.) - // ⚠️ AFTER the fit, never before: the local reflow keeps the dashboard's own - // xterm right, and only the SERVER write is the dashboard's to withhold — - // the mobile-keyboard guard below draws exactly this line. tab-rail-resize - // performs its one settle-time refit through this call and has no fallback. + // ⚠️ BEFORE the fit, never after. This used to fit first and withhold only + // the server write, on the reasoning that the local reflow keeps the + // dashboard's own xterm right. It does not: it leaves this xterm at a shape + // the PTY was never told about, which is the #464 divergence exactly — and + // the popup that DOES own the PTY is drawing for its own width, so the + // dashboard's reflow is to a size nothing is rendering for. Withholding the + // resize means withholding all of it. tab-rail-resize performs its one + // settle-time refit through this call and has no fallback, which is correct: + // a pane it does not own is not its to refit either. if (!this.isSoloWindow && this.detachedSessions?.has(sessionId)) return false; - const dims = this.getTerminalDimensions(); + // Fit, floor, and apply in one step so the numbers below are the numbers + // xterm is actually holding. + const dims = this.syncTerminalGeometry(); if (!dims) return false; // Did the dimensions actually change since the last resize we sent? Callers // use this to skip work (e.g. the post-resize TUI-redraw settle) when no @@ -5468,14 +5553,81 @@ Object.assign(CodemanApp.prototype, { } const body = { ...dims, viewportType }; if (options.force) body.force = true; - await fetch(`/api/sessions/${sessionId}/resize`, { + const res = await fetch(`/api/sessions/${sessionId}/resize`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(body), }); + // Same report the WS path gets as a {"t":"zc"} frame. An older server + // answers `{}`, which reconciles to a no-op rather than throwing. + try { + const applied = (await res.json())?.data ?? {}; + this._onPtyGeometryReport(sessionId, applied.cols, applied.rows); + } catch { + /* a body that is not JSON tells us nothing about the PTY; keep our own geometry */ + } return changed; }, + /** + * Adopt the geometry the server says the PTY actually has. + * + * ⚠️ The server is the authority and this client is not always obeyed. + * `Session.resize` declines a small-viewport request outright while a desktop + * connection holds an active sizing claim, and says nothing — resize was + * write-only until #464. A terminal that keeps its own shape after such a + * refusal does not render "too narrow", it renders GARBLED: Claude Code wraps + * its frame at the width it was told and walks the cursor up that many rows, + * so a mismatch makes its erase count come out short and each repaint paints + * over rows it never cleared. Measured against a real xterm — a PTY believing + * 120 columns against a 62-column terminal draws every wrapped line twice. + * + * Adopting can leave the pane wider than the viewport, and the container is + * `overflow: hidden`, so `.pty-oversized` grants horizontal reach for exactly + * as long as the mismatch lasts. Correct-and-reachable beats correct-and- + * clipped beats garbled; nothing here is worth trapping content behind. + * + * Self-resolving: `_startMobileResizeRetry` re-sends this device's dimensions + * on a timer, so the pane comes back to this screen once the desktop goes + * idle, and the next report clears the class and the notice with it. + */ + _onPtyGeometryReport(sessionId, cols, rows) { + if (!this.terminal || sessionId !== this.activeSessionId) return; + const local = { cols: this.terminal.cols, rows: this.terminal.rows }; + const { adopt, oversized } = window.CodemanTerminalGeometry.reconcilePtyGeometry(local, { cols, rows }); + if (!adopt) { + this._setPtyOversized(false); + return; + } + if (!this._resizeTerminalTo({ cols, rows })) return; + // The numbers we would report next are now the PTY's, not the container's: + // without this the dedupe in throttledResize/sendResize compares against a + // request that was refused and suppresses the retry that recovers the pane. + this._lastResizeDims = { cols, rows }; + this._setPtyOversized(oversized); + }, + + /** + * Let the reader reach a pane wider than their screen, and say why once. + * + * Chrome for a condition that is not happening is clutter, so both the scroll + * affordance and the notice exist only while the mismatch does. The notice is + * once per transition, not per report: reports arrive on every resize, and a + * toast that repeats is noise about a situation the reader can already see. + */ + _setPtyOversized(oversized) { + const container = document.getElementById('terminalContainer'); + if (container) container.classList.toggle('pty-oversized', !!oversized); + if (oversized === this._ptyOversized) return; + this._ptyOversized = oversized; + if (oversized) { + // 53 characters: measured at one line on a 430px phone. The longer + // wording wrapped to two, which is a lot of the terminal to cover for a + // notice about a condition that resolves itself. + this.showToast('Another device is setting the width — scroll sideways', 'info'); + } + }, + /** * Send input to the active session. * @param {string} input - Text to send (include \r for Enter) diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 729c8983..549a5052 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -2115,7 +2115,12 @@ export function registerSessionRoutes( const session = findSessionOrFail(ctx, id, req); session.resize(cols, rows, { viewportType, force }); - return {}; + // Answer with the geometry the PTY ACTUALLY holds, which is not always the + // one asked for: `Session.resize` declines small-viewport requests while a + // desktop connection holds an active sizing claim. A browser terminal left + // at a shape the PTY refused renders garbled output, not merely wrong-sized + // output, so the client adopts these (issue #464). + return { cols: session.ptyCols, rows: session.ptyRows }; }); // ========== Get Last Response (from transcript JSONL) ========== diff --git a/src/web/routes/ws-routes.ts b/src/web/routes/ws-routes.ts index ec7fe9e1..6aae1ddb 100644 --- a/src/web/routes/ws-routes.ts +++ b/src/web/routes/ws-routes.ts @@ -22,6 +22,9 @@ * {"t":"c"} — clear terminal * {"t":"r"} — needs refresh (reload buffer) * {"t":"ia","seq":N} — input ACK (echoes the seq of an applied/deduped input frame) + * {"t":"zc","c":N,"r":N} — resize confirm: the geometry the PTY now holds, which + * is NOT always the one requested (see Session.resize + * arbitration). Clients adopt it — issue #464. * Client -> Server: * {"t":"i","d":"...","seq":N,"cid":"..."} — input (keystroke or paste). seq+cid are * optional reliable-delivery tags: the server applies each @@ -240,6 +243,16 @@ export function registerWsRoutes(app: FastifyInstance, ctx: SessionPort, getHost } const force = msg.f === true; session.resize(msg.c, msg.r, { viewportType, force }); + // Report the geometry that actually took. Resize used to be + // write-only, so a client whose request was declined by the + // arbitration above — or floored, or overridden by another device + // — had no way to find out, and went on rendering a CLI's repaints + // against a screen shape that did not exist (issue #464). Sent + // unconditionally: it is ~30 bytes on a debounced, rare message, + // and always-send means the client needs no "did it take?" state. + if (socket.readyState === 1) { + socket.send(`{"t":"zc","c":${session.ptyCols},"r":${session.ptyRows}}`); + } } } catch { // Ignore malformed messages diff --git a/test/terminal-pty-geometry.test.ts b/test/terminal-pty-geometry.test.ts new file mode 100644 index 00000000..2017e167 --- /dev/null +++ b/test/terminal-pty-geometry.test.ts @@ -0,0 +1,384 @@ +// Port: none (pure helpers + a real headless xterm + source guards). +// +// Issue #464, "text gets muffled sometimes". The report is a phone screenshot +// where lines of Claude Code's output are rendered twice and short tool +// summaries sit inside longer prose rows with the prose's tail still showing. +// +// That is not a dropped frame or a frozen renderer; it is arithmetic. Ink wraps +// its frame at the width the PTY reported and erases the previous frame by +// walking the cursor up the number of rows it BELIEVES that frame occupied. A +// browser terminal narrower than the PTY makes each logical line occupy more +// physical rows than Ink counted, so `eraseLines(n)` clears too few of them and +// the new frame paints over rows that were never erased. +// +// `renders each wrapped line twice when the PTY is wider` below reproduces it +// against the repo's own xterm, and is written as a CONTRAST: the same stream at +// a matching width must come out clean. An implementation that stopped fixing +// anything would fail the second half, not quietly satisfy the first. +// +// The rest pins the invariant the fix rests on: there is exactly ONE function +// that changes the terminal's size, it applies the same floor it reports, and +// the server reports back the geometry the PTY actually holds so a client whose +// resize was declined can adopt it instead of rendering against a screen that +// does not exist. +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import vm from 'node:vm'; +import { describe, expect, it } from 'vitest'; +import xtermHeadless from '@xterm/headless'; + +const { Terminal } = xtermHeadless as unknown as { + Terminal: new (opts: Record) => { + write(data: string, cb?: () => void): void; + buffer: { + active: { length: number; getLine(y: number): { translateToString(trim?: boolean): string } | undefined }; + }; + }; +}; + +const read = (rel: string) => readFileSync(resolve(import.meta.dirname, '..', rel), 'utf8'); + +type Dims = { cols: number; rows: number }; + +function loadGeometry() { + const context = vm.createContext({ window: {}, globalThis: {} }); + vm.runInContext(read('src/web/public/constants.js'), context, { filename: 'constants.js' }); + return ( + context.window as { + CodemanTerminalGeometry: { + clampTerminalDimensions: (p: Partial | null | undefined) => Dims | null; + terminalGeometryAgrees: (a: Dims | null, b: Dims | null) => boolean; + reconcilePtyGeometry: (local: Dims | null, pty: Partial | null) => { adopt: boolean; oversized: boolean }; + TERMINAL_MIN_COLS: number; + TERMINAL_MIN_ROWS: number; + }; + } + ).CodemanTerminalGeometry; +} + +// ─────────────────────────────────────────────────────────────────────────── +// The failure itself, against the real terminal. +// ─────────────────────────────────────────────────────────────────────────── + +/** ansi-escapes `eraseLines(n)`: \x1b[2K per row walking up, then column 1. */ +function eraseLines(n: number): string { + let out = ''; + for (let i = 0; i < n; i++) out += '\x1b[2K' + (i < n - 1 ? '\x1b[1A' : ''); + return n ? out + '\x1b[G' : ''; +} + +/** How many physical rows Ink thinks its frame took, wrapping at `cols`. */ +const rowsAt = (frame: string[], cols: number) => + frame.reduce((n, line) => n + Math.max(1, Math.ceil(line.length / cols)), 0); + +/** + * Ink's repaint loop: erase the previous frame, write the new one. The erase + * count is computed at `ptyCols` — the width the PTY told the CLI about — + * while the terminal is `xtermCols` wide. + */ +function inkStream(frames: string[][], ptyCols: number): string { + let out = ''; + let previousRows = 0; + for (const frame of frames) { + out += eraseLines(previousRows) + frame.join('\r\n'); + previousRows = rowsAt(frame, ptyCols); + } + return out; +} + +async function render(data: string, cols: number, rows = 24): Promise { + const term = new Terminal({ cols, rows, allowProposedApi: true, scrollback: 500 }); + await new Promise((done) => term.write(data, () => done())); + const buf = term.buffer.active; + const lines: string[] = []; + for (let y = 0; y < buf.length; y++) lines.push(buf.getLine(y)?.translateToString(true) ?? ''); + while (lines.length && lines[lines.length - 1] === '') lines.pop(); + return lines; +} + +describe('a terminal that disagrees with the PTY about width', () => { + const XTERM_COLS = 62; + // Prose long enough to wrap, then a live region that shrinks as tool calls + // collapse into one-line summaries — ordinary Claude Code output. + const PROSE = [ + "• Password store entries exist, but GPG can't decrypt — that's the locked keyring after a pod restart. Let me get the browsers sorted.", + ]; + const FRAMES = [ + [...PROSE, ' Reading settings, scanning the pass store and checking whether the agent can reach AWS'], + [...PROSE, ' Ran 1 shell command'], + ]; + + it('renders each wrapped line twice when the PTY is wider', async () => { + const lines = await render(inkStream(FRAMES, 120), XTERM_COLS); + const duplicated = lines.filter((line, i) => line !== '' && lines.indexOf(line) !== i); + expect( + duplicated.length, + `a 120-column PTY against a ${XTERM_COLS}-column terminal must leave ghost rows:\n${lines.join('\n')}` + ).toBeGreaterThan(0); + }); + + // The contrast. Without this half, an implementation that fixed nothing — + // or a stream that never ghosted in the first place — would still pass above. + it('renders each line exactly once when the two agree', async () => { + const lines = await render(inkStream(FRAMES, XTERM_COLS), XTERM_COLS); + const duplicated = lines.filter((line, i) => line !== '' && lines.indexOf(line) !== i); + expect(duplicated, `matched widths must render cleanly:\n${lines.join('\n')}`).toEqual([]); + // And the frame that actually won is the last one. + expect(lines[lines.length - 1]).toBe(' Ran 1 shell command'); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +// The decisions, pure. +// ─────────────────────────────────────────────────────────────────────────── + +describe('clampTerminalDimensions', () => { + const { clampTerminalDimensions, TERMINAL_MIN_COLS, TERMINAL_MIN_ROWS } = loadGeometry(); + + it('floors a proposal too small to be a usable PTY', () => { + expect(clampTerminalDimensions({ cols: 12, rows: 4 })).toEqual({ + cols: TERMINAL_MIN_COLS, + rows: TERMINAL_MIN_ROWS, + }); + }); + + it('leaves a proposal that already clears the floor alone', () => { + expect(clampTerminalDimensions({ cols: 62, rows: 40 })).toEqual({ cols: 62, rows: 40 }); + }); + + it('floors each axis independently — a short phone is not a narrow one', () => { + // The everyday case behind #464: keyboard up, plenty of columns, under ten rows. + expect(clampTerminalDimensions({ cols: 62, rows: 6 })).toEqual({ cols: 62, rows: TERMINAL_MIN_ROWS }); + }); + + it('reports nothing rather than a guess when the terminal cannot be measured', () => { + for (const bad of [null, undefined, {}, { cols: NaN, rows: 10 }, { cols: 40, rows: Infinity }]) { + expect(clampTerminalDimensions(bad as Partial)).toBeNull(); + } + }); +}); + +describe('reconcilePtyGeometry', () => { + const { reconcilePtyGeometry } = loadGeometry(); + + it('does nothing when the terminal already matches the PTY', () => { + expect(reconcilePtyGeometry({ cols: 62, rows: 40 }, { cols: 62, rows: 40 })).toEqual({ + adopt: false, + oversized: false, + }); + }); + + it('adopts a PTY the client never asked for — a declined resize is still the truth', () => { + // Session.resize ignores a small viewport while a desktop claim is live. + expect(reconcilePtyGeometry({ cols: 62, rows: 40 }, { cols: 120, rows: 40 })).toEqual({ + adopt: true, + oversized: true, + }); + }); + + it('adopts without claiming oversized when the PTY is merely shorter or narrower', () => { + expect(reconcilePtyGeometry({ cols: 120, rows: 40 }, { cols: 80, rows: 40 })).toEqual({ + adopt: true, + oversized: false, + }); + expect(reconcilePtyGeometry({ cols: 62, rows: 40 }, { cols: 62, rows: 12 })).toEqual({ + adopt: true, + oversized: false, + }); + }); + + it('keeps its own geometry when the server reported none', () => { + // An older server answers the resize POST with `{}`; that is not evidence. + for (const bad of [null, {}, { cols: 62 }, { cols: 'wide', rows: 40 }]) { + expect(reconcilePtyGeometry({ cols: 62, rows: 40 }, bad as Partial)).toEqual({ + adopt: false, + oversized: false, + }); + } + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +// One owner of the terminal's size. These are source guards because the code +// they cover needs a real DOM (FitAddon measures a rendered element), which +// the CI gate has no way to give it. +// ─────────────────────────────────────────────────────────────────────────── + +describe('exactly one function may change the terminal size', () => { + const terminalUi = read('src/web/public/terminal-ui.js'); + const mobileHandlers = read('src/web/public/mobile-handlers.js'); + + function bodyOf(source: string, signature: string): string { + const start = source.indexOf(signature); + expect(start, `${signature} not found — renamed?`).toBeGreaterThan(-1); + const end = source.indexOf('\n },', start); + expect(end).toBeGreaterThan(start); + return source.slice(start, end); + } + + it('syncTerminalGeometry applies the floor it reports, not the raw proposal', () => { + const body = bodyOf(terminalUi, 'syncTerminalGeometry() {'); + expect(body).toContain('this.fitAddon.fit()'); + // fit() resizes to proposeDimensions() RAW; the floored value is what goes + // to the server, so the floored value is what xterm must end up holding. + expect(body).toContain('this.getTerminalDimensions()'); + expect(body).toContain('this._resizeTerminalTo(dims)'); + }); + + // A whole-repo sweep rather than a spot check, so a NEW call site trips it + // rather than quietly reopening #464. Scoped to the MAIN terminal: the split + // pane, the teammate windows and the log viewer are separate xterm instances + // with their own PTYs (or none), and each owns its own sizing. + it('no other call site fits the main terminal behind its back', () => { + const MAIN_TERMINAL_FIT = /^(?!.*(?:_splitPane|entry\.fitAddon)).*fitAddon[?.]*\.fit\(\)/; + const offenders: string[] = []; + for (const rel of [ + 'src/web/public/terminal-ui.js', + 'src/web/public/mobile-handlers.js', + 'src/web/public/app.js', + 'src/web/public/ralph-panel.js', + 'src/web/public/settings-ui.js', + 'src/web/public/tab-rail-resize.js', + 'src/web/public/notification-manager.js', + ]) { + read(rel) + .split('\n') + .forEach((line, i) => { + const code = line.trim(); + if (code.startsWith('*') || code.startsWith('//')) return; // prose about fit(), not a call + if (MAIN_TERMINAL_FIT.test(line)) offenders.push(`${rel}:${i + 1} ${code}`); + }); + } + // Exactly one: the owner's own fit. + expect( + offenders, + 'every fit of the main terminal must go through syncTerminalGeometry(), which applies ' + + 'the same floor it reports — a bare fit() leaves xterm at the RAW proposal while the ' + + 'server is told the floored one (issue #464)' + ).toHaveLength(1); + expect(offenders[0]).toContain('terminal-ui.js'); + expect(bodyOf(terminalUi, 'syncTerminalGeometry() {')).toContain('this.fitAddon.fit()'); + expect(mobileHandlers).toContain('app.syncTerminalGeometry?.()'); + }); + + it('a font change tells the server, because it moves the cell size', () => { + // Bigger glyphs mean fewer columns in the same box. These three refitted + // and sent nothing, so the CLI kept wrapping at the old column count. + for (const setter of [ + 'setFontSize(size) {', + 'this.terminal.options.fontFamily === resolved', + 'this.terminal.options.fontWeight === fontWeight', + ]) { + expect(terminalUi, `${setter} no longer present`).toContain(setter); + } + expect(bodyOf(terminalUi, 'setFontSize(size) {')).toContain('this._refitAfterCellSizeChange()'); + const helper = bodyOf(terminalUi, '_refitAfterCellSizeChange() {'); + expect(helper).toContain('this.sendResize(this.activeSessionId)'); + expect(helper).toContain('this.syncTerminalGeometry()'); + // Three call sites in the font setters (size, family, weight) plus the two + // font-settle re-fits. + expect((terminalUi.match(/_refitAfterCellSizeChange\(\)/g) ?? []).length).toBeGreaterThanOrEqual(6); + }); + + it('the keyboard one-shot delegates rather than computing its own numbers', () => { + const body = bodyOf(mobileHandlers, '_sendTerminalResize() {'); + expect(body).toContain('app.sendResize'); + // The hand-rolled POST floored what it sent and nothing else. + expect(body).not.toContain('proposeDimensions'); + expect(body).not.toContain('Math.max'); + expect(body).not.toContain('fetch('); + }); + + it('sendResize yields a detached session BEFORE touching geometry, not after', () => { + const body = bodyOf(terminalUi, 'async sendResize(sessionId, options = {}) {'); + const yieldAt = body.indexOf('detachedSessions?.has(sessionId)) return false'); + const fitAt = body.indexOf('this.syncTerminalGeometry()'); + expect(yieldAt, 'the detached-session yield is gone').toBeGreaterThan(-1); + expect(fitAt, 'sendResize no longer syncs geometry').toBeGreaterThan(-1); + expect( + yieldAt, + 'withholding the server resize but reflowing anyway leaves this xterm at a shape ' + + 'the PTY was never told about — withhold both or neither' + ).toBeLessThan(fitAt); + }); + + it('throttledResize withholds the fit wherever it withholds the SIGWINCH', () => { + const start = terminalUi.indexOf('const throttledResize = () => {'); + expect(start).toBeGreaterThan(-1); + const block = terminalUi.slice(start, terminalUi.indexOf("window.addEventListener('resize', throttledResize)")); + const guardAt = block.indexOf('!keyboardUp && !detachedElsewhere'); + const syncAt = block.indexOf('this.syncTerminalGeometry()'); + expect(guardAt).toBeGreaterThan(-1); + expect(syncAt, 'the geometry sync must sit INSIDE the guard').toBeGreaterThan(guardAt); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +// Resize stopped being write-only. +// ─────────────────────────────────────────────────────────────────────────── + +describe('the server reports the geometry the PTY actually holds', () => { + it('Session exposes the effective dimensions', () => { + const session = read('src/session.ts'); + expect(session).toMatch(/get ptyCols\(\): number \{\s*return this\._ptyCols;/); + expect(session).toMatch(/get ptyRows\(\): number \{\s*return this\._ptyRows;/); + }); + + it('the WebSocket answers a resize with what took', () => { + const ws = read('src/web/routes/ws-routes.ts'); + const at = ws.indexOf('session.resize(msg.c, msg.r,'); + expect(at).toBeGreaterThan(-1); + const after = ws.slice(at, at + 1400); + expect(after).toContain('"t":"zc"'); + expect(after).toContain('session.ptyCols'); + expect(after).toContain('session.ptyRows'); + // Documented in the protocol block at the top of the file, like every other frame. + expect(ws).toContain('{"t":"zc","c":N,"r":N}'); + }); + + it('the HTTP resize answers with what took, not an empty object', () => { + const routes = read('src/web/routes/session-routes.ts'); + const at = routes.indexOf("app.post('/api/sessions/:id/resize'"); + expect(at).toBeGreaterThan(-1); + const handler = routes.slice(at, at + 1600); + expect(handler).toContain('return { cols: session.ptyCols, rows: session.ptyRows };'); + }); + + it('the client adopts the report and re-bases its dedupe on it', () => { + const terminalUi = read('src/web/public/terminal-ui.js'); + const start = terminalUi.indexOf('_onPtyGeometryReport(sessionId, cols, rows) {'); + expect(start).toBeGreaterThan(-1); + const body = terminalUi.slice(start, terminalUi.indexOf('\n },', start)); + expect(body).toContain('reconcilePtyGeometry'); + expect(body).toContain('this._resizeTerminalTo({ cols, rows })'); + // Without this the next resize is deduped against a request that was + // REFUSED, which suppresses the retry that recovers the pane. + expect(body).toContain('this._lastResizeDims = { cols, rows }'); + // And the WS frame is wired up at all. + expect(read('src/web/public/app.js')).toContain("msg.t === 'zc'"); + }); + + it('a pane wider than the screen gets horizontal reach for as long as that lasts', () => { + const css = read('src/web/public/styles.css'); + // .terminal-container is overflow:hidden, so adopting a wider PTY without + // this puts the right-hand columns somewhere no gesture can reach them. + // Read the rule's DECLARATIONS, comments stripped: the comments in this block + // quote CSS with braces in it, which a `[^}]*` window cannot survive. + const declarationsOf = (selector: string) => { + const at = css.indexOf(`${selector} {`); + expect(at, `${selector} not found`).toBeGreaterThan(-1); + const body = css.slice(at + selector.length, css.indexOf('\n}', at)); + return body.replace(/\/\*[\s\S]*?\*\//g, ''); + }; + const oversized = declarationsOf('.terminal-container.pty-oversized'); + expect(oversized).toContain('overflow-x: auto;'); + // Both axes, explicitly: mobile.css sets `overflow: visible` on the bare + // selector, and a lone overflow-x would leave overflow-y computing to auto. + expect(oversized).toContain('overflow-y: hidden;'); + // On the base rule, not only the .touch-device variant — mobile.css's + // `.terminal-container { touch-action: none }` is unscoped. + expect(oversized).toContain('touch-action: pan-x;'); + // The class is only ever on while the mismatch is. + expect(read('src/web/public/terminal-ui.js')).toContain("classList.toggle('pty-oversized', !!oversized)"); + }); +}); diff --git a/test/terminal-resilience.test.ts b/test/terminal-resilience.test.ts index 7dbee74c..94fb7b1c 100644 --- a/test/terminal-resilience.test.ts +++ b/test/terminal-resilience.test.ts @@ -227,6 +227,50 @@ describe('terminal capture deadline covers the response body', () => { expect(body).toContain('return { json, headers: res.headers, headersAt };'); }); + // Nothing in the gate pinned the invariant this PR exists to establish, which + // is the same drift it is fixing: a clear()+reset() pair reads as obviously + // equivalent to the queued RIS and is exactly what someone tidies back in. + it('_resetTerminalForReplay is a queued write and nothing else', () => { + const app = readFileSync(resolve(import.meta.dirname, '../src/web/public/app.js'), 'utf8'); + const start = app.indexOf('_resetTerminalForReplay() {'); + expect(start, 'helper not found — renamed?').toBeGreaterThan(-1); + const body = app.slice(start, app.indexOf('\n }', start)); + // RIS, queued through write() so it lands after any bytes already parsing. + expect(body).toContain("this.terminal.write('\\x1bc')"); + expect( + body, + 'reset()/clear() are SYNCHRONOUS and skip the write queue, so bytes queued ' + + 'before them are parsed after and fuse into the snapshot written next' + ).not.toMatch(/\.(reset|clear)\(\)/); + }); + + it('every replay path clears through that helper, never by hand', () => { + const app = readFileSync(resolve(import.meta.dirname, '../src/web/public/app.js'), 'utf8'); + // The three paths that blank the terminal before rewriting it from a capture. + for (const site of ['_onSessionNeedsRefresh(event = {}) {', 'async _onSessionClearTerminal(data) {']) { + const start = app.indexOf(site); + expect(start, `${site} not found — renamed?`).toBeGreaterThan(-1); + const body = app.slice(start, start + 4000); + expect(body, `${site} must clear via _resetTerminalForReplay`).toContain('this._resetTerminalForReplay()'); + expect(body, `${site} hand-rolled a clear again`).not.toContain('this.terminal.clear()'); + } + // And the PAIR appears nowhere in the frontend. A lone `clear()` before + // `showWelcome()` is fine — nothing is written after it, so there is nothing + // for stray bytes to fuse into. `clear()` immediately followed by `reset()` + // is the signature of someone blanking the terminal to rewrite it, which is + // precisely the case that has to be queued instead. + const pair = /\.clear\(\);\s*\n\s*this\.terminal\.reset\(\)/; + for (const rel of [ + 'src/web/public/app.js', + 'src/web/public/panels-ui.js', + 'src/web/public/terminal-ui.js', + 'src/web/public/session-ui.js', + ]) { + const src = readFileSync(resolve(import.meta.dirname, '..', rel), 'utf8'); + expect(src, `${rel} blanks the terminal with clear()+reset() — use _resetTerminalForReplay()`).not.toMatch(pair); + } + }); + it('returns the parsed envelope and headers on a healthy response', async () => { const { url, close } = await serve((res) => { res.writeHead(200, { 'Content-Type': 'application/json', 'server-timing': 'db;dur=12' });