refactor: move settings loading state to settingStore

- Move isReady, isLoading, error state from bootstrapStore to settingStore
- Use useAsyncState in settingStore for load() action
- Rename loadSettingValues to load for consistency
- Update GraphCanvas to get settings state from settingStore
- Update tests to use createTestingPinia with stubActions: false

Amp-Thread-ID: https://ampcode.com/threads/T-019bfc95-aaa8-737d-bcd2-a5bdbc8b158f
Co-authored-by: Amp <amp@ampcode.com>

src/components/graph/GraphCanvas.vue

@@ -185,8 +185,9 @@ const colorPaletteStore = useColorPaletteStore()
 const colorPaletteService = useColorPaletteService()
 const canvasInteractions = useCanvasInteractions()
 const bootstrapStore = useBootstrapStore()
-const { isI18nReady, i18nError, isSettingsReady, settingsError } =
+const { isI18nReady, i18nError } = storeToRefs(bootstrapStore)
-  storeToRefs(bootstrapStore)
+const { isReady: isSettingsReady, error: settingsError } =
+  storeToRefs(settingStore)
 
 const betaMenuEnabled = computed(
   () => settingStore.get('Comfy.UseNewMenu') !== 'Disabled'

src/composables/useCoreCommands.test.ts

@@ -167,25 +167,30 @@ describe('useCoreCommands', () => {
     return {
       get: vi.fn().mockReturnValue(getReturnValue),
       addSetting: vi.fn(),
-      loadSettingValues: vi.fn(),
+      load: vi.fn(),
       set: vi.fn(),
       exists: vi.fn(),
       getDefaultValue: vi.fn(),
+      isReady: true,
+      isLoading: false,
+      error: undefined,
       settingValues: {},
       settingsById: {},
       $id: 'setting',
       $state: {
         settingValues: {},
-        settingsById: {}
+        settingsById: {},
+        isReady: true,
+        isLoading: false,
+        error: undefined
       },
       $patch: vi.fn(),
       $reset: vi.fn(),
       $subscribe: vi.fn(),
       $onAction: vi.fn(),
       $dispose: vi.fn(),
-      _customProperties: new Set(),
+      _customProperties: new Set()
-      _p: {}
+    } satisfies ReturnType<typeof useSettingStore>
-    } as ReturnType<typeof useSettingStore>
   }
 
   beforeEach(() => {

src/platform/settings/settingStore.test.ts

@@ -1,4 +1,5 @@
-import { createPinia, setActivePinia } from 'pinia'
+import { createTestingPinia } from '@pinia/testing'
+import { setActivePinia } from 'pinia'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
 import {
@@ -32,7 +33,7 @@ describe('useSettingStore', () => {
   let store: ReturnType<typeof useSettingStore>
 
   beforeEach(() => {
-    setActivePinia(createPinia())
+    setActivePinia(createTestingPinia({ stubActions: false }))
     store = useSettingStore()
     vi.clearAllMocks()
   })
@@ -42,18 +43,18 @@ describe('useSettingStore', () => {
     expect(store.settingsById).toEqual({})
   })
 
-  describe('loadSettingValues', () => {
+  describe('load', () => {
     it('should load settings from API', async () => {
       const mockSettings = { 'test.setting': 'value' }
       vi.mocked(api.getSettings).mockResolvedValue(mockSettings as any)
 
-      await store.loadSettingValues()
+      await store.load()
 
       expect(store.settingValues).toEqual(mockSettings)
       expect(api.getSettings).toHaveBeenCalled()
     })
 
-    it('should throw error if settings are loaded after registration', async () => {
+    it('should set error if settings are loaded after registration', async () => {
       const setting: SettingParams = {
         id: 'test.setting',
         name: 'test.setting',
@@ -62,9 +63,14 @@ describe('useSettingStore', () => {
       }
       store.addSetting(setting)
 
-      await expect(store.loadSettingValues()).rejects.toThrow(
+      await store.load()
-        'Setting values must be loaded before any setting is registered.'
+
-      )
+      expect(store.error).toBeInstanceOf(Error)
+      if (store.error instanceof Error) {
+        expect(store.error.message).toBe(
+          'Setting values must be loaded before any setting is registered.'
+        )
+      }
     })
   })
 

src/platform/settings/settingStore.ts

@@ -1,4 +1,5 @@
 import _ from 'es-toolkit/compat'
+import { useAsyncState } from '@vueuse/core'
 import { defineStore } from 'pinia'
 import { compare, valid } from 'semver'
 import { ref } from 'vue'
@@ -47,6 +48,31 @@ export const useSettingStore = defineStore('setting', () => {
   const settingValues = ref<Record<string, any>>({})
   const settingsById = ref<Record<string, SettingParams>>({})
 
+  const {
+    isReady,
+    isLoading,
+    error,
+    execute: loadSettingValues
+  } = useAsyncState(
+    async () => {
+      if (Object.keys(settingsById.value).length) {
+        throw new Error(
+          'Setting values must be loaded before any setting is registered.'
+        )
+      }
+      settingValues.value = await api.getSettings()
+      await migrateZoomThresholdToFontSize()
+    },
+    undefined,
+    { immediate: false }
+  )
+
+  async function load() {
+    if (!isReady.value && !isLoading.value) {
+      return loadSettingValues()
+    }
+  }
+
   /**
    * 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.
@@ -188,22 +214,6 @@ export const useSettingStore = defineStore('setting', () => {
     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()
-
-    // Migrate old zoom threshold setting to new font size setting
-    await migrateZoomThresholdToFontSize()
-  }
-
   /**
    * Migrate the old zoom threshold setting to the new font size setting.
    * Preserves the exact zoom threshold behavior by converting it to equivalent font size.
@@ -246,8 +256,11 @@ export const useSettingStore = defineStore('setting', () => {
   return {
     settingValues,
     settingsById,
+    isReady,
+    isLoading,
+    error,
+    load,
     addSetting,
-    loadSettingValues,
     set,
     get,
     exists,

src/stores/bootstrapStore.test.ts

@@ -1,5 +1,9 @@
-import { createPinia, setActivePinia } from 'pinia'
+import { createTestingPinia } from '@pinia/testing'
+import { setActivePinia } from 'pinia'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
+import { ref } from 'vue'
+
+import { useSettingStore } from '@/platform/settings/settingStore'
 
 import { useBootstrapStore } from './bootstrapStore'
 
@@ -16,9 +20,16 @@ vi.mock('@/i18n', () => ({
   mergeCustomNodesI18n: vi.fn()
 }))
 
+const mockIsSettingsReady = ref(false)
+
 vi.mock('@/platform/settings/settingStore', () => ({
   useSettingStore: vi.fn(() => ({
-    loadSettingValues: vi.fn().mockResolvedValue(undefined)
+    load: vi.fn(() => {
+      mockIsSettingsReady.value = true
+    }),
+    isReady: mockIsSettingsReady,
+    isLoading: ref(false),
+    error: ref(undefined)
   }))
 }))
 
@@ -34,14 +45,16 @@ describe('bootstrapStore', () => {
   let store: ReturnType<typeof useBootstrapStore>
 
   beforeEach(() => {
-    setActivePinia(createPinia())
+    mockIsSettingsReady.value = false
+    setActivePinia(createTestingPinia({ stubActions: false }))
     store = useBootstrapStore()
     vi.clearAllMocks()
   })
 
   it('initializes with all flags false', () => {
+    const settingStore = useSettingStore()
     expect(store.isNodeDefsReady).toBe(false)
-    expect(store.isSettingsReady).toBe(false)
+    expect(settingStore.isReady).toBe(false)
     expect(store.isI18nReady).toBe(false)
   })
 
@@ -58,10 +71,11 @@ describe('bootstrapStore', () => {
   })
 
   it('starts store bootstrap (settings, i18n)', async () => {
+    const settingStore = useSettingStore()
     void store.startStoreBootstrap()
 
     await vi.waitFor(() => {
-      expect(store.isSettingsReady).toBe(true)
+      expect(settingStore.isReady).toBe(true)
       expect(store.isI18nReady).toBe(true)
     })
   })

src/stores/bootstrapStore.ts

@@ -1,11 +1,14 @@
 import { useAsyncState } from '@vueuse/core'
 import { defineStore } from 'pinia'
 
+import { useSettingStore } from '@/platform/settings/settingStore'
 import type { ComfyNodeDef } from '@/schemas/nodeDefSchema'
 import { api } from '@/scripts/api'
 import { useUserStore } from '@/stores/userStore'
 
 export const useBootstrapStore = defineStore('bootstrap', () => {
+  const settingStore = useSettingStore()
+
   const {
     state: nodeDefs,
     isReady: isNodeDefsReady,
@@ -20,30 +23,6 @@ export const useBootstrapStore = defineStore('bootstrap', () => {
     { immediate: false }
   )
 
-  const {
-    isReady: isSettingsReady,
-    isLoading: isSettingsLoading,
-    error: settingsError,
-    execute: executeLoadSettings
-  } = useAsyncState(
-    async () => {
-      const { useSettingStore } =
-        await import('@/platform/settings/settingStore')
-      await useSettingStore().loadSettingValues()
-    },
-    undefined,
-    { immediate: false }
-  )
-
-  function loadSettings() {
-    // TODO: This check makes the store "sticky" across logouts. Add a reset
-    // method to clear isSettingsReady, then replace window.location.reload()
-    // with router.push() in SidebarLogoutIcon.vue
-    if (!isSettingsReady.value && !isSettingsLoading.value) {
-      void executeLoadSettings()
-    }
-  }
-
   const {
     isReady: isI18nReady,
     error: i18nError,
@@ -91,7 +70,7 @@ export const useBootstrapStore = defineStore('bootstrap', () => {
     void loadI18n()
 
     if (!userStore.needsLogin) {
-      loadSettings()
+      await settingStore.load()
       syncWorkflows()
     }
   }
@@ -100,13 +79,9 @@ export const useBootstrapStore = defineStore('bootstrap', () => {
     nodeDefs,
     isNodeDefsReady,
     nodeDefsError,
-    isSettingsReady,
-    settingsError,
     isI18nReady,
     i18nError,
     startEarlyBootstrap,
-    startStoreBootstrap,
+    startStoreBootstrap
-    loadSettings,
-    syncWorkflows
   }
 })