From 2f68b0d10947f6931ef1f81203fa28e9d1a4ca04 Mon Sep 17 00:00:00 2001 From: mptyl Date: Sun, 5 Jul 2026 13:18:51 +0200 Subject: [PATCH] fix(frontend): contain viewer/widget crashes with error boundaries A render error in one viewer or gate widget unmounted the whole React root (white screen). Add a reusable ErrorBoundary (class component, no new dep) with resetKeys + an on-brand fallback, and wire it at two surfaces: - WidgetHost: isolates the gate widget (reset on descriptor id) so a malformed gate payload no longer blanks the conversation - SessionDocumentsPanel: wraps each document (reset on doc key/content) so one crashing viewer degrades only its section; siblings and the panel survive The observed crash: a schema-linking doc that parses but lacks `candidates` makes SchemaLinkingViewer throw. Verified live via Playwright against the mock backend. TDD throughout; tsc clean, 123/123 vitest (+8 new tests). Co-Authored-By: Claude Fable 5 --- .../src/components/ErrorBoundary.test.tsx | 101 ++++++++++++++++++ frontend/src/components/ErrorBoundary.tsx | 101 ++++++++++++++++++ .../src/shell/SessionDocumentsPanel.test.tsx | 23 ++++ frontend/src/shell/SessionDocumentsPanel.tsx | 8 +- frontend/src/shell/WidgetHost.test.tsx | 34 ++++++ frontend/src/shell/WidgetHost.tsx | 9 +- 6 files changed, 274 insertions(+), 2 deletions(-) create mode 100644 frontend/src/components/ErrorBoundary.test.tsx create mode 100644 frontend/src/components/ErrorBoundary.tsx create mode 100644 frontend/src/shell/WidgetHost.test.tsx diff --git a/frontend/src/components/ErrorBoundary.test.tsx b/frontend/src/components/ErrorBoundary.test.tsx new file mode 100644 index 00000000..eed10af5 --- /dev/null +++ b/frontend/src/components/ErrorBoundary.test.tsx @@ -0,0 +1,101 @@ +import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { useState } from "react"; +import { ErrorBoundary } from "./ErrorBoundary"; + +/** A child that throws on demand. Toggle `boom` to control whether it crashes. */ +function Boom({ boom, label = "kaboom" }: { boom: boolean; label?: string }) { + if (boom) throw new Error(label); + return
child ok
; +} + +// React logs caught render errors to console.error; silence it so the suite output +// stays readable while still exercising the real error path. +let errorSpy: ReturnType; +beforeEach(() => { + errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); +}); +afterEach(() => { + errorSpy.mockRestore(); +}); + +test("renders children when nothing throws", () => { + render( + + + , + ); + expect(screen.getByText("child ok")).toBeInTheDocument(); +}); + +test("renders the default fallback when a child throws", () => { + render( + + + , + ); + expect(screen.queryByText("child ok")).not.toBeInTheDocument(); + // Default fallback names the labelled surface. + expect(screen.getByText(/couldn't be displayed/i)).toBeInTheDocument(); + expect(screen.getByText(/step/i)).toBeInTheDocument(); +}); + +test("uses a custom fallback render prop with the error", () => { + render( +

