[PR #3402] [CLOSED] fix(node-fixer): reconnect inputs to the new node, not its id (#3126) #3268

New issue

Closed

opened 2026-10-28 21:16:37 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-28 21:16:37 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/ComfyUI-Manager/pull/3402
Author: @StevenLi-phoenix
Created: 10/22/2026
Status: ❌ Closed

Base: main ← Head: fix/3126-recreate-string-node-ids

📝 Commits (1)

ecfecf2 fix(node-fixer): reconnect inputs to the new node, not its id (#3126)

📊 Changes

2 files changed (+157 additions, -1 deletions)

View changed files

📝 js/node_fixer.js (+1 -1)
➕ tests/test_node_fixer_recreate.py (+156 -0)

📄 Description

Fixes #3126

The root cause and the one-argument fix are as @EnragedAntelope diagnosed in the issue.

Problem

On current frontends, "Fix node (recreate)" fails on any node that has a connected input. The console shows TypeError: t.findInputSlot is not a function, and the canvas keeps two copies of the node: the original, still wired, and an unwired replacement.

Root cause

node_info_copy() in js/node_fixer.js re-wired each linked input with:

src_node.connect(link.origin_slot, dest.id, input.name);

NodeId is a string on current frontends. LGraphNode.connect() looks up the target by id only when typeof target_node === 'number'. Any other value is used as the node itself, so the id string got to target_node.findInputSlot(...) and threw. The menu callback adds the replacement node before it calls node_info_copy(), so the throw skipped app.canvas.graph.remove(this) and both nodes stayed on the canvas.

Fix

Pass the node instead of its id, as the output loop in the same function already does (dest.connect(parseInt(i), target_node, link.target_slot)). This is a one-line change.

I did not take the issue's second suggestion, to remove the original before re-wiring. LGraph.remove() disconnects the node's inputs and outputs, and those are the links node_info_copy() reads.

Tests

tests/test_node_fixer_recreate.py lifts the shipped node_info_copy() and the menu callback with tests/js_lift.py. It runs them in node against a small stub graph that has string node ids and the same connect() id-resolution rule as litegraph. It covers:

node_info_copy() re-creates the input link and the output link on the new node.
The "Fix node (recreate)" callback leaves exactly one node, with the links moved to it.

Red, against the pre-fix file:

mkdir -p /tmp/base && git show origin/main:js/node_fixer.js > /tmp/base/node_fixer.js
MANAGER_JS_DIR=/tmp/base pytest tests/test_node_fixer_recreate.py
# 2 failed: AssertionError: 'TypeError: target_node.findInputSlot is not a function' is not None

Green, and the rest of the suite:

pytest tests/test_node_fixer_recreate.py      # 2 passed
pytest tests --ignore=tests/e2e               # 220 passed, 18 skipped (218 passed, 18 skipped before)
ruff check .                                  # All checks passed!

I did not run tests/e2e. It needs the separate E2E ComfyUI root, and it does not cover node_fixer.js.

Manual check: ComfyUI master (46aad33) with comfyui-frontend-package==1.57.1, python main.py --cpu, and headless Chromium. node_fixer.js was loaded as a web extension. I wired CheckpointLoaderSimple -> KSampler -> VAEDecode. Then I called the KSampler's "Fix node (recreate)" entry from getExtraMenuOptions() in the page.

Before: TypeError: t.findInputSlot is not a function. Nodes 1, 2, 3, 4, with KSampler twice. The links still pointed to the original node 2.
After: no error. Nodes 1, 3, 4. Links 1 -> 4 and 4 -> 3.

Not covered

Recreating a node inside a subgraph still fails, with a different cause. node_info_copy() reads links from app.graph (the root graph), so it cannot find links that are inside the subgraph. On 1.57.1 it throws Cannot read properties of undefined (reading 'origin_id'). Inputs that come from the subgraph's input node would need their own handling as well, so I left this for a separate change.

The same dest.id line is also in comfyui_manager/js/node_fixer.js on manager-v4.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/ComfyUI-Manager/pull/3402 **Author:** [@StevenLi-phoenix](https://github.com/StevenLi-phoenix) **Created:** 10/22/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/3126-recreate-string-node-ids` --- ### 📝 Commits (1) - [`ecfecf2`](https://github.com/Comfy-Org/ComfyUI-Manager/commit/ecfecf21543360a3e43d1a772cd1f4f2a0c08d94) fix(node-fixer): reconnect inputs to the new node, not its id (#3126) ### 📊 Changes **2 files changed** (+157 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `js/node_fixer.js` (+1 -1) ➕ `tests/test_node_fixer_recreate.py` (+156 -0) </details> ### 📄 Description Fixes #3126 The root cause and the one-argument fix are as @EnragedAntelope diagnosed in the issue. ## Problem On current frontends, "Fix node (recreate)" fails on any node that has a connected input. The console shows `TypeError: t.findInputSlot is not a function`, and the canvas keeps two copies of the node: the original, still wired, and an unwired replacement. ## Root cause `node_info_copy()` in `js/node_fixer.js` re-wired each linked input with: ```js src_node.connect(link.origin_slot, dest.id, input.name); ``` `NodeId` is a string on current frontends. `LGraphNode.connect()` looks up the target by id only when `typeof target_node === 'number'`. Any other value is used as the node itself, so the id string got to `target_node.findInputSlot(...)` and threw. The menu callback adds the replacement node before it calls `node_info_copy()`, so the throw skipped `app.canvas.graph.remove(this)` and both nodes stayed on the canvas. ## Fix Pass the node instead of its id, as the output loop in the same function already does (`dest.connect(parseInt(i), target_node, link.target_slot)`). This is a one-line change. I did not take the issue's second suggestion, to remove the original before re-wiring. `LGraph.remove()` disconnects the node's inputs and outputs, and those are the links `node_info_copy()` reads. ## Tests `tests/test_node_fixer_recreate.py` lifts the shipped `node_info_copy()` and the menu callback with `tests/js_lift.py`. It runs them in node against a small stub graph that has string node ids and the same `connect()` id-resolution rule as litegraph. It covers: - `node_info_copy()` re-creates the input link and the output link on the new node. - The "Fix node (recreate)" callback leaves exactly one node, with the links moved to it. Red, against the pre-fix file: ``` mkdir -p /tmp/base && git show origin/main:js/node_fixer.js > /tmp/base/node_fixer.js MANAGER_JS_DIR=/tmp/base pytest tests/test_node_fixer_recreate.py # 2 failed: AssertionError: 'TypeError: target_node.findInputSlot is not a function' is not None ``` Green, and the rest of the suite: ``` pytest tests/test_node_fixer_recreate.py # 2 passed pytest tests --ignore=tests/e2e # 220 passed, 18 skipped (218 passed, 18 skipped before) ruff check . # All checks passed! ``` I did not run `tests/e2e`. It needs the separate E2E ComfyUI root, and it does not cover `node_fixer.js`. Manual check: ComfyUI master (46aad33) with `comfyui-frontend-package==1.57.1`, `python main.py --cpu`, and headless Chromium. `node_fixer.js` was loaded as a web extension. I wired CheckpointLoaderSimple -> KSampler -> VAEDecode. Then I called the KSampler's "Fix node (recreate)" entry from `getExtraMenuOptions()` in the page. - Before: `TypeError: t.findInputSlot is not a function`. Nodes 1, 2, 3, 4, with KSampler twice. The links still pointed to the original node 2. - After: no error. Nodes 1, 3, 4. Links 1 -> 4 and 4 -> 3. ## Not covered Recreating a node inside a subgraph still fails, with a different cause. `node_info_copy()` reads links from `app.graph` (the root graph), so it cannot find links that are inside the subgraph. On 1.57.1 it throws `Cannot read properties of undefined (reading 'origin_id')`. Inputs that come from the subgraph's input node would need their own handling as well, so I left this for a separate change. The same `dest.id` line is also in `comfyui_manager/js/node_fixer.js` on `manager-v4`. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-28 21:16:37 +00:00