Comfy-Org / Comfy-Org/ComfyUI_frontend
attachNodeLayout silently discards node geometry when the layout store already has an entry
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 702
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 490
Description
`attachNodeLayout` overwrites a node's own `pos`/`size` from the layout store whenever the store already holds an entry for `(rootGraphId, node.id)`, and says nothing about it.
`src/renderer/core/layout/operations/graphLayoutAttachment.ts:47-50`:
```ts
if (layoutStore.getNodeLayout(graphId, node.id)) {
adoptNodeAttachment(graphId, node)
return
}
```
and `:71-82`, where `adoptNodeAttachment` does `readNodeRect(graphId, node.id, node._posSize)` — writing the store's rect straight over the node's `_pos`/`_size`.
Verified by execution at `5002fae1b12d44831a21367afa7c0f798f7e7a2c`: seed a store entry at `(-500, -600, 11, 12)`, then `graph.add(node)` with the node's own geometry at `(10, 20, 200, 100)`. Both node and store end at `(-500, -600, 11, 12)`. The node's geometry is discarded silently.
This is the only place on the geometry surface where a node-vs-store disagreement resolves store-first. Everywhere else the node writes through and the store wins only as a cache refresh.
**I could not find a production path that reaches it, and I am not claiming this is user-facing.** The guards, named:
- `LGraph.remove` calls `detachNodeLayout` (`LGraph.ts:1313`), which deletes the entry.
- Root-graph `clear()` calls `layoutStore.clearGraph(graphId)` (`LGraph.ts:582`) after `teardownOwnedGraphs` detaches with `removeLayouts: false`, and before `this.id` is reassigned.
- Subgraph release calls `detachGraphLayouts(releasedSubgraphs)` with `removeLayouts: true` (`LGraph.ts:1306`).
- Node replacement goes through `transferLayoutAttachment`, which adopts deliberately and is documented as doing so.
So the ask is a doc comment or an assertion, not a fix. Right now the adoption branch reads like a fast path when it is actually a precedence rule, and the next person to add an `add()` call site has nothing telling them the store wins.
One path I did not run end to end: undo/redo. `changeTracker` is snapshot-based and restores through `configure`, which clears first, so it should be covered — but that is reasoning, not a measurement.
Blame is `refactor!: migrate entity state to dedicated stores` (agent-authored on the #14246 branch), so assigning to the branch owner.
Related: #15620, #15618, #15594, #15577. Found reviewing #14246 (slice C3, layout/geometry).
Contributor guide
Assessment
This issue has not been assessed yet.