diff --git a/web_ui/src/app/canvas/exportCanvasPng.ts b/web_ui/src/app/canvas/exportCanvasPng.ts index 2ca3dcb..a7b0f2a 100644 --- a/web_ui/src/app/canvas/exportCanvasPng.ts +++ b/web_ui/src/app/canvas/exportCanvasPng.ts @@ -132,12 +132,24 @@ function nextPaint(): Promise { }); } +/** Every Suspense fallback that means "this node has not rendered yet". + * Kept as one constant so a third fallback cannot be added without a reader + * of the export path seeing it. */ +export const NODE_LOADING_SELECTOR = ".chart-node-placeholder, .scene-node-loading"; + function sleep(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)); } -/** Polls `root` for any still-loading chart node (ChartNodeView.tsx's - * `.chart-node-placeholder` Suspense fallback) and resolves once none remain. +/** Polls `root` for any still-loading node and resolves once none remain. + * + * TWO fallbacks qualify. `.chart-node-placeholder` is ChartNodeView.tsx's own, + * and was the only one this waited on. ADR-019's node-view code split added + * `.scene-node-loading` (lazyNodeViews.tsx), which is shown while ANY node + * kind's chunk loads - and export is precisely when the most chunks are + * unloaded, because exportCanvasAsPng disables onlyRenderVisibleElements to + * mount the off-viewport nodes, which are exactly the ones whose chunks were + * never fetched. Without this the PNG captures "Loading..." shells. * Bounded by `timeoutMs` so a genuinely stuck or failed chunk load (offline, * a bad deploy) degrades to "capture whatever is there" rather than hanging * the export indefinitely - consistent with this module's broader "never @@ -155,7 +167,7 @@ export async function waitForChartPlaceholdersToClear( pollIntervalMs: number = CHART_CHUNK_POLL_INTERVAL_MS, ): Promise { const deadline = Date.now() + timeoutMs; - while (root.querySelector(".chart-node-placeholder") && Date.now() < deadline) { + while (root.querySelector(NODE_LOADING_SELECTOR) && Date.now() < deadline) { await sleep(pollIntervalMs); } } diff --git a/web_ui/src/app/canvas/lazyNodeViews.test.tsx b/web_ui/src/app/canvas/lazyNodeViews.test.tsx new file mode 100644 index 0000000..99447af --- /dev/null +++ b/web_ui/src/app/canvas/lazyNodeViews.test.tsx @@ -0,0 +1,103 @@ +/** + * ADR-019: the node views load one chunk per kind. These tests cover the two + * ways that can go wrong at runtime, neither of which the split shipped with. + * + * The failure mode that matters: React.lazy throws a PROMISE, which Suspense + * catches - but when that promise REJECTS, Suspense has nothing to do with it + * and the rejection propagates uncaught. React then unmounts the whole tree. + * One node kind failing to load (a stale asset index after a deploy, a dropped + * connection mid-session) would blank the entire canvas and the app chrome + * with it. Measured before the boundary existed: the rejection surfaced as an + * unhandled error. + */ + +import { lazy, type ComponentType } from "react"; +import { render, screen, waitFor } from "@testing-library/react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import * as lazyNodeViews from "./lazyNodeViews"; + +// The real module's own wrapper shape, applied to controllable chunks. Kept +// structurally identical to withNodeSuspense so a change there that drops the +// boundary fails here (see the export-shape test at the bottom, which pins +// that these really are the same wrapper). +function neverResolves() { + return new Promise<{ default: ComponentType }>(() => {}); +} + +describe("a chunk that fails to load", () => { + beforeEach(() => { + vi.spyOn(console, "error").mockImplementation(() => {}); + }); + + // This is the regression proof. Before NodeChunkBoundary existed, a + // rejecting chunk inside only a Suspense boundary surfaced as an UNHANDLED + // error - Suspense catches the thrown promise, never its rejection - and + // React unmounted the tree. Demonstrating that directly is not possible in + // a test file, because the unhandled rejection fails the vitest run itself + // (measured: exit 1). The surviving "App chrome" assertion below is the + // same claim, stated positively. + it("is contained to its own card - the surrounding tree survives", async () => { + const Failing = lazyNodeViews.__testing.withNodeSuspense( + lazy(() => Promise.reject(new Error("Failed to fetch dynamically imported module"))), + ); + + render( +
+

App chrome

