Compare commits

...

4 Commits

Author SHA1 Message Date
dante01yoon
7e4c880db1 fix: address unsaved workflow review feedback 2026-05-05 20:37:57 +09:00
Dante
122234ad49 Merge branch 'main' into fix/fe-419-unsaved-changes-modal-buttons 2026-05-05 18:53:32 +09:00
dante01yoon
3ca00bc4f6 test(e2e): cover dirtyClose modal on workflow tab close (FE-419)
Add three Playwright tests that drive the Close-anyway / Save / dismiss
matrix on the unsaved-changes modal — the dirtyClose dialog had no E2E
coverage before this change.
2026-04-27 15:10:21 +09:00
dante01yoon
7522e6e53f fix: clarify unsaved-changes modal buttons and fix sign-out 3-state
The dirtyClose dialog had three buttons (Cancel | No | Save) and the sign-out
flow collapsed the deny and dismiss outcomes, so clicking "No" cancelled
sign-out instead of signing out without saving, and clicking "Save" never
actually saved. Drop Cancel for dirtyClose, give each caller a context-specific
deny label ("Sign out anyway", "Close anyway"), and have logout save modified
workflows before signing out.

- Hide Cancel for type='dirtyClose'; deny button accepts a denyLabel override;
  Save autofocuses (preserves work on Enter)
- useAuthActions.logout: handle null/false/true distinctly; iterate
  modifiedWorkflows and saveWorkflow before authStore.logout
- workflowService.closeWorkflow: pass denyLabel='Close anyway'
2026-04-27 14:59:16 +09:00
10 changed files with 435 additions and 57 deletions

View File

@@ -1,4 +1,5 @@
import { expect } from '@playwright/test'
import type { Locator, Page } from '@playwright/test'
import { comfyPageFixture as test } from '@e2e/fixtures/ComfyPage'
@@ -146,4 +147,79 @@ test.describe('Workflow tabs', () => {
await topbar.closeWorkflowTab('Unsaved Workflow (2)')
await expect.poll(() => topbar.getTabNames()).toHaveLength(2)
})
test.describe('Closing a modified workflow tab (FE-419)', () => {
async function modifyActiveWorkflow(page: Page, activeTab: Locator) {
await page.evaluate(() => {
const graph = window.app?.graph
const node = window.LiteGraph?.createNode('Note')
if (graph && node) graph.add(node)
})
await expect(
activeTab.getByTestId('workflow-dirty-indicator')
).toHaveCount(1)
}
test('shows "Close anyway" label and no Cancel button on dirtyClose dialog', async ({
comfyPage
}) => {
const topbar = comfyPage.menu.topbar
await topbar.newWorkflowButton.click()
await expect.poll(() => topbar.getTabNames()).toHaveLength(2)
await modifyActiveWorkflow(comfyPage.page, topbar.getActiveTab())
await topbar.closeWorkflowTab('Unsaved Workflow (2)')
const dialog = comfyPage.page.getByRole('dialog')
await expect(dialog).toBeVisible()
await expect(
dialog.getByRole('button', { name: 'Close anyway' })
).toBeVisible()
await expect(dialog.getByRole('button', { name: 'Save' })).toBeVisible()
await expect(dialog.getByRole('button', { name: 'Cancel' })).toHaveCount(
0
)
})
test('clicking "Close anyway" closes the tab without saving', async ({
comfyPage
}) => {
const topbar = comfyPage.menu.topbar
await topbar.newWorkflowButton.click()
await expect.poll(() => topbar.getTabNames()).toHaveLength(2)
await modifyActiveWorkflow(comfyPage.page, topbar.getActiveTab())
await topbar.closeWorkflowTab('Unsaved Workflow (2)')
await comfyPage.page
.getByRole('dialog')
.getByRole('button', { name: 'Close anyway' })
.click()
await expect.poll(() => topbar.getTabNames()).toHaveLength(1)
await expect
.poll(() => topbar.getActiveTabName())
.toContain('Unsaved Workflow')
})
test('dismissing the dialog keeps the modified tab open', async ({
comfyPage
}) => {
const topbar = comfyPage.menu.topbar
await topbar.newWorkflowButton.click()
await expect.poll(() => topbar.getTabNames()).toHaveLength(2)
await modifyActiveWorkflow(comfyPage.page, topbar.getActiveTab())
await topbar.closeWorkflowTab('Unsaved Workflow (2)')
await expect(comfyPage.page.getByRole('dialog')).toBeVisible()
await comfyPage.page.keyboard.press('Escape')
await expect(comfyPage.page.getByRole('dialog')).toBeHidden()
await expect.poll(() => topbar.getTabNames()).toHaveLength(2)
})
})
})

