mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-19 22:09:37 +00:00
fix: re-sync collapsed node slot positions after subgraph fitView (#11240)
## Summary Fix collapsed node connection links rendering at wrong positions when entering a subgraph for the first time. `fitView()` (added in #10995) changes canvas scale/offset, invalidating cached slot positions for collapsed nodes. ## Changes - **What**: Schedule `requestSlotLayoutSyncForAllNodes()` on the next frame after `fitView()` in `restoreViewport()` so collapsed node slot positions are re-measured against the updated transform. Inner RAF guarded against mid-frame graph changes. - **Test coverage**: - Unit tests in `subgraphNavigationStore.viewport.test.ts` verify the RAF chain calls `requestSlotLayoutSyncForAllNodes` after `fitView`, and skip the re-sync when the active graph changes between frames. - E2E screenshot test (`@screenshot` tag) validates correct link rendering on first subgraph entry using a new fixture with a pre-collapsed inner node. ## Review Focus The nested `requestAnimationFrame` is intentional: the outer RAF runs `fitView()`, which updates `ds.scale`/`ds.offset` and triggers a CSS transform update on `TransformPane`. The inner RAF ensures the DOM has reflowed with the new transform before `requestSlotLayoutSyncForAllNodes()` measures `getBoundingClientRect()` on slot elements. --------- Co-authored-by: github-actions <github-actions@github.com>
This commit is contained in:
197
browser_tests/assets/subgraphs/subgraph-with-collapsed-node.json
Normal file
197
browser_tests/assets/subgraphs/subgraph-with-collapsed-node.json
Normal file
@@ -0,0 +1,197 @@
|
||||
{
|
||||
"id": "fe4562c0-3a0b-4614-bdec-7039a58d75b8",
|
||||
"revision": 0,
|
||||
"last_node_id": 2,
|
||||
"last_link_id": 0,
|
||||
"nodes": [
|
||||
{
|
||||
"id": 2,
|
||||
"type": "e5fb1765-9323-4548-801a-5aead34d879e",
|
||||
"pos": [627.5973510742188, 423.0972900390625],
|
||||
"size": [400, 200],
|
||||
"flags": {},
|
||||
"order": 0,
|
||||
"mode": 0,
|
||||
"inputs": [
|
||||
{
|
||||
"name": "positive",
|
||||
"type": "CONDITIONING",
|
||||
"link": null
|
||||
}
|
||||
],
|
||||
"outputs": [
|
||||
{
|
||||
"name": "LATENT",
|
||||
"type": "LATENT",
|
||||
"links": null
|
||||
}
|
||||
],
|
||||
"properties": {},
|
||||
"widgets_values": []
|
||||
}
|
||||
],
|
||||
"links": [],
|
||||
"groups": [],
|
||||
"definitions": {
|
||||
"subgraphs": [
|
||||
{
|
||||
"id": "e5fb1765-9323-4548-801a-5aead34d879e",
|
||||
"version": 1,
|
||||
"state": {
|
||||
"lastGroupId": 0,
|
||||
"lastNodeId": 2,
|
||||
"lastLinkId": 4,
|
||||
"lastRerouteId": 0
|
||||
},
|
||||
"revision": 0,
|
||||
"config": {},
|
||||
"name": "New Subgraph",
|
||||
"inputNode": {
|
||||
"id": -10,
|
||||
"bounding": [347.90441582814213, 417.3822440655296, 120, 60]
|
||||
},
|
||||
"outputNode": {
|
||||
"id": -20,
|
||||
"bounding": [892.5973510742188, 416.0972900390625, 120, 60]
|
||||
},
|
||||
"inputs": [
|
||||
{
|
||||
"id": "c5cc99d8-a2b6-4bf3-8be7-d4949ef736cd",
|
||||
"name": "positive",
|
||||
"type": "CONDITIONING",
|
||||
"linkIds": [1],
|
||||
"pos": {
|
||||
"0": 447.9044189453125,
|
||||
"1": 437.3822326660156
|
||||
}
|
||||
}
|
||||
],
|
||||
"outputs": [
|
||||
{
|
||||
"id": "9bd488b9-e907-4c95-a7a4-85c5597a87af",
|
||||
"name": "LATENT",
|
||||
"type": "LATENT",
|
||||
"linkIds": [2],
|
||||
"pos": {
|
||||
"0": 912.5973510742188,
|
||||
"1": 436.0972900390625
|
||||
}
|
||||
}
|
||||
],
|
||||
"widgets": [],
|
||||
"nodes": [
|
||||
{
|
||||
"id": 1,
|
||||
"type": "KSampler",
|
||||
"pos": [554.8743286132812, 100.95539093017578],
|
||||
"size": [270, 262],
|
||||
"flags": { "collapsed": true },
|
||||
"order": 1,
|
||||
"mode": 0,
|
||||
"inputs": [
|
||||
{
|
||||
"localized_name": "model",
|
||||
"name": "model",
|
||||
"type": "MODEL",
|
||||
"link": null
|
||||
},
|
||||
{
|
||||
"localized_name": "positive",
|
||||
"name": "positive",
|
||||
"type": "CONDITIONING",
|
||||
"link": 1
|
||||
},
|
||||
{
|
||||
"localized_name": "negative",
|
||||
"name": "negative",
|
||||
"type": "CONDITIONING",
|
||||
"link": null
|
||||
},
|
||||
{
|
||||
"localized_name": "latent_image",
|
||||
"name": "latent_image",
|
||||
"type": "LATENT",
|
||||
"link": null
|
||||
}
|
||||
],
|
||||
"outputs": [
|
||||
{
|
||||
"localized_name": "LATENT",
|
||||
"name": "LATENT",
|
||||
"type": "LATENT",
|
||||
"links": [2]
|
||||
}
|
||||
],
|
||||
"properties": {
|
||||
"Node name for S&R": "KSampler"
|
||||
},
|
||||
"widgets_values": [0, "randomize", 20, 8, "euler", "simple", 1]
|
||||
},
|
||||
{
|
||||
"id": 2,
|
||||
"type": "VAEEncode",
|
||||
"pos": [685.1265869140625, 439.1734619140625],
|
||||
"size": [140, 46],
|
||||
"flags": {},
|
||||
"order": 0,
|
||||
"mode": 0,
|
||||
"inputs": [
|
||||
{
|
||||
"localized_name": "pixels",
|
||||
"name": "pixels",
|
||||
"type": "IMAGE",
|
||||
"link": null
|
||||
},
|
||||
{
|
||||
"localized_name": "vae",
|
||||
"name": "vae",
|
||||
"type": "VAE",
|
||||
"link": null
|
||||
}
|
||||
],
|
||||
"outputs": [
|
||||
{
|
||||
"localized_name": "LATENT",
|
||||
"name": "LATENT",
|
||||
"type": "LATENT",
|
||||
"links": [4]
|
||||
}
|
||||
],
|
||||
"properties": {
|
||||
"Node name for S&R": "VAEEncode"
|
||||
}
|
||||
}
|
||||
],
|
||||
"groups": [],
|
||||
"links": [
|
||||
{
|
||||
"id": 1,
|
||||
"origin_id": -10,
|
||||
"origin_slot": 0,
|
||||
"target_id": 1,
|
||||
"target_slot": 1,
|
||||
"type": "CONDITIONING"
|
||||
},
|
||||
{
|
||||
"id": 2,
|
||||
"origin_id": 1,
|
||||
"origin_slot": 0,
|
||||
"target_id": -20,
|
||||
"target_slot": 0,
|
||||
"type": "LATENT"
|
||||
}
|
||||
],
|
||||
"extra": {}
|
||||
}
|
||||
]
|
||||
},
|
||||
"config": {},
|
||||
"extra": {
|
||||
"ds": {
|
||||
"scale": 0.8894351682943402,
|
||||
"offset": [58.7671207025881, 137.7124650620126]
|
||||
},
|
||||
"frontendVersion": "1.24.1"
|
||||
},
|
||||
"version": 0.4
|
||||
}
|
||||
@@ -64,3 +64,29 @@ test.describe(
|
||||
})
|
||||
}
|
||||
)
|
||||
|
||||
test.describe(
|
||||
'Collapsed node links inside subgraph on first entry',
|
||||
{ tag: ['@canvas', '@node', '@vue-nodes', '@subgraph', '@screenshot'] },
|
||||
() => {
|
||||
test('renders collapsed node links correctly after fitView on first subgraph entry', async ({
|
||||
comfyPage
|
||||
}) => {
|
||||
await comfyPage.workflow.loadWorkflow(
|
||||
'subgraphs/subgraph-with-collapsed-node'
|
||||
)
|
||||
await comfyPage.nextFrame()
|
||||
|
||||
await comfyPage.vueNodes.enterSubgraph('2')
|
||||
|
||||
await expect.poll(() => comfyPage.subgraph.isInSubgraph()).toBe(true)
|
||||
|
||||
// fitView runs on first entry and re-syncs slot layouts for the
|
||||
// pre-collapsed KSampler. Screenshot captures the rendered canvas
|
||||
// links to guard against regressing the stale-coordinate bug.
|
||||
await expect(comfyPage.canvas).toHaveScreenshot(
|
||||
'subgraph-entry-collapsed-node-links.png'
|
||||
)
|
||||
})
|
||||
}
|
||||
)
|
||||
|
||||
Binary file not shown.
|
After Width: | Height: | Size: 74 KiB |
@@ -9,6 +9,7 @@ import type { Subgraph } from '@/lib/litegraph/src/litegraph'
|
||||
import { useWorkflowStore } from '@/platform/workflow/management/stores/workflowStore'
|
||||
import { useWorkflowService } from '@/platform/workflow/core/services/workflowService'
|
||||
import { useCanvasStore } from '@/renderer/core/canvas/canvasStore'
|
||||
import { requestSlotLayoutSyncForAllNodes } from '@/renderer/extensions/vueNodes/composables/useSlotElementTracking'
|
||||
import { app } from '@/scripts/app'
|
||||
import { useLitegraphService } from '@/services/litegraphService'
|
||||
import { findSubgraphPathById } from '@/utils/graphTraversalUtil'
|
||||
@@ -143,6 +144,12 @@ export const useSubgraphNavigationStore = defineStore(
|
||||
if (getActiveGraphId() !== graphId) return
|
||||
if (!canvas.graph?.nodes?.length) return
|
||||
useLitegraphService().fitView()
|
||||
// fitView changes scale/offset, so re-sync slot positions for
|
||||
// collapsed nodes whose DOM-relative measurement is now stale.
|
||||
requestAnimationFrame(() => {
|
||||
if (getActiveGraphId() !== graphId) return
|
||||
requestSlotLayoutSyncForAllNodes()
|
||||
})
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -12,10 +12,13 @@ import {
|
||||
VIEWPORT_CACHE_MAX_SIZE
|
||||
} from '@/stores/subgraphNavigationStore'
|
||||
|
||||
const { mockSetDirty, mockFitView } = vi.hoisted(() => ({
|
||||
mockSetDirty: vi.fn(),
|
||||
mockFitView: vi.fn()
|
||||
}))
|
||||
const { mockSetDirty, mockFitView, mockRequestSlotSyncAll } = vi.hoisted(
|
||||
() => ({
|
||||
mockSetDirty: vi.fn(),
|
||||
mockFitView: vi.fn(),
|
||||
mockRequestSlotSyncAll: vi.fn()
|
||||
})
|
||||
)
|
||||
|
||||
vi.mock('@/scripts/app', () => {
|
||||
const mockCanvas = {
|
||||
@@ -66,6 +69,13 @@ vi.mock('@/services/litegraphService', () => ({
|
||||
useLitegraphService: () => ({ fitView: mockFitView })
|
||||
}))
|
||||
|
||||
vi.mock(
|
||||
'@/renderer/extensions/vueNodes/composables/useSlotElementTracking',
|
||||
() => ({
|
||||
requestSlotLayoutSyncForAllNodes: mockRequestSlotSyncAll
|
||||
})
|
||||
)
|
||||
|
||||
const mockCanvas = app.canvas
|
||||
|
||||
let rafCallbacks: FrameRequestCallback[] = []
|
||||
@@ -86,6 +96,7 @@ describe('useSubgraphNavigationStore - Viewport Persistence', () => {
|
||||
mockCanvas.ds.state.offset = [0, 0]
|
||||
mockSetDirty.mockClear()
|
||||
mockFitView.mockClear()
|
||||
mockRequestSlotSyncAll.mockClear()
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
@@ -217,6 +228,53 @@ describe('useSubgraphNavigationStore - Viewport Persistence', () => {
|
||||
expect(mockFitView).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('re-syncs all slot layouts on the frame after fitView', () => {
|
||||
const store = useSubgraphNavigationStore()
|
||||
store.viewportCache.delete(':root')
|
||||
|
||||
const mockGraph = app.graph as { nodes: unknown[]; _nodes: unknown[] }
|
||||
mockGraph.nodes = [{ pos: [0, 0], size: [100, 100] }]
|
||||
mockGraph._nodes = mockGraph.nodes
|
||||
|
||||
store.restoreViewport('root')
|
||||
expect(rafCallbacks).toHaveLength(1)
|
||||
|
||||
// Outer RAF runs fitView and schedules the inner RAF
|
||||
rafCallbacks[0](performance.now())
|
||||
expect(mockFitView).toHaveBeenCalledOnce()
|
||||
expect(mockRequestSlotSyncAll).not.toHaveBeenCalled()
|
||||
expect(rafCallbacks).toHaveLength(2)
|
||||
|
||||
// Inner RAF re-syncs slots after fitView's transform has been applied
|
||||
rafCallbacks[1](performance.now())
|
||||
expect(mockRequestSlotSyncAll).toHaveBeenCalledOnce()
|
||||
|
||||
mockGraph.nodes = []
|
||||
mockGraph._nodes = []
|
||||
})
|
||||
|
||||
it('skips slot re-sync if active graph changed between fitView and inner RAF', () => {
|
||||
const store = useSubgraphNavigationStore()
|
||||
store.viewportCache.delete(':root')
|
||||
|
||||
const mockGraph = app.graph as { nodes: unknown[]; _nodes: unknown[] }
|
||||
mockGraph.nodes = [{ pos: [0, 0], size: [100, 100] }]
|
||||
mockGraph._nodes = mockGraph.nodes
|
||||
|
||||
store.restoreViewport('root')
|
||||
rafCallbacks[0](performance.now())
|
||||
expect(mockFitView).toHaveBeenCalledOnce()
|
||||
|
||||
// User navigated away before the inner RAF fired
|
||||
mockCanvas.subgraph = { id: 'different-graph' } as never
|
||||
rafCallbacks[1](performance.now())
|
||||
|
||||
expect(mockRequestSlotSyncAll).not.toHaveBeenCalled()
|
||||
|
||||
mockGraph.nodes = []
|
||||
mockGraph._nodes = []
|
||||
})
|
||||
|
||||
it('skips fitView if active graph changed before rAF fires', () => {
|
||||
const store = useSubgraphNavigationStore()
|
||||
store.viewportCache.delete(':root')
|
||||
|
||||
Reference in New Issue
Block a user