Skip to content

Commit 708b97b

Browse files
christian-byrneclaudeAustinMroz
authored
fix(widgets): tolerate un-keyable widget ids in widget value store (#13773)
## Summary Custom-node widgets with empty/placeholder names no longer throw and blank the entire node's widget grid on 1.47+ (regression from the store-backed widget-state refactor #12617). ## Changes - **What**: `parseWidgetId` rejects ids whose name segment is empty by throwing. Custom nodes register client-side widgets with empty/placeholder names (spacer/header/preview/button), producing such ids. The throw aborted `BaseWidget.setNodeId` seeding (legacy renderer) and tripped the Vue `NodeWidgets` error boundary — blanking the node's whole widget grid (e.g. rgthree Power Lora Loader rendered no widgets; VHS Load Video / Video Combine lost their injected widgets). Guard `registerWidget`/`getWidget`/`deleteWidget` with `isWidgetId` so the store declines an un-keyable id instead of throwing. `registerWidget` now returns `undefined` for such ids, and `BaseWidget` keeps its correct local `_state` so the widget still functions. One un-storable widget no longer blanks its siblings. - **Breaking**: `useWidgetValueStore().registerWidget` return type is now `WidgetState | undefined` (only `BaseWidget` consumes the return; handled). ## Review Focus - @DrJKL @AustinMroz — this is in the #12617 widget-store path. Confirms the design that a widget which can't be keyed (`graphId:nodeId:` with empty name) is simply not store-backed and keeps its local value. - **Scope**: this fixes only the *missing-widget* facet (widgets that don't render). The *blank-value* facet (pythongosssss Show Text, Impact populated_text) is the separate idempotent-`registerWidget` collision (same root cause, #13073 class) and will follow in a second PR. - **Confirmation**: the empty-name trigger was identified from code + custom-node patterns; still needs a live rgthree/VHS repro on 1.47 to confirm end-to-end. Kept as draft pending that. Unit-level behavior is covered by new regression tests. QA source: staging 1.47 custom-node findings (rgthree / VHS / pythongosssss / Impact), all working on 1.45.21. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Austin Mroz <austin@comfy.org>
1 parent af68bb1 commit 708b97b

5 files changed

Lines changed: 167 additions & 12 deletions

File tree

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
import {
2+
comfyExpect as expect,
3+
comfyPageFixture as test
4+
} from '@e2e/fixtures/ComfyPage'
5+
import { TestIds } from '@e2e/fixtures/selectors'
6+
7+
const HOST_NODE_TYPE = 'KSampler'
8+
const HOST_NODE_TITLE = 'KSampler'
9+
10+
/**
11+
* Regression for #13773: a custom node that registers a client-side widget with
12+
* an empty/placeholder name produces an un-keyable widget id (empty name
13+
* segment). Before the fix, `parseWidgetId` threw on that id, which tripped the
14+
* Vue `NodeWidgets` error boundary and blanked the node's ENTIRE widget grid
15+
* (e.g. rgthree Power Lora Loader rendered no widgets). The un-storable widget
16+
* must not take down its schema-declared siblings.
17+
*/
18+
test.describe(
19+
'Empty-name custom widget',
20+
{ tag: ['@vue-nodes', '@widget'] },
21+
() => {
22+
test.beforeEach(async ({ comfyPage }) => {
23+
// Emulate a custom node registering an extra, un-keyable widget
24+
await comfyPage.nodeOps.clearGraph()
25+
await comfyPage.nodeOps.addNode(HOST_NODE_TITLE, undefined, {
26+
x: 400,
27+
y: 200
28+
})
29+
await comfyPage.page.evaluate(() =>
30+
graph!.nodes[0].addWidget('text', '', '', () => {})
31+
)
32+
})
33+
34+
test('renders schema widgets despite an un-keyable sibling', async ({
35+
comfyPage
36+
}) => {
37+
const node = comfyPage.vueNodes.getNodeByTitle(HOST_NODE_TITLE)
38+
await expect(node).toBeVisible()
39+
40+
// Precondition: the un-keyable empty-name widget really is on the node, so
41+
// this test exercises the regression rather than passing vacuously.
42+
await expect
43+
.poll(() =>
44+
comfyPage.page.evaluate((type) => {
45+
const host = window.app!.graph.nodes.find((n) => n.type === type)
46+
return host?.widgets?.some((w) => w.name === '') ?? false
47+
}, HOST_NODE_TYPE)
48+
)
49+
.toBe(true)
50+
51+
// The widget grid must render (v-else branch), not the error boundary.
52+
await expect(node.getByTestId(TestIds.widgets.container)).toBeVisible()
53+
await expect(node.locator('.node-error')).toHaveCount(0)
54+
55+
// Schema-declared siblings must still render and be interactable.
56+
await expect(node.getByLabel('seed', { exact: true })).toBeVisible()
57+
await expect(node.getByLabel('steps', { exact: true })).toBeVisible()
58+
await expect(node.getByLabel('cfg', { exact: true })).toBeVisible()
59+
})
60+
}
61+
)

src/lib/litegraph/src/widgets/BaseWidget.test.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,4 +237,19 @@ describe('BaseWidget store integration', () => {
237237
expect(state?.disabled).toBe(false)
238238
})
239239
})
240+
241+
describe('un-keyable widget id (empty name)', () => {
242+
it('keeps local state usable when registration is declined', () => {
243+
const widget = createTestWidget(node, { name: '', value: 55 })
244+
245+
expect(() => widget.setNodeId(toNodeId(1))).not.toThrow()
246+
expect(
247+
store.getWidget(widgetId(graph.id, toNodeId(1), ''))
248+
).toBeUndefined()
249+
250+
expect(widget.value).toBe(55)
251+
widget.value = 88
252+
expect(widget.value).toBe(88)
253+
})
254+
})
240255
})

