fix: prevent subgraph node position corruption during graph transitions (#10828)

## 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>
This commit is contained in:
jaeone94
2026-04-07 14:34:56 +09:00
committed by GitHub
parent 84f401bbe9
commit a8d23275d9
5 changed files with 136 additions and 26 deletions

View File

@@ -484,7 +484,16 @@ export class SubgraphHelper {
await this.comfyPage.vueNodes.enterSubgraph(hostNodeId)
await this.comfyPage.settings.setSetting('Comfy.VueNodes.Enabled', false)
await this.comfyPage.nextFrame()
await this.comfyPage.canvas.click()
await this.comfyPage.canvas.dispatchEvent('pointerdown', {
bubbles: true,
cancelable: true,
button: 0
})
await this.comfyPage.canvas.dispatchEvent('pointerup', {
bubbles: true,
cancelable: true,
button: 0
})
await this.comfyPage.canvas.press('Control+a')
await this.comfyPage.nextFrame()
await this.page.evaluate(() => {
@@ -493,7 +502,16 @@ export class SubgraphHelper {
})
await this.comfyPage.nextFrame()
await this.exitViaBreadcrumb()
await this.comfyPage.canvas.click()
await this.comfyPage.canvas.dispatchEvent('pointerdown', {
bubbles: true,
cancelable: true,
button: 0
})
await this.comfyPage.canvas.dispatchEvent('pointerup', {
bubbles: true,
cancelable: true,
button: 0
})
await this.comfyPage.nextFrame()
}

View File

@@ -58,6 +58,16 @@ export class WorkflowHelper {
await this.comfyPage.nextFrame()
}
async waitForDraftPersisted({ timeout = 5000 } = {}) {
await this.comfyPage.page.waitForFunction(
() =>
Object.keys(localStorage).some((k) =>
k.startsWith('Comfy.Workflow.Draft.v2:')
),
{ timeout }
)
}
async loadWorkflow(workflowName: string) {
await this.comfyPage.workflowUploadInput.setInputFiles(
assetPath(`${workflowName}.json`)

View File

@@ -0,0 +1,78 @@
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)
}
})
}
)

View File

@@ -181,7 +181,9 @@ describe('useVueNodeResizeTracking', () => {
resizeObserverState.callback?.([entry], createObserverMock())
expect(rectSpy).toHaveBeenCalledTimes(1)
// 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()
@@ -192,13 +194,13 @@ describe('useVueNodeResizeTracking', () => {
resizeObserverState.callback?.([entry], createObserverMock())
expect(rectSpy).toHaveBeenCalledTimes(1)
expect(rectSpy).not.toHaveBeenCalled()
expect(testState.setSource).not.toHaveBeenCalled()
expect(testState.batchUpdateNodeBounds).not.toHaveBeenCalled()
expect(testState.syncNodeSlotLayoutsFromDOM).not.toHaveBeenCalled()
})
it('updates bounds on first observation when size matches but position differs', () => {
it('preserves layout store position when size matches but DOM position differs', () => {
const nodeId = 'test-node'
const width = 240
const height = 180
@@ -209,7 +211,6 @@ describe('useVueNodeResizeTracking', () => {
left: 100,
top: 200
})
const titleHeight = LiteGraph.NODE_TITLE_HEIGHT
seedNodeLayout({
nodeId,
@@ -221,20 +222,10 @@ describe('useVueNodeResizeTracking', () => {
resizeObserverState.callback?.([entry], createObserverMock())
expect(rectSpy).toHaveBeenCalledTimes(1)
expect(testState.setSource).toHaveBeenCalledWith(LayoutSource.DOM)
expect(testState.batchUpdateNodeBounds).toHaveBeenCalledWith([
{
nodeId,
bounds: {
x: 100,
y: 200 + titleHeight,
width,
height
}
}
])
expect(testState.syncNodeSlotLayoutsFromDOM).toHaveBeenCalledWith(nodeId)
// 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', () => {

View File

@@ -186,13 +186,26 @@ const resizeObserver = new ResizeObserver((entries) => {
continue
}
// Screen-space rect
const rect = element.getBoundingClientRect()
const [cx, cy] = conv.clientPosToCanvasPos([rect.left, rect.top])
const topLeftCanvas = { x: cx, y: cy }
// Use existing position from layout store (source of truth) rather than
// converting screen-space getBoundingClientRect() back to canvas coords.
// The DOM→canvas conversion depends on the current canvas scale/offset,
// which can be stale during graph transitions (e.g. entering a subgraph
// before fitView runs), producing corrupted positions.
const existingPos = nodeLayout?.position
let posX: number
let posY: number
if (existingPos) {
posX = existingPos.x
posY = existingPos.y
} else {
const rect = element.getBoundingClientRect()
const [cx, cy] = conv.clientPosToCanvasPos([rect.left, rect.top])
posX = cx
posY = cy + LiteGraph.NODE_TITLE_HEIGHT
}
const bounds: Bounds = {
x: topLeftCanvas.x,
y: topLeftCanvas.y + LiteGraph.NODE_TITLE_HEIGHT,
x: posX,
y: posY,
width,
height
}