Comfy-Org / Comfy-Org/ComfyUI_frontend

Minimap leaks graph-event listeners on every workflow switch (hooksMap keyed by mutable graph id)

Open
#15,614 0 comments 0 reactions 1 assignee Claimed by @christian-byrne View on GitHub
agent-ok area:minimap perf:memory Potential Bug
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

## Problem

The minimap leaks a full set of graph hooks every time the user opens a different workflow while the
minimap is subscribed. `useMinimapGraph` keys its installed-hooks registry by `graph.id`, a **mutable
string**. The root `LGraph` object is created once and never replaced, so loading a workflow
reassigns `graph.id` **in place**. Every later cleanup then looks the registry up under the new id,
finds nothing, and returns without unwrapping anything. The wrappers stay attached to the immortal
root graph forever, one orphaned set per workflow switch.

The fix is to key the registry by the graph **object** instead of its id:
`WeakMap`. That removes the stale-key class without touching a single call
site.

Reproduced and fixed by execution at `origin/main` `a08a7598aa`. Two failing tests before, both
passing after, and the whole minimap suite green.

## Provenance

* **Found by:** listener-lifecycle review of the minimap composables · re-verified against
`origin/main` `a08a7598aa` on 2026-08-23
* **How:** read `useMinimapGraph.ts` and `useMinimap.ts` in full at `origin/main`; wrote two probe
tests in the existing `useMinimapGraph.test.ts` harness; confirmed both fail before the change and
pass after; ran the full minimap suite and `vue-tsc --noEmit` on the result
* **Why now:** it is a one-type-change fix with an existing test harness and a proven mutation
* **Confidence:** verified by execution

## Evidence

All anchors opened at `origin/main` `a08a7598aa`, in
`src/renderer/extensions/minimap/composables/useMinimapGraph.ts`:

| Line | Code |
| ---- | ---- |
| `:160` | `const hooksMap = new Map()` |
| `:168` | `if (!g \|\| hooksMap.has(g.id)) return` — the duplicate-setup guard |
| `:196` | `hooksMap.set(g.id, entry)` |
| `:228` | `const entry = hooksMap.get(g.id)` — followed by `if (!entry) return` on `:229` |
| `:244` | `hooksMap.delete(g.id)` |

What `setupEventListeners` installs on the graph (`:200-222`): three chained callback wrappers
(`onNodeAdded`, `onNodeRemoved`, `onConnectionChange`) plus one real event listener
(`g.events.addEventListener('node:property:changed', onPropertyChanged)` on `:222`).
`cleanupEventListeners` (`:225-245`) is the only thing that unwinds them.

The id is mutable and the object is not. `src/renderer/extensions/minimap/composables/useMinimap.ts:207-215`:

```ts
watch(graph, (newGraph, oldGraph) => {
if (newGraph && newGraph !== oldGraph) {
graphManager.cleanupEventListeners(oldGraph || undefined)
graphManager.setupEventListeners()
...
}
})
```

That guard is **object identity**. Loading a workflow calls `rootGraph.configure(graphData)`, which
reassigns the id on the same object, so the watcher never fires — no cleanup, no re-setup — while
`hooksMap`'s key silently goes stale.

Sequence:

1. Minimap initialises on workflow A. `hooksMap = { idA -> entry }`; wrappers live on the root graph.
2. User loads workflow B. Same object, so `watch(graph)` does not fire. `g.id` is now `idB`; the
registry key is still `idA`.
3. Any later cleanup does `hooksMap.get(idB)` -> `undefined` -> early return on `:229`. Every wrapper
and the `node:property:changed` listener survive.
4. A later `setupEventListeners()` finds `hooksMap.has(idB) === false` on `:168`, so it installs a
**second** full set and re-chains `onConnectionChange` around its own previous wrapper.

Subgraph enter/exit is **not** affected — the graph object identity does change there, so
`useMinimap.ts` cleans the old graph first. That path is already covered by
`useMinimapGraph.test.ts`.

### Executed at `origin/main` `a08a7598aa`

Two probes added to the existing `useMinimapGraph.test.ts` harness (it already builds a mock graph
with `onNodeAdded`/`onNodeRemoved`/`onConnectionChange` spies and a real `CustomEventTarget`):

