Asset Browser Modal Component #5607

arjansingh · 2025-09-16T08:27:27Z

Summary

These are all the components and updated api logic for our Asset Browser. Next step is wiring up the flow end to end!

Changes

Modal Components + Storybook tests.
Related Compostables.

Not in this PR:

Hooking up to asset widget + running the workflow (next PR).
Pagination of assets + useCacheRequest work (PR after that).

Review Focus

Architecture. Did I Vue it right?

Screenshots (if applicable)

You can download this branch and also run the storybook stories

┆Issue is synchronized with this Notion page by Unito

github-actions · 2025-09-16T08:27:50Z

🎭 Playwright Test Results

✅ All tests passed!

⏰ Completed at: 09/17/2025, 06:44:07 PM UTC

📈 Summary

Total Tests: 450
Passed: 421 ✅
Failed: 0
Flaky: 0
Skipped: 29 ⏭️

📊 Test Reports by Browser

✅ chromium: View Report • ✅ 414 / ❌ 0 / ⚠️ 0 / ⏭️ 29
✅ chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
✅ chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
✅ mobile-chrome: View Report • ✅ 4 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

.storybook/main.ts

.storybook/preview.ts

src/platform/assets/components/AssetBadgeGroup.vue

src/platform/assets/components/AssetBrowserModal.vue

src/platform/assets/fixtures/ui-mock-assets.ts

tests-ui/platform/assets/components/AssetBrowserModal.test.ts

tests-ui/platform/assets/composables/useAssetBrowserDialog.test.ts

arjansingh · 2025-09-17T00:31:35Z

