From f707098f0514865de2b461c3c9ab7b2fc5fd84a5 Mon Sep 17 00:00:00 2001 From: Christian Byrne Date: Sat, 21 Feb 2026 22:38:05 -0800 Subject: [PATCH] fix: subgraph unpacking creates extra link to seed widget (#9046) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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) --- .../subgraphs/subgraph-duplicate-links.json | 183 ++++++++++++++++++ browser_tests/tests/subgraph.spec.ts | 39 ++++ src/lib/litegraph/src/LGraph.test.ts | 107 ++++++++++ src/lib/litegraph/src/LGraph.ts | 32 ++- 4 files changed, 353 insertions(+), 8 deletions(-) create mode 100644 browser_tests/assets/subgraphs/subgraph-duplicate-links.json diff --git a/browser_tests/assets/subgraphs/subgraph-duplicate-links.json b/browser_tests/assets/subgraphs/subgraph-duplicate-links.json new file mode 100644 index 000000000..8d57ef9a9 --- /dev/null +++ b/browser_tests/assets/subgraphs/subgraph-duplicate-links.json @@ -0,0 +1,183 @@ +{ + "id": "a1b2c3d4-e5f6-7890-abcd-ef1234567890", + "revision": 0, + "last_node_id": 2, + "last_link_id": 0, + "nodes": [ + { + "id": 2, + "type": "e5fb1765-aaaa-bbbb-cccc-ddddeeee0001", + "pos": [600, 400], + "size": [200, 100], + "flags": {}, + "order": 0, + "mode": 0, + "inputs": [], + "outputs": [ + { + "name": "LATENT", + "type": "LATENT", + "links": null + } + ], + "properties": {}, + "widgets_values": [] + } + ], + "links": [], + "groups": [], + "definitions": { + "subgraphs": [ + { + "id": "e5fb1765-aaaa-bbbb-cccc-ddddeeee0001", + "version": 1, + "state": { + "lastGroupId": 0, + "lastNodeId": 2, + "lastLinkId": 5, + "lastRerouteId": 0 + }, + "revision": 0, + "config": {}, + "name": "Subgraph With Duplicate Links", + "inputNode": { + "id": -10, + "bounding": [200, 400, 120, 60] + }, + "outputNode": { + "id": -20, + "bounding": [900, 400, 120, 60] + }, + "inputs": [], + "outputs": [ + { + "id": "out-latent-1", + "name": "LATENT", + "type": "LATENT", + "linkIds": [2], + "pos": [920, 420] + } + ], + "widgets": [], + "nodes": [ + { + "id": 1, + "type": "KSampler", + "pos": [400, 100], + "size": [270, 262], + "flags": {}, + "order": 1, + "mode": 0, + "inputs": [ + { + "name": "model", + "type": "MODEL", + "link": null + }, + { + "name": "positive", + "type": "CONDITIONING", + "link": null + }, + { + "name": "negative", + "type": "CONDITIONING", + "link": null + }, + { + "name": "latent_image", + "type": "LATENT", + "link": 1 + } + ], + "outputs": [ + { + "name": "LATENT", + "type": "LATENT", + "links": [2] + } + ], + "properties": { + "Node name for S&R": "KSampler" + }, + "widgets_values": [0, "randomize", 20, 8, "euler", "simple", 1] + }, + { + "id": 2, + "type": "EmptyLatentImage", + "pos": [100, 200], + "size": [200, 106], + "flags": {}, + "order": 0, + "mode": 0, + "inputs": [], + "outputs": [ + { + "name": "LATENT", + "type": "LATENT", + "links": [1, 3, 4, 5] + } + ], + "properties": { + "Node name for S&R": "EmptyLatentImage" + }, + "widgets_values": [512, 512, 1] + } + ], + "groups": [], + "links": [ + { + "id": 1, + "origin_id": 2, + "origin_slot": 0, + "target_id": 1, + "target_slot": 3, + "type": "LATENT" + }, + { + "id": 2, + "origin_id": 1, + "origin_slot": 0, + "target_id": -20, + "target_slot": 0, + "type": "LATENT" + }, + { + "id": 3, + "origin_id": 2, + "origin_slot": 0, + "target_id": 1, + "target_slot": 3, + "type": "LATENT" + }, + { + "id": 4, + "origin_id": 2, + "origin_slot": 0, + "target_id": 1, + "target_slot": 3, + "type": "LATENT" + }, + { + "id": 5, + "origin_id": 2, + "origin_slot": 0, + "target_id": 1, + "target_slot": 3, + "type": "LATENT" + } + ], + "extra": {} + } + ] + }, + "config": {}, + "extra": { + "ds": { + "scale": 1, + "offset": [0, 0] + }, + "frontendVersion": "1.38.14" + }, + "version": 0.4 +} diff --git a/browser_tests/tests/subgraph.spec.ts b/browser_tests/tests/subgraph.spec.ts index d8010c843..94f1fbad7 100644 --- a/browser_tests/tests/subgraph.spec.ts +++ b/browser_tests/tests/subgraph.spec.ts @@ -375,6 +375,45 @@ test.describe('Subgraph Operations', { tag: ['@slow', '@subgraph'] }, () => { }) }) + test.describe('Subgraph Unpacking', () => { + test('Unpacking subgraph with duplicate links does not create extra links', async ({ + comfyPage + }) => { + await comfyPage.workflow.loadWorkflow( + 'subgraphs/subgraph-duplicate-links' + ) + + const result = await comfyPage.page.evaluate(() => { + const graph = window.app!.graph! + const subgraphNode = graph.nodes.find((n) => n.isSubgraphNode()) + if (!subgraphNode || !subgraphNode.isSubgraphNode()) { + return { error: 'No subgraph node found' } + } + + graph.unpackSubgraph(subgraphNode) + + const linkCount = graph.links.size + const nodes = graph.nodes + const ksampler = nodes.find((n) => n.type === 'KSampler') + if (!ksampler) return { error: 'No KSampler found after unpack' } + + const linkedInputCount = ksampler.inputs.filter( + (i) => i.link != null + ).length + + return { linkCount, linkedInputCount, nodeCount: nodes.length } + }) + + expect(result).not.toHaveProperty('error') + // Should have exactly 1 link (EmptyLatentImage→KSampler) + // not 4 (with 3 duplicates). The KSampler→output link is dropped + // because the subgraph output has no downstream connection. + expect(result.linkCount).toBe(1) + // KSampler should have exactly 1 linked input (latent_image) + expect(result.linkedInputCount).toBe(1) + }) + }) + test.describe('Subgraph Creation and Deletion', () => { test('Can create subgraph from selected nodes', async ({ comfyPage }) => { await comfyPage.workflow.loadWorkflow('default') diff --git a/src/lib/litegraph/src/LGraph.test.ts b/src/lib/litegraph/src/LGraph.test.ts index c8ec80872..ef09d8c3e 100644 --- a/src/lib/litegraph/src/LGraph.test.ts +++ b/src/lib/litegraph/src/LGraph.test.ts @@ -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() + }) +}) diff --git a/src/lib/litegraph/src/LGraph.ts b/src/lib/litegraph/src/LGraph.ts index 93a8fee64..e823187f0 100644 --- a/src/lib/litegraph/src/LGraph.ts +++ b/src/lib/litegraph/src/LGraph.ts @@ -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() + 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() - 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