From 299a21d5f5811d52bafc2f91b996518bb6d1a257 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Mon, 21 Sep 2026 04:40:42 +0200 Subject: [PATCH] fix(split-pane): merge-time fixes for split-pane sessions (#453) The maintainer's promised merge-time fixes from the final review of #453: 1. closeSplitPane() tears down a divider drag still in progress, so a split that collapses mid-drag no longer leaves body.split-pane-resizing (the page-wide col-resize cursor and user-select lock) set until a reload. 2. openSplitPane() re-applies the picker's own exclusions (detached session, pid === null, no session record) for a row that went stale while the menu sat open, refusing silently like its neighbouring gates. 3. architecture-invariants: the hard-hide of .btn-split is the @media (max-width: 1179px) rule in styles.css, not mobile.css. 4. SplitTerminalPane.destroy() nulls onclose (and onerror) beside onopen and onmessage. 5. Picker rows drop the data-session-id attribute nothing read. 6. The Pane-A-ends branch collapses with skipPrimaryResize, so the closing resize is no longer aimed at the session the server just removed. 7. The {t:'r'} refresh path is single-flight across the fetch and the chunked write, coalescing a mid-replay refresh into one trailing re-run. Tests: split-pane-auto-collapse-unit gains the drag-teardown, exclusion and skip-resize cases; the new split-pane-terminal-unit covers destroy() and the refresh single-flight. All were run against the pre-fix module to confirm they fail there. Co-Authored-By: Claude Fable 5.1 (cherry picked from commit dbd39aed015ae5ae5870aba398bf4b4ab5118e47) --- docs/architecture-invariants.md | 2 +- src/web/public/terminal-split.js | 154 +++++++++--- test/split-pane-auto-collapse-unit.test.ts | 257 ++++++++++++++++++++- test/split-pane-terminal-unit.test.ts | 234 +++++++++++++++++++ 4 files changed, 616 insertions(+), 31 deletions(-) create mode 100644 test/split-pane-terminal-unit.test.ts diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index 40625548..b024875a 100644 --- a/docs/architecture-invariants.md +++ b/docs/architecture-invariants.md @@ -404,7 +404,7 @@ Anatomy: `.set-shell` → `.set-shell-head` (title + `.set-head-actions`) + `.se ### 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 on phones in mobile.css regardless of the setting, 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) 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. 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) 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. 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 dbac9fa7..e42a8b53 100644 --- a/src/web/public/terminal-split.js +++ b/src/web/public/terminal-split.js @@ -24,23 +24,33 @@ * created/destroyed pane has no equivalent of. */ function writeChunked(terminal, buffer, isDestroyed) { - if (!buffer) return; + if (!buffer) return Promise.resolve(); if (buffer.length <= TERMINAL_CHUNK_SIZE) { terminal.write(buffer); - return; + return Promise.resolve(); } - let offset = 0; - const writeNext = () => { - if (isDestroyed() || !terminal) return; - const chunk = buffer.slice(offset, offset + TERMINAL_CHUNK_SIZE); - offset += chunk.length; - terminal.write(chunk); - if (offset < buffer.length) { - if (typeof requestAnimationFrame === 'function') requestAnimationFrame(writeNext); - else setTimeout(writeNext, 16); - } - }; - writeNext(); + // Resolves once the LAST chunk is written (or the pane was destroyed + // mid-replay), so _loadBuffer() below can hold its single-flight flag + // across the whole replay rather than just the fetch that precedes it. + return new Promise((resolve) => { + let offset = 0; + const writeNext = () => { + if (isDestroyed() || !terminal) { + resolve(); + return; + } + const chunk = buffer.slice(offset, offset + TERMINAL_CHUNK_SIZE); + offset += chunk.length; + terminal.write(chunk); + if (offset < buffer.length) { + if (typeof requestAnimationFrame === 'function') requestAnimationFrame(writeNext); + else setTimeout(writeNext, 16); + } else { + resolve(); + } + }; + writeNext(); + }); } class SplitTerminalPane { @@ -59,6 +69,9 @@ this.ws = null; this._wsReady = false; this._destroyed = false; + // Single-flight state for _loadBuffer()/_refreshBuffer() below. + this._bufferLoading = false; + this._bufferRefreshPending = false; } async connect() { @@ -212,6 +225,8 @@ // pane. When Pane B's computed dimensions happened to already match // the session's last-known size, Session.resize() (session.ts) skips // the resize as a no-op, no repaint fires, and the pane stayed blank. + // The await covers the whole chunked replay, not just the fetch, so a + // live frame from the socket below can never land in the middle of it. await this._loadBuffer(); if (this._destroyed) return; @@ -236,8 +251,7 @@ // data was dropped). The primary pane routes this to // _onSessionNeedsRefresh (app.js:2990) — Pane B has its own // buffer loader for the same reason connect() does. - this.terminal.clear(); - void this._loadBuffer(); + this._refreshBuffer(); } } catch { /* Malformed frame — ignore, matches primary pane's tolerance. */ @@ -280,17 +294,46 @@ // get one full replay. `fetch` here goes through the global wrapper // (constants.js), which already prefixes CodemanBase — unlike the raw // WebSocket URL above, which does not. + // + // Single-flight: the flag is held across the fetch AND the chunked write + // (writeChunked resolves after its last chunk), so two replays can never + // interleave their chunks into one terminal. A second call while one is + // in flight is dropped here; _refreshBuffer() is the caller that queues + // a trailing re-run instead. async _loadBuffer() { + if (this._bufferLoading) return; + this._bufferLoading = true; try { const query = this.sessionMode === 'shell' ? `tail=${TERMINAL_TAIL_SIZE}` : 'full=1'; const res = await fetch(`/api/sessions/${this.sessionId}/terminal?${query}`); const payload = (await res.json())?.data ?? {}; if (payload.terminalBuffer && this.terminal) { - writeChunked(this.terminal, payload.terminalBuffer, () => this._destroyed); + await writeChunked(this.terminal, payload.terminalBuffer, () => this._destroyed); } } catch { /* Best-effort — live output still arrives once the socket connects. */ + } finally { + this._bufferLoading = false; } + if (this._bufferRefreshPending && !this._destroyed) { + this._bufferRefreshPending = false; + this._refreshBuffer(); + } + } + + // The `{t:'r'}` server-refresh path: clear, then replay. Two refresh + // frames in a row used to start two concurrent replays, each clearing + // the terminal under the other's chunked write. A refresh that arrives + // mid-replay is COALESCED into one trailing re-run rather than ignored: + // the in-flight fetch may predate the drop the new frame is reporting, + // and no further frame is coming to correct stale content. + _refreshBuffer() { + if (this._bufferLoading) { + this._bufferRefreshPending = true; + return; + } + this.terminal?.clear(); + void this._loadBuffer(); } // Local reflow only — no PTY resize frame. Split out so a divider drag @@ -330,6 +373,10 @@ if (this.ws) { this.ws.onopen = null; this.ws.onmessage = null; + // onclose fires asynchronously AFTER close(); without this it ran + // its "disconnected" write against a pane already torn down. + this.ws.onclose = null; + this.ws.onerror = null; this.ws.close(); this.ws = null; } @@ -410,7 +457,7 @@ Object.assign(CodemanApp.prototype, { // does exact-string lookup over text nodes, and a session // literally named e.g. "Sessions" would otherwise get translated // on zh-CN (see the .session-name skip on the pane header below). - `` + `` ) .join(''); } @@ -488,6 +535,17 @@ Object.assign(CodemanApp.prototype, { // session, each independently claiming PTY dimensions via its own `{t:'z',...}` // resize frame. Refuse before creating any DOM or SplitTerminalPane. if (sessionId === this.activeSessionId) return; + // The picker's own exclusions (buildSplitPickerSessions in constants.js), + // re-applied here: the menu can sit open while a listed session's CLI + // exits (pid → null) or gets popped out to its own window, and nothing + // re-runs the picker filter for a row that already rendered. Same + // outcome as the picker gives such a session (not offered, silently): + // one with no PTY has nothing reading its pane, so Pane B would show + // nothing and drop every keystroke behind a healthy-looking socket, and + // a detached session's own window already owns its PTY size. + const session = this.sessions.get(sessionId); + if (!session || session.pid === null) return; + if (this.detachedSessions?.has?.(sessionId)) return; if (this._splitPane) this.closeSplitPane(); const wrap = document.querySelector('.terminal-wrap'); @@ -501,7 +559,6 @@ Object.assign(CodemanApp.prototype, { const paneB = document.createElement('div'); paneB.className = 'terminal-pane-b'; - const session = this.sessions.get(sessionId); paneB.innerHTML = `
${escapeHtml(session?.name || 'Session')} @@ -540,8 +597,17 @@ Object.assign(CodemanApp.prototype, { this._updateSplitButtonState(true); }, - closeSplitPane() { + closeSplitPane(options = {}) { if (!this._splitPane) return; + // A split can collapse MID-DRAG (either session ending, the window + // narrowing past the gate, a click on Pane B's own tab). The drag's own + // onUp is what normally clears `body.split-pane-resizing` (a col-resize + // cursor plus user-select:none on EVERY element, styles.css), and it + // relied on pointer capture routing pointerup back to a divider this + // method detaches below, so a mid-drag collapse left the whole page + // locked in resize mode until a reload. Tear the drag down first. + this._splitDividerDragTeardown?.(); + this._splitDividerDragTeardown = null; this._splitPane.destroy(); this._splitPane = null; this._splitSessionId = null; @@ -556,7 +622,13 @@ Object.assign(CodemanApp.prototype, { container.remove(); if (this.fitAddon) this.fitAddon.fit(); - this.sendResize?.(this.activeSessionId, { force: true })?.catch?.(() => {}); + // The Pane-A-ends branch of the _onSessionDeleted wrapper below collapses the split + // while activeSessionId is still the id the server just removed, so a + // resize from here would be aimed at a session that no longer exists; + // the promoted session gets its own resize from selectSession(). + if (!options.skipPrimaryResize) { + this.sendResize?.(this.activeSessionId, { force: true })?.catch?.(() => {}); + } }, // A click on .btn-split does one of two things — open the picker, or @@ -578,6 +650,7 @@ Object.assign(CodemanApp.prototype, { let dragging = false; let dragRaf = null; let pendingClientX = null; + let capturedPointerId = null; // Local-only reflow (flexBasis + both panes' xterm fit, no PTY resize // frame). Coalesced to one call per animation frame below — a raw @@ -614,14 +687,25 @@ Object.assign(CodemanApp.prototype, { }); }; - const onUp = (e) => { + // Everything pointerdown ARMS, undone in one place: the body-level + // cursor/selection lock, the divider's dragging class, pointer capture, + // the move/up/cancel listeners and a queued reflow frame. Shared by onUp + // (a normal drag end) and by closeSplitPane(), via the teardown handle + // stored below, for a split that collapses mid-drag: the pointerup that + // would have run onUp is routed by pointer capture to a divider + // closeSplitPane() has detached, so it never arrives. Idempotent, since + // the teardown runs whether or not a drag is in progress. + const endDrag = () => { dragging = false; divider.classList.remove('dragging'); document.body.classList.remove('split-pane-resizing'); - try { - divider.releasePointerCapture(e.pointerId); - } catch { - /* Already released (pointercancel/lostpointercapture beat us here). */ + if (capturedPointerId !== null) { + try { + divider.releasePointerCapture(capturedPointerId); + } catch { + /* Already released (pointercancel/lostpointercapture beat us here). */ + } + capturedPointerId = null; } divider.removeEventListener('pointermove', onMove); divider.removeEventListener('pointerup', onUp); @@ -629,8 +713,16 @@ Object.assign(CodemanApp.prototype, { if (dragRaf) { cancelAnimationFrame(dragRaf); dragRaf = null; - applyDragPercent(pendingClientX); } + }; + + const onUp = () => { + // A reflow frame still queued at release carries the final pointer + // position; apply it once, synchronously, so the panes end where the + // pointer did rather than one frame short. + const hadQueuedFrame = dragRaf !== null; + endDrag(); + if (hadQueuedFrame) applyDragPercent(pendingClientX); // Send the real PTY resize exactly once here, at drag end, for BOTH // panes — never per-move (matching the codebase's established // trailing-edge debounce convention, see throttledResize in @@ -656,6 +748,7 @@ Object.assign(CodemanApp.prototype, { document.body.classList.add('split-pane-resizing'); try { divider.setPointerCapture(e.pointerId); + capturedPointerId = e.pointerId; } catch { /* Capture failed — the drag still works via the listeners below. */ } @@ -663,6 +756,7 @@ Object.assign(CodemanApp.prototype, { divider.addEventListener('pointerup', onUp); divider.addEventListener('pointercancel', onUp); }); + this._splitDividerDragTeardown = endDrag; }, }); @@ -678,7 +772,11 @@ CodemanApp.prototype._onSessionDeleted = function (data) { // acknowledgement rule in CLAUDE.md — only a human opening a session // acknowledges it). const promoted = this._splitSessionId; - this.closeSplitPane(); + // activeSessionId is still data.id here (the original handler below is + // what retires it), so closeSplitPane()'s closing resize would be aimed + // at the session the server just removed. Skip it; selectSession() sizes + // the promoted session itself. + this.closeSplitPane({ skipPrimaryResize: true }); // Closing Pane A's own tab (closeSession(), app.js) adds data.id to // _closingSessions BEFORE awaiting the delete, then owns the follow-up // selection itself once the delete lands — same race _onSessionDeleted's diff --git a/test/split-pane-auto-collapse-unit.test.ts b/test/split-pane-auto-collapse-unit.test.ts index 0ecf75ae..e5fe7c69 100644 --- a/test/split-pane-auto-collapse-unit.test.ts +++ b/test/split-pane-auto-collapse-unit.test.ts @@ -11,17 +11,56 @@ // session id is lost. This file pins that ordering plus the sibling branches // (Pane B ends, unrelated session ends) so a regression fails in the normal CI // gate rather than only in the browser suite nobody runs by default. +// +// The same vm harness also pins two merge-time fixes from the final review of +// #453 that no browser test reaches: closeSplitPane() tearing down a divider +// drag that is still in progress (the body-level `split-pane-resizing` lock +// otherwise outlived the split), and openSplitPane() re-applying the picker's +// own exclusions for a row that went stale while the menu sat open. import { readFileSync } from 'node:fs'; import { resolve } from 'node:path'; import vm from 'node:vm'; -import { describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +/** A class-list stub backed by a Set, enough for add/remove/contains. */ +function fakeClassList() { + const classes = new Set(); + return { + add: (c: string) => void classes.add(c), + remove: (c: string) => void classes.delete(c), + contains: (c: string) => classes.has(c), + }; +} + +/** + * The only `document` surface the methods under test touch: `body.classList` + * (the drag's cursor/selection lock) and `querySelector` (the split container + * and the header button, both absent here, which is the "already collapsed / + * no button rendered" path every early-return in closeSplitPane() takes). + * `querySelector` is reassignable per test so a test can plant a sentinel. + */ +const fakeDocument = { + body: { classList: fakeClassList() }, + querySelector: (_selector: string): unknown => null, + getElementById: (_id: string): unknown => null, +}; + +const rafCalls: Array<() => void> = []; +const cancelledRafs: number[] = []; function loadCodemanAppClass() { const dir = resolve(import.meta.dirname, '../src/web/public'); const terminalSplitSrc = readFileSync(resolve(dir, 'terminal-split.js'), 'utf8'); const context = vm.createContext({ console: { ...console, log: vi.fn(), warn: vi.fn(), error: vi.fn() }, - window: {}, + // innerWidth clears the desktop-only gate so openSplitPane() reaches the + // exclusions under test; SPLIT_PANE_MIN_WIDTH is the bare global + // constants.js would otherwise define. + window: { innerWidth: 1600 }, + SPLIT_PANE_MIN_WIDTH: 1180, + document: fakeDocument, + requestAnimationFrame: (fn: () => void) => rafCalls.push(fn), + cancelAnimationFrame: (id: number) => cancelledRafs.push(id), }); // A minimal fake CodemanApp — terminal-split.js only needs `_onSessionDeleted` // and `selectSession` to already exist on the prototype (it wraps both), and @@ -41,6 +80,9 @@ function loadCodemanAppClass() { return (context as { __CodemanApp: new () => unknown }).__CodemanApp as { prototype: { _onSessionDeleted: (this: unknown, data: { id: string }) => unknown; + closeSplitPane: (this: unknown, options?: { skipPrimaryResize?: boolean }) => unknown; + openSplitPane: (this: unknown, sessionId: string) => unknown; + _installSplitDividerDrag: (this: unknown, divider: unknown, wrap: unknown, paneB: unknown) => unknown; }; }; } @@ -86,6 +128,11 @@ describe('terminal-split.js _onSessionDeleted wrapper (I6)', () => { CodemanApp.prototype._onSessionDeleted.call(app, { id: 'session-a' }); expect(app.closeSplitPane).toHaveBeenCalledTimes(1); + // activeSessionId is still the deleted id while the split collapses (the + // original handler, called last, is what retires it), so the closing + // resize must be skipped or it targets a session the server already + // removed; selectSession() below sizes the promoted session itself. + expect(app.closeSplitPane).toHaveBeenCalledWith({ skipPrimaryResize: true }); // Pinned ordering: selectSession must receive the id _splitSessionId held // BEFORE closeSplitPane ran (which nulls it), not whatever it holds after. // { auto: true } because this is an app-driven promotion, not the user @@ -100,6 +147,9 @@ describe('terminal-split.js _onSessionDeleted wrapper (I6)', () => { CodemanApp.prototype._onSessionDeleted.call(app, { id: 'session-b' }); expect(app.closeSplitPane).toHaveBeenCalledTimes(1); + // Pane A's session is alive and stays active: the closing resize is + // wanted here, so no skip option must be passed. + expect(app.closeSplitPane).toHaveBeenCalledWith(); expect(app.selectSession).not.toHaveBeenCalled(); expect(app.__originalDeletedCalls).toEqual([{ id: 'session-b' }]); }); @@ -146,3 +196,206 @@ describe('terminal-split.js _onSessionDeleted wrapper (I6)', () => { expect(app.__originalDeletedCalls).toEqual([{ id: 'session-a' }]); }); }); + +// ── closeSplitPane() vs a divider drag in progress ────────────────────────── + +type DragEl = ReturnType; + +/** A DOM element stub: class list, a listener registry, pointer-capture spies. */ +function fakeElement() { + const listeners = new Map void>>(); + return { + style: {} as Record, + classList: fakeClassList(), + parentElement: null as unknown, + listeners, + addEventListener(type: string, fn: (e: unknown) => void) { + if (!listeners.has(type)) listeners.set(type, new Set()); + listeners.get(type)!.add(fn); + }, + removeEventListener(type: string, fn: (e: unknown) => void) { + listeners.get(type)?.delete(fn); + }, + setPointerCapture: vi.fn(), + releasePointerCapture: vi.fn(), + }; +} + +function listenerCount(el: DragEl, type: string): number { + return el.listeners.get(type)?.size ?? 0; +} + +type DragTestApp = TestApp & { + _splitDividerDragTeardown?: (() => void) | null; + sendResize: ReturnType; +}; + +describe('closeSplitPane() tears down a divider drag that is still in progress', () => { + afterEach(() => { + fakeDocument.querySelector = () => null; + fakeDocument.body.classList.remove('split-pane-resizing'); + }); + + /** + * A split-active app with the REAL _installSplitDividerDrag() wired to stub + * elements, then armed the way the browser arms it: a primary-button + * pointerdown on the divider. `document.querySelector` hands closeSplitPane() + * a stub container so it runs its full body (reparent, remove, resize) rather + * than the already-collapsed early return. + */ + function makeDraggingApp() { + const app = Object.create((CodemanApp as { prototype: object }).prototype) as DragTestApp; + app.activeSessionId = 'session-a'; + app._splitSessionId = 'session-b'; + app._splitPane = { destroy: vi.fn() }; + app._closingSessions = new Set(); + app.sendResize = vi.fn(() => Promise.resolve(true)); + const divider = fakeElement(); + const wrap = fakeElement(); + const paneB = fakeElement(); + const container = { querySelector: () => wrap, parentElement: { insertBefore: vi.fn() }, remove: vi.fn() }; + fakeDocument.querySelector = (selector: string) => (selector === '.terminal-split-container' ? container : null); + + CodemanApp.prototype._installSplitDividerDrag.call(app, divider, wrap, paneB); + const [onDown] = divider.listeners.get('pointerdown')!; + onDown({ button: 0, pointerId: 7, preventDefault: vi.fn() }); + return { app, divider, container }; + } + + it('precondition: pointerdown arms the page-wide resize lock, capture and the drag listeners', () => { + const { divider } = makeDraggingApp(); + + expect(fakeDocument.body.classList.contains('split-pane-resizing')).toBe(true); + expect(divider.classList.contains('dragging')).toBe(true); + expect(divider.setPointerCapture).toHaveBeenCalledWith(7); + for (const type of ['pointermove', 'pointerup', 'pointercancel']) { + expect(listenerCount(divider, type), type).toBe(1); + } + }); + + it('clears body.split-pane-resizing, releases capture and drops the drag listeners', () => { + const { app, divider, container } = makeDraggingApp(); + + CodemanApp.prototype.closeSplitPane.call(app); + + // The lock is `cursor: col-resize; user-select: none` on EVERY element + // (styles.css); left set, it outlives the split until a reload. + expect(fakeDocument.body.classList.contains('split-pane-resizing')).toBe(false); + expect(divider.classList.contains('dragging')).toBe(false); + expect(divider.releasePointerCapture).toHaveBeenCalledWith(7); + for (const type of ['pointermove', 'pointerup', 'pointercancel']) { + expect(listenerCount(divider, type), type).toBe(0); + } + expect(app._splitDividerDragTeardown).toBeNull(); + expect(app._splitPane).toBeNull(); + expect(container.remove).toHaveBeenCalledTimes(1); + // An ordinary close still resizes the (live) primary pane's session. + expect(app.sendResize).toHaveBeenCalledWith('session-a', { force: true }); + }); + + it('cancels a reflow frame the drag had queued', () => { + const { app, divider } = makeDraggingApp(); + const [onMove] = divider.listeners.get('pointermove')!; + onMove({ clientX: 400 }); + const queuedRafId = rafCalls.length; + + CodemanApp.prototype.closeSplitPane.call(app); + + expect(cancelledRafs).toContain(queuedRafId); + }); + + it('closeSplitPane({ skipPrimaryResize: true }) collapses without resizing the primary session', () => { + const { app, container } = makeDraggingApp(); + + CodemanApp.prototype.closeSplitPane.call(app, { skipPrimaryResize: true }); + + expect(app.sendResize).not.toHaveBeenCalled(); + expect(app._splitPane).toBeNull(); + expect(container.remove).toHaveBeenCalledTimes(1); + expect(fakeDocument.body.classList.contains('split-pane-resizing')).toBe(false); + }); +}); + +// ── openSplitPane() re-applies the picker's exclusions ────────────────────── + +type OpenTestApp = TestApp & { + activeWebviewId: string | null; + sessions: Map; + detachedSessions: Set; +}; + +describe('openSplitPane() re-applies the picker exclusions at open time', () => { + // buildSplitPickerSessions() (constants.js) never lists a detached session + // or one with `pid === null`, but the menu can sit open while a listed + // session's CLI exits or gets popped out, and the row's click carries only + // the id. openSplitPane() must refuse those the same way the picker would + // have (silently, like its neighbouring gates) BEFORE touching the DOM. + const DOM_REACHED = 'sentinel: openSplitPane reached the DOM stage'; + + function makeApp(): OpenTestApp { + const app = Object.create((CodemanApp as { prototype: object }).prototype) as OpenTestApp; + app.activeSessionId = 'session-a'; + app.activeWebviewId = null; + app._splitPane = null; + app._splitSessionId = null; + app._closingSessions = new Set(); + app.closeSplitPane = vi.fn(); + app.selectSession = vi.fn(); + app.sessions = new Map([ + ['session-a', { pid: 101, name: 'w1-active' }], + ['session-exited', { pid: null, name: 'w2-exited' }], + ['session-detached', { pid: 103, name: 'w3-detached' }], + ['session-ok', { pid: 104, name: 'w4-ok' }], + ]); + app.detachedSessions = new Set(['session-detached']); + return app; + } + + beforeEach(() => { + fakeDocument.querySelector = vi.fn(() => { + throw new Error(DOM_REACHED); + }); + }); + afterEach(() => { + fakeDocument.querySelector = () => null; + }); + + it('control: a listed, attached session gets past every guard to the DOM stage', () => { + const app = makeApp(); + expect(() => CodemanApp.prototype.openSplitPane.call(app, 'session-ok')).toThrow(DOM_REACHED); + expect(fakeDocument.querySelector).toHaveBeenCalledWith('.terminal-wrap'); + }); + + it('refuses a session whose CLI has exited (pid === null) without touching the DOM', () => { + const app = makeApp(); + expect(CodemanApp.prototype.openSplitPane.call(app, 'session-exited')).toBeUndefined(); + expect(fakeDocument.querySelector).not.toHaveBeenCalled(); + expect(app._splitPane).toBeNull(); + }); + + it('refuses a detached (popped-out) session', () => { + const app = makeApp(); + expect(CodemanApp.prototype.openSplitPane.call(app, 'session-detached')).toBeUndefined(); + expect(fakeDocument.querySelector).not.toHaveBeenCalled(); + expect(app._splitPane).toBeNull(); + }); + + it('refuses an id with no session record at all', () => { + const app = makeApp(); + expect(CodemanApp.prototype.openSplitPane.call(app, 'session-ghost')).toBeUndefined(); + expect(fakeDocument.querySelector).not.toHaveBeenCalled(); + expect(app._splitPane).toBeNull(); + }); + + it('a refusal leaves an already-open split alone', () => { + const app = makeApp(); + app._splitPane = { destroy: vi.fn() }; + app._splitSessionId = 'session-ok'; + + CodemanApp.prototype.openSplitPane.call(app, 'session-exited'); + + expect(app.closeSplitPane).not.toHaveBeenCalled(); + expect(app._splitPane).not.toBeNull(); + expect(app._splitSessionId).toBe('session-ok'); + }); +}); diff --git a/test/split-pane-terminal-unit.test.ts b/test/split-pane-terminal-unit.test.ts new file mode 100644 index 00000000..8ce88461 --- /dev/null +++ b/test/split-pane-terminal-unit.test.ts @@ -0,0 +1,234 @@ +// test/split-pane-terminal-unit.test.ts +// Port: N/A (no server/browser; SplitTerminalPane is loaded via `vm`, like +// split-pane-auto-collapse-unit.test.ts loads the CodemanApp patches). +// +// Unit coverage for the two SplitTerminalPane (terminal-split.js) fixes from +// the final review of #453 that need no browser: destroy() nulling EVERY socket +// handler (onclose used to survive it and fire its "disconnected" write into a +// pane already torn down), and the `{t:'r'}` server-refresh path being +// single-flight. Two refresh frames in a row used to start two concurrent +// replays, each clearing the terminal under the other's chunked write; a +// refresh arriving mid-replay is now coalesced into ONE trailing re-run rather +// than dropped, because the in-flight fetch may predate the drop the new frame +// reports and no further frame comes to correct stale content. +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import vm from 'node:vm'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const TERMINAL_CHUNK_SIZE = 32 * 1024; + +type FakeTerminal = { + write: ReturnType; + clear: ReturnType; + dispose: ReturnType; +}; +type FakeSocket = { + onopen: unknown; + onmessage: unknown; + onclose: unknown; + onerror: unknown; + close: ReturnType; +}; +type PaneUnderTest = { + ws: FakeSocket | null; + terminal: FakeTerminal | null; + _destroyed: boolean; + _bufferLoading: boolean; + _bufferRefreshPending: boolean; + destroy(): void; + _loadBuffer(): Promise; + _refreshBuffer(): void; +}; + +const fetchMock = vi.fn(); +/** requestAnimationFrame stand-in: chunked writes queue here and are drained by hand. */ +const rafQueue: Array<() => void> = []; + +function loadSplitTerminalPane() { + const dir = resolve(import.meta.dirname, '../src/web/public'); + const src = readFileSync(resolve(dir, 'terminal-split.js'), 'utf8'); + const context = vm.createContext({ + console: { ...console, log: vi.fn(), warn: vi.fn(), error: vi.fn() }, + window: {}, + fetch: (...args: unknown[]) => fetchMock(...args), + requestAnimationFrame: (fn: () => void) => rafQueue.push(fn), + // The constants.js globals the module reads at call time. + TERMINAL_CHUNK_SIZE, + TERMINAL_TAIL_SIZE: 1024 * 1024, + }); + // The module's tail patches CodemanApp.prototype; nothing on it runs here. + vm.runInContext(`class CodemanApp { _onSessionDeleted() {} selectSession() {} }\n${src}`, context); + return (context.window as { SplitTerminalPane: new (id: string, mount: unknown, opts?: object) => PaneUnderTest }) + .SplitTerminalPane; +} + +const SplitTerminalPane = loadSplitTerminalPane(); + +function makePane(mode = 'claude'): PaneUnderTest & { terminal: FakeTerminal } { + const pane = new SplitTerminalPane('s1', {}, { mode }); + pane.terminal = { write: vi.fn(), clear: vi.fn(), dispose: vi.fn() }; + return pane as PaneUnderTest & { terminal: FakeTerminal }; +} + +function jsonResponse(terminalBuffer: string) { + return { json: async () => ({ data: { terminalBuffer } }) }; +} + +function deferred() { + let resolve!: (value: T) => void; + const promise = new Promise((r) => { + resolve = r; + }); + return { promise, resolve }; +} + +/** Lets every microtask the vm-side promise chain queued run. */ +const settle = () => new Promise((r) => setTimeout(r, 0)); + +beforeEach(() => { + fetchMock.mockReset(); + rafQueue.length = 0; +}); + +describe('SplitTerminalPane.destroy()', () => { + it('nulls every WebSocket handler, onclose included, before closing the socket', () => { + const pane = makePane(); + const terminal = pane.terminal; + const ws: FakeSocket = { onopen: vi.fn(), onmessage: vi.fn(), onclose: vi.fn(), onerror: vi.fn(), close: vi.fn() }; + pane.ws = ws; + + pane.destroy(); + + // close() fires onclose asynchronously, so a handler left attached ran its + // "disconnected" write against a pane whose terminal was already disposed. + expect(ws.onopen).toBeNull(); + expect(ws.onmessage).toBeNull(); + expect(ws.onclose).toBeNull(); + expect(ws.onerror).toBeNull(); + expect(ws.close).toHaveBeenCalledTimes(1); + expect(pane.ws).toBeNull(); + expect(terminal.dispose).toHaveBeenCalledTimes(1); + expect(pane.terminal).toBeNull(); + expect(pane._destroyed).toBe(true); + }); +}); + +describe('SplitTerminalPane server-refresh single-flight', () => { + it('a refresh with nothing in flight clears and fetches straight away', async () => { + const pane = makePane(); + fetchMock.mockResolvedValueOnce(jsonResponse('one')); + + pane._refreshBuffer(); + await settle(); + + expect(pane.terminal.clear).toHaveBeenCalledTimes(1); + expect(fetchMock).toHaveBeenCalledWith('/api/sessions/s1/terminal?full=1'); + expect(pane.terminal.write).toHaveBeenCalledWith('one'); + expect(pane._bufferLoading).toBe(false); + }); + + it('a shell pane asks for the bounded tail, matching connect()', async () => { + const pane = makePane('shell'); + fetchMock.mockResolvedValueOnce(jsonResponse('tail')); + + pane._refreshBuffer(); + await settle(); + + expect(fetchMock).toHaveBeenCalledWith(`/api/sessions/s1/terminal?tail=${1024 * 1024}`); + }); + + it('refreshes arriving mid-fetch neither clear nor fetch again, and run ONCE after the replay lands', async () => { + const pane = makePane(); + const first = deferred>(); + const second = deferred>(); + fetchMock.mockReturnValueOnce(first.promise).mockReturnValueOnce(second.promise); + + pane._refreshBuffer(); + expect(pane.terminal.clear).toHaveBeenCalledTimes(1); + expect(fetchMock).toHaveBeenCalledTimes(1); + + // Two more frames while the first replay is still in flight. + pane._refreshBuffer(); + pane._refreshBuffer(); + expect(pane.terminal.clear).toHaveBeenCalledTimes(1); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(pane._bufferRefreshPending).toBe(true); + + first.resolve(jsonResponse('replay-1')); + await settle(); + + expect(pane.terminal.write).toHaveBeenCalledWith('replay-1'); + // Exactly one trailing re-run for the two coalesced frames, not two. + expect(pane.terminal.clear).toHaveBeenCalledTimes(2); + expect(fetchMock).toHaveBeenCalledTimes(2); + + second.resolve(jsonResponse('replay-2')); + await settle(); + + expect(pane.terminal.write).toHaveBeenLastCalledWith('replay-2'); + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(pane._bufferLoading).toBe(false); + expect(pane._bufferRefreshPending).toBe(false); + }); + + it('holds the flag across the chunked write, not just the fetch', async () => { + const pane = makePane(); + // Three chunks: two full ones plus a tail, so the last two are queued on + // requestAnimationFrame and the replay is mid-write after the fetch lands. + const big = 'x'.repeat(TERMINAL_CHUNK_SIZE * 2 + 5); + fetchMock.mockResolvedValueOnce(jsonResponse(big)); + + pane._refreshBuffer(); + await settle(); + expect(pane.terminal.write).toHaveBeenCalledTimes(1); + expect(rafQueue).toHaveLength(1); + expect(pane._bufferLoading).toBe(true); + + // A refresh mid-write must not clear the terminal under the chunks still + // to come, nor start a second fetch. + pane._refreshBuffer(); + expect(pane.terminal.clear).toHaveBeenCalledTimes(1); + expect(fetchMock).toHaveBeenCalledTimes(1); + + fetchMock.mockResolvedValueOnce(jsonResponse('after')); + rafQueue.shift()!(); + rafQueue.shift()!(); + await settle(); + + expect(pane.terminal.write).toHaveBeenCalledTimes(4); + expect(pane.terminal.write).toHaveBeenLastCalledWith('after'); + expect(pane.terminal.clear).toHaveBeenCalledTimes(2); + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(pane._bufferLoading).toBe(false); + }); + + it('a pending refresh is dropped once the pane is destroyed', async () => { + const pane = makePane(); + const first = deferred>(); + fetchMock.mockReturnValueOnce(first.promise); + + pane._refreshBuffer(); + pane._refreshBuffer(); + pane.destroy(); + first.resolve(jsonResponse('late')); + await settle(); + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(pane._bufferLoading).toBe(false); + }); + + it('a failed fetch releases the flag so the next refresh can run', async () => { + const pane = makePane(); + fetchMock.mockRejectedValueOnce(new Error('offline')); + + pane._refreshBuffer(); + await settle(); + expect(pane._bufferLoading).toBe(false); + + fetchMock.mockResolvedValueOnce(jsonResponse('back')); + pane._refreshBuffer(); + await settle(); + expect(pane.terminal.write).toHaveBeenCalledWith('back'); + }); +});