Skip to content

Commit 34433df

Browse files
committed
fix: make the digest the sole authority on minimap repaints
The digest existed but two paths bypassed it: the graphChanged event hook repainted unconditionally - so a bring-to-front from any widget pointerdown still forced a full rebuild - and the execution watcher forced a redraw per progress message. Both now funnel through one checkAndRepaint that compares digests first; events are hints to check, not commands to repaint. For that to be safe the digests must cover everything the renderer reads, so they now include node bgcolor, groups (position, size, colour) and per-node execution state, split into a geometry digest that drives bounds recomputation and a visual digest that only repaints. The execution deep watcher is deleted outright: state is drawn as discrete outline colours, so the poll picks up each transition within a tick and a run costs zero work per progress message. Group drags, previously tracked by nothing, now repaint too. Fixes a poll-lifecycle regression from the earlier canvas gating: init() runs synchronously from the immediate canvas watcher, before the template ref mounts, so a start decision taken inside init() saw a null canvasRef and never started the loop - events masked it. Polling is now derived state: watch(initialized && canDraw && !hidden), started and stopped as conditions change rather than decided once. Every new behaviour is mutation-tested: dropping the bgcolor, group or execution terms, reverting the event gate to an unconditional repaint, or restoring the one-shot init decision each fails a named test. Verified in-browser: a widget click now changes zero minimap pixels; drags, adds and removes still repaint.
1 parent c8fd84c commit 34433df

4 files changed

Lines changed: 246 additions & 88 deletions

File tree

src/renderer/extensions/minimap/composables/useMinimap.intervalLifecycle.test.ts

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,66 @@ describe('useMinimap change-detection interval', () => {
204204
}
205205
})
206206