custom: {err.message}

}> + + , + ); + expect(screen.getByText("custom: explode")).toBeInTheDocument(); +}); + +test("calls onError when a child throws", () => { + const onError = vi.fn(); + render( + + + , + ); + expect(onError).toHaveBeenCalledTimes(1); + expect(onError.mock.calls[0][0]).toBeInstanceOf(Error); + expect((onError.mock.calls[0][0] as Error).message).toBe("reported"); +}); + +test("recovers when resetKeys change to non-crashing content", async () => { + function Harness() { + const [boom, setBoom] = useState(true); + return ( + <> + + + + + + ); + } + render(); + // Starts crashed → fallback visible. + expect(screen.getByText(/couldn't be displayed/i)).toBeInTheDocument(); + // Flip the data (and thus resetKeys); the boundary clears and re-renders children. + await userEvent.click(screen.getByRole("button", { name: "fix" })); + expect(screen.getByText("child ok")).toBeInTheDocument(); + expect(screen.queryByText(/couldn't be displayed/i)).not.toBeInTheDocument(); +}); + +test("Try again resets the boundary", async () => { + let boom = true; + function Flaky() { + if (boom) throw new Error("transient"); + return
recovered
; + } + render( + + + , + ); + expect(screen.getByText(/couldn't be displayed/i)).toBeInTheDocument(); + // Simulate the underlying condition clearing, then retry. + boom = false; + await userEvent.click(screen.getByRole("button", { name: /try again/i })); + expect(screen.getByText("recovered")).toBeInTheDocument(); +}); diff --git a/frontend/src/components/ErrorBoundary.tsx b/frontend/src/components/ErrorBoundary.tsx new file mode 100644 index 00000000..bdc216cc --- /dev/null +++ b/frontend/src/components/ErrorBoundary.tsx @@ -0,0 +1,101 @@ +import { Component, type ErrorInfo, type ReactNode } from "react"; + +interface Props { + children: ReactNode; + /** + * When any value in this array changes, a boundary that is currently showing an + * error clears itself. Pass the identity of the thing being rendered (a session + * id, widget id, document key) so navigating away from bad data recovers instead + * of staying stuck on the fallback. + */ + resetKeys?: unknown[]; + /** Render a custom fallback. Receives the caught error and a reset callback. */ + fallback?: (error: Error, reset: () => void) => ReactNode; + /** Names the wrapped surface in the default fallback ("step", "\"Final SQL\""). */ + label?: string; + /** Side effect on catch, e.g. reporting. */ + onError?: (error: Error, info: ErrorInfo) => void; +} + +interface State { + error: Error | null; +} + +function changed(a: unknown[] = [], b: unknown[] = []): boolean { + return a.length !== b.length || a.some((v, i) => !Object.is(v, b[i])); +} + +/** + * Catches render errors in its subtree and shows a fallback instead of letting the + * crash unmount the whole React root (a white screen). One bad viewer or widget + * should degrade only its own region — the rail, composer, and other documents keep + * working. + */ +export class ErrorBoundary extends Component { + state: State = { error: null }; + + static getDerivedStateFromError(error: Error): State { + return { error }; + } + + componentDidCatch(error: Error, info: ErrorInfo) { + this.props.onError?.(error, info); + } + + componentDidUpdate(prev: Props) { + if (this.state.error && changed(prev.resetKeys, this.props.resetKeys)) { + this.reset(); + } + } + + reset = () => this.setState({ error: null }); + + render() { + const { error } = this.state; + if (!error) return this.props.children; + if (this.props.fallback) return this.props.fallback(error, this.reset); + return ; + } +} + +function DefaultFallback({ + error, + label, + onRetry, +}: { + error: Error; + label?: string; + onRetry: () => void; +}) { + const subject = label ? `${label} ` : ""; + return ( +
+

This {subject}couldn't be displayed.

+

+ Something went wrong while rendering it. The rest of the session is unaffected. +

+
+ + {error.message && ( +
+ + Details + + 
+              {error.message}
+
+
+ )} +
+
+ ); +} diff --git a/frontend/src/shell/SessionDocumentsPanel.test.tsx b/frontend/src/shell/SessionDocumentsPanel.test.tsx index 113de5fa..11e58dd7 100644 --- a/frontend/src/shell/SessionDocumentsPanel.test.tsx +++ b/frontend/src/shell/SessionDocumentsPanel.test.tsx @@ -50,3 +50,26 @@ test("hides Resume for an archived session", async () => { await screen.findByText("Original question"); expect(screen.queryByRole("button", { name: /resume/i })).not.toBeInTheDocument(); }); + +test("a malformed document renders a fallback without taking down its siblings", async () => { + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + // Valid JSON but missing `candidates` — SchemaLinkingViewer crashes on this shape. + // Without a per-document boundary this crash blanks the whole panel (React 18 + // unmounts the tree); the good sibling document must still render. + // resetHandlers(...) replaces the beforeEach handler so this response is used. + server.resetHandlers( + http.get("http://localhost:8787/sessions/s1/documents", () => + HttpResponse.json([ + { phase: "F4", key: "schema", title: "Schema linking", format: "schema-linking", content: '{"joins":[]}' }, + { phase: "—", key: "question", title: "Original question", format: "text", content: "SIBLING-SURVIVES" }, + ]), + ), + ); + wrap(); + // The healthy sibling rendered — the panel did not white-screen. + expect(await screen.findByText("SIBLING-SURVIVES")).toBeInTheDocument(); + // The broken document degraded to a fallback in place. + expect(screen.getByRole("alert")).toBeInTheDocument(); + expect(screen.getByText(/couldn't be displayed/i)).toBeInTheDocument(); + errorSpy.mockRestore(); +}); diff --git a/frontend/src/shell/SessionDocumentsPanel.tsx b/frontend/src/shell/SessionDocumentsPanel.tsx index 3e18094e..920f782a 100644 --- a/frontend/src/shell/SessionDocumentsPanel.tsx +++ b/frontend/src/shell/SessionDocumentsPanel.tsx @@ -3,6 +3,7 @@ import { X } from "lucide-react"; import { getSessionDocuments } from "../api/sessions"; import type { SessionDocument, SessionSummary } from "../api/types"; import { Button } from "../components/ui/button"; +import { ErrorBoundary } from "../components/ErrorBoundary"; import { SqlViewer } from "../viewers/SqlViewer"; import { SchemaLinkingViewer } from "../viewers/SchemaLinkingViewer"; import { MarkdownView } from "../viewers/MarkdownView"; @@ -110,7 +111,12 @@ export function SessionDocumentsPanel({ session, onClose, onResume }: Props) { )} - + {/* Isolate each document: a viewer that crashes on malformed + content degrades to a fallback in place, leaving the rest of + the panel intact instead of blanking the whole tree. */} + + + ))} diff --git a/frontend/src/shell/WidgetHost.test.tsx b/frontend/src/shell/WidgetHost.test.tsx new file mode 100644 index 00000000..95cff725 --- /dev/null +++ b/frontend/src/shell/WidgetHost.test.tsx @@ -0,0 +1,34 @@ +import { render, screen } from "@testing-library/react"; +import { WidgetHost } from "./WidgetHost"; +import { useSessionStore } from "../store/sessionStore"; +import type { WidgetDescriptor } from "../api/types"; + +// Replace the widget registry with a renderer that always throws, simulating a +// viewer/widget that crashes on malformed gate data (the real white-screen path). +vi.mock("../widgets", () => ({ + resolve: () => function Exploding() { + throw new Error("bad gate payload"); + }, +})); + +let errorSpy: ReturnType; +beforeEach(() => { + errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + useSessionStore.getState().resetSession(); +}); +afterEach(() => { + errorSpy.mockRestore(); + useSessionStore.getState().resetSession(); +}); + +const gate: WidgetDescriptor = { id: "g1", widget: "select", title: "Choose" }; + +test("a crashing widget shows a fallback instead of unmounting the app", () => { + useSessionStore.setState({ pendingWidget: gate }); + render(); + // No throw escaped: the fallback rendered. + expect(screen.getByRole("alert")).toBeInTheDocument(); + expect(screen.getByText(/couldn't be displayed/i)).toBeInTheDocument(); + // The store (and thus the surrounding app) is still alive and interactive. + expect(useSessionStore.getState().pendingWidget).toEqual(gate); +}); diff --git a/frontend/src/shell/WidgetHost.tsx b/frontend/src/shell/WidgetHost.tsx index 8ea978c2..72dbc9cf 100644 --- a/frontend/src/shell/WidgetHost.tsx +++ b/frontend/src/shell/WidgetHost.tsx @@ -2,6 +2,7 @@ import { useSessionStore } from "../store/sessionStore"; import { resolve } from "../widgets"; import { postResponse } from "../api/sessions"; import type { UiResponse } from "../api/types"; +import { ErrorBoundary } from "../components/ErrorBoundary"; export function WidgetHost({ sessionId }: { sessionId: string | null }) { const pending = useSessionStore((s) => s.pendingWidget); @@ -32,5 +33,11 @@ export function WidgetHost({ sessionId }: { sessionId: string | null }) { }); clearPending(); }; - return ; + // A malformed gate payload must not white-screen the whole app: isolate the + // widget render. resetKeys on the descriptor id so the next gate starts clean. + return ( + + + + ); }