fix: make SubgraphNode.graph nullable to allow proper cleanup #8180
Conversation
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
dosubot
bot
added
the
size:L
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/30/2026, 08:28:52 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Tests: ✅ PassedResults: 507 passed, 0 failed, 0 flaky, 8 skipped (Total: 515) 📊 Browser Reports
|
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
📝 WalkthroughWalkthroughThis PR adds garbage collection for subgraph definitions. When a SubgraphNode is removed, the system now triggers cleanup callbacks for inner nodes, checks for other references to the subgraph definition, and deletes the definition if no remaining references exist. Changes
Suggested reviewers
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![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: 0
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)
988-1029: Type narrowing fromisSubgraphNode()causes pipeline failure.The
isSubgraphNode()type guard narrowsnodetoSubgraphNode. After the if block, TypeScript still considersnodeto potentially be aSubgraphNode, and ifSubgraphNodehasgraphas a readonly property, the assignment at line 1029 (node.graph = null) fails with error TS2540.🐛 Proposed fix - cast to restore assignability
// Handle SubgraphNode-specific cleanup if (node.isSubgraphNode()) { const subgraphId = node.subgraph.id // ... rest of subgraph cleanup } // callback - node.onRemoved?.() + ;(node as LGraphNode).onRemoved?.() - node.graph = null + ;(node as LGraphNode).graph = nullAlternatively, store a reference before type narrowing:
+ const graphNode: LGraphNode = node + // Handle SubgraphNode-specific cleanup if (node.isSubgraphNode()) { // ... subgraph cleanup } // callback - node.onRemoved?.() + graphNode.onRemoved?.() - node.graph = null + graphNode.graph = null
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 26 kB (baseline 26 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 974 kB (baseline 974 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.89 kB (baseline 2.89 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.71 MB (baseline 2.71 MB) • 🔴 +273 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.1 MB (baseline 7.1 MB) • 🟢 -198 BBundles that do not match a named category
Status: 34 added / 34 removed |
- Remove readonly from SubgraphNode.graph constructor parameter - Add override graph: GraphOrSubgraph | null as class property - Add NullGraphError guard in rootGraph getter - Add null guard in ExecutableNodeDTO.resolveInput - Add null guard in imagePreviewStore.revokeSubgraphPreviews - Update test files with non-null assertions where graph is accessed Amp-Thread-ID: https://ampcode.com/threads/T-019bdd79-24d5-73d6-bfb8-294c4a1e9592 Co-authored-by: Amp <amp@ampcode.com>
- Add node ID to NullGraphError messages for easier debugging - Add test verifying rootGraph throws after node removal Amp-Thread-ID: https://ampcode.com/threads/T-019c0dad-0101-74ca-92fd-0fd1a992d889 Co-authored-by: Amp <amp@ampcode.com>
christian-byrne
force-pushed
the
fix/subgraph-lifecycle-clear-recursive-teardown
branch
from
6ae4797 to
e757836
Compare
dosubot
bot
added
size:M
christian-byrne
changed the title
AustinMroz
approved these changes
Feb 4, 2026
christian-byrne
deleted the
fix/subgraph-lifecycle-clear-recursive-teardown
branch
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
Fix
SubgraphNode.graphproperty to matchLGraphNodelifecycle contract. Previously declared asoverride readonly graphvia constructor parameter promotion, which preventedLGraph.remove()from settingnode.graph = null.Changes
readonlyfromSubgraphNode.graphconstructor parameteroverride graph: GraphOrSubgraph | nullas class propertyNullGraphErrorguard inrootGraphgetter with node ID for debuggingExecutableNodeDTO.resolveInputandimagePreviewStore.revokeSubgraphPreviewsrootGraphthrows after node removalTesting
NullGraphErroris thrown when accessingrootGraphon removed node