mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-02 04:02:20 +00:00
fix: extract and harden subgraph node ID deduplication (#9510)
## Summary Extract and harden subgraph node ID deduplication to prevent widget store key collisions when multiple subgraph copies share identical node IDs. ## Changes - **What**: Extract `deduplicateSubgraphNodeIds` from `LGraph.ts` into `utils/subgraphDeduplication.ts`, decomposed into focused helpers (`remapNodeIds`, `findNextAvailableId`, `patchSerialisedLinks`, `patchPromotedWidgets`, `patchProxyWidgets`). Clone inputs internally so caller data is never mutated. Add safety limit on ID search to prevent unbounded loops. Add `console.warn` on remapped IDs matching existing `ensureGlobalIdUniqueness` behavior. Add test fixture and 5 behavioral tests covering ID remapping, link patching, promoted widget patching, proxyWidget patching, and no-op when IDs are unique. ## Review Focus - The cloning strategy in `deduplicateSubgraphNodeIds` — it `structuredClone`s subgraphs and rootNodes, returning the clones. The caller uses `effectiveNodesData` to thread the patched root nodes through to node creation. - The `MAX_NODE_ID` safety limit (100M) — is this a reasonable ceiling? ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9510-fix-extract-and-harden-subgraph-node-ID-deduplication-31b6d73d365081f48c7de75e2bfc48b3) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <amp@ampcode.com>
This commit is contained in:
@@ -9,6 +9,7 @@ import {
|
||||
LiteGraph,
|
||||
LLink
|
||||
} from '@/lib/litegraph/src/litegraph'
|
||||
import type { SerialisableGraph } from '@/lib/litegraph/src/types/serialisation'
|
||||
import type { UUID } from '@/lib/litegraph/src/utils/uuid'
|
||||
import { usePromotionStore } from '@/stores/promotionStore'
|
||||
import { useWidgetValueStore } from '@/stores/widgetValueStore'
|
||||
@@ -17,6 +18,10 @@ import {
|
||||
createTestSubgraphNode
|
||||
} from './subgraph/__fixtures__/subgraphHelpers'
|
||||
|
||||
import { duplicateSubgraphNodeIds } from './__fixtures__/duplicateSubgraphNodeIds'
|
||||
import { nestedSubgraphProxyWidgets } from './__fixtures__/nestedSubgraphProxyWidgets'
|
||||
import { nodeIdSpaceExhausted } from './__fixtures__/nodeIdSpaceExhausted'
|
||||
import { uniqueSubgraphNodeIds } from './__fixtures__/uniqueSubgraphNodeIds'
|
||||
import { test } from './__fixtures__/testExtensions'
|
||||
|
||||
function swapNodes(nodes: LGraphNode[]) {
|
||||
@@ -656,3 +661,121 @@ describe('Subgraph Unpacking', () => {
|
||||
expect(definitionIds).toContain(subgraph.id)
|
||||
})
|
||||
})
|
||||
|
||||
describe('deduplicateSubgraphNodeIds (via configure)', () => {
|
||||
const SUBGRAPH_A = '11111111-1111-4111-8111-111111111111' as UUID
|
||||
const SUBGRAPH_B = '22222222-2222-4222-8222-222222222222' as UUID
|
||||
const SHARED_NODE_IDS = [3, 8, 37]
|
||||
|
||||
beforeEach(() => {
|
||||
setActivePinia(createTestingPinia({ stubActions: false }))
|
||||
LiteGraph.registerNodeType('dummy', DummyNode)
|
||||
})
|
||||
|
||||
function loadFixture(): SerialisableGraph {
|
||||
return structuredClone(duplicateSubgraphNodeIds)
|
||||
}
|
||||
|
||||
function configureFromFixture() {
|
||||
const graphData = loadFixture()
|
||||
const graph = new LGraph()
|
||||
graph.configure(graphData)
|
||||
return { graph, graphData }
|
||||
}
|
||||
|
||||
function nodeIdSet(graph: LGraph, subgraphId: UUID) {
|
||||
return new Set(graph.subgraphs.get(subgraphId)!.nodes.map((n) => n.id))
|
||||
}
|
||||
|
||||
it('remaps duplicate node IDs so subgraphs have no overlap', () => {
|
||||
const { graph } = configureFromFixture()
|
||||
|
||||
const idsA = nodeIdSet(graph, SUBGRAPH_A)
|
||||
const idsB = nodeIdSet(graph, SUBGRAPH_B)
|
||||
|
||||
for (const id of SHARED_NODE_IDS) {
|
||||
expect(idsA.has(id as NodeId)).toBe(true)
|
||||
}
|
||||
for (const id of idsA) {
|
||||
expect(idsB.has(id)).toBe(false)
|
||||
}
|
||||
})
|
||||
|
||||
it('patches link references in remapped subgraph', () => {
|
||||
const { graph } = configureFromFixture()
|
||||
const idsB = nodeIdSet(graph, SUBGRAPH_B)
|
||||
|
||||
for (const link of graph.subgraphs.get(SUBGRAPH_B)!.links.values()) {
|
||||
expect(idsB.has(link.origin_id)).toBe(true)
|
||||
expect(idsB.has(link.target_id)).toBe(true)
|
||||
}
|
||||
})
|
||||
|
||||
it('patches promoted widget references in remapped subgraph', () => {
|
||||
const { graph } = configureFromFixture()
|
||||
const idsB = nodeIdSet(graph, SUBGRAPH_B)
|
||||
|
||||
for (const widget of graph.subgraphs.get(SUBGRAPH_B)!.widgets) {
|
||||
expect(idsB.has(widget.id)).toBe(true)
|
||||
}
|
||||
})
|
||||
|
||||
it('patches proxyWidgets in root-level nodes referencing remapped IDs', () => {
|
||||
const { graph } = configureFromFixture()
|
||||
|
||||
const idsA = new Set(
|
||||
graph.subgraphs.get(SUBGRAPH_A)!.nodes.map((n) => String(n.id))
|
||||
)
|
||||
const idsB = new Set(
|
||||
graph.subgraphs.get(SUBGRAPH_B)!.nodes.map((n) => String(n.id))
|
||||
)
|
||||
|
||||
const pw102 = graph.getNodeById(102 as NodeId)?.properties?.proxyWidgets
|
||||
expect(Array.isArray(pw102)).toBe(true)
|
||||
for (const entry of pw102 as unknown[][]) {
|
||||
expect(Array.isArray(entry)).toBe(true)
|
||||
expect(idsA.has(String(entry[0]))).toBe(true)
|
||||
}
|
||||
|
||||
const pw103 = graph.getNodeById(103 as NodeId)?.properties?.proxyWidgets
|
||||
expect(Array.isArray(pw103)).toBe(true)
|
||||
for (const entry of pw103 as unknown[][]) {
|
||||
expect(Array.isArray(entry)).toBe(true)
|
||||
expect(idsB.has(String(entry[0]))).toBe(true)
|
||||
}
|
||||
})
|
||||
|
||||
it('patches proxyWidgets inside nested subgraph nodes', () => {
|
||||
const graph = new LGraph()
|
||||
graph.configure(structuredClone(nestedSubgraphProxyWidgets))
|
||||
|
||||
const idsB = new Set(
|
||||
graph.subgraphs.get(SUBGRAPH_B)!.nodes.map((n) => String(n.id))
|
||||
)
|
||||
|
||||
const innerNode = graph.subgraphs
|
||||
.get(SUBGRAPH_A)!
|
||||
.nodes.find((n) => n.id === (50 as NodeId))
|
||||
const pw = innerNode?.properties?.proxyWidgets
|
||||
expect(Array.isArray(pw)).toBe(true)
|
||||
for (const entry of pw as unknown[][]) {
|
||||
expect(Array.isArray(entry)).toBe(true)
|
||||
expect(idsB.has(String(entry[0]))).toBe(true)
|
||||
}
|
||||
})
|
||||
|
||||
it('throws when node ID space is exhausted', () => {
|
||||
expect(() => {
|
||||
const graph = new LGraph()
|
||||
graph.configure(structuredClone(nodeIdSpaceExhausted))
|
||||
}).toThrow('Node ID space exhausted')
|
||||
})
|
||||
|
||||
it('is a no-op when subgraph node IDs are already unique', () => {
|
||||
const graph = new LGraph()
|
||||
graph.configure(structuredClone(uniqueSubgraphNodeIds))
|
||||
|
||||
expect(nodeIdSet(graph, SUBGRAPH_A)).toEqual(new Set([10, 11, 12]))
|
||||
expect(nodeIdSet(graph, SUBGRAPH_B)).toEqual(new Set([20, 21, 22]))
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user