mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-07-11 01:28:03 +00:00
## Summary Removes the deprecated Group Nodes feature and replaces it with a load-time migration that auto-converts any group nodes in a loaded workflow into Subgraphs (with accepted lossiness). ## Changes - **What**: - `groupNode.ts` is now a migration-only extension. `beforeConfigureGraph` registers temporary node types from `extra.groupNodes` so instances are created during `configure`; a new `afterConfigureGraph` hook converts every group node in the root graph to a subgraph (via `LGraph.convertToSubgraph`), re-scanning until none remain, then deletes `extra.groupNodes`. A failed conversion removes the offending node so loading never hangs or breaks. - Kept the minimum needed: `GroupNodeConfig` (builds the input/output/widget maps), a slimmed `GroupNodeHandler` exposing a rewritten `convertToNodes()` that no longer depends on the execution DTOs, the `globalDefs`/`addCustomNodeDefs` path, and the `nodeDefStore` `Object.assign` shim the migration relies on to detect group nodes. - Deleted: the Manage Group Nodes dialog (`groupNodeManage.ts`/`.css`), execution DTOs (`executableGroupNodeDto.ts`, `executableGroupNodeChildDTO.ts`), the create/builder flow, recreate, commands, keybindings, menus, the `isGroupNode` branches in the right-side panel / error grouping / focus composable, the group-node branches in node templates, dead i18n keys, and the now-unused `serialise` clipboard helper. - Rewrote `browser_tests/tests/groupNode.spec.ts` to assert auto-conversion; deleted the `ManageGroupNode` page object and `manageGroupNode()` helper. - Net: ~2,700 lines removed across 23 files (7 files deleted). - **Breaking**: Group nodes can no longer be created, managed, or executed. Existing workflows still load — their group nodes are converted to subgraphs on open. ## Review Focus - The load-time migration in `afterConfigureGraph` and the rewritten `GroupNodeHandler.convertToNodes()` (no longer uses the execution `getInnerNodes()` / DTOs; derives inner node type/index from `groupData.nodeData.nodes` and relies on `deserialiseAndCreate` + selection ordering). - Kept `nodeDefStore`'s `Object.assign(this, obj)` shim: the migration depends on it to propagate the group-node marker symbol onto the registered node definition. ### Accepted lossiness - Group nodes nested inside subgraphs (or inside other group nodes) convert into the root graph rather than their original container — essentially nonexistent in real legacy workflows since group nodes predate subgraphs. - Temporary `workflow>name` node types stay registered for the session; instantiating one auto-converts it to a subgraph. ## Verification `pnpm typecheck`, `typecheck:browser`, `knip`, `oxlint`, `eslint`, and `oxfmt` are green (also enforced by pre-commit hooks). Unit tests for the touched files could not be run locally due to a pre-existing environment error (`file:///assets/images/*.svg` passed to a Node filename API at import time, which also fails on unmodified test files); the browser spec requires a live server. --------- Co-authored-by: Amp <amp@ampcode.com> Co-authored-by: github-actions <github-actions@github.com> Co-authored-by: AustinMroz <austin@comfy.org> Co-authored-by: GitHub Action <action@github.com>
185 lines
4.9 KiB
TypeScript
185 lines
4.9 KiB
TypeScript
import { fromAny } from '@total-typescript/shoehorn'
|
|
import { createPinia, setActivePinia } from 'pinia'
|
|
import { nextTick, ref } from 'vue'
|
|
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
|
|
|
import type { MissingNodeType } from '@/types/comfy'
|
|
|
|
vi.mock('@/scripts/app', () => ({
|
|
app: {
|
|
rootGraph: {
|
|
serialize: vi.fn(() => ({})),
|
|
getNodeById: vi.fn()
|
|
}
|
|
}
|
|
}))
|
|
|
|
vi.mock('@/utils/graphTraversalUtil', () => ({
|
|
getNodeByExecutionId: vi.fn(),
|
|
getExecutionIdByNode: vi.fn(),
|
|
getRootParentNode: vi.fn(() => null),
|
|
forEachNode: vi.fn(),
|
|
mapAllNodes: vi.fn(() => [])
|
|
}))
|
|
|
|
vi.mock('@/platform/distribution/types', () => ({
|
|
isCloud: false
|
|
}))
|
|
|
|
vi.mock('@/i18n', () => ({
|
|
te: vi.fn(() => false),
|
|
t: vi.fn((key: string) => key),
|
|
st: vi.fn((_key: string, fallback: string) => fallback)
|
|
}))
|
|
|
|
vi.mock('@/stores/comfyRegistryStore', () => ({
|
|
useComfyRegistryStore: () => ({
|
|
inferPackFromNodeName: vi.fn()
|
|
})
|
|
}))
|
|
|
|
vi.mock('@/utils/nodeTitleUtil', () => ({
|
|
resolveNodeDisplayName: vi.fn(() => '')
|
|
}))
|
|
|
|
vi.mock('@/utils/litegraphUtil', () => ({
|
|
isLGraphNode: vi.fn(() => false)
|
|
}))
|
|
|
|
import { useMissingNodesErrorStore } from '@/platform/nodeReplacement/missingNodesErrorStore'
|
|
import { useErrorGroups } from './useErrorGroups'
|
|
|
|
function makeMissingNodeType(
|
|
type: string,
|
|
opts: {
|
|
nodeId?: string
|
|
isReplaceable?: boolean
|
|
replacement?: { new_node_id: string }
|
|
} = {}
|
|
): MissingNodeType {
|
|
return {
|
|
type,
|
|
nodeId: opts.nodeId ?? '1',
|
|
isReplaceable: opts.isReplaceable ?? false,
|
|
replacement: opts.replacement
|
|
? {
|
|
old_node_id: type,
|
|
new_node_id: opts.replacement.new_node_id,
|
|
old_widget_ids: null,
|
|
input_mapping: null,
|
|
output_mapping: null
|
|
}
|
|
: undefined
|
|
}
|
|
}
|
|
|
|
describe('swapNodeGroups computed', () => {
|
|
beforeEach(() => {
|
|
setActivePinia(createPinia())
|
|
})
|
|
|
|
function getSwapNodeGroups(nodeTypes: MissingNodeType[]) {
|
|
useMissingNodesErrorStore().surfaceMissingNodes(nodeTypes)
|
|
|
|
const searchQuery = ref('')
|
|
const { swapNodeGroups } = useErrorGroups(searchQuery)
|
|
return swapNodeGroups
|
|
}
|
|
|
|
it('returns empty array when no missing nodes', () => {
|
|
const swap = getSwapNodeGroups([])
|
|
expect(swap.value).toEqual([])
|
|
})
|
|
|
|
it('returns empty array when no nodes are replaceable', () => {
|
|
const swap = getSwapNodeGroups([
|
|
makeMissingNodeType('NodeA', { isReplaceable: false }),
|
|
makeMissingNodeType('NodeB', { isReplaceable: false })
|
|
])
|
|
expect(swap.value).toEqual([])
|
|
})
|
|
|
|
it('groups replaceable nodes by type', async () => {
|
|
const swap = getSwapNodeGroups([
|
|
makeMissingNodeType('OldNode', {
|
|
nodeId: '1',
|
|
isReplaceable: true,
|
|
replacement: { new_node_id: 'NewNode' }
|
|
}),
|
|
makeMissingNodeType('OldNode', {
|
|
nodeId: '2',
|
|
isReplaceable: true,
|
|
replacement: { new_node_id: 'NewNode' }
|
|
})
|
|
])
|
|
await nextTick()
|
|
expect(swap.value).toHaveLength(1)
|
|
expect(swap.value[0].type).toBe('OldNode')
|
|
expect(swap.value[0].newNodeId).toBe('NewNode')
|
|
expect(swap.value[0].nodeTypes).toHaveLength(2)
|
|
})
|
|
|
|
it('creates separate groups for different types', async () => {
|
|
const swap = getSwapNodeGroups([
|
|
makeMissingNodeType('TypeA', {
|
|
nodeId: '1',
|
|
isReplaceable: true,
|
|
replacement: { new_node_id: 'NewA' }
|
|
}),
|
|
makeMissingNodeType('TypeB', {
|
|
nodeId: '2',
|
|
isReplaceable: true,
|
|
replacement: { new_node_id: 'NewB' }
|
|
})
|
|
])
|
|
await nextTick()
|
|
expect(swap.value).toHaveLength(2)
|
|
expect(swap.value.map((g) => g.type)).toEqual(['TypeA', 'TypeB'])
|
|
})
|
|
|
|
it('sorts groups alphabetically by type', async () => {
|
|
const swap = getSwapNodeGroups([
|
|
makeMissingNodeType('Zebra', {
|
|
nodeId: '1',
|
|
isReplaceable: true,
|
|
replacement: { new_node_id: 'NewZ' }
|
|
}),
|
|
makeMissingNodeType('Alpha', {
|
|
nodeId: '2',
|
|
isReplaceable: true,
|
|
replacement: { new_node_id: 'NewA' }
|
|
})
|
|
])
|
|
await nextTick()
|
|
expect(swap.value[0].type).toBe('Alpha')
|
|
expect(swap.value[1].type).toBe('Zebra')
|
|
})
|
|
|
|
it('excludes string nodeType entries', async () => {
|
|
const swap = getSwapNodeGroups([
|
|
fromAny<MissingNodeType, unknown>('StringGroupNode'),
|
|
makeMissingNodeType('OldNode', {
|
|
nodeId: '1',
|
|
isReplaceable: true,
|
|
replacement: { new_node_id: 'NewNode' }
|
|
})
|
|
])
|
|
await nextTick()
|
|
expect(swap.value).toHaveLength(1)
|
|
expect(swap.value[0].type).toBe('OldNode')
|
|
})
|
|
|
|
it('sets newNodeId to undefined when replacement is missing', async () => {
|
|
const swap = getSwapNodeGroups([
|
|
makeMissingNodeType('OldNode', {
|
|
nodeId: '1',
|
|
isReplaceable: true
|
|
// no replacement
|
|
})
|
|
])
|
|
await nextTick()
|
|
expect(swap.value).toHaveLength(1)
|
|
expect(swap.value[0].newNodeId).toBeUndefined()
|
|
})
|
|
})
|