View File

@@ -1,4 +1,5 @@
import { render, screen } from '@testing-library/vue'
import userEvent from '@testing-library/user-event'
import { createPinia, setActivePinia } from 'pinia'
import PrimeVue from 'primevue/config'
import { beforeEach, describe, expect, it, vi } from 'vitest'
@@ -42,4 +43,43 @@ describe('ConfirmationDialogContent', () => {
renderComponent({ message: longFilename })
expect(screen.getByText(longFilename)).toBeInTheDocument()
})
it('omits the Cancel button when type is dirtyClose', () => {
renderComponent({ type: 'dirtyClose' })
expect(screen.queryByText('g.cancel')).not.toBeInTheDocument()
expect(screen.getByText('g.save')).toBeInTheDocument()
})
it('uses the provided denyLabel for the deny button on dirtyClose', () => {
renderComponent({ type: 'dirtyClose', denyLabel: 'Sign out anyway' })
expect(screen.getByText('Sign out anyway')).toBeInTheDocument()
expect(screen.queryByText('g.no')).not.toBeInTheDocument()
})
it('calls onConfirm(false) when deny is clicked on dirtyClose', async () => {
const onConfirm = vi.fn()
renderComponent({
type: 'dirtyClose',
denyLabel: 'Close anyway',
onConfirm
})
await userEvent.click(screen.getByRole('button', { name: 'Close anyway' }))
expect(onConfirm).toHaveBeenCalledWith(false)
})
it('calls onConfirm(true) when save is clicked on dirtyClose', async () => {
const onConfirm = vi.fn()
renderComponent({ type: 'dirtyClose', onConfirm })
await userEvent.click(screen.getByRole('button', { name: 'g.save' }))
expect(onConfirm).toHaveBeenCalledWith(true)
})
it('falls back to "no" label when denyLabel is not provided', () => {
renderComponent({ type: 'dirtyClose' })
expect(screen.getByText('g.no')).toBeInTheDocument()
})
})

View File

@@ -55,7 +55,7 @@
</div>
<Button
v-if="type !== 'info'"
v-if="type !== 'info' && type !== 'dirtyClose'"
variant="secondary"
autofocus
@click="onCancel"
@@ -86,9 +86,9 @@
<template v-else-if="type === 'dirtyClose'">
<Button variant="secondary" @click="onDeny">
<i class="pi pi-times" />
{{ $t('g.no') }}
{{ denyLabel ?? $t('g.no') }}
</Button>
<Button @click="onConfirm">
<Button autofocus @click="onConfirm">
<i class="pi pi-save" />
{{ $t('g.save') }}
</Button>
@@ -131,6 +131,7 @@ const props = defineProps<{
onConfirm: (value?: boolean) => void
itemList?: string[]
hint?: string
denyLabel?: string
}>()
const { t } = useI18n()

View File

@@ -23,6 +23,7 @@
<div class="relative">
<span
v-if="shouldShowStatusIndicator"
data-testid="workflow-dirty-indicator"
class="absolute top-1/2 left-1/2 z-10 w-4 -translate-1/2 bg-(--comfy-menu-bg) text-2xl font-bold group-hover:hidden"
></span
>

View File

