From e2de4b19fc3e2a60aa4072961bc90cc16e79edf8 Mon Sep 17 00:00:00 2001 From: Jin Yi Date: Sun, 7 Sep 2025 15:11:12 +0900 Subject: [PATCH] Fix version detection for disabled packs (#5395) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: normalize pack IDs to fix version detection for disabled packs When a pack is disabled, ComfyUI-Manager returns it with a version suffix (e.g., "ComfyUI-GGUF@1_1_4") while enabled packs don't have this suffix. This inconsistency caused disabled packs to incorrectly show as having updates available even when they were on the latest version. Changes: - Add normalizePackId utility to consistently remove version suffixes - Apply normalization in refreshInstalledList and WebSocket updates - Use the utility across conflict detection and node help modules - Ensure pack version info is preserved in the object's ver field This fixes the "Update Available" indicator incorrectly showing for disabled packs that are already on the latest version. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude * feature: test code added * test: packUtils test code added * test: address PR review feedback for test improvements - Remove unnecessary .not.toThrow() assertion in useManagerQueue test - Add clarifying comments for version normalization test logic - Replace 'as any' with vi.mocked() for better type safety --------- Co-authored-by: Claude --- src/composables/useConflictDetection.ts | 5 +- src/composables/useManagerQueue.ts | 4 +- src/stores/comfyManagerStore.ts | 10 +- src/utils/nodeHelpUtil.ts | 4 +- src/utils/packUtils.ts | 35 +++ .../tests/composables/useManagerQueue.test.ts | 57 ++++ .../tests/store/comfyManagerStore.test.ts | 93 +++++++ tests-ui/tests/utils/packUtils.test.ts | 254 ++++++++++++++++++ 8 files changed, 450 insertions(+), 12 deletions(-) create mode 100644 src/utils/packUtils.ts create mode 100644 tests-ui/tests/utils/packUtils.test.ts diff --git a/src/composables/useConflictDetection.ts b/src/composables/useConflictDetection.ts index 8edcea41c..abf4d9498 100644 --- a/src/composables/useConflictDetection.ts +++ b/src/composables/useConflictDetection.ts @@ -22,6 +22,7 @@ import type { NodePackRequirements, SystemEnvironment } from '@/types/conflictDetectionTypes' +import { normalizePackId } from '@/utils/packUtils' import { cleanVersion, satisfiesVersion, @@ -874,9 +875,7 @@ function mergeConflictsByPackageName( conflicts.forEach((conflict) => { // Normalize package name by removing version suffix (@1_0_3) for consistent merging - const normalizedPackageName = conflict.package_name.includes('@') - ? conflict.package_name.substring(0, conflict.package_name.indexOf('@')) - : conflict.package_name + const normalizedPackageName = normalizePackId(conflict.package_name) if (mergedMap.has(normalizedPackageName)) { // Package already exists, merge conflicts diff --git a/src/composables/useManagerQueue.ts b/src/composables/useManagerQueue.ts index a02abae40..86fb8d947 100644 --- a/src/composables/useManagerQueue.ts +++ b/src/composables/useManagerQueue.ts @@ -5,6 +5,7 @@ import { Ref, computed, ref } from 'vue' import { app } from '@/scripts/app' import { useDialogService } from '@/services/dialogService' import { components } from '@/types/generatedManagerTypes' +import { normalizePackKeys } from '@/utils/packUtils' type ManagerTaskHistory = Record< string, @@ -98,7 +99,8 @@ export const useManagerQueue = ( taskHistory.value = filterHistoryByClientId(state.history) if (state.installed_packs) { - installedPacks.value = state.installed_packs + // Normalize pack keys to ensure consistent access + installedPacks.value = normalizePackKeys(state.installed_packs) } updateProcessingState() } diff --git a/src/stores/comfyManagerStore.ts b/src/stores/comfyManagerStore.ts index a23f7bd81..9f2663a11 100644 --- a/src/stores/comfyManagerStore.ts +++ b/src/stores/comfyManagerStore.ts @@ -1,5 +1,4 @@ import { useEventListener, whenever } from '@vueuse/core' -import { mapKeys } from 'es-toolkit/compat' import { defineStore } from 'pinia' import { v4 as uuidv4 } from 'uuid' import { ref, watch } from 'vue' @@ -14,6 +13,7 @@ import { useComfyManagerService } from '@/services/comfyManagerService' import { useDialogService } from '@/services/dialogService' import { TaskLog } from '@/types/comfyManagerTypes' import { components } from '@/types/generatedManagerTypes' +import { normalizePackKeys } from '@/utils/packUtils' type InstallPackParams = components['schemas']['InstallPackParams'] type InstalledPacksResponse = components['schemas']['InstalledPacksResponse'] @@ -185,12 +185,8 @@ export const useComfyManagerStore = defineStore('comfyManager', () => { const refreshInstalledList = async () => { const packs = await managerService.listInstalledPacks() if (packs) { - // The keys are 'cleaned' by stripping the version suffix. - // The pack object itself (the value) still contains the version info. - const packsWithCleanedKeys = mapKeys(packs, (_value, key) => { - return key.split('@')[0] - }) - installedPacks.value = packsWithCleanedKeys + // Normalize pack keys to ensure consistent access + installedPacks.value = normalizePackKeys(packs) } isStale.value = false } diff --git a/src/utils/nodeHelpUtil.ts b/src/utils/nodeHelpUtil.ts index 814787a7e..2bcd89961 100644 --- a/src/utils/nodeHelpUtil.ts +++ b/src/utils/nodeHelpUtil.ts @@ -1,12 +1,14 @@ import type { ComfyNodeDefImpl } from '@/stores/nodeDefStore' import { NodeSourceType, getNodeSource } from '@/types/nodeSource' +import { normalizePackId } from '@/utils/packUtils' export function extractCustomNodeName( pythonModule: string | undefined ): string | null { const modules = pythonModule?.split('.') || [] if (modules.length >= 2 && modules[0] === 'custom_nodes') { - return modules[1].split('@')[0] + // Use normalizePackId to remove version suffix + return normalizePackId(modules[1]) } return null } diff --git a/src/utils/packUtils.ts b/src/utils/packUtils.ts new file mode 100644 index 000000000..cbb6fe100 --- /dev/null +++ b/src/utils/packUtils.ts @@ -0,0 +1,35 @@ +import { mapKeys } from 'es-toolkit/compat' + +/** + * Normalizes a pack ID by removing the version suffix. + * + * ComfyUI-Manager returns pack IDs in different formats: + * - Enabled packs: "packname" (without version) + * - Disabled packs: "packname@1_0_3" (with version suffix) + * - Latest versions from registry: "packname" (without version) + * + * Since the pack object itself contains the version info (ver field), + * we normalize all pack IDs to just the base name for consistent access. + * This ensures we can always find a pack by its base name (nodePack.id) + * regardless of its enabled/disabled state. + * + * @param packId - The pack ID that may contain a version suffix + * @returns The normalized pack ID without version suffix + */ +export function normalizePackId(packId: string): string { + return packId.split('@')[0] +} + +/** + * Normalizes all keys in a pack record by removing version suffixes. + * This is used when receiving pack data from the server to ensure + * consistent key format across the application. + * + * @param packs - Record of packs with potentially versioned keys + * @returns Record with normalized keys + */ +export function normalizePackKeys( + packs: Record +): Record { + return mapKeys(packs, (_value, key) => normalizePackId(key)) +} diff --git a/tests-ui/tests/composables/useManagerQueue.test.ts b/tests-ui/tests/composables/useManagerQueue.test.ts index 964c0686b..0d9313446 100644 --- a/tests-ui/tests/composables/useManagerQueue.test.ts +++ b/tests-ui/tests/composables/useManagerQueue.test.ts @@ -161,5 +161,62 @@ describe('useManagerQueue', () => { expect(taskHistory.value).toHaveProperty('task1') expect(taskHistory.value).not.toHaveProperty('task2') }) + + it('normalizes pack IDs when updating installed packs', () => { + const queue = createManagerQueue() + + const mockState = { + history: {}, + running_queue: [], + pending_queue: [], + installed_packs: { + 'ComfyUI-GGUF@1_1_4': { + enabled: false, + cnr_id: 'ComfyUI-GGUF', + ver: '1.1.4' + }, + 'test-pack': { + enabled: true, + cnr_id: 'test-pack', + ver: '2.0.0' + } + } + } + + queue.updateTaskState(mockState) + + // Packs should be accessible by normalized keys + expect(installedPacks.value['ComfyUI-GGUF']).toEqual({ + enabled: false, + cnr_id: 'ComfyUI-GGUF', + ver: '1.1.4' + }) + expect(installedPacks.value['test-pack']).toEqual({ + enabled: true, + cnr_id: 'test-pack', + ver: '2.0.0' + }) + + // Version suffixed keys should not exist after normalization + // The pack should be accessible by its base name only (without @version) + expect(installedPacks.value['ComfyUI-GGUF@1_1_4']).toBeUndefined() + }) + + it('handles empty installed_packs gracefully', () => { + const queue = createManagerQueue() + + const mockState: any = { + history: {}, + running_queue: [], + pending_queue: [], + installed_packs: undefined + } + + // Just call the function - if it throws, the test will fail automatically + queue.updateTaskState(mockState) + + // installedPacks should remain unchanged + expect(installedPacks.value).toEqual({}) + }) }) }) diff --git a/tests-ui/tests/store/comfyManagerStore.test.ts b/tests-ui/tests/store/comfyManagerStore.test.ts index b377c258b..a1d748111 100644 --- a/tests-ui/tests/store/comfyManagerStore.test.ts +++ b/tests-ui/tests/store/comfyManagerStore.test.ts @@ -439,4 +439,97 @@ describe('useComfyManagerStore', () => { expect(store.isPackInstalling('pack-3')).toBe(false) }) }) + + describe('refreshInstalledList with pack ID normalization', () => { + it('normalizes pack IDs by removing version suffixes', async () => { + const mockPacks = { + 'ComfyUI-GGUF@1_1_4': { + enabled: false, + cnr_id: 'ComfyUI-GGUF', + ver: '1.1.4', + aux_id: undefined + }, + 'ComfyUI-Manager': { + enabled: true, + cnr_id: 'ComfyUI-Manager', + ver: '2.0.0', + aux_id: undefined + } + } + + vi.mocked(mockManagerService.listInstalledPacks).mockResolvedValue( + mockPacks + ) + + const store = useComfyManagerStore() + await store.refreshInstalledList() + + // Both packs should be accessible by their base name + expect(store.installedPacks['ComfyUI-GGUF']).toEqual({ + enabled: false, + cnr_id: 'ComfyUI-GGUF', + ver: '1.1.4', + aux_id: undefined + }) + expect(store.installedPacks['ComfyUI-Manager']).toEqual({ + enabled: true, + cnr_id: 'ComfyUI-Manager', + ver: '2.0.0', + aux_id: undefined + }) + + // Version suffixed keys should not exist + expect(store.installedPacks['ComfyUI-GGUF@1_1_4']).toBeUndefined() + }) + + it('handles duplicate keys after normalization', async () => { + const mockPacks = { + 'test-pack': { + enabled: true, + cnr_id: 'test-pack', + ver: '1.0.0', + aux_id: undefined + }, + 'test-pack@1_1_0': { + enabled: false, + cnr_id: 'test-pack', + ver: '1.1.0', + aux_id: undefined + } + } + + vi.mocked(mockManagerService.listInstalledPacks).mockResolvedValue( + mockPacks + ) + + const store = useComfyManagerStore() + await store.refreshInstalledList() + + // The normalized key should exist (last one wins with mapKeys) + expect(store.installedPacks['test-pack']).toBeDefined() + expect(store.installedPacks['test-pack'].ver).toBe('1.1.0') + }) + + it('preserves version information for disabled packs', async () => { + const mockPacks = { + 'disabled-pack@2_0_0': { + enabled: false, + cnr_id: 'disabled-pack', + ver: '2.0.0', + aux_id: undefined + } + } + + vi.mocked(mockManagerService.listInstalledPacks).mockResolvedValue( + mockPacks + ) + + const store = useComfyManagerStore() + await store.refreshInstalledList() + + // Pack should be accessible by base name with version preserved + expect(store.getInstalledPackVersion('disabled-pack')).toBe('2.0.0') + expect(store.isPackInstalled('disabled-pack')).toBe(true) + }) + }) }) diff --git a/tests-ui/tests/utils/packUtils.test.ts b/tests-ui/tests/utils/packUtils.test.ts new file mode 100644 index 000000000..6af9ef43f --- /dev/null +++ b/tests-ui/tests/utils/packUtils.test.ts @@ -0,0 +1,254 @@ +import { describe, expect, it } from 'vitest' + +import { normalizePackId, normalizePackKeys } from '@/utils/packUtils' + +describe('packUtils', () => { + describe('normalizePackId', () => { + it('should return pack ID unchanged when no version suffix exists', () => { + expect(normalizePackId('ComfyUI-GGUF')).toBe('ComfyUI-GGUF') + expect(normalizePackId('ComfyUI-Manager')).toBe('ComfyUI-Manager') + expect(normalizePackId('simple-pack')).toBe('simple-pack') + }) + + it('should remove version suffix with underscores', () => { + expect(normalizePackId('ComfyUI-GGUF@1_1_4')).toBe('ComfyUI-GGUF') + expect(normalizePackId('ComfyUI-Manager@2_0_0')).toBe('ComfyUI-Manager') + expect(normalizePackId('pack@1_0_0_beta')).toBe('pack') + }) + + it('should remove version suffix with dots', () => { + expect(normalizePackId('ComfyUI-GGUF@1.1.4')).toBe('ComfyUI-GGUF') + expect(normalizePackId('pack@2.0.0')).toBe('pack') + }) + + it('should handle multiple @ symbols by only removing after first @', () => { + expect(normalizePackId('pack@1_0_0@extra')).toBe('pack') + expect(normalizePackId('my@pack@1_0_0')).toBe('my') + }) + + it('should handle empty string', () => { + expect(normalizePackId('')).toBe('') + }) + + it('should handle pack ID with @ but no version', () => { + expect(normalizePackId('pack@')).toBe('pack') + }) + + it('should handle special characters in pack name', () => { + expect(normalizePackId('my-pack_v2@1_0_0')).toBe('my-pack_v2') + expect(normalizePackId('pack.with.dots@2_0_0')).toBe('pack.with.dots') + expect(normalizePackId('UPPERCASE-Pack@1_0_0')).toBe('UPPERCASE-Pack') + }) + + it('should handle edge cases', () => { + // Only @ symbol + expect(normalizePackId('@')).toBe('') + expect(normalizePackId('@1_0_0')).toBe('') + + // Whitespace + expect(normalizePackId(' pack @1_0_0')).toBe(' pack ') + expect(normalizePackId('pack @1_0_0')).toBe('pack ') + }) + }) + + describe('normalizePackKeys', () => { + it('should normalize all keys with version suffixes', () => { + const input = { + 'ComfyUI-GGUF': { ver: '1.1.4', enabled: true }, + 'ComfyUI-Manager@2_0_0': { ver: '2.0.0', enabled: false }, + 'another-pack@1_0_0': { ver: '1.0.0', enabled: true } + } + + const expected = { + 'ComfyUI-GGUF': { ver: '1.1.4', enabled: true }, + 'ComfyUI-Manager': { ver: '2.0.0', enabled: false }, + 'another-pack': { ver: '1.0.0', enabled: true } + } + + expect(normalizePackKeys(input)).toEqual(expected) + }) + + it('should handle empty object', () => { + expect(normalizePackKeys({})).toEqual({}) + }) + + it('should handle keys without version suffixes', () => { + const input = { + pack1: { data: 'value1' }, + pack2: { data: 'value2' } + } + + expect(normalizePackKeys(input)).toEqual(input) + }) + + it('should handle mixed keys (with and without versions)', () => { + const input = { + 'normal-pack': { ver: '1.0.0' }, + 'versioned-pack@2_0_0': { ver: '2.0.0' }, + 'another-normal': { ver: '3.0.0' }, + 'another-versioned@4_0_0': { ver: '4.0.0' } + } + + const expected = { + 'normal-pack': { ver: '1.0.0' }, + 'versioned-pack': { ver: '2.0.0' }, + 'another-normal': { ver: '3.0.0' }, + 'another-versioned': { ver: '4.0.0' } + } + + expect(normalizePackKeys(input)).toEqual(expected) + }) + + it('should handle duplicate keys after normalization (last one wins)', () => { + const input = { + 'pack@1_0_0': { ver: '1.0.0', data: 'first' }, + 'pack@2_0_0': { ver: '2.0.0', data: 'second' }, + pack: { ver: '3.0.0', data: 'third' } + } + + const result = normalizePackKeys(input) + + // The exact behavior depends on object iteration order, + // but there should only be one 'pack' key in the result + expect(Object.keys(result)).toEqual(['pack']) + expect(result.pack).toBeDefined() + expect(result.pack.ver).toBeDefined() + }) + + it('should preserve value references', () => { + const value1 = { ver: '1.0.0', complex: { nested: 'data' } } + const value2 = { ver: '2.0.0', complex: { nested: 'data2' } } + + const input = { + 'pack1@1_0_0': value1, + 'pack2@2_0_0': value2 + } + + const result = normalizePackKeys(input) + + // Values should be the same references, not cloned + expect(result.pack1).toBe(value1) + expect(result.pack2).toBe(value2) + }) + + it('should handle special characters in keys', () => { + const input = { + '@1_0_0': { ver: '1.0.0' }, + 'my-pack.v2@2_0_0': { ver: '2.0.0' }, + 'UPPERCASE@3_0_0': { ver: '3.0.0' } + } + + const expected = { + '': { ver: '1.0.0' }, + 'my-pack.v2': { ver: '2.0.0' }, + UPPERCASE: { ver: '3.0.0' } + } + + expect(normalizePackKeys(input)).toEqual(expected) + }) + + it('should work with different value types', () => { + const input = { + 'pack1@1_0_0': 'string value', + 'pack2@2_0_0': 123, + 'pack3@3_0_0': null, + 'pack4@4_0_0': undefined, + 'pack5@5_0_0': true, + pack6: [] + } + + const expected = { + pack1: 'string value', + pack2: 123, + pack3: null, + pack4: undefined, + pack5: true, + pack6: [] + } + + expect(normalizePackKeys(input)).toEqual(expected) + }) + }) + + describe('Integration scenarios from JSDoc examples', () => { + it('should handle the examples from normalizePackId JSDoc', () => { + expect(normalizePackId('ComfyUI-GGUF')).toBe('ComfyUI-GGUF') + expect(normalizePackId('ComfyUI-GGUF@1_1_4')).toBe('ComfyUI-GGUF') + }) + + it('should handle the examples from normalizePackKeys JSDoc', () => { + const input = { + 'ComfyUI-GGUF': { ver: '1.1.4', enabled: true }, + 'ComfyUI-Manager@2_0_0': { ver: '2.0.0', enabled: false } + } + + const expected = { + 'ComfyUI-GGUF': { ver: '1.1.4', enabled: true }, + 'ComfyUI-Manager': { ver: '2.0.0', enabled: false } + } + + expect(normalizePackKeys(input)).toEqual(expected) + }) + }) + + describe('Real-world scenarios', () => { + it('should handle typical ComfyUI-Manager response with mixed enabled/disabled packs', () => { + // Simulating actual server response pattern + const serverResponse = { + // Enabled packs come without version suffix + 'ComfyUI-Essential': { ver: '1.2.3', enabled: true, aux_id: undefined }, + 'ComfyUI-Impact': { ver: '2.0.0', enabled: true, aux_id: undefined }, + // Disabled packs come with version suffix + 'ComfyUI-GGUF@1_1_4': { + ver: '1.1.4', + enabled: false, + aux_id: undefined + }, + 'ComfyUI-Manager@2_5_0': { + ver: '2.5.0', + enabled: false, + aux_id: undefined + } + } + + const normalized = normalizePackKeys(serverResponse) + + // All keys should be normalized (no version suffixes) + expect(Object.keys(normalized)).toEqual([ + 'ComfyUI-Essential', + 'ComfyUI-Impact', + 'ComfyUI-GGUF', + 'ComfyUI-Manager' + ]) + + // Values should be preserved + expect(normalized['ComfyUI-GGUF']).toEqual({ + ver: '1.1.4', + enabled: false, + aux_id: undefined + }) + }) + + it('should allow consistent access by pack ID regardless of enabled state', () => { + const packsBeforeToggle = { + 'my-pack': { ver: '1.0.0', enabled: true } + } + + const packsAfterToggle = { + 'my-pack@1_0_0': { ver: '1.0.0', enabled: false } + } + + const normalizedBefore = normalizePackKeys(packsBeforeToggle) + const normalizedAfter = normalizePackKeys(packsAfterToggle) + + // Both should have the same key after normalization + expect(normalizedBefore['my-pack']).toBeDefined() + expect(normalizedAfter['my-pack']).toBeDefined() + + // Can access by the same key regardless of the original format + expect(Object.keys(normalizedBefore)).toEqual( + Object.keys(normalizedAfter) + ) + }) + }) +})