Comfy-Org / Comfy-Org/ComfyUI_frontend
ECS branch: replacing a subgraph-input link via connect() dispatches onConnectionsChange disconnect twice
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 699
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 490
Description
On `feature/ecs-migration` (f1bfb313d6), connecting a new output onto an interior-node input that is currently fed by the subgraph input node dispatches `onConnectionsChange(INPUT, slot, false, link, subgraphInput)` on the target node twice. Merge base (a2603c59) dispatches once. Node-to-node replacement dispatches once on both refs.
## Mechanism
1. `LGraphNode.connectSlots` calls `finalizeInputLinkRemoval` for the replaced link (`src/lib/litegraph/src/LGraphNode.ts:3201`).
2. `finalizeInputLinkRemoval` calls `graph.inputNode._disconnectNodeInput(node, input, link)` (`src/lib/litegraph/src/node/slotLinks.ts:112`).
3. `_disconnectNodeInput` dispatches `onConnectionsChange` internally because the slot is still live, so `node.inputs.indexOf(input) !== -1` (`src/lib/litegraph/src/subgraph/SubgraphInputNode.ts:227-236`).
4. `finalizeInputLinkRemoval` then dispatches `onConnectionsChange` again itself (`src/lib/litegraph/src/node/slotLinks.ts:113-120`).
The internal dispatch in `_disconnectNodeInput` is guarded for the removed-slot case (`indexOf === -1`), but in the link-replacement path the slot is present, so both dispatches fire.
## Repro (vitest, runs in-repo)
```ts
import { describe, expect, it } from 'vitest'
import { LGraphNode } from '@/lib/litegraph/src/litegraph'
import { NodeSlotType } from '@/lib/litegraph/src/types/globalEnums'
import { createBoundaryLinkedSubgraph, createTestRootGraph } from './__fixtures__/subgraphHelpers'
function recordDispatches(node: LGraphNode) {
const calls: { type: number; slot: number; connected: boolean }[] = []
node.onConnectionsChange = (type, slot, connected) => {
calls.push({ type, slot, connected })
}
return calls
}
describe('onConnectionsChange dispatch count when a new connect replaces an existing input link', () => {
it('control: replacing a node-to-node link dispatches INPUT disconnect exactly once', () => {
const graph = createTestRootGraph()
const source1 = new LGraphNode('Source1')
source1.addOutput('out', '*')
graph.add(source1)
const source2 = new LGraphNode('Source2')
source2.addOutput('out', '*')
graph.add(source2)
const target = new LGraphNode('Target')
target.addInput('in', '*')
graph.add(target)
source1.connect(0, target, 0)
const calls = recordDispatches(target)
source2.connect(0, target, 0)
expect(calls.filter((c) => c.type === NodeSlotType.INPUT && !c.connected)).toHaveLength(1)
})
it('hazard: replacing a subgraph-input link dispatches INPUT disconnect exactly once', () => {
const { subgraph, interior } = createBoundaryLinkedSubgraph()
const source = new LGraphNode('InteriorSource')
source.addOutput('out', '*')
subgraph.add(source)
const calls = recordDispatches(interior)
source.connect(0, interior, 0)
expect(calls.filter((c) => c.type === NodeSlotType.INPUT && !c.connected)).toHaveLength(1) // FAILS: got 2
})
})
```
Results (vitest 4.1.10, Node 24.15.0):
| Ref | Control | Hazard |
| --- | --- | --- |
| a2603c59 merge base | 1 PASS | 1 PASS |
| f1bfb313d6 head | 1 PASS | 2 FAIL |
## Impact
Every `onConnectionsChange` implementer receives a duplicate disconnected callback for one user action: first-party widget conversion in `widgetInputs.ts` and any custom node implementing the hook. Non-idempotent handlers (state mutation, undo checkpoints, widget rebuild) run twice.
## Fix direction
One owner for the dispatch: either drop the tail dispatch in `_disconnectNodeInput` (making `finalizeInputLinkRemoval` and the direct `disconnectInput` path each responsible for their own), or drop the compensating dispatch in `finalizeInputLinkRemoval`. Note `LGraphNode.disconnectInput` (LGraphNode.ts:3446) relies on the internal dispatch, so removing the internal one requires adding a dispatch there.
Found during ECS migration review (PR #14246 program), task rv-23-fu-01.
Contributor guide
Assessment
This issue has not been assessed yet.