From 8d3bde54693de900a263591b715c64a94619970e Mon Sep 17 00:00:00 2001 From: timkjr Date: Sat, 19 Sep 2026 11:46:58 -0500 Subject: [PATCH] fix(split-pane): address the rest of Ark0N's PR #453 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Exclude popped-out (detached) sessions from the split picker: SplitTerminalPane._sendResize() has no yield-to-detached-window check the way the primary pane's sendResize() does, so splitting against a detached session put its own window and Pane B in a fight over the same PTY's dimensions. Simplest fix per the review: keep them out of buildSplitPickerSessions() entirely. - Show a visible dead state when Pane B's WebSocket drops. onData already silently discards keystrokes while the socket isn't OPEN (there is no reconnect for v1), so a dropped socket left the pane looking normal while it quietly ate everything typed into it. - openSplitPane() returns early with no active session, so a split triggered from the home screen no longer creates and connects Pane B behind the opaque welcome overlay with nothing to show for it. - onMove() during a divider drag now bails when the split has auto-collapsed mid-drag (the other pane's session ending) instead of throwing on `divider.parentElement` being null. - Promote Pane B via `selectSession(id, { auto: true })` when Pane A's session ends — this is an app-driven selection, not the user clicking a tab, so it must not spend the promoted session's idle alert. Co-Authored-By: Claude Sonnet 5 --- src/web/public/constants.js | 8 +++++- src/web/public/terminal-split.js | 30 +++++++++++++++++++--- test/split-pane-auto-collapse-unit.test.ts | 4 ++- 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/src/web/public/constants.js b/src/web/public/constants.js index 1fac74a3..72cc6e7d 100644 --- a/src/web/public/constants.js +++ b/src/web/public/constants.js @@ -1526,10 +1526,16 @@ function clampDividerPercent(rawPercent, min = 20, max = 80) { return rawPercent; } -function buildSplitPickerSessions(sessions, sessionOrder, excludeId) { +function buildSplitPickerSessions(sessions, sessionOrder, excludeId, detachedIds) { const result = []; for (const id of sessionOrder) { if (id === excludeId) continue; + // A detached (popped-out) session's own window already yields its PTY + // size (see sendResize's detachedElsewhere guard in terminal-ui.js) — + // Pane B's SplitTerminalPane._sendResize() has no such check, so letting + // one into the picker put its detached window and Pane B in a fight over + // the same PTY's dimensions. + if (detachedIds?.has?.(id)) continue; const session = sessions.get(id); if (!session) continue; result.push({ id, label: session.name || 'Session' }); diff --git a/src/web/public/terminal-split.js b/src/web/public/terminal-split.js index d39c5b96..dceb6aa6 100644 --- a/src/web/public/terminal-split.js +++ b/src/web/public/terminal-split.js @@ -99,9 +99,15 @@ // closed socket per the WebSocket spec (no exception, no log). No // reconnect logic here — Pane B is deliberately plainer than the // primary pane (see the fileoverview above); a drop just stops - // resizing until the parent recreates the pane. + // resizing until the parent recreates the pane. But onData already + // silently drops keystrokes while _wsReady is false (below), so + // without a visible marker a dropped socket left Pane B looking + // normal while it quietly ate everything typed into it. v1 scope is + // "say so", not reconnect — collapsing the split would lose the + // user's place in Pane B's scrollback for a transient blip. this.ws.onclose = () => { this._wsReady = false; + this.terminal?.write('\r\n\x1b[2m[Pane B disconnected — close and reopen the split to reconnect]\x1b[0m\r\n'); }; this.ws.onerror = () => { @@ -163,7 +169,8 @@ Object.assign(CodemanApp.prototype, { const candidates = window.CodemanSplitPane.buildSplitPickerSessions( this.sessions, this.sessionOrder, - this.activeSessionId + this.activeSessionId, + this.detachedSessions ); // Route a pre-existing menu through the SAME dismiss path used // everywhere else, instead of a raw `.remove()`: a genuinely still-open @@ -240,6 +247,11 @@ Object.assign(CodemanApp.prototype, { }, openSplitPane(sessionId) { + // No active session means there is no `.terminal-wrap` to split against + // (the welcome overlay is showing) — without this, a split opened from + // 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 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 @@ -314,6 +326,12 @@ Object.assign(CodemanApp.prototype, { const onMove = (e) => { if (!dragging) return; const container = divider.parentElement; + // The split can auto-collapse mid-drag (the other pane's session + // ending, or the picker's own close button) — closeSplitPane() removes + // `.terminal-split-container` from the DOM, which detaches `divider` + // too, so `divider.parentElement` is null on the very next mousemove + // and every drag threw here until mouseup finally removed the listener. + if (!container) return; const rect = container.getBoundingClientRect(); const rawPercent = ((e.clientX - rect.left) / rect.width) * 100; const percent = window.CodemanSplitPane.clampDividerPercent(rawPercent); @@ -356,10 +374,14 @@ CodemanApp.prototype._onSessionDeleted = function (data) { this.closeSplitPane(); } else if (this._splitPane && this.activeSessionId === data.id) { // Pane A's session ended: promote Pane B by closing the split and - // selecting its session as the new (single) active pane. + // selecting its session as the new (single) active pane. This is an + // app-driven selection, not the user clicking a tab, so it must not + // spend the promoted session's idle alert (see the Approvals Inbox + // acknowledgement rule in CLAUDE.md — only a human opening a session + // acknowledges it). const promoted = this._splitSessionId; this.closeSplitPane(); - if (promoted) this.selectSession(promoted); + if (promoted) this.selectSession(promoted, { auto: true }); } return _originalOnSessionDeleted.call(this, data); }; diff --git a/test/split-pane-auto-collapse-unit.test.ts b/test/split-pane-auto-collapse-unit.test.ts index c98f5d9f..e71440ba 100644 --- a/test/split-pane-auto-collapse-unit.test.ts +++ b/test/split-pane-auto-collapse-unit.test.ts @@ -83,7 +83,9 @@ describe('terminal-split.js _onSessionDeleted wrapper (I6)', () => { expect(app.closeSplitPane).toHaveBeenCalledTimes(1); // Pinned ordering: selectSession must receive the id _splitSessionId held // BEFORE closeSplitPane ran (which nulls it), not whatever it holds after. - expect(app.selectSession).toHaveBeenCalledWith('session-b'); + // { auto: true } because this is an app-driven promotion, not the user + // clicking a tab — it must not spend the promoted session's idle alert. + expect(app.selectSession).toHaveBeenCalledWith('session-b', { auto: true }); expect(app.__originalDeletedCalls).toEqual([{ id: 'session-a' }]); });