Comfy-Org / Comfy-Org/ComfyUI_frontend
ECS branch: replaceWithMapping dispatches node:added without node:before-removed, so error hooks and selection leak the replaced node
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 699
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 490
Description
`replaceWithMapping` dispatches `node:added` for the replacement but never dispatches the removal half for the node it replaced, and the first-party consumer of those events pairs them.
Verified at PR #14246 head `907ca2b1479381eff7a617e656c9002806b5fa15`. `platform/nodeReplacement/useNodeReplacement.ts`.
`onNodeAdded` was already called at the merge base, from the caller. What is new on this branch is `nodeGraph.events.dispatch('node:added', { node: newNode })`. Grepping the whole file for `before-removed|node:removed|onAdded|fireNodeRemovalLifecycles` yields a single hit — `node.onRemoved?.()` at `:176`.
## Consumer 1: `useErrorClearingHooks` attaches and never detaches
- `node:added` (`useErrorClearingHooks.ts:603`) runs `installNodeHooksRecursive(newNode)`, `promotionErrors.attachNode(newNode)` and `scheduleAddedNodeScan(newNode, pendingScans)`.
- `node:before-removed` (`:604`) is what would run `removeNodeErrors`, `restoreNodeHooksRecursive`, `promotionErrors.detachNode` and cancel the old node's pending scans (`:588-601`). It is never dispatched.
So each replacement attaches the new node and never detaches the old one. `pendingScans` is a **strong** `Map>` (`:543`), so the placeholder is retained and its scan is left uncancelled to fire against a node whose `graph` is `null`. `newNode.has_errors = false` flips the class field but is not what `removeNodeErrors` clears.
## Consumer 2: selection is not released
`canvasStore.ts:136-143` deselects on `node:before-removed`. A placeholder the user had selected stays selected as the old, detached object. `LGraph.removeNode` additionally clears `canvas.selected_nodes[node.id]` (`LGraph.ts:1322-1325`); replacement does not, so `app.ts:1184` and `DomWidget.vue:65` keep reading it.
## Third gap, latent
`newNode.onAdded?.(nodeGraph)` is never called, though `LGraph.add` calls it at `:1190`. Corpus check over 29 local packs / 157 frontend source files (control `/registerExtension/` = 84 files, non-zero): **0 packs ship a `node_replacements` manifest today**, so `replacement.new_node_id` only ever resolves to a core type and nothing observes the miss. **3 packs / 9 files** do define `onAdded` with load-bearing setup — `fast_groups_muter` registry join, `bookmark` and `fast_actions_button` hotkey binding, ComfyUI-Custom-Scripts `binding.js` widget wiring. The day a pack publishes a replacement mapping onto its own node type, those break silently.
## Minor, same area
`:176` calls `node.onRemoved?.()` directly rather than through `fireNodeRemovalLifecycles`, so the node is never added to the `nodesBeingRemoved` weak set that `LGraph.remove` uses as its reentrancy guard (`LGraph.ts:1243`, `:1256`). Unreachable today because the only nodes this function accepts are bare `new LGraphNode` placeholders with no `onRemoved`, but it is a trap if the `last_serialization` filter at `:276` ever widens.
## Suggested
Dispatch `node:before-removed` before `:176` and `node:removed` plus `onNodeRemoved` after `:188`, and call `newNode.onAdded?.(nodeGraph)` alongside the `node:added` dispatch.
Raised originally as a review thread on #14246; filed so the finding does not depend on that thread.
Related: #12882 (other `replaceWithMapping` concerns).
Contributor guide
Assessment
This issue has not been assessed yet.