Skip to content

Commit fb030dc

Browse files
christian-byrneGlary-Botampagent
authored
fix: lazily migrate non-UUID workflow ids to a fresh UUID (#11631)
*PR Created by the Glary-Bot Agent* --- ## Summary Regenerate the top-level workflow `id` on load when it is present but not a canonical UUID, so legacy slug ids from custom-node templates no longer survive round-trip save/publish and break the cloud share loader's zod validation. ## Changes - **What**: - Added `isValidUuid` helper in `packages/shared-frontend-utils/src/formatUtil.ts`. - Tightened `ensureWorkflowId` in `workflowStore.ts`: `!base.id` → `!isValidUuid(base.id)`, so slug-shaped ids get replaced with a fresh UUID, not just missing ones. - Normalized non-UUID ids to `undefined` in the same-path reuse equality check in `afterLoadNewGraph` so the first-load-rewrites, second-load-reuses flow works and does not open a duplicate tab. - Unit coverage for `isValidUuid`, for the new migration branch of `createTemporary`, and two regression tests for the same-path reuse fix (slug-to-UUID rewrite; both-sides-legacy-slug). - **Breaking**: none. All missing-id cases still produce a UUID; valid UUIDs still pass through unchanged. Only new behavior is replacing slug ids instead of keeping them. ## Review Focus - **Subgraph references are untouched.** Only the top-level `workflow.id` is migrated. `zSubgraphDefinition.id` and the `node.type` UUIDs that reference it are left alone, because subgraph-instance containers reference their definitions by UUID and regenerating those WOULD orphan instances. This migration is safe only for the outer id. - **Same-path reuse normalization.** Initial review flagged that rewriting the id on first load would break same-path reuse when the second load arrives with the original slug. `normalizeWorkflowIdForReuse` collapses non-UUID ids to `undefined` for the equality check; regression tests cover both the mixed (slug vs fresh UUID) and both-sides-slug cases. - **Permissive UUID regex.** `isValidUuid` matches the canonical 8-4-4-4-12 hex form case-insensitively for any version/variant, not just v4. Some historical workflows carry non-v4 UUIDs that were generated by older code paths; accepting them avoids triggering another round of re-migration that would orphan graph identity across saves. ## Background Investigated in `#live-ops`: a publicly shared SAM3 workflow failed to load with `Invalid uuid at "id"` because the workflow JSON had `"id": "video-point-prompt-example"` — a slug carried over from `PozzettiAndrea/ComfyUI-SAM3/workflows/video_point_prompt.json`. Today's frontend enforces `z.string().uuid().optional()` on load via the cloud share service, which throws when a legacy slug is present. See the thread for full git-archaeology and subgraph-reference audit. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11631-fix-lazily-migrate-non-UUID-workflow-ids-to-a-fresh-UUID-34d6d73d365081c79bc8e1d709e0c2f9) by [Unito](https://www.unito.io) --------- Co-authored-by: Glary-Bot <glary-bot@users.noreply.github.com> Co-authored-by: Amp <amp@ampcode.com>
1 parent 3f1950f commit fb030dc

9 files changed

Lines changed: 428 additions & 88 deletions

File tree

‎packages/shared-frontend-utils/src/formatUtil.test.ts‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
escapeI18nMessage,
88
formatLocalizedMediumDate,
99
formatLocalizedNumber,
10+
generateUUID,
1011
getFilePathSeparatorVariants,
1112
getFilenameDetails,
1213
getMediaTypeFromFilename,
@@ -15,6 +16,7 @@ import {
1516
isCivitaiModelUrl,
1617
isCivitaiUrl,
1718
isPreviewableMediaType,
19+
isValidUuid,
1820
joinFilePath,
1921
truncateFilename
2022
} from './formatUtil'
@@ -514,6 +516,32 @@ describe('formatUtil', () => {
514516
})
515517
})
516518

