fix(split-pane): stop leaking document listeners on repeated split-picker toggles

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

src/web/public/index.html

@@ -191,7 +191,7 @@
         </button>
         <button class="btn-icon-header btn-file-viewer" onclick="app.toggleFileBrowserButton()" title="File Viewer" aria-label="Open file viewer" aria-expanded="false"><svg width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" aria-hidden="true"><path d="M3 7a2 2 0 0 1 2-2h4l2 2h8a2 2 0 0 1 2 2v8a2 2 0 0 1-2 2H5a2 2 0 0 1-2-2z"/></svg></button>
         <button class="btn-icon-header btn-multimonitor btn-multimonitor--hidden" onclick="app.launchMultiMonitor()" title="Open Codeman across all displays" aria-label="Open Codeman across all displays"><svg width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" aria-hidden="true"><rect x="2" y="4" width="13" height="9" rx="1.5"/><rect x="11" y="9" width="11" height="8" rx="1.5"/></svg></button>
-        <button class="btn-icon-header btn-split btn-split--hidden" onclick="app.openSplitPicker()" title="Split: open a second session beside this one" aria-label="Split: open a second session beside this one"><svg width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" aria-hidden="true"><rect x="2" y="3" width="20" height="18" rx="2"/><line x1="12" y1="3" x2="12" y2="21"/></svg></button>
+        <button class="btn-icon-header btn-split btn-split--hidden" onclick="app.openSplitPicker(event)" title="Split: open a second session beside this one" aria-label="Split: open a second session beside this one"><svg width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" aria-hidden="true"><rect x="2" y="3" width="20" height="18" rx="2"/><line x1="12" y1="3" x2="12" y2="21"/></svg></button>
         <button class="btn-icon-header btn-ultracode-agents btn-ultracode-agents--hidden" onclick="app.toggleUltracodeAgentsPanel()" title="Ultracode / Workflow agents" aria-label="Open ultracode workflow agents"><svg width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" aria-hidden="true"><circle cx="6" cy="6" r="2.5"/><circle cx="6" cy="18" r="2.5"/><circle cx="18" cy="12" r="2.5"/><path d="M8.2 7.2 15.6 11M8.2 16.8 15.6 13"/></svg></button>
         <div class="header-plan-usage header-plan-usage--hidden" id="planUsageChip" title="Claude and Codex plan usage limits">—</div>
         <button class="btn-icon-header btn-notifications" onclick="app.toggleNotifications()" title="Notifications" aria-label="Toggle notifications" style="display:none;">

src/web/public/terminal-split.js

