mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-20 14:30:41 +00:00
fix: don't override loadGraphData viewport on cache miss (#10810)
## Summary Fix regression from #10247 where template workflows (e.g. LTX2.3) loaded with a broken viewport. ## Problem `restoreViewport()` called `fitView()` on every cache miss via rAF. This raced with `loadGraphData`'s own viewport restore (`extra.ds` for saved workflows, or its own `fitView()` for templates at line 1266 of app.ts). The second `fitView()` overwrote the correct viewport, causing templates with subgraphs to display incorrectly. ## Fix On cache miss, check if any nodes are already visible in the current viewport before calling `fitView()`. If `loadGraphData` already positioned things correctly, we don't override it. Only intervene when the viewport is genuinely empty (first visit to a subgraph with no prior cached state AND no loadGraphData restore). ## Review Focus Single-file change in `subgraphNavigationStore.ts`. The visibility check mirrors the same pattern used in `app.ts:1272-1281` where loadGraphData itself checks for visible nodes. ## E2E Regression Test The existing Playwright tests in `browser_tests/tests/subgraphViewport.spec.ts` (added in #10247) already cover viewport restoration after subgraph navigation. The specific regression (template load viewport race) is not practically testable in E2E because: 1. Template loading requires the backend's template API which returns different templates per environment 2. The race condition depends on exact timing between `loadGraphData`'s viewport restore and the rAF-deferred `restoreViewport` — Playwright cannot reliably reproduce frame-level timing races 3. The fix is a guard condition (skip fitView if nodes visible) that makes the behavior idempotent regardless of timing ## Alternative to #10790 This can replace the full revert in #10790 — it preserves the viewport persistence feature while fixing the template regression. Fixes regression from #10247
This commit is contained in:
committed by
GitHub
parent
0b83926c3e
commit
6cd3b59d5f
@@ -12,6 +12,7 @@ import { useCanvasStore } from '@/renderer/core/canvas/canvasStore'
|
||||
import { useLitegraphService } from '@/services/litegraphService'
|
||||
import { app } from '@/scripts/app'
|
||||
import { findSubgraphPathById } from '@/utils/graphTraversalUtil'
|
||||
import { anyItemOverlapsRect } from '@/utils/mathUtil'
|
||||
import { isNonNullish, isSubgraph } from '@/utils/typeGuardUtil'
|
||||
|
||||
export const VIEWPORT_CACHE_MAX_SIZE = 32
|
||||
@@ -138,11 +139,19 @@ export const useSubgraphNavigationStore = defineStore(
|
||||
return
|
||||
}
|
||||
|
||||
// Cache miss — fit to content after the canvas has the new graph.
|
||||
// rAF fires after layout + paint, when nodes are positioned.
|
||||
const expectedGraphId = graphId
|
||||
// Cache miss — fit to content only if no nodes are currently visible.
|
||||
// loadGraphData may have already restored extra.ds or called fitView
|
||||
// for templates, so only intervene when the viewport is truly empty.
|
||||
requestAnimationFrame(() => {
|
||||
if (getActiveGraphId() !== expectedGraphId) return
|
||||
if (getActiveGraphId() !== graphId) return
|
||||
if (!canvas.graph) return
|
||||
|
||||
const nodes = canvas.graph.nodes
|
||||
if (!nodes?.length) return
|
||||
|
||||
canvas.ds.computeVisibleArea(canvas.viewport)
|
||||
if (anyItemOverlapsRect(nodes, canvas.ds.visible_area)) return
|
||||
|
||||
useLitegraphService().fitView()
|
||||
})
|
||||
}
|
||||
|
||||
@@ -24,8 +24,11 @@ vi.mock('@/scripts/app', () => {
|
||||
scale: 1,
|
||||
offset: [0, 0],
|
||||
state: { scale: 1, offset: [0, 0] },
|
||||
fitToBounds: vi.fn()
|
||||
fitToBounds: vi.fn(),
|
||||
visible_area: [0, 0, 1000, 1000],
|
||||
computeVisibleArea: vi.fn()
|
||||
},
|
||||
viewport: [0, 0, 1000, 1000],
|
||||
setDirty: mockSetDirty,
|
||||
get empty() {
|
||||
return true
|
||||
@@ -184,6 +187,11 @@ describe('useSubgraphNavigationStore - Viewport Persistence', () => {
|
||||
// Ensure no cached entry
|
||||
store.viewportCache.delete(':root')
|
||||
|
||||
// Add a node outside the visible area so anyItemOverlapsRect returns false
|
||||
const mockGraph = app.graph as { nodes: unknown[]; _nodes: unknown[] }
|
||||
mockGraph.nodes = [{ pos: [9999, 9999], size: [100, 100] }]
|
||||
mockGraph._nodes = mockGraph.nodes
|
||||
|
||||
// Use the root graph ID so the stale-guard passes
|
||||
store.restoreViewport('root')
|
||||
|
||||
@@ -194,6 +202,10 @@ describe('useSubgraphNavigationStore - Viewport Persistence', () => {
|
||||
rafCallbacks[0](performance.now())
|
||||
|
||||
expect(mockFitView).toHaveBeenCalledOnce()
|
||||
|
||||
// Cleanup
|
||||
mockGraph.nodes = []
|
||||
mockGraph._nodes = []
|
||||
})
|
||||
|
||||
it('skips fitView if active graph changed before rAF fires', () => {
|
||||
|
||||
Reference in New Issue
Block a user