From 36af183f97fb3d0cc480d339be7ae5a2348391b1 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Sun, 4 Oct 2026 23:34:46 +0200 Subject: [PATCH] fix(split-pane): leave the owed marker to a trailing refresh, and say what a pull's request phase holds (#524 review) - Stale second marker above a trailing refresh's replay: xterm parses write() on a later tick while clear() is synchronous, so a marker stamped in a load's finally, just before _endBufferLoad() starts the trailing refresh, landed in the freshly cleared buffer above that refresh's replay. _stampMarkerIfOwed() now returns early while a refresh is pending; that refresh re-owes the marker on a closed socket and writes the one copy below its own replay. Pinned by marker-count assertions on the two existing trailing-refresh tests plus a new async-parse fake (writes parsed on a later tick, clear() synchronous) for back-to-back refreshes and a pull with a queued refresh and a close mid-pull; all four fail without the guard. Also checked against a real @xterm/headless 6.0.0. - Marker withheld for up to the 45 s request budget: kept the behaviour and made the comment and the docs truthful. The pull's request phase holds no live output, but it holds the single-flight flag, so a coalesced {t:'r'} refresh and a close's owed marker wait for the response. Writing the marker at once during that phase would need a separate "awaiting response" state and, with a refresh pending, reopens the same write-vs-clear() race as above; a Codeman restart resets the in-flight request along with the socket, so that pull fails at once and stamps. - Stale comments: _onSocketClosed() now says the deferral covers any load, _writeDisconnectedMarker() points at _stampMarkerIfOwed(), and the pull's finally comment describes the hand-off to a trailing refresh. - Invariants doc: dropped "the initial load" from the loads a close can land in (connect() awaits it before creating the socket), reworded the "nested refresh stamps its own" sentence to describe the guard, and noted what the request phase holds. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/architecture-invariants.md | 2 +- src/web/public/terminal-split.js | 42 +++++++++++------ test/split-pane-terminal-unit.test.ts | 66 ++++++++++++++++++++++++++- 3 files changed, 92 insertions(+), 18 deletions(-) diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index 3f6af1a5..233e0fda 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -789,7 +789,7 @@ Further detail: with many sessions the horizontal strip stops being scannable, w ### Split-pane sessions -**Split-pane sessions** (`showSplitButton`, header button, default OFF, per-device like `showFileViewerButton` — not in `SettingsUpdateSchema`, `displayKeys` in settings-ui.js): shows two live sessions side-by-side in one Codeman window. Pane A is the untouched, existing singleton terminal (`this.terminal`/`this._ws` in terminal-ui.js); Pane B is a new, independent `SplitTerminalPane` (terminal-split.js) with its own xterm instance and its own `/ws/sessions/:id/terminal` WebSocket. ⚠️ **Pane B is deliberately plainer than Pane A** — no local-echo overlay, no CJK IME, no touch/mobile handlers, no keyboard accessory bar — since this is a desktop-only feature (a split view needs a wide viewport) and those features exist for mobile/touch input; `.btn-split` is hard-hidden below 1180px regardless of the setting by the `@media (max-width: 1179px)` rule in styles.css (mobile.css only carries a comment pointing at it: that file loads up to 1023px, so it cannot cover the 1024-1179px tablet range the feature also needs to stay off), and the per-device setting means turning it on at a desk can never sync it onto a phone in the first place. No persistence: closing the browser tab or reloading always returns to the normal single-pane view; there is no localStorage key for split state. ⚠️ Splitting a session against itself is disallowed (the picker excludes the active session), **as is splitting against a popped-out (detached) session** — `buildSplitPickerSessions()` excludes `detachedSessions` because a detached session's own window is already claiming its PTY size, and `MAX_WS_PER_SESSION` needs no change since Pane A/B are always two different sessions. ⚠️ Either pane's session ending (deleted locally or from another client) auto-collapses the split — Pane A's session ending promotes Pane B to the new single pane via `selectSession(id, { auto: true })` (an app-driven selection, so it must not spend the session's idle alert — see the Approvals Inbox note above), never by trying to hot-swap the lightweight `SplitTerminalPane` object into the primary singleton state. ⚠️ Pane B refits on every window/sidebar/tab-rail resize via the SAME trailing-edge `ResizeObserver` callback that resizes Pane A (`throttledResize` in terminal-ui.js) — it only ever measured Pane A's own container, so without an explicit `this._splitPane?.fit()` call there Pane B silently kept its stale PTY size through every resize that did not happen to be a divider drag. ⚠️ A dropped WebSocket leaves Pane B visibly dead (a message written into its own xterm buffer) rather than silently swallowing keystrokes with nothing on screen to explain why — there is no reconnect logic for v1, matching the "deliberately plainer than Pane A" design. Related but distinct: `detachSession()` already opens one session in a separate OS-level browser window (`isSoloWindow`) — that is prior art for "two sessions visible at once" but not for one window with a draggable in-page divider, which is what this feature adds. ⚠️ Pane B installs its own `attachCustomKeyEventHandler` gating the same app-level chords Pane A's own handler gates (command palette, Alt+1-9/[/] tab nav, Alt+B sidebar toggle, Ctrl+Z suspend, Shift/Ctrl+Enter newline, smart-copy Ctrl+C) — without it the document capture-phase handler's `preventDefault()` (which never stops xterm) let each chord ALSO write its raw byte/escape sequence into Pane B's live PTY on top of whatever the app action did to Pane A (COD-153). Ctrl+Z is swallowed unless Pane B's own session is `mode === 'shell'`, mirroring terminal-ui.js's reasoning: in a plain shell it is the user's own job-control tool, everywhere else it silently suspends an unattended agent loop. Shift/Ctrl+Enter POSTs to `/api/sessions/:id/send-key` (`{key:'S-Enter'|'C-Enter'}`, tmux `send-keys -H` for a real 0x0a, or the CLI's declared `capabilities.newline` chord) targeting THIS pane's own `sessionId` rather than the primary pane's `activeSessionId` — without it xterm's plain `\r` would submit an incomplete prompt instead of adding a line to it. Smart-copy Ctrl+C/Ctrl+Shift+C is re-implemented against `this.terminal` (Pane B's own) rather than reusing `app.copyTerminalSelection()`, which reads Pane A's terminal and would copy the wrong pane's selection; Ctrl+Shift+C never falls through even with nothing to copy, mirroring terminal-ui.js's own `ev.shiftKey` branch. ⚠️ **This is a UX-parity fix, not an interrupt-safety one** — verified live in a real browser: xterm's `evaluateKeyboardEvent` routes a shifted ctrl-letter into a branch that assigns `c.key` only for two special cases (`_`→US, `@`→NUL), so it emits no data for Ctrl+Shift+C at all regardless of any application gate; a synthetic keydown with the gate removed produces zero WS frames, proving no accidental interrupt reaches the PTY either way. What gating the whole copy block on `hasSelection()` (an earlier draft) actually cost: with no selection, a selection-less Ctrl+Shift+C fell straight to `return true`, silently ceding the keystroke to the BROWSER's own handling (e.g. Chrome's Inspect-Element binding) with no feedback and no copy attempt — Pane A always intercepts it. Ctrl+V stays on xterm's own default paste, since Pane B has no image-paste trap to route it to. ⚠️ `buildSplitPickerSessions()` also excludes any session with `pid === null` (an exited CLI, a crash-looped session whose breaker tripped, a restore that never re-attached): Pane B has no equivalent of `selectSession()`'s auto re-attach POST, so a pane opened onto one has nothing reading its tmux pane — no `terminal` events ever arrive, and `Session.write()` silently drops every keystroke with no ack either way, so the loss is invisible behind a socket that reports healthy. Pane B's input frames deliberately carry no `cid`/`seq` (`ws-routes.ts` supports that), matching the no-overlay/no-IME "deliberately plainer" list above, since it has no exactly-once delivery layer to key them against. ⚠️ A SHELL Pane B pulls scrollback itself when the wheel goes up at the top of its buffer (`_maybeLoadMoreHistory`/`_pullHistory`): tmux repaints a burst of output instead of scrolling it, so Pane B's own xterm holds about one screen of scrollback while tmux holds every line, and it loaded history exactly once at connect and never again. It is the same bounded pull as Pane A's (`?full=1&tail=TERMINAL_TAIL_SIZE`, no rewrite when the window holds no more rows than the pane already has or the pane is at its `scrollback + rows` cap, and a 60 s back-off instead of 4 s when that skipped window was truncated or the pane is full, since each ask costs the server a whole-history `capture-pane`), against Pane B's OWN terminal rather than `app.terminal`, so it cannot share `_maybeRefetchFullHistory`. The wheel listener is capture-phase because xterm `stopPropagation()`s the events it consumes; the alternate-screen skip (nano, vim, less) only matters for a direct-PTY shell, since under tmux the browser xterm never enters the alternate buffer; skipped too for a detached session (mirrors `_sendResize()`'s own check and app.js's `_maybeRefetchFullHistory`), since its own window already owns its PTY size and scrollback. Live frames arriving from the response onward, a `{t:'c'}` clear frame included, are held with their arrival time (`_liveQueue`, opened right after `await fetch(...)` beside `capturedAt`; a frame from before it is replaced by the capture or written unchanged, so the pane keeps painting during the round trip) and replayed in order only if they arrived after the capture (the response's arrival stands in for the capture instant, as in `_finishBufferLoad`, so a frame inside that one round trip can be lost or doubled); the request uses the primary pane's budget (`CodemanFetchDeadline.terminalFetchDeadlineMs({ full: true })`, 10 s only if that helper is absent) and the body read gets 10 s once the headers land, because from then on the pull holds the pane's live output. ⚠️ The "Pane B disconnected" marker must be the LAST thing on screen. A replay's own `\x1bc` would otherwise wipe a marker written before the pull and paint a fresh, current-looking history while `onData` keeps silently dropping every keystroke on the dead socket (a Codeman restart drops the socket while the tmux session, and so the HTTP pull, still succeeds), so `_pullHistory()` re-stamps it after the live-frame flush. A close DURING any load (a pull, a refresh, the initial load) writes nothing: `_onSocketClosed()` sets `_markerOwed` while `_bufferLoading` is true, since written there it would sit above the held frames the pull flushes after a skip, a downgrade, a failed fetch or the deadline, above a refresh's replay, or land mid-way through a chunked replay. Each load settles the marker in its OWN `finally` (`_stampMarkerIfOwed()`), after the queue flush and before a trailing refresh starts, so a nested refresh stamps its own and never an earlier one's onto its freshly cleared terminal. Anything that wipes the terminal on a closed socket (a replay's `\x1bc`, a refresh's `clear()`) sets `_markerOwed` too, so the marker is rewritten whether or not the close landed during the load. Tracked via `_wsClosed`/`_markerOwed` rather than routed through `_onLiveOutput()`, since a close landing before the response is stamped before the cutoff and would be dropped with the rest of the pre-capture queue. There is no "Load full history" banner in Pane B, so a shell history past that 1 MiB window stays out of reach there. Non-shell Pane B is unchanged: it already loads `full=1`, and its history is out of scope for this pull (codex and Claude's inline renderer do grow tmux history; this just isn't how they recover it). Design: `docs/split-pane-sessions-plan.md`. +**Split-pane sessions** (`showSplitButton`, header button, default OFF, per-device like `showFileViewerButton` — not in `SettingsUpdateSchema`, `displayKeys` in settings-ui.js): shows two live sessions side-by-side in one Codeman window. Pane A is the untouched, existing singleton terminal (`this.terminal`/`this._ws` in terminal-ui.js); Pane B is a new, independent `SplitTerminalPane` (terminal-split.js) with its own xterm instance and its own `/ws/sessions/:id/terminal` WebSocket. ⚠️ **Pane B is deliberately plainer than Pane A** — no local-echo overlay, no CJK IME, no touch/mobile handlers, no keyboard accessory bar — since this is a desktop-only feature (a split view needs a wide viewport) and those features exist for mobile/touch input; `.btn-split` is hard-hidden below 1180px regardless of the setting by the `@media (max-width: 1179px)` rule in styles.css (mobile.css only carries a comment pointing at it: that file loads up to 1023px, so it cannot cover the 1024-1179px tablet range the feature also needs to stay off), and the per-device setting means turning it on at a desk can never sync it onto a phone in the first place. No persistence: closing the browser tab or reloading always returns to the normal single-pane view; there is no localStorage key for split state. ⚠️ Splitting a session against itself is disallowed (the picker excludes the active session), **as is splitting against a popped-out (detached) session** — `buildSplitPickerSessions()` excludes `detachedSessions` because a detached session's own window is already claiming its PTY size, and `MAX_WS_PER_SESSION` needs no change since Pane A/B are always two different sessions. ⚠️ Either pane's session ending (deleted locally or from another client) auto-collapses the split — Pane A's session ending promotes Pane B to the new single pane via `selectSession(id, { auto: true })` (an app-driven selection, so it must not spend the session's idle alert — see the Approvals Inbox note above), never by trying to hot-swap the lightweight `SplitTerminalPane` object into the primary singleton state. ⚠️ Pane B refits on every window/sidebar/tab-rail resize via the SAME trailing-edge `ResizeObserver` callback that resizes Pane A (`throttledResize` in terminal-ui.js) — it only ever measured Pane A's own container, so without an explicit `this._splitPane?.fit()` call there Pane B silently kept its stale PTY size through every resize that did not happen to be a divider drag. ⚠️ A dropped WebSocket leaves Pane B visibly dead (a message written into its own xterm buffer) rather than silently swallowing keystrokes with nothing on screen to explain why — there is no reconnect logic for v1, matching the "deliberately plainer than Pane A" design. Related but distinct: `detachSession()` already opens one session in a separate OS-level browser window (`isSoloWindow`) — that is prior art for "two sessions visible at once" but not for one window with a draggable in-page divider, which is what this feature adds. ⚠️ Pane B installs its own `attachCustomKeyEventHandler` gating the same app-level chords Pane A's own handler gates (command palette, Alt+1-9/[/] tab nav, Alt+B sidebar toggle, Ctrl+Z suspend, Shift/Ctrl+Enter newline, smart-copy Ctrl+C) — without it the document capture-phase handler's `preventDefault()` (which never stops xterm) let each chord ALSO write its raw byte/escape sequence into Pane B's live PTY on top of whatever the app action did to Pane A (COD-153). Ctrl+Z is swallowed unless Pane B's own session is `mode === 'shell'`, mirroring terminal-ui.js's reasoning: in a plain shell it is the user's own job-control tool, everywhere else it silently suspends an unattended agent loop. Shift/Ctrl+Enter POSTs to `/api/sessions/:id/send-key` (`{key:'S-Enter'|'C-Enter'}`, tmux `send-keys -H` for a real 0x0a, or the CLI's declared `capabilities.newline` chord) targeting THIS pane's own `sessionId` rather than the primary pane's `activeSessionId` — without it xterm's plain `\r` would submit an incomplete prompt instead of adding a line to it. Smart-copy Ctrl+C/Ctrl+Shift+C is re-implemented against `this.terminal` (Pane B's own) rather than reusing `app.copyTerminalSelection()`, which reads Pane A's terminal and would copy the wrong pane's selection; Ctrl+Shift+C never falls through even with nothing to copy, mirroring terminal-ui.js's own `ev.shiftKey` branch. ⚠️ **This is a UX-parity fix, not an interrupt-safety one** — verified live in a real browser: xterm's `evaluateKeyboardEvent` routes a shifted ctrl-letter into a branch that assigns `c.key` only for two special cases (`_`→US, `@`→NUL), so it emits no data for Ctrl+Shift+C at all regardless of any application gate; a synthetic keydown with the gate removed produces zero WS frames, proving no accidental interrupt reaches the PTY either way. What gating the whole copy block on `hasSelection()` (an earlier draft) actually cost: with no selection, a selection-less Ctrl+Shift+C fell straight to `return true`, silently ceding the keystroke to the BROWSER's own handling (e.g. Chrome's Inspect-Element binding) with no feedback and no copy attempt — Pane A always intercepts it. Ctrl+V stays on xterm's own default paste, since Pane B has no image-paste trap to route it to. ⚠️ `buildSplitPickerSessions()` also excludes any session with `pid === null` (an exited CLI, a crash-looped session whose breaker tripped, a restore that never re-attached): Pane B has no equivalent of `selectSession()`'s auto re-attach POST, so a pane opened onto one has nothing reading its tmux pane — no `terminal` events ever arrive, and `Session.write()` silently drops every keystroke with no ack either way, so the loss is invisible behind a socket that reports healthy. Pane B's input frames deliberately carry no `cid`/`seq` (`ws-routes.ts` supports that), matching the no-overlay/no-IME "deliberately plainer" list above, since it has no exactly-once delivery layer to key them against. ⚠️ A SHELL Pane B pulls scrollback itself when the wheel goes up at the top of its buffer (`_maybeLoadMoreHistory`/`_pullHistory`): tmux repaints a burst of output instead of scrolling it, so Pane B's own xterm holds about one screen of scrollback while tmux holds every line, and it loaded history exactly once at connect and never again. It is the same bounded pull as Pane A's (`?full=1&tail=TERMINAL_TAIL_SIZE`, no rewrite when the window holds no more rows than the pane already has or the pane is at its `scrollback + rows` cap, and a 60 s back-off instead of 4 s when that skipped window was truncated or the pane is full, since each ask costs the server a whole-history `capture-pane`), against Pane B's OWN terminal rather than `app.terminal`, so it cannot share `_maybeRefetchFullHistory`. The wheel listener is capture-phase because xterm `stopPropagation()`s the events it consumes; the alternate-screen skip (nano, vim, less) only matters for a direct-PTY shell, since under tmux the browser xterm never enters the alternate buffer; skipped too for a detached session (mirrors `_sendResize()`'s own check and app.js's `_maybeRefetchFullHistory`), since its own window already owns its PTY size and scrollback. Live frames arriving from the response onward, a `{t:'c'}` clear frame included, are held with their arrival time (`_liveQueue`, opened right after `await fetch(...)` beside `capturedAt`; a frame from before it is replaced by the capture or written unchanged, so the pane keeps painting during the round trip) and replayed in order only if they arrived after the capture (the response's arrival stands in for the capture instant, as in `_finishBufferLoad`, so a frame inside that one round trip can be lost or doubled); the request uses the primary pane's budget (`CodemanFetchDeadline.terminalFetchDeadlineMs({ full: true })`, 45 s, 10 s only if that helper is absent) and the body read gets 10 s once the headers land, because from then on the pull holds the pane's live output. The request phase holds no live output, but it does hold the single-flight flag, so a coalesced `{t:'r'}` refresh and a marker owed by a close (below) wait for the response, at worst for that whole budget (accepted: a Codeman restart resets an in-flight request along with the socket, so that pull fails at once and stamps the marker). ⚠️ The "Pane B disconnected" marker must be the LAST thing on screen. A replay's own `\x1bc` would otherwise wipe a marker written before the pull and paint a fresh, current-looking history while `onData` keeps silently dropping every keystroke on the dead socket (a Codeman restart drops the socket while the tmux session, and so the HTTP pull, still succeeds), so `_pullHistory()` re-stamps it after the live-frame flush. A close DURING any load (a pull, a refresh; `connect()` awaits the initial load before it creates the socket, so no close lands in that one) writes nothing: `_onSocketClosed()` sets `_markerOwed` while `_bufferLoading` is true, since written there it would sit above the held frames the pull flushes after a skip, a downgrade, a failed fetch or the deadline, above a refresh's replay, or land mid-way through a chunked replay. Each load settles the marker in its OWN `finally` (`_stampMarkerIfOwed()`), after the queue flush, EXCEPT when a trailing refresh is pending: that refresh's `clear()` is synchronous while xterm parses a `write()` on a later tick, so a marker stamped just before it lands in the freshly cleared buffer ABOVE the refresh's replay (a second, stale copy; the default fake terminal in `test/split-pane-terminal-unit.test.ts` writes synchronously and cannot show it, so an async-parse fake there pins it). The marker stays owed instead, and the trailing refresh, which re-owes it on a closed socket anyway, writes the one copy below its own replay. Anything that wipes the terminal on a closed socket (a replay's `\x1bc`, a refresh's `clear()`) sets `_markerOwed` too, so the marker is rewritten whether or not the close landed during the load. Tracked via `_wsClosed`/`_markerOwed` rather than routed through `_onLiveOutput()`, since a close landing before the response is stamped before the cutoff and would be dropped with the rest of the pre-capture queue. There is no "Load full history" banner in Pane B, so a shell history past that 1 MiB window stays out of reach there. Non-shell Pane B is unchanged: it already loads `full=1`, and its history is out of scope for this pull (codex and Claude's inline renderer do grow tmux history; this just isn't how they recover it). Design: `docs/split-pane-sessions-plan.md`. ### Gesture control: the setting diff --git a/src/web/public/terminal-split.js b/src/web/public/terminal-split.js index 990411d3..7a8e96c2 100644 --- a/src/web/public/terminal-split.js +++ b/src/web/public/terminal-split.js @@ -307,10 +307,13 @@ } // The socket's close, split out of connect() so the tests can drive it. - // While a history pull is running the marker waits for the pull's finally - // block: written now, it would sit above the output the pull is still - // holding (flushed after it on a skip, a downgrade or a failed fetch) or - // land in the middle of a chunked replay. + // While any load runs (a history pull or a `{t:'r'}` refresh) the marker is + // only owed, and that load's finally block settles it (_stampMarkerIfOwed()): + // written now, it would sit above the output a pull is still holding (flushed + // after it on a skip, a downgrade or a failed fetch), above a refresh's + // replay, or in the middle of a chunked replay. A pull still waiting for its + // response holds the marker too, for as long as the request takes (up to its + // budget, see _pullHistory()). _onSocketClosed() { this._wsReady = false; this._wsClosed = true; @@ -320,16 +323,21 @@ // Settles a marker the pane owes: set when a close lands during a load (the // replay would otherwise sit below it) or when a load wipes the terminal on - // a closed socket. Called from each load's own finally, before a trailing - // refresh starts, so a nested refresh stamps its own. + // a closed socket. Called from each load's own finally, just before + // _endBufferLoad() starts any trailing refresh. _stampMarkerIfOwed() { + // A trailing refresh is about to clear() synchronously, while xterm parses + // a write() on a later tick: a marker written here would land in the + // freshly cleared buffer ABOVE that refresh's replay, a second, stale copy. + // The refresh re-owes the marker on a closed socket and stamps it itself. + if (this._bufferRefreshPending && !this._destroyed) return; const owed = this._markerOwed; this._markerOwed = false; if (owed && this._wsClosed && !this._destroyed) this._writeDisconnectedMarker(); } - // Extracted so both _onSocketClosed() and a history pull that ends on a - // closed socket can write it (see _pullHistory()'s finally block). + // Extracted so both _onSocketClosed() and a load that ends owing it on a + // closed socket can write it (see _stampMarkerIfOwed()). _writeDisconnectedMarker() { this.terminal?.write('\r\n\x1b[2m[Pane B disconnected — close and reopen the split to reconnect]\x1b[0m\r\n'); } @@ -452,12 +460,15 @@ let replayed = false; let capturedAt = 0; // Two budgets on one signal. The request itself gets the primary pane's - // (CodemanFetchDeadline, constants.js): nothing is held while it runs. Once - // the headers land live output IS held, so the body read gets the short one - // instead: a body that hangs would otherwise freeze the pane for the long - // budget. Aborting lands in the catch below, which releases the flag and - // the queue. AbortSignal.timeout() alone cannot be re-armed, hence the - // controller; without AbortController the pull simply has no deadline. + // (CodemanFetchDeadline, constants.js): live output is not held while it + // runs, but the single-flight flag is, so a coalesced `{t:'r'}` refresh and + // the marker owed by a close (_onSocketClosed()) both wait for it, at worst + // for that whole budget. Once the headers land live output IS held, so the + // body read gets the short one instead: a body that hangs would otherwise + // freeze the pane for the long budget. Aborting lands in the catch below, + // which releases the flag and the queue. AbortSignal.timeout() alone cannot + // be re-armed, hence the controller; without AbortController the pull + // simply has no deadline. const controller = global.AbortController ? new global.AbortController() : null; let abortTimer = null; const armDeadline = (ms) => { @@ -539,7 +550,8 @@ // it while a load runs), and a replay's own `\x1bc` (flagged above) wipes // one written before it, which would paint a fresh, current-looking // history while onData keeps silently dropping every keystroke on the - // dead socket. A trailing refresh (_endBufferLoad) settles its own. + // dead socket. With a trailing refresh pending (_endBufferLoad) the marker + // is left to that refresh, which writes it below its own replay. this._stampMarkerIfOwed(); this._endBufferLoad(); } diff --git a/test/split-pane-terminal-unit.test.ts b/test/split-pane-terminal-unit.test.ts index 0beb6334..b408283e 100644 --- a/test/split-pane-terminal-unit.test.ts +++ b/test/split-pane-terminal-unit.test.ts @@ -805,8 +805,11 @@ describe('SplitTerminalPane scroll-to-top history pull', () => { }); it('back-to-back refreshes on a closed socket leave exactly one marker, at the end', async () => { - // R1's finally runs the trailing refresh R2; each load settles its own - // marker, so R1 never stamps onto R2's freshly cleared terminal. + // R1's finally runs the trailing refresh R2, so R1 leaves the owed marker to + // R2 instead of stamping it: in real xterm R1's write would still be queued + // when R2's synchronous clear() runs, and would land above R2's replay. The + // write mock records every stamp whatever clear() does, so counting marker + // writes pins that R1 never stamps (the async-parse test below shows why). const pane = makePane('shell'); pane._wsClosed = true; const first = deferred>(); @@ -825,6 +828,7 @@ describe('SplitTerminalPane scroll-to-top history pull', () => { const writes = pane.terminal.write.mock.calls.map((c) => c[0]); expect(writes.at(-1)).toSatisfy(isMarker); expect(writes.lastIndexOf('second')).toBe(writes.length - 2); + expect(writes.filter(isMarker)).toHaveLength(1); }); it('the pull gives the request the long budget and the body read the short one', async () => { @@ -921,6 +925,64 @@ describe('SplitTerminalPane scroll-to-top history pull', () => { expect(writes).toContain('refreshed'); expect(isMarker(writes.at(-1))).toBe(true); expect(writes.lastIndexOf('refreshed')).toBeLessThan(writes.length - 1); + // The pull left the owed marker to the refresh rather than stamping it too. + expect(writes.filter(isMarker)).toHaveLength(1); + }); + + it.each([ + [ + 'back-to-back refreshes', + async (pane: PaneUnderTest) => { + pane._wsClosed = true; + fetchMock.mockResolvedValueOnce(jsonResponse('first')).mockResolvedValueOnce(jsonResponse('second')); + pane._refreshBuffer(); + pane._refreshBuffer(); // coalesced into the trailing re-run + }, + ], + [ + 'a pull with a queued refresh and a close mid-pull', + async (pane: PaneUnderTest) => { + const held = headersOnly(); + fetchMock.mockResolvedValueOnce(held.response).mockResolvedValueOnce(jsonResponse('second')); + void pane._pullHistory(); + await settle(); // the response landed: the queue is open + pane._refreshBuffer(); // coalesced into the trailing re-run + pane._onSocketClosed(); + held.release(rowsOf(30)); // no replay + }, + ], + ])('with xterm parsing writes on a later tick, %s leave one marker on screen, last', async (_label, drive) => { + // Real xterm queues write() and parses it on a later tick (WriteBuffer's + // setTimeout), while clear() rewrites the buffer at once. The default fake + // applies writes synchronously and so cannot show a marker overtaken by a + // trailing refresh's clear(): parsed after it, that marker sat above the + // refresh's replay as a second, stale copy. + const pane = makePane('shell'); + const screen: string[] = []; + const pending: Array<{ data: string; done?: () => void }> = []; + pane.terminal.write = vi.fn((data: string, done?: () => void) => { + if (pending.length === 0) { + setTimeout(() => { + for (const entry of pending.splice(0)) { + if (entry.data === '\x1bc') screen.length = 0; + else if (entry.data) screen.push(entry.data); + entry.done?.(); + } + }, 0); + } + pending.push({ data, done }); + }); + pane.terminal.clear = vi.fn(() => { + screen.length = 0; + }); + + await drive(pane); + for (let i = 0; i < 5; i++) await settle(); + + expect(pane._bufferLoading).toBe(false); + expect(screen.filter(isMarker)).toHaveLength(1); + expect(screen.at(-1)).toSatisfy(isMarker); + expect(screen.indexOf('second')).toBe(screen.length - 2); }); it('a refresh on an open socket does not stamp a marker', async () => {