Comfy-Org / Comfy-Org/ComfyUI_frontend
ECS branch: five widget id/registration/order defects (stale metadata on re-register, mutating read in a computed, id collision, un-keyable ids in the order, no-op restore)
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 699
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 490
Description
Five defects in how widget ids are minted, registered and ordered on `feature/ecs-migration`. Filed as one issue because they share a root cause: the store key is minted once from mutable inputs, and the order list is not held to the same validity rule as the state map.
Verified at PR #14246 head `907ca2b1479381eff7a617e656c9002806b5fa15`.
## 1. `registerWidget` discards new metadata on re-registration
`stores/widgetValueStore.ts`:
```ts
const existing = getWidget(widgetId)
if (existing && existing.type === init.type) {
appendNodeWidgetOrder(widgetId)
return existing as WidgetState
}
```
Keyed on `type` alone, so re-registration silently drops the new `options`, `label`, `serialize` and `disabled`. `registerWidgetRenderState` directly below does `Object.assign(existing, init)` — the asymmetry looks unintended, and two callers depend on re-registration refreshing state:
- `SubgraphNode._setWidget` (`SubgraphNode.ts:672-683`) runs on every `input-connected` event and re-registers with the new interior widget's options and label. Reconnect a promoted input to a different interior source **of the same type** and the host keeps the first source's options. For a promoted combo that is a stale dropdown list.
- `promotionUtils.ts:301-307` is a workaround for exactly this: after going through the same path it writes `promotedState.label = sourceSlot.label` directly, because the `label` handed to `registerWidget` was dropped.
Related: `updateOptions` is added by this branch and has no production caller — only `widgetValueStore.test.ts` and `useProcessedWidgets.test.ts`.
## 2. `getNodeWidgetOrder` mutates the store on read, inside a Vue computed
```ts
function getNodeWidgetOrder(graphId: UUID, nodeId: NodeId): WidgetId[] {
const graphOrders = getGraphNodeWidgetOrders(graphId) // also creates
const order = graphOrders.get(nodeId)
if (order) return order
const nextOrder = reactive([])
graphOrders.set(nodeId, nextOrder) // insert on read
return nextOrder
}
```
Any `(graphId, nodeId)` pair that is merely queried gets a `reactive([])` inserted. `getNodeWidgetIds` and `getNodeWidgets` both go through it, and `getNodeWidgetIds` is reached from the `processedWidgets` computed via `computeProcessedWidgets` -> `resolveWidgetIds` (`useProcessedWidgets.ts:266`). Two effects:
- the computed reads `graphOrders.get(nodeId)` then `set(nodeId, ...)`, so its first evaluation for each node invalidates itself;
- `removeNodeWidgetOrder` deletes the key once the order empties, and the next read puts it straight back, so the delete never sticks and the map only shrinks via `clearGraph`.
A non-mutating read (`graphOrders.get(nodeId) ?? EMPTY`) with insertion left to `appendNodeWidgetOrder` fixes both.
## 3. `mapLiveWidgetsById` disambiguates on a different key than the minter
`utils/litegraphUtil.ts`:
```ts
const duplicateKey = `${widget.name}:${widget.type}`
```
while `getWidgetIdForNode` suffixes on **name only** (`name#N`). Two widgets sharing a name but not a type both compute index 0, produce the identical id, and the second clobbers the first in `byId`.
Separately, nothing mints `name#N` at all: `BaseWidget.widgetId` (`BaseWidget.ts:136-141`), `setNodeId`, `SubgraphNode._setWidget:669` and `promotionUtils.seedNestedPromotedInputState:332` all use the bare name. So any `#N` id is guaranteed absent from the store.
Either way `orderedIds.filter((id) => liveWidgets.has(id))` (`useProcessedWidgets.ts:496-498`) drops everything past the first same-named widget. At the merge base, `getWidgetIdentity` keyed `dedupeIdentity` on `${widgetId}:${type}`, so two same-name different-type widgets both rendered.
Keying the counter on `widget.name` fixes the collision half. The `#N` scheme itself needs either a minting counterpart or removal.
## 4. The order list admits ids `isWidgetId` rejects
`utils/widget.ts:32-37`, reached from `syncWidgetOrder` (`node/widgetsView.ts:30`):
```ts
.filter((id): id is WidgetId => id !== undefined)
```
`widget.widgetId` returns a string whenever the graph id and node id exist, including `::` for an empty widget name, which `WIDGET_ID_PATTERN` rejects because the name segment must be non-empty. `registerWidget` refuses that id and warns, but `replaceNodeWidgetOrder` puts it into the order anyway, and only `removeNodeWidgetOrder` ever takes it out. `getNodeWidgets` then drops it silently on every read and `reconcileNodeWidgetOrder` carries it forward.
Reachable: rgthree-comfy `base_node_mode_changer.js:34` does `addWidget('toggle', '', false, ...)`.
Filtering on `isWidgetId(id)` rather than `id !== undefined` keeps un-keyable ids out of the order.
## 5. `applySubgraphInputOrder`'s value-restore loop is a no-op
`core/graph/subgraph/promotionUtils.ts`, after `reorderSubgraphInputs`:
```ts
for (const [newIndex, oldIndex] of orderedIndices.entries()) {
const value = widgetValues[oldIndex]
const id = subgraphNode.inputs[newIndex]?.widgetId
...setValue(id, value)
}
```
`input.widgetId` is `widgetId(rootGraph.id, subgraphNode.id, name)`, minted from the input **name**. `reorderSubgraphInputs` -> `replaceNodeInputs` reorders the same input objects, so `inputs[newIndex]` is the object that was at `oldIndex` and still carries the same `widgetId`. Each iteration writes a widget's own value back to itself.
Harmless today, but it reads as protection against a positional remap that does not exist, and it would mask a real defect if these ids ever did become positional. If the intent was to guard the name-keying invariant, an assertion says that more clearly than a no-op write.
Raised originally as review threads on #14246; filed so the findings do not depend on those threads.
Related: #15600 (rename strands the key — the same minted-once root cause), #15630, #15632.
Contributor guide
Assessment
This issue has not been assessed yet.