mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-24 22:58:08 +00:00
fix(subgraph): address PR review follow-ups
- useGraphNodeManager: guard String(undefined) on promoted source node id - SubgraphNode.serialize: preserve null widget values (drop ?? undefined) - SubgraphNode: trim verbose JSDoc on _pendingWidgetsValuesReplay - promotedWidgetView.findBoundSubgraphSlot: collapse double-loop into one pass - promotedWidgetView.set label: lift asymmetry rationale to JSDoc - widgetValueStore.getOrRegister: drop unsound <TValue> cast; document register-wins semantics - legacyProxyWidgetNormalization: document regex collision trade-off - useProcessedWidgets test: align STORE_NAME with real makeCompositeKey shape Amp-Thread-ID: https://ampcode.com/threads/T-019df4ad-2bed-75ad-91b2-4ef5886b48fa Co-authored-by: Amp <amp@ampcode.com>
This commit is contained in:
@@ -324,13 +324,13 @@ function safeWidgetMapper(
|
||||
const sourceWidgetName = isPromotedWidgetView(widget)
|
||||
? (sourceWidget?.name ?? promotedSource?.sourceWidgetName)
|
||||
: undefined
|
||||
const sourceLocalId = isPromotedWidgetView(widget)
|
||||
? String(
|
||||
sourceNode?.id ??
|
||||
promotedSource?.disambiguatingSourceNodeId ??
|
||||
promotedSource?.sourceNodeId
|
||||
)
|
||||
const rawSourceLocalId = isPromotedWidgetView(widget)
|
||||
? (sourceNode?.id ??
|
||||
promotedSource?.disambiguatingSourceNodeId ??
|
||||
promotedSource?.sourceNodeId)
|
||||
: undefined
|
||||
const sourceLocalId =
|
||||
rawSourceLocalId != null ? String(rawSourceLocalId) : undefined
|
||||
const source: PromotedWidgetSource | undefined =
|
||||
isPromotedWidgetView(widget) && sourceLocalId && sourceWidgetName
|
||||
? {
|
||||
|
||||
@@ -2,6 +2,8 @@ import type { PromotedWidgetSource } from '@/core/graph/subgraph/promotedWidgetT
|
||||
import { resolveConcretePromotedWidget } from '@/core/graph/subgraph/resolveConcretePromotedWidget'
|
||||
import type { SubgraphNode } from '@/lib/litegraph/src/subgraph/SubgraphNode'
|
||||
|
||||
// Collision: a widget literally named `"<digits>: rest"` is ambiguous;
|
||||
// `normalizeLegacyProxyWidgetEntry` resolves the literal name first.
|
||||
const LEGACY_PROXY_WIDGET_PREFIX_PATTERN = /^\s*(\d+)\s*:\s*(.+)$/
|
||||
|
||||
type PromotedWidgetPatch = Omit<PromotedWidgetSource, 'sourceNodeId'>
|
||||
|
||||
@@ -179,6 +179,7 @@ class PromotedWidgetView implements IPromotedWidgetView {
|
||||
return state?.label ?? this.displayName
|
||||
}
|
||||
|
||||
/** Slot-bound: only update existing cell. Unbound: materialize. */
|
||||
set label(value: string | undefined) {
|
||||
const slot = this.getBoundSubgraphSlot()
|
||||
if (slot) slot.label = value || undefined
|
||||
@@ -186,12 +187,6 @@ class PromotedWidgetView implements IPromotedWidgetView {
|
||||
// Pre-attach sentinel guard: skip per-instance cell write before LGraph.add().
|
||||
if (this.subgraphNode.id === -1) return
|
||||
|
||||
// When a slot exists it is the durable home for the label, so only update
|
||||
// an already-materialized per-instance cell. When no slot is bound (e.g.
|
||||
// a freshly selected promoted widget that has no subgraph IO yet), the
|
||||
// per-instance cell is the only place the new label can live, so
|
||||
// materialize it on demand. Without this the rename would silently no-op
|
||||
// and the getter would return `displayName` after the slot-fallback.
|
||||
if (slot) {
|
||||
const existing = this.getWidgetState()
|
||||
if (existing) existing.label = value
|
||||
@@ -219,19 +214,16 @@ class PromotedWidgetView implements IPromotedWidgetView {
|
||||
}
|
||||
|
||||
private findBoundSubgraphSlot(): SubgraphSlotRef | undefined {
|
||||
// Identity match wins; otherwise fall back to source-identity match
|
||||
// (sibling view bound to the same promoted source).
|
||||
let sourceMatch: SubgraphSlotRef | undefined
|
||||
for (const input of this.subgraphNode.inputs ?? []) {
|
||||
const slot = input._subgraphSlot as SubgraphSlotRef | undefined
|
||||
if (!slot) continue
|
||||
|
||||
if (input._widget === this) {
|
||||
return slot
|
||||
}
|
||||
}
|
||||
|
||||
for (const input of this.subgraphNode.inputs ?? []) {
|
||||
const slot = input._subgraphSlot as SubgraphSlotRef | undefined
|
||||
if (!slot) continue
|
||||
if (input._widget === this) return slot
|
||||
|
||||
if (sourceMatch) continue
|
||||
const w = input._widget
|
||||
if (
|
||||
w &&
|
||||
@@ -240,10 +232,10 @@ class PromotedWidgetView implements IPromotedWidgetView {
|
||||
w.sourceWidgetName === this.sourceWidgetName &&
|
||||
w.disambiguatingSourceNodeId === this.disambiguatingSourceNodeId
|
||||
) {
|
||||
return slot
|
||||
sourceMatch = slot
|
||||
}
|
||||
}
|
||||
return undefined
|
||||
return sourceMatch
|
||||
}
|
||||
|
||||
get hidden(): boolean {
|
||||
|
||||
@@ -110,15 +110,7 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph {
|
||||
* lifecycle to persist.
|
||||
*/
|
||||
private _pendingPromotions: PromotedWidgetSource[] = []
|
||||
/**
|
||||
* Widget values buffered during `configure()` when the SubgraphNode is not
|
||||
* yet attached (`id === -1`). The PromotedWidgetView setters short-circuit
|
||||
* at id === -1 to avoid orphan cells in the widget value store, so we stash
|
||||
* the replay values here and drain them in `onAdded()` once the node has a
|
||||
* real id. This preserves per-instance promoted widget values across
|
||||
* `LGraphNode.clone()` (Ctrl+C/V), which configures the cloned node before
|
||||
* adding it to the graph.
|
||||
*/
|
||||
/** Widgets_values buffered during pre-attach configure(); drained in onAdded(). */
|
||||
private _pendingWidgetsValuesReplay?: TWidgetValue[]
|
||||
private _cacheVersion = 0
|
||||
private _linkedEntriesCache?: {
|
||||
@@ -1685,7 +1677,7 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph {
|
||||
merged[idx] =
|
||||
value != null && typeof value === 'object'
|
||||
? (structuredClone(toRaw(value)) as TWidgetValue)
|
||||
: (value ?? undefined)
|
||||
: (value as TWidgetValue)
|
||||
})
|
||||
if (merged.length > 0) serialized.widgets_values = merged
|
||||
|
||||
|
||||
@@ -14,6 +14,7 @@ import { usePromotionStore } from '@/stores/promotionStore'
|
||||
import { useExecutionErrorStore } from '@/stores/executionErrorStore'
|
||||
import { useMissingModelStore } from '@/platform/missingModel/missingModelStore'
|
||||
import { useWidgetValueStore } from '@/stores/widgetValueStore'
|
||||
import { makeCompositeKey } from '@/utils/compositeKey'
|
||||
|
||||
vi.mock('@/renderer/core/canvas/canvasStore', () => ({
|
||||
useCanvasStore: () => ({
|
||||
@@ -411,7 +412,11 @@ describe('per-instance value lookup for promoted widgets', () => {
|
||||
const GRAPH_ID = 'graph-test'
|
||||
const INTERIOR_NODE_ID = '5'
|
||||
const INTERIOR_WIDGET_NAME = 'text'
|
||||
const STORE_NAME = `${INTERIOR_NODE_ID}\u0001${INTERIOR_WIDGET_NAME}\u0001`
|
||||
const STORE_NAME = makeCompositeKey([
|
||||
INTERIOR_NODE_ID,
|
||||
INTERIOR_WIDGET_NAME,
|
||||
''
|
||||
])
|
||||
|
||||
beforeEach(() => {
|
||||
setActivePinia(createTestingPinia({ stubActions: false }))
|
||||
|
||||
@@ -60,14 +60,12 @@ export const useWidgetValueStore = defineStore('widgetValue', () => {
|
||||
return widgetStates.get(key) as WidgetState<TValue>
|
||||
}
|
||||
|
||||
function getOrRegister<TValue = unknown>(
|
||||
graphId: UUID,
|
||||
state: WidgetState<TValue>
|
||||
): WidgetState<TValue> {
|
||||
/** First registration wins; later `state` seeds are discarded. */
|
||||
function getOrRegister(graphId: UUID, state: WidgetState): WidgetState {
|
||||
const widgetStates = getWidgetStateMap(graphId)
|
||||
const key = makeKey(state.nodeId, state.name)
|
||||
const existing = widgetStates.get(key)
|
||||
if (existing) return existing as WidgetState<TValue>
|
||||
if (existing) return existing
|
||||
widgetStates.set(key, state)
|
||||
return state
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user