Comfy-Org / Comfy-Org/ComfyUI_frontend

ECS branch: node.widgets additions are dropped unless the widget is already store-registered, so getCustomWidgets widgets never render in Vue nodes

Open
#15,630 2 comments 1 reaction 1 assignee Claimed by @DrJKL View on GitHub
area:vue-migration area:widgets Potential Bug
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

`node.widgets` commits every mutation form, but the commit payload is filtered to widgets that already have `widgetValueStore` state. Removals therefore always land and additions land only if the widget is already registered — so a widget pushed straight onto the array never renders in Vue nodes.

Reviewed at `5002fae1b12d44831a21367afa7c0f798f7e7a2c` (PR #14246), merge base `6532665db947acb61ed044fe91a1d4fe1fb84c8b`.

## The mutation view itself is fine

`src/lib/litegraph/src/node/widgetsView.ts:35-51` over `createArrayMutationView`. All of these reach `syncWidgetOrder`:

| write | commits? | via |
| --- | --- | --- |
| `node.widgets = [...]` | yes | setter calls `state.view.splice(...)` (`widgetsView.ts:47`), and `splice` is in `arrayMutationMethods` (`infrastructure/createMutationView.ts:8-18`) |
| `node.widgets = undefined` | yes | explicit `state.commit(state.target)` (`widgetsView.ts:42`) |
| `.push(w)` / `.splice(i,1)` | yes | `arrayMutationMethods` |
| `node.widgets[0] = w` | yes | Proxy `set` trap (`createMutationView.ts:70-79`) |
| `node.widgets.length = 0` | yes | same trap, caught by the length compare in `commitIfChanged` (`:34-40`) |
| `delete node.widgets[0]` | yes | Proxy `deleteProperty` trap (`:80-87`) |

`src/lib/litegraph/src/node/widgetsView.test.ts:47-62` already asserts index-set and `length=`.

## The asymmetry is one level down

Two filters, same predicate, opposite outcomes:

- `getWidgetIds` drops any widget whose `widgetId` is `undefined` — `src/lib/litegraph/src/utils/widget.ts:32-38`.
- `replaceNodeWidgetOrder` drops any id with no store state for this node — `src/stores/widgetValueStore.ts:222-229`:

```ts
const nextOrder = orderedWidgetIds.filter((id) => widgetStates.get(id)?.nodeId === localNodeId)
```

The Vue render list is built from that store order, not from the array: `LGraphNode.vue:628-634` and `src/renderer/extensions/vueNodes/composables/useProcessedWidgets.ts:488-499`.

Registration only happens at `LGraph.add` (`src/lib/litegraph/src/LGraph.ts:1174-1185`, gated on `isNodeBindable`), `addCustomWidget` (`LGraphNode.ts:2248-2258`), `SubgraphNode.addCustomWidget` (`SubgraphNode.ts:853-865`) and node replacement (`useNodeReplacement.ts:191`). **Nothing back-fills a widget appended directly to the array**, and a plain object literal does not pass `isNodeBindable`, so joining the graph does not rescue it either.

So: removal → id absent from `getWidgetIds` → dropped from order → gone from render. Addition of an unregistered widget → id absent from `getWidgetIds` → order unchanged → never rendered.

## Reachability, measured

Corpus: 29 custom-node packs on one host, 157 frontend source files (`.js`/`.ts` under `*/web/*`, excluding minified bundles, `litegraph.d.ts` and `node_modules`). Control `/registerExtension/` matched **84 files**.

`/widgets\.push/` matched **4 files, 4 sites, 3 packs**. Three of the four are the `getCustomWidgets` idiom, pushing a bare object literal with no `widgetId`:

- `ComfyUI-Advanced-ControlNet/web/js/autosize.js:47`
- `ComfyUI-AnimateDiff-Evolved/web/js/autosize.js:47`
- `ComfyUI-AnimateDiff-Evolved/web/js/deprecate_nodes.js:90`

all preceded by `if (!node.widgets) node.widgets = []`.

Two of the three return `computeSize -> [0, -4]` (invisible spacers, no visible harm). **`deprecate_nodes.js:81-85` returns `[width, 20]` when `inputData[1]['text']` is set — a visible deprecation notice that would silently vanish.**

First-party code trips this too: `src/core/graph/widgets/dynamicWidgets.ts:78-95` (`ensureWidgetForInput`, used by `COMFY_AUTOGROW_V3`) pushes `{ draw, name, options:{}, serialize:false, type:'shim', y:0 }` with no `widgetId` and no `setNodeId`. `shouldRenderAsVue` returns `true` for it (`widgetRegistry.ts:305-310`), so on `main` it rendered as `WidgetLegacy`; on this branch it cannot reach the render list.

The fourth hit, `rgthree-comfy/web/comfyui/fast_actions_button.js:208`, is a reorder of an already-registered widget and is unaffected.

## Blast radius

**Render and edit only, not persistence.** `serialize()` iterates `this.widgets` (`LGraphNode.ts:1235-1248`), and `replaceNodeWidgetOrder` never deletes `WidgetState` — only `deleteWidget` does. Values survive a save; the widget is unreachable in the Vue UI.

Gated on Vue nodes being enabled, which is off by default in OSS, so this lands on Cloud first.

## Behaviour change

On `main`, `widgets` was a plain field (`main:LGraphNode.ts:311`) that `useGraphNodeManager.ts:376-406` lazily wrapped in a reactive mirror where the **array was the truth**, and the render list was `node.widgets.map(safeWidgetMapper)` (`main:useGraphNodeManager.ts:126, 438-439, 464`). Additions used to render. `useGraphNodeManager.ts` no longer exists on the branch.

## Docs

`docs/architecture/node-data-store.md:214-217` says "Array mutations, indexed and `length` writes, and assignment to `node.widgets` synchronize that order." Mechanically true, but it never states the store-registration precondition, which is the entire failure mode. Line 230's advice ("prefer `node.addWidget`") is the correct workaround but reads as a style preference rather than a correctness requirement.

`docs/architecture/ecs/ecs-state-authority-audit.md:101-102` ("`LGraphNode.widgets` remains the ordered live widget-object collection used by legacy drawing and extension APIs") is stale for the Vue path.

## Note on two store-order writers with opposite semantics

`replaceNodeWidgetOrder` (`widgetValueStore.ts:222-238`) honours removals. `setNodeWidgetOrder` -> `reconcileNodeWidgetOrder` (`:184-202`) **cannot remove** — omitted-but-tracked ids get appended back. Callers of the latter: `LGraph.ts:1180`, `promotionUtils.ts:177`, `dynamicWidgets.ts:70`. Worth making that difference explicit in the names.

## Suggested fix

Back-fill registration in `syncWidgetOrder` for any widget in the array that lacks store state, or make `getWidgetIds` mint an id rather than drop the widget. Either restores "additions land", which makes the accessor fully honoured.

Distinct from #15600, which is the *rename* variant of the same store-order filter. Part of a branch-wide audit of compatibility-accessor asymmetry — see the umbrella issue. Related: #15620, #15618, #15577, #15594.

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.