fix: invalidate loader node dropdown cache after model asset deletion (#8434)

## 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)


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## 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.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Amp <amp@ampcode.com>
Co-authored-by: Alexander Brown <drjkl@comfy.org>
This commit is contained in:
Christian Byrne
2026-02-21 20:55:32 -08:00
committed by GitHub
parent 38675e658f
commit ef5198be25
4 changed files with 308 additions and 10 deletions

View File

@@ -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> = {}): AssetItem {
return {
id: 'test-asset-id',
@@ -115,7 +155,7 @@ function createMockAsset(overrides: Partial<AssetItem> = {}): 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> } }) => {
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> } }) => {
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> } }) => {
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> } }) => {
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'
)
})
})
})

View File

@@ -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<string>()
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({

View File

@@ -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()
})
})
})

View File

@@ -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
}
})