From ef5198be254c328448377a0cdfe67f4276003d77 Mon Sep 17 00:00:00 2001 From: Christian Byrne Date: Sat, 21 Feb 2026 20:55:32 -0800 Subject: [PATCH] fix: invalidate loader node dropdown cache after model asset deletion (#8434) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary When deleting a model asset (checkpoint, lora, etc.), the loader node dropdowns now update correctly by invalidating the category-keyed cache. ## Problem After deleting a model asset in the asset browser, the loader node dropdowns (e.g., CheckpointLoaderSimple, LoraLoader) still showed the deleted model. Users had to refresh or re-open the dropdown to see the updated list. ## Solution After successful asset deletion, check each deleted asset's tags for model categories (checkpoints, loras, etc.) and call `assetsStore.invalidateCategory()` for each affected category. This triggers a refetch when the dropdown is next accessed. ## Changes - In `useMediaAssetActions.ts`: - After deletion, iterate through deleted assets' tags - Check if each tag corresponds to a model category using `modelToNodeStore.getAllNodeProviders()` - Call `invalidateCategory()` for each affected category - In `useMediaAssetActions.test.ts`: - Added mocks for `useAssetsStore` and `useModelToNodeStore` - Added tests for deletion invalidation behavior ## Testing - Added unit tests verifying: - Model cache is invalidated when deleting model assets - Multiple categories are invalidated when deleting multiple assets - Non-model assets (input, output) don't trigger invalidation ## Part of Stack This is **PR 2 of 2** in a stacked PR series: 1. **[PR 1](https://github.com/Comfy-Org/ComfyUI_frontend/pull/8433)**: Refactor asset cache to category-keyed (architectural improvement) 2. **This PR**: Fix deletion invalidation using the clean architecture ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8434-fix-invalidate-loader-node-dropdown-cache-after-model-asset-deletion-2f76d73d3650813181aedc373d9799c6) by [Unito](https://www.unito.io) ## Summary by CodeRabbit * **Bug Fixes** * Improved model cache invalidation after asset deletions — only relevant model categories are invalidated and non-model assets are ignored. * Fixed edge-rendering behavior so reroutes are cleared correctly in the canvas. * **Chores** * Added category-aware cache management and targeted refreshes for model assets. * **Tests** * Expanded tests for cache invalidation, category handling, workflow interactions, and related mocks. --------- Co-authored-by: Amp Co-authored-by: Alexander Brown --- .../composables/useMediaAssetActions.test.ts | 158 +++++++++++++++++- .../composables/useMediaAssetActions.ts | 18 ++ src/stores/assetsStore.test.ts | 103 +++++++++++- src/stores/assetsStore.ts | 39 ++++- 4 files changed, 308 insertions(+), 10 deletions(-) diff --git a/src/platform/assets/composables/useMediaAssetActions.test.ts b/src/platform/assets/composables/useMediaAssetActions.test.ts index 2d0a7a291..250525b09 100644 --- a/src/platform/assets/composables/useMediaAssetActions.test.ts +++ b/src/platform/assets/composables/useMediaAssetActions.test.ts @@ -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 type { LGraphNode } from '@/lib/litegraph/src/litegraph' @@ -35,12 +36,32 @@ vi.mock('vue-i18n', () => ({ }) })) +const mockShowDialog = vi.hoisted(() => vi.fn()) vi.mock('@/stores/dialogStore', () => ({ useDialogStore: () => ({ - showDialog: vi.fn() + showDialog: mockShowDialog }) })) +const mockInvalidateModelsForCategory = vi.hoisted(() => vi.fn()) +const mockSetAssetDeleting = vi.hoisted(() => vi.fn()) +const mockUpdateHistory = vi.hoisted(() => vi.fn()) +const mockUpdateInputs = vi.hoisted(() => vi.fn()) +const mockHasCategory = vi.hoisted(() => vi.fn()) +vi.mock('@/stores/assetsStore', () => ({ + useAssetsStore: () => ({ + setAssetDeleting: mockSetAssetDeleting, + updateHistory: mockUpdateHistory, + updateInputs: mockUpdateInputs, + invalidateModelsForCategory: mockInvalidateModelsForCategory, + hasCategory: mockHasCategory + }) +})) + +vi.mock('@/stores/modelToNodeStore', () => ({ + useModelToNodeStore: () => ({}) +})) + vi.mock('@/composables/useCopyToClipboard', () => ({ useCopyToClipboard: () => ({ copyToClipboard: vi.fn() @@ -93,14 +114,33 @@ vi.mock('@/utils/typeGuardUtil', () => ({ isResultItemType: vi.fn().mockReturnValue(true) })) +const mockGetAssetType = vi.hoisted(() => vi.fn()) vi.mock('@/platform/assets/utils/assetTypeUtil', () => ({ - getAssetType: vi.fn().mockReturnValue('input') + getAssetType: mockGetAssetType })) vi.mock('../schemas/assetMetadataSchema', () => ({ getOutputAssetMetadata: vi.fn().mockReturnValue(null) })) +const mockDeleteAsset = vi.hoisted(() => vi.fn()) +vi.mock('../services/assetService', () => ({ + assetService: { + deleteAsset: mockDeleteAsset + } +})) + +vi.mock('@/scripts/api', () => ({ + api: { + deleteItem: vi.fn(), + apiURL: vi.fn((path: string) => `http://localhost:8188/api${path}`), + internalURL: vi.fn((path: string) => `http://localhost:8188${path}`), + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + user: 'test-user' + } +})) + function createMockAsset(overrides: Partial = {}): AssetItem { return { id: 'test-asset-id', @@ -115,7 +155,7 @@ function createMockAsset(overrides: Partial = {}): AssetItem { describe('useMediaAssetActions', () => { beforeEach(() => { vi.resetModules() - setActivePinia(createPinia()) + setActivePinia(createTestingPinia({ stubActions: false })) vi.clearAllMocks() capturedFilenames.values = [] mockIsCloud.value = false @@ -218,4 +258,114 @@ describe('useMediaAssetActions', () => { }) }) }) + + describe('deleteAssets - model cache invalidation', () => { + beforeEach(() => { + mockIsCloud.value = true + mockGetAssetType.mockReturnValue('input') + mockDeleteAsset.mockResolvedValue(undefined) + mockInvalidateModelsForCategory.mockClear() + mockSetAssetDeleting.mockClear() + mockUpdateHistory.mockClear() + mockUpdateInputs.mockClear() + mockHasCategory.mockClear() + // By default, hasCategory returns true for model categories + mockHasCategory.mockImplementation( + (tag: string) => tag === 'checkpoints' || tag === 'loras' + ) + }) + + it('should invalidate model cache when deleting a model asset', async () => { + const actions = useMediaAssetActions() + + const modelAsset = createMockAsset({ + id: 'checkpoint-1', + name: 'model.safetensors', + tags: ['models', 'checkpoints'] + }) + + mockShowDialog.mockImplementation( + ({ props }: { props: { onConfirm: () => Promise } }) => { + void props.onConfirm() + } + ) + + await actions.deleteAssets(modelAsset) + + // Only 'checkpoints' exists in cache; 'models' is excluded + expect(mockInvalidateModelsForCategory).toHaveBeenCalledTimes(1) + expect(mockInvalidateModelsForCategory).toHaveBeenCalledWith( + 'checkpoints' + ) + }) + + it('should invalidate multiple categories for multiple assets', async () => { + const actions = useMediaAssetActions() + + const assets = [ + createMockAsset({ id: '1', tags: ['models', 'checkpoints'] }), + createMockAsset({ id: '2', tags: ['models', 'loras'] }) + ] + + mockShowDialog.mockImplementation( + ({ props }: { props: { onConfirm: () => Promise } }) => { + void props.onConfirm() + } + ) + + await actions.deleteAssets(assets) + + expect(mockInvalidateModelsForCategory).toHaveBeenCalledWith( + 'checkpoints' + ) + expect(mockInvalidateModelsForCategory).toHaveBeenCalledWith('loras') + }) + + it('should not invalidate model cache for non-model assets', async () => { + const actions = useMediaAssetActions() + + const inputAsset = createMockAsset({ + id: 'input-1', + name: 'image.png', + tags: ['input'] + }) + + mockShowDialog.mockImplementation( + ({ props }: { props: { onConfirm: () => Promise } }) => { + void props.onConfirm() + } + ) + + await actions.deleteAssets(inputAsset) + + // 'input' tag is excluded, so no cache invalidation + expect(mockInvalidateModelsForCategory).not.toHaveBeenCalled() + }) + + it('should only invalidate categories that exist in cache', async () => { + const actions = useMediaAssetActions() + + // hasCategory returns false for 'unknown-category' + mockHasCategory.mockImplementation((tag: string) => tag === 'checkpoints') + + const assets = [ + createMockAsset({ id: '1', tags: ['models', 'checkpoints'] }), + createMockAsset({ id: '2', tags: ['models', 'unknown-category'] }) + ] + + mockShowDialog.mockImplementation( + ({ props }: { props: { onConfirm: () => Promise } }) => { + void props.onConfirm() + } + ) + + await actions.deleteAssets(assets) + + // Only checkpoints should be invalidated (unknown-category not in cache) + expect(mockInvalidateModelsForCategory).toHaveBeenCalledTimes(1) + expect(mockInvalidateModelsForCategory).toHaveBeenCalledWith( + 'checkpoints' + ) + }) + }) }) diff --git a/src/platform/assets/composables/useMediaAssetActions.ts b/src/platform/assets/composables/useMediaAssetActions.ts index bc8d0442b..0dfd7c773 100644 --- a/src/platform/assets/composables/useMediaAssetActions.ts +++ b/src/platform/assets/composables/useMediaAssetActions.ts @@ -26,6 +26,8 @@ import type { AssetItem } from '../schemas/assetSchema' import { MediaAssetKey } from '../schemas/mediaAssetSchema' import { assetService } from '../services/assetService' +const EXCLUDED_TAGS = new Set(['models', 'input', 'output']) + export function useMediaAssetActions() { const { t } = useI18n() const toast = useToast() @@ -639,6 +641,22 @@ export function useMediaAssetActions() { await assetsStore.updateInputs() } + // Invalidate model caches for affected categories + const modelCategories = new Set() + + for (const asset of assetArray) { + for (const tag of asset.tags ?? []) { + if (EXCLUDED_TAGS.has(tag)) continue + if (assetsStore.hasCategory(tag)) { + modelCategories.add(tag) + } + } + } + + for (const category of modelCategories) { + assetsStore.invalidateModelsForCategory(category) + } + // Show appropriate feedback based on results if (failed.length === 0) { toast.add({ diff --git a/src/stores/assetsStore.test.ts b/src/stores/assetsStore.test.ts index 04266b6f1..7fbfc102c 100644 --- a/src/stores/assetsStore.test.ts +++ b/src/stores/assetsStore.test.ts @@ -507,12 +507,12 @@ describe('assetsStore - Model Assets Cache (Cloud)', () => { mockIsCloud.value = false }) - const createMockAsset = (id: string) => ({ + const createMockAsset = (id: string, tags: string[] = ['models']) => ({ id, name: `asset-${id}`, size: 100, created_at: new Date().toISOString(), - tags: ['models'], + tags, preview_url: `http://test.com/${id}` }) @@ -751,4 +751,103 @@ describe('assetsStore - Model Assets Cache (Cloud)', () => { expect(store.getAssets('tag:models')).toEqual([]) }) }) + + describe('hasCategory', () => { + it('should return true for loaded categories', async () => { + const store = useAssetsStore() + const assets = [createMockAsset('asset-1')] + + vi.mocked(assetService.getAssetsForNodeType).mockResolvedValue(assets) + await store.updateModelsForNodeType('CheckpointLoaderSimple') + + expect(store.hasCategory('checkpoints')).toBe(true) + }) + + it('should return true for tag-based category when tag: prefix is not used', async () => { + const store = useAssetsStore() + const assets = [createMockAsset('asset-1')] + + vi.mocked(assetService.getAssetsByTag).mockResolvedValue(assets) + await store.updateModelsForTag('models') + + // hasCategory('models') checks for both 'models' and 'tag:models' + expect(store.hasCategory('models')).toBe(true) + }) + + it('should return false for unloaded categories', () => { + const store = useAssetsStore() + + expect(store.hasCategory('checkpoints')).toBe(false) + expect(store.hasCategory('unknown-category')).toBe(false) + }) + + it('should return false after category is invalidated', async () => { + const store = useAssetsStore() + const assets = [createMockAsset('asset-1')] + + vi.mocked(assetService.getAssetsForNodeType).mockResolvedValue(assets) + await store.updateModelsForNodeType('CheckpointLoaderSimple') + + expect(store.hasCategory('checkpoints')).toBe(true) + + store.invalidateCategory('checkpoints') + + expect(store.hasCategory('checkpoints')).toBe(false) + }) + }) + + describe('invalidateModelsForCategory', () => { + it('should clear cache for category and trigger refetch on next access', async () => { + const store = useAssetsStore() + const initialAssets = [createMockAsset('initial-1')] + const refreshedAssets = [ + createMockAsset('refreshed-1'), + createMockAsset('refreshed-2') + ] + + vi.mocked(assetService.getAssetsForNodeType).mockResolvedValueOnce( + initialAssets + ) + await store.updateModelsForNodeType('CheckpointLoaderSimple') + expect(store.getAssets('CheckpointLoaderSimple')).toHaveLength(1) + + store.invalidateModelsForCategory('checkpoints') + + // Cache should be cleared + expect(store.hasCategory('checkpoints')).toBe(false) + expect(store.getAssets('CheckpointLoaderSimple')).toEqual([]) + + // Next fetch should get fresh data + vi.mocked(assetService.getAssetsForNodeType).mockResolvedValueOnce( + refreshedAssets + ) + await store.updateModelsForNodeType('CheckpointLoaderSimple') + expect(store.getAssets('CheckpointLoaderSimple')).toHaveLength(2) + }) + + it('should clear tag-based caches', async () => { + const store = useAssetsStore() + const tagAssets = [createMockAsset('tag-1'), createMockAsset('tag-2')] + + vi.mocked(assetService.getAssetsByTag).mockResolvedValue(tagAssets) + await store.updateModelsForTag('checkpoints') + await store.updateModelsForTag('models') + + expect(store.getAssets('tag:checkpoints')).toHaveLength(2) + expect(store.getAssets('tag:models')).toHaveLength(2) + + store.invalidateModelsForCategory('checkpoints') + + expect(store.getAssets('tag:checkpoints')).toEqual([]) + expect(store.getAssets('tag:models')).toEqual([]) + }) + + it('should handle unknown categories gracefully', () => { + const store = useAssetsStore() + + expect(() => + store.invalidateModelsForCategory('unknown-category') + ).not.toThrow() + }) + }) }) diff --git a/src/stores/assetsStore.ts b/src/stores/assetsStore.ts index c94676401..bea1c556f 100644 --- a/src/stores/assetsStore.ts +++ b/src/stores/assetsStore.ts @@ -375,6 +375,18 @@ export const useAssetsStore = defineStore('assets', () => { return modelStateByCategory.value.has(category) } + /** + * Check if a category exists in the cache. + * Checks both direct category keys and tag-prefixed keys. + * @param category The category to check (e.g., 'checkpoints', 'loras') + */ + function hasCategory(category: string): boolean { + return ( + modelStateByCategory.value.has(category) || + modelStateByCategory.value.has(`tag:${category}`) + ) + } + /** * Internal helper to fetch and cache assets for a category. * Loads first batch immediately, then progressively loads remaining batches. @@ -608,17 +620,30 @@ export const useAssetsStore = defineStore('assets', () => { } } + /** + * Invalidate model caches for a given category (e.g., 'checkpoints', 'loras') + * Clears the category cache and tag-based caches so next access triggers refetch + * @param category The model category to invalidate (e.g., 'checkpoints') + */ + function invalidateModelsForCategory(category: string): void { + invalidateCategory(category) + invalidateCategory(`tag:${category}`) + invalidateCategory('tag:models') + } + return { getAssets, isLoading, getError, hasMore, hasAssetKey, + hasCategory, updateModelsForNodeType, updateModelsForTag, invalidateCategory, updateAssetMetadata, - updateAssetTags + updateAssetTags, + invalidateModelsForCategory } } @@ -629,11 +654,13 @@ export const useAssetsStore = defineStore('assets', () => { getError: () => undefined, hasMore: () => false, hasAssetKey: () => false, + hasCategory: () => false, updateModelsForNodeType: async () => {}, invalidateCategory: () => {}, updateModelsForTag: async () => {}, updateAssetMetadata: async () => {}, - updateAssetTags: async () => {} + updateAssetTags: async () => {}, + invalidateModelsForCategory: () => {} } } @@ -643,11 +670,13 @@ export const useAssetsStore = defineStore('assets', () => { getError, hasMore, hasAssetKey, + hasCategory, updateModelsForNodeType, updateModelsForTag, invalidateCategory, updateAssetMetadata, - updateAssetTags + updateAssetTags, + invalidateModelsForCategory } = getModelState() // Watch for completed downloads and refresh model caches @@ -718,12 +747,14 @@ export const useAssetsStore = defineStore('assets', () => { getError, hasMore, hasAssetKey, + hasCategory, // Model assets - actions updateModelsForNodeType, updateModelsForTag, invalidateCategory, updateAssetMetadata, - updateAssetTags + updateAssetTags, + invalidateModelsForCategory } })