feat: Media Asset Metadata: Tagging, Properties and Search Integration#11276
Draft
comfygeorge wants to merge 36 commits intoComfy-Org:mainfrom
Draft
feat: Media Asset Metadata: Tagging, Properties and Search Integration#11276comfygeorge wants to merge 36 commits intoComfy-Org:mainfrom
comfygeorge wants to merge 36 commits intoComfy-Org:mainfrom
Conversation
Add a full-screen media asset browser, accessible via an expand button on the Media Assets sidebar header. The dialog reuses existing sidebar components (grid/list views, filter bar, context menu, lightbox, selection, pagination) composed within a BaseModalLayout. Includes a right-side info panel for viewing and editing asset metadata, tags, and description.
Expose focus() from MediaAssetFilterBar and call it onMounted in AssetsSidebarTab, matching the pattern used by BaseWorkflowsSidebarTab.
Composes reka-ui ComboboxRoot + TagsInputRoot following the official pattern. Provides autocomplete suggestions, custom tag creation via a "Create" dropdown item, case-insensitive matching, and alias character normalization. Full keyboard navigation and ARIA accessibility from reka-ui with zero custom code.
…ocomplete Bold matched characters in suggestion dropdown using the existing highlightQuery utility. Style the "Create new tag" option with italic label and tag-chip pill for the tag name, making it visually distinct from regular suggestions.
Add tags to Fuse.js search keys so tags are searchable via fuzzy match. Show tag suggestions with "tag:" prefix in the search dropdown when typing, using SearchAutocomplete with custom styled suggestion items. Aggregate available tags from current assets via useAvailableMediaTags composable.
Stacked image preview with CSS rotations and count badge, used in the media asset info panel when multiple assets are selected. Includes Storybook stories covering single, multi, and placeholder states.
Use TagsInputAutocomplete as the search input in MediaAssetFilterBar with allowCreate=false and tag: prefix in suggestion dropdown. Tags and free text are separate data paths: tag chips filter by exact match, typed text goes to Fuse.js fuzzy search. Add filterTags stage to useMediaAssetFiltering. Expose query as defineModel on TagsInputAutocomplete. Add click-to-select on TagsInputItem chips via injectTagsInputRootContext.
SearchInputWithTags uses SearchInput's variant system for visual parity (search icon, clear button, size variants) with TagsInputAutocomplete's interaction model (tag chips, suggestion dropdown, keyboard nav). Uses the same searchInputVariants and searchInputSizeConfig — no duplication of SearchInput's visual code. SearchInput itself is untouched. Replace TagsInputAutocomplete + SearchInput conditional in MediaAssetFilterBar with a single SearchInputWithTags. Wire tag suggestions and filterTags into the Media Assets sidebar.
Support type:video, type:image etc. alongside tag: filters using prefixed strings. Add chipClass and chipLabel props to SearchInputWithTags for per-chip styling and display text. Type chips render with primary accent colour, tag chips with default border style. MediaAssetFilterBar merges tag + type suggestions, parses prefixes to route selections to filterTags vs mediaTypeFilters. Bidirectional sync with the checkbox filter menu.
Extend the filter menu with a searchable tag checklist below the type checkboxes. Tags section appears when tagSuggestions is provided, with search input and scrollable checkbox list. Bidirectional sync with the search bar chips via MediaAssetFilterBar. Show filter button when tag suggestions are available, not only in cloud mode. Add Storybook stories for isolated development.
Add an info button to the tab bar that appears when assets are selected. Opens a Popover containing MediaAssetInfoPanel — same component as the modal right panel, zero duplication. Supports single and multi-select with full tag editing and API writes. Add compact mode to MediaAssetInfoPanel: transparent background, hidden preview for single assets, reduced preview size for multi-select, no top border on the first section. Add hover-to-spread animation on ImageStack: back layers fan out with wider rotation and offset on mouseenter, smooth 300ms transition.
Use blue primary accent pill styling for the media type field, matching the type chips in the search bar. Only show the filename row when it differs from the display name to reduce clutter for generated assets.
Refocus the search bar after switching between Generated/Imported tabs. Fix SearchInputWithTags.focus() to use a ref on ComboboxAnchor instead of document.querySelector, with instanceof guard to prevent crashes.
… browser - Left panel now shows tag sub-items under Generated/Imported, sorted by count. Clicking a tag filters to that category + tag, syncing with the search bar chips. - Folder view: output count badges on grid cards, clicking drills into job outputs with back button breadcrumb via #contentFilter slot. - Image set expansion: selecting an asset with outputCount > 1 resolves its sub-images and passes them as multi-select to the info panel. - FilterBar: sync parent filterTags → selectedChips so nav-driven tag filters appear as chips in the search bar.
Change px-2 to p-2 on the back button container in folder view, matching the vertical padding used by sibling elements (filter bar, tab list) in the header slot.
Add useMarqueeSelection composable for drag-to-select in VirtualGrid. Wire into AssetsSidebarGridView and AssetsSidebarListView with ref forwarding. Add data-asset-id attribute to MediaAssetCard for element lookup. Add relative positioning to VirtualGrid container for marquee overlay. Add Storybook stories for grid view.
Group both category items (Generated, Imported) at the top, then show a single Tags section below with tags for the currently active category only. Switching categories updates the tag list. Reduces nav clutter and avoids showing irrelevant tags.
Use setPointerCapture to ensure pointerup fires even when the pointer leaves the browser window. Add pointercancel and visibilitychange listeners as safety nets for edge cases.
Add a user_properties object inside user_metadata for storing typed key-value properties (string, boolean, number) on assets. Includes: - PropertiesEditor component with add/edit/delete, autocomplete suggestions from other assets, and type picker for new properties - PropertyRow with type-specific editors (textarea, ToggleGroup, FormattedNumberStepper) using existing UI components - Storybook stories including DynamicSuggestions for cross-asset suggestion testing - usePropertySuggestions composable scanning loaded assets - Zod schema + TypeScript types for UserProperty/UserProperties - Wired into MediaAssetInfoPanel, BrowserModal, and SidebarTab - Fix: Popover.vue custom click-outside handler prevents combobox dropdown interactions from dismissing the parent popover Also includes adjacent changes by another contributor: - isGeneratedOutput detection for tagging/description guards - async flushMetadata with Promise.all for batch writes - generatedOutputReadonly i18n key
Replace the icon-only info button with an icon + text button for better discoverability. Uses existing Button size="sm" variant.
Add a slide-in right panel to the MediaLightbox that shows MediaAssetInfoPanel for the currently viewed asset. Panel state persists via localStorage (Comfy.Lightbox.InfoPanelOpen). - New optional props on MediaLightbox: asset, tagSuggestions, propertySuggestions - Browser modal and sidebar tab pass the current asset to the lightbox - Panel uses rounded container with design system tokens - Nav arrows scoped inside media area, gap-based spacing
Use CSS grid column transition (same pattern as BaseModalLayout) to smoothly animate the info panel. Panel content stays fixed width while the grid column expands/collapses with overflow clipping.
Right-click an asset to open its info panel. In the sidebar this opens the info popover, in the browser modal it opens the right panel. Supports bulk mode with count label. - Add open-info emit to MediaAssetContextMenu (single + bulk items) - Expose Popover open state as v-model for programmatic control - Wire sidebar @Open-Info to set infoPopoverOpen - Wire modal @Open-Info to set focusedAsset + isRightPanelOpen Also includes adjacent i18n change: generatedOutputReadonly wording.
- Guard findIndex -1 in list view story before calling handleAssetClick - Use normalizeForMatch in SearchInputWithTags.filteredSuggestions so aliasChars (e.g. - matching _) work consistently with resolveTag - Only reset query/dropdown when tags are added, not removed, in both SearchInputWithTags and TagsInputAutocomplete watchers - Include folder loading state in browser modal loading computed - Guard stale focusedAsset watcher with request counter to prevent slow responses from overwriting the info panel - Call refreshAssets after bulk delete so removed assets disappear
- SearchInputWithTags: add canCreate prop, #create slot, openDropdown() - useMediaAssetFiltering: add parsePropFilter, matchesPropertyFilter, propertyFilters ref with filter stage in pipeline - MediaAssetFilterBar: context-aware prop: suggestions with operator completions, incomplete chip intercept, amber chip styling - MediaAssetBrowserModal: wire propertyFilters and propertySuggestions
|
@comfygeorge is attempting to deploy a commit to the ComfyUI Team on Vercel. A member of the Team first needs to authorize it. |
dosubot
bot
added
the
size:XXL
Contributor
📝 WalkthroughWalkthroughThis pull request introduces comprehensive asset browsing and filtering capabilities, including a new media asset browser modal, drag-to-select marquee functionality, multi-asset property editing with suggestions, and enhanced tag-based filtering with properties across the assets management system. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Grid as VirtualGrid
participant Marquee as useMarqueeSelection
participant SelectionStore as Asset Selection Store
participant Selection as Selection Manager
User->>Grid: Pointer down on container
Grid->>Marquee: Register pointer event
Marquee->>Marquee: Snapshot current selection<br/>Determine mode (replace/add/toggle)
loop While dragging
User->>Grid: Pointer move
Grid->>Marquee: Update pointer position
Marquee->>Marquee: Compute marquee rect<br/>including scroll offset
Marquee->>Marquee: Query items within rect<br/>[data-asset-id] selector
Marquee->>Marquee: Apply selection mode logic
Marquee->>SelectionStore: onSelectionChange(newIds)
SelectionStore->>Selection: Update selected assets
Selection->>Grid: Render updated state<br/>via overlay slot
end
User->>Grid: Pointer up / Leave grid
Marquee->>Marquee: Finalize selection
Marquee->>Marquee: Install click swallow<br/>to prevent synthetic click
Marquee->>Grid: Hide marquee visuals
sequenceDiagram
participant User
participant Modal as MediaAssetBrowserModal
participant FilterBar as MediaAssetFilterBar
participant Filtering as useMediaAssetFiltering
participant Store as Assets Store
participant Panel as MediaAssetInfoPanel
User->>Modal: Open asset browser
Modal->>Store: Fetch initial assets
Modal->>Modal: Render list/grid view
User->>FilterBar: Select tag/property filters
FilterBar->>FilterBar: Build chips from selections
FilterBar->>Filtering: Apply tag/property/type filters
Filtering->>Store: Query filtered assets
Filtering->>Modal: Return filtered results
Modal->>Modal: Re-render asset list
User->>Modal: Click asset to select
Modal->>Store: Update selection state
Modal->>Panel: Load asset metadata
Panel->>Panel: Render preview + properties
User->>Panel: Edit property/tag
Panel->>Store: updateAssetMetadata()
Store->>Modal: Propagate changes
Modal->>Modal: Update displayed asset
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
🎨 Storybook:
|
🎭 Playwright: ⏳ Running... |
![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 (2)
src/components/sidebar/tabs/queue/MediaLightbox.vue (1)
150-157:⚠️ Potential issue | 🟠 MajorOnly capture the previously focused element when the lightbox opens.
This watcher overwrites
previouslyFocusedElementon everyactiveIndexchange. After the user navigates next/previous, closing returns focus to the dialog or nav button instead of the original opener.Suggested fix
watch( () => activeIndex, (index) => { + const wasOpen = galleryVisible.value galleryVisible.value = index !== -1 - if (index !== -1) { + if (index !== -1 && !wasOpen) { previouslyFocusedElement = document.activeElement as HTMLElement | null void nextTick(() => dialogRef.value?.focus()) } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/sidebar/tabs/queue/MediaLightbox.vue` around lines 150 - 157, The watcher currently overwrites previouslyFocusedElement on every activeIndex change; change the watcher signature to (index, oldIndex) and only capture previouslyFocusedElement when opening (oldIndex === -1 && index !== -1) so navigation (next/previous) doesn't replace the original opener reference; keep the galleryVisible assignment and the nextTick(() => dialogRef.value?.focus()) call as-is when opening.src/components/sidebar/tabs/AssetsSidebarTab.vue (1)
600-610:⚠️ Potential issue | 🟡 MinorMove
filterBarRefdeclaration before theactiveTabwatcher.Line 610 references
filterBarRefin awatch(..., { immediate: true })callback. Although the actual access is deferred vianextTick()(so no runtime error occurs), declaring the ref at line 749 creates unnecessary fragility and violates expected declaration-before-use ordering. Moveconst filterBarRef = ref<InstanceType<typeof MediaAssetFilterBar>>()to before the watcher (around line 555) for clarity and safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/sidebar/tabs/AssetsSidebarTab.vue` around lines 600 - 610, The watcher on activeTab references filterBarRef inside a nextTick callback, but filterBarRef is declared later; move the declaration const filterBarRef = ref<InstanceType<typeof MediaAssetFilterBar>>() to above the activeTab watcher (e.g., around where other refs are initialized) so the ref is declared before use; ensure imports/typing for MediaAssetFilterBar remain correct and that the watcher still calls nextTick(() => filterBarRef.value?.focus()) unchanged.
🧹 Nitpick comments (9)
src/platform/assets/schemas/userPropertySchema.test.ts (2)
30-32: Remove duplicate test case.This test for
coverageOpacityClass(2, 5)expecting'opacity-55'is identical to the test at lines 18-20. Consider removing it or testing a different boundary value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/assets/schemas/userPropertySchema.test.ts` around lines 30 - 32, The test file contains a duplicated spec for coverageOpacityClass(2, 5) expecting 'opacity-55'—remove the duplicate test (the one matching the earlier spec) or replace it with a different boundary case (e.g., test coverageOpacityClass(1, 5) or coverageOpacityClass(3, 5)) so each assertion is unique; look for the duplicate calls to coverageOpacityClass in the test suite to update or delete the redundant it(...) block.
5-33: Consider adding edge case test fortotal = 0.The current tests don't cover the case where
totalis 0, which would cause division by zero. Adding a test would help document the expected behavior (and catch the issue flagged inuserPropertySchema.ts).♻️ Suggested additional test
it('handles zero total gracefully', () => { expect(coverageOpacityClass(0, 0)).toBe('opacity-40') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/assets/schemas/userPropertySchema.test.ts` around lines 5 - 33, Add a unit test for the edge case where total is 0 and update coverageOpacityClass to guard against division by zero: add a test like expect(coverageOpacityClass(0, 0)).toBe('opacity-40') in the userPropertySchema.test.ts suite, and in the coverageOpacityClass function ensure it returns 'opacity-40' (or the same fallback used for <=33% coverage) when total === 0 before performing any division or ratio calculations.src/platform/assets/composables/useMultiPropertyEditing.test.ts (1)
9-19: Consider usingsatisfies AssetItemfor type validation in test helper.The
createAssethelper returns a partial object. Usingsatisfies AssetItemwould provide compile-time validation that the mock shape remains compatible with the interface as it evolves.♻️ Suggested improvement
function createAsset( id: string, userProperties: UserProperties = {} ): AssetItem { - return { + return { id, name: `${id}.png`, tags: [], user_metadata: { user_properties: userProperties } - } + } satisfies AssetItem }Based on learnings: "In test files matching **/*.test.ts under src, when creating test helper functions that construct mock objects implementing an interface (e.g., AssetItem), prefer using satisfies InterfaceType for shape validation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/assets/composables/useMultiPropertyEditing.test.ts` around lines 9 - 19, The createAsset test helper currently returns a loose object that can drift from the AssetItem interface; update the helper to use a TypeScript "satisfies AssetItem" assertion on the returned object so the compiler verifies the mock shape (keep the same properties id, name, tags, user_metadata.user_properties) while preserving the flexible type returned by createAsset; locate the createAsset function and change its return expression to use "satisfies AssetItem" to get compile-time shape validation without altering runtime behavior.src/components/ui/tags-input/TagsInputItem.vue (1)
21-32: The[dir]selector may be fragile for locating the tags input root.The
el.closest('[dir]')relies on thedirattribute being present on the TagsInputRoot. While this works with reka-ui's default behavior, it could match an unrelated ancestor if another element has adirattribute. Consider using a more specific selector or data attribute if stability becomes a concern.This is acceptable for now since reka-ui's TagsInputRoot sets
dir, but worth noting for future maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/tags-input/TagsInputItem.vue` around lines 21 - 32, handleClick currently uses el.closest('[dir]') to find the tags input root which can erroneously match unrelated ancestors; change the selector to a more specific hook (e.g. a data attribute or a unique class on TagsInputRoot) and update the lookup in handleClick (the el.closest call) to use that new selector so it reliably finds the correct root before querying the input; keep rest of logic (nextTick, input?.focus, updating context.selectedElement via itemRef) unchanged.src/platform/assets/composables/useMediaAssetFiltering.ts (1)
29-56: Consider adding explicit handling for the~operator on non-string types.The
matchesPropertyFilterfunction handles~(contains) only for strings at line 52. For numbers and booleans,~silently returnsfalse(falls through to line 55). This may be intentional, but documenting or explicitly handling this would improve clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/assets/composables/useMediaAssetFiltering.ts` around lines 29 - 56, matchesPropertyFilter currently only applies the '~' (contains) operator for strings and silently falls through for numbers/booleans; update the function (matchesPropertyFilter) to explicitly handle the '~' operator for non-string types by returning false immediately when op === '~' and typeof value !== 'string' (or alternatively normalize/convert types if you prefer), so behavior is explicit and easier to read/maintain; ensure the new check is placed before the type-specific branches and keeps existing semantics for string '~' matching.src/platform/assets/components/properties/PropertiesEditor.test.ts (1)
29-31: Consider a more robust textarea query.The custom name matcher
(_, el) => (el as HTMLElement).tagName === 'TEXTAREA'is unconventional. If the component has a single textarea,container.querySelector('textarea')or using a label association would be more idiomatic.That said, this works correctly for the current test scenario.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/assets/components/properties/PropertiesEditor.test.ts` around lines 29 - 31, The test uses a non-idiomatic custom name matcher in the query within(container).getByRole('textbox', { name: (_, el) => (el as HTMLElement).tagName === 'TEXTAREA' }); replace that matcher with a more robust approach: locate the textarea directly (e.g., container.querySelector('textarea')) or use a semantic RTL query such as getByRole('textbox') when there is only one textarea, or getByLabelText if the textarea has an associated label; update the test in PropertiesEditor.test.ts to use one of these alternatives so the query is clearer and more maintainable.src/platform/assets/components/mediaInfo/ImageStack.vue (1)
69-69: Unused computed property.
containerStylealways returns an empty object and could be removed along with its usage on line 4. This appears to be scaffolding for future customization that wasn't needed.♻️ Proposed fix
<div class="relative aspect-square" - :style="containerStyle" `@mouseenter`="isHovered = true"-const containerStyle = computed(() => ({})) - const isHovered = ref(false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/assets/components/mediaInfo/ImageStack.vue` at line 69, The computed property containerStyle is unused (always returns an empty object) so remove the containerStyle definition and any bindings referencing it (e.g., :style="containerStyle") in ImageStack.vue; also remove the now-unused computed import if it becomes unused and optionally replace the removed binding with a direct style or class if layout relied on it or add a brief TODO comment where future dynamic styles will go.src/platform/assets/components/MediaAssetFilterBar.vue (1)
203-223: Consider adding error recovery for the anti-reentrancy flag.The
interceptingflag guards against re-entrancy during the chip interception flow. However, if an error occurs within thenextTickcallbacks,interceptingremainstrueindefinitely, potentially blocking future interceptions.🔧 Optional: Add try-finally for robustness
watch(selectedChips, (newChips, oldChips) => { if (!oldChips || intercepting) return const added = newChips.filter((c) => !oldChips.includes(c)) const incomplete = added.find( (c) => c.startsWith('prop:') && !parsePropFilter(c) ) if (incomplete) { intercepting = true selectedChips.value = newChips.filter((c) => c !== incomplete) - void nextTick(() => { - searchModel.value = incomplete - void nextTick(() => { - searchRef.value?.focus() - searchRef.value?.openDropdown() - intercepting = false - }) - }) + void nextTick(async () => { + try { + searchModel.value = incomplete + await nextTick() + searchRef.value?.focus() + searchRef.value?.openDropdown() + } finally { + intercepting = false + } + }) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/assets/components/MediaAssetFilterBar.vue` around lines 203 - 223, The intercepting flag in the watch handler (watch on selectedChips) can be left true if an exception occurs inside the nested nextTick callbacks; wrap the interception flow so intercepting is always reset (use try/finally semantics around the async work that sets searchModel and focuses/opens via searchRef) — e.g., capture the interception early, run the nextTick sequence inside a try block and ensure intercepting = false in a finally block, referencing selectedChips, intercepting, nextTick, searchModel and searchRef to locate and update the logic.src/platform/assets/components/MediaAssetBrowserModal.vue (1)
507-525: Consider extracting the ResultItemImpl adapter to a utility function.The
Object.definePropertypattern to add aurlgetter toResultItemImplworks but is somewhat unconventional. This could be extracted to a utility function for clarity and reuse if similar adapters are needed elsewhere.♻️ Optional: Extract asset-to-gallery-item adapter
// In a utility file like src/platform/assets/utils/galleryAdapter.ts function assetToGalleryItem(asset: AssetItem): ResultItemImpl { const mediaType = getMediaTypeFromFilename(asset.name) const item = new ResultItemImpl({ filename: asset.name, subfolder: '', type: 'output', nodeId: '0', mediaType: mediaType === 'image' ? 'images' : mediaType }) Object.defineProperty(item, 'url', { get: () => asset.preview_url || '', configurable: true }) return item }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/assets/components/MediaAssetBrowserModal.vue` around lines 507 - 525, Extract the adapter logic that builds ResultItemImpl into a reusable function (e.g., assetToGalleryItem) and replace the inline mapping used by galleryItems with a map that calls this function; specifically move the usage of getMediaTypeFromFilename, the ResultItemImpl construction, and the Object.defineProperty that defines the url getter (using asset.preview_url) into the new assetToGalleryItem(asset: AssetItem): ResultItemImpl utility so the galleryItems computed simply does previewableAssets.value.map(assetToGalleryItem).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/sidebar/tabs/AssetsSidebarTab.vue`:
- Around line 510-526: The watcher for selectedAssets can apply stale async
results to expandedSelectedAssets because resolveOutputAssetItems() is awaited;
fix by capturing a guard token (e.g., local counter or the asset id) before
calling resolveOutputAssetItems and after the await verify the token still
matches the current selection; specifically, in the watch callback for
selectedAssets, record a unique identifier (asset.id or an incrementing
requestId) prior to awaiting resolveOutputAssetItems(assetMetadata, ...) and
only assign expandedSelectedAssets.value if the identifier still matches and
selectedAssets still contains that same asset; apply the same guard when
clearing expandedSelectedAssets to avoid race conditions with late promises.
- Around line 603-607: When resetting UI state on tab change (in the same block
that calls clearSelection() and clears searchQuery.value and filterTags.value),
also reset the propertyFilters ref so hidden property chips don't persist across
tabs; locate the propertyFilters ref used in AssetsSidebarTab.vue and set it to
an empty collection (same type as currently used) at the same point you clear
searchQuery and filterTags so Generated/Imported tabs start with no property
filters applied.
In `@src/components/ui/Popover.vue`:
- Around line 37-53: Replace the current watch(open, ...) pattern with a
watchEffect that registers the pointerdown listener only when open.value is
true, store the requestAnimationFrame id and the cleanup function returned by
useEventListener in local variables, and call onCleanup to cancel the RAF (via
cancelAnimationFrame) and invoke cleanup() whenever the effect stops (popover
closes, slot's close(), a menu selection, or component unmount); this ensures
the listener installed by useEventListener is always removed and the pending RAF
is canceled before re-opening.
In `@src/platform/assets/components/properties/AddPropertyRow.vue`:
- Around line 74-92: The "Create" ComboboxItem uses the raw keyQuery when
invoking selection, allowing leading/trailing whitespace to create malformed
keys; update the selection path to trim whitespace before handling by calling
handleKeySelected with the trimmed key (e.g., use keyQuery.trim() or
keyQuery.value.trim() depending on whether keyQuery is a ref) wherever the
create item is selected (reference ComboboxItem and handleKeySelected), and
apply the same trim fix to the other create-item usage around the 248-257 block
so both keyboard Enter and click selection normalize keys consistently.
- Around line 200-203: The labels in the propertyTypes array are hardcoded;
update AddPropertyRow.vue to use vue-i18n in the composition API: import and
call useI18n() in setup, then replace the label strings in the propertyTypes
const with calls to t('properties.typeText'), t('properties.typeNumber'), and
t('properties.typeBoolean') (or the exact keys you added), ensuring
propertyTypes remains exported/used the same way so the type-picker displays
localized labels.
In `@src/platform/assets/schemas/assetSchema.ts`:
- Around line 106-107: The user_properties field currently uses
z.record(z.unknown()) which permits malformed values; change its validator to
use the existing typed property schema used elsewhere (e.g., propertySchema or
typedPropertySchema) so each record value is validated as a typed property
object; update user_properties to z.record(propertySchema).optional() (or
z.record(typedPropertySchema).optional()) and import the correct schema symbol
so payloads like { foo: null } are rejected.
In `@src/platform/assets/schemas/userPropertySchema.ts`:
- Around line 72-78: The function coverageOpacityClass has no guard for total==0
which makes ratio = count/total invalid; update coverageOpacityClass to first
handle non-positive total (e.g., if total <= 0) and return a safe default class
(for example return 'font-semibold' when count >= total or when count > 0 and
total <= 0, otherwise return the lowest-opacity default like 'opacity-40'), then
continue with the existing ratio logic; ensure you modify only
coverageOpacityClass so callers need no changes.
In `@src/platform/assets/services/assetService.ts`:
- Around line 764-774: The listAllTags function currently returns [] on non-OK
responses and returns raw data without validation; update it to mirror other API
methods by (1) throwing an Error if api.fetchApi(`/tags?...`) returns !res.ok,
(2) define a Zod schema (e.g., TagsListSchema) that validates { tags: Array<{
name: string; count: number }> }, (3) parse the JSON with
TagsListSchema.safeParse(data) and if parse fails throw
fromZodError(parse.error), and (4) return the validated tags array (data.tags)
instead of unvalidated data or silent empty array; reference listAllTags,
api.fetchApi, TagsListSchema (new), safeParse, and fromZodError when making the
changes.
---
Outside diff comments:
In `@src/components/sidebar/tabs/AssetsSidebarTab.vue`:
- Around line 600-610: The watcher on activeTab references filterBarRef inside a
nextTick callback, but filterBarRef is declared later; move the declaration
const filterBarRef = ref<InstanceType<typeof MediaAssetFilterBar>>() to above
the activeTab watcher (e.g., around where other refs are initialized) so the ref
is declared before use; ensure imports/typing for MediaAssetFilterBar remain
correct and that the watcher still calls nextTick(() =>
filterBarRef.value?.focus()) unchanged.
In `@src/components/sidebar/tabs/queue/MediaLightbox.vue`:
- Around line 150-157: The watcher currently overwrites previouslyFocusedElement
on every activeIndex change; change the watcher signature to (index, oldIndex)
and only capture previouslyFocusedElement when opening (oldIndex === -1 && index
!== -1) so navigation (next/previous) doesn't replace the original opener
reference; keep the galleryVisible assignment and the nextTick(() =>
dialogRef.value?.focus()) call as-is when opening.
---
Nitpick comments:
In `@src/components/ui/tags-input/TagsInputItem.vue`:
- Around line 21-32: handleClick currently uses el.closest('[dir]') to find the
tags input root which can erroneously match unrelated ancestors; change the
selector to a more specific hook (e.g. a data attribute or a unique class on
TagsInputRoot) and update the lookup in handleClick (the el.closest call) to use
that new selector so it reliably finds the correct root before querying the
input; keep rest of logic (nextTick, input?.focus, updating
context.selectedElement via itemRef) unchanged.
In `@src/platform/assets/components/MediaAssetBrowserModal.vue`:
- Around line 507-525: Extract the adapter logic that builds ResultItemImpl into
a reusable function (e.g., assetToGalleryItem) and replace the inline mapping
used by galleryItems with a map that calls this function; specifically move the
usage of getMediaTypeFromFilename, the ResultItemImpl construction, and the
Object.defineProperty that defines the url getter (using asset.preview_url) into
the new assetToGalleryItem(asset: AssetItem): ResultItemImpl utility so the
galleryItems computed simply does
previewableAssets.value.map(assetToGalleryItem).
In `@src/platform/assets/components/MediaAssetFilterBar.vue`:
- Around line 203-223: The intercepting flag in the watch handler (watch on
selectedChips) can be left true if an exception occurs inside the nested
nextTick callbacks; wrap the interception flow so intercepting is always reset
(use try/finally semantics around the async work that sets searchModel and
focuses/opens via searchRef) — e.g., capture the interception early, run the
nextTick sequence inside a try block and ensure intercepting = false in a
finally block, referencing selectedChips, intercepting, nextTick, searchModel
and searchRef to locate and update the logic.
In `@src/platform/assets/components/mediaInfo/ImageStack.vue`:
- Line 69: The computed property containerStyle is unused (always returns an
empty object) so remove the containerStyle definition and any bindings
referencing it (e.g., :style="containerStyle") in ImageStack.vue; also remove
the now-unused computed import if it becomes unused and optionally replace the
removed binding with a direct style or class if layout relied on it or add a
brief TODO comment where future dynamic styles will go.
In `@src/platform/assets/components/properties/PropertiesEditor.test.ts`:
- Around line 29-31: The test uses a non-idiomatic custom name matcher in the
query within(container).getByRole('textbox', { name: (_, el) => (el as
HTMLElement).tagName === 'TEXTAREA' }); replace that matcher with a more robust
approach: locate the textarea directly (e.g.,
container.querySelector('textarea')) or use a semantic RTL query such as
getByRole('textbox') when there is only one textarea, or getByLabelText if the
textarea has an associated label; update the test in PropertiesEditor.test.ts to
use one of these alternatives so the query is clearer and more maintainable.
In `@src/platform/assets/composables/useMediaAssetFiltering.ts`:
- Around line 29-56: matchesPropertyFilter currently only applies the '~'
(contains) operator for strings and silently falls through for numbers/booleans;
update the function (matchesPropertyFilter) to explicitly handle the '~'
operator for non-string types by returning false immediately when op === '~' and
typeof value !== 'string' (or alternatively normalize/convert types if you
prefer), so behavior is explicit and easier to read/maintain; ensure the new
check is placed before the type-specific branches and keeps existing semantics
for string '~' matching.
In `@src/platform/assets/composables/useMultiPropertyEditing.test.ts`:
- Around line 9-19: The createAsset test helper currently returns a loose object
that can drift from the AssetItem interface; update the helper to use a
TypeScript "satisfies AssetItem" assertion on the returned object so the
compiler verifies the mock shape (keep the same properties id, name, tags,
user_metadata.user_properties) while preserving the flexible type returned by
createAsset; locate the createAsset function and change its return expression to
use "satisfies AssetItem" to get compile-time shape validation without altering
runtime behavior.
In `@src/platform/assets/schemas/userPropertySchema.test.ts`:
- Around line 30-32: The test file contains a duplicated spec for
coverageOpacityClass(2, 5) expecting 'opacity-55'—remove the duplicate test (the
one matching the earlier spec) or replace it with a different boundary case
(e.g., test coverageOpacityClass(1, 5) or coverageOpacityClass(3, 5)) so each
assertion is unique; look for the duplicate calls to coverageOpacityClass in the
test suite to update or delete the redundant it(...) block.
- Around line 5-33: Add a unit test for the edge case where total is 0 and
update coverageOpacityClass to guard against division by zero: add a test like
expect(coverageOpacityClass(0, 0)).toBe('opacity-40') in the
userPropertySchema.test.ts suite, and in the coverageOpacityClass function
ensure it returns 'opacity-40' (or the same fallback used for <=33% coverage)
when total === 0 before performing any division or ratio calculations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2a67bca0-fb28-4d3a-913e-0a5ef423a2fc
📒 Files selected for processing (44)
src/components/common/VirtualGrid.vuesrc/components/sidebar/tabs/AssetsSidebarGridView.stories.tssrc/components/sidebar/tabs/AssetsSidebarGridView.vuesrc/components/sidebar/tabs/AssetsSidebarListView.stories.tssrc/components/sidebar/tabs/AssetsSidebarListView.vuesrc/components/sidebar/tabs/AssetsSidebarTab.vuesrc/components/sidebar/tabs/queue/MediaLightbox.vuesrc/components/ui/Popover.vuesrc/components/ui/search-input/PropertySearch.stories.tssrc/components/ui/search-input/SearchAutocompleteTagSuggestions.stories.tssrc/components/ui/search-input/SearchInputWithTags.stories.tssrc/components/ui/search-input/SearchInputWithTags.vuesrc/components/ui/tags-input/TagsInputAutocomplete.stories.tssrc/components/ui/tags-input/TagsInputAutocomplete.vuesrc/components/ui/tags-input/TagsInputItem.vuesrc/composables/useMarqueeSelection.tssrc/locales/en/main.jsonsrc/platform/assets/components/MediaAssetBrowserModal.vuesrc/platform/assets/components/MediaAssetCard.vuesrc/platform/assets/components/MediaAssetContextMenu.vuesrc/platform/assets/components/MediaAssetFilterBar.vuesrc/platform/assets/components/MediaAssetFilterMenu.stories.tssrc/platform/assets/components/MediaAssetFilterMenu.vuesrc/platform/assets/components/mediaInfo/ImageStack.stories.tssrc/platform/assets/components/mediaInfo/ImageStack.vuesrc/platform/assets/components/mediaInfo/MediaAssetInfoPanel.vuesrc/platform/assets/components/modelInfo/ModelInfoField.vuesrc/platform/assets/components/properties/AddPropertyRow.vuesrc/platform/assets/components/properties/PropertiesEditor.stories.tssrc/platform/assets/components/properties/PropertiesEditor.test.tssrc/platform/assets/components/properties/PropertiesEditor.vuesrc/platform/assets/components/properties/PropertyRow.vuesrc/platform/assets/components/properties/PropertyRowActions.vuesrc/platform/assets/composables/useAvailableMediaTags.tssrc/platform/assets/composables/useMediaAssetBrowserDialog.tssrc/platform/assets/composables/useMediaAssetFiltering.tssrc/platform/assets/composables/useMultiPropertyEditing.test.tssrc/platform/assets/composables/useMultiPropertyEditing.tssrc/platform/assets/composables/usePropertySuggestions.tssrc/platform/assets/schemas/assetSchema.tssrc/platform/assets/schemas/userPropertySchema.test.tssrc/platform/assets/schemas/userPropertySchema.tssrc/platform/assets/services/assetService.tssrc/stores/assetsStore.ts
Comment on lines
+37
to
+53
| watch(open, (isOpen) => { | ||
| if (!isOpen) return | ||
| requestAnimationFrame(() => { | ||
| const cleanup = useEventListener( | ||
| document, | ||
| 'pointerdown', | ||
| (e: PointerEvent) => { | ||
| const target = e.target as HTMLElement | ||
| if (target.closest('[data-reka-popper-content-wrapper]')) return | ||
| if (target.closest('[role="listbox"]')) return | ||
| if (target.closest('[role="option"]')) return | ||
| open.value = false | ||
| cleanup() | ||
| } | ||
| ) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -name "Popover.vue" src/components/ui/Repository: Comfy-Org/ComfyUI_frontend
Length of output: 240
🏁 Script executed:
cat -n src/components/ui/Popover.vue | head -80Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3078
🏁 Script executed:
rg "open\.value\s*=" src/components/ui/Popover.vue -A 2 -B 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 231
🏁 Script executed:
rg "watch.*open" src/components/ui/Popover.vue -A 20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 607
🏁 Script executed:
rg "slot.*close" src/components/ui/Popover.vue -B 2 -A 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 269
🏁 Script executed:
rg "useEventListener" --type ts --type vue -A 5 -B 2 | head -50Repository: Comfy-Org/ComfyUI_frontend
Length of output: 97
🏁 Script executed:
rg "useEventListener" -A 5 -B 2 | head -80Repository: Comfy-Org/ComfyUI_frontend
Length of output: 5588
🏁 Script executed:
cat -n src/components/ui/Popover.vue | sed -n '32,80p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2295
🏁 Script executed:
web_search
VueUse useEventListener cleanup behavior return valueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 165
🌐 Web query:
VueUse useEventListener cleanup function behavior return value
💡 Result:
The VueUse useEventListener function returns a cleanup function (typed as Fn, which is => void) that, when called, manually unregisters the registered event listener(s) by invoking removeEventListener on the target(s) with the corresponding event(s), listener(s), and options. Key behaviors: - Automatically registers listeners via addEventListener reactively (using watchImmediate on targets, events, listeners, options). - Automatically cleans up on Vue scope disposal (e.g., component unmount) via onCleanup, calling all individual cleanup functions. - Manual cleanup: Calling the returned function triggers immediate removal of current listeners. Safe to call multiple times (subsequent calls are no-ops after cleanup). - Handles multiple targets/events/listeners by collecting an array of individual cleanup fns and invoking them all. - Reactive changes (e.g., target ref update) trigger cleanup of old listeners before registering new ones. - Options are cloned on removal to prevent mutation issues (post-fix for issues like #2825). Example from docs: const cleanup = useEventListener(document, 'keydown', (e) => { console.log(e.key) }) cleanup // Unregisters the listener Source code confirms: Returns watchImmediate result, which provides onCleanup for disposals; manual call invokes the current cleanups array.
Citations:
- 1: https://vueuse.org/useEventListener
- 2: https://vueuse.org/core/useeventlistener/
- 3: https://v13-8-0.vueuse.org/core/useEventListener/
- 4: http://vueuse.org/core/useEventListener/
- 5: useEventListener can be confused to not clean up events. vueuse/vueuse#2825
- 6: https://github.com/vueuse/vueuse/blob/main/packages/core/useEventListener/index.ts
🏁 Script executed:
cat -n src/components/ui/Popover.vue | sed -n '56,90p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1794
🏁 Script executed:
rg "PopoverRoot|open\.value|defineModel.*open" src/components/ui/Popover.vue -B 2 -A 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 503
🏁 Script executed:
rg "watchEffect|onCleanup" src/components/ui/ -B 2 -A 3Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
find src/components -name "*.vue" -exec grep -l "useEventListener" {} \;Repository: Comfy-Org/ComfyUI_frontend
Length of output: 593
🏁 Script executed:
cat -n src/components/graph/NodeContextMenu.vue | grep -A 15 "useEventListener"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1316
🏁 Script executed:
cat -n src/components/searchbox/NodeSearchBoxPopover.vue | grep -A 20 "watch\|useEventListener"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4838
🏁 Script executed:
cat -n src/components/ui/Popover.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 4335
🏁 Script executed:
rg "onEscapeKeyDown\|KeyboardEvent" src/components/ui/Popover.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 52
Clean up the document listener on every close/unmount path.
The pointerdown listener is only removed after an outside click. If the popover closes via the slot's close function (line 74), menu item selection (line 96), or component unmount, that listener remains attached. Opening the popover again registers an additional listener, causing accumulation. Since @interact-outside.prevent disables reka-ui's built-in dismissal, this listener leak directly impacts the main close path. Use watchEffect with onCleanup to ensure the listener is always removed when the popover closes, and cancel any pending requestAnimationFrame callback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/Popover.vue` around lines 37 - 53, Replace the current
watch(open, ...) pattern with a watchEffect that registers the pointerdown
listener only when open.value is true, store the requestAnimationFrame id and
the cleanup function returned by useEventListener in local variables, and call
onCleanup to cancel the RAF (via cancelAnimationFrame) and invoke cleanup()
whenever the effect stops (popover closes, slot's close(), a menu selection, or
component unmount); this ensures the listener installed by useEventListener is
always removed and the pending RAF is canceled before re-opening.
Comment on lines
+764
to
+774
| async function listAllTags( | ||
| options?: { limit?: number } | ||
| ): Promise<{ name: string; count: number }[]> { | ||
| const params = new URLSearchParams({ | ||
| limit: String(options?.limit ?? 200), | ||
| order: 'count_desc' | ||
| }) | ||
| const res = await api.fetchApi(`/tags?${params}`) | ||
| if (!res.ok) return [] | ||
| const data = await res.json() | ||
| return data.tags ?? [] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file structure and size
wc -l src/platform/assets/services/assetService.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 118
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '764,774p' src/platform/assets/services/assetService.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 442
🏁 Script executed:
# Search for safeParse usage in the asset service file to understand the pattern
rg 'safeParse' src/platform/assets/services/assetService.ts -B 2 -A 5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1597
🏁 Script executed:
# Look for other tag-related functions and error handling patterns
rg -A 10 'function.*[Tt]ag' src/platform/assets/services/assetService.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1414
🏁 Script executed:
# Search for tagsOperationResultSchema definition to understand validation scope
rg 'tagsOperationResultSchema' src/platform/assets/services/assetService.ts -B 5 -A 10Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1292
🏁 Script executed:
# Check if there's a schema definition file for these schemas
fd -e ts -path '*/schemas/*' | head -20Repository: Comfy-Org/ComfyUI_frontend
Length of output: 238
🏁 Script executed:
# Look for error handling patterns in other API methods to understand the convention
rg 'throw.*Error|throw from' src/platform/assets/services/assetService.ts | head -15Repository: Comfy-Org/ComfyUI_frontend
Length of output: 480
🏁 Script executed:
# Look at the assetSchema file to see what schemas are defined
cat -n src/platform/assets/schemas/assetSchema.ts | head -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4105
🏁 Script executed:
# Search for any schema related to listAllTags or tags list response
rg 'tags|Tags' src/platform/assets/schemas/assetSchema.ts -B 2 -A 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1317
🏁 Script executed:
# Check what fromZodError does and where it comes from
rg 'fromZodError|import.*fromZodError' src/platform/assets/services/assetService.ts -B 2 -A 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 827
🏁 Script executed:
# Check if there's a schema definition for tag list response anywhere
rg 'name.*count|count.*name' src/platform/assets/schemas/ -B 2 -A 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
# Search the entire codebase for tag schema definitions we may have missed
rg -t ts 'z\.object.*name.*count|z\.object.*count.*name'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
# Look at the full context of listAllTags and similar methods side-by-side
sed -n '740,800p' src/platform/assets/services/assetService.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1527
🏁 Script executed:
# Check the full assetSchema.ts to see if there's a missing schema for tag list
cat -n src/platform/assets/schemas/assetSchema.ts | tail -50Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2365
🏁 Script executed:
# Search for any reference to how tags list response is structured
rg 'tagsOperationResultSchema|tags.*schema|tag.*list' src/platform/assets/schemas/assetSchema.ts -B 1 -A 3Repository: Comfy-Org/ComfyUI_frontend
Length of output: 434
🏁 Script executed:
# Check how listAllTags is used in the codebase to understand expected behavior
rg 'listAllTags' --type ts -B 2 -A 5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1160
🏁 Script executed:
# Final check: see how other API methods in the file handle errors to confirm pattern
rg -A 8 'if \(!res\.ok\)' src/platform/assets/services/assetService.ts | head -40Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1095
Add schema validation and proper error handling to listAllTags.
This method silently returns [] on network/server errors and raw data.tags without validation, unlike all other API methods in this file. This masks outages in the tag filter UI and can leak malformed responses. Add a Zod schema for the tags list response and follow the established pattern: check !res.ok to throw an error, parse the response with safeParse(), and throw fromZodError() on validation failure instead of failing open.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/assets/services/assetService.ts` around lines 764 - 774, The
listAllTags function currently returns [] on non-OK responses and returns raw
data without validation; update it to mirror other API methods by (1) throwing
an Error if api.fetchApi(`/tags?...`) returns !res.ok, (2) define a Zod schema
(e.g., TagsListSchema) that validates { tags: Array<{ name: string; count:
number }> }, (3) parse the JSON with TagsListSchema.safeParse(data) and if parse
fails throw fromZodError(parse.error), and (4) return the validated tags array
(data.tags) instead of unvalidated data or silent empty array; reference
listAllTags, api.fetchApi, TagsListSchema (new), safeParse, and fromZodError
when making the changes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
src/components/ui/search-input/SearchInputWithTags.test.ts (3)
4-4: Remove the commented-out import.Keep the file clean by deleting this dead line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/search-input/SearchInputWithTags.test.ts` at line 4, Remove the dead commented-import line from the test file: delete the commented-out "// import { nextTick } from 'vue'" in SearchInputWithTags.test.ts so the file contains only active imports and test code (no commented-out imports).
26-27: TyperenderComponentprops to avoid loose test inputs.The helper should accept typed partial props instead of an untyped object.
💡 Proposed change
+type SearchInputWithTagsProps = + InstanceType<typeof SearchInputWithTags>['$props'] + - function renderComponent(props = {}) { + function renderComponent( + props: Partial<SearchInputWithTagsProps> = {} + ) {Based on learnings: In test files, prefer typing helper props as
Partial<ComponentProps<typeof Component>>for stronger type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/search-input/SearchInputWithTags.test.ts` around lines 26 - 27, The test helper renderComponent currently accepts an untyped props object—change its parameter type to a typed partial so callers get proper prop typings; update renderComponent(props = {}) to accept props: Partial<ComponentProps<typeof SearchInputWithTags>> (import ComponentProps from React if needed) and propagate that typed props into the render call so tests use strongly typed inputs for SearchInputWithTags.
136-147: Model-separation test is too shallow for its intent.This currently re-checks chip rendering without asserting model independence. Consider simulating query input and asserting only
onUpdate:queryis emitted for typing (and no unintended tag model mutation).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/search-input/SearchInputWithTags.test.ts` around lines 136 - 147, The test "keeps tag model separate from query model" currently only checks chip rendering; update it to simulate typing into the query input (use renderComponent to mount and screen.getByRole/getByPlaceholderText to find the input), type characters to trigger updates, then assert that onUpdate:query (onUpdateQuery) was called with the typed value while onUpdate:modelValue (onUpdateModelValue) was not called and the tag count (screen.getAllByTestId('tag-chip').length) remains unchanged; this proves query edits don't mutate the tag model.src/components/ui/tags-input/TagsInputAutocomplete.test.ts (1)
25-26: TightenrenderComponentprop typing for safer tests.
props = {}makes the helper accept arbitrary keys silently. Type this as partial component props so invalid test inputs fail fast.💡 Proposed change
+type TagsInputAutocompleteProps = + InstanceType<typeof TagsInputAutocomplete>['$props'] + - function renderComponent(props = {}) { + function renderComponent( + props: Partial<TagsInputAutocompleteProps> = {} + ) {Based on learnings: In test files, type helper props as
Partial<ComponentProps<typeof Component>>rather than loose object shapes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/tags-input/TagsInputAutocomplete.test.ts` around lines 25 - 26, The helper renderComponent currently accepts props = {} which allows arbitrary keys; tighten its typing by changing the parameter to props: Partial<ComponentProps<typeof TagsInputAutocomplete>> = {} (and add import { ComponentProps } from 'react' if missing) so invalid test props are caught at compile time; update any call sites accordingly to satisfy the narrower type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/search-input/SearchInputWithTags.test.ts`:
- Around line 123-132: The test "applies chipClass to chips" currently only
asserts the number of chips, so update it to assert the actual classes produced
by the chipClass callback: keep the chipClass variable and renderComponent({
modelValue: ['tag:landscape', 'type:video'], chipClass }), then locate the
specific chip elements (e.g., use screen.getByText('landscape') and
screen.getByText('video') or filter screen.getAllByTestId('tag-chip') by their
textContent) and assert that the 'tag:landscape' chip has the 'tag-chip' class
and the 'type:video' chip has the 'type-chip' class (use classList or a matcher
like toHaveClass) so the test fails if chipClass is not applied.
- Around line 93-97: The test "shows search icon when empty" currently uses
document.querySelector (violates testing-library/no-node-access); update it to
assert user-visible behavior instead: call renderComponent(), use
testing-library queries (e.g., screen.getByPlaceholderText with the input's
placeholder) to verify the input is present, and use screen.queryByRole or
screen.queryByLabelText to assert that no clear/clear-button element is present
when empty; replace the direct DOM lookup in this test with those queries
(reference: the test named "shows search icon when empty" and the
renderComponent helper).
In `@src/components/ui/tags-input/TagsInputAutocomplete.test.ts`:
- Around line 32-36: The ComboboxRoot test stub declares a disabled prop but
doesn't expose it, so update the stub templates used in
TagsInputAutocomplete.test.ts (the ComboboxRoot stubs around the first block and
the block at lines ~160-164) to bind the prop to an attribute (e.g., add
data-disabled="{{disabled}}" or :data-disabled="disabled" in the template) and
then change the assertions to check that attribute
(expect(getByTestId('combobox-root')).toHaveAttribute('data-disabled', 'true')
or equivalent) to validate that the disabled prop actually propagates from the
component under test.
- Around line 107-115: The test 'excludes already-selected tags from
suggestions' currently only asserts that selected tags are absent, which can
pass if no suggestions render; update the test (located around the
renderComponent({ modelValue: [...] }) call and the filteredSuggestions check)
to also assert positively that at least one expected remaining suggestion is
present — e.g., pick a known suggestion from the component's fixture data (such
as 'Cherry' or another tag guaranteed to be in the source suggestions) and add
expect(optionTexts).toContain('<expected-tag>') so the test verifies filtering,
not an empty dropdown.
In `@src/platform/assets/composables/useMediaAssetFiltering.ts`:
- Around line 29-54: matchesPropertyFilter currently coerces arbitrary targets
for boolean and string comparisons, causing queries like prop:isFavorite=maybe
to match false values and treating string ordering ops as equality; update
matchesPropertyFilter so that for boolean values it only accepts target ===
'true' or target === 'false' (otherwise return false), and for string values
only allow '=' and '~' (case-insensitive contains) — for ops '>', '<', '>=',
'<=' when value is a string return false rather than falling through to
equality; keep numeric handling as-is. Ensure you adjust the branches in
matchesPropertyFilter to explicitly check accepted ops per type and return false
for unsupported op/target combinations.
- Around line 22-26: parsePropFilter currently restricts property names to \w+
which disallows spaces and punctuation; update the parsing to accept arbitrary
property-key characters by replacing the key capture with a non-greedy "anything
before the operator" pattern (e.g. use /^(.+?)(>=|<=|>|<|=|~)(.+)$/) so keys
like "negative prompt" or "model-id" parse correctly, then trim whitespace from
the captured key and target and validate the key is non-empty before returning
the { key, op, target } object in parsePropFilter.
---
Nitpick comments:
In `@src/components/ui/search-input/SearchInputWithTags.test.ts`:
- Line 4: Remove the dead commented-import line from the test file: delete the
commented-out "// import { nextTick } from 'vue'" in SearchInputWithTags.test.ts
so the file contains only active imports and test code (no commented-out
imports).
- Around line 26-27: The test helper renderComponent currently accepts an
untyped props object—change its parameter type to a typed partial so callers get
proper prop typings; update renderComponent(props = {}) to accept props:
Partial<ComponentProps<typeof SearchInputWithTags>> (import ComponentProps from
React if needed) and propagate that typed props into the render call so tests
use strongly typed inputs for SearchInputWithTags.
- Around line 136-147: The test "keeps tag model separate from query model"
currently only checks chip rendering; update it to simulate typing into the
query input (use renderComponent to mount and
screen.getByRole/getByPlaceholderText to find the input), type characters to
trigger updates, then assert that onUpdate:query (onUpdateQuery) was called with
the typed value while onUpdate:modelValue (onUpdateModelValue) was not called
and the tag count (screen.getAllByTestId('tag-chip').length) remains unchanged;
this proves query edits don't mutate the tag model.
In `@src/components/ui/tags-input/TagsInputAutocomplete.test.ts`:
- Around line 25-26: The helper renderComponent currently accepts props = {}
which allows arbitrary keys; tighten its typing by changing the parameter to
props: Partial<ComponentProps<typeof TagsInputAutocomplete>> = {} (and add
import { ComponentProps } from 'react' if missing) so invalid test props are
caught at compile time; update any call sites accordingly to satisfy the
narrower type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b3b888e-07fa-44cc-899d-8a4d40d41a0c
📒 Files selected for processing (3)
src/components/ui/search-input/SearchInputWithTags.test.tssrc/components/ui/tags-input/TagsInputAutocomplete.test.tssrc/platform/assets/composables/useMediaAssetFiltering.ts
Comment on lines
+93
to
+97
| it('shows search icon when empty', () => { | ||
| renderComponent() | ||
| const icon = document.querySelector('.icon-\\[lucide--search\\]') | ||
| expect(icon).toBeInTheDocument() | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the test file exists and read its content
fd -t f "SearchInputWithTags.test.ts" --exec wc -l {}Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1879
🏁 Script executed:
# Get the relevant section of the test file
cat -n src/components/ui/search-input/SearchInputWithTags.test.ts | sed -n '85,105p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 798
🏁 Script executed:
# Check the component implementation to understand what's being tested
fd -t f "SearchInputWithTags.vue" -o "SearchInputWithTags.ts"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 233
🏁 Script executed:
# Check ESLint configuration for testing-library rules
find . -name ".eslintrc*" -o -name "eslint.config.*" | head -5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 464
🏁 Script executed:
# Check the ESLint config for testing-library rules
cat eslint.config.ts | grep -A 10 -B 5 "testing-library"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1956
🏁 Script executed:
# Find the SearchInputWithTags component
fd -t f "SearchInputWithTags.vue"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 124
🏁 Script executed:
# Get the full test file to understand the component better
cat -n src/components/ui/search-input/SearchInputWithTags.test.ts | head -50Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1980
🏁 Script executed:
# Read the SearchInputWithTags component to verify the assertions
cat src/components/ui/search-input/SearchInputWithTags.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 9453
Replace direct DOM node access at line 95 (CI lint blocker).
document.querySelector violates testing-library/no-node-access, which is configured as an error. The proposed change tests actual user-facing behavior—that the input field is present with the correct placeholder and no clear action exists when empty—rather than implementation details.
💡 Proposed change
- it('shows search icon when empty', () => {
+ it('shows input and no clear action when empty', () => {
renderComponent()
- const icon = document.querySelector('.icon-\\[lucide--search\\]')
- expect(icon).toBeInTheDocument()
+ expect(screen.getByPlaceholderText('Search...')).toBeInTheDocument()
+ expect(
+ screen.queryByRole('button', { name: 'Clear' })
+ ).not.toBeInTheDocument()
})📝 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
| it('shows search icon when empty', () => { | |
| renderComponent() | |
| const icon = document.querySelector('.icon-\\[lucide--search\\]') | |
| expect(icon).toBeInTheDocument() | |
| }) | |
| it('shows input and no clear action when empty', () => { | |
| renderComponent() | |
| expect(screen.getByPlaceholderText('Search...')).toBeInTheDocument() | |
| expect( | |
| screen.queryByRole('button', { name: 'Clear' }) | |
| ).not.toBeInTheDocument() | |
| }) |
🧰 Tools
🪛 ESLint
[error] 95-95: Avoid direct Node access. Prefer using the methods from Testing Library.
(testing-library/no-node-access)
[error] 95-95: Avoid direct Node access. Prefer using the methods from Testing Library.
(testing-library/no-node-access)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/search-input/SearchInputWithTags.test.ts` around lines 93 -
97, The test "shows search icon when empty" currently uses
document.querySelector (violates testing-library/no-node-access); update it to
assert user-visible behavior instead: call renderComponent(), use
testing-library queries (e.g., screen.getByPlaceholderText with the input's
placeholder) to verify the input is present, and use screen.queryByRole or
screen.queryByLabelText to assert that no clear/clear-button element is present
when empty; replace the direct DOM lookup in this test with those queries
(reference: the test named "shows search icon when empty" and the
renderComponent helper).
Comment on lines
+123
to
+132
| it('applies chipClass to chips', () => { | ||
| const chipClass = (value: string) => | ||
| value.startsWith('type:') ? 'type-chip' : 'tag-chip' | ||
| renderComponent({ | ||
| modelValue: ['tag:landscape', 'type:video'], | ||
| chipClass | ||
| }) | ||
| const chips = screen.getAllByTestId('tag-chip') | ||
| expect(chips).toHaveLength(2) | ||
| }) |
Contributor
There was a problem hiding this comment.
applies chipClass to chips does not assert class behavior.
The test currently checks count only, so it won’t fail if chipClass stops being applied.
💡 Proposed change
const chips = screen.getAllByTestId('tag-chip')
expect(chips).toHaveLength(2)
+ expect(chips.some((chip) => chip.classList.contains('type-chip'))).toBe(
+ true
+ )
+ expect(chips.some((chip) => chip.classList.contains('tag-chip'))).toBe(
+ true
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/search-input/SearchInputWithTags.test.ts` around lines 123
- 132, The test "applies chipClass to chips" currently only asserts the number
of chips, so update it to assert the actual classes produced by the chipClass
callback: keep the chipClass variable and renderComponent({ modelValue:
['tag:landscape', 'type:video'], chipClass }), then locate the specific chip
elements (e.g., use screen.getByText('landscape') and screen.getByText('video')
or filter screen.getAllByTestId('tag-chip') by their textContent) and assert
that the 'tag:landscape' chip has the 'tag-chip' class and the 'type:video' chip
has the 'type-chip' class (use classList or a matcher like toHaveClass) so the
test fails if chipClass is not applied.
Comment on lines
+32
to
+36
| ComboboxRoot: { | ||
| template: | ||
| '<div data-testid="combobox-root"><slot /></div>', | ||
| props: ['modelValue', 'open', 'multiple', 'disabled'] | ||
| }, |
Contributor
There was a problem hiding this comment.
Disabled-state test does not verify disabled propagation.
The stub accepts disabled but does not expose it, and the assertion only checks presence of the root. Assert a bound attribute so the test validates actual propagation.
💡 Proposed change
ComboboxRoot: {
- template:
- '<div data-testid="combobox-root"><slot /></div>',
+ template:
+ '<div data-testid="combobox-root" :data-disabled="String(disabled)"><slot /></div>',
props: ['modelValue', 'open', 'multiple', 'disabled']
},
...
const root = screen.getByTestId('combobox-root')
- expect(root).toBeInTheDocument()
+ expect(root).toHaveAttribute('data-disabled', 'true')Also applies to: 160-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/tags-input/TagsInputAutocomplete.test.ts` around lines 32 -
36, The ComboboxRoot test stub declares a disabled prop but doesn't expose it,
so update the stub templates used in TagsInputAutocomplete.test.ts (the
ComboboxRoot stubs around the first block and the block at lines ~160-164) to
bind the prop to an attribute (e.g., add data-disabled="{{disabled}}" or
:data-disabled="disabled" in the template) and then change the assertions to
check that attribute
(expect(getByTestId('combobox-root')).toHaveAttribute('data-disabled', 'true')
or equivalent) to validate that the disabled prop actually propagates from the
component under test.
Comment on lines
+107
to
+115
| it('excludes already-selected tags from suggestions', () => { | ||
| renderComponent({ modelValue: ['Apple', 'Banana'] }) | ||
| // With Apple and Banana selected, dropdown should not contain them | ||
| // This tests the filteredSuggestions computed | ||
| const options = screen.queryAllByRole('option') | ||
| const optionTexts = options.map((el) => el.textContent) | ||
| expect(optionTexts).not.toContain('Apple') | ||
| expect(optionTexts).not.toContain('Banana') | ||
| }) |
Contributor
There was a problem hiding this comment.
Suggestion filtering assertion can pass vacuously.
This test only checks that selected tags are absent; it still passes if no options render at all. Add at least one positive assertion for an expected remaining suggestion.
💡 Proposed change
const options = screen.queryAllByRole('option')
const optionTexts = options.map((el) => el.textContent)
+ expect(optionTexts).toContain('Cherry')
expect(optionTexts).not.toContain('Apple')
expect(optionTexts).not.toContain('Banana')📝 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
| it('excludes already-selected tags from suggestions', () => { | |
| renderComponent({ modelValue: ['Apple', 'Banana'] }) | |
| // With Apple and Banana selected, dropdown should not contain them | |
| // This tests the filteredSuggestions computed | |
| const options = screen.queryAllByRole('option') | |
| const optionTexts = options.map((el) => el.textContent) | |
| expect(optionTexts).not.toContain('Apple') | |
| expect(optionTexts).not.toContain('Banana') | |
| }) | |
| it('excludes already-selected tags from suggestions', () => { | |
| renderComponent({ modelValue: ['Apple', 'Banana'] }) | |
| // With Apple and Banana selected, dropdown should not contain them | |
| // This tests the filteredSuggestions computed | |
| const options = screen.queryAllByRole('option') | |
| const optionTexts = options.map((el) => el.textContent) | |
| expect(optionTexts).toContain('Cherry') | |
| expect(optionTexts).not.toContain('Apple') | |
| expect(optionTexts).not.toContain('Banana') | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/tags-input/TagsInputAutocomplete.test.ts` around lines 107
- 115, The test 'excludes already-selected tags from suggestions' currently only
asserts that selected tags are absent, which can pass if no suggestions render;
update the test (located around the renderComponent({ modelValue: [...] }) call
and the filteredSuggestions check) to also assert positively that at least one
expected remaining suggestion is present — e.g., pick a known suggestion from
the component's fixture data (such as 'Cherry' or another tag guaranteed to be
in the source suggestions) and add
expect(optionTexts).toContain('<expected-tag>') so the test verifies filtering,
not an empty dropdown.
Comment on lines
+22
to
+26
| export function parsePropFilter(chip: string): PropertyFilter | null { | ||
| const body = chip.startsWith('prop:') ? chip.slice(5) : chip | ||
| const match = body.match(/^(\w+)(>=|<=|>|<|=|~)(.+)$/) | ||
| if (!match) return null | ||
| return { key: match[1], op: match[2], target: match[3] } |
Contributor
There was a problem hiding this comment.
The parser is too restrictive for custom property names.
Line 24 only accepts \w+ as the key, so names like negative prompt, model-id, or seed value can never be used in prop: filters. That undercuts the “custom properties” part of this feature.
Suggested fix
export function parsePropFilter(chip: string): PropertyFilter | null {
const body = chip.startsWith('prop:') ? chip.slice(5) : chip
- const match = body.match(/^(\w+)(>=|<=|>|<|=|~)(.+)$/)
+ const match = body.match(/^(.+?)(>=|<=|>|<|=|~)(.+)$/)
if (!match) return null
- return { key: match[1], op: match[2], target: match[3] }
+ const key = match[1].trim()
+ const target = match[3].trim()
+ if (!key || !target) return null
+ return { key, op: match[2], target }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/assets/composables/useMediaAssetFiltering.ts` around lines 22 -
26, parsePropFilter currently restricts property names to \w+ which disallows
spaces and punctuation; update the parsing to accept arbitrary property-key
characters by replacing the key capture with a non-greedy "anything before the
operator" pattern (e.g. use /^(.+?)(>=|<=|>|<|=|~)(.+)$/) so keys like "negative
prompt" or "model-id" parse correctly, then trim whitespace from the captured
key and target and validate the key is non-empty before returning the { key, op,
target } object in parsePropFilter.
- Guard stale async in expandedSelectedAssets watcher (onCleanup) - Reset propertyFilters on tab switch in sidebar and modal - Remove unused listAllTags from assetService - Reject invalid boolean/string operator combos in property filter - Trim property key on click selection in AddPropertyRow - Localize type-picker labels via i18n keys - Guard division by zero in coverageOpacityClass - Use typed zUserProperties schema instead of z.record(z.unknown())
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/platform/assets/composables/useMediaAssetFiltering.ts (1)
22-27:⚠️ Potential issue | 🟠 MajorProperty key regex still restricts custom property names.
The regex
^(\w+)(>=|<=|>|<|=|~)(.+)$on line 24 only matches word characters ([A-Za-z0-9_]) for the key, so property names like"negative prompt","model-id", or"seed value"cannot be used inprop:filters. This was flagged in a past review but appears unaddressed.🐛 Suggested fix for property key parsing
export function parsePropFilter(chip: string): PropertyFilter | null { const body = chip.startsWith('prop:') ? chip.slice(5) : chip - const match = body.match(/^(\w+)(>=|<=|>|<|=|~)(.+)$/) + const match = body.match(/^(.+?)(>=|<=|>|<|=|~)(.+)$/) if (!match) return null - return { key: match[1], op: match[2], target: match[3] } + const key = match[1].trim() + const target = match[3].trim() + if (!key || !target) return null + return { key, op: match[2], target } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/assets/composables/useMediaAssetFiltering.ts` around lines 22 - 27, The regex in parsePropFilter currently restricts property keys to \w characters; update the key capture to allow spaces, dashes, and other non-operator characters by replacing the key token with a non-greedy "anything but the operator" capture (e.g., use a pattern like ^(.+?)(>=|<=|>|<|=|~)(.+)$), then trim the captured key before returning; keep the operator alternatives ordered with multi-char operators first (>=, <=) and retain the existing prop: prefix handling in parsePropFilter.
🧹 Nitpick comments (5)
src/platform/assets/composables/useMediaAssetFiltering.ts (1)
34-48: Add default case to number switch for completeness.The number comparison switch handles
=,>,<,>=,<=but falls through to the boolean/string checks for unsupported operators like~. While the string check for~will catch this, explicitly returningfalsefor unsupported numeric operators would be clearer.♻️ Add explicit default case
switch (op) { case '=': return value === n case '>': return value > n case '<': return value < n case '>=': return value >= n case '<=': return value <= n + default: + return false } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/assets/composables/useMediaAssetFiltering.ts` around lines 34 - 48, In the numeric branch of the filter (the if (typeof value === 'number') block) add an explicit default case to the switch(op) so unsupported numeric operators (e.g., '~' or anything else) return false instead of falling through to the later string/boolean checks; update the switch in that block (the one handling '=', '>', '<', '>=', '<=') to include a default: return false for clarity and correctness.src/platform/assets/schemas/userPropertySchema.ts (1)
43-52: Consider adding exhaustiveness check for type safety.The switch statement handles all three
PropertyTypecases, but TypeScript won't enforce exhaustiveness without an explicit check. Adding a default case withneverassertion ensures future property types are handled.♻️ Optional exhaustiveness check
export function createDefaultProperty(type: PropertyType): UserProperty { switch (type) { case 'string': return { type: 'string', value: '' } case 'boolean': return { type: 'boolean', value: false } case 'number': return { type: 'number', value: 0 } + default: { + const _exhaustive: never = type + throw new Error(`Unhandled property type: ${_exhaustive}`) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/assets/schemas/userPropertySchema.ts` around lines 43 - 52, The switch in createDefaultProperty(type: PropertyType): UserProperty currently returns for 'string'|'boolean'|'number' but lacks an exhaustiveness check; add a default branch that asserts the unreachable case (e.g., treat type as never and throw or use a helper unreachable function) so the compiler will error if PropertyType gains new members—update createDefaultProperty to include this never assertion in the default path to ensure type safety.src/platform/assets/components/properties/AddPropertyRow.vue (1)
200-216: Property type labels now use i18n but won't react to language changes.The
propertyTypesarray usest()for labels, addressing the past localization feedback. However, since it's defined as a plain array (not acomputed), the labels are evaluated once at component mount and won't update if the user changes language at runtime.If runtime language switching is supported, consider making this a computed:
♻️ Make propertyTypes reactive for i18n
-const propertyTypes: { type: PropertyType; icon: string; label: string }[] = [ - { - type: 'string', - icon: 'icon-[lucide--type]', - label: t('properties.typeText') - }, - { - type: 'number', - icon: 'icon-[lucide--hash]', - label: t('properties.typeNumber') - }, - { - type: 'boolean', - icon: 'icon-[lucide--square-check]', - label: t('properties.typeBoolean') - } -] +const propertyTypes = computed(() => [ + { + type: 'string' as const, + icon: 'icon-[lucide--type]', + label: t('properties.typeText') + }, + { + type: 'number' as const, + icon: 'icon-[lucide--hash]', + label: t('properties.typeNumber') + }, + { + type: 'boolean' as const, + icon: 'icon-[lucide--square-check]', + label: t('properties.typeBoolean') + } +])Then update line 104 to use
propertyTypes.value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/assets/components/properties/AddPropertyRow.vue` around lines 200 - 216, propertyTypes is built once with t(...) and not reactive to locale changes; convert it into a Vue computed that returns the array (e.g., const propertyTypes = computed(() => [...labels using t('...')...])), ensure useI18n's t is used inside the computed so labels update on language change, and update any places that currently reference propertyTypes to use propertyTypes.value (e.g., in templates or methods) so the reactive array is consumed correctly.src/components/sidebar/tabs/AssetsSidebarTab.vue (1)
86-94: Guard against undefinedselectedAssets[0]in the info panel.The
Popoveris shown whenhasSelectionis true, butselectedAssets[0]could still be undefined if the selection state changes between render cycles. TheMediaAssetInfoPanelreceivesselectedAssets[0]directly which could cause issues if undefined.Consider adding a defensive check:
🛡️ Suggested defensive binding
<div class="max-h-[70vh] w-72 overflow-y-auto"> <MediaAssetInfoPanel - :asset="selectedAssets[0]" + :asset="selectedAssets[0]!" :assets="infoPanelAssets" :tag-suggestions="availableTags" :property-suggestions="propertySuggestions" compact `@zoom`="handleZoomClick" /> </div>Alternatively, add a
v-ifguard on theMediaAssetInfoPanelitself to ensure the asset exists before rendering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/sidebar/tabs/AssetsSidebarTab.vue` around lines 86 - 94, The MediaAssetInfoPanel is being passed selectedAssets[0] which can be undefined between render cycles; update the binding to guard the prop or prevent rendering—either add a v-if (e.g., v-if="selectedAssets && selectedAssets.length") on the MediaAssetInfoPanel or change the :asset binding to a safe expression (e.g., :asset="selectedAssets[0] || null") and ensure MediaAssetInfoPanel handles null/undefined; adjust references to selectedAssets, hasSelection, and infoPanelAssets accordingly so the panel only mounts when a valid asset exists.src/platform/assets/components/MediaAssetBrowserModal.vue (1)
250-258: Tag counting function iterates all assets on every computed access.The
getTagCountsfunction iterates through all assets and their tags. It's called in two computed properties (outputTagCountsandinputTagCounts), each recalculated when their respective media arrays change. For large asset lists, this could become a performance concern.Consider memoizing or debouncing if performance issues arise with large datasets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/assets/components/MediaAssetBrowserModal.vue` around lines 250 - 258, The getTagCounts function currently recomputes by iterating all assets each time it's used (by the outputTagCounts and inputTagCounts computed properties); to fix, memoize results per assets array (or per assets array version) so repeated accesses avoid re-scanning: implement a small cache (e.g., WeakMap or Map) keyed by the AssetItem[] reference or a derived version/hash and return the cached Map if the same key is seen, otherwise compute and store; update getTagCounts to consult this cache and keep references to the symbols getTagCounts, outputTagCounts, and inputTagCounts when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/platform/assets/composables/useMediaAssetFiltering.ts`:
- Around line 22-27: The regex in parsePropFilter currently restricts property
keys to \w characters; update the key capture to allow spaces, dashes, and other
non-operator characters by replacing the key token with a non-greedy "anything
but the operator" capture (e.g., use a pattern like ^(.+?)(>=|<=|>|<|=|~)(.+)$),
then trim the captured key before returning; keep the operator alternatives
ordered with multi-char operators first (>=, <=) and retain the existing prop:
prefix handling in parsePropFilter.
---
Nitpick comments:
In `@src/components/sidebar/tabs/AssetsSidebarTab.vue`:
- Around line 86-94: The MediaAssetInfoPanel is being passed selectedAssets[0]
which can be undefined between render cycles; update the binding to guard the
prop or prevent rendering—either add a v-if (e.g., v-if="selectedAssets &&
selectedAssets.length") on the MediaAssetInfoPanel or change the :asset binding
to a safe expression (e.g., :asset="selectedAssets[0] || null") and ensure
MediaAssetInfoPanel handles null/undefined; adjust references to selectedAssets,
hasSelection, and infoPanelAssets accordingly so the panel only mounts when a
valid asset exists.
In `@src/platform/assets/components/MediaAssetBrowserModal.vue`:
- Around line 250-258: The getTagCounts function currently recomputes by
iterating all assets each time it's used (by the outputTagCounts and
inputTagCounts computed properties); to fix, memoize results per assets array
(or per assets array version) so repeated accesses avoid re-scanning: implement
a small cache (e.g., WeakMap or Map) keyed by the AssetItem[] reference or a
derived version/hash and return the cached Map if the same key is seen,
otherwise compute and store; update getTagCounts to consult this cache and keep
references to the symbols getTagCounts, outputTagCounts, and inputTagCounts when
making the change.
In `@src/platform/assets/components/properties/AddPropertyRow.vue`:
- Around line 200-216: propertyTypes is built once with t(...) and not reactive
to locale changes; convert it into a Vue computed that returns the array (e.g.,
const propertyTypes = computed(() => [...labels using t('...')...])), ensure
useI18n's t is used inside the computed so labels update on language change, and
update any places that currently reference propertyTypes to use
propertyTypes.value (e.g., in templates or methods) so the reactive array is
consumed correctly.
In `@src/platform/assets/composables/useMediaAssetFiltering.ts`:
- Around line 34-48: In the numeric branch of the filter (the if (typeof value
=== 'number') block) add an explicit default case to the switch(op) so
unsupported numeric operators (e.g., '~' or anything else) return false instead
of falling through to the later string/boolean checks; update the switch in that
block (the one handling '=', '>', '<', '>=', '<=') to include a default: return
false for clarity and correctness.
In `@src/platform/assets/schemas/userPropertySchema.ts`:
- Around line 43-52: The switch in createDefaultProperty(type: PropertyType):
UserProperty currently returns for 'string'|'boolean'|'number' but lacks an
exhaustiveness check; add a default branch that asserts the unreachable case
(e.g., treat type as never and throw or use a helper unreachable function) so
the compiler will error if PropertyType gains new members—update
createDefaultProperty to include this never assertion in the default path to
ensure type safety.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dab83b18-1839-4590-bc6e-f773961b35ba
📒 Files selected for processing (6)
src/components/sidebar/tabs/AssetsSidebarTab.vuesrc/platform/assets/components/MediaAssetBrowserModal.vuesrc/platform/assets/components/properties/AddPropertyRow.vuesrc/platform/assets/composables/useMediaAssetFiltering.tssrc/platform/assets/schemas/assetSchema.tssrc/platform/assets/schemas/userPropertySchema.ts
Member
pythongosssss
left a comment
pythongosssss
left a comment
There was a problem hiding this comment.
Really cool feature and implementation, only a few bits of duplication to sort out, additional testing and some smaller nits from me.
A general review of the feature/code:
- Marquee selection is a great for usability
- Search is really powerful, we would need to extend it to have a backend implementation but as a PoC is great
- Multi select/batch editing works well, good consideration of the different values items could have
- Multi-type custom property editor is intuitive and easy to use
- Fits in the project style/guidelines well
| > | ||
| <TabList v-model="activeTab"> | ||
| <Tab value="output">{{ $t('sideToolbar.labels.generated') }}</Tab> | ||
| <Tab value="input">{{ $t('sideToolbar.labels.imported') }}</Tab> | ||
| </TabList> | ||
| <Popover | ||
| v-if="hasSelection" | ||
| v-model="infoPopoverOpen" |
Member
There was a problem hiding this comment.
If a user uses the context menu to trigger this, that doesnt trigger a selection on the item and so wont show the correct item
| // Custom click-outside handler since we disabled reka-ui's interact-outside. | ||
| // reka-ui's DismissableLayer cannot distinguish between genuine outside clicks | ||
| // and interactions with nested dismiss layers (e.g. ComboboxContent dropdowns). | ||
| watch(open, (isOpen) => { |
Member
There was a problem hiding this comment.
Reka supports closing with escape etc, would look at an alternative to this without needing to reimplement
Member
There was a problem hiding this comment.
There looks to be a lot of overlap here with TagsInputAutocomplete.vue, worth extracting common functions normalizeForMatch, resolveTag, highlightSegments, etc. into a shared composable
| } | ||
| }) | ||
|
|
||
| function multiTagClass(tag: string): string | undefined { |
Member
There was a problem hiding this comment.
This looks like a dupe of coverageOpacityClass
Member
There was a problem hiding this comment.
Opportunities to share a bunch of functionality here with AssetSidebarTab.vue
| Math.min(visibleImages.value.length, maxVisible) | ||
| ) | ||
|
|
||
| const containerStyle = computed(() => ({})) |
| <script setup lang="ts"> | ||
| import { computed, ref } from 'vue' | ||
|
|
||
| const { |
Member
There was a problem hiding this comment.
images/count have defaults but aren't marked optional
| set: (value: string) => queueMetadataUpdate({ user_description: value }) | ||
| }) | ||
|
|
||
| function formatFileSize(bytes: number): string { |
Member
There was a problem hiding this comment.
@/utils/formatUtil/formatSize
| ) | ||
| } | ||
|
|
||
| const filteredSuggestions = computed(() => { |
Member
There was a problem hiding this comment.
Could this also use useFilter like TagsInputAutocomplete
Member
There was a problem hiding this comment.
Worth splitting out the core logic here in a similar pattern to useMultiPropertyEditing
Author
|
Thank you for the review @pythongosssss! Good feedback, I'll do a cleanup pass when I find a moment |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Implements media asset metadata, including tagging, custom properties and integration with search. Supports batch editing when multiple assets are selected
Key changes: