mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-03-08 06:30:04 +00:00
fix: spin out workflow tab/load stability regressions (#9345)
## 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 <amp@ampcode.com>
This commit is contained in:
@@ -311,6 +311,172 @@ describe('useWorkflowService', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('saveWorkflow', () => {
|
||||
let workflowStore: ReturnType<typeof useWorkflowStore>
|
||||
|
||||
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<typeof useWorkflowStore>
|
||||
|
||||
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<typeof useWorkflowStore>
|
||||
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<typeof useAppMode>
|
||||
let workflowStore: ReturnType<typeof useWorkflowStore>
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user