fix: coderabbit reviews

browser_tests/tests/templates.spec.ts

@@ -106,7 +106,7 @@ test.describe('Templates', { tag: ['@slow', '@workflow'] }, () => {
     await comfyPage.setup({ clearStorage: true })
 
     // Expect the templates dialog to be shown
-    expect(await comfyPage.templates.content.isVisible()).toBe(true)
+    await expect(comfyPage.templates.content).toBeVisible()
   })
 
   test('Uses proper locale files for templates', async ({ comfyPage }) => {

src/locales/en/main.json

@@ -3079,5 +3079,11 @@
       "duplicateName": "A secret with this name already exists",
       "duplicateProvider": "A secret for this provider already exists"
     }
+  },
+  "autoSaveOffPersistWarning": {
+    "title": "Disable Persist?",
+    "body": "Turning off Auto Save will also disable workflow persistence. Your workflow state will no longer be restored on page reload.",
+    "confirm": "Disable Persist",
+    "keepPersist": "Keep Persist"
   }
 }

src/platform/settings/constants/coreSettings.test.ts

@@ -0,0 +1,150 @@
+import { createTestingPinia } from '@pinia/testing'
+import { setActivePinia } from 'pinia'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+import { CORE_SETTINGS } from '@/platform/settings/constants/coreSettings'
+import { useSettingStore } from '@/platform/settings/settingStore'
+import { useDialogStore } from '@/stores/dialogStore'
+
+vi.mock('@/scripts/api', () => ({
+  api: {
+    getSettings: vi.fn(),
+    storeSetting: vi.fn(),
+    storeSettings: vi.fn()
+  }
+}))
+
+vi.mock('@/scripts/app', () => ({
+  app: {
+    ui: {
+      settings: {
+        dispatchChange: vi.fn()
+      }
+    }
+  }
+}))
+
+describe('Comfy.Workflow.Persist defaultsByInstallVersion', () => {
+  let settingStore: ReturnType<typeof useSettingStore>
+
+  const persistSetting = CORE_SETTINGS.find(
+    (s) => s.id === 'Comfy.Workflow.Persist'
+  )!
+
+  beforeEach(() => {
+    setActivePinia(createTestingPinia({ stubActions: false }))
+    settingStore = useSettingStore()
+    vi.clearAllMocks()
+  })
+
+  it('should have defaultValue true', () => {
+    expect(persistSetting.defaultValue).toBe(true)
+  })
+
+  it('should have defaultsByInstallVersion entry for 1.40.7', () => {
+    expect(persistSetting.defaultsByInstallVersion).toEqual({
+      '1.40.7': false
+    })
+  })
+
+  it('should default to true for existing users with no installed version', () => {
+    settingStore.addSetting(persistSetting)
+
+    expect(settingStore.get('Comfy.Workflow.Persist')).toBe(true)
+  })
+
+  it('should default to true for existing users with older installed version', () => {
+    settingStore.settingValues['Comfy.InstalledVersion'] = '1.30.0'
+    settingStore.addSetting(persistSetting)
+
+    expect(settingStore.get('Comfy.Workflow.Persist')).toBe(true)
+  })
+
+  it('should default to false for fresh installs on 1.40.7', () => {
+    settingStore.settingValues['Comfy.InstalledVersion'] = '1.40.7'
+    settingStore.addSetting(persistSetting)
+
+    expect(settingStore.get('Comfy.Workflow.Persist')).toBe(false)
+  })
+
+  it('should default to false for fresh installs on versions after 1.40.7', () => {
+    settingStore.settingValues['Comfy.InstalledVersion'] = '1.50.0'
+    settingStore.addSetting(persistSetting)
+
+    expect(settingStore.get('Comfy.Workflow.Persist')).toBe(false)
+  })
+})
+
+describe('Comfy.Workflow.AutoSave onChange', () => {
+  let settingStore: ReturnType<typeof useSettingStore>
+  let dialogStore: ReturnType<typeof useDialogStore>
+
+  const persistSetting = CORE_SETTINGS.find(
+    (s) => s.id === 'Comfy.Workflow.Persist'
+  )!
+  const autoSaveSetting = CORE_SETTINGS.find(
+    (s) => s.id === 'Comfy.Workflow.AutoSave'
+  )!
+
+  async function triggerAutoSaveOff() {
+    await settingStore.set('Comfy.Workflow.AutoSave', 'after delay')
+    await settingStore.set('Comfy.Workflow.AutoSave', 'off')
+  }
+
+  function getFooterProps() {
+    const dialog = dialogStore.dialogStack[0]
+    return dialog.footerProps as Record<string, (() => void) | undefined>
+  }
+
+  beforeEach(() => {
+    setActivePinia(createTestingPinia({ stubActions: false }))
+    settingStore = useSettingStore()
+    dialogStore = useDialogStore()
+    vi.clearAllMocks()
+
+    settingStore.addSetting(persistSetting)
+    settingStore.addSetting(autoSaveSetting)
+  })
+
+  it('should show confirm dialog when setting AutoSave to off while Persist is enabled', async () => {
+    await triggerAutoSaveOff()
+
+    expect(dialogStore.dialogStack).toHaveLength(1)
+  })
+
+  it('should not show dialog when Persist is already disabled', async () => {
+    await settingStore.set('Comfy.Workflow.Persist', false)
+    await triggerAutoSaveOff()
+
+    expect(dialogStore.dialogStack).toHaveLength(0)
+  })
+
+  it('should disable Persist when user confirms', async () => {
+    await triggerAutoSaveOff()
+
+    getFooterProps().onConfirm!()
+
+    expect(settingStore.get('Comfy.Workflow.Persist')).toBe(false)
+  })
+
+  it('should keep Persist when user clicks secondary action', async () => {
+    await triggerAutoSaveOff()
+
+    getFooterProps().onCancel!()
+
+    expect(settingStore.get('Comfy.Workflow.Persist')).toBe(true)
+  })
+
+  it('should disable Persist when dialog is dismissed', async () => {
+    await triggerAutoSaveOff()
+
+    const dialog = dialogStore.dialogStack[0]
+    dialog.dialogComponentProps.onClose?.()
+
+    expect(settingStore.get('Comfy.Workflow.Persist')).toBe(false)
+  })
+
+  it('should not show dialog on initial registration (old === undefined)', () => {
+    expect(dialogStore.dialogStack).toHaveLength(0)
+  })
+})

