Comfy-Org / Comfy-Org/ComfyUI_frontend

vueNodes: reset LiteGraph.vueNodesSuspended on scope dispose

Open
#15,285 1 comment 1 reaction 1 assignee Claimed by @benjcooley View on GitHub
area:litegraph area:vue-migration Potential Bug
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

### FU1 — Culling makes a latent shared-converter scope-ownership bug live: link drags and drops will land at the wrong coordinates

> `useSharedCanvasPositionConversion()` (`src/composables/element/useCanvasPositionConversion.ts:44-48`)
> caches its result in a module-level `sharedConverter` forever, but the
> `useElementBounding(canvasElement)` it calls internally registers a ResizeObserver, a
> MutationObserver on `style`/`class`, a capturing `scroll` listener and a window `resize` listener,
> all bound via VueUse's `tryOnScopeDispose` to whatever effect scope is active on the first call.
> Of the six call sites, five run with no active scope and are therefore safe
> (`canvasPointerEvent.ts:38`, `useSlotElementTracking.ts:156`, `useVueNodeResizeTracking.ts:120`,
> `useCanvasDrop.ts:32`, all inside functions or handlers). One does not:
> `useSlotLinkInteraction` is called at `` top level in `InputSlot.vue:141` and
> `OutputSlot.vue:131`, and calls the shared converter at `useSlotLinkInteraction.ts:129`. On
> Vue-nodes load, node components mount before any pointer interaction, so the first slot component
> to render owns the converter. Before #15030 node components never unmounted, so that owner was
> immortal and the arrangement worked by accident. With culling, that node is culled on the first
> pan away from it, VueUse tears down all four observers and listeners, and `sharedConverter` stays
> cached with frozen `left`/`top`. `canvasPointerEvent.ts:39` calls `conversion.update()` before use
> and self-heals; the three that do not are `useSlotLinkInteraction` (link dragging),
> `useSlotElementTracking` (collapsed-node slot layouts) and `useCanvasDrop` (drag-and-drop
> placement), so link drags and drops land off by exactly the offset delta after any sidebar toggle,
> panel resize, window resize or page scroll. Fix is to own the shared instance in a detached
> `effectScope(true)` rather than borrowing whichever component scope happens to be active. Filed
> rather than commented because none of these files are in #15030's diff and the agent reasoned the
> mount ordering rather than instrumenting which call site wins the race; that should be confirmed
> first. This is the highest-value item in this section: it is the "once-per-session leak becomes
> once-per-pan-sweep" class, and interaction failures are exactly what got mount/unmount culling
> reverted in #5510 and #5767.

---
Found during round 3 review of #15031. Filed as a follow-up so it is not lost when that PR merges.

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.