diff --git a/browser_tests/tests/subgraph/subgraphHashValidation.spec.ts b/browser_tests/tests/subgraph/subgraphHashValidation.spec.ts index 050970d6ac..105a68a55b 100644 --- a/browser_tests/tests/subgraph/subgraphHashValidation.spec.ts +++ b/browser_tests/tests/subgraph/subgraphHashValidation.spec.ts @@ -3,8 +3,33 @@ import { expect } from '@playwright/test' import { comfyPageFixture as test } from '@e2e/fixtures/ComfyPage' -async function expectCanvasOnRootGraph(comfyPage: { page: Page }) { - const state = await comfyPage.page.evaluate(() => ({ +async function waitForStoreInitialSync(page: Page) { + await expect + .poll(async () => { + const state = await page.evaluate(() => ({ + rootId: window.app?.rootGraph?.id ?? '', + hash: window.location.hash + })) + return state.rootId !== '' && state.hash === `#${state.rootId}` + }) + .toBe(true) +} + +async function expectCanvasOnRootGraph(page: Page) { + await expect + .poll(async () => + page.evaluate(() => ({ + rootId: window.app!.rootGraph.id, + canvasGraphId: window.app!.canvas.graph?.id, + hash: window.location.hash + })) + ) + .toEqual({ + rootId: expect.any(String), + canvasGraphId: expect.stringMatching(/.+/), + hash: expect.stringMatching(/^#.+/) + }) + const state = await page.evaluate(() => ({ rootId: window.app!.rootGraph.id, canvasGraphId: window.app!.canvas.graph?.id, hash: window.location.hash @@ -20,6 +45,7 @@ test.describe( test('redirects URL and canvas to root for a non-existent subgraph hash', async ({ comfyPage }) => { + await waitForStoreInitialSync(comfyPage.page) const rootId = await comfyPage.page.evaluate( () => window.app!.rootGraph.id ) @@ -31,14 +57,17 @@ test.describe( }, `#${phantomId}`) await expect - .poll(() => comfyPage.page.evaluate(() => window.location.hash)) + .poll(() => comfyPage.page.evaluate(() => window.location.hash), { + timeout: 5000 + }) .toBe(`#${rootId}`) - await expectCanvasOnRootGraph(comfyPage) + await expectCanvasOnRootGraph(comfyPage.page) }) test('redirects URL and canvas to root when hash is malformed', async ({ comfyPage }) => { + await waitForStoreInitialSync(comfyPage.page) const rootId = await comfyPage.page.evaluate( () => window.app!.rootGraph.id ) @@ -48,9 +77,11 @@ test.describe( }) await expect - .poll(() => comfyPage.page.evaluate(() => window.location.hash)) + .poll(() => comfyPage.page.evaluate(() => window.location.hash), { + timeout: 5000 + }) .toBe(`#${rootId}`) - await expectCanvasOnRootGraph(comfyPage) + await expectCanvasOnRootGraph(comfyPage.page) }) } ) diff --git a/src/schemas/subgraphIdSchema.test.ts b/src/schemas/subgraphIdSchema.test.ts index a7fb0c79ae..5b5eeab02e 100644 --- a/src/schemas/subgraphIdSchema.test.ts +++ b/src/schemas/subgraphIdSchema.test.ts @@ -2,18 +2,18 @@ import { describe, expect, it } from 'vitest' import { createUuidv4 } from '@/lib/litegraph/src/utils/uuid' -import { isValidSubgraphId, zSubgraphId } from './subgraphIdSchema' +import { isUuidShapedSubgraphId, zSubgraphId } from './subgraphIdSchema' + +const CANONICAL_UUID = '550e8400-e29b-41d4-a716-446655440000' describe('subgraphIdSchema', () => { describe('zSubgraphId', () => { it('accepts a freshly generated UUID v4', () => { - const id = createUuidv4() - expect(zSubgraphId.safeParse(id).success).toBe(true) + expect(zSubgraphId.safeParse(createUuidv4()).success).toBe(true) }) it('accepts a canonical UUID string', () => { - const id = '550e8400-e29b-41d4-a716-446655440000' - expect(zSubgraphId.safeParse(id).success).toBe(true) + expect(zSubgraphId.safeParse(CANONICAL_UUID).success).toBe(true) }) it.each([ @@ -22,8 +22,8 @@ describe('subgraphIdSchema', () => { ['plain word', 'subgraph'], ['hash leftover', '#abc'], ['hex but not UUID-shaped', 'abcdef0123456789'], - ['UUID with leading hash', '#550e8400-e29b-41d4-a716-446655440000'], - ['UUID with whitespace', ' 550e8400-e29b-41d4-a716-446655440000 '] + ['UUID with leading hash', `#${CANONICAL_UUID}`], + ['UUID with whitespace', ` ${CANONICAL_UUID} `] ])('rejects %s', (_label, value) => { expect(zSubgraphId.safeParse(value).success).toBe(false) }) @@ -38,15 +38,15 @@ describe('subgraphIdSchema', () => { }) }) - describe('isValidSubgraphId', () => { + describe('isUuidShapedSubgraphId', () => { it('returns true for a valid UUID', () => { - expect(isValidSubgraphId(createUuidv4())).toBe(true) + expect(isUuidShapedSubgraphId(createUuidv4())).toBe(true) }) it('returns false for an invalid value', () => { - expect(isValidSubgraphId('not-a-uuid')).toBe(false) - expect(isValidSubgraphId(undefined)).toBe(false) - expect(isValidSubgraphId(42)).toBe(false) + expect(isUuidShapedSubgraphId('not-a-uuid')).toBe(false) + expect(isUuidShapedSubgraphId(undefined)).toBe(false) + expect(isUuidShapedSubgraphId(42)).toBe(false) }) }) }) diff --git a/src/schemas/subgraphIdSchema.ts b/src/schemas/subgraphIdSchema.ts index ad8e000b33..f065910ac4 100644 --- a/src/schemas/subgraphIdSchema.ts +++ b/src/schemas/subgraphIdSchema.ts @@ -1,17 +1,10 @@ import { z } from 'zod' -/** - * Schema for a subgraph identifier (RFC 4122 UUID). - * - * Subgraph IDs are generated by `createUuidv4()` and persisted in workflow - * JSON as `z.string().uuid()` (see workflowSchema.ts). They are also embedded - * in the URL hash for in-app navigation; hash values are untrusted input and - * must be validated before use as a lookup key. - */ +/** Hash values from the URL bar are untrusted; validate before lookup. */ export const zSubgraphId = z.string().uuid() type SubgraphId = z.infer -export function isValidSubgraphId(value: unknown): value is SubgraphId { +export function isUuidShapedSubgraphId(value: unknown): value is SubgraphId { return zSubgraphId.safeParse(value).success } diff --git a/src/stores/subgraphNavigationStore.navigateToHash.test.ts b/src/stores/subgraphNavigationStore.navigateToHash.test.ts index ac037823b5..8fa22459a0 100644 --- a/src/stores/subgraphNavigationStore.navigateToHash.test.ts +++ b/src/stores/subgraphNavigationStore.navigateToHash.test.ts @@ -4,6 +4,8 @@ import { setActivePinia } from 'pinia' import { beforeEach, describe, expect, it, vi } from 'vitest' import { nextTick, ref } from 'vue' +import type * as VueRouter from 'vue-router' + import type { LGraph, Subgraph } from '@/lib/litegraph/src/litegraph' import type { ComfyWorkflow } from '@/platform/workflow/management/stores/workflowStore' import { app } from '@/scripts/app' @@ -27,9 +29,13 @@ const routerMocks = vi.hoisted(() => ({ const routeHashRef = ref('') -vi.mock('vue-router', () => ({ - useRouter: () => routerMocks -})) +vi.mock('vue-router', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + useRouter: () => routerMocks + } +}) vi.mock('@vueuse/router', () => ({ useRouteHash: () => routeHashRef @@ -72,10 +78,6 @@ vi.mock('@/renderer/core/canvas/canvasStore', () => ({ }) })) -vi.mock('@/utils/graphTraversalUtil', () => ({ - findSubgraphPathById: vi.fn().mockReturnValue(null) -})) - vi.mock('@/services/litegraphService', () => ({ useLitegraphService: () => ({ fitView: vi.fn() }) })) @@ -137,26 +139,26 @@ describe('useSubgraphNavigationStore - navigateToHash validation', () => { expect(routerMocks.replace).not.toHaveBeenCalled() }) - it('redirects URL to root when hash references a deleted subgraph', async () => { + it('redirects to root when hash references a deleted subgraph', async () => { useSubgraphNavigationStore() routeHashRef.value = `#${ids.deletedSubgraph}` - await flushHashWatcher() - - expect(routerMocks.replace).toHaveBeenCalledWith(`#${app.rootGraph.id}`) + await vi.waitFor(() => + expect(routerMocks.replace).toHaveBeenCalledWith(`#${app.rootGraph.id}`) + ) }) - it('redirects URL to root when hash is malformed (not a UUID)', async () => { + it('redirects to root when hash is malformed (not a UUID)', async () => { useSubgraphNavigationStore() routeHashRef.value = '#not-a-valid-uuid' - await flushHashWatcher() - - expect(routerMocks.replace).toHaveBeenCalledWith(`#${app.rootGraph.id}`) + await vi.waitFor(() => + expect(routerMocks.replace).toHaveBeenCalledWith(`#${app.rootGraph.id}`) + ) expect(app.canvas.setGraph).not.toHaveBeenCalled() }) - it('does not redirect when hash is a non-UUID root graph id (e.g. workflow slug)', async () => { + it('does not redirect when hash equals a non-UUID root graph id (loaded workflow slug)', async () => { const slugRootId = 'test-missing-models-in-subgraph' app.rootGraph.id = slugRootId app.canvas.graph = fromPartial({ id: slugRootId }) @@ -169,7 +171,16 @@ describe('useSubgraphNavigationStore - navigateToHash validation', () => { expect(app.canvas.setGraph).not.toHaveBeenCalled() }) - it('does not redirect or re-set graph when hash equals current graph', async () => { + it('redirects when hash is a non-UUID slug that does not match root', async () => { + useSubgraphNavigationStore() + + routeHashRef.value = '#some-other-slug' + await vi.waitFor(() => + expect(routerMocks.replace).toHaveBeenCalledWith(`#${app.rootGraph.id}`) + ) + }) + + it('does not redirect or re-set graph when hash equals current root graph', async () => { app.canvas.graph = fromPartial({ id: ids.root }) useSubgraphNavigationStore() @@ -180,74 +191,70 @@ describe('useSubgraphNavigationStore - navigateToHash validation', () => { expect(routerMocks.replace).not.toHaveBeenCalled() }) - it('redirects to root even when canvas still references a deleted subgraph (stale-graph guard)', async () => { - const stale = makeSubgraph(ids.deletedSubgraph) - app.canvas.graph = stale - useSubgraphNavigationStore() - - routeHashRef.value = `#${ids.deletedSubgraph}` - await flushHashWatcher() - await flushHashWatcher() - - expect(routerMocks.replace).toHaveBeenCalledWith(`#${app.rootGraph.id}`) - expect(app.canvas.setGraph).toHaveBeenCalledWith(app.rootGraph) - }) - - it('recovers canvas to root even if router.replace rejects during redirect', async () => { - routerMocks.replace.mockRejectedValueOnce(new Error('navigation aborted')) - const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) - const stale = makeSubgraph(ids.deletedSubgraph) - app.canvas.graph = stale - useSubgraphNavigationStore() - - routeHashRef.value = `#${ids.deletedSubgraph}` - await flushHashWatcher() - await flushHashWatcher() - - expect(app.canvas.setGraph).toHaveBeenCalledWith(app.rootGraph) - warnSpy.mockRestore() - }) - - it('redirects to root when the empty-hash transition cannot resolve the locator', async () => { - routeHashRef.value = `#${ids.deletedSubgraph}` + it('does not redirect when transitioning to an empty hash on the root graph', async () => { + routeHashRef.value = `#${ids.root}` + app.canvas.graph = fromPartial({ id: ids.root }) useSubgraphNavigationStore() await flushHashWatcher() routerMocks.replace.mockClear() + vi.mocked(app.canvas.setGraph).mockClear() routeHashRef.value = '' await flushHashWatcher() expect(routerMocks.replace).not.toHaveBeenCalled() + expect(app.canvas.setGraph).not.toHaveBeenCalled() }) - it('redirects to root when a workflow loads but the target subgraph is still missing', async () => { + it('redirects when canvas still references a deleted subgraph (stale-graph guard)', async () => { + app.canvas.graph = makeSubgraph(ids.deletedSubgraph) + useSubgraphNavigationStore() + + routeHashRef.value = `#${ids.deletedSubgraph}` + await vi.waitFor(() => { + expect(routerMocks.replace).toHaveBeenCalledWith(`#${app.rootGraph.id}`) + expect(app.canvas.setGraph).toHaveBeenCalledWith(app.rootGraph) + }) + }) + + it('recovers canvas to root even if router.replace rejects', async () => { + routerMocks.replace.mockRejectedValueOnce(new Error('navigation aborted')) + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + app.canvas.graph = makeSubgraph(ids.deletedSubgraph) + useSubgraphNavigationStore() + + routeHashRef.value = `#${ids.deletedSubgraph}` + await vi.waitFor(() => + expect(app.canvas.setGraph).toHaveBeenCalledWith(app.rootGraph) + ) + warnSpy.mockRestore() + }) + + it('redirects when a workflow load resolves but the subgraph is still missing', async () => { const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) workflowStoreState.openWorkflows = [ fromPartial({ path: 'phantom-workflow.json', - filename: 'phantom-workflow.json', activeState: { id: ids.deletedSubgraph, definitions: { subgraphs: [] } } }) ] - useSubgraphNavigationStore() routeHashRef.value = `#${ids.deletedSubgraph}` - await flushHashWatcher() - await flushHashWatcher() - - expect(workflowServiceMocks.openWorkflow).toHaveBeenCalled() - expect(routerMocks.replace).toHaveBeenCalledWith(`#${app.rootGraph.id}`) - expect(warnSpy).toHaveBeenCalledWith( - expect.stringContaining('subgraph not found after workflow load') - ) + await vi.waitFor(() => { + expect(workflowServiceMocks.openWorkflow).toHaveBeenCalled() + expect(routerMocks.replace).toHaveBeenCalledWith(`#${app.rootGraph.id}`) + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('subgraph not found after workflow load') + ) + }) warnSpy.mockRestore() }) - it('redirects to root when openWorkflow rejects', async () => { + it('redirects when openWorkflow rejects during recovery', async () => { const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) workflowServiceMocks.openWorkflow.mockRejectedValueOnce( new Error('load failed') @@ -255,24 +262,46 @@ describe('useSubgraphNavigationStore - navigateToHash validation', () => { workflowStoreState.openWorkflows = [ fromPartial({ path: 'broken-workflow.json', - filename: 'broken-workflow.json', activeState: { id: ids.deletedSubgraph, definitions: { subgraphs: [] } } }) ] - useSubgraphNavigationStore() routeHashRef.value = `#${ids.deletedSubgraph}` - await flushHashWatcher() - await flushHashWatcher() + await vi.waitFor(() => { + expect(routerMocks.replace).toHaveBeenCalledWith(`#${app.rootGraph.id}`) + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('workflow load failed') + ) + }) + warnSpy.mockRestore() + }) - expect(routerMocks.replace).toHaveBeenCalledWith(`#${app.rootGraph.id}`) - expect(warnSpy).toHaveBeenCalledWith( - expect.stringContaining('workflow load failed') - ) + it('routeHash watcher does not re-enter navigateToHash during recovery redirect', async () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + // Simulate the real router replace: trigger the routeHash watcher + // exactly the way vue-router does when the URL is replaced. + routerMocks.replace.mockImplementation((target) => { + const hash = typeof target === 'string' ? target : '' + routeHashRef.value = hash + return Promise.resolve(undefined) + }) + app.canvas.graph = makeSubgraph(ids.deletedSubgraph) + useSubgraphNavigationStore() + + routeHashRef.value = `#${ids.deletedSubgraph}` + await vi.waitFor(() => { + expect(routerMocks.replace).toHaveBeenCalledWith(`#${app.rootGraph.id}`) + }) + + // navigateToHash for the deleted id ran once and produced exactly one + // redirect. The watcher must NOT have fired again for the rewritten + // (root) hash and produced a second redirect. + expect(routerMocks.replace).toHaveBeenCalledTimes(1) + expect(app.canvas.setGraph).toHaveBeenCalledWith(app.rootGraph) warnSpy.mockRestore() }) }) diff --git a/src/stores/subgraphNavigationStore.ts b/src/stores/subgraphNavigationStore.ts index 388dbb3931..b74f55646a 100644 --- a/src/stores/subgraphNavigationStore.ts +++ b/src/stores/subgraphNavigationStore.ts @@ -2,7 +2,11 @@ import QuickLRU from '@alloc/quick-lru' import { useRouteHash } from '@vueuse/router' import { defineStore } from 'pinia' import { computed, ref, shallowRef, watch } from 'vue' -import { useRouter } from 'vue-router' +import { + NavigationFailureType, + isNavigationFailure, + useRouter +} from 'vue-router' import type { DragAndScaleState } from '@/lib/litegraph/src/DragAndScale' import type { Subgraph } from '@/lib/litegraph/src/litegraph' @@ -10,7 +14,7 @@ import { useWorkflowStore } from '@/platform/workflow/management/stores/workflow import { useWorkflowService } from '@/platform/workflow/core/services/workflowService' import { useCanvasStore } from '@/renderer/core/canvas/canvasStore' import { requestSlotLayoutSyncForAllNodes } from '@/renderer/extensions/vueNodes/composables/useSlotElementTracking' -import { isValidSubgraphId } from '@/schemas/subgraphIdSchema' +import { isUuidShapedSubgraphId } from '@/schemas/subgraphIdSchema' import { app } from '@/scripts/app' import { useLitegraphService } from '@/services/litegraphService' import { findSubgraphPathById } from '@/utils/graphTraversalUtil' @@ -201,23 +205,26 @@ export const useSubgraphNavigationStore = defineStore( { flush: 'sync' } ) - //Allow navigation with forward/back buttons. Use a counter so that - //nested/overlapping async navigations don't release suppression early. - let blockHashUpdateDepth = 0 + // Counter so nested/overlapping async navigations don't release + // suppression early; gates both the canvasStore.currentGraph watcher + // (updateHash) and the routeHash watcher to prevent re-entrant + // navigateToHash calls during router.replace(). + let blockNavDepth = 0 let initialLoad = true - async function withHashUpdateBlocked(op: () => Promise): Promise { - blockHashUpdateDepth++ + async function withNavBlocked(op: () => Promise): Promise { + blockNavDepth++ try { return await op() } finally { - blockHashUpdateDepth-- + blockNavDepth-- } } function ensureCanvasOnRoot() { const root = app.rootGraph const canvas = canvasStore.getCanvas() + if (!root || !canvas) return if (canvas.graph?.id !== root.id) canvas.setGraph(root) } @@ -225,14 +232,17 @@ export const useSubgraphNavigationStore = defineStore( const root = app.rootGraph console.warn(`[subgraphNavigation] ${reason}; redirecting to root graph`) try { - await withHashUpdateBlocked(() => router.replace('#' + root.id)) + await withNavBlocked(() => router.replace('#' + root.id)) } catch (err) { - //Router failures shouldn't strand the canvas on a deleted subgraph; - //we still need to recover even when the URL update is rejected. - console.warn( - '[subgraphNavigation] router.replace rejected during recovery', - err - ) + if ( + !isNavigationFailure(err, NavigationFailureType.duplicated) && + !isNavigationFailure(err, NavigationFailureType.cancelled) + ) { + console.warn( + '[subgraphNavigation] router.replace rejected during recovery', + err + ) + } } finally { ensureCanvasOnRoot() } @@ -246,7 +256,7 @@ export const useSubgraphNavigationStore = defineStore( const isRoot = locatorId === root.id const targetGraph = isRoot ? root - : isValidSubgraphId(locatorId) + : isUuidShapedSubgraphId(locatorId) ? root.subgraphs.get(locatorId) : undefined if (targetGraph) { @@ -261,9 +271,12 @@ export const useSubgraphNavigationStore = defineStore( const subgraphs = activeState.definitions?.subgraphs ?? [] for (const graph of [activeState, ...subgraphs]) { if (graph.id !== locatorId) continue - //This will trigger a navigation, which can break forward history + // This will trigger a navigation, which can break forward history. + // After openWorkflow resolves, app.rootGraph has been swapped, so we + // intentionally re-read app.rootGraph below instead of using the + // `root` captured at function entry. try { - await withHashUpdateBlocked(() => + await withNavBlocked(() => useWorkflowService().openWorkflow(workflow) ) } catch (err) { @@ -288,8 +301,18 @@ export const useSubgraphNavigationStore = defineStore( await redirectToRoot(`subgraph not found: ${locatorId}`) } + async function safeRouterCall(op: () => Promise, label: string) { + try { + await op() + } catch (err) { + if (!isNavigationFailure(err, NavigationFailureType.duplicated)) { + console.warn(`[subgraphNavigation] ${label} rejected`, err) + } + } + } + async function updateHash() { - if (blockHashUpdateDepth > 0) return + if (blockNavDepth > 0) return if (initialLoad) { initialLoad = false if (!routeHash.value) return @@ -300,16 +323,22 @@ export const useSubgraphNavigationStore = defineStore( } const newId = canvasStore.getCanvas().graph?.id ?? '' - if (!routeHash.value) await router.replace('#' + app.rootGraph.id) + if (!routeHash.value) { + await safeRouterCall( + () => router.replace('#' + app.rootGraph.id), + 'router.replace' + ) + } const currentId = routeHash.value?.slice(1) if (!newId || newId === currentId) return - await router.push('#' + newId) + await safeRouterCall(() => router.push('#' + newId), 'router.push') } - //update navigation hash - //NOTE: Doesn't apply on workflow load watch(() => canvasStore.currentGraph, updateHash) - watch(routeHash, () => navigateToHash(String(routeHash.value))) + watch(routeHash, () => { + if (blockNavDepth > 0) return + void navigateToHash(String(routeHash.value)) + }) /** Save the current viewport for the active graph/workflow. Called by * workflowService.beforeLoadNewGraph() before the canvas is overwritten. */