mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-19 13:59:28 +00:00
## Summary Fix workflow loading for nested subgraphs with duplicate node IDs by configuring subgraph definitions in topological (leaf-first) order. ## Changes - **What**: Three pre-existing bugs that surface when loading nested subgraphs with colliding node IDs: 1. Subgraph definitions configured in serialization order — a parent subgraph's `SubgraphNode.configure` would run before its referenced child subgraph was populated, causing link/widget resolution failures. 2. `_resolveLegacyEntry` returned `undefined` when `input._widget` wasn't set yet, instead of falling back to `resolveSubgraphInputTarget`. 3. `_removeDuplicateLinks` removed duplicate links without updating `SubgraphOutput.linkIds`, leaving stale references that broke prompt execution. - **What (housekeeping)**: Moved `subgraphDeduplication.ts` from `utils/` to `subgraph/` directory where it belongs. ## Review Focus - Topological sort correctness: Kahn's algorithm with edges from dependency→dependent ensures leaves configure first. Cycle fallback returns original order. - IO slot link repair: `_repairIOSlotLinkIds` runs after `_configureSubgraph` creates IO slots, patching any `linkIds` that point to links removed by `_removeDuplicateLinks` during `super.configure()`. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10314-fix-configure-nested-subgraph-definitions-in-dependency-order-3286d73d36508171b149e238b8de84c2) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <amp@ampcode.com>
111 lines
3.4 KiB
TypeScript
111 lines
3.4 KiB
TypeScript
import { expect } from '@playwright/test'
|
|
|
|
import { comfyPageFixture as test } from '../fixtures/ComfyPage'
|
|
|
|
test.describe('Nested subgraph configure order', { tag: ['@subgraph'] }, () => {
|
|
const WORKFLOW = 'subgraphs/subgraph-nested-duplicate-ids'
|
|
|
|
test('Loads without "No link found" or "Failed to resolve legacy -1" console warnings', async ({
|
|
comfyPage
|
|
}) => {
|
|
const warnings: string[] = []
|
|
comfyPage.page.on('console', (msg) => {
|
|
const text = msg.text()
|
|
if (
|
|
text.includes('No link found') ||
|
|
text.includes('Failed to resolve legacy -1') ||
|
|
text.includes('No inner link found')
|
|
) {
|
|
warnings.push(text)
|
|
}
|
|
})
|
|
|
|
await comfyPage.workflow.loadWorkflow(WORKFLOW)
|
|
|
|
expect(warnings).toEqual([])
|
|
})
|
|
|
|
test('All three subgraph levels resolve promoted widgets', async ({
|
|
comfyPage
|
|
}) => {
|
|
await comfyPage.workflow.loadWorkflow(WORKFLOW)
|
|
await comfyPage.nextFrame()
|
|
|
|
const results = await comfyPage.page.evaluate(() => {
|
|
const graph = window.app!.canvas.graph!
|
|
const allGraphs = [graph, ...graph.subgraphs.values()]
|
|
|
|
return allGraphs.flatMap((g) =>
|
|
g._nodes
|
|
.filter(
|
|
(n) => typeof n.isSubgraphNode === 'function' && n.isSubgraphNode()
|
|
)
|
|
.map((hostNode) => {
|
|
const proxyWidgets = Array.isArray(
|
|
hostNode.properties?.proxyWidgets
|
|
)
|
|
? hostNode.properties.proxyWidgets
|
|
: []
|
|
|
|
const widgetEntries = proxyWidgets
|
|
.filter(
|
|
(e: unknown): e is [string, string] =>
|
|
Array.isArray(e) &&
|
|
e.length >= 2 &&
|
|
typeof e[0] === 'string' &&
|
|
typeof e[1] === 'string'
|
|
)
|
|
.map(([interiorNodeId, widgetName]: [string, string]) => {
|
|
const sg = hostNode.isSubgraphNode() ? hostNode.subgraph : null
|
|
const interiorNode = sg?.getNodeById(Number(interiorNodeId))
|
|
return {
|
|
interiorNodeId,
|
|
widgetName,
|
|
resolved: interiorNode !== null && interiorNode !== undefined
|
|
}
|
|
})
|
|
|
|
return {
|
|
hostNodeId: String(hostNode.id),
|
|
widgetEntries
|
|
}
|
|
})
|
|
)
|
|
})
|
|
|
|
expect(
|
|
results.length,
|
|
'Should have subgraph host nodes at multiple nesting levels'
|
|
).toBeGreaterThanOrEqual(2)
|
|
|
|
for (const { hostNodeId, widgetEntries } of results) {
|
|
expect(
|
|
widgetEntries.length,
|
|
`Host node ${hostNodeId} should have promoted widgets`
|
|
).toBeGreaterThan(0)
|
|
|
|
for (const { interiorNodeId, widgetName, resolved } of widgetEntries) {
|
|
expect(interiorNodeId).not.toBe('-1')
|
|
expect(Number(interiorNodeId)).toBeGreaterThan(0)
|
|
expect(widgetName).toBeTruthy()
|
|
expect(
|
|
resolved,
|
|
`Widget "${widgetName}" (interior node ${interiorNodeId}) on host ${hostNodeId} should resolve`
|
|
).toBe(true)
|
|
}
|
|
}
|
|
})
|
|
|
|
test('Prompt execution succeeds without 400 error', async ({ comfyPage }) => {
|
|
await comfyPage.workflow.loadWorkflow(WORKFLOW)
|
|
await comfyPage.nextFrame()
|
|
|
|
const responsePromise = comfyPage.page.waitForResponse('**/api/prompt')
|
|
|
|
await comfyPage.command.executeCommand('Comfy.QueuePrompt')
|
|
|
|
const response = await responsePromise
|
|
expect(response.status()).not.toBe(400)
|
|
})
|
|
})
|