519+
describe('isValidUuid', () => {
520+
it.for([
521+
['lowercase', '9cea40bb-b0cf-4b40-a758-8935cfe8d52f'],
522+
['uppercase', '9CEA40BB-B0CF-4B40-A758-8935CFE8D52F'],
523+
['nil', '00000000-0000-0000-0000-000000000000'],
524+
['arbitrary version and variant', 'ffffffff-ffff-7fff-ffff-ffffffffffff'],
525+
['generated', generateUUID()]
526+
])('accepts a %s UUID', ([, value]) => {
527+
expect(isValidUuid(value)).toBe(true)
528+
})
529+
530+
it.for([
531+
['legacy slug', 'video-point-prompt-example'],
532+
['missing value', undefined],
533+
['null', null],
534+
['empty string', ''],
535+
['non-string', 123],
536+
['wrong length', '9cea40bb-b0cf-4b40-a758-8935cfe8d52'],
537+
['missing separators', '9cea40bbb0cf4b40a7588935cfe8d52f'],
538+
['surrounding whitespace', ' 9cea40bb-b0cf-4b40-a758-8935cfe8d52f'],
539+
['non-hex character', 'gcea40bb-b0cf-4b40-a758-8935cfe8d52f']
540+
])('rejects a %s', ([, value]) => {
541+
expect(isValidUuid(value)).toBe(false)
542+
})
543+
})
544+
517545
describe('escapeI18nMessage', () => {
518546
it('wraps message-syntax characters in literal interpolations', () => {
519547
expect(escapeI18nMessage('a@b')).toBe("a{'@'}b")

‎packages/shared-frontend-utils/src/formatUtil.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -403,6 +403,13 @@ export const paramsToCacheKey = (params: unknown): string => {
403403
return String(params)
404404
}
405405

406+
const UUID_PATTERN =
407+
/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i
408+
409+
/** Accepts canonical UUIDs of any version or variant for legacy compatibility. */
410+
export const isValidUuid = (value: unknown): value is string =>
411+
typeof value === 'string' && UUID_PATTERN.test(value)
412+
406413
/**
407414
* Generates a RFC4122 compliant UUID v4 using the native crypto API when available
408415
* @returns A properly formatted UUID string

‎src/platform/workflow/core/services/workflowService.test.ts‎

Lines changed: 114 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import { useAppMode } from '@/composables/useAppMode'
2222
import type { ComfyWorkflowJSON } from '@/platform/workflow/validation/schemas/workflowSchema'
2323
import { createMockChangeTracker } from '@/utils/__tests__/litegraphTestUtils'
2424
import type { AppMode } from '@/utils/appMode'
25+
import { isValidUuid } from '@/utils/formatUtil'
2526
import { t } from '@/i18n'
2627

2728
function createModeTestWorkflow(
@@ -62,6 +63,10 @@ function makeWorkflowData(
6263
}
6364
}
6465

66+
function makeWorkflowDataWithId(id: string): ComfyWorkflowJSON {
67+
return { ...makeWorkflowData(), id }
68+
}
69+
6570
const { mockConfirm, mockTrackWorkflowSaved } = vi.hoisted(() => ({
6671
mockConfirm: vi.fn(),
6772
mockTrackWorkflowSaved: vi.fn()
@@ -591,6 +596,9 @@ describe('useWorkflowService', () => {
591596
)
592597
vi.mocked(workflowStore.isActive).mockReturnValue(true)
593598
vi.mocked(workflowStore.openWorkflow).mockResolvedValue(existingWorkflow)
599+
vi.mocked(workflowStore.createNewTemporary).mockReturnValue(
600+
createModeTestWorkflow({ path: 'workflows/repeat (2).json' })
601+
)
594602
})
595603

596604
it('should restore the stashed previews of the newly active workflow', async () => {
@@ -603,14 +611,14 @@ describe('useWorkflowService', () => {
603611
).toHaveBeenCalledWith(existingWorkflow.path)
604612
})
605613

606-
it('should reuse the active workflow when loading the same path repeatedly', async () => {
607-
const workflowId = 'repeat-workflow-id'
614+
it('should reuse equivalent UUIDs regardless of casing', async () => {
615+
const workflowId = '9cea40bb-b0cf-4b40-a758-8935cfe8d52f'
608616
existingWorkflow.changeTracker.activeState.id = workflowId
609617

610-
await useWorkflowService().afterLoadNewGraph('repeat', {
611-
id: workflowId,
612-
nodes: [{ id: 1, type: 'TestNode', pos: [0, 0], size: [100, 100] }]
613-
} as never)
618+
await useWorkflowService().afterLoadNewGraph(
619+
'repeat',
620+
makeWorkflowDataWithId(workflowId.toUpperCase())
621+
)
614622

615623
expect(workflowStore.getWorkflowByPath).toHaveBeenCalledWith(
616624
'workflows/repeat.json'
@@ -622,9 +630,7 @@ describe('useWorkflowService', () => {
622630
})
623631

624632
it('should reuse active workflow for repeated same-path loads without ids', async () => {
625-
await useWorkflowService().afterLoadNewGraph('repeat', {
626-
nodes: [{ id: 1, type: 'TestNode', pos: [0, 0], size: [100, 100] }]
627-
} as never)
633+
await useWorkflowService().afterLoadNewGraph('repeat', makeWorkflowData())
628634

629635
expect(workflowStore.getWorkflowByPath).toHaveBeenCalledWith(
630636
'workflows/repeat.json'
@@ -636,11 +642,10 @@ describe('useWorkflowService', () => {
636642
})
637643

638644
it('should reuse active workflow when only one side has an id', async () => {
639-
existingWorkflow.changeTracker.activeState.id = 'existing-id'
645+
existingWorkflow.changeTracker.activeState.id =
646+
'9cea40bb-b0cf-4b40-a758-8935cfe8d52f'
640647

641-
await useWorkflowService().afterLoadNewGraph('repeat', {
642-
nodes: [{ id: 1, type: 'TestNode', pos: [0, 0], size: [100, 100] }]
643-
} as never)
648+
await useWorkflowService().afterLoadNewGraph('repeat', makeWorkflowData())
644649

645650
expect(workflowStore.openWorkflow).toHaveBeenCalledWith(existingWorkflow)
646651
expect(existingWorkflow.changeTracker.reset).toHaveBeenCalled()
@@ -649,10 +654,10 @@ describe('useWorkflowService', () => {
649654
})
650655

651656
it('should reuse active workflow when only workflowData has an id', async () => {
652-
await useWorkflowService().afterLoadNewGraph('repeat', {
653-
id: 'incoming-id',
654-
nodes: [{ id: 1, type: 'TestNode', pos: [0, 0], size: [100, 100] }]
655-
} as never)
657+
await useWorkflowService().afterLoadNewGraph(
658+
'repeat',
659+
makeWorkflowDataWithId('9cea40bb-b0cf-4b40-a758-8935cfe8d52f')
660+
)
656661

657662
expect(workflowStore.openWorkflow).toHaveBeenCalledWith(existingWorkflow)
658663
expect(existingWorkflow.changeTracker.reset).toHaveBeenCalled()
@@ -661,18 +666,13 @@ describe('useWorkflowService', () => {
661666
})
662667

663668
it('should create new temporary when ids differ', async () => {
664-
existingWorkflow.changeTracker.activeState.id = 'existing-id'
669+
existingWorkflow.changeTracker.activeState.id =
670+
'9cea40bb-b0cf-4b40-a758-8935cfe8d52f'
665671

666-
const tempWorkflow = createModeTestWorkflow({
667-
path: 'workflows/repeat (2).json'
668-
})
669-
vi.mocked(workflowStore.createNewTemporary).mockReturnValue(tempWorkflow)
670-
vi.mocked(workflowStore.openWorkflow).mockResolvedValue(tempWorkflow)
671-
672-
await useWorkflowService().afterLoadNewGraph('repeat', {
673-
id: 'different-id',
674-
nodes: [{ id: 1, type: 'TestNode', pos: [0, 0], size: [100, 100] }]
675-
} as never)
672+
await useWorkflowService().afterLoadNewGraph(
673+
'repeat',
674+
makeWorkflowDataWithId('11111111-2222-3333-4444-555555555555')
675+
)
676676

677677
expect(workflowStore.createNewTemporary).toHaveBeenCalled()
678678
})
@@ -687,7 +687,7 @@ describe('useWorkflowService', () => {
687687

688688
await useWorkflowService().afterLoadNewGraph(
689689
'shared',
690-
{ nodes: [] } as never,
690+
makeWorkflowData(),
691691
'share-1'
692692
)
693693

@@ -697,19 +697,18 @@ describe('useWorkflowService', () => {
697697
it('preserves share attribution on repeated same-path loads', async () => {
698698
existingWorkflow.shareId = 'share-1'
699699

700-
await useWorkflowService().afterLoadNewGraph('repeat', {
701-
nodes: [{ id: 1, type: 'TestNode', pos: [0, 0], size: [100, 100] }]
702-
} as never)
700+
await useWorkflowService().afterLoadNewGraph('repeat', makeWorkflowData())
703701

704702
expect(existingWorkflow.shareId).toBe('share-1')
705703
})
706704

707705
it('preserves share attribution on workflow object reloads', async () => {
708706
existingWorkflow.shareId = 'share-1'
709707

710-
await useWorkflowService().afterLoadNewGraph(existingWorkflow, {
711-
nodes: [{ id: 1, type: 'TestNode', pos: [0, 0], size: [100, 100] }]
712-
} as never)
708+
await useWorkflowService().afterLoadNewGraph(
709+
existingWorkflow,
710+
makeWorkflowData()
711+
)
713712

714713
expect(existingWorkflow.shareId).toBe('share-1')
715714
})
@@ -719,9 +718,7 @@ describe('useWorkflowService', () => {
719718

720719
await useWorkflowService().afterLoadNewGraph(
721720
'repeat',
722-
{
723-
nodes: [{ id: 1, type: 'TestNode', pos: [0, 0], size: [100, 100] }]
724-
} as never,
721+
makeWorkflowData(),
725722
'share-2'
726723
)
727724

@@ -733,14 +730,90 @@ describe('useWorkflowService', () => {
733730

734731
await useWorkflowService().afterLoadNewGraph(
735732
existingWorkflow,
736-
{
737-
nodes: [{ id: 1, type: 'TestNode', pos: [0, 0], size: [100, 100] }]
738-
} as never,
733+
makeWorkflowData(),
739734
'share-2'
740735
)
741736

742737
expect(existingWorkflow.shareId).toBe('share-2')
743738
})
739+
740+
it('reuses a migrated workflow only for its original legacy id', async () => {
741+
const existingUuid = '9cea40bb-b0cf-4b40-a758-8935cfe8d52f'
742+
existingWorkflow.changeTracker.activeState.id = existingUuid
743+
existingWorkflow.legacyId = 'video-point-prompt-example'
744+
745+
await useWorkflowService().afterLoadNewGraph(
746+
'repeat',
747+
makeWorkflowDataWithId('video-point-prompt-example')
748+
)
749+
750+
expect(workflowStore.openWorkflow).toHaveBeenCalledWith(existingWorkflow)
751+
expect(existingWorkflow.changeTracker.reset).toHaveBeenCalledWith(
752+
expect.objectContaining({ id: existingUuid })
753+
)
754+
expect(existingWorkflow.changeTracker.restore).toHaveBeenCalled()
755+
expect(workflowStore.createNewTemporary).not.toHaveBeenCalled()
756+
})
757+
758+
it.for([
759+
{
760+
label: 'a different legacy id',
761+
existingId: 'legacy-workflow-name',
762+
incomingId: 'different-legacy-name'
763+
},
764+
{
765+
label: 'an unrelated legacy id after migration',
766+
existingId: '9cea40bb-b0cf-4b40-a758-8935cfe8d52f',
767+
incomingId: 'different-legacy-name',
768+
legacyId: 'legacy-workflow-name'
769+
}
770+
])(
771+
'opens a new tab for $label',
772+
async ({ existingId, incomingId, legacyId }) => {
773+
existingWorkflow.changeTracker.activeState.id = existingId
774+
existingWorkflow.legacyId = legacyId
775+
776+
await useWorkflowService().afterLoadNewGraph(
777+
'repeat',
778+
makeWorkflowDataWithId(incomingId)
779+
)
780+
781+
expect(workflowStore.createNewTemporary).toHaveBeenCalled()
782+
expect(existingWorkflow.changeTracker.reset).not.toHaveBeenCalled()
783+
}
784+
)
785+
786+
it('migrates a workflow-object reload and records its legacy id', async () => {
787+
const existingUuid = '9cea40bb-b0cf-4b40-a758-8935cfe8d52f'
788+
existingWorkflow.changeTracker.activeState.id = existingUuid
789+
790+
await useWorkflowService().afterLoadNewGraph(
791+
existingWorkflow,
792+
makeWorkflowDataWithId('video-point-prompt-example')
793+
)
794+
795+
expect(workflowStore.openWorkflow).toHaveBeenCalledWith(existingWorkflow)
796+
expect(existingWorkflow.changeTracker.reset).toHaveBeenCalledWith(
797+
expect.objectContaining({ id: existingUuid })
798+
)
799+
expect(existingWorkflow.legacyId).toBe('video-point-prompt-example')
800+
expect(existingWorkflow.changeTracker.restore).toHaveBeenCalled()
801+
})
802+
803+
it('generates a fresh UUID when a workflow-object reload has no valid id', async () => {
804+
existingWorkflow.changeTracker.activeState.id = 'legacy-workflow-name'
805+
806+
await useWorkflowService().afterLoadNewGraph(
807+
existingWorkflow,
808+
makeWorkflowDataWithId('different-legacy-name')
809+
)
810+
811+
const resetArg = vi.mocked(existingWorkflow.changeTracker.reset).mock
812+
.calls[0]?.[0]
813+
expect(isValidUuid(resetArg?.id)).toBe(true)
814+
expect(resetArg?.id).not.toBe('different-legacy-name')
815+
expect(resetArg?.id).not.toBe('legacy-workflow-name')
816+
})
744817
})
745818

746819
describe('per-workflow mode switching', () => {

‎src/platform/workflow/core/services/workflowService.ts‎

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,11 @@ import {
1010
normalizePendingWarnings,
1111
updatePendingWarnings
1212
} from '@/platform/workflow/core/utils/pendingWarnings'
13+
import {
14+
areWorkflowIdsEquivalent,
15+
ensureWorkflowId,
16+
getLegacyWorkflowId
17+
} from '@/platform/workflow/core/utils/workflowId'
1318
import { useWorkflowDraftStoreV2 } from '@/platform/workflow/persistence/stores/workflowDraftStoreV2'
1419
import {
1520
ComfyWorkflow,
@@ -493,12 +498,15 @@ export const useWorkflowService = () => {
493498
//
494499
// This prevents accidental duplicate tabs when startup/load flows
495500
// invoke loadGraphData more than once for the same workflow name.
501+
const existingId = existingWorkflow?.activeState?.id
496502
const isSameActiveWorkflowLoad =
497503
!!existingWorkflow &&
498504
workflowStore.isActive(existingWorkflow) &&
499-
(existingWorkflow.activeState?.id === undefined ||
500-
workflowData.id === undefined ||
501-
existingWorkflow.activeState.id === workflowData.id)
505+
areWorkflowIdsEquivalent(
506+
existingId,
507+
workflowData.id,
508+
existingWorkflow.legacyId
509+
)
502510

503511
if (
504512
existingWorkflow &&
@@ -519,7 +527,10 @@ export const useWorkflowService = () => {
519527
if (shareId) {
520528
loadedWorkflow.shareId = shareId
521529
}
522-
loadedWorkflow.changeTracker.reset(workflowData)
530+
loadedWorkflow.legacyId ??= getLegacyWorkflowId(workflowData.id)
531+
loadedWorkflow.changeTracker.reset(
532+
ensureWorkflowId(workflowData, loadedWorkflow.activeState?.id)
533+
)
523534
loadedWorkflow.changeTracker.restore()
524535
return
525536
}
@@ -546,7 +557,10 @@ export const useWorkflowService = () => {
546557
loadedWorkflow.initialMode = freshLoadMode
547558
trackIfEnteringApp(loadedWorkflow)
548559
}
549-
loadedWorkflow.changeTracker.reset(workflowData)
560+
loadedWorkflow.legacyId ??= getLegacyWorkflowId(workflowData.id)
561+
loadedWorkflow.changeTracker.reset(
562+
ensureWorkflowId(workflowData, loadedWorkflow.activeState?.id)
563+
)
550564
loadedWorkflow.changeTracker.restore()
551565
}
552566

0 commit comments

Comments
 (0)