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 Model Info panel with right-side panel wiring to the Asset Browser, many asset metadata utilities and schema types, optimistic cache update APIs in the assets store, asset card/grid focus and show-info events, ownership/filter removal, i18n/test updates, and small prop destructuring tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Card as AssetCard
participant Grid as AssetGrid
participant Modal as AssetBrowserModal
participant Panel as ModelInfoPanel
participant Store as assetsStore
participant API as API
User->>Card: click / focus
Card->>Grid: emit assetFocus
Grid->>Modal: re-emit assetFocus
Modal->>Modal: set focusedAsset
User->>Card: click "Show Info"
Card->>Grid: emit assetShowInfo
Grid->>Modal: re-emit assetShowInfo
Modal->>Modal: set isRightPanelOpen = true
Modal->>Panel: render with focused asset
User->>Panel: edit metadata (name/tags/description)
Panel->>Panel: debounce pendingUpdates
Panel->>Store: call updateAssetMetadata()
Store->>Store: optimistically update cache
Store->>API: send updateAsset(payload)
API-->>Store: respond success
Store-->>Panel: confirm persisted
Possibly related PRs
Suggested reviewers
Comment |
🎭 Playwright Tests:
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/22/2026, 03:30:23 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
DrJKL
changed the title
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.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.11 MB (baseline 3.06 MB) • 🔴 +50.3 kBStores, services, APIs, and repositories
Status: 8 added / 8 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.5 MB (baseline 10.4 MB) • 🔴 +84.6 kBExternal libraries and shared vendor chunks
Status: 4 added / 4 removed Other — 6.28 MB (baseline 6.28 MB) • 🟢 -91 BBundles that do not match a named category
Status: 41 added / 41 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
|
|
DrJKL
force-pushed
the
drjkl/others-call-it-a-garage-sale
branch
from
bfa147f to
feadad5
Compare
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.
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.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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/renderer/extensions/vueNodes/widgets/composables/useAssetWidgetData.ts (1)
50-59: Redundant null guard, but harmless.The
?? []at line 51 is unnecessary sinceassets(line 35-38) already guarantees an array via?? []. The defensive guard doesn't hurt but adds noise.♻️ Optional simplification
const dropdownItems = computed<DropdownItem[]>(() => { - return (assets.value ?? []).map((asset) => ({ + return assets.value.map((asset) => ({ id: asset.id,
🤖 Fix all issues with AI agents
In `@src/platform/assets/components/AssetBrowserModal.vue`:
- Around line 26-29: The header div currently clears focusedAsset with
`@click`="focusedAsset = null", which bubbles and also fires when interacting with
children (e.g., the SearchBox or Upload button). Change the handler to use
`@click.self` so it only runs when the div itself is clicked (leave focusedAsset
variable name as is); update the div with `@click.self`="focusedAsset = null" to
avoid interfering with child component interactions.
- Around line 56-57: The click handler on the modal root currently assigns
focusedAsset = null and is triggering when interacting with child controls
(e.g., AssetFilterBar), causing unwanted bubbling; change the handler to only
fire for direct clicks by using `@click.self` on the element with the current
`@click` or move the clear logic into the AssetFilterBar component (e.g., emit a
close or clear event and let the parent set focusedAsset = null) so interactions
inside AssetFilterBar no longer clear focusedAsset.
- Around line 157-159: The current eager call "void fetchModelTypes()" silences
promise errors and can swallow failures; replace it with explicit error handling
by invoking fetchModelTypes() inside an async context and catching/logging
failures (e.g., using try/catch around await fetchModelTypes() or adding
.catch(...) to the promise) so errors from useModelTypes().fetchModelTypes() are
surfaced (include a clear log message) to aid debugging and ensure
ModelInfoPanel has visibility into load failures.
In `@src/platform/assets/components/modelInfo/ModelInfoPanel.test.ts`:
- Around line 37-48: The $t mock in mountPanel should be typed to match the real
i18n translation function to catch invalid keys/params; update the mock
signature in the mountPanel function (where ModelInfoPanel is mounted) to use
the actual i18n type (e.g., derive from the i18n instance's t type or
ComponentCustomProperties['$t'] / typeof i18n.global.t) instead of an untyped
(key: string, params?: Record<string,string>) signature so the compiler enforces
the correct key/param shapes during tests.
- Around line 115-123: Update the test mock so it matches the real asset shape:
change the user_metadata key used in ModelInfoPanel.test from the plural array
"base_models" to the singular string "base_model" (e.g., user_metadata: {
base_model: 'SDXL' }) so getAssetBaseModels will read and populate the field;
locate the usage in the test that calls createMockAsset and mountPanel to adjust
the mock accordingly.
In `@src/platform/assets/schemas/assetSchema.ts`:
- Around line 99-107: The zAssetUserMetadata schema is defined but only its
inferred type AssetUserMetadata is exported, so runtime validation isn’t
available; either export the schema (export const zAssetUserMetadata) so callers
can perform runtime checks or replace usages that expect validation with direct
zAssetUserMetadata.parse/ safeParse calls where appropriate (look for references
to AssetUserMetadata, zAssetUserMetadata) and update exports accordingly to
ensure runtime validation is accessible.
In `@src/platform/assets/utils/assetMetadataUtils.ts`:
- Around line 47-56: The getAssetBaseModels function currently returns
base_model strings without excluding empty or whitespace-only values; update the
logic in getAssetBaseModels to trim and filter out empty/whitespace entries:
when baseModel is an array, change the filter predicate to ensure typeof m ===
'string' and m.trim().length > 0, and when baseModel is a single string, return
[baseModel.trim()] only if baseModel.trim().length > 0; keep other behavior
unchanged.
- Around line 125-128: getSourceName currently returns localized display
strings; change it to return stable source keys (e.g., 'civitai' | 'huggingface'
| 'source') instead so translation happens at the call site via vue-i18n. Update
the function getSourceName(url: string) to return 'civitai' when
url.includes('civitai.com'), 'huggingface' when url.includes('huggingface.co'),
and 'source' otherwise; consumers should map those keys to translations (reuse
existing locale keys like providerCivitai and providerHuggingFace in
src/locales/en/main.json or add new entries as needed). Ensure no direct i18n
calls are added inside assetMetadataUtils.ts and adjust any call sites to
translate the returned key via vue-i18n.
♻️ Duplicate comments (4)
src/platform/assets/components/modelInfo/ModelInfoPanel.vue (2)
141-166: Duplicate "Description" labels remain unaddressed.Both the read-only description (line 143) and the editable user description (line 147) use the same label key
assetBrowser.modelInfo.description. As noted in the previous review, distinct labels would improve clarity. The design team input was requested - awaiting resolution.
243-250: Guardasset.user_metadatabefore spreading to prevent runtime error.When
asset.user_metadataisundefined, the spread{ ...asset.user_metadata, ...pendingUpdates.value }will work in JavaScript but the explicit guard makes intent clearer and prevents potential issues.🐛 Suggested fix
const debouncedFlushMetadata = useDebounceFn(() => { if (isImmutable.value) return assetsStore.updateAssetMetadata( asset.id, - { ...asset.user_metadata, ...pendingUpdates.value }, + { ...(asset.user_metadata ?? {}), ...pendingUpdates.value }, cacheKey ) }, 500)src/stores/assetsStore.ts (1)
429-480: Add rollback/error handling for optimistic cache updates.
IfassetService.updateAssetfails, the cache stays out of sync. Capture previous values and restore (or re-fetch) on failure to keep UI 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(modelStateByKey.value.keys()) + const previousByKey = new Map<string, AssetItem>() for (const key of keysToCheck) { const state = modelStateByKey.value.get(key) if (!state?.assets) continue const existingAsset = state.assets.get(assetId) if (existingAsset) { + previousByKey.set(key, existingAsset) const updatedAsset = { ...existingAsset, ...updates } state.assets.set(assetId, updatedAsset) assetsArrayCache.delete(key) - 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 + } }src/platform/assets/utils/assetMetadataUtils.ts (1)
136-139: Avoid returning empty string fromgetAssetModelType.
Line 137 can yield''for tags like''ormodels/, which is awkward for consumers.♻️ Suggested fix
-export function getAssetModelType(asset: AssetItem): string | null { - const typeTag = asset.tags?.find((tag) => tag !== 'models') - if (!typeTag) return null - return typeTag.includes('/') ? (typeTag.split('/').pop() ?? null) : typeTag -} +export function getAssetModelType(asset: AssetItem): string | null { + const rawTag = asset.tags?.find((tag) => tag && tag !== 'models') + const typeTag = rawTag?.trim() + if (!typeTag) return null + const lastSegment = typeTag.includes('/') ? typeTag.split('/').pop() : typeTag + return lastSegment && lastSegment.length > 0 ? lastSegment : null +}
Comment on lines
+157
to
+159
| // Eagerly fetch model types so they're available when ModelInfoPanel loads | ||
| const { fetchModelTypes } = useModelTypes() | ||
| void fetchModelTypes() |
Contributor
There was a problem hiding this comment.
Unhandled promise rejection from fetchModelTypes.
The void prefix silences the floating promise lint error but doesn't handle potential errors. If fetchModelTypes fails, the error will be silently swallowed. Consider adding error handling or at minimum logging failures.
🐛 Proposed fix
// Eagerly fetch model types so they're available when ModelInfoPanel loads
const { fetchModelTypes } = useModelTypes()
-void fetchModelTypes()
+fetchModelTypes().catch((err) => {
+ console.warn('Failed to fetch model types:', err)
+})📝 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
| // Eagerly fetch model types so they're available when ModelInfoPanel loads | |
| const { fetchModelTypes } = useModelTypes() | |
| void fetchModelTypes() | |
| // Eagerly fetch model types so they're available when ModelInfoPanel loads | |
| const { fetchModelTypes } = useModelTypes() | |
| fetchModelTypes().catch((err) => { | |
| console.warn('Failed to fetch model types:', err) | |
| }) |
🤖 Prompt for AI Agents
In `@src/platform/assets/components/AssetBrowserModal.vue` around lines 157 - 159,
The current eager call "void fetchModelTypes()" silences promise errors and can
swallow failures; replace it with explicit error handling by invoking
fetchModelTypes() inside an async context and catching/logging failures (e.g.,
using try/catch around await fetchModelTypes() or adding .catch(...) to the
promise) so errors from useModelTypes().fetchModelTypes() are surfaced (include
a clear log message) to aid debugging and ensure ModelInfoPanel has visibility
into load failures.
Contributor
Author
There was a problem hiding this comment.
I like to live dangerously.
Contributor
There was a problem hiding this comment.
@DrJKL Fair enough! 🚀 I'll leave the eager fetch as-is.
🧠 Learnings used
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-16T21:33:47.062Z
Learning: Applies to src/**/*.{ts,tsx,vue} : Avoid using `ts-expect-error` - fix the underlying type issue instead
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T19:47:02.860Z
Learning: Applies to src/**/*.{vue,ts} : Implement proper error handling
Comment on lines
37
to
48
| const mountPanel = (asset: AssetDisplayItem) => { | ||
| return mount(ModelInfoPanel, { | ||
| props: { asset }, | ||
| global: { | ||
| plugins: [createTestingPinia({ stubActions: false }), i18n], | ||
| mocks: { | ||
| $t: (key: string, params?: Record<string, string>) => | ||
| params ? `${key}:${JSON.stringify(params)}` : key | ||
| } | ||
| } | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider typing the $t mock more strictly.
The $t mock function signature could benefit from stricter typing to catch translation key issues during test development.
♻️ Optional improvement
mocks: {
- $t: (key: string, params?: Record<string, string>) =>
+ $t: (key: string, params?: Record<string, unknown>) =>
params ? `${key}:${JSON.stringify(params)}` : key
}🤖 Prompt for AI Agents
In `@src/platform/assets/components/modelInfo/ModelInfoPanel.test.ts` around lines
37 - 48, The $t mock in mountPanel should be typed to match the real i18n
translation function to catch invalid keys/params; update the mock signature in
the mountPanel function (where ModelInfoPanel is mounted) to use the actual i18n
type (e.g., derive from the i18n instance's t type or
ComponentCustomProperties['$t'] / typeof i18n.global.t) instead of an untyped
(key: string, params?: Record<string,string>) signature so the compiler enforces
the correct key/param shapes during tests.
Comment on lines
115
to
123
| it('renders base models field', () => { | ||
| const asset = createMockAsset({ | ||
| user_metadata: { base_models: ['SDXL'] } | ||
| }) | ||
| const wrapper = mountPanel(asset) | ||
| expect(wrapper.text()).toContain( | ||
| 'assetBrowser.modelInfo.compatibleBaseModels' | ||
| ) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'base_model' src/platform/assets/utils/assetMetadataUtils.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 388
Fix test mock data: use base_model instead of base_models.
The test provides user_metadata: { base_models: ['SDXL'] } (plural array), but getAssetBaseModels reads from user_metadata?.base_model (singular string). The test passes because it only checks for the i18n key presence, but the mock data won't actually populate the field, failing to exercise the intended rendering path. Change the mock to user_metadata: { base_model: 'SDXL' }.
🤖 Prompt for AI Agents
In `@src/platform/assets/components/modelInfo/ModelInfoPanel.test.ts` around lines
115 - 123, Update the test mock so it matches the real asset shape: change the
user_metadata key used in ModelInfoPanel.test from the plural array
"base_models" to the singular string "base_model" (e.g., user_metadata: {
base_model: 'SDXL' }) so getAssetBaseModels will read and populate the field;
locate the usage in the test that calls createMockAsset and mountPanel to adjust
the mock accordingly.
Comment on lines
+99
to
+107
| /** User-editable metadata fields for model assets */ | ||
| const zAssetUserMetadata = z.object({ | ||
| name: z.string().optional(), | ||
| base_model: z.array(z.string()).optional(), | ||
| additional_tags: z.array(z.string()).optional(), | ||
| user_description: z.string().optional() | ||
| }) | ||
|
|
||
| export type AssetUserMetadata = z.infer<typeof zAssetUserMetadata> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider whether zAssetUserMetadata should be exported or used for validation.
The schema zAssetUserMetadata is defined but only its inferred type is exported. If runtime validation of user metadata is needed (e.g., before API calls), consider exporting the schema itself. If only the type is needed, this is fine as-is.
♻️ Optional: Export schema for runtime validation
/** User-editable metadata fields for model assets */
-const zAssetUserMetadata = z.object({
+export const zAssetUserMetadata = z.object({
name: z.string().optional(),
base_model: z.array(z.string()).optional(),
additional_tags: z.array(z.string()).optional(),
user_description: z.string().optional()
})📝 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
| /** User-editable metadata fields for model assets */ | |
| const zAssetUserMetadata = z.object({ | |
| name: z.string().optional(), | |
| base_model: z.array(z.string()).optional(), | |
| additional_tags: z.array(z.string()).optional(), | |
| user_description: z.string().optional() | |
| }) | |
| export type AssetUserMetadata = z.infer<typeof zAssetUserMetadata> | |
| /** User-editable metadata fields for model assets */ | |
| export const zAssetUserMetadata = z.object({ | |
| name: z.string().optional(), | |
| base_model: z.array(z.string()).optional(), | |
| additional_tags: z.array(z.string()).optional(), | |
| user_description: z.string().optional() | |
| }) | |
| export type AssetUserMetadata = z.infer<typeof zAssetUserMetadata> |
🤖 Prompt for AI Agents
In `@src/platform/assets/schemas/assetSchema.ts` around lines 99 - 107, The
zAssetUserMetadata schema is defined but only its inferred type
AssetUserMetadata is exported, so runtime validation isn’t available; either
export the schema (export const zAssetUserMetadata) so callers can perform
runtime checks or replace usages that expect validation with direct
zAssetUserMetadata.parse/ safeParse calls where appropriate (look for references
to AssetUserMetadata, zAssetUserMetadata) and update exports accordingly to
ensure runtime validation is accessible.
Contributor
There was a problem hiding this comment.
@DrJKL Understood! Keeping the type export only is fine—no need to expose the schema if it won't be used for runtime validation. 👍
🧠 Learnings used
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7746
File: src/platform/assets/services/assetService.ts:484-491
Timestamp: 2026-01-06T19:20:56.167Z
Learning: In `src/platform/assets/services/assetService.ts`, prefer using `schema.safeParse()` over `schema.parse()` for validating API responses to avoid throwing ZodError with internal schema details; follow the existing pattern: call `safeParse()`, check `result.success`, and use `fromZodError(result.error)` to format error messages for logging while throwing user-friendly errors.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-16T21:33:34.380Z
Learning: Applies to src/**/*.{ts,tsx} : Minimize the surface area (exported values) of each module and composable
Comment on lines
+125
to
+128
| export function getSourceName(url: string): string { | ||
| if (url.includes('civitai.com')) return 'Civitai' | ||
| if (url.includes('huggingface.co')) return 'Hugging Face' | ||
| return 'Source' |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/platform/assets/utils/assetMetadataUtils.ts | head -150Repository: Comfy-Org/ComfyUI_frontend
Length of output: 6129
🏁 Script executed:
rg "getSourceName" -A 3 -B 3Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3591
🏁 Script executed:
fd -t f "assetMetadataUtils" --exec head -5 {}Repository: Comfy-Org/ComfyUI_frontend
Length of output: 434
🏁 Script executed:
rg "sourceName" src/platform/assets/components/modelInfo/ModelInfoPanel.vue -A 5 -B 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 781
🏁 Script executed:
grep -n "sourceName" src/platform/assets/components/modelInfo/ModelInfoPanel.vue | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 235
🏁 Script executed:
cat src/platform/assets/components/modelInfo/ModelInfoPanel.vue | sed -n '1,50p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1820
🏁 Script executed:
rg "viewOnSource" src/locales/en/main.jsonRepository: Comfy-Org/ComfyUI_frontend
Length of output: 111
🏁 Script executed:
rg "assetBrowser" src/locales/en/main.json | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 89
🏁 Script executed:
rg "Civitai|Hugging Face" src/locales/en/main.jsonRepository: Comfy-Org/ComfyUI_frontend
Length of output: 428
🏁 Script executed:
cat src/platform/assets/components/modelInfo/ModelInfoPanel.vue | sed -n '35,55p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 776
Localize source names using i18n.
These strings are user-facing. Return a stable source key (e.g., 'civitai' | 'huggingface' | 'source') and translate at the call site via vue-i18n. Translation keys already exist in src/locales/en/main.json (providerCivitai, providerHuggingFace); reuse or add new entries as needed.
🤖 Prompt for AI Agents
In `@src/platform/assets/utils/assetMetadataUtils.ts` around lines 125 - 128,
getSourceName currently returns localized display strings; change it to return
stable source keys (e.g., 'civitai' | 'huggingface' | 'source') instead so
translation happens at the call site via vue-i18n. Update the function
getSourceName(url: string) to return 'civitai' when url.includes('civitai.com'),
'huggingface' when url.includes('huggingface.co'), and 'source' otherwise;
consumers should map those keys to translations (reuse existing locale keys like
providerCivitai and providerHuggingFace in src/locales/en/main.json or add new
entries as needed). Ensure no direct i18n calls are added inside
assetMetadataUtils.ts and adjust any call sites to translate the returned key
via vue-i18n.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/platform/assets/components/modelInfo/ModelInfoPanel.vue`:
- Around line 236-241: The watcher currently only watches asset.user_metadata so
pendingUpdates can persist across asset switches when user_metadata is
undefined; update the watch to observe both asset.id and asset.user_metadata
(e.g., watch(() => [asset.id, asset.user_metadata], ...)) and keep the reset
logic setting pendingUpdates.value = {} so it runs on any asset change; also
remove the non-existent debouncedFlushMetadata.cancel?.() call (it’s not part of
`@vueuse/core` useDebounceFn) to avoid relying on an unsupported cancellation API.
- Around line 264-271: The removal filter in debouncedSaveModelType incorrectly
compares full tag strings to currentModelType when tags may be namespaced;
update the filter over asset.tags (used before calling
assetsStore.updateAssetTags) to compare the tag's last segment (e.g.,
tag.split('/').pop()) or call getAssetModelType on each tag so you remove tags
whose extracted model type equals currentModelType, then concat newModelType and
call assetsStore.updateAssetTags(asset.id, newTags, cacheKey).
♻️ Duplicate comments (4)
src/platform/assets/utils/assetMetadataUtils.ts (3)
47-56: Filter/trim empty base model values.This avoids blank badges/options when metadata contains empty strings.
♻️ Proposed tweak
- if (Array.isArray(baseModel)) { - return baseModel.filter((m): m is string => typeof m === 'string') - } - if (typeof baseModel === 'string' && baseModel) { - return [baseModel] - } + if (Array.isArray(baseModel)) { + return baseModel.filter( + (m): m is string => typeof m === 'string' && m.trim().length > 0 + ) + } + if (typeof baseModel === 'string' && baseModel.trim().length > 0) { + return [baseModel.trim()] + }
125-128: Return a stable source key and localize at the call site.User-facing strings shouldn’t originate in TS utilities. Return a key (e.g.,
civitai | huggingface | source) and translate where you render.As per coding guidelines, localize user-facing strings via vue-i18n.
136-139: Avoid empty/whitespace model types.Trailing slashes or whitespace can return empty strings; normalize and guard.
♻️ Suggested normalization
-export function getAssetModelType(asset: AssetItem): string | null { - const typeTag = asset.tags?.find((tag) => tag && tag !== 'models') - if (!typeTag) return null - return typeTag.includes('/') ? (typeTag.split('/').pop() ?? null) : typeTag -} +export function getAssetModelType(asset: AssetItem): string | null { + const typeTag = asset.tags?.find((tag) => tag?.trim() && tag.trim() !== 'models') + if (!typeTag) return null + const normalized = typeTag.trim() + const leaf = normalized.includes('/') ? normalized.split('/').pop() : normalized + return leaf?.trim().length ? leaf.trim() : null +}src/platform/assets/components/modelInfo/ModelInfoPanel.vue (1)
141-148: Use distinct labels for source vs. user descriptions.The two fields currently share the same label key and will render identical headings.
As per coding guidelines, add distinct vue-i18n keys in
src/locales/en/main.json.
Comment on lines
+236
to
+241
| watch( | ||
| () => asset.user_metadata, | ||
| () => { | ||
| pendingUpdates.value = {} | ||
| } | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/platform/assets/components/modelInfo/ModelInfoPanel.vue | sed -n '230,250p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 748
🏁 Script executed:
wc -l src/platform/assets/components/modelInfo/ModelInfoPanel.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 133
🏁 Script executed:
cat -n src/platform/assets/components/modelInfo/ModelInfoPanel.vue | head -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4140
🏁 Script executed:
rg "debouncedFlushMetadata" src/platform/assets/components/modelInfo/ModelInfoPanel.vue -B 2 -A 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 349
🏁 Script executed:
rg "pendingUpdates" src/platform/assets/components/modelInfo/ModelInfoPanel.vue -B 1 -A 1Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1069
🏁 Script executed:
rg "useDebounceFn" src/platform/assets/components/modelInfo/ModelInfoPanel.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 240
🏁 Script executed:
rg "cancel" src/platform/assets/components/modelInfo/ModelInfoPanel.vue -B 2 -A 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 251
🏁 Script executed:
rg "useDebounceFn" src/ -A 5 | rg "cancel" -B 5 -A 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🌐 Web query:
@vueuse/core useDebounceFn cancel method documentation
💡 Result:
Short answer: The official @vueuse/core docs for useDebounceFn do not document a public .cancel() method — the composable returns a debounced function (a PromisifyFn) and cancellations are internal; to observe cancellation you can pass the option rejectOnCancel: true so the returned promise rejects when a pending call is cancelled. [1]
If you need an explicit cancel/flush API, some community wrappers / alternative implementations (e.g. vue-hooks-plus or other utilities) expose run/cancel/flush methods; those are not part of the official @vueuse/core useDebounceFn API. [2][3]
Sources:
- Official VueUse useDebounceFn docs (shows returned debounced function and rejectOnCancel option). [1]
- vue-hooks-plus documentation (example of a debounce API that exposes cancel/flush). [2]
- W3cub / mirrored docs showing same official behavior. [3]
🏁 Script executed:
rg "@vueuse/core" package.jsonRepository: Comfy-Org/ComfyUI_frontend
Length of output: 101
🏁 Script executed:
rg "useDebounceFn" src/ --type typescript -B 3 -A 8 | head -80Repository: Comfy-Org/ComfyUI_frontend
Length of output: 104
🏁 Script executed:
rg "debouncedFlushMetadata" src/platform/assets/components/modelInfo/ModelInfoPanel.vue -B 5 -A 15Repository: Comfy-Org/ComfyUI_frontend
Length of output: 963
Watch asset changes to prevent stale pendingUpdates writes across asset switches.
When asset.user_metadata remains undefined during an asset switch, the watcher doesn't trigger, leaving pendingUpdates populated with data from the previous asset. The next debounced flush would then update the wrong asset. Watch asset.id alongside asset.user_metadata to ensure resets occur on any asset change.
Note: The proposed debouncedFlushMetadata.cancel?.() in the diff is not part of the official @vueuse/core useDebounceFn API and will not cancel the debounced call. Remove that line from the fix.
🤖 Prompt for AI Agents
In `@src/platform/assets/components/modelInfo/ModelInfoPanel.vue` around lines 236
- 241, The watcher currently only watches asset.user_metadata so pendingUpdates
can persist across asset switches when user_metadata is undefined; update the
watch to observe both asset.id and asset.user_metadata (e.g., watch(() =>
[asset.id, asset.user_metadata], ...)) and keep the reset logic setting
pendingUpdates.value = {} so it runs on any asset change; also remove the
non-existent debouncedFlushMetadata.cancel?.() call (it’s not part of
`@vueuse/core` useDebounceFn) to avoid relying on an unsupported cancellation API.
Comment on lines
+264
to
+271
| const debouncedSaveModelType = useDebounceFn((newModelType: string) => { | ||
| if (isImmutable.value) return | ||
| const currentModelType = getAssetModelType(asset) | ||
| if (currentModelType === newModelType) return | ||
| const newTags = asset.tags | ||
| .filter((tag) => tag !== currentModelType) | ||
| .concat(newModelType) | ||
| assetsStore.updateAssetTags(asset.id, newTags, cacheKey) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the asset type definition to understand the tags property
fd -e ts -e d.ts | xargs grep -l "tags.*:" | grep -E "(types|models|asset)" | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1008
🏁 Script executed:
# Look at the ModelInfoPanel.vue file to understand context
cat -n src/platform/assets/components/modelInfo/ModelInfoPanel.vue | sed -n '250,280p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1238
🏁 Script executed:
# Find getAssetModelType function
rg -n "getAssetModelType" -A 5 --type ts --type vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 97
🏁 Script executed:
# Find updateAssetTags function
rg -n "updateAssetTags" -A 10 --type tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 2573
🏁 Script executed:
# Search for asset type definitions
rg -n "interface.*[Aa]sset|type.*[Aa]sset" --type ts | grep -v node_modules | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2089
🏁 Script executed:
# Check the AssetItem definition in assetSchema
cat -n src/platform/assets/schemas/assetSchema.ts | head -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4103
🏁 Script executed:
# Find getAssetModelType function
rg -n "getAssetModelType" -A 10 -B 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 7784
🏁 Script executed:
# Check how tags are used elsewhere in the codebase
rg -n "asset\.tags" -B 2 -A 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 6400
🏁 Script executed:
# Check the complete getAssetModelType function and verify tag format
cat -n src/platform/assets/utils/assetMetadataUtils.ts | sed -n '130,145p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 782
🏁 Script executed:
# Look at test cases for getAssetModelType to understand expected behavior
cat -n src/platform/assets/utils/assetMetadataUtils.test.ts | sed -n '223,245p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 905
🏁 Script executed:
# Check if asset is guaranteed to be validated through Zod schema
rg -n "AssetItem|assetItemSchema" -A 3 -B 3 | grep -A 5 -B 5 "ModelInfoPanel"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
# Check the prop definition for asset in ModelInfoPanel.vue
cat -n src/platform/assets/components/modelInfo/ModelInfoPanel.vue | sed -n '1,230p' | grep -A 10 "defineProps"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 408
🏁 Script executed:
# Look for other usages of asset.tags without guards to understand the pattern
rg -n "\.tags\." -B 2 -A 2 | head -60Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4700
🏁 Script executed:
# Check what happens when updateAssetTags is called - does it accept empty arrays or require specific format?
cat -n src/stores/assetsStore.ts | sed -n '473,490p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 643
🏁 Script executed:
# Find AssetDisplayItem type definition
rg -n "AssetDisplayItem" -B 2 -A 5 | head -40Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3403
🏁 Script executed:
# Check if AssetDisplayItem extends AssetItem or has its own definition
rg -n "type AssetDisplayItem|interface AssetDisplayItem" -A 10Repository: Comfy-Org/ComfyUI_frontend
Length of output: 857
🏁 Script executed:
# Verify the tag removal logic with examples from tests
cat -n src/platform/assets/utils/assetMetadataUtils.test.ts | sed -n '223,245p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 905
🏁 Script executed:
# Check if there are any tests for the debouncedSaveModelType function
rg -n "debouncedSaveModelType|handleModelTypeChange" -B 3 -A 10Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2019
🏁 Script executed:
# Look for any tag normalization utilities
rg -n "normaliz|canonicali" --type ts --type vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 97
🏁 Script executed:
# Look at how tags are structured in fixtures/tests to understand the format
cat -n src/platform/assets/fixtures/ui-mock-assets.ts | head -80Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4012
🏁 Script executed:
# Check if there are any integration tests for the ModelInfoPanel tag update logic
fd -e test.ts -o -e test.tsx | xargs grep -l "ModelInfoPanel" | head -5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 186
🏁 Script executed:
# Check how the new model type is being constructed when it's added back to tags
cat -n src/platform/assets/components/modelInfo/ModelInfoPanel.vue | sed -n '287,297p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 493
🏁 Script executed:
# Look at the test fixtures more carefully to see actual tag values
cat -n src/platform/assets/fixtures/ui-mock-assets.ts | sed -n '75,150p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3120
🏁 Script executed:
# Final verification: check if tags are actually used with path format in real code
rg -n "models/" src/platform/assets --type ts | grep -i tag | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 315
Guard tag removal logic to handle namespaced tags correctly.
Tags can be namespaced with forward slashes (e.g., models/loras). When removing the old model type tag, the current filter compares the full tag string against the extracted type name, causing old namespaced tags to not be removed. The filter should extract the last segment from each tag before comparison, or better yet, use getAssetModelType to identify which tag to remove.
const debouncedSaveModelType = useDebounceFn((newModelType: string) => {
if (isImmutable.value) return
const currentModelType = getAssetModelType(asset)
if (currentModelType === newModelType) return
- const newTags = asset.tags
- .filter((tag) => tag !== currentModelType)
+ const newTags = asset.tags
+ .filter((tag) => tag === 'models' || !tag.includes(currentModelType ?? ''))
.concat(newModelType)
assetsStore.updateAssetTags(asset.id, newTags, cacheKey)
}, 500)🤖 Prompt for AI Agents
In `@src/platform/assets/components/modelInfo/ModelInfoPanel.vue` around lines 264
- 271, The removal filter in debouncedSaveModelType incorrectly compares full
tag strings to currentModelType when tags may be namespaced; update the filter
over asset.tags (used before calling assetsStore.updateAssetTags) to compare the
tag's last segment (e.g., tag.split('/').pop()) or call getAssetModelType on
each tag so you remove tags whose extracted model type equals currentModelType,
then concat newModelType and call assetsStore.updateAssetTags(asset.id, newTags,
cacheKey).
Add hasAssetKey method to assetsStore to detect initialized keys Change useAssetWidgetData watch to check loading state and key existence instead of array length Amp-Thread-ID: https://ampcode.com/threads/T-019be386-8ca3-7206-afbd-dc75c597a50d Co-authored-by: Amp <[email protected]>
- Use composition API useI18n instead of options API \$t - Update test to check Civitai icon instead of text
Amp-Thread-ID: https://ampcode.com/threads/T-019be3bd-bbbc-717a-a99c-598db9d92302 Co-authored-by: Amp <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/platform/assets/components/AssetBrowserModal.vue`:
- Around line 60-72: The click handler on the AssetGrid clears focusedAsset for
any inner click and should only run when the grid backdrop itself is clicked;
update the AssetGrid's click listener to use the `.self` modifier (change
`@click`="focusedAsset = null" to use the self modifier) so clicks that originate
from asset cards or child elements (which emit `@asset-focus/`@asset-select via
handleAssetFocus/handleAssetSelectAndEmit) do not bubble and immediately clear
the focus.
Comment on lines
60
to
72
| <template #content> | ||
| <AssetGrid | ||
| :assets="filteredAssets" | ||
| :loading="isLoading" | ||
| :focused-asset-id="focusedAsset?.id" | ||
| :empty-message | ||
| @asset-focus="handleAssetFocus" | ||
| @asset-select="handleAssetSelectAndEmit" | ||
| @asset-deleted="refreshAssets" | ||
| @asset-show-info="handleShowInfo" | ||
| @click="focusedAsset = null" | ||
| /> | ||
| </template> |
Contributor
There was a problem hiding this comment.
Missing .self modifier on click handler.
The @click="focusedAsset = null" on AssetGrid will trigger for any click inside the grid, including clicks on asset cards. This causes the focused asset to be cleared immediately after being set via @asset-focus. This is the same event bubbling issue that was fixed on lines 28 and 56 with .self.
🐛 Proposed fix
<AssetGrid
:assets="filteredAssets"
:loading="isLoading"
:focused-asset-id="focusedAsset?.id"
:empty-message
`@asset-focus`="handleAssetFocus"
`@asset-select`="handleAssetSelectAndEmit"
`@asset-deleted`="refreshAssets"
`@asset-show-info`="handleShowInfo"
- `@click`="focusedAsset = null"
+ `@click.self`="focusedAsset = null"
/>📝 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
| <template #content> | |
| <AssetGrid | |
| :assets="filteredAssets" | |
| :loading="isLoading" | |
| :focused-asset-id="focusedAsset?.id" | |
| :empty-message | |
| @asset-focus="handleAssetFocus" | |
| @asset-select="handleAssetSelectAndEmit" | |
| @asset-deleted="refreshAssets" | |
| @asset-show-info="handleShowInfo" | |
| @click="focusedAsset = null" | |
| /> | |
| </template> | |
| <template `#content`> | |
| <AssetGrid | |
| :assets="filteredAssets" | |
| :loading="isLoading" | |
| :focused-asset-id="focusedAsset?.id" | |
| :empty-message | |
| `@asset-focus`="handleAssetFocus" | |
| `@asset-select`="handleAssetSelectAndEmit" | |
| `@asset-deleted`="refreshAssets" | |
| `@asset-show-info`="handleShowInfo" | |
| `@click.self`="focusedAsset = null" | |
| /> | |
| </template> |
🤖 Prompt for AI Agents
In `@src/platform/assets/components/AssetBrowserModal.vue` around lines 60 - 72,
The click handler on the AssetGrid clears focusedAsset for any inner click and
should only run when the grid backdrop itself is clicked; update the AssetGrid's
click listener to use the `.self` modifier (change `@click`="focusedAsset = null"
to use the self modifier) so clicks that originate from asset cards or child
elements (which emit `@asset-focus/`@asset-select via
handleAssetFocus/handleAssetSelectAndEmit) do not bubble and immediately clear
the focus.
Contributor
Author
There was a problem hiding this comment.
It does not. Clicking on cards works just fine.
Contributor
There was a problem hiding this comment.
@DrJKL, thank you for the clarification! You're right—I apologize for the incorrect concern. The Vue custom events (@asset-focus, @asset-select, etc.) emitted from AssetGrid don't bubble like native DOM events would, and the implementation handles click interactions correctly without the .self modifier causing any issues.
🧠 Learnings used
Learnt from: simula-r
Repo: Comfy-Org/ComfyUI_frontend PR: 7252
File: src/renderer/extensions/vueNodes/components/ImagePreview.vue:151-158
Timestamp: 2025-12-11T03:55:57.926Z
Learning: In src/renderer/extensions/vueNodes/components/ImagePreview.vue and LGraphNode.vue, keyboard navigation for image galleries should respond to node-level focus (via keyEvent injection from LGraphNode), not require focus within the image preview wrapper itself. This allows users to navigate the gallery with arrow keys immediately when the node is focused/selected.
DrJKL
added a commit
that referenced
this pull request
Jan 22, 2026
## Summary Add session download tracking to track which assets were downloaded during the current session. This enables UI features like: - Badge count on "Imported" nav showing newly downloaded assets - Visual indicator on asset cards for recently downloaded items ## Changes - Add `acknowledged` flag to `AssetDownload` interface - Add `unacknowledgedDownloads` computed for filtering - Add `sessionDownloadCount` computed for badge display - Add `isDownloadedThisSession(identifier)` to check individual assets - Add `acknowledgeDownload(identifier)` to mark assets as seen ## Testing - 6 new unit tests covering all session tracking functionality - Run: `pnpm test:unit -- src/stores/assetDownloadStore.test.ts` ## Related - Part of Asset Browser improvements (#8090) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8213-feat-add-session-download-tracking-to-assetDownloadStore-2ef6d73d365081538045e8544d26bafa) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <[email protected]>
DrJKL
added
needs-backport
|
1 similar comment
|
DrJKL
added a commit
that referenced
this pull request
Jan 22, 2026
Cherry-picked from main commit 93e7a4f Adapted PropertiesAccordionItem.vue: removed TransitionCollapse dependency (not in cloud/1.37) Amp-Thread-ID: https://ampcode.com/threads/T-019be3e5-7a8a-7048-bf94-8474134f5467 Co-authored-by: Amp <[email protected]>
DrJKL
added a commit
that referenced
this pull request
Jan 22, 2026
…ser right panel (#8241) ## Summary Backport of #8090 to cloud/1.37 branch. Cherry-picked from main commit 93e7a4f. ## Conflict Resolutions - `src/components/rightSidePanel/layout/PropertiesAccordionItem.vue`: Took PR version but removed `TransitionCollapse` dependency (not present in cloud/1.37). The transition animation is omitted; collapse/expand works without animation. ## Original PR Description Adds an editable Model Info Panel to show and modify asset details in the asset browser. ### Changes - Add `ModelInfoPanel` component with editable display name, description, model type, base models, and tags - Add `updateAssetMetadata` action in `assetsStore` with optimistic cache updates - Add shadcn-vue `Select` components with design system styling - Add utility functions in `assetMetadataUtils` for extracting model metadata - Convert `BaseModalLayout` right panel state to `defineModel` pattern - Add slide-in animation and collapse button for right panel - Add `class` prop to `PropertiesAccordionItem` for custom styling - Fix keyboard handling: Escape in TagsInput/TextArea doesn't close parent modal ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8241-backport-cloud-1-37-feat-assets-add-ModelInfoPanel-for-asset-browser-right-panel-2f06d73d365081ffb57dca42a82349b6) by [Unito](https://www.unito.io) Co-authored-by: Amp <[email protected]>
godwiniheuwa
pushed a commit
to godwiniheuwa/ComfyUI_frontend
that referenced
this pull request
Jan 22, 2026
…8213) ## Summary Add session download tracking to track which assets were downloaded during the current session. This enables UI features like: - Badge count on "Imported" nav showing newly downloaded assets - Visual indicator on asset cards for recently downloaded items ## Changes - Add `acknowledged` flag to `AssetDownload` interface - Add `unacknowledgedDownloads` computed for filtering - Add `sessionDownloadCount` computed for badge display - Add `isDownloadedThisSession(identifier)` to check individual assets - Add `acknowledgeDownload(identifier)` to mark assets as seen ## Testing - 6 new unit tests covering all session tracking functionality - Run: `pnpm test:unit -- src/stores/assetDownloadStore.test.ts` ## Related - Part of Asset Browser improvements (Comfy-Org#8090) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8213-feat-add-session-download-tracking-to-assetDownloadStore-2ef6d73d365081538045e8544d26bafa) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <[email protected]>
godwiniheuwa
pushed a commit
to godwiniheuwa/ComfyUI_frontend
that referenced
this pull request
Jan 22, 2026
…-Org#8090) ## Summary Adds an editable Model Info Panel to show and modify asset details in the asset browser. ## Changes - Add `ModelInfoPanel` component with editable display name, description, model type, base models, and tags - Add `updateAssetMetadata` action in `assetsStore` with optimistic cache updates - Add shadcn-vue `Select` components with design system styling - Add utility functions in `assetMetadataUtils` for extracting model metadata - Convert `BaseModalLayout` right panel state to `defineModel` pattern - Add slide-in animation and collapse button for right panel - Add `class` prop to `PropertiesAccordionItem` for custom styling - Fix keyboard handling: Escape in TagsInput/TextArea doesn't close parent modal ## Testing - Unit tests for `ModelInfoPanel` component - Unit tests for `assetMetadataUtils` functions --------- Co-authored-by: Amp <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: GitHub Action <[email protected]>
This was referenced Jan 23, 2026
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