@@ -0,0 +1,195 @@
import { createPinia, setActivePinia } from 'pinia'
import { beforeEach, describe, expect, it, vi } from 'vitest'
import { useAuthActions } from '@/composables/auth/useAuthActions'
import type { ComfyWorkflow } from '@/platform/workflow/management/stores/workflowStore'
type ModifiedWorkflow = Pick<ComfyWorkflow, 'path' | 'isModified'>
const mockAuthStore = vi.hoisted(() => ({
logout: vi.fn().mockResolvedValue(undefined)
}))
const mockToastStore = vi.hoisted(() => ({
add: vi.fn()
}))
const mockWorkflowStore = vi.hoisted(() => ({
modifiedWorkflows: [] as ModifiedWorkflow[]
}))
const mockWorkflowService = vi.hoisted(() => ({
saveWorkflow: vi.fn().mockResolvedValue(true)
}))
const mockDialogService = vi.hoisted(() => ({
confirm: vi.fn()
}))
vi.mock('@/i18n', () => ({
t: (key: string, values?: { workflow?: string }) =>
values?.workflow ? `${key}:${values.workflow}` : key
}))
vi.mock('@/platform/distribution/types', () => ({
isCloud: false
}))
vi.mock('@/platform/telemetry', () => ({
useTelemetry: vi.fn(() => undefined)
}))
vi.mock('@/platform/updates/common/toastStore', () => ({
useToastStore: vi.fn(() => mockToastStore)
}))
vi.mock('@/platform/workflow/management/stores/workflowStore', () => ({
useWorkflowStore: vi.fn(() => mockWorkflowStore)
}))
vi.mock('@/platform/workflow/core/services/workflowService', () => ({
useWorkflowService: vi.fn(() => mockWorkflowService)
}))
vi.mock('@/services/dialogService', () => ({
useDialogService: vi.fn(() => mockDialogService)
}))
vi.mock('@/stores/authStore', () => ({
useAuthStore: vi.fn(() => mockAuthStore)
}))
vi.mock('@/composables/billing/useBillingContext', () => ({
useBillingContext: vi.fn(() => ({
isActiveSubscription: { value: false },
isFreeTier: { value: true },
type: { value: 'free' }
}))
}))
vi.mock('@/composables/useErrorHandling', () => ({
useErrorHandling: () => ({
wrapWithErrorHandlingAsync: <TArgs extends unknown[], TReturn>(
action: (...args: TArgs) => Promise<TReturn> | TReturn
) => action,
toastErrorHandler: vi.fn()
})
}))
function makeWorkflow(path: string): ModifiedWorkflow {
return { path, isModified: true } satisfies ModifiedWorkflow
}
describe('useAuthActions.logout', () => {
beforeEach(() => {
setActivePinia(createPinia())
vi.clearAllMocks()
mockWorkflowStore.modifiedWorkflows = []
})
it('logs out without prompting when no workflows are modified', async () => {
const { logout } = useAuthActions()
await logout()
expect(mockDialogService.confirm).not.toHaveBeenCalled()
expect(mockWorkflowService.saveWorkflow).not.toHaveBeenCalled()
expect(mockAuthStore.logout).toHaveBeenCalledTimes(1)
})
it('cancels sign-out when the dialog is dismissed (null)', async () => {
mockWorkflowStore.modifiedWorkflows = [makeWorkflow('a.json')]
mockDialogService.confirm.mockResolvedValueOnce(null)
const { logout } = useAuthActions()
await logout()
expect(mockDialogService.confirm).toHaveBeenCalledTimes(1)
expect(mockWorkflowService.saveWorkflow).not.toHaveBeenCalled()
expect(mockAuthStore.logout).not.toHaveBeenCalled()
})
it('signs out without saving when the user picks "Sign out anyway" (false)', async () => {
mockWorkflowStore.modifiedWorkflows = [makeWorkflow('a.json')]
mockDialogService.confirm.mockResolvedValueOnce(false)
const { logout } = useAuthActions()
await logout()
expect(mockDialogService.confirm).toHaveBeenCalledTimes(1)
expect(mockWorkflowService.saveWorkflow).not.toHaveBeenCalled()
expect(mockAuthStore.logout).toHaveBeenCalledTimes(1)
})
it('cancels sign-out when saving a workflow is cancelled', async () => {
mockWorkflowStore.modifiedWorkflows = [makeWorkflow('a.json')]
mockDialogService.confirm.mockResolvedValueOnce(true)
mockWorkflowService.saveWorkflow.mockResolvedValueOnce(false)
const { logout } = useAuthActions()
await logout()
expect(mockWorkflowService.saveWorkflow).toHaveBeenCalledTimes(1)
expect(mockAuthStore.logout).not.toHaveBeenCalled()
})
it('does not log out if a workflow save fails', async () => {
mockWorkflowStore.modifiedWorkflows = [
makeWorkflow('a.json'),
makeWorkflow('b.json')
]
mockDialogService.confirm.mockResolvedValueOnce(true)
mockWorkflowService.saveWorkflow.mockRejectedValueOnce(
new Error('disk full')
)
const { logout } = useAuthActions()
await expect(logout()).rejects.toThrow('auth.signOut.saveFailed:a.json')
expect(mockWorkflowService.saveWorkflow).toHaveBeenCalledTimes(1)
expect(mockAuthStore.logout).not.toHaveBeenCalled()
})
it('saves every modified workflow before signing out when user picks Save (true)', async () => {
const workflows = [makeWorkflow('a.json'), makeWorkflow('b.json')]
mockWorkflowStore.modifiedWorkflows = workflows
mockDialogService.confirm.mockResolvedValueOnce(true)
const { logout } = useAuthActions()
await logout()
expect(mockWorkflowService.saveWorkflow).toHaveBeenCalledTimes(2)
expect(mockWorkflowService.saveWorkflow).toHaveBeenNthCalledWith(
1,
workflows[0]
)
expect(mockWorkflowService.saveWorkflow).toHaveBeenNthCalledWith(
2,
workflows[1]
)
expect(mockAuthStore.logout).toHaveBeenCalledTimes(1)
expect(
mockWorkflowService.saveWorkflow.mock.invocationCallOrder[1]
).toBeLessThan(mockAuthStore.logout.mock.invocationCallOrder[0])
expect(
mockWorkflowService.saveWorkflow.mock.invocationCallOrder[0]
).toBeLessThan(mockWorkflowService.saveWorkflow.mock.invocationCallOrder[1])
})
it('passes denyLabel "Sign out anyway" to the dialog', async () => {
mockWorkflowStore.modifiedWorkflows = [makeWorkflow('a.json')]
mockDialogService.confirm.mockResolvedValueOnce(null)
const { logout } = useAuthActions()
await logout()
expect(mockDialogService.confirm).toHaveBeenCalledWith(
expect.objectContaining({
type: 'dirtyClose',
title: 'auth.signOut.unsavedChangesTitle',
message: 'auth.signOut.unsavedChangesMessage',
denyLabel: 'auth.signOut.signOutAnyway'
})
)
})
})

