fix: split panes duplicate when a file opens — stale uid collisions with persisted ids

The uid counter is module-global and resets on every reload, while the
persisted split tree keeps its old ids (pane:1, split:1, ...). A split
created AFTER a reload minted a fresh "pane:1" beside the persisted
"pane:1"; mapLeaf then visited BOTH leaves, so every open (explorer click,
produced-files chip) landed the tab in both panes of a top-bottom split.

- loadState seeds the counter past the raw persisted ids before sanitize
- setSession re-seeds on cache hits (another session's smaller max must
  not leave the counter below this session's ids)
- sanitize re-ids stale duplicate pane/split ids (heals states already
  corrupted) and follows the activePane rename
- openTab falls back to the first pane when the active pane is gone
  instead of silently dropping the open

Unit tests cover the re-id heal and the stale-activePane fallback;
verified end-to-end in headless Chrome: a state with two "pane:1" leaves
loads healed and a file click lands in exactly one pane.

src/client/state.ts

@@ -66,6 +66,39 @@ function uid(prefix: string): string {
   return `${prefix}:${nextIdCounter}`
 }
 
+/**
+ * The largest numeric suffix across a raw persisted state's counter ids
+ * (`pane:N` / `tab:N` / `split:N`). The uid counter is module-global and
+ * resets on every reload, so a split minted AFTER a reload would collide
+ * with the persisted ids (a fresh "pane:1" beside the persisted "pane:1");
+ * mapLeaf would then visit BOTH leaves and every open would land in both
+ * panes of the split. Seeding the counter past the persisted ids keeps
+ * fresh ids disjoint.
+ */
+function maxCounterId(parsed: unknown): number {
+  let max = 0
+  const consider = (id: unknown): void => {
+    if (typeof id !== 'string') return
+    const match = /^(?:pane|tab|split):(\d+)$/.exec(id)
+    if (match !== null) max = Math.max(max, Number(match[1]))
+  }
+  const walk = (node: unknown): void => {
+    if (node === null || typeof node !== 'object') return
+    const record = node as Record<string, unknown>
+    consider(record.id)
+    if (Array.isArray(record.tabs)) {
+      for (const tab of record.tabs) {
+        if (tab !== null && typeof tab === 'object') consider((tab as Record<string, unknown>).id)
+      }
+    }
+    if (Array.isArray(record.children)) {
+      for (const child of record.children) walk(child)
+    }
+  }
+  walk((parsed as Record<string, unknown> | null)?.splits)
+  return max
+}
+
 /** A fresh default state: open, one explorer tab in one pane. `width` is
  * the caller's preferred panel width (default PANEL_DEFAULT); the store
  * seeds new sessions with half the viewport. */
@@ -268,7 +301,12 @@ function isSingle(type: TabType): boolean {
  * in the active pane (or the first pane when the active one is gone).
  */
 export function openTab(state: SidebarState, tab: SidebarTab): SidebarState {
-  const targetId = state.activePane ?? firstLeaf(state.splits).id
+  let targetId = state.activePane ?? firstLeaf(state.splits).id
+  // A stale activePane (its pane was closed since) must not swallow the
+  // open: fall back to the first pane instead of dropping the tab.
+  if (!allLeaves(state.splits).some(leaf => leaf.id === targetId)) {
+    targetId = firstLeaf(state.splits).id
+  }
   if (isSingle(tab.type)) {
     for (const leaf of allLeaves(state.splits)) {
       const existing = leaf.tabs.find(candidate => candidate.type === tab.type)
@@ -371,7 +409,11 @@ function loadState(sessionId: string): SidebarState {
   try {
     const raw = localStorage.getItem(`${STORAGE_PREFIX}:${sessionId}`)
     if (raw !== null) {
-      const sanitized = sanitizeState(JSON.parse(raw) as unknown)
+      const parsed = JSON.parse(raw) as unknown
+      // Seed the uid counter past the persisted ids (it resets on reload);
+      // sanitize re-ids any duplicates the pre-seeding counter left behind.
+      nextIdCounter = maxCounterId(parsed)
+      const sanitized = sanitizeState(parsed)
       if (sanitized !== undefined) return sanitized
     }
   } catch {
@@ -404,21 +446,44 @@ export function sanitizeState(parsed: unknown): SidebarState | undefined {
   }
   if (typeof record.activePane !== 'string' && record.activePane !== null) return undefined
   if (!Array.isArray(record.expanded) || record.expanded.some(item => typeof item !== 'string')) return undefined
-  const splits = sanitizeNode(record.splits)
+  const reid = new Map<string, string>()
+  const splits = sanitizeNode(record.splits, new Set(), reid)
   if (splits === undefined) return undefined
   const maxWidth = typeof window !== 'undefined' ? window.innerWidth : Infinity
   return {
     panelOpen: record.panelOpen,
     width: Math.max(PANEL_MIN, Math.min(record.width, maxWidth)),
-    activePane: typeof record.activePane === 'string' ? record.activePane : null,
+    // A stale duplicate pane id may have been re-ided; follow the rename so
+    // new tabs still land in the pane the user was using.
+    activePane: typeof record.activePane === 'string' ? (reid.get(record.activePane) ?? record.activePane) : null,
     nextTerminal: record.nextTerminal,
     expanded: record.expanded as string[],
     splits,
   }
 }
 
+/**
+ * One tree node id, deduplicated against the ids already seen in this
+ * state. Duplicates are exactly the pre-seeding counter-reset corruption
+ * (a "pane:1"/"split:1" minted after a reload beside the persisted ones):
+ * keeping both would make mapLeaf visit two leaves at once and every open
+ * would land in both panes, so the repeat gets a fresh id.
+ * @returns the id to use (the original, or a fresh uid for repeats).
+ */
+function uniqueNodeId(id: string, seen: Set<string>, reid: Map<string, string>): string {
+  if (!seen.has(id)) {
+    seen.add(id)
+    return id
+  }
+  const prefix = /^split:\d+$/.test(id) ? 'split' : 'pane'
+  const fresh = uid(prefix)
+  seen.add(fresh)
+  reid.set(id, fresh)
+  return fresh
+}
+
 /** Validate one split-tree node (leaf or split) and rebuild it cleanly. */
-function sanitizeNode(node: unknown): SplitNode | undefined {
+function sanitizeNode(node: unknown, seen: Set<string>, reid: Map<string, string>): SplitNode | undefined {
   if (node === null || typeof node !== 'object') return undefined
   const record = node as Record<string, unknown>
   if (record.kind === 'leaf') {
@@ -443,14 +508,14 @@ function sanitizeNode(node: unknown): SplitNode | undefined {
     }
     const active = typeof record.active === 'string' ? record.active : null
     if (active !== null && !tabs.some(tab => tab.id === active)) return undefined
-    return { kind: 'leaf', id: record.id, tabs, active }
+    return { kind: 'leaf', id: uniqueNodeId(record.id, seen, reid), tabs, active }
   }
   if (record.kind === 'split') {
     if (typeof record.id !== 'string' || (record.dir !== 'row' && record.dir !== 'col')) return undefined
     if (!Array.isArray(record.children) || !Array.isArray(record.sizes)) return undefined
     const children: SplitNode[] = []
     for (const child of record.children) {
-      const clean = sanitizeNode(child)
+      const clean = sanitizeNode(child, seen, reid)
       if (clean === undefined) return undefined
       children.push(clean)
     }
@@ -461,7 +526,7 @@ function sanitizeNode(node: unknown): SplitNode | undefined {
     ) {
       return undefined
     }
-    return { kind: 'split', id: record.id, dir: record.dir, sizes: record.sizes as number[], children }
+    return { kind: 'split', id: uniqueNodeId(record.id, seen, reid), dir: record.dir, sizes: record.sizes as number[], children }
   }
   return undefined
 }
@@ -483,6 +548,11 @@ export class SidebarStore {
       if (state === undefined) {
         state = loadState(sessionId)
         this.bySession.set(sessionId, state)
+      } else {
+        // Cache hit: another session's load/ops may have left the uid
+        // counter below THIS session's persisted ids — re-seed so fresh
+        // pane/split ids can never collide with its tree.
+        nextIdCounter = maxCounterId(state)
       }
       this.snapshot = { sessionId, state }
     }

tests/unit.spec.ts

@@ -2,8 +2,8 @@ import { describe, expect, it } from 'vitest'
 import { compareEntries, parentOf, rootLabel, requireAbsolute } from '../src/fs-tree.ts'
 import { parseLogLines, parsePorcelainZ } from '../src/git.ts'
 import {
-  activateTab, closeTab, makeDefaultState, moveTab, moveTabToEdge, openTab,
-  resizeSplit, sanitizeState, splitPane, tabOpenIn, toggleExpanded,
+  activateTab, allLeaves, closeTab, insertLeafAt, makeDefaultState, moveTab, moveTabToEdge,
+  openTab, resizeSplit, sanitizeState, splitPane, tabOpenIn, toggleExpanded,
   type SidebarState, type SplitNode,
 } from '../src/client/state.ts'
 import { extOf, languageKeyForExt } from '../src/client/lang.ts'
@@ -317,6 +317,53 @@ describe('persisted state sanitization', () => {
     withBadActive.splits.active = 'ghost-tab'
     expect(sanitizeState(withBadActive)).toBeUndefined()
   })
+
+  it('re-ids stale duplicate pane/split ids and follows the activePane rename', () => {
+    // The pre-seeding counter reset could mint a fresh "pane:1" beside the
+    // persisted "pane:1": mapLeaf then hit BOTH leaves and every open landed
+    // in both panes. Sanitize must give the repeat a fresh id.
+    const corrupted = JSON.parse(JSON.stringify(makeDefaultState(400)))
+    corrupted.activePane = 'pane:1'
+    corrupted.splits = {
+      kind: 'split',
+      id: 'split:1',
+      dir: 'col',
+      sizes: [0.5, 0.5],
+      children: [
+        { kind: 'leaf', id: 'pane:1', tabs: [], active: null },
+        { kind: 'leaf', id: 'pane:1', tabs: [{ id: 'tab:1', type: 'explorer', title: 'Explorer' }], active: 'tab:1' },
+      ],
+    }
+    const clean = sanitizeState(corrupted)
+    expect(clean).toBeDefined()
+    const leaves = allLeaves(clean!.splits)
+    // The first occurrence keeps its id; the repeat gets a fresh unique one
+    // (exact suffix depends on the module-level uid counter, so assert shape).
+    expect(leaves[0]!.id).toBe('pane:1')
+    expect(new Set(leaves.map(leaf => leaf.id)).size).toBe(2)
+    expect(clean!.activePane).toBe(leaves[1]!.id)
+    // And an open must land in exactly one pane of the healed tree.
+    const opened = openTab(clean!, { id: 'editor:/a.ts', type: 'editor', title: 'a.ts', path: '/a.ts' })
+    const owners = allLeaves(opened.splits).filter(leaf => leaf.tabs.some(tab => tab.path === '/a.ts'))
+    expect(owners).toHaveLength(1)
+  })
+
+  it('falls back from a stale active pane instead of dropping the open', () => {
+    let s = makeDefaultState()
+    const paneA = allLeaves(s.splits)[0]!.id
+    const explorerTab = allLeaves(s.splits)[0]!.tabs.find(tab => tab.type === 'explorer')!.id
+    s = closeTab(s, paneA, explorerTab)
+    s = openTab(s, { id: 'editor:/a.ts', type: 'editor', title: 'a.ts', path: '/a.ts' })
+    const split = insertLeafAt(s.splits, paneA, 'col', { id: 'terminal:1', type: 'terminal', title: 'Terminal 1' }, false)
+    s = { ...s, splits: split.node, activePane: paneA }
+    // Closing the editor empties paneA; the pane is removed but activePane
+    // still points at it. The next open must land in the surviving pane.
+    s = closeTab(s, paneA, 'editor:/a.ts')
+    s = openTab(s, { id: 'editor:/b.ts', type: 'editor', title: 'b.ts', path: '/b.ts' })
+    const owners = allLeaves(s.splits).filter(leaf => leaf.tabs.some(tab => tab.path === '/b.ts'))
+    expect(owners).toHaveLength(1)
+    expect(owners[0]!.tabs.some(tab => tab.type === 'terminal')).toBe(true)
+  })
 })
 
 describe('path helpers', () => {