```ts
it('cleans up after the graph id is reassigned in place', () => {
const originalOnNodeAdded = vi.fn()
mockGraph.onNodeAdded = originalOnNodeAdded
const graphManager = useMinimapGraph(ref(mockGraph) as Ref, onGraphChangedMock)

graphManager.setupEventListeners()
expect(mockGraph.onNodeAdded).not.toBe(originalOnNodeAdded)

mockGraph.id = 'test-graph-456' // what rootGraph.configure() does

graphManager.cleanupEventListeners()
expect(mockGraph.onNodeAdded).toBe(originalOnNodeAdded)
})

it('setup after an id change does not double-install', () => {
const graphManager = useMinimapGraph(ref(mockGraph) as Ref, onGraphChangedMock)
graphManager.setupEventListeners()
const firstWrapper = mockGraph.onNodeAdded
mockGraph.id = 'test-graph-789'
graphManager.setupEventListeners()
expect(mockGraph.onNodeAdded).toBe(firstWrapper)
})
```

Before the fix: `Tests 2 failed | 33 passed (35)` — both probes red.

The fix, five mechanical edits in `useMinimapGraph.ts` and nothing else:

```
:160 const hooksMap = new Map() -> new WeakMap()
:168 hooksMap.has(g.id) -> hooksMap.has(g)
:196 hooksMap.set(g.id, entry) -> hooksMap.set(g, entry)
:228 hooksMap.get(g.id) -> hooksMap.get(g)
:244 hooksMap.delete(g.id) -> hooksMap.delete(g)
```

`LGraph` is already imported as a type on `:7`, so no import change is needed.

After the fix: `vitest run src/renderer/extensions/minimap` -> **`Test Files 9 passed (9)`,
`Tests 122 passed (122)`** (including both probes). `NODE_OPTIONS=--max-old-space-size=8192 vue-tsc
--noEmit` -> **exit 0**, no errors.

## Acceptance criteria

- [ ] `hooksMap` in `src/renderer/extensions/minimap/composables/useMinimapGraph.ts` is keyed by the
`LGraph` object, not by `graph.id`. `WeakMap` is preferred over `Map` so a discarded graph is
not retained by the registry
- [ ] A regression test in `useMinimapGraph.test.ts` that reassigns `graph.id` on the same object
between `setupEventListeners()` and `cleanupEventListeners()` and asserts the original
callbacks are restored. Assert on at least `onNodeAdded`; asserting all three is better
- [ ] A second test asserting that `setupEventListeners()` after an id change does **not** install a
second wrapper (the `:168` guard must still hold)
- [ ] Both tests are mutation-verified: revert the keying change and confirm both go red, then
restore and confirm green. Report the before/after counts in the PR
- [ ] `vitest run src/renderer/extensions/minimap` is green, and no other test in the repo changed
behaviour

## Out of scope

Do not attempt these in the same PR. Each is a separate decision or a separate defect.

* **`GraphCanvasMenu.vue` and `ZoomControlsModal.vue` never call `destroy()`.** Both call
`useMinimap()` and only `MiniMap.vue` disposes it. That is a real second leak, but the fix is a
lifecycle decision about who owns the composable instance, not a keying change.
* **`watch(graph)` in `useMinimap.ts:207` never cleans up on a transition to `null`** (the guard is
`if (newGraph && newGraph !== oldGraph)`). Related, separately arguable.
* **Anything on the `feature/ecs-migration` branch.** That branch restructures these registrations
and takes each orphaned set from 1 event listener to 4. This issue is about `main` only. Do not
read that branch, do not target it, and do not try to reconcile the two.
* Do not change `useChainCallback`, the throttle interval, or the `entry.live` mechanism.

## Working notes for whoever picks this up

Traps in this repo that will otherwise cost you a cycle:

* **This is a pnpm workspace with eight `node_modules` directories** — the root plus
`apps/desktop-ui`, `apps/website`, and `packages/{design-system,object-info-parser,tailwind-utils,shared-frontend-utils,ingest-types}`.
If you work in a worktree with only the root linked, `vitest` dies at
`Failed to resolve import "clsx"` and `vue-tsc` dies on `astro/tsconfigs/strict not found` — **at
every commit**, which makes any comparison you run meaningless.
* **Run a positive control before trusting any red result.**
`vitest run src/lib/litegraph/src/LLink.test.ts` should report `Tests 3 passed (3)`. If it does
not, your setup is broken, not the code.
* **Vitest 4 has no `basic` reporter.** `--reporter=basic` exits 1 with
`Failed to load custom Reporter from basic` before running anything. Omit the flag.
* **There is no root `vitest.config.ts`.** The config is `vite.config.mts`, read by default.
`vitest run --config vitest.config.ts` dies with `UNRESOLVED_ENTRY`, which reads like a broken
checkout rather than a wrong flag.
* **`vue-tsc --noEmit` OOMs at Node's 2GB default heap and exits 134**, and a grep filtered for
`error TS` then returns zero — byte-identical to a clean run. Always run it as
`NODE_OPTIONS=--max-old-space-size=8192 vue-tsc --noEmit` and always report the exit code.
* **`oxlint`'s exit code is ambiguous in both directions.** Most rules here are warnings, so a 0 exit
does not mean no findings; and it also exits 1 when every input path is ignored
(`No files found to lint`). Read the output, not just `$?`.

