fix: stabilize nested subgraph promoted widget resolution#9282
Merged
Conversation
- align projected widget disabled state with promoted view state during draw - add regression coverage for combo promotion with disabled interior widget Amp-Thread-ID: https://ampcode.com/threads/T-019c9c34-1142-726d-9fb2-b5b6dcb4c435 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c9c6e-aa59-73db-bca4-eb6bb2ffa2a5 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c9c6e-aa59-73db-bca4-eb6bb2ffa2a5 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c9d8e-8f2f-76c9-a819-231fd5c8e10d Co-authored-by: Amp <amp@ampcode.com>
- prevent subgraph definition deletion while other nodes still reference the same subgraph type - stop unpack from force-deleting definitions after removing the unpacked node - add regression coverage for unpacking one instance while another remains Amp-Thread-ID: https://ampcode.com/threads/T-019c9e10-f581-7091-b103-05ec41896bda Co-authored-by: Amp <amp@ampcode.com>
- make promoted widget resolution helper types internal to satisfy knip pre-push Amp-Thread-ID: https://ampcode.com/threads/T-019c9e10-f581-7091-b103-05ec41896bda Co-authored-by: Amp <amp@ampcode.com>
🎭 Playwright: ✅ 547 passed, 0 failed · 6 flaky📊 Browser Reports
|
🎨 Storybook: ✅ Built — View Storybook |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements multi-stage promoted-widget resolution across nested subgraphs, surfaces concrete source metadata into widget mappings (storeNodeId/storeName), adds buffered/linked promotion handling with stable view keys and caching, updates renderer/state lookups to use store-backed identifiers, and adds extensive tests and assets for nested promotion behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Consumer
participant GM as useGraphNodeManager
participant SN as SubgraphNode
participant RCP as resolveConcretePromotedWidget
participant PV as PromotedWidgetView
participant Store as WidgetStore
Consumer->>GM: request mapped widget info
GM->>SN: compute promotedSourceSeed / resolveSubgraphInputLink
GM->>RCP: resolveConcretePromotedWidget(hostNode, nodeId, widgetName)
RCP->>RCP: traverse chain (detect cycle, cap depth)
alt resolved
RCP-->>GM: ResolvedPromotedWidget { node, widget }
else failure
RCP-->>GM: failure reason
end
GM->>PV: derive effectiveWidget and store metadata (storeNodeId/storeName)
PV->>Store: lookup state using storeNodeId/storeName
Store-->>PV: widget state
PV-->>GM: SafeWidgetData (including storeNodeId/storeName, displayName, computedDisabled)
GM-->>Consumer: mapped widget payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🧹 Nitpick comments (2)
src/lib/litegraph/src/subgraph/PromotedWidgetViewManager.ts (1)
98-104: NamespaceviewKeyin cache keys to prevent accidental collisions.At Line 103, using the raw
viewKeycan cause unrelated widgets to reuse the same cached view if they share that value.Suggested adjustment
private makeKey( interiorNodeId: string, widgetName: string, viewKey?: string ): string { - return viewKey ?? `${interiorNodeId}:${widgetName}` + return viewKey + ? `${interiorNodeId}:${widgetName}:${viewKey}` + : `${interiorNodeId}:${widgetName}` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/subgraph/PromotedWidgetViewManager.ts` around lines 98 - 104, The cache key builder makeKey(interiorNodeId, widgetName, viewKey) currently returns the raw viewKey which can collide across widgets; change it to namespace the viewKey (e.g., prefix with a fixed token like "vk:" or include widgetName) before using it in the key so cached entries are unique per widget and node. Update makeKey to compute a namespacedViewKey from viewKey when present and then return `${interiorNodeId}:${widgetName}:${namespacedViewKey}` (or omit the extra segment when viewKey is absent) so other functions using makeKey get distinct keys.src/core/graph/subgraph/promotedWidgetView.test.ts (1)
995-1368: Extract a shared two-layer subgraph fixture to reduce test drift.The same nested graph construction is repeated across multiple tests; pulling it into one helper will make future resolver changes safer and reduce copy/paste maintenance.
♻️ Suggested refactor sketch
+function createTwoLayerPromotionFixture() { + const subgraphA = createTestSubgraph({ + inputs: [{ name: 'a_input', type: '*' }] + }) + const innerNode = new LGraphNode('InnerNode') + const innerInput = innerNode.addInput('picker_input', '*') + const widget = innerNode.addWidget('combo', 'picker', 'a', () => {}, { + values: ['a', 'b'] + }) + innerInput.widget = { name: 'picker' } + subgraphA.add(innerNode) + subgraphA.inputNode.slots[0].connect(innerInput, innerNode) + + const subgraphNodeA = createTestSubgraphNode(subgraphA, { id: 11 }) + const subgraphB = createTestSubgraph({ + inputs: [{ name: 'b_input', type: '*' }] + }) + subgraphB.add(subgraphNodeA) + subgraphNodeA._internalConfigureAfterSlots() + subgraphB.inputNode.slots[0].connect(subgraphNodeA.inputs[0], subgraphNodeA) + const subgraphNodeB = createTestSubgraphNode(subgraphB, { id: 22 }) + + return { subgraphNodeA, subgraphNodeB, innerNode, widget } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/graph/subgraph/promotedWidgetView.test.ts` around lines 995 - 1368, Many tests repeat the same two-layer nested subgraph construction (using createTestSubgraph, createTestSubgraphNode, LGraphNode, innerNode, subgraphNodeA, subgraphNodeB, input wiring and widget setup); extract that setup into a single helper (e.g., createTwoLayerPromotedSubgraph) that returns the relevant objects (subgraphA, innerNode, comboWidget/inner widgets, subgraphNodeA, subgraphB, subgraphNodeB) and update each test to call this helper and operate on the returned handles instead of duplicating the construction logic so tests are shorter and less error-prone when the resolver changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/composables/graph/useGraphNodeManager.ts`:
- Line 348: The slotName assignment uses an undefined identifier `name` which
breaks TypeScript; update the ternary to compare against the in-scope
`displayName` (i.e., set slotName: displayName !== widget.name ? widget.name :
undefined) so the code compiles; locate this in the useGraphNodeManager
composable where `slotName` is set alongside `widget` and `displayName`.
In `@src/core/graph/subgraph/resolveConcretePromotedWidget.ts`:
- Around line 82-92: The cycle detection uses a string key built from
currentHost.id which can collide across different host objects; in
resolveConcretePromotedWidget replace use of currentHost.id in visitKey with a
host-object-unique identifier by creating a WeakMap<Host, number> (e.g.,
hostUidMap) and assign an incremental uid per host, then build visitKey =
`${hostUidMap.get(currentHost)}:${currentNodeId}:${currentWidgetName}`; apply
the same change to the other occurrence that builds visitKey (the second
cycle-check block using visited, visitKey,
currentHost/currentNodeId/currentWidgetName) so cycle tracking is scoped to host
object identity rather than numeric id.
In `@src/lib/litegraph/src/subgraph/SubgraphNode.ts`:
- Around line 223-225: The promotion write can occur while the node id is still
-1, so guard the store reconciliation to avoid persisting promotions for a
placeholder id: before calling store.setPromotions(this.rootGraph.id, this.id,
finalEntries) (inside the hasCompleteLinkedCoverage && hasChanged branch),
ensure this.id !== -1 (and/or that the node has been added) and that widgets
haven’t been accessed prematurely; only call store.setPromotions when the node
has a valid id to respect the deferred-promotion contract.
---
Nitpick comments:
In `@src/core/graph/subgraph/promotedWidgetView.test.ts`:
- Around line 995-1368: Many tests repeat the same two-layer nested subgraph
construction (using createTestSubgraph, createTestSubgraphNode, LGraphNode,
innerNode, subgraphNodeA, subgraphNodeB, input wiring and widget setup); extract
that setup into a single helper (e.g., createTwoLayerPromotedSubgraph) that
returns the relevant objects (subgraphA, innerNode, comboWidget/inner widgets,
subgraphNodeA, subgraphB, subgraphNodeB) and update each test to call this
helper and operate on the returned handles instead of duplicating the
construction logic so tests are shorter and less error-prone when the resolver
changes.
In `@src/lib/litegraph/src/subgraph/PromotedWidgetViewManager.ts`:
- Around line 98-104: The cache key builder makeKey(interiorNodeId, widgetName,
viewKey) currently returns the raw viewKey which can collide across widgets;
change it to namespace the viewKey (e.g., prefix with a fixed token like "vk:"
or include widgetName) before using it in the key so cached entries are unique
per widget and node. Update makeKey to compute a namespacedViewKey from viewKey
when present and then return
`${interiorNodeId}:${widgetName}:${namespacedViewKey}` (or omit the extra
segment when viewKey is absent) so other functions using makeKey get distinct
keys.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/composables/graph/useGraphNodeManager.tssrc/core/graph/subgraph/promotedWidgetView.test.tssrc/core/graph/subgraph/promotedWidgetView.tssrc/core/graph/subgraph/resolveConcretePromotedWidget.tssrc/core/graph/subgraph/resolvePromotedWidgetSource.tssrc/lib/litegraph/src/LGraph.test.tssrc/lib/litegraph/src/LGraph.tssrc/lib/litegraph/src/subgraph/PromotedWidgetViewManager.tssrc/lib/litegraph/src/subgraph/SubgraphInput.tssrc/lib/litegraph/src/subgraph/SubgraphNode.tssrc/renderer/extensions/vueNodes/components/NodeWidgets.vuesrc/renderer/extensions/vueNodes/widgets/utils/resolvePromotedWidget.test.ts
Amp-Thread-ID: https://ampcode.com/threads/T-019c9e3c-3cd9-735d-bea2-003c803dd83e Co-authored-by: Amp <amp@ampcode.com>
📦 Bundle: 4.47 MB gzip 🔴 +2.74 kBDetailsSummary
Category Glance App Entry Points — 17.9 kB (baseline 17.9 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.03 MB (baseline 1.03 MB) • 🔴 +2.16 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 72.1 kB (baseline 72.1 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 435 kB (baseline 435 kB) • 🔴 +1 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 736 B (baseline 736 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 47 kB (baseline 47 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 2.57 MB (baseline 2.56 MB) • 🔴 +11.3 kBStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 55.5 kB (baseline 55.5 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 11 added / 11 removed Vendor & Third-Party — 8.84 MB (baseline 8.84 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.87 MB (baseline 7.87 MB) • ⚪ 0 BBundles that do not match a named category
Status: 52 added / 52 removed |
⚡ Performance Report
Raw data{
"timestamp": "2026-02-28T21:40:29.956Z",
"gitSha": "353ffab64eb2ac67619db051f898a78a006e6b50",
"branch": "drjkl/preview-fix",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2021.3099999999997,
"styleRecalcs": 123,
"styleRecalcDurationMs": 19.648,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 423.928,
"heapDeltaBytes": -2584984
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2016.7120000000125,
"styleRecalcs": 181,
"styleRecalcDurationMs": 55.801,
"layouts": 12,
"layoutDurationMs": 3.172,
"taskDurationMs": 1032.649,
"heapDeltaBytes": -3854088
},
{
"name": "dom-widget-clipping",
"durationMs": 581.1559999999929,
"styleRecalcs": 43,
"styleRecalcDurationMs": 13.901999999999997,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 362.695,
"heapDeltaBytes": 7622124
}
]
} |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/litegraph/src/subgraph/SubgraphNode.ts (1)
174-176:⚠️ Potential issue | 🟠 MajorGuard reconciliation writes until the node has a real id.
Line 175 can still persist entries for
this.id === -1whenwidgetsis read pre-add, which bypasses the deferred promotion flow and can leave orphaned promotions.🐛 Proposed fix
- if (shouldPersistLinkedOnly && hasChanged) { + if (this.id !== -1 && shouldPersistLinkedOnly && hasChanged) { store.setPromotions(this.rootGraph.id, this.id, mergedEntries) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/subgraph/SubgraphNode.ts` around lines 174 - 176, The code currently calls store.setPromotions(this.rootGraph.id, this.id, mergedEntries) when shouldPersistLinkedOnly && hasChanged, which can persist promotions for a sentinel id (-1) if widgets were read before the node was added; update the guard to also require the node has a real id (e.g., this.id !== -1) before calling store.setPromotions (or otherwise defer the write until after the node is assigned a real id), so change the conditional around the setPromotions call in SubgraphNode (referencing shouldPersistLinkedOnly, hasChanged, this.id, store.setPromotions, mergedEntries, this.rootGraph.id) to prevent orphaned promotions.
🧹 Nitpick comments (2)
src/composables/graph/useGraphNodeManager.ts (1)
290-306: Verify the graph ID parameter usage.Line 294 passes an empty string when
nodeis not a subgraph node. Based on the function signature_graphId: string(underscore prefix), this parameter appears unused, but consider adding a comment explaining this or using a more explicit sentinel value.The resolution result handling correctly extracts the resolved source with proper fallback to the original widget.
Suggestion: Add clarifying comment
const resolvedSourceResult = isPromotedWidgetView(widget) && promotedSourceSeed ? resolveConcretePromotedWidget( node, - node.isSubgraphNode() ? node.rootGraph.id : '', + // Graph ID parameter is currently unused by the resolver + node.isSubgraphNode() ? node.rootGraph.id : '', promotedSourceSeed.sourceNodeId, promotedSourceSeed.sourceWidgetName ) : null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/composables/graph/useGraphNodeManager.ts` around lines 290 - 306, The call to resolveConcretePromotedWidget passes node.rootGraph.id when node.isSubgraphNode() is true but an empty string otherwise; clarify intent by adding a brief comment next to this call explaining that the _graphId parameter is intentionally an unused/placeholder value for non-subgraph nodes (or note that resolveConcretePromotedWidget treats empty string as a sentinel), referencing resolveConcretePromotedWidget, node.isSubgraphNode(), and node.rootGraph.id so future readers know this is deliberate and not a bug.src/lib/litegraph/src/subgraph/SubgraphNode.ts (1)
144-152: Use linear-time dedup in linked entry collection.This dedup runs in a hot getter path and is currently O(n²) (
findIndexinsidefilter). ASet-based key check will be simpler and scale better.♻️ Proposed refactor
- return linkedEntries.filter((entry, index, array) => { - const firstIndex = array.findIndex( - (candidate) => - candidate.inputName === entry.inputName && - candidate.interiorNodeId === entry.interiorNodeId && - candidate.widgetName === entry.widgetName - ) - return firstIndex === index - }) + const seen = new Set<string>() + const deduped: Array<{ + inputName: string + interiorNodeId: string + widgetName: string + }> = [] + + for (const entry of linkedEntries) { + const key = this._makePromotionViewKey( + entry.inputName, + entry.interiorNodeId, + entry.widgetName + ) + if (seen.has(key)) continue + seen.add(key) + deduped.push(entry) + } + + return deduped🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/subgraph/SubgraphNode.ts` around lines 144 - 152, The current O(n²) dedup using linkedEntries.filter with array.findIndex should be replaced with a linear-time Set-based dedup: iterate linkedEntries once, build a Set of composite keys (e.g. `${entry.inputName}:${entry.interiorNodeId}:${entry.widgetName}`) and push each entry to the result array only if its key is not already in the Set; update the code at the linkedEntries.filter location (the dedup block referencing entry.inputName, entry.interiorNodeId, entry.widgetName) to perform this single-pass Set check and return the result array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/litegraph/src/subgraph/SubgraphNode.ts`:
- Around line 174-176: The code currently calls
store.setPromotions(this.rootGraph.id, this.id, mergedEntries) when
shouldPersistLinkedOnly && hasChanged, which can persist promotions for a
sentinel id (-1) if widgets were read before the node was added; update the
guard to also require the node has a real id (e.g., this.id !== -1) before
calling store.setPromotions (or otherwise defer the write until after the node
is assigned a real id), so change the conditional around the setPromotions call
in SubgraphNode (referencing shouldPersistLinkedOnly, hasChanged, this.id,
store.setPromotions, mergedEntries, this.rootGraph.id) to prevent orphaned
promotions.
---
Nitpick comments:
In `@src/composables/graph/useGraphNodeManager.ts`:
- Around line 290-306: The call to resolveConcretePromotedWidget passes
node.rootGraph.id when node.isSubgraphNode() is true but an empty string
otherwise; clarify intent by adding a brief comment next to this call explaining
that the _graphId parameter is intentionally an unused/placeholder value for
non-subgraph nodes (or note that resolveConcretePromotedWidget treats empty
string as a sentinel), referencing resolveConcretePromotedWidget,
node.isSubgraphNode(), and node.rootGraph.id so future readers know this is
deliberate and not a bug.
In `@src/lib/litegraph/src/subgraph/SubgraphNode.ts`:
- Around line 144-152: The current O(n²) dedup using linkedEntries.filter with
array.findIndex should be replaced with a linear-time Set-based dedup: iterate
linkedEntries once, build a Set of composite keys (e.g.
`${entry.inputName}:${entry.interiorNodeId}:${entry.widgetName}`) and push each
entry to the result array only if its key is not already in the Set; update the
code at the linkedEntries.filter location (the dedup block referencing
entry.inputName, entry.interiorNodeId, entry.widgetName) to perform this
single-pass Set check and return the result array.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/composables/graph/useGraphNodeManager.tssrc/lib/litegraph/src/subgraph/SubgraphNode.ts
…aversal - Extract resolveSubgraphInputLink helper to deduplicate link traversal - Unify resolveConcretePromotedWidget into traversePromotedWidgetChain - Use discriminated union (status field) for resolution results - Remove unused _graphId parameter from resolution functions - Add max-depth guard for promoted widget chain traversal - Use value-based cache keys in PromotedWidgetViewManager - Extract _syncPromotions, _removePromotedView helpers in SubgraphNode - Use design tokens for disconnected placeholder rendering - Add unit tests for resolveConcretePromotedWidget Amp-Thread-ID: https://ampcode.com/threads/T-019ca0a1-e047-75b9-9583-2f08a0efffe1 Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/core/graph/subgraph/resolveConcretePromotedWidget.ts (1)
25-35:⚠️ Potential issue | 🟠 MajorCycle detection key is not host-identity-safe.
Line 31 keys visits by
currentHost.id, which can collide across different nested hosts and trigger false'cycle'failures in valid traversals.Suggested fix
function traversePromotedWidgetChain( hostNode: SubgraphNode, nodeId: string, widgetName: string ): PromotedWidgetResolutionResult { const visited = new Set<string>() + const hostUidMap = new WeakMap<SubgraphNode, number>() + let nextHostUid = 0 let currentHost = hostNode let currentNodeId = nodeId let currentWidgetName = widgetName for (let depth = 0; depth < MAX_PROMOTED_WIDGET_CHAIN_DEPTH; depth++) { - const key = `${currentHost.id}:${currentNodeId}:${currentWidgetName}` + let hostUid = hostUidMap.get(currentHost) + if (hostUid === undefined) { + hostUid = nextHostUid++ + hostUidMap.set(currentHost, hostUid) + } + const key = `${hostUid}:${currentNodeId}:${currentWidgetName}` if (visited.has(key)) { return { status: 'failure', failure: 'cycle' } } visited.add(key)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/graph/subgraph/resolveConcretePromotedWidget.ts` around lines 25 - 35, The cycle-detection key currently uses currentHost.id which can collide across different host objects; replace the stringified key construction by deriving a host-identity that is stable and unique per host object (not its .id property) — e.g. create a WeakMap<HostNode, number> (or a Map with generated symbol ids) to assign each host object a unique numeric id and use that hostIdentity in the key; update the key construction (used with visited Set in resolveConcretePromotedWidget) from `${currentHost.id}:${currentNodeId}:${currentWidgetName}` to something like `${hostIdentity}:${currentNodeId}:${currentWidgetName}`, ensuring the WeakMap lookup/assignment happens outside or just before the loop so different host instances cannot collide.
🧹 Nitpick comments (2)
src/lib/litegraph/src/subgraph/SubgraphNode.ts (2)
199-199: Type precision:undefinedkey appears unused.The return type
Map<string | undefined, string>suggests undefined keys are valid, but_buildDisplayNameByViewKeyalways produces string keys via_makePromotionViewKey. Consider tightening toMap<string, string>for clarity.♻️ Proposed type refinement
- displayNameByViewKey: Map<string | undefined, string> + displayNameByViewKey: Map<string, string>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/subgraph/SubgraphNode.ts` at line 199, The Map type for displayNameByViewKey is too permissive—change its declaration from Map<string | undefined, string> to Map<string, string> and update any declarations/returns in SubgraphNode where this map is constructed (notably _buildDisplayNameByViewKey) to reflect that _makePromotionViewKey always returns a string; ensure callers of displayNameByViewKey and the return type of _buildDisplayNameByViewKey (and any related signatures) accept Map<string, string> so types remain consistent across SubgraphNode and its consumers.
128-136: Side effect in collection method.Lines 132-133 mutate
input.widgetwhile collecting linked entries. This side effect in a method that appears to be a pure collector could cause subtle issues if called during different lifecycle phases. Consider extracting the widget initialization to a dedicated sync method, or document that this method has side effects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/subgraph/SubgraphNode.ts` around lines 128 - 136, The loop that builds linkedEntries mutates input.widget (lines with input.widget ??= and input.widget.name =), causing side effects during what should be a pure collection; remove those two mutations from the collector and extract them into a new method on SubgraphNode (e.g., initializeInputWidgets or ensureInputWidgets) which iterates this.inputs and sets widget = { name: input.name } and widget.name = input.name when missing; update call sites to invoke initializeInputWidgets at the appropriate lifecycle points before the collector runs (or, if mutation must remain, add a clear comment on the collector method documenting the side effect) and keep references to _resolveLinkedPromotionByInputName and linkedEntries unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/composables/graph/useGraphNodeManager.ts`:
- Around line 391-402: The reactive computed getter for safeWidgets repopulates
the persistent Map slotMetadata without clearing it, causing stale entries when
node.inputs change; modify the safeWidgets computation (inside the
reactiveComputed used to produce safeWidgets) to call slotMetadata.clear()
before iterating node.inputs and repopulating it so slotMetadata only contains
current input/widget entries used by safeWidgetMapper.
In `@src/core/graph/subgraph/resolveSubgraphInputLink.ts`:
- Around line 30-31: The code in resolveSubgraphInputLink is assuming
inputNode.inputs exists before calling find, which can throw on partial/stale
graphs; update the logic around the inputNode and inputs lookup (where
targetInput is assigned from inputNode.inputs.find with linkId) to first check
that inputNode is defined and that inputNode.inputs is an Array (e.g.
Array.isArray(inputNode.inputs)) before calling find, and if not present simply
continue the loop or handle the missing inputs case the same way it previously
handled a missing targetInput so resolution doesn't short-circuit.
---
Duplicate comments:
In `@src/core/graph/subgraph/resolveConcretePromotedWidget.ts`:
- Around line 25-35: The cycle-detection key currently uses currentHost.id which
can collide across different host objects; replace the stringified key
construction by deriving a host-identity that is stable and unique per host
object (not its .id property) — e.g. create a WeakMap<HostNode, number> (or a
Map with generated symbol ids) to assign each host object a unique numeric id
and use that hostIdentity in the key; update the key construction (used with
visited Set in resolveConcretePromotedWidget) from
`${currentHost.id}:${currentNodeId}:${currentWidgetName}` to something like
`${hostIdentity}:${currentNodeId}:${currentWidgetName}`, ensuring the WeakMap
lookup/assignment happens outside or just before the loop so different host
instances cannot collide.
---
Nitpick comments:
In `@src/lib/litegraph/src/subgraph/SubgraphNode.ts`:
- Line 199: The Map type for displayNameByViewKey is too permissive—change its
declaration from Map<string | undefined, string> to Map<string, string> and
update any declarations/returns in SubgraphNode where this map is constructed
(notably _buildDisplayNameByViewKey) to reflect that _makePromotionViewKey
always returns a string; ensure callers of displayNameByViewKey and the return
type of _buildDisplayNameByViewKey (and any related signatures) accept
Map<string, string> so types remain consistent across SubgraphNode and its
consumers.
- Around line 128-136: The loop that builds linkedEntries mutates input.widget
(lines with input.widget ??= and input.widget.name =), causing side effects
during what should be a pure collection; remove those two mutations from the
collector and extract them into a new method on SubgraphNode (e.g.,
initializeInputWidgets or ensureInputWidgets) which iterates this.inputs and
sets widget = { name: input.name } and widget.name = input.name when missing;
update call sites to invoke initializeInputWidgets at the appropriate lifecycle
points before the collector runs (or, if mutation must remain, add a clear
comment on the collector method documenting the side effect) and keep references
to _resolveLinkedPromotionByInputName and linkedEntries unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/composables/graph/useGraphNodeManager.tssrc/core/graph/subgraph/promotedWidgetTypes.tssrc/core/graph/subgraph/promotedWidgetView.test.tssrc/core/graph/subgraph/promotedWidgetView.tssrc/core/graph/subgraph/resolveConcretePromotedWidget.test.tssrc/core/graph/subgraph/resolveConcretePromotedWidget.tssrc/core/graph/subgraph/resolvePromotedWidgetSource.tssrc/core/graph/subgraph/resolveSubgraphInputLink.tssrc/lib/litegraph/src/subgraph/PromotedWidgetViewManager.tssrc/lib/litegraph/src/subgraph/SubgraphNode.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/litegraph/src/subgraph/PromotedWidgetViewManager.ts
PromotedWidgetView.computedDisabled always returned false and ignored updates from updateComputedDisabled(), so externally linked slots were never visually disabled. DomWidget.vue read computedDisabled from the original inner widget (always true because it's linked to SubgraphInput) instead of the PromotedWidgetView that owns the position override. - Store computedDisabled in PromotedWidgetView so linked slots disable - DomWidget.vue reads from positionOverride.widget when overridden - Add nested promotion E2E test with fixture Amp-Thread-ID: https://ampcode.com/threads/T-019ca0b0-4e18-73e8-8d2d-6f2a0ee273be Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
browser_tests/tests/subgraphPromotion.spec.ts (1)
594-623: Consider using a more robust disabled detection.The test relies on wrapper opacity to determine if a textarea is editable. While this works, it's coupled to the specific CSS implementation of the disabled state.
Consider checking the
disabledattribute orreadonlyproperty of the textarea directly, or asserting against the known widget names that should be editable based on the test fixture:// Alternative: Check for disabled attribute directly const isDisabled = await textarea.evaluate((el) => (el as HTMLTextAreaElement).disabled || (el as HTMLTextAreaElement).readOnly )That said, if the opacity styling is the intended UX indicator and this test is specifically validating that UX, the current approach is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@browser_tests/tests/subgraphPromotion.spec.ts` around lines 594 - 623, The test "Unlinked promoted textarea widgets are editable on the subgraph exterior" currently infers editability by reading the wrapper's CSS opacity; replace that brittle check with a direct DOM property check on each textarea (use the existing textareas and textarea variables) by evaluating the element's disabled and readOnly properties and only attempt fill/assert when those properties are false; update the loop around textareas.nth(i) and the conditional that uses opacity to instead call textarea.evaluate to read (el as HTMLTextAreaElement).disabled || (el as HTMLTextAreaElement).readOnly and proceed when that result is false so the test asserts real editability rather than a visual style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/graph/widgets/DomWidget.vue`:
- Around line 113-123: The override branch currently treats
override.widget.computedDisabled as false when undefined, which can re-enable
interactions; change the expression that computes isDisabled so that when
override exists you fall back to the base widget's computedDisabled: replace the
logic in the widgetState/override block (where isDisabled is computed) to use
override ? (override.widget.computedDisabled ?? widget.computedDisabled) :
widget.computedDisabled so pointerEvents and opacity continue to derive from the
correct disabled source.
---
Nitpick comments:
In `@browser_tests/tests/subgraphPromotion.spec.ts`:
- Around line 594-623: The test "Unlinked promoted textarea widgets are editable
on the subgraph exterior" currently infers editability by reading the wrapper's
CSS opacity; replace that brittle check with a direct DOM property check on each
textarea (use the existing textareas and textarea variables) by evaluating the
element's disabled and readOnly properties and only attempt fill/assert when
those properties are false; update the loop around textareas.nth(i) and the
conditional that uses opacity to instead call textarea.evaluate to read (el as
HTMLTextAreaElement).disabled || (el as HTMLTextAreaElement).readOnly and
proceed when that result is false so the test asserts real editability rather
than a visual style.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
browser_tests/assets/subgraphs/subgraph-nested-promotion.jsonbrowser_tests/tests/subgraphPromotion.spec.tssrc/components/graph/widgets/DomWidget.vuesrc/core/graph/subgraph/promotedWidgetView.ts
Prevents stale entries from accumulating when node.inputs change, which could cause safeWidgetMapper to return incorrect slot metadata. Amp-Thread-ID: https://ampcode.com/threads/T-019ca0c1-d0be-76f6-9ee3-2d7a13a86fb5 Co-authored-by: Amp <amp@ampcode.com>
When override.widget.computedDisabled is undefined, fall back to the base widget's computedDisabled instead of false to prevent re-enabling disabled widgets. Amp-Thread-ID: https://ampcode.com/threads/T-019ca0c1-d0be-76f6-9ee3-2d7a13a86fb5 Co-authored-by: Amp <amp@ampcode.com>
The setter is not a no-op; it accepts boolean values and treats undefined as false. Split into two tests covering both behaviors. Amp-Thread-ID: https://ampcode.com/threads/T-019ca0c1-d0be-76f6-9ee3-2d7a13a86fb5 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019ca131-86bd-754d-9633-cbe5c4a89012 Co-authored-by: Amp <amp@ampcode.com>
dosubot
bot
added
the
size:XXL
| forEachNode(node.subgraph, (innerNode) => { | ||
| innerNode.onRemoved?.() | ||
| innerNode.graph?.onNodeRemoved?.(innerNode) | ||
| }) |
Contributor
There was a problem hiding this comment.
issue (non-blocking): when the last reference to a subgraph node is removed, the old code also deleted rootGraph.subgraphs entries for nested SubgraphNodes via forEachNode. That recursive cleanup was dropped here, so nested subgraph definitions remain orphaned in rootGraph.subgraphs until the workflow is reloaded.
Serialization already filters by usedSubgraphIds, so this isn't a correctness bug, but it is a session-lifetime memory leak for workflows that repeatedly create/remove deeply nested subgraphs, and the allGraphs scan in this same method grows monotonically.
I realize there's a lot of context and recent PRs around this area, and this is likely an accepted tradeoff for this PR to fix the immediate bugs. For the long term though, we need a better architecture for subgraph definition lifecycle management - probably something that reconciles definitions centrally rather than relying on node removal callbacks. Worth tracking as a follow-up.
| widgetName: string, | ||
| viewKey?: string | ||
| ): string { | ||
| return viewKey ?? `${interiorNodeId}:${widgetName}` |
Contributor
There was a problem hiding this comment.
issue (non-blocking): makeKey returns viewKey ?? \${interiorNodeId}:${widgetName}`- whenviewKeyis provided,interiorNodeIdandwidgetName` are completely ignored. This means:
-
remove(id, name)generates${id}:${name}but cannot find entries stored under aviewKey -
_removePromotedViewworks around this by calling bothremove()andremoveByViewKey(), but that's a fragile implicit contract - During
reconcile's stale-key cleanup (line 54-56), entries stored under a viewKey cannot be evicted by their(id, name)pair
Consider either incorporating interiorNodeId:widgetName as a prefix in the viewKey path, or renaming the parameter to keyOverride and documenting the dual-keying contract explicitly.
Contributor
Author
There was a problem hiding this comment.
There once was a key with no base,
That wandered all over the place.
Now node-widget leads,
Then viewKey succeeds,
And cache ops all run in one space.
| y: number, | ||
| H: number | ||
| ) { | ||
| const backgroundColor = readDesignToken( |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): readDesignToken calls getComputedStyle(document.documentElement).getPropertyValue(token) four times per drawDisconnectedPlaceholder invocation. This is on the canvas draw path, so for N disconnected promoted widgets it's 4N getComputedStyle calls per frame. getComputedStyle can force style recalculation.
Since these CSS custom properties only change on theme switch, consider caching the values at module level - e.g. a lazy let cachedTokens: Record<string, string> | null that's populated on first call and invalidated via a matchMedia listener or theme-change event.
Contributor
Author
There was a problem hiding this comment.
A placeholder painted each frame,
With style reads again and again.
Now token values stay,
In a cache by the way,
So redraws are far less of a strain.
| private _getPromotedViews(): PromotedWidgetView[] { | ||
| const store = usePromotionStore() | ||
| const entries = store.getPromotionsRef(this.rootGraph.id, this.id) | ||
| const linkedEntries = this._getLinkedPromotionEntries() |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): _getPromotedViews() calls _getLinkedPromotionEntries() on every widgets getter access. That method iterates all inputs, resolves links via resolveSubgraphInputLink (which does .find() on slots, iterates linkIds, resolves links, and does another .find() on inner inputs), then deduplicates. The widgets getter is a very hot path - rendering, layout, serialization all hit it.
While PromotedWidgetViewManager.reconcile() caches the view objects, the entry key computation and link resolution preceding it runs unconditionally every time. Consider caching _getLinkedPromotionEntries() and invalidating only when inputs/links actually change (via the existing input-connected/input-disconnected/removing-input event handlers).
Contributor
Author
There was a problem hiding this comment.
A getter kept chasing each link,
On every tiny re-think.
Now entries are cached,
With invalidates matched,
So hot paths no longer must clink.
| } | ||
| } | ||
|
|
||
| function resolveSourceSeedByInputName(inputName: string): { |
Contributor
There was a problem hiding this comment.
suggestion: resolveSourceSeedByInputName is a near-exact copy of SubgraphNode._resolveLinkedPromotionByInputName (same branching logic, same resolveSubgraphInputLink call, just different field names: sourceNodeId vs interiorNodeId). This is change amplification - any future change to resolution semantics must be updated in both places.
Consider extracting a shared resolver function in resolveSubgraphInputLink.ts that both call sites use.
Contributor
Author
There was a problem hiding this comment.
Two resolvers marched side by side,
With branching and checks duplicated wide.
Now one helper knows,
Where the true target goes,
And both callers use that one guide.
| hostNode: LGraphNode, | ||
| widget: IBaseWidget | ||
| ): ResolvedPromotedWidgetSource | undefined { | ||
| ): ResolvedPromotedWidget | undefined { |
Contributor
There was a problem hiding this comment.
note (non-blocking): resolvePromotedWidgetSource now resolves through nested promoted widget chains (via resolveConcretePromotedWidget) instead of only resolving the immediate source. Extensions calling this function may now receive a deeper interior node/widget pair than before. The structural type shape is unchanged, but the behavioral contract is different. If any extension needs the shallow (one-level) resolution, they should use the new resolvePromotedWidgetAtHost export.
| expect(renderedText).toContain('a') | ||
| }) | ||
|
|
||
| test('draw shows value through two input-based promotion layers', () => { |
Contributor
There was a problem hiding this comment.
nitpick (non-blocking): the nested promotion tests ("draw shows value through two...", "value updates propagate...", "state lookup resolves...", etc.) repeat nearly identical 15-line setup blocks to create a two-level nested subgraph. Extracting a shared createTwoLevelNestedSubgraph() helper would reduce setup noise and make the unique assertion in each test stand out.
Contributor
Author
There was a problem hiding this comment.
Nested dawn once cloned,
One helper now sets the stage,
Assertions stand bright.
|
|
||
| const subgraphNodeB = createTestSubgraphNode(subgraphB, { id: 22 }) | ||
| const widgetValueStore = useWidgetValueStore() | ||
| const getWidgetSpy = vi.spyOn(widgetValueStore, 'getWidget') |
Contributor
There was a problem hiding this comment.
nitpick (non-blocking): this test uses vi.spyOn(widgetValueStore, 'getWidget') to assert that getWidget was called with the concrete inner node's ID. This is a change-detector test - if the implementation changes how it retrieves widget state (caching, batching), this breaks even though behavior is correct. The value propagation tests nearby already verify the same semantics more robustly by checking actual values.
Contributor
Author
There was a problem hiding this comment.
Spyglass set aside,
Behavior sings what is true,
Value proves the path.
| import type { SubgraphNode } from '@/lib/litegraph/src/subgraph/SubgraphNode' | ||
| import type { IBaseWidget } from '@/lib/litegraph/src/types/widgets' | ||
|
|
||
| type MockGraphNode = { |
Contributor
There was a problem hiding this comment.
nitpick (non-blocking): these tests use hand-rolled MockGraphNode/MockSubgraph types instead of the createTestSubgraph/createTestSubgraphNode helpers used elsewhere. This keeps tests fast but creates a divergence risk - if SubgraphNode gains new properties that affect isPromotedWidgetView or isSubgraphNode checks, these mocks won't catch regressions. Consider adding at least one integration-level test using real fixtures (the nested promotion tests in promotedWidgetView.test.ts may already cover this).
Contributor
Author
There was a problem hiding this comment.
Fixtures replace stage props.
Integration texture enters the test grain.
X-ray mocks give way to living subgraph bones.
The helpers (createTestSubgraph, createTestSubgraphNode) anchor reality.
Under drift risk, coverage hardens.
Regression now meets a truer mirror.
Edges hold.
DrJKL
commented
Feb 28, 2026
|
|
||
| test('Can drag node', { tag: '@screenshot' }, async ({ comfyPage }) => { | ||
| await comfyPage.nodeOps.dragTextEncodeNode2() | ||
| await comfyPage.nextFrame() |
Contributor
Author
There was a problem hiding this comment.
This is a stabilization fix for the test, the DOM widget z-index is inconsistent at point of screenshot otherwise.
Contributor
Author
There was a problem hiding this comment.
Might be worth coming back and adding a proper wait condition later.
Amp-Thread-ID: https://ampcode.com/threads/T-019ca5ea-3a44-713f-bf6d-e77f4ada2442 Co-authored-by: Amp <amp@ampcode.com>
DrJKL
added a commit
that referenced
this pull request
Feb 28, 2026
Fix multiple issues with promoted widget resolution in nested subgraphs, ensuring correct value propagation, slot matching, and rendering for deeply nested promoted widgets. - **What**: Stabilize nested subgraph promoted widget resolution chain - Use deep source keys for promoted widget values in Vue rendering mode - Resolve effective widget options from the source widget instead of the promoted view - Stabilize slot resolution for nested promoted widgets - Preserve combo value rendering for promoted subgraph widgets - Prevent subgraph definition deletion while other nodes still reference the same type - Clean up unused exported resolution types - `resolveConcretePromotedWidget.ts` — new recursive resolution logic for deeply nested promoted widgets - `useGraphNodeManager.ts` — option extraction now uses `effectiveWidget` for promoted widgets - `SubgraphNode.ts` — unpack no longer force-deletes definitions referenced by other nodes ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9282-fix-stabilize-nested-subgraph-promoted-widget-resolution-3146d73d365081208a4fe931bb7569cf) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <amp@ampcode.com> Co-authored-by: GitHub Action <action@github.com>
DrJKL
added
needs-backport
|
1 similar comment
|
DrJKL
added a commit
that referenced
this pull request
Mar 5, 2026
Amp-Thread-ID: https://ampcode.com/threads/T-019cbfdf-bf82-76de-b36c-91758530a0ce Co-authored-by: Amp <amp@ampcode.com>
DrJKL
added a commit
that referenced
this pull request
Mar 5, 2026
- Add svgBitmapCache.ts from #9172 (dependency of #9282) - Update drawImage to use workflowBitmapCache - Fix getWidget 3-arg signature in domWidget.ts and test - Remove unconditional subgraphs.delete in unpackSubgraph - Regenerate pnpm-lock.yaml Amp-Thread-ID: https://ampcode.com/threads/T-019cbfdf-bf82-76de-b36c-91758530a0ce Co-authored-by: Amp <amp@ampcode.com>
|
|
christian-byrne
pushed a commit
that referenced
this pull request
Mar 8, 2026
Fix multiple issues with promoted widget resolution in nested subgraphs, ensuring correct value propagation, slot matching, and rendering for deeply nested promoted widgets. - **What**: Stabilize nested subgraph promoted widget resolution chain - Use deep source keys for promoted widget values in Vue rendering mode - Resolve effective widget options from the source widget instead of the promoted view - Stabilize slot resolution for nested promoted widgets - Preserve combo value rendering for promoted subgraph widgets - Prevent subgraph definition deletion while other nodes still reference the same type - Clean up unused exported resolution types - `resolveConcretePromotedWidget.ts` — new recursive resolution logic for deeply nested promoted widgets - `useGraphNodeManager.ts` — option extraction now uses `effectiveWidget` for promoted widgets - `SubgraphNode.ts` — unpack no longer force-deletes definitions referenced by other nodes ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9282-fix-stabilize-nested-subgraph-promoted-widget-resolution-3146d73d365081208a4fe931bb7569cf) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <amp@ampcode.com> Co-authored-by: GitHub Action <action@github.com>
christian-byrne
added a commit
that referenced
this pull request
Mar 8, 2026
…esolution (#9282) (#9616) Backport of #9282 to core/1.40. MUST — user-facing subgraph widget resolution bug. 12 conflict files resolved: - 8 modify/delete: new files introduced by the PR (kept as new) - 1 add/add: resolveSubgraphInputTarget.ts (merged with existing from #9542 backport) - 3 content: accepted incoming changes **Original PR:** #9282 **Pipeline ticket:** 15e1f241-efaa-4fe5-88ca-4ccc7bfb3345 Co-authored-by: Alexander Brown <drjkl@comfy.org> Co-authored-by: Amp <amp@ampcode.com> Co-authored-by: GitHub Action <action@github.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
4 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 multiple issues with promoted widget resolution in nested subgraphs, ensuring correct value propagation, slot matching, and rendering for deeply nested promoted widgets.
Changes
Review Focus
resolveConcretePromotedWidget.ts— new recursive resolution logic for deeply nested promoted widgetsuseGraphNodeManager.ts— option extraction now useseffectiveWidgetfor promoted widgetsSubgraphNode.ts— unpack no longer force-deletes definitions referenced by other nodes┆Issue is synchronized with this Notion page by Unito