fix(desktop): keep v2 terminals and browsers stable across workspace switches

apps/desktop/plans/20260423-1226-v2-pane-persistence-across-workspace-switch.md

@@ -0,0 +1,130 @@
+# v2 pane persistence across workspace switch
+
+## Context
+
+Switching v2 workspaces unmounts the entire `<WorkspaceTrpcProvider>` subtree
+(`layout.tsx:79` uses `key={`${workspace.id}:${hostUrl}`}`). Every pane React
+component for the outgoing workspace is torn down and recreated for the
+incoming one. Load-bearing long-lived state (xterm instance + WebSocket,
+webview guest process, CodeMirror `EditorView`) must live OUTSIDE the
+remounting subtree to survive. This note captures the root cause for each
+pane kind and the fix pattern so we don't have to re-derive it.
+
+## Shared root cause
+
+The `key` on `WorkspaceTrpcProvider` is load-bearing — it exists
+(commit `57557f806`) to prevent crashes from hook calls bleeding across
+trpc clients during transitions. We cannot remove it. Any pane that wants
+to survive workspace switches must:
+
+1. Hold its long-lived state in a module-level registry singleton.
+2. Own a DOM node (or native handle) parented *outside* the React
+   workspace subtree (body-level `<div>` is the simplest).
+3. Let the React component be a thin placeholder that only drives
+   position/visibility of the registry-owned node.
+
+Think "VSCode `TerminalInstance` + `setVisible`" or the existing
+`browserRuntimeRegistry` root-container pattern.
+
+## Terminal — fixed in PR #3687
+
+Was broken: `registry.attach()` fused DOM attach with WebSocket open and was
+gated on `ensureSession`. The wrapper was `wrapper.remove()`'d on every
+React unmount, so workspace switch was visible detach + reattach. The
+`ensureSession` gate also added tRPC latency on warm returns, and opened
+the WS against a nonexistent session on cold mount → "Session not found".
+
+Fixed by:
+- Park wrapper in a hidden body-level `#v2-terminal-parking` div on
+  detach instead of `.remove()`.
+- Split `attach` into `mount` (sync DOM) and `connect` (called only after
+  `ensureSession` resolves).
+- Narrow `TerminalPane` effect deps to `[terminalId]`; read `workspaceId`
+  and `websocketUrl` through refs. `websocketUrl` changes go through a
+  separate `registry.reconnect` that no-ops on a cold transport.
+
+Refs: `terminal-runtime.ts`, `terminal-runtime-registry.ts`,
+`TerminalPane.tsx`.
+
+## Browser — fixed
+
+### Symptom
+
+Switching workspaces destroyed the browser webview (and the guest page
+along with it) instead of preserving state across the switch.
+
+### Root cause
+
+Confirmed via instrumentation: `browserRuntimeRegistry.destroy` was
+being called on workspace switch with a stack rooted in React commit.
+The only caller was `usePaneRegistry.tsx`'s `onRemoved` wiring:
+
+```ts
+onRemoved: (pane) => browserRuntimeRegistry.destroy(pane.id),
+```
+
+`onRemoved` comes from `packages/panes/.../Workspace.tsx`, which diffs
+`previousPanesRef` against `current` in a `useEffect` and calls
+`registry[kind].onRemoved` for any id that disappeared. The diff lives
+inside a single Workspace component instance. Under ideal conditions —
+the v2 layout's `key={`${workspace.id}:${hostUrl}`}` remounts on every
+switch — this diff should never observe cross-workspace "removal"
+because each workspace has its own Workspace component.
+
+But the remount isn't always prompt: layout.tsx's `useLiveQuery` can
+return stale WS-A data for a tick while `page.tsx`'s query has already
+flipped to WS-B. During that tick the `key` hasn't changed yet, so the
+existing `WorkspaceContent` stays mounted, `useV2WorkspacePaneLayout`
+calls `store.replaceState(WS-B panes)` on the *same* store instance,
+and the Panes library's diff correctly observes "the browser pane from
+WS-A is gone now" → fires `onRemoved` → destroys the webview. By the
+time the user returns to WS-A, the entry is gone; `attach()` runs the
+`createEntry()` cold path and the webview is recreated with its
+`initialUrl`, losing state.
+
+The terminal never hit this because terminal destruction goes through
+`useGlobalTerminalLifecycle`, which sweeps against *all* workspaces'
+persisted `paneLayout` rows and only destroys ids that are provably
+absent everywhere. Cross-workspace "removal" isn't a real removal from
+that sweep's perspective.
+
+### Fix
+
+Mirrored the terminal pattern: added `useGlobalBrowserLifecycle` under
+`_authenticated/components/GlobalBrowserLifecycle/`, mounted it next to
+`<GlobalTerminalLifecycle />` in `_authenticated/layout.tsx`, and
+removed the `onRemoved` wiring from `usePaneRegistry.tsx`. The new hook
+extracts browser `pane.id`s from every workspace's `paneLayout`, diffs
+against the previous set, and schedules `browserRuntimeRegistry.destroy`
+on a 500 ms grace delay (same timing as the terminal sweep) so
+cross-workspace pane moves don't trigger premature teardown.
+
+Hypothesis #1 (placeholder-rect race) and #3 (webview recycling on
+`visibility: hidden`) from the original list did not reproduce once #2
+was fixed — the instrumentation showed `updateLayout` applying correct
+non-zero rects and the webview surviving detach as long as no `destroy`
+call fired. Left in place as known-good; will revisit if a future
+regression points at either.
+
+## File / Code editor — lower priority
+
+File-viewer panes use CodeMirror `EditorView` created in a `useEffect([])`
+inside `CodeEditor.tsx:153-171`, disposed on unmount. Workspace switch
+therefore loses: undo history, cursor position, scroll position, any
+unsaved viewport scroll. Not reported yet but predictable; users may
+complain after terminal/browser are solid.
+
+Fix pattern is identical: a module-level `codeEditorRegistry` keyed by
+`${workspaceId}:${filePath}` (or pane id, if file viewer panes are
+per-workspace) that owns the `EditorView` and its host div, with a body-
+level root container. `CodeEditor` becomes a placeholder that registers
+a rect.
+
+Defer until it's a reported problem — the migration is mechanical but
+the value is speculative and CodeMirror re-init is already fast.
+
+## Not in scope
+
+- v1 terminal. Sunset per CLAUDE.md / memory.
+- v2 chat pane. Currently a "temporarily disabled" stub.
+- Diff / comment / devtools. No long-lived state.

apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts

@@ -29,7 +29,7 @@ interface RegistryEntry {
 	runtime: TerminalRuntime | null;
 	transport: TerminalTransport;
 	linkManager: TerminalLinkManager | null;
-	/** Stored until linkManager is created (attach called after setLinkHandlers). */
+	/** Stored until linkManager is created (mount called after setLinkHandlers). */
 	pendingLinkHandlers: TerminalLinkHandlers | null;
 }
 
@@ -51,18 +51,28 @@ class TerminalRuntimeRegistryImpl {
 		return entry;
 	}
 
-	attach(
+	/**
+	 * Ensure the xterm runtime exists and attach it to `container`.
+	 * Synchronous. DOM-only — the WebSocket transport is untouched.
+	 *
+	 * Matches VSCode's pattern (`TerminalInstance.attachToElement`) and
+	 * Tabby's (`XTermFrontend.attach`): the terminal renders immediately
+	 * with a blank cursor, the backend pipe catches up via `connect()` once
+	 * the caller has confirmed the server session exists. Decoupling the
+	 * DOM from the transport is what lets a terminal survive workspace
+	 * switches without an in-flight WebSocket being opened against a
+	 * nonexistent session.
+	 */
+	mount(
 		terminalId: string,
 		container: HTMLDivElement,
-		wsUrl: string,
 		appearance: TerminalAppearance,
 	) {
 		const entry = this.getOrCreateEntry(terminalId);
 
 		if (!entry.runtime) {
 			entry.runtime = createRuntime(terminalId, appearance);
 			entry.linkManager = new TerminalLinkManager(entry.runtime.terminal);
-			// Apply pending handlers if setLinkHandlers was called before attach
 			if (entry.pendingLinkHandlers) {
 				entry.linkManager.setHandlers(entry.pendingLinkHandlers);
 				entry.pendingLinkHandlers = null;
@@ -72,17 +82,47 @@ class TerminalRuntimeRegistryImpl {
 		}
 
 		const { runtime, transport } = entry;
-
 		attachToContainer(runtime, container, () => {
 			sendResize(transport, runtime.terminal.cols, runtime.terminal.rows);
 		});
+	}
+
+	/**
+	 * Open (or re-use) the WebSocket transport for this terminal.
+	 * Caller is responsible for ensuring the server session exists before
+	 * calling — otherwise the server replies "Session not found".
+	 *
+	 * Idempotent: no-op if already connected/connecting to the same URL.
+	 */
+	connect(terminalId: string, wsUrl: string) {
+		const entry = this.entries.get(terminalId);
+		if (!entry?.runtime) return;
+		connect(entry.transport, entry.runtime.terminal, wsUrl);
+	}
 
-		connect(transport, runtime.terminal, wsUrl);
+	/**
+	 * Swap the transport onto a new URL when it's already been brought up
+	 * once. Used by effects watching `websocketUrl` — they fire on initial
+	 * mount when the transport is still `"disconnected"` and ensureSession
+	 * is in-flight, and we must not pre-empt that with a premature connect.
+	 *
+	 * Skipped states: `"disconnected"` (never opened; caller should use
+	 * `connect()` via the ensureSession path). Allowed states: `"connecting"`
+	 * (connect() cleanly aborts the in-flight socket), `"open"` (standard
+	 * swap), and `"closed"` (previously live and mid-auto-reconnect — swap
+	 * the URL so the reconnect targets the new endpoint).
+	 */
+	reconnect(terminalId: string, wsUrl: string) {
+		const entry = this.entries.get(terminalId);
+		if (!entry?.runtime) return;
+		if (entry.transport.connectionState === "disconnected") return;
+		if (entry.transport.currentUrl === wsUrl) return;
+		connect(entry.transport, entry.runtime.terminal, wsUrl);
 	}
 
 	/**
 	 * Set link handler callbacks for a terminal. Safe to call before or after
-	 * attach(). If the runtime already exists, link providers are re-registered.
+	 * mount(). If the runtime already exists, link providers are re-registered.
 	 */
 	setLinkHandlers(terminalId: string, handlers: TerminalLinkHandlers) {
 		const entry = this.getOrCreateEntry(terminalId);
@@ -93,6 +133,11 @@ class TerminalRuntimeRegistryImpl {
 		}
 	}
 
+	/**
+	 * Park the wrapper in the hidden body-level container. Runtime and
+	 * transport stay alive; DOM is moved off the React-controlled tree so
+	 * it survives the parent unmount without re-entering xterm.open().
+	 */
 	detach(terminalId: string) {
 		const entry = this.entries.get(terminalId);
 		if (!entry?.runtime) return;

apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts

@@ -124,6 +124,34 @@ function hostIsVisible(container: HTMLDivElement | null): boolean {
 	return container.clientWidth > 0 && container.clientHeight > 0;
 }
 
+// Body-level hidden container that owns wrapper divs of terminals whose
+// React component is currently unmounted (e.g. workspace switch). Keeps
+// xterm attached to the document so it survives provider remounts without
+// a detach/reattach flash — VSCode's setVisible(false) model. Looked up
+// by DOM id so it's HMR-safe (module-level `let` would leak on re-eval).
+// `inert` removes the whole subtree from the tab order and the accessibility
+// tree, and also moves focus out of it — so a parked terminal's internal
+// <textarea> can't receive keystrokes meant for the active pane.
+const PARKING_CONTAINER_ID = "v2-terminal-parking";
+function getParkingContainer(): HTMLDivElement {
+	const existing = document.getElementById(PARKING_CONTAINER_ID);
+	if (existing) return existing as HTMLDivElement;
+
+	const el = document.createElement("div");
+	el.id = PARKING_CONTAINER_ID;
+	el.setAttribute("inert", "");
+	el.setAttribute("aria-hidden", "true");
+	el.style.position = "fixed";
+	el.style.left = "-9999px";
+	el.style.top = "-9999px";
+	el.style.width = "100vw";
+	el.style.height = "100vh";
+	el.style.overflow = "hidden";
+	el.style.pointerEvents = "none";
+	document.body.appendChild(el);
+	return el;
+}
+
 function measureAndResize(runtime: TerminalRuntime) {
 	if (!hostIsVisible(runtime.container)) return;
 	runtime.fitAddon.fit();
@@ -178,6 +206,16 @@ export function attachToContainer(
 	container: HTMLDivElement,
 	onResize?: () => void,
 ) {
+	// If we're already attached to this exact container, do nothing. Prevents
+	// redundant refresh/focus/fit from transient remounts during provider key
+	// churn — VSCode setVisible() is idempotent for the same host element.
+	const sameContainer =
+		runtime.container === container &&
+		runtime.wrapper.parentElement === container;
+	if (sameContainer && runtime.resizeObserver) {
+		return;
+	}
+
 	runtime.container = container;
 	container.appendChild(runtime.wrapper);
 	measureAndResize(runtime);
@@ -201,7 +239,9 @@ export function detachFromContainer(runtime: TerminalRuntime) {
 	persistDimensions(runtime.terminalId, runtime.lastCols, runtime.lastRows);
 	runtime.resizeObserver?.disconnect();
 	runtime.resizeObserver = null;
-	runtime.wrapper.remove();
+	// Park instead of .remove() so xterm survives the React unmount —
+	// see getParkingContainer.
+	getParkingContainer().appendChild(runtime.wrapper);
 	runtime.container = null;
 }