Comfy-Org / Comfy-Org/ComfyUI_frontend
LGraphNode.connect by node id always returns null; on feature/ecs-migration it throws a bare string and aborts rewire loops
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 699
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 490
Description
`LGraphNode.connect(slot, targetNodeId, targetSlot)` silently returns `null` for every node id the graph actually mints, so any extension that rewires by id restores nothing. On `feature/ecs-migration` the same call throws a bare string instead, aborting multi-link rewire loops on their first iteration.
Three separable defects in one block, `src/lib/litegraph/src/LGraphNode.ts:3048-3054` (`2902-2908` on `main`):
```ts
if (target_node && typeof target_node === 'number') {
const nodeById = graph.getNodeById(target_node)
if (!nodeById) throw 'target node is null'
target_node = nodeById
}
if (!target_node) throw 'target node is null'
```
## 1. The by-id overload is unreachable (`main`, live today)
`NodeId` is a branded **string** since #13085 (`src/types/nodeId.ts:3`), and `LGraph` mints ids with `toNodeId(++state.lastNodeId)`, i.e. `'2'`, never `2`. `typeof target_node === 'number'` is therefore false for every id the graph produces, `getNodeById` is never called, and a non-empty string is truthy so the `!target_node` guard does not fire either. Execution falls through with `target_node` still a string, and `connect` returns `null`.
Measured on `main` @ `a2603c59a6`: connect two nodes, spread-copy the link, `disconnectOutput`, then `connect(0, copy.target_id, copy.target_slot)`.
| | |
| --- | --- |
| `typeof copy.target_id` | `'string'` |
| `copy.target_id` | `'2'` |
| `connect(...)` returns | `null` |
| threw | nothing |
| links restored | **0 of 1** |
This is the documented legacy overload (`@param target_node the target node`, and the numeric branch exists only to serve it). ComfyUI-Custom-Scripts `web/js/quickNodes.js:139` uses exactly this call, so "Add Clip Skip" and "Add LoRA" already drop the user's downstream wiring on `main`, by a different mechanism than the spread bug in #15594.
## 2. Bare-string `throw` (`feature/ecs-migration`)
On the branch `{ ...link }` yields no `target_id` at all (#15594), so the call reaches `if (!target_node) throw 'target node is null'`.
Measured @ `5002fae1b1`: the thrown value is the primitive string `'target node is null'`. `caught instanceof Error` is `false`, so there is no stack, no `cause`, and every reporter that keys on `Error` degrades. `reportError`/Sentry get a string with no frame pointing at the extension that made the call.
## 3. No rollback, so the abort is destructive
The callers this hits copy links **because** the disconnect is destructive, then rewire in a loop with no `try`/`catch`:
```js
const clipLinks = this.outputs[1].links.map((l) => ({ ...graph.links[l] }))
this.disconnectOutput(1)
this.connect(1, clipSkipNode, 0)
for (const clipLink of clipLinks) {
clipSkipNode.connect(0, clipLink.target_id, clipLink.target_slot)
}
```
Measured @ `5002fae1b1` with three downstream links: `disconnectOutput` removes all 3, the first `connect` throws, and **0 of 3** are restored. The throw is what turns a partial restore into a total one. Any caller with correct topology that hits one stale target id loses every link after it in the loop for the same reason.
## Why this is worth fixing at the call site rather than in each pack
Defect 1 burns every by-id caller, including ones whose topology is perfectly correct. It is not specific to spread copies and it is not specific to the ECS branch.
## Suggested resolution
Not taken here because it needs an owner's call on the ecosystem-visible behaviour:
- **Defect 1**: resolve `target_node` through `parseNodeId` / `getNodeById` whenever it is not an `LGraphNode`, not only when `typeof` is `'number'`. Restores the documented overload. Needs a decision on whether a bare numeric string may also name a node by title.
- **Defect 2**: `throw new Error('...')`, or `return null` to match the four sibling invalid-argument paths in the same method. Both are observable to extensions that currently catch the string.
- **Defect 3**: needs a rollback design (collect failures and reconnect what succeeded, or make `disconnectOutput` + rewire a single reversible operation). Out of scope for a drive-by.
Blame: defect 1 is #13085 (`DrJKL`), defect 2 predates the vendoring and blames to `webfiltered` (`0a09ecc7ac`, `5469bfdd52`).
Context: #15594, #15604. Found while closing gap-10 of the ECS migration review.
Contributor guide
Assessment
This issue has not been assessed yet.