From 4277ef495a56eac982d87ee0eb1a35c0024e7285 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Wed, 29 Apr 2026 09:56:26 +0100 Subject: [PATCH] fix(web): self-recover dockview layout from corrupt persisted state (#729) --- .../task/dockview-layout-restore.test.ts | 140 ++++++++++++ .../task/dockview-layout-restore.ts | 3 +- .../task/task-select-helpers.test.ts | 56 +++++ .../components/task/task-select-helpers.ts | 30 +++ .../components/task/task-session-sidebar.tsx | 22 +- apps/web/e2e/pages/session-page.ts | 35 +++ .../task/sessionless-task-switch.spec.ts | 191 ++++++++++++++++ apps/web/lib/state/dockview-layout-health.ts | 46 ++++ .../lib/state/dockview-session-switch.test.ts | 39 +++- apps/web/lib/state/dockview-session-switch.ts | 16 +- apps/web/lib/state/dockview-store.ts | 20 ++ .../dockview-empty-task-recovery/plan.md | 212 ++++++++++++++++++ 12 files changed, 791 insertions(+), 19 deletions(-) create mode 100644 apps/web/components/task/dockview-layout-restore.test.ts create mode 100644 apps/web/components/task/task-select-helpers.test.ts create mode 100644 apps/web/components/task/task-select-helpers.ts create mode 100644 apps/web/e2e/tests/task/sessionless-task-switch.spec.ts create mode 100644 apps/web/lib/state/dockview-layout-health.ts create mode 100644 docs/specs/dockview-empty-task-recovery/plan.md diff --git a/apps/web/components/task/dockview-layout-restore.test.ts b/apps/web/components/task/dockview-layout-restore.test.ts new file mode 100644 index 0000000000..7bb03a639d --- /dev/null +++ b/apps/web/components/task/dockview-layout-restore.test.ts @@ -0,0 +1,140 @@ +import { describe, it, expect } from "vitest"; +import { sanitizeLayout } from "./dockview-layout-restore"; + +const VALID_COMPONENTS = new Set(["chat", "files", "shell", "git"]); + +/** + * Build a minimal valid SerializedDockview-shaped object — matches what + * dockview's api.toJSON() produces for a 3-column layout (sidebar | center | right). + */ +function buildLayout(opts?: { centerSize?: number; sidebarSize?: number; rightSize?: number }) { + return { + grid: { + root: { + type: "branch" as const, + size: 600, + data: [ + { + type: "leaf" as const, + size: opts?.sidebarSize ?? 350, + data: { id: "g-sidebar", views: ["files"], activeView: "files" }, + }, + { + type: "leaf" as const, + size: opts?.centerSize ?? 800, + data: { id: "g-center", views: ["chat"], activeView: "chat" }, + }, + { + type: "leaf" as const, + size: opts?.rightSize ?? 450, + data: { id: "g-right", views: ["git", "shell"], activeView: "git" }, + }, + ], + }, + height: 600, + width: 1600, + orientation: "HORIZONTAL" as const, + }, + panels: { + files: { id: "files", contentComponent: "files" }, + chat: { id: "chat", contentComponent: "chat" }, + git: { id: "git", contentComponent: "git" }, + shell: { id: "shell", contentComponent: "shell" }, + }, + activeGroup: "g-center", + }; +} + +describe("sanitizeLayout - size validation", () => { + it("returns the layout unchanged when all sizes are positive", () => { + const layout = buildLayout(); + const result = sanitizeLayout(layout, VALID_COMPONENTS); + expect(result).not.toBeNull(); + expect(result.grid.root.data).toHaveLength(3); + }); + + it("returns null when a leaf node has size 0", () => { + const layout = buildLayout({ centerSize: 0 }); + const result = sanitizeLayout(layout, VALID_COMPONENTS); + expect(result).toBeNull(); + }); + + it("returns null when a leaf node has negative size", () => { + const layout = buildLayout({ sidebarSize: -50 }); + const result = sanitizeLayout(layout, VALID_COMPONENTS); + expect(result).toBeNull(); + }); + + it("returns null when a branch node has size 0", () => { + const layout = buildLayout(); + layout.grid.root.size = 0; + const result = sanitizeLayout(layout, VALID_COMPONENTS); + expect(result).toBeNull(); + }); + + it("returns null when nested branch has invalid size", () => { + const layout = { + grid: { + root: { + type: "branch" as const, + size: 800, + data: [ + { + type: "leaf" as const, + size: 350, + data: { id: "g-sidebar", views: ["files"], activeView: "files" }, + }, + { + type: "branch" as const, + size: 0, + data: [ + { + type: "leaf" as const, + size: 800, + data: { id: "g-center", views: ["chat"], activeView: "chat" }, + }, + ], + }, + ], + }, + height: 600, + width: 1600, + orientation: "HORIZONTAL" as const, + }, + panels: { + files: { id: "files", contentComponent: "files" }, + chat: { id: "chat", contentComponent: "chat" }, + }, + activeGroup: "g-center", + }; + const result = sanitizeLayout(layout, VALID_COMPONENTS); + expect(result).toBeNull(); + }); + + it("returns null when grid.width is 0", () => { + const layout = buildLayout(); + layout.grid.width = 0; + const result = sanitizeLayout(layout, VALID_COMPONENTS); + expect(result).toBeNull(); + }); + + it("returns null when grid.height is 0", () => { + const layout = buildLayout(); + layout.grid.height = 0; + const result = sanitizeLayout(layout, VALID_COMPONENTS); + expect(result).toBeNull(); + }); + + it("preserves existing component-validation behavior alongside size checks", () => { + const layout = buildLayout(); + // Add an unknown panel — sanitizer should remove it but keep the rest. + layout.panels = { + ...layout.panels, + // @ts-expect-error - injecting an extra unknown panel + unknown: { id: "unknown", contentComponent: "unknown-component" }, + }; + const result = sanitizeLayout(layout, VALID_COMPONENTS); + expect(result).not.toBeNull(); + expect(Object.keys(result.panels)).not.toContain("unknown"); + }); +}); diff --git a/apps/web/components/task/dockview-layout-restore.ts b/apps/web/components/task/dockview-layout-restore.ts index 3436b905ab..d9c87dd9ed 100644 --- a/apps/web/components/task/dockview-layout-restore.ts +++ b/apps/web/components/task/dockview-layout-restore.ts @@ -1,13 +1,14 @@ import type { DockviewReadyEvent, SerializedDockview } from "dockview-react"; import { useDockviewStore } from "@/lib/state/dockview-store"; import { applyLayoutFixups } from "@/lib/state/dockview-layout-builders"; +import { isLayoutShapeHealthy } from "@/lib/state/dockview-layout-health"; import { getSessionLayout, getSessionMaximizeState } from "@/lib/local-storage"; const LAYOUT_STORAGE_KEY = "dockview-layout-v1"; // eslint-disable-next-line @typescript-eslint/no-explicit-any export function sanitizeLayout(layout: any, validComponents: Set): any { - if (!layout?.panels || !layout?.grid?.root) return null; + if (!isLayoutShapeHealthy(layout)) return null; const invalidIds = new Set(); // eslint-disable-next-line @typescript-eslint/no-explicit-any diff --git a/apps/web/components/task/task-select-helpers.test.ts b/apps/web/components/task/task-select-helpers.test.ts new file mode 100644 index 0000000000..2224b03332 --- /dev/null +++ b/apps/web/components/task/task-select-helpers.test.ts @@ -0,0 +1,56 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { finalizeNoSessionSelect, type FinalizeNoSessionSelectDeps } from "./task-select-helpers"; + +const NEW_TASK_ID = "task-new"; +const OLD_SESSION_ID = "old-session"; + +function makeDeps(overrides?: Partial): FinalizeNoSessionSelectDeps { + return { + setActiveTask: vi.fn(), + releaseLayoutToDefault: vi.fn(), + replaceTaskUrl: vi.fn(), + ...overrides, + }; +} + +describe("finalizeNoSessionSelect", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("clears the dockview to a default layout, releasing the outgoing session", () => { + const deps = makeDeps(); + finalizeNoSessionSelect(NEW_TASK_ID, OLD_SESSION_ID, deps); + expect(deps.releaseLayoutToDefault).toHaveBeenCalledWith(OLD_SESSION_ID); + }); + + it("releases the layout even when there is no prior session", () => { + const deps = makeDeps(); + finalizeNoSessionSelect(NEW_TASK_ID, null, deps); + // We still need to reset the dockview so the new task starts from a clean + // default layout — passing null lets the store fall back to its current + // layout session id internally. + expect(deps.releaseLayoutToDefault).toHaveBeenCalledWith(null); + }); + + it("sets the new active task and replaces the URL", () => { + const deps = makeDeps(); + finalizeNoSessionSelect(NEW_TASK_ID, OLD_SESSION_ID, deps); + expect(deps.setActiveTask).toHaveBeenCalledWith(NEW_TASK_ID); + expect(deps.replaceTaskUrl).toHaveBeenCalledWith(NEW_TASK_ID); + }); + + it("releases the layout BEFORE setting the new active task", () => { + // Order matters — releasing the layout depends on the still-active session + // for portal cleanup. If we cleared activeSessionId first the release + // would target the wrong (already-cleared) session. + const calls: string[] = []; + const deps = makeDeps({ + releaseLayoutToDefault: vi.fn(() => calls.push("release")), + setActiveTask: vi.fn(() => calls.push("setActiveTask")), + replaceTaskUrl: vi.fn(() => calls.push("replaceTaskUrl")), + }); + finalizeNoSessionSelect(NEW_TASK_ID, OLD_SESSION_ID, deps); + expect(calls).toEqual(["release", "setActiveTask", "replaceTaskUrl"]); + }); +}); diff --git a/apps/web/components/task/task-select-helpers.ts b/apps/web/components/task/task-select-helpers.ts new file mode 100644 index 0000000000..26f2d4a0fa --- /dev/null +++ b/apps/web/components/task/task-select-helpers.ts @@ -0,0 +1,30 @@ +/** + * Helpers for task selection in the sidebar. Extracted as pure functions so + * the no-session fallback path can be unit-tested without standing up the + * dockview runtime. + */ + +export type FinalizeNoSessionSelectDeps = { + /** Set the new active task in the kanban store (also clears activeSessionId). */ + setActiveTask: (taskId: string) => void; + /** Save the outgoing session's layout, release its portals, then build the default layout. */ + releaseLayoutToDefault: (oldSessionId: string | null) => void; + /** Push the new task id into the URL without reloading. */ + replaceTaskUrl: (taskId: string) => void; +}; + +/** + * Finalize a sidebar task selection when no session could be resolved or + * launched for the new task. Releasing the dockview to default first ensures + * portal cleanup targets the still-active outgoing session before + * `setActiveTask` clears `activeSessionId` to null. + */ +export function finalizeNoSessionSelect( + taskId: string, + oldSessionId: string | null, + deps: FinalizeNoSessionSelectDeps, +): void { + deps.releaseLayoutToDefault(oldSessionId); + deps.setActiveTask(taskId); + deps.replaceTaskUrl(taskId); +} diff --git a/apps/web/components/task/task-session-sidebar.tsx b/apps/web/components/task/task-session-sidebar.tsx index 6666690ff4..d9090e4487 100644 --- a/apps/web/components/task/task-session-sidebar.tsx +++ b/apps/web/components/task/task-session-sidebar.tsx @@ -17,7 +17,12 @@ import { replaceTaskUrl } from "@/lib/links"; import { useAllWorkflowSnapshots } from "@/hooks/domains/kanban/use-all-workflow-snapshots"; import { useTaskActions, useArchiveAndSwitchTask } from "@/hooks/use-task-actions"; import { useTaskRemoval } from "@/hooks/use-task-removal"; -import { performLayoutSwitch, useDockviewStore } from "@/lib/state/dockview-store"; +import { + performLayoutSwitch, + releaseLayoutToDefault, + useDockviewStore, +} from "@/lib/state/dockview-store"; +import { finalizeNoSessionSelect } from "./task-select-helpers"; import { INTENT_PR_REVIEW } from "@/lib/state/layout-manager"; import { launchSession } from "@/lib/services/session-launch-service"; import { buildPrepareRequest } from "@/lib/services/session-launch-helpers"; @@ -461,15 +466,24 @@ function useSidebarActions(store: StoreApi) { replaceTaskUrl(taskId); return; } - // No session — prepare workspace and switch to it + // No session — prepare workspace and switch to it. If preparation + // fails to launch a session, fall back to a clean default layout + // instead of leaving the dockview pointing at the outgoing session. const switched = await prepareAndSwitchTask( taskId, store, switchToSession, setPreparingTaskId, ); - if (!switched) setActiveTask(taskId); - replaceTaskUrl(taskId); + if (switched) { + replaceTaskUrl(taskId); + } else { + finalizeNoSessionSelect(taskId, currentOldSessionId, { + setActiveTask, + releaseLayoutToDefault, + replaceTaskUrl, + }); + } }); }, [loadTaskSessionsForTask, switchToSession, setActiveTask, store], diff --git a/apps/web/e2e/pages/session-page.ts b/apps/web/e2e/pages/session-page.ts index 1f65f4594c..5cc0586a9a 100644 --- a/apps/web/e2e/pages/session-page.ts +++ b/apps/web/e2e/pages/session-page.ts @@ -492,6 +492,41 @@ export class SessionPage { await this.expectNoLayoutGap(); } + /** + * Assert the live dockview groups all have positive widths and that the sum + * of the root-level column widths is approximately equal to the api width. + * Catches "central group has zero/wrong width" corruption that persists + * across task switches when a corrupted layout is saved to per-session storage. + */ + async expectLayoutHealthy(): Promise { + const result = await this.page.evaluate(() => { + type Group = { id: string; width: number; height: number }; + type Api = { width: number; height: number; groups: Group[] }; + const api = (window as unknown as { __dockviewApi__?: Api }).__dockviewApi__; + if (!api) return { error: "dockview api not exposed" }; + const bad = api.groups.filter((g) => !(g.width > 1)); + const totalWidth = api.groups.reduce((s, g) => s + (g.width > 0 ? g.width : 0), 0); + return { + apiWidth: api.width, + groups: api.groups.map((g) => ({ id: g.id, width: g.width })), + badCount: bad.length, + totalWidth, + }; + }); + expect(result.error, result.error).toBeUndefined(); + expect( + result.badCount, + `Found ${result.badCount} dockview groups with width <= 1: ${JSON.stringify(result.groups)}`, + ).toBe(0); + // Sum of group widths should match api width within a small rounding tolerance. + // Note: groups can be stacked vertically so totalWidth may exceed apiWidth (one column, + // multiple groups) — only flag if totalWidth is much smaller than apiWidth (squished). + expect( + result.totalWidth! >= (result.apiWidth ?? 0) - 4, + `Total group widths (${result.totalWidth}) much smaller than api width (${result.apiWidth}): ${JSON.stringify(result.groups)}`, + ).toBe(true); + } + /** * Assert the dockview layout columns fill the container with no large empty gap. * Catches bugs where columns don't expand after api.fromJSON() + setConstraints diff --git a/apps/web/e2e/tests/task/sessionless-task-switch.spec.ts b/apps/web/e2e/tests/task/sessionless-task-switch.spec.ts new file mode 100644 index 0000000000..82bfef8e14 --- /dev/null +++ b/apps/web/e2e/tests/task/sessionless-task-switch.spec.ts @@ -0,0 +1,191 @@ +import { test, expect } from "../../fixtures/test-base"; +import { SessionPage } from "../../pages/session-page"; + +/** + * Bug: when a task is created via MCP `create_task_kandev` with + * `start_agent=false`, it has no session/environment. Selecting that task in + * the sidebar previously left the dockview layout corrupted (wrong widths on + * the central group) and did not update the breadcrumb to the new task. The + * corruption then persisted across subsequent task switches because the bad + * layout was saved back to per-session storage. + * + * This test reproduces the failing flow end-to-end and asserts: + * 1. Selecting the sessionless subtask updates the URL + breadcrumb. + * 2. The dockview layout remains healthy (no zero-width groups). + * 3. Switching back to the parent task keeps the layout healthy + * (regression on persisted-corruption). + */ + +const SUBTASK_TITLE = "Sessionless MCP Subtask"; + +test.describe("Sessionless task switching", () => { + test("selecting an MCP-created sessionless subtask keeps layout healthy", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(90_000); + + // Parent task agent script: create a subtask via MCP with start_agent=false. + const script = [ + 'e2e:thinking("Creating sessionless subtask...")', + "e2e:delay(100)", + `e2e:mcp:kandev:create_task_kandev({"parent_id":"self","title":"${SUBTASK_TITLE}","description":"Sessionless subtask for layout-recovery test","start_agent":false})`, + "e2e:delay(100)", + 'e2e:message("Done.")', + ].join("\n"); + + const parent = await apiClient.createTaskWithAgent( + seedData.workspaceId, + "Sessionless Parent Task", + seedData.agentProfileId, + { + description: script, + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }, + ); + + await testPage.goto(`/t/${parent.id}`); + const session = new SessionPage(testPage); + await session.waitForLoad(); + + // Wait for parent agent to settle so the layout has stabilised. + await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); + + // Poll the API until the MCP-created subtask exists. We need its ID for URL + // assertions. The subtask is created by the agent asynchronously, so the + // idle indicator alone isn't a guarantee. + type TaskEntry = { id: string; title: string; primary_session_id?: string | null }; + let subtask: TaskEntry | undefined; + await expect + .poll( + async () => { + const all = await apiClient.listTasks(seedData.workspaceId); + subtask = all.tasks.find((t: TaskEntry) => t.title === SUBTASK_TITLE); + return subtask?.id ?? null; + }, + { timeout: 30_000, message: "Waiting for MCP-created subtask to appear" }, + ) + .toBeTruthy(); + const subtaskId = subtask!.id; + // Sanity: the bug requires a sessionless task. start_agent=false implies no + // primary session was created on the backend. + expect(subtask!.primary_session_id ?? null).toBeNull(); + + // Baseline: layout is healthy on the parent. + await session.expectLayoutHealthy(); + + // Wait for the subtask to appear in the sidebar, then click it. + const subtaskRow = session.taskInSidebar(SUBTASK_TITLE); + await expect(subtaskRow).toBeVisible({ timeout: 10_000 }); + await session.clickTaskInSidebar(SUBTASK_TITLE); + + // URL should switch to the subtask within a reasonable time. + await expect(testPage).toHaveURL(new RegExp(`/t/${subtaskId}(?:\\?|$)`), { + timeout: 10_000, + }); + + // Breadcrumb (task title in the topbar) must reflect the new task. + const breadcrumb = testPage.locator('[aria-current="page"]'); + await expect(breadcrumb).toHaveText(SUBTASK_TITLE, { timeout: 10_000 }); + + // Layout must remain healthy on the sessionless task. + await session.expectLayoutHealthy(); + + // Switch back to the parent task. The bug surfaces here: persisted bad + // layout state corrupts the central-group widths on this second switch. + await session.clickTaskInSidebar("Sessionless Parent Task"); + await expect(testPage).toHaveURL(new RegExp(`/t/${parent.id}(?:\\?|$)`), { + timeout: 10_000, + }); + await expect(breadcrumb).toHaveText("Sessionless Parent Task", { timeout: 10_000 }); + await session.expectLayoutHealthy(); + }); + + test("self-recovers when sessionStorage holds a corrupted layout for a known session", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(90_000); + + // Two tasks with agents — A navigated first, then B, then back to A. The + // back-to-A switch goes through performSessionSwitch's slow path which + // historically applied the saved (potentially corrupt) blob verbatim. + const taskA = await apiClient.createTaskWithAgent( + seedData.workspaceId, + "Layout Recovery Task A", + seedData.agentProfileId, + { + description: "/e2e:simple-message", + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }, + ); + const taskB = await apiClient.createTaskWithAgent( + seedData.workspaceId, + "Layout Recovery Task B", + seedData.agentProfileId, + { + description: "/e2e:simple-message", + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }, + ); + + await testPage.goto(`/t/${taskA.id}`); + const session = new SessionPage(testPage); + await session.waitForLoad(); + await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); + await session.expectLayoutHealthy(); + + // Resolve A's session id so we can target it precisely. + type TaskEntry = { id: string; primary_session_id?: string | null }; + let sessionAId: string | null = null; + await expect + .poll( + async () => { + const all = await apiClient.listTasks(seedData.workspaceId); + sessionAId = + all.tasks.find((x: TaskEntry) => x.id === taskA.id)?.primary_session_id ?? null; + return sessionAId; + }, + { timeout: 15_000, message: "Waiting for task A session id" }, + ) + .toBeTruthy(); + + // Switch to B, then back — this primes both layouts in sessionStorage. + await session.clickTaskInSidebar("Layout Recovery Task B"); + await expect(testPage).toHaveURL(new RegExp(`/t/${taskB.id}(?:\\?|$)`), { timeout: 10_000 }); + await session.expectLayoutHealthy(); + + // Inject a corrupt layout for A while we're focused on B. + await testPage.evaluate((sid) => { + const corrupt = { + grid: { + root: { + type: "leaf", + size: 800, + data: { id: "g1", views: ["chat"], activeView: "chat" }, + }, + height: 600, + width: 0, // <-- corruption + orientation: "HORIZONTAL", + }, + panels: { chat: { id: "chat", contentComponent: "chat" } }, + activeGroup: "g1", + }; + window.sessionStorage.setItem(`kandev.dockview.layout.${sid}`, JSON.stringify(corrupt)); + }, sessionAId!); + + // Switch back to A — performSessionSwitch should drop the corrupt blob + // and rebuild the default instead of applying the zero-width layout. + await session.clickTaskInSidebar("Layout Recovery Task A"); + await expect(testPage).toHaveURL(new RegExp(`/t/${taskA.id}(?:\\?|$)`), { timeout: 10_000 }); + await session.expectLayoutHealthy(); + }); +}); diff --git a/apps/web/lib/state/dockview-layout-health.ts b/apps/web/lib/state/dockview-layout-health.ts new file mode 100644 index 0000000000..c04976f192 --- /dev/null +++ b/apps/web/lib/state/dockview-layout-health.ts @@ -0,0 +1,46 @@ +/** + * Layout-shape health validation. A persisted dockview layout can become + * corrupted (zero/negative `size` on a node, missing views, zero container + * dimensions) — applying it via api.fromJSON() yields collapsed groups the + * user can't recover from without clearing sessionStorage. + * + * Both the initial-mount restore path and the session-switch path consult + * this validator before handing data to dockview, so any code that loads a + * layout from storage can guard against persistent corruption. + */ + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +function hasValidSizes(node: any): boolean { + if (!node || typeof node !== "object") return false; + if (node.size !== undefined && !(typeof node.size === "number" && node.size > 0)) { + return false; + } + if (node.type === "leaf") { + const views = node.data?.views; + return Array.isArray(views) && views.length > 0; + } + if (node.type === "branch") { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const children = node.data as any[] | undefined; + if (!Array.isArray(children) || children.length === 0) return false; + return children.every(hasValidSizes); + } + return true; +} + +/** + * Returns true when the serialized dockview layout has a healthy structure: + * - container has positive width/height + * - every grid node has a positive `size` + * - every leaf has at least one view + * - top-level `panels` and `grid.root` are present + */ +// eslint-disable-next-line @typescript-eslint/no-explicit-any +export function isLayoutShapeHealthy(layout: any): boolean { + if (!layout?.panels || !layout?.grid?.root) return false; + const { width, height } = layout.grid; + if (!(typeof width === "number" && width > 0) || !(typeof height === "number" && height > 0)) { + return false; + } + return hasValidSizes(layout.grid.root); +} diff --git a/apps/web/lib/state/dockview-session-switch.test.ts b/apps/web/lib/state/dockview-session-switch.test.ts index fd44a08652..b0c19f4ec7 100644 --- a/apps/web/lib/state/dockview-session-switch.test.ts +++ b/apps/web/lib/state/dockview-session-switch.test.ts @@ -34,6 +34,27 @@ function makeMockApi() { } as unknown as SessionSwitchParams["api"]; } +/** Build a SerializedDockview-shaped fixture that passes isLayoutShapeHealthy. */ +function makeHealthyLayoutWith(extraPanels: Record) { + return { + grid: { + root: { + type: "leaf" as const, + size: 800, + data: { id: "g1", views: ["chat"], activeView: "chat" }, + }, + height: 600, + width: 800, + orientation: "HORIZONTAL" as const, + }, + panels: { + chat: { contentComponent: "chat" }, + ...extraPanels, + }, + activeGroup: "g1", + } as unknown as ReturnType; +} + function makeParams(overrides?: Partial): SessionSwitchParams { return { api: makeMockApi(), @@ -62,9 +83,7 @@ describe("performSessionSwitch", () => { }); it("calls api.layout on the fast path when saved layout matches", () => { - vi.mocked(getSessionLayout).mockReturnValueOnce({ grid: {} } as unknown as ReturnType< - typeof getSessionLayout - >); + vi.mocked(getSessionLayout).mockReturnValueOnce(makeHealthyLayoutWith({})); vi.mocked(savedLayoutMatchesLive).mockReturnValueOnce(true); const params = makeParams(); @@ -105,10 +124,9 @@ describe("performSessionSwitch", () => { }); it("skips fast path when saved layout has ephemeral panels (file-editor)", () => { - const savedLayout = { - grid: {}, - panels: { "preview:file-editor": { contentComponent: "file-editor" } }, - } as unknown as ReturnType; + const savedLayout = makeHealthyLayoutWith({ + "preview:file-editor": { contentComponent: "file-editor" }, + }); vi.mocked(getSessionLayout).mockReturnValueOnce(savedLayout).mockReturnValueOnce(savedLayout); vi.mocked(savedLayoutMatchesLive).mockReturnValueOnce(true); const params = makeParams(); @@ -120,10 +138,9 @@ describe("performSessionSwitch", () => { }); it("skips fast path when saved layout has ephemeral panels (diff-viewer)", () => { - const savedLayout = { - grid: {}, - panels: { "preview:file-diff": { contentComponent: "diff-viewer" } }, - } as unknown as ReturnType; + const savedLayout = makeHealthyLayoutWith({ + "preview:file-diff": { contentComponent: "diff-viewer" }, + }); vi.mocked(getSessionLayout).mockReturnValueOnce(savedLayout).mockReturnValueOnce(savedLayout); vi.mocked(savedLayoutMatchesLive).mockReturnValueOnce(true); const params = makeParams(); diff --git a/apps/web/lib/state/dockview-session-switch.ts b/apps/web/lib/state/dockview-session-switch.ts index ede49dfa21..707c01e68a 100644 --- a/apps/web/lib/state/dockview-session-switch.ts +++ b/apps/web/lib/state/dockview-session-switch.ts @@ -7,11 +7,19 @@ import type { DockviewApi, SerializedDockview } from "dockview-react"; import { getSessionLayout } from "@/lib/local-storage"; import { applyLayoutFixups } from "./dockview-layout-builders"; +import { isLayoutShapeHealthy } from "./dockview-layout-health"; import { fromDockviewApi, savedLayoutMatchesLive, layoutStructuresMatch } from "./layout-manager"; import type { LayoutState, LayoutGroupIds } from "./layout-manager"; const EPHEMERAL_COMPONENTS = new Set(["file-editor", "diff-viewer", "commit-detail"]); +/** Fetch the saved layout for a session, dropping it if its shape is corrupted. */ +function getHealthySessionLayout(sessionId: string): object | null { + const saved = getSessionLayout(sessionId); + if (!saved) return null; + return isLayoutShapeHealthy(saved) ? saved : null; +} + /** Check whether a serialized dockview layout contains ephemeral panels. */ function savedLayoutHasEphemeralPanels(serialized: SerializedDockview): boolean { // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -77,7 +85,7 @@ function removeEphemeralPanels(api: DockviewApi, keepSessionId?: string): void { function tryFastSessionSwitch(params: SessionSwitchParams): LayoutGroupIds | null { const { api, newSessionId, getDefaultLayout } = params; const currentLayout = fromDockviewApi(api); - const saved = getSessionLayout(newSessionId); + const saved = getHealthySessionLayout(newSessionId); let structuresMatch = false; if (saved) { @@ -153,8 +161,10 @@ export function performSessionSwitch(params: SessionSwitchParams): LayoutGroupId const fastResult = tryFastSessionSwitch(params); if (fastResult) return fastResult; - // Slow path: full layout rebuild via fromJSON - const saved = getSessionLayout(newSessionId); + // Slow path: full layout rebuild via fromJSON. Use the validating loader + // so a corrupted blob from a previous failed switch is dropped instead of + // applied verbatim (which would collapse the central group). + const saved = getHealthySessionLayout(newSessionId); if (saved) { try { api.fromJSON(saved as SerializedDockview); diff --git a/apps/web/lib/state/dockview-store.ts b/apps/web/lib/state/dockview-store.ts index 3e20f1daa5..3faf65198f 100644 --- a/apps/web/lib/state/dockview-store.ts +++ b/apps/web/lib/state/dockview-store.ts @@ -681,3 +681,23 @@ export const useDockviewStore = create((set, get) => ({ export function performLayoutSwitch(oldSessionId: string | null, newSessionId: string): void { useDockviewStore.getState().switchSessionLayout(oldSessionId, newSessionId); } + +/** + * Release the dockview to a clean default layout — used when selecting a task + * that has no session (and prepare failed to launch one). Without this the + * dockview keeps the outgoing session's panels live but disconnected from any + * active session, and the corrupted state can be persisted on the next save. + */ +export function releaseLayoutToDefault(oldSessionId: string | null): void { + const { api, currentLayoutSessionId, preMaximizeLayout, buildDefaultLayout } = + useDockviewStore.getState(); + if (!api) return; + const effectiveOld = oldSessionId ?? currentLayoutSessionId; + saveOutgoingSession(api, effectiveOld, preMaximizeLayout); + useDockviewStore.setState({ + preMaximizeLayout: null, + maximizedGroupId: null, + currentLayoutSessionId: null, + }); + buildDefaultLayout(api); +} diff --git a/docs/specs/dockview-empty-task-recovery/plan.md b/docs/specs/dockview-empty-task-recovery/plan.md new file mode 100644 index 0000000000..e47ca6e892 --- /dev/null +++ b/docs/specs/dockview-empty-task-recovery/plan.md @@ -0,0 +1,212 @@ +# Plan — Dockview Recovery for Sessionless Task Switches + +> Implementation plan for the `dockview-empty-task-recovery` spec. +> Workflow: TDD (Red → Green → Refactor) per `.claude/skills/tdd/SKILL.md`. + +## 1. Overview + +Tasks created via the kandev MCP tool with `start_agent=false` have no session/environment. +Selecting such a task in the task-details sidebar today causes: + +1. The Dockview layout to corrupt (wrong widths on the central group). +2. The top bar (task title, branch) to stay on the previous task. +3. The corruption to persist across subsequent task switches because the broken + layout is saved back to per-session storage. + +Goals: + +- Make sessionless task selection update the active task, URL, and Dockview layout deterministically. +- Detect a corrupted Dockview layout after restore and self-heal by rebuilding the default. +- Cover with one E2E test (mock-agent + MCP `create_task_kandev` with `start_agent=false`) + and targeted unit tests on the restore + switch logic. + +Out of scope: + +- Refactoring `task-session-sidebar.tsx` beyond what the fix requires. +- Changing how layouts are persisted to storage. +- Touching backend MCP behavior beyond the existing `start_agent=false` path (already correct). + +## 2. Prerequisites + +- No DB migrations or config changes. +- The mock-agent's `e2e:mcp:kandev:create_task_kandev(...)` already works + (`apps/backend/cmd/mock-agent/script.go:120`, `executeMCPCommand`); the new test + exercises that path with `start_agent: false`. +- `__dockviewApi__` is exposed on `window` for E2E inspection + (`apps/web/lib/state/dockview-store.ts:614`). + +## 3. Implementation Steps + +### Step 1 — Failing E2E: sessionless-task selection (RED) + +Description: write the spec that reproduces the bug end-to-end. + +- **File (new)**: `apps/web/e2e/tests/task/sessionless-task-switch.spec.ts` +- Flow: + 1. Seed task A with an agent (`apiClient.createTaskWithAgent`, + `description: "/e2e:simple-message"`). + 2. Navigate to task A; wait for `session.idleInput()` and stable layout. + 3. Use the agent to create task B sessionless via mock script: + `e2e:mcp:kandev:create_task_kandev({"parent_id":"self","title":"Empty Task","description":"…","start_agent":false})` + (mock-agent forwards to the real MCP handler). + 4. Click task B in the sidebar via `session.clickTaskInSidebar("Empty Task")`. + 5. Assert breadcrumb shows "Empty Task" within 10s. + 6. Assert URL matches `/t/`. + 7. Assert Dockview central group width ≥ a sane minimum (use a new helper, + see Step 2). Specifically: no group in the live grid has `width <= 1` and + the sum of root children widths ≈ `api.width`. + 8. Click task A back: `idleInput()` becomes visible again and central group + widths remain valid (regression on layout-corruption persistence). + +Expectation: this test fails on `main` (title stays on A, central group width is broken). + +### Step 2 — E2E helper: layout sanity assertion + +Description: add a small helper to assert no Dockview groups have zero/near-zero width. + +- **File (modify)**: `apps/web/e2e/pages/session-page.ts` +- Add method `expectLayoutHealthy()` that does a `page.evaluate` on `__dockviewApi__`, + walking `api.groups` and asserting `g.width > 1` for every group, and that + `Math.abs(sum(rootChildrenWidth) - api.width) <= 4` (rounding tolerance). +- Reuse `expectNoLayoutGap()` patterns where possible — keep additions <40 lines. + +### Step 3 — Failing unit test: `tryRestoreLayout` rejects corrupted layouts (RED) + +Description: pin the contract that a structurally-broken saved layout is discarded. + +- **File (new)**: `apps/web/components/task/dockview-layout-restore.test.ts` +- Cases: + - Returns `false` when `getSessionLayout` yields a layout where any leaf has + `views: []` after sanitization (covered today by `cleanNode` returning null, + keep as a regression). + - **NEW failing case**: returns `false` when the sanitized layout's + `grid.root.size <= 0` or any branch child has `size <= 0` (zero-width groups). + - Mock `getSessionLayout` from `@/lib/local-storage` and `applyLayoutFixups` + from `@/lib/state/dockview-layout-builders` (mirror existing + `dockview-session-switch.test.ts` mock style). + +### Step 4 — Implement layout sanity check in `sanitizeLayout` (GREEN) + +Description: extend the existing sanitizer to reject grids with zero/missing sizes, +so a corrupted persisted layout is dropped instead of restored. + +- **File (modify)**: `apps/web/components/task/dockview-layout-restore.ts` +- Add an internal `hasValidSizes(node): boolean` walker: + - For `leaf`: `typeof node.size === "number" && node.size > 0` (or size missing + and parent is the root single-leaf — handle by treating undefined size at root + as valid since dockview infers it from container width). + - For `branch`: every child `hasValidSizes` and `children.length > 0`. +- In `sanitizeLayout`, after `cleanNode` produces `cleanedRoot`, return `null` + when `!hasValidSizes(cleanedRoot)`. +- This causes `tryRestoreLayout` to fall through to the default-build path. + +### Step 5 — Failing unit test: sidebar select-task always updates store + URL for sessionless tasks (RED) + +Description: lock down `handleSelectTask`'s contract for the sessionless branch. + +- **File (new)**: `apps/web/components/task/task-session-sidebar-select.test.ts` +- Extract the pure decision into a testable helper (see Step 6) and assert: + - When `prepareAndSwitchTask` resolves `false` (no session created), the helper + invokes `setActiveTask(taskId)` AND calls `performLayoutSwitch` with the old + session and a sentinel "reset" path (or equivalent: triggers + `resetLayout()` — see Step 6). + - URL is replaced exactly once (mock `replaceTaskUrl`). + - The `oldSessionId` captured before `launchSession` is the one passed to + layout-switch (regression on the comment at lines 322–325). + +### Step 6 — Implement sessionless-switch fix (GREEN) + +Description: when no session can be created, explicitly tear down the prior +session's layout instead of leaving it stale. + +- **File (modify)**: `apps/web/components/task/task-session-sidebar.tsx` + - In `prepareAndSwitchTask` (lines 316–345), when `launchSession` returns no + `session_id` OR throws: before returning `false`, call a new helper + `releaseLayoutForTask(oldSessionId)` that: + - Saves the outgoing session's layout (already handled by + `switchSessionLayout` when passed both args), then + - Calls `useDockviewStore.getState().resetLayout()` which builds the default + layout via `buildDefaultLayout(api)` (`dockview-store.ts:664–667`). + - In `handleSelectTask` (lines 445–476), the `if (!switched) setActiveTask(taskId)` + branch must run BEFORE the next paint (it already does, but ensure + `replaceTaskUrl` is called in both branches — currently fine). +- **File (modify)**: `apps/web/lib/state/dockview-store.ts` — no API change; we + reuse `resetLayout`. If a small helper export is cleaner, add + `releaseSessionLayout(oldSessionId)` that wraps `saveOutgoingSession` + + `resetLayout` and is called from the sidebar. + +### Step 7 — Optional backend test (only if needed) + +Description: only add if Step 1's E2E reveals the mock-agent path doesn't propagate +`start_agent=false` correctly. Otherwise skip — the existing +`TestCreateTask_ToolSchema_HasParentID` already locks the schema. + +- **File (modify)**: `apps/backend/internal/agentctl/server/mcp/handlers_test.go` + - Add `TestCreateTask_StartAgentFalse_DoesNotAutoStart` that asserts the + forwarded payload includes `start_agent: false` when the tool arg is `false`. + +### Step 8 — Verify (REFACTOR + sign-off) + +- Run from `apps/`: + - `pnpm --filter @kandev/web test -- dockview-layout-restore` (unit). + - `pnpm --filter @kandev/web test -- task-session-sidebar-select` (unit). + - `pnpm --filter @kandev/web e2e -- sessionless-task-switch` (E2E). +- Run `make -C apps/backend test` if Step 7 was needed. +- Run `make fmt` then `make typecheck test lint` per AGENTS.md. + +## 4. File Changes Summary + +Created: + +- `apps/web/e2e/tests/task/sessionless-task-switch.spec.ts` +- `apps/web/components/task/dockview-layout-restore.test.ts` +- `apps/web/components/task/task-session-sidebar-select.test.ts` + +Modified: + +- `apps/web/components/task/dockview-layout-restore.ts` — add `hasValidSizes` and + reject corrupted grids in `sanitizeLayout`. +- `apps/web/components/task/task-session-sidebar.tsx` — extract testable helper; + release layout when sessionless task is selected. +- `apps/web/lib/state/dockview-store.ts` — optional `releaseSessionLayout` export + (only if it improves call-site clarity). +- `apps/web/e2e/pages/session-page.ts` — add `expectLayoutHealthy()` helper. +- `apps/backend/internal/agentctl/server/mcp/handlers_test.go` — only if Step 7 + is required. + +Deleted: none. + +## 5. Testing Strategy + +Unit (vitest): + +- `dockview-layout-restore.test.ts`: sanitizer rejects zero-size branches/leaves; + sanitizer keeps healthy layouts unchanged (snapshot-style structural compare). +- `task-session-sidebar-select.test.ts`: extracted `prepareAndSwitchTask` helper + releases layout + falls back to `setActiveTask` when launch fails; captured + `oldSessionId` is forwarded to layout-switch. + +E2E (Playwright, `KANDEV_MOCK_AGENT=only`): + +- `sessionless-task-switch.spec.ts`: covers the full bug scenario including + return-trip to the original task. + +Manual smoke (one-off): + +1. Start kandev in dev mode. +2. From the agent, run `create_task_kandev` with `start_agent=false`. +3. Click the new task in the sidebar — confirm top bar updates and the central + group keeps a sane width. +4. Click back to the agent task — confirm layout restored without artefacts. + +## 6. Rollback Plan + +- All changes are confined to frontend modules and one optional backend test. +- Revert by `git revert` of the merge commit; no DB migrations or persisted-format + changes (the sanitizer is strictly more conservative — it only rejects layouts + the app already cannot render). + +## 7. Estimated Effort + +- Complexity: **medium** (multi-layer fix: store + restore sanitizer + sidebar handler). +- Time: ~0.5–1 day including TDD cycle and CI fixups.