Comfy-Org / Comfy-Org/ComfyUI_frontend

ECS: LGraph.add() re-registers nested subgraph definitions but not the re-added host's own released definition

Open
#15,789 0 comments 0 reactions 0 assignees View on GitHub
area:subgraph Potential Bug
Dominant language
TypeScript
Stars
2k
Forks
704
Avg merge
1d 8h
Merged PRs (30d)
512

Description

## Problem / Goal

On `feature/ecs-migration` (#14246), removing the last instance of a subgraph releases its definition: `LGraph.remove()` → `findReleasableSubgraphs` → unregister link topologies, detach nodes from stores, delete from `rootGraph.subgraphs`, detach graph layouts. This is intentional and pinned by tests (`LLink.store.test.ts` 'unregisters a subgraph definition's links when its last instance is removed', `useErrorClearingHooks.promotion.test.ts`, subgraphPasteDelete ownership tests; ADR-0003).

The asymmetry: `LGraph.add()` (~line 1196) walks a newly added `SubgraphNode` and re-registers **nested inner** subgraph definitions — but not the added host's **own** subgraph if that definition was previously released. A host constructed from a released live `Subgraph` object therefore configures against missing interior links ('No link found for link ID …'), and its promoted inputs get no `widgetId`.

Impact is low today: production same-id re-entry paths (`configure()` with definitions, paste, `createSubgraph`) re-register definitions first, so the naive path is only reachable by constructing a `SubgraphNode` directly from a released live object. The boundary is characterized in `src/core/graph/subgraph/promotionAfterReplacement.test.ts` ('releases the definition with its last host…').

Goal: decide whether the own-vs-nested distinction is deliberate, and either make `add()` symmetric or document the boundary at the code site.

## Proposed Solution

Either:
1. **Deliberate:** add a short note at the `add()` re-registration block stating that a host's own released definition is intentionally NOT re-registered (re-entry must go through configure/paste), so this isn't re-discovered as a bug; or
2. **Symmetry fix:** have `add()` also re-register the added host's own definition when it is absent from `rootGraph.subgraphs` (note: released link topologies would still need re-registration for the definition to be usable, so this is more than a `subgraphs.set`).

## Acceptance Criteria

- [ ] Maintainer verdict recorded (deliberate boundary vs. symmetry fix)
- [ ] If deliberate: comment at the `LGraph.add()` re-registration block documenting the own-vs-nested boundary
- [ ] If fixed: `add()` re-registers the host's own released definition, with a unit test covering re-add of a last-instance-released subgraph host
- [ ] Existing release-contract tests (`LLink.store.test.ts`, `promotionAfterReplacement.test.ts` boundary test) still pass or are updated with the same rigor

Refs: #14246, ADR-0003.

Contributor guide

Open the contributing guide

Research direction

Start at LGraph.add() around line 1196 and compare its re-registration behavior with LGraph.remove() and findReleasableSubgraphs. Read ADR-0003 and the cited LLink.store.test.ts, promotionAfterReplacement.test.ts, useErrorClearingHooks.promotion.test.ts, and subgraphPasteDelete ownership tests. Done means recording the deliberate-boundary verdict with a code comment, or implementing and testing host re-registration while preserving the release-contract tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
frontend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.