mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-03-10 15:40:24 +00:00
refactor: parallelize bootstrap and simplify lifecycle with VueUse (#8307)
## Summary Refactors bootstrap and lifecycle management to parallelize initialization, use Vue best practices, and fix a logout state bug. ## Changes ### Bootstrap Store (`bootstrapStore.ts`) - Extract early bootstrap logic into a dedicated store using `useAsyncState` - Parallelize settings, i18n, and workflow sync loading (previously sequential) - Handle multi-user login scenarios by deferring settings/workflows until authenticated ### GraphCanvas Refactoring - Move non-DOM composables (`useGlobalLitegraph`, `useCopy`, `usePaste`, etc.) to script setup level for earlier initialization - Move `watch` and `whenever` declarations outside `onMounted` (Vue best practice) - Use `until()` from VueUse to await bootstrap store readiness instead of direct async calls ### GraphView Simplification - Replace manual `addEventListener`/`removeEventListener` with `useEventListener` - Replace `setInterval` with `useIntervalFn` for automatic cleanup - Move core command registration to script setup level ### Bug Fix Using `router.push()` for logout caused `isSettingsReady` to persist as `true`, making new users inherit the previous user's cached settings. Reverted to `window.location.reload()` with TODOs for future store reset implementation. ## Testing - Verified login/logout cycle clears settings correctly - Verified bootstrap sequence completes without errors --------- Co-authored-by: Amp <amp@ampcode.com>
This commit is contained in:
@@ -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 {
|
||||
@@ -33,7 +34,7 @@ describe('useSettingStore', () => {
|
||||
let store: ReturnType<typeof useSettingStore>
|
||||
|
||||
beforeEach(() => {
|
||||
setActivePinia(createPinia())
|
||||
setActivePinia(createTestingPinia({ stubActions: false }))
|
||||
store = useSettingStore()
|
||||
vi.clearAllMocks()
|
||||
})
|
||||
@@ -43,20 +44,20 @@ 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 Partial<Settings> as Settings
|
||||
)
|
||||
|
||||
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',
|
||||
@@ -65,9 +66,14 @@ describe('useSettingStore', () => {
|
||||
}
|
||||
store.addSetting(setting)
|
||||
|
||||
await expect(store.loadSettingValues()).rejects.toThrow(
|
||||
'Setting values must be loaded before any setting is registered.'
|
||||
)
|
||||
await store.load()
|
||||
|
||||
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.'
|
||||
)
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
@@ -85,18 +91,24 @@ describe('useSettingStore', () => {
|
||||
expect(store.settingsById['test.setting']).toEqual(setting)
|
||||
})
|
||||
|
||||
it('should throw error for duplicate setting ID', () => {
|
||||
it('should warn and skip for duplicate setting ID', () => {
|
||||
const setting: SettingParams = {
|
||||
id: 'test.setting',
|
||||
name: 'test.setting',
|
||||
type: 'text',
|
||||
defaultValue: 'default'
|
||||
}
|
||||
const consoleWarnSpy = vi
|
||||
.spyOn(console, 'warn')
|
||||
.mockImplementation(() => {})
|
||||
|
||||
store.addSetting(setting)
|
||||
expect(() => store.addSetting(setting)).toThrow(
|
||||
'Setting test.setting must have a unique ID.'
|
||||
store.addSetting(setting)
|
||||
|
||||
expect(consoleWarnSpy).toHaveBeenCalledWith(
|
||||
'Setting already registered: test.setting'
|
||||
)
|
||||
consoleWarnSpy.mockRestore()
|
||||
})
|
||||
|
||||
it('should migrate deprecated values', () => {
|
||||
|
||||
@@ -1,4 +1,6 @@
|
||||
import { retry } from 'es-toolkit'
|
||||
import _ from 'es-toolkit/compat'
|
||||
import { until, useAsyncState } from '@vueuse/core'
|
||||
import { defineStore } from 'pinia'
|
||||
import { compare, valid } from 'semver'
|
||||
import { ref } from 'vue'
|
||||
@@ -47,6 +49,39 @@ 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 retry(() => api.getSettings(), {
|
||||
retries: 3,
|
||||
delay: (attempt) => Math.min(1000 * Math.pow(2, attempt), 8000)
|
||||
})
|
||||
await migrateZoomThresholdToFontSize()
|
||||
},
|
||||
undefined,
|
||||
{ immediate: false }
|
||||
)
|
||||
|
||||
async function load(): Promise<void> {
|
||||
if (isReady.value) return
|
||||
|
||||
if (isLoading.value) {
|
||||
await until(isLoading).toBe(false)
|
||||
return
|
||||
}
|
||||
|
||||
await 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.
|
||||
@@ -170,7 +205,11 @@ export const useSettingStore = defineStore('setting', () => {
|
||||
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.`)
|
||||
// Setting already registered - skip to allow component remounting
|
||||
// TODO: Add store reset methods to bootstrapStore and settingStore, then
|
||||
// replace window.location.reload() with router.push() in SidebarLogoutIcon.vue
|
||||
console.warn(`Setting already registered: ${setting.id}`)
|
||||
return
|
||||
}
|
||||
|
||||
settingsById.value[setting.id] = setting
|
||||
@@ -184,22 +223,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.
|
||||
@@ -242,8 +265,11 @@ export const useSettingStore = defineStore('setting', () => {
|
||||
return {
|
||||
settingValues,
|
||||
settingsById,
|
||||
isReady,
|
||||
isLoading,
|
||||
error,
|
||||
load,
|
||||
addSetting,
|
||||
loadSettingValues,
|
||||
set,
|
||||
get,
|
||||
exists,
|
||||
|
||||
Reference in New Issue
Block a user