fix: garbage collect subgraph definitions when SubgraphNode is removed#8187
Merged
christian-byrne merged 1 commit intomainfrom Jan 30, 2026
Merged
fix: garbage collect subgraph definitions when SubgraphNode is removed#8187christian-byrne merged 1 commit intomainfrom
christian-byrne merged 1 commit intomainfrom
Conversation
dosubot
bot
added
the
size:L
Contributor
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds subgraph garbage-collection when removing a SubgraphNode: calls inner-node onRemoved handlers, invokes subgraph.onNodeRemoved for each inner node, deletes the subgraph definition from the root graph, and makes SubgraphNode.graph nullable with null guards and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant LGraph
participant SubgraphNode
participant RootGraph
participant InnerNodes
Caller->>LGraph: remove(node)
alt node is SubgraphNode
LGraph->>SubgraphNode: read subgraph id / graph
LGraph->>RootGraph: check rootGraph.subgraphs and other refs
alt subgraph still referenced
LGraph-->>Caller: regular removal path
else no other references
LGraph->>InnerNodes: call innerNode.onRemoved() for each
LGraph->>SubgraphNode: call subgraph.onNodeRemoved(innerNode) for each
LGraph->>RootGraph: delete rootGraph.subgraphs[subgraphId]
LGraph-->>Caller: finish removal
end
else
LGraph-->>Caller: regular removal path
end
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎭 Playwright Tests: ✅ PassedResults: 507 passed, 0 failed, 0 flaky, 8 skipped (Total: 515) 📊 Browser Reports
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/28/2026, 06:06:19 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/litegraph/src/LGraph.ts (1)
1026-1029: Replacenode.graph = nullwithnode.graph = undefinedand ensure SubgraphNode's readonly graph property is handled appropriately.TS2540 occurs because
SubgraphNode.graphis a readonly constructor parameter property override (line 70 of SubgraphNode.ts), while the baseLGraphNode.graphis mutable. The assignment at line 1029 fails for SubgraphNode instances. Additionally, per codebase conventions, useundefinedinstead ofnull(note line 944 already usesundefinedfor LGraphGroup). Either:
- Change line 1029 to
node.graph = undefinedand provide a setter or cleanup method for SubgraphNode, or- Remove the assignment entirely if not needed for SubgraphNode cleanup.
🤖 Fix all issues with AI agents
In `@src/lib/litegraph/src/LGraph.test.ts`:
- Around line 213-251: Replace the hand-crafted ExportedSubgraph JSON in
createSubgraphWithNodes with the shared fixtures: call createTestSubgraph (and
createTestSubgraphNode for inner nodes) to build the subgraph and nodes instead
of manually assembling subgraphData and new LGraphNode instances; update the
function to accept a root LGraph and nodeCount, create the subgraph via
createTestSubgraph(rootGraph) and add nodes using createTestSubgraphNode(...) or
the fixture's helper to keep IDs/config consistent, then return { subgraph,
innerNodes } as before so tests continue to use the same return shape.
In `@src/lib/litegraph/src/LGraph.ts`:
- Around line 988-1023: The current logic fires inner-node
onRemoved/onNodeRemoved and scans nested subgraphs while still counting the node
being removed as a reference; change the flow to first determine whether this
subgraph definition has any references excluding the node being removed (compute
hasOtherReferences by scanning this.rootGraph.nodes and nested
this.rootGraph.subgraphs but skip the current node), and only if no other
references remain call innerNode.onRemoved?.() and
node.subgraph.onNodeRemoved?.(innerNode) for each inner node (also ensure you
skip the node itself when iterating) and then delete the definition from
this.rootGraph.subgraphs; keep the checks using node.isSubgraphNode(),
node.subgraph.id and this.rootGraph.subgraphs.delete(subgraphId) to locate the
relevant code paths.
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 25.9 kB (baseline 25.9 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 960 kB (baseline 960 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 80.7 kB (baseline 80.7 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 471 kB (baseline 471 kB) • 🟢 -8 BConfiguration panels, inspectors, and settings screens
Status: 12 added / 12 removed User & Accounts — 3.94 kB (baseline 3.94 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 3 added / 3 removed Editors & Dialogs — 2.86 kB (baseline 2.86 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 33.7 kB (baseline 33.7 kB) • ⚪ 0 BReusable component library chunks
Status: 4 added / 4 removed Data & Services — 2.7 MB (baseline 2.7 MB) • 🔴 +303 BStores, services, APIs, and repositories
Status: 8 added / 8 removed Utilities & Hooks — 25.3 kB (baseline 25.3 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 7 added / 7 removed Vendor & Third-Party — 10.7 MB (baseline 10.7 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.05 MB (baseline 7.05 MB) • 🟢 -195 BBundles that do not match a named category
Status: 34 added / 34 removed |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stores/imagePreviewStore.ts (1)
300-308: Don’t skip inner preview cleanup whengraphis null.The early return prevents revoking previews for inner nodes, which can leak object URLs when
SubgraphNode.graphis cleared during removal. Revoke inner previews unconditionally and guard only the outer-node cleanup.🛠️ Proposed fix
function revokeSubgraphPreviews(subgraphNode: SubgraphNode) { - const { graph } = subgraphNode - if (!graph) return - - const graphId = graph.isRootGraph ? '' : graph.id + ':' - revokePreviewsByLocatorId(graphId + subgraphNode.id) for (const node of subgraphNode.subgraph.nodes) { revokePreviewsByLocatorId(subgraphNode.subgraph.id + node.id) } + + const { graph } = subgraphNode + if (!graph) return + + const graphId = graph.isRootGraph ? '' : graph.id + ':' + revokePreviewsByLocatorId(graphId + subgraphNode.id) }
🤖 Fix all issues with AI agents
In `@design-subgraph-lifecycle.md`:
- Around line 1-13: Change the top-level heading "## Subgraph Lifecycle Cleanup
(Issue `#8145`)" to the appropriate level (match surrounding document structure,
e.g., "###" if this is a subsection) and fix the typo "associatred" to
"associated" in the Goals section so the heading level and spelling/readability
issues are corrected; update the heading and the word "associatred" (search for
that exact token) in design-subgraph-lifecycle.md.
christian-byrne
force-pushed
the
fix/subgraph-definition-gc
branch
from
c116372 to
6a2eb38
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/lib/litegraph/src/LGraph.test.ts`:
- Around line 4-7: The import path for createTestSubgraphData and
createTestSubgraphNode is using an unresolved alias causing ESLint
import-x/no-unresolved; update the import in LGraph.test.ts to use a relative
path to the fixture module (replace
'@/lib/litegraph/src/subgraph/__fixtures__/subgraphHelpers' with the correct
relative path from this test file) so createTestSubgraphData and
createTestSubgraphNode resolve correctly, or alternatively adjust the project
resolver if the alias should be supported.
In `@src/lib/litegraph/src/LGraph.ts`:
- Around line 988-996: The removal currently only deletes the immediate subgraph
registry entry; update the SubgraphNode cleanup in the block guarded by
node.isSubgraphNode() to perform a depth-first recursion over
node.subgraph.nodes: for each innerNode call innerNode.onRemoved?.() and
node.subgraph.onNodeRemoved?.(innerNode), and if innerNode.isSubgraphNode() also
delete its registry entry from this.rootGraph.subgraphs
(this.rootGraph.subgraphs.delete(innerNode.subgraph.id)) and recurse into
innerNode.subgraph.nodes to repeat the same cleanup so nested subgraph
definitions are removed and no orphaned entries remain.
♻️ Duplicate comments (1)
src/lib/litegraph/src/LGraph.test.ts (1)
216-227: PrefercreateTestSubgraphfor fixture setup.The subgraph test guidance prefers
createTestSubgraphto keep IDs/state consistent and reduce brittle fixture wiring. Based on learnings, please use the shared subgraph helpers in tests.♻️ Proposed refactor
-import { - createTestSubgraphData, - createTestSubgraphNode -} from './subgraph/__fixtures__/subgraphHelpers' +import { + createTestSubgraph, + createTestSubgraphNode +} from './subgraph/__fixtures__/subgraphHelpers' @@ function createSubgraphWithNodes(rootGraph: LGraph, nodeCount: number) { - const subgraph = rootGraph.createSubgraph(createTestSubgraphData()) + const subgraph = createTestSubgraph(rootGraph)
src/lib/litegraph/src/LGraph.ts
Outdated
Comment on lines
988
to
996
| // Handle SubgraphNode-specific cleanup | ||
| // Each SubgraphNode owns a unique Subgraph definition (no linked subgraphs) | ||
| if (node.isSubgraphNode()) { | ||
| for (const innerNode of node.subgraph.nodes) { | ||
| innerNode.onRemoved?.() | ||
| node.subgraph.onNodeRemoved?.(innerNode) | ||
| } | ||
| this.rootGraph.subgraphs.delete(node.subgraph.id) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for nested subgraph scenarios in tests
rg -n -A5 -B5 'nested.*subgraph|subgraph.*nested' --type=ts src/Repository: Comfy-Org/ComfyUI_frontend
Length of output: 28681
🏁 Script executed:
# Look at the remove() method and surrounding context
sed -n '970,1010p' src/lib/litegraph/src/LGraph.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1206
🏁 Script executed:
# Find all SubgraphNode-related files to understand architecture
fd -e ts -e tsx 'SubgraphNode|subgraph' src/lib/litegraph/Repository: Comfy-Org/ComfyUI_frontend
Length of output: 460
🏁 Script executed:
# Check if onRemoved callbacks handle subgraph cleanup
rg -n 'onRemoved' --type=ts src/lib/litegraph/Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1936
🏁 Script executed:
# Look for SubgraphNode class definition and constructor
ast-grep --pattern 'class SubgraphNode {
$$$
}'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
# Get SubgraphNode.onRemoved() implementation
sed -n '557,590p' src/lib/litegraph/src/subgraph/SubgraphNode.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 998
🏁 Script executed:
# Also check the test that validates this behavior
sed -n '230,280p' src/lib/litegraph/src/LGraph.test.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1678
🏁 Script executed:
# Search for any periodic cleanup or garbage collection of orphaned subgraphs
rg -n 'orphan|cleanup.*subgraph|prune' --type=ts src/lib/litegraph/Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
# Check if nested subgraph removal is tested in the edge cases
grep -n -A20 'should.*nested.*remov\|remov.*nested' src/lib/litegraph/src/subgraph/SubgraphEdgeCases.test.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
# Search broadly for any nested subgraph removal tests
rg -n 'remove.*nested|nested.*remove|removeSubgraph' --type=ts src/lib/litegraph/Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
# Check what happens when you remove a node that's inside a subgraph
sed -n '225,280p' src/lib/litegraph/src/LGraph.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1606
Handle recursive cleanup for nested SubgraphNodes during removal.
The current cleanup logic only handles the immediate subgraph definition. If an innerNode is itself a SubgraphNode, its nested subgraph definition is not removed from rootGraph.subgraphs, leaving orphaned entries in the registry. While innerNode.onRemoved?.() cleans up event listeners and widgets, it does not handle subgraph definition cleanup (which only occurs in the remove() method).
Since nested subgraphs are architecturally supported (see Subgraph.MAX_NESTED_SUBGRAPHS = 1000), this should be handled recursively:
🔧 Suggested fix
if (node.isSubgraphNode()) {
for (const innerNode of node.subgraph.nodes) {
+ // Recursively clean up nested SubgraphNodes
+ if (innerNode.isSubgraphNode()) {
+ this.rootGraph.subgraphs.delete(innerNode.subgraph.id)
+ }
innerNode.onRemoved?.()
node.subgraph.onNodeRemoved?.(innerNode)
}
this.rootGraph.subgraphs.delete(node.subgraph.id)
}🤖 Prompt for AI Agents
In `@src/lib/litegraph/src/LGraph.ts` around lines 988 - 996, The removal
currently only deletes the immediate subgraph registry entry; update the
SubgraphNode cleanup in the block guarded by node.isSubgraphNode() to perform a
depth-first recursion over node.subgraph.nodes: for each innerNode call
innerNode.onRemoved?.() and node.subgraph.onNodeRemoved?.(innerNode), and if
innerNode.isSubgraphNode() also delete its registry entry from
this.rootGraph.subgraphs
(this.rootGraph.subgraphs.delete(innerNode.subgraph.id)) and recurse into
innerNode.subgraph.nodes to repeat the same cleanup so nested subgraph
definitions are removed and no orphaned entries remain.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/litegraph/src/LGraph.test.ts`:
- Around line 230-277: Add a test that ensures a subgraph definition is retained
when another SubgraphNode still references it: create an LGraph, call
createSubgraphWithNodes(rootGraph, n) to get a subgraph, then create two
SubgraphNode instances for that same subgraph via
createTestSubgraphNode(subgraph, ...), add both to rootGraph, remove one with
rootGraph.remove(theFirstSubgraphNode), and assert that
rootGraph.subgraphs.has(subgraph.id) is still true; use the existing test style
and the same helpers (LGraph, createSubgraphWithNodes, createTestSubgraphNode,
rootGraph.subgraphs) so the new test covers the “definition retained when other
references exist” branch.
♻️ Duplicate comments (2)
src/lib/litegraph/src/LGraph.test.ts (2)
4-7: Fix unresolved fixture import to avoid CI failure.ESLint reports
import-x/no-unresolvedfor this path, which will fail linting. Confirm the file location and resolver settings; if the file exists, update the import to the correct relative path or configure the resolver for.tsfixtures.Run this to verify the file location and current import resolution:
#!/bin/bash # Locate the fixture file(s) and show the current import line. fd -a 'subgraphHelpers\.ts$' rg -n "subgraphHelpers" src/lib/litegraph/src/LGraph.test.tsIf the file is at
src/lib/litegraph/src/subgraph/__fixtures__/subgraphHelpers.tsbut the resolver still fails, consider an explicit extension:💡 Possible fix (if resolver requires extensions)
-import { - createTestSubgraphData, - createTestSubgraphNode -} from './subgraph/__fixtures__/subgraphHelpers' +import { + createTestSubgraphData, + createTestSubgraphNode +} from './subgraph/__fixtures__/subgraphHelpers.ts'
216-228: Use the shared subgraph fixture helper for setup consistency.
createSubgraphWithNodesbuilds a subgraph viacreateTestSubgraphData()+rootGraph.createSubgraph(...). Repo guidance for subgraph tests prefers thecreateTestSubgraph(...)helper to keep subgraph definitions consistent.[assert]
♻️ Suggested refactor
-import { - createTestSubgraphData, - createTestSubgraphNode -} from './subgraph/__fixtures__/subgraphHelpers' +import { + createTestSubgraph, + createTestSubgraphNode +} from './subgraph/__fixtures__/subgraphHelpers' function createSubgraphWithNodes(rootGraph: LGraph, nodeCount: number) { - const subgraph = rootGraph.createSubgraph(createTestSubgraphData()) + const subgraph = createTestSubgraph(rootGraph)Based on learnings, please use the shared subgraph helpers in tests.
Comment on lines
+230
to
+277
| it('removing SubgraphNode fires onRemoved for inner nodes', () => { | ||
| const rootGraph = new LGraph() | ||
| const { subgraph, innerNodes } = createSubgraphWithNodes(rootGraph, 2) | ||
| const removedNodeIds = new Set<string>() | ||
|
|
||
| for (const node of innerNodes) { | ||
| node.onRemoved = () => removedNodeIds.add(String(node.id)) | ||
| } | ||
|
|
||
| const subgraphNode = createTestSubgraphNode(subgraph, { pos: [100, 100] }) | ||
| rootGraph.add(subgraphNode) | ||
|
|
||
| expect(subgraph.nodes.length).toBe(2) | ||
|
|
||
| rootGraph.remove(subgraphNode) | ||
|
|
||
| expect(removedNodeIds.size).toBe(2) | ||
| }) | ||
|
|
||
| it('removing SubgraphNode fires onNodeRemoved callback', () => { | ||
| const rootGraph = new LGraph() | ||
| const { subgraph } = createSubgraphWithNodes(rootGraph, 2) | ||
| const graphRemovedNodeIds = new Set<string>() | ||
|
|
||
| subgraph.onNodeRemoved = (node) => graphRemovedNodeIds.add(String(node.id)) | ||
|
|
||
| const subgraphNode = createTestSubgraphNode(subgraph, { pos: [100, 100] }) | ||
| rootGraph.add(subgraphNode) | ||
|
|
||
| rootGraph.remove(subgraphNode) | ||
|
|
||
| expect(graphRemovedNodeIds.size).toBe(2) | ||
| }) | ||
|
|
||
| it('subgraph definition is removed when SubgraphNode is removed', () => { | ||
| const rootGraph = new LGraph() | ||
| const { subgraph } = createSubgraphWithNodes(rootGraph, 1) | ||
| const subgraphId = subgraph.id | ||
|
|
||
| const subgraphNode = createTestSubgraphNode(subgraph, { pos: [100, 100] }) | ||
| rootGraph.add(subgraphNode) | ||
|
|
||
| expect(rootGraph.subgraphs.has(subgraphId)).toBe(true) | ||
|
|
||
| rootGraph.remove(subgraphNode) | ||
|
|
||
| expect(rootGraph.subgraphs.has(subgraphId)).toBe(false) | ||
| }) |
Contributor
There was a problem hiding this comment.
Add coverage for “definition retained when other references exist.”
The suite covers removal and callback behavior, but it doesn’t exercise the retention case where another SubgraphNode still references the same subgraph definition. That’s part of the stated objectives and is a key branch in the new GC logic.
Based on learnings, add a test that:
- creates two
SubgraphNodes referencing the same subgraph definition, - removes one, and
- asserts
rootGraph.subgraphsstill contains the definition.
🤖 Prompt for AI Agents
In `@src/lib/litegraph/src/LGraph.test.ts` around lines 230 - 277, Add a test that
ensures a subgraph definition is retained when another SubgraphNode still
references it: create an LGraph, call createSubgraphWithNodes(rootGraph, n) to
get a subgraph, then create two SubgraphNode instances for that same subgraph
via createTestSubgraphNode(subgraph, ...), add both to rootGraph, remove one
with rootGraph.remove(theFirstSubgraphNode), and assert that
rootGraph.subgraphs.has(subgraph.id) is still true; use the existing test style
and the same helpers (LGraph, createSubgraphWithNodes, createTestSubgraphNode,
rootGraph.subgraphs) so the new test covers the “definition retained when other
references exist” branch.
104e2e3 to
12ed001
Compare
dosubot
bot
added
size:M
AustinMroz
approved these changes
Jan 26, 2026
christian-byrne
force-pushed
the
fix/subgraph-definition-gc
branch
from
12ed001 to
eed245b
Compare
Contributor
Author
|
Merging now so we can test on 1.39 |
christian-byrne
added a commit
that referenced
this pull request
Jan 30, 2026
#8187) ## Summary When removing a SubgraphNode via `LGraph.remove()`: - Fire `onRemoved` for all nodes inside the subgraph - Fire `onNodeRemoved` callback on the subgraph for each inner node - Remove subgraph definition from `rootGraph.subgraphs` when no other nodes reference it (checks both root graph nodes and nodes inside other subgraphs) This ensures proper cleanup of subgraph definitions and lifecycle callbacks for nested nodes when subgraph nodes are deleted. ## Changes ### LGraph.ts Added SubgraphNode-specific cleanup in `remove()` method that: 1. Iterates inner nodes and fires their `onRemoved` callbacks 2. Fires `onNodeRemoved` on the subgraph for downstream listeners (e.g., minimap) 3. Garbage collects the subgraph definition when no other nodes reference it ### SubgraphNode.ts Fixed `graph` property to match `LGraphNode` lifecycle contract. Previously it was declared as `override readonly graph` via constructor parameter promotion, which prevented `LGraph.remove()` from setting `node.graph = null`. Changed to a regular mutable property with null guard in `rootGraph` getter. ### LGraph.test.ts Added 4 tests: - `removing SubgraphNode fires onRemoved for inner nodes` - `removing SubgraphNode fires onNodeRemoved callback` - `subgraph definition is removed when last referencing node is removed` - `subgraph definition is retained when other nodes still reference it` ## Related - Fixes #8145 - Part of the subgraph lifecycle cleanup plan (Slice 2: Definition garbage collection)
snomiao
pushed a commit
that referenced
this pull request
Jan 30, 2026
#8187) ## Summary When removing a SubgraphNode via `LGraph.remove()`: - Fire `onRemoved` for all nodes inside the subgraph - Fire `onNodeRemoved` callback on the subgraph for each inner node - Remove subgraph definition from `rootGraph.subgraphs` when no other nodes reference it (checks both root graph nodes and nodes inside other subgraphs) This ensures proper cleanup of subgraph definitions and lifecycle callbacks for nested nodes when subgraph nodes are deleted. ## Changes ### LGraph.ts Added SubgraphNode-specific cleanup in `remove()` method that: 1. Iterates inner nodes and fires their `onRemoved` callbacks 2. Fires `onNodeRemoved` on the subgraph for downstream listeners (e.g., minimap) 3. Garbage collects the subgraph definition when no other nodes reference it ### SubgraphNode.ts Fixed `graph` property to match `LGraphNode` lifecycle contract. Previously it was declared as `override readonly graph` via constructor parameter promotion, which prevented `LGraph.remove()` from setting `node.graph = null`. Changed to a regular mutable property with null guard in `rootGraph` getter. ### LGraph.test.ts Added 4 tests: - `removing SubgraphNode fires onRemoved for inner nodes` - `removing SubgraphNode fires onNodeRemoved callback` - `subgraph definition is removed when last referencing node is removed` - `subgraph definition is retained when other nodes still reference it` ## Related - Fixes #8145 - Part of the subgraph lifecycle cleanup plan (Slice 2: Definition garbage collection)
DrJKL
pushed a commit
that referenced
this pull request
Jan 31, 2026
#8187) ## Summary When removing a SubgraphNode via `LGraph.remove()`: - Fire `onRemoved` for all nodes inside the subgraph - Fire `onNodeRemoved` callback on the subgraph for each inner node - Remove subgraph definition from `rootGraph.subgraphs` when no other nodes reference it (checks both root graph nodes and nodes inside other subgraphs) This ensures proper cleanup of subgraph definitions and lifecycle callbacks for nested nodes when subgraph nodes are deleted. ## Changes ### LGraph.ts Added SubgraphNode-specific cleanup in `remove()` method that: 1. Iterates inner nodes and fires their `onRemoved` callbacks 2. Fires `onNodeRemoved` on the subgraph for downstream listeners (e.g., minimap) 3. Garbage collects the subgraph definition when no other nodes reference it ### SubgraphNode.ts Fixed `graph` property to match `LGraphNode` lifecycle contract. Previously it was declared as `override readonly graph` via constructor parameter promotion, which prevented `LGraph.remove()` from setting `node.graph = null`. Changed to a regular mutable property with null guard in `rootGraph` getter. ### LGraph.test.ts Added 4 tests: - `removing SubgraphNode fires onRemoved for inner nodes` - `removing SubgraphNode fires onNodeRemoved callback` - `subgraph definition is removed when last referencing node is removed` - `subgraph definition is retained when other nodes still reference it` ## Related - Fixes #8145 - Part of the subgraph lifecycle cleanup plan (Slice 2: Definition garbage collection)
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.
Summary
When removing a SubgraphNode via
LGraph.remove():onRemovedfor all nodes inside the subgraphonNodeRemovedcallback on the subgraph for each inner noderootGraph.subgraphswhen no other nodes reference it (checks both root graph nodes and nodes inside other subgraphs)This ensures proper cleanup of subgraph definitions and lifecycle callbacks for nested nodes when subgraph nodes are deleted.
Changes
LGraph.ts
Added SubgraphNode-specific cleanup in
remove()method that:onRemovedcallbacksonNodeRemovedon the subgraph for downstream listeners (e.g., minimap)SubgraphNode.ts
Fixed
graphproperty to matchLGraphNodelifecycle contract. Previously it was declared asoverride readonly graphvia constructor parameter promotion, which preventedLGraph.remove()from settingnode.graph = null. Changed to a regular mutable property with null guard inrootGraphgetter.LGraph.test.ts
Added 4 tests:
removing SubgraphNode fires onRemoved for inner nodesremoving SubgraphNode fires onNodeRemoved callbacksubgraph definition is removed when last referencing node is removedsubgraph definition is retained when other nodes still reference itRelated