Fix edge cases in subgraph removal logic #8758
Conversation
Deeply nested subgraphs are also cleaned Adding a subgraphNode to the graph also ensures nested subgraphs are added to subgraph definitions
dosubot
bot
added
the
size:S
📝 WalkthroughWalkthroughThis change updates LGraph's node add/remove logic to traverse subgraphs: when adding a subgraph node it registers inner subgraph nodes with the root graph; when removing, it traverses inner nodes to call cleanup hooks and remove their subgraph entries from the root graph. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/09/2026, 09:21:25 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
|
Playwright: ✅ 515 passed, 0 failed · 4 flaky 📊 Browser Reports
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/litegraph/src/LGraph.ts`:
- Around line 1045-1053: Normalize the optional chaining and avoid premature
widget cleanup: make the isSubgraphNode usage consistent (either remove the ?.
in the earlier check or add it here) by editing calls to isSubgraphNode so they
match across LGraph (use plain isSubgraphNode() since it exists on LGraphNode),
and remove or defer the call to innerNode.onRemoved?.() inside the forEachNode
loop (where innerNode, innerNode.subgraph, onNodeRemoved and
this.rootGraph.subgraphs are handled) so that widget cleanup hooks are not
invoked during subgraph restructuring; instead only call onRemoved when a node
is being truly deleted from the root graph or add a guard that ensures onRemoved
runs only for permanent removals.
| if (node.isSubgraphNode()) { | ||
| forEachNode(node.subgraph, (innerNode) => { | ||
| innerNode.onRemoved?.() | ||
| subgraphNode.subgraph.onNodeRemoved?.(innerNode) | ||
| } | ||
| this.rootGraph.subgraphs.delete(subgraphNode.subgraph.id) | ||
| innerNode.graph?.onNodeRemoved?.(innerNode) | ||
| if (innerNode.isSubgraphNode()) | ||
| this.rootGraph.subgraphs.delete(innerNode.subgraph.id) | ||
| }) | ||
| this.rootGraph.subgraphs.delete(node.subgraph.id) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get context around the problematic lines in LGraph.ts
echo "=== Code context around lines 1045-1053 ==="
sed -n '1040,1060p' src/lib/litegraph/src/LGraph.ts
echo ""
echo "=== Code context around line 977 ==="
sed -n '970,985p' src/lib/litegraph/src/LGraph.ts
echo ""
echo "=== Search for isSubgraphNode definition ==="
rg -n 'isSubgraphNode' src/lib/litegraph --type=ts -A2 -B2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 5981
🏁 Script executed:
#!/bin/bash
# Check LGraphNode class definition to understand inheritance and method definitions
echo "=== Searching for LGraphNode class ==="
rg -n 'class LGraphNode' src/lib/litegraph --type=ts -A10
echo ""
echo "=== Check if isSubgraphNode is defined in LGraphNode ==="
ast-grep --pattern 'class LGraphNode {
$$$
isSubgraphNode() {
$$$
}
$$$
}'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1779
🏁 Script executed:
#!/bin/bash
# Search for onRemoved implementation and usage
echo "=== Search for onRemoved definition/usage ==="
rg -n 'onRemoved' src/lib/litegraph --type=ts -B2 -A2
echo ""
echo "=== Search for widget.onRemove usage ==="
rg -n 'widget\.onRemove' src/lib/litegraph --type=ts -B2 -A2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 6796
🏁 Script executed:
#!/bin/bash
# Check removeWidget implementation to understand the widget cleanup flow
echo "=== Search for removeWidget method ==="
rg -n 'removeWidget' src/lib/litegraph --type=ts -B2 -A5
echo ""
echo "=== Check widgetValueStore to understand state management ==="
rg -n 'widgetValueStore' src/lib/litegraph --type=ts -B1 -A1Repository: Comfy-Org/ComfyUI_frontend
Length of output: 5153
Harmonize optional chaining usage and reconsider calling onRemoved on inner nodes during subgraph removal.
Line 977 defensively uses node.isSubgraphNode?.() while lines 1045 and 1049 call isSubgraphNode() directly without optional chaining. Since isSubgraphNode() is defined on the base LGraphNode class, the optional chaining is unnecessary but the inconsistency should be resolved—either remove it from line 977 or add it to lines 1045 and 1049 for consistency.
More critically, calling innerNode.onRemoved?.() at line 1047 during subgraph parent removal may trigger widget cleanup handlers (widget.onRemove?.()) that assume permanent deletion. During subgraph conversion operations (convert-to-subgraph, unpack), inner nodes and their widgets may be restructured rather than permanently deleted. If onRemoved hooks unregister widget state from stores, this premature cleanup could cause state loss during legitimate graph restructuring flows. Consider whether inner nodes should have their onRemoved called at all in this context, or defer it until they are truly removed from the root graph.
🤖 Prompt for AI Agents
In `@src/lib/litegraph/src/LGraph.ts` around lines 1045 - 1053, Normalize the
optional chaining and avoid premature widget cleanup: make the isSubgraphNode
usage consistent (either remove the ?. in the earlier check or add it here) by
editing calls to isSubgraphNode so they match across LGraph (use plain
isSubgraphNode() since it exists on LGraphNode), and remove or defer the call to
innerNode.onRemoved?.() inside the forEachNode loop (where innerNode,
innerNode.subgraph, onNodeRemoved and this.rootGraph.subgraphs are handled) so
that widget cleanup hooks are not invoked during subgraph restructuring; instead
only call onRemoved when a node is being truly deleted from the root graph or
add a guard that ensures onRemoved runs only for permanent removals.
2 participants
#8187 made removal of subgraphs cleanup the subgraph definition for the removed graph and call onRemove handlers. However, it missed some edge cases and broke subgraph conversion of selections containing subgraphs which this PR tries to address.
Reminder: under this change, nodes can continue to exist after their onRemoved handler has been called
┆Issue is synchronized with this Notion page by Unito