fix save as not using correct extension or persisting mode on change

This commit is contained in:
pythongosssss
2026-03-28 07:10:22 -07:00
parent 82242f1b00
commit 4ef2ce96bc
6 changed files with 305 additions and 83 deletions

View File

@@ -6,6 +6,7 @@ import { useBuilderSave } from './useBuilderSave'
const mockSetMode = vi.hoisted(() => vi.fn())
const mockToastErrorHandler = vi.hoisted(() => vi.fn())
const mockTrackEnterLinear = vi.hoisted(() => vi.fn())
const mockTrackDefaultViewSet = vi.hoisted(() => vi.fn())
const mockSaveWorkflow = vi.hoisted(() => vi.fn<() => Promise<void>>())
const mockSaveWorkflowAs = vi.hoisted(() =>
vi.fn<() => Promise<boolean | null>>()
@@ -13,7 +14,6 @@ const mockSaveWorkflowAs = vi.hoisted(() =>
const mockShowLayoutDialog = vi.hoisted(() => vi.fn())
const mockShowConfirmDialog = vi.hoisted(() => vi.fn())
const mockCloseDialog = vi.hoisted(() => vi.fn())
const mockSetWorkflowDefaultView = vi.hoisted(() => vi.fn())
const mockExitBuilder = vi.hoisted(() => vi.fn())
const mockActiveWorkflow = ref<{
@@ -30,7 +30,10 @@ vi.mock('@/composables/useErrorHandling', () => ({
}))
vi.mock('@/platform/telemetry', () => ({
useTelemetry: () => ({ trackEnterLinear: mockTrackEnterLinear })
useTelemetry: () => ({
trackEnterLinear: mockTrackEnterLinear,
trackDefaultViewSet: mockTrackDefaultViewSet
})
}))
vi.mock('@/platform/workflow/core/services/workflowService', () => ({
@@ -60,10 +63,6 @@ vi.mock('@/stores/dialogStore', () => ({
useDialogStore: () => ({ closeDialog: mockCloseDialog })
}))
vi.mock('./builderViewOptions', () => ({
setWorkflowDefaultView: mockSetWorkflowDefaultView
}))
vi.mock('@/components/dialog/confirm/confirmDialog', () => ({
showConfirmDialog: mockShowConfirmDialog
}))
@@ -190,7 +189,7 @@ describe('useBuilderSave', () => {
}
}
it('onSave calls saveWorkflowAs then setWorkflowDefaultView on success', async () => {
it('onSave calls saveWorkflowAs with isApp and tracks telemetry', async () => {
mockSaveWorkflowAs.mockResolvedValueOnce(true)
const { onSave } = getSaveDialogProps()
@@ -199,35 +198,40 @@ describe('useBuilderSave', () => {
expect(mockSaveWorkflowAs).toHaveBeenCalledWith(
mockActiveWorkflow.value,
{
filename: 'new-name'
filename: 'new-name',
isApp: true
}
)
expect(mockSetWorkflowDefaultView).toHaveBeenCalledWith(
mockActiveWorkflow.value,
true
)
expect(mockTrackDefaultViewSet).toHaveBeenCalledWith({
default_view: 'app'
})
})
it('onSave uses fresh activeWorkflow reference for setWorkflowDefaultView', async () => {
const newWorkflow = { filename: 'new-name', initialMode: 'app' }
mockSaveWorkflowAs.mockImplementationOnce(async () => {
mockActiveWorkflow.value = newWorkflow
return true
})
it('onSave passes isApp: false when saving as graph', async () => {
mockSaveWorkflowAs.mockResolvedValueOnce(true)
const { onSave } = getSaveDialogProps()
await onSave('new-name', true)
await onSave('new-name', false)
expect(mockSetWorkflowDefaultView).toHaveBeenCalledWith(newWorkflow, true)
expect(mockSaveWorkflowAs).toHaveBeenCalledWith(
mockActiveWorkflow.value,
{
filename: 'new-name',
isApp: false
}
)
expect(mockTrackDefaultViewSet).toHaveBeenCalledWith({
default_view: 'graph'
})
})
it('onSave does not mutate or close when saveWorkflowAs returns falsy', async () => {
it('onSave does not track or close when saveWorkflowAs returns falsy', async () => {
mockSaveWorkflowAs.mockResolvedValueOnce(null)
const { onSave } = getSaveDialogProps()
await onSave('new-name', false)
expect(mockSetWorkflowDefaultView).not.toHaveBeenCalled()
expect(mockTrackDefaultViewSet).not.toHaveBeenCalled()
expect(mockCloseDialog).not.toHaveBeenCalled()
})

View File

@@ -10,7 +10,6 @@ import { useAppModeStore } from '@/stores/appModeStore'
import { useDialogStore } from '@/stores/dialogStore'
import { ref } from 'vue'
import { setWorkflowDefaultView } from './builderViewOptions'
import BuilderSaveDialogContent from './BuilderSaveDialogContent.vue'
const SAVE_DIALOG_KEY = 'builder-save'
@@ -71,13 +70,14 @@ export function useBuilderSave() {
if (!workflow) return
const saved = await workflowService.saveWorkflowAs(workflow, {
filename
filename,
isApp: openAsApp
})
if (!saved) return
const activeWorkflow = workflowStore.activeWorkflow
if (!activeWorkflow) return
setWorkflowDefaultView(activeWorkflow, openAsApp)
useTelemetry()?.trackDefaultViewSet({
default_view: openAsApp ? 'app' : 'graph'
})
closeDialog(SAVE_DIALOG_KEY)
showSuccessDialog(openAsApp ? 'app' : 'graph')
} catch (e) {

View File

@@ -37,8 +37,6 @@ export function useAppMode() {
)
function setMode(newMode: AppMode) {
if (newMode === mode.value) return
const workflow = workflowStore.activeWorkflow
if (workflow) workflow.activeMode = newMode
}

View File

@@ -71,7 +71,7 @@ vi.mock('@/services/dialogService', () => ({
vi.mock('@/scripts/app', () => ({
app: {
canvas: { ds: { offset: [0, 0], scale: 1 } },
rootGraph: { serialize: vi.fn(() => ({})) },
rootGraph: { serialize: vi.fn(() => ({})), extra: {} },
loadGraphData: vi.fn()
}
}))
@@ -93,7 +93,11 @@ vi.mock('@/renderer/core/thumbnail/useWorkflowThumbnail', () => ({
}))
vi.mock('@/platform/telemetry', () => ({
useTelemetry: () => null
useTelemetry: () => ({
trackDefaultViewSet: vi.fn(),
trackWorkflowSaved: vi.fn(),
trackEnterLinear: vi.fn()
})
}))
vi.mock('@/platform/workflow/persistence/stores/workflowDraftStore', () => ({
@@ -328,48 +332,6 @@ describe('useWorkflowService', () => {
})
})
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
@@ -538,6 +500,20 @@ describe('useWorkflowService', () => {
expect(workflow.initialMode).toBe('graph')
expect(appMode.mode.value).toBe('builder:arrange')
})
it('sets activeMode even when initialMode already matches', () => {
const workflow = createModeTestWorkflow({
initialMode: 'app',
activeMode: null
})
workflowStore.activeWorkflow = workflow
// mode.value is 'app' via initialMode fallback, but activeMode
// must still be set so the UI transitions to app view
appMode.setMode('app')
expect(workflow.activeMode).toBe('app')
})
})
describe('afterLoadNewGraph initializes initialMode', () => {
@@ -686,6 +662,7 @@ describe('useWorkflowService', () => {
service = useWorkflowService()
vi.spyOn(workflowStore, 'saveWorkflow').mockResolvedValue()
vi.spyOn(workflowStore, 'renameWorkflow').mockResolvedValue()
app.rootGraph.extra = {}
})
function createTemporaryWorkflow(
@@ -703,6 +680,34 @@ describe('useWorkflowService', () => {
return workflow as LoadedComfyWorkflow
}
it('should rename then save when workflow is temporary', async () => {
const workflow = createTemporaryWorkflow()
vi.mocked(workflowStore.getWorkflowByPath).mockReturnValue(null)
const result = await service.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 service.saveWorkflowAs(workflow)
expect(result).toBe(false)
expect(workflowStore.saveWorkflow).not.toHaveBeenCalled()
})
it('appends .app.json extension when initialMode is app', async () => {
const workflow = createTemporaryWorkflow()
workflow.initialMode = 'app'
@@ -737,6 +742,211 @@ describe('useWorkflowService', () => {
'workflows/my-workflow.json'
)
})
it('uses isApp option over initialMode when provided (graph -> app)', async () => {
const workflow = createTemporaryWorkflow()
workflow.initialMode = 'graph'
await service.saveWorkflowAs(workflow, {
filename: 'my-workflow',
isApp: true
})
expect(workflowStore.renameWorkflow).toHaveBeenCalledWith(
workflow,
'workflows/my-workflow.app.json'
)
})
it('uses isApp option over initialMode when provided (app -> graph)', async () => {
const workflow = createTemporaryWorkflow()
workflow.initialMode = 'app'
await service.saveWorkflowAs(workflow, {
filename: 'my-workflow',
isApp: false
})
expect(workflowStore.renameWorkflow).toHaveBeenCalledWith(
workflow,
'workflows/my-workflow.json'
)
})
it('creates a copy when saving same name with different mode (not self-overwrite)', async () => {
const source = createModeTestWorkflow({
path: 'workflows/test.json',
initialMode: 'graph'
})
const copy = createModeTestWorkflow({
path: 'workflows/test.app.json'
})
vi.spyOn(workflowStore, 'saveAs').mockReturnValue(copy)
vi.spyOn(workflowStore, 'openWorkflow').mockResolvedValue(copy)
await service.saveWorkflowAs(source, {
filename: 'test',
isApp: true
})
// Different extension means different path, so it's not a self-overwrite
// — a new copy is created instead of modifying the source in place
expect(source.initialMode).toBe('graph')
expect(workflowStore.saveAs).toHaveBeenCalledWith(
source,
'workflows/test.app.json'
)
expect(workflowStore.saveWorkflow).toHaveBeenCalledWith(copy)
})
it('self-overwrites when saving same name with same mode', async () => {
const source = createModeTestWorkflow({
path: 'workflows/test.app.json',
initialMode: 'app'
})
vi.spyOn(workflowStore, 'getWorkflowByPath').mockReturnValue(source)
mockConfirm.mockResolvedValue(true)
await service.saveWorkflowAs(source, {
filename: 'test',
isApp: true
})
// Same path → self-overwrite: saves in place via saveWorkflow, no copy
expect(workflowStore.saveAs).not.toHaveBeenCalled()
expect(workflowStore.saveWorkflow).toHaveBeenCalledWith(source)
})
it('does not modify source workflow mode when saving persisted workflow as different mode', async () => {
const source = createModeTestWorkflow({
path: 'workflows/original.json',
initialMode: 'graph'
})
const copy = createModeTestWorkflow({
path: 'workflows/copy.app.json'
})
vi.spyOn(workflowStore, 'saveAs').mockReturnValue(copy)
vi.spyOn(workflowStore, 'openWorkflow').mockResolvedValue(copy)
await service.saveWorkflowAs(source, {
filename: 'copy',
isApp: true
})
expect(source.initialMode).toBe('graph')
expect(copy.initialMode).toBe('app')
expect(workflowStore.saveAs).toHaveBeenCalledWith(
source,
'workflows/copy.app.json'
)
expect(workflowStore.saveWorkflow).toHaveBeenCalledWith(copy)
})
it('does not modify source workflow mode when saving app as graph', async () => {
const source = createModeTestWorkflow({
path: 'workflows/original.app.json',
initialMode: 'app'
})
const copy = createModeTestWorkflow({
path: 'workflows/copy.json'
})
vi.spyOn(workflowStore, 'saveAs').mockReturnValue(copy)
vi.spyOn(workflowStore, 'openWorkflow').mockResolvedValue(copy)
await service.saveWorkflowAs(source, {
filename: 'copy',
isApp: false
})
expect(source.initialMode).toBe('app')
expect(copy.initialMode).toBe('graph')
expect(workflowStore.saveAs).toHaveBeenCalledWith(
source,
'workflows/copy.json'
)
expect(workflowStore.saveWorkflow).toHaveBeenCalledWith(copy)
})
function captureLinearModeAtSaveTime() {
let value: boolean | undefined
vi.mocked(workflowStore.saveWorkflow).mockImplementation(async () => {
value = app.rootGraph.extra?.linearMode as boolean | undefined
})
return () => value
}
it('sets linearMode in graph data before saving (graph -> app)', async () => {
const workflow = createTemporaryWorkflow()
workflow.initialMode = 'graph'
app.rootGraph.extra = { linearMode: false }
const getLinearMode = captureLinearModeAtSaveTime()
await service.saveWorkflowAs(workflow, {
filename: 'my-workflow',
isApp: true
})
expect(getLinearMode()).toBe(true)
})
it('sets linearMode in graph data before saving (app -> graph)', async () => {
const workflow = createTemporaryWorkflow()
workflow.initialMode = 'app'
app.rootGraph.extra = { linearMode: true }
const getLinearMode = captureLinearModeAtSaveTime()
await service.saveWorkflowAs(workflow, {
filename: 'my-workflow',
isApp: false
})
expect(getLinearMode()).toBe(false)
})
it('sets linearMode before saving persisted workflow copy', async () => {
const source = createModeTestWorkflow({
path: 'workflows/original.json',
initialMode: 'graph'
})
app.rootGraph.extra = { linearMode: false }
const copy = createModeTestWorkflow({
path: 'workflows/original.app.json'
})
vi.spyOn(workflowStore, 'saveAs').mockReturnValue(copy)
vi.spyOn(workflowStore, 'openWorkflow').mockResolvedValue(copy)
const getLinearMode = captureLinearModeAtSaveTime()
await service.saveWorkflowAs(source, {
filename: 'original',
isApp: true
})
expect(getLinearMode()).toBe(true)
})
it('does not change initialMode when isApp is omitted (persisted copy)', async () => {
const source = createModeTestWorkflow({
path: 'workflows/original.app.json',
initialMode: 'app'
})
// Real saveAs copies initialMode from source; replicate that here
const copy = createModeTestWorkflow({
path: 'workflows/copy.app.json',
initialMode: 'app'
})
vi.spyOn(workflowStore, 'saveAs').mockReturnValue(copy)
vi.spyOn(workflowStore, 'openWorkflow').mockResolvedValue(copy)
await service.saveWorkflowAs(source, { filename: 'copy' })
// saveWorkflowAs should not change initialMode when isApp is omitted
expect(copy.initialMode).toBe('app')
})
})
describe('saveWorkflow', () => {

View File

@@ -116,12 +116,12 @@ export const useWorkflowService = () => {
*/
const saveWorkflowAs = async (
workflow: ComfyWorkflow,
options: { filename?: string } = {}
options: { filename?: string; isApp?: boolean } = {}
): Promise<boolean> => {
const newFilename = options.filename ?? (await workflow.promptSave())
if (!newFilename) return false
const isApp = workflow.initialMode === 'app'
const isApp = options.isApp ?? workflow.initialMode === 'app'
const newPath =
workflow.directory + '/' + appendWorkflowJsonExt(newFilename, isApp)
const existingWorkflow = workflowStore.getWorkflowByPath(newPath)
@@ -138,17 +138,26 @@ export const useWorkflowService = () => {
}
}
workflow.changeTracker?.checkState()
if (isSelfOverwrite) {
workflow.changeTracker?.checkState()
await saveWorkflow(workflow)
} else if (workflow.isTemporary) {
await renameWorkflow(workflow, newPath)
await workflowStore.saveWorkflow(workflow)
} else {
const tempWorkflow = workflowStore.saveAs(workflow, newPath)
await openWorkflow(tempWorkflow)
await workflowStore.saveWorkflow(tempWorkflow)
let target: ComfyWorkflow
if (workflow.isTemporary) {
await renameWorkflow(workflow, newPath)
target = workflow
} else {
target = workflowStore.saveAs(workflow, newPath)
await openWorkflow(target)
}
if (options.isApp !== undefined) {
app.rootGraph.extra.linearMode = isApp
target.initialMode = isApp ? 'app' : 'graph'
}
target.changeTracker?.checkState()
await workflowStore.saveWorkflow(target)
}
useTelemetry()?.trackWorkflowSaved({ is_app: isApp, is_new: true })

View File

@@ -282,6 +282,7 @@ const zExtra = z
workflowRendererVersion: zRendererType.optional(),
BlueprintDescription: z.string().optional(),
BlueprintSearchAliases: z.array(z.string()).optional(),
linearMode: z.boolean().optional(),
linearData: z
.object({
inputs: z.array(z.tuple([zNodeId, z.string()])).optional(),