From e0d4477edc9dc366806300c5a4564cc3fb9f51be Mon Sep 17 00:00:00 2001 From: Michael Grundberg Date: Fri, 18 Sep 2026 18:53:05 +0200 Subject: [PATCH] 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.