mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-20 14:30:41 +00:00
a8d23275d9
## 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>
79 lines
2.6 KiB
TypeScript
79 lines
2.6 KiB
TypeScript
import { expect } from '@playwright/test'
|
|
|
|
import { comfyPageFixture as test } from '../../fixtures/ComfyPage'
|
|
|
|
test.describe(
|
|
'Subgraph node positions after draft reload',
|
|
{ tag: ['@subgraph'] },
|
|
() => {
|
|
test('Node positions are preserved after draft reload with subgraph auto-entry', async ({
|
|
comfyPage
|
|
}) => {
|
|
test.setTimeout(30000)
|
|
|
|
// Enable workflow persistence explicitly
|
|
await comfyPage.settings.setSetting('Comfy.Workflow.Persist', true)
|
|
|
|
// Load a workflow containing a subgraph
|
|
await comfyPage.workflow.loadWorkflow('subgraphs/basic-subgraph')
|
|
|
|
// Enter the subgraph programmatically (fixture node is too small for UI click)
|
|
await comfyPage.page.evaluate(() => {
|
|
const sg = [...window.app!.rootGraph.subgraphs.values()][0]
|
|
if (sg) window.app!.canvas.setGraph(sg)
|
|
})
|
|
await comfyPage.nextFrame()
|
|
await expect.poll(() => comfyPage.subgraph.isInSubgraph()).toBe(true)
|
|
|
|
const positionsBefore = await comfyPage.page.evaluate(() => {
|
|
const sg = [...window.app!.rootGraph.subgraphs.values()][0]
|
|
return sg.nodes.map((n) => ({
|
|
id: n.id,
|
|
x: n.pos[0],
|
|
y: n.pos[1]
|
|
}))
|
|
})
|
|
|
|
expect(positionsBefore.length).toBeGreaterThan(0)
|
|
|
|
// Wait for the debounced draft persistence to flush to localStorage
|
|
await comfyPage.workflow.waitForDraftPersisted()
|
|
|
|
// Reload the page (draft auto-loads with hash preserved)
|
|
await comfyPage.page.reload({ waitUntil: 'networkidle' })
|
|
await comfyPage.page.waitForFunction(
|
|
() => window.app && window.app.extensionManager
|
|
)
|
|
await comfyPage.page.waitForSelector('.p-blockui-mask', {
|
|
state: 'hidden'
|
|
})
|
|
await comfyPage.nextFrame()
|
|
|
|
// Wait for subgraph auto-entry via hash navigation
|
|
await expect
|
|
.poll(() => comfyPage.subgraph.isInSubgraph(), { timeout: 10000 })
|
|
.toBe(true)
|
|
|
|
// Verify all internal node positions are preserved
|
|
const positionsAfter = await comfyPage.page.evaluate(() => {
|
|
const sg = [...window.app!.rootGraph.subgraphs.values()][0]
|
|
return sg.nodes.map((n) => ({
|
|
id: n.id,
|
|
x: n.pos[0],
|
|
y: n.pos[1]
|
|
}))
|
|
})
|
|
|
|
for (const before of positionsBefore) {
|
|
const after = positionsAfter.find((n) => n.id === before.id)
|
|
expect(
|
|
after,
|
|
`Node ${before.id} should exist after reload`
|
|
).toBeDefined()
|
|
expect(after!.x).toBeCloseTo(before.x, 0)
|
|
expect(after!.y).toBeCloseTo(before.y, 0)
|
|
}
|
|
})
|
|
}
|
|
)
|