Comfy-Org / Comfy-Org/ComfyUI_frontend
ECS branch: renaming a widget after registration strands its store key and hides the widget in Nodes 2.0
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 699
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 490
Description
Renaming a widget after it has been registered strands its `widgetValueStore` entry under the old key, and on `feature/ecs-migration` that makes the widget disappear from the Vue node renderer.
Reviewed at `9f6ab9adda576652bfe52ee44fa432e8058d727d` (PR #14246), merge base `6532665db947acb61ed044fe91a1d4fe1fb84c8b`.
## Mechanism
- `BaseWidget.name` is a plain field (`src/lib/litegraph/src/widgets/BaseWidget.ts:80`). There is no setter and no writeback.
- The store key is minted once, in `setNodeId` (`BaseWidget.ts:152`), as `widgetId(graphId, nodeId, this.name)`.
- `widget.name = 'x'` afterwards moves the `widgetId` getter (`BaseWidget.ts:139`) to a new id. Nothing re-registers under it, and the `widgets` mutation view only commits on array mutations, so a field rename is invisible to `syncWidgetOrder`.
- `computeProcessedWidgets` takes its id list from the store order and filters it against `mapLiveWidgetsById` (`src/utils/litegraphUtil.ts:343`), which keys the live widget under its *current* name. The old id is not in the live map, so it is filtered out; if an array mutation later replaces the order with the new id, `processWidget` (`src/renderer/extensions/vueNodes/composables/useProcessedWidgets.ts:363-364`) finds no store entry and returns `null`. Either way the widget is not rendered, and nothing is logged.
## Why this is a regression, not pre-existing
At the merge base, `computeProcessedWidgets` iterated `nodeData.widgets` and treated `widgetState === undefined` as "no stored overrides", still rendering the widget from live data (`useProcessedWidgets.ts:259-271` at `6532665db9`). The branch makes the store the sole source of the widget list, so a key mismatch is now fatal rather than degraded.
## 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, so the harness is live.
1 pack renames a registered widget, at 3 sites, all in `rgthree-comfy`:
| Site | Node |
| ---- | ---- |
| `web/comfyui/base_node_mode_changer.js:44` | Fast Muter, Fast Bypasser |
| `web/comfyui/fast_groups_muter.js:253` | Fast Groups Muter, Fast Groups Bypasser |
| `web/comfyui/fast_actions_button.js:97` | Fast Actions Button |
`base_node_mode_changer.js:34` additionally creates the widget as `addWidget('toggle', '', false, ...)`. The empty name makes the id `::`, which `WIDGET_ID_PATTERN` rejects, so `registerWidget` already refuses it and warns before the rename ever happens. That is a second, independent way for the same node to end up unrendered.
Not measured: whether the same nodes render correctly on `main`'s Vue renderer end to end. The mechanism is read off the diff; a browser check of one rgthree Fast Muter node under Nodes 2.0 on both refs would settle it.
## Related, same key surface
`getWidgetIds` (`src/lib/litegraph/src/utils/widget.ts:35-37`) filters only `undefined`, not un-keyable strings, so `replaceNodeWidgetOrder` accepts `::` into the stored order where `getWidget` can never resolve it and only `removeNodeWidgetOrder` removes it.
## Suggested fix
Either a `name` setter on `BaseWidget` that deletes the old store entry and re-registers under the new id, or a re-register pass in the `widgets` mutation-view commit. The first-party renames at `SubgraphNode._setWidget:666-673` and `promotionUtils.ts:333-336` already do this by hand on the next line, which is the convention this would make enforceable.
A regression test needs to assert the rendered widget list after `widget.name = 'x'` on a node already in the graph, not just that the store holds an entry.
## Review thread
https://github.com/Comfy-Org/ComfyUI_frontend/pull/14246#discussion_r3834855114
Contributor guide
Assessment
This issue has not been assessed yet.