Comfy-Org / Comfy-Org/ComfyUI_frontend

clearViewGeometry drops entity-level LayoutChanges queued in the same tick and leaves a stale flush microtask armed

Open
#15,698 0 comments 0 reactions 1 assignee Claimed by @DrJKL View on GitHub
Potential Bug
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

`clearViewGeometry` empties the pending-change queues and clears the queued flags without cancelling the microtask already scheduled to drain them.

Verified at PR #14246 head `907ca2b1479381eff7a617e656c9002806b5fa15`. `renderer/core/layout/store/layoutStore.ts`.

`queueChange` sets `isGlobalDispatchQueued = true` and arms `queueMicrotask(() => { this.flushQueuedGlobalChanges() })` (`:1266-1271`). `clearViewGeometry` then does:

```ts
this.pendingGlobalChanges = []
this.isGlobalDispatchQueued = false
this.pendingGeometryChanges = []
this.isGeometryDispatchQueued = false
```

with no handle to cancel the armed microtask. If `clearViewGeometry` runs before that microtask fires, the queued changes are dropped; the microtask still runs, finds an empty array, and returns.

Two consequences:

- **Any `LayoutChange` queued earlier in the same tick is silently lost.** These are entity-level operations — `createNode`, `moveNode`, `resizeNode`, `deleteNode`, group and reroute ops — not view geometry, so the doc comment at `:979-984` ("slot and link layouts, the spatial indexes over them, and the listeners and queues bound to them") undersells what is being discarded. The queue is not filtered by graph either, so a change belonging to a graph that is **not** being left goes with it.
- **The flag reset re-arms scheduling while a stale microtask is still pending**, so the next `queueChange` in the same tick schedules a second one.

Both current call sites (`GraphCanvas.vue:272` and `:281`, the renderer toggle) happen to be safe: neither applies a layout operation earlier in the same synchronous block. That is a scheduling accident rather than an invariant, and the third caller will not be.

Cheapest fix: keep a handle on the scheduled flush and make `clearViewGeometry` either drain it first, or leave the queues alone — entity changes are not view geometry.

Raised originally as a review thread on #14246; filed so the finding does not depend on that thread.

Related: #15625 (the listener registry in the same method), #15682 (the flush loop's missing try/catch).

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.