Comfy-Org / Comfy-Org/ComfyUI_frontend
ECS: LGraph.add() re-registers nested subgraph definitions but not the re-added host's own released definition
- 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
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