From dbaecdebbadcdd882c774dc96d2d6fc635faf896 Mon Sep 17 00:00:00 2001 From: Terry Jia Date: Thu, 3 Jul 2025 20:41:59 -0400 Subject: [PATCH] add newUserService and unit tests --- src/components/graph/GraphCanvas.vue | 11 +- src/services/newUserService.ts | 77 ++++ .../tests/services/newUserService.test.ts | 388 ++++++++++++++++++ 3 files changed, 467 insertions(+), 9 deletions(-) create mode 100644 src/services/newUserService.ts create mode 100644 tests-ui/tests/services/newUserService.test.ts diff --git a/src/components/graph/GraphCanvas.vue b/src/components/graph/GraphCanvas.vue index 1a85b4efb..3c0d20f99 100644 --- a/src/components/graph/GraphCanvas.vue +++ b/src/components/graph/GraphCanvas.vue @@ -78,6 +78,7 @@ import { app as comfyApp } from '@/scripts/app' import { ChangeTracker } from '@/scripts/changeTracker' import { IS_CONTROL_WIDGET, updateControlWidgetLabel } from '@/scripts/widgets' import { useColorPaletteService } from '@/services/colorPaletteService' +import { newUserService } from '@/services/newUserService' import { useWorkflowService } from '@/services/workflowService' import { useCommandStore } from '@/stores/commandStore' import { useExecutionStore } from '@/stores/executionStore' @@ -303,15 +304,7 @@ onMounted(async () => { settingStore.addSetting(setting) }) - // to determine if the user is "new user" or not, we check if the settingValues is empty. only it is empty, we set Comfy.InstalledVersion. - // more info: user settings are loaded in the BE folder ComfyUI/user/default/comfy.settings.json and new instance of ComfyUI will have no settings. - // if this settings file is not empty, it means the user has already used ComfyUI before, and we should not set the Comfy.InstalledVersion, according to https://github.com/Comfy-Org/ComfyUI_frontend/issues/4073 - if (Object.keys(settingStore.settingValues).length == 0) { - await settingStore.set( - 'Comfy.InstalledVersion', - __COMFYUI_FRONTEND_VERSION__ - ) - } + await newUserService().initializeIfNewUser(settingStore) // @ts-expect-error fixme ts strict error await comfyApp.setup(canvasRef.value) diff --git a/src/services/newUserService.ts b/src/services/newUserService.ts new file mode 100644 index 000000000..97f5f8314 --- /dev/null +++ b/src/services/newUserService.ts @@ -0,0 +1,77 @@ +import type { useSettingStore } from '@/stores/settingStore' + +export const newUserService = () => { + let isNewUserDetermined = false + let isNewUserCached: boolean | null = null + let pendingCallbacks: Array<() => Promise> = [] + + function checkIsNewUser( + settingStore: ReturnType + ): boolean { + const isNewUserSettings = + Object.keys(settingStore.settingValues).length === 0 || + !settingStore.get('Comfy.TutorialCompleted') + const hasNoWorkflow = !localStorage.getItem('workflow') + const hasNoPreviousWorkflow = !localStorage.getItem( + 'Comfy.PreviousWorkflow' + ) + + return isNewUserSettings && hasNoWorkflow && hasNoPreviousWorkflow + } + + async function registerInitCallback(callback: () => Promise) { + if (isNewUserDetermined) { + if (isNewUserCached) { + try { + await callback() + } catch (error) { + console.error('New user initialization callback failed:', error) + } + } + } else { + pendingCallbacks.push(callback) + } + } + + async function initializeIfNewUser( + settingStore: ReturnType + ) { + if (isNewUserDetermined) return + + isNewUserCached = checkIsNewUser(settingStore) + + console.log(`New user status determined: ${isNewUserCached}`) + + isNewUserDetermined = true + + if (!isNewUserCached) { + pendingCallbacks = [] + return + } + + await settingStore.set( + 'Comfy.InstalledVersion', + __COMFYUI_FRONTEND_VERSION__ + ) + + for (const callback of pendingCallbacks) { + try { + await callback() + } catch (error) { + console.error('New user initialization callback failed:', error) + } + } + + pendingCallbacks = [] + } + + function isNewUser(): boolean | null { + return isNewUserDetermined ? isNewUserCached : null + } + + return { + registerInitCallback, + initializeIfNewUser, + isNewUser + } +} diff --git a/tests-ui/tests/services/newUserService.test.ts b/tests-ui/tests/services/newUserService.test.ts new file mode 100644 index 000000000..dd22a77f7 --- /dev/null +++ b/tests-ui/tests/services/newUserService.test.ts @@ -0,0 +1,388 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import { newUserService } from '@/services/newUserService' + +const mockLocalStorage = vi.hoisted(() => ({ + getItem: vi.fn(), + setItem: vi.fn(), + removeItem: vi.fn(), + clear: vi.fn() +})) + +Object.defineProperty(window, 'localStorage', { + value: mockLocalStorage, + writable: true +}) + +vi.mock('@/config/version', () => ({ + __COMFYUI_FRONTEND_VERSION__: '1.24.0' +})) + +//@ts-expect-error Define global for the test +global.__COMFYUI_FRONTEND_VERSION__ = '1.24.0' + +describe('newUserService', () => { + let service: ReturnType + let mockSettingStore: any + + beforeEach(() => { + vi.clearAllMocks() + service = newUserService() + + mockSettingStore = { + settingValues: {}, + get: vi.fn(), + set: vi.fn() + } + + mockLocalStorage.getItem.mockReturnValue(null) + }) + + describe('checkIsNewUser logic', () => { + it('should identify new user when all conditions are met', async () => { + mockSettingStore.settingValues = {} + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return undefined + return undefined + }) + mockLocalStorage.getItem.mockReturnValue(null) + + await service.initializeIfNewUser(mockSettingStore) + + expect(service.isNewUser()).toBe(true) + }) + + it('should identify new user when settings exist but TutorialCompleted is undefined', async () => { + mockSettingStore.settingValues = { 'some.setting': 'value' } + + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return undefined + return undefined + }) + + mockLocalStorage.getItem.mockReturnValue(null) + + await service.initializeIfNewUser(mockSettingStore) + + expect(service.isNewUser()).toBe(true) + }) + + it('should identify existing user when tutorial is completed', async () => { + mockSettingStore.settingValues = { 'Comfy.TutorialCompleted': true } + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return true + return undefined + }) + mockLocalStorage.getItem.mockReturnValue(null) + + await service.initializeIfNewUser(mockSettingStore) + + expect(service.isNewUser()).toBe(false) + }) + + it('should identify existing user when workflow exists', async () => { + mockSettingStore.settingValues = {} + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return undefined + return undefined + }) + mockLocalStorage.getItem.mockImplementation((key: string) => { + if (key === 'workflow') return 'some-workflow' + return null + }) + + await service.initializeIfNewUser(mockSettingStore) + + expect(service.isNewUser()).toBe(false) + }) + + it('should identify existing user when previous workflow exists', async () => { + mockSettingStore.settingValues = {} + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return undefined + return undefined + }) + mockLocalStorage.getItem.mockImplementation((key: string) => { + if (key === 'Comfy.PreviousWorkflow') return 'some-previous-workflow' + return null + }) + + await service.initializeIfNewUser(mockSettingStore) + + expect(service.isNewUser()).toBe(false) + }) + + it('should identify new user when tutorial is explicitly false', async () => { + mockSettingStore.settingValues = { 'Comfy.TutorialCompleted': false } + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return false + return undefined + }) + mockLocalStorage.getItem.mockReturnValue(null) + + await service.initializeIfNewUser(mockSettingStore) + + expect(service.isNewUser()).toBe(true) + }) + + it('should identify existing user when has both settings and tutorial completed', async () => { + mockSettingStore.settingValues = { + 'some.setting': 'value', + 'Comfy.TutorialCompleted': true + } + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return true + return undefined + }) + mockLocalStorage.getItem.mockReturnValue(null) + + await service.initializeIfNewUser(mockSettingStore) + + expect(service.isNewUser()).toBe(false) + }) + + it('should identify existing user when only one condition fails', async () => { + mockSettingStore.settingValues = {} + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return undefined + return undefined + }) + mockLocalStorage.getItem.mockImplementation((key: string) => { + if (key === 'workflow') return 'some-workflow' + if (key === 'Comfy.PreviousWorkflow') return null + return null + }) + + await service.initializeIfNewUser(mockSettingStore) + + expect(service.isNewUser()).toBe(false) + }) + }) + + describe('registerInitCallback', () => { + it('should execute callback immediately if new user is already determined', async () => { + const mockCallback = vi.fn().mockResolvedValue(undefined) + + mockSettingStore.settingValues = {} + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return undefined + return undefined + }) + mockLocalStorage.getItem.mockReturnValue(null) + + await service.initializeIfNewUser(mockSettingStore) + expect(service.isNewUser()).toBe(true) + + await service.registerInitCallback(mockCallback) + + expect(mockCallback).toHaveBeenCalledTimes(1) + }) + + it('should queue callbacks when user status is not determined', async () => { + const mockCallback = vi.fn().mockResolvedValue(undefined) + + await service.registerInitCallback(mockCallback) + + expect(mockCallback).not.toHaveBeenCalled() + expect(service.isNewUser()).toBeNull() + }) + + it('should handle callback errors gracefully', async () => { + const mockCallback = vi + .fn() + .mockRejectedValue(new Error('Callback error')) + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + + mockSettingStore.settingValues = {} + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return undefined + return undefined + }) + mockLocalStorage.getItem.mockReturnValue(null) + + await service.initializeIfNewUser(mockSettingStore) + + await service.registerInitCallback(mockCallback) + + expect(consoleSpy).toHaveBeenCalledWith( + 'New user initialization callback failed:', + expect.any(Error) + ) + consoleSpy.mockRestore() + }) + }) + + describe('initializeIfNewUser', () => { + it('should set installed version for new users', async () => { + mockSettingStore.settingValues = {} + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return undefined + return undefined + }) + mockLocalStorage.getItem.mockReturnValue(null) + + await service.initializeIfNewUser(mockSettingStore) + + expect(mockSettingStore.set).toHaveBeenCalledWith( + 'Comfy.InstalledVersion', + '1.24.0' + ) + }) + + it('should not set installed version for existing users', async () => { + mockSettingStore.settingValues = { 'some.setting': 'value' } + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return true + return undefined + }) + mockLocalStorage.getItem.mockReturnValue(null) + + await service.initializeIfNewUser(mockSettingStore) + + expect(mockSettingStore.set).not.toHaveBeenCalled() + }) + + it('should execute pending callbacks for new users', async () => { + const mockCallback1 = vi.fn().mockResolvedValue(undefined) + const mockCallback2 = vi.fn().mockResolvedValue(undefined) + + await service.registerInitCallback(mockCallback1) + await service.registerInitCallback(mockCallback2) + + mockSettingStore.settingValues = {} + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return undefined + return undefined + }) + mockLocalStorage.getItem.mockReturnValue(null) + + await service.initializeIfNewUser(mockSettingStore) + + expect(mockCallback1).toHaveBeenCalledTimes(1) + expect(mockCallback2).toHaveBeenCalledTimes(1) + }) + + it('should not execute pending callbacks for existing users', async () => { + const mockCallback = vi.fn().mockResolvedValue(undefined) + + await service.registerInitCallback(mockCallback) + + mockSettingStore.settingValues = { 'some.setting': 'value' } + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return true + return undefined + }) + mockLocalStorage.getItem.mockReturnValue(null) + + await service.initializeIfNewUser(mockSettingStore) + + expect(mockCallback).not.toHaveBeenCalled() + }) + + it('should handle callback errors during initialization', async () => { + const mockCallback = vi.fn().mockRejectedValue(new Error('Init error')) + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + + await service.registerInitCallback(mockCallback) + + mockSettingStore.settingValues = {} + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return undefined + return undefined + }) + mockLocalStorage.getItem.mockReturnValue(null) + + await service.initializeIfNewUser(mockSettingStore) + + expect(consoleSpy).toHaveBeenCalledWith( + 'New user initialization callback failed:', + expect.any(Error) + ) + consoleSpy.mockRestore() + }) + + it('should not reinitialize if already determined', async () => { + mockSettingStore.settingValues = {} + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return undefined + return undefined + }) + mockLocalStorage.getItem.mockReturnValue(null) + + await service.initializeIfNewUser(mockSettingStore) + expect(mockSettingStore.set).toHaveBeenCalledTimes(1) + + await service.initializeIfNewUser(mockSettingStore) + expect(mockSettingStore.set).toHaveBeenCalledTimes(1) + }) + + it('should log new user status', async () => { + const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) + + mockSettingStore.settingValues = {} + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return undefined + return undefined + }) + mockLocalStorage.getItem.mockReturnValue(null) + + await service.initializeIfNewUser(mockSettingStore) + + expect(consoleSpy).toHaveBeenCalledWith( + 'New user status determined: true' + ) + consoleSpy.mockRestore() + }) + }) + + describe('isNewUser', () => { + it('should return null before determination', () => { + expect(service.isNewUser()).toBeNull() + }) + + it('should return cached result after determination', async () => { + mockSettingStore.settingValues = {} + mockSettingStore.get.mockReturnValue(undefined) + mockLocalStorage.getItem.mockReturnValue(null) + + await service.initializeIfNewUser(mockSettingStore) + + expect(service.isNewUser()).toBe(true) + }) + }) + + describe('edge cases', () => { + it('should handle settingStore.get returning false as not completed', async () => { + mockSettingStore.settingValues = { 'Comfy.TutorialCompleted': false } + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return false + return undefined + }) + mockLocalStorage.getItem.mockReturnValue(null) + + await service.initializeIfNewUser(mockSettingStore) + + expect(service.isNewUser()).toBe(true) + }) + + it('should handle multiple callback registrations after initialization', async () => { + const mockCallback1 = vi.fn().mockResolvedValue(undefined) + const mockCallback2 = vi.fn().mockResolvedValue(undefined) + + mockSettingStore.settingValues = {} + mockSettingStore.get.mockImplementation((key: string) => { + if (key === 'Comfy.TutorialCompleted') return undefined + return undefined + }) + mockLocalStorage.getItem.mockReturnValue(null) + + await service.initializeIfNewUser(mockSettingStore) + + await service.registerInitCallback(mockCallback1) + await service.registerInitCallback(mockCallback2) + + expect(mockCallback1).toHaveBeenCalledTimes(1) + expect(mockCallback2).toHaveBeenCalledTimes(1) + }) + }) +})