-
Notifications
You must be signed in to change notification settings - Fork 458
feat: add dynamic Fuse.js options loading for template filtering #7822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' | ||
| import { nextTick, ref } from 'vue' | ||
| import type { IFuseOptions } from 'fuse.js' | ||
|
|
||
| import type { TemplateInfo } from '@/platform/workflow/templates/types/template' | ||
|
|
||
|
|
@@ -29,12 +30,20 @@ vi.mock('@/platform/telemetry', () => ({ | |
| })) | ||
| })) | ||
|
|
||
| const mockGetFuseOptions = vi.hoisted(() => vi.fn()) | ||
| vi.mock('@/scripts/api', () => ({ | ||
| api: { | ||
| getFuseOptions: mockGetFuseOptions | ||
| } | ||
| })) | ||
|
|
||
| const { useTemplateFiltering } = | ||
| await import('@/composables/useTemplateFiltering') | ||
|
|
||
| describe('useTemplateFiltering', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| mockGetFuseOptions.mockResolvedValue(null) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
|
|
@@ -258,4 +267,118 @@ describe('useTemplateFiltering', () => { | |
| 'beta-pro' | ||
| ]) | ||
| }) | ||
|
|
||
| describe('loadFuseOptions', () => { | ||
| it('updates fuseOptions when getFuseOptions returns valid options', async () => { | ||
| const templates = ref<TemplateInfo[]>([ | ||
| { | ||
| name: 'test-template', | ||
| description: 'Test template', | ||
| mediaType: 'image', | ||
| mediaSubtype: 'png' | ||
| } | ||
| ]) | ||
|
|
||
| const customFuseOptions: IFuseOptions<TemplateInfo> = { | ||
| keys: [ | ||
| { name: 'name', weight: 0.5 }, | ||
| { name: 'description', weight: 0.5 } | ||
| ], | ||
| threshold: 0.4, | ||
| includeScore: true | ||
| } | ||
|
|
||
| mockGetFuseOptions.mockResolvedValueOnce(customFuseOptions) | ||
|
|
||
| const { loadFuseOptions, filteredTemplates } = | ||
| useTemplateFiltering(templates) | ||
|
|
||
| await loadFuseOptions() | ||
|
|
||
| expect(mockGetFuseOptions).toHaveBeenCalledTimes(1) | ||
| expect(filteredTemplates.value).toBeDefined() | ||
| }) | ||
|
|
||
| it('does not update fuseOptions when getFuseOptions returns null', async () => { | ||
| const templates = ref<TemplateInfo[]>([ | ||
| { | ||
| name: 'test-template', | ||
| description: 'Test template', | ||
| mediaType: 'image', | ||
| mediaSubtype: 'png' | ||
| } | ||
| ]) | ||
|
|
||
| mockGetFuseOptions.mockResolvedValueOnce(null) | ||
|
|
||
| const { loadFuseOptions, filteredTemplates } = | ||
| useTemplateFiltering(templates) | ||
|
|
||
| const initialResults = filteredTemplates.value | ||
|
|
||
| await loadFuseOptions() | ||
|
|
||
| expect(mockGetFuseOptions).toHaveBeenCalledTimes(1) | ||
| expect(filteredTemplates.value).toEqual(initialResults) | ||
| }) | ||
|
|
||
| it('handles errors when getFuseOptions fails', async () => { | ||
| const templates = ref<TemplateInfo[]>([ | ||
| { | ||
| name: 'test-template', | ||
| description: 'Test template', | ||
| mediaType: 'image', | ||
| mediaSubtype: 'png' | ||
| } | ||
| ]) | ||
|
|
||
| mockGetFuseOptions.mockRejectedValueOnce(new Error('Network error')) | ||
|
|
||
| const { loadFuseOptions, filteredTemplates } = | ||
| useTemplateFiltering(templates) | ||
|
|
||
| const initialResults = filteredTemplates.value | ||
|
|
||
| await expect(loadFuseOptions()).rejects.toThrow('Network error') | ||
| expect(filteredTemplates.value).toEqual(initialResults) | ||
| }) | ||
|
|
||
| it('recreates Fuse instance when fuseOptions change', async () => { | ||
| const templates = ref<TemplateInfo[]>([ | ||
| { | ||
| name: 'searchable-template', | ||
| description: 'This is a searchable template', | ||
| mediaType: 'image', | ||
| mediaSubtype: 'png' | ||
| }, | ||
| { | ||
| name: 'another-template', | ||
| description: 'Another template', | ||
| mediaType: 'image', | ||
| mediaSubtype: 'png' | ||
| } | ||
| ]) | ||
|
|
||
| const { loadFuseOptions, searchQuery, filteredTemplates } = | ||
| useTemplateFiltering(templates) | ||
|
|
||
| const customFuseOptions = { | ||
| keys: [{ name: 'name', weight: 1.0 }], | ||
| threshold: 0.2, | ||
| includeScore: true, | ||
| includeMatches: true | ||
| } | ||
|
|
||
| mockGetFuseOptions.mockResolvedValueOnce(customFuseOptions) | ||
|
|
||
| await loadFuseOptions() | ||
| await nextTick() | ||
|
|
||
| searchQuery.value = 'searchable' | ||
| await nextTick() | ||
|
|
||
| expect(filteredTemplates.value.length).toBeGreaterThan(0) | ||
| expect(mockGetFuseOptions).toHaveBeenCalledTimes(1) | ||
| }) | ||
|
Comment on lines
+346
to
+382
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Test assertion could be strengthened to verify Fuse options actually affect search behavior. The test at line 380 only asserts 🔎 Stronger test approach it('recreates Fuse instance when fuseOptions change', async () => {
const templates = ref<TemplateInfo[]>([
{
name: 'searchable-template',
description: 'This is a searchable template',
mediaType: 'image',
mediaSubtype: 'png'
},
{
name: 'another-template',
description: 'Another template',
mediaType: 'image',
mediaSubtype: 'png'
}
])
const { loadFuseOptions, searchQuery, filteredTemplates } =
useTemplateFiltering(templates)
+ // Capture baseline results with default options
+ searchQuery.value = 'searchable'
+ await nextTick()
+ const resultsBeforeOptionsLoad = [...filteredTemplates.value]
const customFuseOptions = {
- keys: [{ name: 'name', weight: 1.0 }],
- threshold: 0.2,
+ keys: [{ name: 'description', weight: 1.0 }], // Different key priority
+ threshold: 0.8, // More lenient threshold
includeScore: true,
includeMatches: true
}
mockGetFuseOptions.mockResolvedValueOnce(customFuseOptions)
await loadFuseOptions()
await nextTick()
- searchQuery.value = 'searchable'
+ // Re-trigger search with new options
+ searchQuery.value = 'another'
await nextTick()
- expect(filteredTemplates.value.length).toBeGreaterThan(0)
+ // Verify results changed due to different options
+ expect(filteredTemplates.value).not.toEqual(resultsBeforeOptionsLoad)
+ expect(filteredTemplates.value.length).toBeGreaterThan(0)
expect(mockGetFuseOptions).toHaveBeenCalledTimes(1)
})🤖 Prompt for AI Agents |
||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,12 +1,28 @@ | ||||||||||||||||||||||||||||||||||||
| import { refDebounced, watchDebounced } from '@vueuse/core' | ||||||||||||||||||||||||||||||||||||
| import Fuse from 'fuse.js' | ||||||||||||||||||||||||||||||||||||
| import type { IFuseOptions } from 'fuse.js' | ||||||||||||||||||||||||||||||||||||
| import { computed, ref, watch } from 'vue' | ||||||||||||||||||||||||||||||||||||
| import type { Ref } from 'vue' | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| import { useSettingStore } from '@/platform/settings/settingStore' | ||||||||||||||||||||||||||||||||||||
| import { useTelemetry } from '@/platform/telemetry' | ||||||||||||||||||||||||||||||||||||
| import type { TemplateInfo } from '@/platform/workflow/templates/types/template' | ||||||||||||||||||||||||||||||||||||
| import { debounce } from 'es-toolkit/compat' | ||||||||||||||||||||||||||||||||||||
| import { api } from '@/scripts/api' | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Fuse.js configuration for fuzzy search | ||||||||||||||||||||||||||||||||||||
| const defaultFuseOptions: IFuseOptions<TemplateInfo> = { | ||||||||||||||||||||||||||||||||||||
| keys: [ | ||||||||||||||||||||||||||||||||||||
| { name: 'name', weight: 0.3 }, | ||||||||||||||||||||||||||||||||||||
| { name: 'title', weight: 0.3 }, | ||||||||||||||||||||||||||||||||||||
| { name: 'description', weight: 0.1 }, | ||||||||||||||||||||||||||||||||||||
| { name: 'tags', weight: 0.2 }, | ||||||||||||||||||||||||||||||||||||
| { name: 'models', weight: 0.3 } | ||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||
| threshold: 0.33, | ||||||||||||||||||||||||||||||||||||
| includeScore: true, | ||||||||||||||||||||||||||||||||||||
| includeMatches: true | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| export function useTemplateFiltering( | ||||||||||||||||||||||||||||||||||||
| templates: Ref<TemplateInfo[]> | TemplateInfo[] | ||||||||||||||||||||||||||||||||||||
|
|
@@ -31,26 +47,14 @@ export function useTemplateFiltering( | |||||||||||||||||||||||||||||||||||
| | 'model-size-low-to-high' | ||||||||||||||||||||||||||||||||||||
| >(settingStore.get('Comfy.Templates.SortBy')) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const fuseOptions = ref<IFuseOptions<TemplateInfo>>(defaultFuseOptions) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const templatesArray = computed(() => { | ||||||||||||||||||||||||||||||||||||
| const templateData = 'value' in templates ? templates.value : templates | ||||||||||||||||||||||||||||||||||||
| return Array.isArray(templateData) ? templateData : [] | ||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Fuse.js configuration for fuzzy search | ||||||||||||||||||||||||||||||||||||
| const fuseOptions = { | ||||||||||||||||||||||||||||||||||||
| keys: [ | ||||||||||||||||||||||||||||||||||||
| { name: 'name', weight: 0.3 }, | ||||||||||||||||||||||||||||||||||||
| { name: 'title', weight: 0.3 }, | ||||||||||||||||||||||||||||||||||||
| { name: 'description', weight: 0.1 }, | ||||||||||||||||||||||||||||||||||||
| { name: 'tags', weight: 0.2 }, | ||||||||||||||||||||||||||||||||||||
| { name: 'models', weight: 0.3 } | ||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||
| threshold: 0.33, | ||||||||||||||||||||||||||||||||||||
| includeScore: true, | ||||||||||||||||||||||||||||||||||||
| includeMatches: true | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const fuse = computed(() => new Fuse(templatesArray.value, fuseOptions)) | ||||||||||||||||||||||||||||||||||||
| const fuse = computed(() => new Fuse(templatesArray.value, fuseOptions.value)) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const availableModels = computed(() => { | ||||||||||||||||||||||||||||||||||||
| const modelSet = new Set<string>() | ||||||||||||||||||||||||||||||||||||
|
|
@@ -237,6 +241,13 @@ export function useTemplateFiltering( | |||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||
| }, 500) | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| const loadFuseOptions = async () => { | ||||||||||||||||||||||||||||||||||||
| const fetchedOptions = await api.getFuseOptions() | ||||||||||||||||||||||||||||||||||||
| if (fetchedOptions) { | ||||||||||||||||||||||||||||||||||||
| fuseOptions.value = fetchedOptions | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+244
to
+249
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Error handling is still missing despite being marked as addressed. The As currently implemented, a fetch failure in 🔎 Recommended fix with graceful fallback const loadFuseOptions = async () => {
+ try {
const fetchedOptions = await api.getFuseOptions()
if (fetchedOptions) {
fuseOptions.value = fetchedOptions
}
+ } catch (error) {
+ console.warn('Failed to load Fuse options, using defaults:', error)
+ // fuseOptions.value remains at defaultFuseOptions
+ }
}Based on coding guidelines requiring proper error handling in code. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Watch for filter changes and track them | ||||||||||||||||||||||||||||||||||||
| watch( | ||||||||||||||||||||||||||||||||||||
| [searchQuery, selectedModels, selectedUseCases, selectedRunsOn, sortBy], | ||||||||||||||||||||||||||||||||||||
|
|
@@ -309,6 +320,7 @@ export function useTemplateFiltering( | |||||||||||||||||||||||||||||||||||
| resetFilters, | ||||||||||||||||||||||||||||||||||||
| removeModelFilter, | ||||||||||||||||||||||||||||||||||||
| removeUseCaseFilter, | ||||||||||||||||||||||||||||||||||||
| removeRunsOnFilter | ||||||||||||||||||||||||||||||||||||
| removeRunsOnFilter, | ||||||||||||||||||||||||||||||||||||
| loadFuseOptions | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 904
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 159
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 538
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 133
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1078
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1047
Test correctly expects error propagation - but implementation lacks error handling.
The test is correct:
loadFuseOptions()has no try-catch and will propagate errors fromapi.getFuseOptions(). However, the implementation violates the coding guideline requiring "Implement proper error handling in code" for TypeScript/Vue files.The function should either:
Currently, the function propagates errors without context, and callers in
WorkflowTemplateSelectorDialog.vuerely onuseAsyncStatefor error handling rather than explicit try-catch. While functional due to the wrapper, this lacks explicit error handling as required by guidelines.