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 <noreply@anthropic.com>
(cherry picked from commit dbd39aed015ae5ae5870aba398bf4b4ab5118e47)
This commit is contained in:
Codeman maintainer
2026-09-21 04:53:19 +02:00
parent d8e85285c9
commit 299a21d5f5
4 changed files with 616 additions and 31 deletions
+255 -2
View File
@@ -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<string>();
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<typeof fakeElement>;
/** A DOM element stub: class list, a listener registry, pointer-capture spies. */
function fakeElement() {
const listeners = new Map<string, Set<(e: unknown) => void>>();
return {
style: {} as Record<string, string>,
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<typeof vi.fn>;
};
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<string, { pid: number | null; name?: string }>;
detachedSessions: Set<string>;
};
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');
});
});
+234
View File
@@ -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<typeof vi.fn>;
clear: ReturnType<typeof vi.fn>;
dispose: ReturnType<typeof vi.fn>;
};
type FakeSocket = {
onopen: unknown;
onmessage: unknown;
onclose: unknown;
onerror: unknown;
close: ReturnType<typeof vi.fn>;
};
type PaneUnderTest = {
ws: FakeSocket | null;
terminal: FakeTerminal | null;
_destroyed: boolean;
_bufferLoading: boolean;
_bufferRefreshPending: boolean;
destroy(): void;
_loadBuffer(): Promise<void>;
_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<T>() {
let resolve!: (value: T) => void;
const promise = new Promise<T>((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<ReturnType<typeof jsonResponse>>();
const second = deferred<ReturnType<typeof jsonResponse>>();
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<ReturnType<typeof jsonResponse>>();
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');
});
});