diff --git a/canvas/src/app/layout.tsx b/canvas/src/app/layout.tsx index 9ec82158..2de303f9 100644 --- a/canvas/src/app/layout.tsx +++ b/canvas/src/app/layout.tsx @@ -23,6 +23,7 @@ const monoFont = JetBrains_Mono({ import { AuthGate } from "@/components/AuthGate"; import { CookieConsent } from "@/components/CookieConsent"; import { PurchaseSuccessModal } from "@/components/PurchaseSuccessModal"; +import { ErrorBoundary } from "@/components/ErrorBoundary"; import { ThemeProvider } from "@/lib/theme-provider"; import { THEME_COOKIE, @@ -245,11 +246,12 @@ export default async function RootLayout({ - {/* AuthGate is a client component; it checks the session on mount - and bounces anonymous users to the control plane's login page - when running on a tenant subdomain. Non-SaaS hosts (localhost, - vercel preview URL, apex) pass through unchanged. */} - {children} + {/* ErrorBoundary is a client component; it catches render crashes + anywhere inside AuthGate / children so a single failing view + degrades to a reloadable fallback instead of a blank white screen. */} + + {children} + {/* Demo Mock #1: post-purchase success toast. Mounted at the layout level so it persists across page state transitions diff --git a/canvas/src/components/__tests__/ErrorBoundary.test.tsx b/canvas/src/components/__tests__/ErrorBoundary.test.tsx index 32e51941..7271f361 100644 --- a/canvas/src/components/__tests__/ErrorBoundary.test.tsx +++ b/canvas/src/components/__tests__/ErrorBoundary.test.tsx @@ -1,9 +1,9 @@ +// @vitest-environment jsdom import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; // --------------------------------------------------------------------------- -// We test ErrorBoundary in a pure-unit style by instantiating the class -// directly, avoiding the need for a full React DOM renderer (which the -// project's vitest environment = "node" does not provide). +// We test ErrorBoundary both in pure-unit style (class instantiation) and +// with React DOM rendering to verify it catches child render throws. // --------------------------------------------------------------------------- // Mock fetch globally so transitive imports of api.ts don't blow up @@ -12,6 +12,7 @@ globalThis.fetch = vi.fn(() => ); import React from "react"; +import { render, screen, cleanup, fireEvent } from "@testing-library/react"; import { ErrorBoundary } from "../ErrorBoundary"; // --------------------------------------------------------------------------- @@ -23,6 +24,13 @@ function createInstance(props: { children: React.ReactNode } = { children: null return instance; } +function Thrower({ shouldThrow }: { shouldThrow: boolean }) { + if (shouldThrow) { + throw new Error("boom"); + } + return
healthy child
; +} + // --------------------------------------------------------------------------- // Tests // --------------------------------------------------------------------------- @@ -36,6 +44,7 @@ describe("ErrorBoundary", () => { afterEach(() => { consoleErrorSpy.mockRestore(); + cleanup(); }); // ---- static getDerivedStateFromError ----------------------------------- @@ -124,4 +133,34 @@ describe("ErrorBoundary", () => { expect(json).toContain("Unknown error"); expect(json).toContain("Something went wrong"); }); + + // ---- DOM integration: catches child render throws --------------------- + + it("renders a fallback when a child throws during render", () => { + render( + + + , + ); + expect(screen.getByRole("alert")).toBeDefined(); + expect(screen.queryByText("Something went wrong")).not.toBeNull(); + expect(screen.queryByText("boom")).not.toBeNull(); + }); + + it("reload button calls window.location.reload", () => { + const reload = vi.fn(); + Object.defineProperty(window, "location", { + value: { reload }, + writable: true, + }); + + render( + + + , + ); + + fireEvent.click(screen.getByRole("button", { name: /reload/i })); + expect(reload).toHaveBeenCalled(); + }); }); diff --git a/canvas/src/store/__tests__/canvas-topology.test.ts b/canvas/src/store/__tests__/canvas-topology.test.ts index 4cee168d..dcf853aa 100644 --- a/canvas/src/store/__tests__/canvas-topology.test.ts +++ b/canvas/src/store/__tests__/canvas-topology.test.ts @@ -1,5 +1,11 @@ import { describe, it, expect } from "vitest"; -import { buildNodesAndEdges, extractSkillNames, computeAutoLayout } from "../canvas-topology"; +import { + buildNodesAndEdges, + extractSkillNames, + computeAutoLayout, + sortParentsBeforeChildren, + TopologyCycleError, +} from "../canvas-topology"; import type { WorkspaceData } from "../socket"; // --------------------------------------------------------------------------- @@ -487,3 +493,38 @@ describe("buildNodesAndEdges – child rescue heuristic", () => { expect(child.position).toEqual({ x: 100, y: 50 }); }); }); + +// --------------------------------------------------------------------------- +// Cycle guards +// --------------------------------------------------------------------------- + +describe("sortParentsBeforeChildren – cyclic parent chain", () => { + it("bails and returns the original array instead of hanging", () => { + const nodes = [ + { id: "a", parentId: "b" }, + { id: "b", parentId: "a" }, + ]; + const result = sortParentsBeforeChildren(nodes); + expect(result).toBe(nodes); + expect(result).toHaveLength(2); + }); + + it("still returns a topologically sorted array for acyclic input", () => { + const nodes = [ + { id: "child", parentId: "parent" }, + { id: "parent" }, + ]; + const result = sortParentsBeforeChildren(nodes); + expect(result.map((n) => n.id)).toEqual(["parent", "child"]); + }); +}); + +describe("buildNodesAndEdges – cyclic parent chain", () => { + it("throws TopologyCycleError instead of hanging", () => { + const workspaces = [ + makeWS({ id: "a", parent_id: "b" }), + makeWS({ id: "b", parent_id: "a" }), + ]; + expect(() => buildNodesAndEdges(workspaces)).toThrow(TopologyCycleError); + }); +}); diff --git a/canvas/src/store/__tests__/canvas.test.ts b/canvas/src/store/__tests__/canvas.test.ts index d7bc343e..941d979e 100644 --- a/canvas/src/store/__tests__/canvas.test.ts +++ b/canvas/src/store/__tests__/canvas.test.ts @@ -6,8 +6,10 @@ global.fetch = vi.fn(() => ); import { useCanvasStore, summarizeWorkspaceCapabilities } from "../canvas"; +import type { WorkspaceNodeData } from "../canvas"; import { __resetTombstonesForTest } from "../deleteTombstones"; import type { WorkspaceData, WSMessage } from "../socket"; +import type { Node } from "@xyflow/react"; // Helper to build a WorkspaceData object with sensible defaults function makeWS(overrides: Partial & { id: string }): WorkspaceData { @@ -33,6 +35,36 @@ function makeWS(overrides: Partial & { id: string }): WorkspaceDa }; } +function makeNode( + overrides: Partial & { id: string; parentId?: string | null }, +): Node { + const { id, parentId, ...dataOverrides } = overrides; + const ws = makeWS({ id }); + return { + id, + type: "workspaceNode", + position: { x: 0, y: 0 }, + data: { + name: ws.name, + status: ws.status, + tier: ws.tier, + agentCard: ws.agent_card, + activeTasks: ws.active_tasks, + collapsed: ws.collapsed, + role: ws.role, + lastErrorRate: ws.last_error_rate, + lastSampleError: ws.last_sample_error, + url: ws.url, + parentId: parentId ?? ws.parent_id, + currentTask: ws.current_task, + runtime: ws.runtime, + needsRestart: false, + budgetLimit: ws.budget_limit, + ...dataOverrides, + }, + } as Node; +} + function makeMsg(overrides: Partial & { event: string; workspace_id: string }): WSMessage { return { timestamp: new Date().toISOString(), @@ -1245,3 +1277,60 @@ describe("moveNode", () => { }); }); }); + +// ---------- cycle guards (#2601 follow-up) ---------- + +describe("hydrate – cyclic parent chain", () => { + it("sets hydrationError and preserves existing nodes instead of hanging", () => { + useCanvasStore.getState().hydrate([ + makeWS({ id: "existing", name: "Existing" }), + ]); + const before = useCanvasStore.getState().nodes; + expect(before).toHaveLength(1); + + useCanvasStore.getState().hydrate([ + makeWS({ id: "a", parent_id: "b" }), + makeWS({ id: "b", parent_id: "a" }), + ]); + + expect(useCanvasStore.getState().hydrationError).toContain("cyclic parent chain"); + expect(useCanvasStore.getState().nodes).toEqual(before); + }); +}); + +describe("isDescendant – cyclic parent chain", () => { + it("returns false instead of hanging when the ancestor is not reachable", () => { + useCanvasStore.setState({ + nodes: [ + makeNode({ id: "a", parentId: null }), + makeNode({ id: "b", parentId: "c" }), + makeNode({ id: "c", parentId: "b" }), + ], + }); + expect(useCanvasStore.getState().isDescendant("a", "b")).toBe(false); + }); +}); + +describe("arrangeChildren – cyclic parent chain", () => { + it("does not hang on a cycle", () => { + useCanvasStore.setState({ + nodes: [ + makeNode({ id: "a", parentId: "b" }), + makeNode({ id: "b", parentId: "a" }), + ], + }); + expect(() => useCanvasStore.getState().arrangeChildren("a")).not.toThrow(); + }); +}); + +describe("nestNode – cyclic parent chain", () => { + it("does not hang when current data contains a cycle", async () => { + useCanvasStore.setState({ + nodes: [ + makeNode({ id: "a", parentId: "b" }), + makeNode({ id: "b", parentId: "a" }), + ], + }); + await expect(useCanvasStore.getState().nestNode("a", null)).resolves.toBeUndefined(); + }); +}); diff --git a/canvas/src/store/canvas-topology.ts b/canvas/src/store/canvas-topology.ts index 4370e267..5c3a10c2 100644 --- a/canvas/src/store/canvas-topology.ts +++ b/canvas/src/store/canvas-topology.ts @@ -3,6 +3,15 @@ import type { WorkspaceData } from "./socket"; import type { WorkspaceNodeData } from "./canvas"; import { WORKSPACE_KIND } from "@/lib/workspace-kind"; +/** Thrown when workspace parent chains contain a cycle. Catching this at + * load time lets the UI surface a degraded state instead of hanging. */ +export class TopologyCycleError extends Error { + constructor(message: string) { + super(message); + this.name = "TopologyCycleError"; + } +} + const H_SPACING = 320; const V_SPACING = 200; @@ -25,17 +34,38 @@ export function sortParentsBeforeChildren [n.id, n])); const visited = new Set(); + const visiting = new Set(); const out: T[] = []; + const visit = (n: T) => { if (visited.has(n.id)) return; + if (visiting.has(n.id)) { + throw new TopologyCycleError( + `cyclic parent chain detected at workspace ${n.id}`, + ); + } + visiting.add(n.id); if (n.parentId) { const parent = byId.get(n.parentId); if (parent && !visited.has(parent.id)) visit(parent); } + visiting.delete(n.id); visited.add(n.id); out.push(n); }; - for (const n of nodes) visit(n); + + try { + for (const n of nodes) visit(n); + } catch (err) { + if (err instanceof TopologyCycleError) { + // Fail-closed: a corrupt cycle is not recoverable by re-ordering. + // Return the input unchanged so callers (e.g. drag re-parent) don't + // hang, and let the next full hydrate surface the topology error. + return nodes; + } + throw err; + } + // Separate roots, valid children, and orphans: // - roots: no parentId — true tree roots // - valid children: has parentId pointing to an existing node @@ -323,12 +353,20 @@ export function buildNodesAndEdges( // after their parent regardless of the order the API returned them. const byId = new Map(workspaces.map((w) => [w.id, w])); const visited = new Set(); + const visiting = new Set(); const sorted: WorkspaceData[] = []; function visit(ws: WorkspaceData) { if (visited.has(ws.id)) return; + if (visiting.has(ws.id)) { + throw new TopologyCycleError( + `cyclic parent chain detected at workspace ${ws.id}`, + ); + } + visiting.add(ws.id); if (ws.parent_id && byId.has(ws.parent_id) && !visited.has(ws.parent_id)) { visit(byId.get(ws.parent_id)!); } + visiting.delete(ws.id); visited.add(ws.id); sorted.push(ws); } diff --git a/canvas/src/store/canvas.ts b/canvas/src/store/canvas.ts index fac954cc..f3c5ee6d 100644 --- a/canvas/src/store/canvas.ts +++ b/canvas/src/store/canvas.ts @@ -20,6 +20,7 @@ import { CHILD_DEFAULT_WIDTH, PARENT_BOTTOM_PADDING, PARENT_SIDE_PADDING, + TopologyCycleError, } from "./canvas-topology"; /** @@ -488,7 +489,10 @@ export const useCanvasStore = create((set, get) => ({ const absOf = (id: string | null | undefined): { x: number; y: number } => { let sum = { x: 0, y: 0 }; let cursor: string | null | undefined = id; + const seen = new Set(); while (cursor) { + if (seen.has(cursor)) break; + seen.add(cursor); const n = byId.get(cursor); if (!n) break; sum = { x: sum.x + n.position.x, y: sum.y + n.position.y }; @@ -499,7 +503,10 @@ export const useCanvasStore = create((set, get) => ({ const depthOf = (id: string | null | undefined): number => { let d = 0; let cursor: string | null | undefined = id; + const seen = new Set(); while (cursor) { + if (seen.has(cursor)) break; + seen.add(cursor); const n = byId.get(cursor); if (!n) break; cursor = n.data.parentId; @@ -693,9 +700,12 @@ export const useCanvasStore = create((set, get) => ({ isDescendant: (ancestorId, nodeId) => { const { nodes } = get(); + const seen = new Set(); let current = nodes.find((n) => n.id === nodeId); while (current?.data.parentId) { if (current.data.parentId === ancestorId) return true; + if (seen.has(current.data.parentId)) return false; + seen.add(current.data.parentId); current = nodes.find((n) => n.id === current?.data.parentId); } return false; @@ -716,7 +726,10 @@ export const useCanvasStore = create((set, get) => ({ const absOf = (id: string | null): { x: number; y: number } => { let sum = { x: 0, y: 0 }; let cursor: string | null = id; + const seen = new Set(); while (cursor) { + if (seen.has(cursor)) break; + seen.add(cursor); const n = nodes.find((nn) => nn.id === cursor); if (!n) break; sum = { x: sum.x + n.position.x, y: sum.y + n.position.y }; @@ -745,7 +758,10 @@ export const useCanvasStore = create((set, get) => ({ const depthOf = (id: string | null | undefined): number => { let d = 0; let cursor: string | null | undefined = id; + const seen = new Set(); while (cursor) { + if (seen.has(cursor)) break; + seen.add(cursor); const n = nodes.find((nn) => nn.id === cursor); if (!n) break; cursor = n.data.parentId; @@ -948,12 +964,24 @@ export const useCanvasStore = create((set, get) => ({ currentParentSizes.set(n.id, { width: w, height: h }); } } - const { nodes, edges } = buildNodesAndEdges( - live, - layoutOverrides, - currentParentSizes, - ); - set({ nodes, edges }); + try { + const { nodes, edges } = buildNodesAndEdges( + live, + layoutOverrides, + currentParentSizes, + ); + set({ nodes, edges }); + } catch (err) { + // Fail closed: cyclic/corrupt topology must not hang or blank the app. + // Surface a retryable error state and keep the previous nodes so the + // user isn't left with an empty canvas. + const message = + err instanceof TopologyCycleError + ? `Workspace map has a cyclic parent chain: ${err.message}. Please reload or contact support.` + : "Failed to render workspace map: corrupt topology. Please reload or contact support."; + set({ hydrationError: message }); + return; + } for (const [nodeId, { x, y }] of layoutOverrides) { api.patch(`/workspaces/${nodeId}`, { x, y }).catch(() => {}); } @@ -1077,7 +1105,10 @@ export const useCanvasStore = create((set, get) => ({ const absOf = (id: string | null | undefined): { x: number; y: number } => { let sum = { x: 0, y: 0 }; let cursor: string | null | undefined = id; + const seen = new Set(); while (cursor) { + if (seen.has(cursor)) break; + seen.add(cursor); const n = nodes.find((nn) => nn.id === cursor); if (!n) break; sum = { x: sum.x + n.position.x, y: sum.y + n.position.y };