From fafef0aa003e947917e216b62160100601357c36 Mon Sep 17 00:00:00 2001 From: timkjr Date: Sat, 19 Sep 2026 15:53:23 -0500 Subject: [PATCH] fix(split-pane): gate app-level chords out of Pane B, address Ark0N's third pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pane B had no attachCustomKeyEventHandler of its own, so the document capture-phase shortcut handler's preventDefault() (which does not stop xterm) left Ctrl+K/Alt+1/Alt+B ALSO writing their raw byte/escape sequence into Pane B's live PTY on top of whatever the app action did to Pane A. Pane B now installs the same registry-aware gates the primary pane's own attachCustomKeyEventHandler uses. Ctrl+V is left on xterm's default paste — no image-paste trap to route it to. Plus the rest of the review's smaller items: - Narrowing the window past the desktop gate now closes an open split instead of leaving it stranded on screen. - Split is refused while a web tab is active (activeWebviewId), which used to open Pane B's socket behind a hidden container. - Pane B now handles the server's `{t:'r'}` refresh frame via a shared _loadBuffer() helper (also used by connect()), instead of ignoring it. - The divider drag now uses pointer events + setPointerCapture (mirrors tab-rail-resize.js), a button!==0 guard, preventDefault, and a body.split-pane-resizing cursor/selection lock — a plain mousedown drag selected the text under the cursor as it crossed both terminals. - Pane B's close control and the picker rows are real ` ) .join(''); } @@ -340,6 +393,11 @@ Object.assign(CodemanApp.prototype, { // the home screen still created the container and connected Pane B, just // behind the opaque overlay with nothing visible to show for it. if (!this.activeSessionId) return; + // A web tab hides `.terminal-wrap`'s container via CSS with nothing + // gating the button itself, and `activeSessionId` survives openWebview() + // — without this, picking a session opens Pane B's socket behind a + // hidden container with nothing on screen to show for it. + if (this.activeWebviewId) return; // A stale picker click (opened before switching tabs) or clicking Pane // B's own session tab while split can otherwise land here with // sessionId === activeSessionId: two live WebSockets to the same @@ -363,7 +421,7 @@ Object.assign(CodemanApp.prototype, { paneB.innerHTML = `
${escapeHtml(session?.name || 'Session')} - × +
`; @@ -472,11 +530,18 @@ Object.assign(CodemanApp.prototype, { }); }; - const onUp = () => { + const onUp = (e) => { dragging = false; divider.classList.remove('dragging'); - document.removeEventListener('mousemove', onMove); - document.removeEventListener('mouseup', onUp); + document.body.classList.remove('split-pane-resizing'); + try { + divider.releasePointerCapture(e.pointerId); + } catch { + /* Already released (pointercancel/lostpointercapture beat us here). */ + } + divider.removeEventListener('pointermove', onMove); + divider.removeEventListener('pointerup', onUp); + divider.removeEventListener('pointercancel', onUp); if (dragRaf) { cancelAnimationFrame(dragRaf); dragRaf = null; @@ -492,11 +557,27 @@ Object.assign(CodemanApp.prototype, { this._splitPane?.fit(); }; - divider.addEventListener('mousedown', () => { + // Pointer events + setPointerCapture (mirrors tab-rail-resize.js) instead + // of mousedown/document-level mousemove: a plain mousedown drag selects + // the text under the cursor as it crosses both terminals, and pointer + // capture routes move/up straight to `divider` regardless of what's under + // the cursor mid-drag, so no document-level listener leak is possible if + // the pointer is released off-window. `body.split-pane-resizing` (mirrors + // `body.tab-rail-resizing`) locks the cursor/selection for the drag. + divider.addEventListener('pointerdown', (e) => { + if (e.button !== 0) return; + e.preventDefault(); dragging = true; divider.classList.add('dragging'); - document.addEventListener('mousemove', onMove); - document.addEventListener('mouseup', onUp); + document.body.classList.add('split-pane-resizing'); + try { + divider.setPointerCapture(e.pointerId); + } catch { + /* Capture failed — the drag still works via the listeners below. */ + } + divider.addEventListener('pointermove', onMove); + divider.addEventListener('pointerup', onUp); + divider.addEventListener('pointercancel', onUp); }); }, }); diff --git a/test/app-settings-structure.test.ts b/test/app-settings-structure.test.ts index cdbbe1aa..813f3610 100644 --- a/test/app-settings-structure.test.ts +++ b/test/app-settings-structure.test.ts @@ -99,7 +99,10 @@ describe('App Settings modal structure', () => { for (const [, attrs, body] of previewed) { const kind = attrs.match(/data-preview="([a-z]+)"/)?.[1]; expect(['header', 'panel', 'toolbar', 'float']).toContain(kind); - expect(attrs, `chip ${body} needs a preview order`).toMatch(/data-preview-order="\d+"/); + // A decimal (e.g. "11.5") is allowed — Split sits between Multi-monitor + // (11) and Ultracode Agents (12) in the real header, and Number() + // parses it fine for the preview's own sort. + expect(attrs, `chip ${body} needs a preview order`).toMatch(/data-preview-order="\d+(\.\d+)?"/); // A text token replaces the icon for readouts (plan usage, CPU, font size). const hasIcon = body.includes('class="set-chip-ico') || attrs.includes('data-preview-text='); expect(hasIcon, `chip ${body} has nothing to render in the preview`).toBe(true); diff --git a/test/split-pane-terminal.browser.test.ts b/test/split-pane-terminal.browser.test.ts index f747c0b0..4c168943 100644 --- a/test/split-pane-terminal.browser.test.ts +++ b/test/split-pane-terminal.browser.test.ts @@ -167,4 +167,67 @@ describe('SplitTerminalPane in a real browser', () => { await fetch(`/api/sessions/${id}`, { method: 'DELETE' }); }, sessionId); }); + + it('gates app-level chords out of Pane B instead of forwarding their raw bytes', async () => { + // Regression guard for PR #453's Ctrl+K/Alt+1/Alt+B leak: Pane B had no + // attachCustomKeyEventHandler of its own, so the document capture-phase + // shortcut handler's preventDefault() (which does not stop xterm) left + // every one of these chords ALSO writing its raw byte/escape sequence into + // Pane B's live PTY on top of whatever the app action did to Pane A. + const sessionId = await page.evaluate(async () => { + const res = await fetch('/api/sessions', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ workingDir: '/tmp', mode: 'shell' }), + }); + const id = (await res.json()).data.session.id; + await fetch(`/api/sessions/${id}/shell`, { method: 'POST' }); + return id; + }); + + const sentFrames = await page.evaluate(async (id) => { + const mount = document.createElement('div'); + mount.style.width = '400px'; + mount.style.height = '300px'; + document.body.appendChild(mount); + + const pane = new (window as any).SplitTerminalPane(id, mount); + await pane.connect(); + await new Promise((resolve) => { + const check = () => (pane._wsReady ? resolve(undefined) : setTimeout(check, 100)); + check(); + }); + + const sent: string[] = []; + const realSend = pane.ws.send.bind(pane.ws); + pane.ws.send = (payload: string) => { + sent.push(payload); + return realSend(payload); + }; + + pane.terminal.focus(); + // Dispatch straight at xterm's own textarea, matching how a real + // keypress reaches attachCustomKeyEventHandler — page.keyboard.press() + // goes through the OS/CDP input pipeline and would also trigger the + // app's document-capture handler (opening a real command palette), + // 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 })); + }; + fire({ key: 'k', code: 'KeyK', ctrlKey: true }); // command palette + fire({ key: '1', code: 'Digit1', altKey: true }); // Alt+1 tab switch + fire({ key: 'b', code: 'KeyB', altKey: true }); // Alt+B sidebar toggle + + pane.destroy(); + document.body.removeChild(mount); + return sent; + }, sessionId); + + expect(sentFrames.every((f) => JSON.parse(f).t !== 'i')).toBe(true); + + await page.evaluate(async (id) => { + await fetch(`/api/sessions/${id}`, { method: 'DELETE' }); + }, sessionId); + }); });