Comfy-Org / Comfy-Org/ComfyUI_frontend

Node replacement: address remaining concerns in replaceWithMapping() flagged by AustinMroz

Open
#12,882 1 comment 1 reaction 1 assignee Claimed by @jaeone94 View on GitHub
Potential Bug
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

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.