From 46d8b9204969200bec058b1dc5e9d316c15310f8 Mon Sep 17 00:00:00 2001 From: timkjr Date: Sun, 20 Sep 2026 13:10:13 -0500 Subject: [PATCH] fix(split-pane): port Ctrl+Shift+C's never-falls-through guarantee to Pane B The smart-copy gate only entered its selection-check block behind hasSelection(), so a selection-less Ctrl+Shift+C skipped straight to `return true` and ceded the keystroke to the browser's own handling (e.g. Chrome's Inspect-Element binding) instead of matching Pane A's "never falls through" contract for that chord. Verified live in a real browser that this is a UX-parity fix, not an interrupt-safety one: xterm's evaluateKeyboardEvent never emits PTY data for a shifted ctrl-letter regardless of any gate (only "_" and "@" get special-cased), so no accidental 0x03 was ever at risk. The regression test added here asserts on the dispatched event's defaultPrevented rather than the absence of a WS frame, since the frame-count check passes vacuously for this exact key combo whether or not the gate fires. Co-Authored-By: Claude Sonnet 5 --- docs/architecture-invariants.md | 2 +- src/web/public/terminal-split.js | 56 +++++++++++++++--------- test/split-pane-terminal.browser.test.ts | 20 ++++++++- 3 files changed, 54 insertions(+), 24 deletions(-) diff --git a/docs/architecture-invariants.md b/docs/architecture-invariants.md index c152b6ef..085a4ada 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 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+V stays on xterm's own default paste, since Pane B has no image-paste trap to route it to; Ctrl+Shift+C (the explicit copy chord) is left un-ported as lower value. ⚠️ `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 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`. ### Gesture control: the setting diff --git a/src/web/public/terminal-split.js b/src/web/public/terminal-split.js index cf5e24fc..dbac9fa7 100644 --- a/src/web/public/terminal-split.js +++ b/src/web/public/terminal-split.js @@ -95,14 +95,11 @@ // action already did to Pane A (COD-153; mirrors the primary pane's own // gates at terminal-ui.js's attachCustomKeyEventHandler: command palette, // Alt+1-9/[/] tab nav, Alt+B sidebar toggle, Ctrl+Z suspend, Shift/Ctrl+Enter - // newline, and smart-copy Ctrl+C). Routed through the same registry-aware - // predicates so a rebind or a disable restores plain terminal behavior - // here too. Ctrl+V is deliberately left on xterm's own default - // (plain-text paste): Pane B has no image-paste trap to route it to, so - // intercepting it here would only break paste. Ctrl+Shift+C (the - // explicit, never-falls-through copy chord) is also left un-ported — - // lower value than the plain Ctrl+C case above, since Pane B is rarely - // the pane a user is actively selecting text in. + // newline, and smart-copy Ctrl+C/Ctrl+Shift+C). Routed through the same + // registry-aware predicates so a rebind or a disable restores plain + // terminal behavior here too. Ctrl+V is deliberately left on xterm's own + // default (plain-text paste): Pane B has no image-paste trap to route it + // to, so intercepting it here would only break paste. this.terminal.attachCustomKeyEventHandler((ev) => { if (ev.isComposing || ev.key === 'Process' || ev.keyCode === 229) return true; if ( @@ -158,17 +155,26 @@ } // Smart copy (mirrors terminal-ui.js's Ctrl+C gate, #211): with a // selection, Ctrl+C copies THIS pane's own selection instead of - // sending ^C; with none, it must fall through unchanged or the - // interrupt key is lost. Re-implemented against this.terminal rather - // than reusing app.copyTerminalSelection(), which reads app.terminal - // — Pane A's — and would copy the wrong pane's selection. - if ( - ev.type === 'keydown' && - global.app?.shouldCopyTerminalSelectionFromShortcut?.(ev) && - this.terminal?.hasSelection?.() - ) { - const raw = this.terminal.getSelection(); - const isColumnSelection = this.terminal._core?._selectionService?._activeSelectionMode === 3; + // sending ^C; with none, plain Ctrl+C must fall through unchanged or + // the interrupt key is lost. Ctrl+Shift+C is different: it is the + // explicit, never-falls-through copy chord, and the predicate above + // does not distinguish it from plain Ctrl+C — ev.shiftKey does, below. + // xterm's own evaluateKeyboardEvent routes a shifted ctrl-letter into + // a branch that assigns c.key only for a couple of special cases + // ("_"->US, "@"->NUL), neither of which is "c", so it emits NOTHING + // for Ctrl+Shift+C either way — this is not about an accidental + // interrupt byte reaching the PTY (verified live: it does not). + // Gating this whole block on hasSelection() (an earlier draft) meant + // that with no selection Ctrl+Shift+C skipped 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 + // attempt to copy, unlike Pane A, which always intercepts it. + // Re-implemented against this.terminal rather than reusing + // app.copyTerminalSelection(), which reads app.terminal — Pane A's — + // and would copy the wrong pane's selection. + if (ev.type === 'keydown' && global.app?.shouldCopyTerminalSelectionFromShortcut?.(ev)) { + const raw = this.terminal?.getSelection?.() || ''; + const isColumnSelection = this.terminal?._core?._selectionService?._activeSelectionMode === 3; const selection = isColumnSelection ? raw : (global.CodemanCopySelection?.clean?.(raw) ?? raw); if (selection.trim()) { ev.preventDefault(); @@ -181,8 +187,16 @@ // Nothing worth copying — clear for feedback (a padding-only // selection cleans to '' and this press still falls through to the // PTY as 0x03, matching the primary pane's own rule). - this.terminal.clearSelection?.(); - global.app.showToast?.('Nothing to copy', 'warning'); + if (this.terminal?.hasSelection?.()) { + this.terminal.clearSelection?.(); + global.app.showToast?.('Nothing to copy', 'warning'); + } + // Ctrl+Shift+C never falls through, even with nothing to copy — + // matches terminal-ui.js's own ev.shiftKey branch. + if (ev.shiftKey) { + ev.preventDefault(); + return false; + } } return true; }); diff --git a/test/split-pane-terminal.browser.test.ts b/test/split-pane-terminal.browser.test.ts index 4fe7c07a..bd5b211a 100644 --- a/test/split-pane-terminal.browser.test.ts +++ b/test/split-pane-terminal.browser.test.ts @@ -213,7 +213,9 @@ describe('SplitTerminalPane in a real browser', () => { // which is not what this test is isolating. const textarea = (pane.terminal as any)._core?.textarea || (pane.terminal as any).textarea; const fire = (init: KeyboardEventInit) => { - textarea.dispatchEvent(new KeyboardEvent('keydown', { bubbles: true, cancelable: true, ...init })); + const event = new KeyboardEvent('keydown', { bubbles: true, cancelable: true, ...init }); + textarea.dispatchEvent(event); + return event.defaultPrevented; }; // keyCode is what xterm's evaluateKeyboardEvent switches on to decide // whether to produce a data frame at all — at keyCode 0 (unset) it can @@ -262,6 +264,19 @@ describe('SplitTerminalPane in a real browser', () => { await new Promise((r) => setTimeout(r, 50)); app._copyText = realCopyText; + // Ctrl+Shift+C with NO selection: the blanket "no 'i' frames" check + // below is NOT what proves this gate works — xterm's own + // evaluateKeyboardEvent never emits data for a shifted ctrl-letter in + // the first place (verified live: removing the gate entirely still + // produces zero WS frames for this exact key), so an absent 'i' frame + // is true whether or not the app-level shiftKey branch fires. What the + // branch actually buys is `preventDefault()`, so the browser's own + // handling of the chord (e.g. Chrome's Inspect-Element binding) is + // pre-empted, mirroring Pane A's own "never falls through" contract — + // asserted directly via the dispatched event's defaultPrevented. + pane.terminal.clearSelection(); + const ctrlShiftCPrevented = fire({ key: 'c', code: 'KeyC', keyCode: 67, ctrlKey: true, shiftKey: true }); + // Alt+B only reaches shouldToggleSessionSidebarFromShortcut's gate when // the sidebar layout is actually active (app.js:4325) — under the // default header-strip layout the app doesn't treat Alt+B as its own @@ -286,12 +301,13 @@ describe('SplitTerminalPane in a real browser', () => { pane.destroy(); document.body.removeChild(mount); - return { sent, sendKeyCalls, copiedText }; + return { sent, sendKeyCalls, copiedText, ctrlShiftCPrevented }; }, sessionId); expect(result.sent.every((f) => JSON.parse(f).t !== 'i')).toBe(true); expect(result.sendKeyCalls).toEqual([{ key: 'S-Enter' }]); expect(result.copiedText).toContain('SPLITPANE_COPY_MARKER'); + expect(result.ctrlShiftCPrevented).toBe(true); await page.evaluate(async (id) => { await fetch(`/api/sessions/${id}`, { method: 'DELETE' });