mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-29 18:52:19 +00:00
Resolve issues with undo with Nodes 2.0 to fix link dragging/rendering (#8808)
## Summary Resolves the following issue: 1. Enable Nodes 2.0 2. Load default workflow 3. Move any node e.g. VAE decode 4. Undo All links go invisible, input/output slots no longer function ## Changes - **What** - Fixes slot layouts being deleted during undo/redo in `handleDeleteNode`, which prevented link dragging from nodes after undo. Vue patches (not remounts) components with the same key, so `onMounted` never fires to re-register them - these were already being cleared up on unmounted - Fixes links disappearing after undo by clearing `pendingSlotSync` when slot layouts already exist (undo/redo preserved them), rather than waiting for Vue mounts that do not happen ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8808-Resolve-issues-with-undo-with-Nodes-2-0-to-fix-link-dragging-rendering-3046d73d3650818bbb0adf0104a5792d) by [Unito](https://www.unito.io)
This commit is contained in:
@@ -1,9 +1,14 @@
|
|||||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||||
|
|
||||||
import { LiteGraph } from '@/lib/litegraph/src/litegraph'
|
import { LiteGraph } from '@/lib/litegraph/src/litegraph'
|
||||||
|
import { getSlotKey } from '@/renderer/core/layout/slots/slotIdentifier'
|
||||||
import { layoutStore } from '@/renderer/core/layout/store/layoutStore'
|
import { layoutStore } from '@/renderer/core/layout/store/layoutStore'
|
||||||
import { LayoutSource } from '@/renderer/core/layout/types'
|
import { LayoutSource } from '@/renderer/core/layout/types'
|
||||||
import type { LayoutChange, NodeLayout } from '@/renderer/core/layout/types'
|
import type {
|
||||||
|
LayoutChange,
|
||||||
|
NodeLayout,
|
||||||
|
SlotLayout
|
||||||
|
} from '@/renderer/core/layout/types'
|
||||||
|
|
||||||
describe('layoutStore CRDT operations', () => {
|
describe('layoutStore CRDT operations', () => {
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
@@ -406,4 +411,49 @@ describe('layoutStore CRDT operations', () => {
|
|||||||
LiteGraph.NODE_TITLE_HEIGHT = originalTitleHeight
|
LiteGraph.NODE_TITLE_HEIGHT = originalTitleHeight
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it.for([
|
||||||
|
{ type: 'input' as const, isInput: true },
|
||||||
|
{ type: 'output' as const, isInput: false }
|
||||||
|
])(
|
||||||
|
'should preserve $type slot layouts when deleting a node',
|
||||||
|
({ type, isInput }) => {
|
||||||
|
const nodeId = 'slot-persist-node'
|
||||||
|
const layout = createTestNode(nodeId)
|
||||||
|
|
||||||
|
layoutStore.applyOperation({
|
||||||
|
type: 'createNode',
|
||||||
|
entity: 'node',
|
||||||
|
nodeId,
|
||||||
|
layout,
|
||||||
|
timestamp: Date.now(),
|
||||||
|
source: LayoutSource.External,
|
||||||
|
actor: 'test'
|
||||||
|
})
|
||||||
|
|
||||||
|
const slotKey = getSlotKey(nodeId, 0, isInput)
|
||||||
|
const slotLayout: SlotLayout = {
|
||||||
|
nodeId,
|
||||||
|
index: 0,
|
||||||
|
type,
|
||||||
|
position: { x: 110, y: 120 },
|
||||||
|
bounds: { x: 105, y: 115, width: 10, height: 10 }
|
||||||
|
}
|
||||||
|
layoutStore.batchUpdateSlotLayouts([{ key: slotKey, layout: slotLayout }])
|
||||||
|
expect(layoutStore.getSlotLayout(slotKey)).toEqual(slotLayout)
|
||||||
|
|
||||||
|
layoutStore.applyOperation({
|
||||||
|
type: 'deleteNode',
|
||||||
|
entity: 'node',
|
||||||
|
nodeId,
|
||||||
|
previousLayout: layout,
|
||||||
|
timestamp: Date.now(),
|
||||||
|
source: LayoutSource.External,
|
||||||
|
actor: 'test'
|
||||||
|
})
|
||||||
|
|
||||||
|
// Slot layout must survive so Vue-patched components can still drag links
|
||||||
|
expect(layoutStore.getSlotLayout(slotKey)).toEqual(slotLayout)
|
||||||
|
}
|
||||||
|
)
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -151,6 +151,10 @@ class LayoutStoreImpl implements LayoutStore {
|
|||||||
return this._pendingSlotSync
|
return this._pendingSlotSync
|
||||||
}
|
}
|
||||||
|
|
||||||
|
get hasSlotLayouts(): boolean {
|
||||||
|
return this.slotLayouts.size > 0
|
||||||
|
}
|
||||||
|
|
||||||
setPendingSlotSync(value: boolean): void {
|
setPendingSlotSync(value: boolean): void {
|
||||||
this._pendingSlotSync = value
|
this._pendingSlotSync = value
|
||||||
}
|
}
|
||||||
@@ -513,23 +517,6 @@ class LayoutStoreImpl implements LayoutStore {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* Delete all slot layouts for a node
|
|
||||||
*/
|
|
||||||
deleteNodeSlotLayouts(nodeId: NodeId): void {
|
|
||||||
const keysToDelete: string[] = []
|
|
||||||
for (const [key, layout] of this.slotLayouts) {
|
|
||||||
if (layout.nodeId === nodeId) {
|
|
||||||
keysToDelete.push(key)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
for (const key of keysToDelete) {
|
|
||||||
this.slotLayouts.delete(key)
|
|
||||||
// Remove from spatial index
|
|
||||||
this.slotSpatialIndex.remove(key)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Clear all slot layouts and their spatial index (O(1) operations)
|
* Clear all slot layouts and their spatial index (O(1) operations)
|
||||||
* Used when switching rendering modes (Vue ↔ LiteGraph)
|
* Used when switching rendering modes (Vue ↔ LiteGraph)
|
||||||
@@ -1117,13 +1104,10 @@ class LayoutStoreImpl implements LayoutStore {
|
|||||||
// During undo/redo, Vue components may still hold references to the old ref.
|
// During undo/redo, Vue components may still hold references to the old ref.
|
||||||
// If we delete the trigger, Vue won't be notified when the node is re-created.
|
// If we delete the trigger, Vue won't be notified when the node is re-created.
|
||||||
// The trigger will be called in finalizeOperation to notify Vue of the change.
|
// The trigger will be called in finalizeOperation to notify Vue of the change.
|
||||||
|
// We also intentionally do NOT delete slot layouts here for the same reason,
|
||||||
|
// and cleanup is handled by onUnmounted in useSlotElementTracking.
|
||||||
// Remove from spatial index
|
// Remove from spatial index
|
||||||
this.spatialIndex.remove(operation.nodeId)
|
this.spatialIndex.remove(operation.nodeId)
|
||||||
|
|
||||||
// Clean up associated slot layouts
|
|
||||||
this.deleteNodeSlotLayouts(operation.nodeId)
|
|
||||||
|
|
||||||
// Clean up associated links
|
// Clean up associated links
|
||||||
const linksToDelete = this.findLinksConnectedToNode(operation.nodeId)
|
const linksToDelete = this.findLinksConnectedToNode(operation.nodeId)
|
||||||
|
|
||||||
|
|||||||
@@ -302,7 +302,6 @@ export interface LayoutStore {
|
|||||||
deleteLinkLayout(linkId: LinkId): void
|
deleteLinkLayout(linkId: LinkId): void
|
||||||
deleteLinkSegmentLayout(linkId: LinkId, rerouteId: RerouteId | null): void
|
deleteLinkSegmentLayout(linkId: LinkId, rerouteId: RerouteId | null): void
|
||||||
deleteSlotLayout(key: string): void
|
deleteSlotLayout(key: string): void
|
||||||
deleteNodeSlotLayouts(nodeId: NodeId): void
|
|
||||||
deleteRerouteLayout(rerouteId: RerouteId): void
|
deleteRerouteLayout(rerouteId: RerouteId): void
|
||||||
clearAllSlotLayouts(): void
|
clearAllSlotLayouts(): void
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,134 @@
|
|||||||
|
import { mount } from '@vue/test-utils'
|
||||||
|
import { createTestingPinia } from '@pinia/testing'
|
||||||
|
import { setActivePinia } from 'pinia'
|
||||||
|
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||||
|
import { defineComponent, nextTick, ref } from 'vue'
|
||||||
|
|
||||||
|
import { getSlotKey } from '@/renderer/core/layout/slots/slotIdentifier'
|
||||||
|
import { layoutStore } from '@/renderer/core/layout/store/layoutStore'
|
||||||
|
import { LayoutSource } from '@/renderer/core/layout/types'
|
||||||
|
import type { SlotLayout } from '@/renderer/core/layout/types'
|
||||||
|
import { useNodeSlotRegistryStore } from '@/renderer/extensions/vueNodes/stores/nodeSlotRegistryStore'
|
||||||
|
|
||||||
|
import {
|
||||||
|
flushScheduledSlotLayoutSync,
|
||||||
|
useSlotElementTracking
|
||||||
|
} from './useSlotElementTracking'
|
||||||
|
|
||||||
|
const mockGraph = vi.hoisted(() => ({ _nodes: [] as unknown[] }))
|
||||||
|
|
||||||
|
vi.mock('@/scripts/app', () => ({
|
||||||
|
app: { canvas: { graph: mockGraph, setDirty: vi.fn() } }
|
||||||
|
}))
|
||||||
|
|
||||||
|
vi.mock('@/composables/element/useCanvasPositionConversion', () => ({
|
||||||
|
useSharedCanvasPositionConversion: () => ({
|
||||||
|
clientPosToCanvasPos: ([x, y]: [number, number]) => [x, y]
|
||||||
|
})
|
||||||
|
}))
|
||||||
|
|
||||||
|
const NODE_ID = 'test-node'
|
||||||
|
const SLOT_INDEX = 0
|
||||||
|
|
||||||
|
function createWrapperComponent(type: 'input' | 'output') {
|
||||||
|
return defineComponent({
|
||||||
|
setup() {
|
||||||
|
const el = ref<HTMLElement | null>(null)
|
||||||
|
useSlotElementTracking({
|
||||||
|
nodeId: NODE_ID,
|
||||||
|
index: SLOT_INDEX,
|
||||||
|
type,
|
||||||
|
element: el
|
||||||
|
})
|
||||||
|
return { el }
|
||||||
|
},
|
||||||
|
// No template ref — el starts null so the immediate watcher doesn't fire
|
||||||
|
// before the stop handle is assigned
|
||||||
|
template: '<div />'
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Mount the wrapper, set the element ref, and wait for slot registration.
|
||||||
|
*/
|
||||||
|
async function mountAndRegisterSlot(type: 'input' | 'output') {
|
||||||
|
const wrapper = mount(createWrapperComponent(type))
|
||||||
|
wrapper.vm.el = document.createElement('div')
|
||||||
|
await nextTick()
|
||||||
|
flushScheduledSlotLayoutSync()
|
||||||
|
return wrapper
|
||||||
|
}
|
||||||
|
|
||||||
|
describe('useSlotElementTracking', () => {
|
||||||
|
beforeEach(() => {
|
||||||
|
setActivePinia(createTestingPinia({ stubActions: false }))
|
||||||
|
layoutStore.initializeFromLiteGraph([])
|
||||||
|
layoutStore.applyOperation({
|
||||||
|
type: 'createNode',
|
||||||
|
entity: 'node',
|
||||||
|
nodeId: NODE_ID,
|
||||||
|
layout: {
|
||||||
|
id: NODE_ID,
|
||||||
|
position: { x: 0, y: 0 },
|
||||||
|
size: { width: 200, height: 100 },
|
||||||
|
zIndex: 0,
|
||||||
|
visible: true,
|
||||||
|
bounds: { x: 0, y: 0, width: 200, height: 100 }
|
||||||
|
},
|
||||||
|
timestamp: Date.now(),
|
||||||
|
source: LayoutSource.External,
|
||||||
|
actor: 'test'
|
||||||
|
})
|
||||||
|
mockGraph._nodes = [{ id: 1 }]
|
||||||
|
})
|
||||||
|
|
||||||
|
it.each([
|
||||||
|
{ type: 'input' as const, isInput: true },
|
||||||
|
{ type: 'output' as const, isInput: false }
|
||||||
|
])('cleans up $type slot layout on unmount', async ({ type, isInput }) => {
|
||||||
|
const wrapper = await mountAndRegisterSlot(type)
|
||||||
|
|
||||||
|
const slotKey = getSlotKey(NODE_ID, SLOT_INDEX, isInput)
|
||||||
|
const registryStore = useNodeSlotRegistryStore()
|
||||||
|
expect(registryStore.getNode(NODE_ID)?.slots.has(slotKey)).toBe(true)
|
||||||
|
expect(layoutStore.getSlotLayout(slotKey)).not.toBeNull()
|
||||||
|
|
||||||
|
wrapper.unmount()
|
||||||
|
|
||||||
|
expect(layoutStore.getSlotLayout(slotKey)).toBeNull()
|
||||||
|
expect(registryStore.getNode(NODE_ID)).toBeUndefined()
|
||||||
|
})
|
||||||
|
|
||||||
|
it('clears pendingSlotSync when slot layouts already exist', () => {
|
||||||
|
// Seed a slot layout (simulates slot layouts persisting through undo/redo)
|
||||||
|
const slotKey = getSlotKey(NODE_ID, SLOT_INDEX, true)
|
||||||
|
const slotLayout: SlotLayout = {
|
||||||
|
nodeId: NODE_ID,
|
||||||
|
index: 0,
|
||||||
|
type: 'input',
|
||||||
|
position: { x: 0, y: 0 },
|
||||||
|
bounds: { x: 0, y: 0, width: 10, height: 10 }
|
||||||
|
}
|
||||||
|
layoutStore.batchUpdateSlotLayouts([{ key: slotKey, layout: slotLayout }])
|
||||||
|
|
||||||
|
// Simulate what app.ts onConfigure does: set pending, then flush
|
||||||
|
layoutStore.setPendingSlotSync(true)
|
||||||
|
expect(layoutStore.pendingSlotSync).toBe(true)
|
||||||
|
|
||||||
|
// No slots were scheduled (undo/redo — onMounted didn't fire),
|
||||||
|
// but slot layouts already exist. Flush should clear the flag.
|
||||||
|
flushScheduledSlotLayoutSync()
|
||||||
|
|
||||||
|
expect(layoutStore.pendingSlotSync).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
|
it('keeps pendingSlotSync when graph has nodes but no slot layouts', () => {
|
||||||
|
// No slot layouts exist (simulates initial mount before Vue registers slots)
|
||||||
|
layoutStore.setPendingSlotSync(true)
|
||||||
|
|
||||||
|
flushScheduledSlotLayoutSync()
|
||||||
|
|
||||||
|
// Should remain pending — waiting for Vue components to mount
|
||||||
|
expect(layoutStore.pendingSlotSync).toBe(true)
|
||||||
|
})
|
||||||
|
})
|
||||||
@@ -37,12 +37,13 @@ export function flushScheduledSlotLayoutSync() {
|
|||||||
// No pending nodes - check if we should wait for Vue components to mount
|
// No pending nodes - check if we should wait for Vue components to mount
|
||||||
const graph = app.canvas?.graph
|
const graph = app.canvas?.graph
|
||||||
const hasNodes = graph && graph._nodes && graph._nodes.length > 0
|
const hasNodes = graph && graph._nodes && graph._nodes.length > 0
|
||||||
if (hasNodes) {
|
if (hasNodes && !layoutStore.hasSlotLayouts) {
|
||||||
// Graph has nodes but Vue hasn't mounted them yet - keep flag set
|
// Graph has nodes but no slot layouts yet - Vue hasn't mounted.
|
||||||
// so late mounts can re-assert it via scheduleSlotLayoutSync()
|
// Keep flag set so late mounts can re-assert via scheduleSlotLayoutSync()
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
// No nodes in graph - safe to clear the flag (no Vue components will mount)
|
// Either no nodes (nothing to wait for) or slot layouts already exist
|
||||||
|
// (undo/redo preserved them). Clear the flag so links can render.
|
||||||
layoutStore.setPendingSlotSync(false)
|
layoutStore.setPendingSlotSync(false)
|
||||||
app.canvas?.setDirty(true, true)
|
app.canvas?.setDirty(true, true)
|
||||||
return
|
return
|
||||||
|
|||||||
Reference in New Issue
Block a user