From 3cdb4bf42ecb2028b9a2c5aaa2b181bac7343630 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Fri, 18 Sep 2026 21:38:40 +0200 Subject: [PATCH] docs(terminal): the merge-time notes promised on #436 The four edits the review said would be folded in at merge, none of them code: the changeset becomes one user-facing paragraph, since it is what CHANGELOG.md and the release notes print; the `_bufferLoadFinishOpts` comment now names the second contributor to the duplicate window (`captureActivePaneBuffer` is `execSync`, so anything painted into the pane before the server read it is in the capture and is broadcast after the reply) and says why a `history` payload keeps the pre-existing discard when its exposure is the same; the `_finishBufferLoad` doc block moves from above `_beginBufferLoad` onto the function it documents; and the test file's header describes both rules the file now pins instead of only COD-144. Co-Authored-By: Claude Fable 5.1 --- ...y-output-that-arrived-after-the-capture.md | 48 +------------------ src/web/public/app.js | 37 +++++++++----- src/web/public/terminal-ui.js | 39 +++++++++------ test/terminal-buffer-flush.test.ts | 39 +++++++++------ 4 files changed, 75 insertions(+), 88 deletions(-) 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 0cb24af0..023623ef 100644 --- a/.changeset/fix-replay-output-that-arrived-after-the-capture.md +++ b/.changeset/fix-replay-output-that-arrived-after-the-capture.md @@ -2,50 +2,4 @@ "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. - -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. +fix(terminal): keep the output a pane capture could not contain. Opening a session, a backpressure refresh, a clear-terminal reload and a full-history re-pull all load the screen from a tmux pane capture, and anything the CLI printed between that capture and the end of the load used to be dropped, so its next partial redraw landed on a frame the terminal had never seen: missing or garbled output right after a tab switch or a refresh, plainest in a shell session. Each load now replays exactly the output that arrived after the capture, through one shared rule for all four paths, and a refresh that restores your scroll position no longer snaps back to the bottom afterwards. diff --git a/src/web/public/app.js b/src/web/public/app.js index 88fb115d..bb40152d 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -1917,23 +1917,36 @@ class CodemanApp { * 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. + * the terminal never received. + * + * A `history` payload is the server's byte buffer alone: the direct-PTY + * fallback, or a mux pane whose capture came back empty. The route reads + * that buffer in the same synchronous tick it takes the capture, so it is + * current up to the route's own read and no further, which is the same + * exposure. It deliberately keeps the pre-existing discard all the same: + * both cases are rare, neither has been measured, and a duplicated Ink + * redraw is more visible than a few milliseconds of missing output. + * `capturedFromMux` below is the one line to widen if either turns out to + * matter. * * `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. * - * 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. + * What this cutoff does NOT cover, and there are two contributors. The + * server appends output to the byte buffer and emits it in the same tick, + * but BROADCASTS on a batch timer (8ms over WebSocket, 16 to 50ms over SSE), + * and the terminal route runs synchronously from `capture-pane` to its + * return, so a batch already pending when the capture ran leaves the server + * after the reply, arrives after `headersReceivedAt`, and is replayed + * although the capture holds it. Separately, `captureActivePaneBuffer` is + * `execSync`, which blocks the event loop for the whole capture: anything + * tmux had already painted into the pane that the server had not yet read + * from the attach PTY is in the capture too, is broadcast only after the + * reply, and replays the same way. The duplicate is one batch interval plus + * one capture 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. diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index 5c1af2e7..c767ad36 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -3924,6 +3924,30 @@ Object.assign(CodemanApp.prototype, { }); }, + /** + * Open a buffer load: live terminal events are queued from here until + * `_finishBufferLoad` decides what to do with them. Returns the load token the + * finish call must present; a stale token makes that call a no-op. + * + * @param {string} [owner] Reuse an existing token to re-enter the same load + * (see below); omit it to start a new one. + * @returns {string} The load token. + */ + _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; + if (!reentering) this._loadBufferQueue = []; + return loadOwner; + }, + /** * Complete a buffer load: unblock live SSE writes. * Called when chunkedTerminalWrite finishes (or is skipped for empty buffers). @@ -3960,21 +3984,6 @@ Object.assign(CodemanApp.prototype, { * 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; - if (!reentering) this._loadBufferQueue = []; - return loadOwner; - }, - _finishBufferLoad(owner, opts) { if (owner !== undefined && this._bufferLoadOwner !== owner) { return false; diff --git a/test/terminal-buffer-flush.test.ts b/test/terminal-buffer-flush.test.ts index 9bc61771..bacd2923 100644 --- a/test/terminal-buffer-flush.test.ts +++ b/test/terminal-buffer-flush.test.ts @@ -1,20 +1,31 @@ /** - * @fileoverview Regression tests for the buffer-load flush path (COD-144). + * @fileoverview Regression tests for the buffer-load flush path: what becomes of + * the live terminal events queued while a buffer load runs, once the load ends. * - * Bug: newly launched Shell sessions rendered BLANK until a tab-switch. The - * buffer-load path (`selectSession` → `_beginBufferLoad`/`_finishBufferLoad`) - * QUEUES live SSE terminal events while `_isLoadingBuffer` is true, then on - * completion DISCARDS the queue (`_loadBufferQueue = null`). That de-dup is - * correct for an established session (the fetched buffer already contains the - * queued output, so replaying it would duplicate Ink redraws). But for a - * brand-new shell the fetch resolves BEFORE the PTY emits its prompt — the - * fetched buffer is empty and the prompt arrives only as a queued event, which - * then gets discarded → blank terminal. + * Two rules, each from a real bug. * - * Fix: `_finishBufferLoad(owner, { flushQueued })` REPLAYS the queued events - * through `batchTerminalWrite()` (after `_isLoadingBuffer` is cleared, so they - * write through normally) ONLY when the load painted nothing. The default path - * (no opts) still discards, preserving de-dup for established sessions. + * COD-144: newly launched Shell sessions rendered BLANK until a tab-switch. The + * load path (`selectSession` → `_beginBufferLoad`/`_finishBufferLoad`) queues + * live events while `_isLoadingBuffer` is true and used to DISCARD the queue on + * completion. Right for a buffer built from the server's byte history (the + * queued output is already in it, so replaying it duplicates Ink redraws), + * wrong for a brand-new shell whose fetch resolves BEFORE the PTY emits its + * prompt: the prompt arrived only as a queued event and was thrown away. A + * caller that knows the load painted nothing passes `{ flushQueued: true }` + * and the queue is REPLAYED through `batchTerminalWrite()` after + * `_isLoadingBuffer` is cleared, so the events write through normally. + * + * #436: a tmux pane capture is current only as of the instant `capture-pane` + * ran, so everything the CLI printed between the capture and the end of the + * chunked write was queued and dropped, and its next partial redraw landed on + * a frame the terminal never received. 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 + * fetch-and-write paths take that policy from one helper, + * `_bufferLoadFinishOpts`, and a static scan below pins each of them to it, + * because the same fix had already been written into one path out of four, + * twice. A path that replays and then restores a scroll position re-takes the + * sticky-scroll baseline (`_syncStickyScrollBaseline`), pinned the same way. * * Loaded via `vm` with a stubbed context (no jsdom — jsdom is broken on this * box; see connection-indicator.test.ts). We extract the REAL