Repository navigation
Conversation
useGraph unmounted its owned graph synchronously in a layout-effect cleanup. React StrictMode replays effect cleanup and setup, so the memoized graph was destroyed while the hook kept returning it (one layer, no canvases with GraphCanvas). Disposal of an owned graph is now deferred by one macrotask and cancelled when the effect sets up again for the same graph. Pending disposals are keyed by graph, so a key change or Fast Refresh never cancels another graph's disposal. The effect is passive so a React 18 Suspense boundary that hides shown content does not dispose the graph. External graphs are never disposed by the hook. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewer's GuideThe PR fixes premature destruction of internally created graphs by pairing ownership with graph creation, deferring owned-graph disposal through a per-graph timer in a passive effect, and canceling that disposal during StrictMode replay. Comprehensive tests cover StrictMode, Suspense, keyed remounts, graph switching, unmounting, and preservation of externally owned graphs. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Preview is ready. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/graph-react/src/hooks/useGraph.ts" line_range="86" />
<code_context>
- expect(graph.graphConstants.camera.SPEED).toBe(2);
- expect(graph.graphConstants.camera.PAN_SPEED).toBe(1);
- unmount();
- graph.unmount();
-});
</code_context>
<issue_to_address>
**Replacement graph stops rendering**
When an owned graph is disposed after its replacement has started, `graph.unmount()` stops the shared global scheduler, so the replacement graph remains mounted in READY state but receives no scheduled updates and stops rendering.
Prevent disposal of an old graph from stopping the shared scheduler while a replacement graph is using it.
</issue_to_address>
### Comment 2
<location path="packages/graph-react/src/hooks/useGraph.ts" line_range="76" />
<code_context>
+ // not disposed. In development React 18 StrictMode renders twice on mount, so one such Graph is
+ // created per mount; suspended first mounts and interrupted renders can create more.
+ useEffect(() => {
+ if (!owned) {
+ return undefined;
+ }
</code_context>
<issue_to_address>
**Reused graph gets unmounted**
When `config.graph` changes from `undefined` to the same graph previously created by this hook, `useEffect` returns on the `!owned` branch before clearing the pending disposal timer. The timer then calls `graph.unmount()` even though the hook still returns that graph as external, leaving the caller with a dead graph.
Clear any pending disposal for the graph before returning from the `!owned` branch.
Also at `packages/graph-react/src/hooks/useGraph.ts:77`.
</issue_to_address>
### Comment 3
<location path="packages/graph-react/src/hooks/useGraph.ts" line_range="52" />
<code_context>
- layers: config.layers, // layers init only once
- });
+ return {
+ graph: new Graph({
+ configurationName: config.name || "",
+ blocks: [],
</code_context>
<issue_to_address>
**Discarded graphs leak schedulers**
When react 18 development StrictMode invokes the initializer twice, or concurrent rendering abandons an initial render, the abandoned `Graph` never reaches the passive-effect cleanup, and its layer components' `Scheduler`s remain registered with the global scheduler; repeated mounts or render retries accumulate scheduler registrations.
Avoid creating the graph during render, or provide a safe cleanup path for graphs created by renders that never commit.
</issue_to_address>
### Comment 4
<location path="packages/graph-react/src/hooks/useGraph.ts" line_range="86" />
<code_context>
- expect(graph.graphConstants.camera.SPEED).toBe(2);
- expect(graph.graphConstants.camera.PAN_SPEED).toBe(1);
- unmount();
- graph.unmount();
-});
</code_context>
<issue_to_address>
**Teardown errors escape asynchronously**
When a layer teardown throws during disposal of an owned graph, `graph.unmount()` runs in the timer callback outside React's commit handling, so the teardown exception escapes as an uncaught asynchronous error instead of reaching an Error Boundary; teardown also stops partway.
Route disposal errors through React's error-handling path instead of allowing them to escape from the timer callback.
</issue_to_address>Sourcery assessment
Approval pending. 4 findings to address first.
Blocking findings: packages/graph-react/src/hooks/useGraph.ts:86, packages/graph-react/src/hooks/useGraph.ts:76, packages/graph-react/src/hooks/useGraph.ts:52, packages/graph-react/src/hooks/useGraph.ts:86
| layers: config.layers, // layers init only once | ||
| }); | ||
| return { | ||
| graph: new Graph({ |
There was a problem hiding this comment.
🟡 Medium · Discarded graphs leak schedulers
When react 18 development StrictMode invokes the initializer twice, or concurrent rendering abandons an initial render, the abandoned Graph never reaches the passive-effect cleanup, and its layer components' Schedulers remain registered with the global scheduler; repeated mounts or render retries accumulate scheduler registrations.
Avoid creating the graph during render, or provide a safe cleanup path for graphs created by renders that never commit.
Prompt for AI agents
In `packages/graph-react/src/hooks/useGraph.ts` at line 52:
**Discarded graphs leak schedulers**
When react 18 development StrictMode invokes the initializer twice, or concurrent rendering abandons an initial render, the abandoned `Graph` never reaches the passive-effect cleanup, and its layer components' `Scheduler`s remain registered with the global scheduler; repeated mounts or render retries accumulate scheduler registrations.
Avoid creating the graph during render, or provide a safe cleanup path for graphs created by renders that never commit.| graph, | ||
| setTimeout(() => { | ||
| pendingDisposals.delete(graph); | ||
| graph.unmount(); |
There was a problem hiding this comment.
🟡 Medium · Teardown errors escape asynchronously
When a layer teardown throws during disposal of an owned graph, graph.unmount() runs in the timer callback outside React's commit handling, so the teardown exception escapes as an uncaught asynchronous error instead of reaching an Error Boundary; teardown also stops partway.
Route disposal errors through React's error-handling path instead of allowing them to escape from the timer callback.
Prompt for AI agents
In `packages/graph-react/src/hooks/useGraph.ts` at line 86:
**Teardown errors escape asynchronously**
When a layer teardown throws during disposal of an owned graph, `graph.unmount()` runs in the timer callback outside React's commit handling, so the teardown exception escapes as an uncaught asynchronous error instead of reaching an Error Boundary; teardown also stops partway.
Route disposal errors through React's error-handling path instead of allowing them to escape from the timer callback.Address review findings on the deferred disposal: - Dispose an owned graph synchronously in the passive cleanup, and defer it only when the cleanup runs in the same synchronous pass as the graph's first setup (React 18 StrictMode replay). Normal unmount errors reach error boundaries again. - Graph.unmount() stops the shared global scheduler. Track graphs held by mounted useGraph instances (ref-counted, owned and external) and restart the scheduler after every disposal and setup while any of them is running, so a replacement or another graph is not frozen. - Remember the graph the hook created, so passing it back as config.graph keeps it owned instead of disposing it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
useGraphcreated its graph inuseMemoand calledgraph.unmount()in auseLayoutEffectcleanup when it owned the graph (noconfig.graphpassed). React StrictMode replays effect cleanup and setup on mount, so the memoized graph was destroyed but still returned by the hook.Repro (React 18.3.1):
render(<StrictMode><App /></StrictMode>)whereAppcallsuseGraph({})and renders<GraphCanvas graph={graph} renderBlock={() => <div />} />. Result:graph.layers.getLayers().length === 1and no<canvas>. Without StrictMode, or with an externalgraph, the result is 5 layers and 3 canvases.Fix (
packages/graph-react/src/hooks/useGraph.ts)queueMicrotask).setTimeout(0)and cancelled by the replayed setup.WeakMapkeyed by graph, so one graph's setup never cancels another graph's disposal.Graph.unmount()stops the global scheduler that every graph shares, and that scheduler is not ref-counted.useGraphinstances, both owned and external.READY.config.graph, the graph stays owned. It is not disposed by that switch, and it is still disposed on unmount.useEffect. In React 18, a Suspense boundary that hides already-shown content runs layout-effect cleanups but keeps passive effects, so the graph survives the hide.config.graph) are never disposed by the hook.Behaviour notes for consumers
setTimeout(0), for examplerenderHook(() => useGraph({})); unmount();without anawaitin between.graph.unmount()on this path are uncaught and do not reach an error boundary.useGraphholds, and that is started in the layout phase of the same commit, is now stopped by that disposal. Graphs held byuseGraphare restarted.<Activity>also disconnects passive effects. The peer range is React 18 only.Known limitation (documented in code)
A Graph built by a render that never commits never reaches the effect, so it is never disposed.
useMemo,useReflazy init anduseStateinitializers all run twice in React 18.3.1.new Graph()registers its layer schedulers inglobalSchedulerin the constructor, andGraph.unmount()never removes them. That is a pre-existing core issue.Tests (
useGraph.test.ts→useGraph.test.tsxfor JSX)Fail on the original hook and pass now:
keydisposes the old graph and keeps the new one;Pass on the original hook, and failed on the first version of this PR; they now guard against regressions:
config.graphis not disposed.Other guards: unmount under StrictMode disposes the committed graph; switching from an owned to an external graph disposes only the owned one; an external graph is never disposed, with and without StrictMode.
Checks
All of these pass:
pnpm run typecheckpnpm run test:type-configspnpm test: graph 329 passed and 6 todo, devtools 11 passed, graph-react 159 passed and 4 skippedpnpm run lint: one existing warning in the storybookcustomConnectionLabelstorynode scripts/strict-diagnostics.mjspnpm run test:package-contractpassed on the first commit; the second commit changes onlyuseGraphinternals and tests.Review
Sourcery findings:
unmount()stopped the scheduler a replacement graph had started. Valid regression; fixed (synchronous disposal plus scheduler restart).config.graphwas disposed. Valid regression; fixed (per-instance ownership).Independent review rounds:
🤖 Generated with Claude Code