web: StrictMode-safe runtime lifecycle + progressive rendering + state dedupe#3
web: StrictMode-safe runtime lifecycle + progressive rendering + state dedupe#3ragnorc wants to merge 1 commit into
Conversation
…dedupe
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).
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6a21af4. Configure here.
| resolveStatePointer(this.snapshot.state, change.path), | ||
| change.value, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
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.
Reviewed by Cursor Bugbot for commit 6a21af4. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a21af45ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const effective = changes.filter( | ||
| (change) => | ||
| !valuesEqual( | ||
| resolveStatePointer(this.snapshot.state, change.path), | ||
| change.value, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
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 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| const handlers = useMemo( | ||
| () => ({ | ||
| mutate: async (params: Record<string, unknown>) => { | ||
| setDismissedMutationError(null); | ||
| await runtime.dispatch("mutate", { params }); | ||
| await runtimeRef.current?.dispatch("mutate", { params }); | ||
| }, | ||
| }), | ||
| [runtime], | ||
| [], | ||
| ); |
There was a problem hiding this comment.
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!
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
- Text/Quote `text_column` is now required (z.string().min(1)) — a Text/Quote cell with rows but no text column was silently rendering the empty fallback instead of failing validation. (#3) - validate: warn when a *required* (non-nullable) catalog param is bound to $state without a default — it resolves at runtime, which validate can't see, so flag it rather than passing silently. (#2) - validate: document that the server-bound checks are intentional — structural parse already runs offline above; the meaningful ref/param validation needs a resolvable source by design. (#1, accepted-by-design) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Follow-ups to #2 (stacked on its branch). Makes the web renderer actually work.
Fixes
RuntimeAppnow creates the runtime inside the effect (notuseState) andmain.tsxrestores<React.StrictMode>. Previously StrictMode's dev mount→cleanup→mount ran the cleanup (runtime.dispose()) on the singleuseStateruntime, then re-subscribed to the now-disposed runtime — which never notified, leaving the page stuck on the loading skeleton. Each mount now builds a fresh runtime; the disposed one's in-flight reads are aborted.status === "ready"(all-or-nothing). It renders every cell as soon as the snapshot has them, each showing its own loading / data / error state — so a slow or failed cell never blanks the whole page. (The runtime already notifies per-cell viasetCellExecution.)runtime.applyStateChangesdrops changes whose value equals the current state at that pointer. This breaks the webSelectre-run loop (the@json-render/reactcontrol re-emits its current value on every render →applyStateChanges→ re-run → re-render → …). Harmless for the TUI.Plus: documented
ReadOutput.targetas the normalized branch/snapshot label. After the SDK migration the facade already collapses the server's{branch, snapshot}to a string, and no consumer reads the structured form (verified — it's set/passed-through only), so the fullstring → objectrestructure (which would ripple throughcatalog/runtime/client/fixture+ ~13 test fixtures) is intentionally not done — it'd be churn for a display-only field. Reopen if a consumer ever needs branch-vs-snapshot.Verification
@omnigraph/runtimetypecheck + 13 tests (the dedupe doesn't regress existing state-change tests).@omnigraph/webtypecheck + 4 tests;pnpm -r buildgreen.pnpm --filter @omnigraph/web devthen openhttp://127.0.0.1:5173/?mode=fixture— the company demo (which has aSelectcontrol) should render all cells with StrictMode on and no re-run loop.Merge after #2 (or retarget to
mainonce #2 lands).Note
Medium Risk
Touches core runtime state application and web lifecycle; dedupe could skip legitimate updates if object key order differs, but scope is UI/runtime integration rather than auth or data persistence.
Overview
Makes the web notebook renderer usable under React StrictMode and avoids blank pages while cells load.
Runtime lifecycle:
RuntimeAppcreates the notebook runtime inside auseEffect(withruntimeRef) instead ofuseState, so StrictMode’s mount → cleanup → remount disposes the first instance and subscribes to a fresh one.main.tsxre-enables<React.StrictMode>and drops the dev workaround comment.Progressive UI: The app no longer waits for overall
status === "ready"before showing cells. It renders the cell list whenever a snapshot exists (onlynullsnapshot orfataluse full-page states).CellCardshows per-cell skeleton bars while a data cell has no spec yet.State loop fix:
applyStateChangesfilters out changes whose value already matches state at that JSON pointer (valuesEqual, including shallow object compare viaJSON.stringify), so controls that re-emit the same value on every render do not re-trigger dependent cell runs.Docs only: JSDoc on
ReadOutput.targetexplains it as the normalized branch/snapshot label string.Reviewed by Cursor Bugbot for commit 6a21af4. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
This PR fixes three problems introduced by the SDK migration in #2: a StrictMode double-invoke that left the page on a loading skeleton forever, an all-or-nothing rendering gate that blanked the page when any cell was slow, and an infinite re-run loop caused by a bound
Selectcontrol re-emitting its current value on every render.App.tsx,main.tsx): the runtime is now created insideuseEffectso each mount builds a fresh runtime rather than re-subscribing to a disposed one;<React.StrictMode>is restored.App.tsx):snapshot === null(pre-effect) shows the full skeleton; once the runtime is ready the page renders cells immediately with per-cell skeleton bars for unresolved data cells, so a slow or failed cell never blanks the whole page.runtime/index.ts):applyStateChangesfilters out changes whose value already equals the current state pointer value, breaking the Select re-run loop;valuesEqualhandles primitives via===and objects viaJSON.stringify.Confidence Score: 4/5
The three core fixes are correctly implemented; the only rough edge is the JSON.stringify-based equality in valuesEqual, which is order-sensitive for object keys and conflates undefined/NaN with null.
The StrictMode lifecycle and progressive rendering changes are solid. The valuesEqual helper has known limitations with JSON.stringify that could cause the dedup to silently fail or produce false positives in edge cases, but these do not affect the primary Select re-run loop fix for typical string values.
packages/runtime/src/index.ts — specifically the new valuesEqual helper; the JSON.stringify approach warrants a closer look if any control state values are objects.
Important Files Changed
Sequence Diagram
sequenceDiagram participant SM as React StrictMode participant RA as RuntimeApp participant RT as NotebookRuntime participant UI as Progressive UI SM->>RA: mount (1st — dev only) RA->>RT: createNotebookRuntime() RT-->>RA: "runtime A (cells pre-populated, status=loading)" RA->>RA: setSnapshot(runtime A snapshot) RA->>RT: runtime A.subscribe(setSnapshot) SM->>RA: cleanup (dev only) RA->>RT: unsubscribe + runtime A.dispose() RA->>RA: "runtimeRef.current = null" SM->>RA: mount (2nd — real mount) RA->>RT: createNotebookRuntime() RT-->>RA: runtime B (fresh, cells loading) RA->>RA: setSnapshot(runtime B snapshot) → shows per-cell skeletons RA->>RT: runtime B.subscribe(setSnapshot) RT->>RT: async cell reads complete RT->>RA: notify() → setSnapshot(updated cells) RA->>UI: render cell (spec ready) UI->>RA: onStateChange(changes) RA->>RT: runtimeRef.current?.applyStateChanges(changes) RT->>RT: valuesEqual check — drop no-op changes RT->>RT: rerunCells(affected) only if changedReviews (1): Last reviewed commit: "web: StrictMode-safe runtime lifecycle, ..." | Re-trigger Greptile