Open
Conversation
- add root filter buttons - replace input/output selection with popover - replace price badge with one from node header - fix bug with hovering selecting item under mouse automatically - fix tailwind merge with custom sizes removing them - general tidy/refactor/test
🎭 Playwright: ✅ 666 passed, 0 failed · 5 flaky📊 Browser Reports
|
🎨 Storybook: ✅ Built — View Storybook |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CreditBadge and a multi-select type-filter popover; refactors V2 node search to a roving-focus sidebar, data-driven filter bar, unified input, and new content/list flows; extends tailwind-merge; adds node-source helpers, i18n keys, browser test helpers, highlight matching enhancements, and removes the old FilterPanel. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Input as "NodeSearchInput"
participant Sidebar as "Category Sidebar"
participant FilterBar as "Filter Bar"
participant TypePopover as "Type Filter Popover"
participant Content as "NodeSearchContent"
participant Results as "Results List"
User->>Input: type query / navigate / select
Input->>Content: emit navigate / select / update searchQuery
User->>Sidebar: select/expand category
Sidebar->>Content: emit selectCategory / autoExpand
User->>FilterBar: open type filters / click category button
FilterBar->>TypePopover: open popover
User->>TypePopover: toggle type(s) / clear
TypePopover->>FilterBar: emit toggle / clear
FilterBar->>Content: emit toggleFilter / selectCategory
Content->>Results: compute results (searchQuery + rootFilter + category + types)
Results->>User: render updated list
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
🧹 Nitpick comments (1)
src/components/searchbox/v2/NodeSearchContent.test.ts (1)
448-459: Add the no-movement assertion for the hover regression.This only proves
pointermoveselects. It does not protect the bug called out in the PR, where merely hovering without movement was enough. Assert thatpointerenterleaves the current item unchanged before thepointermovestep.Based on learnings: For Vue component testing, use Vue Test Utils and aim for behavioral coverage of critical and new features.💡 Suggested test shape
const results = getResultItems(wrapper) + await results[1].trigger('pointerenter') + await nextTick() + expect(results[0].attributes('aria-selected')).toBe('true') + expect(results[1].attributes('aria-selected')).not.toBe('true') await results[1].trigger('pointermove') await nextTick()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/searchbox/v2/NodeSearchContent.test.ts` around lines 448 - 459, The test "should select item on hover via pointermove" currently only asserts pointermove behavior; to guard against the regression where pointerenter (hover without movement) incorrectly selects an item, first trigger a pointerenter on the target result (use getResultItems(wrapper)[1].trigger('pointerenter')) and assert the selection state did not change (e.g., the previously-selected item's aria-selected remains unchanged or the hovered item's aria-selected is not 'true') before proceeding to trigger('pointermove') and the existing assertion; use the existing helpers setupFavorites and getResultItems to locate items.
🤖 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/node/CreditBadge.vue`:
- Around line 2-17: Wrap the two spans in CreditBadge.vue inside a single
internal container so the badge halves are rendered together (making the seam
self-contained) instead of relying on external layout; specifically, create a
wrapper element that contains the existing root span (the one using cn(...) with
'rounded-l-full pr-1' / 'rounded-full') and the trailing span (the v-if="rest"
element that currently uses '-ml-2.5 ... rounded-r-full'), remove the dependency
on parent-level negative margin by moving overlap/positioning into the new
container (use inline-flex/relative positioning and internal negative spacing so
the left and right halves join consistently), and keep the same per-half classes
(the cn(...) span and the v-if="rest" span) so their visuals remain unchanged
while the seam is controlled internally.
In `@src/components/searchbox/v2/NodeSearchCategoryTreeNode.vue`:
- Around line 12-37: The chevron hover state is using the global selector
"group-hover/categories" so every row responds to sidebar hover; add a local
group on this row by including "group/categories" (or just "group") in the
button's class list (the button that emits 'select' for node.key) and update the
icon class in the i element (where node.children?.length and !isExpanded are
used) to use "group-hover:opacity-100" instead of
"group-hover/categories:opacity-100" so the chevron opacity change is scoped to
the hovered row only.
In `@src/components/searchbox/v2/NodeSearchContent.test.ts`:
- Around line 57-68: The mockBookmarks helper is masking "no bookmarks"
scenarios because its bookmarkList defaults to ['placeholder']; change
mockBookmarks so its bookmarkList default is an empty array (bookmarkList:
string[] = []) and update any tests that relied on the placeholder to explicitly
pass a non-empty list; ensure when tests call mockBookmarks(false) they either
omit the bookmarkList (to simulate no bookmarks) or explicitly pass [] so the
Favorites visibility logic is exercised properly (refer to mockBookmarks,
useNodeBookmarkStore, and the isBookmarked spy).
In `@src/components/searchbox/v2/NodeSearchContent.vue`:
- Around line 214-223: The onSelectCategory handler should preserve the current
searchQuery instead of clearing it; modify the onSelectCategory function
(referencing rootFilter, selectedCategory, searchQuery, searchInputRef, and
nextTick) to remove the line that sets searchQuery.value = '' so switching root
filters keeps the typed query and lets getMostRelevantResults combine rootFilter
with searchResults; keep the rest of the logic (toggling rootFilter, setting
selectedCategory to 'most-relevant', and focusing searchInputRef) unchanged.
In `@src/components/searchbox/v2/NodeSearchFilterBar.vue`:
- Around line 25-37: When tf.values is non-empty, add an accessible, localized
summary inside the button (hidden visually with the sr-only class) so screen
readers hear the selected type names; locate the button rendering around the
chipClass(...) usage in NodeSearchFilterBar.vue and insert an sr-only span
(rendered only when tf.values.length > 0) that uses vue-i18n ($t) to produce a
string from src/locales/en/main.json (e.g., a key like main.selectedTypes with a
types interpolation) and pass the selected values joined (tf.values.slice(0,
MAX_VISIBLE_DOTS) or tf.values.join(', ') as appropriate) so the screen reader
reads the selected types along with tf.chip.label, keeping existing visual dots
and getLinkTypeColor behavior unchanged.
In `@src/components/searchbox/v2/NodeSearchListItem.vue`:
- Around line 24-52: The source/provider badges are icon-only or aria-hidden
when showCategory is false, leaving no accessible label; update the spans that
render source/provider (the v-if block using showSourceBadge, isCustom,
nodeDef.nodeSource.displayText, providerName, badgePillClass, ComfyLogo,
getProviderIcon) to include an accessible text alternative: add an offscreen
text element (e.g., a visually-hidden/sr-only span) or an aria-label on the
badge container that contains nodeDef.nodeSource.displayText or providerName,
and ensure the decorative icons keep aria-hidden while the container exposes the
accessible name so screen readers still get the source/provider when
showCategory is false.
In `@src/components/searchbox/v2/NodeSearchTypeFilterPopover.test.ts`:
- Around line 9-11: The test is using createMockChip()'s default sorted fixture
which masks regressions; modify the assertions that currently call
createMockChip() (around the test verifying sorting) to pass an explicitly
unsorted array (e.g., ['MODEL','IMAGE','LATENT']) into createMockChip so the
component must actually sort values to satisfy the assertion; update all
occurrences in that test block (the assertions between lines ~82-91) to use the
unsorted fixture instead of the default.
In `@src/components/searchbox/v2/NodeSearchTypeFilterPopover.vue`:
- Around line 36-39: The template in NodeSearchTypeFilterPopover.vue uses
t('g.itemsSelected', { selectedCount: selectedValues.length }) which prevents
vue-i18n pluralization; change the template to call t('g.itemSelected', { count:
selectedValues.length }) and update the locale entry to a single pluralized key
(e.g., "itemSelected": "{count} item selected | {count} items selected") so
vue-i18n can pick singular/plural correctly; ensure you update any occurrences
of itemsSelected to the new itemSelected plural key and keep the binding using
selectedValues.length.
---
Nitpick comments:
In `@src/components/searchbox/v2/NodeSearchContent.test.ts`:
- Around line 448-459: The test "should select item on hover via pointermove"
currently only asserts pointermove behavior; to guard against the regression
where pointerenter (hover without movement) incorrectly selects an item, first
trigger a pointerenter on the target result (use
getResultItems(wrapper)[1].trigger('pointerenter')) and assert the selection
state did not change (e.g., the previously-selected item's aria-selected remains
unchanged or the hovered item's aria-selected is not 'true') before proceeding
to trigger('pointermove') and the existing assertion; use the existing helpers
setupFavorites and getResultItems to locate items.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c94f1c40-0661-448a-936d-9af22522f870
📒 Files selected for processing (24)
packages/tailwind-utils/src/index.tssrc/components/node/CreditBadge.vuesrc/components/node/NodePreviewCard.vuesrc/components/node/NodePricingBadge.vuesrc/components/searchbox/v2/NodeSearchCategorySidebar.test.tssrc/components/searchbox/v2/NodeSearchCategorySidebar.vuesrc/components/searchbox/v2/NodeSearchCategoryTreeNode.vuesrc/components/searchbox/v2/NodeSearchContent.test.tssrc/components/searchbox/v2/NodeSearchContent.vuesrc/components/searchbox/v2/NodeSearchFilterBar.test.tssrc/components/searchbox/v2/NodeSearchFilterBar.vuesrc/components/searchbox/v2/NodeSearchFilterPanel.vuesrc/components/searchbox/v2/NodeSearchInput.test.tssrc/components/searchbox/v2/NodeSearchInput.vuesrc/components/searchbox/v2/NodeSearchListItem.vuesrc/components/searchbox/v2/NodeSearchTypeFilterPopover.test.tssrc/components/searchbox/v2/NodeSearchTypeFilterPopover.vuesrc/components/searchbox/v2/__test__/testUtils.tssrc/components/sidebar/ComfyMenuButton.vuesrc/locales/en/main.jsonsrc/locales/en/settings.jsonsrc/renderer/extensions/vueNodes/components/NodeHeader.vuesrc/types/nodeSource.test.tssrc/types/nodeSource.ts
💤 Files with no reviewable changes (1)
- src/components/searchbox/v2/NodeSearchFilterPanel.vue
pythongosssss
added
the
Design
- improved keyboard navigation and aria - fixed alignment of elements - updated fonts and sizes - more tidy + nits - tests
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/components/searchbox/v2/NodeSearchFilterBar.vue (1)
25-37:⚠️ Potential issue | 🟠 MajorExpose active type selections to screen readers.
When selections are shown as dots, this button still announces only the base chip label. The active filter state disappears for assistive tech users. Add a localized
sr-onlysummary of the selected values inside the trigger.Suggested fix
<button type="button" :class="chipClass(false, tf.values.length > 0)"> + <span v-if="tf.values.length > 0" class="sr-only"> + {{ + t('g.nodeSearchTypeFilterSummary', { + label: tf.chip.label, + values: tf.values.join(', ') + }) + }} + </span> <span v-if="tf.values.length > 0" class="flex items-center"> <span v-for="val in tf.values.slice(0, MAX_VISIBLE_DOTS)"As per coding guidelines: Use
vue-i18nfor ALL user-facing strings, configured insrc/locales/en/main.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/searchbox/v2/NodeSearchFilterBar.vue` around lines 25 - 37, The button currently only renders visual dots and the base chip label, which hides the active selections from screen readers; add an invisible screen-reader-only summary inside the same button that lists the selected values using vue-i18n. Concretely, in NodeSearchFilterBar.vue next to the visual dots (inside the <button> that uses chipClass and tf.chip.label), insert an element with class "sr-only" that uses the component's i18n (e.g. $t or useI18n) to render a localized string key (add it to src/locales/en/main.json) and pass the selected values derived from tf.values (respecting MAX_VISIBLE_DOTS if you want the same truncation) — reference tf.values, MAX_VISIBLE_DOTS, tf.chip.label and getLinkTypeColor to locate the spot — so assistive tech reads "label: value1, value2…" while keeping visuals unchanged.
🧹 Nitpick comments (1)
src/components/searchbox/v2/NodeSearchCategorySidebar.test.ts (1)
40-48: Type the mount helper against the component props.
props = {}is fully loose here, so a typo in a new prop silently turns into a no-op test setup. Typing this asPartial<ComponentProps<typeof NodeSearchCategorySidebar>>keeps the helper aligned with the component contract as the API evolves.Based on learnings: In test files, for test helpers like mountComponent, type the props parameter as
Partial<ComponentProps<typeof Component>>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/searchbox/v2/NodeSearchCategorySidebar.test.ts` around lines 40 - 48, The createWrapper test helper currently accepts an untyped props = {} which allows silent typos; change its signature to accept props: Partial<ComponentProps<typeof NodeSearchCategorySidebar>> and update the mount call to use that typed props variable; also import ComponentProps from 'vue' (or the appropriate typing source) at the top of the test file so the type resolves. This keeps the helper aligned with NodeSearchCategorySidebar's props contract and prevents silent setup mistakes.
🤖 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/searchbox/v2/NodeSearchCategorySidebar.vue`:
- Around line 181-189: The watcher on categoryTree currently emits 'autoExpand'
for a single-root tree even when the current selection is a deeper descendant,
which clobbers deeper selections; modify the watcher (the watch over
categoryTree that inspects nodes) to first check the current selected category
(e.g., the prop/ref named selectedCategory) and skip emitting emit('autoExpand',
nodes[0].key) if selectedCategory is already a descendant of
nodes[0].key—implement a small helper like isDescendant(selectedCategory,
rootKey) (used by the watcher) that returns true when the selectedCategory is
the root or any deeper child so the autoExpand event only fires when there is no
deeper selection to preserve.
In `@src/components/searchbox/v2/NodeSearchCategoryTreeNode.vue`:
- Around line 2-20: The wrapper currently carries role="treeitem" and
aria-expanded while RovingFocusItem focuses the inner button, so screen readers
miss expanded/selected state; move the treeitem role and state attributes onto
the focus target by applying role="treeitem",
:aria-expanded="node.children?.length ? isExpanded : undefined", and
:aria-selected="selectedCategory === node.key || undefined" to the
RovingFocusItem's child button (the element referenced by buttonEl) or make the
wrapper itself the roving focus target in RovingFocusItem; ensure aria-current
remains or is replaced by aria-selected so the focused button exposes selection
and expanded state (use the same symbols: selectedCategory, isExpanded,
node.key, buttonEl, RovingFocusItem).
In `@src/components/searchbox/v2/NodeSearchContent.vue`:
- Around line 186-198: rootFilteredNodeDefs is filtering blueprint children
using the full BLUEPRINT_CATEGORY/... path while getCategoryResults compares
against root-relative keys, causing mismatches; update filtering in
rootFilteredNodeDefs (and similarly in getCategoryResults at the other location)
to normalize category forms before comparing: when rootFilter.value is
BLUEPRINT_CATEGORY treat child keys as full-path by prefixing BLUEPRINT_CATEGORY
+ '/' (or conversely strip the BLUEPRINT_CATEGORY/ prefix from node.category) so
both sides use the same representation; adjust the BLUEPRINT_CATEGORY case and
the partner/default branches to perform this normalization using the existing
rootFilter, BLUEPRINT_CATEGORY constant, node.category and the
sourceCategoryFilters mapping.
---
Duplicate comments:
In `@src/components/searchbox/v2/NodeSearchFilterBar.vue`:
- Around line 25-37: The button currently only renders visual dots and the base
chip label, which hides the active selections from screen readers; add an
invisible screen-reader-only summary inside the same button that lists the
selected values using vue-i18n. Concretely, in NodeSearchFilterBar.vue next to
the visual dots (inside the <button> that uses chipClass and tf.chip.label),
insert an element with class "sr-only" that uses the component's i18n (e.g. $t
or useI18n) to render a localized string key (add it to
src/locales/en/main.json) and pass the selected values derived from tf.values
(respecting MAX_VISIBLE_DOTS if you want the same truncation) — reference
tf.values, MAX_VISIBLE_DOTS, tf.chip.label and getLinkTypeColor to locate the
spot — so assistive tech reads "label: value1, value2…" while keeping visuals
unchanged.
---
Nitpick comments:
In `@src/components/searchbox/v2/NodeSearchCategorySidebar.test.ts`:
- Around line 40-48: The createWrapper test helper currently accepts an untyped
props = {} which allows silent typos; change its signature to accept props:
Partial<ComponentProps<typeof NodeSearchCategorySidebar>> and update the mount
call to use that typed props variable; also import ComponentProps from 'vue' (or
the appropriate typing source) at the top of the test file so the type resolves.
This keeps the helper aligned with NodeSearchCategorySidebar's props contract
and prevents silent setup mistakes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81701592-fd0c-43e8-9919-215f4e140234
📒 Files selected for processing (15)
browser_tests/fixtures/components/ComfyNodeSearchBoxV2.tsbrowser_tests/tests/nodeSearchBoxV2.spec.tssrc/components/node/NodePreviewCard.vuesrc/components/searchbox/NodeSearchBoxPopover.vuesrc/components/searchbox/v2/NodeSearchCategorySidebar.test.tssrc/components/searchbox/v2/NodeSearchCategorySidebar.vuesrc/components/searchbox/v2/NodeSearchCategoryTreeNode.vuesrc/components/searchbox/v2/NodeSearchContent.test.tssrc/components/searchbox/v2/NodeSearchContent.vuesrc/components/searchbox/v2/NodeSearchFilterBar.vuesrc/components/searchbox/v2/NodeSearchInput.vuesrc/components/searchbox/v2/NodeSearchListItem.vuesrc/components/searchbox/v2/NodeSearchTypeFilterPopover.test.tssrc/components/searchbox/v2/NodeSearchTypeFilterPopover.vuesrc/components/searchbox/v2/__test__/testUtils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/searchbox/v2/test/testUtils.ts
- src/components/searchbox/v2/NodeSearchContent.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/components/searchbox/v2/NodeSearchCategoryTreeNode.vue (2)
13-20:⚠️ Potential issue | 🟠 MajorExpose selection with
aria-selectedon the focused treeitem.The focus target now has
role="treeitem"andaria-expanded, but selection is still only conveyed viaaria-current. In a selectable tree, assistive tech looks foraria-selected; without it, the focused node's selected state is not announced.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/searchbox/v2/NodeSearchCategoryTreeNode.vue` around lines 13 - 20, Add an aria-selected attribute to the treeitem button so assistive tech can announce selection: on the element referenced by buttonEl (the button with role="treeitem" and :data-testid using node.key) bind aria-selected to the same check used for aria-current (e.g. selectedCategory === node.key) so it returns a boolean selection state; keep the existing :aria-current and :aria-expanded logic (isExpanded) unchanged.
23-40:⚠️ Potential issue | 🟡 MinorScope chevron reveal to the current row.
The icon still uses
group-hover/categoriesandgroup-focus-within/categories, which bind to the sidebar-level group. Hovering or focusing any category will therefore reveal chevrons for unrelated rows too. Add a local group on this button and target that instead.♻️ Possible fix
- 'flex w-full cursor-pointer items-center gap-2 rounded-lg border-none bg-transparent py-2.5 pr-3 text-left font-inter text-sm transition-colors', + 'group flex w-full cursor-pointer items-center gap-2 rounded-lg border-none bg-transparent py-2.5 pr-3 text-left font-inter text-sm transition-colors', selectedCategory === node.key ? CATEGORY_SELECTED_CLASS : CATEGORY_UNSELECTED_CLASS @@ - ? 'icon-[lucide--chevron-down] opacity-0 group-focus-within/categories:opacity-100 group-hover/categories:opacity-100' + ? 'icon-[lucide--chevron-down] opacity-0 group-focus-within:opacity-100 group-hover:opacity-100' : '',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/searchbox/v2/NodeSearchCategoryTreeNode.vue` around lines 23 - 40, The chevron reveal selectors currently target the sidebar-level group (group-hover/categories and group-focus-within/categories), causing chevrons to show for unrelated rows; add a local per-row group class on the button (e.g., "group/category-row" or similar) in the root element that renders each node (the element using cn(...) and `@click`="$emit('select', node.key)"), then update the chevron class selectors in the <i> element (the computed string that contains 'icon-[lucide--chevron-down] opacity-0 ...') to use the new local group selectors (group-hover/<your-group> and group-focus-within/<your-group>) so hover/focus only affect that row.
🧹 Nitpick comments (2)
src/components/searchbox/v2/NodeSearchTypeFilterPopover.test.ts (2)
76-167: Add a keyboard-path test forescapeClose.This suite covers the click flows, but it never asserts the
escapeCloseemit or any Escape-key interaction even though that behavior is part of the component API and this PR is explicitly about keyboard/ARIA refinements. A focused test that opens the popover, sendsEscape, and checkswrapper.emitted('escapeClose')would protect the new UX from regressions.Based on learnings: "Applies to src/**/*.test.ts : For Vue component testing, use Vue Test Utils and follow advice about making components easy to test; aim for behavioral coverage of critical and new features."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/searchbox/v2/NodeSearchTypeFilterPopover.test.ts` around lines 76 - 167, Add a test that opens the popover (reuse createWrapper() and await openPopover(wrapper)), then simulate pressing the Escape key on the popover or document (e.g., trigger a 'keydown' with key === 'Escape' on wrapper or wrapper.element), await nextTick(), and assert that wrapper.emitted('escapeClose') exists and has length 1; reference existing helpers getOptions, createWrapper, openPopover, and wrapper.emitted to locate where to add the test.
9-49: Keep the test helpers tied to the real component types.
createWrapper()duplicates the prop contract, andcreateMockChip()sidesteps the filter contract withas unknown as. That makes this suite drift-prone: a prop orFilterChip['filter']change can stop being type-checked here. Please derivepropsfromComponentProps<typeof NodeSearchTypeFilterPopover>and build the mock with the concrete filter type/satisfiesinstead of the double-cast.♻️ Suggested direction
+import type { ComponentProps } from 'vue-component-type-helpers' ... - function createWrapper( - props: { - chip?: FilterChip - selectedValues?: string[] - } = {} - ) { + function createWrapper( + props: Partial<ComponentProps<typeof NodeSearchTypeFilterPopover>> = {} + ) {Based on learnings: "In TypeScript test files, avoid duplicating interface/type definitions..." and "for test helper functions that construct mock objects implementing an interface, prefer using satisfies InterfaceType for shape validation instead of type assertions like as Partial as InterfaceType or as any."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/searchbox/v2/NodeSearchTypeFilterPopover.test.ts` around lines 9 - 49, Tests currently duplicate the component prop contract and cast the mock filter with as unknown as, making type drift likely; change createWrapper to type its props parameter from ComponentProps<typeof NodeSearchTypeFilterPopover> (or derive the chip/selectedValues types from that) and rebuild createMockChip so its returned object uses TypeScript's satisfies with the concrete FilterChip and its filter type (e.g. return {...} satisfies FilterChip and ensure filter conforms to FilterChip['filter'] or the concrete filter interface) instead of using as unknown as, so the compiler will validate the mock shape against the real component types (reference NodeSearchTypeFilterPopover, createWrapper, createMockChip, FilterChip, ComponentProps, and use satisfies).
🤖 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/searchbox/v2/NodeSearchCategorySidebar.vue`:
- Around line 181-195: The watch handler that emits 'autoExpand' (and the other
watcher updating selectedCategory at lines noted) fails to reset the collapsed
state, so a parent-driven selection can remain collapsed; update the handlers
that respond to external selectedCategory changes (the watch on categoryTree
that emits('autoExpand') and the watcher that sets selectedCategory externally)
to also clear selectedCollapsed (the same flag cleared inside selectCategory()),
i.e., set selectedCollapsed.value = false whenever you programmatically change
or accept a new selectedCategory/rootKey so the newly selected node is expanded.
In `@src/components/searchbox/v2/NodeSearchContent.test.ts`:
- Around line 229-233: The test is currently finding the "Essentials" button
across the whole dialog and may match a sidebar control; change the lookup to
scope it inside the filter bar component (e.g., locate the NodeSearchFilterBar
via wrapper.findComponent(NodeSearchFilterBar) or a data-test selector for the
filter bar) then search that component's buttons for the one whose text includes
"Essentials" and trigger the click; update the test so references to
essentialsBtn come from the filter-bar wrapper rather than wrapper.findAll(...)
to ensure the root-filter control is exercised.
In `@src/components/searchbox/v2/NodeSearchContent.vue`:
- Around line 62-73: The per-row keyboard handlers currently rely on the global
selectedIndex and can become desynced when onPointerMove mutates selectedIndex
or when displayedResults changes; update the template handlers so each row's key
actions reference the row's own index (use the local index in
`@keydown.up/`@keydown.down/@keydown.enter and in the click handler instead of
reading selectedIndex), ensure navigateResults and selectCurrentResult accept
and use an index parameter (e.g., navigateResults(delta, fromKeyboard, rowIndex)
/ selectCurrentResult(rowIndex)), and add logic (a watcher or computed) to clamp
or reset selectedIndex whenever displayedResults changes so it is always within
0..displayedResults.length-1 (or set to -1 when empty) to avoid out-of-range
activation; keep onPointerMove behavior but ensure it focuses the hovered row or
updates selectedIndex consistently with the per-row handlers.
---
Duplicate comments:
In `@src/components/searchbox/v2/NodeSearchCategoryTreeNode.vue`:
- Around line 13-20: Add an aria-selected attribute to the treeitem button so
assistive tech can announce selection: on the element referenced by buttonEl
(the button with role="treeitem" and :data-testid using node.key) bind
aria-selected to the same check used for aria-current (e.g. selectedCategory ===
node.key) so it returns a boolean selection state; keep the existing
:aria-current and :aria-expanded logic (isExpanded) unchanged.
- Around line 23-40: The chevron reveal selectors currently target the
sidebar-level group (group-hover/categories and group-focus-within/categories),
causing chevrons to show for unrelated rows; add a local per-row group class on
the button (e.g., "group/category-row" or similar) in the root element that
renders each node (the element using cn(...) and `@click`="$emit('select',
node.key)"), then update the chevron class selectors in the <i> element (the
computed string that contains 'icon-[lucide--chevron-down] opacity-0 ...') to
use the new local group selectors (group-hover/<your-group> and
group-focus-within/<your-group>) so hover/focus only affect that row.
---
Nitpick comments:
In `@src/components/searchbox/v2/NodeSearchTypeFilterPopover.test.ts`:
- Around line 76-167: Add a test that opens the popover (reuse createWrapper()
and await openPopover(wrapper)), then simulate pressing the Escape key on the
popover or document (e.g., trigger a 'keydown' with key === 'Escape' on wrapper
or wrapper.element), await nextTick(), and assert that
wrapper.emitted('escapeClose') exists and has length 1; reference existing
helpers getOptions, createWrapper, openPopover, and wrapper.emitted to locate
where to add the test.
- Around line 9-49: Tests currently duplicate the component prop contract and
cast the mock filter with as unknown as, making type drift likely; change
createWrapper to type its props parameter from ComponentProps<typeof
NodeSearchTypeFilterPopover> (or derive the chip/selectedValues types from that)
and rebuild createMockChip so its returned object uses TypeScript's satisfies
with the concrete FilterChip and its filter type (e.g. return {...} satisfies
FilterChip and ensure filter conforms to FilterChip['filter'] or the concrete
filter interface) instead of using as unknown as, so the compiler will validate
the mock shape against the real component types (reference
NodeSearchTypeFilterPopover, createWrapper, createMockChip, FilterChip,
ComponentProps, and use satisfies).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3116984-109c-4568-82ba-a979aee6312b
📒 Files selected for processing (9)
src/components/input/MultiSelect.vuesrc/components/searchbox/v2/NodeSearchCategorySidebar.vuesrc/components/searchbox/v2/NodeSearchCategoryTreeNode.vuesrc/components/searchbox/v2/NodeSearchContent.test.tssrc/components/searchbox/v2/NodeSearchContent.vuesrc/components/searchbox/v2/NodeSearchTypeFilterPopover.test.tssrc/components/searchbox/v2/NodeSearchTypeFilterPopover.vuesrc/components/searchbox/v2/__test__/testUtils.tssrc/locales/en/main.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/searchbox/v2/NodeSearchTypeFilterPopover.vue
- src/components/searchbox/v2/test/testUtils.ts
pythongosssss
added
the
preview-cpu
- fix expanding after manual categ collapse
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/searchbox/v2/NodeSearchContent.test.ts (1)
136-146:⚠️ Potential issue | 🟡 MinorThese “no bookmarks” cases still fabricate a visible Favorites category.
mockBookmarks(false, ['placeholder'])keepsbookmarks.length > 0, so both tests exercise an impossible UI state: Favorites is rendered even though the test says no bookmarks exist. UsemockBookmarks(false)when you want to verify Favorites is hidden, and drive the empty-results case through a reachable path instead of a fake non-empty bookmark list.Based on learnings: "Do not write tests that just test the mocks; ensure tests fail when code behaves unexpectedly."
Also applies to: 535-543
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/searchbox/v2/NodeSearchContent.test.ts` around lines 136 - 146, The test is fabricating a visible Favorites category by calling mockBookmarks(false, ['placeholder']) which leaves bookmarks.length > 0; change calls to mockBookmarks(false) so the mocked bookmarks array is empty, then use createWrapper() and trigger the real UI path (e.g., clicking the favorites toggle via the '[data-testid="category-favorites"]' selector) to assert that Favorites is hidden and that the empty-results text ("No results") appears. Update both the test around the createMockNodeDef + mockBookmarks call shown and the duplicated case at lines ~535-543 to use mockBookmarks(false) and drive the empty-state through the real UI interactions (createWrapper, trigger, nextTick) rather than relying on a non-empty placeholder bookmark.
🤖 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/searchbox/v2/NodeSearchCategorySidebar.vue`:
- Around line 132-139: The sourceCategories computed currently always pushes the
'custom' category; change it to only include 'custom' when there are actual
custom nodes by checking the same node set used for categoryTree (e.g., inspect
nodeDefs / visibleNodeDefs for any items whose source/type indicates custom) or
accept a hasCustomNodes flag and gate the push on that boolean; update the
computed (sourceCategories) to use hasEssentialNodes and the new custom-presence
check instead of unconditionally pushing { id: 'custom', ... } so empty custom
categories are not rendered.
---
Duplicate comments:
In `@src/components/searchbox/v2/NodeSearchContent.test.ts`:
- Around line 136-146: The test is fabricating a visible Favorites category by
calling mockBookmarks(false, ['placeholder']) which leaves bookmarks.length > 0;
change calls to mockBookmarks(false) so the mocked bookmarks array is empty,
then use createWrapper() and trigger the real UI path (e.g., clicking the
favorites toggle via the '[data-testid="category-favorites"]' selector) to
assert that Favorites is hidden and that the empty-results text ("No results")
appears. Update both the test around the createMockNodeDef + mockBookmarks call
shown and the duplicated case at lines ~535-543 to use mockBookmarks(false) and
drive the empty-state through the real UI interactions (createWrapper, trigger,
nextTick) rather than relying on a non-empty placeholder bookmark.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7802cf32-1953-489b-8f0d-46d863cb0d16
📒 Files selected for processing (2)
src/components/searchbox/v2/NodeSearchCategorySidebar.vuesrc/components/searchbox/v2/NodeSearchContent.test.ts
- loop focus in dialog
- fix tab vs click chevron focus visibility
pythongosssss
added
preview-cpu
pythongosssss
added
preview-cpu
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/searchbox/v2/NodeSearchCategorySidebar.vue`:
- Around line 219-223: collapseCategory currently clears expandedCategory
entirely which over-collapses nested trees; change it so expandedCategory is set
to the parent path of the collapsed node instead of ''. In the
collapseCategory(categoryId: string) function compute the parent path of
categoryId (e.g., remove the last segment after the path delimiter used for
category ids; if no parent exists, set expandedCategory.value = ''), then assign
that parent path to expandedCategory.value while still updating
lastEmittedCategory = categoryId and selectedCategory.value = categoryId; use
the existing symbols collapseCategory, expandedCategory, lastEmittedCategory,
and selectedCategory to locate and update the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 628189fd-5b38-4db3-b384-508a23aabf2a
📒 Files selected for processing (4)
browser_tests/tests/nodeSearchBoxV2Extended.spec.tssrc/components/searchbox/v2/NodeSearchCategorySidebar.test.tssrc/components/searchbox/v2/NodeSearchCategorySidebar.vuesrc/components/searchbox/v2/NodeSearchCategoryTreeNode.vue
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/components/searchbox/v2/NodeSearchCategorySidebar.vue (2)
168-182: Variable shadowing:rootKeyin watch shadows the prop.Line 172 declares
const rootKey = nodes[0].keywhich shadows therootKeyprop from line 101. While this doesn't cause a bug (the prop isn't used in the watch), it reduces clarity.♻️ Suggested rename
(nodes) => { if (nodes.length === 1 && nodes[0].children?.length) { - const rootKey = nodes[0].key + const singleRootKey = nodes[0].key if ( - selectedCategory.value !== rootKey && - !selectedCategory.value.startsWith(rootKey + '/') + selectedCategory.value !== singleRootKey && + !selectedCategory.value.startsWith(singleRootKey + '/') ) { - emit('autoExpand', rootKey) + emit('autoExpand', singleRootKey) } } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/searchbox/v2/NodeSearchCategorySidebar.vue` around lines 168 - 182, The local variable rootKey inside the watch callback shadows the component prop rootKey; rename the local variable (for example to firstNodeKey or rootNodeKey) in the watch callback where it is defined and used (the watch on categoryTree that compares selectedCategory.value and emits 'autoExpand') and update all references in that callback (the const declaration, the selectedCategory.value checks, and the emit('autoExpand', ...) call) to use the new name so the prop rootKey is no longer shadowed.
125-125: Clarify intent:sourceCategoriesalways returns empty array.If source categories have been permanently removed from this sidebar, consider removing the
sourceCategoriescomputed, the related template section (lines 27-47), and simplifying the conditional styling on line 57. This would reduce dead code.Alternatively, if this is intentional scaffolding for future use, a brief comment would help.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/searchbox/v2/NodeSearchCategorySidebar.vue` at line 125, The computed sourceCategories currently always returns an empty array (sourceCategories) which leaves dead code in the template; either remove the computed property and the matching template block that renders source categories (the sidebar source categories section) and simplify any conditional class or v-if/v-show logic that depends on sourceCategories, or if you intend it as scaffolding, add a concise comment above the sourceCategories computed explaining it's intentionally empty and reserved for future use so reviewers understand it's deliberate; update any class bindings or conditionals that reference sourceCategories to reflect its removal or documented intent.src/components/searchbox/v2/NodeSearchCategorySidebar.test.ts (1)
468-478: Test relies on implementation detail of tree item structure.The test queries
closest('[role="treeitem"]')which depends on the internal DOM structure ofNodeSearchCategoryTreeNode. If that component's structure changes, this test could break even if behavior is correct.Consider adding a data-testid to the treeitem element in
NodeSearchCategoryTreeNodefor more robust selection, or document this coupling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/searchbox/v2/NodeSearchCategorySidebar.test.ts` around lines 468 - 478, The test in NodeSearchCategorySidebar.test.ts relies on the DOM structure by calling closest('[role="treeitem"]'), which couples it to NodeSearchCategoryTreeNode internals; instead add a stable hook: add a data-testid (e.g. data-testid="treeitem-<category>"/or generic data-testid="category-treeitem") to the treeitem element rendered by NodeSearchCategoryTreeNode, and update the test to select that data-testid (e.g. wrapper.find('[data-testid="category-treeitem"]')) rather than using closest, so the test targets the component contract not its internal markup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/searchbox/v2/NodeSearchCategorySidebar.test.ts`:
- Around line 468-478: The test in NodeSearchCategorySidebar.test.ts relies on
the DOM structure by calling closest('[role="treeitem"]'), which couples it to
NodeSearchCategoryTreeNode internals; instead add a stable hook: add a
data-testid (e.g. data-testid="treeitem-<category>"/or generic
data-testid="category-treeitem") to the treeitem element rendered by
NodeSearchCategoryTreeNode, and update the test to select that data-testid (e.g.
wrapper.find('[data-testid="category-treeitem"]')) rather than using closest, so
the test targets the component contract not its internal markup.
In `@src/components/searchbox/v2/NodeSearchCategorySidebar.vue`:
- Around line 168-182: The local variable rootKey inside the watch callback
shadows the component prop rootKey; rename the local variable (for example to
firstNodeKey or rootNodeKey) in the watch callback where it is defined and used
(the watch on categoryTree that compares selectedCategory.value and emits
'autoExpand') and update all references in that callback (the const declaration,
the selectedCategory.value checks, and the emit('autoExpand', ...) call) to use
the new name so the prop rootKey is no longer shadowed.
- Line 125: The computed sourceCategories currently always returns an empty
array (sourceCategories) which leaves dead code in the template; either remove
the computed property and the matching template block that renders source
categories (the sidebar source categories section) and simplify any conditional
class or v-if/v-show logic that depends on sourceCategories, or if you intend it
as scaffolding, add a concise comment above the sourceCategories computed
explaining it's intentionally empty and reserved for future use so reviewers
understand it's deliberate; update any class bindings or conditionals that
reference sourceCategories to reflect its removal or documented intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65a12518-129d-4a6c-b020-6cae9fe1c34f
📒 Files selected for processing (2)
src/components/searchbox/v2/NodeSearchCategorySidebar.test.tssrc/components/searchbox/v2/NodeSearchCategorySidebar.vue
dosubot
bot
added
the
size:XXL
Yourz
previously approved these changes
Mar 19, 2026
pythongosssss
dismissed stale reviews from Yourz and coderabbitai[bot]
via
8e5dc15
viva-jinyi
requested changes
Mar 20, 2026
| ) | ||
| " | ||
| > | ||
| <i class="icon-[lucide--component] h-full bg-amber-400" /> |
Member
There was a problem hiding this comment.
can we use font size instead of h-full?
Member
Author
There was a problem hiding this comment.
This was taken directly from src/renderer/extensions/vueNodes/components/NodeHeader.vue, i'd rather not change it in case there is some issue with it rendering there
| class="-mt-1 truncate text-xs text-muted-foreground" | ||
| > | ||
| {{ nodeDef.category.replaceAll('/', ' > ') }} | ||
| {{ nodeDef.category.replaceAll('/', ' / ') }} |
Member
There was a problem hiding this comment.
I think it'd be better to use computed!
| <p | ||
| v-if="nodeDef.description" | ||
| class="m-0 text-[11px] leading-normal font-normal text-muted-foreground" | ||
| class="m-0 max-h-[30vh] overflow-y-auto text-[11px] leading-normal font-normal text-muted-foreground" |
Member
There was a problem hiding this comment.
can we use text-xs instead of using text-[11px]?
Member
Author
There was a problem hiding this comment.
I'm not aware of the specifics of this, but since this was recently redesigned i'd imagine this was a specific part of the design
| > | ||
| {{ preset.label }} | ||
| </button> | ||
| <button |
Member
There was a problem hiding this comment.
I think we can use common button component
Member
Author
There was a problem hiding this comment.
Using the Button component means needing to fight against the styling it adds or adding a new variant that would only be used in this context - I think it adds more overhead for little benefit to use it here
| /> | ||
| </template> | ||
| <RovingFocusItem as-child> | ||
| <button |
Member
There was a problem hiding this comment.
it would be better to be replaced with button component
Member
Author
There was a problem hiding this comment.
Using the Button component means needing to fight against the styling it adds or adding a new variant that would only be used in this context - I think it adds more overhead for little benefit to use it here
| root: { | ||
| class: useSearchBoxV2 | ||
| ? 'w-4/5 min-w-[32rem] max-w-[56rem] border-0 bg-transparent mt-[10vh] max-md:w-[95%] max-md:min-w-0 overflow-visible' | ||
| ? 'w-4/5 min-w-[32rem] max-w-[56rem] border-0 bg-transparent max-md:w-[95%] max-md:min-w-0 overflow-visible' |
Member
There was a problem hiding this comment.
I think w-full max-w-[56rem] min-w-[32rem] max-md:min-w-0 bg-transparent border-0 would work too?
Member
Author
There was a problem hiding this comment.
It's a bit different, as that then removes the spacing around at smaller screen sizes, but that's probably not a bad thing, so done 👍
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
3 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.
Summary
Addresses feedback from the initial v2 node search implementation for improved UI and UX
Changes
Screenshots (if applicable)
zsearch.mp4
┆Issue is synchronized with this Notion page by Unito