src/lib/litegraph/src/widgets/BaseWidget.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,13 +147,14 @@ export abstract class BaseWidget<TWidget extends IBaseWidget = IBaseWidget>
147147
const graphId = this.node.graph?.rootGraph.id
148148
if (!graphId) return
149149

150-
this._state = useWidgetValueStore().registerWidget(
150+
const registered = useWidgetValueStore().registerWidget(
151151
widgetId(graphId, nodeId, this.name),
152152
{
153153
...this._state,
154154
value: this.value
155155
}
156156
)
157+
if (registered) this._state = registered
157158
}
158159

159160
constructor(widget: TWidget & { node: LGraphNode })

src/stores/widgetValueStore.test.ts

Lines changed: 72 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { beforeEach, describe, expect, it } from 'vitest'
55
import type { UUID } from '@/utils/uuid'
66
import { toNodeId } from '@/types/nodeId'
77
import { widgetId } from '@/types/widgetId'
8+
import type { WidgetId } from '@/types/widgetId'
89
import type { WidgetState } from '@/types/widgetState'
910

1011
import { useWidgetValueStore } from './widgetValueStore'
@@ -35,7 +36,7 @@ describe('useWidgetValueStore', () => {
3536

3637
it('widgetState.value can be read and written directly', () => {
3738
const store = useWidgetValueStore()
38-
const registered = store.registerWidget(seedA, state('number', 100))
39+
const registered = store.registerWidget(seedA, state('number', 100))!
3940
expect(registered.value).toBe(100)
4041

4142
registered.value = 200
@@ -79,7 +80,7 @@ describe('useWidgetValueStore', () => {
7980
describe('widget registration', () => {
8081
it('registers a widget with minimal properties', () => {
8182
const store = useWidgetValueStore()
82-
const registered = store.registerWidget(seedA, state('number', 12345))
83+
const registered = store.registerWidget(seedA, state('number', 12345))!
8384

8485
expect(registered.nodeId).toBe('node-1')
8586
expect(registered.name).toBe('seed')
@@ -96,21 +97,36 @@ describe('useWidgetValueStore', () => {
9697
const registered = store.registerWidget(
9798
seedA,
9899
state('number', 12345, { y: 42 })
99-
)
100+
)!
100101

101102
expect(registered.y).toBe(42)
102103
})
103104

104105
it('registerWidget is idempotent and does not overwrite existing state', () => {
105106
const store = useWidgetValueStore()
106-
const first = store.registerWidget(seedA, state('number', 11))
107+
const first = store.registerWidget(seedA, state('number', 11))!
107108
first.value = 99
108109

109-
const second = store.registerWidget(seedA, state('number', 11))
110+
const second = store.registerWidget(seedA, state('number', 11))!
110111
expect(second).toBe(first)
111112
expect(second.value).toBe(99)
112113
})
113114

115+
it('replaces a stale entry when the widget type changes', () => {
116+
const store = useWidgetValueStore()
117+
const first = store.registerWidget(seedA, state('number', 5))!
118+
first.value = 42
119+
120+
// After a subgraph convert the graphId:nodeId:name key can be reused by a
121+
// different widget. A type mismatch means it is not the same widget, so
122+
// the live type/value must win rather than the stale entry (BUG: a text
123+
// widget rendered as int until reload).
124+
const reconciled = store.registerWidget(seedA, state('string', 'hello'))!
125+
expect(reconciled.type).toBe('string')
126+
expect(reconciled.value).toBe('hello')
127+
expect(store.getWidget(seedA)?.type).toBe('string')
128+
})
129+
114130
it('registers a widget with all properties', () => {
115131
const store = useWidgetValueStore()
116132
const registered = store.registerWidget(
@@ -121,7 +137,7 @@ describe('useWidgetValueStore', () => {
121137
serialize: false,
122138
options: { multiline: true }
123139
})
124-
)
140+
)!
125141

126142
expect(registered.label).toBe('Prompt Text')
127143
expect(registered.disabled).toBe(true)
@@ -187,15 +203,15 @@ describe('useWidgetValueStore', () => {
187203
describe('direct property mutation', () => {
188204
it('disabled can be set directly via getWidget', () => {
189205
const store = useWidgetValueStore()
190-
const registered = store.registerWidget(seedA, state('number', 100))
206+
const registered = store.registerWidget(seedA, state('number', 100))!
191207

192208
registered.disabled = true
193209
expect(store.getWidget(seedA)?.disabled).toBe(true)
194210
})
195211

196212
it('label can be set directly via getWidget', () => {
197213
const store = useWidgetValueStore()
198-
const registered = store.registerWidget(seedA, state('number', 100))
214+
const registered = store.registerWidget(seedA, state('number', 100))!
199215

200216
registered.label = 'Random Seed'
201217
expect(store.getWidget(seedA)?.label).toBe('Random Seed')
@@ -226,4 +242,52 @@ describe('useWidgetValueStore', () => {
226242
expect(store.getWidget(seedB)?.value).toBe(2)
227243
})
228244
})
245+
246+
describe('un-keyable widget ids', () => {
247+
// A custom node can register a widget with an empty/malformed name (spacer,
248+
// header, preview, button). Such an id cannot be keyed; the store must
249+
// decline it rather than throw and blank every widget on the node.
250+
const malformedIds = [
251+
widgetId(graphA, toNodeId('node-1'), '') as WidgetId, // empty name
252+
'no-colons' as WidgetId,
253+
`${graphA}:node-1` as WidgetId, // missing name segment
254+
`${graphA}:node-1:seed:extra` as WidgetId, // extra segment
255+
`:node-1:seed` as WidgetId, // empty graphId
256+
`${graphA}::seed` as WidgetId // empty nodeId
257+
]
258+
259+
it('registerWidget declines every un-keyable id instead of throwing', () => {
260+
const store = useWidgetValueStore()
261+
for (const id of malformedIds) {
262+
expect(() =>
263+
store.registerWidget(id, state('button', null))
264+
).not.toThrow()
265+
expect(store.registerWidget(id, state('button', null))).toBeUndefined()
266+
}
267+
})
268+
269+
it('getWidget / setValue / deleteWidget tolerate un-keyable ids', () => {
270+
const store = useWidgetValueStore()
271+
for (const id of malformedIds) {
272+
expect(store.getWidget(id)).toBeUndefined()
273+
expect(store.setValue(id, 1)).toBe(false)
274+
expect(store.deleteWidget(id)).toBe(false)
275+
}
276+
})
277+
278+
it('declined operations never disturb a valid sibling on the same node', () => {
279+
const store = useWidgetValueStore()
280+
store.registerWidget(seedA, state('number', 7))
281+
282+
for (const id of malformedIds) {
283+
store.registerWidget(id, state('button', null))
284+
store.setValue(id, 999)
285+
store.deleteWidget(id)
286+
}
287+
288+
expect(store.getWidget(seedA)?.value).toBe(7)
289+
expect(store.setValue(seedA, 8)).toBe(true)
290+
expect(store.getWidget(seedA)?.value).toBe(8)
291+
})
292+
})
229293
})

src/stores/widgetValueStore.ts

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { reactive, ref } from 'vue'
44
import type { UUID } from '@/utils/uuid'
55
import { parseNodeId } from '@/types/nodeId'
66
import type { NodeId, SerializedNodeId } from '@/types/nodeId'
7-
import { parseWidgetId } from '@/types/widgetId'
7+
import { isWidgetId, parseWidgetId } from '@/types/widgetId'
88
import type { WidgetId } from '@/types/widgetId'
99
import type { WidgetState, WidgetStateInit } from '@/types/widgetState'
1010

@@ -27,9 +27,19 @@ export const useWidgetValueStore = defineStore('widgetValue', () => {
2727
function registerWidget<TValue = unknown>(
2828
widgetId: WidgetId,
2929
init: WidgetStateInit<TValue>
30-
): WidgetState<TValue> {
30+
): WidgetState<TValue> | undefined {
31+
if (!isWidgetId(widgetId)) {
32+
console.warn(
33+
'widgetValueStore.registerWidget: ignoring un-keyable widget id',
34+
widgetId
35+
)
36+
return undefined
37+
}
38+
3139
const existing = getWidget(widgetId)
32-
if (existing) return existing as WidgetState<TValue>
40+
if (existing && existing.type === init.type) {
41+
return existing as WidgetState<TValue>
42+
}
3343

3444
const { graphId, nodeId, name } = parseWidgetId(widgetId)
3545
const state: WidgetState<TValue> = {
@@ -44,6 +54,8 @@ export const useWidgetValueStore = defineStore('widgetValue', () => {
4454
}
4555

4656
function getWidget(widgetId: WidgetId): WidgetState | undefined {
57+
if (!isWidgetId(widgetId)) return undefined
58+
4759
const { graphId } = parseWidgetId(widgetId)
4860
return getGraphWidgetStates(graphId).get(widgetId)
4961
}
@@ -56,6 +68,8 @@ export const useWidgetValueStore = defineStore('widgetValue', () => {
5668
}
5769

5870
function deleteWidget(widgetId: WidgetId): boolean {
71+
if (!isWidgetId(widgetId)) return false
72+
5973
const { graphId } = parseWidgetId(widgetId)
6074
return getGraphWidgetStates(graphId).delete(widgetId)
6175
}

0 commit comments

Comments
 (0)