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.