mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-25 23:25:02 +00:00
fix(widgets): collapse duplicate COLOR widget rendering on Color to RGB Int (FE-842) (#12447)
## Summary Fix the duplicate \`<WidgetColorPicker>\` rendering on the \`Color to RGB Int\` node (and any other COLOR-using V3 node that the runtime double-registers a widget for). <img width="480" alt="after-fix-dedupe-proof" src="https://github.com/user-attachments/assets/5c801806-ed5d-493f-92b6-e0b99dd8e408" /> ## Changes - **What**: - \`useProcessedWidgets.getWidgetIdentity\`: fall back to the host \`nodeId\` parameter for the dedupe identity root when neither \`storeNodeId/widget.nodeId\` nor \`sourceExecutionId\` is set. Normal root-graph widgets now dedupe identically to promoted/execution-scoped widgets, so any duplicate same-name+same-type widget collapses to one render. \`sourceExecutionId\` precedence is preserved. - \`useColorWidget\`: read top-level \`default\` from the V2 spec (falls back to nested \`options.default\` for hand-authored V2 specs), and short-circuit if a same-name color widget already exists on \`node.widgets\` so a second \`addWidget('color', …)\` call from upstream hooks (or a \`configure\` round-trip) no longer duplicates the row. - **Tests**: - New \`useColorWidget.test.ts\` covers top-level default, nested-options fallback, no-default fallback, and the idempotency guard. - \`useProcessedWidgets.test.ts\` gets a regression case for two identical color widgets on the same node collapsing to one render, plus an updated \`getWidgetIdentity\` case for the host-nodeId fallback. ## Review Focus - \`getWidgetIdentity\` precedence change. The fallback only fires when none of \`storeNodeId\`, \`widget.nodeId\`, or \`sourceExecutionId\` are present, so promoted/exec-scoped widgets (incl. the \"unresolved same-name promoted entries distinct by source execution identity\" \`NodeWidgets\` test) are unaffected. - \`useColorWidget\` idempotency guard is defensive — the root cause of the second \`addWidget\` call (cloud-only hook or persisted \`info.widgets\` configure round-trip) is not in this diff; that's tracked separately. Fixes [FE-842](https://linear.app/comfyorg/issue/FE-842/color-to-rgb-int-node-shows-duplicate-color-widgets)
This commit is contained in:
committed by
github-actions[bot]
parent
325dc8ee15
commit
659bacdffd
@@ -12,19 +12,22 @@ test.describe('Vue Widget Reactivity', { tag: '@vue-nodes' }, () => {
|
||||
await comfyPage.page.evaluate(() => {
|
||||
const graph = window.graph as TestGraphAccess
|
||||
const node = graph._nodes_by_id['4']
|
||||
node.widgets!.push(node.widgets![0])
|
||||
node.widgets!.push({ ...node.widgets![0], name: 'added_widget_1' })
|
||||
})
|
||||
await expect(loadCheckpointNode).toHaveCount(2)
|
||||
await comfyPage.page.evaluate(() => {
|
||||
const graph = window.graph as TestGraphAccess
|
||||
const node = graph._nodes_by_id['4']
|
||||
node.widgets![2] = node.widgets![0]
|
||||
node.widgets![2] = { ...node.widgets![0], name: 'added_widget_2' }
|
||||
})
|
||||
await expect(loadCheckpointNode).toHaveCount(3)
|
||||
await comfyPage.page.evaluate(() => {
|
||||
const graph = window.graph as TestGraphAccess
|
||||
const node = graph._nodes_by_id['4']
|
||||
node.widgets!.splice(0, 0, node.widgets![0])
|
||||
node.widgets!.splice(0, 0, {
|
||||
...node.widgets![0],
|
||||
name: 'added_widget_3'
|
||||
})
|
||||
})
|
||||
await expect(loadCheckpointNode).toHaveCount(4)
|
||||
})
|
||||
|
||||
@@ -54,15 +54,30 @@ describe('getWidgetIdentity', () => {
|
||||
expect(renderKey).toBe(dedupeIdentity)
|
||||
})
|
||||
|
||||
it('returns transient renderKey for widgets without stable identity', () => {
|
||||
it('falls back to host nodeId so duplicate normal widgets dedupe', () => {
|
||||
const widget = createMockWidget({
|
||||
nodeId: undefined,
|
||||
storeNodeId: undefined,
|
||||
sourceExecutionId: undefined
|
||||
})
|
||||
const { dedupeIdentity, renderKey } = getWidgetIdentity(widget, '5', 3)
|
||||
expect(dedupeIdentity).toBe('node:5:test_widget:test_widget:combo')
|
||||
expect(renderKey).toBe(dedupeIdentity)
|
||||
})
|
||||
|
||||
it('returns transient renderKey when no nodeId is available at all', () => {
|
||||
const widget = createMockWidget({
|
||||
nodeId: undefined,
|
||||
storeNodeId: undefined,
|
||||
sourceExecutionId: undefined
|
||||
})
|
||||
const { dedupeIdentity, renderKey } = getWidgetIdentity(
|
||||
widget,
|
||||
undefined,
|
||||
3
|
||||
)
|
||||
expect(dedupeIdentity).toBeUndefined()
|
||||
expect(renderKey).toBe('transient:5:test_widget:test_widget:combo:3')
|
||||
expect(renderKey).toBe('transient::test_widget:test_widget:combo:3')
|
||||
})
|
||||
|
||||
it('uses sourceExecutionId for identity when no nodeId', () => {
|
||||
@@ -360,6 +375,46 @@ describe('computeProcessedWidgets borderStyle', () => {
|
||||
expect(result).toHaveLength(1)
|
||||
expect(result[0].hidden).toBe(false)
|
||||
})
|
||||
|
||||
it('collapses duplicate normal widgets on the same node to one render', () => {
|
||||
const colorA = createMockWidget({
|
||||
name: 'color',
|
||||
type: 'color',
|
||||
nodeId: undefined,
|
||||
storeNodeId: undefined,
|
||||
sourceExecutionId: undefined
|
||||
})
|
||||
const colorB = createMockWidget({
|
||||
name: 'color',
|
||||
type: 'color',
|
||||
nodeId: undefined,
|
||||
storeNodeId: undefined,
|
||||
sourceExecutionId: undefined
|
||||
})
|
||||
|
||||
const result = computeProcessedWidgets({
|
||||
nodeData: {
|
||||
id: '1',
|
||||
type: 'ColorToRGBInt',
|
||||
widgets: [colorA, colorB],
|
||||
title: 'Color to RGB Int',
|
||||
mode: 0,
|
||||
selected: false,
|
||||
executing: false,
|
||||
inputs: [],
|
||||
outputs: []
|
||||
},
|
||||
graphId: 'graph-test',
|
||||
showAdvanced: false,
|
||||
isGraphReady: false,
|
||||
rootGraph: null,
|
||||
ui: noopUi
|
||||
})
|
||||
|
||||
expect(result).toHaveLength(1)
|
||||
expect(result[0].name).toBe('color')
|
||||
expect(result[0].renderKey).toBe('node:1:color:color:color')
|
||||
})
|
||||
})
|
||||
|
||||
describe('createWidgetUpdateHandler (via computeProcessedWidgets)', () => {
|
||||
|
||||
@@ -129,11 +129,15 @@ export function getWidgetIdentity(
|
||||
const rawWidgetId = widget.storeNodeId ?? widget.nodeId
|
||||
const storeWidgetName = widget.storeName ?? widget.name
|
||||
const slotNameForIdentity = widget.slotName ?? widget.name
|
||||
const hostNodeIdRoot =
|
||||
nodeId !== undefined && nodeId !== ''
|
||||
? `node:${String(stripGraphPrefix(nodeId))}`
|
||||
: undefined
|
||||
const stableIdentityRoot = rawWidgetId
|
||||
? `node:${String(stripGraphPrefix(rawWidgetId))}`
|
||||
: widget.sourceExecutionId
|
||||
? `exec:${widget.sourceExecutionId}`
|
||||
: undefined
|
||||
: hostNodeIdRoot
|
||||
|
||||
const dedupeIdentity = stableIdentityRoot
|
||||
? `${stableIdentityRoot}:${storeWidgetName}:${slotNameForIdentity}:${widget.type}`
|
||||
|
||||
@@ -0,0 +1,76 @@
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import type { LGraphNode } from '@/lib/litegraph/src/litegraph'
|
||||
import type {
|
||||
IColorWidget,
|
||||
IWidgetOptions
|
||||
} from '@/lib/litegraph/src/types/widgets'
|
||||
import type { InputSpec } from '@/schemas/nodeDef/nodeDefSchemaV2'
|
||||
import { useColorWidget } from '@/renderer/extensions/vueNodes/widgets/composables/useColorWidget'
|
||||
|
||||
function createMockNode(): LGraphNode {
|
||||
const widgets: IColorWidget[] = []
|
||||
const addWidget = vi.fn(
|
||||
(
|
||||
type: string,
|
||||
name: string,
|
||||
value: string,
|
||||
_callback: () => void,
|
||||
options: IWidgetOptions
|
||||
) => {
|
||||
const widget = {
|
||||
type,
|
||||
name,
|
||||
value,
|
||||
options,
|
||||
callback: _callback
|
||||
} as unknown as IColorWidget
|
||||
widgets.push(widget)
|
||||
return widget
|
||||
}
|
||||
)
|
||||
|
||||
return { widgets, addWidget } as unknown as LGraphNode
|
||||
}
|
||||
|
||||
const colorSpec: InputSpec = {
|
||||
type: 'COLOR',
|
||||
name: 'color',
|
||||
default: '#ffffff',
|
||||
socketless: true
|
||||
}
|
||||
|
||||
describe('useColorWidget', () => {
|
||||
it('reads the top-level default from the V2 spec', () => {
|
||||
const node = createMockNode()
|
||||
const widget = useColorWidget()(node, colorSpec)
|
||||
expect(widget.value).toBe('#ffffff')
|
||||
})
|
||||
|
||||
it('falls back to nested options.default when top-level default is absent', () => {
|
||||
const node = createMockNode()
|
||||
const widget = useColorWidget()(node, {
|
||||
type: 'COLOR',
|
||||
name: 'color',
|
||||
options: { default: '#abcdef' }
|
||||
} as InputSpec)
|
||||
expect(widget.value).toBe('#abcdef')
|
||||
})
|
||||
|
||||
it('falls back to #000000 when no default is declared', () => {
|
||||
const node = createMockNode()
|
||||
const widget = useColorWidget()(node, {
|
||||
type: 'COLOR',
|
||||
name: 'color'
|
||||
} as InputSpec)
|
||||
expect(widget.value).toBe('#000000')
|
||||
})
|
||||
|
||||
it('returns the existing widget instead of creating a duplicate', () => {
|
||||
const node = createMockNode()
|
||||
const first = useColorWidget()(node, colorSpec)
|
||||
const second = useColorWidget()(node, colorSpec)
|
||||
expect(second).toBe(first)
|
||||
expect(node.widgets).toHaveLength(1)
|
||||
})
|
||||
})
|
||||
@@ -8,8 +8,14 @@ import type { ComfyWidgetConstructorV2 } from '@/scripts/widgets'
|
||||
|
||||
export const useColorWidget = (): ComfyWidgetConstructorV2 => {
|
||||
return (node: LGraphNode, inputSpec: InputSpecV2): IColorWidget => {
|
||||
const { name, options } = inputSpec as ColorInputSpec
|
||||
const defaultValue = options?.default || '#000000'
|
||||
const colorSpec = inputSpec as ColorInputSpec
|
||||
const { name, options } = colorSpec
|
||||
const defaultValue = colorSpec.default ?? options?.default ?? '#000000'
|
||||
|
||||
const existing = node.widgets?.find(
|
||||
(w): w is IColorWidget => w.name === name && w.type === 'color'
|
||||
)
|
||||
if (existing) return existing
|
||||
|
||||
const widget = node.addWidget('color', name, defaultValue, () => {}, {
|
||||
serialize: true
|
||||
|
||||
Reference in New Issue
Block a user