From c9515b1d4c6d2469179506b78bb9d4b3956582b7 Mon Sep 17 00:00:00 2001 From: Michael Grundberg Date: Tue, 15 Sep 2026 12:56:09 +0200 Subject: [PATCH 1/4] fix(terminal): keep the output a pane capture could not contain Live terminal events are queued while a buffer load runs, and the load discards that queue when it ends. That is right when the loaded buffer is the server's accumulated byte history. The route appends to that history right up to the moment it serializes the response, so a queued event already appears in it and replaying it would duplicate output, most visibly Ink's cursor-up redraws. A tmux pane capture is a photograph, current only as of the instant `capture-pane` ran. Output printed afterwards was queued and then dropped, and nothing scheduled a re-fetch to recover it: `_onSessionNeedsRefresh` is wired only to the 128KB overflow path. The CLI's next partial redraw then landed on a frame the terminal never received. How much went missing depended on which capture the route served. A `?full=1` load returns the capture alone, with no history in front of it, so it lost everything from the capture to the end of the chunked write. A `?tail=` load returns history, a clear, and then the capture, and the route reads that history after the capture, so it lost everything from the response to the end of that write. The chunked write dominates either way. An agent CLI hides the loss on its next full redraw; a shell session does not, because its output is linear and nothing repaints it. Queue entries now carry their arrival time, and `_finishBufferLoad` takes a `since` cutoff, so a capture load replays exactly the tail that arrived after the response headers. The earlier events stay dropped, because a payload that carries history does hold those. All four paths that fetch a terminal buffer and write it now decide this the same way, through one `_bufferLoadFinishOpts` helper, so they cannot drift apart: `selectSession`, `_onSessionNeedsRefresh`, `_onSessionClearTerminal` and `_maybeRefetchFullHistory`. The second of those is the one that stings. It exists to restore output the client already dropped once under backpressure, and it was dropping more output while performing that recovery. The cache-hit write inside `selectSession` stays on discard deliberately: it runs before the fetch, so its queue holds only events the capture that follows already contains. Two further things had to change for that tail to still exist when the load ends, and a browser test is what found both. `chunkedTerminalWrite` is what ends the load for every non-empty buffer, so the flush policy travels to its own finish calls; the call in `selectSession` runs only when the write was skipped. `_beginBufferLoad` no longer empties the queue when one load re-enters it, which it does on every write, because that reset discarded the whole fetch window before anything could replay it. The response already distinguishes the sources. `source` reads `mux-visible` or `mux-full-history` for a capture and `history` for the byte stream. Follows #395, #396 and #397, which fixed the ways the replayed frame itself could disagree with the terminal. Co-Authored-By: Claude Opus 5 (1M context) --- ...y-output-that-arrived-after-the-capture.md | 36 ++++ config/test-suites.ts | 1 + src/web/public/app.js | 71 ++++++- src/web/public/terminal-ui.js | 57 ++++-- test/capture-load-window.browser.test.ts | 177 ++++++++++++++++++ test/terminal-buffer-flush.test.ts | 97 +++++++++- 6 files changed, 413 insertions(+), 26 deletions(-) create mode 100644 .changeset/fix-replay-output-that-arrived-after-the-capture.md create mode 100644 test/capture-load-window.browser.test.ts diff --git a/.changeset/fix-replay-output-that-arrived-after-the-capture.md b/.changeset/fix-replay-output-that-arrived-after-the-capture.md new file mode 100644 index 00000000..03e09189 --- /dev/null +++ b/.changeset/fix-replay-output-that-arrived-after-the-capture.md @@ -0,0 +1,36 @@ +--- +"aicodeman": patch +--- + +fix(terminal): keep the output a pane capture could not contain + +Live terminal events are queued while a buffer load runs, and the load discards +that queue when it ends. That is right when the loaded buffer is the server's +accumulated byte history: the route appends to that history right up to the +moment it serializes the response, so the queued events already appear in it and +replaying them would duplicate output. + +A tmux pane capture is a photograph, current only as of the instant +`capture-pane` ran. Output printed afterwards was queued and then dropped, with +nothing scheduling a re-fetch, and the CLI's next partial redraw landed on a +frame the terminal never received. A `?full=1` load returns the capture alone, +so it lost everything from the capture to the end of the chunked write. A +`?tail=` load carries the byte history in front of the capture, so it lost +everything from the response to the end of that write. A shell session shows +this most plainly, because its output is linear and nothing repaints it. + +Queue entries now carry their arrival time, and `_finishBufferLoad` takes a +`since` cutoff so a capture load replays exactly the tail that arrived after the +response headers. All four paths that fetch a terminal buffer and write it use +the same rule, through one shared `_bufferLoadFinishOpts` helper: selecting a +session, the backpressure refresh, the clear-terminal reload, and the +full-history re-pull. The backpressure refresh matters most, because it exists +to restore output the client already dropped once and could drop more while +doing it. + +Two things had to change for that tail to still exist when the load ends. +`chunkedTerminalWrite` is what ends the load for any non-empty buffer, so it +takes the flush policy and applies it at its own finish sites. +`_beginBufferLoad` no longer empties the queue when the same load re-enters it, +which it does on every write, because that reset discarded the fetch window +before anything could replay it. diff --git a/config/test-suites.ts b/config/test-suites.ts index 02cc154b..233e0e7d 100644 --- a/config/test-suites.ts +++ b/config/test-suites.ts @@ -27,6 +27,7 @@ export const BROWSER_TEST_GLOBS = [ 'test/webgl-fallback.test.ts', 'test/terminal-copy-shortcut.test.ts', 'test/terminal-keycode229-recovery.browser.test.ts', + 'test/capture-load-window.browser.test.ts', 'test/codex-predictive-echo.test.ts', // also needs a real codex binary ]; diff --git a/src/web/public/app.js b/src/web/public/app.js index f91f748b..ab303da5 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -1906,6 +1906,30 @@ class CodemanApp { this._onSessionClearTerminal(data); } + /** + * How a buffer load that just fetched `payload` must end. + * + * A tmux pane capture is a point-in-time frame, so nothing that reached the + * browser after the response headers can already be in it. Such a load + * replays exactly that tail; discarding it drops the CLI's output for the + * rest of the load window, and its next partial redraw then lands on a frame + * the terminal never received. A payload built from the server's accumulated + * byte history needs the opposite: that history is current up to the + * response, so replaying the queue on top of it would duplicate output. + * + * `headersReceivedAt` is the caller's own `performance.now()` reading from + * the moment the response arrived, compared only against other client-side + * readings, so there is no clock skew to worry about. + * + * @param {{source?: string}} payload - The parsed `data` of a terminal response. + * @param {number} headersReceivedAt - When that response reached this client. + * @returns {{flushQueued: boolean, since: number}} Options for `_finishBufferLoad`. + */ + _bufferLoadFinishOpts(payload, headersReceivedAt) { + const capturedFromMux = payload?.source === 'mux-visible' || payload?.source === 'mux-full-history'; + return { flushQueued: capturedFromMux, since: headersReceivedAt }; + } + _onSessionTerminal(data) { if (data.id === this.activeSessionId) { if (data.data.length > 32768) _crashDiag.log(`TERMINAL: ${(data.data.length/1024).toFixed(0)}KB`); @@ -1915,7 +1939,7 @@ class CodemanApp { // jump over the cap. Dropped data is recovered from the canonical buffer. const queued = (this.pendingWrites?.reduce((s, w) => s + w.length, 0) || 0) + (this.flickerFilterBuffer?.length || 0) - + (this._loadBufferQueue?.reduce((s, w) => s + w.length, 0) || 0) + + (this._loadBufferQueue?.reduce((s, w) => s + w.data.length, 0) || 0) + (this._terminalWriteInFlightBytes || 0); if (queued + data.data.length > 131072) { // 128KB — drop to prevent accumulation // Schedule a self-recovery once the @@ -2498,9 +2522,11 @@ class CodemanApp { ? `/api/sessions/${sessionId}/terminal?full=1` : `/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}` ); + let headersReceivedAt = performance.now(); let data = (await res.json())?.data ?? {}; if (useFullHistory && data.terminalBuffer && this._replayWouldShrinkBuffer(data.terminalBuffer)) { res = await fetch(`/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}`); + headersReceivedAt = performance.now(); data = (await res.json())?.data ?? {}; } // Bail on a tab switch mid-fetch: writing here would paint this session's @@ -2516,7 +2542,12 @@ class CodemanApp { const linesFromBottom = before ? Math.max(0, (before.baseY || 0) - (before.viewportY || 0)) : 0; this.terminal.clear(); this.terminal.reset(); - await this.chunkedTerminalWrite(data.terminalBuffer); + await this.chunkedTerminalWrite( + data.terminalBuffer, + TERMINAL_CHUNK_SIZE, + undefined, + this._bufferLoadFinishOpts(data, headersReceivedAt) + ); // A tail fetch can be partial, and the banner would otherwise keep // describing the pre-refresh buffer (#258). this._setHistoryTruncation(sessionId, data); @@ -2552,6 +2583,7 @@ class CodemanApp { // Fetch buffer, clear terminal, write buffer, resize (no Ctrl+L needed) try { const res = await fetch(`/api/sessions/${data.id}/terminal`); + const headersReceivedAt = performance.now(); const termData = (await res.json())?.data ?? {}; this.terminal.clear(); @@ -2561,7 +2593,12 @@ class CodemanApp { // (markers don't help here - this is a static buffer reload, not live Ink redraws) const cleanBuffer = termData.terminalBuffer.replace(DEC_SYNC_STRIP_RE, ''); // Use chunked write to avoid UI freeze with large buffers (can be 1-2MB) - await this.chunkedTerminalWrite(cleanBuffer); + await this.chunkedTerminalWrite( + cleanBuffer, + TERMINAL_CHUNK_SIZE, + undefined, + this._bufferLoadFinishOpts(termData, headersReceivedAt) + ); } // Fire-and-forget resize — don't block on it @@ -5782,7 +5819,12 @@ class CodemanApp { parsedAt, bufferLength: parsedBufferLength, completed, - } = await this.chunkedTerminalWrite(buffer, TERMINAL_CHUNK_SIZE, sessionId); + } = await this.chunkedTerminalWrite( + buffer, + TERMINAL_CHUNK_SIZE, + sessionId, + this._bufferLoadFinishOpts(payload, headersReceivedAt) + ); timing.resetAndParseMs = parsedAt - replayStartedAt; if (!completed || this.activeSessionId !== sessionId) return; // Keep shell tab restores bounded too. A user-triggered full-history pull @@ -6241,6 +6283,15 @@ class CodemanApp { } const data = (await res.json())?.data ?? {}; const bodyParsedAt = performance.now(); + // How this load must end, decided here because `chunkedTerminalWrite` is + // what actually ends it for a non-empty buffer. A tmux pane capture is a + // point-in-time frame, so nothing that reached the browser after the + // response headers can already be in it. Replay exactly that tail; + // discarding it drops the CLI's output for the rest of the load window, + // and its next partial redraw then lands on a frame the terminal never + // received. `since` keeps the pre-capture events dropped, because the + // capture does hold those and replaying them would duplicate output. + const finishOpts = this._bufferLoadFinishOpts(data, headersReceivedAt); _crashDiag.log(`FETCH_DONE: ${data.terminalBuffer ? (data.terminalBuffer.length/1024).toFixed(0) + 'KB' : 'empty'} truncated=${data.truncated}`); let freshResetAndParseMs = 0; @@ -6267,7 +6318,8 @@ class CodemanApp { const { parsedAt: freshParsedAt } = await this.chunkedTerminalWrite( data.terminalBuffer, TERMINAL_CHUNK_SIZE, - bufferLoadOwner + bufferLoadOwner, + finishOpts ); freshResetAndParseMs = freshParsedAt - replayStartedAt; if (this._isStaleSelect(selectGen)) { @@ -6317,7 +6369,14 @@ class CodemanApp { // COD-144: when the load painted nothing, FLUSH the queued events instead of // discarding — a new session's prompt arrives only as a queued SSE event. if (this._isLoadingBuffer) { - this._finishBufferLoad(bufferLoadOwner, { flushQueued: bufferWasEmpty }); + // Only reached when the write was skipped. COD-144 lives here: a new + // session's first prompt exists only as a queued event that predates the + // response, so an empty paint replays its queue WHOLE rather than from + // the header timestamp. + this._finishBufferLoad( + bufferLoadOwner, + bufferWasEmpty ? { flushQueued: true, since: 0 } : finishOpts + ); } // Drop the guard so user input clears state normally this._restoringFlushedState = false; diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index 85c13350..1a265d61 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -3267,7 +3267,11 @@ Object.assign(CodemanApp.prototype, { // to prevent interleaving historical buffer data with live SSE data. // This is critical: interleaving causes cursor position chaos with Ink redraws. if (this._isLoadingBuffer) { - if (this._loadBufferQueue) this._loadBufferQueue.push(data); + // Each entry records when it arrived. A flush of a tmux-capture load + // replays only what arrived after the capture; without the timestamp it + // would have to replay the whole queue, duplicating the events the + // capture already contains. See _finishBufferLoad's `since`. + if (this._loadBufferQueue) this._loadBufferQueue.push({ at: performance.now(), data }); return; } @@ -3747,9 +3751,14 @@ Object.assign(CodemanApp.prototype, { * and a tick-Worker so progress continues on occluded / idle-throttled tabs. * @param {string} buffer - The full terminal buffer to write * @param {number} chunkSize - Size of each chunk (default 32KB) + * @param {string} [loadOwner] - Load token to finish under + * @param {{ flushQueued?: boolean, since?: number }} [finishOpts] - Passed to + * `_finishBufferLoad`. This method ends the load for every non-empty buffer, + * so a caller that wants the queue replayed has to say so HERE; the call in + * `selectSession` only runs when the write was skipped entirely. * @returns {Promise<{parsedAt: number, bufferLength: number, completed: boolean}>} Parse marker snapshot */ - chunkedTerminalWrite(buffer, chunkSize = TERMINAL_CHUNK_SIZE, loadOwner) { + chunkedTerminalWrite(buffer, chunkSize = TERMINAL_CHUNK_SIZE, loadOwner, finishOpts) { // Generation counter: if a newer chunkedTerminalWrite starts (tab switch), // older writes abort instead of continuing to push stale data into the terminal. const writeGen = ++this._chunkedWriteGen; @@ -3762,7 +3771,7 @@ Object.assign(CodemanApp.prototype, { completed, }); if (!buffer || buffer.length === 0) { - this._finishBufferLoad(bufferLoadOwner); + this._finishBufferLoad(bufferLoadOwner, finishOpts); resolve(parseSnapshot()); return; } @@ -3776,7 +3785,7 @@ Object.assign(CodemanApp.prototype, { this.terminal.write(cleanBuffer, () => resolve(parseSnapshot())); // The write is now ordered in xterm's queue. Release live output before // parsing completes; subsequent writes stay behind it without being lost. - this._finishBufferLoad(bufferLoadOwner); + this._finishBufferLoad(bufferLoadOwner, finishOpts); return; } @@ -3807,7 +3816,7 @@ Object.assign(CodemanApp.prototype, { ); resolve(result); }); - this._finishBufferLoad(bufferLoadOwner); + this._finishBufferLoad(bufferLoadOwner, finishOpts); return; } @@ -3826,10 +3835,20 @@ Object.assign(CodemanApp.prototype, { * Called when chunkedTerminalWrite finishes (or is skipped for empty buffers). * * By default queued SSE events are DISCARDED, not flushed. For an established - * session the loaded buffer from the API is the source of truth up to the - * response timestamp; SSE events queued during the fetch+write overlap already - * appear in that buffer, so flushing them writes duplicate data (especially Ink - * cursor-up redraws), corrupting the terminal display. + * session whose buffer came from the server's accumulated byte history, that + * history is the source of truth up to the response timestamp; SSE events + * queued during the fetch+write overlap already appear in it, so flushing + * them writes duplicate data (especially Ink cursor-up redraws), corrupting + * the terminal display. + * + * A tmux PANE CAPTURE is the exception, and the reason `since` exists. A + * capture is a point-in-time frame taken part-way through the fetch, so it is + * the source of truth only up to CAPTURE time — not up to the response. Every + * event that arrives between the capture and the end of the chunked write is + * queued and, under a plain discard, lost outright: nothing re-fetches, and + * the CLI's next partial redraw lands on a frame the terminal never received. + * The caller passes the response's own arrival time as `since` so exactly + * that tail is replayed and the pre-capture events stay dropped. * * COD-144: a brand-new session is the exception. Its terminal fetch can resolve * BEFORE the PTY emits its first prompt, so the fetched buffer is empty and the @@ -3843,14 +3862,22 @@ Object.assign(CodemanApp.prototype, { * After unblocking, new SSE/WS events deliver subsequent output normally. * * @param {string} [owner] Load token from `_beginBufferLoad`; a stale owner is a no-op. - * @param {{ flushQueued?: boolean }} [opts] When `flushQueued` is true, replay any queued events. + * @param {{ flushQueued?: boolean, since?: number }} [opts] When `flushQueued` + * is true, replay queued events whose arrival timestamp is at or after + * `since` (default 0, meaning the whole queue). */ _beginBufferLoad(owner) { if (this._bufferLoadSeq === undefined) this._bufferLoadSeq = 0; const loadOwner = owner === undefined ? `buffer-${++this._bufferLoadSeq}` : owner; + // `selectSession` opens the load before its fetch, and `chunkedTerminalWrite` + // opens it again under the SAME owner when it starts writing. Resetting the + // queue on that second call would throw away everything that arrived during + // the fetch, which on the capture path is output no buffer holds. Re-entering + // one load keeps its queue; a genuinely new load still starts empty. + const reentering = this._bufferLoadOwner === loadOwner && Array.isArray(this._loadBufferQueue); this._bufferLoadOwner = loadOwner; this._isLoadingBuffer = true; - this._loadBufferQueue = []; + if (!reentering) this._loadBufferQueue = []; return loadOwner; }, @@ -3864,9 +3891,13 @@ Object.assign(CodemanApp.prototype, { this._bufferLoadOwner = null; // COD-144: replay (rather than discard) queued live events when the load // painted nothing — the queued prompt is the only content a new session has. + // A tmux-capture load replays too, but only the tail: `since` cuts the queue + // at the moment the capture stopped being able to contain what arrived. if (opts?.flushQueued && queued && queued.length) { - for (const data of queued) { - this.batchTerminalWrite(data); + const since = typeof opts.since === 'number' ? opts.since : 0; + for (const entry of queued) { + if (entry.at < since) continue; + this.batchTerminalWrite(entry.data); } } return true; diff --git a/test/capture-load-window.browser.test.ts b/test/capture-load-window.browser.test.ts new file mode 100644 index 00000000..9f1ee810 --- /dev/null +++ b/test/capture-load-window.browser.test.ts @@ -0,0 +1,177 @@ +/** + * @fileoverview Output arriving after a pane capture survives the buffer load. + * + * `batchTerminalWrite` queues live terminal events while a buffer load runs, + * and `_finishBufferLoad` discards that queue by default. That is right when + * the loaded buffer is the server's accumulated byte history, which is current + * up to the response. A tmux pane capture is current only up to CAPTURE time, + * so anything arriving between the capture and the end of the chunked write is + * queued and then dropped, with nothing scheduling a re-fetch. + * + * The queue now stamps each entry with its arrival time, and a capture load + * replays the tail that arrived after the response headers. These drive the + * real client in chromium: the event is injected from inside the response's + * own `json()` call, which is the one place guaranteed to land after the + * headers and before the chunked write. + * + * Port: 3256 (capture load window) + * + * Run: npx vitest run --config config/vitest.browser.config.ts test/capture-load-window.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 = 3256; +const BASE_URL = `http://localhost:${PORT}`; +const MARKER = 'ARRIVED-AFTER-THE-CAPTURE'; + +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); + +/** + * Select the session with the terminal fetch stubbed, injecting one live event + * from inside `json()`. Returns how many terminal rows carry the marker, so a + * flush that replays too much fails as loudly as one that replays nothing. + */ +async function runLoad(page: Page, sessionId: string, source: string): Promise { + return page.evaluate( + async ({ sid, src, marker }) => { + const app = ( + window as unknown as { + app: { + selectSession: (id: string, o?: object) => Promise; + _onSessionTerminal: (e: { id: string; data: string }) => void; + terminal: { + buffer: { + active: { + length: number; + getLine: (i: number) => { translateToString: (t: boolean) => string } | undefined; + }; + }; + }; + }; + } + ).app; + + const realFetch = window.fetch.bind(window); + window.fetch = ((input: RequestInfo | URL, init?: RequestInit) => { + const url = String(typeof input === 'string' ? input : ((input as Request).url ?? input)); + if (!url.includes('/terminal')) return realFetch(input as RequestInfo, init); + return Promise.resolve({ + ok: true, + status: 200, + // `selectSession` timestamps the headers the moment this promise + // resolves, then calls json(). Injecting here puts the event after + // that timestamp and inside the load window, which is exactly the + // gap a pane capture cannot cover. + json: async () => { + app._onSessionTerminal({ id: sid, data: `\r\n${marker}\r\n` }); + return { + success: true, + data: { + terminalBuffer: '\x1b[1;1Hcaptured frame line one\r\n', + status: 'idle', + fullSize: 512, + retainedBytes: 512, + truncated: false, + truncationReason: null, + source: src, + captureCols: 80, + captureRows: 24, + }, + }; + }, + }) as unknown as Promise; + }) as typeof window.fetch; + + try { + await app.selectSession(sid); + await new Promise((r) => setTimeout(r, 1200)); + const buf = app.terminal.buffer.active; + let hits = 0; + for (let i = 0; i < buf.length; i++) { + if (buf.getLine(i)?.translateToString(true).includes(marker)) hits += 1; + } + return hits; + } finally { + window.fetch = realFetch; + } + }, + { sid: sessionId, src: source, marker: MARKER } + ); +} + +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 loads from /vendor, so the terminal appears a beat after the app. + // Without it every buffer assertion below would throw rather than compare. + 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-load-window-test' }), + }); + const body = await res.json(); + return body.data?.session?.id ?? body.data?.id ?? body.id; + }); +} + +describe('output emitted during a capture load', () => { + let context: BrowserContext; + let page: Page; + + afterAll(async () => { + await context?.close(); + }); + + it('reaches the terminal exactly once when the buffer came from a pane capture', async () => { + context = await browser.newContext({ viewport: { width: 1280, height: 800 } }); + page = await context.newPage(); + const sessionId = await openSession(page); + expect(sessionId).toBeTruthy(); + + // Exactly once. The cutoff exists so the flush cannot also replay events the + // payload already carried, which would double the output rather than heal it. + expect(await runLoad(page, sessionId, 'mux-visible')).toBe(1); + + await page.evaluate( + (sid: string) => fetch(`/api/sessions/${sid}`, { method: 'DELETE' }).then(() => undefined), + sessionId + ); + await context.close(); + }, 60_000); + + it('stays dropped when the buffer came from the accumulated byte history', async () => { + // The byte history already contains everything up to the response, so + // replaying the queue on top of it would duplicate the output — most + // visibly Ink's cursor-up redraws. The discard has to survive this fix. + context = await browser.newContext({ viewport: { width: 1280, height: 800 } }); + page = await context.newPage(); + const sessionId = await openSession(page); + + expect(await runLoad(page, sessionId, 'history')).toBe(0); + + await page.evaluate( + (sid: string) => fetch(`/api/sessions/${sid}`, { method: 'DELETE' }).then(() => undefined), + sessionId + ); + await context.close(); + }, 60_000); +}); diff --git a/test/terminal-buffer-flush.test.ts b/test/terminal-buffer-flush.test.ts index 54267beb..0500a3e6 100644 --- a/test/terminal-buffer-flush.test.ts +++ b/test/terminal-buffer-flush.test.ts @@ -56,10 +56,10 @@ type BufferLoadApp = { _bufferLoadSeq: number; _bufferLoadOwner: string | null; _isLoadingBuffer: boolean; - _loadBufferQueue: string[] | null; + _loadBufferQueue: { at: number; data: string }[] | null; batchTerminalWrite: (data: string) => void; _beginBufferLoad: (owner?: string) => string; - _finishBufferLoad: (owner?: string, opts?: { flushQueued?: boolean }) => boolean; + _finishBufferLoad: (owner?: string, opts?: { flushQueued?: boolean; since?: number }) => boolean; }; /** @@ -84,10 +84,13 @@ function makeApp() { return { app, writes }; } -/** Simulate live SSE events arriving while a buffer load is in progress (the queue path). */ -function pushWhileLoading(app: BufferLoadApp, data: string) { - // Mirrors batchTerminalWrite's queue branch: if loading, push to the queue. - if (app._isLoadingBuffer && app._loadBufferQueue) app._loadBufferQueue.push(data); +/** + * Simulate a live SSE event arriving while a buffer load is in progress. + * Mirrors batchTerminalWrite's queue branch, which stamps each entry with its + * arrival time so a flush can replay only the tail (see the `since` tests). + */ +function pushWhileLoading(app: BufferLoadApp, data: string, at = performance.now()) { + if (app._isLoadingBuffer && app._loadBufferQueue) app._loadBufferQueue.push({ at, data }); } describe('buffer-load flush (COD-144)', () => { @@ -153,11 +156,91 @@ describe('buffer-load flush (COD-144)', () => { // State untouched — still loading, queue intact, nothing replayed. expect(app._isLoadingBuffer).toBe(true); expect(app._bufferLoadOwner).toBe('real-owner'); - expect(app._loadBufferQueue).toEqual(['queued']); + expect(app._loadBufferQueue).toEqual([{ at: expect.any(Number), data: 'queued' }]); expect(app.batchTerminalWrite).not.toHaveBeenCalled(); expect(writes).toEqual([]); }); + // ── The tmux-capture tail: `since` ── + // + // A pane capture is a point-in-time frame taken part-way through the fetch, so + // it holds what arrived BEFORE the capture and nothing after. selectSession + // passes the response's arrival time as `since`, which splits the queue at + // exactly that line: pre-capture events are already painted and must stay + // dropped, post-capture events exist nowhere else and must be replayed. + + it('flushes only the entries at or after `since`', () => { + const { app, writes } = makeApp(); + const owner = app._beginBufferLoad('load-since'); + pushWhileLoading(app, 'already-in-the-capture', 100); + pushWhileLoading(app, 'arrived-at-the-headers', 200); + pushWhileLoading(app, 'arrived-after-the-headers', 300); + + app._finishBufferLoad(owner, { flushQueued: true, since: 200 }); + + // The pre-capture event stays dropped; the boundary entry counts as after. + expect(writes).toEqual(['arrived-at-the-headers', 'arrived-after-the-headers']); + }); + + it('flushQueued without `since` still replays the whole queue', () => { + // The COD-144 path: a brand-new session's first prompt predates the + // response, so cutting the queue would drop the only content it has. + const { app, writes } = makeApp(); + const owner = app._beginBufferLoad('load-no-since'); + pushWhileLoading(app, 'prompt', 10); + pushWhileLoading(app, 'more', 20); + + app._finishBufferLoad(owner, { flushQueued: true }); + + expect(writes).toEqual(['prompt', 'more']); + }); + + it('a `since` past every entry flushes nothing', () => { + const { app, writes } = makeApp(); + const owner = app._beginBufferLoad('load-since-late'); + pushWhileLoading(app, 'old', 10); + + app._finishBufferLoad(owner, { flushQueued: true, since: 999 }); + + expect(writes).toEqual([]); + expect(app.batchTerminalWrite).not.toHaveBeenCalled(); + }); + + // ── Re-entering one load ── + // + // `selectSession` opens the load before its fetch, and `chunkedTerminalWrite` + // opens it again under the SAME owner when it starts writing. A reset on that + // second call would silently throw away everything queued during the fetch, + // which on the capture path is output no buffer holds. + + it('re-entering the same load keeps what the queue already holds', () => { + const { app, writes } = makeApp(); + const owner = app._beginBufferLoad('load-reenter'); + pushWhileLoading(app, 'arrived-during-the-fetch', 100); + + // chunkedTerminalWrite re-opens the load it was handed. + app._beginBufferLoad(owner); + pushWhileLoading(app, 'arrived-during-the-write', 200); + + app._finishBufferLoad(owner, { flushQueued: true, since: 50 }); + + expect(writes).toEqual(['arrived-during-the-fetch', 'arrived-during-the-write']); + }); + + it('a genuinely different load still starts with an empty queue', () => { + const { app, writes } = makeApp(); + app._beginBufferLoad('load-first'); + pushWhileLoading(app, 'belongs-to-the-abandoned-load', 100); + + // A tab switch starts a new load under a new owner. Its events are not ours. + const second = app._beginBufferLoad('load-second'); + pushWhileLoading(app, 'belongs-to-this-load', 200); + + app._finishBufferLoad(second, { flushQueued: true, since: 0 }); + + expect(writes).toEqual(['belongs-to-this-load']); + }); + it('empty queue + flushQueued is a no-op (no throw, no writes)', () => { const { app, writes } = makeApp(); const owner = app._beginBufferLoad('load-empty'); From 75a028e825d8e38dc55bf42c4b618c92566df114 Mon Sep 17 00:00:00 2001 From: Michael Grundberg Date: Fri, 18 Sep 2026 16:43:04 +0200 Subject: [PATCH 2/4] fix(terminal): re-take the sticky-scroll baseline after a replay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A capture load now replays its queued tail, and that replay runs through `batchTerminalWrite`, which samples `_wasAtBottomBeforeWrite` before it queues. It runs inside `chunkedTerminalWrite`, before that promise resolves, with the terminal freshly reset and rewritten — so the sample is always true. The caller then restored the reader's position and the next `flushPendingWrites` scrolled straight back to the bottom off the latched flag, undoing it. The only thing in the way was `_hasRecentUserScrollUp()`, a 1500ms window a server-triggered refresh is usually past. `_syncStickyScrollBaseline()` re-takes the flag from wherever the viewport now sits, and the two paths that restore a position call it right after doing so: `_onSessionNeedsRefresh` and `_maybeRefetchFullHistory`. Those are the paths #259 and #205 exist for, and they are also where a non-empty queue is most likely, since a needsRefresh fires when output is flooding. Re-taking rather than suppressing the sampling: suppressing leaves whatever stale value the flag held from before the load, which on the full-history re-pull has no reason to be false. `selectSession` and `_onSessionClearTerminal` deliberately end at the bottom, so the sampled true is already the truth there and they do not call it. `_bufferLoadFinishOpts` gains the coverage the CI gate can see: both mux sources flush, `history` does not, and a payload naming no source does not. Its only coverage was the browser suite, which CI does not run. The JSDoc and the changeset now record the one duplicate window this cutoff cannot close. The server appends output to the byte buffer in the same tick it emits, but broadcasts on a batch timer — 8ms over WebSocket, 16 to 50ms over SSE — so a batch pending when `capture-pane` ran leaves the server after the reply and is replayed although the capture holds it. It is one batch interval wide against a recovery window spanning the whole chunked write, and closing it means flushing that batch server side before the capture. The second browser test asserts its session was created, so a failed create fails it instead of passing with zero hits. docs/architecture-invariants.md no longer claims the replay leaves the queued-event discard window alone. That clause now describes what decides how a load ends, the baseline rule, the batch window, and the three covering tests. Co-Authored-By: Claude Opus 5 (1M context) --- ...y-output-that-arrived-after-the-capture.md | 15 +++ docs/architecture-invariants.md | 2 +- src/web/public/app.js | 20 ++++ src/web/public/terminal-ui.js | 21 ++++ test/capture-load-window.browser.test.ts | 3 + test/terminal-buffer-flush.test.ts | 98 +++++++++++++++++++ test/terminal-flush-budget.test.ts | 45 +++++++++ 7 files changed, 203 insertions(+), 1 deletion(-) diff --git a/.changeset/fix-replay-output-that-arrived-after-the-capture.md b/.changeset/fix-replay-output-that-arrived-after-the-capture.md index 03e09189..0cb24af0 100644 --- a/.changeset/fix-replay-output-that-arrived-after-the-capture.md +++ b/.changeset/fix-replay-output-that-arrived-after-the-capture.md @@ -34,3 +34,18 @@ takes the flush policy and applies it at its own finish sites. `_beginBufferLoad` no longer empties the queue when the same load re-enters it, which it does on every write, because that reset discarded the fetch window before anything could replay it. + +A path that replays its queue and then restores a scroll position re-takes the +sticky-scroll baseline (`_syncStickyScrollBaseline`). The replay runs with the +terminal freshly reset, so it reads as sitting at the bottom, and the next flush +would scroll there and undo the restore. The backpressure refresh and the +full-history re-pull are the two paths that restore a position, and both are +ones a reader reaches while scrolled up. + +One duplicate window stays open and is not closable from the browser. The server +appends output to the byte buffer in the same tick it emits, but broadcasts on a +batch timer, 8ms over WebSocket and 16 to 50ms over SSE. A batch already pending +when `capture-pane` ran therefore leaves the server after the reply and is +replayed although the capture holds it. It is one batch interval wide, against a +recovery window that spans the whole chunked write, and closing it means +flushing that session's pending batch before taking the capture. diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index 20f6bda9..3a0e8733 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -116,7 +116,7 @@ Tests: `test/docker-hosts.test.ts`, `test/docker-exec-options.test.ts`, `test/do ### Full-scrollback replay -**Full-scrollback replay** (COD-164/#148, reworked for #205): `GET /api/sessions/:id/terminal?full=1` returns the ENTIRE tmux scrollback (capture-pane `-e -S -` bounded by the configured history limit, explicit `maxBuffer` from the terminal-history config, early byte-cap before normalization, CRLF-normalized for shell panes). On success the capture is returned ALONE (`source='mux-full-history'` — it supersedes the byte buffer; no duplication). The first load of each non-shell TUI session per page requests `full=1` (`_fullHistoryLoaded` Set in app.js — the old one-shot `_initialFullBufferLoad` flag was consumed by whichever tab auto-selected, leaving every other TUI tab one frame of history). Shell sessions instead load a bounded 1 MiB `?tail=` window on every selection and automatic drop recovery: a 100k-line shell capture can be tens of MiB, and automatically parsing it makes tab-switch latency scale with the entire session. Shell full history is explicit-button-only; reaching the top during an ordinary wheel/touch gesture must not reset xterm and replay the multi-megabyte capture on its main thread. Other modes may still re-pull `full=1` at the TOP, and pressing **Load full history** forces the request for any recoverably truncated session (`_maybeRefetchFullHistory`, 4s per-session gesture cooldown, in-flight + tab-switch guards, viewport position held across the replay); Shell full pulls are not retained in the tab cache, so the next switch stays bounded. Chunked replay enqueues 32 KiB pieces across safe yields, appends an xterm parse marker, then releases the live-output gate; output arriving after that release stays ordered behind the snapshot, while the marker callback supplies accurate parse timing without extending the pre-existing queued-event discard window. Live output is separately one-chunk-in-flight: xterm's callback releases each 32/64 KiB write before the next is submitted, keeping the remainder in the app queue where the 128 KiB cap can observe it instead of hiding an unbounded backlog in xterm's private WriteBuffer. While WebSocket owns terminal I/O, parallel SSE terminal/output-recovery events are discarded before JSON parsing; fallback recovery is single-flight per active session so backpressure cannot start overlapping reset+replay cycles. The route exposes capture/prepare totals in `Server-Timing`, while `[TERMINAL-PERF]` separates TTFB, body/JSON, reset+parse and total time for both selection and on-demand full pulls; parse completion is not a browser compositor/GPU paint measurement. The re-pull exists because xterm's buffer is only a WINDOW onto tmux's history and two things shrink it: tmux coalesces bursty output into pane REPAINTS that overwrite rows instead of emitting linefeeds (measured: a 60-line burst added 1 row of browser scrollback and destroyed 34), and a tab switch replays only the visible frame. tmux's own history is intact throughout — the browser just has to ask for it again. On-demand rather than automatic because at a 100k history limit the capture can be megabytes. ⚠️ **The capture ENDS with a cursor move back to the pane's own caret position** (`formatCursorRestore`, from the same `display-message` query the visible-frame path uses). The linear replay otherwise leaves the caret wherever the last character landed — the bottom-most row carrying text, which for an agent CLI is the status line — so the caret sat on the composer's border instead of its input line and every cursor-relative update the CLI sent afterwards was measured from the wrong row, until its next full redraw silently repaired it (that self-repair is why the report read as "it fixes itself as soon as Claude writes a line"). ⚠️ **The move is RELATIVE — up `rows - 1 - cursor_y`, then `\r`, then right `cursor_x` — never `CUP`.** `\x1b[;H` numbers rows from the top of the browser's screen, so it lands correctly only while the browser's row count equals `pane_height`, and nothing guarantees that: `resizeWindow` issues its tmux resize fire-and-forget and returns immediately, so a capture can be taken before a requested resize has applied, and `_onSessionNeedsRefresh` sends no resize at all. Counting up from the last replayed row anchors to the content both ends share. Restoring the cursor makes ROW ALIGNMENT load-bearing on this path: **no transform that can DELETE A LINE may run over a full-history capture**, because every deletion shifts the frame out from under the restored position. Four had accumulated — trailing blank rows stripped by `\n+$`, `stripInkRedrawBloat`, the `CLAUDE_BANNER_PATTERN` trim that cuts everything above the banner, and `LEADING_WHITESPACE_PATTERN` — each correct for a byte stream of successive frames and each wrong for a single rendered frame. ⚠️ **Those skips key on `isFullCapture`, meaning a capture actually came back — never on `?full=1` alone.** When `captureActivePaneBuffer` returns null (ENOBUFS, a timeout, a vanished pane, or a session with no mux at all) the reply falls back to `session.terminalBuffer`, which IS a byte stream and must still be stripped; gating on the query flag returned it whole, and a direct-PTY session takes that path on every first selection rather than only during an outage. ⚠️ A capture holding nothing visible (`hasVisibleContent`) returns `''`, because the caller reads an empty capture as "unavailable" and keeps its byte history — retaining trailing blank rows made an all-blank pane non-empty, which would have replaced real history with a blank screen from the server side, where `_replayWouldShrinkBuffer` cannot see it. ⚠️ **"One line per screen row" holds only where no row was hard-wrapped**: `-J` joins a wrapped row into its logical line (measured: a 100-character line in a 40-column pane captures as 10 lines against a 12-row pane), and the counts reconcile only once the browser xterm re-wraps at the same width — the same assumption `_estimateReplayRows` already documents. Tests: `test/tmux-capture-full-history.test.ts` covers the cursor move, the trim pairing and `hasVisibleContent`; `test/routes/session-routes.test.ts` covers a surviving blank first row, an unstripped byte-history fallback, and an empty capture leaving history intact. ⚠️ **The re-pull must never DOWNGRADE the buffer** (#205 round 2): the same reasoning that makes it a win for a shell pane makes it destructive for a repaint-mode CLI pane, where tmux keeps no history of its own (`history_size≈0` measured for a Claude pane) and the capture is roughly ONE frame while xterm may hold hundreds of rows of replayed frames — `_resetTerminalForReplay()` + rewrite then deletes history mid-scroll ("goes back a bit, repeats blocks, gets worse the further up I go"; measured A/B on a live pane: 341 rows → 42 with the guard off). `_replayWouldShrinkBuffer()` (terminal-ui.js) estimates the capture's rendered rows — escape sequences stripped, `capture-pane -J` re-wrapping accounted for — and the pull is skipped when that is more than one screen short of `buffer.active.length`. The one-screen tolerance matters: both sides are estimates (the buffer length counts trailing blank rows), so only a clear downgrade is refused. A refused session joins `_fullHistoryRepullUseless`, raising its cooldown from 4s to 60s so a hollow pane stops re-fetching megabytes on every scroll-up. Tests: `test/tmux-capture-full-history.test.ts`, `test/tmux-scrollback-eol.test.ts`, `test/terminal-scroll-routing.test.ts`, `test/terminal-flush-budget.test.ts`. +**Full-scrollback replay** (COD-164/#148, reworked for #205): `GET /api/sessions/:id/terminal?full=1` returns the ENTIRE tmux scrollback (capture-pane `-e -S -` bounded by the configured history limit, explicit `maxBuffer` from the terminal-history config, early byte-cap before normalization, CRLF-normalized for shell panes). On success the capture is returned ALONE (`source='mux-full-history'` — it supersedes the byte buffer; no duplication). The first load of each non-shell TUI session per page requests `full=1` (`_fullHistoryLoaded` Set in app.js — the old one-shot `_initialFullBufferLoad` flag was consumed by whichever tab auto-selected, leaving every other TUI tab one frame of history). Shell sessions instead load a bounded 1 MiB `?tail=` window on every selection and automatic drop recovery: a 100k-line shell capture can be tens of MiB, and automatically parsing it makes tab-switch latency scale with the entire session. Shell full history is explicit-button-only; reaching the top during an ordinary wheel/touch gesture must not reset xterm and replay the multi-megabyte capture on its main thread. Other modes may still re-pull `full=1` at the TOP, and pressing **Load full history** forces the request for any recoverably truncated session (`_maybeRefetchFullHistory`, 4s per-session gesture cooldown, in-flight + tab-switch guards, viewport position held across the replay); Shell full pulls are not retained in the tab cache, so the next switch stays bounded. Chunked replay enqueues 32 KiB pieces across safe yields, appends an xterm parse marker, then releases the live-output gate; output arriving after that release stays ordered behind the snapshot, and the marker callback supplies accurate parse timing. ⚠️ **How the load ENDS depends on where the payload came from**, and `_bufferLoadFinishOpts` (app.js) is the one place that decides it for all four fetch-and-write paths. A payload built from the server's accumulated byte history is current up to the response, so the events queued during the load already appear in it and stay DISCARDED; replaying them would duplicate output, most visibly Ink's cursor-up redraws. A pane capture (`mux-visible` or `mux-full-history`) is current only up to CAPTURE time, so `_finishBufferLoad` replays the queue from the response's own arrival timestamp (`since`) and the pre-capture events stay dropped. ⚠️ **A path that then restores a scroll position must re-take the sticky-scroll baseline** (`_syncStickyScrollBaseline`): the replay runs inside `chunkedTerminalWrite` before its promise resolves, with the terminal freshly reset, so `batchTerminalWrite` samples `_wasAtBottomBeforeWrite` as true and the next `flushPendingWrites` would scroll to the bottom over the restore. ⚠️ The cutoff is a client-side timestamp and the server broadcasts on a batch timer (8ms WebSocket, 16-50ms SSE), so a batch pending when the capture ran arrives after the response and replays although the capture holds it — bounded by one batch interval, and closable only server side by flushing that batch before the capture. Tests for the three: `test/terminal-flush-budget.test.ts` pins which sources flush, `test/terminal-buffer-flush.test.ts` pins the `since` cutoff and the baseline re-take, and `test/capture-load-window.browser.test.ts` drives both against a live server. Live output is separately one-chunk-in-flight: xterm's callback releases each 32/64 KiB write before the next is submitted, keeping the remainder in the app queue where the 128 KiB cap can observe it instead of hiding an unbounded backlog in xterm's private WriteBuffer. While WebSocket owns terminal I/O, parallel SSE terminal/output-recovery events are discarded before JSON parsing; fallback recovery is single-flight per active session so backpressure cannot start overlapping reset+replay cycles. The route exposes capture/prepare totals in `Server-Timing`, while `[TERMINAL-PERF]` separates TTFB, body/JSON, reset+parse and total time for both selection and on-demand full pulls; parse completion is not a browser compositor/GPU paint measurement. The re-pull exists because xterm's buffer is only a WINDOW onto tmux's history and two things shrink it: tmux coalesces bursty output into pane REPAINTS that overwrite rows instead of emitting linefeeds (measured: a 60-line burst added 1 row of browser scrollback and destroyed 34), and a tab switch replays only the visible frame. tmux's own history is intact throughout — the browser just has to ask for it again. On-demand rather than automatic because at a 100k history limit the capture can be megabytes. ⚠️ **The capture ENDS with a cursor move back to the pane's own caret position** (`formatCursorRestore`, from the same `display-message` query the visible-frame path uses). The linear replay otherwise leaves the caret wherever the last character landed — the bottom-most row carrying text, which for an agent CLI is the status line — so the caret sat on the composer's border instead of its input line and every cursor-relative update the CLI sent afterwards was measured from the wrong row, until its next full redraw silently repaired it (that self-repair is why the report read as "it fixes itself as soon as Claude writes a line"). ⚠️ **The move is RELATIVE — up `rows - 1 - cursor_y`, then `\r`, then right `cursor_x` — never `CUP`.** `\x1b[;H` numbers rows from the top of the browser's screen, so it lands correctly only while the browser's row count equals `pane_height`, and nothing guarantees that: `resizeWindow` issues its tmux resize fire-and-forget and returns immediately, so a capture can be taken before a requested resize has applied, and `_onSessionNeedsRefresh` sends no resize at all. Counting up from the last replayed row anchors to the content both ends share. Restoring the cursor makes ROW ALIGNMENT load-bearing on this path: **no transform that can DELETE A LINE may run over a full-history capture**, because every deletion shifts the frame out from under the restored position. Four had accumulated — trailing blank rows stripped by `\n+$`, `stripInkRedrawBloat`, the `CLAUDE_BANNER_PATTERN` trim that cuts everything above the banner, and `LEADING_WHITESPACE_PATTERN` — each correct for a byte stream of successive frames and each wrong for a single rendered frame. ⚠️ **Those skips key on `isFullCapture`, meaning a capture actually came back — never on `?full=1` alone.** When `captureActivePaneBuffer` returns null (ENOBUFS, a timeout, a vanished pane, or a session with no mux at all) the reply falls back to `session.terminalBuffer`, which IS a byte stream and must still be stripped; gating on the query flag returned it whole, and a direct-PTY session takes that path on every first selection rather than only during an outage. ⚠️ A capture holding nothing visible (`hasVisibleContent`) returns `''`, because the caller reads an empty capture as "unavailable" and keeps its byte history — retaining trailing blank rows made an all-blank pane non-empty, which would have replaced real history with a blank screen from the server side, where `_replayWouldShrinkBuffer` cannot see it. ⚠️ **"One line per screen row" holds only where no row was hard-wrapped**: `-J` joins a wrapped row into its logical line (measured: a 100-character line in a 40-column pane captures as 10 lines against a 12-row pane), and the counts reconcile only once the browser xterm re-wraps at the same width — the same assumption `_estimateReplayRows` already documents. Tests: `test/tmux-capture-full-history.test.ts` covers the cursor move, the trim pairing and `hasVisibleContent`; `test/routes/session-routes.test.ts` covers a surviving blank first row, an unstripped byte-history fallback, and an empty capture leaving history intact. ⚠️ **The re-pull must never DOWNGRADE the buffer** (#205 round 2): the same reasoning that makes it a win for a shell pane makes it destructive for a repaint-mode CLI pane, where tmux keeps no history of its own (`history_size≈0` measured for a Claude pane) and the capture is roughly ONE frame while xterm may hold hundreds of rows of replayed frames — `_resetTerminalForReplay()` + rewrite then deletes history mid-scroll ("goes back a bit, repeats blocks, gets worse the further up I go"; measured A/B on a live pane: 341 rows → 42 with the guard off). `_replayWouldShrinkBuffer()` (terminal-ui.js) estimates the capture's rendered rows — escape sequences stripped, `capture-pane -J` re-wrapping accounted for — and the pull is skipped when that is more than one screen short of `buffer.active.length`. The one-screen tolerance matters: both sides are estimates (the buffer length counts trailing blank rows), so only a clear downgrade is refused. A refused session joins `_fullHistoryRepullUseless`, raising its cooldown from 4s to 60s so a hollow pane stops re-fetching megabytes on every scroll-up. Tests: `test/tmux-capture-full-history.test.ts`, `test/tmux-scrollback-eol.test.ts`, `test/terminal-scroll-routing.test.ts`, `test/terminal-flush-budget.test.ts`. ### Terminal scrollback: strip flavors and wheel/touch forwarding diff --git a/src/web/public/app.js b/src/web/public/app.js index ab303da5..e96e78b7 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -1921,6 +1921,16 @@ class CodemanApp { * the moment the response arrived, compared only against other client-side * readings, so there is no clock skew to worry about. * + * What this cutoff does NOT cover: the server appends output to the byte + * buffer and emits it in the same tick, but it BROADCASTS on a batch timer — + * 8ms over WebSocket, 16 to 50ms over SSE. The terminal route runs + * synchronously from `capture-pane` to its return, so a batch that was + * already pending when the capture ran leaves the server after the reply, + * arrives after `headersReceivedAt`, and is replayed although the capture + * holds it. The duplicate is one batch interval wide, against a recovery + * window that spans the whole chunked write. Closing it belongs on the + * server: flush that session's pending batch before taking the capture. + * * @param {{source?: string}} payload - The parsed `data` of a terminal response. * @param {number} headersReceivedAt - When that response reached this client. * @returns {{flushQueued: boolean, since: number}} Options for `_finishBufferLoad`. @@ -2557,6 +2567,10 @@ class CodemanApp { }); if (target === null || typeof this.terminal.scrollToLine !== 'function') this.terminal.scrollToBottom(); else this.terminal.scrollToLine(target); + // The load's own replay sampled the sticky-scroll baseline while the + // terminal sat at the bottom of a just-rewritten buffer, so the next + // flush would scroll back down and undo the restore above. + this._syncStickyScrollBaseline(); // Re-position local echo overlay at new prompt location this._localEchoOverlay?.rerender(); // Resize PTY to match actual browser dimensions (critical for OpenCode @@ -5842,6 +5856,12 @@ class CodemanApp { const delta = parsedBufferLength - rowsBefore; if (delta > 0) this.terminal.scrollToLine(delta); else this.terminal.scrollToTop(); + // The load's own replay sampled the sticky-scroll baseline while the + // terminal sat at the bottom of a just-rewritten buffer, so the next + // flush would scroll back down and undo the restore above. This path is + // reached only from a scroll-up gesture, so being dragged down is the + // exact opposite of what the user asked for. + this._syncStickyScrollBaseline(); timing.totalMs = performance.now() - requestStartedAt; this._recordTerminalLoadTiming(timing); } catch { diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index 1a265d61..b56f5510 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -3131,6 +3131,27 @@ Object.assign(CodemanApp.prototype, { return buffer.viewportY >= buffer.baseY - 2; }, + /** + * Re-take the sticky-scroll baseline from where the viewport now sits. + * + * `batchTerminalWrite` samples `_wasAtBottomBeforeWrite` before it queues + * data, and `flushPendingWrites` scrolls to the bottom off that sample. A + * buffer load that replays its queue samples at the worst possible moment: + * `_finishBufferLoad` runs inside `chunkedTerminalWrite`, before its promise + * resolves, with the terminal freshly reset and rewritten, so the sample is + * always true. A caller that then restores the reader's position would have + * that restore undone by the next flush. + * + * Every caller that scrolls the viewport somewhere other than the bottom + * after a load must call this, so the baseline describes the position the + * caller chose. `selectSession` and `_onSessionClearTerminal` deliberately + * end at the bottom, so for them the sampled true is already the truth and + * they do not call it. + */ + _syncStickyScrollBaseline() { + this._wasAtBottomBeforeWrite = this.isTerminalAtBottom(); + }, + // Record manual scroll gestures so sticky-scroll can give an upward scroll a // short grace window (see _hasRecentUserScrollUp). A downward scroll that // lands back at the bottom clears the suppression immediately. diff --git a/test/capture-load-window.browser.test.ts b/test/capture-load-window.browser.test.ts index 9f1ee810..8cea77ef 100644 --- a/test/capture-load-window.browser.test.ts +++ b/test/capture-load-window.browser.test.ts @@ -165,6 +165,9 @@ describe('output emitted during a capture load', () => { context = await browser.newContext({ viewport: { width: 1280, height: 800 } }); page = await context.newPage(); const sessionId = await openSession(page); + // Without this, a failed create passes the zero-hit assertion below + // vacuously — nothing was loaded, so nothing was replayed. + expect(sessionId).toBeTruthy(); expect(await runLoad(page, sessionId, 'history')).toBe(0); diff --git a/test/terminal-buffer-flush.test.ts b/test/terminal-buffer-flush.test.ts index 0500a3e6..36f652ef 100644 --- a/test/terminal-buffer-flush.test.ts +++ b/test/terminal-buffer-flush.test.ts @@ -84,6 +84,50 @@ function makeApp() { return { app, writes }; } +/** + * A stub carrying the REAL `batchTerminalWrite` on top of the real begin/finish + * methods, so a replay samples the sticky-scroll baseline exactly as it does in + * the browser. The terminal is a fake whose `buffer.active` the test moves by + * hand, which is what a caller's `scrollToLine` does to a real one. + */ +function makeScrollApp() { + const buffer = { viewportY: 0, baseY: 100 }; + const app = { + buffer, + terminal: { buffer: { active: buffer } }, + sessions: new Map(), + activeSessionId: null, + pendingWrites: [] as string[], + writeFrameScheduled: false, + _wasAtBottomBeforeWrite: false, + _bufferLoadSeq: 0, + _bufferLoadOwner: null as string | null, + _isLoadingBuffer: false, + _loadBufferQueue: null as { at: number; data: string }[] | null, + _scheduleTerminalWriteFlush: vi.fn(), + batchTerminalWrite: mixin.batchTerminalWrite as (data: string) => void, + isTerminalAtBottom: mixin.isTerminalAtBottom as () => boolean, + _syncStickyScrollBaseline: mixin._syncStickyScrollBaseline as () => void, + _beginBufferLoad: mixin._beginBufferLoad as BufferLoadApp['_beginBufferLoad'], + _finishBufferLoad: mixin._finishBufferLoad as BufferLoadApp['_finishBufferLoad'], + }; + return app; +} + +/** + * Slice one class method out of app.js, from its header to the next method's. + * + * Bounding the slice matters: the two methods checked below are not followed by + * a JSDoc block, so a scan for the next comment would run on into unrelated + * code and match its scroll calls instead of theirs. + */ +function methodBody(source: string, method: string): string { + const start = source.search(new RegExp(`^ {2}(?:async )?${method}\\(`, 'm')); + expect(start, `${method} not found in app.js`).toBeGreaterThan(-1); + const next = /^ {2}(?:async )?[A-Za-z_$][\w$]*\(/m.exec(source.slice(start + 1)); + return next ? source.slice(start, start + 1 + next.index) : source.slice(start); +} + /** * Simulate a live SSE event arriving while a buffer load is in progress. * Mirrors batchTerminalWrite's queue branch, which stamps each entry with its @@ -241,6 +285,60 @@ describe('buffer-load flush (COD-144)', () => { expect(writes).toEqual(['belongs-to-this-load']); }); + // ── The sticky-scroll baseline across a replay ── + // + // `batchTerminalWrite` samples `_wasAtBottomBeforeWrite` before queueing, and + // `flushPendingWrites` scrolls to the bottom off that sample. The replay runs + // inside `chunkedTerminalWrite` before its promise resolves, with the terminal + // freshly reset and rewritten, so the sample is always true. A caller that + // then restores the reader's position would have that restore undone. + + it('the replay latches the baseline true, and the viewport restore re-takes it', () => { + const app = makeScrollApp(); + const owner = app._beginBufferLoad('load-scroll'); + pushWhileLoading(app as unknown as BufferLoadApp, 'output-after-the-capture', 100); + + // The load ends with the terminal reset and rewritten, so it reads as bottom. + app.buffer.viewportY = app.buffer.baseY; + app._finishBufferLoad(owner, { flushQueued: true, since: 0 }); + expect(app._wasAtBottomBeforeWrite).toBe(true); + + // The caller now puts the reader back where they were reading. + app.buffer.viewportY = 40; + app._syncStickyScrollBaseline(); + + // The next flush must leave them there. + expect(app._wasAtBottomBeforeWrite).toBe(false); + }); + + it('a restore that lands back at the bottom keeps sticky scroll armed', () => { + const app = makeScrollApp(); + const owner = app._beginBufferLoad('load-scroll-bottom'); + pushWhileLoading(app as unknown as BufferLoadApp, 'output-after-the-capture', 100); + + app.buffer.viewportY = app.buffer.baseY; + app._finishBufferLoad(owner, { flushQueued: true, since: 0 }); + app._syncStickyScrollBaseline(); + + // A reader who was already at the bottom still wants to be carried along. + expect(app._wasAtBottomBeforeWrite).toBe(true); + }); + + it('both callers that restore a scroll position re-take the baseline', () => { + // The wiring lives in app.js, outside this file's vm harness. Without it the + // two methods below restore the viewport and the next flush undoes it. + const source = readFileSync(resolve(import.meta.dirname, '../src/web/public/app.js'), 'utf8'); + + for (const method of ['_onSessionNeedsRefresh', '_maybeRefetchFullHistory']) { + const body = methodBody(source, method); + const restoreAt = body.lastIndexOf('scrollToLine('); + const syncAt = body.indexOf('this._syncStickyScrollBaseline()'); + expect(restoreAt, `${method} no longer restores a scroll position`).toBeGreaterThan(-1); + expect(syncAt, `${method} never re-takes the baseline`).toBeGreaterThan(-1); + expect(syncAt, `${method} re-takes the baseline before its restore`).toBeGreaterThan(restoreAt); + } + }); + it('empty queue + flushQueued is a no-op (no throw, no writes)', () => { const { app, writes } = makeApp(); const owner = app._beginBufferLoad('load-empty'); diff --git a/test/terminal-flush-budget.test.ts b/test/terminal-flush-budget.test.ts index 514e93da..aa103661 100644 --- a/test/terminal-flush-budget.test.ts +++ b/test/terminal-flush-budget.test.ts @@ -322,6 +322,51 @@ describe('terminal flush budget', () => { expect(app._bufferLoadOwner).toBe(null); }); + // ── Which payloads end their load by replaying the queue ── + // + // A pane capture is current only up to capture time, so the tail that arrived + // after the response exists nowhere else and has to be replayed. The server's + // accumulated byte history is current up to the response, so replaying on top + // of it would duplicate output. `_bufferLoadFinishOpts` is the one place that + // decides this, for all four paths that fetch a terminal buffer and write it. + + it('replays the tail for a visible-pane capture', () => { + const { CodemanApp } = loadAppHarness(); + const app = Object.create(CodemanApp.prototype) as any; + + expect(app._bufferLoadFinishOpts({ source: 'mux-visible' }, 1234)).toEqual({ + flushQueued: true, + since: 1234, + }); + }); + + it('replays the tail for a full-history capture', () => { + const { CodemanApp } = loadAppHarness(); + const app = Object.create(CodemanApp.prototype) as any; + + expect(app._bufferLoadFinishOpts({ source: 'mux-full-history' }, 1234)).toEqual({ + flushQueued: true, + since: 1234, + }); + }); + + it('discards the queue for the accumulated byte history', () => { + const { CodemanApp } = loadAppHarness(); + const app = Object.create(CodemanApp.prototype) as any; + + expect(app._bufferLoadFinishOpts({ source: 'history' }, 1234).flushQueued).toBe(false); + }); + + it('discards the queue for a payload that names no source', () => { + // Fails toward the safe answer: a duplicated Ink redraw corrupts the screen, + // while a dropped tail is repaired by the CLI's next full repaint. + const { CodemanApp } = loadAppHarness(); + const app = Object.create(CodemanApp.prototype) as any; + + expect(app._bufferLoadFinishOpts({}, 1234).flushQueued).toBe(false); + expect(app._bufferLoadFinishOpts(undefined, 1234).flushQueued).toBe(false); + }); + it('does not snap back to bottom during Codex Working redraws right after the user scrolls up', () => { const { app } = loadTerminalUiHarness('codex'); const scrollToBottom = vi.fn(); From cfd771d1d8121e80791b8cc9cf148853c5b845d1 Mon Sep 17 00:00:00 2001 From: Michael Grundberg Date: Fri, 18 Sep 2026 18:44:04 +0200 Subject: [PATCH 3/4] test(terminal): pin all four buffer-load paths to the shared flush helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first version of this fix decided the flush policy in `selectSession` alone, and a later pass found it still covering one path of four. Nothing in the CI gate stops a fifth path, or an inlined `{ flushQueued: true }`, from splitting that policy up again — the browser suite that would notice is excluded from `npm test`. A static scan over `selectSession`, `_onSessionNeedsRefresh`, `_onSessionClearTerminal` and `_maybeRefetchFullHistory` asserts each one asks `_bufferLoadFinishOpts`, reusing the `methodBody` slice the sticky-scroll guard already needed. Verified by inlining the policy back into `_onSessionClearTerminal`, which fails it by name. Co-Authored-By: Claude Opus 5 (1M context) --- test/terminal-buffer-flush.test.ts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/terminal-buffer-flush.test.ts b/test/terminal-buffer-flush.test.ts index 36f652ef..9bc61771 100644 --- a/test/terminal-buffer-flush.test.ts +++ b/test/terminal-buffer-flush.test.ts @@ -339,6 +339,26 @@ describe('buffer-load flush (COD-144)', () => { } }); + it('every path that fetches a terminal buffer and writes it asks the shared helper', () => { + // Drift guard. The first version of this fix covered one of the four paths, + // and a later pass found it still covering one of four. Nothing else in the + // gate stops a fifth path, or an inlined `{ flushQueued: true }`, from + // splitting the policy up again; the browser suite that would notice does + // not run in CI. + const source = readFileSync(resolve(import.meta.dirname, '../src/web/public/app.js'), 'utf8'); + + for (const method of [ + 'selectSession', + '_onSessionNeedsRefresh', + '_onSessionClearTerminal', + '_maybeRefetchFullHistory', + ]) { + expect(methodBody(source, method), `${method} decides the flush policy itself`).toContain( + 'this._bufferLoadFinishOpts(' + ); + } + }); + it('empty queue + flushQueued is a no-op (no throw, no writes)', () => { const { app, writes } = makeApp(); const owner = app._beginBufferLoad('load-empty'); From 3730bc7df5279c073b23f82f2eb86f8913abe993 Mon Sep 17 00:00:00 2001 From: Michael Grundberg Date: Fri, 18 Sep 2026 18:56:15 +0200 Subject: [PATCH 4/4] docs(terminal): correct what selectSession does with the viewport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The JSDoc on `_syncStickyScrollBaseline` said `selectSession` deliberately ends at the bottom, so the baseline the replay samples is already true there. It does not. `selectSession` calls `scrollToBottom()` after the write and then ends at `scrollToLastNonEmptyLine()` (app.js:6512), which targets `lastNonEmptyLine - rows + 2` and therefore parks ABOVE `baseY` whenever the replayed frame keeps trailing blank rows — which a full capture does on purpose, since no transform that can delete a line may run over one. Its baseline really is a stale true. What covers it is the sticky snap itself: since de864e7d that snap fires only when the flush found the viewport already at the bottom (`preserveViewportY === null`), which a parked selectSession viewport is not. That commit landed on master after this branch was cut, so the guard arrives with the merge rather than being present here. `_onSessionClearTerminal` is unchanged in the comment and was correct: it resets and rewrites with no scroll afterwards, so it does end at the bottom. Comment only; no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) --- src/web/public/terminal-ui.js | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index b56f5510..9b56d0aa 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -3142,11 +3142,22 @@ Object.assign(CodemanApp.prototype, { * always true. A caller that then restores the reader's position would have * that restore undone by the next flush. * - * Every caller that scrolls the viewport somewhere other than the bottom - * after a load must call this, so the baseline describes the position the - * caller chose. `selectSession` and `_onSessionClearTerminal` deliberately - * end at the bottom, so for them the sampled true is already the truth and - * they do not call it. + * `_onSessionNeedsRefresh` and `_maybeRefetchFullHistory` restore a position + * and both call this, so their baseline describes the position they chose. + * + * The other two load paths do not call it, for different reasons. + * `_onSessionClearTerminal` resets and rewrites with no scroll afterwards, + * so the sampled true is already the truth there. `selectSession` does NOT + * end at the bottom, whatever its `scrollToBottom()` after the write + * suggests: it ends at `scrollToLastNonEmptyLine()`, which targets + * `lastNonEmptyLine - rows + 2` and therefore parks ABOVE `baseY` whenever + * the replayed frame keeps trailing blank rows, which a full capture does on + * purpose. Its baseline is a stale true. What decides whether that matters + * is the sticky snap in `flushPendingWrites`, and since de864e7d that snap + * fires only when the flush found the viewport already at the bottom + * (`preserveViewportY === null`), which a parked selectSession viewport is + * not. Do not read the absent call here as a claim that selectSession lands + * at the bottom. */ _syncStickyScrollBaseline() { this._wasAtBottomBeforeWrite = this.isTerminalAtBottom();