Comfy-Org / Comfy-Org/ComfyUI_frontend
chore(agent): subgraph materialization follow-ups deferred from #16922
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 699
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 490
Description
## Problem
#16922 makes an agent-seeded subgraph definition go through `rootGraph.createSubgraph()` so root nodes typed by it instantiate as `SubgraphNode` instead of a missing-node placeholder. Four review findings on that PR are real but out of scope for the short-term fix. They belong to the agent subgraph technical design (link will be added when the design doc is public).
1. **Stale definitions after `doc_reset` / `follower_replaced`** ([review thread](https://github.com/Comfy-Org/ComfyUI_frontend/pull/16922#discussion_r3930880347)). The materializer only ever adds definitions. A lineage break that drops or replaces a definition leaves the old one in `rootGraph.subgraphs`, in `nodeDefStore`, and registered as a `LiteGraph` node type. Nodes are pruned on reset today; definitions are not.
2. **Interior/root node id collisions** ([review thread](https://github.com/Comfy-Org/ComfyUI_frontend/pull/16922#discussion_r3930880350)). `registerSubgraphDefinitions` runs before `reconcile(graph)`, so `collectReservedNodeIds()` sees only nodes already on the live graph, not store-only root nodes from the same frame. The op layer (`@comfyorg/comfy-multi-player`) does not enforce id uniqueness across root and interior scopes. Any renumbering LiteGraph applies to an interior node also desyncs that node's id from the document, so the fix is an id-mapping design decision, not a reservation tweak.
3. **Full definitions are deep-copied every frame** ([review thread](https://github.com/Comfy-Org/ComfyUI_frontend/pull/16922#discussion_r3930880354)). `reconcileLiveGraph` calls `readSubgraphDefinitions(doc)` on every applied frame although registration only needs definitions whose id is absent from `rootGraph.subgraphs`. Cheap today (definitions are small, interior order is static in v1); wrong shape once definitions grow or fork.
4. **No deterministic browser test crosses reader → materializer → `SubgraphNode` lifecycle** ([review thread](https://github.com/Comfy-Org/ComfyUI_frontend/pull/16922#discussion_r3930822341)). Unit tests join `readSubgraphDefinitions` and `reconcileAgentAdapters` by hand; the composable test mocks both. The blind reviewer notes `browser_tests/fixtures/ws.ts` plus the agent API fixture should make an in-repo Playwright test feasible.
## Context
Discovered during #16922 (agent adds subgraph → missing-node placeholder) from the cursor-review lens fleet and an independent blind review.
## Suggested Approach
Resolve 1–3 in the agent subgraph design doc first (definition lifecycle on lineage break, id scoping between doc and live graph, incremental definition reads). Item 4 can start independently: seed a definition through the ws fixture and assert the root node is a `SubgraphNode` with `configure` observing named widget values.
## Estimated Effort
medium (design) + small per item once decided
Contributor guide
Research direction
Start with the materialization path around reconcileLiveGraph, readSubgraphDefinitions, registerSubgraphDefinitions, and reconcile, then review the linked findings from #16922. Read browser_tests/fixtures/ws.ts and the agent API fixture to assess the proposed Playwright lifecycle test. Done means the agent subgraph design resolves definition lifecycle, id scoping, and incremental reads, with item 4 covered by a browser test asserting SubgraphNode behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- playwright, typescript
- Domain
- frontend, testing
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100