-
Notifications
You must be signed in to change notification settings - Fork 7
Description
Problem
The pane tree data structure conflates two fundamentally different concerns in a single PaneContent object:
-
Pane identity/configuration — what this pane should be:
mode(claude/shell/codex),shell,resumeSessionId,initialCwd. This is declarative, stable, and meaningful to share across browser tabs. -
Terminal lifecycle state — what this pane is doing right now:
createRequestId,terminalId,status(creating/running/exited/error). This is per-connection, ephemeral, and meaningful only to the browser tab that owns the WebSocket connection.
Both live inside TerminalPaneContent, which is persisted to localStorage and synced cross-tab as a single blob. This creates an impossible merge problem at the heart of cross-tab sync.
Why This Is Architectural, Not Just a Bug
mergeTerminalState() in panesSlice.ts exists to untangle these concerns at merge time. It inspects createRequestId, terminalId, and status to decide whether a local terminal's lifecycle is "more advanced" than the incoming remote state. This approach is fundamentally fragile for three reasons:
1. It reconstructs ownership semantics from state comparisons
Each browser tab has its own WebSocket connection, creates its own terminals, and manages its own attach/detach flow independently. When Tab A persists {terminalId: 'T1', status: 'running'} and Tab B picks that up via cross-tab sync, Tab B has no relationship to terminal T1 — it has its own terminal for that pane position. The merge function tries to decide which tab's lifecycle should "win" by comparing fields, but there is no correct answer because the lifecycles are independent.
2. Every new field requires manual merge logic updates
Today mergeTerminalState checks createRequestId, terminalId, and status. It does NOT check resumeSessionId, which caused a bug where cross-tab sync silently swapped which Claude session a pane was attached to. Any future field added to PaneContent (e.g., lastActivityAt, errorMessage, retryCount) will face the same question: is this field "identity" or "lifecycle"? There's no type-level enforcement — it's just institutional knowledge that the merge function needs updating.
3. The merge can never be correct for all cases
Consider these scenarios that mergeTerminalState must handle:
- Tab A is creating a terminal (status:
creating, noterminalId), Tab B's sync arrives with aterminalIdfor the same pane → should Tab A take it? No — it's Tab B's terminal. - Both tabs have
terminalIds for the same pane position but different terminals → which wins? Neither is wrong. - Tab A has
resumeSessionId: SESSION_A, sync arrives withSESSION_Bfor samecreateRequestId→ the merge currently takes the incoming session, silently changing which Claude session the pane represents.
These aren't edge cases — they're the normal operating mode of multi-tab usage.
Proposed Architecture
Separate the pane tree into two layers:
Synced Layer (persisted + cross-tab synced)
The pane tree layout and content configuration:
interface TerminalPaneConfig {
kind: 'terminal'
mode: TerminalMode
shell: ShellType
resumeSessionId?: string // which session to resume
initialCwd?: string // where to start
}This layer is what localStorage persistence and BroadcastChannel sync operate on. Cross-tab sync can freely overwrite it — it's all shared intent, no conflicts.
Local Layer (not synced, not persisted to cross-tab broadcast)
Per-pane terminal lifecycle, keyed by paneId:
interface TerminalLifecycle {
createRequestId: string // idempotency key for this tab's terminal.create
terminalId?: string // server-assigned PTY ID for this tab's connection
status: TerminalStatus // this tab's view of the terminal lifecycle
}This lives in a separate Redux slice (or a non-synced section of the panes slice). It is:
- Initialized on component mount from the synced config
- Updated by WebSocket messages (
terminal.created,terminal.exit, etc.) - Persisted to localStorage for same-tab refresh recovery, but NOT broadcast cross-tab
- Reconstructed on page load: read config → generate new
createRequestId→ sendterminal.create
What Disappears
With this separation, mergeTerminalState() disappears entirely. Cross-tab sync for panes becomes a simple overwrite of the config layer. Each tab's terminal lifecycle is fully independent and never in conflict. The entire class of bugs — wrong terminalId, wrong resumeSessionId, stale status, merge race conditions — becomes impossible.
Migration Path
- Define
TerminalPaneConfigandTerminalLifecycletypes - Create a
terminalLifecycleslice (or non-synced map within panes) - Update
TerminalViewto read config from pane tree, lifecycle from local state - Remove
createRequestId,terminalId,statusfromPaneContent - Update persistence: config syncs cross-tab, lifecycle persists only for same-tab restore
- Delete
mergeTerminalStateand all associated merge tests - Update
crossTabSync.ts—hydratePanesbecomes a simple layout overwrite
Impact
- Eliminates all cross-tab terminal state merge bugs (current and future)
- Simplifies the mental model: "pane tree = what should exist, lifecycle = what's happening"
- Reduces persistence middleware complexity
- Enables future features like per-tab terminal pools without merge conflicts
The current mergeTerminalState approach has required 4+ patches in the last 2 weeks (0d8074a, d6b386d, 781046a, 18a472a) and continues to produce bugs. Each patch adds more conditional logic to an inherently impossible merge. The architectural fix prevents the entire class.