feat: implement progressive pagination for Asset Browser model assets#8212
Merged
feat: implement progressive pagination for Asset Browser model assets#8212
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces per-node-type public maps with keyed accessors (getAssets, isModelLoading, getError, hasMore), implements per-key progressive pagination and batched loading in the assets store, updates assetService to use PaginationOptions, and migrates components and tests to the new keyed store API. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as AssetBrowserModal / Consumer
participant Store as assetsStore
participant Service as assetService
participant API as External API
Browser->>Store: updateModelsForNodeType(key)
Store->>Service: getAssetsForNodeType(nodeType, {limit, offset})
Service->>API: HTTP GET /assets?include_tags=...&limit=...&offset=...
API-->>Service: batch response (items, hasMore)
Service-->>Store: return batch
Store->>Store: init/merge per-key state (assets, offset, hasMore)
alt hasMore
loop loadRemainingBatches
Store->>Service: getAssetsForNodeType(nodeType, {limit, offset})
Service->>API: HTTP GET /assets?limit=...&offset=...
API-->>Service: next batch
Service-->>Store: append batch -> update offset/hasMore
end
end
Store-->>Browser: getAssets(key) returns aggregated assets
Possibly related PRs
Suggested reviewers
✨ Finishing touches
Comment |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/22/2026, 01:46:44 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Tests:
|
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22.3 kB (baseline 22.3 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.02 MB (baseline 1.02 MB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 80.7 kB (baseline 80.7 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 11 added / 11 removed Panels & Settings — 430 kB (baseline 430 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 8 added / 8 removed User & Accounts — 3.94 kB (baseline 3.94 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 3 added / 3 removed Editors & Dialogs — 2.8 kB (baseline 2.8 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 32.8 kB (baseline 32.8 kB) • ⚪ 0 BReusable component library chunks
Status: 7 added / 7 removed Data & Services — 3.05 MB (baseline 3.05 MB) • 🔴 +2.03 kBStores, services, APIs, and repositories
Status: 7 added / 7 removed Utilities & Hooks — 18.1 kB (baseline 18.1 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 7 added / 7 removed Vendor & Third-Party — 10.4 MB (baseline 10.4 MB) • 🟢 -32 BExternal libraries and shared vendor chunks
Status: 1 added / 1 removed Other — 6.28 MB (baseline 6.28 MB) • 🟢 -48 BBundles that do not match a named category
Status: 34 added / 34 removed |
dosubot
bot
added
the
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/platform/assets/services/assetService.ts`:
- Around line 4-8: Move the PaginationOptions interface so it appears after all
import statements (i.e., after the import block) to follow the import-first
convention; locate the current declaration of PaginationOptions in
assetService.ts and cut/paste it immediately below the last import, ensuring no
imports are interleaved with the interface and preserving its exact name and
shape.
In `@src/renderer/extensions/vueNodes/widgets/composables/useAssetWidgetData.ts`:
- Line 51: Remove the redundant null coalescing fallback in the return map
inside useAssetWidgetData: the computed assets already guarantees an array (via
getAssets(key) defaulting to []), so replace the expression that uses
(assets.value ?? []) with just assets.value in the return mapping; update the
return statement referencing assets (from the computed named assets) and ensure
no other code expects a nullable value.
- Around line 68-69: The code redundantly uses null coalescing when calling
assetsStore.getAssets(currentNodeType); since getAssets always returns an array,
remove the "?? []" and assign directly to existingAssets (const existingAssets =
assetsStore.getAssets(currentNodeType)), then keep hasData =
existingAssets.length > 0 unchanged; update references to existingAssets if any
follow.
In `@src/stores/assetsStore.ts`:
- Around line 354-381: The background loader loadRemainingBatches currently logs
errors but never updates the model state, so partial failures are invisible;
modify loadRemainingBatches to set an error on the per-key state
(modelStateByKey.value.get(key)?.error = err or similar) inside the catch, and
also set state.hasMore = false (or another partial-failure flag on the state) to
stop further attempts; after successfully processing each batch (after updating
state.offset and state.hasMore) insert a small await delay (e.g., await new
Promise(resolve => setTimeout(resolve, 50))) to throttle requests and avoid
rapid-fire fetching.
- Around line 298-300: getAssets currently does
Array.from(modelStateByKey.value.get(key)?.assets.values() ?? []), which
allocates a new array on every call and breaks downstream reactivity; update
getAssets to avoid creating a new array each call by either returning the Map
(or its values iterator) directly from modelStateByKey.value.get(key)?.assets so
callers can compute/memoize the array, or implement a simple memo cache keyed by
`key` (e.g., store the last array for each key and only rebuild it when the
underlying Map changes). Adjust callers that expect an array to use computed()
or to convert the iterator once; reference getAssets and modelStateByKey to
locate and apply the change.
src/renderer/extensions/vueNodes/widgets/composables/useAssetWidgetData.ts
Outdated
Show resolved
Hide resolved
src/renderer/extensions/vueNodes/widgets/composables/useAssetWidgetData.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/stores/assetsStore.ts`:
- Around line 273-286: The state object returned by createInitialState is
currently wrapped with reactive(), causing Vue to deeply convert the assets Map;
change this to use shallowReactive(state) so state.assets (the Map replaced
later via assignments like state.assets = new Map(...)) is not deeply proxied.
Update the import to pull shallowReactive from 'vue' and keep the rest of the
ModelPaginationState usage (including state.assets replacements) unchanged.
- Around line 304-317: getAssets caches arrays by Map reference
(assetsArrayCache) but loadRemainingBatches mutates state.assets in-place via
state.assets.set(), creating a race where getAssets can return a stale array;
fix by invalidating the cache before in-place mutations: in the code path
loadRemainingBatches (or any code that calls state.assets.set/clear), call
assetsArrayCache.delete(key) immediately before mutating state.assets (or
alternatively implement a small version counter on the state and check it in
getAssets), so getAssets sees the deletion and will rebuild the array from the
mutated Map.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/stores/assetsStore.ts`:
- Around line 335-405: Concurrent calls corrupt pagination because background
loadRemainingBatches mutates shared state; add a per-key request token to
modelStateByKey entries (e.g., state.requestId), generate a new token at the
start of updateModelsForKey (inside updateModelsForKey) and assign it to
state.requestId, pass that token into loadRemainingBatches (or have
loadRemainingBatches read state.requestId at each iteration) and before any
mutation of state.offset/hasMore/assets/error ensure the token matches the
current state.requestId; if it differs, stop the background loop so stale
loaders don't advance pagination. Also ensure when updateModelsForKey starts a
new background load (void loadRemainingBatches(...)) it uses the newly generated
token so previous loops exit.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/stores/assetsStore.test.ts`:
- Around line 544-554: The test is brittle because it asserts reference equality
between firstArrayRef and secondArrayRef from store.getAssets; remove the
expect(secondArrayRef).toBe(firstArrayRef) assertion and instead assert
behavior: verify the array contains the new asset id (e.g., map(a =>
a.id).toContain('new-asset')) and that the asset count changed appropriately
(e.g., length increased by 1) after the mutation; if you still need to test
caching semantics separately, add a dedicated test that explicitly documents
expected caching behaviour for store.getAssets rather than coupling it to this
mutation test.
- Around line 463-471: Move the Pinia setup into the test suite lifecycle by
calling setActivePinia(createPinia()) inside the existing beforeEach and remove
inline setActivePinia/createPinia and useAssetsStore() calls from individual
tests; ensure afterEach still resets mockIsCloud.value and clear mocks as needed
and that tests now call useAssetsStore() only when needed without reinitializing
Pinia.
- Around line 482-514: Add a concurrency test to ensure
updateModelsForNodeType/updateModelsForKey cancels or ignores stale background
loads: mock assetService.getAssetsForNodeType to return a fast first batch, a
second hanging Promise (resolve later) and then an empty/fresh batch; call
store.updateModelsForNodeType twice (starting the second before the hanging
batch resolves), then resolve the slow promise and assert that the final
store.getAssets(nodeType) does not contain the stale asset from the cancelled
batch; reference updateModelsForNodeType/updateModelsForKey,
assetService.getAssetsForNodeType, and store.getAssets to locate the relevant
test hooks and store logic.
♻️ Duplicate comments (1)
src/stores/assetsStore.ts (1)
335-406: Concurrent refreshes can still corrupt pagination state.If
updateModelsForKeyis called again while a priorloadRemainingBatchesloop is running (e.g., from a download-triggered refresh), both loops mutate the samestate.offset/hasMore, potentially skipping pages or advancing offsets twice.A per-key request generation guard was suggested in a previous review but doesn't appear to be implemented. Consider adding a
requestIdtoModelPaginationStateand checking it at each iteration ofloadRemainingBatches:🔧 Suggested fix (per-key requestId guard)
interface ModelPaginationState { assets: Map<string, AssetItem> offset: number hasMore: boolean isLoading: boolean error?: Error + requestId: number } function createInitialState(): ModelPaginationState { const state: ModelPaginationState = { assets: new Map(), offset: 0, hasMore: true, - isLoading: false + isLoading: false, + requestId: 0 } return shallowReactive(state) } async function updateModelsForKey( key: string, fetcher: (options: PaginationOptions) => Promise<AssetItem[]> ): Promise<AssetItem[]> { const state = getOrCreateState(key) + state.requestId += 1 + const currentRequestId = state.requestId resetPaginationForKey(key) // ... existing code ... if (state.hasMore) { - void loadRemainingBatches(key, fetcher) + void loadRemainingBatches(key, fetcher, currentRequestId) } // ... } async function loadRemainingBatches( key: string, - fetcher: (options: PaginationOptions) => Promise<AssetItem[]> + fetcher: (options: PaginationOptions) => Promise<AssetItem[]>, + requestId: number ): Promise<void> { const state = modelStateByKey.value.get(key) - if (!state) return + if (!state || state.requestId !== requestId) return while (state.hasMore) { + if (state.requestId !== requestId) break // ... fetch logic ... + if (state.requestId !== requestId) break // ... mutation logic ... } }
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/stores/assetsStore.ts`:
- Around line 382-388: state.assets is mutated in-place with
state.assets.set(...) so shallowReactive won't detect changes and components
like the fetchedAssets computed won't update; instead of looping and calling
state.assets.set inside the for loop, build a new Map that merges existing
entries with the assetsToAdd (e.g. const newMap = new Map(state.assets); for
(const a of assetsToAdd) newMap.set(a.id, a);) and then assign state.assets =
newMap after assetsArrayCache.delete(key) so the reassignment triggers
reactivity; update the block that uses assetsToAdd and state.assets accordingly.
♻️ Duplicate comments (1)
src/stores/assetsStore.test.ts (1)
548-551: Reference-equality assertion is brittle.As noted ранее, this couples the test to the current caching implementation. Prefer asserting the updated contents and length instead of
toBeon array references.
DrJKL
force-pushed
the
drjkl/progressive-pagination
branch
from
eb8d043 to
83f5d49
Compare
Comment on lines
209
to
226
| const data = await handleAssetRequest( | ||
| `${ASSETS_ENDPOINT}?include_tags=${MODELS_TAG},${category}&limit=${DEFAULT_LIMIT}`, | ||
| `${ASSETS_ENDPOINT}?${queryParams.toString()}`, | ||
| `assets for ${nodeType}` | ||
| ) |
Contributor
There was a problem hiding this comment.
small nit: should we modify handleAssetRequest signature to accept query params instead of doing string templating here?
Contributor
There was a problem hiding this comment.
Would be good and ideally this is type-checked agains the types that are generated from openapi spec!
Contributor
There was a problem hiding this comment.
openapi spec should be merged sometime today, but i dont think that should block
Contributor
Author
There was a problem hiding this comment.
Good call, updated. The interface there is a good target to swap out once we have the spec types.
src/stores/assetsStore.ts
Outdated
| state.hasMore = assets.length === MODEL_BATCH_SIZE | ||
|
|
||
| if (state.hasMore) { | ||
| void loadRemainingBatches(key, fetcher, state) |
Contributor
There was a problem hiding this comment.
instead of duplicating fetching and setting logic in this new function, can we have one while loop doing all the work?
luke-mino-altherr
previously approved these changes
Jan 21, 2026
Contributor
luke-mino-altherr
left a comment
luke-mino-altherr
left a comment
There was a problem hiding this comment.
some nits, otherwise looks good
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/stores/assetsStore.ts`:
- Around line 387-396: On first-batch failure you must remove the orphaned
request from pendingRequestByKey and make the error visible to the UI: inside
the catch block in the batch loader (where you currently set state.error and
return) call pendingRequestByKey.delete(key) before returning, and then
propagate the error into the persisted model state so getError can see it (for
example, update modelStateByKey.get(key).error = state.error or create/update
the entry in modelStateByKey with the same error/hasMore/isLoading values). This
prevents the memory leak and ensures UI error visibility; use the existing
symbols pendingRequestByKey, state, key, and modelStateByKey to locate and
implement the change.
- Move Pinia setup into beforeEach lifecycle hooks - Replace createPinia with createTestingPinia - Remove inline setActivePinia calls from individual tests
- Replace reference equality check with behavioral assertions - Add dedicated test for getAssets caching semantics
- Add ?? [] fallback for getAssets in computed and watch handler - Use shared EMPTY_ASSETS constant for stable cache reference Amp-Thread-ID: https://ampcode.com/threads/T-019be204-2bc0-702f-904c-2f22f51b2280 Co-authored-by: Amp <amp@ampcode.com>
…stOptions interface Amp-Thread-ID: https://ampcode.com/threads/T-019be24c-6847-734f-9e61-a49ea791d836 Co-authored-by: Amp <amp@ampcode.com>
- Merge updateModelsForKey and loadRemainingBatches into one loadBatches function - Change return type to void since callers use getAssets() instead - Address PR review feedback Amp-Thread-ID: https://ampcode.com/threads/T-019be255-b4d6-73ed-a9dd-62b92b0af1c4 Co-authored-by: Amp <amp@ampcode.com>
- Remove export from AssetRequestOptions (only used internally) - Move assetsArrayCache.delete() inside loadBatches after first batch fetch Amp-Thread-ID: https://ampcode.com/threads/T-019be270-a632-728b-843d-270d1971593e Co-authored-by: Amp <amp@ampcode.com>
- Track pending requests separately from committed state - Only replace modelStateByKey after first batch succeeds - Update isStale() to check both committed and pending state Amp-Thread-ID: https://ampcode.com/threads/T-019be270-a632-728b-843d-270d1971593e Co-authored-by: Amp <amp@ampcode.com>
- If no existing data, commit new state right away for loading indicator - If existing data, keep it visible until first batch succeeds Amp-Thread-ID: https://ampcode.com/threads/T-019be270-a632-728b-843d-270d1971593e Co-authored-by: Amp <amp@ampcode.com>
The while loop handles all batch iterations; recursive call was redundant. Amp-Thread-ID: https://ampcode.com/threads/T-019be286-74b8-72c3-b177-07cfb7e14476 Co-authored-by: Amp <amp@ampcode.com>
…rder Amp-Thread-ID: https://ampcode.com/threads/T-019be289-70f2-74c0-aa43-6a13d5ff7523 Co-authored-by: Amp <amp@ampcode.com>
Prevents memory leak by deleting orphaned request from pendingRequestByKey when first batch fails to load. Amp-Thread-ID: https://ampcode.com/threads/T-019be291-311a-7339-b764-d7ded3487894 Co-authored-by: Amp <amp@ampcode.com>
DrJKL
force-pushed
the
drjkl/progressive-pagination
branch
from
0fdc73e to
c281278
Compare
viva-jinyi
approved these changes
Jan 22, 2026
| import { useModelToNodeStore } from '@/stores/modelToNodeStore' | ||
|
|
||
| export interface PaginationOptions { | ||
| limit?: number |
Contributor
Author
There was a problem hiding this comment.
There are defaults set below, and the API itself doesn't require them.
| } | ||
|
|
||
| interface AssetRequestOptions extends PaginationOptions { | ||
| includeTags: string[] |
Member
There was a problem hiding this comment.
it could be optional, isnt it?
Contributor
Author
There was a problem hiding this comment.
This one is always set. Not sure if it's required by the API. Once we get the spec synced we'll know that we are aligned with the backend expectations.
DrJKL
added
needs-backport
|
1 similar comment
|
| limit = DEFAULT_LIMIT, | ||
| offset = 0 | ||
| }: { limit?: number; offset?: number } = {} | ||
| { limit = DEFAULT_LIMIT, offset = 0 }: PaginationOptions = {} |
Contributor
There was a problem hiding this comment.
I suppose we won't have the type generation in time for this PR? related comment above
Contributor
Author
There was a problem hiding this comment.
Not without some form of time travel
DrJKL
added a commit
that referenced
this pull request
Jan 22, 2026
…#8212) Implements progressive pagination for model assets - returns the first batch immediately while loading remaining batches in the background. - Adds `ModelPaginationState` tracking (assets Map, offset, hasMore, loading, error) - `updateModelsForKey()` returns first batch, then calls `loadRemainingBatches()` to fetch the rest - Accessor functions `getAssets(key)`, `isModelLoading(key)` replace direct Map access - Adds `PaginationOptions` interface (`{ limit?, offset? }`) - `AssetBrowserModal.vue` uses new accessor API - Updated mocks for new accessor pattern ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8212-feat-implement-progressive-pagination-for-Asset-Browser-model-assets-2ef6d73d36508157af04d1264780997e) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <amp@ampcode.com>
DrJKL
added a commit
that referenced
this pull request
Jan 22, 2026
…#8212) Implements progressive pagination for model assets - returns the first batch immediately while loading remaining batches in the background. - Adds `ModelPaginationState` tracking (assets Map, offset, hasMore, loading, error) - `updateModelsForKey()` returns first batch, then calls `loadRemainingBatches()` to fetch the rest - Accessor functions `getAssets(key)`, `isModelLoading(key)` replace direct Map access - Adds `PaginationOptions` interface (`{ limit?, offset? }`) - `AssetBrowserModal.vue` uses new accessor API - Updated mocks for new accessor pattern ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8212-feat-implement-progressive-pagination-for-Asset-Browser-model-assets-2ef6d73d36508157af04d1264780997e) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <amp@ampcode.com>
DrJKL
added a commit
that referenced
this pull request
Jan 22, 2026
…t Browser model assets (#8240) Backport of #8212 to cloud/1.37 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8240-backport-cloud-1-37-feat-implement-progressive-pagination-for-Asset-Browser-model-asse-2f06d73d365081b199a0dd6bcc242bba) by [Unito](https://www.unito.io) Co-authored-by: Amp <amp@ampcode.com>
godwiniheuwa
pushed a commit
to godwiniheuwa/ComfyUI_frontend
that referenced
this pull request
Jan 22, 2026
…Comfy-Org#8212) ## Summary Implements progressive pagination for model assets - returns the first batch immediately while loading remaining batches in the background. ## Changes ### Store (`assetsStore.ts`) - Adds `ModelPaginationState` tracking (assets Map, offset, hasMore, loading, error) - `updateModelsForKey()` returns first batch, then calls `loadRemainingBatches()` to fetch the rest - Accessor functions `getAssets(key)`, `isModelLoading(key)` replace direct Map access ### API (`assetService.ts`) - Adds `PaginationOptions` interface (`{ limit?, offset? }`) ### Components - `AssetBrowserModal.vue` uses new accessor API ### Tests - Updated mocks for new accessor pattern ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8212-feat-implement-progressive-pagination-for-Asset-Browser-model-assets-2ef6d73d36508157af04d1264780997e) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <amp@ampcode.com>
This was referenced Jan 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements progressive pagination for model assets - returns the first batch immediately while loading remaining batches in the background.
Changes
Store (
assetsStore.ts)ModelPaginationStatetracking (assets Map, offset, hasMore, loading, error)updateModelsForKey()returns first batch, then callsloadRemainingBatches()to fetch the restgetAssets(key),isModelLoading(key)replace direct Map accessAPI (
assetService.ts)PaginationOptionsinterface ({ limit?, offset? })Components
AssetBrowserModal.vueuses new accessor APITests
┆Issue is synchronized with this Notion page by Unito