View File

@@ -9,6 +9,7 @@ import { t } from '@/i18n'
import { isCloud } from '@/platform/distribution/types'
import { useTelemetry } from '@/platform/telemetry'
import { useToastStore } from '@/platform/updates/common/toastStore'
import { useWorkflowService } from '@/platform/workflow/core/services/workflowService'
import { useWorkflowStore } from '@/platform/workflow/management/stores/workflowStore'
import { useDialogService } from '@/services/dialogService'
import { useAuthStore } from '@/stores/authStore'
@@ -53,14 +54,30 @@ export const useAuthActions = () => {
const logout = wrapWithErrorHandlingAsync(async () => {
const workflowStore = useWorkflowStore()
if (workflowStore.modifiedWorkflows.length > 0) {
const modifiedWorkflows = workflowStore.modifiedWorkflows
if (modifiedWorkflows.length > 0) {
const dialogService = useDialogService()
const confirmed = await dialogService.confirm({
title: t('auth.signOut.unsavedChangesTitle'),
message: t('auth.signOut.unsavedChangesMessage'),
type: 'dirtyClose'
type: 'dirtyClose',
denyLabel: t('auth.signOut.signOutAnyway')
})
if (!confirmed) return
if (confirmed === null) return
if (confirmed === true) {
const workflowService = useWorkflowService()
for (const workflow of modifiedWorkflows) {
try {
const saved = await workflowService.saveWorkflow(workflow)
if (!saved) return
} catch {
throw new Error(
t('auth.signOut.saveFailed', { workflow: workflow.path })
)
}
}
}
}
await authStore.logout()

View File

@@ -979,6 +979,7 @@
"dirtyCloseTitle": "Save Changes?",
"dirtyClose": "The files below have been changed. Would you like to save them before closing?",
"dirtyCloseHint": "Hold Shift to close without prompt",
"dirtyCloseAnyway": "Close anyway",
"confirmOverwriteTitle": "Overwrite existing file?",
"confirmOverwrite": "The file below already exists. Would you like to overwrite it?",
"workflowTreeType": {
@@ -2209,7 +2210,9 @@
"success": "Signed out successfully",
"successDetail": "You have been signed out of your account.",
"unsavedChangesTitle": "Unsaved Changes",
"unsavedChangesMessage": "You have unsaved changes that will be lost when you sign out. Do you want to continue?"
"unsavedChangesMessage": "You have unsaved changes that will be lost when you sign out. Do you want to continue?",
"signOutAnyway": "Sign out anyway",
"saveFailed": "Sign-out cancelled because saving \"{workflow}\" failed."
},
"passwordUpdate": {
"success": "Password Updated",

View File

@@ -418,24 +418,51 @@ describe('useWorkflowService', () => {
})
vi.mocked(workflowStore.saveWorkflow).mockResolvedValue()
await useWorkflowService().saveWorkflow(workflow)
const result = await useWorkflowService().saveWorkflow(workflow)
expect(result).toBe(true)
expect(workflowStore.saveWorkflow).toHaveBeenCalledWith(workflow)
})
it('should call saveWorkflowAs for temporary workflows', async () => {
it('should return false when temporary workflow save is cancelled', 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)
const result = await useWorkflowService().saveWorkflow(workflow)
expect(result).toBe(false)
expect(workflowStore.saveWorkflow).not.toHaveBeenCalled()
})
})
describe('closeWorkflow', () => {
let workflowStore: ReturnType<typeof useWorkflowStore>
let service: ReturnType<typeof useWorkflowService>
beforeEach(() => {
workflowStore = useWorkflowStore()
service = useWorkflowService()
})
it('keeps a temporary workflow open when Save As is cancelled', async () => {
const workflow = createModeTestWorkflow({
path: 'workflows/Unsaved Workflow.json'
})
workflow.isModified = true
Object.defineProperty(workflow, 'isTemporary', { get: () => true })
vi.spyOn(workflow, 'promptSave').mockResolvedValue(null)
mockConfirm.mockResolvedValue(true)
const closed = await service.closeWorkflow(workflow)
expect(closed).toBe(false)
expect(workflowStore.closeWorkflow).not.toHaveBeenCalled()
})
})
describe('afterLoadNewGraph', () => {
let workflowStore: ReturnType<typeof useWorkflowStore>
let existingWorkflow: LoadedComfyWorkflow

View File

@@ -174,40 +174,39 @@ export const useWorkflowService = () => {
* Save a workflow
* @param workflow The workflow to save
*/
const saveWorkflow = async (workflow: ComfyWorkflow) => {
const saveWorkflow = async (workflow: ComfyWorkflow): Promise<boolean> => {
if (workflow.isTemporary) {
await saveWorkflowAs(workflow)
} else {
workflow.changeTracker?.prepareForSave()
const isApp = workflow.initialMode === 'app'
const expectedPath =
workflow.directory +
'/' +
appendWorkflowJsonExt(workflow.filename, isApp)
if (workflow.path !== expectedPath) {
const existing = workflowStore.getWorkflowByPath(expectedPath)
if (existing && !existing.isTemporary) {
if ((await confirmOverwrite(expectedPath)) !== true) {
await workflowStore.saveWorkflow(workflow)
return
}
await deleteWorkflow(existing, true)
}
await renameWorkflow(workflow, expectedPath)
toastStore.add({
severity: 'info',
summary: t(
isApp
? 'workflowService.savedAsApp'
: 'workflowService.savedAsWorkflow'
),
life: 3000
})
}
await workflowStore.saveWorkflow(workflow)
useTelemetry()?.trackWorkflowSaved({ is_app: isApp, is_new: false })
return await saveWorkflowAs(workflow)
}
workflow.changeTracker?.prepareForSave()
const isApp = workflow.initialMode === 'app'
const expectedPath =
workflow.directory + '/' + appendWorkflowJsonExt(workflow.filename, isApp)
if (workflow.path !== expectedPath) {
const existing = workflowStore.getWorkflowByPath(expectedPath)
if (existing && !existing.isTemporary) {
if ((await confirmOverwrite(expectedPath)) !== true) {
await workflowStore.saveWorkflow(workflow)
return true
}
await deleteWorkflow(existing, true)
}
await renameWorkflow(workflow, expectedPath)
toastStore.add({
severity: 'info',
summary: t(
isApp
? 'workflowService.savedAsApp'
: 'workflowService.savedAsWorkflow'
),
life: 3000
})
}
await workflowStore.saveWorkflow(workflow)
useTelemetry()?.trackWorkflowSaved({ is_app: isApp, is_new: false })
return true
}
/**
@@ -284,13 +283,15 @@ export const useWorkflowService = () => {
type: 'dirtyClose',
message: t('sideToolbar.workflowTab.dirtyClose'),
itemList: [workflow.path],
hint: options.hint
hint: options.hint,
denyLabel: t('sideToolbar.workflowTab.dirtyCloseAnyway')
})
// Cancel
if (confirmed === null) return false
if (confirmed === true) {
await saveWorkflow(workflow)
const saved = await saveWorkflow(workflow)
if (!saved) return false
}
}

View File

@@ -42,6 +42,31 @@ export type ConfirmationDialogType =
| 'reinstall'
| 'info'
interface BaseConfirmOptions {
/** Dialog heading */
title: string
/** The main message body */
message: string
/** Displayed as an unordered list immediately below the message body */
itemList?: string[]
hint?: string
}
type ConfirmOptions = BaseConfirmOptions &
(
| {
/** Pre-configured dialog type */
type: 'dirtyClose'
/** Override the deny button label. Defaults to `g.no`. */
denyLabel?: string
}
| {
/** Pre-configured dialog type */
type?: Exclude<ConfirmationDialogType, 'dirtyClose'>
denyLabel?: never
}
)
/**
* Minimal interface for execution error dialogs.
* Satisfied by both ExecutionErrorWsMessage (WebSocket) and ExecutionError (Jobs API).
@@ -244,18 +269,9 @@ export const useDialogService = () => {
message,
type = 'default',
itemList = [],
hint
}: {
/** Dialog heading */
title: string
/** The main message body */
message: string
/** Pre-configured dialog type */
type?: ConfirmationDialogType
/** Displayed as an unordered list immediately below the message body */
itemList?: string[]
hint?: string
}): Promise<boolean | null> {
hint,
denyLabel
}: ConfirmOptions): Promise<boolean | null> {
return new Promise((resolve) => {
const options: ShowDialogOptions = {
key: 'global-prompt',
@@ -266,7 +282,8 @@ export const useDialogService = () => {
type,
itemList,
onConfirm: resolve,
hint
hint,
denyLabel
},
dialogComponentProps: {
onClose: () => resolve(null)