fix: stabilize subgraph promoted widget identity and rendering#9896
Open
fix: stabilize subgraph promoted widget identity and rendering#9896
Conversation
Amp-Thread-ID: https://ampcode.com/threads/T-019ce89a-f2cb-74a2-bc08-8ede37bd0f4b Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019ce8ac-681e-752a-b879-8b01e27a1bd4 Co-authored-by: Amp <amp@ampcode.com>
Add null checks for app.canvas in onClick and onDoubleClick handlers to prevent TypeError when canvas is not yet initialized. Amp-Thread-ID: https://ampcode.com/threads/T-019ce8bc-bf85-778b-8f19-d1e2e715d797 Co-authored-by: Amp <amp@ampcode.com>
DrJKL
added
area:subgraph
area:widgets
area:litegraph
area:vue-nodes
🎨 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:
📝 WalkthroughWalkthroughAdds stable per-widget render keys and identity-based deduplication; shifts promoted-widget mapping from input-name to inputKey with per-node caches and rebinding; prefers linked input widget states for promoted reads/writes; introduces matchPromotedInput, widget render-key util, type guard, fixture helpers, and extensive tests. Changes
Sequence DiagramsequenceDiagram
participant SubgraphNode as SubgraphNode
participant Cache as PromotedViewsCache
participant PW as PromotedWidgetView
participant WidgetStore as WidgetValueStore
participant NodeWidgets as NodeWidgetsComponent
participant Renderer as Renderer
SubgraphNode->>Cache: read cached promoted views (by signature)
alt cache hit
Cache-->>SubgraphNode: return cached promoted views
else cache miss
SubgraphNode->>SubgraphNode: resolve linked promotions by subgraphInput (inputKey)
SubgraphNode->>SubgraphNode: build PromotionEntry[] and keys
SubgraphNode->>PW: create/update promoted views for each entry
PW->>WidgetStore: read/write linked input widget states
PW-->>SubgraphNode: return promoted widget descriptors
SubgraphNode->>Cache: write promoted views with new signature
end
SubgraphNode-->>NodeWidgets: provide combined widget list (native + promoted)
NodeWidgets->>NodeWidgets: compute widget identity & renderKey
NodeWidgets->>NodeWidgets: deduplicate by identity (prefer visible)
NodeWidgets->>Renderer: render deduplicated widgets with stable keys
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
🎭 Playwright: ✅ 557 passed, 0 failed · 6 flaky📊 Browser Reports
|
📦 Bundle: 4.98 MB gzip 🔴 +2.72 kBDetailsSummary
Category Glance App Entry Points — 21.7 kB (baseline 21.7 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.08 MB (baseline 1.08 MB) • 🔴 +426 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 75.3 kB (baseline 75.3 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 456 kB (baseline 456 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16.6 kB (baseline 16.6 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 81.8 kB (baseline 81.8 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 59 kB (baseline 59 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 3.17 MB (baseline 3.16 MB) • 🔴 +12.4 kBStores, services, APIs, and repositories
Status: 15 added / 15 removed Utilities & Hooks — 68.9 kB (baseline 68.9 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 12 added / 12 removed Vendor & Third-Party — 9.78 MB (baseline 9.78 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 8.18 MB (baseline 8.18 MB) • ⚪ 0 BBundles that do not match a named category
Status: 52 added / 52 removed |
⚡ Performance Report
Raw data{
"timestamp": "2026-03-14T08:31:20.983Z",
"gitSha": "3b12301c6d6c2e83446590c3f1744d5cc8374ccb",
"branch": "drjkl/fix/subgraph-widgets",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2046.9489999999837,
"styleRecalcs": 14,
"styleRecalcDurationMs": 14.616999999999997,
"layouts": 1,
"layoutDurationMs": 0.3070000000000001,
"taskDurationMs": 372.166,
"heapDeltaBytes": 1414804
},
{
"name": "canvas-idle",
"durationMs": 2037.113999999974,
"styleRecalcs": 10,
"styleRecalcDurationMs": 13.059999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 370.891,
"heapDeltaBytes": 851504
},
{
"name": "canvas-idle",
"durationMs": 2020.390999999961,
"styleRecalcs": 12,
"styleRecalcDurationMs": 10.482999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 350.95300000000003,
"heapDeltaBytes": -6008364
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2108.7589999999636,
"styleRecalcs": 85,
"styleRecalcDurationMs": 51.037000000000006,
"layouts": 12,
"layoutDurationMs": 3.828,
"taskDurationMs": 1112.489,
"heapDeltaBytes": -2370760
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1780.2770000000123,
"styleRecalcs": 74,
"styleRecalcDurationMs": 37.693,
"layouts": 12,
"layoutDurationMs": 3.45,
"taskDurationMs": 783.701,
"heapDeltaBytes": -2828240
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2029.42900000005,
"styleRecalcs": 84,
"styleRecalcDurationMs": 44.053000000000004,
"layouts": 12,
"layoutDurationMs": 3.5090000000000003,
"taskDurationMs": 978.9079999999999,
"heapDeltaBytes": -3446472
},
{
"name": "dom-widget-clipping",
"durationMs": 556.5619999999853,
"styleRecalcs": 13,
"styleRecalcDurationMs": 9.514,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 344.27799999999996,
"heapDeltaBytes": 10761080
},
{
"name": "dom-widget-clipping",
"durationMs": 569.0940000000069,
"styleRecalcs": 14,
"styleRecalcDurationMs": 8.690999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 351.279,
"heapDeltaBytes": 12564460
},
{
"name": "dom-widget-clipping",
"durationMs": 563.9620000000036,
"styleRecalcs": 13,
"styleRecalcDurationMs": 9.431999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 347.50899999999996,
"heapDeltaBytes": 12727016
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 553.7889999999948,
"styleRecalcs": 48,
"styleRecalcDurationMs": 13.054000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 348.0489999999999,
"heapDeltaBytes": 12557392
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 556.5530000000081,
"styleRecalcs": 49,
"styleRecalcDurationMs": 12.376999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 349.08399999999995,
"heapDeltaBytes": 12927456
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 593.9649999999688,
"styleRecalcs": 49,
"styleRecalcDurationMs": 12.736,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 368.65799999999996,
"heapDeltaBytes": 12626668
},
{
"name": "subgraph-idle",
"durationMs": 2015.7600000000002,
"styleRecalcs": 11,
"styleRecalcDurationMs": 11.067,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 419.06100000000004,
"heapDeltaBytes": 872164
},
{
"name": "subgraph-idle",
"durationMs": 1993.0309999999736,
"styleRecalcs": 12,
"styleRecalcDurationMs": 10.68,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 359.23,
"heapDeltaBytes": 54996
},
{
"name": "subgraph-idle",
"durationMs": 2009.7240000000056,
"styleRecalcs": 12,
"styleRecalcDurationMs": 11.234,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 349.9,
"heapDeltaBytes": 305292
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1670.7450000000108,
"styleRecalcs": 78,
"styleRecalcDurationMs": 38.99100000000001,
"layouts": 16,
"layoutDurationMs": 5.088,
"taskDurationMs": 663.2679999999999,
"heapDeltaBytes": -7841908
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 2022.153000000003,
"styleRecalcs": 90,
"styleRecalcDurationMs": 50.518,
"layouts": 17,
"layoutDurationMs": 4.667999999999999,
"taskDurationMs": 913.895,
"heapDeltaBytes": -6986120
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1714.334000000008,
"styleRecalcs": 77,
"styleRecalcDurationMs": 35.574,
"layouts": 16,
"layoutDurationMs": 4.1659999999999995,
"taskDurationMs": 654.842,
"heapDeltaBytes": -6399068
}
]
} |
![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: 4
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/subgraph/SubgraphNode.ts (1)
369-389:⚠️ Potential issue | 🟠 MajorDo not treat unresolved fallback promotions as stale during hydration.
When every input has a linked representative,
_shouldPersistLinkedOnly()returnstrueeven if some fallback entries are only temporarily unresolved._syncPromotions()then drops those entries from the store, so an independent promotion that has not rebound yet can be lost permanently during configure/hydration. This should only prune fallbacks that are positively proven to alias one of the linked entries.Also applies to: 424-440
🤖 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 369 - 389, The current logic in _buildPromotionPersistenceState (and the similar block around lines 424-440) calls _shouldPersistLinkedOnly and drops fallback entries whenever every input has a linked representative, which incorrectly prunes temporarily unresolved fallbacks; change the logic so we only persist linked-only when each fallbackStoredEntry is positively proven to alias a linkedPromotionEntry (i.e., modify _shouldPersistLinkedOnly to validate aliasing between fallbackStoredEntries and linkedPromotionEntries, or alternatively adjust _buildPromotionPersistenceState to include fallbackStoredEntries unless alias validation passes), and ensure _syncPromotions uses this stricter decision so independent promotions that are unresolved are not removed during hydration/configure; reference functions: _buildPromotionPersistenceState, _collectLinkedAndFallbackEntries, _shouldPersistLinkedOnly, and _syncPromotions.
🧹 Nitpick comments (1)
src/renderer/extensions/vueNodes/components/NodeWidgets.vue (1)
279-283: Drive visibility and dedupe from the merged widget options.
isWidgetVisible()and the storedhidden/advancedflags still readwidget.options, but the rendered widget useswidgetState?.optionsthroughwidgetOptions. If store state overrides visibility flags, the new dedupe logic can still keep the stale copy or reserve rows for a widget that will not render. Pulling visibility from the same merged options object would keep these paths consistent.Also applies to: 350-356, 401-406, 423-429
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/components/NodeWidgets.vue` around lines 279 - 283, isWidgetVisible and other places currently read visibility flags from the original SafeWidgetData.options, but the UI renders using the merged widgetOptions (from widgetState?.options), causing stale visibility/dedupe logic; change isWidgetVisible (and the references at the other occurrences) to read hidden/advanced from the merged widgetOptions used for rendering (e.g., use widgetOptions or compute mergedOptions = { ...widget.options, ...widgetState?.options } and then derive const hidden = mergedOptions.hidden ?? false and advanced = mergedOptions.advanced ?? false) so visibility and dedupe use the same source as the rendered widget.
🤖 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/rightSidePanel/parameters/SectionWidgets.vue`:
- Around line 96-105: getWidgetRenderKey currently includes runtime
widgetNode.id which changes when a node transitions from temporary (-1) to
configured, causing remounts; update getWidgetRenderKey so both promoted and
non-promoted keys use a stable, per-view identity instead of widgetNode.id (e.g.
use a persisted/stable id property on the widget or its view such as
widget.viewId or widget.stableId, or fallback to widget.sourceNodeId for
promoted widgets) — change the promoted branch (inside isPromotedWidgetView) and
the fallback branch to build keys from stable identifiers
(widget.sourceViewId/widget.sourceNodeId/widget.stableId and
widget.name/widget.type) rather than widgetNode.id so items do not remount on id
transitions.
In `@src/composables/graph/useGraphNodeManager.ts`:
- Around line 245-261: The current matchedInput search can pick a same-name
input before the one actually bound to this promoted widget; change the lookup
to prefer an exact widget binding first: find an input where input._widget ===
widget, and only if that returns undefined fall back to finding by input.name
=== widget.name; update the matchedInput assignment in useGraphNodeManager (the
variable named matchedInput that currently uses node.inputs?.find(...)) and keep
the rest of the logic using promotedInputName, displayName, and
resolvePromotedSourceByInputName unchanged.
In `@src/lib/litegraph/src/subgraph/SubgraphNode.ts`:
- Around line 263-313: Promoted views stay cached across input renames because
signatures use inputKey only, so update the cache/signature logic to include the
input's display name (or widget name) to invalidate on rename: modify
_makePromotedEntriesSignature and _makeLinkedEntriesSignature to incorporate the
input display name (e.g., entry.inputName or entry.widgetName as appropriate)
when building signatures, and ensure
_writePromotedViewsCache/_readPromotedViewsCache use those updated signatures;
alternatively, make PromotedWidgetView.displayName mutable and update it during
reconcile so renames are reflected even when the cached PromotedWidgetView
instance is reused (refer to PromotedWidgetView, PromotionEntry,
LinkedPromotionEntry, _readPromotedViewsCache, _writePromotedViewsCache,
_makePromotedEntriesSignature, and _makeLinkedEntriesSignature).
- Around line 823-877: The serialize path still matches promoted inputs to
subgraph inputs by name, so duplicate names collide; update the
SubgraphNode.serialize (and the analogous code at the second location
referenced) to persist the stable slot identity using input._subgraphSlot (e.g.,
serialize the slot id or another stable identifier) instead of input.name, and
update deserialization to look up the slot by that id and reassign
input._subgraphSlot accordingly; ensure you handle missing/undefined
_subgraphSlot by falling back to the existing name-based lookup so older saves
remain compatible.
---
Outside diff comments:
In `@src/lib/litegraph/src/subgraph/SubgraphNode.ts`:
- Around line 369-389: The current logic in _buildPromotionPersistenceState (and
the similar block around lines 424-440) calls _shouldPersistLinkedOnly and drops
fallback entries whenever every input has a linked representative, which
incorrectly prunes temporarily unresolved fallbacks; change the logic so we only
persist linked-only when each fallbackStoredEntry is positively proven to alias
a linkedPromotionEntry (i.e., modify _shouldPersistLinkedOnly to validate
aliasing between fallbackStoredEntries and linkedPromotionEntries, or
alternatively adjust _buildPromotionPersistenceState to include
fallbackStoredEntries unless alias validation passes), and ensure
_syncPromotions uses this stricter decision so independent promotions that are
unresolved are not removed during hydration/configure; reference functions:
_buildPromotionPersistenceState, _collectLinkedAndFallbackEntries,
_shouldPersistLinkedOnly, and _syncPromotions.
---
Nitpick comments:
In `@src/renderer/extensions/vueNodes/components/NodeWidgets.vue`:
- Around line 279-283: isWidgetVisible and other places currently read
visibility flags from the original SafeWidgetData.options, but the UI renders
using the merged widgetOptions (from widgetState?.options), causing stale
visibility/dedupe logic; change isWidgetVisible (and the references at the other
occurrences) to read hidden/advanced from the merged widgetOptions used for
rendering (e.g., use widgetOptions or compute mergedOptions = {
...widget.options, ...widgetState?.options } and then derive const hidden =
mergedOptions.hidden ?? false and advanced = mergedOptions.advanced ?? false) so
visibility and dedupe use the same source as the rendered widget.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 994d5c92-643b-4820-b540-dae4f100ae1c
📒 Files selected for processing (13)
src/components/rightSidePanel/parameters/SectionWidgets.vuesrc/composables/graph/useGraphNodeManager.test.tssrc/composables/graph/useGraphNodeManager.tssrc/core/graph/subgraph/promotedWidgetView.test.tssrc/core/graph/subgraph/promotedWidgetView.tssrc/lib/litegraph/src/subgraph/SubgraphIO.test.tssrc/lib/litegraph/src/subgraph/SubgraphInputNode.tssrc/lib/litegraph/src/subgraph/SubgraphNode.tssrc/lib/litegraph/src/subgraph/__fixtures__/subgraphComplexPromotion1.tssrc/renderer/extensions/vueNodes/components/NodeWidgets.test.tssrc/renderer/extensions/vueNodes/components/NodeWidgets.vuesrc/renderer/extensions/vueNodes/composables/useSlotLinkInteraction.tssrc/stores/promotionStore.ts
2 tasks
Amp-Thread-ID: https://ampcode.com/threads/T-019ce8d6-da19-708e-89f7-ec384469c5a7 Co-authored-by: Amp <amp@ampcode.com>
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 (2)
src/renderer/extensions/vueNodes/components/NodeWidgets.vue (1)
398-407:⚠️ Potential issue | 🟠 Major
AppInputid is no longer unique per widget.
id: bareWidgetIdcollides for all widgets on the same node. This can break input-label association and widget-level targeting.💡 Suggested fix
- id: bareWidgetId, + id: `${bareWidgetId}:${widget.slotName ?? widget.name}`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/components/NodeWidgets.vue` around lines 398 - 407, The id assigned to each widget in the object pushed into result (id: bareWidgetId) is not unique across widgets on the same node; change the id generation to include a widget-unique suffix (e.g., combine bareWidgetId with renderKey, widget.id, or the widget index) so each widget id is unique and preserves input–label associations and per-widget targeting; update the object created in the result.push call (the id property) to concatenate bareWidgetId with the chosen unique token (renderKey/widget.id/index) consistently wherever those ids are consumed.src/lib/litegraph/src/subgraph/SubgraphNode.ts (1)
593-599:⚠️ Potential issue | 🔴 CriticalGuard the
input-addedexisting-input path against missing links.This branch can throw when
subgraphInput.linkIds[0]is absent/stale (subgraph.links[linkId]is undefined), which breaks subgraph input reconciliation at runtime.Proposed fix
if (existingInput) { - const linkId = subgraphInput.linkIds[0] - const { inputNode, input } = subgraph.links[linkId].resolve(subgraph) + const linkId = subgraphInput.linkIds[0] + if (linkId === undefined) return + + const link = this.subgraph.getLink(linkId) + if (!link) return + + const { inputNode, input } = link.resolve(subgraph) const widget = inputNode?.widgets?.find?.((w) => w.name === name) if (widget && inputNode) this._setWidget( subgraphInput, existingInput,🤖 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 593 - 599, In the existing-input path of SubgraphNode where you use subgraphInput.linkIds[0] and access subgraph.links[linkId].resolve(subgraph), add guards to ensure linkIds[0] exists and subgraph.links[linkId] is defined before calling resolve; if either is missing, skip reconciliation for that existingInput (e.g., return/continue or fall back to a safe no-op) and ensure you also check that resolve(...) returns a non-null object before accessing inputNode or input and before reading inputNode.widgets; this prevents runtime exceptions when link metadata is stale.
♻️ Duplicate comments (2)
src/composables/graph/useGraphNodeManager.ts (1)
245-259:⚠️ Potential issue | 🟠 MajorPrefer exact
_widgetslot match before name fallback.Name-first matching can still select the wrong same-name slot and produce incorrect promoted-source mapping.
🤖 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 245 - 259, The current matching picks an input by name first which can choose the wrong same-name slot; change the lookup to prefer an exact slot identity match (input._widget === widget) before falling back to name equality: in the logic around matchedInput (and consequently promotedInputName/displayName and promotedSource) first search node.inputs for input._widget === widget, if none found then search for input.name === widget.name, then compute promotedSource using resolvePromotedSourceByInputName(displayName) only when the matched input was the exact widget match; otherwise use directSource.src/lib/litegraph/src/subgraph/SubgraphNode.ts (1)
1113-1125:⚠️ Potential issue | 🟠 MajorSerialize widget-value sync via
_subgraphSlotidentity, not slot name.Runtime mapping is now identity-based, but serialization still resolves by
slot.name === input.name. Duplicate-name inputs can still sync to the wrong subgraph slot before save.Proposed fix
- const subgraphInput = this.subgraph.inputNode.slots.find( - (slot) => slot.name === input.name - ) + const subgraphInput = + input._subgraphSlot ?? + this.subgraph.inputNode.slots.find( + (slot) => slot.name === input.name + ) if (!subgraphInput) continueAlso applies to: 1389-1391
🤖 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 1113 - 1125, The serialization currently matches subgraph slots by slot.name causing duplicate-named inputs to mis-map; update the logic in getSlotFromWidget and getWidgetFromSlot to use the slot identity/property used at runtime (the `_subgraphSlot` identity or the actual slot object) instead of comparing names—specifically, when resolving inputs in getSlotFromWidget use input._subgraphSlot (or strict object equality to `widget._subgraphSlot`) to find the matching input, and when resolving widgets in getWidgetFromSlot ensure you return the widget bound to the exact input._subgraphSlot identity; apply the same identity-based change to the corresponding code around the other occurrence (lines referencing similar logic at the 1389–1391 area).
🤖 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/core/graph/subgraph/promotedWidgetView.ts`:
- Around line 136-159: The current early return when getLinkedInputWidgets()
yields any linked widgets can drop writes if widgetStore entries aren't yet
ready; modify the block using useWidgetValueStore() so you track whether any
state was actually updated (e.g., a boolean updated = false), set updated = true
when you assign state.value for linked widgets or resolvedState for
resolveDeepest(), and only return early if updated is true; otherwise fall
through to the normal set logic so set value isn't a no-op when stores are not
yet hydrated (refer to getLinkedInputWidgets, useWidgetValueStore,
resolveDeepest, stripGraphPrefix, graphId in your changes).
---
Outside diff comments:
In `@src/lib/litegraph/src/subgraph/SubgraphNode.ts`:
- Around line 593-599: In the existing-input path of SubgraphNode where you use
subgraphInput.linkIds[0] and access subgraph.links[linkId].resolve(subgraph),
add guards to ensure linkIds[0] exists and subgraph.links[linkId] is defined
before calling resolve; if either is missing, skip reconciliation for that
existingInput (e.g., return/continue or fall back to a safe no-op) and ensure
you also check that resolve(...) returns a non-null object before accessing
inputNode or input and before reading inputNode.widgets; this prevents runtime
exceptions when link metadata is stale.
In `@src/renderer/extensions/vueNodes/components/NodeWidgets.vue`:
- Around line 398-407: The id assigned to each widget in the object pushed into
result (id: bareWidgetId) is not unique across widgets on the same node; change
the id generation to include a widget-unique suffix (e.g., combine bareWidgetId
with renderKey, widget.id, or the widget index) so each widget id is unique and
preserves input–label associations and per-widget targeting; update the object
created in the result.push call (the id property) to concatenate bareWidgetId
with the chosen unique token (renderKey/widget.id/index) consistently wherever
those ids are consumed.
---
Duplicate comments:
In `@src/composables/graph/useGraphNodeManager.ts`:
- Around line 245-259: The current matching picks an input by name first which
can choose the wrong same-name slot; change the lookup to prefer an exact slot
identity match (input._widget === widget) before falling back to name equality:
in the logic around matchedInput (and consequently promotedInputName/displayName
and promotedSource) first search node.inputs for input._widget === widget, if
none found then search for input.name === widget.name, then compute
promotedSource using resolvePromotedSourceByInputName(displayName) only when the
matched input was the exact widget match; otherwise use directSource.
In `@src/lib/litegraph/src/subgraph/SubgraphNode.ts`:
- Around line 1113-1125: The serialization currently matches subgraph slots by
slot.name causing duplicate-named inputs to mis-map; update the logic in
getSlotFromWidget and getWidgetFromSlot to use the slot identity/property used
at runtime (the `_subgraphSlot` identity or the actual slot object) instead of
comparing names—specifically, when resolving inputs in getSlotFromWidget use
input._subgraphSlot (or strict object equality to `widget._subgraphSlot`) to
find the matching input, and when resolving widgets in getWidgetFromSlot ensure
you return the widget bound to the exact input._subgraphSlot identity; apply the
same identity-based change to the corresponding code around the other occurrence
(lines referencing similar logic at the 1389–1391 area).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f240ebdd-6104-446d-8567-0e0bf577c7fa
📒 Files selected for processing (8)
src/composables/graph/useGraphNodeManager.tssrc/core/graph/subgraph/promotedWidgetView.test.tssrc/core/graph/subgraph/promotedWidgetView.tssrc/core/graph/subgraph/widgetNodeTypeGuard.tssrc/lib/litegraph/src/strings.tssrc/lib/litegraph/src/subgraph/SubgraphNode.tssrc/lib/litegraph/src/subgraph/__fixtures__/subgraphHelpers.tssrc/renderer/extensions/vueNodes/components/NodeWidgets.vue
Amp-Thread-ID: https://ampcode.com/threads/T-019ce909-a90d-715a-863a-c35fe51c29fc Co-authored-by: Amp <amp@ampcode.com>
dosubot
bot
added
the
size:XXL
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
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/subgraph/SubgraphNode.ts (1)
1299-1320:⚠️ Potential issue | 🟠 MajorView-key cleanup uses a mismatched key shape and can leave stale aliases.
Line 1299 builds a removal key without
inputName, but creation paths include it. Line 1315 also passesview.nameasinputKey, which does not match the current key contract. This can leave staleviewKeyaliases in the manager and cause incorrect reuse.Proposed fix
for (const input of this.inputs) { if (input._widget !== view || !input._subgraphSlot) continue + const inputName = input.label ?? input.name this._promotedViewManager.removeByViewKey( view.sourceNodeId, view.sourceWidgetName, this._makePromotionViewKey( String(input._subgraphSlot.id), view.sourceNodeId, - view.sourceWidgetName + view.sourceWidgetName, + inputName ) ) } - // Reconciled views can also be keyed by inputName-scoped view keys. - // Remove both key shapes to avoid stale cache entries across promote/rebind flows. - this._promotedViewManager.removeByViewKey( - view.sourceNodeId, - view.sourceWidgetName, - this._makePromotionViewKey( - view.name, - view.sourceNodeId, - view.sourceWidgetName - ) - ) + // If legacy key cleanup is still needed, remove using the exact legacy format + // expected by PromotedWidgetViewManager (not _makePromotionViewKey).🤖 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 1299 - 1320, The removal calls to _promotedViewManager.removeByViewKey use the wrong key shape compared to creation: replace the mismatched arguments so both removals call _makePromotionViewKey with the same input identifier used at creation (the subgraph input name/id), e.g. use String(input._subgraphSlot.id) (or the canonical inputName variable) instead of view.name in the second call and ensure the first call also includes the inputName-shaped key; update the two _promotedViewManager.removeByViewKey invocations that reference _makePromotionViewKey so they pass the identical input key format used when views were promoted (matching the creation path).
🤖 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/core/graph/subgraph/promotedWidgetView.test.ts`:
- Around line 1533-1566: The test currently allows 0 resolver calls; change the
final assertion to ensure the resolver actually runs once on the first access
and is not re-run on the second by: call subgraphNode.widgets once, assert
resolveSpy.mock.calls.length === 1, then access subgraphNode.widgets a second
time and assert resolveSpy.mock.calls.length remains 1; reference the spy on
_resolveLinkedPromotionBySubgraphInput and the two uses of subgraphNode.widgets
to implement these two assertions.
In `@src/core/graph/subgraph/promotedWidgetView.ts`:
- Around line 327-336: getLinkedInputWidgets() is matching inputs by raw
`_widget === this`, which fails during configure/hydration; replace that check
with a call to matchPromotedInput(this, input) while still restricting to inputs
that have `_subgraphSlot`. Locate the method getLinkedInputWidgets and change
the inputs.find predicate to use matchPromotedInput(this, input) (or equivalent)
instead of `_widget === this`, ensuring you still only consider inputs where
`_subgraphSlot` is truthy so the function continues to return linked promoted
inputs for getWidgetState() and the value setter during rebinding.
In `@src/lib/litegraph/src/subgraph/SubgraphNode.ts`:
- Around line 624-626: The current existing-input path uses a name-based lookup
(inputNode?.widgets?.find(w => w.name === name)) which can select the wrong
widget when names duplicate; replace that lookup by using the resolved slot
object `input` returned from `link.resolve(subgraph)` and call the utility
`getWidgetFromSlot(inputNode, input)` (or the equivalent helper in this module)
to retrieve the widget by slot identity; update the logic around `const {
inputNode, input } = link.resolve(subgraph)` to use `getWidgetFromSlot` instead
of the `.find` by name so the widget selection is slot-safe.
In `@src/renderer/extensions/vueNodes/components/NodeWidgets.vue`:
- Around line 242-269: The dedupeIdentity in getWidgetIdentity currently calls
stripGraphPrefix(rawWidgetId) which collapses scoped ids (e.g. "65:19" and
"66:19") and causes distinct promoted widgets to merge; update getWidgetIdentity
to use the full rawWidgetId (or include widget.sourceExecutionId) when building
stableIdentityRoot instead of stripGraphPrefix(rawWidgetId), ensuring
dedupeIdentity preserves scope; ensure renderKey logic remains consistent and
add a regression test that creates two widgets with scoped ids like "65:19" and
"66:19" to verify they produce distinct dedupeIdentity/renderKey values.
---
Outside diff comments:
In `@src/lib/litegraph/src/subgraph/SubgraphNode.ts`:
- Around line 1299-1320: The removal calls to
_promotedViewManager.removeByViewKey use the wrong key shape compared to
creation: replace the mismatched arguments so both removals call
_makePromotionViewKey with the same input identifier used at creation (the
subgraph input name/id), e.g. use String(input._subgraphSlot.id) (or the
canonical inputName variable) instead of view.name in the second call and ensure
the first call also includes the inputName-shaped key; update the two
_promotedViewManager.removeByViewKey invocations that reference
_makePromotionViewKey so they pass the identical input key format used when
views were promoted (matching the creation path).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32a3d62a-356b-42af-8473-fd515269104b
📒 Files selected for processing (12)
src/components/rightSidePanel/parameters/SectionWidgets.vuesrc/composables/graph/useGraphNodeManager.test.tssrc/composables/graph/useGraphNodeManager.tssrc/core/graph/subgraph/matchPromotedInput.test.tssrc/core/graph/subgraph/matchPromotedInput.tssrc/core/graph/subgraph/promotedWidgetView.test.tssrc/core/graph/subgraph/promotedWidgetView.tssrc/core/graph/subgraph/widgetRenderKey.test.tssrc/core/graph/subgraph/widgetRenderKey.tssrc/lib/litegraph/src/subgraph/SubgraphNode.tssrc/renderer/extensions/vueNodes/components/NodeWidgets.test.tssrc/renderer/extensions/vueNodes/components/NodeWidgets.vue
🚧 Files skipped from review as they are similar to previous changes (2)
- src/composables/graph/useGraphNodeManager.ts
- src/composables/graph/useGraphNodeManager.test.ts
Comment on lines
+1533
to
+1566
| test('does not re-resolve linked entries when linked input state is unchanged', () => { | ||
| const subgraph = createTestSubgraph({ | ||
| inputs: [{ name: 'string_a', type: '*' }] | ||
| }) | ||
| const subgraphNode = createTestSubgraphNode(subgraph, { id: 97 }) | ||
| subgraphNode.graph?.add(subgraphNode) | ||
|
|
||
| const linkedNodeA = new LGraphNode('LinkedNodeA') | ||
| const linkedInputA = linkedNodeA.addInput('string_a', '*') | ||
| linkedNodeA.addWidget('text', 'string_a', 'a', () => {}) | ||
| linkedInputA.widget = { name: 'string_a' } | ||
| subgraph.add(linkedNodeA) | ||
|
|
||
| const linkedNodeB = new LGraphNode('LinkedNodeB') | ||
| const linkedInputB = linkedNodeB.addInput('string_a', '*') | ||
| linkedNodeB.addWidget('text', 'string_a', 'b', () => {}) | ||
| linkedInputB.widget = { name: 'string_a' } | ||
| subgraph.add(linkedNodeB) | ||
|
|
||
| subgraph.inputNode.slots[0].connect(linkedInputA, linkedNodeA) | ||
| subgraph.inputNode.slots[0].connect(linkedInputB, linkedNodeB) | ||
|
|
||
| const resolveSpy = vi.spyOn( | ||
| subgraphNode as unknown as { | ||
| _resolveLinkedPromotionBySubgraphInput: (...args: unknown[]) => unknown | ||
| }, | ||
| '_resolveLinkedPromotionBySubgraphInput' | ||
| ) | ||
|
|
||
| void subgraphNode.widgets | ||
| void subgraphNode.widgets | ||
|
|
||
| expect(resolveSpy.mock.calls.length).toBeLessThanOrEqual(1) | ||
| }) |
Contributor
There was a problem hiding this comment.
Tighten the resolver call-count assertion to avoid false positives.
Line 1565 currently allows 0 calls, so this test can pass even if linked resolution never runs. Assert one call after first read, then unchanged after second read.
🔧 Suggested test assertion tightening
- void subgraphNode.widgets
- void subgraphNode.widgets
-
- expect(resolveSpy.mock.calls.length).toBeLessThanOrEqual(1)
+ void subgraphNode.widgets
+ expect(resolveSpy).toHaveBeenCalledTimes(1)
+ void subgraphNode.widgets
+ expect(resolveSpy).toHaveBeenCalledTimes(1)As per coding guidelines: "Do not write tests that just test the mocks; ensure tests fail when code behaves unexpectedly."
🤖 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 1533 - 1566,
The test currently allows 0 resolver calls; change the final assertion to ensure
the resolver actually runs once on the first access and is not re-run on the
second by: call subgraphNode.widgets once, assert resolveSpy.mock.calls.length
=== 1, then access subgraphNode.widgets a second time and assert
resolveSpy.mock.calls.length remains 1; reference the spy on
_resolveLinkedPromotionBySubgraphInput and the two uses of subgraphNode.widgets
to implement these two assertions.
Comment on lines
+242
to
+269
| function getWidgetIdentity( | ||
| widget: SafeWidgetData, | ||
| nodeId: string | number | undefined, | ||
| index: number | ||
| ): { | ||
| dedupeIdentity?: string | ||
| renderKey: string | ||
| } { | ||
| const rawWidgetId = widget.storeNodeId ?? widget.nodeId | ||
| const storeWidgetName = widget.storeName ?? widget.name | ||
| const slotNameForIdentity = widget.slotName ?? widget.name | ||
| const stableIdentityRoot = rawWidgetId | ||
| ? `node:${String(stripGraphPrefix(rawWidgetId))}` | ||
| : widget.sourceExecutionId | ||
| ? `exec:${widget.sourceExecutionId}` | ||
| : undefined | ||
|
|
||
| const dedupeIdentity = stableIdentityRoot | ||
| ? `${stableIdentityRoot}:${storeWidgetName}:${slotNameForIdentity}:${widget.type}` | ||
| : undefined | ||
| const renderKey = | ||
| dedupeIdentity ?? | ||
| `transient:${String(nodeId ?? '')}:${storeWidgetName}:${slotNameForIdentity}:${widget.type}:${index}` | ||
|
|
||
| return { | ||
| dedupeIdentity, | ||
| renderKey | ||
| } |
Contributor
There was a problem hiding this comment.
Don't strip the scoped source id out of the dedupe identity.
stripGraphPrefix() turns scoped ids like 65:19 and 66:19 into the same node:19. If two promoted widgets share local node id/name/type but come from different subgraph execution paths, uniqueWidgets collapses them into one row and one renderKey, hiding a legitimate widget. Keep the full scoped id here—or append sourceExecutionId—and leave stripGraphPrefix() only in the widget store lookups below. Please add a regression with ids like 65:19 and 66:19.
💡 Possible fix
- const stableIdentityRoot = rawWidgetId
- ? `node:${String(stripGraphPrefix(rawWidgetId))}`
- : widget.sourceExecutionId
- ? `exec:${widget.sourceExecutionId}`
- : undefined
+ const stableIdentityRoot = widget.sourceExecutionId
+ ? `exec:${widget.sourceExecutionId}`
+ : rawWidgetId
+ ? `node:${String(rawWidgetId)}`
+ : undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/extensions/vueNodes/components/NodeWidgets.vue` around lines 242
- 269, The dedupeIdentity in getWidgetIdentity currently calls
stripGraphPrefix(rawWidgetId) which collapses scoped ids (e.g. "65:19" and
"66:19") and causes distinct promoted widgets to merge; update getWidgetIdentity
to use the full rawWidgetId (or include widget.sourceExecutionId) when building
stableIdentityRoot instead of stripGraphPrefix(rawWidgetId), ensuring
dedupeIdentity preserves scope; ensure renderKey logic remains consistent and
add a regression test that creates two widgets with scoped ids like "65:19" and
"66:19" to verify they produce distinct dedupeIdentity/renderKey values.
Contributor
Author
There was a problem hiding this comment.
The nodes should all have unique IDs.
Contributor
There was a problem hiding this comment.
@DrJKL, understood — if node IDs are guaranteed to be unique across the graph (including across subgraph execution paths), then stripGraphPrefix can safely be used in the dedup key without risk of collapsing distinct nodes. My concern doesn't apply here. Thanks for the clarification!
✏️ Learnings added
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 9896
File: src/renderer/extensions/vueNodes/components/NodeWidgets.vue:242-269
Timestamp: 2026-03-14T01:15:36.437Z
Learning: In Comfy-Org/ComfyUI_frontend, LGraph node IDs are globally unique across all subgraph scopes and execution paths. Therefore, calling `stripGraphPrefix()` on a scoped id (e.g. "65:19" → "19") in deduplication/identity keys inside NodeWidgets.vue is safe and will not collide with IDs from other nodes, even those under different graph prefixes. Do not flag stripGraphPrefix usage in node-ID-based dedup keys as a collision risk.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8594
File: src/lib/litegraph/src/widgets/BaseWidget.ts:146-155
Timestamp: 2026-02-06T09:27:31.576Z
Learning: In the ComfyUI frontend codebase: `LGraphNode.removeWidget()` (and the `widget.onRemove?.()` hook) is called during subgraph conversion operations where widgets are being moved or restructured, not just when they're being permanently deleted. Therefore, widget unregistration from `widgetValueStore` should not be tied to `onRemove`, as this would cause premature state loss during legitimate graph restructuring operations.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 9120
File: src/lib/litegraph/src/LGraph.ts:1600-1644
Timestamp: 2026-03-12T14:43:07.816Z
Learning: In Comfy-Org/ComfyUI_frontend (src/lib/litegraph/src/LGraph.ts), `_removeDuplicateLinks()` is intentionally scoped to freshly-deserialized data: it only runs during `configure()` (after `reroute.validateLinks()`) and `unpackSubgraph`. It does not need to handle live reroute state cleanup because reroutes are validated separately before this method executes. Do not suggest reroute `linkIds` cleanup inside `_removeDuplicateLinks`.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 8951
File: src/platform/workflow/management/stores/workflowStore.ts:301-301
Timestamp: 2026-03-05T23:21:47.113Z
Learning: In src/platform/workflow/management/stores/workflowStore.ts, `createTemporary()` and `createNewTemporary()` intentionally inject a UUID `id` into workflow data via `ensureWorkflowId()` to give workflows stable identity for sharing. This is a known behavioral change introduced in PR `#8951`. Extensions relying on serialized content matching their input may be silently affected.
Learnt from: benceruleanlu
Repo: Comfy-Org/ComfyUI_frontend PR: 9894
File: src/lib/litegraph/src/LGraphCanvas.ts:0-0
Timestamp: 2026-03-14T00:14:35.308Z
Learning: In Comfy-Org/ComfyUI_frontend (src/lib/litegraph/src/LGraphCanvas.ts), _drawVueDragAlignmentGuides must only be invoked when LiteGraph.vueNodesMode is true. In drawFrontCanvas’s fallback path (used when overlayCtx is missing), keep the call gated to Vue mode so classic (non-Vue) rendering never shows Vue drag guides. It is correct to allow guides to render on the main canvas when overlayCtx is unavailable in Vue mode; do not gate on overlay availability.
Learnt from: dante01yoon
Repo: Comfy-Org/ComfyUI_frontend PR: 9712
File: src/renderer/extensions/vueNodes/widgets/components/DisplayCarousel.vue:61-64
Timestamp: 2026-03-12T08:28:20.395Z
Learning: In DisplayCarousel.vue (src/renderer/extensions/vueNodes/widgets/components/DisplayCarousel.vue), the "remove image" button (`handleRemove`) intentionally clears the entire node output (all images for the node), not just the currently active image. This mirrors the original PrimeVue/WidgetGalleria implementation behavior. Do not flag this as a bug during reviews; it is expected node-wide clearing behavior.
Learnt from: AustinMroz
Repo: Comfy-Org/ComfyUI_frontend PR: 9523
File: src/renderer/extensions/linearMode/PartnerNodesList.vue:23-33
Timestamp: 2026-03-07T21:51:45.520Z
Learning: In app-mode Vue components under src/renderer/extensions (e.g., PartnerNodesList.vue and similar components in ComfyUI_frontend), computed properties that traverse app.graph do not need a reactive dependency on workflowStore.activeWorkflow since nodes cannot be added or removed in app mode and the graph is effectively frozen. Do not flag missing reactive graph-change dependencies as an issue in app-mode components. Apply this guidance to similar files in the ComfyUI_frontend repository to avoid unnecessary reactivity checks on a frozen graph.
Learnt from: benceruleanlu
Repo: Comfy-Org/ComfyUI_frontend PR: 7297
File: src/components/actionbar/ComfyActionbar.vue:33-43
Timestamp: 2025-12-09T21:40:12.361Z
Learning: In Vue single-file components, allow inline Tailwind CSS class strings for static classes and avoid extracting them into computed properties solely for readability. Prefer keeping static class names inline for simplicity and performance. For dynamic or conditional classes, use Vue bindings (e.g., :class) to compose classes.
Applies to all Vue files in the repository (e.g., src/**/*.vue) where Tailwind utilities are used for static styling.
Learnt from: simula-r
Repo: Comfy-Org/ComfyUI_frontend PR: 7252
File: src/renderer/extensions/vueNodes/components/ImagePreview.vue:151-158
Timestamp: 2025-12-11T03:55:51.755Z
Learning: In Vue components under src/renderer/extensions/vueNodes (e.g., ImagePreview.vue and LGraphNode.vue), implement image gallery keyboard navigation so that it responds to the node's focus state rather than requiring focus inside the image preview wrapper. Achieve this by wiring keyEvent handling at the node focus level and injecting or propagating key events (arrow keys) to the gallery when the node is focused/selected. This improves accessibility and aligns navigation with node-level focus behavior.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7358
File: src/components/dialog/content/signin/SignUpForm.vue:45-54
Timestamp: 2025-12-11T12:25:15.470Z
Learning: This repository uses CI automation to format code (pnpm format). Do not include manual formatting suggestions in code reviews for Comfy-Org/ComfyUI_frontend. If formatting issues are detected, rely on the CI formatter or re-run pnpm format. Focus reviews on correctness, readability, performance, accessibility, and maintainability rather than style formatting.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7537
File: src/components/ui/button/Button.vue:17-17
Timestamp: 2025-12-16T22:26:49.463Z
Learning: In Vue 3.5+ with <script setup>, when using defineProps<Props>() with partial destructuring (e.g., const { as = 'button', class: customClass = '' } = defineProps<Props>() ), props that are not destructured (e.g., variant, size) stay accessible by name in the template scope. This pattern is valid: you can destructure only a subset of props for convenience while referencing the remaining props directly in template expressions. Apply this guideline to Vue components across the codebase (all .vue files).
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7598
File: src/components/sidebar/tabs/AssetsSidebarTab.vue:131-131
Timestamp: 2025-12-18T02:07:38.870Z
Learning: Tailwind CSS v4 safe utilities (e.g., items-center-safe, justify-*-safe, place-*-safe) are allowed in Vue components under src/ and in story files. Do not flag these specific safe variants as invalid when reviewing code in src/**/*.vue or related stories.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7603
File: src/components/queue/QueueOverlayHeader.vue:49-59
Timestamp: 2025-12-18T21:15:46.862Z
Learning: In the ComfyUI_frontend repository, for Vue components, do not add aria-label to buttons that have visible text content (e.g., buttons containing <span> text). The visible text provides the accessible name. Use aria-label only for elements without visible labels (e.g., icon-only buttons). If a button has no visible label, provide a clear aria-label or associate with an aria-labelledby describing its action.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7649
File: src/components/graph/selectionToolbox/ColorPickerButton.vue:15-18
Timestamp: 2025-12-21T01:06:02.786Z
Learning: In Comfy-Org/ComfyUI_frontend, in Vue component files, when a filled icon is required (e.g., 'pi pi-circle-fill'), you may mix PrimeIcons with Lucide icons since Lucide lacks filled variants. This mixed usage is acceptable when one icon library does not provide an equivalent filled icon. Apply consistently across Vue components in the src directory where icons are used, and document the rationale when a mixed approach is chosen.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7649
File: src/platform/cloud/subscription/components/PricingTable.vue:185-201
Timestamp: 2025-12-22T21:36:08.369Z
Learning: In Vue components, avoid creating single-use variants for common UI components (e.g., Button and other shared components). Aim for reusable variants that cover multiple use cases. It’s acceptable to temporarily mix variant props with inline Tailwind classes when a styling need is unique to one place, but plan and consolidate into shared, reusable variants as patterns emerge across the codebase.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7893
File: src/components/button/IconGroup.vue:5-6
Timestamp: 2026-01-08T02:26:18.357Z
Learning: In components that use the cn utility from '@/utils/tailwindUtil' with tailwind-merge, rely on the behavior that conflicting Tailwind classes are resolved by keeping the last one. For example, cn('base-classes bg-default', propClass) will have any conflicting background class from propClass override bg-default. This additive pattern is intentional and aligns with the shadcn-ui convention; ensure you document or review expectations accordingly in Vue components.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7906
File: src/components/sidebar/tabs/AssetsSidebarTab.vue:545-552
Timestamp: 2026-01-12T17:39:27.738Z
Learning: In Vue/TypeScript files (src/**/*.{ts,tsx,vue}), prefer if/else statements over ternary operators when performing side effects or actions (e.g., mutating state, calling methods with side effects). Ternaries should be reserved for computing and returning values.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8195
File: src/platform/assets/components/MediaAssetFilterBar.vue:16-16
Timestamp: 2026-01-21T01:28:27.626Z
Learning: In Vue templates (Vue 3.4+ with the build step), when binding to data or props that are camelCase (e.g., mediaTypeFilters), you can use kebab-case in the template bindings (e.g., :media-type-filters). This is acceptable and will resolve to the corresponding camelCase variable. Do not require CamelCase in template bindings.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8090
File: src/platform/assets/components/modelInfo/ModelInfoField.vue:8-11
Timestamp: 2026-01-22T02:28:58.105Z
Learning: In Vue 3 script setup, props defined with defineProps are automatically available by name in the template without destructuring. Destructuring the result of defineProps inside script can break reactivity; prefer accessing props by name in the template. If you need to use props in the script, reference them via the defined props object rather than destructuring, or use toRefs when you intend to destructure while preserving reactivity.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8497
File: src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue:223-236
Timestamp: 2026-02-01T21:10:29.567Z
Learning: In Vue single-file components, do not review or require edits to comments. Favor self-documenting code through clear naming, strong types, and explicit APIs. If a comment is misleading or outdated, consider removing it, but avoid suggesting adding or fixing comments. This guideline aligns with preferring code clarity over comment maintenance across all .vue files.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 8592
File: src/components/topbar/WorkflowExecutionIndicator.vue:28-28
Timestamp: 2026-02-03T21:35:40.889Z
Learning: In Vue single-file components where the i18n t function is only used within the template, prefer using the built-in $t in the template instead of importing useI18n and destructuring t in the script. This avoids unnecessary imports when t is not used in the script. If you need i18n in the script (Composition API), only then use useI18n and access t from its returned object. Ensure this pattern applies to all Vue components with template-only i18n usage.
Learnt from: Yourz
Repo: Comfy-Org/ComfyUI_frontend PR: 8548
File: src/components/common/TreeExplorerV2Node.vue:9-47
Timestamp: 2026-02-07T14:47:06.751Z
Learning: In all Vue files that render Reka UI context menu components (ContextMenuRoot, ContextMenuTrigger, etc.), avoid dynamic component wrapping with <component :is="...">. Use conditional rendering with v-if / v-else to render separate branches, even if it results in some template duplication. This improves readability and maintainability across files that use these components.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8753
File: src/renderer/extensions/vueNodes/widgets/components/WidgetDOM.vue:17-28
Timestamp: 2026-02-09T03:24:47.113Z
Learning: When destructuring reactive properties from Pinia stores in Vue components, use storeToRefs() to preserve reactivity. Example: const store = useCanvasStore(); const { canvas } = storeToRefs(store); access as canvas.value (e.g., canvas.value). Ensure you import storeToRefs from 'pinia' and use it wherever you destructure store properties in the setup function.
Learnt from: pythongosssss
Repo: Comfy-Org/ComfyUI_frontend PR: 8775
File: src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDefault.vue:62-68
Timestamp: 2026-02-10T17:59:25.893Z
Learning: In Vue 3 single-file components, code defined outside the script setup scope cannot access variables or helpers defined inside setup. If a default function (e.g., defineModel) is hoisted outside script setup, it will not be able to call setup helpers or reference setup-scoped variables. Place such logic inside setup or expose necessary values via returns/defineExpose to be accessible.
Learnt from: Yourz
Repo: Comfy-Org/ComfyUI_frontend PR: 9085
File: src/components/sidebar/tabs/NodeLibrarySidebarTabV2.vue:45-50
Timestamp: 2026-02-27T09:39:47.145Z
Learning: In Vue components that use Reka UI primitives such as DropdownMenuTrigger and SelectTrigger with as-child, it is acceptable to render a raw <button> when the styling is context-specific and there is no existing Button variant to map to. The as-child pattern preserves accessibility while enabling composition. Do not create single-use Button variants for these cases; prefer inline styling with a raw button for clarity and to avoid unnecessary abstraction. This guideline applies to all Vue components (files ending in .vue) in projects using Reka UI.
Learnt from: jaeone94
Repo: Comfy-Org/ComfyUI_frontend PR: 9360
File: src/renderer/extensions/vueNodes/components/LGraphNode.vue:576-602
Timestamp: 2026-03-04T16:12:55.683Z
Learning: In Vue node components (e.g., LGraphNode.vue), keep computed properties that map RenderShape to CSS classes to explicitly handle only RenderShape.BOX and RenderShape.CARD, letting all other variants (ROUND, CIRCLE, HollowCircle, ARROW, GRID) fall through to a default rounded case. This reflects that only Default, Box, and Card shapes are user-selectable via the context menu; the other variants are internal for canvas rendering and should not be treated as selectable. Do not flag missing explicit cases as issues. Consider applying this defaulting pattern to similar node-shape components in the same Vue extension (src/renderer/extensions/vueNodes/components) to maintain consistency.
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 9427
File: src/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdownMenuFilter.vue:33-33
Timestamp: 2026-03-06T00:53:28.835Z
Learning: When reviewing code, note that the enforce-canonical-classes (better-tailwindcss) rule may auto-fix Tailwind v3 !class-name syntax by converting it to v4 class-name! syntax. Do not treat these auto-fixed class-name! instances as newly introduced issues; the perceived change is in syntax placement, not in usage or intent. This guidance applies across all .vue and .ts files in the repository.
Learnt from: sonnybox
Repo: Comfy-Org/ComfyUI_frontend PR: 9446
File: src/renderer/extensions/vueNodes/widgets/components/WidgetTextarea.vue:45-45
Timestamp: 2026-03-06T01:55:00.013Z
Learning: Treat wrap-break-word as a valid Tailwind CSS utility for overflow-wrap: break-word in Tailwind v4+ projects. Do not flag this class as invalid in any Vue (.vue) or TypeScript (.ts/.tsx) files within the repository (e.g., Comfy-Org/ComfyUI_frontend) or other Tailwind v4+ projects. When reviewing, verify that the class is used to enable word breaking in long text content and reference the Tailwind docs: https://tailwindcss.com/docs/overflow-wrap.
Learnt from: benceruleanlu
Repo: Comfy-Org/ComfyUI_frontend PR: 9749
File: src/platform/assets/components/MediaAssetContextMenu.vue:88-93
Timestamp: 2026-03-11T19:12:43.621Z
Learning: For Vue components using PrimeVue ContextMenu, the root element merges all passthrough attributes via v-bind="ptmi('root')". Passing an id through :pt="{ root: { id: someId } }" will reliably set the root element's id, allowing document.getElementById(someId) to reference the overlay. Do not flag this pattern as unreliable in reviews for components using PrimeVue ContextMenu with pt.root.id passthrough (e.g., src/platform/assets/components/MediaAssetContextMenu.vue).
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 9045
File: src/renderer/extensions/vueNodes/components/SlotContextMenu.vue:90-114
Timestamp: 2026-03-12T14:40:06.356Z
Learning: Do not wrap dialogService.prompt() calls in try/catch blocks in Vue components under src/renderer/extensions/vueNodes/components (e.g., SlotContextMenu.vue and similar). The dialogService handles its own errors internally; adding defensive try/catch provides no value and can clutter code. If a specific call site requires different handling, handle it at the call site with explicit error handling only when necessary.
Amp-Thread-ID: https://ampcode.com/threads/T-019ce943-aad6-72d5-bcf5-bb4466f25a65 Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/core/graph/subgraph/promotedWidgetView.test.ts (1)
1562-1568:⚠️ Potential issue | 🟡 MinorStrengthen this resolver call-count assertion to avoid false positives.
This currently allows
0calls, so the test can pass even if linked resolution never executes.Proposed test tightening
- void subgraphNode.widgets - const initialResolveCount = resolveSpy.mock.calls.length - expect(initialResolveCount).toBeLessThanOrEqual(1) - - void subgraphNode.widgets - expect(resolveSpy).toHaveBeenCalledTimes(initialResolveCount) + void subgraphNode.widgets + expect(resolveSpy).toHaveBeenCalledTimes(1) + + void subgraphNode.widgets + expect(resolveSpy).toHaveBeenCalledTimes(1)As per coding guidelines: "Do not write tests that just test the mocks; ensure tests fail when code behaves unexpectedly."
🤖 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 1562 - 1568, The test currently allows zero resolver calls which can hide regressions; change the assertion around resolveSpy and subgraphNode.widgets so the first access actually triggered resolution and the second access is idempotent: after the first read of subgraphNode.widgets capture resolveSpy.mock.calls.length and assert it is at least 1 (e.g. expect(initialResolveCount).toBeGreaterThanOrEqual(1) or equal 1) to ensure resolution ran, then call void subgraphNode.widgets again and assert resolveSpy was not incremented (expect(resolveSpy).toHaveBeenCalledTimes(initialResolveCount)); use the existing resolveSpy and subgraphNode.widgets identifiers when updating the test.src/renderer/extensions/vueNodes/components/NodeWidgets.vue (1)
250-264:⚠️ Potential issue | 🟠 MajorKeep scoped ids intact and ignore temporary
-1ids in the dedupe key.
stableIdentityRootstill prefersrawWidgetIdand runs it throughstripGraphPrefix(). That collapses distinct promoted widgets like65:19and66:19into the same identity, and it also treats hydration-time placeholders like-1as stable, so entries can merge or remount when the real id arrives. Preferwidget.sourceExecutionIdwhen it exists, otherwise use the full scopedrawWidgetId, and only use the transient fallback while the id is unresolved.💡 Suggested fix
function getWidgetIdentity( widget: SafeWidgetData, nodeId: string | number | undefined, index: number ): { dedupeIdentity?: string renderKey: string } { const rawWidgetId = widget.storeNodeId ?? widget.nodeId + const hasStableWidgetId = + rawWidgetId !== undefined && + rawWidgetId !== null && + String(rawWidgetId) !== '-1' const storeWidgetName = widget.storeName ?? widget.name const slotNameForIdentity = widget.slotName ?? widget.name - const stableIdentityRoot = rawWidgetId - ? `node:${String(stripGraphPrefix(rawWidgetId))}` - : widget.sourceExecutionId - ? `exec:${widget.sourceExecutionId}` - : undefined + const stableIdentityRoot = widget.sourceExecutionId + ? `exec:${widget.sourceExecutionId}` + : hasStableWidgetId + ? `node:${String(rawWidgetId)}` + : undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/components/NodeWidgets.vue` around lines 250 - 264, stableIdentityRoot currently prefers rawWidgetId and runs stripGraphPrefix on it which collapses scoped ids and treats placeholder -1 as stable; change the logic in the stableIdentityRoot/dedupeIdentity/renderKey computation to prefer widget.sourceExecutionId when present, otherwise use the full scoped rawWidgetId (do not call stripGraphPrefix), and treat rawWidgetId values that are unset or equal to "-1" as unresolved so the renderKey falls back to the transient form; ensure dedupeIdentity still composes from stableIdentityRoot, storeWidgetName, slotNameForIdentity, and widget.type when stableIdentityRoot is defined.src/renderer/extensions/vueNodes/components/NodeWidgets.test.ts (1)
139-173:⚠️ Potential issue | 🟡 MinorThese new identity tests still miss the broken collision path.
The "distinct promoted sources" case uses different local ids (
...:19vs...:20), and the transient case usesundefinedids instead of the real-1placeholder. The currentgetWidgetIdentity()bug still passes both. Please add a same-local-id/different-scope case (for example65:19vs66:19) and a hydration case that starts at-1and then resolves to a concrete id.Also applies to: 231-258
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/components/NodeWidgets.test.ts` around lines 139 - 173, Update the test in NodeWidgets.test.ts to cover the missing collision paths for getWidgetIdentity: add a same-local-id/different-scope case (e.g., two widgets created via createMockWidget with nodeId/storeNodeId like "65:19" and "66:19" but identical slot/storeName) to ensure different scopes don’t collide, and add a hydration/transient case using the placeholder "-1" (create one widget with nodeId/storeNodeId "-1" and another with the eventual resolved id like "65:19") to verify deduplication handles transient-to-resolved identity changes; keep using createMockNodeData and mountComponent and assert the expected widget count as in the existing test so getWidgetIdentity’s collision bug is exercised.
🤖 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/core/graph/subgraph/promotedWidgetView.test.ts`:
- Around line 1562-1568: The test currently allows zero resolver calls which can
hide regressions; change the assertion around resolveSpy and
subgraphNode.widgets so the first access actually triggered resolution and the
second access is idempotent: after the first read of subgraphNode.widgets
capture resolveSpy.mock.calls.length and assert it is at least 1 (e.g.
expect(initialResolveCount).toBeGreaterThanOrEqual(1) or equal 1) to ensure
resolution ran, then call void subgraphNode.widgets again and assert resolveSpy
was not incremented
(expect(resolveSpy).toHaveBeenCalledTimes(initialResolveCount)); use the
existing resolveSpy and subgraphNode.widgets identifiers when updating the test.
In `@src/renderer/extensions/vueNodes/components/NodeWidgets.test.ts`:
- Around line 139-173: Update the test in NodeWidgets.test.ts to cover the
missing collision paths for getWidgetIdentity: add a
same-local-id/different-scope case (e.g., two widgets created via
createMockWidget with nodeId/storeNodeId like "65:19" and "66:19" but identical
slot/storeName) to ensure different scopes don’t collide, and add a
hydration/transient case using the placeholder "-1" (create one widget with
nodeId/storeNodeId "-1" and another with the eventual resolved id like "65:19")
to verify deduplication handles transient-to-resolved identity changes; keep
using createMockNodeData and mountComponent and assert the expected widget count
as in the existing test so getWidgetIdentity’s collision bug is exercised.
In `@src/renderer/extensions/vueNodes/components/NodeWidgets.vue`:
- Around line 250-264: stableIdentityRoot currently prefers rawWidgetId and runs
stripGraphPrefix on it which collapses scoped ids and treats placeholder -1 as
stable; change the logic in the stableIdentityRoot/dedupeIdentity/renderKey
computation to prefer widget.sourceExecutionId when present, otherwise use the
full scoped rawWidgetId (do not call stripGraphPrefix), and treat rawWidgetId
values that are unset or equal to "-1" as unresolved so the renderKey falls back
to the transient form; ensure dedupeIdentity still composes from
stableIdentityRoot, storeWidgetName, slotNameForIdentity, and widget.type when
stableIdentityRoot is defined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 93cf6d98-2c28-452e-b52c-43e04a3be414
📒 Files selected for processing (5)
src/core/graph/subgraph/promotedWidgetView.test.tssrc/core/graph/subgraph/promotedWidgetView.tssrc/lib/litegraph/src/subgraph/SubgraphNode.tssrc/renderer/extensions/vueNodes/components/NodeWidgets.test.tssrc/renderer/extensions/vueNodes/components/NodeWidgets.vue
DrJKL
added
preview
needs-backport
| } | ||
|
|
||
| set value(value: IBaseWidget['value']) { | ||
| const linkedWidgets = this.getLinkedInputWidgets() |
Contributor
There was a problem hiding this comment.
issue: get value() reads from getLinkedInputWidgetStates()[0] (the first linked state), while set value() writes to ALL linked widgets plus the deepest-resolved widget. If getLinkedInputWidgets() returns entries in non-deterministic order, getter and setter operate on different state objects.
Also, if some getWidget() calls return undefined during the setter loop, partial updates occur -- some linked states get the new value, others don't, and the didUpdateState flag prevents the fallback path from running.
Consider making the getter read from the same canonical state the setter writes to as its primary target.
Contributor
Author
There was a problem hiding this comment.
Re-checked this deeply and kept behavior unchanged for now (｡•ㅅ•｡). The read and write asymmetry is intentional in this flow: representative read path plus fanout write path for linked consistency.
Avoided semantic expansion in this PR and focused on correctness-critical fixes first ♡.
| ) | ||
| } | ||
|
|
||
| private getLinkedInputWidgets(): Array<{ |
Contributor
There was a problem hiding this comment.
suggestion: getLinkedInputWidgets() performs a linear scan of all inputs with nested calls to matchPromotedInput(), getConnectedWidgets(), and filter(hasWidgetNode). It's called from getWidgetState() -> value getter on every render frame, but unlike resolveDeepest() (which has cachedDeepestByFrame), it has no caching. Consider adding frame-based caching similar to cachedDeepestByFrame.
Contributor
Author
There was a problem hiding this comment.
Very fair perf thought~ (´꒳`)♡ We did not add caching in this pass to keep invalidation complexity low without profiler evidence of a hotspot.
If perf traces show this path is hot, we can add a minimal frame-scoped cache follow-up (˶ᵔ ᵕ ᵔ˶)/.
| return deduplicatedEntries | ||
| } | ||
|
|
||
| private _hasMissingBoundSourceWidget(): boolean { |
Contributor
There was a problem hiding this comment.
suggestion: _hasMissingBoundSourceWidget() iterates all inputs with getNodeById + widget array scan, and it's called as a cache-freshness signal on every widgets getter access (i.e., every frame) from both _getLinkedPromotionEntries() and _getPromotedViews(). This creates a hybrid cache invalidation model (reactive heuristic bypassing _cacheVersion) that's hard to reason about. Consider either making it part of the dirty-flag invalidation (set on relevant events) or removing it from the cache key and relying solely on _cacheVersion.
Contributor
Author
There was a problem hiding this comment.
Reviewed this skeptically and decided not to redesign freshness invalidation in this PR ૮₍˶ •. • ⑅₎ა.
Keeping the current model for now to avoid broad architectural risk while landing concrete correctness fixes first.
|
|
||
| const existingWidget = uniqueWidgets[existingIndex] | ||
| if (existingWidget && !existingWidget.isVisible && visible) { | ||
| uniqueWidgets[existingIndex] = { |
Contributor
There was a problem hiding this comment.
issue: When a duplicate is found and the replacement is visible but the original wasn't, the entire entry is replaced including identity -- which may have a different renderKey. Since renderKey is used as Vue's :key, this forces Vue to destroy and recreate the component rather than patch in-place. Consider preserving the original renderKey when replacing the dedup entry.
Contributor
Author
There was a problem hiding this comment.
Double-checked this path and did not apply a change ( •̀ᴗ•́ )✧.
In the dedupe replacement branch, dedupeIdentity and renderKey remain aligned, so that specific replacement flow should not force remount by key change.
| inputName = '' | ||
| ): string { | ||
| return `${inputName}:${interiorNodeId}:${widgetName}` | ||
| return `${inputKey}:${interiorNodeId}:${widgetName}:${inputName}` |
Contributor
There was a problem hiding this comment.
suggestion: The key format ${inputKey}:${interiorNodeId}:${widgetName}:${inputName} uses : as delimiter, but node IDs can contain colons (e.g., "65:18" for subgraph execution paths), creating ambiguous key boundaries. Consider using a delimiter that cannot appear in any component (e.g., \x1F unit separator) or JSON.stringify([...]).
Contributor
Author
There was a problem hiding this comment.
Implemented this one~ ✨ We switched SubgraphNode._makePromotionViewKey to structured encoding so key generation is unambiguous and collision-safe even when segments contain separator-like text (｡•̀ᴗ-)✧.
Added regression coverage for collision-shaped tuples too, so this path is now locked down nicely ದ್ದി(˵ •̀ ᴗ - ˵ ).
| } as IBaseWidget | ||
| } | ||
|
|
||
| describe(getStableWidgetRenderKey, () => { |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): The test file covers key stability and uniqueness but doesn't test the promoted vs widget prefix branch -- the function's primary differentiator (isPromotedWidgetView(widget) ? 'promoted' : 'widget'). Consider adding a test with a mock PromotedWidgetView verifying the key starts with 'promoted:'.
Contributor
Author
There was a problem hiding this comment.
Totally reasonable suggestion~ ♡(˶╹̆ ▿ ╹̆˵)♡
Treated this as non-blocking in this pass and prioritized higher-impact correctness items first. Happy to add this branch-coverage polish in a tiny follow-up (^_^)/.
| if (!inputs) return undefined | ||
|
|
||
| return ( | ||
| inputs.find((input) => input._widget === widget) ?? |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): The name-fallback path (inputs.find((input) => input.name === widget.name)) can match the wrong input when there are duplicate names -- the exact scenario this PR is fixing. The first name match wins, which may be incorrect. Consider documenting the constraint that callers must scope inputs to avoid ambiguity, or removing the name fallback.
Contributor
Author
There was a problem hiding this comment.
Fixed with a stricter fallback in matchPromotedInput (´｡• ᵕ •｡`)/. We still prioritize exact _widget matching, and now name fallback only applies when the name is uniquely matched; duplicate-name ambiguity now returns undefined instead of guessing ૮꒰ ˶• ༝ •˶꒱ა.
Added tests for both unique fallback and duplicate rejection.
| const cachedViews = this._promotedViewsCache | ||
| if ( | ||
| cachedViews?.version === this._cacheVersion && | ||
| cachedViews.entriesRef === entries && |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): cachedViews.entriesRef === entries uses reference equality, but getPromotionsRef() returns ?? [] (in promotionStore.ts:65), which creates a new array on every call for nodes with no promotions. This means the cache always misses for those nodes. Consider caching a stable empty-array sentinel in getPromotionsRef.
Contributor
Author
There was a problem hiding this comment.
Implemented~ We now return a stable shared empty ref from getPromotionsRef in the empty case, so referential checks stay stable and we avoid unnecessary cache churn (˶ᵔ ᵕ ᵔ˶).
Added a dedicated test for stable empty ref behavior too (ฅ́˘ฅ̀).
| if (!subgraphData) | ||
| throw new Error('Expected fixture to contain one subgraph definition') | ||
|
|
||
| LiteGraph.registerNodeType( |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): FixtureStringConcatenateNode is registered globally via LiteGraph.registerNodeType but never unregistered. This leaks across test files and could cause ordering-dependent test failures. Consider adding cleanup in afterEach or using vitest hooks to unregister after tests complete.
Contributor
Author
There was a problem hiding this comment.
Great catch, fixed! We added fixture node-type cleanup so Fixture/StringConcatenate registration is torn down cleanly and does not leak across tests ᐢ•˕•ᐢ.
Also added a focused helper cleanup test and wired teardown in the fixture-heavy promoted-widget tests (❀◠‿◠).
- avoid ambiguous same-name fallback in matchPromotedInput - use unambiguous promotion view key encoding in SubgraphNode - stabilize empty promotion refs for cache checks - add fixture node-type teardown and regression tests Amp-Thread-ID: https://ampcode.com/threads/T-019ceacb-1ff8-723b-bd39-29e41cb522b0 Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/litegraph/src/subgraph/SubgraphNode.ts (1)
543-557:⚠️ Potential issue | 🟠 MajorUse collision-free encoding for the remaining promotion and slot signatures.
_makePromotionEntryKey()and the rebindsignaturestill concatenate arbitrary strings with:. The new colon-placement regression test shows names likea:bare in scope, and scoped node ids can also contain:. Different tuples can therefore collapse to the same key, which can prune the wrong fallback promotion or rebind the wrong subgraph slot during configure/hydration.🔧 Suggested fix
- const signature = `${slot.name}:${String(slot.type)}` + const signature = JSON.stringify([slot.name, String(slot.type)]) @@ - const signature = `${input.name}:${String(input.type)}` + const signature = JSON.stringify([input.name, String(input.type)]) @@ - return `${interiorNodeId}:${widgetName}` + return JSON.stringify([interiorNodeId, widgetName])Also applies to: 823-864
🤖 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 543 - 557, The current key construction in _makePromotionEntryKey and the rebind/signature logic concatenates arbitrary strings with ":" which can collide for values containing colons; change _makePromotionEntryKey to produce a collision-free encoding (e.g. JSON.stringify([interiorNodeId, widgetName])) and update any related rebind/signature and slot-promotion key generators to use the same tuple-encoding approach (use JSON.stringify on arrays of the components) so all promotion, view and slot keys are produced consistently and avoid collisions.
🧹 Nitpick comments (1)
src/lib/litegraph/src/subgraph/__fixtures__/subgraphHelpers.test.ts (1)
12-12: ExportFIXTURE_STRING_CONCAT_TYPEfromsubgraphHelpers.tsand import it in the test to avoid duplication.The constant
FIXTURE_STRING_CONCAT_TYPEis defined in bothsubgraphHelpers.tsand this test file. It's currently not exported fromsubgraphHelpers.ts, but exporting it and importing it here would establish a single source of truth, reducing the risk of accidental drift if the fixture type name changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/subgraph/__fixtures__/subgraphHelpers.test.ts` at line 12, The constant FIXTURE_STRING_CONCAT_TYPE is duplicated in the test; export FIXTURE_STRING_CONCAT_TYPE from subgraphHelpers.ts and remove the local declaration in subgraphHelpers.test.ts, then import { FIXTURE_STRING_CONCAT_TYPE } from the subgraphHelpers module in the test; ensure the export is a named export (export const FIXTURE_STRING_CONCAT_TYPE = 'Fixture/StringConcatenate') and update any references in the test to use the imported symbol.
🤖 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/core/graph/subgraph/promotedWidgetView.test.ts`:
- Around line 107-109: Hoist the fixture cleanup to the file scope so the
registered LiteGraph node type is removed for all tests: remove the inner
describe's afterEach(() => cleanupComplexPromotionFixtureNodeType()) and add a
top-level afterEach(() => cleanupComplexPromotionFixtureNodeType()) (or afterAll
if you prefer cleanup only once after the whole file) next to the file-scope
setupComplexPromotionFixture() calls so cleanupComplexPromotionFixtureNodeType()
always runs after tests that call setupComplexPromotionFixture(); apply the same
change for the other block covering SubgraphNode.widgets getter (the later range
mentioned).
---
Duplicate comments:
In `@src/lib/litegraph/src/subgraph/SubgraphNode.ts`:
- Around line 543-557: The current key construction in _makePromotionEntryKey
and the rebind/signature logic concatenates arbitrary strings with ":" which can
collide for values containing colons; change _makePromotionEntryKey to produce a
collision-free encoding (e.g. JSON.stringify([interiorNodeId, widgetName])) and
update any related rebind/signature and slot-promotion key generators to use the
same tuple-encoding approach (use JSON.stringify on arrays of the components) so
all promotion, view and slot keys are produced consistently and avoid
collisions.
---
Nitpick comments:
In `@src/lib/litegraph/src/subgraph/__fixtures__/subgraphHelpers.test.ts`:
- Line 12: The constant FIXTURE_STRING_CONCAT_TYPE is duplicated in the test;
export FIXTURE_STRING_CONCAT_TYPE from subgraphHelpers.ts and remove the local
declaration in subgraphHelpers.test.ts, then import { FIXTURE_STRING_CONCAT_TYPE
} from the subgraphHelpers module in the test; ensure the export is a named
export (export const FIXTURE_STRING_CONCAT_TYPE = 'Fixture/StringConcatenate')
and update any references in the test to use the imported symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4645c850-02fc-4214-9e9e-f76ab66a73dc
📒 Files selected for processing (9)
src/core/graph/subgraph/matchPromotedInput.test.tssrc/core/graph/subgraph/matchPromotedInput.tssrc/core/graph/subgraph/promotedWidgetView.test.tssrc/lib/litegraph/src/subgraph/SubgraphNode.test.tssrc/lib/litegraph/src/subgraph/SubgraphNode.tssrc/lib/litegraph/src/subgraph/__fixtures__/subgraphHelpers.test.tssrc/lib/litegraph/src/subgraph/__fixtures__/subgraphHelpers.tssrc/stores/promotionStore.test.tssrc/stores/promotionStore.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/litegraph/src/subgraph/fixtures/subgraphHelpers.ts
- src/core/graph/subgraph/matchPromotedInput.test.ts
- src/core/graph/subgraph/matchPromotedInput.ts
Amp-Thread-ID: https://ampcode.com/threads/T-019ceb70-b12c-75ac-9a58-7d2b39dc569a Co-authored-by: Amp <amp@ampcode.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
3 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 subgraph promoted widget identity/rendering so on-node widgets stay correct through configure/hydration churn, duplicate names, and linked+independent coexistence.
Changes
getSlotFromWidget/getWidgetFromSlot) to avoid same-name collisions.NodeWidgetsidentity and dedup to avoid visual aliasing, prefer visible duplicates over hidden stale entries, include type/source execution identity, and avoid collapsing transient unresolved entries.useGraphNodeManagerpromoted source mapping to resolve by input target only when the promoted view is actually bound to that input.seed,seed_1, etc.) for same-name multi-source promotions.subgraph_complex_promotion_1, linked+independent same-name cases, duplicate-name identity mapping, dedup behavior, and input-name uniqueness.Review Focus
Validate behavior around transient configure/hydration states (
-1id to concrete id), duplicate-name promotions, linked representative recovery, and that dedup never hides legitimate widgets while still removing true duplicates.┆Issue is synchronized with this Notion page by Unito