Skip to content

feat: Stale-while-revalidate pattern for AssetBrowserModal #7880

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DrJKL merged 6 commits into from
Jan 7, 2026
Merged

feat: Stale-while-revalidate pattern for AssetBrowserModal #7880

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
82509e6
feat: Use store cache for AssetBrowserModal with stale-while-revalida…
DrJKL Jan 7, 2026
1432157
refactor: Simplify AssetBrowserModal and useModelUpload
DrJKL Jan 7, 2026
9629529
test: remove redundant mock clears.
DrJKL Jan 7, 2026
deaccc2
Update src/platform/assets/components/AssetBrowserModal.vue
DrJKL Jan 7, 2026
70c80a7
test: Clear maps between tests
DrJKL Jan 7, 2026
8517c3c
refactor: Reduce try block scope in updateModelsForKey
DrJKL Jan 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 50 additions & 39 deletions src/platform/assets/components/AssetBrowserModal.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,30 +4,23 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'

import AssetBrowserModal from '@/platform/assets/components/AssetBrowserModal.vue'
import type { AssetItem } from '@/platform/assets/schemas/assetSchema'

const mockAssetService = vi.hoisted(() => ({
getAssetsForNodeType: vi.fn(),
getAssetsByTag: vi.fn(),
getAssetDetails: vi.fn((id: string) =>
Promise.resolve({
id,
name: 'Test Model',
user_metadata: {
filename: 'Test Model'
}
})
)
}))
import { useAssetsStore } from '@/stores/assetsStore'

vi.mock('@/i18n', () => ({
t: (key: string, params?: Record<string, string>) =>
params ? `${key}:${JSON.stringify(params)}` : key,
d: (date: Date) => date.toLocaleDateString()
}))

vi.mock('@/platform/assets/services/assetService', () => ({
assetService: mockAssetService
}))
vi.mock('@/stores/assetsStore', () => {
const store = {
modelAssetsByNodeType: new Map<string, unknown[]>(),
modelLoadingByNodeType: new Map<string, boolean>(),
updateModelsForNodeType: vi.fn(),
updateModelsForTag: vi.fn()
}
return { useAssetsStore: () => store }
})
Comment thread
coderabbitai[bot] marked this conversation as resolved.

