mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-20 06:20:11 +00:00
fix: address PR 10851 review feedback
This commit is contained in:
@@ -202,7 +202,9 @@ test.describe('Nested Subgraphs', { tag: ['@subgraph'] }, () => {
|
||||
* Node 6 (Sub 1) has proxyWidgets promoting widgets from inner nodes,
|
||||
* and those promotions are also promoted up to node 5 (Sub 0). When
|
||||
* navigating into Sub 0, node 6 should show the promoted ring on its
|
||||
* widgets.
|
||||
* widgets. The fixture's proxyWidgets entries are scoped to Sub 1's
|
||||
* local graph, so the nested `string_a` promotion correctly points at
|
||||
* inner node 9 instead of the outer SubgraphNode 5.
|
||||
*/
|
||||
test.describe(
|
||||
'Promoted indicator on 3-level nested subgraphs (#10612)',
|
||||
@@ -225,14 +227,8 @@ test.describe('Nested Subgraphs', { tag: ['@subgraph'] }, () => {
|
||||
const outerRings = outerNode.locator(`.${PROMOTED_BORDER_CLASS}`)
|
||||
await comfyExpect(outerRings).toHaveCount(0)
|
||||
|
||||
// Navigate programmatically — the enter-subgraph button on
|
||||
// node 5 is obscured by the canvas z-999 overlay at root level.
|
||||
await comfyPage.page.evaluate(() => {
|
||||
const node = window.app!.graph!.getNodeById('5')
|
||||
if (node?.isSubgraphNode()) {
|
||||
window.app!.canvas.setGraph(node.subgraph)
|
||||
}
|
||||
})
|
||||
// Exercise the same enter-subgraph control users click.
|
||||
await comfyPage.vueNodes.enterSubgraph('5')
|
||||
await comfyPage.nextFrame()
|
||||
await comfyPage.vueNodes.waitForNodes()
|
||||
|
||||
|
||||
@@ -70,6 +70,43 @@ describe('normalizeLegacyProxyWidgetEntry', () => {
|
||||
})
|
||||
})
|
||||
|
||||
it('infers the leaf-node disambiguator for nested subgraph entries', () => {
|
||||
const rootGraph = createTestRootGraph()
|
||||
const innerSubgraph = createTestSubgraph({
|
||||
rootGraph,
|
||||
inputs: [{ name: 'seed', type: 'number' }]
|
||||
})
|
||||
|
||||
const samplerNode = new LGraphNode('Sampler')
|
||||
const samplerInput = samplerNode.addInput('seed', 'number')
|
||||
samplerNode.addWidget('number', 'noise_seed', 42, () => {})
|
||||
samplerInput.widget = { name: 'noise_seed' }
|
||||
innerSubgraph.add(samplerNode)
|
||||
innerSubgraph.inputNode.slots[0].connect(samplerNode.inputs[0], samplerNode)
|
||||
|
||||
const outerSubgraph = createTestSubgraph({ rootGraph })
|
||||
const nestedNode = createTestSubgraphNode(innerSubgraph, {
|
||||
parentGraph: outerSubgraph
|
||||
})
|
||||
outerSubgraph.add(nestedNode)
|
||||
|
||||
const hostNode = createTestSubgraphNode(outerSubgraph, {
|
||||
parentGraph: rootGraph
|
||||
})
|
||||
|
||||
const result = normalizeLegacyProxyWidgetEntry(
|
||||
hostNode,
|
||||
String(nestedNode.id),
|
||||
'seed'
|
||||
)
|
||||
|
||||
expect(result).toEqual({
|
||||
sourceNodeId: String(nestedNode.id),
|
||||
sourceWidgetName: 'seed',
|
||||
disambiguatingSourceNodeId: String(samplerNode.id)
|
||||
})
|
||||
})
|
||||
|
||||
it('strips a single legacy prefix from widget name', () => {
|
||||
const rootGraph = createTestRootGraph()
|
||||
const innerSubgraph = createTestSubgraph({
|
||||
|
||||
@@ -6,34 +6,30 @@ const LEGACY_PROXY_WIDGET_PREFIX_PATTERN = /^\s*(\d+)\s*:\s*(.+)$/
|
||||
|
||||
type PromotedWidgetPatch = Omit<PromotedWidgetSource, 'sourceNodeId'>
|
||||
|
||||
function canResolve(
|
||||
hostNode: SubgraphNode,
|
||||
sourceNodeId: string,
|
||||
widgetName: string,
|
||||
disambiguator?: string
|
||||
): boolean {
|
||||
return (
|
||||
resolveConcretePromotedWidget(
|
||||
hostNode,
|
||||
sourceNodeId,
|
||||
widgetName,
|
||||
disambiguator
|
||||
).status === 'resolved'
|
||||
)
|
||||
}
|
||||
|
||||
function tryResolveCandidate(
|
||||
function resolveCandidate(
|
||||
hostNode: SubgraphNode,
|
||||
sourceNodeId: string,
|
||||
widgetName: string,
|
||||
disambiguator?: string
|
||||
): PromotedWidgetPatch | undefined {
|
||||
if (!canResolve(hostNode, sourceNodeId, widgetName, disambiguator))
|
||||
return undefined
|
||||
const result = resolveConcretePromotedWidget(
|
||||
hostNode,
|
||||
sourceNodeId,
|
||||
widgetName,
|
||||
disambiguator
|
||||
)
|
||||
if (result.status !== 'resolved') return undefined
|
||||
|
||||
const sourceNode = hostNode.subgraph.getNodeById(sourceNodeId)
|
||||
const inferredDisambiguator =
|
||||
disambiguator ??
|
||||
(sourceNode?.isSubgraphNode() ? String(result.resolved.node.id) : undefined)
|
||||
|
||||
return {
|
||||
sourceWidgetName: widgetName,
|
||||
...(disambiguator && { disambiguatingSourceNodeId: disambiguator })
|
||||
...(inferredDisambiguator && {
|
||||
disambiguatingSourceNodeId: inferredDisambiguator
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -59,7 +55,7 @@ function resolveLegacyPrefixedEntry(
|
||||
]
|
||||
|
||||
for (const disambiguator of disambiguators) {
|
||||
const resolved = tryResolveCandidate(
|
||||
const resolved = resolveCandidate(
|
||||
hostNode,
|
||||
sourceNodeId,
|
||||
remaining,
|
||||
@@ -76,18 +72,19 @@ export function normalizeLegacyProxyWidgetEntry(
|
||||
sourceWidgetName: string,
|
||||
disambiguatingSourceNodeId?: string
|
||||
): PromotedWidgetSource {
|
||||
if (
|
||||
canResolve(
|
||||
hostNode,
|
||||
sourceNodeId,
|
||||
sourceWidgetName,
|
||||
disambiguatingSourceNodeId
|
||||
)
|
||||
) {
|
||||
const exactMatch = resolveCandidate(
|
||||
hostNode,
|
||||
sourceNodeId,
|
||||
sourceWidgetName,
|
||||
disambiguatingSourceNodeId
|
||||
)
|
||||
if (exactMatch) {
|
||||
return {
|
||||
sourceNodeId,
|
||||
sourceWidgetName,
|
||||
...(disambiguatingSourceNodeId && { disambiguatingSourceNodeId })
|
||||
sourceWidgetName: exactMatch.sourceWidgetName,
|
||||
...(exactMatch.disambiguatingSourceNodeId && {
|
||||
disambiguatingSourceNodeId: exactMatch.disambiguatingSourceNodeId
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -98,14 +95,14 @@ export function normalizeLegacyProxyWidgetEntry(
|
||||
disambiguatingSourceNodeId
|
||||
)
|
||||
|
||||
const patchDisambiguatingSourceNodeId =
|
||||
const normalizedDisambiguatingSourceNodeId =
|
||||
patch?.disambiguatingSourceNodeId ?? disambiguatingSourceNodeId
|
||||
|
||||
return {
|
||||
sourceNodeId,
|
||||
sourceWidgetName: patch?.sourceWidgetName ?? sourceWidgetName,
|
||||
...(patchDisambiguatingSourceNodeId && {
|
||||
disambiguatingSourceNodeId: patchDisambiguatingSourceNodeId
|
||||
...(normalizedDisambiguatingSourceNodeId && {
|
||||
disambiguatingSourceNodeId: normalizedDisambiguatingSourceNodeId
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
27
src/core/graph/subgraph/promotionLookup.ts
Normal file
27
src/core/graph/subgraph/promotionLookup.ts
Normal file
@@ -0,0 +1,27 @@
|
||||
import { getActivePinia } from 'pinia'
|
||||
|
||||
import { usePromotionStore } from '@/stores/promotionStore'
|
||||
|
||||
let cachedPromotionStore: ReturnType<typeof usePromotionStore> | undefined
|
||||
|
||||
function getPromotionStore() {
|
||||
const activePinia = getActivePinia()
|
||||
if (!cachedPromotionStore || cachedPromotionStore.$pinia !== activePinia) {
|
||||
cachedPromotionStore = usePromotionStore()
|
||||
}
|
||||
return cachedPromotionStore
|
||||
}
|
||||
|
||||
export function isWidgetPromoted(
|
||||
graphId: string,
|
||||
sourceNodeId: string,
|
||||
sourceWidgetName: string,
|
||||
disambiguatingSourceNodeId?: string
|
||||
): boolean {
|
||||
return getPromotionStore().isWidgetPromoted(
|
||||
graphId,
|
||||
sourceNodeId,
|
||||
sourceWidgetName,
|
||||
disambiguatingSourceNodeId
|
||||
)
|
||||
}
|
||||
@@ -1103,26 +1103,6 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph {
|
||||
})
|
||||
.filter((e): e is NonNullable<typeof e> => e !== null)
|
||||
|
||||
// Infer disambiguatingSourceNodeId for entries whose source node is a
|
||||
// SubgraphNode. The renderer computes a promotionSourceNodeId from the
|
||||
// concrete (leaf) node deep in the promotion chain; the store key must
|
||||
// carry the same value so that exact-match lookups succeed (#10612).
|
||||
for (const entry of entries) {
|
||||
if (entry.disambiguatingSourceNodeId) continue
|
||||
|
||||
const sourceNode = this.subgraph.getNodeById(entry.sourceNodeId)
|
||||
if (!sourceNode?.isSubgraphNode()) continue
|
||||
|
||||
const result = resolveConcretePromotedWidget(
|
||||
this,
|
||||
entry.sourceNodeId,
|
||||
entry.sourceWidgetName
|
||||
)
|
||||
if (result.status === 'resolved') {
|
||||
entry.disambiguatingSourceNodeId = String(result.resolved.node.id)
|
||||
}
|
||||
}
|
||||
|
||||
store.setPromotions(this.rootGraph.id, this.id, entries)
|
||||
|
||||
// Write back resolved entries so legacy or stale entries don't persist
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import { isWidgetPromoted } from '@/core/graph/subgraph/promotionLookup'
|
||||
import { t } from '@/i18n'
|
||||
import { drawTextInArea } from '@/lib/litegraph/src/draw'
|
||||
import { cachedMeasureText } from '@/lib/litegraph/src/utils/textMeasureCache'
|
||||
@@ -17,7 +18,6 @@ import type {
|
||||
NodeBindable,
|
||||
TWidgetType
|
||||
} from '@/lib/litegraph/src/types/widgets'
|
||||
import { usePromotionStore } from '@/stores/promotionStore'
|
||||
import type { WidgetState } from '@/stores/widgetValueStore'
|
||||
import { useWidgetValueStore } from '@/stores/widgetValueStore'
|
||||
|
||||
@@ -211,11 +211,7 @@ export abstract class BaseWidget<TWidget extends IBaseWidget = IBaseWidget>
|
||||
if (
|
||||
graphId &&
|
||||
!suppressPromotedOutline &&
|
||||
usePromotionStore().isWidgetPromoted(
|
||||
graphId,
|
||||
String(this.node.id),
|
||||
this.name
|
||||
)
|
||||
isWidgetPromoted(graphId, String(this.node.id), this.name)
|
||||
)
|
||||
return LiteGraph.WIDGET_PROMOTED_OUTLINE_COLOR
|
||||
return this.advanced
|
||||
|
||||
@@ -9,6 +9,7 @@ import type {
|
||||
} from '@/composables/graph/useGraphNodeManager'
|
||||
import { useAppMode } from '@/composables/useAppMode'
|
||||
import { showNodeOptions } from '@/composables/graph/useMoreOptionsMenu'
|
||||
import { isWidgetPromoted } from '@/core/graph/subgraph/promotionLookup'
|
||||
import type { IWidgetOptions } from '@/lib/litegraph/src/types/widgets'
|
||||
import { LGraphEventMode } from '@/lib/litegraph/src/types/globalEnums'
|
||||
import { useSettingStore } from '@/platform/settings/settingStore'
|
||||
@@ -29,7 +30,6 @@ import {
|
||||
stripGraphPrefix,
|
||||
useWidgetValueStore
|
||||
} from '@/stores/widgetValueStore'
|
||||
import { usePromotionStore } from '@/stores/promotionStore'
|
||||
import { useMissingModelStore } from '@/platform/missingModel/missingModelStore'
|
||||
import { useExecutionErrorStore } from '@/stores/executionErrorStore'
|
||||
import type { LGraph } from '@/lib/litegraph/src/litegraph'
|
||||
@@ -167,7 +167,6 @@ export function computeProcessedWidgets({
|
||||
}: ComputeProcessedWidgetsOptions): ProcessedWidget[] {
|
||||
if (!nodeData?.widgets) return []
|
||||
|
||||
const promotionStore = usePromotionStore()
|
||||
const executionErrorStore = useExecutionErrorStore()
|
||||
const missingModelStore = useMissingModelStore()
|
||||
const widgetValueStore = useWidgetValueStore()
|
||||
@@ -253,7 +252,7 @@ export function computeProcessedWidgets({
|
||||
const bareWidgetId = String(
|
||||
stripGraphPrefix(widget.storeNodeId ?? widget.nodeId ?? nodeId ?? '')
|
||||
)
|
||||
const promotionSourceNodeId = widget.storeName
|
||||
const disambiguatingSourceNodeId = widget.storeName
|
||||
? String(bareWidgetId)
|
||||
: undefined
|
||||
|
||||
@@ -270,14 +269,17 @@ export function computeProcessedWidgets({
|
||||
? { ...mergedOptions, disabled: true }
|
||||
: mergedOptions
|
||||
|
||||
// Nested SubgraphNode promotions are keyed by the subgraph slot name.
|
||||
// storeName still identifies the concrete leaf widget and is only used
|
||||
// as the disambiguator when multiple promoted views share that slot.
|
||||
const sourceWidgetName = widget.slotName ?? widget.name
|
||||
const isPromoted =
|
||||
graphId &&
|
||||
promotionStore.isWidgetPromoted(
|
||||
isWidgetPromoted(
|
||||
graphId,
|
||||
hostNodeId,
|
||||
sourceWidgetName,
|
||||
promotionSourceNodeId
|
||||
disambiguatingSourceNodeId
|
||||
)
|
||||
const borderStyle = isPromoted
|
||||
? 'ring ring-component-node-widget-promoted'
|
||||
|
||||
@@ -12,10 +12,8 @@ vi.mock('@/stores/domWidgetStore', () => ({
|
||||
})
|
||||
}))
|
||||
|
||||
vi.mock('@/stores/promotionStore', () => ({
|
||||
usePromotionStore: () => ({
|
||||
isWidgetPromoted: isWidgetPromotedMock
|
||||
})
|
||||
vi.mock('@/core/graph/subgraph/promotionLookup', () => ({
|
||||
isWidgetPromoted: isWidgetPromotedMock
|
||||
}))
|
||||
|
||||
vi.mock('@/utils/formatUtil', () => ({
|
||||
|
||||
@@ -2,6 +2,7 @@ import _ from 'es-toolkit/compat'
|
||||
import { type Component, toRaw } from 'vue'
|
||||
|
||||
import { useChainCallback } from '@/composables/functional/useChainCallback'
|
||||
import { isWidgetPromoted } from '@/core/graph/subgraph/promotionLookup'
|
||||
import {
|
||||
LGraphNode,
|
||||
LegacyWidget,
|
||||
@@ -13,7 +14,6 @@ import type {
|
||||
} from '@/lib/litegraph/src/types/widgets'
|
||||
import type { InputSpec } from '@/schemas/nodeDef/nodeDefSchemaV2'
|
||||
import { useDomWidgetStore } from '@/stores/domWidgetStore'
|
||||
import { usePromotionStore } from '@/stores/promotionStore'
|
||||
import { generateUUID } from '@/utils/formatUtil'
|
||||
|
||||
export interface BaseDOMWidget<
|
||||
@@ -187,12 +187,7 @@ abstract class BaseDOMWidgetImpl<V extends object | string>
|
||||
|
||||
const graphId = this.node.graph?.rootGraph.id
|
||||
const isPromoted =
|
||||
graphId &&
|
||||
usePromotionStore().isWidgetPromoted(
|
||||
graphId,
|
||||
String(this.node.id),
|
||||
this.name
|
||||
)
|
||||
graphId && isWidgetPromoted(graphId, String(this.node.id), this.name)
|
||||
if (!isPromoted) {
|
||||
this.options.onDraw?.(this)
|
||||
return
|
||||
|
||||
@@ -193,6 +193,29 @@ describe(usePromotionStore, () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('isWidgetPromoted', () => {
|
||||
it('matches exact disambiguated promotion keys', () => {
|
||||
store.promote(graphA, nodeId, {
|
||||
sourceNodeId: '3',
|
||||
sourceWidgetName: 'text',
|
||||
disambiguatingSourceNodeId: '1'
|
||||
})
|
||||
|
||||
expect(store.isWidgetPromoted(graphA, '3', 'text', '1')).toBe(true)
|
||||
expect(store.isWidgetPromoted(graphA, '3', 'text', '2')).toBe(false)
|
||||
})
|
||||
|
||||
it('falls back to the base key when disambiguatingSourceNodeId is omitted', () => {
|
||||
store.promote(graphA, nodeId, {
|
||||
sourceNodeId: '3',
|
||||
sourceWidgetName: 'text',
|
||||
disambiguatingSourceNodeId: '1'
|
||||
})
|
||||
|
||||
expect(store.isWidgetPromoted(graphA, '3', 'text')).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
describe('setPromotions', () => {
|
||||
it('replaces existing entries', () => {
|
||||
store.promote(graphA, nodeId, {
|
||||
|
||||
@@ -49,11 +49,13 @@ export const usePromotionStore = defineStore('promotion', () => {
|
||||
}
|
||||
|
||||
function _decrementKey(refCounts: Map<string, number>, key: string): void {
|
||||
const count = (refCounts.get(key) ?? 1) - 1
|
||||
if (count <= 0) {
|
||||
const current = refCounts.get(key)
|
||||
if (current === undefined) return
|
||||
|
||||
if (current <= 1) {
|
||||
refCounts.delete(key)
|
||||
} else {
|
||||
refCounts.set(key, count)
|
||||
refCounts.set(key, current - 1)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user