---

Original issue text, preserved verbatim (this body was narrowed on 2026-08-23 to be finishable from the issue alone)

## Summary

The minimap leaks a full set of graph-event listeners every time the user opens a different workflow while the minimap is subscribed. Found while reviewing PR #14246 at `5002fae1b12d44831a21367afa7c0f798f7e7a2c`; the root cause is on `main`, and #14246 makes each leaked set 4x larger.

## Root cause: `hooksMap` is keyed by a mutable id

`src/renderer/extensions/minimap/composables/useMinimapGraph.ts` keys its installed-hooks registry by `g.id`:

- `:209` `hooksMap.set(g.id, entry)`
- `:229` `const entry = hooksMap.get(g.id)`
- `:238` `hooksMap.delete(g.id)`

The root `LGraph` object is created once (`src/scripts/app.ts:938`) and never replaced. Loading a workflow calls `rootGraph.configure(graphData)` (`app.ts:1410`), and `_configureBase` reassigns the id (`src/lib/litegraph/src/LGraph.ts:2566-2570`).

Sequence:

1. Minimap init on workflow A. `hooksMap = { idA -> entry }`; listeners live on the root graph.
2. Load workflow B. Object identity is unchanged, so `watch(graph)` in `useMinimap.ts:207-215` never fires. No cleanup, no re-setup. `g.id` is now `idB`; the key is still `idA`.
3. Any later cleanup path (`destroy()` at `useMinimapGraph.ts:277`, the canvas watcher at `useMinimap.ts:191`, entering a subgraph at `:209`) does `hooksMap.get(idB)` -> `undefined` -> `if (!entry) return` at `:230`. Every listener survives.
4. Exiting the subgraph calls `setupEventListeners()`, whose guard `hooksMap.has(idB)` is false, so it installs a second full set and re-chains `onConnectionChange` around its own previous wrapper.

Net: one orphaned listener set per workflow switch, permanently attached to the immortal root graph, firing duplicate throttled repaints and retaining the composable closure.

Subgraph enter/exit is not affected: the graph object identity does change there, so `useMinimap.ts:209` cleans the old graph first. That path is covered by `useMinimapGraph.test.ts:103-118` and `:167`.

## What #14246 changes

It moves `node:added` / `node:removed` off monkey-patched callback slots onto real `g.events` listeners and adds a `configured` listener (`useMinimapGraph.ts:220-223`). Un-removed `g.events` listeners per orphan go 1 -> 4; un-restored callback patches go 2 -> 1. The `entry.live` guard at `:214` protects the chained `onConnectionChange` but has no equivalent on the four `addEventListener` calls, so they keep firing after `entry.live = false`.

## Two smaller leaks in the same composable

- `src/components/graph/GraphCanvasMenu.vue:113` and `src/components/graph/modals/ZoomControlsModal.vue:82` both call `useMinimap()` and never call `destroy()`. Only `MiniMap.vue:141-143` does. Their instances still run `init()`, because `watch(canvas, ..., { immediate: true })` at `useMinimap.ts:187-204` calls it whenever `canvas.value && graph.value`. Each unmount of those two components leaks 3 window listeners, 1 api listener, 4 graph-event listeners and 1 un-restored `onConnectionChange`.
- `watch(graph)` at `useMinimap.ts:207-208` is guarded by `if (newGraph && newGraph !== oldGraph)`, so a transition to `null` never cleans up, and a later `destroy()` hits the null guard at `useMinimapGraph.ts:227-228` and leaves everything installed.

## Suggested fix

Key the registry by the graph object: `WeakMap` instead of `Map`. That removes the stale-key class without touching any call site.

## Test gap

At both refs there is no test that changes `graph.id` between setup and cleanup, no test that exercises a workflow switch through `useMinimap`'s `watch(graph)`, no test that dispatches `node:removed` or `configured` after cleanup, and no test that mounts/unmounts `GraphCanvasMenu.vue` or `ZoomControlsModal.vue`. All three gaps above are in that blind spot.

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.