[Manager] Fix enabled/disabled node pack logic (#3061)

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
   }
 
-  /**
+  const getPackId = (pack: ManagerPackInstalled) => pack.cnr_id || pack.aux_id
-   * 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 isInstalledPackId = (packName: string | undefined): boolean =>
     !!packName && installedPacksIds.value.has(packName)
@@ -63,13 +47,57 @@ export const useComfyManagerStore = defineStore('comfyManager', () => {
       return acc
     }, new Set<string>())
 
-  watchEffect(() => {
+  /**
-    const packs = Object.values(installedPacks.value)
+   * A pack is disabled if there is a disabled entry and no corresponding
-    const [disabled, enabled] = partition(packs, isDisabledPack)
+   * enabled entry. If `packname@1.0.2` is disabled, but `packname@1.0.3` is
-    enabledPacksIds.value = packsToIdSet(enabled)
+   * enabled, then `packname` is considered enabled.
-    disabledPacksIds.value = packsToIdSet(disabled)
+   *
+   * @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<string>()
+    const disabledIds = new Set<string>()
+
+    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()

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<string, ManagerPackInstalled>
+  expectState: 'enabled' | 'disabled'
+  /** @default 'name' */
+  packName?: string
+}
+
+describe('useComfyManagerStore', () => {
+  let mockManagerService: {
+    isLoading: ReturnType<typeof ref<boolean>>
+    error: ReturnType<typeof ref<string | null>>
+    listInstalledPacks: ReturnType<typeof vi.fn>
+  }
+
+  const triggerPacksChange = async (
+    installedPacks: InstalledPacksResponse,
+    store: ReturnType<typeof useComfyManagerStore>
+  ) => {
+    // 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)
+      }
+    )
+  })
+})