Move setting impl from ComfySettingsDialog to settingStore (#2085)

browser_tests/extensionAPI.spec.ts

@@ -1,4 +1,4 @@
-import { expect, Locator } from '@playwright/test'
+import { expect } from '@playwright/test'
 import { comfyPageFixture as test } from './fixtures/ComfyPage'
 
 test.describe('Topbar commands', () => {

src/components/graph/GraphCanvas.vue

@@ -68,6 +68,7 @@ import { useWorkflowService } from '@/services/workflowService'
 import { useColorPaletteStore } from '@/stores/workspace/colorPaletteStore'
 import { useColorPaletteService } from '@/services/colorPaletteService'
 import { IS_CONTROL_WIDGET, updateControlWidgetLabel } from '@/scripts/widgets'
+import { CORE_SETTINGS } from '@/constants/coreSettings'
 
 const emit = defineEmits(['ready'])
 const canvasRef = ref<HTMLCanvasElement | null>(null)
@@ -337,6 +338,10 @@ onMounted(async () => {
   // ChangeTracker needs to be initialized before setup, as it will overwrite
   // some listeners of litegraph canvas.
   ChangeTracker.init(comfyApp)
+  await settingStore.loadSettingValues()
+  CORE_SETTINGS.forEach((setting) => {
+    settingStore.addSetting(setting)
+  })
   await comfyApp.setup(canvasRef.value)
   canvasStore.canvas = comfyApp.canvas
   canvasStore.canvas.render_canvas_border = false

src/scripts/app.ts

@@ -1001,15 +1001,9 @@ export class ComfyApp {
    */
   async setup(canvasEl: HTMLCanvasElement) {
     this.canvasEl = canvasEl
-    // Show menu container for GraphView.
-    this.ui.menuContainer.style.display = 'block'
-
     this.resizeCanvas()
 
-    await Promise.all([
-      useWorkspaceStore().workflow.syncWorkflows(),
-      this.ui.settings.load()
-    ])
+    await useWorkspaceStore().workflow.syncWorkflows()
     await useExtensionService().loadExtensions()
 
     this.#addProcessMouseHandler()

src/scripts/ui/settings.ts

@@ -1,49 +1,19 @@
-import { api } from '@/scripts/api'
 import type { ComfyApp } from '@/scripts/app'
 import { ComfyDialog } from './dialog'
-import type { Setting, SettingParams } from '@/types/settingTypes'
+import type { SettingParams } from '@/types/settingTypes'
 import type { Settings } from '@/types/apiTypes'
 import { useSettingStore } from '@/stores/settingStore'
 import { useToastStore } from '@/stores/toastStore'
 
 export class ComfySettingsDialog extends ComfyDialog<HTMLDialogElement> {
   app: ComfyApp
-  settingsValues: any
-  settingsLookup: Record<string, Setting>
-  settingsParamLookup: Record<string, SettingParams>
 
   constructor(app: ComfyApp) {
     super()
     this.app = app
-    this.settingsValues = {}
-    this.settingsLookup = {}
-    this.settingsParamLookup = {}
   }
 
-  get settings() {
-    return Object.values(this.settingsLookup)
-  }
-
-  private tryMigrateDeprecatedValue(id: string, value: any) {
-    if (this.app.vueAppReady) {
-      const settingStore = useSettingStore()
-      const setting = settingStore.settingsById[id]
-      if (setting?.migrateDeprecatedValue) {
-        return setting.migrateDeprecatedValue(value)
-      }
-    }
-    return value
-  }
-
-  #dispatchChange<T>(id: string, value: T, oldValue?: T) {
-    // Keep the settingStore updated. Not using `store.set` as it would trigger
-    // setSettingValue again.
-    // `load` re-dispatch the change for any settings added before load so
-    // settingStore is always up to date.
-    if (this.app.vueAppReady) {
-      useSettingStore().settingValues[id] = value
-    }
-
+  dispatchChange<T>(id: string, value: T, oldValue?: T) {
     this.dispatchEvent(
       new CustomEvent(id + '.change', {
         detail: {
@@ -54,68 +24,74 @@ export class ComfySettingsDialog extends ComfyDialog<HTMLDialogElement> {
     )
   }
 
-  async load() {
-    this.settingsValues = await api.getSettings()
-
-    // Trigger onChange for any settings added before load
-    for (const id in this.settingsLookup) {
-      const compatId = id
-      this.settingsValues[compatId] = this.tryMigrateDeprecatedValue(
-        id,
-        this.settingsValues[compatId]
-      )
-      const value = this.settingsValues[compatId]
-      this.settingsLookup[id].onChange?.(value)
-      this.#dispatchChange(id, value)
-    }
+  /**
+   * @deprecated Use `settingStore.settingValues` instead.
+   */
+  get settingsValues() {
+    return useSettingStore().settingValues
   }
 
+  /**
+   * @deprecated Use `settingStore.settingsById` instead.
+   */
+  get settingsLookup() {
+    return useSettingStore().settingsById
+  }
+
+  /**
+   * @deprecated Use `settingStore.settingsById` instead.
+   */
+  get settingsParamLookup() {
+    return useSettingStore().settingsById
+  }
+
+  /**
+   * @deprecated Use `settingStore.get` instead.
+   */
   getSettingValue<K extends keyof Settings>(
     id: K,
     defaultValue?: Settings[K]
   ): Settings[K] {
-    let value = this.settingsValues[id]
-    return (value ?? defaultValue) as Settings[K]
+    if (defaultValue !== undefined) {
+      console.warn(
+        `Parameter defaultValue is deprecated. The default value in settings definition will be used instead.`
+      )
+    }
+    return useSettingStore().get(id)
   }
 
-  getSettingDefaultValue(id: string) {
-    const param = this.settingsParamLookup[id]
-    return typeof param?.defaultValue === 'function'
-      ? param.defaultValue()
-      : param?.defaultValue
+  /**
+   * @deprecated Use `settingStore.getDefaultValue` instead.
+   */
+  getSettingDefaultValue<K extends keyof Settings>(id: K): Settings[K] {
+    return useSettingStore().getDefaultValue(id)
   }
 
+  /**
+   * @deprecated Use `settingStore.set` instead.
+   */
   async setSettingValueAsync<K extends keyof Settings>(
     id: K,
     value: Settings[K]
   ) {
-    value = this.tryMigrateDeprecatedValue(id, value)
-
-    let oldValue = this.getSettingValue(id, undefined)
-    this.settingsValues[id] = value
-
-    if (id in this.settingsLookup) {
-      this.settingsLookup[id].onChange?.(value, oldValue)
-    }
-    this.#dispatchChange(id, value, oldValue)
-
-    await api.storeSetting(id, value)
-  }
-
-  setSettingValue<K extends keyof Settings>(id: K, value: Settings[K]) {
-    this.setSettingValueAsync(id, value).catch((err) => {
-      useToastStore().addAlert(`Error saving setting '${id}': ${err}`)
-    })
-  }
-
-  refreshSetting(id: keyof Settings) {
-    const value = this.getSettingValue(id)
-    this.settingsLookup[id].onChange?.(value)
-    this.#dispatchChange(id, value)
+    await useSettingStore().set(id, value)
   }
 
   /**
-   * Deprecated for external callers/extensions. Use `ComfyExtension.settings` field instead.
+   * @deprecated Use `settingStore.set` instead.
+   */
+  setSettingValue<K extends keyof Settings>(id: K, value: Settings[K]) {
+    useSettingStore()
+      .set(id, value)
+      .catch((err) => {
+        useToastStore().addAlert(`Error saving setting '${id}': ${err}`)
+      })
+  }
+
+  /**
+   * @deprecated Deprecated for external callers/extensions. Use
+   * `ComfyExtension.settings` field instead.
+   *
    * Example:
    * ```ts
    * app.registerExtension({
@@ -132,41 +108,15 @@ export class ComfySettingsDialog extends ComfyDialog<HTMLDialogElement> {
    * ```
    */
   addSetting(params: SettingParams) {
-    const { id, name, type, defaultValue, onChange } = params
-    if (!id) {
-      throw new Error('Settings must have an ID')
-    }
+    const settingStore = useSettingStore()
+    settingStore.addSetting(params)
 
-    if (id in this.settingsLookup) {
-      throw new Error(`Setting ${id} of type ${type} must have a unique ID.`)
-    }
-
-    const value = this.getSettingValue(id) ?? defaultValue
-
-    // Trigger initial setting of value
-    onChange?.(value, undefined)
-    this.#dispatchChange(id, value)
-
-    this.settingsParamLookup[id] = params
-    if (this.app.vueAppReady) {
-      useSettingStore().settingsById[id] = params
-    }
-    this.settingsLookup[id] = {
-      id,
-      onChange,
-      name,
-      render: () => {
-        console.warn('[ComfyUI] Setting render is deprecated', id)
-      }
-    } as Setting
-
-    const self = this
     return {
       get value() {
-        return self.getSettingValue(id, defaultValue)
+        return settingStore.get(params.id)
       },
       set value(v) {
-        self.setSettingValue(id, v)
+        settingStore.set(params.id, v)
       }
     }
   }

src/services/extensionService.ts

@@ -50,7 +50,9 @@ export const useExtensionService = () => {
     useKeybindingStore().loadExtensionKeybindings(extension)
     useCommandStore().loadExtensionCommands(extension)
     useMenuItemStore().loadExtensionMenuCommands(extension)
-    settingStore.loadExtensionSettings(extension)
+    extension.settings?.forEach((setting) => {
+      settingStore.addSetting(setting)
+    })
     useBottomPanelStore().registerExtensionBottomPanelTabs(extension)
     if (extension.getCustomWidgets) {
       // TODO(huchenlei): We should deprecate the async return value of

src/stores/settingStore.ts

@@ -1,22 +1,11 @@
-/**
- * TODO: Migrate scripts/ui/settings.ts here
- *
- * Currently the reactive settings act as a proxy of the legacy settings.
- * Every time a setting is changed, the settingStore dispatch the change to the
- * legacy settings. Every time the legacy settings are changed, the legacy
- * settings directly updates the settingStore.settingValues.
- */
-
 import { ref, computed } from 'vue'
 import { defineStore } from 'pinia'
-import { app } from '@/scripts/app'
-import { ComfySettingsDialog } from '@/scripts/ui/settings'
 import type { Settings } from '@/types/apiTypes'
 import type { SettingParams } from '@/types/settingTypes'
 import type { TreeNode } from 'primevue/treenode'
-import type { ComfyExtension } from '@/types/comfy'
 import { buildTree } from '@/utils/treeUtil'
-import { CORE_SETTINGS } from '@/constants/coreSettings'
+import { api } from '@/scripts/api'
+import { app } from '@/scripts/app'
 
 export const getSettingInfo = (setting: SettingParams) => {
   const parts = setting.category || setting.id.split('.')
@@ -31,6 +20,19 @@ export interface SettingTreeNode extends TreeNode {
   data?: SettingParams
 }
 
+function tryMigrateDeprecatedValue(setting: SettingParams, value: any) {
+  return setting?.migrateDeprecatedValue?.(value) ?? value
+}
+
+function onChange(setting: SettingParams, value: any, oldValue: any) {
+  if (setting?.onChange && value !== oldValue) {
+    setting.onChange(value)
+    // Backward compatibility with old settings dialog.
+    // Some extensions still listens event emitted by the old settings dialog.
+    app.ui.settings.dispatchChange(setting.id, value, oldValue)
+  }
+}
+
 export const useSettingStore = defineStore('setting', () => {
   const settingValues = ref<Record<string, any>>({})
   const settingsById = ref<Record<string, SettingParams>>({})
@@ -57,47 +59,95 @@ export const useSettingStore = defineStore('setting', () => {
     return root
   })
 
-  function addSettings(settingsDialog: ComfySettingsDialog) {
-    for (const id in settingsDialog.settingsLookup) {
-      const value = settingsDialog.getSettingValue(id)
-      settingValues.value[id] = value
-    }
-    settingsById.value = settingsDialog.settingsParamLookup
-
-    CORE_SETTINGS.forEach((setting: SettingParams) => {
-      settingsDialog.addSetting(setting)
-    })
-  }
-
-  function loadExtensionSettings(extension: ComfyExtension) {
-    extension.settings?.forEach((setting: SettingParams) => {
-      app.ui.settings.addSetting(setting)
-    })
-  }
-
+  /**
+   * Check if a setting's value exists, i.e. if the user has set it manually.
+   * @param key - The key of the setting to check.
+   * @returns Whether the setting exists.
+   */
   function exists(key: string) {
     return settingValues.value[key] !== undefined
   }
 
+  /**
+   * Set a setting value.
+   * @param key - The key of the setting to set.
+   * @param value - The value to set.
+   */
   async function set<K extends keyof Settings>(key: K, value: Settings[K]) {
-    settingValues.value[key] = value
-    await app.ui.settings.setSettingValueAsync(key, value)
+    const newValue = tryMigrateDeprecatedValue(settingsById.value[key], value)
+    const oldValue = settingValues.value[key]
+    onChange(settingsById.value[key], newValue, oldValue)
+
+    settingValues.value[key] = newValue
+    await api.storeSetting(key, newValue)
   }
 
+  /**
+   * Get a setting value.
+   * @param key - The key of the setting to get.
+   * @returns The value of the setting.
+   */
   function get<K extends keyof Settings>(key: K): Settings[K] {
-    return (
-      settingValues.value[key] ?? app.ui.settings.getSettingDefaultValue(key)
-    )
+    return settingValues.value[key] ?? getDefaultValue(key)
+  }
+
+  /**
+   * Get the default value of a setting.
+   * @param key - The key of the setting to get.
+   * @returns The default value of the setting.
+   */
+  function getDefaultValue<K extends keyof Settings>(key: K): Settings[K] {
+    const param = settingsById.value[key]
+    return typeof param?.defaultValue === 'function'
+      ? param.defaultValue()
+      : param?.defaultValue
+  }
+
+  /**
+   * Register a setting.
+   * @param setting - The setting to register.
+   */
+  function addSetting(setting: SettingParams) {
+    if (!setting.id) {
+      throw new Error('Settings must have an ID')
+    }
+    if (setting.id in settingsById.value) {
+      throw new Error(`Setting ${setting.id} must have a unique ID.`)
+    }
+
+    settingsById.value[setting.id] = setting
+
+    if (settingValues.value[setting.id] !== undefined) {
+      settingValues.value[setting.id] = tryMigrateDeprecatedValue(
+        setting,
+        settingValues.value[setting.id]
+      )
+    }
+    onChange(setting, get(setting.id), undefined)
+  }
+
+  /*
+   * Load setting values from server.
+   * This needs to be called before any setting is registered.
+   */
+  async function loadSettingValues() {
+    if (Object.keys(settingsById.value).length) {
+      throw new Error(
+        'Setting values must be loaded before any setting is registered.'
+      )
+    }
+    settingValues.value = await api.getSettings()
   }
 
   return {
     settingValues,
     settingsById,
     settingTree,
-    addSettings,
-    loadExtensionSettings,
+    addSetting,
+    loadSettingValues,
     set,
     get,
-    exists
+    exists,
+    getDefaultValue
   }
 })

src/views/GraphView.vue

@@ -94,7 +94,7 @@ watchEffect(() => {
 watchEffect(() => {
   const useNewMenu = settingStore.get('Comfy.UseNewMenu')
   if (useNewMenu === 'Disabled') {
-    app.ui.menuContainer.style.removeProperty('display')
+    app.ui.menuContainer.style.setProperty('display', 'block')
     app.ui.restoreMenuPosition()
   } else {
     app.ui.menuContainer.style.setProperty('display', 'none')
@@ -108,7 +108,6 @@ watchEffect(() => {
 })
 
 const init = () => {
-  settingStore.addSettings(app.ui.settings)
   const coreCommands = useCoreCommands()
   useCommandStore().registerCommands(coreCommands)
   useMenuItemStore().registerCoreMenuCommands()

tests-ui/tests/fast/store/settingStore.test.ts

@@ -0,0 +1,141 @@
+import { setActivePinia, createPinia } from 'pinia'
+import { useSettingStore } from '@/stores/settingStore'
+import { api } from '@/scripts/api'
+import type { SettingParams } from '@/types/settingTypes'
+
+// Mock the api
+jest.mock('@/scripts/api', () => ({
+  api: {
+    getSettings: jest.fn(),
+    storeSetting: jest.fn()
+  }
+}))
+
+// Mock the app
+jest.mock('@/scripts/app', () => ({
+  app: {
+    ui: {
+      settings: {
+        dispatchChange: jest.fn()
+      }
+    }
+  }
+}))
+
+describe('useSettingStore', () => {
+  let store: ReturnType<typeof useSettingStore>
+
+  beforeEach(() => {
+    setActivePinia(createPinia())
+    store = useSettingStore()
+    jest.clearAllMocks()
+  })
+
+  it('should initialize with empty settings', () => {
+    expect(store.settingValues).toEqual({})
+    expect(store.settingsById).toEqual({})
+    expect(store.settingTree.children).toEqual([])
+  })
+
+  describe('loadSettingValues', () => {
+    it('should load settings from API', async () => {
+      const mockSettings = { 'test.setting': 'value' }
+      ;(api.getSettings as jest.Mock).mockResolvedValue(mockSettings)
+
+      await store.loadSettingValues()
+
+      expect(store.settingValues).toEqual(mockSettings)
+      expect(api.getSettings).toHaveBeenCalled()
+    })
+
+    it('should throw error if settings are loaded after registration', async () => {
+      const setting: SettingParams = {
+        id: 'test.setting',
+        name: 'test.setting',
+        type: 'text',
+        defaultValue: 'default'
+      }
+      store.addSetting(setting)
+
+      await expect(store.loadSettingValues()).rejects.toThrow(
+        'Setting values must be loaded before any setting is registered.'
+      )
+    })
+  })
+
+  describe('addSetting', () => {
+    it('should register a new setting', () => {
+      const setting: SettingParams = {
+        id: 'test.setting',
+        name: 'test.setting',
+        type: 'text',
+        defaultValue: 'default'
+      }
+
+      store.addSetting(setting)
+
+      expect(store.settingsById['test.setting']).toEqual(setting)
+    })
+
+    it('should throw error for duplicate setting ID', () => {
+      const setting: SettingParams = {
+        id: 'test.setting',
+        name: 'test.setting',
+        type: 'text',
+        defaultValue: 'default'
+      }
+
+      store.addSetting(setting)
+      expect(() => store.addSetting(setting)).toThrow(
+        'Setting test.setting must have a unique ID.'
+      )
+    })
+
+    it('should migrate deprecated values', () => {
+      const setting: SettingParams = {
+        id: 'test.setting',
+        name: 'test.setting',
+        type: 'text',
+        defaultValue: 'default',
+        migrateDeprecatedValue: (value: string) => value.toUpperCase()
+      }
+
+      store.settingValues['test.setting'] = 'oldvalue'
+      store.addSetting(setting)
+
+      expect(store.settingValues['test.setting']).toBe('OLDVALUE')
+    })
+  })
+
+  describe('get and set', () => {
+    it('should get default value when setting not exists', () => {
+      const setting: SettingParams = {
+        id: 'test.setting',
+        name: 'test.setting',
+        type: 'text',
+        defaultValue: 'default'
+      }
+      store.addSetting(setting)
+
+      expect(store.get('test.setting')).toBe('default')
+    })
+
+    it('should set value and trigger onChange', async () => {
+      const onChangeMock = jest.fn()
+      const setting: SettingParams = {
+        id: 'test.setting',
+        name: 'test.setting',
+        type: 'text',
+        defaultValue: 'default',
+        onChange: onChangeMock
+      }
+      store.addSetting(setting)
+
+      await store.set('test.setting', 'newvalue')
+
+      expect(store.get('test.setting')).toBe('newvalue')
+      expect(onChangeMock).toHaveBeenCalledWith('newvalue')
+      expect(api.storeSetting).toHaveBeenCalledWith('test.setting', 'newvalue')
+    })
+  })
+})

tests-ui/utils/index.ts

@@ -1,5 +1,5 @@
 // @ts-strict-ignore
-import { APIConfig, mockApi, mockSettingStore, mockNodeDefStore } from './setup'
+import { APIConfig, mockApi, mockNodeDefStore } from './setup'
 import { Ez, EzGraph, EzNameSpace } from './ezgraph'
 import lg from './litegraph'
 import fs from 'fs'
@@ -41,10 +41,7 @@ export async function start(config: StartConfig = {}): Promise<StartResult> {
   document.body.innerHTML = html.toString()
 
   mockApi(config)
-  mockSettingStore()
   const { app } = await import('../../src/scripts/app')
-  const { useSettingStore } = await import('../../src/stores/settingStore')
-  useSettingStore().addSettings(app.ui.settings)
   mockNodeDefStore()
 
   const { LiteGraph, LGraphCanvas } = await import('@comfyorg/litegraph')

tests-ui/utils/setup.ts

@@ -1,6 +1,4 @@
 // @ts-strict-ignore
-import type { ComfySettingsDialog } from '@/scripts/ui/settings'
-import type { ComfyApp } from '@/scripts/app'
 import '../../src/scripts/api'
 import { ComfyNodeDef } from '@/types/apiTypes'
 
@@ -98,31 +96,6 @@ export function mockApi(config: APIConfig = {}) {
   }))
 }
 
-export const mockSettingStore = () => {
-  let app: ComfyApp | null = null
-
-  const mockedSettingStore = {
-    addSettings(settings: ComfySettingsDialog) {
-      app = settings.app
-    },
-
-    set(key: string, value: any) {
-      app?.ui.settings.setSettingValue(key, value)
-    },
-
-    get(key: string) {
-      return (
-        app?.ui.settings.getSettingValue(key) ??
-        app?.ui.settings.getSettingDefaultValue(key)
-      )
-    }
-  }
-
-  jest.mock('@/stores/settingStore', () => ({
-    useSettingStore: jest.fn(() => mockedSettingStore)
-  }))
-}
-
 export const mockNodeDefStore = () => {
   const mockedNodeDefStore = {
     addNodeDef: jest.fn((nodeDef: ComfyNodeDef) => {})