From e1e7dc5bd8c3b815bbb1928722cb79e172a0ef69 Mon Sep 17 00:00:00 2001 From: Rounak Datta Date: Tue, 22 Sep 2026 18:53:46 +0530 Subject: [PATCH] fix(terminal): Ark0N's read of the #464 geometry work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five items, two of which he could only see by running it, plus six smaller ones. Taking the two blockers first, because both were wrong in ways the existing tests could not catch. **Adopting the PTY's rows put the CLI's input line off-screen.** A phone that took a desktop's 43 rows into a viewport with room for 18 painted an `.xterm-screen` far taller than its container; xterm's own viewport then had nothing to scroll, so the bottom of the frame sat below the container with no gesture able to reach it. Output visible, typing invisible, for as long as the desktop kept the claim hot. `reconcilePtyGeometry` adopts COLUMNS ONLY now: width is the axis Ink's wrap and `eraseLines` arithmetic depend on, and keeping the local row count keeps the composer at the bottom of a viewport that scrolls. Measured at his geometry — a 360x300 container against a 198x43 pane now keeps 13 rows, takes 198 columns, paints 202px into a 210px container, and the input line is inside the box. **`capture-geometry-retry.browser.test.ts` failed, and CI could not see it** because the file is in `BROWSER_TEST_GLOBS`. Its premise WAS the clamp — `getTerminalDimensions()` floored while `fitAddon.fit()` did not — which this work removes at the source, so it can never hold again at any viewport. The case survives on its own terms: a pane already drawing at the requested size must not be replayed. Its premise is now the #464 invariant itself, that the floored report and the terminal agree, which is a stronger guard because the clamp coming back fails it here rather than silently restoring the replay loop. The helper docblock that repeated the old premise is corrected too. **A session with no pane reported 120x40 and the client adopted it.** `resize()` writes `_ptyCols`/`_ptyRows` only when `ptyProcess` is set and nothing seeds them from the spawn geometry, so a dead-pane session still held the constructor defaults — clicking that tab resized the browser terminal to 120x40 and, on anything narrower, claimed another device owned the pane when none existed. `Session.ptyGeometry` returns null without a pane, the HTTP route answers `{}` and the socket sends no frame at all. The raw `ptyCols`/`ptyRows` getters are deleted rather than left available to be misused again. **The 40-column floor clipped the pane with nothing able to reach it.** The affordance keyed on a PTY mismatch, and the floor produces no mismatch — xterm and the PTY agree throughout, the terminal is simply wider than the box. It keys on what does not FIT now, MEASURED (`.xterm-screen` against the container, on the next frame, because the screen takes its width with the render) rather than derived from cell arithmetic. Measured at 360px: font 24 applies 40 columns and paints 560px, and all 200px of the overhang is reachable. `.pty-oversized` is renamed `.term-overflows-x`, because after this the old name describes only one of the two causes. **"Scroll sideways" did not work on touch for the sessions it targets.** `touch-action: pan-x` is cancelled before it starts by the `preventDefault()` `touchstart` calls on every 'content' tap. The terminal's own touchmove handler pans the container now, with the axis locked once per gesture so a diagonal cannot pan and scroll at once, and the CSS grants no `touch-action` at all — handing the browser a pan AS WELL would move the pane twice for one finger on the taps where that preventDefault does not run. Measured under real touch dispatch: a 140px swipe reaches `scrollLeft` 140 where it reached 0 before, the buffer does not move with it, and a vertical swipe still scrolls the scrollback. Three defects in the above, found while checking it rather than by being told: - `canPanHorizontally` first tested `scrollWidth > clientWidth` alone, which is true of a container that is not a scroller — a sideways swipe would have locked the axis, done nothing, AND suppressed the vertical scroll it should have been. Gated on the class as well. - The notice advised scrolling sideways whenever the PTY was wider, including when it still fitted and nothing scrolled. It is gated on measured overflow, and on a comparison against the width this container WOULD request rather than the one it currently holds — once adopted those are equal, so the second question answers itself false while the condition is still true. - `_syncTerminalOverflowAffordance` could throw out of `document.getElementById` before reaching its try block. It runs off every geometry change, so a cosmetic affordance could have taken the resize down with it. The smaller items: - `docs/architecture-invariants.md` no longer explains the equality guard as a clamp signature; it records what the clamp used to do and why it cannot any more. Edited by hand — that file is outside the Prettier glob, and letting Prettier near it rewrote eleven unrelated emphasis markers. - `throttledResize`'s HTTP fallback reads the reply. It is the path where a declined resize is least likely to be noticed, because no socket means no `{"t":"zc"}` frame either. - The changeset covers the whole release: the geometry work, the queued replay clear, the renderer watchdog, the body-covering fetch deadline, the WebSocket output-gap reconcile, the build-generated service-worker precache and per-build cache key, and the crash-trail hygiene. - `@xterm/headless` is declared in the root devDependencies instead of being reached through workspace hoisting. - The output-gap marker is cleared after any response arrives, not only when the capture was non-empty: a server that answers with an empty capture HAS reconciled us, and leaving the marker set refetched on every reconnect. - `e587d845`'s message claimed a test asserted the failed-load copy against the built asset. It did not — that assertion lived in a probe deleted with the other scratch scripts, so the claim was false when it was written. There is a real test now, and it reads the source rather than `dist/`, because `dist/` is not committed and a test that skips when it is absent would pass for the wrong reason in CI. `Session.ptyGeometry` gets behavioural coverage against the real class in `session-resize-arbitration.test.ts` rather than a source guard, including the contrast — a pane that does exist still reports, and still follows a resize — so "always null" would fail it. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/pty-geometry-464.md | 16 +- CLAUDE.md | 2 +- docs/architecture-invariants.md | 2 +- package-lock.json | 1 + package.json | 1 + src/session.ts | 21 ++- src/web/public/app.js | 17 +- src/web/public/constants.js | 27 +-- src/web/public/styles.css | 49 +++-- src/web/public/terminal-ui.js | 190 ++++++++++++++++---- src/web/routes/session-routes.ts | 5 +- src/web/routes/ws-routes.ts | 7 +- test/capture-geometry-retry.browser.test.ts | 50 ++++-- test/detached-session-pane-sizing.test.ts | 5 + test/session-resize-arbitration.test.ts | 31 ++++ test/terminal-pty-geometry.test.ts | 159 ++++++++++++---- 16 files changed, 438 insertions(+), 145 deletions(-) diff --git a/.changeset/pty-geometry-464.md b/.changeset/pty-geometry-464.md index ecc5a047..ea4267b7 100644 --- a/.changeset/pty-geometry-464.md +++ b/.changeset/pty-geometry-464.md @@ -2,12 +2,18 @@ "aicodeman": patch --- -fix(terminal): the PTY and the browser terminal must never disagree about size (#464) +fix(terminal): five ways the terminal silently stopped being correct -"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. +**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()` sized xterm to the raw measurement while every server-facing path reported it floored at 40x10 (font size 44 on a 430px phone 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 both transports answer a resize with the geometry the PTY actually holds, which the client adopts by **columns only**. A terminal left wider than the box that shows it now earns horizontal reach for as long as that lasts, whether the cause is another device's claim or the 40-column floor. -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. +**A replay clear must be in-stream, never `reset()`/`clear()`.** xterm's `write()` is queued while `Terminal.reset()` is synchronous and does not reset the parser, so bytes queued just before a reset are parsed after it and fuse into the snapshot written next — `write('p8'); reset(); write('rmissions')` renders `p8rmissions`. All three replay paths now go through one queued `\x1bc`, and the gate pins that no module blanks the terminal with a `clear()`+`reset()` pair. -`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. +**A renderer that stops painting now heals itself.** iOS discards scheduled `requestAnimationFrame` callbacks when a PWA backgrounds, and xterm's `RenderDebouncer` only clears its handle from inside that callback, so one drop leaves every later refresh a no-op while the buffer keeps updating correctly. Codeman has exactly one xterm for the whole page load, so a single backgrounding wedged it until a reload. A watchdog cancels the stale handle and forces a repaint. -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. +**Every terminal capture carries a deadline that covers the response body.** `await fetch()` settles on headers, so clearing the timer there left the multi-megabyte `?full=1` body unbounded — measured at 4026ms under a 1000ms deadline. A capture that outruns its deadline during a tab switch now falls back to the bounded tail rather than leaving a blank pane, a mute session and a tab stuck reporting `aria-busy`. + +**Output lost to a half-open WebSocket is reconciled.** Terminal output frames carry no sequence number, so a socket that dies while SSE stays up leaves a hole nothing replays. The session is marked and the next successful open repaints it, with the marker cleared only once a repaint has actually happened. + +**The service worker's precache is generated by the build, and its cache key rotates per build.** The list was hand-maintained with pre-hash names, so every entry 404'd in production and the failure was swallowed (15 of 23 verified failing against a running instance); `caches.match` now passes `ignoreSearch: true`, without which no precached entry was reachable behind the build's `?v=` cache-bust. The old constant cache name meant `activate` never deleted anything, so assets from every past release accumulated forever. + +Also: the crash trail is flattened and length-capped, so a server-controlled WebSocket close reason can no longer forge entries. diff --git a/CLAUDE.md b/CLAUDE.md index f2030ab6..a0467891 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -346,7 +346,7 @@ 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`, the full pane width reachable by scrolling (how many pixels depends on the content, so no figure is pinned here), and the terminal's own vertical scroll still working under `pan-x`. Removing the class returns `scrollLeft` to 0, so a resolved mismatch cannot leave the pane parked off-screen. +**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.ptyGeometry` (`{"t":"zc"}` on the socket, the body of the resize POST) and `_onPtyGeometryReport` adopts it — a terminal that keeps a WIDTH the PTY refused renders GARBLED, not merely wrong-sized. ⚠️ **`ptyGeometry` is null without a live pane**, never the field values: `resize()` writes `_ptyCols`/`_ptyRows` only when `ptyProcess` is set and nothing seeds them from the spawn geometry, so a dead-pane session still holds the constructor defaults of 120x40 — reporting those made a client adopt a size no process was ever told and claim another device owned the pane when none existed. ⚠️ **COLUMNS ONLY.** Adopting the PTY's ROWS was a regression: a phone taking a desktop's 43 rows into a viewport with room for 18 painted an `.xterm-screen` far taller than its container, xterm's own viewport then had nothing to scroll, and the CLI's input line sat below the container with no gesture able to reach it — output visible, typing invisible, for as long as the claim stayed hot. Width is the axis the wrap arithmetic depends on; rows only decide how much is on screen, and keeping the local count keeps the composer at the bottom of a viewport that scrolls. ⚠️ **`.term-overflows-x` keys on what does not FIT, not on a PTY mismatch**, because the 40-column floor widens the terminal past a narrow container with the PTY agreeing throughout (measured at 360px: font 18 paints 433px, font 24 paints 578px — 38% unreachable, and `increaseFontSize` reaches 24 in two taps). `_syncTerminalOverflowAffordance()` MEASURES `.xterm-screen` against the container on the next frame rather than deriving it from cell arithmetic. ⚠️ That rule sets **both** overflow axes — mobile.css loads later with `.terminal-container { overflow: visible }`, 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 — and sets **no `touch-action`**: the terminal's own touchmove handler pans it (`canPanHorizontally`), because `touchstart` preventDefault()s every 'content' tap and that cancels a native `pan-x` before it starts (measured: a 140px swipe reached scrollLeft 141 without that preventDefault and 0 with it). Removing the class returns `scrollLeft` to 0, so a resolved mismatch cannot leave the pane parked off-screen. **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`. diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index b024875a..8fd3588d 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -136,7 +136,7 @@ Tests: `test/docker-hosts.test.ts`, `test/docker-exec-options.test.ts`, `test/do **Full-scrollback replay** (COD-164/#148, reworked for #205): `GET /api/sessions/:id/terminal?full=1` returns the ENTIRE tmux scrollback (capture-pane `-e -S -` bounded by the configured history limit, explicit `maxBuffer` from the terminal-history config, early byte-cap before normalization, CRLF-normalized for shell panes). On success the capture is returned ALONE (`source='mux-full-history'` — it supersedes the byte buffer; no duplication). The first load of each non-shell TUI session per page requests `full=1` (`_fullHistoryLoaded` Set in app.js — the old one-shot `_initialFullBufferLoad` flag was consumed by whichever tab auto-selected, leaving every other TUI tab one frame of history). Shell sessions instead load a bounded 1 MiB `?tail=` window on every selection and automatic drop recovery: a 100k-line shell capture can be tens of MiB, and automatically parsing it makes tab-switch latency scale with the entire session. Shell full history is explicit-button-only; reaching the top during an ordinary wheel/touch gesture must not reset xterm and replay the multi-megabyte capture on its main thread. Other modes may still re-pull `full=1` at the TOP, and pressing **Load full history** forces the request for any recoverably truncated session (`_maybeRefetchFullHistory`, 4s per-session gesture cooldown, in-flight + tab-switch guards, viewport position held across the replay); Shell full pulls are not retained in the tab cache, so the next switch stays bounded. Chunked replay enqueues 32 KiB pieces across safe yields, appends an xterm parse marker, then releases the live-output gate; output arriving after that release stays ordered behind the snapshot, and the marker callback supplies accurate parse timing. ⚠️ **How the load ENDS depends on where the payload came from**, and `_bufferLoadFinishOpts` (app.js) is the one place that decides it for all four fetch-and-write paths. A payload built from the server's accumulated byte history is current up to the response, so the events queued during the load already appear in it and stay DISCARDED; replaying them would duplicate output, most visibly Ink's cursor-up redraws. A pane capture (`mux-visible` or `mux-full-history`) is current only up to CAPTURE time, so `_finishBufferLoad` replays the queue from the response's own arrival timestamp (`since`) and the pre-capture events stay dropped. ⚠️ **A path that then restores a scroll position must re-take the sticky-scroll baseline** (`_syncStickyScrollBaseline`): the replay runs inside `chunkedTerminalWrite` before its promise resolves, with the terminal freshly reset, so `batchTerminalWrite` samples `_wasAtBottomBeforeWrite` as true and the next `flushPendingWrites` would scroll to the bottom over the restore. ⚠️ The cutoff is a client-side timestamp and the server broadcasts on a batch timer (8ms WebSocket, 16-50ms SSE), so a batch pending when the capture ran arrives after the response and replays although the capture holds it — bounded by one batch interval, and closable only server side by flushing that batch before the capture. Tests for the three: `test/terminal-flush-budget.test.ts` pins which sources flush, `test/terminal-buffer-flush.test.ts` pins the `since` cutoff and the baseline re-take, and `test/capture-load-window.browser.test.ts` drives both against a live server. Live output is separately one-chunk-in-flight: xterm's callback releases each 32/64 KiB write before the next is submitted, keeping the remainder in the app queue where the 128 KiB cap can observe it instead of hiding an unbounded backlog in xterm's private WriteBuffer. While WebSocket owns terminal I/O, parallel SSE terminal/output-recovery events are discarded before JSON parsing; fallback recovery is single-flight per active session so backpressure cannot start overlapping reset+replay cycles. The route exposes capture/prepare totals in `Server-Timing`, while `[TERMINAL-PERF]` separates TTFB, body/JSON, reset+parse and total time for both selection and on-demand full pulls; parse completion is not a browser compositor/GPU paint measurement. The re-pull exists because xterm's buffer is only a WINDOW onto tmux's history and two things shrink it: tmux coalesces bursty output into pane REPAINTS that overwrite rows instead of emitting linefeeds (measured: a 60-line burst added 1 row of browser scrollback and destroyed 34), and a tab switch replays only the visible frame. tmux's own history is intact throughout — the browser just has to ask for it again. On-demand rather than automatic because at a 100k history limit the capture can be megabytes. ⚠️ **The capture ENDS with a cursor move back to the pane's own caret position** (`formatCursorRestore`, from the same `display-message` query the visible-frame path uses). The linear replay otherwise leaves the caret wherever the last character landed — the bottom-most row carrying text, which for an agent CLI is the status line — so the caret sat on the composer's border instead of its input line and every cursor-relative update the CLI sent afterwards was measured from the wrong row, until its next full redraw silently repaired it (that self-repair is why the report read as "it fixes itself as soon as Claude writes a line"). ⚠️ **The move is RELATIVE — up `rows - 1 - cursor_y`, then `\r`, then right `cursor_x` — never `CUP`.** `\x1b[;H` numbers rows from the top of the browser's screen, so it lands correctly only while the browser's row count equals `pane_height`, and nothing guarantees that: `resizeWindow` issues its tmux resize fire-and-forget and returns immediately, so a capture can be taken before a requested resize has applied, and `_onSessionNeedsRefresh` sends no resize at all. Counting up from the last replayed row anchors to the content both ends share. Restoring the cursor makes ROW ALIGNMENT load-bearing on this path: **no transform that can DELETE A LINE may run over a full-history capture**, because every deletion shifts the frame out from under the restored position. Four had accumulated — trailing blank rows stripped by `\n+$`, `stripInkRedrawBloat`, the `CLAUDE_BANNER_PATTERN` trim that cuts everything above the banner, and `LEADING_WHITESPACE_PATTERN` — each correct for a byte stream of successive frames and each wrong for a single rendered frame. ⚠️ **Those skips key on `isFullCapture`, meaning a capture actually came back — never on `?full=1` alone.** When `captureActivePaneBuffer` returns null (ENOBUFS, a timeout, a vanished pane, or a session with no mux at all) the reply falls back to `session.terminalBuffer`, which IS a byte stream and must still be stripped; gating on the query flag returned it whole, and a direct-PTY session takes that path on every first selection rather than only during an outage. ⚠️ A capture holding nothing visible (`hasVisibleContent`) returns `''`, because the caller reads an empty capture as "unavailable" and keeps its byte history — retaining trailing blank rows made an all-blank pane non-empty, which would have replaced real history with a blank screen from the server side, where `_replayWouldShrinkBuffer` cannot see it. ⚠️ **"One line per screen row" holds only where no row was hard-wrapped**: `-J` joins a wrapped row into its logical line (measured: a 100-character line in a 40-column pane captures as 10 lines against a 12-row pane), and the counts reconcile only once the browser xterm re-wraps at the same width — the same assumption `_estimateReplayRows` already documents. Tests: `test/tmux-capture-full-history.test.ts` covers the cursor move, the trim pairing and `hasVisibleContent`; `test/routes/session-routes.test.ts` covers a surviving blank first row, an unstripped byte-history fallback, and an empty capture leaving history intact. ⚠️ **The re-pull must never DOWNGRADE the buffer** (#205 round 2): the same reasoning that makes it a win for a shell pane makes it destructive for a repaint-mode CLI pane, where tmux keeps no history of its own (`history_size≈0` measured for a Claude pane) and the capture is roughly ONE frame while xterm may hold hundreds of rows of replayed frames — `_resetTerminalForReplay()` + rewrite then deletes history mid-scroll ("goes back a bit, repeats blocks, gets worse the further up I go"; measured A/B on a live pane: 341 rows → 42 with the guard off). `_replayWouldShrinkBuffer()` (terminal-ui.js) estimates the capture's rendered rows — escape sequences stripped, `capture-pane -J` re-wrapping accounted for — and the pull is skipped when that is more than one screen short of `buffer.active.length`. The one-screen tolerance matters: both sides are estimates (the buffer length counts trailing blank rows), so only a clear downgrade is refused. A refused session joins `_fullHistoryRepullUseless`, raising its cooldown from 4s to 60s so a hollow pane stops re-fetching megabytes on every scroll-up. Tests: `test/tmux-capture-full-history.test.ts`, `test/tmux-scrollback-eol.test.ts`, `test/terminal-scroll-routing.test.ts`, `test/terminal-flush-budget.test.ts`. -**A capture reports the geometry it was taken at** (#435): a visible frame repaints each row at an absolute position, counting up to the pane's height and out to the pane's width, so a terminal smaller than that pane damages it two ways at once. Too short and every address past the browser's own height clamps onto the last line, overwriting the rows underneath (measured: against a 50-row pane, a 30-row terminal rendered 28 of a 45-line command and drew the survivors twice). Too narrow and each row is painted out to the pane's width, so the browser wraps every painted row and the wrap on the last one scrolls the whole frame up by one. Nothing in the response used to say what geometry the frame was built for, so the client could not see either case. `PaneCaptureOptions.capturedGeometry` carries it out, and the terminal response publishes it as `captureCols`/`captureRows`. ⚠️ **Both fields are ABSENT unless a frame was really positioned**, and every consumer must test `Number.isFinite` rather than truthiness: `mux-visible` is necessary but not sufficient, because when the `display-message` cursor query fails `capturePaneBuffer` skips the snapshot repaint and returns the raw capture, and the route still labels that non-empty body `mux-visible`. A body that positioned nothing has no geometry to describe and nothing to repair, so a comparison that fires there buys a second capture, a reset plus chunked rewrite, a dropped and reopened WebSocket and a discarded xterm snapshot for no gain. ⚠️ **The comparison runs on `mux-visible` ONLY.** A full-history body is linear scrollback closed by a RELATIVE cursor move, which is relative precisely so the browser's row count need not match the pane's, and a byte-history body carries no row alignment at all, so a size mismatch damages neither and a replay repairs neither. That gate matters because the first select of every non-shell session per page takes the full-history path, where an ungated comparison would fire most often on the one response it cannot help, at the price of a second whole-scrollback capture. ⚠️ **The replay is capped at one attempt and latches per session when it cannot converge.** `resizeRetry` stops two competing fits trading replays forever; a pane already drawing at the size just requested is left alone, which is the signature of a clamp rather than a race (`getTerminalDimensions()` floors at 40x10 while `fitAddon.fit()` does not, so a terminal under 40 columns or 10 rows reports a pane permanently bigger than itself and would replay on every tab switch); and a pass that still does not converge joins `_geometryRetryUseless`, so the case `Session.resize` declines outright (a small viewport while a desktop viewport's size claim is live, where the retry re-sends the same declined resize and captures the same pane) costs one attempt per session per page load instead of one per select. ⚠️ **A retry pass must not re-arm `_fullHistoryLoaded`**: it did not consume the full-history pull, and re-arming it would spend a whole-scrollback capture on the next select. That branch is currently unreachable by construction, since reaching it needs `source === 'mux-visible'` while a `full=1` pass is answered `mux-full-history` or `history`; a static test over the source is the habit this repo uses for an invariant nothing can execute. Tests: `test/capture-geometry-retry.browser.test.ts` (eight cases, five of which fail against the merge base), `test/tmux-capture-full-history.test.ts`, `test/routes/session-routes.test.ts`. +**A capture reports the geometry it was taken at** (#435): a visible frame repaints each row at an absolute position, counting up to the pane's height and out to the pane's width, so a terminal smaller than that pane damages it two ways at once. Too short and every address past the browser's own height clamps onto the last line, overwriting the rows underneath (measured: against a 50-row pane, a 30-row terminal rendered 28 of a 45-line command and drew the survivors twice). Too narrow and each row is painted out to the pane's width, so the browser wraps every painted row and the wrap on the last one scrolls the whole frame up by one. Nothing in the response used to say what geometry the frame was built for, so the client could not see either case. `PaneCaptureOptions.capturedGeometry` carries it out, and the terminal response publishes it as `captureCols`/`captureRows`. ⚠️ **Both fields are ABSENT unless a frame was really positioned**, and every consumer must test `Number.isFinite` rather than truthiness: `mux-visible` is necessary but not sufficient, because when the `display-message` cursor query fails `capturePaneBuffer` skips the snapshot repaint and returns the raw capture, and the route still labels that non-empty body `mux-visible`. A body that positioned nothing has no geometry to describe and nothing to repair, so a comparison that fires there buys a second capture, a reset plus chunked rewrite, a dropped and reopened WebSocket and a discarded xterm snapshot for no gain. ⚠️ **The comparison runs on `mux-visible` ONLY.** A full-history body is linear scrollback closed by a RELATIVE cursor move, which is relative precisely so the browser's row count need not match the pane's, and a byte-history body carries no row alignment at all, so a size mismatch damages neither and a replay repairs neither. That gate matters because the first select of every non-shell session per page takes the full-history path, where an ungated comparison would fire most often on the one response it cannot help, at the price of a second whole-scrollback capture. ⚠️ **The replay is capped at one attempt and latches per session when it cannot converge.** `resizeRetry` stops two competing fits trading replays forever; a pane already drawing at the size just requested is left alone, because a retry would capture the identical frame; and a pass that still does not converge joins `_geometryRetryUseless`, so the case `Session.resize` declines outright (a small viewport while a desktop viewport's size claim is live, where the retry re-sends the same declined resize and captures the same pane) costs one attempt per session per page load instead of one per select. ⚠️ **The clamp used to manufacture that equality, and no longer can** (#464). This paragraph previously explained it as the signature of a clamp: `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 would replay on every tab switch. That divergence is fixed at the source — `syncTerminalGeometry()` (terminal-ui.js) fits, floors and APPLIES in one step, so the browser terminal IS the size it reports. The only remaining reason the two can differ is a resize the server declined, which the server now reports back (`Session.ptyGeometry`, the `{"t":"zc"}` frame and the resize response) for the client to adopt by COLUMNS. The equality guard stays, for the plain case of a pane already at the requested size. ⚠️ **A retry pass must not re-arm `_fullHistoryLoaded`**: it did not consume the full-history pull, and re-arming it would spend a whole-scrollback capture on the next select. That branch is currently unreachable by construction, since reaching it needs `source === 'mux-visible'` while a `full=1` pass is answered `mux-full-history` or `history`; a static test over the source is the habit this repo uses for an invariant nothing can execute. Tests: `test/capture-geometry-retry.browser.test.ts` (eight cases, five of which fail against the merge base), `test/tmux-capture-full-history.test.ts`, `test/routes/session-routes.test.ts`. ### Terminal scrollback: strip flavors and wheel/touch forwarding diff --git a/package-lock.json b/package-lock.json index 18f5c92b..0e620e34 100644 --- a/package-lock.json +++ b/package-lock.json @@ -55,6 +55,7 @@ "@types/web-push": "^3.6.4", "@types/ws": "^8.18.1", "@vitest/coverage-v8": "^4.1.8", + "@xterm/headless": "^6.0.0", "agent-browser": "^0.6.0", "esbuild": "^0.27.3", "eslint": "^9.0.0", diff --git a/package.json b/package.json index fdf53528..7e49f1f5 100644 --- a/package.json +++ b/package.json @@ -123,6 +123,7 @@ "@types/web-push": "^3.6.4", "@types/ws": "^8.18.1", "@vitest/coverage-v8": "^4.1.8", + "@xterm/headless": "^6.0.0", "agent-browser": "^0.6.0", "esbuild": "^0.27.3", "eslint": "^9.0.0", diff --git a/src/session.ts b/src/session.ts index 2b448c92..8773f3af 100644 --- a/src/session.ts +++ b/src/session.ts @@ -3780,19 +3780,26 @@ export class Session extends EventEmitter { private _ptyRows = 40; /** - * The geometry the CLI is actually drawing for. + * The geometry the CLI is actually drawing for, or null when nothing is + * drawing. * * 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. + * the width it was told (issue #464). Both transports report this back. + * + * ⚠️ NULL WITHOUT A PANE, never the field values. `resize()` writes + * `_ptyCols`/`_ptyRows` only when `ptyProcess` is set, and nothing seeds them + * from the spawn geometry, so a session with a dead pane — or one created + * through the API and never started — still holds the constructor defaults + * of 120x40. Reporting those made a client adopt a size no process had ever + * been told, and on anything narrower than 120 columns it claimed another + * device owned the pane when none existed. `reconcilePtyGeometry` treats a + * report with no finite numbers as no evidence, which is the truth here. */ - get ptyCols(): number { - return this._ptyCols; - } - get ptyRows(): number { - return this._ptyRows; + get ptyGeometry(): { cols: number; rows: number } | null { + return this.ptyProcess ? { cols: this._ptyCols, rows: this._ptyRows } : null; } /** diff --git a/src/web/public/app.js b/src/web/public/app.js index f8b9e3a0..eb2ae7ea 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -2722,13 +2722,6 @@ 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 @@ -2737,6 +2730,16 @@ class CodemanApp { this.sendResize(this.activeSessionId); } } + // ⚠️ HERE: after a response arrived, and NOT in the `finally`. The marker + // means "this session lost output", and only a reconcile that actually + // completed settles it. Clearing on every exit meant one that threw — or + // hit the fetch deadline, which is the flaky-link case the marker exists + // for — dropped the gap silently with nothing to retry it. + // ⚠️ Outside the `if (data.terminalBuffer)` too: a server that answers + // with an empty capture HAS reconciled us, there was simply nothing to + // replay. Leaving the marker set there refetched on every reconnect for + // the life of the page. + this._markTerminalBufferReconciled(sessionId); } catch (err) { console.error('needsRefresh reload failed:', err); } finally { diff --git a/src/web/public/constants.js b/src/web/public/constants.js index 1ddf0a46..d79ad46a 100644 --- a/src/web/public/constants.js +++ b/src/web/public/constants.js @@ -1717,23 +1717,28 @@ function terminalGeometryAgrees(a, b) { * 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. + * the asking client ever being told. A terminal that keeps its own WIDTH after + * such a refusal renders garbage, because Ink wraps its frame and counts its + * erase rows at the width it was told. * - * Correct-and-reachable beats correct-and-clipped beats garbled, so a pane - * wider than the viewport also earns horizontal reach — see `.pty-oversized`. + * ⚠️ COLUMNS ONLY. Rows are deliberately left alone, and adopting them was a + * real regression: a phone that took a desktop's 43 rows into a viewport with + * room for 18 painted an `.xterm-screen` far taller than its container, and + * because xterm's own viewport then had nothing to scroll, the bottom of the + * frame — the CLI's input line — sat below the container with no gesture that + * could reach it. Output visible, typing invisible, for as long as the claim + * stayed hot. Width is the axis the wrap arithmetic depends on; rows only + * decide how much is on screen at once, and keeping the local row count keeps + * the composer at the bottom of a viewport that scrolls. * * @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}} + * @returns {{adopt: boolean, cols: number|null}} */ 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 (!pty || !Number.isFinite(pty.cols)) return { adopt: false, cols: null }; + if (!local || !Number.isFinite(local.cols) || local.cols === pty.cols) return { adopt: false, cols: null }; + return { adopt: true, cols: pty.cols }; } if (typeof window !== 'undefined') { diff --git a/src/web/public/styles.css b/src/web/public/styles.css index 44627912..d11550f5 100644 --- a/src/web/public/styles.css +++ b/src/web/public/styles.css @@ -3817,41 +3817,36 @@ 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. */ +/* The terminal is wider than the box that shows it (issue #464). Two causes, + one affordance: another device holds the sizing claim so this terminal has + adopted a width it did not ask for, or the 40-column floor has widened it + past a narrow container. Either way the text is rendered CORRECTLY and simply + does not fit, and without horizontal reach the right-hand columns sit behind + .terminal-container's clip with no gesture that can get to them — measured at + 360px, font 24: 218px of the pane, 38% of it, unreachable. + Present only while that is true; _syncTerminalOverflowAffordance() owns it. */ +.terminal-container.term-overflows-x { + /* ⚠️ BOTH axes, explicitly. mobile.css loads after this file and sets + `.terminal-container { overflow: visible }` — 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 { +.terminal-container.term-overflows-x .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; -} +/* ⚠️ NO `touch-action: pan-x` here, deliberately. The terminal's own touchmove + handler pans this container (see `canPanHorizontally` in terminal-ui.js), + because `touchstart` preventDefault()s every 'content' tap and that cancels + a native pan before it can start. Granting the browser pan-x as well would + double-handle the gestures where that preventDefault does NOT run — a tap on + a scrolled-up viewport — moving the pane twice for one finger. The + `touch-action: none` the other rules set is what keeps JS the sole owner. */ /* Touch devices: prevent browser from claiming the touch gesture before our JS touchmove handler fires. Without this, the browser starts native diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index ef008af2..d3ff4e4b 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -764,6 +764,32 @@ Object.assign(CodemanApp.prototype, { let longPressStartX = 0; let longPressStartY = 0; let touchStartY = 0; + let touchStartX = 0; + // 'x' | 'y' | null — locked on the first travel past the tap slop, so a + // diagonal drag cannot pan and scroll at the same time. + let panAxis = null; + /** + * Can this gesture pan sideways? Only while the terminal is wider than + * the box showing it (`.term-overflows-x`, set by + * `_syncTerminalOverflowAffordance`). + * + * ⚠️ This has to be done in JS. `touch-action: pan-x` alone does nothing + * for the sessions the affordance targets: `touchstart` calls + * preventDefault() for every 'content' tap — the normal case for a + * mouse-tracking TUI sitting at the bottom of its buffer — which cancels + * the browser's pan before it starts. Measured under touch emulation, a + * 140px horizontal swipe reached scrollLeft 141 without that + * preventDefault and 0 with it. It only ever worked for shell sessions, + * while scrolled up, or with a mouse. + */ + const canPanHorizontally = () => + // Both halves. The class is what makes the container a scroller at all + // (`overflow-x: auto`); without it `scrollLeft` silently stays 0, and a + // gesture locked to 'x' on that basis would do nothing AND suppress the + // vertical scroll it should have been. The measurement is the second + // half because sub-pixel cell widths can leave a stray pixel of + // scrollWidth on a terminal that fits perfectly well. + container.classList.contains('term-overflows-x') && container.scrollWidth - container.clientWidth > 1; let tapStartedWithTerminalFocus = false; let tapStartIntentCache = null; // px — ignore micro-drift to distinguish tap from scroll. Shared with the @@ -783,6 +809,8 @@ Object.assign(CodemanApp.prototype, { touchLastX = ev.touches[0].clientX; touchLastY = ev.touches[0].clientY; touchStartY = touchLastY; + touchStartX = touchLastX; + panAxis = null; velocity = 0; pixelAccum = 0; isTouching = true; @@ -851,8 +879,15 @@ Object.assign(CodemanApp.prototype, { } if (ev.touches.length === 1 && isTouching) { const touchY = ev.touches[0].clientY; - if (!didScroll && Math.abs(touchY - touchStartY) >= TAP_THRESHOLD) { - didScroll = true; + const touchX = ev.touches[0].clientX; + if (!didScroll) { + const travelY = Math.abs(touchY - touchStartY); + const travelX = Math.abs(touchX - touchStartX); + const sideways = canPanHorizontally() && travelX >= TAP_THRESHOLD; + if (travelY >= TAP_THRESHOLD || sideways) { + didScroll = true; + panAxis = sideways && travelX > travelY ? 'x' : 'y'; + } } // Below the tap threshold, treat the gesture as a potential tap: // don't preventDefault (iOS needs click synthesis to show the @@ -862,6 +897,15 @@ Object.assign(CodemanApp.prototype, { // fling, so a jittery tap would both position the cursor AND scroll. if (!didScroll) return; ev.preventDefault(); + if (panAxis === 'x') { + // Pan the container, and touch nothing the vertical path owns — + // no pixelAccum, no velocity, so touchend cannot turn a sideways + // swipe into a momentum fling down the scrollback. + container.scrollLeft -= touchX - touchLastX; + touchLastX = touchX; + touchLastY = touchY; + return; + } const delta = touchLastY - touchY; // positive = scroll down pixelAccum += delta; velocity = delta * 1.2; @@ -1087,11 +1131,24 @@ Object.assign(CodemanApp.prototype, { } } if (!sentViaWs) { + // ⚠️ The reply carries the geometry that actually took, and this + // is the path where a declined resize is LEAST likely to be + // noticed: no socket means no `{"t":"zc"}` frame either, so + // discarding it here left the one transport that cannot hear the + // answer also not asking for it. + const resizedSessionId = this.activeSessionId; fetch(`/api/sessions/${this.activeSessionId}/resize`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ cols, rows, viewportType }), - }).catch(() => {}); + }) + .then(async (res) => { + const applied = (await res.json())?.data ?? {}; + this._onPtyGeometryReport(resizedSessionId, applied.cols, applied.rows); + }) + .catch(() => { + /* a resize that never landed tells us nothing about the PTY */ + }); } } } @@ -5454,7 +5511,11 @@ Object.assign(CodemanApp.prototype, { } const dims = this.getTerminalDimensions(); if (!dims) return null; - return this._resizeTerminalTo(dims) ? dims : null; + if (!this._resizeTerminalTo(dims)) return null; + // The floor can leave this terminal wider than the box that shows it, and + // that clips columns with no gesture to reach them (issue #464, item 4). + this._scheduleOverflowAffordanceSync(); + return dims; }, /** @@ -5583,7 +5644,7 @@ Object.assign(CodemanApp.prototype, { * 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 + * `overflow: hidden`, so `.term-overflows-x` 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. * @@ -5594,38 +5655,107 @@ Object.assign(CodemanApp.prototype, { _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; + const { adopt } = window.CodemanTerminalGeometry.reconcilePtyGeometry(local, { cols, rows }); + // Columns only, and the local row count is kept — see reconcilePtyGeometry + // for why adopting rows put the CLI's input line below the container with + // nothing able to scroll to it. + if (adopt && this._resizeTerminalTo({ cols, rows: local.rows })) { + // 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: local.rows }; } - 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); + // Is the PTY at a width this container did not ask for? Compared against + // what we WOULD request, not against what the terminal currently holds: + // once adopted those two are equal, so the second question answers itself + // false and the condition would look resolved while it is still true. + // The floor widens this terminal too, and that is the reader's own font + // setting rather than another device — hence the comparison, not `>`. + const wanted = this.getTerminalDimensions(); + this._paneWidthRefused = !!wanted && Number.isFinite(cols) && cols !== wanted.cols; + this._scheduleOverflowAffordanceSync(); }, /** - * Let the reader reach a pane wider than their screen, and say why once. + * Measure on the NEXT frame, coalesced. * - * 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. + * `terminal.resize()` updates the buffer synchronously but the screen element + * takes its new width with the render, so measuring in the same tick reads + * the size the terminal just left. Coalesced because a settling container + * fires several resizes and only the last one's measurement is the truth. */ - _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'); + _scheduleOverflowAffordanceSync() { + if (typeof requestAnimationFrame !== 'function') { + this._syncTerminalOverflowAffordance(); + return; } + if (this._overflowAffordanceFrame) return; + this._overflowAffordanceFrame = requestAnimationFrame(() => { + this._overflowAffordanceFrame = null; + this._syncTerminalOverflowAffordance(); + }); + }, + + /** + * Let the reader reach a pane wider than the box that shows it. + * + * ⚠️ Keyed on what actually does not FIT, not on a PTY mismatch. Two + * different causes put the terminal wider than its container and both leave + * columns unreachable behind `.terminal-container`'s clip: + * + * - another device holds the sizing claim, so this terminal adopts a width + * it did not ask for; and + * - the 40-column floor. On a 360px phone, font 18 applies 40 columns and + * paints 433px, and font 24 paints 578px — 218px, 38% of the pane, with no + * gesture that could reach it. `increaseFontSize` goes to 24 and applies + * immediately, so that is two taps away, and the PTY agrees with the + * terminal throughout: a mismatch test would never fire. + * + * Measured rather than derived from cell arithmetic, because the cell width + * is fractional and the container's padding is not ours to assume. One pixel + * of slack keeps sub-pixel rounding from flapping the class. + */ + _syncTerminalOverflowAffordance() { + // ⚠️ Nothing in here may throw. It runs off every geometry change, which is + // the resize path, and the affordance is cosmetic: a terminal that cannot + // be measured — disposed mid-resize, or a harness with no real DOM — must + // lose the scroll affordance, never the resize. + let container = null; + let overflows = false; + try { + container = document.getElementById('terminalContainer'); + const screen = container?.querySelector('.xterm-screen'); + if (container && screen) { + overflows = screen.getBoundingClientRect().width - container.clientWidth > 1; + } + } catch { + /* unmeasurable; fall through with the affordance off */ + } + container?.classList.toggle('term-overflows-x', overflows); + // The notice tells the reader to scroll sideways, so it is only true advice + // once there is something to scroll. A wide PTY on a screen wide enough to + // show it needs no explanation and gets none. + if (!overflows || !this._paneWidthRefused) { + this._paneOwnedElsewhere = false; + return; + } + this._notePaneOwnedElsewhere(); + }, + + /** + * Say, once, that this pane's width belongs to another device. + * + * Once per transition, not per report: reports arrive on every resize, and a + * toast that repeats is noise about a situation already on screen. Silent + * when it resolves — the pane simply reflows back to this screen. + */ + _notePaneOwnedElsewhere() { + if (this._paneOwnedElsewhere) return; + this._paneOwnedElsewhere = true; + // 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'); }, /** diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 549a5052..e442e0b6 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -2119,8 +2119,9 @@ export function registerSessionRoutes( // 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 }; + // output, so the client adopts this (issue #464). A session with no pane + // reports nothing rather than the constructor defaults — see `ptyGeometry`. + return session.ptyGeometry ?? {}; }); // ========== Get Last Response (from transcript JSONL) ========== diff --git a/src/web/routes/ws-routes.ts b/src/web/routes/ws-routes.ts index 6aae1ddb..9cefcb21 100644 --- a/src/web/routes/ws-routes.ts +++ b/src/web/routes/ws-routes.ts @@ -250,8 +250,11 @@ export function registerWsRoutes(app: FastifyInstance, ctx: SessionPort, getHost // 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}}`); + // A session with no pane sends nothing at all: its `_ptyCols`/ + // `_ptyRows` are constructor defaults no process was ever told. + const applied = session.ptyGeometry; + if (applied && socket.readyState === 1) { + socket.send(`{"t":"zc","c":${applied.cols},"r":${applied.rows}}`); } } } catch { diff --git a/test/capture-geometry-retry.browser.test.ts b/test/capture-geometry-retry.browser.test.ts index 1f8301de..2a127e78 100644 --- a/test/capture-geometry-retry.browser.test.ts +++ b/test/capture-geometry-retry.browser.test.ts @@ -136,9 +136,10 @@ async function stubTerminalDynamic( /** * Answer every fetch with the geometry the client itself is asking for, read * live from the page. That is the clamp signature: `getTerminalDimensions()` - * floors at 40x10 while `fitAddon.fit()` does not, so a small enough viewport - * makes the pane permanently bigger than the terminal at a size the client - * requested itself. + * floors at 40x10, and since #464 `syncTerminalGeometry()` applies that floor + * to xterm too — so at a small enough viewport the pane, the report and the + * terminal all agree on the floored size, which is the case the equality guard + * is left covering. */ async function stubTerminalAtRequestedSize(page: Page, counter: { n: number; urls: string[] }) { await page.route('**/api/sessions/*/terminal*', async (route) => { @@ -498,13 +499,22 @@ describe('a capture bigger than the terminal', () => { }, 60_000); it('does not replay a pane already at the size the client asked for', async () => { - // `getTerminalDimensions()` floors at 40x10 while `fitAddon.fit()` does - // not, so a viewport this small leaves the terminal shorter than the size - // the client itself requests, and the pane obligingly draws at the floored - // size. The captured height then exceeds the terminal's forever. A replay - // cannot converge, because it re-requests the same floored size and - // captures the same frame, so without the equality guard this retries on - // every tab switch for the life of the page. + // ⚠️ The premise of this case CHANGED with issue #464, and the old one can + // never hold again. It used to be the clamp: `getTerminalDimensions()` + // floors at 40x10 while `fitAddon.fit()` did not, so a viewport this small + // left the terminal shorter than the size the client itself requested, the + // pane drew at the floored size, and the captured height exceeded the + // terminal's forever — a replay that re-requested the same floored size and + // captured the same frame, on every tab switch, for the life of the page. + // + // `syncTerminalGeometry()` now applies the floor to xterm as well, so the + // browser terminal IS the size it reports and that divergence is gone at + // the source. The case survives on its own terms — a pane already drawing + // at the requested size must not be replayed, because the retry would + // capture the identical frame — and its premise is now the #464 invariant + // itself, asserted below: the floored report and the terminal agree. That + // is a stronger guard than the old one, since the clamp coming back would + // fail it here rather than silently restoring the replay loop. context = await browser.newContext({ viewport: { width: 320, height: 200 } }); page = await context.newPage(); const sessionId = await openSession(page); @@ -514,8 +524,9 @@ describe('a capture bigger than the terminal', () => { await consumeFullHistory(page, sessionId, fetches); await select(page, sessionId, { forceReload: true }); - // The premise: the floor really does bind here. Without this the case - // would pass on any viewport, proving nothing. + // The premise: the floor really does bind at this viewport — otherwise the + // case would pass on any viewport, proving nothing — AND the terminal holds + // exactly what it reports, which is what stops the old replay loop. const requested = await page.evaluate( () => ( @@ -523,7 +534,20 @@ describe('a capture bigger than the terminal', () => { ).app.getTerminalDimensions?.() ?? null ); expect(requested).not.toBeNull(); - expect(requested!.rows).toBeGreaterThan(await terminalRows(page)); + const proposed = await page.evaluate( + () => + ( + window as unknown as { app: { fitAddon?: { proposeDimensions?: () => { cols: number; rows: number } } } } + ).app.fitAddon?.proposeDimensions?.() ?? null + ); + expect(proposed, 'the terminal could not be measured').not.toBeNull(); + expect( + proposed!.rows < requested!.rows || proposed!.cols < requested!.cols, + `the floor must bind at this viewport, or the case proves nothing (proposed ${proposed!.cols}x${proposed!.rows}, reported ${requested!.cols}x${requested!.rows})` + ).toBe(true); + // The #464 invariant: what the client reports is what the terminal holds. + expect(requested!.rows).toBe(await terminalRows(page)); + expect(requested!.cols).toBe(await terminalCols(page)); expect(fetches.n).toBe(1); diff --git a/test/detached-session-pane-sizing.test.ts b/test/detached-session-pane-sizing.test.ts index 10b7e4a6..05a5a1e0 100644 --- a/test/detached-session-pane-sizing.test.ts +++ b/test/detached-session-pane-sizing.test.ts @@ -66,6 +66,11 @@ function makeApp(overrides: Record = {}) { // now, so the harness must let it (#464). syncTerminalGeometry: mixin.syncTerminalGeometry, _resizeTerminalTo: mixin._resizeTerminalTo, + // Real, so a geometry change really does re-check whether the terminal now + // overflows its container (#464 item 4) — the fake DOM has no container, so + // it measures nothing and settles on "no overflow", which is the truth here. + _scheduleOverflowAffordanceSync: mixin._scheduleOverflowAffordanceSync, + _syncTerminalOverflowAffordance: mixin._syncTerminalOverflowAffordance, _onPtyGeometryReport: vi.fn(), getTerminalDimensions: () => ({ cols: 120, rows: 40 }), terminal: { cols: 120, rows: 40, resize: vi.fn() }, diff --git a/test/session-resize-arbitration.test.ts b/test/session-resize-arbitration.test.ts index 5f82e527..568acb8e 100644 --- a/test/session-resize-arbitration.test.ts +++ b/test/session-resize-arbitration.test.ts @@ -19,6 +19,37 @@ function attachFakePty(session: Session, cols = 160, rows = 48) { return resize; } +describe('Session.ptyGeometry', () => { + // ⚠️ `resize()` writes `_ptyCols`/`_ptyRows` only when `ptyProcess` is set, + // and nothing seeds them from the spawn geometry — so the fields hold the + // constructor defaults of 120x40 for any session whose pane is not running. + // Reporting those to a client made it adopt a width no process had ever been + // told, and on anything narrower than 120 columns claim another device owned + // the pane when none existed (issue #464). + it('reports nothing for a session that has no pane', () => { + const session = new Session({ workingDir: '/tmp', mode: 'shell' }); + expect(session.ptyGeometry).toBeNull(); + }); + + it('still reports nothing after a resize it could not apply', () => { + const session = new Session({ workingDir: '/tmp', mode: 'shell' }); + session.resize(45, 20, { viewportType: 'mobile' }); + // The resize was swallowed (no pty to resize), so there is no geometry to + // report — NOT the 120x40 the fields still hold. + expect(session.ptyGeometry).toBeNull(); + }); + + it('reports the pane geometry once a pane exists, and follows a resize', () => { + // The contrast, so "always null" would fail this. + const session = new Session({ workingDir: '/tmp', mode: 'shell' }); + attachFakePty(session, 160, 48); + expect(session.ptyGeometry).toEqual({ cols: 160, rows: 48 }); + + session.resize(62, 40, { viewportType: 'mobile' }); + expect(session.ptyGeometry).toEqual({ cols: 62, rows: 40 }); + }); +}); + describe('Session resize arbitration', () => { it('lets a mobile-only session shrink below the spawn default (no desktop connected)', () => { const session = new Session({ workingDir: '/tmp', mode: 'shell' }); diff --git a/test/terminal-pty-geometry.test.ts b/test/terminal-pty-geometry.test.ts index 2017e167..57d30ca7 100644 --- a/test/terminal-pty-geometry.test.ts +++ b/test/terminal-pty-geometry.test.ts @@ -161,39 +161,40 @@ describe('clampTerminalDimensions', () => { 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('does nothing when the terminal already has the PTY\u2019s width', () => { + expect(reconcilePtyGeometry({ cols: 62, rows: 40 }, { cols: 62, rows: 40 })).toEqual({ adopt: false, cols: null }); }); - it('adopts a PTY the client never asked for — a declined resize is still the truth', () => { + it('adopts a width the client never asked for \u2014 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, - }); + expect(reconcilePtyGeometry({ cols: 62, rows: 40 }, { cols: 120, rows: 40 })).toEqual({ adopt: true, cols: 120 }); }); - 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, - }); + // \u26a0\ufe0f The regression this pins: adopting the PTY's ROWS put a phone that took + // a desktop's 43 into a viewport with room for 18, which painted the CLI's + // input line below the container with nothing able to scroll to it. Width is + // the axis the wrap arithmetic needs; rows only decide how much is on screen. + it('never asks for the PTY\u2019s rows, however far off they are', () => { + for (const ptyRows of [43, 4, 400]) { + const out = reconcilePtyGeometry({ cols: 62, rows: 18 }, { cols: 120, rows: ptyRows }); + expect(out).toEqual({ adopt: true, cols: 120 }); + expect(out).not.toHaveProperty('rows'); + } + }); + + it('does nothing when only the rows differ', () => { + expect(reconcilePtyGeometry({ cols: 62, rows: 40 }, { cols: 62, rows: 12 })).toEqual({ adopt: false, cols: null }); + }); + + it('adopts a narrower PTY too \u2014 the width it was told is the width it draws for', () => { + expect(reconcilePtyGeometry({ cols: 120, rows: 40 }, { cols: 80, rows: 40 })).toEqual({ adopt: true, cols: 80 }); }); 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, - }); + // A session with no pane answers `{}` (Session.ptyGeometry is null), and an + // older server answers `{}` too. Neither is evidence about any PTY. + for (const bad of [null, {}, { rows: 40 }, { cols: 'wide', rows: 40 }]) { + expect(reconcilePtyGeometry({ cols: 62, rows: 40 }, bad as Partial)).toEqual({ adopt: false, cols: null }); } }); }); @@ -313,15 +314,78 @@ describe('exactly one function may change the terminal size', () => { }); }); +describe('the failed-load notice fits the narrowest terminal this app will render', () => { + // ⚠️ `e587d845`'s commit message claimed a test asserted this against the + // BUILT asset. It did not: that assertion lived in a throwaway probe that was + // deleted with the rest of the scratch scripts, so the claim was wrong when it + // was written. This is the real one, and it reads the source rather than + // `dist/`, because `dist/` is not committed and a test that skips when it is + // absent would pass for the wrong reason in CI. + const app = read('src/web/public/app.js'); + const { TERMINAL_MIN_COLS } = loadGeometry(); + + /** The literal the catch writes, escapes resolved, SGR stripped. */ + function noticeLines(): string[] { + const at = app.indexOf('if (clearedBeforeFresh && this.terminal)'); + expect(at, 'the failed-load branch is gone — renamed?').toBeGreaterThan(-1); + // Anchored past the comments: one of them quotes a lone '.', which a + // first-quote match happily returns instead of the notice. + const writeAt = app.indexOf('this.terminal.write(', at); + expect(writeAt, 'the failed-load branch no longer writes to the terminal').toBeGreaterThan(-1); + const call = app.slice(writeAt, app.indexOf('\n }', writeAt)); + const literal = call.match(/'((?:[^'\\]|\\.)*)'/); + expect(literal, 'no string literal in the failed-load branch').not.toBeNull(); + return literal![1] + .replace(/\\x1b\[[0-9;]*m/g, '') + .split('\\r\\n') + .filter((line) => line.trim().length > 0); + } + + it('says what failed, that the session lives, and what to do', () => { + const lines = noticeLines(); + expect(lines.length).toBe(3); + expect(lines[0]).toMatch(/did not load/i); + expect(lines[1]).toMatch(/live output/i); + // A dead end is the most expensive defect here: the pane is blank and the + // reader has no idea whether the session is recoverable. + expect(lines[2], 'the notice must name the next step').toMatch(/reload/i); + }); + + it('never wraps, down to the 40-column floor', () => { + // A 52-character sentence measured at 320px wrapped and left a lone '.' on + // a line of its own. The floor is the narrowest this app renders, and it is + // two taps away on a small phone via increaseFontSize. + for (const line of noticeLines()) { + expect( + line.length, + `"${line}" is ${line.length} columns, over the ${TERMINAL_MIN_COLS} floor` + ).toBeLessThanOrEqual(TERMINAL_MIN_COLS); + } + }); + + it('does not tell the reader to reopen the tab, which retries nothing', () => { + // selectSession early-returns when the session is already active, so + // clicking the tab you are already on does not re-fetch. + expect(app).toContain('if (this.activeSessionId === sessionId && !forceReload)'); + expect(noticeLines().join(' ')).not.toMatch(/reopen|switch tab/i); + }); +}); + // ─────────────────────────────────────────────────────────────────────────── // Resize stopped being write-only. // ─────────────────────────────────────────────────────────────────────────── describe('the server reports the geometry the PTY actually holds', () => { - it('Session exposes the effective dimensions', () => { + it('Session reports its geometry only while a pane is actually drawing', () => { 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;/); + // ⚠️ `resize()` writes _ptyCols/_ptyRows only when ptyProcess is set and + // nothing seeds them from the spawn geometry, so a dead-pane session still + // holds the constructor defaults of 120x40. Reporting those made a client + // adopt a size no process was ever told, and claim another device owned the + // pane when none existed. + expect(session).toMatch(/get ptyGeometry\(\): \{ cols: number; rows: number \} \| null \{/); + expect(session).toContain('return this.ptyProcess ? { cols: this._ptyCols, rows: this._ptyRows } : null;'); + expect(session, 'the raw getters would report the defaults again').not.toMatch(/get ptyCols\(\)/); }); it('the WebSocket answers a resize with what took', () => { @@ -330,8 +394,9 @@ describe('the server reports the geometry the PTY actually holds', () => { 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'); + expect(after).toContain('const applied = session.ptyGeometry;'); + // No pane, no frame at all. + expect(after).toContain('if (applied && socket.readyState === 1)'); // Documented in the protocol block at the top of the file, like every other frame. expect(ws).toContain('{"t":"zc","c":N,"r":N}'); }); @@ -341,7 +406,7 @@ describe('the server reports the geometry the PTY actually holds', () => { 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 };'); + expect(handler).toContain('return session.ptyGeometry ?? {};'); }); it('the client adopts the report and re-bases its dedupe on it', () => { @@ -350,10 +415,11 @@ describe('the server reports the geometry the PTY actually holds', () => { expect(start).toBeGreaterThan(-1); const body = terminalUi.slice(start, terminalUi.indexOf('\n },', start)); expect(body).toContain('reconcilePtyGeometry'); - expect(body).toContain('this._resizeTerminalTo({ cols, rows })'); + // Columns only: the local row count is carried through untouched. + expect(body).toContain('this._resizeTerminalTo({ cols, rows: local.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 }'); + expect(body).toContain('this._lastResizeDims = { cols, rows: local.rows }'); // And the WS frame is wired up at all. expect(read('src/web/public/app.js')).toContain("msg.t === 'zc'"); }); @@ -370,15 +436,30 @@ describe('the server reports the geometry the PTY actually holds', () => { const body = css.slice(at + selector.length, css.indexOf('\n}', at)); return body.replace(/\/\*[\s\S]*?\*\//g, ''); }; - const oversized = declarationsOf('.terminal-container.pty-oversized'); + const oversized = declarationsOf('.terminal-container.term-overflows-x'); 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)"); + // ⚠️ NO touch-action here, deliberately. `touch-action: pan-x` does nothing + // for the sessions this targets — `touchstart` preventDefault()s every + // 'content' tap, which cancels the browser's pan before it starts — and + // granting it as well as the JS pan would move the pane twice for one + // finger on the taps where that preventDefault does not run. The terminal's + // own touchmove handler owns both axes; mobile.css's unscoped + // `touch-action: none` is what keeps it the only owner. + expect(css.slice(css.indexOf('.terminal-container.term-overflows-x'))).not.toMatch( + /\.terminal-container\.term-overflows-x[^{]*\{[^}]*touch-action/ + ); + expect(read('src/web/public/terminal-ui.js')).toContain('const canPanHorizontally = () =>'); + expect(read('src/web/public/terminal-ui.js')).toContain("if (panAxis === 'x') {"); + // The class is only ever on while the terminal really is too wide, and it + // is MEASURED rather than derived: the floor widens the terminal past a + // narrow container with the PTY agreeing throughout, so a mismatch test + // would never fire for it (360px, font 24: 218px unreachable). + expect(read('src/web/public/terminal-ui.js')).toContain("classList.toggle('term-overflows-x', overflows)"); + expect(read('src/web/public/terminal-ui.js')).toContain( + 'screen.getBoundingClientRect().width - container.clientWidth > 1' + ); }); });