[test] Add test to verify targetSelector property deletion

Add a specific test case to ensure the keybinding migration properly
deletes the deprecated 'targetSelector' property after migrating to
'targetElementId'. This test provides additional coverage to verify
the migration doesn't leave behind obsolete properties.

The test verifies:
- The old 'targetSelector' property is removed
- The new 'targetElementId' property is added with correct value
- Other properties remain unchanged

src/components/graph/GraphCanvas.vue

@@ -297,6 +297,7 @@ onMounted(async () => {
       throw error
     }
   }
+
   CORE_SETTINGS.forEach((setting) => {
     settingStore.addSetting(setting)
   })

src/constants/coreSettings.ts

@@ -430,14 +430,7 @@ export const CORE_SETTINGS: SettingParams[] = [
     defaultValue: 'Top',
     name: 'Use new menu',
     type: 'combo',
-    options: ['Disabled', 'Top', 'Bottom'],
-    migrateDeprecatedValue: (value: string) => {
-      // Floating is now supported by dragging the docked actionbar off.
-      if (value === 'Floating') {
-        return 'Top'
-      }
-      return value
-    }
+    options: ['Disabled', 'Top', 'Bottom']
   },
   {
     id: 'Comfy.Workflow.WorkflowTabsPosition',
@@ -470,15 +463,7 @@ export const CORE_SETTINGS: SettingParams[] = [
     type: 'hidden',
     defaultValue: [] as Keybinding[],
     versionAdded: '1.3.7',
-    versionModified: '1.7.3',
-    migrateDeprecatedValue: (value: any[]) => {
-      return value.map((keybinding) => {
-        if (keybinding['targetSelector'] === '#graph-canvas') {
-          keybinding['targetElementId'] = 'graph-canvas'
-        }
-        return keybinding
-      })
-    }
+    versionModified: '1.7.3'
   },
   {
     id: 'Comfy.Keybinding.NewBindings',
@@ -716,11 +701,7 @@ export const CORE_SETTINGS: SettingParams[] = [
     name: 'The active color palette id',
     type: 'hidden',
     defaultValue: 'dark',
-    versionModified: '1.6.7',
-    migrateDeprecatedValue(value: string) {
-      // Legacy custom palettes were prefixed with 'custom_'
-      return value.startsWith('custom_') ? value.replace('custom_', '') : value
-    }
+    versionModified: '1.6.7'
   },
   {
     id: 'Comfy.CustomColorPalettes',

src/stores/settingStore.ts

@@ -7,6 +7,7 @@ import { api } from '@/scripts/api'
 import { app } from '@/scripts/app'
 import type { SettingParams } from '@/types/settingTypes'
 import type { TreeNode } from '@/types/treeExplorerTypes'
+import { runSettingMigrations } from '@/utils/migration/settingsMigration'
 
 export const getSettingInfo = (setting: SettingParams) => {
   const parts = setting.category || setting.id.split('.')
@@ -20,10 +21,6 @@ export interface SettingTreeNode extends TreeNode {
   data?: SettingParams
 }
 
-function tryMigrateDeprecatedValue(setting: SettingParams, value: any) {
-  return setting?.migrateDeprecatedValue?.(value) ?? value
-}
-
 function onChange(setting: SettingParams, newValue: any, oldValue: any) {
   if (setting?.onChange) {
     setting.onChange(newValue, oldValue)
@@ -54,16 +51,12 @@ export const useSettingStore = defineStore('setting', () => {
   async function set<K extends keyof Settings>(key: K, value: Settings[K]) {
     // Clone the incoming value to prevent external mutations
     const clonedValue = _.cloneDeep(value)
-    const newValue = tryMigrateDeprecatedValue(
-      settingsById.value[key],
-      clonedValue
-    )
     const oldValue = get(key)
-    if (newValue === oldValue) return
+    if (clonedValue === oldValue) return
 
-    onChange(settingsById.value[key], newValue, oldValue)
-    settingValues.value[key] = newValue
-    await api.storeSetting(key, newValue)
+    onChange(settingsById.value[key], clonedValue, oldValue)
+    settingValues.value[key] = clonedValue
+    await api.storeSetting(key, clonedValue)
   }
 
   /**
@@ -102,12 +95,7 @@ export const useSettingStore = defineStore('setting', () => {
 
     settingsById.value[setting.id] = setting
 
-    if (settingValues.value[setting.id] !== undefined) {
-      settingValues.value[setting.id] = tryMigrateDeprecatedValue(
-        setting,
-        settingValues.value[setting.id]
-      )
-    }
+    // Trigger onChange callback with current value
     onChange(setting, get(setting.id), undefined)
   }
 
@@ -122,6 +110,9 @@ export const useSettingStore = defineStore('setting', () => {
       )
     }
     settingValues.value = await api.getSettings()
+
+    // Run migrations after loading values but before settings are registered
+    await runSettingMigrations()
   }
 
   return {

src/types/settingTypes.ts

@@ -43,8 +43,6 @@ export interface SettingParams extends FormItem {
   category?: string[]
   experimental?: boolean
   deprecated?: boolean
-  // Deprecated values are mapped to new values.
-  migrateDeprecatedValue?: (value: any) => any
   // Version of the setting when it was added
   versionAdded?: string
   // Version of the setting when it was last modified

src/utils/migration/settingsMigration.ts

@@ -0,0 +1,88 @@
+import type { Keybinding } from '@/schemas/keyBindingSchema'
+import { useSettingStore } from '@/stores/settingStore'
+
+export interface SettingMigration {
+  condition: () => boolean
+  migrate: () => Promise<void>
+}
+
+/**
+ * Setting value migrations that transform deprecated values to new formats.
+ * These run after settings are loaded from the server but before they are
+ * registered, ensuring settings have valid values when the app initializes.
+ */
+export const SETTING_MIGRATIONS: SettingMigration[] = [
+  // Migrate Comfy.UseNewMenu "Floating" value to "Top"
+  {
+    condition: () => {
+      const settingStore = useSettingStore()
+      return (
+        settingStore.exists('Comfy.UseNewMenu') &&
+        (settingStore.get('Comfy.UseNewMenu') as string) === 'Floating'
+      )
+    },
+    migrate: async () => {
+      const settingStore = useSettingStore()
+      await settingStore.set('Comfy.UseNewMenu', 'Top')
+    }
+  },
+  // Migrate Comfy.Keybinding.UnsetBindings targetSelector to targetElementId
+  {
+    condition: () => {
+      const settingStore = useSettingStore()
+      if (!settingStore.exists('Comfy.Keybinding.UnsetBindings')) return false
+      const keybindings = settingStore.get(
+        'Comfy.Keybinding.UnsetBindings'
+      ) as Keybinding[]
+      return keybindings.some((kb: any) => 'targetSelector' in kb)
+    },
+    migrate: async () => {
+      const settingStore = useSettingStore()
+      const keybindings = settingStore.get(
+        'Comfy.Keybinding.UnsetBindings'
+      ) as Keybinding[]
+      const migrated = keybindings.map((keybinding) => {
+        // Create a new object to avoid mutating the original
+        const newKeybinding = { ...keybinding }
+        if (
+          'targetSelector' in newKeybinding &&
+          newKeybinding['targetSelector'] === '#graph-canvas'
+        ) {
+          newKeybinding['targetElementId'] = 'graph-canvas'
+          delete newKeybinding['targetSelector']
+        }
+        return newKeybinding
+      })
+      await settingStore.set('Comfy.Keybinding.UnsetBindings', migrated)
+    }
+  },
+  // Migrate Comfy.ColorPalette custom_ prefix
+  {
+    condition: () => {
+      const settingStore = useSettingStore()
+      return (
+        settingStore.exists('Comfy.ColorPalette') &&
+        (settingStore.get('Comfy.ColorPalette') as string).startsWith('custom_')
+      )
+    },
+    migrate: async () => {
+      const settingStore = useSettingStore()
+      const value = settingStore.get('Comfy.ColorPalette') as string
+      await settingStore.set('Comfy.ColorPalette', value.replace('custom_', ''))
+    }
+  }
+]
+
+/**
+ * Runs all setting migrations that meet their conditions.
+ * This is called after loadSettingValues() to ensure all deprecated
+ * setting values are migrated to their current format before the
+ * application starts using them.
+ */
+export async function runSettingMigrations(): Promise<void> {
+  for (const migration of SETTING_MIGRATIONS) {
+    if (migration.condition()) {
+      await migration.migrate()
+    }
+  }
+}

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

@@ -92,21 +92,6 @@ describe('useSettingStore', () => {
         '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', () => {

tests-ui/tests/utils/migration/settingsMigration.test.ts

@@ -0,0 +1,271 @@
+import { createPinia, setActivePinia } from 'pinia'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+import { api } from '@/scripts/api'
+import { useSettingStore } from '@/stores/settingStore'
+import {
+  SETTING_MIGRATIONS,
+  runSettingMigrations
+} from '@/utils/migration/settingsMigration'
+
+// Mock the api
+vi.mock('@/scripts/api', () => ({
+  api: {
+    getSettings: vi.fn(),
+    storeSetting: vi.fn()
+  }
+}))
+
+// Mock the app
+vi.mock('@/scripts/app', () => ({
+  app: {
+    ui: {
+      settings: {
+        dispatchChange: vi.fn()
+      }
+    }
+  }
+}))
+
+describe('settingsMigration', () => {
+  let store: ReturnType<typeof useSettingStore>
+
+  beforeEach(() => {
+    setActivePinia(createPinia())
+    store = useSettingStore()
+    vi.clearAllMocks()
+
+    // Mock the store methods to avoid needing registered settings
+    vi.spyOn(store, 'set').mockImplementation(async (key, value) => {
+      store.settingValues[key] = value
+      await api.storeSetting(key, value)
+    })
+
+    vi.spyOn(store, 'get').mockImplementation((key) => {
+      return store.settingValues[key]
+    })
+
+    vi.spyOn(store, 'exists').mockImplementation((key) => {
+      return key in store.settingValues
+    })
+  })
+
+  describe('Comfy.UseNewMenu migration', () => {
+    it('should migrate "Floating" value to "Top"', async () => {
+      // Setup initial state with old value
+      store.settingValues = { 'Comfy.UseNewMenu': 'Floating' }
+
+      // Check condition
+      const migration = SETTING_MIGRATIONS[0]
+      expect(migration.condition()).toBe(true)
+
+      // Run migration
+      await migration.migrate()
+
+      // Verify the value was updated
+      expect(store.settingValues['Comfy.UseNewMenu']).toBe('Top')
+      expect(api.storeSetting).toHaveBeenCalledWith('Comfy.UseNewMenu', 'Top')
+    })
+
+    it('should not migrate when value is not "Floating"', () => {
+      // Setup with different value
+      store.settingValues = { 'Comfy.UseNewMenu': 'Bottom' }
+
+      // Check condition
+      const migration = SETTING_MIGRATIONS[0]
+      expect(migration.condition()).toBe(false)
+    })
+
+    it('should not migrate when setting does not exist', () => {
+      // No settings
+      store.settingValues = {}
+
+      // Check condition
+      const migration = SETTING_MIGRATIONS[0]
+      expect(migration.condition()).toBe(false)
+    })
+  })
+
+  describe('Comfy.Keybinding.UnsetBindings migration', () => {
+    it('should migrate targetSelector to targetElementId', async () => {
+      // Setup with old format
+      store.settingValues = {
+        'Comfy.Keybinding.UnsetBindings': [
+          { targetSelector: '#graph-canvas', key: 'a' },
+          { targetSelector: '#other', key: 'b' }
+        ]
+      }
+
+      // Check condition
+      const migration = SETTING_MIGRATIONS[1]
+      expect(migration.condition()).toBe(true)
+
+      // Run migration
+      await migration.migrate()
+
+      // Verify the migration
+      const result = store.settingValues['Comfy.Keybinding.UnsetBindings']
+      expect(result).toEqual([
+        { targetElementId: 'graph-canvas', key: 'a' },
+        { targetSelector: '#other', key: 'b' } // Only #graph-canvas is migrated
+      ])
+      expect(api.storeSetting).toHaveBeenCalledWith(
+        'Comfy.Keybinding.UnsetBindings',
+        result
+      )
+    })
+
+    it('should delete targetSelector property after migration', async () => {
+      // Setup with old format
+      store.settingValues = {
+        'Comfy.Keybinding.UnsetBindings': [
+          { targetSelector: '#graph-canvas', key: 'a' }
+        ]
+      }
+
+      // Run migration
+      await SETTING_MIGRATIONS[1].migrate()
+
+      // Verify targetSelector is deleted and targetElementId is added
+      const result = store.settingValues['Comfy.Keybinding.UnsetBindings'][0]
+      expect(result).not.toHaveProperty('targetSelector')
+      expect(result).toHaveProperty('targetElementId', 'graph-canvas')
+      expect(result).toHaveProperty('key', 'a')
+    })
+
+    it('should not migrate when all keybindings use new format', () => {
+      // Setup with new format
+      store.settingValues = {
+        'Comfy.Keybinding.UnsetBindings': [
+          { targetElementId: 'graph-canvas', key: 'a' }
+        ]
+      }
+
+      // Check condition
+      const migration = SETTING_MIGRATIONS[1]
+      expect(migration.condition()).toBe(false)
+    })
+
+    it('should not migrate when setting does not exist', () => {
+      // No settings
+      store.settingValues = {}
+
+      // Check condition
+      const migration = SETTING_MIGRATIONS[1]
+      expect(migration.condition()).toBe(false)
+    })
+
+    it('should handle empty keybindings array', () => {
+      // Empty array
+      store.settingValues = { 'Comfy.Keybinding.UnsetBindings': [] }
+
+      // Check condition
+      const migration = SETTING_MIGRATIONS[1]
+      expect(migration.condition()).toBe(false)
+    })
+  })
+
+  describe('Comfy.ColorPalette migration', () => {
+    it('should remove "custom_" prefix', async () => {
+      // Setup with old format
+      store.settingValues = { 'Comfy.ColorPalette': 'custom_mytheme' }
+
+      // Check condition
+      const migration = SETTING_MIGRATIONS[2]
+      expect(migration.condition()).toBe(true)
+
+      // Run migration
+      await migration.migrate()
+
+      // Verify the migration
+      expect(store.settingValues['Comfy.ColorPalette']).toBe('mytheme')
+      expect(api.storeSetting).toHaveBeenCalledWith(
+        'Comfy.ColorPalette',
+        'mytheme'
+      )
+    })
+
+    it('should not migrate when value does not start with "custom_"', () => {
+      // Setup with value that doesn't need migration
+      store.settingValues = { 'Comfy.ColorPalette': 'dark' }
+
+      // Check condition
+      const migration = SETTING_MIGRATIONS[2]
+      expect(migration.condition()).toBe(false)
+    })
+
+    it('should not migrate when setting does not exist', () => {
+      // No settings
+      store.settingValues = {}
+
+      // Check condition
+      const migration = SETTING_MIGRATIONS[2]
+      expect(migration.condition()).toBe(false)
+    })
+  })
+
+  describe('runSettingMigrations', () => {
+    it('should run all applicable migrations', async () => {
+      // Setup state that triggers all migrations
+      store.settingValues = {
+        'Comfy.UseNewMenu': 'Floating',
+        'Comfy.Keybinding.UnsetBindings': [
+          { targetSelector: '#graph-canvas', key: 'a' }
+        ],
+        'Comfy.ColorPalette': 'custom_mytheme'
+      }
+
+      // Run all migrations
+      await runSettingMigrations()
+
+      // Verify all migrations ran
+      expect(store.settingValues['Comfy.UseNewMenu']).toBe('Top')
+      expect(store.settingValues['Comfy.Keybinding.UnsetBindings']).toEqual([
+        { targetElementId: 'graph-canvas', key: 'a' }
+      ])
+      expect(store.settingValues['Comfy.ColorPalette']).toBe('mytheme')
+
+      // Verify all API calls were made
+      expect(api.storeSetting).toHaveBeenCalledTimes(3)
+    })
+
+    it('should only run migrations that meet their conditions', async () => {
+      // Setup state that only triggers one migration
+      store.settingValues = {
+        'Comfy.UseNewMenu': 'Bottom', // Won't migrate
+        'Comfy.ColorPalette': 'custom_mytheme' // Will migrate
+      }
+
+      // Run migrations
+      await runSettingMigrations()
+
+      // Verify only one migration ran
+      expect(store.settingValues['Comfy.UseNewMenu']).toBe('Bottom')
+      expect(store.settingValues['Comfy.ColorPalette']).toBe('mytheme')
+
+      // Only one API call
+      expect(api.storeSetting).toHaveBeenCalledTimes(1)
+      expect(api.storeSetting).toHaveBeenCalledWith(
+        'Comfy.ColorPalette',
+        'mytheme'
+      )
+    })
+
+    it('should handle no migrations needed', async () => {
+      // Setup state that doesn't trigger any migrations
+      store.settingValues = {
+        'Comfy.UseNewMenu': 'Top',
+        'Comfy.Keybinding.UnsetBindings': [
+          { targetElementId: 'graph-canvas', key: 'a' }
+        ],
+        'Comfy.ColorPalette': 'dark'
+      }
+
+      // Run migrations
+      await runSettingMigrations()
+
+      // No API calls should be made
+      expect(api.storeSetting).not.toHaveBeenCalled()
+    })
+  })
+})