mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-27 00:14:55 +00:00
fix(subgraph): harden proxyWidgets round-trip (review follow-up)
Addresses jaeone94's review on #11559: - write-back during configure now uses `_serializeEntriesWithPendingValues` so legacy-entry normalization doesn't demote the freshly-loaded 4-tuple into an identity-only 2/3-tuple, which would drop per-instance values from `properties.proxyWidgets` until the next full `serialize()`. - `cloneWidgetValue` now uses `structuredClone` with a raw-reference fallback, so a malformed `{value}` blob (circular ref, throwing `toJSON`, etc.) in saved JSON can't crash the whole subgraph load. - `_serializeEntriesWithState` skips the `{value}` wrapper when the resolved value is `undefined`, preventing ghost `null` entries from poisoning the per-instance map and sibling-fallback capture. - `restorePerInstanceValue` early-returns on `undefined` for the same reason. - `set value` now shares `_seedInstanceState` with `restorePerInstanceValue` to prevent drift between the setter and load paths. - `promotionSchema.ts` exposes `getProxyWidgetInlineState` + typed tuple names so inline state is read without `as` casts. Union order is load-bearing and now documented. - Regression test now asserts the LOAD invariant (inner widget value survives stale `widgets_values` in the payload), not just the SAVE invariant.
This commit is contained in:
@@ -66,9 +66,19 @@ interface PromotedSourceWriteMeta {
|
||||
function cloneWidgetValue<TValue extends IBaseWidget['value']>(
|
||||
value: TValue
|
||||
): TValue {
|
||||
return value != null && typeof value === 'object'
|
||||
? (JSON.parse(JSON.stringify(value)) as TValue)
|
||||
: value
|
||||
if (value == null || typeof value !== 'object') return value
|
||||
try {
|
||||
return structuredClone(value) as TValue
|
||||
} catch {
|
||||
/**
|
||||
* Malformed blobs (circular references, values with throwing `toJSON`,
|
||||
* `BigInt` inside `JSON`-backed paths, etc.) used to crash the whole
|
||||
* configure when clone was unconditionally `JSON.parse(JSON.stringify)`.
|
||||
* Fall back to the raw reference so a bad saved `{value}` cannot abort
|
||||
* subgraph load.
|
||||
*/
|
||||
return value
|
||||
}
|
||||
}
|
||||
|
||||
function getPromotedSourceWriteMeta(
|
||||
@@ -219,16 +229,15 @@ class PromotedWidgetView implements IPromotedWidgetView {
|
||||
* direct-edit self-heal does not discard the restored value.
|
||||
*/
|
||||
restorePerInstanceValue(value: IBaseWidget['value']): void {
|
||||
const cloned = cloneWidgetValue(value)
|
||||
this.subgraphNode._instanceWidgetValues.set(this.instanceKey, cloned)
|
||||
setPromotedSourceWriteMeta(
|
||||
this.subgraphNode.rootGraph,
|
||||
this._sharedSourceKey,
|
||||
{
|
||||
value: cloneWidgetValue(value),
|
||||
writerInstanceId: String(this.subgraphNode.id)
|
||||
}
|
||||
)
|
||||
/**
|
||||
* Guard against ghost entries: a save path that stores `undefined` (or
|
||||
* round-trips it through `raw ?? null`) must not poison
|
||||
* `_instanceWidgetValues` with an entry that `captureSiblingFallbackValues`
|
||||
* would then treat as "already written" and skip.
|
||||
*/
|
||||
if (value === undefined) return
|
||||
|
||||
this._seedInstanceState(value)
|
||||
/**
|
||||
* Align shared state with the restored value so `getTrackedValue`'s
|
||||
* direct-edit self-heal does not mistake the pre-existing inner value
|
||||
@@ -241,6 +250,21 @@ class PromotedWidgetView implements IPromotedWidgetView {
|
||||
this._writeValueToSharedState(value)
|
||||
}
|
||||
|
||||
private _seedInstanceState(value: IBaseWidget['value']): void {
|
||||
this.subgraphNode._instanceWidgetValues.set(
|
||||
this.instanceKey,
|
||||
cloneWidgetValue(value)
|
||||
)
|
||||
setPromotedSourceWriteMeta(
|
||||
this.subgraphNode.rootGraph,
|
||||
this._sharedSourceKey,
|
||||
{
|
||||
value: cloneWidgetValue(value),
|
||||
writerInstanceId: String(this.subgraphNode.id)
|
||||
}
|
||||
)
|
||||
}
|
||||
|
||||
private _writeValueToSharedState(value: IBaseWidget['value']): void {
|
||||
const linkedWidgets = this.getLinkedInputWidgets()
|
||||
if (linkedWidgets.length > 0) {
|
||||
@@ -305,63 +329,8 @@ class PromotedWidgetView implements IPromotedWidgetView {
|
||||
|
||||
set value(value: IBaseWidget['value']) {
|
||||
this.captureSiblingFallbackValues()
|
||||
|
||||
// Keep per-instance map in sync for execution (graphToPrompt)
|
||||
this.subgraphNode._instanceWidgetValues.set(
|
||||
this.instanceKey,
|
||||
cloneWidgetValue(value)
|
||||
)
|
||||
setPromotedSourceWriteMeta(
|
||||
this.subgraphNode.rootGraph,
|
||||
this._sharedSourceKey,
|
||||
{
|
||||
value: cloneWidgetValue(value),
|
||||
writerInstanceId: String(this.subgraphNode.id)
|
||||
}
|
||||
)
|
||||
|
||||
const linkedWidgets = this.getLinkedInputWidgets()
|
||||
if (linkedWidgets.length > 0) {
|
||||
const widgetStore = useWidgetValueStore()
|
||||
let didUpdateState = false
|
||||
for (const linkedWidget of linkedWidgets) {
|
||||
const state = widgetStore.getWidget(
|
||||
this.graphId,
|
||||
linkedWidget.nodeId,
|
||||
linkedWidget.widgetName
|
||||
)
|
||||
if (state) {
|
||||
state.value = value
|
||||
didUpdateState = true
|
||||
}
|
||||
}
|
||||
|
||||
const resolved = this.resolveDeepest()
|
||||
if (resolved) {
|
||||
const resolvedState = widgetStore.getWidget(
|
||||
this.graphId,
|
||||
stripGraphPrefix(String(resolved.node.id)),
|
||||
resolved.widget.name
|
||||
)
|
||||
if (resolvedState) {
|
||||
resolvedState.value = value
|
||||
didUpdateState = true
|
||||
}
|
||||
}
|
||||
|
||||
if (didUpdateState) return
|
||||
}
|
||||
|
||||
const state = this.getWidgetState()
|
||||
if (state) {
|
||||
state.value = value
|
||||
return
|
||||
}
|
||||
|
||||
const resolved = this.resolveAtHost()
|
||||
if (resolved && isWidgetValue(value)) {
|
||||
resolved.widget.value = value
|
||||
}
|
||||
this._seedInstanceState(value)
|
||||
this._writeValueToSharedState(value)
|
||||
}
|
||||
|
||||
get label(): string | undefined {
|
||||
|
||||
@@ -4,6 +4,12 @@ import { fromZodError } from 'zod-validation-error'
|
||||
import type { NodeProperty } from '@/lib/litegraph/src/LGraphNode'
|
||||
|
||||
const proxyWidgetStateSchema = z.object({ value: z.unknown() })
|
||||
/**
|
||||
* Order is load-bearing: `z.union` tries variants in declared order and the
|
||||
* first match wins. The 4-tuple (with optional trailing state) must come
|
||||
* before the 3- and 2-tuple variants, otherwise a 4-tuple would match the
|
||||
* 3-tuple form (dropping the trailing state object).
|
||||
*/
|
||||
const proxyWidgetTupleSchema = z.union([
|
||||
z.tuple([
|
||||
z.string(),
|
||||
@@ -15,7 +21,8 @@ const proxyWidgetTupleSchema = z.union([
|
||||
z.tuple([z.string(), z.string()])
|
||||
])
|
||||
const proxyWidgetsPropertySchema = z.array(proxyWidgetTupleSchema)
|
||||
type ProxyWidgetsProperty = z.infer<typeof proxyWidgetsPropertySchema>
|
||||
export type ProxyWidgetsProperty = z.infer<typeof proxyWidgetsPropertySchema>
|
||||
export type ProxyWidgetEntry = ProxyWidgetsProperty[number]
|
||||
|
||||
export function parseProxyWidgets(
|
||||
property: NodeProperty | undefined
|
||||
@@ -34,3 +41,16 @@ export function parseProxyWidgets(
|
||||
}
|
||||
return []
|
||||
}
|
||||
|
||||
/**
|
||||
* Typed accessor for the optional trailing `{ value }` state on a proxyWidgets
|
||||
* entry. Returns undefined for 2- and 3-tuple (identity-only) entries.
|
||||
*
|
||||
* Zod infers the state object as `{ value?: unknown }` because `z.unknown()`
|
||||
* treats undefined as valid input.
|
||||
*/
|
||||
export function getProxyWidgetInlineState(
|
||||
entry: ProxyWidgetEntry
|
||||
): { value?: unknown } | undefined {
|
||||
return entry.length === 4 ? entry[3] : undefined
|
||||
}
|
||||
|
||||
@@ -315,6 +315,10 @@ describe('SubgraphNode multi-instance widget isolation', () => {
|
||||
widgets_values: [999]
|
||||
})
|
||||
|
||||
// LOAD invariant: stale positional widgets_values must not leak into the
|
||||
// promoted view — it falls back to the inner widget's own value (42).
|
||||
expect(instance.widgets?.[0].value).toBe(42)
|
||||
// SAVE invariant: re-serialized output drops the dead widgets_values.
|
||||
expect(instance.serialize().widgets_values).toBeUndefined()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -44,7 +44,10 @@ import {
|
||||
CANVAS_IMAGE_PREVIEW_WIDGET,
|
||||
supportsVirtualCanvasImagePreview
|
||||
} from '@/composables/node/canvasImagePreviewTypes'
|
||||
import { parseProxyWidgets } from '@/core/schemas/promotionSchema'
|
||||
import {
|
||||
getProxyWidgetInlineState,
|
||||
parseProxyWidgets
|
||||
} from '@/core/schemas/promotionSchema'
|
||||
import { useDomWidgetStore } from '@/stores/domWidgetStore'
|
||||
import {
|
||||
makePromotionEntryKey,
|
||||
@@ -71,6 +74,21 @@ type LinkedPromotionEntry = PromotedWidgetSource & {
|
||||
// the SVG's internal stylesheet on every ctx.drawImage() call per frame.
|
||||
const workflowBitmapCache = createBitmapCache(workflowSvg, 32)
|
||||
|
||||
/**
|
||||
* Deep-clone a widget value for inlined serialization. Unlike raw
|
||||
* `JSON.parse(JSON.stringify(...))`, `structuredClone` handles `Date`,
|
||||
* `Map`/`Set`, and `BigInt` natively; the fallback preserves the original
|
||||
* reference for pathological values (circular refs, throwing `toJSON`)
|
||||
* rather than aborting the entire serialize/configure pass.
|
||||
*/
|
||||
function safeDeepClone<T>(value: T): T {
|
||||
try {
|
||||
return structuredClone(value)
|
||||
} catch {
|
||||
return value
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* An instance of a {@link Subgraph}, displayed as a node on the containing (parent) graph.
|
||||
*/
|
||||
@@ -645,16 +663,6 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph {
|
||||
: JSON.stringify([inputKey, sourceNodeId, sourceWidgetName, inputName])
|
||||
}
|
||||
|
||||
private _serializeEntries(
|
||||
entries: PromotedWidgetSource[]
|
||||
): (string[] | [string, string, string])[] {
|
||||
return entries.map((e) =>
|
||||
e.disambiguatingSourceNodeId
|
||||
? [e.sourceNodeId, e.sourceWidgetName, e.disambiguatingSourceNodeId]
|
||||
: [e.sourceNodeId, e.sourceWidgetName]
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Serialize promotion entries with their per-instance value inlined on
|
||||
* each entry as an optional trailing `{value}` state object.
|
||||
@@ -681,20 +689,27 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph {
|
||||
const key = makePromotionEntryKey(e)
|
||||
const view = viewByInstanceKey.get(key)
|
||||
const disambiguator = e.disambiguatingSourceNodeId ?? null
|
||||
|
||||
if (!view?.sourceSerialize) {
|
||||
return disambiguator
|
||||
const identityOnly: [string, string] | [string, string, string] =
|
||||
disambiguator
|
||||
? [e.sourceNodeId, e.sourceWidgetName, disambiguator]
|
||||
: [e.sourceNodeId, e.sourceWidgetName]
|
||||
}
|
||||
|
||||
if (!view?.sourceSerialize) return identityOnly
|
||||
|
||||
const raw = view.serializeValue
|
||||
? view.serializeValue(this, -1)
|
||||
: view.value
|
||||
/**
|
||||
* Downgrade to identity-only when the resolved value is `undefined`.
|
||||
* Without this, the `?? null` coercion below would persist `null` and
|
||||
* the reload path would seed `_instanceWidgetValues[key] = null` —
|
||||
* a ghost entry that `captureSiblingFallbackValues` treats as "already
|
||||
* written" and that `getTrackedValue` would return as a real value.
|
||||
*/
|
||||
if (raw === undefined) return identityOnly
|
||||
|
||||
const cloned =
|
||||
raw != null && typeof raw === 'object'
|
||||
? JSON.parse(JSON.stringify(raw))
|
||||
: (raw ?? null)
|
||||
raw !== null && typeof raw === 'object' ? safeDeepClone(raw) : raw
|
||||
|
||||
return [
|
||||
e.sourceNodeId,
|
||||
@@ -705,6 +720,38 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph {
|
||||
})
|
||||
}
|
||||
|
||||
/**
|
||||
* Write-back variant used during `configure`. Identity comes from the
|
||||
* resolved entries (normalization output), and values come from the
|
||||
* `pendingValues` map built in the same pass. Called before
|
||||
* `_resolveInputWidget` runs, so `_instanceWidgetValues` is still empty
|
||||
* and the view-backed serializer cannot be used here.
|
||||
*/
|
||||
private _serializeEntriesWithPendingValues(
|
||||
entries: PromotedWidgetSource[],
|
||||
pendingValues: Map<string, unknown>
|
||||
): (
|
||||
| [string, string]
|
||||
| [string, string, string]
|
||||
| [string, string, string | null, { value: unknown }]
|
||||
)[] {
|
||||
return entries.map((e) => {
|
||||
const disambiguator = e.disambiguatingSourceNodeId ?? null
|
||||
const key = makePromotionEntryKey(e)
|
||||
if (!pendingValues.has(key)) {
|
||||
return disambiguator
|
||||
? [e.sourceNodeId, e.sourceWidgetName, disambiguator]
|
||||
: [e.sourceNodeId, e.sourceWidgetName]
|
||||
}
|
||||
return [
|
||||
e.sourceNodeId,
|
||||
e.sourceWidgetName,
|
||||
disambiguator,
|
||||
{ value: pendingValues.get(key) }
|
||||
]
|
||||
})
|
||||
}
|
||||
|
||||
private _resolveLegacyEntry(
|
||||
widgetName: string
|
||||
): PromotedWidgetSource | undefined {
|
||||
@@ -1174,13 +1221,10 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph {
|
||||
)
|
||||
}
|
||||
|
||||
if (resolved && rawEntry.length >= 4) {
|
||||
const state = (rawEntry as unknown[])[3]
|
||||
if (state != null && typeof state === 'object' && 'value' in state) {
|
||||
pendingValues.set(
|
||||
makePromotionEntryKey(resolved),
|
||||
(state as { value: unknown }).value
|
||||
)
|
||||
if (resolved) {
|
||||
const state = getProxyWidgetInlineState(rawEntry)
|
||||
if (state) {
|
||||
pendingValues.set(makePromotionEntryKey(resolved), state.value)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1190,8 +1234,19 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph {
|
||||
|
||||
store.setPromotions(this.rootGraph.id, this.id, entries)
|
||||
|
||||
// Write back resolved entries so legacy or stale entries don't persist
|
||||
const serialized = this._serializeEntries(entries)
|
||||
/**
|
||||
* Write back resolved entries so legacy or stale identities don't
|
||||
* persist — but preserve the inline `{value}` state from `pendingValues`
|
||||
* alongside the normalized identity, otherwise this runs on every load
|
||||
* and demotes 4-tuple entries (which always differ byte-for-byte from
|
||||
* the pre-normalization `_serializeEntries` output) into 2/3-tuple,
|
||||
* stripping saved per-instance values from `properties.proxyWidgets`
|
||||
* until the next `serialize()` rebuilds them.
|
||||
*/
|
||||
const serialized = this._serializeEntriesWithPendingValues(
|
||||
entries,
|
||||
pendingValues
|
||||
)
|
||||
if (JSON.stringify(serialized) !== JSON.stringify(raw)) {
|
||||
this.properties.proxyWidgets = serialized
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user