Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions packages/design-system/src/css/style.css
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@
--node-stroke-executing: var(--color-blue-100);
--text-secondary: var(--color-stone-100);
--text-primary: var(--color-charcoal-700);
--input-surface: rgba(0, 0, 0, 0.15);
--input-surface: rgb(0 0 0 / 0.15);
}

.dark-theme {
Expand Down Expand Up @@ -247,7 +247,7 @@
--node-stroke-executing: var(--color-blue-100);
--text-secondary: var(--color-slate-100);
--text-primary: var(--color-pure-white);
--input-surface: rgba(130, 130, 130, 0.1);
--input-surface: rgb(130 130 130 / 0.1);
}

@theme inline {
Expand Down Expand Up @@ -1174,7 +1174,7 @@ audio.comfy-audio.empty-audio-widget {
}

.isLOD .lg-node-header {
border-radius: 0px;
border-radius: 0;
pointer-events: none;
}

Expand Down
73 changes: 68 additions & 5 deletions src/platform/assets/components/AssetBrowserModal.vue
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,17 @@
<template #content>
<AssetGrid
:assets="filteredAssets"
:loading="isLoading"
@asset-select="handleAssetSelectAndEmit"
/>
</template>
</BaseModalLayout>
</template>

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

import SearchBox from '@/components/input/SearchBox.vue'
import BaseModalLayout from '@/components/widget/layout/BaseModalLayout.vue'
Expand All @@ -57,6 +60,9 @@ import AssetGrid from '@/platform/assets/components/AssetGrid.vue'
import type { AssetDisplayItem } from '@/platform/assets/composables/useAssetBrowser'
import { useAssetBrowser } from '@/platform/assets/composables/useAssetBrowser'
import type { AssetItem } from '@/platform/assets/schemas/assetSchema'
import { assetService } from '@/platform/assets/services/assetService'
import { formatCategoryLabel } from '@/platform/assets/utils/categoryLabel'
import { useModelToNodeStore } from '@/stores/modelToNodeStore'
import { OnCloseKey } from '@/types/widgetTypes'

const props = defineProps<{
Expand All @@ -65,29 +71,86 @@ const props = defineProps<{
onSelect?: (asset: AssetItem) => void
onClose?: () => void
showLeftPanel?: boolean
assets?: AssetItem[]
title?: string
assetType?: string
}>()

const { t } = useI18n()

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

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

const fetchAssets = async () => {
if (props.nodeType) {
return (await assetService.getAssetsForNodeType(props.nodeType)) ?? []
}

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

return []
}

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

watch(
() => [props.nodeType, props.assetType],
async () => {
await execute()
},
{ immediate: true }
)
Comment on lines +105 to +111
Copy link
Contributor

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:

onMount(() => execute())

watch(() => [props.nodeType, props.assetType], () => execute())

Copy link
Contributor

Choose a reason for hiding this comment

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

I see you @DrJKL!

Copy link
Contributor

Choose a reason for hiding this comment

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

😜

Copy link
Contributor Author

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 assets as a prop instead of fetching internally, so the useAsyncState + watch({immediate: true}) pattern is no longer present.

For context on the original implementation: watch({immediate: true}) runs during component setup (before DOM mount), while onMounted() runs after mount. For data fetching that doesn't need DOM access, starting earlier provides better perceived performance.

Copy link
Contributor

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.

Copy link
Contributor Author

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.


const {
searchQuery,
selectedCategory,
availableCategories,
contentTitle,
categoryFilteredAssets,
filteredAssets,
updateFilters
} = useAssetBrowser(props.assets)
} = useAssetBrowser(fetchedAssets)

const modelToNodeStore = useModelToNodeStore()

const primaryCategoryTag = computed(() => {
const assets = fetchedAssets.value ?? []
const tagFromAssets = assets
.map((asset) => asset.tags?.find((tag) => tag !== 'models'))
.find((tag): tag is string => typeof tag === 'string' && tag.length > 0)

if (tagFromAssets) return tagFromAssets

if (props.nodeType) {
const mapped = modelToNodeStore.getCategoryForNodeType(props.nodeType)
if (mapped) return mapped
}

if (props.assetType) return props.assetType

return 'models'
})

const activeCategoryTag = computed(() => {
if (selectedCategory.value !== 'all') {
return selectedCategory.value
}
return primaryCategoryTag.value
})

const displayTitle = computed(() => {
return props.title ?? contentTitle.value
if (props.title) return props.title

const label = formatCategoryLabel(activeCategoryTag.value)
return t('assetBrowser.allCategory', { category: label })
})

const shouldShowLeftPanel = computed(() => {
Expand Down
4 changes: 2 additions & 2 deletions src/platform/assets/components/AssetGrid.vue
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,12 @@
<!-- Loading state -->
<div
v-if="loading"
class="col-span-full flex items-center justify-center py-16"
class="col-span-full flex items-center justify-center py-20"
>
<i
class="icon-[lucide--loader]"
:class="
cn('size-6 animate-spin', 'text-stone-300 dark-theme:text-stone-200')
cn('size-12 animate-spin', 'text-stone-300 dark-theme:text-stone-200')
"
/>
</div>
Expand Down
19 changes: 12 additions & 7 deletions src/platform/assets/composables/useAssetBrowser.ts
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'
Expand Down Expand Up @@ -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>([])
Copy link
Contributor

Choose a reason for hiding this comment

The 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 MaybeRefOrGetter and toValue.
It'd also have saved you from having to update all the tests that were passing in pure values instead of refs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make composables more flexible by using MaybeRefOrGetter and toValue

YAGNI

) {
const assets = computed<AssetItem[]>(() => assetsSource.value ?? [])
// State
const searchQuery = ref('')
const selectedCategory = ref('all')
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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(() => {
Expand All @@ -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)
Expand All @@ -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) {
Expand Down
46 changes: 1 addition & 45 deletions src/platform/assets/composables/useAssetBrowserDialog.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import { t } from '@/i18n'
import AssetBrowserModal from '@/platform/assets/components/AssetBrowserModal.vue'
import type { AssetItem } from '@/platform/assets/schemas/assetSchema'
import { assetService } from '@/platform/assets/services/assetService'
import { useDialogStore } from '@/stores/dialogStore'
import type { DialogComponentProps } from '@/stores/dialogStore'

Expand Down Expand Up @@ -51,42 +49,13 @@ export const useAssetBrowserDialog = () => {
dialogStore.closeDialog({ key: dialogKey })
}

const assets: AssetItem[] = await assetService
.getAssetsForNodeType(props.nodeType)
.catch((error) => {
console.error(
'Failed to fetch assets for node type:',
props.nodeType,
error
)
return []
})

// Extract node type category from first asset's tags (e.g., "loras", "checkpoints")
// Tags are ordered: ["models", "loras"] so take the second tag
const nodeTypeCategory =
assets[0]?.tags?.find((tag) => tag !== 'models') ?? 'models'

const acronyms = new Set(['VAE', 'CLIP', 'GLIGEN'])
const categoryLabel = nodeTypeCategory
.split('_')
.map((word) => {
const uc = word.toUpperCase()
return acronyms.has(uc) ? uc : word
})
.join(' ')

const title = t('assetBrowser.allCategory', { category: categoryLabel })

dialogStore.showDialog({
key: dialogKey,
component: AssetBrowserModal,
props: {
nodeType: props.nodeType,
inputName: props.inputName,
currentValue: props.currentValue,
assets,
title,
onSelect: handleAssetSelected,
onClose: () => dialogStore.closeDialog({ key: dialogKey })
},
Expand All @@ -100,25 +69,12 @@ export const useAssetBrowserDialog = () => {
dialogStore.closeDialog({ key: dialogKey })
}

const assets = await assetService
.getAssetsByTag(options.assetType)
.catch((error) => {
console.error(
'Failed to fetch assets for tag:',
options.assetType,
error
)
return []
})

dialogStore.showDialog({
key: dialogKey,
component: AssetBrowserModal,
props: {
nodeType: undefined,
inputName: undefined,
assets,
showLeftPanel: true,
assetType: options.assetType,
title: options.title,
onSelect: handleAssetSelected,
onClose: () => dialogStore.closeDialog({ key: dialogKey })
Expand Down
4 changes: 2 additions & 2 deletions src/platform/assets/schemas/assetSchema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing something from optional to required but nullable can be risky.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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(),
Expand Down
16 changes: 16 additions & 0 deletions src/platform/assets/utils/categoryLabel.ts
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(' ')
}
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ onUnmounted(() => {

<style scoped>
.audio-player-menu {
--p-tieredmenu-item-focus-background: rgba(255, 255, 255, 0.1);
--p-tieredmenu-item-active-background: rgba(255, 255, 255, 0.1);
--p-tieredmenu-item-focus-background: rgb(255 255 255 / 0.1);
--p-tieredmenu-item-active-background: rgb(255 255 255 / 0.1);
}
</style>
Loading