mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-20 14:30:41 +00:00
## Summary
Fix subgraph viewport (zoom + position) drifting when navigating in/out
of subgraphs and switching workflow tabs.
## Problem
Three root causes:
1. **First visit**: `restoreViewport()` silently returned on cache miss,
leaving canvas at stale position
2. **Cross-workflow leakage**: Cache keyed by bare `graphId` — two
workflows with the same subgraph or unsaved workflows shared cache
entries
3. **Stale save on tab switch**: `loadGraphData` and
`changeTracker.restore()` overwrite `canvas.ds` before the async watcher
could save the old viewport
## Solution
1. **Workflow-scoped cache keys**: `${path}#${instanceId}:${graphId}` —
WeakMap assigns unique IDs per workflow object, handling unsaved
workflows with identical paths
2. **`flush: 'sync'` on activeSubgraph watcher**: Fires immediately
during `setGraph()`, BEFORE `loadGraphData`/`changeTracker` can corrupt
`canvas.ds`
3. **Cache miss → rAF fitToBounds**: On first visit, computes bounds
from `graph._nodes` and calls `ds.fitToBounds()` after the browser has
rendered
4. **Workflow switch watcher** (`flush: 'sync'`): Pre-saves viewport
under old workflow identity, suppresses `onNavigated` saves during load
cycle
Key architectural insight: `setGraph()` never touches `canvas.ds`, but
`loadGraphData` and `changeTracker.restore()` both write to it. By using
`flush: 'sync'`, the save happens during `setGraph` (before the
overwrites).
## Review Focus
- `subgraphNavigationStore.ts` — the three fixes and their interaction
- `flush: 'sync'` watchers — critical for correct save timing
- `suppressNavigatedSave` flag — prevents stale saves during async
workflow load
## Breaking Changes
None. Viewport cache is session-only (in-memory LRU). Existing workflows
unaffected.
## Demo Video of Fix
https://github.com/user-attachments/assets/71dd4107-a030-4e68-aa11-47fe00101b25
## Test plan
- [x] Unit: save/restore with workflow-scoped keys
- [x] Unit: cache miss doesn't mutate canvas synchronously
- [x] Unit: navigation integration (enter/exit preserves viewport)
- [x] E2E: first subgraph visit has visible nodes
- [x] Manual: enter subgraph → zoom/pan → exit → re-enter → viewport
restored
- [x] Manual: tab with subgraph → different tab → back → viewport
restored
- [x] Manual: two unsaved workflows → switch between → viewports
isolated
- Fixes #10246
- Related: #8173
<!-- QA_REPORT_SECTION -->
---
## 🔍 Automated QA Report
| | |
|---|---|
| **Status** | ✅ Complete |
| **Report** |
[sno-qa-10247.comfy-qa.pages.dev](https://sno-qa-10247.comfy-qa.pages.dev/)
|
| **CI Run** | [View
workflow](https://github.com/Comfy-Org/ComfyUI_frontend/actions/runs/23373279990)
|
Before/after video recordings with **Behavior Changes** and **Timeline
Comparison** tables.
115 lines
3.2 KiB
TypeScript
115 lines
3.2 KiB
TypeScript
import { expect } from '@playwright/test'
|
|
|
|
import { comfyPageFixture as test } from '../fixtures/ComfyPage'
|
|
|
|
function hasVisibleNodeInViewport() {
|
|
const canvas = window.app!.canvas
|
|
if (!canvas?.graph?._nodes?.length) return false
|
|
|
|
const ds = canvas.ds
|
|
const cw = canvas.canvas.width / window.devicePixelRatio
|
|
const ch = canvas.canvas.height / window.devicePixelRatio
|
|
const visLeft = -ds.offset[0]
|
|
const visTop = -ds.offset[1]
|
|
const visRight = visLeft + cw / ds.scale
|
|
const visBottom = visTop + ch / ds.scale
|
|
|
|
for (const node of canvas.graph._nodes) {
|
|
const [nx, ny] = node.pos
|
|
const [nw, nh] = node.size
|
|
if (
|
|
nx + nw > visLeft &&
|
|
nx < visRight &&
|
|
ny + nh > visTop &&
|
|
ny < visBottom
|
|
)
|
|
return true
|
|
}
|
|
return false
|
|
}
|
|
|
|
test.describe('Subgraph viewport restoration', { tag: '@subgraph' }, () => {
|
|
test('first visit fits viewport to subgraph nodes (LG)', async ({
|
|
comfyPage
|
|
}) => {
|
|
await comfyPage.workflow.loadWorkflow(
|
|
'subgraphs/subgraph-with-promoted-text-widget'
|
|
)
|
|
await comfyPage.nextFrame()
|
|
|
|
await comfyPage.page.evaluate(() => {
|
|
const canvas = window.app!.canvas
|
|
const graph = canvas.graph!
|
|
const sgNode = graph._nodes.find((n) =>
|
|
'isSubgraphNode' in n
|
|
? (n as unknown as { isSubgraphNode: () => boolean }).isSubgraphNode()
|
|
: false
|
|
) as unknown as { subgraph?: typeof graph } | undefined
|
|
if (!sgNode?.subgraph) throw new Error('No subgraph node')
|
|
|
|
canvas.setGraph(sgNode.subgraph)
|
|
})
|
|
|
|
await expect
|
|
.poll(() => comfyPage.page.evaluate(hasVisibleNodeInViewport), {
|
|
timeout: 2000
|
|
})
|
|
.toBe(true)
|
|
})
|
|
|
|
test('first visit fits viewport to subgraph nodes (Vue)', async ({
|
|
comfyPage
|
|
}) => {
|
|
await comfyPage.settings.setSetting('Comfy.VueNodes.Enabled', true)
|
|
await comfyPage.workflow.loadWorkflow(
|
|
'subgraphs/subgraph-with-promoted-text-widget'
|
|
)
|
|
await comfyPage.vueNodes.waitForNodes()
|
|
|
|
await comfyPage.vueNodes.enterSubgraph('11')
|
|
|
|
await expect
|
|
.poll(() => comfyPage.page.evaluate(hasVisibleNodeInViewport), {
|
|
timeout: 2000
|
|
})
|
|
.toBe(true)
|
|
})
|
|
|
|
test('viewport is restored when returning to root (Vue)', async ({
|
|
comfyPage
|
|
}) => {
|
|
await comfyPage.settings.setSetting('Comfy.VueNodes.Enabled', true)
|
|
await comfyPage.workflow.loadWorkflow(
|
|
'subgraphs/subgraph-with-promoted-text-widget'
|
|
)
|
|
await comfyPage.vueNodes.waitForNodes()
|
|
|
|
const rootViewport = await comfyPage.page.evaluate(() => {
|
|
const ds = window.app!.canvas.ds
|
|
return { scale: ds.scale, offset: [...ds.offset] }
|
|
})
|
|
|
|
await comfyPage.vueNodes.enterSubgraph('11')
|
|
await comfyPage.nextFrame()
|
|
|
|
await comfyPage.subgraph.exitViaBreadcrumb()
|
|
|
|
await expect
|
|
.poll(
|
|
() =>
|
|
comfyPage.page.evaluate(() => {
|
|
const ds = window.app!.canvas.ds
|
|
return { scale: ds.scale, offset: [...ds.offset] }
|
|
}),
|
|
{ timeout: 2000 }
|
|
)
|
|
.toEqual({
|
|
scale: expect.closeTo(rootViewport.scale, 2),
|
|
offset: [
|
|
expect.closeTo(rootViewport.offset[0], 0),
|
|
expect.closeTo(rootViewport.offset[1], 0)
|
|
]
|
|
})
|
|
})
|
|
})
|