fix(web): self-recover dockview layout from corrupt persisted state (#729)
This commit is contained in:
@@ -0,0 +1,140 @@
|
||||
import { describe, it, expect } from "vitest";
|
||||
import { sanitizeLayout } from "./dockview-layout-restore";
|
||||
|
||||
const VALID_COMPONENTS = new Set<string>(["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");
|
||||
});
|
||||
});
|
||||
@@ -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<string>): any {
|
||||
if (!layout?.panels || !layout?.grid?.root) return null;
|
||||
if (!isLayoutShapeHealthy(layout)) return null;
|
||||
|
||||
const invalidIds = new Set<string>();
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
|
||||
@@ -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>): 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"]);
|
||||
});
|
||||
});
|
||||
@@ -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);
|
||||
}
|
||||
@@ -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],
|
||||
|
||||
@@ -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<void> {
|
||||
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
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
@@ -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);
|
||||
}
|
||||
@@ -34,6 +34,27 @@ function makeMockApi() {
|
||||
} as unknown as SessionSwitchParams["api"];
|
||||
}
|
||||
|
||||
/** Build a SerializedDockview-shaped fixture that passes isLayoutShapeHealthy. */
|
||||
function makeHealthyLayoutWith(extraPanels: Record<string, { contentComponent: string }>) {
|
||||
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<typeof getSessionLayout>;
|
||||
}
|
||||
|
||||
function makeParams(overrides?: Partial<SessionSwitchParams>): 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<typeof getSessionLayout>;
|
||||
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<typeof getSessionLayout>;
|
||||
const savedLayout = makeHealthyLayoutWith({
|
||||
"preview:file-diff": { contentComponent: "diff-viewer" },
|
||||
});
|
||||
vi.mocked(getSessionLayout).mockReturnValueOnce(savedLayout).mockReturnValueOnce(savedLayout);
|
||||
vi.mocked(savedLayoutMatchesLive).mockReturnValueOnce(true);
|
||||
const params = makeParams();
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -681,3 +681,23 @@ export const useDockviewStore = create<DockviewStore>((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);
|
||||
}
|
||||
|
||||
@@ -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/<taskBId>`.
|
||||
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.
|
||||
Reference in New Issue
Block a user