From 6a21af45ff5c883575327e7386643b4a613c41e4 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Tue, 9 Jun 2026 18:37:55 +0200 Subject: [PATCH] web: StrictMode-safe runtime lifecycle, progressive rendering, state dedupe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the three follow-ups noted in the SDK-adoption PR: - web/App: create the runtime inside the effect (not useState) and restore React.StrictMode. StrictMode's dev mount→cleanup→mount used to dispose the one runtime and re-subscribe to it, wedging the page on the loading skeleton; now each mount builds a fresh runtime. - web/App: render every cell progressively — each shows its own loading / data / error state — instead of gating the whole page on overall status. A slow or failed cell no longer blanks the page (the all-or-nothing gate). - runtime: applyStateChanges drops no-op changes, so a bound control re-emitting its current value (the web Select) no longer triggers an endless re-run loop. - runtime: document ReadOutput.target as the normalized branch/snapshot label — the facade collapses the server's {branch,snapshot} and no consumer needs the structured form, so the full string→object restructure is intentionally not done (it would ripple through 4 packages + tests for a display-only field). --- packages/runtime/src/index.ts | 30 ++++++++++++++- packages/web/src/App.tsx | 71 +++++++++++++++++++++-------------- packages/web/src/main.tsx | 10 ++--- 3 files changed, 76 insertions(+), 35 deletions(-) diff --git a/packages/runtime/src/index.ts b/packages/runtime/src/index.ts index adb167f..5747831 100644 --- a/packages/runtime/src/index.ts +++ b/packages/runtime/src/index.ts @@ -48,6 +48,13 @@ export interface ReadRequest { export interface ReadOutput { query_name: string; + /** + * The branch (or snapshot id) the read resolved against, normalized to a + * single label. The omnigraph server returns a `{branch, snapshot}` object; + * the client facade collapses it to this string because nothing in colombo + * distinguishes the two — it's display metadata only. Keep it a string + * unless/until a consumer needs the structured form. + */ target: string; row_count: number; columns: string[]; @@ -234,11 +241,22 @@ class NotebookRuntimeImpl implements NotebookRuntime { applyStateChanges(changes: RuntimeStateChange[]): void { if (this.disposed || changes.length === 0) return; + // Drop no-op changes: a bound control (e.g. the web Select) can re-emit its + // current value on every render, which would otherwise re-run dependent + // cells in an endless loop. Only proceed when something actually changed. + const effective = changes.filter( + (change) => + !valuesEqual( + resolveStatePointer(this.snapshot.state, change.path), + change.value, + ), + ); + if (effective.length === 0) return; let next = this.snapshot.state; - for (const change of changes) { + for (const change of effective) { next = setAtPointer(next, change.path, change.value); } - const changedPaths = changes.map((change) => change.path); + const changedPaths = effective.map((change) => change.path); this.snapshot = { ...this.snapshot, state: next, @@ -874,3 +892,11 @@ function errorMessage(err: unknown): string { function isAbortError(err: unknown): boolean { return err instanceof Error && err.name === "AbortError"; } + +function valuesEqual(a: unknown, b: unknown): boolean { + if (a === b) return true; + if (a !== null && b !== null && typeof a === "object" && typeof b === "object") { + return JSON.stringify(a) === JSON.stringify(b); + } + return false; +} diff --git a/packages/web/src/App.tsx b/packages/web/src/App.tsx index 36b7b52..1796e85 100644 --- a/packages/web/src/App.tsx +++ b/packages/web/src/App.tsx @@ -1,8 +1,9 @@ -import React, { useCallback, useEffect, useMemo, useState } from "react"; +import React, { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { JSONUIProvider, Renderer } from "@json-render/react"; import { createNotebookRuntime, type CellExecution, + type NotebookRuntime, type RuntimeSnapshot, } from "@omnigraph/runtime"; @@ -60,42 +61,51 @@ export function App(): React.ReactElement { } function RuntimeApp({ config }: { config: AppConfig }): React.ReactElement { - const [runtime] = useState(() => - createNotebookRuntime({ notebook: config.notebook, source: config.source }), - ); - const [snapshot, setSnapshot] = useState(() => - runtime.getSnapshot(), - ); + const runtimeRef = useRef(null); + const [snapshot, setSnapshot] = useState(null); const [dismissedMutationError, setDismissedMutationError] = useState(null); + // Create the runtime *inside* the effect (not useState) so React StrictMode's + // dev mount → cleanup → mount cycle disposes the first runtime and builds a + // fresh one — rather than re-subscribing to a disposed runtime that never + // notifies (which leaves the page stuck on the loading skeleton). + useEffect(() => { + const runtime = createNotebookRuntime({ + notebook: config.notebook, + source: config.source, + }); + runtimeRef.current = runtime; + setSnapshot(runtime.getSnapshot()); + const unsubscribe = runtime.subscribe(() => + setSnapshot(runtime.getSnapshot()), + ); + return () => { + unsubscribe(); + runtime.dispose(); + runtimeRef.current = null; + }; + }, [config]); + const handleStateChange = useCallback( (changes: Array<{ path: string; value: unknown }>) => { - runtime.applyStateChanges(changes); + runtimeRef.current?.applyStateChanges(changes); }, - [runtime], + [], ); const handlers = useMemo( () => ({ mutate: async (params: Record) => { setDismissedMutationError(null); - await runtime.dispatch("mutate", { params }); + await runtimeRef.current?.dispatch("mutate", { params }); }, }), - [runtime], + [], ); - useEffect(() => { - const unsubscribe = runtime.subscribe(() => setSnapshot(runtime.getSnapshot())); - return () => { - unsubscribe(); - runtime.dispose(); - }; - }, [runtime]); - const mutationError: ClassifiedError | null = - snapshot.mutationError !== null && + snapshot?.mutationError != null && snapshot.mutationError !== dismissedMutationError ? classifyMutationError(snapshot.mutationError) : null; @@ -125,18 +135,16 @@ function RuntimeApp({ config }: { config: AppConfig }): React.ReactElement { - {snapshot.status === "loading" && ( - c.id)} - /> - )} - {snapshot.status === "fatal" && ( + {snapshot === null ? ( + c.id)} /> + ) : snapshot.status === "fatal" ? ( - )} - {snapshot.status === "ready" && ( + ) : ( + // Render every cell progressively — each shows its own loading / + // data / error state, so a slow or failed cell never blanks the page.
{snapshot.cells.map((cell) => ( @@ -206,6 +214,13 @@ function CellCard({ cell }: { cell: CellExecution }): React.ReactElement { {cell.error === null && cell.spec !== null && ( )} + {cell.error === null && cell.spec === null && !isControl && ( +
+ + + +
+ )} ); } diff --git a/packages/web/src/main.tsx b/packages/web/src/main.tsx index 0b495f6..5d7d8c9 100644 --- a/packages/web/src/main.tsx +++ b/packages/web/src/main.tsx @@ -5,8 +5,8 @@ import "./index.css"; const root = document.getElementById("root"); if (!root) throw new Error("missing #root mount node"); -// NOTE (dev workaround): StrictMode's double-invoke runs the effect cleanup, -// which calls runtime.dispose() — disposing the runtime mid initial-run so it -// never notifies and the page stays on the loading skeleton. Disabled here -// while pointing at a real server. Real fix belongs in App.tsx lifecycle. -createRoot(root).render(); +createRoot(root).render( + + + , +);