Comfy-Org / Comfy-Org/ComfyUI_frontend

clearViewGeometry() mass-unsubscribes onNodeChange consumers without notifying them

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

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.