.storybook/main.ts

      // Replace plugins entirely to avoid inheritance issues
      plugins: [
        // Only include plugins we explicitly need for Storybook
+        tailwindcss(),


the filtering above was filtering out tailwind apparently.

And all related sub components

DrJKL · 2025-09-17T01:17:22Z

tailwind.config.ts

+  safelist: [
+    'icon-[lucide--folder]',
+    'icon-[lucide--package]',
+    'icon-[lucide--image]',
+    'icon-[lucide--video]',
+    'icon-[lucide--box]',
+    'icon-[lucide--audio-waveform]',
+    'icon-[lucide--message-circle]'
+  ],
+


I don't think you'll need this if you use the pattern in #5453

You're right about the iconify plugin from PR #5453, but that approach only works for static template usage like:

<i class="icon-[lucide--folder]">

The safelist is still needed for dynamic usage where icons are assigned as strings in JavaScript objects:

{ icon: 'icon-[lucide--folder]' } // Tailwind can't detect this

We have 3 icons used this way in useAssetBrowser.ts (we have to map them to api categories) and menu composables. However, 4 icons in the safelist are unused and can be removed.

The iconify plugin can't solve the dynamic string assignment problem. Tha'ts why we need the safelist.

claude · 2025-09-17T01:47:17Z

src/platform/assets/composables/useAssetBrowser.ts

+  // Compute content title from selected category
+  const contentTitle = computed(() => {
+    if (selectedCategory.value === 'all') {
+      return 'All Models'


[quality] medium Priority

Issue: Hardcoded strings 'All Models' and 'Assets' are not internationalized
Context: Project guidelines require all user-facing strings use vue-i18n for consistency and localization
Suggestion: Replace with i18n keys like $t('assetBrowser.allModels') and $t('assetBrowser.assets')

This one might be harder to catch in a linter

claude · 2025-09-17T01:47:24Z

src/platform/assets/composables/useAssetBrowser.ts

+    // Extract description from metadata or create from tags
+    const typeTag = asset.tags.find((tag) => tag !== 'models')
+    const description =
+      asset.user_metadata?.description || `${typeTag || 'Unknown'} model`


[quality] medium Priority

Issue: Hardcoded 'Unknown' string in template literal is not internationalized
Context: Project guidelines require all user-facing strings use vue-i18n
Suggestion: Replace with $t('assetBrowser.unknownModel') or similar i18n key

src/platform/assets/components/AssetCard.vue

claude · 2025-09-17T01:47:44Z

src/platform/assets/components/AssetCard.vue

+    "
+    @click="interactive && $emit('select', asset)"
+    @keydown.enter="interactive && $emit('select', asset)"
+    @keydown.space.prevent="interactive && $emit('select', asset)"


[quality] medium Priority

Issue: Keyboard space handler uses prevent modifier which may interfere with screen reader functionality
Context: For button elements, space key should use natural button behavior rather than preventing default
Suggestion: Remove .prevent from space handler or ensure proper button semantics are maintained for accessibility

claude · 2025-09-17T01:47:51Z

src/platform/assets/schemas/assetSchema.ts

+  created_at: z.string(),
+  updated_at: z.string(),
+  last_access_time: z.string(),
+  user_metadata: z.record(z.any()).optional(),


[quality] low Priority

Issue: z.any() usage in user_metadata field reduces type safety
Context: z.any() defeats the purpose of schema validation and TypeScript type checking
Suggestion: Consider using z.record(z.string()) or a more specific schema for user_metadata to maintain type safety

Ironically this isn't low, this is probably the most important one.

claude · 2025-09-17T01:47:59Z

src/platform/assets/composables/useAssetBrowser.ts

+
+    // Create display stats from API data
+    const stats = {
+      formattedDate: new Date(asset.created_at).toLocaleDateString(),


[architecture] medium Priority

Issue: Date parsing with new Date() lacks error handling for invalid date strings
Context: If asset.created_at has invalid date format, this will create Invalid Date objects leading to 'Invalid Date' display
Suggestion: Add date validation or fallback: new Date(asset.created_at).toLocaleDateString() || 'Unknown date'

Quick warning, the suggested fix would not behave as described

Our i18n library has date localization as well

claude

Comprehensive PR Review

This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.

Review Summary

PR: Asset Browser Modal Component (#5607)
Impact: 2518 additions, 61 deletions across 25 files

Issue Distribution

Critical: 0
High: 1
Medium: 6
Low: 1

Category Breakdown

Architecture: 1 issue
Security: 0 issues
Performance: 1 issue
Code Quality: 6 issues

Key Findings

Architecture & Design

The asset browser implementation follows Vue 3 Composition API patterns well and maintains proper separation of concerns between components and composables. The modal system integration is architected correctly using the dialog store. However, there's a missing error handling pattern for date parsing that could lead to poor user experience.

Security Considerations

No critical security issues identified. The implementation properly uses TypeScript interfaces and schema validation with Zod, though the use of z.any() reduces type safety marginally.

Performance Impact

Asset transformation occurs on every filter operation which could be expensive for large lists. Consider memoizing the transformation results to avoid redundant processing. Bundle size impact appears minimal with good component chunking.

Integration Points

Good integration with existing UI components (SearchBox, LeftSidePanel, BaseModalLayout). Proper use of PrimeVue components and following established patterns. The composable architecture allows for good reusability.

Positive Observations

Excellent use of Vue 3 Composition API patterns
Good component decomposition and reusability
Proper TypeScript typing throughout
Comprehensive test coverage including Storybook stories
Good use of existing UI components rather than reinventing
Proper Tailwind utility usage with dark mode support
Clean modal dialog integration

Critical Issues to Address

Class merging patterns: The AssetCard component uses array-based class merging which violates project guidelines. Must use cn() utility instead.

References

Next Steps

Address the high-priority class merging issue before merge
Consider internationalization for all hardcoded strings
Add error handling for date parsing
Consider performance optimization for large asset lists
Add tests for error scenarios

This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.

arjansingh · 2025-09-17T02:54:22Z

@DrJKL You're absolutely correct about the cn() usage in AssetCard.vue:7. The code is properly using cn() with the correct pattern. The automated review may have been looking at an earlier commit or different section.

Regarding the tailwind config safelist comment: You're right that with the iconify plugin from PR #5453, those hardcoded icon names in the safelist might not be necessary since the plugin handles dynamic icon selection. Should I remove those from the safelist?

viva-jinyi · 2025-09-17T04:35:57Z

src/platform/assets/components/AssetCard.vue

+    :class="
+      cn(
+        // Base layout and container styles (always applied)
+        'rounded-xl overflow-hidden transition-all duration-200 min-w-60 max-w-64',


The min-width and max-width CSS on the button should be removed. It’s better to let the width adjust dynamically based on the browser size, with the parent component controlling the layout using grid-template. Removing those two properties makes the screen look like this.

Before

After

Before

After

As a side note, if the card looks too wide, you can set a smaller minWidth in createGridStyle to balance it out.

i will remove the min/max for now and if we run into issues talking to design on the size of hte cards we'll adjust minWidth in createGridStyle

DrJKL · 2025-09-17T18:23:31Z

src/platform/assets/components/AssetBrowserModal.stories.ts

+      }
+    },
+    template: `
+      <div class="flex items-center justify-center min-h-screen bg-stone-200 dark-theme:bg-stone-200 p-4">


Suggested change

<div class="flex items-center justify-center min-h-screen bg-stone-200 dark-theme:bg-stone-200 p-4">

<div class="flex items-center justify-center min-h-screen bg-stone-200 p-4">

DrJKL · 2025-09-17T18:23:44Z

src/platform/assets/components/AssetBrowserModal.stories.ts

+      return { ...args, onAssetSelect, onClose, assets: singleTypeAssets }
+    },
+    template: `
+      <div class="flex items-center justify-center min-h-screen bg-stone-200 dark-theme:bg-stone-200 p-4">


Suggested change

<div class="flex items-center justify-center min-h-screen bg-stone-200 dark-theme:bg-stone-200 p-4">

<div class="flex items-center justify-center min-h-screen bg-stone-200 p-4">

Here and elsewhere. Don't want to confuse the robots with this pattern.

DrJKL · 2025-09-17T18:25:40Z

src/platform/assets/components/AssetFilterBar.vue

+        @update:model-value="handleFilterChange"
+      >
+        <template #icon>
+          <i-lucide:arrow-up-down class="size-3" />


We should be able to add a lint rule to catch these.

DrJKL · 2025-09-17T18:27:08Z

src/platform/assets/composables/useAssetBrowser.ts

+      {
+        id: 'all',
+        label: t('assetBrowser.allModels'),
+        icon: 'icon-[lucide--folder]'


Does this cause them to be picked up by Tailwind?

Suggested change

icon: 'icon-[lucide--folder]'

icon: cn('icon-[lucide--folder]')

DrJKL · 2025-09-17T18:27:41Z

src/platform/assets/composables/useAssetBrowser.ts

+        .sort()
+        .map((category) => ({
+          id: category,
+          label: category.charAt(0).toUpperCase() + category.slice(1),


This would also probably be an i18n issue.

christian-byrne

LGTM!

christian-byrne · 2025-09-17T23:14:40Z

src/platform/assets/components/AssetFilterBar.vue

+        label="File formats"
+        :options="fileFormatOptions"
+        :class="selectClasses"
+        data-component-id="asset-filter-file-formats"
+        @update:model-value="handleFilterChange"
+      />
+
+      <MultiSelect
+        v-model="baseModels"
+        label="Base models"


Are the labels translated?

Another good catch. I will make sure that happens.

* [ci] ignore playwright mcp directory * [feat] add AssetBrowserModal And all related sub components * [feat] reactive filter functions * [ci] clean up storybook config * [feat] add sematic AssetCard * [fix] i love lucide * [fix] AssetCard layout issues * [fix] add AssetBadge type * [fix] simplify useAssetBrowser * [fix] modal layout * [fix] simplify useAssetBrowserDialog * [fix] add tailwind back to storybook * [fix] better reponsive layout * [fix] missed i18n string * [fix] missing i18n translations * [fix] remove erroneous prevent on keyboard.space * [feat] add asset metadata validation utilities * [fix] remove erroneous test code * [fix] remove forced min and max width on AssetCard * [fix] import statement nits

DrJKL reviewed Sep 16, 2025

View reviewed changes

arjansingh force-pushed the feat/asset-browser-modal-wiring branch from f345a2d to 8ef528b Compare September 16, 2025 22:09

arjansingh commented Sep 17, 2025

View reviewed changes

arjansingh marked this pull request as ready for review September 17, 2025 00:35

arjansingh requested review from a team, KarryCharon, Yorha4D and shinshin86 as code owners September 17, 2025 00:35

arjansingh added 12 commits September 16, 2025 17:35

[ci] ignore playwright mcp directory

294bba0

[feat] add AssetBrowserModal

e8b9693

And all related sub components

[feat] reactive filter functions

534ae71

[ci] clean up storybook config

d36999b

[feat] add sematic AssetCard

8165ec3

[fix] i love lucide

e8bc614

[fix] AssetCard layout issues

2806aa1

[fix] add AssetBadge type

5e761d4

[fix] simplify useAssetBrowser

c1b5e00

[fix] modal layout

f7fe476

[fix] simplify useAssetBrowserDialog

143f9e1

[fix] add tailwind back to storybook

81a0046

dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Sep 17, 2025

arjansingh force-pushed the feat/asset-browser-modal-wiring branch from b395af9 to 81a0046 Compare September 17, 2025 00:35

arjansingh requested review from christian-byrne and viva-jinyi September 17, 2025 00:36

arjansingh changed the title ~~DRAFT - Asset Browser Modal Component and API Data Logic~~ Asset Browser Modal Component Sep 17, 2025

DrJKL assigned DrJKL and unassigned DrJKL Sep 17, 2025

DrJKL reviewed Sep 17, 2025

View reviewed changes

claude bot reviewed Sep 17, 2025

View reviewed changes

src/platform/assets/components/AssetCard.vue Show resolved Hide resolved

claude bot reviewed Sep 17, 2025

View reviewed changes

arjansingh dismissed DrJKL’s stale review via 5499489 September 17, 2025 02:50

[fix] better reponsive layout

de5bb01

arjansingh force-pushed the feat/asset-browser-modal-wiring branch from 5499489 to de5bb01 Compare September 17, 2025 02:50

arjansingh added 4 commits September 16, 2025 20:02

[fix] missed i18n string

d7a9f59

[fix] missing i18n translations

878b2ed

[fix] remove erroneous prevent on keyboard.space

4f26b3f

[feat] add asset metadata validation utilities

5243ef3

christian-byrne assigned viva-jinyi and DrJKL Sep 17, 2025

viva-jinyi reviewed Sep 17, 2025

View reviewed changes

arjansingh added 3 commits September 17, 2025 11:03

[fix] remove erroneous test code

c896a4e

[fix] remove forced min and max width on AssetCard

a969147

[fix] import statement nits

cb82c87

DrJKL previously approved these changes Sep 17, 2025

View reviewed changes

DrJKL removed their assignment Sep 17, 2025

arjansingh dismissed DrJKL’s stale review via cb82c87 September 17, 2025 18:31

arjansingh enabled auto-merge (squash) September 17, 2025 18:44

DrJKL approved these changes Sep 17, 2025

View reviewed changes

christian-byrne approved these changes Sep 17, 2025

View reviewed changes

arjansingh merged commit dfa1cbb into main Sep 17, 2025
21 checks passed

arjansingh deleted the feat/asset-browser-modal-wiring branch September 17, 2025 23:17

	<div class="flex items-center justify-center min-h-screen bg-stone-200 dark-theme:bg-stone-200 p-4">
	<div class="flex items-center justify-center min-h-screen bg-stone-200 p-4">

	icon: 'icon-[lucide--folder]'
	icon: cn('icon-[lucide--folder]')

Asset Browser Modal Component #5607

Asset Browser Modal Component #5607

Uh oh!

Conversation

arjansingh commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Review Focus

Screenshots (if applicable)

Uh oh!

github-actions bot commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎭 Playwright Test Results

📈 Summary

📊 Test Reports by Browser

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude bot Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude bot Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

claude bot Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude bot Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Comprehensive PR Review

Review Summary

Issue Distribution

Category Breakdown

Key Findings

Architecture & Design

Security Considerations

Performance Impact

Integration Points

Positive Observations

Critical Issues to Address

References

Next Steps

Uh oh!

arjansingh commented Sep 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

arjansingh commented Sep 16, 2025 •

edited

Loading

github-actions bot commented Sep 16, 2025 •

edited

Loading