-
Notifications
You must be signed in to change notification settings - Fork 447
load assets browser before fetch completes and show loading state #6189
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.
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
Changes from all commits
feadc7f
7bd66d2
f212a0b
d6e9bb9
e0ad537
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import { computed, ref } from 'vue' | ||
| import type { Ref } from 'vue' | ||
|
|
||
| import { d, t } from '@/i18n' | ||
| import type { FilterState } from '@/platform/assets/components/AssetFilterBar.vue' | ||
|
|
@@ -65,7 +66,10 @@ export interface AssetDisplayItem extends AssetItem { | |
| * Asset Browser composable | ||
| * Manages search, filtering, asset transformation and selection logic | ||
| */ | ||
| export function useAssetBrowser(assets: AssetItem[] = []) { | ||
| export function useAssetBrowser( | ||
| assetsSource: Ref<AssetItem[] | undefined> = ref<AssetItem[] | undefined>([]) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bit of a nit, but I like to make composables more flexible by using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
YAGNI |
||
| ) { | ||
| const assets = computed<AssetItem[]>(() => assetsSource.value ?? []) | ||
| // State | ||
| const searchQuery = ref('') | ||
| const selectedCategory = ref('all') | ||
|
|
@@ -116,9 +120,10 @@ export function useAssetBrowser(assets: AssetItem[] = []) { | |
| } | ||
|
|
||
| const availableCategories = computed(() => { | ||
| const categories = assets | ||
| .filter((asset) => asset.tags[0] === 'models' && asset.tags[1]) | ||
| const categories = assets.value | ||
| .filter((asset) => asset.tags[0] === 'models') | ||
| .map((asset) => asset.tags[1]) | ||
| .filter((tag): tag is string => typeof tag === 'string' && tag.length > 0) | ||
|
|
||
| const uniqueCategories = Array.from(new Set(categories)) | ||
| .sort() | ||
|
|
@@ -152,7 +157,7 @@ export function useAssetBrowser(assets: AssetItem[] = []) { | |
|
|
||
| // Category-filtered assets for filter options (before search/format/base model filters) | ||
| const categoryFilteredAssets = computed(() => { | ||
| return assets.filter(filterByCategory(selectedCategory.value)) | ||
| return assets.value.filter(filterByCategory(selectedCategory.value)) | ||
| }) | ||
|
|
||
| const filteredAssets = computed(() => { | ||
|
|
@@ -161,8 +166,8 @@ export function useAssetBrowser(assets: AssetItem[] = []) { | |
| .filter(filterByFileFormats(filters.value.fileFormats)) | ||
| .filter(filterByBaseModels(filters.value.baseModels)) | ||
|
|
||
| // Sort assets | ||
| filtered.sort((a, b) => { | ||
| const sortedAssets = [...filtered] | ||
| sortedAssets.sort((a, b) => { | ||
| switch (filters.value.sortBy) { | ||
| case 'name-desc': | ||
| return b.name.localeCompare(a.name) | ||
|
|
@@ -179,7 +184,7 @@ export function useAssetBrowser(assets: AssetItem[] = []) { | |
| }) | ||
|
|
||
| // Transform to display format | ||
| return filtered.map(transformAssetForDisplay) | ||
| return sortedAssets.map(transformAssetForDisplay) | ||
| }) | ||
|
|
||
| function updateFilters(newFilters: FilterState) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,9 +4,9 @@ import { z } from 'zod' | |
| const zAsset = z.object({ | ||
| id: z.string(), | ||
| name: z.string(), | ||
| asset_hash: z.string().optional(), | ||
| asset_hash: z.string().nullish(), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing something from optional to required but nullable can be risky.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the OSS, it returns null, on the cloud, it's undefined. Thanks for flagging this out - cc @arjansingh |
||
| size: z.number(), | ||
| mime_type: z.string().optional(), | ||
| mime_type: z.string().nullish(), | ||
| tags: z.array(z.string()).optional().default([]), | ||
| preview_id: z.string().nullable().optional(), | ||
| preview_url: z.string().optional(), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| const ACRONYM_TAGS = new Set(['VAE', 'CLIP', 'GLIGEN']) | ||
|
|
||
| export function formatCategoryLabel(raw?: string): string { | ||
| if (!raw) return 'Models' | ||
|
|
||
| return raw | ||
| .split('_') | ||
| .map((segment) => { | ||
| const upper = segment.toUpperCase() | ||
| if (ACRONYM_TAGS.has(upper)) return upper | ||
|
|
||
| const lower = segment.toLowerCase() | ||
| return lower.charAt(0).toUpperCase() + lower.slice(1) | ||
| }) | ||
| .join(' ') | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: instead of
{ immediate: true }I think could you do something like this to make the lifecycle more explicit:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you @DrJKL!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! After merging with main, this code has been superseded. The component now accepts
assetsas a prop instead of fetching internally, so theuseAsyncState+watch({immediate: true})pattern is no longer present.For context on the original implementation:
watch({immediate: true})runs during component setup (before DOM mount), whileonMounted()runs after mount. For data fetching that doesn't need DOM access, starting earlier provides better perceived performance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude got slightly confused on this one.