Comfy-Org / Comfy-Org/ComfyUI_frontend

attachNodeLayout silently discards node geometry when the layout store already has an entry

Open
#15,624 0 comments 0 reactions 1 assignee Claimed by @DrJKL View on GitHub
Potential Bug
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.