From 96eb7a88de3ddf9039a74602a113aaec7f5c64de Mon Sep 17 00:00:00 2001 From: David Witwer Date: Sat, 5 Sep 2026 16:43:18 -0700 Subject: [PATCH 1/5] fix(harness): show one message in the canvas Render-failed state [SAP-3199] The canvas drew two error strings on top of each other when an agent could not render. The app's Render-failed card is a transparent layer over the iframe (`.canvas-render-error` is inset:0 with no background), and the document inside that iframe painted its own copy of the reason underneath it. The stylesheet already claimed the document's error text was hidden by the board snippet. That was the intent and the mechanism was never there, so the two strings overlapped and neither was readable. Hide the prose the same way the document already hides its title, badge and legend: those are app chrome the SPA draws around the iframe, and the failure reason is the same kind of duplicate. The paragraph gets a stable class, a rule beside the existing header rule hides it under `[data-canvas-embedded]`, and a small head script sets that flag when `window.parent !== window`. It is its own script so the flag is set before first paint and a parse error in the much larger run-state script cannot leave it unset. Opened standalone there is no card coming, so the prose stays and is the only message. Both directions are pinned by specs, mutation-tested three ways. No user-visible string changed, and the card itself is untouched. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WoapZZPdPVtz11TQCK5eQH --- packages/harness/e2e/canvas-render.spec.ts | 100 +++++++++++++++++- packages/harness/src/core/canvas-body.ts | 11 +- .../harness/src/core/canvas-render.test.ts | 5 + packages/harness/src/core/canvas-template.ts | 28 ++++- packages/harness/web/src/styles.css | 9 +- 5 files changed, 145 insertions(+), 8 deletions(-) diff --git a/packages/harness/e2e/canvas-render.spec.ts b/packages/harness/e2e/canvas-render.spec.ts index a87e85698..5a47d86d8 100644 --- a/packages/harness/e2e/canvas-render.spec.ts +++ b/packages/harness/e2e/canvas-render.spec.ts @@ -13,7 +13,7 @@ * `file://` — not `setContent` — so the document's own inline theme script runs * against `location.search`, exactly as the SPA's iframe loads it. */ -import { test, expect } from "@playwright/test"; +import { test, expect, type FrameLocator, type Page } from "@playwright/test"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; @@ -128,6 +128,104 @@ test("a workflow with no extractable definition renders an honest error panel, n expect(errors).toEqual([]); }); +/** + * SAP-3199. Embedded, the SPA draws its own Render-failed card (short claim, + * one-line reason, actions, full reason behind Details) as a TRANSPARENT layer + * over this document. So the composed view has exactly one error message only if + * the document stands its own prose down while it is framed. It used to keep + * painting, and the short reason and the long one drew through each other. + * + * The parent here is a bare file:// shell rather than the real SPA: it loads the + * render the same way (an `allow-scripts`-only sandboxed iframe) and listens on + * the same channel, which is all the document can observe. Asserting against the + * real app is the screenshot pass on the PR, not this spec. + */ +async function embedInParentShell(page: Page, url: string, file: string): Promise { + const parentFile = path.join(path.dirname(file), "workbench-shell.html"); + await fs.writeFile( + parentFile, + ` + + +`, + "utf8", + ); + await page.goto(pathToFileURL(parentFile).href); + return page.frameLocator("#board"); +} + +/** The opening of the failed panel's prose, which is the string that used to + * draw through the app's card. */ +const REASON_TEXT = /Could not extract this agent's step graph/; + +test("an embedded failed render shows one error message, not two drawn through each other", async ({ + page, +}) => { + const errors: string[] = []; + page.on("pageerror", (err) => errors.push(String(err))); + + const { url, file } = await renderToFileUrl(NO_DEFINITION, "no-definition"); + const frame = await embedInParentShell(page, url, file); + + // The reason reaches the workbench exactly once, which is the one message the + // user is shown, and the SPA renders it as the card. + await expect + .poll(() => page.evaluate(() => (window as unknown as { __posted: unknown[] }).__posted.length)) + .toBe(1); + const posted = await page.evaluate( + () => (window as unknown as { __posted: { title: string; reason: string }[] }).__posted[0], + ); + expect(posted.title).toBe("no-definition"); + expect(posted.reason.length).toBeGreaterThan(10); + + // …and the document under the card paints no second copy of it. Restore the + // overlap (drop the class or the stylesheet rule) and this is the assertion + // that goes red. + await expect(frame.locator(".canvas-render-error-note")).toBeAttached(); + await expect(frame.getByText(REASON_TEXT)).toBeHidden(); + // Nothing else in the document paints a message either. Every text-bearing + // element is walked rather than the two we know about, so a THIRD string added + // to the failed panel later cannot quietly reintroduce the overlap. + // + // "Paints" is hit-tested rather than inferred from the box, because the title + // and the "render failed" badge would otherwise count: they live in + // `.canvas-header`, which the template collapses to a clipped 1px box, and the + // children inside it keep their full-size rects even though the parent clips + // every pixel of them away. Asking the document what is actually at the + // element's centre point is the question a reader asks. + const painted = await frame.locator("body").evaluate((body) => { + const shown: string[] = []; + for (const el of Array.from(body.querySelectorAll("p, h1, span, div"))) { + const own = Array.from(el.childNodes) + .filter((n) => n.nodeType === Node.TEXT_NODE) + .map((n) => (n.textContent || "").trim()) + .join(" ") + .trim(); + if (!own) continue; + const box = el.getBoundingClientRect(); + const hit = document.elementFromPoint(box.left + box.width / 2, box.top + box.height / 2); + if (hit && (hit === el || el.contains(hit))) shown.push(own); + } + return shown; + }); + expect(painted).toEqual([]); + expect(errors).toEqual([]); +}); + +test("a failed render opened on its own keeps its prose as the only message", async ({ page }) => { + const { url } = await renderToFileUrl(NO_DEFINITION, "no-definition"); + await page.goto(url); + + // No parent, so no card is coming, and the document must still say what happened. + await expect(page.getByText(REASON_TEXT)).toBeVisible(); + expect(await page.getByText(REASON_TEXT).count()).toBe(1); +}); + test("launched sub-workflows paint as their own dashed nodes with a launch edge", async ({ page }) => { const { url } = await renderToFileUrl(HUB, "hub"); await page.goto(url); diff --git a/packages/harness/src/core/canvas-body.ts b/packages/harness/src/core/canvas-body.ts index b5e9424ca..f75438c0b 100644 --- a/packages/harness/src/core/canvas-body.ts +++ b/packages/harness/src/core/canvas-body.ts @@ -205,7 +205,14 @@ ${renderGraphSvg(graph, enrichment)} /** A degraded panel for a workflow whose graph couldn't be extracted — never * a crash, never a silent fallback to the LLM path, just an honest reason - * styled through the same shell. */ + * styled through the same shell. + * + * The prose carries `canvas-render-error-note` so the template can hide it + * while the document is embedded: the SPA paints its own Render-failed card + * (short claim, one-line reason, actions, full reason behind Details) + * transparently over this document, and two error strings drawn through each + * other was SAP-3199. Opened standalone there is no card, so the prose stays + * as the only message. */ export function buildErrorPanelHtml(title: string, reason: string): string { const errorData = JSON.stringify({ title, reason }).replace(/ @@ -216,7 +223,7 @@ export function buildErrorPanelHtml(title: string, reason: string): string {
-

Could not extract this agent's step graph: ${esc(reason)}. Use the workbench actions to ask your coding agent to fix it or retry the deterministic render.

+

Could not extract this agent's step graph: ${esc(reason)}. Use the workbench actions to ask your coding agent to fix it or retry the deterministic render.

`; diff --git a/packages/harness/src/core/canvas-render.test.ts b/packages/harness/src/core/canvas-render.test.ts index 2aff528f9..6267f2b6b 100644 --- a/packages/harness/src/core/canvas-render.test.ts +++ b/packages/harness/src/core/canvas-render.test.ts @@ -225,6 +225,11 @@ describe("renderCanvasForSession", () => { expect(html).toContain("render failed"); expect(html).toContain("Could not extract this agent's step graph"); expect(html).toContain('id="sapiom-render-error"'); + // Embedded, the SPA's Render-failed card is the one message; the document's + // prose steps aside instead of drawing through it (SAP-3199). + expect(html).toContain('class="canvas-empty-note canvas-render-error-note"'); + expect(html).toContain(":root[data-canvas-embedded] .canvas-render-error-note { display: none; }"); + expect(html).toContain('document.documentElement.setAttribute("data-canvas-embedded", "")'); expect(html).toContain('"title":"broken-flow"'); expect(html).toContain("sapiom-canvas:error"); expect(html).not.toContain('class="canvas-node '); // no diagram — just the note diff --git a/packages/harness/src/core/canvas-template.ts b/packages/harness/src/core/canvas-template.ts index d75c222ea..70389d098 100644 --- a/packages/harness/src/core/canvas-template.ts +++ b/packages/harness/src/core/canvas-template.ts @@ -172,6 +172,13 @@ body { position: absolute; width: 1px; height: 1px; overflow: hidden; clip-path: inset(50%); white-space: nowrap; margin: 0; padding: 0; } +/* Same reason as the header above, for the failed render. The SPA paints its + own Render-failed card (short claim, one-line reason, actions, full reason + behind Details) as a transparent layer directly over this document, so the + document's copy of the reason drew straight through it (SAP-3199). Embedded, + the card is the one message and this prose steps aside; opened standalone + there is no card, so it stays and is the only message. */ +:root[data-canvas-embedded] .canvas-render-error-note { display: none; } .canvas-interconnections { display: flex; flex-direction: column; gap: 12px; } .canvas-panel-title { margin: 0; font-size: 12px; text-transform: uppercase; letter-spacing: 0.06em; color: var(--canvas-text-dim); } .canvas-interconnection-row { display: grid; grid-template-columns: 12px 1fr auto; column-gap: 8px; row-gap: 2px; align-items: baseline; } @@ -260,9 +267,23 @@ template { display: none; } `.trim(); } +/** Marks the document as embedded so the stylesheet can stand down the chrome + * the SPA already draws around the iframe. Its own head script, not a branch + * inside the run-state bundle, so a parse error in that much larger script + * can never leave the flag unset, and in the head, so the flag is on the + * root element before the body paints and nothing flashes. + * + * `window.parent` is readable from a sandboxed frame and comparing the two + * references is not a cross-origin access, so this is safe under the + * `allow-scripts`-only sandbox the SPA loads the board with. */ +const EMBED_SCRIPT = ` +(function () { + if (window.parent !== window) document.documentElement.setAttribute("data-canvas-embedded", ""); +})(); +`.trim(); + /** Reads the current theme from `?theme=light|dark`, falling back to the - * Studio's light product default when the param is absent — the only script - * in the whole document. */ + * Studio's light product default when the param is absent. */ const THEME_SCRIPT = ` (function () { var params = new URLSearchParams(location.search); @@ -400,6 +421,9 @@ export function renderCanvasDocument(bodyHtml: string): string { Agent Studio canvas +