From 5cfb98fb8b81cea28a3708303ff59ce1bd1397be Mon Sep 17 00:00:00 2001 From: Michael Grundberg Date: Fri, 18 Sep 2026 16:45:37 +0200 Subject: [PATCH] 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 ──