Repository navigation
Conversation
…Layer - CoreComponent: a parentless root stops its own scheduler on unmount instead of scheduling another render, so unmounted layer subtrees are no longer rebuilt and unmounted graphs can be garbage collected. - Add terminal graph.removeLayer(layer) / Layers.removeLayer: forgets the layer and unmounts it with its children; repeated calls, layers already destroyed by graph.unmount() and foreign layers are ignored. graph.detachLayer stays reversible. - useLayer (and GraphCanvas, GraphLayer, GraphPortal) destroy their layer on cleanup with graph.removeLayer. - KeyboardService: capture the document at subscribe time so its keydown/keyup listeners are removed after the root is released. - Docs: v2 migration note, layers.md, cleanup examples. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Preview is ready. |
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
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.
Important
Merge order: merge this PR strictly after #390 (
fix/v2-use-graph-strict-mode).In React StrictMode,
useGraphcallsgraph.unmount()on effect replay and then reuses the memoized graph. Before this PR, the unmounted GraphLayer kept running as a zombie, so React-rendered blocks still appeared by accident. With this PR the unmounted roots are really stopped. #390 defers disposal of the graphuseGraphcreates itself and cancels that disposal on replay, so the graph is never unmounted there. The two branches merge cleanly (git merge-tree).Summary
This PR fixes two lifecycle bugs in v2, plus a third leak that the new GC test exposed.
1. Root schedulers were never released
Every root created by
Component.mount, which includes every graph layer, owns aSchedulerregistered inglobalScheduler.CoreComponent.__unmountcalledperformRender()after unmounting and never stopped that scheduler. As a result:graph.unmount()the next tick rebuilt the layer subtrees (GraphLayer went from 1 to 4 nodes);globalScheduler;Fix: a component with no parent stops its own scheduler on unmount and skips
performRender(). Children share their root's scheduler, so unmounting a child leaves it running.2.
useLayerremoved layers without destroying themuseLayercleanup calledgraph.detachLayer(layer). That call is reversible by contract (DevTools and PortConnectionLayer reattach layers), and it only cleans up DOM and listeners. SoComponent.unmountnever ran for the layer or its children, andgraph.unmount()could no longer find it.GraphCanvasusesuseLayer(ReactLayer), so every GraphCanvas unmount left a live ReactLayer behind.Fix: a new terminal
graph.removeLayer(layer), backed byLayers.removeLayer:WeakSetlimits it to layers created by this graph, so repeated calls, layers already destroyed bygraph.unmount(), and layers of other graphs are no-ops.destroyLayersreuses it.useLayercleanup now calls it, and so doGraphCanvas,GraphLayerandGraphPortalthroughuseLayer.graph.unmount()runs before or afterremoveLayer.graph.detachLayerandLayer.detachLayerstay reversible.3. KeyboardService leaked document listeners
A heap-snapshot retainer search after fix 1 found this. The unsubscribe re-read
graph.layers.$root, butgraph.stop(true)clears$rootbefore the state becomesINIT. Thekeydown/keyuplisteners were therefore never removed, anddocumentkept every started graph alive. Fix: capture the document at subscribe time.Docs
layers.md.PortConnectionLayer.mdand the devtools README.Tests
All of these were written first, and each failed for the stated reason.
CoreComponent.test.ts: an unmounted root does not rebuild its child and releases its registration; unmounting a child keeps the shared scheduler running.graph.test.ts: layers of an unmounted graph are not iterated, and the number of registrations does not grow.graph.gc.test.ts: 20/20 unmounted graphs are collected, including graphs whose layer was removed withremoveLayer. Before the fix: 0/20. The test usesv8.setFlagsFromString("--expose-gc")andvm.runInNewContext("gc").Layer.test.ts:removeLayerunmounts the layer and its children, forgets the layer, and it is never iterated again. It also destroys a detached layer exactly once, ignores layers of another graph, and ignores layers already destroyed bygraph.unmount().useLayer.test.ts: cleanup unmounts the layer and its children; StrictMode replay destroys the replayed layer and keeps the live one; the layer is destroyed once when the graph unmounts first.GraphCanvas.test.tsx: unmounting GraphCanvas with an external graph destroys its ReactLayer exactly once.KeyboardService.test.ts: aftergraph.unmount(), a Shift keydown no longer reaches the service. On the original code the handler fired twice.Checks
All of these pass on top of
v2with #388:pnpm run typecheckpnpm run test:type-configspnpm test: graph 44 suites / 342 tests, graph-react 18 suites / 145 tests, devtools 11 testspnpm run lint: 0 errors; 1 warning, already present, in a storybook storynode scripts/strict-diagnostics.mjspnpm run test:package-contractReview
There were three independent reviewer rounds. The last one reported no unresolved findings of P2 or higher.
Round 1 (P2): the last unmounted graph stays alive until the next global frame. This existed before this PR and has two independent causes: removals that
GlobalSchedulerdefers until its next frame, and store debounces that are still pending at unmount (for examplePortsStore.notifyPortMapChanged).graph.stop()from halting the shared global loop for every graph.Round 2 (P2): the StrictMode interaction with
useGraph. It is resolved by the merge order above.P3 findings: these were fixed: doc accuracy, foreign layers, a behaviour-based keyboard test, return types, stale examples. Two were left on purpose:
attachLayerafter removal. The docs say a removed layer cannot be reused.🤖 Generated with Claude Code