From 1280d4110dfdccecae9391a5dc475f08cfde89e8 Mon Sep 17 00:00:00 2001 From: Alexander Brown Date: Fri, 13 Mar 2026 08:43:18 -0700 Subject: [PATCH] fix: simplify ensureCorrectLayoutScale and fix link sync during Vue node drag (#9680) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fix node layout drift from repeated `ensureCorrectLayoutScale` scaling, simplify it to a pure one-time normalizer, and fix links not following Vue nodes during drag. ## Changes - **What**: - `ensureCorrectLayoutScale` simplified to a one-time normalizer: unprojects legacy Vue-scaled coordinates back to canonical LiteGraph coordinates, marks the graph as corrected, and does nothing else. No longer touches the layout store, syncs reroutes, or changes canvas scale. - Removed no-op calls from `useVueNodeLifecycle.ts` (a renderer version string was passed where an `LGraph` was expected). - `layoutStore.finalizeOperation` now calls `notifyChange` synchronously instead of via `setTimeout`. This ensures `useLayoutSync`'s `onChange` callback pushes positions to LiteGraph `node.pos` and calls `canvas.setDirty()` within the same RAF frame as a drag update, fixing links not following Vue nodes during drag. - **Tests**: Added tests for `ensureCorrectLayoutScale` (idempotency, round-trip, unknown-renderer no-op) and `graphRenderTransform` (project/unproject round-trips, anchor caching). ## Review Focus - The `setTimeout(() => this.notifyChange(change), 0)` → `this.notifyChange(change)` change in `layoutStore.ts` is the key fix for the drag-link-sync bug. The listener (`useLayoutSync`) only writes to LiteGraph, not back to the layout store, so synchronous notification is safe. - `ensureCorrectLayoutScale` no longer has any side effects beyond normalizing coordinates and setting `workflowRendererVersion` metadata. --------- Co-authored-by: Amp Co-authored-by: Christian Byrne Co-authored-by: jaeone94 <89377375+jaeone94@users.noreply.github.com> Co-authored-by: AustinMroz Co-authored-by: Hunter Co-authored-by: GitHub Action --- .../tests/rendererToggleStability.spec.ts | 104 +++++++ src/components/TopMenuSection.test.ts | 85 +++++ src/components/TopMenuSection.vue | 26 +- src/composables/graph/useVueNodeLifecycle.ts | 27 +- src/lib/litegraph/src/LGraph.ts | 2 +- .../validation/schemas/workflowSchema.ts | 6 +- .../core/layout/operations/layoutMutations.ts | 29 ++ .../core/layout/store/layoutStore.test.ts | 193 +++++++++++- src/renderer/core/layout/store/layoutStore.ts | 73 ++++- .../core/layout/sync/useLayoutSync.test.ts | 172 +++++++++++ .../core/layout/sync/useLayoutSync.ts | 125 ++++++-- .../transform/graphRenderTransform.test.ts | 84 +++++ .../layout/transform/graphRenderTransform.ts | 52 ++++ src/renderer/core/layout/types.ts | 4 + .../vueNodes/components/LGraphNode.vue | 33 +- .../useSlotElementTracking.test.ts | 41 +++ .../composables/useSlotElementTracking.ts | 77 +++-- .../useVueNodeResizeTracking.test.ts | 290 ++++++++++++++++++ .../composables/useVueNodeResizeTracking.ts | 104 ++++++- .../interactions/resize/useNodeResize.test.ts | 15 +- .../interactions/resize/useNodeResize.ts | 3 +- .../layout/ensureCorrectLayoutScale.test.ts | 271 ++++++++++++++++ .../layout/ensureCorrectLayoutScale.ts | 221 +++++-------- .../vueNodes/layout/useNodeDrag.test.ts | 234 ++++++++++++++ .../extensions/vueNodes/layout/useNodeDrag.ts | 10 +- .../form/dropdown/FormDropdown.test.ts | 109 +++++-- .../components/form/dropdown/FormDropdown.vue | 14 +- src/scripts/app.ts | 2 +- 28 files changed, 2108 insertions(+), 298 deletions(-) create mode 100644 browser_tests/tests/rendererToggleStability.spec.ts create mode 100644 src/renderer/core/layout/sync/useLayoutSync.test.ts create mode 100644 src/renderer/core/layout/transform/graphRenderTransform.test.ts create mode 100644 src/renderer/core/layout/transform/graphRenderTransform.ts create mode 100644 src/renderer/extensions/vueNodes/composables/useVueNodeResizeTracking.test.ts create mode 100644 src/renderer/extensions/vueNodes/layout/ensureCorrectLayoutScale.test.ts create mode 100644 src/renderer/extensions/vueNodes/layout/useNodeDrag.test.ts diff --git a/browser_tests/tests/rendererToggleStability.spec.ts b/browser_tests/tests/rendererToggleStability.spec.ts new file mode 100644 index 0000000000..8bdb673d8a --- /dev/null +++ b/browser_tests/tests/rendererToggleStability.spec.ts @@ -0,0 +1,104 @@ +import { + comfyExpect as expect, + comfyPageFixture as test +} from '../fixtures/ComfyPage' +import type { ComfyPage } from '../fixtures/ComfyPage' +import type { Position } from '../fixtures/types' + +type NodeSnapshot = { id: number } & Position + +async function getAllNodePositions( + comfyPage: ComfyPage +): Promise { + return comfyPage.page.evaluate(() => + window.app!.graph.nodes.map((n) => ({ + id: n.id as number, + x: n.pos[0], + y: n.pos[1] + })) + ) +} + +async function getNodePosition( + comfyPage: ComfyPage, + nodeId: number +): Promise { + return comfyPage.page.evaluate((targetNodeId) => { + const node = window.app!.graph.nodes.find((n) => n.id === targetNodeId) + if (!node) return + + return { + x: node.pos[0], + y: node.pos[1] + } + }, nodeId) +} + +async function expectNodePositionStable( + comfyPage: ComfyPage, + initial: NodeSnapshot, + mode: string +) { + await expect + .poll( + async () => { + const current = await getNodePosition(comfyPage, initial.id) + return current?.x ?? Number.NaN + }, + { message: `node ${initial.id} x drifted in ${mode} mode` } + ) + .toBeCloseTo(initial.x, 1) + + await expect + .poll( + async () => { + const current = await getNodePosition(comfyPage, initial.id) + return current?.y ?? Number.NaN + }, + { message: `node ${initial.id} y drifted in ${mode} mode` } + ) + .toBeCloseTo(initial.y, 1) +} + +async function setVueMode(comfyPage: ComfyPage, enabled: boolean) { + await comfyPage.settings.setSetting('Comfy.VueNodes.Enabled', enabled) + if (enabled) { + await comfyPage.vueNodes.waitForNodes() + } + await comfyPage.nextFrame() +} + +test.describe( + 'Renderer toggle stability', + { tag: ['@node', '@canvas'] }, + () => { + test('node positions do not drift when toggling between Vue and LiteGraph renderers', async ({ + comfyPage + }) => { + const TOGGLE_COUNT = 5 + + const initialPositions = await getAllNodePositions(comfyPage) + expect(initialPositions.length).toBeGreaterThan(0) + + for (let i = 0; i < TOGGLE_COUNT; i++) { + await setVueMode(comfyPage, true) + for (const initial of initialPositions) { + await expectNodePositionStable( + comfyPage, + initial, + `Vue toggle ${i + 1}` + ) + } + + await setVueMode(comfyPage, false) + for (const initial of initialPositions) { + await expectNodePositionStable( + comfyPage, + initial, + `LiteGraph toggle ${i + 1}` + ) + } + } + }) + } +) diff --git a/src/components/TopMenuSection.test.ts b/src/components/TopMenuSection.test.ts index ac626a400b..c734ae9490 100644 --- a/src/components/TopMenuSection.test.ts +++ b/src/components/TopMenuSection.test.ts @@ -78,6 +78,14 @@ vi.mock('@/stores/firebaseAuthStore', () => ({ })) })) +vi.mock('@/scripts/app', () => ({ + app: { + menu: { + element: document.createElement('div') + } + } +})) + type WrapperOptions = { pinia?: ReturnType stubs?: Record @@ -131,6 +139,18 @@ function createWrapper({ }) } +function getLegacyCommandsContainer( + wrapper: ReturnType +): HTMLElement { + const legacyContainer = wrapper.find( + '[data-testid="legacy-topbar-container"]' + ).element + if (!(legacyContainer instanceof HTMLElement)) { + throw new Error('Expected legacy commands container to be present') + } + return legacyContainer +} + function createJob(id: string, status: JobStatus): JobListItem { return { id, @@ -515,4 +535,69 @@ describe('TopMenuSection', () => { expect(wrapper.find('span.bg-red-500').exists()).toBe(true) }) + + it('coalesces legacy topbar mutation scans to one check per frame', async () => { + localStorage.setItem('Comfy.MenuPosition.Docked', 'false') + + const rafCallbacks: FrameRequestCallback[] = [] + vi.stubGlobal('requestAnimationFrame', (cb: FrameRequestCallback) => { + rafCallbacks.push(cb) + return rafCallbacks.length + }) + vi.stubGlobal('cancelAnimationFrame', vi.fn()) + + const pinia = createTestingPinia({ createSpy: vi.fn }) + const settingStore = useSettingStore(pinia) + vi.mocked(settingStore.get).mockImplementation((key) => { + if (key === 'Comfy.UseNewMenu') return 'Top' + if (key === 'Comfy.UI.TabBarLayout') return 'Integrated' + if (key === 'Comfy.RightSidePanel.IsOpen') return true + return undefined + }) + + const wrapper = createWrapper({ pinia, attachTo: document.body }) + + try { + await nextTick() + + const actionbarContainer = wrapper.find('.actionbar-container') + expect(actionbarContainer.classes()).toContain('w-0') + + const legacyContainer = getLegacyCommandsContainer(wrapper) + const querySpy = vi.spyOn(legacyContainer, 'querySelector') + + if (rafCallbacks.length > 0) { + const initialCallbacks = [...rafCallbacks] + rafCallbacks.length = 0 + initialCallbacks.forEach((callback) => callback(0)) + await nextTick() + } + querySpy.mockClear() + querySpy.mockReturnValue(document.createElement('div')) + + for (let index = 0; index < 3; index++) { + const outer = document.createElement('div') + const inner = document.createElement('div') + inner.textContent = `legacy-${index}` + outer.appendChild(inner) + legacyContainer.appendChild(outer) + } + + await vi.waitFor(() => { + expect(rafCallbacks.length).toBeGreaterThan(0) + }) + expect(querySpy).not.toHaveBeenCalled() + + const callbacks = [...rafCallbacks] + rafCallbacks.length = 0 + callbacks.forEach((callback) => callback(0)) + await nextTick() + + expect(querySpy).toHaveBeenCalledTimes(1) + expect(actionbarContainer.classes()).toContain('px-2') + } finally { + wrapper.unmount() + vi.unstubAllGlobals() + } + }) }) diff --git a/src/components/TopMenuSection.vue b/src/components/TopMenuSection.vue index 5994be4da4..81ff088dca 100644 --- a/src/components/TopMenuSection.vue +++ b/src/components/TopMenuSection.vue @@ -39,6 +39,7 @@
@@ -116,7 +117,7 @@