vi.mock('@/stores/modelToNodeStore', () => ({
useModelToNodeStore: () => ({
Expand Down Expand Up @@ -190,9 +183,10 @@ describe('AssetBrowserModal', () => {
})
}

const mockStore = useAssetsStore()

beforeEach(() => {
mockAssetService.getAssetsForNodeType.mockReset()
mockAssetService.getAssetsByTag.mockReset()
vi.clearAllMocks()
})
Comment thread
DrJKL marked this conversation as resolved.
Comment thread
coderabbitai[bot] marked this conversation as resolved.

describe('Integration with useAssetBrowser', () => {
Expand All @@ -201,7 +195,7 @@ describe('AssetBrowserModal', () => {
createTestAsset('asset1', 'Model A', 'checkpoints'),
createTestAsset('asset2', 'Model B', 'loras')
]
mockAssetService.getAssetsForNodeType.mockResolvedValueOnce(assets)
mockStore.modelAssetsByNodeType.set('CheckpointLoaderSimple', assets)

const wrapper = createWrapper({ nodeType: 'CheckpointLoaderSimple' })
await flushPromises()
Expand All @@ -218,7 +212,7 @@ describe('AssetBrowserModal', () => {
createTestAsset('c1', 'model.safetensors', 'checkpoints'),
createTestAsset('l1', 'lora.pt', 'loras')
]
mockAssetService.getAssetsForNodeType.mockResolvedValueOnce(assets)
mockStore.modelAssetsByNodeType.set('CheckpointLoaderSimple', assets)

const wrapper = createWrapper({
nodeType: 'CheckpointLoaderSimple',
Expand All @@ -234,31 +228,54 @@ describe('AssetBrowserModal', () => {
})

describe('Data fetching', () => {
it('fetches assets for node type', async () => {
mockAssetService.getAssetsForNodeType.mockResolvedValueOnce([])

it('triggers store refresh for node type on mount', async () => {
createWrapper({ nodeType: 'CheckpointLoaderSimple' })
await flushPromises()

expect(mockAssetService.getAssetsForNodeType).toHaveBeenCalledWith(
expect(mockStore.updateModelsForNodeType).toHaveBeenCalledWith(
'CheckpointLoaderSimple'
)
})

it('fetches assets for tag when node type not provided', async () => {
mockAssetService.getAssetsByTag.mockResolvedValueOnce([])
it('displays cached assets immediately from store', async () => {
const assets = [createTestAsset('asset1', 'Cached Model', 'checkpoints')]
mockStore.modelAssetsByNodeType.set('CheckpointLoaderSimple', assets)

const wrapper = createWrapper({ nodeType: 'CheckpointLoaderSimple' })

const assetGrid = wrapper.findComponent({ name: 'AssetGrid' })
const gridAssets = assetGrid.props('assets') as AssetItem[]

createWrapper({ assetType: 'loras' })
expect(gridAssets).toHaveLength(1)
expect(gridAssets[0].name).toBe('Cached Model')
})

it('triggers store refresh for asset type (tag) on mount', async () => {
createWrapper({ assetType: 'models' })
await flushPromises()

expect(mockAssetService.getAssetsByTag).toHaveBeenCalledWith('loras')
expect(mockStore.updateModelsForTag).toHaveBeenCalledWith('models')
})

it('uses tag: prefix for cache key when assetType is provided', async () => {
const assets = [createTestAsset('asset1', 'Tagged Model', 'models')]
mockStore.modelAssetsByNodeType.set('tag:models', assets)

const wrapper = createWrapper({ assetType: 'models' })
await flushPromises()

const assetGrid = wrapper.findComponent({ name: 'AssetGrid' })
const gridAssets = assetGrid.props('assets') as AssetItem[]

expect(gridAssets).toHaveLength(1)
expect(gridAssets[0].name).toBe('Tagged Model')
})
Comment on lines 232 to 274

@coderabbitai coderabbitai Bot Jan 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Add tests to verify stale-while-revalidate behavior described in PR objectives.

The PR implements a "stale-while-revalidate pattern" where the loading spinner is shown only when loading AND no cached data exists. However, the current tests don't verify these key behaviors:

  1. Loading spinner visibility conditional on cache state (loading + no cache = show spinner; loading + has cache = hide spinner)
  2. Background refresh occurs even when displaying cached assets

The test "displays cached assets immediately from store" (lines 240-251) verifies cached data is shown but doesn't confirm that updateModelsForNodeType is still called for background refresh.

📝 Suggested tests for stale-while-revalidate behavior 
it('shows loading spinner only when no cached data exists', async () => {
  mockStore.modelLoadingByNodeType.set('CheckpointLoaderSimple', true)
  // No cached data
  
  const wrapper = createWrapper({ nodeType: 'CheckpointLoaderSimple' })
  
  const assetGrid = wrapper.findComponent({ name: 'AssetGrid' })
  expect(assetGrid.props('loading')).toBe(true)
})

it('hides loading spinner when cached data exists during refresh', async () => {
  const assets = [createTestAsset('asset1', 'Cached Model', 'checkpoints')]
  mockStore.modelAssetsByNodeType.set('CheckpointLoaderSimple', assets)
  mockStore.modelLoadingByNodeType.set('CheckpointLoaderSimple', true)
  
  const wrapper = createWrapper({ nodeType: 'CheckpointLoaderSimple' })
  
  const assetGrid = wrapper.findComponent({ name: 'AssetGrid' })
  expect(assetGrid.props('loading')).toBe(false)
  expect(assetGrid.props('assets')).toHaveLength(1)
})

it('triggers background refresh even when cached data is displayed', async () => {
  const assets = [createTestAsset('asset1', 'Cached Model', 'checkpoints')]
  mockStore.modelAssetsByNodeType.set('CheckpointLoaderSimple', assets)
  
  createWrapper({ nodeType: 'CheckpointLoaderSimple' })
  await flushPromises()
  
  expect(mockStore.updateModelsForNodeType).toHaveBeenCalledWith('CheckpointLoaderSimple')
})

Based on learnings: Aim for behavioral coverage of critical and new features in unit tests.

🤖 Prompt for AI Agents 
In @src/platform/assets/components/AssetBrowserModal.test.ts around lines 230 -
272, Add unit tests to cover the stale-while-revalidate behavior: verify loading
spinner logic by setting mockStore.modelLoadingByNodeType for
'CheckpointLoaderSimple' with and without mockStore.modelAssetsByNodeType cached
assets and assert AssetGrid.props('loading') is true when loading and no cache,
but false when loading and cache exists (also assert assets length); and add a
test that sets cached assets then mounts the component and awaits flushPromises
to assert mockStore.updateModelsForNodeType('CheckpointLoaderSimple') is still
called to confirm background refresh.

})

describe('Asset Selection', () => {
it('emits asset-select event when asset is selected', async () => {
const assets = [createTestAsset('asset1', 'Model A', 'checkpoints')]
mockAssetService.getAssetsForNodeType.mockResolvedValueOnce(assets)
mockStore.modelAssetsByNodeType.set('CheckpointLoaderSimple', assets)

const wrapper = createWrapper({ nodeType: 'CheckpointLoaderSimple' })
await flushPromises()
Expand All @@ -271,7 +288,7 @@ describe('AssetBrowserModal', () => {

it('executes onSelect callback when provided', async () => {
const assets = [createTestAsset('asset1', 'Model A', 'checkpoints')]
mockAssetService.getAssetsForNodeType.mockResolvedValueOnce(assets)
mockStore.modelAssetsByNodeType.set('CheckpointLoaderSimple', assets)

const onSelect = vi.fn()
const wrapper = createWrapper({
Expand All @@ -289,8 +306,6 @@ describe('AssetBrowserModal', () => {

describe('Left Panel Conditional Logic', () => {
it('hides left panel by default when showLeftPanel is undefined', async () => {
mockAssetService.getAssetsForNodeType.mockResolvedValueOnce([])

const wrapper = createWrapper({ nodeType: 'CheckpointLoaderSimple' })
await flushPromises()

Expand All @@ -299,8 +314,6 @@ describe('AssetBrowserModal', () => {
})

it('shows left panel when showLeftPanel prop is explicitly true', async () => {
mockAssetService.getAssetsForNodeType.mockResolvedValueOnce([])

const wrapper = createWrapper({
nodeType: 'CheckpointLoaderSimple',
showLeftPanel: true
Expand All @@ -318,7 +331,7 @@ describe('AssetBrowserModal', () => {
createTestAsset('asset1', 'Model A', 'checkpoints'),
createTestAsset('asset2', 'Model B', 'loras')
]
mockAssetService.getAssetsForNodeType.mockResolvedValueOnce(assets)
mockStore.modelAssetsByNodeType.set('CheckpointLoaderSimple', assets)

const wrapper = createWrapper({
nodeType: 'CheckpointLoaderSimple',
Expand All @@ -339,8 +352,6 @@ describe('AssetBrowserModal', () => {

describe('Title Management', () => {
it('passes custom title to BaseModalLayout when title prop provided', async () => {
mockAssetService.getAssetsForNodeType.mockResolvedValueOnce([])

const wrapper = createWrapper({
nodeType: 'CheckpointLoaderSimple',
title: 'Custom Title'
Expand All @@ -353,7 +364,7 @@ describe('AssetBrowserModal', () => {

it('passes computed contentTitle to BaseModalLayout when no title prop', async () => {
const assets = [createTestAsset('asset1', 'Model A', 'checkpoints')]
mockAssetService.getAssetsForNodeType.mockResolvedValueOnce(assets)
mockStore.modelAssetsByNodeType.set('CheckpointLoaderSimple', assets)

const wrapper = createWrapper({ nodeType: 'CheckpointLoaderSimple' })
await flushPromises()
Expand Down
82 changes: 37 additions & 45 deletions src/platform/assets/components/AssetBrowserModal.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,8 @@
</template>

<script setup lang="ts">
import {
breakpointsTailwind,
useAsyncState,
useBreakpoints
} from '@vueuse/core'
import { computed, provide, watch } from 'vue'
import { breakpointsTailwind, useBreakpoints } from '@vueuse/core'
import { computed, provide } from 'vue'
import { useI18n } from 'vue-i18n'

import SearchBox from '@/components/common/SearchBox.vue'
Expand All @@ -81,68 +77,68 @@ import type { AssetDisplayItem } from '@/platform/assets/composables/useAssetBro
import { useAssetBrowser } from '@/platform/assets/composables/useAssetBrowser'
import { useModelUpload } from '@/platform/assets/composables/useModelUpload'
import type { AssetItem } from '@/platform/assets/schemas/assetSchema'
import { assetService } from '@/platform/assets/services/assetService'
import { formatCategoryLabel } from '@/platform/assets/utils/categoryLabel'
import { useAssetDownloadStore } from '@/stores/assetDownloadStore'
import { useAssetsStore } from '@/stores/assetsStore'
import { useModelToNodeStore } from '@/stores/modelToNodeStore'
import { OnCloseKey } from '@/types/widgetTypes'

const { t } = useI18n()
const assetStore = useAssetsStore()
const modelToNodeStore = useModelToNodeStore()
const breakpoints = useBreakpoints(breakpointsTailwind)

const props = defineProps<{
nodeType?: string
assetType?: string
onSelect?: (asset: AssetItem) => void
onClose?: () => void
showLeftPanel?: boolean
title?: string
assetType?: string
}>()

const { t } = useI18n()

const emit = defineEmits<{
'asset-select': [asset: AssetDisplayItem]
close: []
}>()

const breakpoints = useBreakpoints(breakpointsTailwind)

provide(OnCloseKey, props.onClose ?? (() => {}))

const fetchAssets = async () => {
// Compute the cache key based on nodeType or assetType
const cacheKey = computed(() => {
if (props.nodeType) return props.nodeType
if (props.assetType) return `tag:${props.assetType}`
return ''
})

// Read directly from store cache - reactive to any store updates
const fetchedAssets = computed(
() => assetStore.modelAssetsByNodeType.get(cacheKey.value) ?? []
)

const isStoreLoading = computed(
() => assetStore.modelLoadingByNodeType.get(cacheKey.value) ?? false
)

// Only show loading spinner when loading AND no cached data
const isLoading = computed(
() => isStoreLoading.value && fetchedAssets.value.length === 0
)

async function refreshAssets(): Promise<AssetItem[]> {
if (props.nodeType) {
return (await assetService.getAssetsForNodeType(props.nodeType)) ?? []
return await assetStore.updateModelsForNodeType(props.nodeType)
}

if (props.assetType) {
return (await assetService.getAssetsByTag(props.assetType)) ?? []
return await assetStore.updateModelsForTag(props.assetType)
}

return []
}
Comment on lines +127 to 135

@coderabbitai coderabbitai Bot Jan 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider handling empty cacheKey edge case.

When neither nodeType nor assetType is provided, cacheKey is an empty string and refreshAssets returns an empty array without calling any store method. This is likely intentional, but consider adding a warning in development mode if the component is mounted without either prop.

🤖 Prompt for AI Agents 
In @src/platform/assets/components/AssetBrowserModal.vue around lines 127 - 135,
The component currently returns an empty array from refreshAssets when neither
props.nodeType nor props.assetType is set (resulting in an empty cacheKey); add
a development-only warning to surface this misconfiguration by checking both
props (props.nodeType and props.assetType) either inside refreshAssets or in the
component lifecycle (e.g., onMounted) and, if both are falsy and
process.env.NODE_ENV !== 'production', call console.warn (or the app logger)
with a clear message referencing AssetBrowserModal and the props so developers
know the component was mounted without nodeType or assetType.


const {
state: fetchedAssets,
isLoading,
execute
} = useAsyncState<AssetItem[]>(fetchAssets, [], { immediate: false })

watch(
() => [props.nodeType, props.assetType],
async () => {
await execute()
},
{ immediate: true }
)

const assetDownloadStore = useAssetDownloadStore()
// Trigger background refresh on mount
refreshAssets()
Comment thread
DrJKL marked this conversation as resolved.
Outdated

watch(
() => assetDownloadStore.hasActiveDownloads,
async (currentlyActive, previouslyActive) => {
if (previouslyActive && !currentlyActive) {
await execute()
}
}
)
const { isUploadButtonEnabled, showUploadDialog } =
useModelUpload(refreshAssets)

const {
searchQuery,
Expand All @@ -153,8 +149,6 @@ const {
updateFilters
} = useAssetBrowser(fetchedAssets)

const modelToNodeStore = useModelToNodeStore()

const primaryCategoryTag = computed(() => {
const assets = fetchedAssets.value ?? []
const tagFromAssets = assets
Expand Down Expand Up @@ -202,6 +196,4 @@ function handleAssetSelectAndEmit(asset: AssetDisplayItem) {
// It handles the appropriate transformation (filename extraction or full asset)
props.onSelect?.(asset)
}

const { isUploadButtonEnabled, showUploadDialog } = useModelUpload(execute)
</script>
6 changes: 2 additions & 4 deletions src/platform/assets/composables/useModelUpload.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,11 @@ import UploadModelDialog from '@/platform/assets/components/UploadModelDialog.vu
import UploadModelDialogHeader from '@/platform/assets/components/UploadModelDialogHeader.vue'
import UploadModelUpgradeModal from '@/platform/assets/components/UploadModelUpgradeModal.vue'
import UploadModelUpgradeModalHeader from '@/platform/assets/components/UploadModelUpgradeModalHeader.vue'
import type { AssetItem } from '@/platform/assets/schemas/assetSchema'
import { useDialogStore } from '@/stores/dialogStore'
import type { UseAsyncStateReturn } from '@vueuse/core'
import { computed } from 'vue'

export function useModelUpload(
execute?: UseAsyncStateReturn<AssetItem[], [], true>['execute']
onUploadSuccess?: () => Promise<unknown> | void
) {
const dialogStore = useDialogStore()
const { flags } = useFeatureFlags()
Expand Down Expand Up @@ -37,7 +35,7 @@ export function useModelUpload(
component: UploadModelDialog,
props: {
onUploadSuccess: async () => {
await execute?.()
await onUploadSuccess?.()
}
},
dialogComponentProps: {
Expand Down
Loading