From cf732974a98b753d703b7594b3db6c8e9201c496 Mon Sep 17 00:00:00 2001 From: Christian Byrne Date: Fri, 14 Mar 2025 18:25:07 -0700 Subject: [PATCH] [Manager] Fix enabled/disabled node pack logic (#3061) --- src/stores/comfyManagerStore.ts | 76 +++-- .../tests/store/comfyManagerStore.test.ts | 305 ++++++++++++++++++ 2 files changed, 357 insertions(+), 24 deletions(-) create mode 100644 tests-ui/tests/store/comfyManagerStore.test.ts diff --git a/src/stores/comfyManagerStore.ts b/src/stores/comfyManagerStore.ts index 1b3edc7453..22665fda84 100644 --- a/src/stores/comfyManagerStore.ts +++ b/src/stores/comfyManagerStore.ts @@ -1,7 +1,6 @@ import { whenever } from '@vueuse/core' -import { partition } from 'lodash' import { defineStore } from 'pinia' -import { ref, watchEffect } from 'vue' +import { ref, watch } from 'vue' import { useCachedRequest } from '@/composables/useCachedRequest' import { useManagerQueue } from '@/composables/useManagerQueue' @@ -31,22 +30,7 @@ export const useComfyManagerStore = defineStore('comfyManager', () => { isStale.value = true } - /** - * A pack is disabled if there is a disabled entry and no corresponding enabled entry - * @example - * installedPacks = { - * "packname@1_0_2": { enabled: false, cnr_id: "packname" }, - * "packname": { enabled: true, cnr_id: "packname" } - * } - * isDisabled("packname") // false - * - * installedPacks = { - * "packname@1_0_2": { enabled: false, cnr_id: "packname" }, - * } - * isDisabled("packname") // true - */ - const isDisabledPack = (pack: ManagerPackInstalled) => - pack.enabled === false && pack.cnr_id && !installedPacks.value[pack.cnr_id] + const getPackId = (pack: ManagerPackInstalled) => pack.cnr_id || pack.aux_id const isInstalledPackId = (packName: string | undefined): boolean => !!packName && installedPacksIds.value.has(packName) @@ -63,13 +47,57 @@ export const useComfyManagerStore = defineStore('comfyManager', () => { return acc }, new Set()) - watchEffect(() => { - const packs = Object.values(installedPacks.value) - const [disabled, enabled] = partition(packs, isDisabledPack) - enabledPacksIds.value = packsToIdSet(enabled) - disabledPacksIds.value = packsToIdSet(disabled) + /** + * A pack is disabled if there is a disabled entry and no corresponding + * enabled entry. If `packname@1.0.2` is disabled, but `packname@1.0.3` is + * enabled, then `packname` is considered enabled. + * + * @example + * installedPacks = { + * "packname@1_0_2": { enabled: false, cnr_id: "packname" }, + * "packname": { enabled: true, cnr_id: "packname" } + * } + * isDisabled("packname") // false + * + * installedPacks = { + * "packname@1_0_2": { enabled: false, cnr_id: "packname" }, + * } + * isDisabled("packname") // true + */ + const updateDisabledIds = (packs: ManagerPackInstalled[]) => { + // Use temporary variables to avoid triggering reactivity + const enabledIds = new Set() + const disabledIds = new Set() + + for (const pack of packs) { + const id = getPackId(pack) + if (!id) continue + + const { enabled } = pack + + if (enabled === true) enabledIds.add(id) + else if (enabled === false) disabledIds.add(id) + + // If pack in both (has a disabled and enabled version), remove from disabled + const inBothSets = enabledIds.has(id) && disabledIds.has(id) + if (inBothSets) disabledIds.delete(id) + } + + enabledPacksIds.value = enabledIds + disabledPacksIds.value = disabledIds + } + + const updateInstalledIds = (packs: ManagerPackInstalled[]) => { installedPacksIds.value = packsToIdSet(packs) - }) + } + + const onPacksChanged = () => { + const packs = Object.values(installedPacks.value) + updateDisabledIds(packs) + updateInstalledIds(packs) + } + + watch(installedPacks, onPacksChanged, { deep: true }) const refreshInstalledList = async () => { const packs = await managerService.listInstalledPacks() diff --git a/tests-ui/tests/store/comfyManagerStore.test.ts b/tests-ui/tests/store/comfyManagerStore.test.ts new file mode 100644 index 0000000000..1d59e1ca43 --- /dev/null +++ b/tests-ui/tests/store/comfyManagerStore.test.ts @@ -0,0 +1,305 @@ +import { createPinia, setActivePinia } from 'pinia' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { nextTick, ref } from 'vue' + +import { useComfyManagerService } from '@/services/comfyManagerService' +import { useComfyManagerStore } from '@/stores/comfyManagerStore' +import { + InstalledPacksResponse, + ManagerPackInstalled +} from '@/types/comfyManagerTypes' + +vi.mock('@/services/comfyManagerService', () => ({ + useComfyManagerService: vi.fn() +})) + +interface EnabledDisabledTestCase { + desc: string + installed: Record + expectState: 'enabled' | 'disabled' + /** @default 'name' */ + packName?: string +} + +describe('useComfyManagerStore', () => { + let mockManagerService: { + isLoading: ReturnType> + error: ReturnType> + listInstalledPacks: ReturnType + } + + const triggerPacksChange = async ( + installedPacks: InstalledPacksResponse, + store: ReturnType + ) => { + // Simulate change in value to properly trigger watchers. Required even for immediate watchers. + store.installedPacks = {} + await nextTick() + store.installedPacks = installedPacks + } + + beforeEach(() => { + setActivePinia(createPinia()) + vi.clearAllMocks() + mockManagerService = { + isLoading: ref(false), + error: ref(null), + listInstalledPacks: vi.fn().mockResolvedValue({}) + } + + // @ts-expect-error Mocking the return type of useComfyManagerService + vi.mocked(useComfyManagerService).mockReturnValue(mockManagerService) + }) + + const testCases: EnabledDisabledTestCase[] = [ + { + desc: 'Two enabled versions', + installed: { + 'name@1_0_2': { + enabled: true, + cnr_id: 'name', + ver: '1.0.2', + aux_id: undefined + }, + name: { enabled: true, cnr_id: 'name', ver: '1.0.0', aux_id: undefined } + }, + expectState: 'enabled' + }, + { + desc: 'Two disabled versions', + installed: { + 'name@1_0_2': { + enabled: false, + cnr_id: 'name', + ver: '1.0.2', + aux_id: undefined + }, + name: { + enabled: false, + cnr_id: 'name', + ver: '1.0.0', + aux_id: undefined + } + }, + expectState: 'disabled' + }, + { + desc: 'Enabled version and pinned disabled version', + installed: { + 'name@1_0_2': { + enabled: false, + cnr_id: 'name', + ver: '1.0.2', + aux_id: undefined + }, + name: { enabled: true, cnr_id: 'name', ver: '1.0.0', aux_id: undefined } + }, + expectState: 'enabled' + }, + { + desc: 'Disabled version and pinned enabled version', + installed: { + 'name@1_0_2': { + enabled: true, + cnr_id: 'name', + ver: '1.0.2', + aux_id: undefined + }, + name: { + enabled: false, + cnr_id: 'name', + ver: '1.0.0', + aux_id: undefined + } + }, + expectState: 'enabled' + }, + { + desc: 'Pinned enabled version, Pinned disabled version', + installed: { + 'name@1_0_2': { + enabled: false, + cnr_id: 'name', + ver: '1.0.2', + aux_id: undefined + }, + 'name@1_0_3': { + enabled: true, + cnr_id: 'name', + ver: '1.0.3', + aux_id: undefined + } + }, + expectState: 'enabled' + }, + { + desc: 'Two enabled non-CNR versions', + packName: 'author/name', + installed: { + 'author/name@1_0_2': { + enabled: true, + aux_id: 'author/name', + cnr_id: undefined, + ver: '1.0.2' + }, + 'author/name': { + enabled: true, + aux_id: 'author/name', + cnr_id: undefined, + ver: '1.0.0' + } + }, + expectState: 'enabled' + }, + { + desc: 'Two disabled non-CNR versions', + packName: 'author/name', + installed: { + 'author/name@1_0_2': { + enabled: false, + aux_id: 'author/name', + cnr_id: undefined, + ver: '1.0.2' + }, + 'author/name': { + enabled: false, + aux_id: 'author/name', + cnr_id: undefined, + ver: '1.0.0' + } + }, + expectState: 'disabled' + }, + { + desc: 'Non-CNR disabled version, CNR enabled version', + packName: 'author/name', + installed: { + 'author/name': { + enabled: false, + aux_id: 'author/name', + cnr_id: undefined, + ver: '1.0.0' + }, + 'author/name@1_0_2': { + enabled: true, + cnr_id: 'author/name', + ver: '1.0.2', + aux_id: undefined + } + }, + expectState: 'enabled' + }, + { + desc: 'Disabled non-CNR version, CNR disabled version', + packName: 'author/name', + installed: { + 'author/name': { + enabled: false, + aux_id: 'author/name', + cnr_id: undefined, // non-CNR pack + ver: '1.0.0' + }, + 'author/name@1_0_2': { + enabled: false, + cnr_id: 'author/name', // CNR pack + ver: '1.0.2', + aux_id: undefined + } + }, + expectState: 'disabled' + }, + { + desc: 'Enabled non-CNR version, two versions', + packName: 'author/name', + installed: { + 'author/name': { + enabled: true, + aux_id: 'author/name', + cnr_id: undefined, + ver: '1.0.0' + }, + 'author/name@1_0_2': { + enabled: true, + cnr_id: 'author/name', + ver: '1.0.2', + aux_id: undefined + } + }, + expectState: 'enabled' + }, + { + desc: 'Enabled CNR version', + packName: 'name', + installed: { + name: { enabled: true, cnr_id: 'name', ver: '1.0.0', aux_id: undefined } + }, + expectState: 'enabled' + }, + { + desc: 'Disabled CNR version', + packName: 'name', + installed: { + name: { + enabled: false, + cnr_id: 'name', + ver: '1.0.0', + aux_id: undefined + } + }, + expectState: 'disabled' + }, + { + desc: 'Enabled non-CNR version', + packName: 'author/name', + installed: { + 'author/name': { + enabled: true, + aux_id: 'author/name', + cnr_id: undefined, + ver: '1.0.0' + } + }, + expectState: 'enabled' + }, + { + desc: 'Disabled non-CNR version', + packName: 'author/name', + installed: { + 'author/name': { + enabled: false, + aux_id: 'author/name', + cnr_id: undefined, + ver: '1.0.0' + } + }, + expectState: 'disabled' + }, + { + desc: 'Pack not installed', + installed: { + 'a different pack': { + enabled: true, + cnr_id: 'a different pack', + ver: '1.0.0', + aux_id: undefined + } + }, + expectState: 'disabled' + } + ] + + describe('isPackEnabled', () => { + it.each(testCases)( + '$expectState when $desc', + async ({ installed, expectState, packName }) => { + packName ??= 'name' + + const store = useComfyManagerStore() + await triggerPacksChange(installed, store) + + const enabled = expectState === 'enabled' + expect(store.isPackEnabled(packName)).toBe(enabled) + } + ) + }) +})