mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-02-24 00:34:09 +00:00
fix: subgraph unpacking creates extra link to seed widget (#9046)
## Summary Fix subgraph unpacking creating spurious links to widget inputs (e.g. seed) when the subgraph contains ComfySwitchNode with duplicate internal links. ## Changes - **What**: Two fixes in `_unpackSubgraphImpl`: 1. Strip links from serialized node data **before** `configure()` so `onConnectionsChange` doesn't resolve subgraph-internal link IDs against the parent graph's link map (which may contain unrelated links with colliding numeric IDs). 2. Deduplicate links by `(origin, origin_slot, target, target_slot)` before reconnecting, preventing repeated disconnect/reconnect cycles on widget inputs that cause slot index drift. ## Review Focus - The link-stripping before `configure()` mirrors what `LGraphNode.clone()` already does — nodes should be configured without stale link references when links will be recreated separately. - Deduplication is defensive against malformed subgraph data; the duplicate links in the reproduction workflow likely originated from a prior serialization bug. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9046-fix-subgraph-unpacking-creates-extra-link-to-seed-widget-30e6d73d36508125a5fefa1309485516) by [Unito](https://www.unito.io)
This commit is contained in:
@@ -484,3 +484,110 @@ describe('ensureGlobalIdUniqueness', () => {
|
||||
expect(subNode.id).toBe(subId)
|
||||
})
|
||||
})
|
||||
|
||||
describe('Subgraph Unpacking', () => {
|
||||
class TestNode extends LGraphNode {
|
||||
constructor(title?: string) {
|
||||
super(title ?? 'TestNode')
|
||||
this.addInput('input_0', 'number')
|
||||
this.addOutput('output_0', 'number')
|
||||
}
|
||||
}
|
||||
|
||||
class MultiInputNode extends LGraphNode {
|
||||
constructor(title?: string) {
|
||||
super(title ?? 'MultiInputNode')
|
||||
this.addInput('input_0', 'number')
|
||||
this.addInput('input_1', 'number')
|
||||
this.addOutput('output_0', 'number')
|
||||
}
|
||||
}
|
||||
|
||||
function registerTestNodes() {
|
||||
LiteGraph.registerNodeType('test/TestNode', TestNode)
|
||||
LiteGraph.registerNodeType('test/MultiInputNode', MultiInputNode)
|
||||
}
|
||||
|
||||
function createSubgraphOnGraph(rootGraph: LGraph) {
|
||||
return rootGraph.createSubgraph(createTestSubgraphData())
|
||||
}
|
||||
|
||||
it('deduplicates links when unpacking subgraph with duplicate links', () => {
|
||||
registerTestNodes()
|
||||
const rootGraph = new LGraph()
|
||||
const subgraph = createSubgraphOnGraph(rootGraph)
|
||||
|
||||
const sourceNode = LiteGraph.createNode('test/TestNode', 'Source')!
|
||||
const targetNode = LiteGraph.createNode('test/TestNode', 'Target')!
|
||||
subgraph.add(sourceNode)
|
||||
subgraph.add(targetNode)
|
||||
|
||||
// Create a legitimate link
|
||||
sourceNode.connect(0, targetNode, 0)
|
||||
expect(subgraph._links.size).toBe(1)
|
||||
|
||||
// Manually add duplicate links (simulating the bug)
|
||||
const existingLink = subgraph._links.values().next().value!
|
||||
for (let i = 0; i < 3; i++) {
|
||||
const dupLink = new LLink(
|
||||
++subgraph.state.lastLinkId,
|
||||
existingLink.type,
|
||||
existingLink.origin_id,
|
||||
existingLink.origin_slot,
|
||||
existingLink.target_id,
|
||||
existingLink.target_slot
|
||||
)
|
||||
subgraph._links.set(dupLink.id, dupLink)
|
||||
sourceNode.outputs[0].links!.push(dupLink.id)
|
||||
}
|
||||
expect(subgraph._links.size).toBe(4)
|
||||
|
||||
const subgraphNode = createTestSubgraphNode(subgraph, { pos: [100, 100] })
|
||||
rootGraph.add(subgraphNode)
|
||||
|
||||
rootGraph.unpackSubgraph(subgraphNode)
|
||||
|
||||
// After unpacking, there should be exactly 1 link (not 4)
|
||||
expect(rootGraph.links.size).toBe(1)
|
||||
})
|
||||
|
||||
it('preserves correct link connections when unpacking with duplicate links', () => {
|
||||
registerTestNodes()
|
||||
const rootGraph = new LGraph()
|
||||
const subgraph = createSubgraphOnGraph(rootGraph)
|
||||
|
||||
const sourceNode = LiteGraph.createNode('test/MultiInputNode', 'Source')!
|
||||
const targetNode = LiteGraph.createNode('test/MultiInputNode', 'Target')!
|
||||
subgraph.add(sourceNode)
|
||||
subgraph.add(targetNode)
|
||||
|
||||
// Connect source output 0 → target input 0
|
||||
sourceNode.connect(0, targetNode, 0)
|
||||
|
||||
// Add duplicate links to the same connection
|
||||
const existingLink = subgraph._links.values().next().value!
|
||||
const dupLink = new LLink(
|
||||
++subgraph.state.lastLinkId,
|
||||
existingLink.type,
|
||||
existingLink.origin_id,
|
||||
existingLink.origin_slot,
|
||||
existingLink.target_id,
|
||||
existingLink.target_slot
|
||||
)
|
||||
subgraph._links.set(dupLink.id, dupLink)
|
||||
sourceNode.outputs[0].links!.push(dupLink.id)
|
||||
|
||||
const subgraphNode = createTestSubgraphNode(subgraph, { pos: [100, 100] })
|
||||
rootGraph.add(subgraphNode)
|
||||
|
||||
rootGraph.unpackSubgraph(subgraphNode)
|
||||
|
||||
// Verify only 1 link exists
|
||||
expect(rootGraph.links.size).toBe(1)
|
||||
|
||||
// Verify target input 1 does NOT have a link (no spurious connection)
|
||||
const unpackedTarget = rootGraph.nodes.find((n) => n.title === 'Target')!
|
||||
expect(unpackedTarget.inputs[0].link).not.toBeNull()
|
||||
expect(unpackedTarget.inputs[1].link).toBeNull()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -1929,15 +1929,20 @@ export class LGraph
|
||||
node.id = this.last_node_id
|
||||
n_info.id = this.last_node_id
|
||||
|
||||
// Strip links from serialized data before configure to prevent
|
||||
// onConnectionsChange from resolving subgraph-internal link IDs
|
||||
// against the parent graph's link map (which may contain unrelated
|
||||
// links with the same numeric IDs).
|
||||
for (const input of n_info.inputs ?? []) {
|
||||
input.link = null
|
||||
}
|
||||
for (const output of n_info.outputs ?? []) {
|
||||
output.links = []
|
||||
}
|
||||
|
||||
this.add(node, true)
|
||||
node.configure(n_info)
|
||||
node.setPos(node.pos[0] + offsetX, node.pos[1] + offsetY)
|
||||
for (const input of node.inputs) {
|
||||
input.link = null
|
||||
}
|
||||
for (const output of node.outputs) {
|
||||
output.links = []
|
||||
}
|
||||
toSelect.push(node)
|
||||
}
|
||||
const groups = structuredClone(
|
||||
@@ -2043,8 +2048,19 @@ export class LGraph
|
||||
}
|
||||
this.remove(subgraphNode)
|
||||
this.subgraphs.delete(subgraphNode.subgraph.id)
|
||||
|
||||
// Deduplicate links by (oid, oslot, tid, tslot) to prevent repeated
|
||||
// disconnect/reconnect cycles on widget inputs that can shift slot indices.
|
||||
const seenLinks = new Set<string>()
|
||||
const dedupedNewLinks = newLinks.filter((link) => {
|
||||
const key = `${link.oid}:${link.oslot}:${link.tid}:${link.tslot}`
|
||||
if (seenLinks.has(key)) return false
|
||||
seenLinks.add(key)
|
||||
return true
|
||||
})
|
||||
|
||||
const linkIdMap = new Map<LinkId, LinkId[]>()
|
||||
for (const newLink of newLinks) {
|
||||
for (const newLink of dedupedNewLinks) {
|
||||
let created: LLink | null | undefined
|
||||
if (newLink.oid == SUBGRAPH_INPUT_ID) {
|
||||
if (!(this instanceof Subgraph)) {
|
||||
@@ -2102,7 +2118,7 @@ export class LGraph
|
||||
toSelect.push(migratedReroute)
|
||||
}
|
||||
//iterate over newly created links to update reroute parentIds
|
||||
for (const newLink of newLinks) {
|
||||
for (const newLink of dedupedNewLinks) {
|
||||
const linkInstance = this.links.get(newLink.id)
|
||||
if (!linkInstance) {
|
||||
continue
|
||||
|
||||
Reference in New Issue
Block a user