Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions packages/runtime/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[];
Expand Down Expand Up @@ -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,
),
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Batch no-op filter wrong

Low Severity

In applyStateChanges, each incoming change is compared to the pre-batch snapshot state when deciding if it is a no-op. A later entry for the same JSON pointer that matches the original value can be dropped even though an earlier entry in the same batch already changed that pointer, so the final state may not match applying all patches in order.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 6a21af4. Configure here.

Comment on lines +247 to +253

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve overlapping state changes in a batch

When a single applyStateChanges call contains overlapping paths, this filter compares every change to the pre-patch snapshot, so it can drop a child update that is only a no-op before an earlier parent update is applied. For example, starting from {filters:{status:"open"}}, a patch like [{path:"/filters", value:{}}, {path:"/filters/status", value:"open"}] should leave status restored, but the second change is filtered out here and the final state becomes {filters:{}}, causing dependent cells to rerun with the wrong params. The dedupe needs to evaluate changes sequentially against the evolving next state, or avoid dropping changes whose paths overlap another change in the same batch.

Useful? React with 👍 / 👎.

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,
Expand Down Expand Up @@ -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;
}
Comment on lines +896 to +902

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 valuesEqual has two edge cases that can silently misclassify changes

JSON.stringify is key-insertion-order sensitive: valuesEqual({a:1, b:2}, {b:2, a:1}) returns false even though the values are semantically identical. If @json-render/react's Select constructs a new object from the stored value with a different property order on re-emit, the dedupe check fails and the loop recurs. Additionally, JSON.stringify maps both undefined and NaN to null, so valuesEqual({x: undefined}, {x: null}) returns true, silently dropping a genuine state change. Consider replacing JSON.stringify with a proper structural comparison (e.g. a recursive key-sorted comparison or a well-tested deep-equal utility) to avoid both pitfalls.

Fix in Claude Code

71 changes: 43 additions & 28 deletions packages/web/src/App.tsx
Original file line number Diff line number Diff line change
@@ -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";

Expand Down Expand Up @@ -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<RuntimeSnapshot>(() =>
runtime.getSnapshot(),
);
const runtimeRef = useRef<NotebookRuntime | null>(null);
const [snapshot, setSnapshot] = useState<RuntimeSnapshot | null>(null);
const [dismissedMutationError, setDismissedMutationError] =
useState<string | null>(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<string, unknown>) => {
setDismissedMutationError(null);
await runtime.dispatch("mutate", { params });
await runtimeRef.current?.dispatch("mutate", { params });
},
}),
[runtime],
[],
);
Comment on lines 97 to 105

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 handlers and handleStateChange correctly use ref, but stale-closure risk on setDismissedMutationError

The empty dependency arrays are correct because runtimeRef is a mutable ref. However, handlers.mutate also closes over setDismissedMutationError, which is a useState setter and is guaranteed stable by React — so this works today. The pattern is worth a brief comment to clarify intent; without one, a future reader might add runtimeRef to the dep array (causing the runtime to re-create on every effect cycle), or might add a real closure over snapshot instead of reading via the ref.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code


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;
Expand Down Expand Up @@ -125,18 +135,16 @@ function RuntimeApp({ config }: { config: AppConfig }): React.ReactElement {
</span>
</header>

{snapshot.status === "loading" && (
<LoadingSkeleton
cellTitles={config.notebook.cells.map((c) => c.id)}
/>
)}
{snapshot.status === "fatal" && (
{snapshot === null ? (
<LoadingSkeleton cellTitles={config.notebook.cells.map((c) => c.id)} />
) : snapshot.status === "fatal" ? (
<FatalPanel
title="Failed to run notebook"
message={snapshot.error ?? "runtime failed"}
/>
)}
{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.
<div className="space-y-6">
{snapshot.cells.map((cell) => (
<CellCard key={cell.cell.id} cell={cell} />
Expand Down Expand Up @@ -206,6 +214,13 @@ function CellCard({ cell }: { cell: CellExecution }): React.ReactElement {
{cell.error === null && cell.spec !== null && (
<Renderer spec={cell.spec} registry={webRegistry} />
)}
{cell.error === null && cell.spec === null && !isControl && (
<div className="space-y-2">
<SkeletonBar w="w-full" />
<SkeletonBar w="w-5/6" />
<SkeletonBar w="w-2/3" />
</div>
)}
</section>
);
}
Expand Down
10 changes: 5 additions & 5 deletions packages/web/src/main.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(<App />);
createRoot(root).render(
<React.StrictMode>
<App />
</React.StrictMode>,
);