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)

Open
#15,694 0 comments 0 reactions 1 assignee Claimed by @DrJKL View on GitHub
area:widgets Potential Bug
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.