feat(assets): add ModelInfoPanel for asset browser right panel #8090
Conversation
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
Contributor
📝 WalkthroughWalkthroughAdds a Reka-based Select component suite and stories; introduces ModelInfoPanel with tests and editable metadata and optimistic store updates; expands asset metadata utilities/types and store optimistic updates; integrates a right-side panel with focus/blur flows; various UI, layout, i18n, and test timing adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Modal as AssetBrowserModal
participant Grid as AssetGrid
participant Card as AssetCard
participant Panel as ModelInfoPanel
participant Store as assetsStore
User->>Modal: Open asset browser
Modal->>Store: fetchModelTypes()
Store-->>Modal: model types ready
User->>Card: Click asset
Card->>Grid: emit assetFocus(asset)
Grid->>Modal: emit assetFocus(asset)
Modal->>Modal: set focusedAsset, open right panel
Modal->>Panel: provide focused asset
User->>Panel: Edit display name/tags/description
Panel->>Store: updateAssetMetadata (optimistic)
Store-->>Panel: cache updated
Panel->>Store: persist changes via API
User->>Card: Click elsewhere
Card->>Grid: emit assetBlur()
Grid->>Modal: emit assetBlur()
Modal->>Modal: clear focusedAsset, close right panel
Possibly related PRs
Suggested reviewers
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎭 Playwright Tests:
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/20/2026, 06:13:13 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
DrJKL
changed the title
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22.4 kB (baseline 22.4 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.01 MB (baseline 1.02 MB) • 🟢 -7.48 kBGraph 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: 10 added / 10 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: 8 added / 8 removed Data & Services — 3.09 MB (baseline 3.04 MB) • 🔴 +52.4 kBStores, services, APIs, and repositories
Status: 8 added / 8 removed Utilities & Hooks — 18.7 kB (baseline 18.7 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 7 added / 7 removed Vendor & Third-Party — 10.5 MB (baseline 10.4 MB) • 🔴 +84.6 kBExternal libraries and shared vendor chunks
Status: 4 added / 4 removed Other — 6.25 MB (baseline 6.25 MB) • ⚪ 0 BBundles that do not match a named category
Status: 43 added / 43 removed |
DrJKL
force-pushed
the
drjkl/others-call-it-a-garage-sale
branch
4 times, most recently
from
3a09d83 to
f93d463
Compare
DrJKL
force-pushed
the
drjkl/others-call-it-a-garage-sale
branch
from
17e00e1 to
6600086
Compare
Contributor
Author
|
Let me know if you'd like me to extract the Select component as a separate PR. |
Amp-Thread-ID: https://ampcode.com/threads/T-019bc42f-b9b7-71de-9d8f-6584610ab21e Co-authored-by: Amp <[email protected]>
Amp-Thread-ID: https://ampcode.com/threads/T-019bc49a-df5a-7708-8fc2-da5cb1c686d1 Co-authored-by: Amp <[email protected]>
Amp-Thread-ID: https://ampcode.com/threads/T-019bc5e5-232c-72bc-893a-afda46003fd3 Co-authored-by: Amp <[email protected]>
dosubot
bot
added
the
size:XXL
coderabbitai
bot
requested review from
KarryCharon,
Yorha4D and
christian-byrne
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/platform/assets/services/assetService.ts (1)
300-326: UsefromZodErrorfor consistent error handling.The
updateAssetmethod directly includes the Zod error in the thrown message (line 324), which could expose internal schema details. Other methods likevalidateAssetResponse(line 66) usefromZodErrorfor user-friendly error formatting.♻️ Proposed fix
+import { fromZodError } from 'zod-validation-error' + // ... in updateAsset function const newAsset = assetItemSchema.safeParse(await res.json()) if (newAsset.success) { return newAsset.data } - throw new Error( - `Unable to update asset ${id}: Invalid response - ${newAsset.error}` - ) + const error = fromZodError(newAsset.error) + console.error(`Invalid update response for asset ${id}:`, error) + throw new Error(`Unable to update asset ${id}: Invalid response`)Based on learnings, prefer
safeParse()withfromZodError()for logging while throwing user-friendly errors that don't reveal internal schema details.
🤖 Fix all issues with AI agents
In `@src/components/ui/select/SelectValue.vue`:
- Around line 1-12: The file SelectValue.vue uses
defineProps<SelectValueProps>() directly; to follow the project's reactive props
destructuring pattern (as used in SelectLabel.vue), destructure props into a
reactive constant (e.g., const props = defineProps<SelectValueProps>() -> const
{ /* ...props */ } = toRefs(defineProps<SelectValueProps>()) or the project's
established helper) and then bind that reactive object to the <SelectValue>
component so props are consistent across Select wrappers; locate
SelectValue.vue, the defineProps and the <SelectValue v-bind="props"> usage and
convert to the same reactive-destructured pattern used by SelectLabel.vue.
In `@src/components/widget/layout/BaseModalLayout.vue`:
- Around line 22-46: The icon-only Buttons (those invoking toggleLeftPanel,
toggleRightPanel, and closeDialog and conditioned by notMobile, hasRightPanel,
isRightPanelOpen, showLeftPanel) lack accessible names; add descriptive
accessible labels by adding an aria-label (or aria-labelledby) and optional
title to each Button so screen readers can announce their purpose (e.g., left
panel toggle, right panel open/close, close dialog), ensuring the label reflects
the dynamic state when applicable (use showLeftPanel to switch the label text).
In `@src/platform/assets/components/AssetGrid.vue`:
- Around line 75-86: The responsive breakpoint logic in AssetGrid.vue works but
can be simplified: replace multiple reactive refs (is2Xl, isXl, isLg, isMd) with
a single current breakpoint read from useBreakpoints().current() and compute
maxColumns based on that single value; update references to the useBreakpoints
import and the computed maxColumns (function name/symbol: maxColumns, and the
useBreakpoints call) to derive columns from the current breakpoint string
instead of multiple greaterOrEqual refs to reduce verbosity while preserving
behavior.
In `@src/platform/assets/components/modelInfo/ModelInfoPanel.test.ts`:
- Around line 131-155: Add a test in ModelInfoPanel.test.ts that verifies both
description fields render: create an asset via createMockAsset with a value that
getAssetDescription(asset) would return and a user-editable description in
user_metadata (or the prop the component uses), mount it with mountPanel, then
assert the read-only description text (from getAssetDescription) appears and
that the editable textarea/input for the user description exists and contains
the expected user-provided value; reference ModelInfoPanel.vue,
getAssetDescription(asset), mountPanel, and createMockAsset to locate where to
add this spec.
In `@src/platform/assets/components/modelInfo/ModelInfoPanel.vue`:
- Around line 141-157: The two ModelInfoField instances both use the same label
key causing duplicate "Description" headings; update the first (read-only)
ModelInfoField and the second (editable) ModelInfoField to use distinct
translation keys (e.g., change the label prop on the read-only ModelInfoField
and the editable one) so they display different labels (e.g., "Source
Description" vs "Your Notes"); update or add corresponding i18n keys for those
new labels and ensure the editable field still binds to userDescription, uses
ref descriptionTextarea and respects isImmutable.
In `@src/platform/assets/composables/useModelTypes.ts`:
- Around line 78-81: The fetchModelTypes function currently returns undefined on
the early exit but returns execute() (a Promise<ModelTypeOption[]>) otherwise,
causing an inconsistent return type; fix this by making the return contract
explicit: either (A) keep it fire-and-forget and make fetchModelTypes async and
await execute() so it always returns Promise<void> (no value) or (B) keep
returning the result and update the signature to
Promise<ModelTypeOption[]|undefined> and explicitly return undefined on early
exit; locate fetchModelTypes and the execute() call (and checks against
isReady.value / isLoading.value) and apply one of these two approaches
consistently across callers.
In `@src/platform/assets/utils/assetMetadataUtils.test.ts`:
- Around line 71-199: Add unit tests for the three new helpers:
getAssetBaseModels, getAssetModelType, and getAssetUserDescription. For
getAssetBaseModels, add tests that return an array when
user_metadata.base_models is an array of strings, filter out non-string entries,
and return [] when missing or not an array; for getAssetModelType, add tests
that return the expected model type string when user_metadata.model_type is a
string (including mixed-case inputs if normalization exists) and return null or
a default when absent or not a string; for getAssetUserDescription, add tests
that return the description string when user_metadata.user_description is a
string and return null/empty when missing or not a string. Use the same
mockAsset pattern as the existing tests and mirror the style used for
getAssetTriggerPhrases/getAssetAdditionalTags to keep consistency.
In `@src/stores/assetsStore.ts`:
- Around line 349-401: The optimistic cache updates in updateAssetMetadata and
updateAssetTags call updateAssetInCache before the network request and lack
rollback on failure; modify these functions to capture the previous asset
state(s) from modelAssetsByNodeType (using the same key-selection logic as
updateAssetInCache), perform the optimistic update, then wrap the
assetService.updateAsset call in try/catch and on error restore the previous
asset state(s) into modelAssetsByNodeType (or trigger a full re-fetch if
restoration is impractical); ensure the rollback honors the optional cacheKey
behavior and that error is re-thrown or handled/logged after restoring state.
Comment on lines
+131
to
+155
| describe('Model Description Section', () => { | ||
| it('renders trigger phrases when present', () => { | ||
| const asset = createMockAsset({ | ||
| user_metadata: { trained_words: ['trigger1', 'trigger2'] } | ||
| }) | ||
| const wrapper = mountPanel(asset) | ||
| expect(wrapper.text()).toContain('trigger1') | ||
| expect(wrapper.text()).toContain('trigger2') | ||
| }) | ||
|
|
||
| it('renders description section', () => { | ||
| const wrapper = mountPanel(createMockAsset()) | ||
| expect(wrapper.text()).toContain( | ||
| 'assetBrowser.modelInfo.modelDescription' | ||
| ) | ||
| }) | ||
|
|
||
| it('does not render trigger phrases field when empty', () => { | ||
| const asset = createMockAsset() | ||
| const wrapper = mountPanel(asset) | ||
| expect(wrapper.text()).not.toContain( | ||
| 'assetBrowser.modelInfo.triggerPhrases' | ||
| ) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a test for the duplicate description field scenario.
Lines 141-146 test the description section rendering, but ModelInfoPanel.vue (lines 141-157) renders two separate description fields:
- A read-only description from
getAssetDescription(asset)(line 142-146) - An editable user description textarea (line 147-157)
The current test only validates the section label exists. Consider adding a test to verify both description fields render correctly when appropriate.
🤖 Prompt for AI Agents
In `@src/platform/assets/components/modelInfo/ModelInfoPanel.test.ts` around lines
131 - 155, Add a test in ModelInfoPanel.test.ts that verifies both description
fields render: create an asset via createMockAsset with a value that
getAssetDescription(asset) would return and a user-editable description in
user_metadata (or the prop the component uses), mount it with mountPanel, then
assert the read-only description text (from getAssetDescription) appears and
that the editable textarea/input for the user description exists and contains
the expected user-provided value; reference ModelInfoPanel.vue,
getAssetDescription(asset), mountPanel, and createMockAsset to locate where to
add this spec.
Comment on lines
141
to
157
| <ModelInfoField | ||
| v-if="description" | ||
| :label="$t('assetBrowser.modelInfo.description')" | ||
| > | ||
| <p class="text-sm whitespace-pre-wrap">{{ description }}</p> | ||
| </ModelInfoField> | ||
| <ModelInfoField :label="$t('assetBrowser.modelInfo.description')"> | ||
| <textarea | ||
| ref="descriptionTextarea" | ||
| v-model="userDescription" | ||
| :disabled="isImmutable" | ||
| :placeholder="$t('assetBrowser.modelInfo.descriptionPlaceholder')" | ||
| rows="3" | ||
| class="w-full resize-y rounded-lg border border-transparent bg-transparent px-3 py-2 text-sm text-component-node-foreground outline-none transition-colors focus:bg-component-node-widget-background disabled:pointer-events-none" | ||
| @keydown.escape.stop="descriptionTextarea?.blur()" | ||
| /> | ||
| </ModelInfoField> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Duplicate "Description" labels may confuse users.
Both the read-only description (line 143) and the editable user description (line 147) use the same label key assetBrowser.modelInfo.description. Consider using distinct labels (e.g., "Source Description" and "Your Notes") to clarify their purposes.
🤖 Prompt for AI Agents
In `@src/platform/assets/components/modelInfo/ModelInfoPanel.vue` around lines 141
- 157, The two ModelInfoField instances both use the same label key causing
duplicate "Description" headings; update the first (read-only) ModelInfoField
and the second (editable) ModelInfoField to use distinct translation keys (e.g.,
change the label prop on the read-only ModelInfoField and the editable one) so
they display different labels (e.g., "Source Description" vs "Your Notes");
update or add corresponding i18n keys for those new labels and ensure the
editable field still binds to userDescription, uses ref descriptionTextarea and
respects isImmutable.
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Comment on lines
+349
to
+401
| function updateAssetInCache( | ||
| assetId: string, | ||
| updates: Partial<AssetItem>, | ||
| cacheKey?: string | ||
| ) { | ||
| const keysToCheck = cacheKey | ||
| ? [cacheKey] | ||
| : Array.from(modelAssetsByNodeType.keys()) | ||
|
|
||
| for (const key of keysToCheck) { | ||
| const assets = modelAssetsByNodeType.get(key) | ||
| if (!assets) continue | ||
|
|
||
| const index = assets.findIndex((a) => a.id === assetId) | ||
| if (index !== -1) { | ||
| const updatedAsset = { ...assets[index], ...updates } | ||
| const newAssets = [...assets] | ||
| newAssets[index] = updatedAsset | ||
| modelAssetsByNodeType.set(key, newAssets) | ||
| if (cacheKey) return | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Update asset metadata with optimistic cache update | ||
| * @param assetId The asset ID to update | ||
| * @param userMetadata The user_metadata to save | ||
| * @param cacheKey Optional cache key to target for optimistic update | ||
| */ | ||
| async function updateAssetMetadata( | ||
| assetId: string, | ||
| userMetadata: Record<string, unknown>, | ||
| cacheKey?: string | ||
| ) { | ||
| updateAssetInCache(assetId, { user_metadata: userMetadata }, cacheKey) | ||
| await assetService.updateAsset(assetId, { user_metadata: userMetadata }) | ||
| } | ||
|
|
||
| /** | ||
| * Update asset tags with optimistic cache update | ||
| * @param assetId The asset ID to update | ||
| * @param tags The tags array to save | ||
| * @param cacheKey Optional cache key to target for optimistic update | ||
| */ | ||
| async function updateAssetTags( | ||
| assetId: string, | ||
| tags: string[], | ||
| cacheKey?: string | ||
| ) { | ||
| updateAssetInCache(assetId, { tags }, cacheKey) | ||
| await assetService.updateAsset(assetId, { tags }) | ||
| } |
Contributor
There was a problem hiding this comment.
Add rollback/error handling for optimistic updates.
The cache is updated before the network call; if assetService.updateAsset fails, the UI stays out of sync. Capture previous values and revert on failure (or re-fetch) to keep state consistent. As per coding guidelines, implement proper error handling.
💡 Suggested rollback pattern
- function updateAssetInCache(
+ function updateAssetInCache(
assetId: string,
updates: Partial<AssetItem>,
cacheKey?: string
- ) {
+ ): Map<string, AssetItem> {
const keysToCheck = cacheKey
? [cacheKey]
: Array.from(modelAssetsByNodeType.keys())
+ const previousByKey = new Map<string, AssetItem>()
for (const key of keysToCheck) {
const assets = modelAssetsByNodeType.get(key)
if (!assets) continue
const index = assets.findIndex((a) => a.id === assetId)
if (index !== -1) {
+ previousByKey.set(key, assets[index])
const updatedAsset = { ...assets[index], ...updates }
const newAssets = [...assets]
newAssets[index] = updatedAsset
modelAssetsByNodeType.set(key, newAssets)
- if (cacheKey) return
+ if (cacheKey) break
}
}
+ return previousByKey
}
async function updateAssetMetadata(
assetId: string,
userMetadata: Record<string, unknown>,
cacheKey?: string
) {
- updateAssetInCache(assetId, { user_metadata: userMetadata }, cacheKey)
- await assetService.updateAsset(assetId, { user_metadata: userMetadata })
+ const previous = updateAssetInCache(
+ assetId,
+ { user_metadata: userMetadata },
+ cacheKey
+ )
+ try {
+ await assetService.updateAsset(assetId, { user_metadata: userMetadata })
+ } catch (err) {
+ for (const [key, asset] of previous) {
+ updateAssetInCache(assetId, asset, key)
+ }
+ throw err
+ }
}
async function updateAssetTags(
assetId: string,
tags: string[],
cacheKey?: string
) {
- updateAssetInCache(assetId, { tags }, cacheKey)
- await assetService.updateAsset(assetId, { tags })
+ const previous = updateAssetInCache(assetId, { tags }, cacheKey)
+ try {
+ await assetService.updateAsset(assetId, { tags })
+ } catch (err) {
+ for (const [key, asset] of previous) {
+ updateAssetInCache(assetId, asset, key)
+ }
+ throw err
+ }
}🤖 Prompt for AI Agents
In `@src/stores/assetsStore.ts` around lines 349 - 401, The optimistic cache
updates in updateAssetMetadata and updateAssetTags call updateAssetInCache
before the network request and lack rollback on failure; modify these functions
to capture the previous asset state(s) from modelAssetsByNodeType (using the
same key-selection logic as updateAssetInCache), perform the optimistic update,
then wrap the assetService.updateAsset call in try/catch and on error restore
the previous asset state(s) into modelAssetsByNodeType (or trigger a full
re-fetch if restoration is impractical); ensure the rollback honors the optional
cacheKey behavior and that error is re-thrown or handled/logged after restoring
state.
…setUserDescription
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/widget/layout/BaseModalLayout.vue (1)
198-210: Consider converting scoped styles to Tailwind utilities.Per coding guidelines, Vue components should avoid
<style>blocks and use Tailwind for styling. Most of these styles can be expressed as Tailwind utilities:<div class="h-[80vh] w-[90vw] max-w-[1280px] aspect-[20/13] min-[1450px]:max-w-[1724px] rounded-2xl overflow-hidden relative">The custom
1450pxbreakpoint can use Tailwind's arbitrary breakpoint syntaxmin-[1450px]:.♻️ Proposed refactor
<template> <div - class="base-widget-layout rounded-2xl overflow-hidden relative" + class="h-[80vh] w-[90vw] max-w-[1280px] min-[1450px]:max-w-[1724px] aspect-[20/13] rounded-2xl overflow-hidden relative" `@keydown.esc.capture`="handleEscape" >Then remove the
<style scoped>block entirely.
🤖 Fix all issues with AI agents
In `@src/components/widget/layout/BaseModalLayout.vue`:
- Around line 184-196: The escape handler in handleEscape currently only ignores
HTMLInputElement and HTMLTextAreaElement; update it to also ignore
contenteditable elements, select elements, and inputs inside Shadow DOM by:
treat event.target as Node/HTMLElement, return early if (target as
HTMLElement).isContentEditable is true or if tagName === 'SELECT', and
additionally inspect event.composedPath() for any
HTMLInputElement/HTMLTextAreaElement/select or an element with isContentEditable
so components using Shadow DOM are covered; keep the existing logic that closes
the right panel by setting isRightPanelOpen.value = false and stopping
propagation when none of these conditions match.
Comment on lines
+184
to
+196
| function handleEscape(event: KeyboardEvent) { | ||
| const target = event.target | ||
| if ( | ||
| target instanceof HTMLInputElement || | ||
| target instanceof HTMLTextAreaElement | ||
| ) { | ||
| return | ||
| } | ||
| if (isRightPanelOpen.value) { | ||
| event.stopPropagation() | ||
| isRightPanelOpen.value = false | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider expanding the escape handler to cover additional editable elements.
The check for HTMLInputElement and HTMLTextAreaElement may not cover all scenarios where escape should be ignored:
contenteditableelements<select>elements- Components using Shadow DOM inputs
If these element types are used within the right panel, users may experience unexpected panel closure.
🛠️ Optional enhancement
function handleEscape(event: KeyboardEvent) {
const target = event.target
+ if (!(target instanceof HTMLElement)) return
if (
target instanceof HTMLInputElement ||
- target instanceof HTMLTextAreaElement
+ target instanceof HTMLTextAreaElement ||
+ target instanceof HTMLSelectElement ||
+ target.isContentEditable
) {
return
}
if (isRightPanelOpen.value) {
event.stopPropagation()
isRightPanelOpen.value = false
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function handleEscape(event: KeyboardEvent) { | |
| const target = event.target | |
| if ( | |
| target instanceof HTMLInputElement || | |
| target instanceof HTMLTextAreaElement | |
| ) { | |
| return | |
| } | |
| if (isRightPanelOpen.value) { | |
| event.stopPropagation() | |
| isRightPanelOpen.value = false | |
| } | |
| } | |
| function handleEscape(event: KeyboardEvent) { | |
| const target = event.target | |
| if (!(target instanceof HTMLElement)) return | |
| if ( | |
| target instanceof HTMLInputElement || | |
| target instanceof HTMLTextAreaElement || | |
| target instanceof HTMLSelectElement || | |
| target.isContentEditable | |
| ) { | |
| return | |
| } | |
| if (isRightPanelOpen.value) { | |
| event.stopPropagation() | |
| isRightPanelOpen.value = false | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/components/widget/layout/BaseModalLayout.vue` around lines 184 - 196, The
escape handler in handleEscape currently only ignores HTMLInputElement and
HTMLTextAreaElement; update it to also ignore contenteditable elements, select
elements, and inputs inside Shadow DOM by: treat event.target as
Node/HTMLElement, return early if (target as HTMLElement).isContentEditable is
true or if tagName === 'SELECT', and additionally inspect event.composedPath()
for any HTMLInputElement/HTMLTextAreaElement/select or an element with
isContentEditable so components using Shadow DOM are covered; keep the existing
logic that closes the right panel by setting isRightPanelOpen.value = false and
stopping propagation when none of these conditions match.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/platform/assets/components/AssetCard.vue`:
- Around line 40-52: The Info Button component (the icon-only Button with
v-tooltip.bottom and `@click.stop`="$emit('showInfo', asset)") lacks an accessible
name; add an aria-label attribute to that Button and set its value to the same
i18n key used for the tooltip (i.e. use $t('assetBrowser.modelInfo.title')) so
screen readers announce it; ensure you only add aria-label to this icon-only
Button and not to other buttons like MoreButton.
Comment on lines
+40
to
+52
| <Button | ||
| v-tooltip.bottom="$t('assetBrowser.modelInfo.title')" | ||
| variant="secondary" | ||
| size="sm" | ||
| @click.stop="$emit('showInfo', asset)" | ||
| > | ||
| <i class="icon-[lucide--info]" /> | ||
| </Button> | ||
| <MoreButton | ||
| v-if="showAssetOptions" | ||
| ref="dropdown-menu-button" | ||
| size="sm" | ||
| > |
Contributor
There was a problem hiding this comment.
Add an accessible label to the icon-only Info button.
Line 40: the tooltip doesn’t provide an accessible name, so screen readers will announce an unlabeled button. Please add an aria-label using the existing i18n key. Based on learnings, add aria-labels only for icon-only buttons.
✅ Suggested fix
- <Button
- v-tooltip.bottom="$t('assetBrowser.modelInfo.title')"
- variant="secondary"
- size="sm"
- `@click.stop`="$emit('showInfo', asset)"
- >
+ <Button
+ v-tooltip.bottom="$t('assetBrowser.modelInfo.title')"
+ :aria-label="$t('assetBrowser.modelInfo.title')"
+ variant="secondary"
+ size="sm"
+ `@click.stop`="$emit('showInfo', asset)"
+ >🤖 Prompt for AI Agents
In `@src/platform/assets/components/AssetCard.vue` around lines 40 - 52, The Info
Button component (the icon-only Button with v-tooltip.bottom and
`@click.stop`="$emit('showInfo', asset)") lacks an accessible name; add an
aria-label attribute to that Button and set its value to the same i18n key used
for the tooltip (i.e. use $t('assetBrowser.modelInfo.title')) so screen readers
announce it; ensure you only add aria-label to this icon-only Button and not to
other buttons like MoreButton.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/platform/assets/composables/useAssetBrowser.test.ts`:
- Around line 504-508: The test's expected object for navItems.value[2] uses
title: 'BY TYPE' but the mock translation returns 'By type', causing a mismatch;
update the expectation in the test to match the mocked translation (change title
to 'By type') or adjust the mock translation to return 'BY TYPE' so the value
returned by the translation helper and the assertion in the test
(navItems.value[2]) are consistent.
♻️ Duplicate comments (1)
src/platform/assets/components/AssetGrid.vue (1)
81-99: Responsive column logic and static grid style look good.The breakpoint-driven
maxColumnscomputed property correctly uses VueUse'suseBreakpointsas recommended. ConvertinggridStyleto a static constant is appropriate since the values don't change at runtime.Note: A past review suggested an optional simplification using
useBreakpoints().current(), but the current approach is clear and the performance difference is negligible.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/platform/assets/composables/useAssetBrowser.test.ts`:
- Around line 382-386: The test calls updateFilters({ sortBy: 'name', ... })
which uses an undocumented value; change the test to use a documented sort key
(e.g., 'name-asc' or 'name-desc' depending on intended behaviour) so it
exercises the actual branches handled by the implementation (updateFilters /
sort logic that expects 'name-asc', 'name-desc', 'recent', 'popular').
In `@src/platform/assets/composables/useAssetBrowser.ts`:
- Around line 15-17: The NavId type alias currently declared as "type NavId =
'all' | 'imported' | string" collapses to plain string; decide whether to keep
arbitrary strings or tighten the type: either leave NavId as string (or replace
the union with just string) if arbitrary category IDs are intended, or replace
NavId with a stricter type such as a branded type or a template literal (e.g.,
Brand<'NavId'> or `type NavId = \`nav-\${string}\``) to preserve autocomplete
while enforcing constraints; update the declaration of NavId accordingly and
adjust any uses of NavId in this file if necessary (look for references to NavId
in useAssetBrowser composable).
Comment on lines
382
to
386
| updateFilters({ | ||
| sortBy: 'name', | ||
| fileFormats: [], | ||
| baseModels: [], | ||
| ownership: 'all' | ||
| baseModels: [] | ||
| }) |
Contributor
There was a problem hiding this comment.
Use documented sortBy value.
The test uses sortBy: 'name' but the implementation only handles 'name-asc', 'name-desc', 'recent', and 'popular'. While this works (falls through to default), it tests an undocumented value.
🐛 Proposed fix
updateFilters({
- sortBy: 'name',
+ sortBy: 'name-asc',
fileFormats: [],
baseModels: []
})🤖 Prompt for AI Agents
In `@src/platform/assets/composables/useAssetBrowser.test.ts` around lines 382 -
386, The test calls updateFilters({ sortBy: 'name', ... }) which uses an
undocumented value; change the test to use a documented sort key (e.g.,
'name-asc' or 'name-desc' depending on intended behaviour) so it exercises the
actual branches handled by the implementation (updateFilters / sort logic that
expects 'name-asc', 'name-desc', 'recent', 'popular').
Comment on lines
+15
to
+17
| type OwnershipOption = 'all' | 'my-models' | 'public-models' | ||
|
|
||
| type NavId = 'all' | 'imported' | string |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type alias NavId simplifies to string.
The union 'all' | 'imported' | string collapses to just string since any string literal is a subtype of string. If the intent is to provide IDE autocompletion hints for common values while allowing arbitrary strings, this works but loses type safety. Consider either:
- Keep as-is if arbitrary category IDs are intentional (current behavior is correct)
- Or use a branded type / template literal for stricter typing if you want to restrict values
🤖 Prompt for AI Agents
In `@src/platform/assets/composables/useAssetBrowser.ts` around lines 15 - 17, The
NavId type alias currently declared as "type NavId = 'all' | 'imported' |
string" collapses to plain string; decide whether to keep arbitrary strings or
tighten the type: either leave NavId as string (or replace the union with just
string) if arbitrary category IDs are intended, or replace NavId with a stricter
type such as a branded type or a template literal (e.g., Brand<'NavId'> or `type
NavId = \`nav-\${string}\``) to preserve autocomplete while enforcing
constraints; update the declaration of NavId accordingly and adjust any uses of
NavId in this file if necessary (look for references to NavId in useAssetBrowser
composable).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Adds an editable Model Info Panel to show and modify asset details in the asset browser.
Changes
ModelInfoPanelcomponent with editable display name, description, model type, base models, and tagsupdateAssetMetadataaction inassetsStorewith optimistic cache updatesSelectcomponents with design system stylingassetMetadataUtilsfor extracting model metadataBaseModalLayoutright panel state todefineModelpatternclassprop toPropertiesAccordionItemfor custom stylingTesting
ModelInfoPanelcomponentassetMetadataUtilsfunctions