diff --git a/apps/server/src/shared/mcp/whiteboard-tools.ts b/apps/server/src/shared/mcp/whiteboard-tools.ts index 36e4e6ee..286925fb 100644 --- a/apps/server/src/shared/mcp/whiteboard-tools.ts +++ b/apps/server/src/shared/mcp/whiteboard-tools.ts @@ -145,7 +145,7 @@ Text font families: 1=Virgil (handwritten), 3=Cascadia (monospace), 5=Excalifont } \`\`\` -**Points:** Array of [x, y] offsets relative to the element's x, y. First point is always [0, 0]. Add intermediate points for bends. +**Points (required):** Array of at least two [x, y] offsets relative to the element's x, y. First point is always [0, 0]. Add intermediate points for bends. Arrows, lines, and freedraw elements must always carry a \`points\` array — omit it and the element cannot be rendered. **Arrowheads:** \`startArrowhead\` and \`endArrowhead\` can be: null, "arrow", "bar", "dot", "triangle", "diamond". @@ -191,8 +191,10 @@ Children are assigned to frames by setting their \`frameId\` to the frame's id. ### Important notes -- The editor auto-heals many issues (null fields, missing indices). Don't over-validate. +- The editor auto-heals many cosmetic issues (null fields, missing indices). Don't over-validate those. - Always provide \`id\`, \`type\`, \`x\`, \`y\` at minimum. Width and height default to 0 if omitted. +- For \`arrow\`, \`line\`, and \`freedraw\`, \`points\` is mandatory — it is not auto-healed. +- \`freedraw\` also needs a \`pressures\` entry per point (0–1), or \`simulatePressure: true\` to let the editor derive them. - Use readable, descriptive ids — you'll reference them in bindings and future updates. - Elements are merged by id: sending an element with an existing id replaces it entirely. `; diff --git a/apps/server/src/shared/whiteboard-store.ts b/apps/server/src/shared/whiteboard-store.ts index 742bb70d..a127d1f0 100644 --- a/apps/server/src/shared/whiteboard-store.ts +++ b/apps/server/src/shared/whiteboard-store.ts @@ -1,5 +1,7 @@ import type { Pool } from "pg"; +import { sanitizeElements } from "./whiteboard.js"; + export const WHITEBOARD_SNAPSHOT_FILENAME = "whiteboard.png"; export const MAX_ELEMENTS = 20_000; @@ -21,7 +23,12 @@ export async function loadWhiteboard( "SELECT scene, version, updated_by, updated_at FROM whiteboards WHERE agent_id = $1", [agentId] ); - return result.rows[0] ?? null; + const row = result.rows[0]; + if (!row) return null; + // Sanitize on read too, so boards already holding malformed elements render + // instead of crashing, and heal on their next write. + const elements = Array.isArray(row.scene?.elements) ? row.scene.elements : []; + return { ...row, scene: { elements: sanitizeElements(elements) } }; } export function isValidScene(scene: unknown): scene is { elements: unknown[] } { @@ -40,6 +47,7 @@ export async function saveWhiteboard( baseVersion: number, updatedBy: "user" | "agent" ): Promise<{ version: number } | null> { + const clean = { elements: sanitizeElements(scene.elements) }; const result = await pool.query<{ version: string }>( `INSERT INTO whiteboards (agent_id, scene, version, updated_by) VALUES ($1, $2::jsonb, 1, $3) @@ -50,7 +58,7 @@ export async function saveWhiteboard( updated_at = NOW() WHERE whiteboards.version = $4 RETURNING version`, - [agentId, JSON.stringify(scene), updatedBy, baseVersion] + [agentId, JSON.stringify(clean), updatedBy, baseVersion] ); if (result.rows.length === 0) { return null; diff --git a/apps/server/src/shared/whiteboard.ts b/apps/server/src/shared/whiteboard.ts index c59a9bc5..2e5b2886 100644 --- a/apps/server/src/shared/whiteboard.ts +++ b/apps/server/src/shared/whiteboard.ts @@ -35,6 +35,78 @@ export type WhiteboardUpdateResult = { elements: SimplifiedElement[]; }; +// Excalidraw's restoreElements() calls isInvisiblySmallElement() — which reads +// `element.points.length` unguarded — before restoreElement() applies its own +// defaults. A points-less arrow therefore throws and takes down the whole view, +// so we normalize these fields before the data ever reaches the editor. +const POINTS_REQUIRED_TYPES = new Set(["arrow", "line", "draw", "freedraw"]); + +function finite(value: unknown): number { + return typeof value === "number" && Number.isFinite(value) ? value : 0; +} + +function isPoint(value: unknown): boolean { + return ( + Array.isArray(value) && + value.length >= 2 && + Number.isFinite(value[0]) && + Number.isFinite(value[1]) + ); +} + +export function sanitizeElement(raw: unknown): RawElement | null { + if (typeof raw !== "object" || raw === null) return null; + const el = raw as RawElement; + if (typeof el.id !== "string" || typeof el.type !== "string") return null; + + const width = finite(el.width); + const height = finite(el.height); + const out: RawElement = { + ...el, + x: finite(el.x), + y: finite(el.y), + width, + height, + }; + + if (POINTS_REQUIRED_TYPES.has(el.type)) { + const points = Array.isArray(el.points) ? el.points.filter(isPoint) : []; + // Same fallback restoreElement() would apply, just early enough to matter. + const usedFallback = points.length < 2; + out.points = usedFallback + ? [ + [0, 0], + [width, height], + ] + : points; + + // The freedraw renderer indexes pressures[i] per point when + // simulatePressure is falsy, so a missing or short pressures array throws + // the same way missing points did. Default to 0.5 as Excalidraw's own + // restore does. + if (el.type === "freedraw" && el.simulatePressure !== true) { + const pressures = Array.isArray(el.pressures) ? el.pressures : []; + out.pressures = (out.points as unknown[]).map((_, i) => { + const pressure = usedFallback ? undefined : pressures[i]; + return typeof pressure === "number" && Number.isFinite(pressure) + ? pressure + : 0.5; + }); + } + } + + return out; +} + +export function sanitizeElements(elements: unknown[]): unknown[] { + const out: RawElement[] = []; + for (const raw of elements) { + const el = sanitizeElement(raw); + if (el) out.push(el); + } + return out; +} + function num(value: unknown): number { return typeof value === "number" && Number.isFinite(value) ? Math.round(value) diff --git a/apps/server/test/whiteboard.test.ts b/apps/server/test/whiteboard.test.ts index c8cc3e04..aa9b625d 100644 --- a/apps/server/test/whiteboard.test.ts +++ b/apps/server/test/whiteboard.test.ts @@ -1,6 +1,9 @@ import { describe, expect, it, vi } from "vitest"; -import { simplifyElements } from "../src/shared/whiteboard.js"; +import { + sanitizeElements, + simplifyElements, +} from "../src/shared/whiteboard.js"; import { isValidScene, MAX_ELEMENTS } from "../src/shared/whiteboard-store.js"; // ── mergeElements is not exported, so we test it indirectly through @@ -134,6 +137,226 @@ describe("simplifyElements", () => { }); }); +// ── sanitizeElements ── + +describe("sanitizeElements", () => { + type El = Record; + + it("gives a points-less arrow points derived from width/height", () => { + // Regression: Excalidraw's isInvisiblySmallElement() reads + // `element.points.length` before restoreElement() supplies a default. + const [el] = sanitizeElements([ + { + id: "s1-s2-arrow", + type: "arrow", + x: -20, + y: 875, + width: 50, + height: 2, + }, + ]) as El[]; + expect(el.points).toEqual([ + [0, 0], + [50, 2], + ]); + }); + + it("supplies points for line and freedraw too", () => { + for (const type of ["line", "freedraw"]) { + const [el] = sanitizeElements([ + { id: `x-${type}`, type, x: 0, y: 0, width: 10, height: 20 }, + ]) as El[]; + expect(el.points).toEqual([ + [0, 0], + [10, 20], + ]); + } + }); + + it("preserves valid points", () => { + const points = [ + [0, 0], + [30, 10], + [60, 0], + ]; + const [el] = sanitizeElements([ + { id: "a", type: "arrow", x: 0, y: 0, width: 60, height: 0, points }, + ]) as El[]; + expect(el.points).toEqual(points); + }); + + it("replaces malformed or too-short points arrays", () => { + const [short] = sanitizeElements([ + { + id: "a", + type: "arrow", + x: 0, + y: 0, + width: 5, + height: 5, + points: [[0, 0]], + }, + ]) as El[]; + expect(short.points).toEqual([ + [0, 0], + [5, 5], + ]); + + const [junk] = sanitizeElements([ + { + id: "b", + type: "arrow", + x: 0, + y: 0, + width: 5, + height: 5, + points: ["nope", null, [1]], + }, + ]) as El[]; + expect(junk.points).toEqual([ + [0, 0], + [5, 5], + ]); + }); + + it("does not add points to shape elements", () => { + const [el] = sanitizeElements([rect("a")]) as El[]; + expect(el.points).toBeUndefined(); + }); + + it("coerces non-finite geometry to 0", () => { + const [el] = sanitizeElements([ + { id: "a", type: "rectangle", x: "10", y: null, width: NaN, height: 5 }, + ]) as El[]; + expect(el).toMatchObject({ x: 0, y: 0, width: 0, height: 5 }); + }); + + it("drops non-objects and elements missing id or type", () => { + const result = sanitizeElements([ + null, + 42, + "str", + { type: "rectangle" }, + { id: "no-type" }, + rect("ok"), + ]); + expect(result).toHaveLength(1); + expect((result[0] as El).id).toBe("ok"); + }); + + it("keeps unrelated fields untouched", () => { + const [el] = sanitizeElements([ + { ...rect("a"), strokeColor: "#e03131", boundElements: [{ id: "t" }] }, + ]) as El[]; + expect(el.strokeColor).toBe("#e03131"); + expect(el.boundElements).toEqual([{ id: "t" }]); + }); + + it("aligns freedraw pressures with points when simulatePressure is falsy", () => { + // The renderer indexes pressures[i] per point when simulatePressure is + // falsy — a freedraw with points but no pressures array throws. + const [missing] = sanitizeElements([ + { + id: "f1", + type: "freedraw", + x: 0, + y: 0, + width: 10, + height: 20, + points: [ + [0, 0], + [5, 5], + [10, 20], + ], + }, + ]) as El[]; + expect(missing.pressures).toEqual([0.5, 0.5, 0.5]); + + const [short] = sanitizeElements([ + { + id: "f2", + type: "freedraw", + x: 0, + y: 0, + width: 10, + height: 20, + points: [ + [0, 0], + [5, 5], + [10, 20], + ], + simulatePressure: false, + pressures: [0.25, "bad"], + }, + ]) as El[]; + expect(short.pressures).toEqual([0.25, 0.5, 0.5]); + }); + + it("supplies pressures alongside fallback freedraw points", () => { + const [el] = sanitizeElements([ + { + id: "f3", + type: "freedraw", + x: 0, + y: 0, + width: 10, + height: 20, + simulatePressure: false, + pressures: [0.9, 0.8, 0.7], + }, + ]) as El[]; + expect(el.points).toEqual([ + [0, 0], + [10, 20], + ]); + // Stale pressures from dropped points are not carried over. + expect(el.pressures).toEqual([0.5, 0.5]); + }); + + it("leaves pressures alone when simulatePressure is true", () => { + const [el] = sanitizeElements([ + { + id: "f4", + type: "freedraw", + x: 0, + y: 0, + width: 10, + height: 20, + points: [ + [0, 0], + [10, 20], + ], + simulatePressure: true, + pressures: [], + }, + ]) as El[]; + expect(el.pressures).toEqual([]); + }); + + it("preserves real pen pressures for well-formed freedraw", () => { + const points = [ + [0, 0], + [5, 5], + [10, 20], + ]; + const [el] = sanitizeElements([ + { + id: "f5", + type: "freedraw", + x: 0, + y: 0, + width: 10, + height: 20, + points, + simulatePressure: false, + pressures: [0.1, 0.2, 0.3], + }, + ]) as El[]; + expect(el.points).toEqual(points); + expect(el.pressures).toEqual([0.1, 0.2, 0.3]); + }); +}); + // ── isValidScene ── describe("isValidScene", () => { @@ -363,6 +586,61 @@ describe("createWhiteboardHandlers", () => { ).rejects.toThrow("full"); }); + it("persists arrows with points even when the agent omits them", async () => { + const deps = createMockDeps(); + const handlers = createWhiteboardHandlers(deps); + const pool = deps.pool as unknown as { query: ReturnType }; + + mockEmptyLoad(pool); + mockSaveReturn(pool, 1); + + await handlers.updateWhiteboard( + "agt_test", + [{ id: "a1", type: "arrow", x: 0, y: 0, width: 50, height: 2 }], + [] + ); + + const saveCall = pool.query.mock.calls[1]; + const scene = JSON.parse(saveCall[1][1] as string); + expect(scene.elements[0].points).toEqual([ + [0, 0], + [50, 2], + ]); + }); + + it("heals malformed elements already stored on the board", async () => { + const deps = createMockDeps(); + const handlers = createWhiteboardHandlers(deps); + const pool = deps.pool as unknown as { query: ReturnType }; + + mockLoadReturn( + pool, + { + elements: [ + { id: "old", type: "arrow", x: 0, y: 0, width: 9, height: 0 }, + ], + }, + 1 + ); + mockSaveReturn(pool, 2); + + const result = await handlers.updateWhiteboard( + "agt_test", + [rect("b")], + [] + ); + expect(result.elementCount).toBe(2); + + const scene = JSON.parse(pool.query.mock.calls[1][1][1] as string); + const healed = scene.elements.find( + (e: Record) => e.id === "old" + ); + expect(healed.points).toEqual([ + [0, 0], + [9, 0], + ]); + }); + it("ignores incoming elements without required fields", async () => { const deps = createMockDeps(); const handlers = createWhiteboardHandlers(deps); diff --git a/apps/web/src/components/app/whiteboard-pane.tsx b/apps/web/src/components/app/whiteboard-pane.tsx index 3996cc16..d9589385 100644 --- a/apps/web/src/components/app/whiteboard-pane.tsx +++ b/apps/web/src/components/app/whiteboard-pane.tsx @@ -1,4 +1,5 @@ -import { lazy, Suspense, useEffect, useState } from "react"; +import { Component, lazy, Suspense, useEffect, useState } from "react"; +import type { ErrorInfo, ReactNode } from "react"; import { useAtom } from "jotai"; import { whiteboardAgentDrewAtomFamily } from "@/lib/store"; @@ -6,6 +7,35 @@ import { cn } from "@/lib/utils"; const WhiteboardTab = lazy(() => import("@/components/app/whiteboard-tab")); +// A scene the editor can't restore must not escape to the router's error +// boundary, which would blank the entire app instead of just this tab. +class WhiteboardErrorBoundary extends Component< + { children: ReactNode }, + { failed: boolean } +> { + state = { failed: false }; + + static getDerivedStateFromError(): { failed: boolean } { + return { failed: true }; + } + + componentDidCatch(error: Error, info: ErrorInfo): void { + console.error("Whiteboard failed to render", error, info); + } + + render(): ReactNode { + if (this.state.failed) { + return ( +
+ This whiteboard could not be rendered. Reload to try again — if it + keeps failing, ask the agent to clear the board. +
+ ); + } + return this.props.children; + } +} + type WhiteboardPaneProps = { agentId: string | null; active: boolean; @@ -30,15 +60,17 @@ export function WhiteboardPane({ if (!opened || !agentId) return null; return (
- - Loading whiteboard… -
- } - > - - + + + Loading whiteboard… + + } + > + + + ); } diff --git a/e2e/whiteboard.spec.ts b/e2e/whiteboard.spec.ts index 8ef4fdb0..6b36fbf6 100644 --- a/e2e/whiteboard.spec.ts +++ b/e2e/whiteboard.spec.ts @@ -142,6 +142,54 @@ test.describe("Whiteboard", () => { expect(conflictData.version).toBe(1); }); + test("whiteboard renders when an agent writes an arrow without points", async ({ + page, + request, + }) => { + const agent = await createAgentViaAPI(request, { + name: `e2e-agent-wb-arrow-${Date.now()}`, + }); + + // An arrow with no `points` used to crash the whole route: Excalidraw's + // isInvisiblySmallElement() reads `element.points.length` unguarded. + await callMcpTool(request, agent.id, "whiteboard_update", { + elements: [ + { id: "s1", type: "rectangle", x: 0, y: 0, width: 160, height: 70 }, + { id: "s2", type: "rectangle", x: 300, y: 0, width: 160, height: 70 }, + { + id: "s1-s2-arrow", + type: "arrow", + x: 170, + y: 35, + width: 120, + height: 0, + }, + ], + }); + + const getRes = await request.get(`/api/v1/agents/${agent.id}/whiteboard`, { + headers: AUTH_HEADER, + }); + const { scene } = (await getRes.json()) as { + scene: { elements: Array<{ id: string; points?: unknown }> }; + }; + const arrow = scene.elements.find((e) => e.id === "s1-s2-arrow"); + expect(arrow?.points).toEqual([ + [0, 0], + [120, 0], + ]); + + await loadApp(page); + await clickAgentRow(page, agent.id); + await page.getByTestId("center-tab-whiteboard").click(); + await page.waitForURL(/\/agents\/[^/]+\/whiteboard/); + + await expect(page.getByTestId("whiteboard-canvas")).toBeVisible(); + await expect(page.locator(".excalidraw canvas").first()).toBeVisible(); + await expect(page.getByText("Unexpected Application Error")).toHaveCount(0); + await expect(page.getByText("could not be rendered")).toHaveCount(0); + }); + test("whiteboard MCP tool: agent can update and read whiteboard", async ({ request, }) => { diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index 06b60519..5056e8a0 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -1,2 +1,6 @@ packages: - "apps/*" +allowBuilds: + esbuild: set this to true or false + sharp: set this to true or false + workerd: set this to true or false