Comfy-Org / Comfy-Org/ComfyUI_frontend
clearViewGeometry() mass-unsubscribes onNodeChange consumers without notifying them
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 699
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 490
Description
`layoutStore.clearViewGeometry()` calls `this.nodeChangeListeners.clear()` (`src/renderer/core/layout/store/layoutStore.ts:985-987`), dropping every `onNodeChange` subscription without running any of the unsubscribe closures it handed out.
A consumer holding the stop-closure returned by `onNodeChange` (`:922-941`) has no way to observe this. The closure becomes a no-op and the consumer goes deaf permanently.
`clearViewGeometry()` runs on every Vue-nodes enable and every renderer toggle — `src/components/graph/GraphCanvas.vue:272` and `:281`.
Verified by execution at `5002fae1b12d44831a21367afa7c0f798f7e7a2c`. Note the control arm needed repair before the result meant anything: `layoutStore` dispatches changes through `queueMicrotask` (`:1269`), so nothing fires synchronously and a synchronous "listener did not fire" assertion passes for the wrong reason. With `await new Promise(r => setTimeout(r, 0))`:
- control: subscribe, move the node, listener fires.
- test: subscribe, `clearViewGeometry()`, move the node, listener does not fire.
**Reachability: latent.** `git grep onNodeChange 5002fae1b1 -- src` returns zero non-test consumers. `onNodeChange` is the only `layoutStore` subscribe method with no production caller — `onChange` and `onGeometryChange` are both used by `notifyLayoutChanges.ts`, and both are correctly released.
Two ways out, either is fine:
1. Leave `nodeChangeListeners` alone in `clearViewGeometry` — the other things it clears (`slotLayouts`, `linkLayouts`, spatial indexes, `contentSizes`) are view-scoped data, but a subscription is not data.
2. Delete `onNodeChange` as dead API.
The reason to pick one now rather than later: the next person to add an `onNodeChange` consumer gets a bug that only reproduces after a renderer toggle, with a live-looking stop-closure in hand.
Blame is `refactor!: migrate entity state to dedicated stores` (agent-authored on the #14246 branch) for the `clear()` line; `onNodeChange` itself predates it.
Related: #15620, #15618, #15594, #15577. Found reviewing #14246 (slice C3, layout/geometry).
Contributor guide
Assessment
This issue has not been assessed yet.