From 3edf9aae2f53ccb2c940eacc5d102f7b1069a029 Mon Sep 17 00:00:00 2001 From: Michael Grundberg Date: Tue, 15 Sep 2026 12:42:55 +0200 Subject: [PATCH 1/5] fix(terminal): replay a pane capture at the geometry it was taken at A visible-frame capture repaints each row at an absolute position, counting up to the pane's height. A terminal shorter than that clamps every address past its own height onto its last line. The overflow rows then overwrite one another, and the rows underneath are lost. Replaying a real 50-row capture into a 30-row terminal rendered 28 lines of a 45-line command and drew the frame twice. Nothing in the response said what height the frame was built for, so the client could not detect this. A capture now reports the geometry it was really taken at through `capturedGeometry` on `PaneCaptureOptions`, and the terminal response carries it as `captureCols` and `captureRows`. When the captured pane is taller than the terminal, or the size that produced the capture did not survive the load, `selectSession` replays once at the size that stuck. `resizeRetry` caps that at one attempt, so two competing fits cannot trade replays forever. The retry re-arms the full-history flag only when the pass that ran had consumed it. A tab switch takes the bounded tail, so its retry takes the tail too: clearing the flag unconditionally would upgrade that switch into a fresh scrollback capture the user never asked for, which the route's own comments put at tens of megabytes. What this repairs is a capture that won a race against the resize meant to precede it. It does not repair a capture whose pane was too tall because `Session.resize` declined the resize outright, which it does for a small viewport while a desktop viewport's size claim is live. The retry re-sends the same declined resize and captures the same pane, and `resizeRetry` then stops it. Repairing that means changing who owns the pane size, which is a policy question this does not touch. The reported geometry still helps there, because the client can see the mismatch at all rather than being blind to it. Follows #395, #396 and #397, which fixed the other ways the replayed frame and the terminal could disagree. Co-Authored-By: Claude Opus 5 (1M context) --- .../fix-report-the-captured-pane-geometry.md | 26 +++ config/test-suites.ts | 1 + src/mux-interface.ts | 9 + src/session.ts | 14 ++ src/tmux-manager.ts | 5 + src/web/public/app.js | 51 +++++ src/web/routes/session-routes.ts | 23 +- test/capture-geometry-retry.browser.test.ts | 202 ++++++++++++++++++ test/mocks/mock-session.ts | 13 +- test/routes/session-routes.test.ts | 80 ++++++- test/tmux-capture-full-history.test.ts | 52 ++++- 11 files changed, 462 insertions(+), 14 deletions(-) create mode 100644 .changeset/fix-report-the-captured-pane-geometry.md create mode 100644 test/capture-geometry-retry.browser.test.ts diff --git a/.changeset/fix-report-the-captured-pane-geometry.md b/.changeset/fix-report-the-captured-pane-geometry.md new file mode 100644 index 00000000..37f08cfa --- /dev/null +++ b/.changeset/fix-report-the-captured-pane-geometry.md @@ -0,0 +1,26 @@ +--- +"aicodeman": patch +--- + +fix(terminal): replay a pane capture at the geometry it was taken at + +A visible-frame capture repaints each row at an absolute position, counting up +to the pane's height. A terminal shorter than that clamps every address past +its own height onto its last line, so the overflow rows overwrite one another +and the rows underneath are lost. Against a 50-row pane, a 30-row terminal +rendered 28 of a 45-line command and drew the surviving frame twice. + +Nothing in the response said what height the frame was built for, so the client +could not detect this. A capture now reports the geometry it was really taken at +through `capturedGeometry` on `PaneCaptureOptions`, and the terminal response +carries it as `captureCols` and `captureRows`. When the captured pane is taller +than the terminal, or the size that produced the capture did not survive the +load, `selectSession` replays once at the size that stuck. `resizeRetry` caps +that at one attempt, so two competing fits cannot trade replays forever. + +That repairs the case where a capture won a race against the resize meant to +precede it. It does not repair a capture whose pane was too tall because +`Session.resize` declined the resize outright, which it does for a small +viewport while a desktop viewport's size claim is live: the retry re-sends the +same declined resize and captures the same pane. The reported geometry still +helps there, because the client can see the mismatch at all. diff --git a/config/test-suites.ts b/config/test-suites.ts index 233e0e7d..c0c444af 100644 --- a/config/test-suites.ts +++ b/config/test-suites.ts @@ -28,6 +28,7 @@ export const BROWSER_TEST_GLOBS = [ 'test/terminal-copy-shortcut.test.ts', 'test/terminal-keycode229-recovery.browser.test.ts', 'test/capture-load-window.browser.test.ts', + 'test/capture-geometry-retry.browser.test.ts', 'test/codex-predictive-echo.test.ts', // also needs a real codex binary ]; diff --git a/src/mux-interface.ts b/src/mux-interface.ts index e1104d15..f76ca47a 100644 --- a/src/mux-interface.ts +++ b/src/mux-interface.ts @@ -159,6 +159,15 @@ export interface PaneCaptureOptions { * the 1MB execSync default (ENOBUFS). */ maxCaptureBytes?: number; + /** + * Filled in by the implementation with the pane geometry the capture was + * really taken at, which is not always the geometry the caller last asked + * for: a resize and a capture can race, and a pane whose size a desktop + * viewport has claimed ignores a smaller client's resize outright. A + * visible-frame capture addresses every row absolutely, so a consumer + * rendering it needs the real height to know the frame fits. + */ + capturedGeometry?: { cols: number; rows: number }; } /** diff --git a/src/session.ts b/src/session.ts index 16720e59..092650b4 100644 --- a/src/session.ts +++ b/src/session.ts @@ -3737,6 +3737,20 @@ export class Session extends EventEmitter { private _ptyCols = 120; private _ptyRows = 40; + /** + * The geometry the pane is currently drawing at. A caller that captures the + * pane needs this to report the size the frame was built for, and `resize` + * can decline a small viewport's request while a desktop claim is live, so + * the last size asked for is not always the size in force. + */ + 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) diff --git a/src/tmux-manager.ts b/src/tmux-manager.ts index c7040a0d..da2b267a 100644 --- a/src/tmux-manager.ts +++ b/src/tmux-manager.ts @@ -3482,6 +3482,11 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer { { encoding: 'utf-8', timeout: EXEC_TIMEOUT_MS } ) ); + // Report the size the pane was really drawing at. Both replay paths below + // address rows absolutely, so a consumer whose terminal is shorter than + // this piles every overflow row onto its last line and loses the rows it + // overwrote. Only the caller can see both sizes, so hand it this one. + if (opts && geometry) opts.capturedGeometry = { cols: geometry.cols, rows: geometry.rows }; if (fullHistory) { // Without geometry there is no cursor move, so fall back to the old trim. diff --git a/src/web/public/app.js b/src/web/public/app.js index bb40152d..c81e1078 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -6272,6 +6272,10 @@ class CodemanApp { // sendResize is a no-op on the server when dims haven't changed, so // calling it every tab switch is cheap. const dimsChanged = await this.sendResize(sessionId, { forceHttp: true }).catch(() => false); + // The size the capture below will be taken against. The debounced resize + // handler can move the terminal again while the load runs, so this is a + // recorded value rather than a later read of `_lastResizeDims`. + const dimsAtCapture = this.getTerminalDimensions?.(); if (this._isStaleSelect(selectGen)) { this._clearTerminalLoadState(sessionId, selectGen); return; @@ -6520,6 +6524,29 @@ class CodemanApp { // annoyance that disappear on the user's next keypress; data loss is not // acceptable. Do NOT re-introduce Ctrl+L here. this.sendResize(sessionId); + // sendResize fits synchronously before its first await, so this reads the + // size that survived the load rather than the one the capture was taken + // at. The two differ whenever the terminal was still settling. + const dimsAfterLoad = this.getTerminalDimensions?.(); + const sizeMovedUnderLoad = + !!dimsAtCapture && + !!dimsAfterLoad && + (dimsAfterLoad.cols !== dimsAtCapture.cols || dimsAfterLoad.rows !== dimsAtCapture.rows); + // A capture positions every row absolutely, so a pane taller than this + // terminal writes its overflow rows onto the last line and loses the rows + // it overwrote. That happens when the capture wins a race against the + // resize meant to precede it, which is what the retry below repairs. + // + // It also happens when `Session.resize` DECLINED the resize, which it does + // for a small viewport while a desktop viewport's size claim is live. The + // retry cannot repair that one: it re-sends the same declined resize and + // captures the same too-tall pane. `resizeRetry` stops it after the one + // extra attempt, and the frame is shown as-is. Repairing that case means + // changing who owns the pane size, which is a policy question this does + // not touch. What the flag does buy there is that the client can SEE the + // mismatch at all, which it previously could not. + const capturedTallerThanTerminal = + Number.isFinite(data.captureRows) && data.captureRows > (this.terminal?.rows || 0); // Defer secondary panel updates so they don't block the main thread // after terminal content is already visible. @@ -6620,6 +6647,30 @@ class CodemanApp { this._clearTerminalLoadState(sessionId, selectGen); _crashDiag.log(`SELECT_DONE: ${selectDoneMs.toFixed(0)}ms`); console.log(`[CRASH-DIAG] selectSession DONE: ${sessionId.slice(0,8)} in ${selectDoneMs.toFixed(0)}ms`); + // What is on screen was drawn for a geometry this terminal does not have. + // Replaying once against the size that stuck is the only thing that + // repairs it: SIGWINCH reaches the CLI only on a real size change, and + // the pane is already at its final size, so no redraw is coming. + // `resizeRetry` caps this at one attempt, so two competing fits cannot + // trade replays forever. + if ( + (sizeMovedUnderLoad || capturedTallerThanTerminal) && + !options?.resizeRetry && + !this._isStaleSelect(selectGen) + ) { + _crashDiag.log( + `RESIZE_RETRY: capture ${data.captureCols}x${data.captureRows} vs terminal ` + + `${this.terminal?.cols}x${this.terminal?.rows}` + + (sizeMovedUnderLoad ? ' (size moved under load)' : '') + ); + // Re-arm the full-history pull ONLY if this pass actually used one, so + // the retry replays the same content at the geometry that stuck. A pass + // that took the bounded tail must retry on the tail too: clearing the + // flag unconditionally would UPGRADE a tab switch into a fresh + // multi-megabyte scrollback capture it never asked for. + if (useFullHistory) this._fullHistoryLoaded.delete(sessionId); + await this.selectSession(sessionId, { auto: true, forceReload: true, resizeRetry: true }); + } } catch (err) { if (this._isLoadingBuffer) this._finishBufferLoad(bufferLoadOwner); this._restoringFlushedState = false; diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 607ec540..2ecb3272 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -30,6 +30,7 @@ import { type OmpConfig, } from '../../types.js'; import { Session, isAltScreenStripMode, isExternalCliMode, isMuxAltScreenOnlyStripMode } from '../../session.js'; +import type { PaneCaptureOptions } from '../../mux-interface.js'; import { SseEvent } from '../sse-events.js'; import { webviewCapabilities } from '../../webview-capabilities.js'; import { @@ -2632,14 +2633,16 @@ export function registerSessionRoutes( // returns null when unavailable, in which case we fall back to history. const muxName = session.muxName; const captureStartedAt = performance.now(); + // The visible path used to pass no options at all. It passes one now for a + // single reason: `capturedGeometry` comes BACK on it, and the response has + // to tell the client what size the frame it is about to render was built + // for. See PaneCaptureOptions.capturedGeometry. + const captureOpts: PaneCaptureOptions = isFullReload + ? { fullHistory: true, historyLimitLines: tmuxHistoryLimit, maxCaptureBytes: terminalBufferMaxBytes } + : {}; const liveMuxBuffer = muxName && typeof ctx.mux.captureActivePaneBuffer === 'function' - ? ctx.mux.captureActivePaneBuffer( - muxName, - isFullReload - ? { fullHistory: true, historyLimitLines: tmuxHistoryLimit, maxCaptureBytes: terminalBufferMaxBytes } - : undefined - ) + ? ctx.mux.captureActivePaneBuffer(muxName, captureOpts) : null; const captureFinishedAt = performance.now(); const hasLiveMuxBuffer = liveMuxBuffer !== null && liveMuxBuffer.length > 0; @@ -2785,6 +2788,14 @@ export function registerSessionRoutes( // what existed before the cut. The gap is what the indicator reports. retainedBytes: cleanBuffer.length, source, + // The pane geometry this frame was drawn for. A visible-frame capture + // positions every row absolutely, so a client whose terminal has fewer + // rows than this overwrites its last line with the overflow and loses + // the rows underneath. The client compares these against its own size. + // Falls back to the session's own geometry when the capture reported + // none (cursor query failed, or the buffer came from byte history). + captureCols: captureOpts.capturedGeometry?.cols ?? session.ptyCols, + captureRows: captureOpts.capturedGeometry?.rows ?? session.ptyRows, }; }); diff --git a/test/capture-geometry-retry.browser.test.ts b/test/capture-geometry-retry.browser.test.ts new file mode 100644 index 00000000..376784fd --- /dev/null +++ b/test/capture-geometry-retry.browser.test.ts @@ -0,0 +1,202 @@ +/** + * @fileoverview A capture drawn for a taller pane makes the client replay once. + * + * A visible-frame capture repaints each row at an absolute position, counting + * up to the PANE's height. A terminal shorter than that clamps every address + * past its own height onto its last line, so the overflow rows overwrite one + * another and the rows underneath are lost. The client cannot see that from + * the escape sequence, so the terminal response reports the geometry the + * capture was taken at (`captureCols`/`captureRows`) and `selectSession` + * replays once at the size that stuck. + * + * These drive the REAL client in chromium and stub only the terminal endpoint, + * because the mismatch itself needs two viewports to stage against live tmux. + * Without the fix the first assertion below sees one fetch instead of two. + * + * Port: 3252 (capture geometry retry) + * + * Run: npx vitest run --config config/vitest.browser.config.ts test/capture-geometry-retry.browser.test.ts + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { chromium, type Browser, type BrowserContext, type Page } from 'playwright'; +import { WebServer } from '../src/web/server.js'; + +const PORT = 3252; +const BASE_URL = `http://localhost:${PORT}`; + +let server: WebServer; +let browser: Browser; + +beforeAll(async () => { + server = new WebServer(PORT, false, true); // testMode + await server.start(); + browser = await chromium.launch({ headless: true }); +}, 60_000); + +afterAll(async () => { + await browser?.close(); + await server?.stop(); +}, 30_000); + +/** A visible-frame capture: one absolutely-addressed paint per row. */ +function paneSnapshot(rows: number): string { + const parts: string[] = []; + for (let row = 1; row <= rows; row++) parts.push(`\x1b[${row};1Hprobe-row-${row}`); + parts.push(`\x1b[${rows};6H`); + return parts.join(''); +} + +/** + * Serve every terminal fetch from a stub reporting `captureRows`, counting the + * fetches. The real route needs live tmux to produce a mismatched frame. + */ +async function stubTerminal(page: Page, captureRows: number, counter: { n: number; urls: string[] }) { + await page.route('**/api/sessions/*/terminal*', async (route) => { + counter.n += 1; + counter.urls.push(route.request().url()); + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ + success: true, + data: { + terminalBuffer: paneSnapshot(captureRows), + status: 'idle', + fullSize: 1024, + retainedBytes: 1024, + truncated: false, + truncationReason: null, + source: 'mux-visible', + captureCols: 200, + captureRows, + }, + }), + }); + }); +} + +async function openSession(page: Page): Promise { + await page.goto(BASE_URL, { waitUntil: 'domcontentloaded' }); + await page.waitForFunction(() => document.body.classList.contains('app-loaded'), { timeout: 10_000 }); + // xterm is loaded from /vendor, so the terminal appears a beat after the app. + // Without it `app.terminal.rows` reads 0 and every height comparison below + // would pass vacuously. + await page.waitForFunction(() => (window as unknown as { app?: { terminal?: unknown } }).app?.terminal, null, { + timeout: 30_000, + }); + return page.evaluate(async () => { + const res = await fetch('/api/sessions', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ workingDir: '/tmp', name: 'capture-geometry-test' }), + }); + const body = await res.json(); + return body.data?.session?.id ?? body.data?.id ?? body.id; + }); +} + +/** The terminal is sized by the first select, so this only reads after one. */ +async function terminalRows(page: Page): Promise { + return page.evaluate(() => (window as unknown as { app: { terminal?: { rows: number } } }).app.terminal?.rows ?? 0); +} + +async function select(page: Page, sessionId: string, options: object = {}): Promise { + await page.evaluate( + async ({ sid, opts }) => { + const app = (window as unknown as { app: { selectSession: (id: string, o?: object) => Promise } }).app; + await app.selectSession(sid, opts); + }, + { sid: sessionId, opts: options } + ); + await page.waitForTimeout(1500); +} + +async function closeSession(page: Page, sessionId: string): Promise { + await page.evaluate( + (sid: string) => fetch(`/api/sessions/${sid}`, { method: 'DELETE' }).then(() => undefined), + sessionId + ); +} + +describe('a capture taller than the terminal', () => { + let context: BrowserContext; + let page: Page; + + afterAll(async () => { + await context?.close(); + }); + + it('replays once when the captured pane is taller, and stops at one retry', async () => { + context = await browser.newContext({ viewport: { width: 1280, height: 800 } }); + page = await context.newPage(); + const sessionId = await openSession(page); + expect(sessionId).toBeTruthy(); + + // 200 rows is taller than any terminal this viewport can produce, so the + // trigger is the captured height alone and not a size that moved. + const fetches = { n: 0, urls: [] as string[] }; + await stubTerminal(page, 200, fetches); + await select(page, sessionId); + + // The terminal is sized by that select, so the premise is checkable now. + expect(await terminalRows(page)).toBeLessThan(200); + // One original load plus exactly one retry. `resizeRetry` caps it there: + // the retry's own response reports the same mismatch, so an uncapped + // implementation would loop. + expect(fetches.n).toBe(2); + + await closeSession(page, sessionId); + await context.close(); + }, 60_000); + + it('retries at the same scope the first pass used, not a wider one', async () => { + // The retry re-arms the full-history flag only when the pass that ran had + // consumed it. A tab switch takes the bounded tail, so its retry must take + // the tail too; clearing the flag unconditionally would upgrade it into a + // fresh multi-megabyte scrollback capture the user never asked for. + context = await browser.newContext({ viewport: { width: 1280, height: 800 } }); + page = await context.newPage(); + const sessionId = await openSession(page); + + const fetches = { n: 0, urls: [] as string[] }; + await stubTerminal(page, 200, fetches); + + // First select: a fresh session, so this one legitimately pulls full history + // and its retry may do the same. + await select(page, sessionId); + const afterFirst = fetches.n; + expect(afterFirst).toBe(2); + + // Re-select the SAME session. `selectSession` early-returns on an already + // active session unless forceReload is set, and forceReload is the shape a + // tab switch back to this session takes: `_fullHistoryLoaded` still holds + // it, so neither this pass nor its retry should ask for full history again. + await select(page, sessionId, { forceReload: true }); + const tabSwitchUrls = fetches.urls.slice(afterFirst); + expect(tabSwitchUrls.length).toBe(2); + expect(tabSwitchUrls.filter((u) => u.includes('full=1'))).toHaveLength(0); + + await closeSession(page, sessionId); + await context.close(); + }, 60_000); + + it('does not replay when the captured pane fits the terminal', async () => { + context = await browser.newContext({ viewport: { width: 1280, height: 800 } }); + page = await context.newPage(); + const sessionId = await openSession(page); + + // Five rows is shorter than any terminal this viewport can produce, so the + // frame fits, nothing is clamped, and nothing needs repeating. A retry here + // would double the work of every tab switch. + const fetches = { n: 0, urls: [] as string[] }; + await stubTerminal(page, 5, fetches); + await select(page, sessionId); + + expect(await terminalRows(page)).toBeGreaterThan(5); + expect(fetches.n).toBe(1); + + await closeSession(page, sessionId); + await context.close(); + }, 60_000); +}); diff --git a/test/mocks/mock-session.ts b/test/mocks/mock-session.ts index 4666afe0..5f13b4cc 100644 --- a/test/mocks/mock-session.ts +++ b/test/mocks/mock-session.ts @@ -373,8 +373,19 @@ export class MockSession extends EventEmitter { /** Stub for sendInput */ sendInput = vi.fn(); + /** + * The geometry the pane is drawing at, which the terminal route reports on + * every response so a client can tell whether the frame fits its own + * terminal. The stubbed `resize` records it the way the real one does. + */ + ptyCols = 120; + ptyRows = 40; + /** Stub for resize */ - resize = vi.fn(); + resize = vi.fn((cols: number, rows: number) => { + this.ptyCols = cols; + this.ptyRows = rows; + }); /** Stubs for the desktop sizing claims used by resize arbitration */ claimDesktopSizing = vi.fn(); diff --git a/test/routes/session-routes.test.ts b/test/routes/session-routes.test.ts index 763a4f8c..f802a871 100644 --- a/test/routes/session-routes.test.ts +++ b/test/routes/session-routes.test.ts @@ -775,7 +775,60 @@ describe('session-routes', () => { body.data.terminalBuffer.indexOf('visible tmux pane only') ); // No ?full=1 → visible-frame capture (no fullHistory opts). - expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith(harness.ctx._session.muxName, undefined); + expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith( + harness.ctx._session.muxName, + expect.not.objectContaining({ fullHistory: true }) + ); + }); + + // ── The geometry a capture was taken at ── + // + // A visible-frame capture repaints each row at an absolute position + // (`\x1b[;1H`). A terminal with fewer rows than the pane clamps every + // address past its own height onto its last line, so the overflow rows + // overwrite each other and the rows they land on are lost. The client can + // only notice that if the response says what height the frame was built + // for, which is what captureRows/captureCols carry. + + it('reports the geometry the capture was really taken at', async () => { + harness.ctx._session.terminalBuffer = ''; + (harness.ctx.mux as { captureActivePaneBuffer?: unknown }).captureActivePaneBuffer = vi.fn( + (_name: string, opts?: { capturedGeometry?: { cols: number; rows: number } }) => { + // Stand in for TmuxManager, which fills this from the pane itself. + if (opts) opts.capturedGeometry = { cols: 100, rows: 50 }; + return 'visible frame'; + } + ); + + const res = await harness.app.inject({ + method: 'GET', + url: `/api/sessions/${harness.ctx._sessionId}/terminal`, + }); + + const body = JSON.parse(res.body); + expect(body.data.source).toBe('mux-visible'); + expect(body.data.captureCols).toBe(100); + expect(body.data.captureRows).toBe(50); + }); + + it('falls back to the session geometry when the capture reports none', async () => { + // The cursor query can fail, and a byte-history response never captures + // at all. The session's own PTY size is the best answer available, and a + // missing field would read as "no mismatch" and suppress the client's + // repair. + harness.ctx._session.terminalBuffer = 'byte history only'; + (harness.ctx.mux as { captureActivePaneBuffer?: unknown }).captureActivePaneBuffer = vi.fn(() => null); + harness.ctx._session.resize(111, 44, { force: true }); + + const res = await harness.app.inject({ + method: 'GET', + url: `/api/sessions/${harness.ctx._sessionId}/terminal`, + }); + + const body = JSON.parse(res.body); + expect(body.data.source).toBe('history'); + expect(body.data.captureCols).toBe(111); + expect(body.data.captureRows).toBe(44); }); // ── COD-47: full tmux scrollback replay on full page reload ── @@ -985,7 +1038,10 @@ describe('session-routes', () => { expect(res.statusCode).toBe(200); const body = JSON.parse(res.body); // Tail/tab-switch must NOT request fullHistory (undefined opts). - expect(captureSpy).toHaveBeenCalledWith(harness.ctx._session.muxName, undefined); + expect(captureSpy).toHaveBeenCalledWith( + harness.ctx._session.muxName, + expect.not.objectContaining({ fullHistory: true }) + ); expect(body.data.terminalBuffer).toContain('visible frame only'); expect(body.data.terminalBuffer).not.toContain('FULL_HISTORY_SHOULD_NOT_APPEAR'); expect(body.data.source).toBe('mux-visible'); @@ -1045,7 +1101,10 @@ describe('session-routes', () => { expect(body.data.terminalBuffer.indexOf('hello world')).toBeLessThan( body.data.terminalBuffer.indexOf('visible tmux pane only') ); - expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith(harness.ctx._session.muxName, undefined); + expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith( + harness.ctx._session.muxName, + expect.not.objectContaining({ fullHistory: true }) + ); }); it('preserves one-time OAuth authorization URLs in Codex TUI replay history', async () => { @@ -1119,7 +1178,10 @@ describe('session-routes', () => { expect(body.data.terminalBuffer.indexOf('hello world')).toBeLessThan( body.data.terminalBuffer.indexOf('visible tmux pane only') ); - expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith(harness.ctx._session.muxName, undefined); + expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith( + harness.ctx._session.muxName, + expect.not.objectContaining({ fullHistory: true }) + ); }); it('uses live mux pane capture only when the accumulated buffer is empty', async () => { @@ -1138,7 +1200,10 @@ describe('session-routes', () => { const body = JSON.parse(res.body); expect(body.data.terminalBuffer).toContain('visible restored tmux pane'); expect(body.data.terminalBuffer).toContain('› current prompt'); - expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith(harness.ctx._session.muxName, undefined); + expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith( + harness.ctx._session.muxName, + expect.not.objectContaining({ fullHistory: true }) + ); }); it('returns error for unknown session', async () => { @@ -1166,7 +1231,10 @@ describe('session-routes', () => { expect(buf).toContain('\x1b[H\x1b[2J'); expect(buf).toContain('LIVE-PANE-FRAME'); expect(buf.indexOf('history-bytes')).toBeLessThan(buf.indexOf('LIVE-PANE-FRAME')); - expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith(harness.ctx._session.muxName, undefined); + expect(harness.ctx.mux.captureActivePaneBuffer).toHaveBeenCalledWith( + harness.ctx._session.muxName, + expect.not.objectContaining({ fullHistory: true }) + ); }); it('falls back to the byte history when no live pane buffer is available', async () => { diff --git a/test/tmux-capture-full-history.test.ts b/test/tmux-capture-full-history.test.ts index 2a829ccd..4460f75a 100644 --- a/test/tmux-capture-full-history.test.ts +++ b/test/tmux-capture-full-history.test.ts @@ -11,7 +11,7 @@ import { readFileSync } from 'node:fs'; import { resolve } from 'node:path'; import { describe, expect, it } from 'vitest'; -import { formatCursorRestore, hasVisibleContent } from '../src/tmux-manager.js'; +import { formatCursorRestore, formatPaneSnapshot, hasVisibleContent } from '../src/tmux-manager.js'; describe('tmux full-history pane capture (COD-47)', () => { const source = readFileSync(resolve(import.meta.dirname, '../src/tmux-manager.ts'), 'utf8'); @@ -121,3 +121,53 @@ describe('hasVisibleContent', () => { expect(hasVisibleContent('\x1b[m \x1b[0m\n\x1b[m x \x1b[0m')).toBe(true); }); }); + +describe('the geometry a capture reports back', () => { + const source = readFileSync(resolve(import.meta.dirname, '../src/tmux-manager.ts'), 'utf8'); + const methodStart = source.indexOf('capturePaneBuffer(muxName: string'); + const methodEnd = source.indexOf('captureActivePaneBuffer(muxName: string', methodStart); + const methodBody = source.slice(methodStart, methodEnd); + + it('writes the pane size onto the caller options before either replay path returns', () => { + // IS_TEST_MODE no-ops execSync, so assert from source (same approach as the + // capture-flag tests above). The write must precede the fullHistory branch: + // both paths return from inside it, and a caller that got no geometry + // cannot tell a mismatched frame from a matching one. + const write = methodBody.indexOf('opts.capturedGeometry = { cols: geometry.cols, rows: geometry.rows }'); + // Anchor on the REPLAY branch, not the earlier `if (fullHistory)` that only + // sizes the exec buffer. + const replayBranch = methodBody.indexOf('if (!geometry) return normalizeScrollbackEol('); + const visibleReturn = methodBody.indexOf('if (geometry) return formatPaneSnapshot('); + expect(write).toBeGreaterThan(-1); + expect(replayBranch).toBeGreaterThan(-1); + expect(visibleReturn).toBeGreaterThan(-1); + expect(write).toBeLessThan(replayBranch); + expect(write).toBeLessThan(visibleReturn); + }); + + it('reports nothing when the cursor query gave no geometry', () => { + // `queryPaneCursor` returns null on a failed or nonsensical query, and the + // snapshot repaint is skipped in that case. Reporting a size anyway would + // describe a frame that was never positioned. + expect(methodBody).toContain('if (opts && geometry)'); + }); +}); + +describe('why a capture has to report its height', () => { + it('a snapshot addresses rows the receiving terminal may not have', () => { + // formatPaneSnapshot positions every row absolutely. A terminal shorter + // than the pane clamps each address past its own height onto its last + // line, so the overflow rows overwrite one another and the rows underneath + // are lost. Nothing in the escape sequence tells the client this happened — + // hence captureRows on the response. + const lines = Array.from({ length: 50 }, (_, i) => `row-${i + 1}`); + // cursorX 5 keeps the trailing cursor-restore move (`\x1b[50;6H`) out of the + // `;1H` row-paint match below, so the count is row paints alone. + const snapshot = formatPaneSnapshot(lines, { cols: 100, rows: 50, cursorX: 5, cursorY: 49 }); + const addressed = [...snapshot.matchAll(/\x1b\[(\d+);1H/g)].map((m) => Number(m[1])); + + expect(Math.max(...addressed)).toBe(50); + // A 30-row terminal cannot honour 20 of those addresses. + expect(addressed.filter((row) => row > 30)).toHaveLength(20); + }); +}); From 5cfb98fb8b81cea28a3708303ff59ce1bd1397be Mon Sep 17 00:00:00 2001 From: Michael Grundberg Date: Fri, 18 Sep 2026 16:45:37 +0200 Subject: [PATCH 2/5] fix(terminal): compare capture geometry only on a visible-frame response Only a visible-frame capture positions its rows absolutely, so only that frame can be damaged by a terminal of the wrong size. A `full=1` 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 `history` body is the byte stream, which carries no row alignment to protect. The geometry comparison ran on all three, so it fired most often on the one response it cannot help: `_fullHistoryLoaded` is empty on the first select of every non-shell session per page, and a session whose pane a desktop tab holds too tall to ever fit then paid a second whole-scrollback capture, reset and replay on every page load and every first tab switch. `framePositionsRowsAbsolutely` gates both the captured-geometry comparison and `sizeMovedUnderLoad`. A size that moved under a byte-stream or scrollback replay is healed by xterm's own reflow plus the SIGWINCH the trailing `sendResize` already sends. A pane WIDER than the terminal damages the same frame a second way, so `captureCols` is now compared rather than only logged. `formatPaneSnapshot` paints each row out to the pane's own width, so a narrower browser wraps every painted row, and the wrap on the last one scrolls the whole frame up by a row. The terminal response no longer falls back to `session.ptyCols`/`ptyRows` when the capture reported no geometry. The cursor query is what produces the absolute addressing in the first place, so a capture that lost it returned a raw frame that was never positioned, and a byte-history response was never positioned either. Naming the session's own PTY size there described a frame that does not exist and invited a repair for damage that is not present. `_ptyCols` is also written only by `resize()` while the PTY is spawned at the size queried from tmux, so it can be wrong on its own terms. Both fields are now absent instead, and the `Session` getters added for that fallback go with it. Two browser cases cover the new behaviour and each fails without its fix: a `mux-full-history` response with both dimensions mismatched asserts one fetch (two without the gate), and a `mux-visible` response wider than the terminal but short enough to fit asserts two (one without the width comparison). Corrects a claim in the comment above `capturedGeometry` in tmux-manager.ts. Both replay paths do not address rows absolutely; the full-history one ends in a relative move, which is the whole reason the gate is right. Co-Authored-By: Claude Opus 5 (1M context) --- .../fix-report-the-captured-pane-geometry.md | 28 +++-- src/session.ts | 14 --- src/tmux-manager.ts | 11 +- src/web/public/app.js | 33 +++++- src/web/routes/session-routes.ts | 14 ++- test/capture-geometry-retry.browser.test.ts | 101 ++++++++++++++++-- test/mocks/mock-session.ts | 13 +-- test/routes/session-routes.test.ts | 14 +-- 8 files changed, 165 insertions(+), 63 deletions(-) diff --git a/.changeset/fix-report-the-captured-pane-geometry.md b/.changeset/fix-report-the-captured-pane-geometry.md index 37f08cfa..c36c4592 100644 --- a/.changeset/fix-report-the-captured-pane-geometry.md +++ b/.changeset/fix-report-the-captured-pane-geometry.md @@ -10,13 +10,27 @@ its own height onto its last line, so the overflow rows overwrite one another and the rows underneath are lost. Against a 50-row pane, a 30-row terminal rendered 28 of a 45-line command and drew the surviving frame twice. -Nothing in the response said what height the frame was built for, so the client -could not detect this. A capture now reports the geometry it was really taken at -through `capturedGeometry` on `PaneCaptureOptions`, and the terminal response -carries it as `captureCols` and `captureRows`. When the captured pane is taller -than the terminal, or the size that produced the capture did not survive the -load, `selectSession` replays once at the size that stuck. `resizeRetry` caps -that at one attempt, so two competing fits cannot trade replays forever. +A pane wider than the terminal damages the same frame a second way. Each row is +painted out to the pane's own width, so a narrower terminal wraps every painted +row, and the wrap on the last one scrolls the whole frame up by a row. + +Nothing in the response said what geometry the frame was built for, so the client +could not detect either case. A capture now reports the geometry it was really +taken at through `capturedGeometry` on `PaneCaptureOptions`, and the terminal +response carries it as `captureCols` and `captureRows`. When the captured pane is +taller or wider than the terminal, or the size that produced the capture did not +survive the load, `selectSession` replays once at the size that stuck. +`resizeRetry` caps that at one attempt, so two competing fits cannot trade +replays forever. + +That comparison runs on a visible-frame response only. A full-history response is +linear scrollback closed by a relative cursor move, and a byte-history response +carries no row alignment at all, so a size mismatch damages neither and a replay +repairs neither. Gating on the source matters because the first load of every +non-shell session per page takes the full-history path, where an ungated +comparison would capture the whole tmux scrollback a second time. A response +whose capture reported no geometry now omits both fields rather than naming the +session's own PTY size, which describes no frame that was ever positioned. That repairs the case where a capture won a race against the resize meant to precede it. It does not repair a capture whose pane was too tall because diff --git a/src/session.ts b/src/session.ts index 092650b4..16720e59 100644 --- a/src/session.ts +++ b/src/session.ts @@ -3737,20 +3737,6 @@ export class Session extends EventEmitter { private _ptyCols = 120; private _ptyRows = 40; - /** - * The geometry the pane is currently drawing at. A caller that captures the - * pane needs this to report the size the frame was built for, and `resize` - * can decline a small viewport's request while a desktop claim is live, so - * the last size asked for is not always the size in force. - */ - 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) diff --git a/src/tmux-manager.ts b/src/tmux-manager.ts index da2b267a..bc02fac9 100644 --- a/src/tmux-manager.ts +++ b/src/tmux-manager.ts @@ -3482,10 +3482,13 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer { { encoding: 'utf-8', timeout: EXEC_TIMEOUT_MS } ) ); - // Report the size the pane was really drawing at. Both replay paths below - // address rows absolutely, so a consumer whose terminal is shorter than - // this piles every overflow row onto its last line and loses the rows it - // overwrote. Only the caller can see both sizes, so hand it this one. + // Report the size the pane was really drawing at. The visible-frame path + // below addresses every row absolutely, so a consumer whose terminal is + // shorter than this piles the overflow rows onto its last line and loses + // the rows it overwrote. The full-history path instead ends in a RELATIVE + // cursor move, which costs it nothing when the two sizes disagree, so the + // geometry is reported there for diagnosis rather than for repair. Only + // the caller can see both sizes, so hand it this one. if (opts && geometry) opts.capturedGeometry = { cols: geometry.cols, rows: geometry.rows }; if (fullHistory) { diff --git a/src/web/public/app.js b/src/web/public/app.js index c81e1078..9928ae22 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -6528,14 +6528,30 @@ class CodemanApp { // size that survived the load rather than the one the capture was taken // at. The two differ whenever the terminal was still settling. const dimsAfterLoad = this.getTerminalDimensions?.(); + // Only a visible-frame capture positions its rows absolutely, and only + // that frame can be damaged by a terminal of the wrong size. A `full=1` + // body is linear scrollback closed by a RELATIVE cursor move + // (`formatCursorRestore`), which is relative precisely so the browser's + // row count need not match the pane's, and a `history` body is the byte + // stream, which carries no row alignment to protect. Replaying either at + // a different size repairs nothing, and the full-history replay costs a + // second whole-scrollback capture to learn that. Since the first select + // of every non-shell session per page takes the full-history path, an + // ungated comparison fires most often on the one response it cannot help. + const framePositionsRowsAbsolutely = data.source === 'mux-visible'; const sizeMovedUnderLoad = + framePositionsRowsAbsolutely && !!dimsAtCapture && !!dimsAfterLoad && (dimsAfterLoad.cols !== dimsAtCapture.cols || dimsAfterLoad.rows !== dimsAtCapture.rows); // A capture positions every row absolutely, so a pane taller than this // terminal writes its overflow rows onto the last line and loses the rows - // it overwrote. That happens when the capture wins a race against the - // resize meant to precede it, which is what the retry below repairs. + // it overwrote. A pane WIDER than this terminal damages the same frame a + // second way: `formatPaneSnapshot` paints each row out to the pane's own + // width, so a narrower browser wraps every painted row, and the wrap on + // the last one scrolls the whole frame up by a row. Both happen when the + // capture wins a race against the resize meant to precede it, which is + // what the retry below repairs. // // It also happens when `Session.resize` DECLINED the resize, which it does // for a small viewport while a desktop viewport's size claim is live. The @@ -6545,8 +6561,17 @@ class CodemanApp { // changing who owns the pane size, which is a policy question this does // not touch. What the flag does buy there is that the client can SEE the // mismatch at all, which it previously could not. + // + // An ABSENT field is not a fit. It means the capture reported no geometry + // at all, so nothing was positioned and there is nothing to repair. const capturedTallerThanTerminal = - Number.isFinite(data.captureRows) && data.captureRows > (this.terminal?.rows || 0); + framePositionsRowsAbsolutely && + Number.isFinite(data.captureRows) && + data.captureRows > (this.terminal?.rows || 0); + const capturedWiderThanTerminal = + framePositionsRowsAbsolutely && + Number.isFinite(data.captureCols) && + data.captureCols > (this.terminal?.cols || 0); // Defer secondary panel updates so they don't block the main thread // after terminal content is already visible. @@ -6654,7 +6679,7 @@ class CodemanApp { // `resizeRetry` caps this at one attempt, so two competing fits cannot // trade replays forever. if ( - (sizeMovedUnderLoad || capturedTallerThanTerminal) && + (sizeMovedUnderLoad || capturedTallerThanTerminal || capturedWiderThanTerminal) && !options?.resizeRetry && !this._isStaleSelect(selectGen) ) { diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 2ecb3272..a1cd9007 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -2792,10 +2792,16 @@ export function registerSessionRoutes( // positions every row absolutely, so a client whose terminal has fewer // rows than this overwrites its last line with the overflow and loses // the rows underneath. The client compares these against its own size. - // Falls back to the session's own geometry when the capture reported - // none (cursor query failed, or the buffer came from byte history). - captureCols: captureOpts.capturedGeometry?.cols ?? session.ptyCols, - captureRows: captureOpts.capturedGeometry?.rows ?? session.ptyRows, + // + // BOTH FIELDS ARE ABSENT when the capture reported no geometry, and that + // is the honest answer rather than a gap to paper over: the cursor query + // is what produces the absolute addressing in the first place, so a + // capture that lost it returned a raw frame with no row positioning in + // it, and a byte-history response was never positioned at all. Reporting + // the session's own PTY size here would name a geometry no frame was + // built for and invite the client to repair damage that does not exist. + captureCols: captureOpts.capturedGeometry?.cols, + captureRows: captureOpts.capturedGeometry?.rows, }; }); diff --git a/test/capture-geometry-retry.browser.test.ts b/test/capture-geometry-retry.browser.test.ts index 376784fd..1298873c 100644 --- a/test/capture-geometry-retry.browser.test.ts +++ b/test/capture-geometry-retry.browser.test.ts @@ -1,13 +1,20 @@ /** - * @fileoverview A capture drawn for a taller pane makes the client replay once. + * @fileoverview A capture drawn for a bigger pane makes the client replay once. * * A visible-frame capture repaints each row at an absolute position, counting - * up to the PANE's height. A terminal shorter than that clamps every address - * past its own height onto its last line, so the overflow rows overwrite one - * another and the rows underneath are lost. The client cannot see that from - * the escape sequence, so the terminal response reports the geometry the - * capture was taken at (`captureCols`/`captureRows`) and `selectSession` - * replays once at the size that stuck. + * up to the PANE's height and out to the PANE's width. A terminal shorter than + * that clamps every address past its own height onto its last line, so the + * overflow rows overwrite one another and the rows underneath are lost. A + * narrower terminal wraps every painted row, and the wrap on the last one + * scrolls the whole frame up by one. The client cannot see either from the + * escape sequence, so the terminal response reports the geometry the capture + * was taken at (`captureCols`/`captureRows`) and `selectSession` replays once + * at the size that stuck. + * + * The comparison runs on a `mux-visible` response ONLY. The other two sources + * position no rows absolutely, so a size mismatch damages neither and a replay + * repairs neither, and the last case here pins that the expensive one is left + * alone. * * These drive the REAL client in chromium and stub only the terminal endpoint, * because the mismatch itself needs two viewports to stage against live tmux. @@ -50,8 +57,19 @@ function paneSnapshot(rows: number): string { /** * Serve every terminal fetch from a stub reporting `captureRows`, counting the * fetches. The real route needs live tmux to produce a mismatched frame. + * + * `source` and `captureCols` default to the visible-frame case, which is the + * only response whose rows are addressed absolutely and therefore the only one + * a replay can repair. A test that varies either says so. */ -async function stubTerminal(page: Page, captureRows: number, counter: { n: number; urls: string[] }) { +async function stubTerminal( + page: Page, + captureRows: number, + counter: { n: number; urls: string[] }, + options: { source?: string; captureCols?: number } = {} +) { + const source = options.source ?? 'mux-visible'; + const captureCols = options.captureCols ?? 200; await page.route('**/api/sessions/*/terminal*', async (route) => { counter.n += 1; counter.urls.push(route.request().url()); @@ -67,8 +85,8 @@ async function stubTerminal(page: Page, captureRows: number, counter: { n: numbe retainedBytes: 1024, truncated: false, truncationReason: null, - source: 'mux-visible', - captureCols: 200, + source, + captureCols, captureRows, }, }), @@ -76,6 +94,9 @@ async function stubTerminal(page: Page, captureRows: number, counter: { n: numbe }); } +/** The widest terminal this suite's 1280px viewport can produce, with margin. */ +const WIDER_THAN_ANY_TERMINAL_COLS = 500; + async function openSession(page: Page): Promise { await page.goto(BASE_URL, { waitUntil: 'domcontentloaded' }); await page.waitForFunction(() => document.body.classList.contains('app-loaded'), { timeout: 10_000 }); @@ -101,6 +122,11 @@ async function terminalRows(page: Page): Promise { return page.evaluate(() => (window as unknown as { app: { terminal?: { rows: number } } }).app.terminal?.rows ?? 0); } +/** As above, for the width half of the comparison. */ +async function terminalCols(page: Page): Promise { + return page.evaluate(() => (window as unknown as { app: { terminal?: { cols: number } } }).app.terminal?.cols ?? 0); +} + async function select(page: Page, sessionId: string, options: object = {}): Promise { await page.evaluate( async ({ sid, opts }) => { @@ -190,7 +216,7 @@ describe('a capture taller than the terminal', () => { // frame fits, nothing is clamped, and nothing needs repeating. A retry here // would double the work of every tab switch. const fetches = { n: 0, urls: [] as string[] }; - await stubTerminal(page, 5, fetches); + await stubTerminal(page, 5, fetches, { captureCols: 40 }); await select(page, sessionId); expect(await terminalRows(page)).toBeGreaterThan(5); @@ -199,4 +225,57 @@ describe('a capture taller than the terminal', () => { await closeSession(page, sessionId); await context.close(); }, 60_000); + + it('replays once when the captured pane is wider', async () => { + // A pane wider than the terminal damages the same frame a second way. + // `formatPaneSnapshot` paints every row out to the PANE's width, so a + // narrower browser wraps each painted row, and the wrap on the last row + // scrolls the whole frame up by one. The height here fits deliberately, so + // the width is the only thing that can trigger the replay. + context = await browser.newContext({ viewport: { width: 1280, height: 800 } }); + page = await context.newPage(); + const sessionId = await openSession(page); + + const fetches = { n: 0, urls: [] as string[] }; + await stubTerminal(page, 5, fetches, { captureCols: WIDER_THAN_ANY_TERMINAL_COLS }); + await select(page, sessionId); + + expect(await terminalRows(page)).toBeGreaterThan(5); + expect(await terminalCols(page)).toBeLessThan(WIDER_THAN_ANY_TERMINAL_COLS); + expect(fetches.n).toBe(2); + + await closeSession(page, sessionId); + await context.close(); + }, 60_000); + + it('does not replay a full-history response, whatever geometry it reports', async () => { + // A `full=1` 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. A mismatch there is not damage and a replay cannot repair it, so + // the geometry comparison must not fire on it. This is the path that makes + // the gate worth having: `_fullHistoryLoaded` is empty on the first select + // of every non-shell session per page, so an ungated comparison would pull + // the entire tmux scrollback a second time on every page load and every + // first tab switch, for a session whose pane a desktop tab is holding too + // tall to ever fit. + context = await browser.newContext({ viewport: { width: 1280, height: 800 } }); + page = await context.newPage(); + const sessionId = await openSession(page); + + const fetches = { n: 0, urls: [] as string[] }; + await stubTerminal(page, 200, fetches, { + source: 'mux-full-history', + captureCols: WIDER_THAN_ANY_TERMINAL_COLS, + }); + await select(page, sessionId); + + // Both dimensions are mismatched, so height alone is not what spares it. + expect(await terminalRows(page)).toBeLessThan(200); + expect(await terminalCols(page)).toBeLessThan(WIDER_THAN_ANY_TERMINAL_COLS); + expect(fetches.n).toBe(1); + expect(fetches.urls.filter((u) => u.includes('full=1'))).toHaveLength(1); + + await closeSession(page, sessionId); + await context.close(); + }, 60_000); }); diff --git a/test/mocks/mock-session.ts b/test/mocks/mock-session.ts index 5f13b4cc..4666afe0 100644 --- a/test/mocks/mock-session.ts +++ b/test/mocks/mock-session.ts @@ -373,19 +373,8 @@ export class MockSession extends EventEmitter { /** Stub for sendInput */ sendInput = vi.fn(); - /** - * The geometry the pane is drawing at, which the terminal route reports on - * every response so a client can tell whether the frame fits its own - * terminal. The stubbed `resize` records it the way the real one does. - */ - ptyCols = 120; - ptyRows = 40; - /** Stub for resize */ - resize = vi.fn((cols: number, rows: number) => { - this.ptyCols = cols; - this.ptyRows = rows; - }); + resize = vi.fn(); /** Stubs for the desktop sizing claims used by resize arbitration */ claimDesktopSizing = vi.fn(); diff --git a/test/routes/session-routes.test.ts b/test/routes/session-routes.test.ts index f802a871..7b550560 100644 --- a/test/routes/session-routes.test.ts +++ b/test/routes/session-routes.test.ts @@ -811,14 +811,14 @@ describe('session-routes', () => { expect(body.data.captureRows).toBe(50); }); - it('falls back to the session geometry when the capture reports none', async () => { + it('omits the geometry when the capture reports none', async () => { // The cursor query can fail, and a byte-history response never captures - // at all. The session's own PTY size is the best answer available, and a - // missing field would read as "no mismatch" and suppress the client's - // repair. + // at all. Neither frame was positioned, so neither can be damaged by a + // terminal of the wrong size. Naming the session's own PTY size here + // would describe a geometry no frame was built for, and the client would + // read it as a mismatch worth replaying for. harness.ctx._session.terminalBuffer = 'byte history only'; (harness.ctx.mux as { captureActivePaneBuffer?: unknown }).captureActivePaneBuffer = vi.fn(() => null); - harness.ctx._session.resize(111, 44, { force: true }); const res = await harness.app.inject({ method: 'GET', @@ -827,8 +827,8 @@ describe('session-routes', () => { const body = JSON.parse(res.body); expect(body.data.source).toBe('history'); - expect(body.data.captureCols).toBe(111); - expect(body.data.captureRows).toBe(44); + expect(body.data.captureCols).toBeUndefined(); + expect(body.data.captureRows).toBeUndefined(); }); // ── COD-47: full tmux scrollback replay on full page reload ── From e0d4477edc9dc366806300c5a4564cc3fb9f51be Mon Sep 17 00:00:00 2001 From: Michael Grundberg Date: Fri, 18 Sep 2026 18:53:05 +0200 Subject: [PATCH 3/5] fix(terminal): keep the geometry replay to the pass that can converge Three follow-ups to the source gate, each one measured rather than reasoned. A pane already drawing at the size the client just requested is left alone. The replay runs at `dimsAfterLoad`, so it can only change what is on screen if the pane was drawing at some other size; when the reported geometry already IS that size, the second pass captures the identical frame and pays a full reload to do it, including a visible re-flash, a dropped and reopened WebSocket and 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 replayed on every tab switch without ever converging. A race never produces the equality, since its premise is that the pane was still at the size it was asked to leave. The declined-resize case does not produce it either, so that one still costs the single capped attempt and needs the pane-ownership question this does not touch. The full-history re-arm is unreachable and now says so. A pass that consumed the flag sent `full=1`, and the route answers `full=1` with `mux-full-history` or `history`, never `mux-visible`, so the source gate already rules out every such pass. The line stays for the invariant, but its comment no longer reads as if a page load retries, and the suite pins that it does not. The response no longer reports geometry for a body that carries no capture. The full-history path writes `capturedGeometry` from the cursor query and then returns '' for a pane holding nothing visible, which drops the source to `history` with the geometry already recorded: a `full=1` request whose capture reported 100x50 and returned nothing answered `source: "history"` with both fields set. Nothing acted on it, because the client ignores geometry on any other source, but the field said a frame had been drawn at a size when none had. The browser stub now derives `source` from the request the way the route does, rather than answering `full=1` with `mux-visible`, which the route cannot produce. Each case reaches a visible-frame response the way production does, by not being the first select of the page. Three cases pin the new behaviour and each fails without its guard: the clamp case sees two fetches instead of one, the scope case and the full-history case both see a replay the gate forbids, and the width case sees one fetch instead of two. The changeset now describes the change from 1.29.x rather than the difference between the two commits on this branch. Co-Authored-By: Claude Opus 5 (1M context) --- .../fix-report-the-captured-pane-geometry.md | 63 ++++---- src/web/public/app.js | 30 ++++ src/web/routes/session-routes.ts | 23 +-- test/capture-geometry-retry.browser.test.ts | 134 +++++++++++++++--- test/routes/session-routes.test.ts | 25 ++++ 5 files changed, 219 insertions(+), 56 deletions(-) diff --git a/.changeset/fix-report-the-captured-pane-geometry.md b/.changeset/fix-report-the-captured-pane-geometry.md index c36c4592..c973e359 100644 --- a/.changeset/fix-report-the-captured-pane-geometry.md +++ b/.changeset/fix-report-the-captured-pane-geometry.md @@ -5,36 +5,43 @@ fix(terminal): replay a pane capture at the geometry it was taken at A visible-frame capture repaints each row at an absolute position, counting up -to the pane's height. A terminal shorter than that clamps every address past -its own height onto its last line, so the overflow rows overwrite one another -and the rows underneath are lost. Against a 50-row pane, a 30-row terminal -rendered 28 of a 45-line command and drew the surviving frame twice. +to the pane's height and out to the pane's width. A terminal shorter than that +clamps every address past its own height onto its last line, so the overflow +rows overwrite one another and the rows underneath are lost. Against a 50-row +pane, a 30-row terminal rendered 28 of a 45-line command and drew the surviving +frame twice. A narrower terminal damages the same frame a second way: each row +is painted out to the pane's own width, so the browser wraps every painted row, +and the wrap on the last one scrolls the whole frame up by a row. -A pane wider than the terminal damages the same frame a second way. Each row is -painted out to the pane's own width, so a narrower terminal wraps every painted -row, and the wrap on the last one scrolls the whole frame up by a row. +Nothing in the response said what geometry the frame was built for, so the +client could not detect either case. A capture now reports the geometry it was +really taken at through `capturedGeometry` on `PaneCaptureOptions`, and the +terminal response carries it as `captureCols` and `captureRows`. Both fields are +absent unless the response really carries a capture, since a body that was never +positioned has no geometry to describe. When a captured pane is taller or wider +than the terminal, or the size that produced the capture did not survive the +load, `selectSession` replays once at the size that stuck. -Nothing in the response said what geometry the frame was built for, so the client -could not detect either case. A capture now reports the geometry it was really -taken at through `capturedGeometry` on `PaneCaptureOptions`, and the terminal -response carries it as `captureCols` and `captureRows`. When the captured pane is -taller or wider than the terminal, or the size that produced the capture did not -survive the load, `selectSession` replays once at the size that stuck. -`resizeRetry` caps that at one attempt, so two competing fits cannot trade -replays forever. +That comparison runs on a visible-frame response only. A full-history response +is linear scrollback closed by a relative cursor move, and a byte-history +response carries no row alignment at all, so a size mismatch damages neither and +a replay repairs neither. The distinction matters because the first load of +every non-shell session per page takes the full-history path, where a replay +would capture the whole tmux scrollback a second time. -That comparison runs on a visible-frame response only. A full-history response is -linear scrollback closed by a relative cursor move, and a byte-history response -carries no row alignment at all, so a size mismatch damages neither and a replay -repairs neither. Gating on the source matters because the first load of every -non-shell session per page takes the full-history path, where an ungated -comparison would capture the whole tmux scrollback a second time. A response -whose capture reported no geometry now omits both fields rather than naming the -session's own PTY size, which describes no frame that was ever positioned. +Two guards keep the replay to the one pass that can converge. `resizeRetry` caps +it at a single attempt, so two competing fits cannot trade replays forever. A +pane already drawing at the size the client 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 narrower than 40 columns +or shorter than 10 rows reports a pane permanently bigger than itself and would +otherwise replay on every tab switch without ever converging. -That repairs the case where a capture won a race against the resize meant to -precede it. It does not repair a capture whose pane was too tall because +One case is still reported rather than repaired. A pane can be too tall because `Session.resize` declined the resize outright, which it does for a small -viewport while a desktop viewport's size claim is live: the retry re-sends the -same declined resize and captures the same pane. The reported geometry still -helps there, because the client can see the mismatch at all. +viewport while a desktop viewport's size claim is live. The retry re-sends the +same declined resize and captures the same pane, so it costs the one capped +attempt and the frame is shown as it is. Repairing it means deciding who owns +the pane size while a desktop claim is live, which is a policy question this +does not touch. The reported geometry still helps, because the client can see +the mismatch at all rather than being blind to it. diff --git a/src/web/public/app.js b/src/web/public/app.js index 9928ae22..8abf8bec 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -6572,6 +6572,27 @@ class CodemanApp { framePositionsRowsAbsolutely && Number.isFinite(data.captureCols) && data.captureCols > (this.terminal?.cols || 0); + // The retry replays at `dimsAfterLoad`, so it can only change what is on + // screen if the pane was drawing at some OTHER size. When the reported + // geometry already IS that size, the second pass captures the identical + // frame and pays a full reload to do it: another fetch, another + // `_resetTerminalForReplay()` and chunked rewrite (a visible re-flash), + // 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. + const captureMatchesRequestedSize = + !!dimsAfterLoad && data.captureCols === dimsAfterLoad.cols && data.captureRows === dimsAfterLoad.rows; // Defer secondary panel updates so they don't block the main thread // after terminal content is already visible. @@ -6680,6 +6701,7 @@ class CodemanApp { // trade replays forever. if ( (sizeMovedUnderLoad || capturedTallerThanTerminal || capturedWiderThanTerminal) && + !captureMatchesRequestedSize && !options?.resizeRetry && !this._isStaleSelect(selectGen) ) { @@ -6693,6 +6715,14 @@ class CodemanApp { // that took the bounded tail must retry on the tail too: clearing the // flag unconditionally would UPGRADE a tab switch into a fresh // multi-megabyte scrollback capture it never asked for. + // + // UNREACHABLE as written, and kept for the invariant rather than the + // branch. A `useFullHistory` pass sends `full=1`, and the route answers + // `full=1` with `mux-full-history` or `history`, never `mux-visible` + // (see the source ladder in session-routes.ts), so the gate above + // already rules out every pass that consumed the flag. Do not read this + // line as evidence that a page load retries: it does not, and the test + // suite pins that it does not. if (useFullHistory) this._fullHistoryLoaded.delete(sessionId); await this.selectSession(sessionId, { auto: true, forceReload: true, resizeRetry: true }); } diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index a1cd9007..02d70fd8 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -2793,15 +2793,20 @@ export function registerSessionRoutes( // rows than this overwrites its last line with the overflow and loses // the rows underneath. The client compares these against its own size. // - // BOTH FIELDS ARE ABSENT when the capture reported no geometry, and that - // is the honest answer rather than a gap to paper over: the cursor query - // is what produces the absolute addressing in the first place, so a - // capture that lost it returned a raw frame with no row positioning in - // it, and a byte-history response was never positioned at all. Reporting - // the session's own PTY size here would name a geometry no frame was - // built for and invite the client to repair damage that does not exist. - captureCols: captureOpts.capturedGeometry?.cols, - captureRows: captureOpts.capturedGeometry?.rows, + // BOTH FIELDS ARE ABSENT unless this response really carries a capture, + // and that is the honest answer rather than a gap to paper over. Two + // separate things can leave a frame unpositioned. The cursor query is + // what produces the absolute addressing in the first place, so a capture + // that lost it returned a raw frame with no row positioning in it. And a + // capture can report geometry and STILL hand back nothing: the + // full-history path returns '' for a pane holding nothing visible, which + // drops `source` to `history` while `capturedGeometry` is already + // written, so the geometry has to be suppressed HERE rather than trusted + // to be missing. Naming a size for a body that is the byte stream would + // describe a frame that was never drawn and invite the client to repair + // damage that does not exist. + captureCols: hasLiveMuxBuffer ? captureOpts.capturedGeometry?.cols : undefined, + captureRows: hasLiveMuxBuffer ? captureOpts.capturedGeometry?.rows : undefined, }; }); diff --git a/test/capture-geometry-retry.browser.test.ts b/test/capture-geometry-retry.browser.test.ts index 1298873c..5c5e6419 100644 --- a/test/capture-geometry-retry.browser.test.ts +++ b/test/capture-geometry-retry.browser.test.ts @@ -58,9 +58,12 @@ function paneSnapshot(rows: number): string { * Serve every terminal fetch from a stub reporting `captureRows`, counting the * fetches. The real route needs live tmux to produce a mismatched frame. * - * `source` and `captureCols` default to the visible-frame case, which is the - * only response whose rows are addressed absolutely and therefore the only one - * a replay can repair. A test that varies either says so. + * `source` is DERIVED from the request the way the real route derives it: a + * `full=1` request whose capture came back is `mux-full-history`, and every + * other one is `mux-visible`. The route cannot answer `full=1` with + * `mux-visible`, so a stub that did would stage a combination production never + * produces, and a test resting on it would prove nothing about production. A + * test that needs some other source passes it explicitly and says why. */ async function stubTerminal( page: Page, @@ -68,11 +71,12 @@ async function stubTerminal( counter: { n: number; urls: string[] }, options: { source?: string; captureCols?: number } = {} ) { - const source = options.source ?? 'mux-visible'; const captureCols = options.captureCols ?? 200; await page.route('**/api/sessions/*/terminal*', async (route) => { + const url = route.request().url(); counter.n += 1; - counter.urls.push(route.request().url()); + counter.urls.push(url); + const source = options.source ?? (url.includes('full=1') ? 'mux-full-history' : 'mux-visible'); await route.fulfill({ status: 200, contentType: 'application/json', @@ -94,6 +98,44 @@ async function stubTerminal( }); } +/** + * 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. + */ +async function stubTerminalAtRequestedSize(page: Page, counter: { n: number; urls: string[] }) { + await page.route('**/api/sessions/*/terminal*', async (route) => { + counter.n += 1; + counter.urls.push(route.request().url()); + const dims = await page.evaluate( + () => + ( + window as unknown as { app: { getTerminalDimensions?: () => { cols: number; rows: number } | null } } + ).app.getTerminalDimensions?.() ?? null + ); + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ + success: true, + data: { + terminalBuffer: paneSnapshot(dims?.rows ?? 10), + status: 'idle', + fullSize: 1024, + retainedBytes: 1024, + truncated: false, + truncationReason: null, + source: 'mux-visible', + captureCols: dims?.cols, + captureRows: dims?.rows, + }, + }), + }); + }); +} + /** The widest terminal this suite's 1280px viewport can produce, with margin. */ const WIDER_THAN_ANY_TERMINAL_COLS = 500; @@ -138,6 +180,22 @@ async function select(page: Page, sessionId: string, options: object = {}): Prom await page.waitForTimeout(1500); } +/** + * Spend the per-page full-history allowance and forget what it cost. Every + * geometry comparison below runs on a `mux-visible` response, and the route + * only produces one for a request sent WITHOUT `full=1`, so reaching that shape + * means not being the first select of the page — which is what a tab switch is. + */ +async function consumeFullHistory( + page: Page, + sessionId: string, + counter: { n: number; urls: string[] } +): Promise { + await select(page, sessionId); + counter.n = 0; + counter.urls.length = 0; +} + async function closeSession(page: Page, sessionId: string): Promise { await page.evaluate( (sid: string) => fetch(`/api/sessions/${sid}`, { method: 'DELETE' }).then(() => undefined), @@ -145,7 +203,7 @@ async function closeSession(page: Page, sessionId: string): Promise { ); } -describe('a capture taller than the terminal', () => { +describe('a capture bigger than the terminal', () => { let context: BrowserContext; let page: Page; @@ -163,7 +221,11 @@ describe('a capture taller than the terminal', () => { // trigger is the captured height alone and not a size that moved. const fetches = { n: 0, urls: [] as string[] }; await stubTerminal(page, 200, fetches); - await select(page, sessionId); + // A tab switch is where a visible-frame response arrives, so that is what + // this measures. The first select of the page takes the full-history path + // and is covered by its own case below. + await consumeFullHistory(page, sessionId, fetches); + await select(page, sessionId, { forceReload: true }); // The terminal is sized by that select, so the premise is checkable now. expect(await terminalRows(page)).toBeLessThan(200); @@ -188,18 +250,19 @@ describe('a capture taller than the terminal', () => { const fetches = { n: 0, urls: [] as string[] }; await stubTerminal(page, 200, fetches); - // First select: a fresh session, so this one legitimately pulls full history - // and its retry may do the same. + // First select: a fresh session, so this one pulls full history. It does + // NOT retry, because the geometry comparison runs on a visible-frame + // response and a `full=1` request cannot produce one. await select(page, sessionId); - const afterFirst = fetches.n; - expect(afterFirst).toBe(2); + expect(fetches.n).toBe(1); + expect(fetches.urls.filter((u) => u.includes('full=1'))).toHaveLength(1); // Re-select the SAME session. `selectSession` early-returns on an already // active session unless forceReload is set, and forceReload is the shape a // tab switch back to this session takes: `_fullHistoryLoaded` still holds - // it, so neither this pass nor its retry should ask for full history again. + // it, so neither this pass nor its retry asks for full history again. await select(page, sessionId, { forceReload: true }); - const tabSwitchUrls = fetches.urls.slice(afterFirst); + const tabSwitchUrls = fetches.urls.slice(1); expect(tabSwitchUrls.length).toBe(2); expect(tabSwitchUrls.filter((u) => u.includes('full=1'))).toHaveLength(0); @@ -217,7 +280,8 @@ describe('a capture taller than the terminal', () => { // would double the work of every tab switch. const fetches = { n: 0, urls: [] as string[] }; await stubTerminal(page, 5, fetches, { captureCols: 40 }); - await select(page, sessionId); + await consumeFullHistory(page, sessionId, fetches); + await select(page, sessionId, { forceReload: true }); expect(await terminalRows(page)).toBeGreaterThan(5); expect(fetches.n).toBe(1); @@ -238,7 +302,8 @@ describe('a capture taller than the terminal', () => { const fetches = { n: 0, urls: [] as string[] }; await stubTerminal(page, 5, fetches, { captureCols: WIDER_THAN_ANY_TERMINAL_COLS }); - await select(page, sessionId); + await consumeFullHistory(page, sessionId, fetches); + await select(page, sessionId, { forceReload: true }); expect(await terminalRows(page)).toBeGreaterThan(5); expect(await terminalCols(page)).toBeLessThan(WIDER_THAN_ANY_TERMINAL_COLS); @@ -263,10 +328,7 @@ describe('a capture taller than the terminal', () => { const sessionId = await openSession(page); const fetches = { n: 0, urls: [] as string[] }; - await stubTerminal(page, 200, fetches, { - source: 'mux-full-history', - captureCols: WIDER_THAN_ANY_TERMINAL_COLS, - }); + await stubTerminal(page, 200, fetches, { captureCols: WIDER_THAN_ANY_TERMINAL_COLS }); await select(page, sessionId); // Both dimensions are mismatched, so height alone is not what spares it. @@ -278,4 +340,38 @@ describe('a capture taller than the terminal', () => { await closeSession(page, sessionId); await context.close(); }, 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. + context = await browser.newContext({ viewport: { width: 320, height: 200 } }); + page = await context.newPage(); + const sessionId = await openSession(page); + + const fetches = { n: 0, urls: [] as string[] }; + await stubTerminalAtRequestedSize(page, fetches); + 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. + const requested = await page.evaluate( + () => + ( + window as unknown as { app: { getTerminalDimensions?: () => { cols: number; rows: number } | null } } + ).app.getTerminalDimensions?.() ?? null + ); + expect(requested).not.toBeNull(); + expect(requested!.rows).toBeGreaterThan(await terminalRows(page)); + + expect(fetches.n).toBe(1); + + await closeSession(page, sessionId); + await context.close(); + }, 60_000); }); diff --git a/test/routes/session-routes.test.ts b/test/routes/session-routes.test.ts index 7b550560..c4e21dd3 100644 --- a/test/routes/session-routes.test.ts +++ b/test/routes/session-routes.test.ts @@ -831,6 +831,31 @@ describe('session-routes', () => { expect(body.data.captureRows).toBeUndefined(); }); + it('omits the geometry when the capture reported a size but returned nothing', async () => { + // A capture can report geometry and still hand back no frame. The + // full-history path writes `capturedGeometry` from the cursor query, then + // returns '' for a pane holding nothing visible, which drops the source + // to `history` with the geometry already recorded. Reporting it there + // would name a size for a body that is the byte stream. + harness.ctx._session.terminalBuffer = 'byte history only'; + (harness.ctx.mux as { captureActivePaneBuffer?: unknown }).captureActivePaneBuffer = vi.fn( + (_name: string, opts?: { capturedGeometry?: { cols: number; rows: number } }) => { + if (opts) opts.capturedGeometry = { cols: 100, rows: 50 }; + return ''; + } + ); + + const res = await harness.app.inject({ + method: 'GET', + url: `/api/sessions/${harness.ctx._sessionId}/terminal?full=1`, + }); + + const body = JSON.parse(res.body); + expect(body.data.source).toBe('history'); + expect(body.data.captureCols).toBeUndefined(); + expect(body.data.captureRows).toBeUndefined(); + }); + // ── COD-47: full tmux scrollback replay on full page reload ── it('full reload (?full=1) requests full tmux history and replays boundary markers', async () => { // A realistic scrollback-length capture: ~5000 lines, well past one screen. From 383f834704f85668db40cef1fe793008e98e95dd Mon Sep 17 00:00:00 2001 From: Michael Grundberg Date: Fri, 18 Sep 2026 22:16:44 +0200 Subject: [PATCH 4/5] fix(terminal): flush unsent local echo before the geometry replay On a touch device the characters the user has typed live only in the local-echo overlay until Enter; they have never reached the PTY. The replay re-enters `selectSession` with `forceReload` on the session that is still active, and that branch nulled `activeSessionId` before `_cleanupPreviousSession` ran. The flush there is guarded on a session it can still see, so it was skipped, and the unconditional `_localEchoOverlay.clear()` that follows took the characters with it. Measured in chromium against the previous head: typing into the overlay and then making the call the replay makes left `pendingText` empty with nothing crossing into the delivery layer on either transport. The flush moves into `_flushLocalEchoTo(sessionId)`, called from both `_cleanupPreviousSession` and the `forceReload` branch before it nulls the id. The session is a parameter because the two callers mean different ones: cleanup flushes to the tab being left, the branch to the tab being reloaded. This was reachable before this branch, through the one gesture that already takes the `forceReload` path on an active session. What is new is that nothing the user does triggers it. The replay fires on its own the moment a tab switch finishes, which is exactly when someone typing into a still-loading terminal has text in the overlay, and on a phone beside an active desktop tab that is every tab switch. A seventh browser case pins it: it forces the overlay on, since headless chromium reports no touch support and the case would otherwise pass vacuously, asserts the typed characters really are sitting unsent, then triggers the replay and asserts they reached the session. Without the fix it fails with nothing delivered at all. Co-Authored-By: Claude Opus 5 (1M context) --- src/web/public/app.js | 69 +++++++++++++------- test/capture-geometry-retry.browser.test.ts | 72 +++++++++++++++++++++ 2 files changed, 119 insertions(+), 22 deletions(-) diff --git a/src/web/public/app.js b/src/web/public/app.js index 8abf8bec..172625e1 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -5783,28 +5783,7 @@ class CodemanApp { if (ta) ta.dispatchEvent(new CompositionEvent('compositionend', { data: '' })); } } catch {} - // Flush local echo text to PTY before switching tabs. - // Send as a single batch (no Enter) so it lands in the session's readline - // input buffer — avoids "old text resent on Enter" and overlay render bugs. - // Track flushed length so _render() offsets the overlay correctly even before - // the PTY echo arrives in the terminal buffer. - if (this.activeSessionId) { - const echoText = this._localEchoOverlay?.pendingText || ''; - // Include buffer-detected flushed text (from Tab completion, etc.) - // so it's preserved across tab switches. - const existingFlushed = this._localEchoOverlay?.getFlushed()?.count || 0; - const existingFlushedText = this._localEchoOverlay?.getFlushed()?.text || ''; - if (echoText) { - this._sendInputAsync(this.activeSessionId, echoText); - } - const totalOffset = existingFlushed + echoText.length; - if (totalOffset > 0) { - if (!this._flushedOffsets) this._flushedOffsets = new Map(); - if (!this._flushedTexts) this._flushedTexts = new Map(); - this._flushedOffsets.set(this.activeSessionId, totalOffset); - this._flushedTexts.set(this.activeSessionId, existingFlushedText + echoText); - } - } + this._flushLocalEchoTo(this.activeSessionId); this._localEchoOverlay?.clear(); // Predictions are ephemeral + already sent: nothing to save/restore // across a tab switch (unlike the buffer overlay's setFlushed machinery) @@ -5819,6 +5798,45 @@ class CodemanApp { } } + /** + * Hand the local-echo overlay's unsent text to `sessionId` before anything + * clears it, and record what has now been flushed so `_render()` offsets the + * overlay correctly even before the PTY echo comes back. + * + * On a touch device the characters the user has typed live ONLY here until + * Enter — they have never reached the PTY — so whoever clears the overlay + * owes them a flush first. It is sent as one batch with no Enter, so it lands + * in the session's readline buffer rather than submitting a line the user has + * not finished. + * + * ⚠️ The session is a PARAMETER because the two callers are looking at + * different ones. `_cleanupPreviousSession` flushes to the tab being left, + * which is still `activeSessionId` when it runs. The `forceReload` branch in + * `selectSession` flushes to the tab being RELOADED, and must do it before it + * nulls `activeSessionId`: reading the field after that null is what silently + * dropped the text, since the guard here then saw no session and the + * unconditional `clear()` that follows took the characters with it. + * @param {string|null} sessionId + */ + _flushLocalEchoTo(sessionId) { + if (!sessionId) return; + const echoText = this._localEchoOverlay?.pendingText || ''; + // Include buffer-detected flushed text (from Tab completion, etc.) + // so it's preserved across tab switches. + const existingFlushed = this._localEchoOverlay?.getFlushed()?.count || 0; + const existingFlushedText = this._localEchoOverlay?.getFlushed()?.text || ''; + if (echoText) { + this._sendInputAsync(sessionId, echoText); + } + const totalOffset = existingFlushed + echoText.length; + if (totalOffset > 0) { + if (!this._flushedOffsets) this._flushedOffsets = new Map(); + if (!this._flushedTexts) this._flushedTexts = new Map(); + this._flushedOffsets.set(sessionId, totalOffset); + this._flushedTexts.set(sessionId, existingFlushedText + echoText); + } + } + _resetTerminalForReplay() { this.terminal.reset(); this.terminal.write('\x1b[3J\x1b[H\x1b[2J'); @@ -6093,6 +6111,13 @@ class CodemanApp { this._loadBufferQueue = null; this._terminalRefreshOwner = null; this._chunkedWriteGen = (this._chunkedWriteGen || 0) + 1; + // Anything typed but not yet submitted lives in the local-echo overlay and + // has never reached the PTY. `_cleanupPreviousSession` below flushes it, + // but only for a session it can still see, and the null on the next line + // hides this one from it. Flush first or the characters are cleared + // unread. The geometry replay re-enters here with no gesture behind it, + // so on a touch device this fires while the user is still typing. + this._flushLocalEchoTo(sessionId); this.activeSessionId = null; } // Focus terminal SYNCHRONOUSLY before any await — iOS Safari only honors diff --git a/test/capture-geometry-retry.browser.test.ts b/test/capture-geometry-retry.browser.test.ts index 5c5e6419..df209e8a 100644 --- a/test/capture-geometry-retry.browser.test.ts +++ b/test/capture-geometry-retry.browser.test.ts @@ -341,6 +341,78 @@ describe('a capture bigger than the terminal', () => { await context.close(); }, 60_000); + it('hands over text typed but not yet submitted before it replays', async () => { + // On a touch device the characters the user has typed live ONLY in the + // local-echo overlay until Enter; they have never reached the PTY. The + // replay re-enters `selectSession` with `forceReload` on the session that + // is still active, and that branch used to null `activeSessionId` before + // `_cleanupPreviousSession` ran, so the flush there saw no session and the + // unconditional `clear()` afterwards took the characters with it. Nothing + // the user did triggered that: the replay fires on its own the moment a + // tab switch finishes, which is exactly when someone typing into a + // still-loading terminal has text in the overlay. + context = await browser.newContext({ viewport: { width: 1280, height: 800 } }); + page = await context.newPage(); + const sessionId = await openSession(page); + + const fetches = { n: 0, urls: [] as string[] }; + await stubTerminal(page, 200, fetches); + await consumeFullHistory(page, sessionId, fetches); + + // Headless chromium reports `isTouchDevice()` false even with `hasTouch`, + // so the overlay would stay off and the whole case would pass vacuously. + // The setting is what `_updateLocalEchoState()` reads, so it survives the + // recompute that every select runs; the flag is forced too, for the window + // before the next recompute. Record what crosses into the delivery layer, + // which is the seam the text failed to cross. + await page.evaluate(() => { + const w = window as unknown as { + app: { + _localEchoEnabled: boolean; + _sendInputAsync: (id: string, text: string, opts?: unknown) => void; + terminal?: { focus: () => void }; + loadAppSettingsFromStorage: () => Record; + }; + __sentInputs: { id: string; text: string }[]; + }; + const settings = w.app.loadAppSettingsFromStorage(); + settings.localEchoEnabled = true; + localStorage.setItem('codeman-app-settings', JSON.stringify(settings)); + w.app._localEchoEnabled = true; + w.__sentInputs = []; + const original = w.app._sendInputAsync.bind(w.app); + w.app._sendInputAsync = (id: string, text: string, opts?: unknown) => { + w.__sentInputs.push({ id, text }); + return original(id, text, opts); + }; + w.app.terminal?.focus(); + }); + + await page.keyboard.type('hello-unsent'); + // The premise: the characters really are sitting in the overlay, unsent. + // Without this the case would pass on a build where typing goes straight + // to the PTY and there is nothing to lose. + const pendingBefore = await page.evaluate( + () => + (window as unknown as { app: { _localEchoOverlay?: { pendingText: string } } }).app._localEchoOverlay + ?.pendingText ?? '' + ); + expect(pendingBefore).toBe('hello-unsent'); + + // The captured pane is taller than the terminal, so this select replays. + await select(page, sessionId, { forceReload: true }); + expect(fetches.n).toBe(2); + + const sent = await page.evaluate( + () => (window as unknown as { __sentInputs: { id: string; text: string }[] }).__sentInputs + ); + expect(sent.map((s) => s.text)).toContain('hello-unsent'); + expect(sent.find((s) => s.text === 'hello-unsent')?.id).toBe(sessionId); + + await closeSession(page, sessionId); + await context.close(); + }, 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 From 95dc6fe94404e01b9409b237dfab44c1e8b18c0b Mon Sep 17 00:00:00 2001 From: Michael Grundberg Date: Sat, 19 Sep 2026 10:56:58 +0200 Subject: [PATCH 5/5] fix(terminal): remember a geometry replay that did not converge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resizeRetry` caps the recursion inside one select and says nothing about the next one, so a pane this browser cannot size reported the same mismatch on every select and bought the same failed repair each time: two fetches per tab switch for the life of the page, measured as a running count of 2, 4, 6 across three selects. That is the case this branch describes as happening every time rather than occasionally, a phone whose resize `Session.resize` declines while a desktop claim is live, and it is not the only one — any pane Codeman cannot size lands there, including one a second tmux client is also holding. Each wasted pass costs another `capture-pane`, which is `execSync` and blocks the server's event loop, plus a reset and chunked rewrite, a discarded snapshot and cache entry, and a dropped and reopened WebSocket. `_geometryRetryUseless` mirrors the existing `_fullHistoryRepullUseless`: a retry pass whose frame still does not fit adds the session, geometry that fits removes it, and the replay gate consults it. The proof has to come from a retry pass rather than a first one, because the retry ran at the size that stuck and the pane ignored it. Clearing on a fitting frame is what stops a pane that becomes sizeable again, once the desktop tab closes or its claim goes idle, from staying permanently unrepaired. The race case never reaches the latch, since it converges on its first attempt. The new browser case walks all of that: three selects reading 2, 3, 4 instead of 2, 4, 6, then a fitting frame, then a mismatch diagnosed afresh. Without the gate it fails on the second switch with `expected 4 to be 3`. Rebased onto master, which has moved to 1.30.0 and taken #436. The one conflict was `config/test-suites.ts`, where both branches appended a glob to `BROWSER_TEST_GLOBS`; both are kept. Everything else merged clean, #436's own changes to the same buffer-load path included. Co-Authored-By: Claude Opus 5 (1M context) --- src/web/public/app.js | 34 +++++++++ test/capture-geometry-retry.browser.test.ts | 84 +++++++++++++++++++++ 2 files changed, 118 insertions(+) diff --git a/src/web/public/app.js b/src/web/public/app.js index 172625e1..d2a3b9d2 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -549,6 +549,12 @@ class CodemanApp { // repaint-mode CLI pane, where tmux keeps no history of its own). The pull is // refused for those and retried far more slowly — see _maybeRefetchFullHistory. this._fullHistoryRepullUseless = new Set(); + // Sessions where the geometry replay has already been tried and did NOT + // converge, so the pane is one this browser cannot size. Mirrors the Set + // above: `resizeRetry` caps the recursion inside one select, and this is + // what stops a fresh select from paying for the same answer again — see + // the geometry gate in selectSession. + this._geometryRetryUseless = new Set(); this.terminalLoadStates = new Map(); // Map this.respawnStatus = {}; this.respawnTimers = {}; // Track timed respawn timers @@ -6718,6 +6724,33 @@ class CodemanApp { this._clearTerminalLoadState(sessionId, selectGen); _crashDiag.log(`SELECT_DONE: ${selectDoneMs.toFixed(0)}ms`); console.log(`[CRASH-DIAG] selectSession DONE: ${sessionId.slice(0,8)} in ${selectDoneMs.toFixed(0)}ms`); + // Remember whether the replay was worth it, because `resizeRetry` only + // caps the recursion INSIDE one select and says nothing about the next + // one. A pane this browser cannot size — one whose resize `Session.resize` + // declines while a desktop claim is live, or one a second tmux client is + // also holding — reports the same mismatch on every select, so without a + // memo the diagnosis is paid for again on every tab switch, forever: two + // fetches per select rather than one. Each extra pass costs a second + // `capture-pane`, which is `execSync` and blocks the server's event loop, + // plus a reset and chunked rewrite, a discarded snapshot and cache entry, + // and a dropped and reopened WebSocket. + // + // A retry pass that STILL does not fit is the proof, since the retry ran + // at the size that stuck and the pane ignored it. Geometry that fits + // clears the memo, so a pane that becomes sizeable again (the desktop tab + // closes, the claim goes idle) is repaired on the next select. The race + // case is untouched: it converges on its first attempt, so it never + // reaches the branch that latches. + const capturedGeometryFits = + framePositionsRowsAbsolutely && + Number.isFinite(data.captureRows) && + !capturedTallerThanTerminal && + !capturedWiderThanTerminal; + if (capturedGeometryFits) { + this._geometryRetryUseless?.delete(sessionId); + } else if (options?.resizeRetry && (capturedTallerThanTerminal || capturedWiderThanTerminal)) { + (this._geometryRetryUseless ||= new Set()).add(sessionId); + } // What is on screen was drawn for a geometry this terminal does not have. // Replaying once against the size that stuck is the only thing that // repairs it: SIGWINCH reaches the CLI only on a real size change, and @@ -6727,6 +6760,7 @@ class CodemanApp { if ( (sizeMovedUnderLoad || capturedTallerThanTerminal || capturedWiderThanTerminal) && !captureMatchesRequestedSize && + !this._geometryRetryUseless?.has(sessionId) && !options?.resizeRetry && !this._isStaleSelect(selectGen) ) { diff --git a/test/capture-geometry-retry.browser.test.ts b/test/capture-geometry-retry.browser.test.ts index df209e8a..1f8301de 100644 --- a/test/capture-geometry-retry.browser.test.ts +++ b/test/capture-geometry-retry.browser.test.ts @@ -98,6 +98,41 @@ async function stubTerminal( }); } +/** + * As `stubTerminal`, but reading its geometry from a holder the test can change + * between selects. That is what lets one case watch a pane stop fitting and + * start fitting again, which a stub fixed at construction cannot show. + */ +async function stubTerminalDynamic( + page: Page, + counter: { n: number; urls: string[] }, + state: { captureRows: number; captureCols: number } +) { + await page.route('**/api/sessions/*/terminal*', async (route) => { + const url = route.request().url(); + counter.n += 1; + counter.urls.push(url); + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ + success: true, + data: { + terminalBuffer: paneSnapshot(state.captureRows), + status: 'idle', + fullSize: 1024, + retainedBytes: 1024, + truncated: false, + truncationReason: null, + source: url.includes('full=1') ? 'mux-full-history' : 'mux-visible', + captureCols: state.captureCols, + captureRows: state.captureRows, + }, + }), + }); + }); +} + /** * Answer every fetch with the geometry the client itself is asking for, read * live from the page. That is the clamp signature: `getTerminalDimensions()` @@ -341,6 +376,55 @@ describe('a capture bigger than the terminal', () => { await context.close(); }, 60_000); + it('replays once per session, not once per tab switch, when it cannot converge', async () => { + // `resizeRetry` caps the recursion inside ONE select and says nothing about + // the next one, so a pane this browser cannot size reported the same + // mismatch on every select and bought the same failed repair every time: + // two fetches per tab switch for the life of the page. That is the case the + // description calls "every time rather than occasionally", a phone whose + // resize is declined while a desktop claim is live, and it is not the only + // one — any pane Codeman cannot size lands there, a second tmux client + // attached to it included. Each wasted pass costs another `capture-pane`, + // which is `execSync` on the server's event loop, plus a reset and rewrite, + // a discarded snapshot, and a dropped and reopened WebSocket. + context = await browser.newContext({ viewport: { width: 1280, height: 800 } }); + page = await context.newPage(); + const sessionId = await openSession(page); + + const fetches = { n: 0, urls: [] as string[] }; + const pane = { captureRows: 200, captureCols: 200 }; + await stubTerminalDynamic(page, fetches, pane); + await consumeFullHistory(page, sessionId, fetches); + + // First tab switch: one load, one replay, and the replay does not fit + // either, which is the proof that this pane ignores the size it is given. + await select(page, sessionId, { forceReload: true }); + expect(fetches.n).toBe(2); + + // Every switch after it pays once. Unlatched this reads 4 then 6. + await select(page, sessionId, { forceReload: true }); + expect(fetches.n).toBe(3); + await select(page, sessionId, { forceReload: true }); + expect(fetches.n).toBe(4); + + // The memo has to lift when the pane becomes sizeable again, or closing the + // desktop tab that was holding it would leave this session permanently + // unrepaired. A frame that fits clears it... + pane.captureRows = 5; + pane.captureCols = 40; + await select(page, sessionId, { forceReload: true }); + expect(fetches.n).toBe(5); + + // ...so the next genuine mismatch is diagnosed again. + pane.captureRows = 200; + pane.captureCols = 200; + await select(page, sessionId, { forceReload: true }); + expect(fetches.n).toBe(7); + + await closeSession(page, sessionId); + await context.close(); + }, 60_000); + it('hands over text typed but not yet submitted before it replays', async () => { // On a touch device the characters the user has typed live ONLY in the // local-echo overlay until Enter; they have never reached the PTY. The