fix: use direct serialization instead of clone in _serializeItems

Per review feedback from @AustinMroz: replace clone().serialize() with
direct serialization for all nodes, clearing links manually. Avoids the
clone->serialize gap where transient nodes lose external state.
This commit is contained in:
bymyself
2026-03-17 08:52:43 +00:00
parent 6c0968cef7
commit 6c8b890f54
3 changed files with 30 additions and 28 deletions

View File

@@ -3902,25 +3902,21 @@ export class LGraphCanvas implements CustomEventDispatcher<LGraphCanvasEventMap>
// Nodes
if (item.clonable === false) continue
// SubgraphNodes must serialize the original instance directly,
// because clone() creates a transient node whose serialize() queries
// promotionStore with the wrong id → stale proxyWidgets. (#9976)
const cloned =
item instanceof SubgraphNode
? LiteGraph.cloneObject(item.serialize())
: item.clone()?.serialize()
// Serialize the original node directly instead of clone().serialize().
// clone() creates a transient node whose state can diverge from the
// original (e.g. SubgraphNode promotionStore keyed by wrong id,
// PrimitiveNode losing widgets_values). ID deduplication is already
// handled on the deserialize side by _deserializeItems. (#9976)
const cloned = LiteGraph.cloneObject(item.serialize())
if (!cloned) continue
// Clear links on the serialized copy (clone() does this internally;
// for the direct-serialize path we must do it explicitly).
if (item instanceof SubgraphNode) {
if (cloned.inputs) {
for (const input of cloned.inputs) input.link = null
}
if (cloned.outputs) {
for (const output of cloned.outputs) {
if (output.links) output.links.length = 0
}
// Clear links on the serialized copy (clone() used to do this).
if (cloned.inputs) {
for (const input of cloned.inputs) input.link = null
}
if (cloned.outputs) {
for (const output of cloned.outputs) {
if (output.links) output.links.length = 0
}
}

View File

@@ -3,9 +3,9 @@
*
* Verifies:
* 1. serialize() correctly captures instance-scoped promotion metadata
* 2. The _serializeItems copy path should not use clone() for SubgraphNodes
* (clone creates a transient node with wrong id, whose serialize() queries
* promotionStore with the wrong key → empty/stale proxyWidgets)
* 2. Direct serialization (without clone()) preserves correct state — the
* _serializeItems path uses item.serialize() for all nodes, avoiding the
* clone→serialize gap where transient nodes lose external state
* 3. Subgraph definition serialization preserves modified widget values
*
* @see https://github.com/Comfy-Org/ComfyUI_frontend/issues/9976
@@ -181,14 +181,12 @@ describe('SubgraphNode.serialize() state isolation (#9976)', () => {
expect(serializedNode?.widgets_values?.[0]).toBe(777)
})
it('_serializeItems should not use clone().serialize() for SubgraphNodes', () => {
it('direct serialize() preserves proxyWidgets and widget values', () => {
const { subgraph, interiorNode, subgraphNode } =
createSubgraphWithWidgetNode()
// The correct approach: serialize the ORIGINAL node directly
// Direct serialization captures correct proxyWidgets
const originalSerialized = subgraphNode.serialize()
// proxyWidgets should reflect the original node's promotions
expect(originalSerialized.properties?.proxyWidgets).toEqual([
[String(interiorNode.id), 'seed']
])

View File

@@ -926,6 +926,10 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph {
override _internalConfigureAfterSlots() {
this._rebindInputSubgraphSlots()
// Prune inputs that don't map to any subgraph slot definition.
// This prevents stale/duplicate serialized inputs from persisting (#9977).
this.inputs = this.inputs.filter((input) => input._subgraphSlot)
// Ensure proxyWidgets is initialized so it serializes
this.properties.proxyWidgets ??= []
@@ -1439,9 +1443,13 @@ export class SubgraphNode extends LGraphNode implements BaseLGraph {
return super.serialize()
}
override clone() {
// Note: _serializeItems bypasses clone() for SubgraphNodes to avoid
// promotionStore identity mismatch (#9976). This clone path is still
// used by alt+drag and other operations.
return super.clone()
const clone = super.clone()
//TODO: Consider deep cloning subgraphs here.
//It's the safest place to prevent creation of linked subgraphs
//But the frequency of clone().serialize() calls is likely to result in
//pollution of rootGraph.subgraphs
return clone
}
}