diff --git a/browser_tests/tests/minimap.spec.ts b/browser_tests/tests/minimap.spec.ts index c974906845..bc85819bb0 100644 --- a/browser_tests/tests/minimap.spec.ts +++ b/browser_tests/tests/minimap.spec.ts @@ -476,6 +476,42 @@ test.describe('Minimap', { tag: '@canvas' }, () => { }) .toBe(true) }) + + test('Closing minimap after subgraph navigation keeps Vue render in sync', async ({ + comfyPage + }) => { + await comfyPage.settings.setSetting('Comfy.VueNodes.Enabled', true) + await comfyPage.workflow.loadWorkflow('subgraphs/basic-subgraph') + await comfyPage.vueNodes.waitForNodes() + + const subgraphNodeId = await comfyPage.subgraph.findSubgraphNodeId() + + // Round-trip layers Vue's onNodeAdded wrapper on top of the minimap's. + await comfyPage.vueNodes.enterSubgraph(subgraphNodeId) + await comfyPage.subgraph.exitViaBreadcrumb() + + // Minimap unmount must not clobber the Vue wrapper layered above it. + await comfyPage.page.getByTestId(TestIds.canvas.closeMinimapButton).click() + + await comfyPage.page.evaluate((id) => { + const graph = window.app!.graph! + const subgraphNode = graph.getNodeById(id) + if (!subgraphNode?.isSubgraphNode()) { + throw new Error('expected subgraph node at root') + } + graph.unpackSubgraph(subgraphNode) + }, subgraphNodeId) + + await expect + .poll(() => + comfyPage.page.evaluate(() => window.app!.graph!.nodes.length) + ) + .toBe(2) + await expect.poll(() => comfyPage.vueNodes.getNodeCount()).toBe(2) + await expect(comfyPage.vueNodes.getNodeLocator(subgraphNodeId)).toHaveCount( + 0 + ) + }) }) test.describe('Minimap mobile', { tag: ['@mobile', '@canvas'] }, () => { diff --git a/src/renderer/extensions/minimap/composables/useMinimapGraph.test.ts b/src/renderer/extensions/minimap/composables/useMinimapGraph.test.ts index 10f9931e1a..550f88f802 100644 --- a/src/renderer/extensions/minimap/composables/useMinimapGraph.test.ts +++ b/src/renderer/extensions/minimap/composables/useMinimapGraph.test.ts @@ -159,6 +159,50 @@ describe('useMinimapGraph', () => { expect(() => graphManager.cleanupEventListeners()).not.toThrow() }) + it('cleanup leaves a later wrapper alone when one is layered on top', () => { + const graphRef = ref(mockGraph) as Ref + const graphManager = useMinimapGraph(graphRef, onGraphChangedMock) + + graphManager.setupEventListeners() + const minimapWrapper = mockGraph.onNodeAdded + + // Simulate another system ddding its own wrapper on top + const downstream = vi.fn() + const layeredWrapper = vi.fn(function (this: unknown, node: LGraphNode) { + minimapWrapper?.call(this, node) + downstream(node) + }) + mockGraph.onNodeAdded = layeredWrapper + + graphManager.cleanupEventListeners() + + // The newer wrapper must survive cleanup + expect(mockGraph.onNodeAdded).toBe(layeredWrapper) + }) + + it('a buried wrapper becomes inert after cleanup', () => { + const originalOnNodeAdded = vi.fn() + mockGraph.onNodeAdded = originalOnNodeAdded + + const graphRef = ref(mockGraph) as Ref + const graphManager = useMinimapGraph(graphRef, onGraphChangedMock) + + graphManager.setupEventListeners() + const buriedWrapper = mockGraph.onNodeAdded + + // Layer something on top so cleanup can't restore. + mockGraph.onNodeAdded = vi.fn() + graphManager.cleanupEventListeners() + vi.mocked(onGraphChangedMock).mockClear() + + // Call the method directly and ensure it is a no-op + const testNode = { id: '9' } as LGraphNode + buriedWrapper!(testNode) + + expect(originalOnNodeAdded).toHaveBeenCalledWith(testNode) + expect(onGraphChangedMock).not.toHaveBeenCalled() + }) + it('should detect node position changes', () => { const graphRef = ref(mockGraph) as Ref const graphManager = useMinimapGraph(graphRef, onGraphChangedMock) diff --git a/src/renderer/extensions/minimap/composables/useMinimapGraph.ts b/src/renderer/extensions/minimap/composables/useMinimapGraph.ts index 9d7b620d38..141950085a 100644 --- a/src/renderer/extensions/minimap/composables/useMinimapGraph.ts +++ b/src/renderer/extensions/minimap/composables/useMinimapGraph.ts @@ -38,8 +38,13 @@ export function useMinimapGraph( // Track LayoutStore version for change detection const layoutStoreVersion = layoutStore.getVersion() - // Map to store original callbacks per graph ID - const originalCallbacksMap = new Map() + // Cleanup restores originals only when our wrapper is still on top, and + // marks any buried wrapper inert via `isLive()` so it can't fire dead work. + interface InstalledHooks { + originals: GraphCallbacks + wrappers: GraphCallbacks + } + const hooksMap = new Map() const handleGraphChangedThrottled = useThrottleFn(() => { onGraphChanged() @@ -47,40 +52,44 @@ export function useMinimapGraph( const setupEventListeners = () => { const g = graph.value - if (!g) return + if (!g || hooksMap.has(g.id)) return - // Check if we've already wrapped this graph's callbacks - if (originalCallbacksMap.has(g.id)) { - return - } - - // Store the original callbacks for this graph - const originalCallbacks: GraphCallbacks = { + const originals: GraphCallbacks = { onNodeAdded: g.onNodeAdded, onNodeRemoved: g.onNodeRemoved, onConnectionChange: g.onConnectionChange, onTrigger: g.onTrigger } - originalCallbacksMap.set(g.id, originalCallbacks) + const wrappers: GraphCallbacks = {} + const entry: InstalledHooks = { originals, wrappers } + hooksMap.set(g.id, entry) + const isLive = () => hooksMap.get(g.id) === entry - g.onNodeAdded = function (node: LGraphNode) { - originalCallbacks.onNodeAdded?.call(this, node) + wrappers.onNodeAdded = function (node: LGraphNode) { + originals.onNodeAdded?.call(this, node) + if (!isLive()) return void handleGraphChangedThrottled() } + g.onNodeAdded = wrappers.onNodeAdded - g.onNodeRemoved = function (node: LGraphNode) { - originalCallbacks.onNodeRemoved?.call(this, node) + wrappers.onNodeRemoved = function (node: LGraphNode) { + originals.onNodeRemoved?.call(this, node) + if (!isLive()) return nodeStatesCache.delete(node.id) void handleGraphChangedThrottled() } + g.onNodeRemoved = wrappers.onNodeRemoved - g.onConnectionChange = function (node: LGraphNode) { - originalCallbacks.onConnectionChange?.call(this, node) + wrappers.onConnectionChange = function (node: LGraphNode) { + originals.onConnectionChange?.call(this, node) + if (!isLive()) return void handleGraphChangedThrottled() } + g.onConnectionChange = wrappers.onConnectionChange - g.onTrigger = function (event: LGraphTriggerEvent) { - originalCallbacks.onTrigger?.call(this, event) + wrappers.onTrigger = function (event: LGraphTriggerEvent) { + originals.onTrigger?.call(this, event) + if (!isLive()) return // Listen for visual property changes that affect minimap rendering if ( @@ -94,24 +103,25 @@ export function useMinimapGraph( void handleGraphChangedThrottled() } } + g.onTrigger = wrappers.onTrigger } const cleanupEventListeners = (oldGraph?: LGraph) => { const g = oldGraph || graph.value if (!g) return + const entry = hooksMap.get(g.id) + if (!entry) return + const { originals, wrappers } = entry - const originalCallbacks = originalCallbacksMap.get(g.id) - if (!originalCallbacks) { - // Graph was never set up (e.g., minimap destroyed before init) - nothing to clean up - return - } + if (g.onNodeAdded === wrappers.onNodeAdded) + g.onNodeAdded = originals.onNodeAdded + if (g.onNodeRemoved === wrappers.onNodeRemoved) + g.onNodeRemoved = originals.onNodeRemoved + if (g.onConnectionChange === wrappers.onConnectionChange) + g.onConnectionChange = originals.onConnectionChange + if (g.onTrigger === wrappers.onTrigger) g.onTrigger = originals.onTrigger - g.onNodeAdded = originalCallbacks.onNodeAdded - g.onNodeRemoved = originalCallbacks.onNodeRemoved - g.onConnectionChange = originalCallbacks.onConnectionChange - g.onTrigger = originalCallbacks.onTrigger - - originalCallbacksMap.delete(g.id) + hooksMap.delete(g.id) } const checkForChangesInternal = () => {