mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-20 14:30:41 +00:00
## Summary Fix subgraph internal node positions being permanently corrupted when entering a subgraph after a draft workflow reload. The corruption accumulated across page refreshes, causing nodes to progressively drift apart or compress together. ## Changes - **What**: In the `ResizeObserver` callback (`useVueNodeResizeTracking.ts`), node positions are now read from the Layout Store (source of truth, initialized from LiteGraph) instead of reverse-converting DOM screen coordinates via `getBoundingClientRect()` + `clientPosToCanvasPos()`. The fallback to DOM-based conversion is retained only for nodes not yet present in the Layout Store. ## Root Cause `ResizeObserver` was using `getBoundingClientRect()` to get DOM element positions, then converting them to canvas coordinates via `clientPosToCanvasPos()`. This conversion depends on the current `canvas.ds.scale` and `canvas.ds.offset`. During graph transitions (e.g., entering a subgraph from a draft-loaded workflow), the canvas viewport was stale — it still had the **parent graph's zoom level** because `fitView()` hadn't run yet (it's scheduled via `requestAnimationFrame`). The `ResizeObserver` callback fired before `fitView`, converting DOM positions using the wrong scale/offset, and writing the corrupted positions to the Layout Store. The `useLayoutSync` writeback then permanently overwrote the LiteGraph node positions. The corruption accumulated across sessions: 1. Load workflow → enter subgraph → `ResizeObserver` writes corrupted positions 2. Draft auto-saves the corrupted positions to localStorage 3. Page refresh → draft loads with corrupted positions → enter subgraph → positions corrupted further 4. Each cycle amplifies the drift based on the parent graph's zoom level This is the same class of bug that PR #9121 fixed for **slot** positions — the DOM→canvas coordinate conversion is inherently fragile during viewport transitions. This PR applies the same principle to **node** positions. ## Why This Only Affects `main` (No Backport Needed) This bug requires two features that only exist on `main`, not on `core/1.41` or `core/1.42`: 1. **PR #10247** changed `subgraphNavigationStore`'s watcher to `flush: 'sync'` and added `requestAnimationFrame(fitView)` on viewport cache miss. This creates the timing window where `ResizeObserver` fires before `fitView` corrects the canvas scale. 2. **PR #6811** added hash-based subgraph auto-entry on page load, which triggers graph transitions during the draft reload flow. On 1.41/1.42, `restoreViewport` does nothing on cache miss (no `fitView` scheduling), and the watcher uses default async flush — so the `ResizeObserver` never runs with a stale viewport. ## Review Focus - The core change is small: use `nodeLayout.position` (already in the Layout Store from `initializeFromLiteGraph`) instead of computing position from `getBoundingClientRect()`. This eliminates the dependency on canvas scale/offset being up-to-date during `ResizeObserver` callbacks. - The fallback path (`getBoundingClientRect` → `clientPosToCanvasPos`) is retained for nodes not yet in the Layout Store (e.g., first render of a newly created node). At that point the canvas transform is stable, so the conversion is safe. - Unit tests updated to reflect that position is no longer overwritten from DOM when Layout Store already has the position. - E2E test added: load subgraph workflow → enter subgraph → reload (draft) → verify positions preserved. ## E2E Test Fixes - `subgraphDraftPositions.spec.ts`: replaced `comfyPage.setup({ clearStorage: false })` with `page.reload()` + explicit draft persistence polling. The `setup()` method performs a full navigation via `goto()` which bypassed the draft auto-load flow. - `SubgraphHelper.packAllInteriorNodes`: replaced `canvas.click()` with `dispatchEvent('pointerdown'/'pointerup')`. The position fix places subgraph nodes at their correct locations, which now overlap with DOM widget textareas that intercept pointer events. ## Test Plan - [x] Unit tests pass (`useVueNodeResizeTracking.test.ts`) - [x] E2E test: `subgraphDraftPositions.spec.ts` — draft reload preserves subgraph node positions - [x] Manual: load workflow with subgraph, zoom in/out on root graph, enter subgraph, verify no position drift - [x] Manual: repeat with page refresh (draft reload) — positions should be stable across reloads - [x] Manual: drag nodes inside subgraph — positions should update correctly - [x] Manual: create new node inside subgraph — position should be set correctly (fallback path) ## Screenshots Before <img width="1331" height="879" alt="스크린샷 2026-04-03 오전 3 56 48" src="https://github.com/user-attachments/assets/377d1b2e-6d47-4884-8181-920e22fa6541" /> After <img width="1282" height="715" alt="스크린샷 2026-04-03 오전 3 58 24" src="https://github.com/user-attachments/assets/34528f6c-0225-4538-9383-227c849bccad" /> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-10828-fix-prevent-subgraph-node-position-corruption-during-graph-transitions-3366d73d365081418502dbb78da54013) by [Unito](https://www.unito.io) --------- Co-authored-by: Alexander Brown <drjkl@comfy.org>
282 lines
7.4 KiB
TypeScript
282 lines
7.4 KiB
TypeScript
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
|
import { ref } from 'vue'
|
|
import type { Ref } from 'vue'
|
|
|
|
import { LiteGraph } from '@/lib/litegraph/src/litegraph'
|
|
import { LayoutSource } from '@/renderer/core/layout/types'
|
|
import type { NodeId, NodeLayout } from '@/renderer/core/layout/types'
|
|
|
|
type ResizeEntryLike = Pick<
|
|
ResizeObserverEntry,
|
|
| 'target'
|
|
| 'borderBoxSize'
|
|
| 'contentBoxSize'
|
|
| 'devicePixelContentBoxSize'
|
|
| 'contentRect'
|
|
>
|
|
|
|
const resizeObserverState = vi.hoisted(() => {
|
|
const state = {
|
|
callback: null as ResizeObserverCallback | null,
|
|
observe: vi.fn<(element: Element) => void>(),
|
|
unobserve: vi.fn<(element: Element) => void>(),
|
|
disconnect: vi.fn<() => void>()
|
|
}
|
|
|
|
const MockResizeObserver: typeof ResizeObserver = class MockResizeObserver implements ResizeObserver {
|
|
observe = state.observe
|
|
unobserve = state.unobserve
|
|
disconnect = state.disconnect
|
|
|
|
constructor(callback: ResizeObserverCallback) {
|
|
state.callback = callback
|
|
}
|
|
}
|
|
|
|
globalThis.ResizeObserver = MockResizeObserver
|
|
|
|
return state
|
|
})
|
|
|
|
const testState = vi.hoisted(() => ({
|
|
linearMode: false,
|
|
nodeLayouts: new Map<NodeId, NodeLayout>(),
|
|
batchUpdateNodeBounds: vi.fn(),
|
|
setSource: vi.fn(),
|
|
syncNodeSlotLayoutsFromDOM: vi.fn()
|
|
}))
|
|
|
|
vi.mock('@vueuse/core', () => ({
|
|
useDocumentVisibility: () => ref<'visible' | 'hidden'>('visible')
|
|
}))
|
|
|
|
vi.mock('@/renderer/core/canvas/canvasStore', () => ({
|
|
useCanvasStore: () => ({
|
|
linearMode: testState.linearMode
|
|
})
|
|
}))
|
|
|
|
vi.mock('@/composables/element/useCanvasPositionConversion', () => ({
|
|
useSharedCanvasPositionConversion: () => ({
|
|
clientPosToCanvasPos: ([x, y]: [number, number]) => [x, y]
|
|
})
|
|
}))
|
|
|
|
vi.mock('@/renderer/core/layout/store/layoutStore', () => ({
|
|
layoutStore: {
|
|
batchUpdateNodeBounds: testState.batchUpdateNodeBounds,
|
|
setSource: testState.setSource,
|
|
getNodeLayoutRef: (nodeId: NodeId): Ref<NodeLayout | null> =>
|
|
ref<NodeLayout | null>(testState.nodeLayouts.get(nodeId) ?? null)
|
|
}
|
|
}))
|
|
|
|
vi.mock('./useSlotElementTracking', () => ({
|
|
syncNodeSlotLayoutsFromDOM: testState.syncNodeSlotLayoutsFromDOM
|
|
}))
|
|
|
|
import './useVueNodeResizeTracking'
|
|
|
|
function createResizeEntry(options?: {
|
|
nodeId?: NodeId
|
|
width?: number
|
|
height?: number
|
|
left?: number
|
|
top?: number
|
|
collapsed?: boolean
|
|
}) {
|
|
const {
|
|
nodeId = 'test-node',
|
|
width = 240,
|
|
height = 180,
|
|
left = 100,
|
|
top = 200,
|
|
collapsed = false
|
|
} = options ?? {}
|
|
|
|
const element = document.createElement('div')
|
|
element.dataset.nodeId = nodeId
|
|
if (collapsed) {
|
|
element.dataset.collapsed = ''
|
|
}
|
|
const rectSpy = vi.fn(() => new DOMRect(left, top, width, height))
|
|
element.getBoundingClientRect = rectSpy
|
|
const boxSizes = [{ inlineSize: width, blockSize: height }]
|
|
|
|
const entry = {
|
|
target: element,
|
|
borderBoxSize: boxSizes,
|
|
contentBoxSize: boxSizes,
|
|
devicePixelContentBoxSize: boxSizes,
|
|
contentRect: new DOMRect(left, top, width, height)
|
|
} satisfies ResizeEntryLike
|
|
|
|
return {
|
|
entry,
|
|
rectSpy
|
|
}
|
|
}
|
|
|
|
function createObserverMock(): ResizeObserver {
|
|
return {
|
|
observe: vi.fn(),
|
|
unobserve: vi.fn(),
|
|
disconnect: vi.fn()
|
|
}
|
|
}
|
|
|
|
function seedNodeLayout(options: {
|
|
nodeId: NodeId
|
|
left: number
|
|
top: number
|
|
width: number
|
|
height: number
|
|
}) {
|
|
const { nodeId, left, top, width, height } = options
|
|
const titleHeight = LiteGraph.NODE_TITLE_HEIGHT
|
|
const contentHeight = height - titleHeight
|
|
|
|
testState.nodeLayouts.set(nodeId, {
|
|
id: nodeId,
|
|
position: { x: left, y: top + titleHeight },
|
|
size: { width, height: contentHeight },
|
|
zIndex: 0,
|
|
visible: true,
|
|
bounds: {
|
|
x: left,
|
|
y: top + titleHeight,
|
|
width,
|
|
height: contentHeight
|
|
}
|
|
})
|
|
}
|
|
|
|
describe('useVueNodeResizeTracking', () => {
|
|
beforeEach(() => {
|
|
testState.linearMode = false
|
|
testState.nodeLayouts.clear()
|
|
testState.batchUpdateNodeBounds.mockReset()
|
|
testState.setSource.mockReset()
|
|
testState.syncNodeSlotLayoutsFromDOM.mockReset()
|
|
resizeObserverState.observe.mockReset()
|
|
resizeObserverState.unobserve.mockReset()
|
|
resizeObserverState.disconnect.mockReset()
|
|
})
|
|
|
|
it('skips repeated no-op resize entries after first measurement', () => {
|
|
const nodeId = 'test-node'
|
|
const width = 240
|
|
const height = 180
|
|
const left = 100
|
|
const top = 200
|
|
const { entry, rectSpy } = createResizeEntry({
|
|
nodeId,
|
|
width,
|
|
height,
|
|
left,
|
|
top
|
|
})
|
|
|
|
seedNodeLayout({ nodeId, left, top, width, height })
|
|
|
|
resizeObserverState.callback?.([entry], createObserverMock())
|
|
|
|
// When layout store already has correct position, getBoundingClientRect
|
|
// is not needed — position is read from the store instead.
|
|
expect(rectSpy).not.toHaveBeenCalled()
|
|
expect(testState.setSource).not.toHaveBeenCalled()
|
|
expect(testState.batchUpdateNodeBounds).not.toHaveBeenCalled()
|
|
expect(testState.syncNodeSlotLayoutsFromDOM).not.toHaveBeenCalled()
|
|
|
|
testState.setSource.mockReset()
|
|
testState.batchUpdateNodeBounds.mockReset()
|
|
testState.syncNodeSlotLayoutsFromDOM.mockReset()
|
|
|
|
resizeObserverState.callback?.([entry], createObserverMock())
|
|
|
|
expect(rectSpy).not.toHaveBeenCalled()
|
|
expect(testState.setSource).not.toHaveBeenCalled()
|
|
expect(testState.batchUpdateNodeBounds).not.toHaveBeenCalled()
|
|
expect(testState.syncNodeSlotLayoutsFromDOM).not.toHaveBeenCalled()
|
|
})
|
|
|
|
it('preserves layout store position when size matches but DOM position differs', () => {
|
|
const nodeId = 'test-node'
|
|
const width = 240
|
|
const height = 180
|
|
const { entry, rectSpy } = createResizeEntry({
|
|
nodeId,
|
|
width,
|
|
height,
|
|
left: 100,
|
|
top: 200
|
|
})
|
|
|
|
seedNodeLayout({
|
|
nodeId,
|
|
left: 90,
|
|
top: 190,
|
|
width,
|
|
height
|
|
})
|
|
|
|
resizeObserverState.callback?.([entry], createObserverMock())
|
|
|
|
// Position from DOM should NOT override layout store position
|
|
expect(rectSpy).not.toHaveBeenCalled()
|
|
expect(testState.setSource).not.toHaveBeenCalled()
|
|
expect(testState.batchUpdateNodeBounds).not.toHaveBeenCalled()
|
|
})
|
|
|
|
it('updates node bounds + slot layouts when size changes', () => {
|
|
const nodeId = 'test-node'
|
|
const { entry } = createResizeEntry({
|
|
nodeId,
|
|
width: 240,
|
|
height: 180,
|
|
left: 100,
|
|
top: 200
|
|
})
|
|
const titleHeight = LiteGraph.NODE_TITLE_HEIGHT
|
|
|
|
seedNodeLayout({
|
|
nodeId,
|
|
left: 100,
|
|
top: 200,
|
|
width: 220,
|
|
height: 140
|
|
})
|
|
|
|
resizeObserverState.callback?.([entry], createObserverMock())
|
|
|
|
expect(testState.setSource).toHaveBeenCalledWith(LayoutSource.DOM)
|
|
expect(testState.batchUpdateNodeBounds).toHaveBeenCalledWith([
|
|
{
|
|
nodeId,
|
|
bounds: {
|
|
x: 100,
|
|
y: 200 + titleHeight,
|
|
width: 240,
|
|
height: 180
|
|
}
|
|
}
|
|
])
|
|
expect(testState.syncNodeSlotLayoutsFromDOM).toHaveBeenCalledWith(nodeId)
|
|
})
|
|
|
|
it('resyncs slot anchors for collapsed nodes without writing bounds', () => {
|
|
const nodeId = 'test-node'
|
|
const { entry, rectSpy } = createResizeEntry({
|
|
nodeId,
|
|
collapsed: true
|
|
})
|
|
|
|
resizeObserverState.callback?.([entry], createObserverMock())
|
|
|
|
expect(rectSpy).not.toHaveBeenCalled()
|
|
expect(testState.setSource).not.toHaveBeenCalled()
|
|
expect(testState.batchUpdateNodeBounds).not.toHaveBeenCalled()
|
|
expect(testState.syncNodeSlotLayoutsFromDOM).toHaveBeenCalledWith(nodeId)
|
|
})
|
|
})
|