new v2 node search box with categories #8745
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 Build Status✅ Build completed successfully! ⏰ Completed at: 02/11/2026, 10:48:25 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
|
Playwright: ✅ 525 passed, 0 failed · 2 flaky 📊 Browser Reports
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/components/searchbox/v2/NodeSearchContent.vue`:
- Around line 267-268: Add a brief clarifying comment to the empty 'essentials'
case in NodeSearchContent.vue (the switch/case handling search filters) to
indicate the behavior is intentional and that essentials filtering is applied
elsewhere (e.g., nodeOrganizationService), then leave the return [] as-is so
future readers understand it's not a placeholder.
In `@src/components/searchbox/v2/NodeSearchFilterChip.vue`:
- Around line 8-14: The icon-only remove button in NodeSearchFilterChip.vue
lacks an accessible name; update the <button> (the element that calls
`@click`="$emit('remove', $event)") to include a localized aria-label using the
i18n key (e.g. use $t('g.removeFilter')) so screen readers can announce it, and
add "removeFilter": "Remove filter" under the "g" namespace in
src/locales/en/main.json.
In `@src/components/searchbox/v2/NodeSearchListItem.vue`:
- Around line 10-16: The highlighted HTML from highlightQuery used in
NodeSearchListItem.vue must be sanitized with DOMPurify before binding to
v-html: import DOMPurify into the component, add a helper (e.g., sanitizeHtml)
or wrap highlightQuery calls in a computed/method that returns
DOMPurify.sanitize(highlightQuery(..., currentQuery)), and update the template
to use that sanitized output for both nodeDef.display_name and nodeDef.name
(respecting showIdName). Ensure the sanitizer is applied wherever
highlightQuery(...) is passed to v-html to prevent XSS.
In `@src/stores/workspace/searchBoxStore.ts`:
- Around line 13-16: Remove the exported ref useSearchBoxV2 from the workspace
searchBoxStore (or stop exporting it) since it's unused; if it actually needs to
remain public, wrap its initialization in a window-safe guard so it reads the
search param only when typeof window !== 'undefined' (e.g., compute the boolean
with that guard before creating the ref) and then export that safe value
instead; update the export list to reflect the removal or the new guarded symbol
so no SSR/test runtime access to window occurs.
🧹 Nitpick comments (12)
src/components/searchbox/v2/CategoryTreeNode.vue (1)
2-18: Use shared button components for category items.
These are interactive buttons; the repo standard is to use the shared button components for consistent styling and behavior.Based on learnings: In Vue files across the ComfyUI_frontend repo, when a button is needed, prefer the repo's common button components from src/components/button/ (IconButton.vue, TextButton.vue, IconTextButton.vue) over plain HTML elements.
src/components/searchbox/v2/NodeSearchCategorySidebar.vue (2)
5-15: Use shared button components for preset categories.
Prefer the shared button components for consistent behavior and theming.Based on learnings: In Vue files across the ComfyUI_frontend repo, when a button is needed, prefer the repo's common button components from src/components/button/ (IconButton.vue, TextButton.vue, IconTextButton.vue) over plain HTML elements.
62-63: Prefer a function declaration forstripRootPrefix.
This is a pure helper and fits the repo’s declaration preference.♻️ Suggested refactor
- const stripRootPrefix = (key: string) => key.replace(/^root\//, '') + function stripRootPrefix(key: string) { + return key.replace(/^root\//, '') + }As per coding guidelines: Do not use function expressions if it's possible to use function declarations instead.
src/components/searchbox/v2/NodeSearchCategorySidebar.test.ts (1)
35-45: Consider adding error handling for missing buttons.The
clickCategoryhelper silently continues if no button is found (btn?.trigger). For test reliability, consider throwing an error when the button is not found.💡 Proposed improvement
async function clickCategory( wrapper: ReturnType<typeof mount>, text: string, exact = false ) { const btn = wrapper .findAll('button') .find((b) => (exact ? b.text().trim() === text : b.text().includes(text))) + if (!btn) { + throw new Error(`Button with text "${text}" not found`) + } - await btn?.trigger('click') + await btn.trigger('click') await nextTick() }src/components/searchbox/NodeSearchBoxPopover.vue (1)
90-100: Consider extracting the magic number 1320 as a named constant.The window width threshold of 1320px for enabling node preview could benefit from being a named constant for clarity and maintainability.
💡 Proposed improvement
+const MIN_PREVIEW_WINDOW_WIDTH = 1320 + const enableNodePreview = computed( () => useSearchBoxV2.value && settingStore.get('Comfy.NodeSearchBoxImpl.NodePreview') && - windowWidth.value >= 1320 + windowWidth.value >= MIN_PREVIEW_WINDOW_WIDTH )src/components/searchbox/v2/NodeSearchFilterChip.vue (2)
8-14: Prefer shared button components for the remove control.Please use the repo’s common button components (e.g., IconButton/TextButton) instead of a raw
<button>to keep styling consistent with the design system.Based on learnings: In Vue files across the ComfyUI_frontend repo, when a button is needed, prefer the repo's common button components from src/components/button/ (IconButton.vue, TextButton.vue, IconTextButton.vue) over plain HTML elements.
19-23: Use reactive prop destructuring for defineProps.This aligns with the repo’s prop declaration convention.
♻️ Suggested fix
-defineProps<{ +const { label, text, typeColor } = defineProps<{ label: string text: string typeColor?: string -}>() +}>()As per coding guidelines: Define props using TypeScript style: const { prop1, prop2 = default } = defineProps<{ prop1: Type; prop2?: Type }>(); use reactive prop destructuring, not withDefaults or runtime props.
src/components/searchbox/v2/NodeSearchFilterBar.test.ts (1)
33-40: Type the test wrapper props with ComponentProps to avoid drift.This keeps test helpers aligned with the component’s real prop surface.
♻️ Suggested fix
import { beforeEach, describe, expect, it, vi } from 'vitest' import { nextTick } from 'vue' +import type { ComponentProps } from 'vue-component-type-helpers' import NodeSearchFilterBar from '@/components/searchbox/v2/NodeSearchFilterBar.vue' @@ - async function createWrapper(props = {}) { + type NodeSearchFilterBarProps = ComponentProps<typeof NodeSearchFilterBar> + + async function createWrapper( + props: Partial<NodeSearchFilterBarProps> = {} + ) { const wrapper = mount(NodeSearchFilterBar, { - props, + props: props as NodeSearchFilterBarProps, global: { plugins: [testI18n] } })Based on learnings: In test files (e.g., any file ending with .test.ts or .test.tsx), for test helpers like mountComponent, type the props parameter as Partial<ComponentProps>. When passing to mount, cast as ComponentProps instead of using Record<string, unknown> with a double-cast through unknown.
src/components/searchbox/v2/NodeSearchFilterBar.vue (1)
3-19: Swap raw buttons for shared button components.Use the common button components (e.g., TextButton/IconTextButton) for the chip controls and preserve
aria-pressed/classes via props or class slots.Based on learnings: In Vue files across the ComfyUI_frontend repo, when a button is needed, prefer the repo's common button components from src/components/button/ (IconButton.vue, TextButton.vue, IconTextButton.vue) over plain HTML elements.
browser_tests/fixtures/components/ComfyNodeSearchBoxV2.ts (1)
11-24: Use centralized TestIds constants for v2 selectors.Replace the hard-coded test IDs with constants from
browser_tests/fixtures/selectors.ts(add new entries if they don’t exist yet) to keep selectors consistent and easier to maintain.Based on learnings: Use the centralized TestIds from
browser_tests/fixtures/selectors.tsfor test element selection.src/components/searchbox/v2/NodeSearchContent.test.ts (1)
440-449: Consider adding a guard for clearer test failure messages.
findFilterBarButtonreturnsundefinedif no button matches, but callers use!to assert non-null. If the button isn't found (e.g., due to a regression in the component), the test fails with a confusing "Cannot read properties of undefined" rather than an explicit "button not found" message.💡 Optional: Add assertion helper for clearer failures
function findFilterBarButton(wrapper: VueWrapper, label: string) { - return wrapper + const button = wrapper .findAll('button[aria-pressed]') .find((b) => b.text() === label) + if (!button) { + throw new Error(`Filter bar button with label "${label}" not found`) + } + return button } async function enterFilterMode(wrapper: VueWrapper) { - await findFilterBarButton(wrapper, 'Input')!.trigger('click') + await findFilterBarButton(wrapper, 'Input').trigger('click') await nextTick() }src/components/searchbox/v2/NodeSearchContent.vue (1)
20-34: Inconsistent i18n function usage in template.Line 24 uses the destructured
t(...)function while lines 56 and 118 use the template-provided$t(...). Sincetis needed in the script for computed properties (lines 188-189), this is acceptable, but consider usingtconsistently throughout the template for uniformity.💅 Optional: Use consistent i18n function
<NodeSearchFilterChip v-for="filter in filters" :key="filter.filterDef.id" - :label="t(`g.${filter.filterDef.id}`)" + :label="$t(`g.${filter.filterDef.id}`)" :text="filter.value"Or alternatively, use
t(...)everywhere in the template for consistency with the script usage.
| case 'essentials': | ||
| return [] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "essentials" --type=vue --type=ts -C2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 97
🏁 Script executed:
cat -n src/components/searchbox/v2/NodeSearchContent.vue | sed -n '257,284p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1055
🏁 Script executed:
rg -n "essentials" -C2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 40978
🏁 Script executed:
rg -n "TODO|FIXME" src/components/searchbox/v2/NodeSearchContent.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 52
Add a clarifying comment for the empty 'essentials' case.
The essentials case returns an empty array without explanation. Since this appears intentional (as 'essentials' filtering is handled elsewhere), add a brief comment to clarify why it's empty—for example:
case 'essentials':
// Essentials filtering is handled by nodeOrganizationService; return empty for search
return []This prevents future confusion about whether this is a placeholder or intentional behavior.
🤖 Prompt for AI Agents
In `@src/components/searchbox/v2/NodeSearchContent.vue` around lines 267 - 268,
Add a brief clarifying comment to the empty 'essentials' case in
NodeSearchContent.vue (the switch/case handling search filters) to indicate the
behavior is intentional and that essentials filtering is applied elsewhere
(e.g., nodeOrganizationService), then leave the return [] as-is so future
readers understand it's not a placeholder.
Yourz
force-pushed
the
feat/node-library-sidebar-v2
branch
from
0081378 to
f6fdf54
Compare
- input/output/source filters - uses new preview, extracted price + provider badges - adds as ghost - tests
pythongosssss
force-pushed
the
pysssss/v2-node-search
branch
from
4d97c77 to
6f4373c
Compare
…to pysssss/v2-node-search
pythongosssss
added
the
preview-cpu
1 participant
Summary
Changes
Review Focus
Screenshots (if applicable)
┆Issue is synchronized with this Notion page by Unito