src/platform/settings/constants/coreSettings.ts

@@ -7,6 +7,9 @@ import type { Keybinding } from '@/platform/keybindings/types'
 import { NodeBadgeMode } from '@/types/nodeSource'
 import { LinkReleaseTriggerAction } from '@/types/searchBoxTypes'
 import { breakpointsTailwind } from '@vueuse/core'
+import { showConfirmDialog } from '@/components/dialog/confirm/confirmDialog'
+import { useDialogStore } from '@/stores/dialogStore'
+import { t } from '@/i18n'
 
 /**
  * Core settings are essential configuration parameters required for ComfyUI's basic functionality.
@@ -1053,13 +1056,51 @@ export const CORE_SETTINGS: SettingParams[] = [
     type: 'combo',
     options: ['off', 'after delay'], // Room for other options like on focus change, tab change, window change
     defaultValue: 'off', // Popular request by users (https://github.com/Comfy-Org/ComfyUI_frontend/issues/1584#issuecomment-2536610154)
-    versionAdded: '1.16.0'
+    versionAdded: '1.16.0',
+    onChange: (val: unknown, old?: unknown) => {
+      if (val !== 'off' || old === undefined) return
+
+      const settingStore = useSettingStore()
+      if (!settingStore.get('Comfy.Workflow.Persist')) return
+
+      const dialogStore = useDialogStore()
+      let keepPersist = false
+      const dialog = showConfirmDialog({
+        headerProps: {
+          title: t('autoSaveOffPersistWarning.title')
+        },
+        props: {
+          promptText: t('autoSaveOffPersistWarning.body')
+        },
+        footerProps: {
+          confirmText: t('autoSaveOffPersistWarning.confirm'),
+          confirmClass:
+            'bg-primary-background hover:bg-primary-background-hover',
+          cancelText: t('autoSaveOffPersistWarning.keepPersist'),
+          onConfirm: () => {
+            dialogStore.closeDialog(dialog)
+          },
+          onCancel: () => {
+            keepPersist = true
+            dialogStore.closeDialog(dialog)
+          }
+        }
+      })
+      dialog.dialogComponentProps.onClose = () => {
+        if (!keepPersist) {
+          void settingStore.set('Comfy.Workflow.Persist', false)
+        }
+      }
+    }
   },
   {
     id: 'Comfy.Workflow.Persist',
     name: 'Persist workflow state and restore on page (re)load',
     type: 'boolean',
     defaultValue: true,
+    defaultsByInstallVersion: {
+      '1.40.7': false
+    },
     versionAdded: '1.16.1'
   },
   {

src/platform/settings/settingStore.ts

@@ -183,7 +183,7 @@ export const useSettingStore = defineStore('setting', () => {
 
     const versionedDefault = getVersionedDefaultValue(key, param)
 
-    if (versionedDefault) {
+    if (versionedDefault !== null) {
       return versionedDefault
     }
 

src/platform/workflow/persistence/composables/useWorkflowPersistence.ts

@@ -142,9 +142,12 @@ export function useWorkflowPersistence() {
   }
 
   const initializeWorkflow = async () => {
-    if (!workflowPersistenceEnabled.value) return
-
     try {
+      if (!workflowPersistenceEnabled.value) {
+        await loadDefaultWorkflow()
+        return
+      }
+
       const restored = await loadPreviousWorkflowFromStorage()
       if (!restored) {
         await loadDefaultWorkflow()