mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-20 06:20:11 +00:00
[backport core/1.40] fix: invalidate loader node dropdown cache after model asset deletion (#9597)
Backport of #8434 to `core/1.40` Automatically created by backport workflow. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9597-backport-core-1-40-fix-invalidate-loader-node-dropdown-cache-after-model-asset-deletio-31d6d73d36508151b468ff8a9d634762) by [Unito](https://www.unito.io) Co-authored-by: Christian Byrne <cbyrne@comfy.org> Co-authored-by: Amp <amp@ampcode.com> Co-authored-by: Alexander Brown <drjkl@comfy.org>
This commit is contained in:
@@ -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'
|
||||
)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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()
|
||||
@@ -633,6 +635,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({
|
||||
|
||||
@@ -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()
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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
|
||||
}
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user