From 5327fef29fdbc00afd046959d7771f7d43a3c5e2 Mon Sep 17 00:00:00 2001 From: Alexander Brown Date: Wed, 4 Mar 2026 17:35:18 -0800 Subject: [PATCH] fix: spin out workflow tab/load stability regressions (#9345) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Spin out workflow tab/load stability fixes from the share-by-url branch so they can merge independently and reduce regression risk. ## Changes - **What**: Fixes duplicate tabs on repeated same-workflow loads by making active-workflow reload idempotent in `afterLoadNewGraph`; fixes tab flicker on save/rename by removing async detach/attach gaps in `workflowStore`; hardens duplicate workflow path by loading before clone and assigning a new workflow `id`. ## Review Focus Please review the idempotency gate in `afterLoadNewGraph` (`activeState.id === workflowData.id`) and the save/rename path update sequencing in `workflowStore` to confirm behavior remains correct for restoration and re-import flows. ## Screenshots (if applicable) N/A (workflow logic and tests only) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9345-fix-spin-out-workflow-tab-load-stability-regressions-3186d73d365081fe922bdc61dcf8d8f8) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp --- .../core/services/workflowService.test.ts | 166 ++++++++++++++++++ .../workflow/core/services/workflowService.ts | 31 +++- .../management/stores/workflowStore.ts | 25 +-- 3 files changed, 205 insertions(+), 17 deletions(-) diff --git a/src/platform/workflow/core/services/workflowService.test.ts b/src/platform/workflow/core/services/workflowService.test.ts index 21a2b7205c..9a9df0cb3a 100644 --- a/src/platform/workflow/core/services/workflowService.test.ts +++ b/src/platform/workflow/core/services/workflowService.test.ts @@ -311,6 +311,172 @@ describe('useWorkflowService', () => { }) }) + describe('saveWorkflow', () => { + let workflowStore: ReturnType + + beforeEach(() => { + setActivePinia(createTestingPinia()) + workflowStore = useWorkflowStore() + }) + + it('should delegate to workflowStore.saveWorkflow for persisted workflows', async () => { + const workflow = createModeTestWorkflow({ + path: 'workflows/persisted.json' + }) + vi.mocked(workflowStore.saveWorkflow).mockResolvedValue() + + await useWorkflowService().saveWorkflow(workflow) + + expect(workflowStore.saveWorkflow).toHaveBeenCalledWith(workflow) + }) + + it('should call saveWorkflowAs for temporary workflows', async () => { + const workflow = createModeTestWorkflow({ + path: 'workflows/Unsaved Workflow.json' + }) + Object.defineProperty(workflow, 'isTemporary', { get: () => true }) + vi.spyOn(workflow, 'promptSave').mockResolvedValue(null) + + await useWorkflowService().saveWorkflow(workflow) + + expect(workflowStore.saveWorkflow).not.toHaveBeenCalled() + }) + }) + + describe('saveWorkflowAs', () => { + let workflowStore: ReturnType + + beforeEach(() => { + setActivePinia(createTestingPinia()) + workflowStore = useWorkflowStore() + }) + + it('should rename then save when workflow is temporary', async () => { + const workflow = createModeTestWorkflow({ + path: 'workflows/Unsaved Workflow.json' + }) + Object.defineProperty(workflow, 'isTemporary', { get: () => true }) + vi.mocked(workflowStore.getWorkflowByPath).mockReturnValue(null) + vi.mocked(workflowStore.renameWorkflow).mockResolvedValue() + vi.mocked(workflowStore.saveWorkflow).mockResolvedValue() + + const result = await useWorkflowService().saveWorkflowAs(workflow, { + filename: 'my-workflow' + }) + + expect(result).toBe(true) + expect(workflowStore.renameWorkflow).toHaveBeenCalledWith( + workflow, + 'workflows/my-workflow.json' + ) + expect(workflowStore.saveWorkflow).toHaveBeenCalledWith(workflow) + }) + + it('should return false when no filename is provided', async () => { + const workflow = createModeTestWorkflow({ + path: 'workflows/test.json' + }) + vi.spyOn(workflow, 'promptSave').mockResolvedValue(null) + + const result = await useWorkflowService().saveWorkflowAs(workflow) + + expect(result).toBe(false) + expect(workflowStore.saveWorkflow).not.toHaveBeenCalled() + }) + }) + + describe('afterLoadNewGraph', () => { + let workflowStore: ReturnType + let existingWorkflow: LoadedComfyWorkflow + + beforeEach(() => { + setActivePinia(createTestingPinia()) + workflowStore = useWorkflowStore() + existingWorkflow = createModeTestWorkflow({ + path: 'workflows/repeat.json' + }) + vi.mocked(workflowStore.getWorkflowByPath).mockReturnValue( + existingWorkflow + ) + vi.mocked(workflowStore.isActive).mockReturnValue(true) + vi.mocked(workflowStore.openWorkflow).mockResolvedValue(existingWorkflow) + }) + + it('should reuse the active workflow when loading the same path repeatedly', async () => { + const workflowId = 'repeat-workflow-id' + existingWorkflow.changeTracker.activeState.id = workflowId + + await useWorkflowService().afterLoadNewGraph('repeat', { + id: workflowId, + nodes: [{ id: 1, type: 'TestNode', pos: [0, 0], size: [100, 100] }] + } as never) + + expect(workflowStore.getWorkflowByPath).toHaveBeenCalledWith( + 'workflows/repeat.json' + ) + expect(workflowStore.openWorkflow).toHaveBeenCalledWith(existingWorkflow) + expect(existingWorkflow.changeTracker.reset).toHaveBeenCalled() + expect(existingWorkflow.changeTracker.restore).toHaveBeenCalled() + expect(workflowStore.createNewTemporary).not.toHaveBeenCalled() + }) + + it('should reuse active workflow for repeated same-path loads without ids', async () => { + await useWorkflowService().afterLoadNewGraph('repeat', { + nodes: [{ id: 1, type: 'TestNode', pos: [0, 0], size: [100, 100] }] + } as never) + + expect(workflowStore.getWorkflowByPath).toHaveBeenCalledWith( + 'workflows/repeat.json' + ) + expect(workflowStore.openWorkflow).toHaveBeenCalledWith(existingWorkflow) + expect(existingWorkflow.changeTracker.reset).toHaveBeenCalled() + expect(existingWorkflow.changeTracker.restore).toHaveBeenCalled() + expect(workflowStore.createNewTemporary).not.toHaveBeenCalled() + }) + + it('should reuse active workflow when only one side has an id', async () => { + existingWorkflow.changeTracker.activeState.id = 'existing-id' + + await useWorkflowService().afterLoadNewGraph('repeat', { + nodes: [{ id: 1, type: 'TestNode', pos: [0, 0], size: [100, 100] }] + } as never) + + expect(workflowStore.openWorkflow).toHaveBeenCalledWith(existingWorkflow) + expect(existingWorkflow.changeTracker.reset).toHaveBeenCalled() + expect(existingWorkflow.changeTracker.restore).toHaveBeenCalled() + expect(workflowStore.createNewTemporary).not.toHaveBeenCalled() + }) + + it('should reuse active workflow when only workflowData has an id', async () => { + await useWorkflowService().afterLoadNewGraph('repeat', { + id: 'incoming-id', + nodes: [{ id: 1, type: 'TestNode', pos: [0, 0], size: [100, 100] }] + } as never) + + expect(workflowStore.openWorkflow).toHaveBeenCalledWith(existingWorkflow) + expect(existingWorkflow.changeTracker.reset).toHaveBeenCalled() + expect(existingWorkflow.changeTracker.restore).toHaveBeenCalled() + expect(workflowStore.createNewTemporary).not.toHaveBeenCalled() + }) + + it('should create new temporary when ids differ', async () => { + existingWorkflow.changeTracker.activeState.id = 'existing-id' + + const tempWorkflow = createModeTestWorkflow({ + path: 'workflows/repeat (2).json' + }) + vi.mocked(workflowStore.createNewTemporary).mockReturnValue(tempWorkflow) + vi.mocked(workflowStore.openWorkflow).mockResolvedValue(tempWorkflow) + + await useWorkflowService().afterLoadNewGraph('repeat', { + id: 'different-id', + nodes: [{ id: 1, type: 'TestNode', pos: [0, 0], size: [100, 100] }] + } as never) + + expect(workflowStore.createNewTemporary).toHaveBeenCalled() + }) + }) + describe('per-workflow mode switching', () => { let appMode: ReturnType let workflowStore: ReturnType diff --git a/src/platform/workflow/core/services/workflowService.ts b/src/platform/workflow/core/services/workflowService.ts index 53047c19b3..e0625d4db5 100644 --- a/src/platform/workflow/core/services/workflowService.ts +++ b/src/platform/workflow/core/services/workflowService.ts @@ -24,7 +24,11 @@ import type { AppMode } from '@/composables/useAppMode' import { useDomWidgetStore } from '@/stores/domWidgetStore' import { useExecutionErrorStore } from '@/stores/executionErrorStore' import { useWorkspaceStore } from '@/stores/workspaceStore' -import { appendJsonExt, appendWorkflowJsonExt } from '@/utils/formatUtil' +import { + appendJsonExt, + appendWorkflowJsonExt, + generateUUID +} from '@/utils/formatUtil' function linearModeToAppMode(linearMode: unknown): AppMode | null { if (typeof linearMode !== 'boolean') return null @@ -415,10 +419,24 @@ export const useWorkflowService = () => { const fullPath = ComfyWorkflow.basePath + appendJsonExt(path) const existingWorkflow = workflowStore.getWorkflowByPath(fullPath) - // If the workflow exists and is NOT loaded yet (restoration case), - // use the existing workflow instead of creating a new one. - // If it IS loaded, this is a re-import case - create new with suffix. - if (existingWorkflow?.isPersisted && !existingWorkflow.isLoaded) { + // Reuse an existing workflow when this is a restoration case + // (persisted but currently unloaded) or an idempotent repeated load + // of the currently active same-path workflow. + // + // This prevents accidental duplicate tabs when startup/load flows + // invoke loadGraphData more than once for the same workflow name. + const isSameActiveWorkflowLoad = + !!existingWorkflow && + workflowStore.isActive(existingWorkflow) && + (existingWorkflow.activeState?.id === undefined || + workflowData.id === undefined || + existingWorkflow.activeState.id === workflowData.id) + + if ( + existingWorkflow && + ((existingWorkflow.isPersisted && !existingWorkflow.isLoaded) || + isSameActiveWorkflowLoad) + ) { const loadedWorkflow = await workflowStore.openWorkflow(existingWorkflow) if (loadedWorkflow.initialMode === undefined) { @@ -499,7 +517,10 @@ export const useWorkflowService = () => { * Takes an existing workflow and duplicates it with a new name */ const duplicateWorkflow = async (workflow: ComfyWorkflow) => { + if (!workflow.isLoaded) await workflow.load() const state = JSON.parse(JSON.stringify(workflow.activeState)) + // Ensure duplicates are always treated as distinct workflows. + if (state) state.id = generateUUID() const suffix = workflow.isPersisted ? ' (Copy)' : '' // Remove the suffix `(2)` or similar const filename = workflow.filename.replace(/\s*\(\d+\)$/, '') + suffix diff --git a/src/platform/workflow/management/stores/workflowStore.ts b/src/platform/workflow/management/stores/workflowStore.ts index b618147f10..e493ff882b 100644 --- a/src/platform/workflow/management/stores/workflowStore.ts +++ b/src/platform/workflow/management/stores/workflowStore.ts @@ -479,12 +479,15 @@ export const useWorkflowStore = defineStore('workflow', () => { const wasBookmarked = bookmarkStore.isBookmarked(oldPath) const draftStore = useWorkflowDraftStore() - const openIndex = detachWorkflow(workflow) - // Perform the actual rename operation first - try { - await workflow.rename(newPath) - } finally { - attachWorkflow(workflow, openIndex) + await workflow.rename(newPath) + + // Synchronously swap old path for new path in lookup and open paths + // to avoid a tab flicker caused by an async gap between detach/attach. + delete workflowLookup.value[oldPath] + workflowLookup.value[workflow.path] = workflow + const openIndex = openWorkflowPaths.value.indexOf(oldPath) + if (openIndex !== -1) { + openWorkflowPaths.value.splice(openIndex, 1, workflow.path) } draftStore.moveDraft(oldPath, newPath, workflow.key) @@ -525,13 +528,11 @@ export const useWorkflowStore = defineStore('workflow', () => { const saveWorkflow = async (workflow: ComfyWorkflow) => { isBusy.value = true try { - // Detach the workflow and re-attach to force refresh the tree objects. + await workflow.save() + // Synchronously detach and re-attach to force refresh the tree objects + // without an async gap that would cause the tab to disappear. const openIndex = detachWorkflow(workflow) - try { - await workflow.save() - } finally { - attachWorkflow(workflow, openIndex) - } + attachWorkflow(workflow, openIndex) } finally { isBusy.value = false }