mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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.
|
||||
|
||||
+25
-12
@@ -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.
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user