diff --git a/apps/web/components/task/dockview-desktop-layout.tsx b/apps/web/components/task/dockview-desktop-layout.tsx index bf40226597..2cb72220a0 100644 --- a/apps/web/components/task/dockview-desktop-layout.tsx +++ b/apps/web/components/task/dockview-desktop-layout.tsx @@ -10,7 +10,7 @@ import { } from "dockview-react"; import { themeKandev } from "@/lib/layout/dockview-theme"; import { useDockviewStore, performLayoutSwitch } from "@/lib/state/dockview-store"; -import { tryRestoreLayout } from "./dockview-layout-restore"; +import { restoreEnvLayout } from "./dockview-layout-restore"; import { setupContainerResizeSync, setupGroupTracking, @@ -513,8 +513,8 @@ export const DockviewDesktopLayout = memo(function DockviewDesktopLayout({ setApi(api); const currentEnvId = envIdRef.current; - // If a layout intent was passed via URL, skip saved layout restoration - const restored = !initialLayout && tryRestoreLayout(api, currentEnvId, VALID_COMPONENTS); + const restored = + !initialLayout && restoreEnvLayout(api, currentEnvId, appStore, VALID_COMPONENTS); if (!restored) { buildDefaultLayout(api, initialLayout ?? (compact ? "compact" : undefined)); } diff --git a/apps/web/components/task/dockview-layout-restore.test.ts b/apps/web/components/task/dockview-layout-restore.test.ts index 26cba61145..d0d9bcc9e6 100644 --- a/apps/web/components/task/dockview-layout-restore.test.ts +++ b/apps/web/components/task/dockview-layout-restore.test.ts @@ -1,7 +1,14 @@ -import { describe, it, expect } from "vitest"; -import { sanitizeLayout } from "./dockview-layout-restore"; +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { + collectPhantomSessionIdsForEnv, + sanitizeLayout, + tryRestoreLayout, +} from "./dockview-layout-restore"; +import * as localStorage from "@/lib/local-storage"; const VALID_COMPONENTS = new Set(["chat", "files", "shell", "git", "terminal"]); +const PHANTOM_PANEL_ID = "session:phantom"; +const ALIVE_PANEL_ID = "session:alive-1"; /** * Build a minimal valid SerializedDockview-shaped object — matches what @@ -186,4 +193,185 @@ describe("sanitizeLayout - session panel handling", () => { expect(Object.keys(result.panels)).not.toContain(SESSION_PANEL_ID); expect(Object.keys(result.panels)).toContain("chat"); }); + + describe("excludeSessionIds (per-env restore filter)", () => { + function buildLayoutWithTwoSessions(sidA: string, sidB: string) { + const layout = buildLayout(); + layout.grid.root.data[1].data.views = [`session:${sidA}`, `session:${sidB}`]; + layout.grid.root.data[1].data.activeView = `session:${sidA}`; + layout.panels = { + files: { id: "files", contentComponent: "files" }, + chat: { id: "chat", contentComponent: "chat" }, + git: { id: "git", contentComponent: "git" }, + shell: { id: "shell", contentComponent: "shell" }, + [`session:${sidA}`]: { id: `session:${sidA}`, contentComponent: "chat" }, + [`session:${sidB}`]: { id: `session:${sidB}`, contentComponent: "chat" }, + }; + return layout; + } + + it("strips session panels whose ids are in excludeSessionIds (phantom panels from deleted tasks)", () => { + // Regression for "phantom panels from local/session storage": a stale + // session id from a previously-deleted task can end up serialized in an + // env-layout (e.g. via a debounced save firing during a task switch). + // The env restore must drop session panels we know belong to another env. + const aliveId = "alive-1"; + const phantomId = "phantom-deleted"; + const layout = buildLayoutWithTwoSessions(aliveId, phantomId); + const result = sanitizeLayout(layout, VALID_COMPONENTS, { + excludeSessionIds: new Set([phantomId]), + }); + expect(result).not.toBeNull(); + const alivePanel = `session:${aliveId}`; + expect(Object.keys(result.panels)).toContain(alivePanel); + expect(Object.keys(result.panels)).not.toContain(`session:${phantomId}`); + // The leaf's views list also drops the phantom. + const centerLeaf = result.grid.root.data[1]; + expect(centerLeaf.data.views).toEqual([alivePanel]); + expect(centerLeaf.data.activeView).toBe(alivePanel); + }); + + it("keeps unmapped session ids (preserves still-loading sessions across restore)", () => { + // A session id present in the saved layout that the store has not yet + // mapped (WS hasn't arrived) MUST be kept — otherwise the user loses + // their valid chat tab on load. Reconcile cleans it up later if stale. + const layout = buildLayoutWithTwoSessions("a", "b"); + const result = sanitizeLayout(layout, VALID_COMPONENTS, { + excludeSessionIds: new Set(), + }); + expect(result).not.toBeNull(); + expect(Object.keys(result.panels)).toContain("session:a"); + expect(Object.keys(result.panels)).toContain("session:b"); + }); + + it("ignores excludeSessionIds when undefined (preserves default per-env behavior)", () => { + // Without the option, behavior is unchanged: session panels are kept. + const layout = buildLayoutWithTwoSessions("a", "b"); + const result = sanitizeLayout(layout, VALID_COMPONENTS); + expect(result).not.toBeNull(); + expect(Object.keys(result.panels)).toContain("session:a"); + expect(Object.keys(result.panels)).toContain("session:b"); + }); + + it("rejects mixing stripSessionPanels with excludeSessionIds at the type level", () => { + // The two options are intent-mutually-exclusive; the type union enforces + // it so a caller can't silently get strip-wins behavior. + const layout = buildLayoutWithTwoSessions("a", "b"); + const result = sanitizeLayout(layout, VALID_COMPONENTS, { + stripSessionPanels: true, + // @ts-expect-error - the discriminated union forbids passing both + excludeSessionIds: new Set(["a"]), + }); + // Compile-time guard is the contract; this just asserts the call still + // returns a usable value so the test isn't a no-op at runtime. + expect(result).not.toBeNull(); + }); + }); +}); + +describe("tryRestoreLayout - phantom session panel filtering", () => { + beforeEach(() => { + vi.restoreAllMocks(); + }); + + function makeFakeApi() { + return { + fromJSON: vi.fn(), + layout: vi.fn(), + width: 1600, + height: 600, + groups: [], + panels: [], + activeGroup: { id: "g-center" }, + getPanel: vi.fn(() => null), + onDidActiveGroupChange: vi.fn(() => ({ dispose: vi.fn() })), + onDidLayoutChange: vi.fn(() => ({ dispose: vi.fn() })), + } as unknown as Parameters[0]; + } + + it("restores the (stripped) layout when every session in the saved env-layout is a phantom", () => { + // Saved layout had a single session panel — a phantom from a previously- + // deleted task. After stripping, no session panels remain. We still + // restore the (truncated) layout; useAutoSessionTab will add the active + // session's chat panel afterwards. + const layout = buildLayout(); + layout.grid.root.data[1].data.views = [PHANTOM_PANEL_ID]; + layout.grid.root.data[1].data.activeView = PHANTOM_PANEL_ID; + Object.assign(layout.panels, { + [PHANTOM_PANEL_ID]: { id: PHANTOM_PANEL_ID, contentComponent: "chat" }, + }); + vi.spyOn(localStorage, "getEnvLayout").mockReturnValue(layout); + vi.spyOn(localStorage, "getEnvMaximizeState").mockReturnValue(null); + + const api = makeFakeApi(); + const restored = tryRestoreLayout(api, "env-new", VALID_COMPONENTS, new Set(["phantom"])); + expect(restored).toBe(true); + expect(api.fromJSON).toHaveBeenCalledOnce(); + // The phantom was stripped from the layout passed to fromJSON. + const restoredLayout = (api.fromJSON as ReturnType).mock.calls[0][0]; + expect(Object.keys(restoredLayout.panels)).not.toContain(PHANTOM_PANEL_ID); + }); + + it("restores normally when at least one session panel survives the filter", () => { + const layout = buildLayout(); + layout.grid.root.data[1].data.views = [ALIVE_PANEL_ID, PHANTOM_PANEL_ID]; + layout.grid.root.data[1].data.activeView = ALIVE_PANEL_ID; + Object.assign(layout.panels, { + [ALIVE_PANEL_ID]: { id: ALIVE_PANEL_ID, contentComponent: "chat" }, + [PHANTOM_PANEL_ID]: { id: PHANTOM_PANEL_ID, contentComponent: "chat" }, + }); + vi.spyOn(localStorage, "getEnvLayout").mockReturnValue(layout); + vi.spyOn(localStorage, "getEnvMaximizeState").mockReturnValue(null); + + const api = makeFakeApi(); + const restored = tryRestoreLayout(api, "env-X", VALID_COMPONENTS, new Set(["phantom"])); + expect(restored).toBe(true); + expect(api.fromJSON).toHaveBeenCalledOnce(); + }); + + it("restores normally when the saved layout has no session panels at all (legit non-session env layout)", () => { + // An env may persist a layout with only files/terminal/etc. — no session + // panels to begin with. The fallback guard must NOT discard this layout. + const layout = buildLayout(); // center has "chat" only, no session:* ids + vi.spyOn(localStorage, "getEnvLayout").mockReturnValue(layout); + vi.spyOn(localStorage, "getEnvMaximizeState").mockReturnValue(null); + + const api = makeFakeApi(); + const restored = tryRestoreLayout(api, "env-X", VALID_COMPONENTS, new Set()); + expect(restored).toBe(true); + expect(api.fromJSON).toHaveBeenCalledOnce(); + }); +}); + +/** + * Regression: an env-layout could be restored with a session panel whose id + * referred to a previously-deleted task's session. The fix strips session + * panels we KNOW belong to a different env; sessions not yet mapped in the + * store are preserved (they may still be loading via WS). + */ +describe("collectPhantomSessionIdsForEnv", () => { + it("returns session ids whose mapping is a different env", () => { + const state = { + environmentIdBySessionId: { + "sess-1": "env-A", + "sess-2": "env-A", + "sess-3": "env-B", + }, + }; + expect(collectPhantomSessionIdsForEnv(state, "env-A")).toEqual(new Set(["sess-3"])); + expect(collectPhantomSessionIdsForEnv(state, "env-B")).toEqual(new Set(["sess-1", "sess-2"])); + }); + + it("returns every mapped session as phantom when the env has no own sessions yet", () => { + const state = { environmentIdBySessionId: { "sess-1": "env-A" } }; + expect(collectPhantomSessionIdsForEnv(state, "env-new")).toEqual(new Set(["sess-1"])); + }); + + it("does NOT classify a session as a phantom when its mapping is absent (still loading via WS)", () => { + // A session id present in a saved layout but not in environmentIdBySessionId + // could be a not-yet-arrived session for this very env. Keep it; reconcile + // will clean it up later if it really is stale. + const state = { environmentIdBySessionId: {} }; + expect(collectPhantomSessionIdsForEnv(state, "env-A")).toEqual(new Set()); + }); }); diff --git a/apps/web/components/task/dockview-layout-restore.ts b/apps/web/components/task/dockview-layout-restore.ts index 9fdd9a25d8..c9ec807a3f 100644 --- a/apps/web/components/task/dockview-layout-restore.ts +++ b/apps/web/components/task/dockview-layout-restore.ts @@ -1,9 +1,11 @@ import type { DockviewReadyEvent, SerializedDockview } from "dockview-react"; +import type { StoreApi } from "zustand"; import { useDockviewStore } from "@/lib/state/dockview-store"; import { applyLayoutFixups } from "@/lib/state/dockview-layout-builders"; import { isLayoutShapeHealthy } from "@/lib/state/dockview-layout-health"; import { measureDockviewContainer } from "@/lib/state/dockview-measure"; import type { LayoutState } from "@/lib/state/layout-manager"; +import type { AppState } from "@/lib/state/store"; import { getEnvLayout, getEnvMaximizeState, removeEnvMaximizeState } from "@/lib/local-storage"; const LAYOUT_STORAGE_KEY = "dockview-layout-v1"; @@ -12,7 +14,9 @@ const LAYOUT_STORAGE_KEY = "dockview-layout-v1"; export function sanitizeLayout( layout: any, validComponents: Set, - options: { stripSessionPanels?: boolean } = {}, + options: + | { stripSessionPanels: true; excludeSessionIds?: never } + | { stripSessionPanels?: false | undefined; excludeSessionIds?: Set } = {}, ): any { if (!isLayoutShapeHealthy(layout)) return null; @@ -29,6 +33,18 @@ export function sanitizeLayout( if (id.startsWith("session:")) { if (options.stripSessionPanels) { invalidIds.add(id); + } else if (options.excludeSessionIds) { + // Per-env restore: drop session panels that we know belong to a + // different env (a phantom from a previously-deleted task). Sessions + // we have no mapping for are kept — they may be a still-loading WS + // arrival, and useAutoSessionTab's reconcile will clean them up if + // they turn out to be stale. + const sid = id.slice("session:".length); + if (options.excludeSessionIds.has(sid)) { + invalidIds.add(id); + } else { + validPanels[id] = panel; + } } else { validPanels[id] = panel; } @@ -111,21 +127,37 @@ function tryRestoreMaximizeOnly(api: DockviewReadyEvent["api"], envId: string): } } +/** + * Restore the per-env saved layout, after sanitizing phantom session panels. + * Returns true on a successful fromJSON, false when no usable saved layout + * exists (caller falls through to maximize-only / global / default build). + */ +function tryRestoreEnvLayout( + api: DockviewReadyEvent["api"], + envId: string, + validComponents: Set, + phantomSessionIds: Set | undefined, +): boolean { + const envLayout = getEnvLayout(envId); + if (!envLayout) return false; + const sanitized = sanitizeLayout(envLayout, validComponents, { + excludeSessionIds: phantomSessionIds, + }); + if (!sanitized) return false; + api.fromJSON(sanitized as SerializedDockview); + applyFixupsWithMaximize(api, envId); + return true; +} + export function tryRestoreLayout( api: DockviewReadyEvent["api"], currentEnvId: string | null, validComponents: Set, + phantomSessionIds?: Set, ): boolean { if (currentEnvId) { try { - const envLayout = getEnvLayout(currentEnvId); - if (envLayout) { - const sanitized = sanitizeLayout(envLayout, validComponents); - if (!sanitized) return false; - api.fromJSON(sanitized as SerializedDockview); - applyFixupsWithMaximize(api, currentEnvId); - return true; - } + if (tryRestoreEnvLayout(api, currentEnvId, validComponents, phantomSessionIds)) return true; } catch { // fall through to maximize-only / global fallback } @@ -151,3 +183,39 @@ export function tryRestoreLayout( return false; } + +/** + * Collect session ids that DEFINITIVELY belong to a different env than `envId`. + * These are phantoms (typically from a previously-deleted task) that must be + * stripped on env-layout restore. + * + * Sessions absent from `environmentIdBySessionId` are NOT classified as + * phantoms — they may be a still-loading WS arrival that legitimately belongs + * to this env. `useAutoSessionTab`'s reconcile cleans up anything that turns + * out to be stale once the store catches up. + */ +export function collectPhantomSessionIdsForEnv( + state: { environmentIdBySessionId: Record }, + envId: string, +): Set { + const result = new Set(); + for (const [sessionId, mappedEnv] of Object.entries(state.environmentIdBySessionId)) { + if (mappedEnv && mappedEnv !== envId) result.add(sessionId); + } + return result; +} + +/** + * Restore the env's saved layout, stripping session panels that we KNOW + * belong to a different env — guards against phantom panels from + * previously-deleted tasks resurfacing on restore. + */ +export function restoreEnvLayout( + api: DockviewReadyEvent["api"], + envId: string | null, + appStore: StoreApi, + validComponents: Set, +): boolean { + const phantoms = envId ? collectPhantomSessionIdsForEnv(appStore.getState(), envId) : undefined; + return tryRestoreLayout(api, envId, validComponents, phantoms); +} diff --git a/apps/web/lib/state/dockview-env-switch.test.ts b/apps/web/lib/state/dockview-env-switch.test.ts index 5e6b2f25f0..a289d6ab00 100644 --- a/apps/web/lib/state/dockview-env-switch.test.ts +++ b/apps/web/lib/state/dockview-env-switch.test.ts @@ -204,6 +204,47 @@ describe("performEnvSwitch", () => { }); }); +describe("performEnvSwitch slow-path stale session strip", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("closes stale session chat panels after the slow-path fromJSON", () => { + // Regression: a saved env layout could carry a `session:*` panel from a + // previously-deleted task (phantom). On the slow-path restore, that + // panel would land in the live api as a stray tab. removeStaleSessionPanels + // must close any session:* panel whose id != the incoming active session. + vi.mocked(getEnvLayout) + .mockReturnValueOnce(makeHealthyLayoutWith({})) + .mockReturnValueOnce(makeHealthyLayoutWith({})); + // Force the slow path: layouts don't structurally match. + vi.mocked(savedLayoutMatchesLive).mockReturnValueOnce(false); + + const closeStale = vi.fn(); + const closeFileEditor = vi.fn(); + const closeKeep = vi.fn(); + const api = { + ...makeMockApi(), + // api.fromJSON is a no-op mock; populate `panels` with what would exist + // post-restore so removeStaleSessionPanels' filter has something to act on. + panels: [ + { id: "session:old-session", api: { component: "chat", close: closeStale } }, + { id: NEW_SESSION_PANEL_ID, api: { component: "chat", close: closeKeep } }, + // file editors are NOT session panels — they must NOT be closed. + { id: "preview:file-editor", api: { component: "file-editor", close: closeFileEditor } }, + ], + } as unknown as EnvSwitchParams["api"]; + const params = makeParams({ api }); + + performEnvSwitch(params); + + expect(closeStale).toHaveBeenCalledOnce(); + expect(closeKeep).not.toHaveBeenCalled(); + expect(closeFileEditor).not.toHaveBeenCalled(); + expect(params.api.fromJSON).toHaveBeenCalledOnce(); + }); +}); + describe("performEnvSwitch fast-path active view restoration", () => { beforeEach(() => { vi.clearAllMocks(); diff --git a/apps/web/lib/state/dockview-env-switch.ts b/apps/web/lib/state/dockview-env-switch.ts index 1863e10aaa..0529d4349e 100644 --- a/apps/web/lib/state/dockview-env-switch.ts +++ b/apps/web/lib/state/dockview-env-switch.ts @@ -147,6 +147,30 @@ function removeEphemeralPanels(api: DockviewApi, keepSessionId: string | null): } } +/** + * Close stale session chat panels — any `session:*` panel whose id isn't + * `session:${keepSessionId}`. Used after `fromJSON` in the slow path to + * strip phantom sessions carried in from a saved layout, WITHOUT touching + * file-editors/diffs/browser/etc. that legitimately belong to this env. + */ +function removeStaleSessionPanels(api: DockviewApi, keepSessionId: string | null): void { + const keepId = keepSessionId ? `session:${keepSessionId}` : null; + // keepId=null (sessionless task) → strips all session panels, unlike the + // fast path's shouldRemoveDuringSwitch which keeps them. In practice + // sessionless tasks should have no session panels; useAutoSessionTab + // re-adds the panel when a session arrives. + const toRemove = api.panels.filter( + (p) => p.api.component === "chat" && p.id.startsWith("session:") && p.id !== keepId, + ); + for (const p of toRemove) { + try { + p.api.close(); + } catch { + /* panel may already be gone */ + } + } +} + /** * Given the panels of a group and the id of the panel being replaced, return * the target tab index for the replacement among the siblings that will @@ -257,7 +281,7 @@ function addIncomingSessionPanel( * env-scoped portals before calling this function. */ export function performEnvSwitch(params: EnvSwitchParams): LayoutGroupIds { - const { api, newEnvId, safeWidth, safeHeight, buildDefault } = params; + const { api, newEnvId, activeSessionId, safeWidth, safeHeight, buildDefault } = params; const fastResult = tryFastEnvSwitch(params); if (fastResult) return fastResult; @@ -266,6 +290,12 @@ export function performEnvSwitch(params: EnvSwitchParams): LayoutGroupIds { if (saved) { try { api.fromJSON(saved as SerializedDockview); + // Saved layout may carry a stale session panel from a previously-deleted + // task (phantom). Strip session panels that don't belong to the incoming + // active session — file editors/diffs/etc. were legitimately part of + // this env's saved state and must NOT be touched. + // useAutoSessionTab will add the current session's panel if missing. + removeStaleSessionPanels(api, activeSessionId); api.layout(safeWidth, safeHeight); return applyLayoutFixups(api); } catch (err) {