+ +
, + ); + + await waitFor(() => expect(screen.getByRole("alert")).toBeTruthy()); + // The point of the whole exercise: everything around the bad card is + // still mounted. + expect(screen.getByText("App chrome")).toBeTruthy(); + expect(screen.getByRole("alert").textContent).toContain("could not load"); + }); + + it("says how to recover, and does not offer a retry that cannot work", async () => { + const Failing = lazyNodeViews.__testing.withNodeSuspense( + lazy(() => Promise.reject(new Error("boom"))), + ); + render(); + await waitFor(() => expect(screen.getByRole("alert")).toBeTruthy()); + // React.lazy caches the rejection, so re-rendering replays the failure + // forever. A retry button here would be a button that never works. + expect(screen.queryByRole("button")).toBeNull(); + expect(screen.getByRole("alert").textContent).toContain("Reload"); + }); + + it("logs the underlying error rather than swallowing it", async () => { + const Failing = lazyNodeViews.__testing.withNodeSuspense( + lazy(() => Promise.reject(new Error("chunk 404"))), + ); + render(); + await waitFor(() => expect(screen.getByRole("alert")).toBeTruthy()); + const logged = (console.error as unknown as { mock: { calls: unknown[][] } }).mock.calls; + expect(logged.some((call) => String(call[0]).includes("chunk failed to load"))).toBe(true); + }); +}); + +describe("a chunk that is still loading", () => { + it("shows the loading shell, which export polls for", () => { + const Pending = lazyNodeViews.__testing.withNodeSuspense(lazy(neverResolves)); + const { container } = render(); + // exportCanvasPng.ts's NODE_LOADING_SELECTOR waits on exactly this class; + // if it is ever renamed, the PNG export starts capturing loading shells. + expect(container.querySelector(".scene-node-loading")).toBeTruthy(); + expect(container.querySelector('[aria-busy="true"]')).toBeTruthy(); + }); +}); + +describe("the exported node views", () => { + it("are all wrapped, so no kind can bypass the boundary", () => { + const views = Object.entries(lazyNodeViews).filter(([name]) => name.endsWith("NodeView")); + // 17 kinds; GroupNodeView backs both frame and container. + expect(views.length).toBe(17); + for (const [name, view] of views) { + expect(typeof view, name).toBe("function"); + expect((view as ComponentType).displayName, name).toContain("LazyNodeView"); + } + }); +}); diff --git a/web_ui/src/app/canvas/lazyNodeViews.tsx b/web_ui/src/app/canvas/lazyNodeViews.tsx index f936dca..cd4107b 100644 --- a/web_ui/src/app/canvas/lazyNodeViews.tsx +++ b/web_ui/src/app/canvas/lazyNodeViews.tsx @@ -27,7 +27,7 @@ // component renders, so an empty fallback would collapse the card and let // edges snap to a zero-size box for a frame or two. -import { lazy, Suspense, type ComponentType } from "react"; +import { Component, lazy, Suspense, type ComponentType, type ReactNode } from "react"; /** The shell shown while a kind's chunk is still loading. */ function NodeChunkFallback() { @@ -38,6 +38,55 @@ function NodeChunkFallback() { ); } +/** Shown when a chunk could not be fetched at all. + * + * There is deliberately no retry button. React.lazy caches the REJECTION as + * well as the resolution, so re-rendering the same lazy component replays the + * failure forever - a retry that cannot work is worse than none. Reloading + * re-runs the import from scratch, which is the only thing that can recover. */ +function NodeChunkError() { + return ( +
+
+ This card could not load. Reload the app to try again. +
+
+ ); +} + +/** + * Catches a failed chunk fetch for ONE node. + * + * Without it a rejected import propagates uncaught and React unmounts the + * whole tree - one node kind failing to load (a stale index after a deploy, a + * dropped connection mid-session) would blank the entire canvas and the app + * chrome with it. Verified: a rejecting lazy() inside only a Suspense + * boundary surfaces as an unhandled error, because Suspense catches the + * PROMISE, never the rejection. + * + * A class component because that is the only thing React lets be an error + * boundary. Per node, for the same reason the Suspense boundary is per node: + * the blast radius of one bad chunk should be one card. + */ +class NodeChunkBoundary extends Component<{ children: ReactNode }, { failed: boolean }> { + state = { failed: false }; + + static getDerivedStateFromError() { + return { failed: true }; + } + + componentDidCatch(error: unknown) { + // Left visible rather than swallowed: a chunk that will not load is a + // deploy/network problem worth seeing in the console, and this component + // cannot surface it any other way. + console.error("node view chunk failed to load", error); + } + + render() { + return this.state.failed ? : this.props.children; + } +} + /** * Wrap a lazily-imported node component in its own Suspense boundary. * @@ -45,19 +94,29 @@ function NodeChunkFallback() { * one that throws a promise - so the boundary has to live between them, * here, rather than at any call site. */ -function withNodeSuspense

(Component: ComponentType

): ComponentType

{ +function withNodeSuspense

(Loaded: ComponentType

): ComponentType

{ function LazyNodeView(props: P) { + // Boundary OUTSIDE Suspense: Suspense catches the thrown promise, the + // boundary catches its rejection. Nested the other way the rejection + // escapes past the boundary entirely. return ( - }> - - + + }> + + + ); } // Keeps React DevTools and any test that queries by displayName readable. - LazyNodeView.displayName = `LazyNodeView(${Component.displayName || "chunk"})`; + LazyNodeView.displayName = `LazyNodeView(${Loaded.displayName || "chunk"})`; return LazyNodeView; } +// Exposed for lazyNodeViews.test.tsx only. The wrapper's behaviour on a +// failed chunk is the thing worth testing, and it cannot be reached through +// the real exports without a real failing chunk. +export const __testing = { withNodeSuspense } + // Named exports, so each import() is unwrapped to a default for lazy(). export const ArtifactNodeView = withNodeSuspense( lazy(() => import("./ArtifactNodeView").then((m) => ({ default: m.ArtifactNodeView }))), diff --git a/web_ui/src/app/styles.css b/web_ui/src/app/styles.css index 9fbfd12..e26ac29 100644 --- a/web_ui/src/app/styles.css +++ b/web_ui/src/app/styles.css @@ -8512,6 +8512,26 @@ mark.document-view-search-match-current { border-radius: 8px; } +/* ADR-019 node-view code split: the shell shown while a node kind's chunk + loads (lazyNodeViews.tsx). It shipped with NO styling at all, so the + fallback collapsed to whatever .scene-node alone gives - React Flow has + already positioned and sized the node by then, and an unsized card lets + edges snap to a near-zero box for a frame or two. Mirrors + .chart-node-placeholder below, which is the same idea for chart chunks. */ +.scene-node-loading { + display: flex; + align-items: center; + justify-content: center; + min-width: 260px; + min-height: 120px; + padding: 16px; + color: var(--gl-surface-text-muted); + font-size: 11px; + text-align: center; + border: 1px dashed var(--gl-surface-border); + border-radius: 8px; +} + .chart-node-placeholder { display: flex; align-items: center;