@@ -124,7 +124,14 @@
 })(window);
 
 Object.assign(CodemanApp.prototype, {
-  openSplitPicker() {
+  openSplitPicker(event) {
+    // Mirrors toggleRunModeMenu (session-ui.js): stopPropagation on the
+    // OPENING click so it never reaches the outside-click listener this
+    // same call is about to register — without it, a click landing on the
+    // button's own inner <svg> (matched by neither `menu.contains()` nor
+    // the old exact-node check below) bubbled straight through to
+    // `document` and self-closed the menu it just opened.
+    event?.stopPropagation();
     if (this._splitPane) {
       this.closeSplitPane();
       return;
@@ -134,8 +141,13 @@ Object.assign(CodemanApp.prototype, {
       this.sessionOrder,
       this.activeSessionId
     );
-    const existing = document.getElementById('splitPickerMenu');
+    // Route a pre-existing menu through the SAME dismiss path used
-    if (existing) existing.remove();
+    // everywhere else, instead of a raw `.remove()`: a genuinely still-open
+    // menu has live document listeners (see below), and a raw removal left
+    // them attached forever — only the single-slot field below got
+    // overwritten, so every prior pair but the last was orphaned on
+    // `document` with no way to ever find and remove it again.
+    this._dismissSplitPicker();
 
     const menu = document.createElement('div');
     menu.id = 'splitPickerMenu';
@@ -162,14 +174,32 @@ Object.assign(CodemanApp.prototype, {
     // Dismiss on outside click or Escape — same one-shot listener pattern as
     // session-ui.js's other transient popovers (toggleCaseSettings(),
     // toggleRunModeMenu()). Deferred by a tick so the click that OPENED the
-    // menu (still bubbling) doesn't immediately close it. Picking an item
+    // menu (still bubbling) doesn't immediately close it — reinforced by
+    // the button's own stopPropagation() above, which is what actually
+    // stops that same click reaching `document` at all. Picking an item
     // (above) calls the SAME dismiss method, so these listeners never
     // outlive the menu either way.
+    //
+    // Self-removing by identity: each handler removes ITSELF (and its
+    // sibling) the moment it fires, rather than leaning solely on the
+    // `this._splitPickerDismissHandlers` field. That field is still kept in
+    // sync (so `_dismissSplitPicker()` called from elsewhere — the picker
+    // item's onclick above, or a still-open menu at the top of this method
+    // — can find and remove the CURRENT pair), but no path here can ever
+    // again leave a pair attached to `document` with nothing referencing it.
     const closeOnOutsideClick = (e) => {
-      if (!menu.contains(e.target) && e.target !== splitBtn) this._dismissSplitPicker();
+      if (menu.contains(e.target) || e.target.closest('.btn-split')) return;
+      document.removeEventListener('click', closeOnOutsideClick);
+      document.removeEventListener('keydown', closeOnEscape);
+      this._splitPickerDismissHandlers = null;
+      menu.remove();
     };
     const closeOnEscape = (e) => {
-      if (e.key === 'Escape') this._dismissSplitPicker();
+      if (e.key !== 'Escape') return;
+      document.removeEventListener('click', closeOnOutsideClick);
+      document.removeEventListener('keydown', closeOnEscape);
+      this._splitPickerDismissHandlers = null;
+      menu.remove();
     };
     this._splitPickerDismissHandlers = { closeOnOutsideClick, closeOnEscape };
     setTimeout(() => document.addEventListener('click', closeOnOutsideClick), 0);

test/split-pane-picker-listener-leak-unit.test.ts

@@ -0,0 +1,262 @@
+// test/split-pane-picker-listener-leak-unit.test.ts
+// Port: N/A (no server/browser — loaded via `vm`, like split-pane-auto-collapse-unit.test.ts).
+//
+// Fast, CI-visible regression coverage for a listener leak in `openSplitPicker()` /
+// `_dismissSplitPicker()` (terminal-split.js): repeatedly opening the split picker
+// (e.g. clicking the `.btn-split` button, whose real clicks land on its inner
+// <svg>) used to leave a `click`/`keydown` listener pair attached to `document`
+// on every cycle, because a pre-existing menu was torn down with a raw
+// `existing.remove()` instead of through `_dismissSplitPicker()`, and the
+// single-slot `_splitPickerDismissHandlers` field was overwritten rather than
+// used to clean up the previous pair first.
+//
+// This is a `vm`-driven DOM-listener-count assertion rather than a real-browser
+// interaction test (`test/split-pane-orchestration.browser.test.ts` already
+// covers the real click-through-svg interaction and is excluded from `npm test`
+// per CLAUDE.md's Testing section) — it exercises the exact document
+// addEventListener/removeEventListener calls the fix and the bug both hinge on,
+// with no xterm/WebSocket/tmux involved.
+import { readFileSync } from 'node:fs';
+import { resolve } from 'node:path';
+import vm from 'node:vm';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+
+/** A tiny fake DOM element: enough surface for createElement/appendChild/contains/remove/closest. */
+function makeFakeElement(tag: string) {
+  const children: Array<ReturnType<typeof makeFakeElement>> = [];
+  let id: string | undefined;
+  let parent: ReturnType<typeof makeFakeElement> | null = null;
+  const el = {
+    tag,
+    style: {} as Record<string, string>,
+    innerHTML: '',
+    className: '',
+    get id() {
+      return id;
+    },
+    set id(v: string | undefined) {
+      id = v;
+    },
+    appendChild(child: ReturnType<typeof makeFakeElement>) {
+      children.push(child);
+      (child as { _parent: unknown })._parent = el;
+      return child;
+    },
+    contains(target: unknown): boolean {
+      if (target === el) return true;
+      return children.some((c) => c.contains(target));
+    },
+    // Real Element.closest(): walk up from THIS element (inclusive), matching
+    // a bare class selector like `.btn-split` against a space-separated
+    // className — enough to exercise the fix's `e.target.closest('.btn-split')`
+    // check without a real DOM.
+    closest(selector: string): ReturnType<typeof makeFakeElement> | null {
+      const cls = selector.replace(/^\./, '');
+      let node: ReturnType<typeof makeFakeElement> | null = el;
+      while (node) {
+        if (node.className.split(/\s+/).includes(cls)) return node;
+        node = (node as unknown as { __parent: ReturnType<typeof makeFakeElement> | null }).__parent;
+      }
+      return null;
+    },
+    remove() {
+      if (parent) {
+        parent = null;
+      }
+    },
+    getBoundingClientRect() {
+      return { bottom: 0, right: 0, left: 0, top: 0, width: 0, height: 0 };
+    },
+    set _parent(p: ReturnType<typeof makeFakeElement>) {
+      parent = p;
+      (el as unknown as { __parent: ReturnType<typeof makeFakeElement> }).__parent = p;
+    },
+  };
+  return el;
+}
+
+/** A minimal `document` mock tracking real addEventListener/removeEventListener identity. */
+function makeFakeDocument() {
+  const elementsById = new Map<string, ReturnType<typeof makeFakeElement>>();
+  const listeners: Record<string, Array<(...args: unknown[]) => unknown>> = {
+    click: [],
+    keydown: [],
+  };
+
+  return {
+    createElement(tag: string) {
+      return makeFakeElement(tag);
+    },
+    body: {
+      appendChild(el: ReturnType<typeof makeFakeElement>) {
+        if (el.id) elementsById.set(el.id, el);
+      },
+    },
+    getElementById(id: string) {
+      const el = elementsById.get(id);
+      if (el) {
+        // Support the real `?.remove()` call site removing it from the registry.
+        const originalRemove = el.remove.bind(el);
+        el.remove = () => {
+          elementsById.delete(id);
+          originalRemove();
+        };
+      }
+      return el;
+    },
+    querySelector(sel: string) {
+      return sel === '.btn-split' ? makeFakeElement('button') : null;
+    },
+    addEventListener(type: string, fn: (...args: unknown[]) => unknown) {
+      (listeners[type] ??= []).push(fn);
+    },
+    removeEventListener(type: string, fn: (...args: unknown[]) => unknown) {
+      listeners[type] = (listeners[type] ?? []).filter((f) => f !== fn);
+    },
+    __listenerCount(type: string) {
+      return (listeners[type] ?? []).length;
+    },
+    // Test-only helpers: fire a snapshot of the currently-registered
+    // listeners (a handler removing itself mid-dispatch must not skip or
+    // double-invoke a sibling — snapshotting avoids that ambiguity here).
+    __dispatchClick(target: unknown) {
+      for (const fn of [...(listeners.click ?? [])]) fn({ target });
+    },
+    __dispatchKeydown(key: string) {
+      for (const fn of [...(listeners.keydown ?? [])]) fn({ key });
+    },
+  };
+}
+
+function loadCodemanAppClass(documentMock: ReturnType<typeof makeFakeDocument>) {
+  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: 1024 },
+    document: documentMock,
+    setTimeout,
+    // terminal-split.js's picker markup calls the global `escapeHtml()` helper
+    // (defined in constants.js at runtime); a plain identity stub is enough
+    // here since no candidate labels/ids in this test contain HTML.
+    escapeHtml: (s: unknown) => String(s),
+  });
+  const fakeAppSrc = `
+    class CodemanApp {
+      constructor() {
+        this.sessions = new Map();
+        this.sessionOrder = [];
+        this.activeSessionId = 'active-session';
+        this._splitPane = null;
+      }
+    }
+  `;
+  vm.runInContext(
+    `${fakeAppSrc}\nwindow.CodemanSplitPane = { buildSplitPickerSessions: () => [] };\n${terminalSplitSrc}\nglobalThis.__CodemanApp = CodemanApp;`,
+    context
+  );
+  return (
+    context as { __CodemanApp: new () => { openSplitPicker: (e?: unknown) => void; _dismissSplitPicker: () => void } }
+  ).__CodemanApp;
+}
+
+describe('terminal-split.js split-picker document-listener leak (repeated open/close)', () => {
+  let documentMock: ReturnType<typeof makeFakeDocument>;
+  let CodemanApp: ReturnType<typeof loadCodemanAppClass>;
+
+  beforeEach(() => {
+    vi.useFakeTimers();
+    documentMock = makeFakeDocument();
+    CodemanApp = loadCodemanAppClass(documentMock);
+  });
+
+  afterEach(() => {
+    vi.useRealTimers();
+  });
+
+  it('does not grow document click/keydown listeners across repeated open cycles with no dismissal', () => {
+    const app = new CodemanApp();
+
+    // Simulates the real failure sequence: the button's onclick re-fires
+    // `openSplitPicker()` on every click (e.g. landing on the button's inner
+    // <svg>) without an intervening outside click or Escape ever resolving.
+    for (let i = 0; i < 5; i++) {
+      app.openSplitPicker();
+      vi.runAllTimers(); // flush the deferred `setTimeout(() => addEventListener('click', ...))`
+    }
+
+    // Exactly one pair should be live — the CURRENT menu's — never one per cycle.
+    expect(documentMock.__listenerCount('click')).toBe(1);
+    expect(documentMock.__listenerCount('keydown')).toBe(1);
+  });
+
+  it('drops to zero listeners after the outside-click handler fires', () => {
+    const app = new CodemanApp();
+    app.openSplitPicker();
+    vi.runAllTimers();
+    expect(documentMock.__listenerCount('click')).toBe(1);
+
+    app._dismissSplitPicker();
+
+    expect(documentMock.__listenerCount('click')).toBe(0);
+    expect(documentMock.__listenerCount('keydown')).toBe(0);
+  });
+
+  it('never accumulates listeners across many open→dismiss cycles', () => {
+    const app = new CodemanApp();
+
+    for (let i = 0; i < 20; i++) {
+      app.openSplitPicker();
+      vi.runAllTimers();
+      app._dismissSplitPicker();
+    }
+
+    expect(documentMock.__listenerCount('click')).toBe(0);
+    expect(documentMock.__listenerCount('keydown')).toBe(0);
+  });
+
+  it("does not dismiss when the click lands on the split button's own inner icon (closest() match, not exact-node equality)", () => {
+    const app = new CodemanApp();
+    app.openSplitPicker();
+    vi.runAllTimers();
+    expect(documentMock.__listenerCount('click')).toBe(1);
+
+    // Mirrors the real DOM: the button carries the onclick and the
+    // `.btn-split` class, but the actual click target is its inner <svg>.
+    // The old `e.target !== splitBtn` check matched this (target !== button)
+    // and dismissed the menu the same click had just (re)opened.
+    const button = makeFakeElement('button');
+    button.className = 'btn-split';
+    const svg = button.appendChild(makeFakeElement('svg'));
+
+    documentMock.__dispatchClick(svg);
+
+    expect(documentMock.__listenerCount('click')).toBe(1);
+    expect(documentMock.__listenerCount('keydown')).toBe(1);
+  });
+
+  it('dismisses and removes both listeners by identity when a genuine outside click fires', () => {
+    const app = new CodemanApp();
+    app.openSplitPicker();
+    vi.runAllTimers();
+    expect(documentMock.__listenerCount('click')).toBe(1);
+
+    const outside = makeFakeElement('div');
+    documentMock.__dispatchClick(outside);
+
+    expect(documentMock.__listenerCount('click')).toBe(0);
+    expect(documentMock.__listenerCount('keydown')).toBe(0);
+  });
+
+  it('dismisses and removes both listeners by identity when Escape fires', () => {
+    const app = new CodemanApp();
+    app.openSplitPicker();
+    vi.runAllTimers();
+    expect(documentMock.__listenerCount('keydown')).toBe(1);
+
+    documentMock.__dispatchKeydown('Escape');
+
+    expect(documentMock.__listenerCount('click')).toBe(0);
+    expect(documentMock.__listenerCount('keydown')).toBe(0);
+  });
+});