fix(subgraph): close re-entry + router-failure gaps; harden tests

Multi-agent review addressed:

Code:
- Gate routeHash watcher behind blockNavDepth so router.replace() during
  redirectToRoot() can't re-enter navigateToHash and double-fire setGraph.
- Rename blockHashUpdateDepth -> blockNavDepth (now gates both watchers).
- ensureCanvasOnRoot(): null-guard rootGraph and canvas so teardown/HMR
  paths can't throw inside a finally and swallow the original error.
- Catch router.replace rejections in redirectToRoot and updateHash; treat
  isNavigationFailure(duplicated/cancelled) as benign.
- Wrap router.push/replace in updateHash() with safeRouterCall to stop
  bare-await rejections from becoming unhandled promise rejections.
- Rename isValidSubgraphId -> isUuidShapedSubgraphId so the predicate's
  scope ('eligible for root.subgraphs.get') is on the tin.

Tests:
- Add regression test asserting the routeHash watcher does NOT re-enter
  navigateToHash when router.replace() rewrites the hash during recovery.
- Strengthen empty-hash test: transition from non-empty -> empty, assert
  no redirect AND no canvas reset.
- Strengthen stale-canvas test: assert setGraph(rootGraph) in addition to
  router.replace.
- Add 'non-UUID slug that doesn't match root' test to lock in the
  validation path (the slug-equals-root test alone passes without any
  validator).
- Replace flushHashWatcher double-calls with vi.waitFor for determinism.
- Drop unused findSubgraphPathById mock (not consulted in any nav path).

E2E:
- Wait for the store's initial hash/route sync before mutating
  location.hash to avoid racing the initialLoad branch.
- Add an explicit timeout to the URL-hash poll so a missed redirect
  fails fast on loaded CI.

Schema:
- Trim docstring to the only non-obvious bit (untrusted-input boundary).
- Dedupe canonical UUID literal in the test.
This commit is contained in:
Glary-Bot
2026-05-21 13:27:08 +00:00
parent 6dc9509892
commit d71db8fc35
5 changed files with 200 additions and 118 deletions

View File

@@ -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)
})
}
)

View File

@@ -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)
})
})
})

View File

@@ -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<typeof zSubgraphId>
export function isValidSubgraphId(value: unknown): value is SubgraphId {
export function isUuidShapedSubgraphId(value: unknown): value is SubgraphId {
return zSubgraphId.safeParse(value).success
}

View File

@@ -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<typeof VueRouter>()
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<LGraph>({ 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<LGraph>({ 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<LGraph>({ 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<ComfyWorkflow>({
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<ComfyWorkflow>({
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()
})
})

View File

@@ -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<T>(op: () => Promise<T>): Promise<T> {
blockHashUpdateDepth++
async function withNavBlocked<T>(op: () => Promise<T>): Promise<T> {
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<unknown>, 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. */