Comfy-Org / Comfy-Org/ComfyUI_frontend
Node replacement: address remaining concerns in replaceWithMapping() flagged by AustinMroz
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 699
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 490
Description
## Background
During review of PR #12872 (fix: bind replacement node widgets to reused node id), [@AustinMroz](https://github.com/AustinMroz) approved the change but raised three remaining concerns about the node replacement code in `src/platform/nodeReplacement/useNodeReplacement.ts` that were explicitly noted as out of scope for that PR.
## Concerns
### 1. Widget configuring logic is flawed
Rather than iterating over `inputMapping`, the code needs to (carefully) iterate over `newNode`'s widgets and query `inputMapping` for the replacement value. The current approach may miss widgets not present in `inputMapping` or incorrectly skip widgets that should be configured.
### 2. `node.onAdded` is not called
During the node replacement flow, `node.onAdded` is never invoked. This lifecycle hook is normally called when a node is added to a graph via `LGraph.add()`, and skipping it may cause initialization side-effects to be missed for the replacement node.
### 3. Bypassing `graph.add()` is brittle
The current implementation writes directly to `graph._nodes` and `graph._nodes_by_id` instead of going through the normal `LGraph.add()` flow. This is fragile and makes future code divergence risky, as any new logic added to `LGraph.add()` would be silently skipped during node replacement.
## References
- PR #12872 — original fix that surfaced these concerns
- AustinMroz's review comment: https://github.com/Comfy-Org/ComfyUI_frontend/pull/12872#pullrequestreview-2942481373
## Requested by
@jaeone94
Contributor guide
Assessment
This issue has not been assessed yet.