207+
it('a graphChanged event with nothing drawn changed does not repaint', async () => {
208+
// graphChanged fires for zIndex (every widget pointerdown), widget values
209+
// and title edits. The event is a hint to compare digests, not a command
210+
// to repaint.
211+
const { api } = await import('@/scripts/api')
212+
await initMinimap()
213+
await vi.advanceTimersByTimeAsync(POLL_MS + 10)
214+
const settled = drawCalls()
215+
216+
const graphChangedHandler = vi
217+
.mocked(api.addEventListener)
218+
.mock.calls.find(([name]) => name === 'graphChanged')?.[1] as () => void
219+
expect(graphChangedHandler).toBeTypeOf('function')
220+
221+
graphChangedHandler()
222+
await vi.advanceTimersByTimeAsync(POLL_MS * 6)
223+
expect(drawCalls()).toBe(settled)
224+
225+
// The same path must still repaint when the picture did change.
226+
moveNode()
227+
graphChangedHandler()
228+
await vi.advanceTimersByTimeAsync(POLL_MS + 10)
229+
expect(drawCalls()).toBeGreaterThan(settled)
230+
})
231+
232+
it('starts polling when the canvas mounts after init', async () => {
233+
// In the real component init() runs synchronously from the immediate
234+
// canvas watcher, before the template ref has mounted, so a start decision
235+
// taken inside init() sees a null canvasRef and never starts the loop.
236+
const canvasRef = shallowRef<HTMLCanvasElement | null>(null)
237+
const container = {
238+
getBoundingClientRect: vi.fn(() => new DOMRect(0, 0, 250, 200) as DOMRect)
239+
}
240+
const minimap = useMinimap({
241+
containerRefMaybe: shallowRef(
242+
container as Partial<HTMLDivElement> as HTMLDivElement
243+
),
244+
canvasRefMaybe: canvasRef
245+
})
246+
active = minimap
247+
await minimap.init()
248+
await nextTick()
249+
250+
// Template ref mounts after init.
251+
canvasRef.value = createMockMinimapCanvas({
252+
getContext: vi
253+
.fn()
254+
.mockImplementation((id) =>
255+
id === '2d' ? context : null
256+
) as HTMLCanvasElement['getContext']
257+
})
258+
await nextTick()
259+
await vi.runOnlyPendingTimersAsync()
260+
261+
moveNode()
262+
await vi.advanceTimersByTimeAsync(POLL_MS + 10)
263+
264+
expect(drawCalls()).toBeGreaterThan(0)
265+
})
266+
207267
it('stops polling after destroy', async () => {
208268
const minimap = await initMinimap()
209269
minimap.destroy()

src/renderer/extensions/minimap/composables/useMinimap.ts

Lines changed: 42 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import type { ShallowRef } from 'vue'
55
import type { LGraph } from '@/lib/litegraph/src/litegraph'
66
import { useSettingStore } from '@/platform/settings/settingStore'
77
import { useWorkflowStore } from '@/platform/workflow/management/stores/workflowStore'
8-
import { useExecutionStore } from '@/stores/executionStore'
98
import { useCanvasStore } from '@/renderer/core/canvas/canvasStore'
109

1110
import type { MinimapCanvas, MinimapSettingsKey } from '../types'
@@ -15,7 +14,7 @@ import { useMinimapRenderer } from './useMinimapRenderer'
1514
import { useMinimapSettings } from './useMinimapSettings'
1615
import { useMinimapViewport } from './useMinimapViewport'
1716

18-
/** How often to poll for node drags/resizes, which emit no graph event. */
17+
/** How often to compare digests for state that emits no event; see the poll. */
1918
const CHANGE_DETECTION_INTERVAL_MS = 100
2019

2120
export function useMinimap({
@@ -83,12 +82,18 @@ export function useMinimap({
8382
// a full layout-map rebuild, so every path that redraws checks this first.
8483
const canDraw = computed(() => visible.value && !!canvasRef.value)
8584

86-
// Graph event management
87-
const graphManager = useMinimapGraph(graph, () => {
85+
// Graph events are hints to compare the digests, not commands to repaint:
86+
// graphChanged fires for plenty the minimap does not draw (zIndex from every
87+
// widget pointerdown, widget values, title edits), and the digest is the one
88+
// place that knows whether the picture moved.
89+
const checkAndRepaint = () => {
8890
if (!canDraw.value) return
89-
renderer.forceFullRedraw()
90-
renderer.updateMinimap(viewport.updateBounds, viewport.updateViewport)
91-
})
91+
if (graphManager.checkForChanges()) {
92+
renderer.updateMinimap(viewport.updateBounds, viewport.updateViewport)
93+
}
94+
}
95+
96+
const graphManager = useMinimapGraph(graph, () => checkAndRepaint())
9297

9398
// Rendering
9499
const renderer = useMinimapRenderer(
@@ -102,43 +107,41 @@ export function useMinimap({
102107
height
103108
)
104109

105-
// Most edits already force a redraw through useMinimapGraph's event hooks,
106-
// which cover node add/remove, connection changes and visual properties;
107-
// moves and resizes reach layoutStore and could be observed the same way.
108-
// This loop is the backstop for the writes that reach neither: snapPoint
109-
// mutating `_pos` elements in place, extensions assigning `node.pos[0]`
110-
// directly (only `size` is Proxy-wrapped against that), and `has_errors`,
111-
// which has no event at all. Subscribing to layoutStore changes and letting
112-
// the poll cover only that residue would allow a much longer period.
110+
// Most edits reach the digest comparison through useMinimapGraph's event
111+
// hooks. This loop is the backstop for state that emits no event at all:
112+
// snapPoint mutating `_pos` elements in place, extensions assigning
113+
// `node.pos[0]` directly (only `size` is Proxy-wrapped against that),
114+
// `has_errors`, group drags, and execution-state transitions, which arrive
115+
// in executionStore rather than as graph events. Repaints happen only when
116+
// the digests move, so idle ticks cost one O(n) comparison and nothing else.
113117
const { pause: pauseChangeDetection, resume: resumeChangeDetection } =
114-
useIntervalFn(
115-
() => {
116-
if (!canDraw.value) return
117-
if (graphManager.checkForChanges()) {
118-
renderer.updateMinimap(viewport.updateBounds, viewport.updateViewport)
119-
}
120-
},
121-
CHANGE_DETECTION_INTERVAL_MS,
122-
{ immediate: false }
123-
)
124-
125-
// rAF was suspended entirely on a hidden tab; setInterval is only clamped to
126-
// roughly 1s, so without this a parked tab keeps digesting the whole graph.
118+
useIntervalFn(checkAndRepaint, CHANGE_DETECTION_INTERVAL_MS, {
119+
immediate: false
120+
})
121+
122+
// Polling is derived state, not something init() decides once. init() runs
123+
// synchronously from the immediate canvas watcher - before the template ref
124+
// has mounted - so any start decision taken there sees canvasRef as null and
125+
// the poll never starts. Deriving from the refs means the loop starts the
126+
// moment the canvas mounts and stops the moment any condition lapses.
127127
//
128-
// `immediate` matters: a tab opened in the background (middle-click, session
129-
// restore) is already hidden at mount, so no visibilitychange ever fires and
130-
// a transition-only watcher would never pause the loop it just started.
128+
// Document visibility is part of the condition because rAF was suspended
129+
// entirely on a hidden tab while setInterval is only clamped, and a tab that
130+
// is hidden at mount (middle-click, session restore) never fires a
131+
// visibilitychange to a transition-only watcher.
131132
const documentVisibility = useDocumentVisibility()
132-
const documentHidden = computed(() => documentVisibility.value === 'hidden')
133-
134-
/** Both start paths consult this, so a hidden tab never starts polling. */
135-
const shouldPoll = () => canDraw.value && !documentHidden.value
133+
const shouldPoll = computed(
134+
() =>
135+
initialized.value &&
136+
canDraw.value &&
137+
documentVisibility.value !== 'hidden'
138+
)
136139

137140
watch(
138-
documentHidden,
139-
(hidden) => {
140-
if (hidden) pauseChangeDetection()
141-
else if (canDraw.value) resumeChangeDetection()
141+
shouldPoll,
142+
(active) => {
143+
if (active) resumeChangeDetection()
144+
else pauseChangeDetection()
142145
},
143146
{ immediate: true }
144147
)
@@ -164,7 +167,6 @@ export function useMinimap({
164167
renderer.updateMinimap(viewport.updateBounds, viewport.updateViewport)
165168
viewport.updateViewport()
166169

167-
if (shouldPoll()) resumeChangeDetection()
168170
if (visible.value) viewport.startViewportSync()
169171
initialized.value = true
170172
}
@@ -226,28 +228,12 @@ export function useMinimap({
226228

227229
renderer.updateMinimap(viewport.updateBounds, viewport.updateViewport)
228230
viewport.updateViewport()
229-
if (shouldPoll()) resumeChangeDetection()
230231
viewport.startViewportSync()
231232
} else {
232-
pauseChangeDetection()
233233
viewport.stopViewportSync()
234234
}
235235
})
236236

237-
const executionStore = useExecutionStore()
238-
watch(
239-
() => executionStore.nodeProgressStates,
240-
() => {
241-
// Fires per progress message during a run, so the canvas check matters
242-
// here more than anywhere else.
243-
if (canDraw.value) {
244-
renderer.forceFullRedraw()
245-
renderer.updateMinimap(viewport.updateBounds, viewport.updateViewport)
246-
}
247-
},
248-
{ deep: true }
249-
)
250-
251237
const toggle = async () => {
252238
visible.value = !visible.value
253239
await settingStore.set('Comfy.Minimap.Visible', visible.value)

src/renderer/extensions/minimap/composables/useMinimapGraph.test.ts

Lines changed: 66 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,13 @@ vi.mock('@vueuse/core', () => ({
2222
useThrottleFn: vi.fn((fn) => fn)
2323
}))
2424

25+
const { mockProgressStates } = vi.hoisted(() => ({
26+
mockProgressStates: {} as Record<string, { state: string }>
27+
}))
28+
2529
vi.mock('@/stores/executionStore', () => ({
2630
useExecutionStore: vi.fn(() => ({
27-
nodeProgressStates: {}
31+
nodeProgressStates: mockProgressStates
2832
}))
2933
}))
3034

@@ -54,6 +58,9 @@ describe('useMinimapGraph', () => {
5458
})
5559

5660
onGraphChangedMock = vi.fn()
61+
for (const key of Object.keys(mockProgressStates)) {
62+
delete mockProgressStates[key]
63+
}
5764
})
5865

5966
it('should initialize with empty state', () => {
@@ -399,6 +406,64 @@ describe('useMinimapGraph', () => {
399406
expect(graphManager.checkForChanges()).toBe(false)
400407
})
401408

409+
it('should detect a background colour change', () => {
410+
const graphRef = ref(mockGraph) as Ref<LGraph | null>
411+
const graphManager = useMinimapGraph(graphRef, onGraphChangedMock)
412+
413+
graphManager.checkForChanges()
414+
expect(graphManager.checkForChanges()).toBe(false)
415+
416+
mockGraph._nodes[0].bgcolor = '#ff0000'
417+
418+
expect(graphManager.checkForChanges()).toBe(true)
419+
})
420+
421+
it('should detect a group move', () => {
422+
mockGraph._groups = [
423+
{ pos: [0, 0], size: [400, 300], color: '#111111' }
424+
] as LGraph['_groups']
425+
426+
const graphRef = ref(mockGraph) as Ref<LGraph | null>
427+
const graphManager = useMinimapGraph(graphRef, onGraphChangedMock)
428+
429+
graphManager.checkForChanges()
430+
expect(graphManager.checkForChanges()).toBe(false)
431+
432+
mockGraph._groups[0].pos[0] = 250
433+
434+
expect(graphManager.checkForChanges()).toBe(true)
435+
})
436+
437+
it('should detect an execution-state transition', () => {
438+
const graphRef = ref(mockGraph) as Ref<LGraph | null>
439+
const graphManager = useMinimapGraph(graphRef, onGraphChangedMock)
440+
441+
graphManager.checkForChanges()
442+
expect(graphManager.checkForChanges()).toBe(false)
443+
444+
mockProgressStates['1'] = { state: 'running' }
445+
expect(graphManager.checkForChanges()).toBe(true)
446+
expect(graphManager.checkForChanges()).toBe(false)
447+
448+
mockProgressStates['1'] = { state: 'finished' }
449+
expect(graphManager.checkForChanges()).toBe(true)
450+
})
451+
452+
it('a visual-only change repaints without recomputing bounds', () => {
453+
const graphRef = ref(mockGraph) as Ref<LGraph | null>
454+
const graphManager = useMinimapGraph(graphRef, onGraphChangedMock)
455+
456+
graphManager.checkForChanges()
457+
graphManager.updateFlags.value.bounds = false
458+
graphManager.updateFlags.value.nodes = false
459+
460+
mockGraph._nodes[0].bgcolor = '#00ff00'
461+
graphManager.checkForChanges()
462+
463+
expect(graphManager.updateFlags.value.nodes).toBe(true)
464+
expect(graphManager.updateFlags.value.bounds).toBe(false)
465+
})
466+
402467
it('should detect an error-state change', () => {
403468
const graphRef = ref(mockGraph) as Ref<LGraph | null>
404469
const graphManager = useMinimapGraph(graphRef, onGraphChangedMock)

0 commit comments

Comments
 (0)