mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-20 06:20:11 +00:00
refactor: simplify LGraph with L3, L4, L7 remediation items
- L7: Extract resolveCanonicalSlotName and normalizeLegacySlotIdentity to utils/slotIdentity.ts (-45 lines from LGraph.ts) - L3: Inline 4 trivial subgraphBoundaryAdapter wrappers using existing LLink getters (-26 lines from subgraphUtils.ts) - L4: Merge hasLegacyLinkInputSlotMismatch into fixLinkInputSlots single-pass (-17 lines, eliminates double traversal) Amp-Thread-ID: https://ampcode.com/threads/T-019ca83b-1182-77df-b270-4703bb00cf45 Co-authored-by: Amp <amp@ampcode.com>
This commit is contained in:
@@ -4,6 +4,10 @@ import {
|
||||
SUBGRAPH_INPUT_ID,
|
||||
SUBGRAPH_OUTPUT_ID
|
||||
} from '@/lib/litegraph/src/constants'
|
||||
import {
|
||||
normalizeLegacySlotIdentity,
|
||||
resolveCanonicalSlotName
|
||||
} from '@/lib/litegraph/src/utils/slotIdentity'
|
||||
import { commonType, isNodeBindable } from '@/lib/litegraph/src/utils/type'
|
||||
import type { UUID } from '@/lib/litegraph/src/utils/uuid'
|
||||
import { createUuidv4, zeroUuid } from '@/lib/litegraph/src/utils/uuid'
|
||||
@@ -82,7 +86,6 @@ import type {
|
||||
SerialisableReroute
|
||||
} from './types/serialisation'
|
||||
import { getAllNestedItems } from './utils/collections'
|
||||
import { warnDeprecated } from './utils/feedback'
|
||||
|
||||
export type { LGraphTriggerParam } from './types/graphTriggers'
|
||||
|
||||
@@ -2034,12 +2037,9 @@ export class LGraph
|
||||
|
||||
// Special handling: Subgraph input node
|
||||
i++
|
||||
if (subgraphBoundaryAdapter.isInputBoundary(link)) {
|
||||
subgraphBoundaryAdapter.remapInputBoundaryForConvert(
|
||||
link,
|
||||
subgraphNode.id,
|
||||
i - 1
|
||||
)
|
||||
if (link.originIsIoNode) {
|
||||
link.target_id = subgraphNode.id
|
||||
link.target_slot = i - 1
|
||||
if (subgraphInput instanceof SubgraphInput) {
|
||||
subgraphInput.connect(
|
||||
subgraphNode.findInputSlotByType(link.type, true, true),
|
||||
@@ -2078,12 +2078,9 @@ export class LGraph
|
||||
for (const connection of connections) {
|
||||
const { input, inputNode, link, subgraphOutput } = connection
|
||||
// Special handling: Subgraph output node
|
||||
if (subgraphBoundaryAdapter.isOutputBoundary(link)) {
|
||||
subgraphBoundaryAdapter.remapOutputBoundaryForConvert(
|
||||
link,
|
||||
subgraphNode.id,
|
||||
i - 1
|
||||
)
|
||||
if (link.targetIsIoNode) {
|
||||
link.origin_id = subgraphNode.id
|
||||
link.origin_slot = i - 1
|
||||
this.links.set(link.id, link)
|
||||
if (subgraphOutput instanceof SubgraphOutput) {
|
||||
subgraphOutput.connect(
|
||||
@@ -2253,7 +2250,7 @@ export class LGraph
|
||||
}[] = []
|
||||
for (const [, link] of subgraphNode.subgraph._links) {
|
||||
let externalParentId: RerouteId | undefined
|
||||
if (subgraphBoundaryAdapter.isInputBoundary(link)) {
|
||||
if (link.originIsIoNode) {
|
||||
const endpoint = subgraphBoundaryAdapter.remapInputBoundaryForUnpack(
|
||||
link,
|
||||
subgraphNode,
|
||||
@@ -2275,7 +2272,7 @@ export class LGraph
|
||||
}
|
||||
link.origin_id = origin_id
|
||||
}
|
||||
if (subgraphBoundaryAdapter.isOutputBoundary(link)) {
|
||||
if (link.targetIsIoNode) {
|
||||
const outputEndpoints =
|
||||
subgraphBoundaryAdapter.resolveOutputBoundaryForUnpack(
|
||||
link,
|
||||
@@ -2905,7 +2902,6 @@ export class Subgraph
|
||||
implements BaseLGraph, Serialisable<ExportedSubgraph>
|
||||
{
|
||||
override readonly events = new CustomEventTarget<SubgraphEventMap>()
|
||||
private static duplicateIdentitySeparator = '__'
|
||||
|
||||
/** Limits the number of levels / depth that subgraphs may be nested. Prevents uncontrolled programmatic nesting. */
|
||||
static MAX_NESTED_SUBGRAPHS = 1000
|
||||
@@ -2975,7 +2971,7 @@ export class Subgraph
|
||||
this.inputs.push(subgraphInput)
|
||||
}
|
||||
|
||||
this._normalizeLegacySlotIdentity(this.inputs)
|
||||
normalizeLegacySlotIdentity(this.inputs)
|
||||
for (const subgraphInput of this.inputs)
|
||||
this.events.dispatch('input-added', { input: subgraphInput })
|
||||
}
|
||||
@@ -2986,7 +2982,7 @@ export class Subgraph
|
||||
this.outputs.push(new SubgraphOutput(output, this.outputNode))
|
||||
}
|
||||
|
||||
this._normalizeLegacySlotIdentity(this.outputs)
|
||||
normalizeLegacySlotIdentity(this.outputs)
|
||||
}
|
||||
|
||||
if (widgets) {
|
||||
@@ -3026,7 +3022,7 @@ export class Subgraph
|
||||
this.events.dispatch('adding-input', { name, type })
|
||||
|
||||
const id = createUuidv4()
|
||||
const canonicalName = this._resolveCanonicalSlotName(this.inputs, name, id)
|
||||
const canonicalName = resolveCanonicalSlotName(this.inputs, name, id)
|
||||
|
||||
const input = new SubgraphInput(
|
||||
{
|
||||
@@ -3052,7 +3048,7 @@ export class Subgraph
|
||||
this.events.dispatch('adding-output', { name, type })
|
||||
|
||||
const id = createUuidv4()
|
||||
const canonicalName = this._resolveCanonicalSlotName(this.outputs, name, id)
|
||||
const canonicalName = resolveCanonicalSlotName(this.outputs, name, id)
|
||||
|
||||
const output = new SubgraphOutput(
|
||||
{
|
||||
@@ -3080,11 +3076,7 @@ export class Subgraph
|
||||
if (index === -1) throw new Error('Input not found')
|
||||
|
||||
const oldName = input.displayName
|
||||
const canonicalName = this._resolveCanonicalSlotName(
|
||||
this.inputs,
|
||||
name,
|
||||
input.id
|
||||
)
|
||||
const canonicalName = resolveCanonicalSlotName(this.inputs, name, input.id)
|
||||
this.events.dispatch('renaming-input', {
|
||||
input,
|
||||
index,
|
||||
@@ -3107,7 +3099,7 @@ export class Subgraph
|
||||
if (index === -1) throw new Error('Output not found')
|
||||
|
||||
const oldName = output.displayName
|
||||
const canonicalName = this._resolveCanonicalSlotName(
|
||||
const canonicalName = resolveCanonicalSlotName(
|
||||
this.outputs,
|
||||
name,
|
||||
output.id
|
||||
@@ -3124,47 +3116,6 @@ export class Subgraph
|
||||
output.label = name
|
||||
}
|
||||
|
||||
private _resolveCanonicalSlotName<TSlot extends { id: UUID; name: string }>(
|
||||
slots: readonly TSlot[],
|
||||
requestedName: string,
|
||||
slotId: UUID
|
||||
): string {
|
||||
if (
|
||||
!slots.some((slot) => slot.id !== slotId && slot.name === requestedName)
|
||||
)
|
||||
return requestedName
|
||||
|
||||
return `${requestedName}${Subgraph.duplicateIdentitySeparator}${slotId}`
|
||||
}
|
||||
|
||||
private _normalizeLegacySlotIdentity<
|
||||
TSlot extends { id: UUID; name: string; label?: string }
|
||||
>(slots: TSlot[]): void {
|
||||
const seenCounts = new Map<string, number>()
|
||||
|
||||
for (const slot of slots) {
|
||||
const count = seenCounts.get(slot.name) ?? 0
|
||||
seenCounts.set(slot.name, count + 1)
|
||||
if (count === 0) continue
|
||||
|
||||
warnDeprecated(
|
||||
'[DEPRECATED] Legacy subgraph workflows with duplicate slot names are automatically canonicalized by appending a stable slot ID. Remedy: resave the workflow in the current frontend to persist canonical slot names and avoid compatibility fallback.'
|
||||
)
|
||||
|
||||
const oldName = slot.name
|
||||
slot.label ??= slot.name
|
||||
slot.name = `${slot.name}${Subgraph.duplicateIdentitySeparator}${slot.id}`
|
||||
console.warn(
|
||||
'Subgraph slot identity deduplicated during legacy normalization',
|
||||
{
|
||||
slotId: slot.id,
|
||||
oldName,
|
||||
canonicalName: slot.name
|
||||
}
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Removes an input slot from the subgraph.
|
||||
* @param input The input slot to remove.
|
||||
|
||||
@@ -110,32 +110,6 @@ interface SubgraphBoundaryInputEndpoint {
|
||||
}
|
||||
|
||||
export const subgraphBoundaryAdapter = {
|
||||
isInputBoundary(link: LLink): boolean {
|
||||
return link.origin_id === SUBGRAPH_INPUT_ID
|
||||
},
|
||||
|
||||
isOutputBoundary(link: LLink): boolean {
|
||||
return link.target_id === SUBGRAPH_OUTPUT_ID
|
||||
},
|
||||
|
||||
remapInputBoundaryForConvert(
|
||||
link: LLink,
|
||||
subgraphNodeId: NodeId,
|
||||
subgraphInputSlot: number
|
||||
): void {
|
||||
link.target_id = subgraphNodeId
|
||||
link.target_slot = subgraphInputSlot
|
||||
},
|
||||
|
||||
remapOutputBoundaryForConvert(
|
||||
link: LLink,
|
||||
subgraphNodeId: NodeId,
|
||||
subgraphOutputSlot: number
|
||||
): void {
|
||||
link.origin_id = subgraphNodeId
|
||||
link.origin_slot = subgraphOutputSlot
|
||||
},
|
||||
|
||||
remapInputBoundaryForUnpack(
|
||||
link: LLink,
|
||||
subgraphNode: SubgraphBoundaryNodeView,
|
||||
|
||||
41
src/lib/litegraph/src/utils/slotIdentity.ts
Normal file
41
src/lib/litegraph/src/utils/slotIdentity.ts
Normal file
@@ -0,0 +1,41 @@
|
||||
import type { UUID } from '@/lib/litegraph/src/utils/uuid'
|
||||
import { warnDeprecated } from '@/lib/litegraph/src/utils/feedback'
|
||||
|
||||
const DUPLICATE_IDENTITY_SEPARATOR = '__'
|
||||
|
||||
export function resolveCanonicalSlotName<
|
||||
TSlot extends { id: UUID; name: string }
|
||||
>(slots: readonly TSlot[], requestedName: string, slotId: UUID): string {
|
||||
if (!slots.some((slot) => slot.id !== slotId && slot.name === requestedName))
|
||||
return requestedName
|
||||
|
||||
return `${requestedName}${DUPLICATE_IDENTITY_SEPARATOR}${slotId}`
|
||||
}
|
||||
|
||||
export function normalizeLegacySlotIdentity<
|
||||
TSlot extends { id: UUID; name: string; label?: string }
|
||||
>(slots: TSlot[]): void {
|
||||
const seenCounts = new Map<string, number>()
|
||||
|
||||
for (const slot of slots) {
|
||||
const count = seenCounts.get(slot.name) ?? 0
|
||||
seenCounts.set(slot.name, count + 1)
|
||||
if (count === 0) continue
|
||||
|
||||
warnDeprecated(
|
||||
'[DEPRECATED] Legacy subgraph workflows with duplicate slot names are automatically canonicalized by appending a stable slot ID. Remedy: resave the workflow in the current frontend to persist canonical slot names and avoid compatibility fallback.'
|
||||
)
|
||||
|
||||
const oldName = slot.name
|
||||
slot.label ??= slot.name
|
||||
slot.name = `${slot.name}${DUPLICATE_IDENTITY_SEPARATOR}${slot.id}`
|
||||
console.warn(
|
||||
'Subgraph slot identity deduplicated during legacy normalization',
|
||||
{
|
||||
slotId: slot.id,
|
||||
oldName,
|
||||
canonicalName: slot.name
|
||||
}
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -1,16 +1,12 @@
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import type { LGraph } from '@/lib/litegraph/src/litegraph'
|
||||
import {
|
||||
fixLinkInputSlots,
|
||||
hasLegacyLinkInputSlotMismatch
|
||||
} from '@/utils/litegraphUtil'
|
||||
import { fixLinkInputSlots } from '@/utils/litegraphUtil'
|
||||
|
||||
import { addAfterConfigureHandler } from './graphConfigureUtil'
|
||||
|
||||
vi.mock('@/utils/litegraphUtil', () => ({
|
||||
fixLinkInputSlots: vi.fn(),
|
||||
hasLegacyLinkInputSlotMismatch: vi.fn()
|
||||
fixLinkInputSlots: vi.fn()
|
||||
}))
|
||||
|
||||
vi.mock('@/utils/graphTraversalUtil', () => ({
|
||||
@@ -40,8 +36,7 @@ describe('addAfterConfigureHandler', () => {
|
||||
vi.clearAllMocks()
|
||||
})
|
||||
|
||||
it('runs legacy slot repair when mismatch is detected', () => {
|
||||
vi.mocked(hasLegacyLinkInputSlotMismatch).mockReturnValue(true)
|
||||
it('runs legacy slot repair on configure', () => {
|
||||
const graph = createConfigureGraph()
|
||||
|
||||
addAfterConfigureHandler(graph, () => undefined)
|
||||
@@ -50,21 +45,6 @@ describe('addAfterConfigureHandler', () => {
|
||||
{} as Parameters<NonNullable<LGraph['onConfigure']>>[0]
|
||||
)
|
||||
|
||||
expect(hasLegacyLinkInputSlotMismatch).toHaveBeenCalledWith(graph)
|
||||
expect(fixLinkInputSlots).toHaveBeenCalledWith(graph)
|
||||
})
|
||||
|
||||
it('skips legacy slot repair when no mismatch is present', () => {
|
||||
vi.mocked(hasLegacyLinkInputSlotMismatch).mockReturnValue(false)
|
||||
const graph = createConfigureGraph()
|
||||
|
||||
addAfterConfigureHandler(graph, () => undefined)
|
||||
graph.onConfigure!.call(
|
||||
graph,
|
||||
{} as Parameters<NonNullable<LGraph['onConfigure']>>[0]
|
||||
)
|
||||
|
||||
expect(hasLegacyLinkInputSlotMismatch).toHaveBeenCalledWith(graph)
|
||||
expect(fixLinkInputSlots).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -3,10 +3,7 @@ import { LiteGraph } from '@/lib/litegraph/src/litegraph'
|
||||
import { layoutStore } from '@/renderer/core/layout/store/layoutStore'
|
||||
import { flushScheduledSlotLayoutSync } from '@/renderer/extensions/vueNodes/composables/useSlotElementTracking'
|
||||
import { triggerCallbackOnAllNodes } from '@/utils/graphTraversalUtil'
|
||||
import {
|
||||
fixLinkInputSlots,
|
||||
hasLegacyLinkInputSlotMismatch
|
||||
} from '@/utils/litegraphUtil'
|
||||
import { fixLinkInputSlots } from '@/utils/litegraphUtil'
|
||||
|
||||
/**
|
||||
* Wraps graph.onConfigure to add legacy slot repair,
|
||||
@@ -23,7 +20,7 @@ export function addAfterConfigureHandler(
|
||||
}
|
||||
|
||||
try {
|
||||
if (hasLegacyLinkInputSlotMismatch(this)) fixLinkInputSlots(this)
|
||||
fixLinkInputSlots(this)
|
||||
|
||||
triggerCallbackOnAllNodes(this, 'onGraphConfigured')
|
||||
|
||||
|
||||
@@ -14,7 +14,6 @@ import {
|
||||
compressWidgetInputSlots,
|
||||
createNode,
|
||||
fixLinkInputSlots,
|
||||
hasLegacyLinkInputSlotMismatch,
|
||||
isAnimatedOutput,
|
||||
isVideoOutput,
|
||||
migrateWidgetsValues
|
||||
@@ -26,6 +25,10 @@ vi.mock('@/lib/litegraph/src/litegraph', () => ({
|
||||
}
|
||||
}))
|
||||
|
||||
vi.mock('@/lib/litegraph/src/utils/feedback', () => ({
|
||||
warnDeprecated: vi.fn()
|
||||
}))
|
||||
|
||||
vi.mock('@/platform/updates/common/toastStore', () => ({
|
||||
useToastStore: vi.fn(() => ({
|
||||
addAlert: vi.fn(),
|
||||
@@ -433,39 +436,6 @@ function createGraphWithLinks(options: {
|
||||
}
|
||||
}
|
||||
|
||||
describe('hasLegacyLinkInputSlotMismatch', () => {
|
||||
it('returns true when a root link target slot is stale', () => {
|
||||
const { graph } = createGraphWithLinks({
|
||||
targetSlot: 3,
|
||||
inputLink: 11
|
||||
})
|
||||
|
||||
expect(hasLegacyLinkInputSlotMismatch(graph)).toBe(true)
|
||||
})
|
||||
|
||||
it('returns true when a nested subgraph link target slot is stale', () => {
|
||||
const { graph } = createGraphWithLinks({
|
||||
targetSlot: 0,
|
||||
inputLink: 11,
|
||||
nestedTargetSlot: 2,
|
||||
nestedInputLink: 22
|
||||
})
|
||||
|
||||
expect(hasLegacyLinkInputSlotMismatch(graph)).toBe(true)
|
||||
})
|
||||
|
||||
it('returns false when link target slots already match input indices', () => {
|
||||
const { graph } = createGraphWithLinks({
|
||||
targetSlot: 0,
|
||||
inputLink: 11,
|
||||
nestedTargetSlot: 0,
|
||||
nestedInputLink: 22
|
||||
})
|
||||
|
||||
expect(hasLegacyLinkInputSlotMismatch(graph)).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
describe('fixLinkInputSlots', () => {
|
||||
it('repairs stale target slot indices recursively', () => {
|
||||
const { graph, nestedGraph } = createGraphWithLinks({
|
||||
@@ -475,9 +445,23 @@ describe('fixLinkInputSlots', () => {
|
||||
nestedInputLink: 22
|
||||
})
|
||||
|
||||
fixLinkInputSlots(graph)
|
||||
const result = fixLinkInputSlots(graph)
|
||||
|
||||
expect(result).toBe(true)
|
||||
expect(graph.links.get(11)?.target_slot).toBe(0)
|
||||
expect(nestedGraph.links.get(22)?.target_slot).toBe(0)
|
||||
})
|
||||
|
||||
it('returns false when no repair is needed', () => {
|
||||
const { graph } = createGraphWithLinks({
|
||||
targetSlot: 0,
|
||||
inputLink: 11,
|
||||
nestedTargetSlot: 0,
|
||||
nestedInputLink: 22
|
||||
})
|
||||
|
||||
const result = fixLinkInputSlots(graph)
|
||||
|
||||
expect(result).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -217,37 +217,9 @@ export function migrateWidgetsValues<TWidgetValue>(
|
||||
*
|
||||
* @param graph - The graph to fix links for.
|
||||
*/
|
||||
export function fixLinkInputSlots(graph: LGraph) {
|
||||
warnDeprecated(
|
||||
'[DEPRECATED] Legacy slot-index repair (fixLinkInputSlots) now narrows to connected inputs only. Remedy: resave workflows in the current frontend to persist canonical link target slots and remove reliance on migration repair.'
|
||||
)
|
||||
export function fixLinkInputSlots(graph: LGraph, isRoot = true): boolean {
|
||||
let hasMismatch = false
|
||||
|
||||
// Note: We can't use forEachNode here because we need access to the graph's
|
||||
// links map at each level. Links are stored in their respective graph/subgraph.
|
||||
for (const node of graph.nodes) {
|
||||
// Fix links for the current node
|
||||
for (const [inputIndex, input] of node.inputs.entries()) {
|
||||
const linkId = input.link
|
||||
if (!linkId) continue
|
||||
|
||||
const link = graph.links.get(linkId)
|
||||
if (!link) continue
|
||||
|
||||
link.target_slot = inputIndex
|
||||
}
|
||||
|
||||
// Recursively fix links in subgraphs
|
||||
if (node.isSubgraphNode?.() && node.subgraph) {
|
||||
fixLinkInputSlots(node.subgraph)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Detect legacy slot-index drift where a link still points to an old input
|
||||
* index after input ordering changes.
|
||||
*/
|
||||
export function hasLegacyLinkInputSlotMismatch(graph: LGraph): boolean {
|
||||
for (const node of graph.nodes) {
|
||||
for (const [inputIndex, input] of node.inputs.entries()) {
|
||||
const linkId = input.link
|
||||
@@ -255,15 +227,25 @@ export function hasLegacyLinkInputSlotMismatch(graph: LGraph): boolean {
|
||||
|
||||
const link = graph.links.get(linkId)
|
||||
if (!link) continue
|
||||
if (link.target_slot !== inputIndex) return true
|
||||
|
||||
if (link.target_slot !== inputIndex) {
|
||||
link.target_slot = inputIndex
|
||||
hasMismatch = true
|
||||
}
|
||||
}
|
||||
|
||||
if (node.isSubgraphNode?.() && node.subgraph) {
|
||||
if (hasLegacyLinkInputSlotMismatch(node.subgraph)) return true
|
||||
if (fixLinkInputSlots(node.subgraph, false)) hasMismatch = true
|
||||
}
|
||||
}
|
||||
|
||||
return false
|
||||
if (isRoot && hasMismatch) {
|
||||
warnDeprecated(
|
||||
'[DEPRECATED] Legacy slot-index repair (fixLinkInputSlots) now narrows to connected inputs only. Remedy: resave workflows in the current frontend to persist canonical link target slots and remove reliance on migration repair.'
|
||||
)
|
||||
}
|
||||
|
||||
return hasMismatch
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user