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 ──