Comfy-Org / Comfy-Org/ComfyUI_frontend

fix: subgraph-output floating links write origin side instead of target

Open
#12,986 0 comments 0 reactions 0 assignees View on GitHub
area:subgraph Potential Bug
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

## Summary

`FloatingRenderLink.connectToSubgraphOutput` writes the subgraph-output boundary to the **origin** side of the floating link (`origin_id` / `origin_slot`), but a subgraph output node is a data *sink* and should be written to the **target** side (`target_id` / `target_slot`).

Writing the output node into `origin_id` leaves the link's target unbound and bypasses `LLink.targetIsIoNode` resolution.

## Location

`src/lib/litegraph/src/canvas/FloatingRenderLink.ts` (`connectToSubgraphOutput`)

```ts
connectToSubgraphOutput(
output: SubgraphOutput,
_events?: CustomEventTarget
): void {
const floatingLink = this.link
floatingLink.origin_id = SUBGRAPH_OUTPUT_NODE_ID
floatingLink.origin_slot = output.parent.slots.indexOf(output)

this.fromSlot._floatingLinks?.delete(floatingLink)
output._floatingLinks ??= new Set()
output._floatingLinks.add(floatingLink)
}
```

## Why this looks wrong

The sibling method `connectToSubgraphInput` correctly writes the **origin** side, because a subgraph input node is a data *source*:

```ts
floatingLink.origin_id = SUBGRAPH_INPUT_NODE_ID
floatingLink.origin_slot = input.parent.slots.indexOf(input)
```

By symmetry, the output (sink) side should write `target_*`. This is corroborated by `src/lib/litegraph/src/subgraph/subgraphUtils.ts`, which builds subgraph-output links using the target side:

```ts
linkData.target_id = SUBGRAPH_OUTPUT_NODE_ID
linkData.target_slot = outputs.length
```

So the rest of the system treats subgraph output as the target side; the floating-link path here is inconsistent.

## Proposed fix

```diff
connectToSubgraphOutput(
output: SubgraphOutput,
_events?: CustomEventTarget
): void {
const floatingLink = this.link
- floatingLink.origin_id = SUBGRAPH_OUTPUT_NODE_ID
- floatingLink.origin_slot = output.parent.slots.indexOf(output)
+ floatingLink.target_id = SUBGRAPH_OUTPUT_NODE_ID
+ floatingLink.target_slot = output.parent.slots.indexOf(output)

this.fromSlot._floatingLinks?.delete(floatingLink)
output._floatingLinks ??= new Set()
output._floatingLinks.add(floatingLink)
}
```

## Acceptance criteria

- Dragging a floating link onto a subgraph output produces a link whose **target** endpoint is `SUBGRAPH_OUTPUT_NODE_ID` with the correct slot.
- Add a regression test covering drag-to-subgraph-output that asserts the resulting link endpoints and survives a serialize/deserialize round-trip.
- Verify floating-link rendering for subgraph outputs is unaffected.

## Notes

- Surfaced by CodeRabbit during review of #12958 (NodeId branded-number refactor). It is a **pre-existing** behavior unrelated to that typing change, so it was deliberately split out rather than bundled into the refactor PR.
- Needs a litegraph/subgraph-owner review given the impact on floating-link resolution, rendering, and serialization.

Contributor guide

Open the contributing guide

Research direction

Start in src/lib/litegraph/src/canvas/FloatingRenderLink.ts at connectToSubgraphOutput, then compare connectToSubgraphInput and the target-side construction in src/lib/litegraph/src/subgraph/subgraphUtils.ts. Add a regression test for dragging to a subgraph output, including endpoint assertions and a serialize/deserialize round-trip; verify floating-link rendering remains unaffected.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
frontend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
74/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.