From 30ea6774561d3d1f7301c03a2d1f93bc36a87dfc Mon Sep 17 00:00:00 2001 From: DrJKL Date: Fri, 8 May 2026 01:19:26 -0700 Subject: [PATCH] fix(subgraph): address promotion review gaps Amp-Thread-ID: https://ampcode.com/threads/T-019e05c3-bed1-706a-a7a7-27733a6ab1e4 Co-authored-by: Amp --- src/components/builder/AppBuilder.vue | 11 ++-- .../node/usePromotedPreviews.test.ts | 51 +++++++++++++++++++ src/composables/node/usePromotedPreviews.ts | 43 ++++++++++++++-- .../migration/classifyProxyEntry.test.ts | 29 +++++++++++ .../subgraph/migration/classifyProxyEntry.ts | 28 +++++++--- .../migration/repairValueWidget.test.ts | 41 +++++++++++++++ .../subgraph/migration/repairValueWidget.ts | 27 ++++++++-- .../litegraph/src/subgraph/SubgraphNode.ts | 18 ++++--- src/stores/appModeStore.ts | 27 ++++++++-- src/utils/litegraphUtil.ts | 12 +++++ 10 files changed, 257 insertions(+), 30 deletions(-) diff --git a/src/components/builder/AppBuilder.vue b/src/components/builder/AppBuilder.vue index 5f82a1f664..dcdb363c49 100644 --- a/src/components/builder/AppBuilder.vue +++ b/src/components/builder/AppBuilder.vue @@ -29,6 +29,7 @@ import { renameWidget } from '@/utils/widgetUtil' import { useAppMode } from '@/composables/useAppMode' import { nodeTypeValidForApp, useAppModeStore } from '@/stores/appModeStore' import { resolveNodeWidget } from '@/utils/litegraphUtil' +import { createNodeLocatorId } from '@/types/nodeIdentification' import { cn } from '@comfyorg/tailwind-utils' type BoundStyle = { top: string; left: string; width: string; height: string } @@ -157,10 +158,12 @@ function handleClick(e: MouseEvent) { } if (!isSelectInputsMode.value || widget.options.canvasOnly) return - const storeId = isPromotedWidgetView(widget) ? widget.sourceNodeId : node.id - const storeName = isPromotedWidgetView(widget) - ? widget.sourceWidgetName - : widget.name + const isPromoted = isPromotedWidgetView(widget) + const storeId = + isPromoted && app.rootGraph?.id + ? createNodeLocatorId(app.rootGraph.id, node.id) + : node.id + const storeName = widget.name const index = appModeStore.selectedInputs.findIndex( ([nodeId, widgetName]) => storeId == nodeId && storeName === widgetName ) diff --git a/src/composables/node/usePromotedPreviews.test.ts b/src/composables/node/usePromotedPreviews.test.ts index 84f460bff7..6f360d501a 100644 --- a/src/composables/node/usePromotedPreviews.test.ts +++ b/src/composables/node/usePromotedPreviews.test.ts @@ -268,4 +268,55 @@ describe(usePromotedPreviews, () => { expect(promotedPreviews.value).toHaveLength(1) expect(promotedPreviews.value[0].urls).toEqual(mockUrls) }) + + it('renders leaf media exposed through a nested subgraph host', () => { + const innerSetup = createSetup() + const leafNode = addInteriorNode(innerSetup, { + id: 10, + previewMediaType: 'image' + }) + + const outerSetup = createSetup() + const innerHost = createTestSubgraphNode(innerSetup.subgraph, { id: 20 }) + outerSetup.subgraph.add(innerHost) + + const store = usePreviewExposureStore() + store.addExposure( + outerSetup.subgraphNode.rootGraph.id, + createNodeLocatorId(outerSetup.subgraphNode.rootGraph.id, innerHost.id), + { + sourceNodeId: String(leafNode.id), + sourcePreviewName: '$$canvas-image-preview' + } + ) + store.addExposure( + outerSetup.subgraphNode.rootGraph.id, + createNodeLocatorId( + outerSetup.subgraphNode.rootGraph.id, + outerSetup.subgraphNode.id + ), + { + sourceNodeId: String(innerHost.id), + sourcePreviewName: '$$canvas-image-preview' + } + ) + + const mockUrls = ['/view?filename=leaf.png'] + seedOutputs(innerSetup.subgraph.id, [leafNode.id]) + getNodeImageUrls.mockImplementation((node: LGraphNode) => + node === leafNode ? mockUrls : [] + ) + + const { promotedPreviews } = usePromotedPreviews( + () => outerSetup.subgraphNode + ) + expect(promotedPreviews.value).toEqual([ + { + sourceNodeId: '10', + sourceWidgetName: '$$canvas-image-preview', + type: 'image', + urls: mockUrls + } + ]) + }) }) diff --git a/src/composables/node/usePromotedPreviews.ts b/src/composables/node/usePromotedPreviews.ts index 8905a2b936..3070af657f 100644 --- a/src/composables/node/usePromotedPreviews.ts +++ b/src/composables/node/usePromotedPreviews.ts @@ -6,6 +6,7 @@ import { SubgraphNode } from '@/lib/litegraph/src/subgraph/SubgraphNode' import { useNodeOutputStore } from '@/stores/nodeOutputStore' import { usePreviewExposureStore } from '@/stores/previewExposureStore' import { createNodeLocatorId } from '@/types/nodeIdentification' +import type { UUID } from '@/lib/litegraph/src/utils/uuid' interface PromotedPreview { /** Source node id resolved on the host's interior subgraph. */ @@ -41,6 +42,7 @@ export function usePromotedPreviews( ) const exposurePairs = exposures.map((exposure) => ({ + exposureName: exposure.name, sourceNodeId: exposure.sourceNodeId, sourceWidgetName: exposure.sourcePreviewName })) @@ -48,16 +50,49 @@ export function usePromotedPreviews( if (!exposurePairs.length) return [] const previews: PromotedPreview[] = [] + const hostNodesByLocator = new Map([ + [hostLocator, node] + ]) + + const resolveNestedHost = ( + rootGraphId: UUID, + currentHostLocator: string, + sourceNodeId: string + ) => { + const currentHost = hostNodesByLocator.get(currentHostLocator) + const sourceNode = currentHost?.subgraph.getNodeById(sourceNodeId) + if (!(sourceNode instanceof SubgraphNode)) return undefined + + const nestedHostLocator = createNodeLocatorId(rootGraphId, sourceNode.id) + hostNodesByLocator.set(nestedHostLocator, sourceNode) + return { rootGraphId, hostNodeLocator: nestedHostLocator } + } for (const pair of exposurePairs) { - const interiorNode = node.subgraph.getNodeById(pair.sourceNodeId) + const resolved = previewExposureStore.resolveChain( + rootGraphId, + hostLocator, + pair.exposureName, + resolveNestedHost + ) + const leaf = resolved?.leaf ?? { + sourceNodeId: pair.sourceNodeId, + sourcePreviewName: pair.sourceWidgetName + } + const leafHostLocator = + resolved?.steps.at(-1)?.hostNodeLocator ?? hostLocator + const leafHost = hostNodesByLocator.get(leafHostLocator) ?? node + const interiorNode = leafHost.subgraph.getNodeById(leaf.sourceNodeId) if (!interiorNode) continue // Read from both reactive refs to establish Vue dependency // tracking. getNodeImageUrls reads from non-reactive // app.nodeOutputs / app.nodePreviewImages, so without this // access the computed would never re-evaluate. - const locatorId = createNodeLocatorId(node.subgraph.id, pair.sourceNodeId) + const locatorId = createNodeLocatorId( + leafHost.subgraph.id, + leaf.sourceNodeId + ) const reactiveOutputs = nodeOutputStore.nodeOutputs[locatorId] const reactivePreviews = nodeOutputStore.nodePreviewImages[locatorId] if (!reactiveOutputs?.images?.length && !reactivePreviews?.length) @@ -74,8 +109,8 @@ export function usePromotedPreviews( : 'image' previews.push({ - sourceNodeId: pair.sourceNodeId, - sourceWidgetName: pair.sourceWidgetName, + sourceNodeId: leaf.sourceNodeId, + sourceWidgetName: leaf.sourcePreviewName, type, urls }) diff --git a/src/core/graph/subgraph/migration/classifyProxyEntry.test.ts b/src/core/graph/subgraph/migration/classifyProxyEntry.test.ts index c86f118a0d..5a7df49085 100644 --- a/src/core/graph/subgraph/migration/classifyProxyEntry.test.ts +++ b/src/core/graph/subgraph/migration/classifyProxyEntry.test.ts @@ -114,6 +114,35 @@ describe(classifyProxyEntry, () => { subgraphInputName: 'second_seed' }) }) + + it('quarantines ambiguous already-linked inputs without a disambiguator', () => { + const host = buildHost() + const innerNode = new LGraphNode('Inner') + innerNode.addWidget('number', 'seed', 0, () => {}) + host.subgraph.add(innerNode) + + for (const inputName of ['first_seed', 'second_seed']) { + const input = host.addInput(inputName, '*') + input._widget = fromPartial({ + node: host, + name: 'seed', + sourceNodeId: String(innerNode.id), + sourceWidgetName: 'seed' + }) + } + + const normalized = makeSource(String(innerNode.id), 'seed') + const result = classifyProxyEntry({ + hostNode: host, + normalized, + cohort: [normalized] + }) + + expect(result).toEqual({ + classification: 'unknown', + plan: { kind: 'quarantine', reason: 'ambiguousSubgraphInput' } + }) + }) }) describe('quarantine branches', () => { diff --git a/src/core/graph/subgraph/migration/classifyProxyEntry.ts b/src/core/graph/subgraph/migration/classifyProxyEntry.ts index cb0469f531..af472e1607 100644 --- a/src/core/graph/subgraph/migration/classifyProxyEntry.ts +++ b/src/core/graph/subgraph/migration/classifyProxyEntry.ts @@ -26,10 +26,16 @@ interface ClassifyProxyEntryArgs { const PRIMITIVE_NODE_TYPE = 'PrimitiveNode' -function findLinkedSubgraphInputName( +type LinkedInputMatch = + | { kind: 'none' } + | { kind: 'one'; inputName: string } + | { kind: 'ambiguous' } + +function findLinkedSubgraphInputMatch( hostNode: SubgraphNode, normalized: PromotedWidgetSource -): string | undefined { +): LinkedInputMatch { + const matches: string[] = [] for (const input of hostNode.inputs) { const widget = input._widget if (!widget || !isPromotedWidgetView(widget)) continue @@ -40,10 +46,12 @@ function findLinkedSubgraphInputName( widget.disambiguatingSourceNodeId === normalized.disambiguatingSourceNodeId) ) { - return input.name + matches.push(input.name) } } - return undefined + if (matches.length === 0) return { kind: 'none' } + if (matches.length === 1) return { kind: 'one', inputName: matches[0] } + return { kind: 'ambiguous' } } function collectPrimitiveTargets( @@ -84,11 +92,17 @@ export function classifyProxyEntry( ): ClassificationResult { const { hostNode, normalized, cohort } = args - const linkedInputName = findLinkedSubgraphInputName(hostNode, normalized) - if (linkedInputName !== undefined) { + const linkedInput = findLinkedSubgraphInputMatch(hostNode, normalized) + if (linkedInput.kind === 'one') { return { classification: 'value', - plan: { kind: 'alreadyLinked', subgraphInputName: linkedInputName } + plan: { kind: 'alreadyLinked', subgraphInputName: linkedInput.inputName } + } + } + if (linkedInput.kind === 'ambiguous') { + return { + classification: 'unknown', + plan: { kind: 'quarantine', reason: 'ambiguousSubgraphInput' } } } diff --git a/src/core/graph/subgraph/migration/repairValueWidget.test.ts b/src/core/graph/subgraph/migration/repairValueWidget.test.ts index a331166030..25090a6a60 100644 --- a/src/core/graph/subgraph/migration/repairValueWidget.test.ts +++ b/src/core/graph/subgraph/migration/repairValueWidget.test.ts @@ -158,6 +158,47 @@ describe(repairValueWidget, () => { expect(secondInput._widget?.value).toBe(99) }) + it('does not apply host value when already-linked inputs are ambiguous', () => { + const host = buildHost() + const innerNode = new LGraphNode('Inner') + innerNode.addWidget('number', 'seed', 0, () => {}) + host.subgraph.add(innerNode) + + const firstInput = host.addInput('first_seed', '*') + firstInput._widget = fromPartial({ + node: host, + name: 'seed', + sourceNodeId: String(innerNode.id), + sourceWidgetName: 'seed', + value: 1 + }) + const secondInput = host.addInput('second_seed', '*') + secondInput._widget = fromPartial({ + node: host, + name: 'seed', + sourceNodeId: String(innerNode.id), + sourceWidgetName: 'seed', + value: 2 + }) + + const result = repairValueWidget({ + hostNode: host, + entry: buildEntry({ + sourceNodeId: String(innerNode.id), + sourceWidgetName: 'seed', + plan: { + kind: 'alreadyLinked', + subgraphInputName: undefined as never + }, + hostValue: 99 + }) + }) + + expect(result).toEqual({ ok: false, reason: 'ambiguousSubgraphInput' }) + expect(firstInput._widget?.value).toBe(1) + expect(secondInput._widget?.value).toBe(2) + }) + it('returns missingSubgraphInput when the linked SubgraphInput is gone', () => { const host = buildHost() const innerNode = new LGraphNode('Inner') diff --git a/src/core/graph/subgraph/migration/repairValueWidget.ts b/src/core/graph/subgraph/migration/repairValueWidget.ts index 439db848f2..72aa5ed799 100644 --- a/src/core/graph/subgraph/migration/repairValueWidget.ts +++ b/src/core/graph/subgraph/migration/repairValueWidget.ts @@ -25,9 +25,17 @@ function findHostInputForLinkedSource( hostNode: SubgraphNode, sourceNodeId: string, sourceWidgetName: string, + subgraphInputName: string | undefined, disambiguatingSourceNodeId?: string -) { - return hostNode.inputs.find((input) => { +): + | { kind: 'none' } + | { kind: 'one'; input: INodeInputSlot } + | { kind: 'ambiguous' } { + const candidates = subgraphInputName + ? hostNode.inputs.filter((input) => input.name === subgraphInputName) + : hostNode.inputs + + const matches = candidates.filter((input) => { const widget = input._widget if (!widget || !isPromotedWidgetView(widget)) return false return ( @@ -37,6 +45,9 @@ function findHostInputForLinkedSource( widget.disambiguatingSourceNodeId === disambiguatingSourceNodeId) ) }) + if (matches.length === 0) return { kind: 'none' } + if (matches.length === 1) return { kind: 'one', input: matches[0] } + return { kind: 'ambiguous' } } function applyHostValue( @@ -55,14 +66,20 @@ function repairAlreadyLinked( hostNode, entry.normalized.sourceNodeId, entry.normalized.sourceWidgetName, + entry.plan.kind === 'alreadyLinked' + ? entry.plan.subgraphInputName + : undefined, entry.normalized.disambiguatingSourceNodeId ) - if (!hostInput?._widget) { + if (hostInput.kind === 'ambiguous') { + return { ok: false, reason: 'ambiguousSubgraphInput' } + } + if (hostInput.kind === 'none' || !hostInput.input._widget) { return { ok: false, reason: 'missingSubgraphInput' } } - applyHostValue(hostInput._widget, entry.hostValue) - return { ok: true, subgraphInputName: hostInput.name } + applyHostValue(hostInput.input._widget, entry.hostValue) + return { ok: true, subgraphInputName: hostInput.input.name } } function repairCreateSubgraphInput( diff --git a/src/lib/litegraph/src/subgraph/SubgraphNode.ts b/src/lib/litegraph/src/subgraph/SubgraphNode.ts index ecf18c10b1..e78f76d165 100644 --- a/src/lib/litegraph/src/subgraph/SubgraphNode.ts +++ b/src/lib/litegraph/src/subgraph/SubgraphNode.ts @@ -726,7 +726,7 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph { let valueIndex = 0 for (const input of this.inputs) { const widget = input._widget - if (!widget) continue + if (!widget || !isPromotedWidgetView(widget)) continue if (valueIndex >= widgetValues.length) return widget.value = widgetValues[valueIndex] valueIndex += 1 @@ -1232,18 +1232,24 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph { const serialized = super.serialize() const widgetStore = useWidgetValueStore() - const widgetValues = this.inputs.flatMap((input) => { + const widgetValues: TWidgetValue[] = [] + let hasSerializableValue = false + + for (const input of this.inputs) { const widget = input._widget - if (!widget || !isPromotedWidgetView(widget)) return [] + if (!widget || !isPromotedWidgetView(widget)) continue const state = widgetStore.getWidget( rootGraphId, this.id, getPromotedWidgetHostStateName(widget) ) - return state && isWidgetValue(state.value) ? [state.value] : [] - }) + const value = + state && isWidgetValue(state.value) ? state.value : undefined + widgetValues.push(value) + hasSerializableValue ||= value !== undefined + } - if (widgetValues.length > 0) serialized.widgets_values = widgetValues + if (hasSerializableValue) serialized.widgets_values = widgetValues return serialized } diff --git a/src/stores/appModeStore.ts b/src/stores/appModeStore.ts index e8bfdef8bb..5c20c20b02 100644 --- a/src/stores/appModeStore.ts +++ b/src/stores/appModeStore.ts @@ -20,7 +20,7 @@ import { ChangeTracker } from '@/scripts/changeTracker' import { isPromotedWidgetView } from '@/core/graph/subgraph/promotedWidgetTypes' import type { IBaseWidget } from '@/lib/litegraph/src/types/widgets' import { createNodeLocatorId } from '@/types/nodeIdentification' -import { resolveNode } from '@/utils/litegraphUtil' +import { resolveNode, resolveNodeWidget } from '@/utils/litegraphUtil' export function nodeTypeValidForApp(type: string) { return !['Note', 'MarkdownNote'].includes(type) @@ -61,7 +61,7 @@ export const useAppModeStore = defineStore('appMode', () => { ? rawInputs .map(migrateLegacyInputTuple) .filter((entry): entry is LinearInput => entry !== null) - .filter(([nodeId]) => resolveNode(nodeId)) + .filter(selectedInputExists) : rawInputs, outputs: app.rootGraph ? rawOutputs.filter((nodeId) => resolveNode(nodeId)) @@ -69,6 +69,15 @@ export const useAppModeStore = defineStore('appMode', () => { } } + function selectedInputExists([nodeId, widgetName]: LinearInput): boolean { + if (typeof nodeId === 'string' && nodeId.includes(':')) { + if (typeof app.rootGraph?.getNodeById !== 'function') return true + const [, widget] = resolveNodeWidget(nodeId, widgetName) + return Boolean(widget) + } + return Boolean(resolveNode(nodeId)) + } + /** * If a legacy tuple references the interior `(sourceNodeId, widgetName)` * of a now-promoted widget, project it through the wrapping host @@ -102,6 +111,9 @@ export const useAppModeStore = defineStore('appMode', () => { const rootGraph = app.rootGraph if (!rootGraph) return null + const matches: Array<{ hostLocator: string; subgraphInputName: string }> = + [] + for (const node of rootGraph.nodes) { if (!(node instanceof SubgraphNode)) continue @@ -112,14 +124,21 @@ export const useAppModeStore = defineStore('appMode', () => { widget.sourceNodeId === String(legacySourceNodeId) && widget.sourceWidgetName === legacyWidgetName ) { - return { + matches.push({ hostLocator: createNodeLocatorId(rootGraph.id, node.id), subgraphInputName: inputSlot.name - } + }) } } } + if (matches.length === 1) return matches[0] + if (matches.length > 1) { + console.warn( + '[appModeStore] dropping ambiguous legacy selectedInput tuple', + { storedId: legacySourceNodeId, widgetName: legacyWidgetName } + ) + } return null } diff --git a/src/utils/litegraphUtil.ts b/src/utils/litegraphUtil.ts index e140d5a7df..60e5e1a76b 100644 --- a/src/utils/litegraphUtil.ts +++ b/src/utils/litegraphUtil.ts @@ -26,6 +26,7 @@ import type { InputSpec } from '@/schemas/nodeDef/nodeDefSchemaV2' import { useToastStore } from '@/platform/updates/common/toastStore' import { app } from '@/scripts/app' import { t } from '@/i18n' +import { parseNodeLocatorId } from '@/types/nodeIdentification' type ImageNode = LGraphNode & { imgs: HTMLImageElement[] | undefined } type VideoNode = LGraphNode & { @@ -333,6 +334,17 @@ export function resolveNodeWidget( widgetName?: string, graph: LGraph = app.rootGraph ): [LGraphNode, IBaseWidget] | [LGraphNode] | [] { + if (widgetName && typeof nodeId === 'string') { + const locator = parseNodeLocatorId(nodeId) + if (locator?.subgraphUuid) { + const host = graph.getNodeById(locator.localNodeId) + if (host?.isSubgraphNode()) { + const widget = host.widgets?.find((w) => w.name === widgetName) + return widget ? [host, widget] : [] + } + } + } + const node = graph.getNodeById(nodeId) if (!widgetName) return node ? [node] : [] if (node) {