From d9edcbb9eded6089558832f6e61e484032910c30 Mon Sep 17 00:00:00 2001 From: dovvnloading <157447210+dovvnloading@users.noreply.github.com> Date: Fri, 4 Sep 2026 10:24:20 -0400 Subject: [PATCH] Contain a failed node-view chunk, and stop exporting its loading shell Three gaps in the code split that shipped in #399. A chunk that fails to load took the whole tree with it. 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. Measured: it surfaces as an unhandled error. In the app that means one node kind failing to fetch - a stale asset index after a deploy, a connection dropped mid-session - blanks the entire canvas and the app chrome with it. Each node now has its own error boundary outside its Suspense boundary, so the blast radius of a bad chunk is one card. There is deliberately no retry button: React.lazy caches the rejection as well as the resolution, so re-rendering replays the failure forever and a retry that cannot work is worse than none. The card says to reload, which is the only thing that recovers. PNG export could capture "Loading..." shells. exportCanvasPng polls for ChartNodeView's `.chart-node-placeholder` and resolved as soon as none remained - it knew nothing about `.scene-node-loading`, the fallback #399 added. Export is the worst possible moment for that gap: it deliberately disables onlyRenderVisibleElements to mount the off-viewport nodes, which are exactly the ones whose chunks were never fetched. Both fallbacks now live in one NODE_LOADING_SELECTOR constant. The fallback had no styling at all. `.scene-node-loading` appears in no stylesheet - #399's own comment describes "a card-shaped shell" that in fact collapsed to whatever `.scene-node` alone gives, which is the zero-size box the comment says it exists to prevent. It now mirrors `.chart-node-placeholder`. Test plan: - 5 new tests: a failing chunk leaves the surrounding tree mounted, the error card names the recovery and offers no dead retry, the underlying error is logged not swallowed, the loading shell carries the class export polls for, and all 17 exported views go through the wrapper so no kind can bypass the boundary. - The direct "Suspense alone does not contain a rejection" demonstration is deliberately NOT a test: the unhandled rejection fails the vitest run itself (measured, exit 1). Recorded as a comment on the test that asserts the same thing positively. - npm run check clean: 93 files, 2195 tests, 0 lint errors, bundle 781,017 bytes against the 804,000 ceiling. Co-Authored-By: Claude Opus 5 --- web_ui/src/app/canvas/exportCanvasPng.ts | 18 +++- web_ui/src/app/canvas/lazyNodeViews.test.tsx | 103 +++++++++++++++++++ web_ui/src/app/canvas/lazyNodeViews.tsx | 71 +++++++++++-- web_ui/src/app/styles.css | 20 ++++ 4 files changed, 203 insertions(+), 9 deletions(-) create mode 100644 web_ui/src/app/canvas/lazyNodeViews.test.tsx diff --git a/web_ui/src/app/canvas/exportCanvasPng.ts b/web_ui/src/app/canvas/exportCanvasPng.ts index 2ca3dcbd..a7b0f2a1 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 00000000..99447afc --- /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 f936dcaf..cd4107b2 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 9fbfd12f..e26ac29b 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;