Comfy-Org / Comfy-Org/ComfyUI_frontend

ECS branch: one unmatched input name abandons a whole node's input-slot realignment on load

Open
#15,581 2 comments 0 reactions 1 assignee Claimed by @christian-byrne View on GitHub
area:links claimed Potential Bug
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

Found reviewing #14246 (`feature/ecs-migration`) at `333906c4cbe31d4d76dd79b7f053f1bc617dcf1a`. Not reachable on `main`.

## What changed

`realignInputLinkSlots` (`src/lib/litegraph/src/linkDeduplication.ts:118-170`) replaces `fixLinkInputSlots`, which the branch deletes from `src/utils/litegraphUtil.ts` along with its call site in `ComfyApp.addAfterConfigureHandler`.

Both exist to fix https://github.com/Comfy-Org/ComfyUI_frontend/issues/3348: a node's `configure` may reorder its inputs into node-definition order, which moves the slot each serialized link targets.

`main` did this, per link, unconditionally:

```ts
const linkId = input.link
const link = graph.links.get(linkId)
link.target_slot = inputIndex
```

The branch cannot use `input.link` any more, because it is derived from the link store from `target_slot`, so reading it here would be circular. It matches serialized input names against live input names instead.

## The failure

1. A serialized input whose name has no live counterpart (the node's `configure` renamed it or dropped it) yields `slots.length === 0` and is skipped at `linkDeduplication.ts:141`. Its link keeps its serialized `target_slot`.
2. When another link's move targets that stale slot, `validateEndpointUpdates` finds an incumbent that is not a participant in the batch and returns `occupied-target` (`src/stores/linkStore.ts:318-325`).
3. `realignInputLinkSlots` logs to `console.error` and `break`s at `linkDeduplication.ts:158-160`, abandoning every remaining move for that node, not just the rejected one.

Net effect: one unmatched input name can leave that node's other links wired to the wrong inputs after a workflow load. On `main` a partial mismatch cost exactly the mismatched link, because each assignment was independent and could not be rejected.

The only signal is `console.error`. The node is not marked `has_errors` and nothing surfaces to the user.

## Why this has not been measured

The ECS compatibility battery compares workflow and prompt JSON across 1,811 packs, but only for the stock 7-node workflow with the pack loaded. It has never run a workflow containing a pack's own node types. A node whose `configure` rewrites its input list is precisely what that battery does not exercise, so "0 DIFF" says nothing about this path.

## Coverage gap

`src/lib/litegraph/src/LGraph.inputSlotRealign.test.ts` covers the happy path well, but its fixture `ReorderTargetNode` (lines 24-37) only sorts `data.inputs` into definition order. No test renames an input, drops one, or drives `updateEndpoints` to a rejected batch.

## Suggested fix

`updateEndpoints` already accepts a `removals` list. Passing the blocking incumbents there would let the batch land atomically.

**Edited 2026-08-23: the per-link fallback this originally offered as a second option is struck.** Mutation-tested against the repro in #15592: the `removals` route flips **4 of 4** assertions, the per-link fallback flips **1 of 4**, and the one it flips is the `console.error` check rather than any wiring assertion. The collision cascades (link 3 squats slot 0, so link 1 cannot leave slot 1, so link 2 cannot enter it), so a per-link retry meets the same non-participant incumbent the batch did. Use `removals`. Detail: https://github.com/Comfy-Org/ComfyUI_frontend/issues/15581#issuecomment-5377128892

Either way, add tests for:

- an input renamed by `configure` while a sibling moves slots
- an input dropped by `configure` while a sibling moves slots
- a rejected `updateEndpoints` batch, asserting the surviving moves still land

## Review comment

https://github.com/Comfy-Org/ComfyUI_frontend/pull/14246#discussion_r3834017436

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.