feat: replace model library tree with model browser dialog#8385
Closed
jay-comfy wants to merge 32 commits intoComfy-Org:mainfrom
Closed
feat: replace model library tree with model browser dialog#8385jay-comfy wants to merge 32 commits intoComfy-Org:mainfrom
jay-comfy wants to merge 32 commits intoComfy-Org:mainfrom
Conversation
dosubot
bot
added
the
size:XXL
Contributor
📝 WalkthroughWalkthroughThis PR introduces a comprehensive Model Browser feature enabling users to browse, search, filter, and select models from the UI. It includes a modal dialog with model discovery, metadata viewing, filtering, sorting, keyboard navigation, and bookmarking capabilities. Additionally, it refactors node creation logic into a centralized utility and integrates the browser into the sidebar. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SideToolbar
participant useModelBrowserDialog as Dialog Composable
participant dialogStore as Dialog Store
participant ModelBrowserDialog as Browser Dialog
participant ModelCard
participant LocalModelInfoPanel
participant nodeCreation as Node Creation
User->>SideToolbar: Click model-library tab
SideToolbar->>useModelBrowserDialog: show()
useModelBrowserDialog->>dialogStore: Configure and display dialog<br/>(ModelBrowserDialog component)
dialogStore->>ModelBrowserDialog: Render modal with models
Note over ModelBrowserDialog: Load models, setup filters/sort
User->>ModelBrowserDialog: Search/filter models
ModelBrowserDialog->>ModelCard: Render filtered models
User->>ModelCard: Click or focus on model
ModelCard->>LocalModelInfoPanel: Display selected model details
LocalModelInfoPanel->>User: Show metadata & "Use" button
User->>LocalModelInfoPanel: Click "Use" button
LocalModelInfoPanel->>ModelBrowserDialog: Emit select event
ModelBrowserDialog->>nodeCreation: createModelNode(model)
nodeCreation->>dialogStore: Close dialog
User->>User: Node created in canvas
sequenceDiagram
participant App as Application
participant createNodeFromModel as Node Creation Utility
participant modelStore as Model Store
participant litegraphService as LiteGraph Service
participant workflowStore as Workflow Store
participant canvas as Canvas/Graph
App->>createNodeFromModel: createNodeFromModel(category, filename, itemId, options)
createNodeFromModel->>modelStore: Resolve provider for category
alt Provider found
createNodeFromModel->>litegraphService: Get position (provided or canvas center)
createNodeFromModel->>litegraphService: createNode(provider.nodeDef)
alt Node created successfully
createNodeFromModel->>litegraphService: Locate widget on node
alt Widget found
createNodeFromModel->>litegraphService: Set widget value to filename
createNodeFromModel->>workflowStore: Determine target graph (main or subgraph)
createNodeFromModel->>canvas: Add node to graph
createNodeFromModel->>App: Return Result{ success, value: LGraphNode }
else Widget missing
createNodeFromModel->>App: Return Result{ success: false, error: MISSING_WIDGET }
end
else Node creation failed
createNodeFromModel->>App: Return Result{ success: false, error: NODE_CREATION_FAILED }
end
else Provider not found
createNodeFromModel->>App: Return Result{ success: false, error: NO_PROVIDER }
end
Possibly related PRs
Suggested reviewers
✨ Finishing touches
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
⏰ Started at: 01/29/2026, 12:38:57 AM UTC 🚀 Building Storybook
⏱️ Please wait while the Storybook build is in progress... |
🎭 Playwright Tests: ⏳ Running...Tests started at 01/29/2026, 12:38:56 AM UTC 📊 Browser Tests
|
coderabbitai
bot
requested review from
KarryCharon,
Yorha4D,
jtydhr88,
luke-mino-altherr and
shinshin86
![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: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/widget/panel/LeftSidePanel.vue (1)
10-16: Handle collapsible groups without titles safely.Line 16 still indexes
collapsedGroups[item.title]even when a group has no title. WithNavTitlenow gated, any title-less collapsible group can end up sharing theundefinedkey and has no toggle. Either guarantee titles for collapsible groups or guard the collapsed-state lookup whenitem.titleis falsy.🔧 Minimal guard to avoid `undefined` key usage
- <template v-if="!item.collapsible || !collapsedGroups[item.title]"> + <template + v-if=" + !item.collapsible || + !item.title || + !collapsedGroups[item.title] + " + >
🤖 Fix all issues with AI agents
In `@src/platform/assets/utils/createModelNodeFromAsset.ts`:
- Around line 116-126: The function calling createNodeFromModel returns
result.error with a type assertion to NodeCreationError; instead explicitly
build and return a NodeCreationError object by mapping result.error.message,
result.error.code (and any other expected NodeCreationError fields) and set
assetId = result.error.itemId, omitting itemId, instead of using "as
NodeCreationError"; update the error return in createModelNodeFromAsset to
construct the exact shape required (refer to createNodeFromModel's result and
the NodeCreationError type) so no type assertion is needed.
In `@src/platform/models/browser/components/LocalModelInfoPanel.vue`:
- Around line 31-42: Replace the raw primary <button> in LocalModelInfoPanel.vue
with the project's common button component (e.g., IconTextButton or IconButton)
from src/components/button/, import that component into the LocalModelInfoPanel
component, and bind the same props/events (preserve the click handler
emit('use', model), icon class i="icon-[lucide--plus]", text
$t('modelBrowser.use'), and accessibility/disabled behavior) so styling/theming
follows the design system; also audit accordion headers in this file and swap
plain buttons there to the common button components where applicable.
- Around line 7-18: The preview fallback state isn't reset when the model
changes—because imageError remains true after a failed load, subsequent model
previews are blocked; in LocalModelInfoPanel.vue add logic to reset imageError
(set imageError = false) whenever the model or its previewUrl changes (e.g., add
a watch on model.previewUrl or model.id, or reset inside a computed/updated
hook) so the <img> can attempt to load the new preview; reference the imageError
reactive flag and the model.previewUrl used in the <img> tag to locate where to
add the reset.
- Around line 238-244: The textarea currently uses a hardcoded placeholder
string; update the placeholder binding to use the i18n translation instead of
the literal text (keep the existing model.description fallback). Replace the
placeholder expression on the <textarea> (the element binding referencing
model.description) with a call to the app's translation helper (e.g. $t or
i18n.t) using the existing translation key that corresponds to "No description
set" so it reads something like :placeholder="model.description ||
$t('existing_translation_key')" while leaving the element, disabled attribute,
and :value="model.description" intact.
In `@src/platform/models/browser/components/ModelBrowserDialog.test.ts`:
- Around line 29-71: Enable fake timers and restore mocks/timers to stabilize
the debounced search tests: call vi.useFakeTimers() in beforeEach (alongside the
existing Element.prototype.scrollIntoView and getComputedStyle spy setup),
replace any real setTimeout-awaits in tests with vi.advanceTimersByTime(300) (or
the debounce duration) and await nextTick() to let reactive updates run (e.g.,
in tests using useModelBrowserFiltering/searchQuery/filteredModels), and add an
afterEach that calls vi.useRealTimers() and vi.restoreAllMocks() to clean up
global mocks and timers between tests.
In `@src/platform/models/browser/components/ModelBrowserDialog.vue`:
- Around line 358-395: The dialog can remain open after a successful selection
because handleClose() returns early if isRightPanelOpen.value is true; update
handleModelSelect so that on result.success you first ensure the right panel is
closed (set isRightPanelOpen.value = false) before calling handleClose() or
directly emit('close') after emit('select', model.original), so the dialog
always closes after a successful node creation; refer to handleModelSelect,
handleClose, isRightPanelOpen, and emit('select')/emit('close') when making the
change.
- Around line 395-401: The error branch in ModelBrowserDialog.vue currently only
logs to console; update the else branch in the
createNodeFromModel/createModelNode flow to show a user-facing toast using the
existing useToast pattern: import or call useToast, build a clear message that
includes result.error.message (e.g., "Failed to create model: <error>. Please
try again."), call toast.error(...) so the dialog remains open for retry, and
keep the console.error for debugging; reference the existing
createModelNode/createNodeFromModel logic and the useToast utility when
implementing this change.
In `@src/platform/models/browser/components/ModelBrowserFilterBar.vue`:
- Around line 58-75: Replace the verbose paired prop+emit declarations for the
v-model bindings with defineModel to reduce boilerplate: consolidate
selectedFileFormats/ 'update:selectedFileFormats', selectedModelTypes/
'update:selectedModelTypes', sortBy/ 'update:sortBy', sortDirection/
'update:sortDirection', and viewMode/ 'update:viewMode' into defineModel calls
(keeping types SelectOption[] and the union types 'name'|'size'|'modified' and
'asc'|'desc' and 'grid'|'list'); update the component to import/use defineModel
per repo convention and remove the corresponding defineProps/defineEmits entries
for those v-model bindings while leaving non-v-model props
(availableFileFormats, availableModelTypes, sortOptions) as-is.
In `@src/platform/models/browser/components/ModelBrowserSortButton.vue`:
- Around line 5-32: Replace the raw <button> elements in
ModelBrowserSortButton.vue with the shared button component that matches the UI
(use IconTextButton.vue or TextButton.vue with an icon when a label is present)
to ensure design-system consistency: map the current props and behavior (iterate
over sortOptions, use :key="option.value", compute class/active state from
sortBy and sortDirection, keep the aria-label logic, call
handleSortClick(option.value) on click, and render getDisplayLabel(option) plus
the icon state) into the chosen shared component's API so styles and theming
come from src/components/button/* instead of a plain button element.
- Around line 16-29: Remove the redundant aria-label on the sort button and
instead add aria-pressed to reflect active state; keep the visible label
rendered by <span>{{ getDisplayLabel(option) }}</span> and add a visually-hidden
span for screen readers containing the localized direction text using vue-i18n
keys (e.g., $t('modelBrowser.sortAscending') /
$t('modelBrowser.sortDescending')) based on the component's sortBy and
sortDirection, updating any template logic around handleSortClick, sortBy,
sortDirection and getDisplayLabel to determine when to render the localized
direction; also add the new modelBrowser.sortAscending and
modelBrowser.sortDescending strings to src/locales/en/main.json.
In `@src/platform/models/browser/components/ModelBrowserStates.test.ts`:
- Around line 27-28: Replace assertions that select by CSS utility classes
(e.g., wrapper.find('.animate-spin')) in ModelBrowserStates.test.ts with
accessibility- or content-based queries: update the component to expose the
spinner via a semantic attribute (role="status" or aria-label="loading-spinner")
if it doesn't already, then change tests to assert presence with
wrapper.find('[role="status"]') or
wrapper.find('[aria-label="loading-spinner"]') and verify visible text like
wrapper.text().toContain('modelBrowser.loading') or the retry button label;
apply the same refactor for the other occurrences noted (the class-based checks
around the other blocks) so tests no longer depend on Tailwind/Lucide class
names.
In `@src/platform/models/browser/components/ModelCard.test.ts`:
- Around line 178-192: The test 'should show gradient fallback when image fails
to load' in ModelCard should avoid asserting on Tailwind class names; instead,
trigger the image error and assert behavior: either check that the original
<img> is hidden/removed (e.g., expect(img.exists() === false or
expect(img.attributes('hidden')/style) ) or assert the presence of a
semantic/test hook like a data-testid or aria-label on the fallback (e.g.,
data-testid="image-fallback" or aria-label="model image fallback") — update the
test to look for that behavior or test-hook rather than the '.bg-gradient-to-br'
class, and if needed add a data-testid/aria attribute to the ModelCard fallback
element to make the assertion stable.
- Around line 115-128: The test "should emit select event when Use button is
clicked" in ModelCard.test.ts currently picks the Use button by array index
(buttons[buttons.length - 1]), which is fragile; update the selector to a stable
target by locating the button using a data attribute or accessible label (e.g.,
give the Use button a data-testid or aria-label in the ModelCard component and
use wrapper.find('[data-testid="use-button"]') or
wrapper.get('button[aria-label="Use"]') in the test) or find by visible text
content, then trigger click and assert the emitted 'select' event as before;
update references to wrapper.findAll/find/get and the model variable
accordingly.
In `@src/platform/models/browser/components/ModelCard.vue`:
- Around line 2-20: The button-like div with role="button" needs Space key
activation; add a key handler so pressing Space triggers the same action as
click. Update the component's template to listen for the Space key (e.g., add
`@keydown.space.prevent`="handleClick") alongside the existing `@keydown.enter` and
`@click` handlers so Space calls handleClick (or emits the same event) and
prevents default scrolling; keep handleSelect for dblclick as-is.
- Around line 70-94: Replace the two raw <button> elements with the repo shared
button components (IconButton / IconTextButton) by importing and registering
them, pass the aria-label prop and use `@click.stop` to call handleToggleBookmark
and emit('show-info', model) respectively, and map the icon state (isBookmarked
? bookmark-check : bookmark) into the icon prop or icon slot so the first button
toggles icon based on isBookmarked and the second shows the file-text icon;
ensure you remove the duplicated utility classes from the raw buttons and rely
on the shared components' styling while preserving focus/disabled behavior.
In `@src/platform/models/browser/components/ViewModeToggle.vue`:
- Around line 5-31: Replace the raw <button> elements in ViewModeToggle.vue with
the shared IconButton (or IconTextButton) component from src/components/button/,
wiring its click to emit('update:modelValue', 'grid'|'list'), pass the active
state (e.g., :active="modelValue === 'grid'") so the component applies proper
theming, and ensure the rendered control communicates toggle semantics via
aria-pressed (or a pressed prop) reflecting modelValue; also ensure the button
component is used with non-submit behavior (type="button" or equivalent prop) to
avoid accidental form submits.
- Around line 14-45: Replace the separate defineProps and defineEmits usage with
a single defineModel call to align with the v-model contract: remove the
existing defineProps<{ modelValue: 'grid' | 'list' }>() and defineEmits<{
'update:modelValue': [value: 'grid' | 'list'] }>() and instead declare
defineModel<'grid' | 'list'>('modelValue') (or equivalent pattern used in your
codebase) so the modelValue type and the 'update:modelValue' emit are unified;
update any references to modelValue and the emit event to rely on the new
defineModel binding in ViewModeToggle.vue.
In `@src/platform/models/browser/composables/useModelBookmarks.ts`:
- Around line 63-116: Convert the pure helper arrow functions isBookmarked and
filterBookmarkedModels into function declarations (e.g., function
isBookmarked(modelId: string): boolean { ... } and function
filterBookmarkedModels(models: EnrichedModel[]): EnrichedModel[] { ... })
preserving their existing bodies, return types, and usage of
bookmarkedModelIds.value; keep toggleBookmark, addBookmark, removeBookmark,
bookmarkedIds, and bookmarkCount as-is so external callers and reactive behavior
remain unchanged.
In `@src/platform/models/browser/composables/useModelBrowserFiltering.ts`:
- Around line 96-107: The computed searchFiltered currently creates a new Fuse
instance on every evaluation; memoize the Fuse index per modelTypeFiltered value
by creating a separate computed (or ref) that builds the Fuse instance (new
Fuse(modelTypeFiltered.value, fuseOptions)) whenever modelTypeFiltered changes,
then have searchFiltered reuse that memoized Fuse to call fuse.search(query) and
map results; reference the searchFiltered computed, the modelTypeFiltered
computed/ref, fuseOptions, and the Fuse constructor to locate and replace the
instantiation logic.
In `@src/platform/models/browser/composables/useModelKeyboardNav.test.ts`:
- Around line 1-29: The tests mutate global behavior by assigning
Element.prototype.scrollIntoView and spying on window.getComputedStyle in nested
beforeEach; add an afterEach that restores the original prototype and clears
spies/mocks (capture the original Element.prototype.scrollIntoView into a local
variable in the top-level describe and reassign it back in afterEach, and call
vi.restoreAllMocks() or vi.resetAllMocks() to remove the vi.spyOn on
window.getComputedStyle) so global DOM mocks are cleaned up between tests and no
leaked state remains.
In `@src/platform/models/browser/composables/useModelKeyboardNav.ts`:
- Around line 101-120: In useModelKeyboardNav.ts inside the ArrowUp handler, add
the same fallback for when no item is focused in grid mode: after computing
columns and the currentIndex check in the grid branch (where you call
getGridColumns(), check currentIndex >= columns and set
focusedModel/gridItems[...] and scrollFocusedIntoView()), add an else-if that
handles currentIndex === -1 && gridItems.value.length > 0 and sets
focusedModel.value = gridItems.value[0].model followed by
scrollFocusedIntoView(); this mirrors the list-mode fallback and keeps behavior
consistent across viewMode, using the existing symbols (viewMode,
getGridColumns, currentIndex, focusedModel, gridItems, scrollFocusedIntoView).
- Around line 46-55: The getGridColumns() function uses a fragile global
selector document.querySelector('.grid') which may match unrelated elements;
modify the composable to accept a container reference or element (e.g., pass a
containerRef or HTMLElement into the composable/getGridColumns) and use that ref
instead of document.querySelector, or change the selector to a specific
attribute like '[data-model-browser-grid]' and ensure the parent component adds
data-model-browser-grid to the model browser grid container; update any callers
to provide the ref/attribute so getGridColumns (and its use of
window.getComputedStyle and viewMode.value) reliably targets the intended grid
container.
In `@src/platform/models/browser/composables/useModelLoader.ts`:
- Around line 7-14: Add a short module-level comment above the modelCache and
loadingPromise declarations explaining that these are intentionally shared
module-level state to enable cross-instance caching, request deduplication, and
TTL-based invalidation (referencing modelCache.CACHE_TTL), and that clearCache()
will clear this shared cache for all instances; keep the comment concise and
mention intended behavior to avoid surprising future maintainers.
- Around line 48-57: Replace the manual nested loops that build enrichedModels
with a flatMap-based transformation: iterate modelStore.modelFolders and for
each folder map its Object.values(folder.models) to
transformToEnrichedModel(model, folder.directory), then flatten into the
enrichedModels array (e.g., const enrichedModels: EnrichedModel[] =
modelStore.modelFolders.flatMap(folder => Object.values(folder.models).map(m =>
transformToEnrichedModel(m, folder.directory)))); this keeps the same logic but
simplifies the code around enrichedModels, modelStore.modelFolders, and
transformToEnrichedModel.
In `@src/platform/models/browser/utils/createModelNode.ts`:
- Around line 49-56: The error mapping in createModelNode currently uses a type
assertion and preserves itemId; instead, construct a new NodeCreationError
object explicitly by copying needed fields from result.error, assign modelId:
result.error.itemId, and omit itemId (do not use "as NodeCreationError"); return
{ success: false, error: newError } so the shape matches NodeCreationError
without type assertions.
In `@src/platform/models/browser/utils/modelTransform.ts`:
- Around line 88-106: Remove the dead function loadModelMetadata from
src/platform/models/browser/utils/modelTransform.ts since it is neither called
nor exported; delete the entire async function declaration and its body, and
then run a quick build/TS check to remove or adjust any now-unused imports/types
(e.g., EnrichedModel) and remove the function from any exports or test stubs if
present.
In `@src/platform/models/browser/utils/modelTypeIcons.ts`:
- Around line 5-26: The icons map is being recreated on every call inside
getModelTypeIcon causing unnecessary allocations; move the const icons object
out to module scope (e.g., define a top-level ICONS constant) and have
getModelTypeIcon(type: string) simply reference that ICONS constant and return
ICONS[type] || 'icon-[comfy--ai-model]'; keep the same key names and function
signature to avoid other changes.
Comment on lines
+116
to
+126
| // Use shared core function to create node | ||
| const result = createNodeFromModel(category, filename, validAsset.id, options) | ||
|
|
||
| const litegraphService = useLitegraphService() | ||
| const pos = options?.position ?? litegraphService.getCanvasCenter() | ||
|
|
||
| const node = LiteGraph.createNode( | ||
| provider.nodeDef.name, | ||
| provider.nodeDef.display_name, | ||
| { pos } | ||
| ) | ||
|
|
||
| if (!node) { | ||
| console.error(`Failed to create node for type: ${provider.nodeDef.name}`) | ||
| if (!result.success) { | ||
| // Map generic error to asset-specific error format | ||
| return { | ||
| success: false, | ||
| error: { | ||
| code: 'NODE_CREATION_FAILED', | ||
| message: `Failed to create node for type: ${provider.nodeDef.name}`, | ||
| assetId: validAsset.id, | ||
| details: { nodeType: provider.nodeDef.name } | ||
| } | ||
| ...result.error, | ||
| assetId: result.error.itemId | ||
| } as NodeCreationError |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid type assertion by remapping itemId explicitly.
as NodeCreationError hides the extra itemId field. Remap and drop it so the returned error matches the declared shape.
♻️ Proposed fix
if (!result.success) {
- // Map generic error to asset-specific error format
- return {
- success: false,
- error: {
- ...result.error,
- assetId: result.error.itemId
- } as NodeCreationError
- }
+ const { itemId, ...rest } = result.error
+ return {
+ success: false,
+ error: {
+ ...rest,
+ assetId: itemId
+ }
+ }
}📝 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
| // Use shared core function to create node | |
| const result = createNodeFromModel(category, filename, validAsset.id, options) | |
| const litegraphService = useLitegraphService() | |
| const pos = options?.position ?? litegraphService.getCanvasCenter() | |
| const node = LiteGraph.createNode( | |
| provider.nodeDef.name, | |
| provider.nodeDef.display_name, | |
| { pos } | |
| ) | |
| if (!node) { | |
| console.error(`Failed to create node for type: ${provider.nodeDef.name}`) | |
| if (!result.success) { | |
| // Map generic error to asset-specific error format | |
| return { | |
| success: false, | |
| error: { | |
| code: 'NODE_CREATION_FAILED', | |
| message: `Failed to create node for type: ${provider.nodeDef.name}`, | |
| assetId: validAsset.id, | |
| details: { nodeType: provider.nodeDef.name } | |
| } | |
| ...result.error, | |
| assetId: result.error.itemId | |
| } as NodeCreationError | |
| // Use shared core function to create node | |
| const result = createNodeFromModel(category, filename, validAsset.id, options) | |
| if (!result.success) { | |
| const { itemId, ...rest } = result.error | |
| return { | |
| success: false, | |
| error: { | |
| ...rest, | |
| assetId: itemId | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/platform/assets/utils/createModelNodeFromAsset.ts` around lines 116 -
126, The function calling createNodeFromModel returns result.error with a type
assertion to NodeCreationError; instead explicitly build and return a
NodeCreationError object by mapping result.error.message, result.error.code (and
any other expected NodeCreationError fields) and set assetId =
result.error.itemId, omitting itemId, instead of using "as NodeCreationError";
update the error return in createModelNodeFromAsset to construct the exact shape
required (refer to createNodeFromModel's result and the NodeCreationError type)
so no type assertion is needed.
Comment on lines
+7
to
+18
| v-if="model.previewUrl" | ||
| class="relative w-full aspect-square bg-modal-panel-background border-t border-border-default" | ||
| > | ||
| <img | ||
| v-if="!imageError" | ||
| :src="model.previewUrl" | ||
| :alt="model.displayName" | ||
| loading="lazy" | ||
| decoding="async" | ||
| class="size-full object-cover" | ||
| @error="imageError = true" | ||
| /> |
Contributor
There was a problem hiding this comment.
Reset preview fallback when the model changes.
If one preview fails, imageError remains true and can block later previews; reset it when the model/preview changes.
🛠️ Suggested fix
-import { computed, ref } from 'vue'
+import { computed, ref, watch } from 'vue'
@@
const imageError = ref(false)
+watch(
+ () => model.previewUrl,
+ () => {
+ imageError.value = false
+ }
+)🤖 Prompt for AI Agents
In `@src/platform/models/browser/components/LocalModelInfoPanel.vue` around lines
7 - 18, The preview fallback state isn't reset when the model changes—because
imageError remains true after a failed load, subsequent model previews are
blocked; in LocalModelInfoPanel.vue add logic to reset imageError (set
imageError = false) whenever the model or its previewUrl changes (e.g., add a
watch on model.previewUrl or model.id, or reset inside a computed/updated hook)
so the <img> can attempt to load the new preview; reference the imageError
reactive flag and the model.previewUrl used in the <img> tag to locate where to
add the reset.
Comment on lines
+31
to
+42
| <div | ||
| class="sticky top-0 z-20 px-4 py-3 bg-modal-panel-background border-t border-border-default" | ||
| > | ||
| <div class="flex gap-2"> | ||
| <button | ||
| type="button" | ||
| class="flex-1 inline-flex items-center justify-center gap-2 cursor-pointer whitespace-nowrap appearance-none border-none font-medium font-inter transition-colors focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring disabled:pointer-events-none disabled:opacity-50 [&_svg]:pointer-events-none [&_svg]:size-4 [&_svg]:shrink-0 bg-primary-background text-primary-foreground hover:bg-primary-background-hover h-10 rounded-lg px-4 py-2 text-sm" | ||
| @click="emit('use', model)" | ||
| > | ||
| <i class="icon-[lucide--plus]" /> | ||
| {{ $t('modelBrowser.use') }} | ||
| </button> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use shared button components for the primary action.
The raw <button> bypasses the design system; please switch to the common button components (and apply similarly to the accordion headers if feasible) so styling/theming stays consistent.
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. These components wrap PrimeVue with the project’s design system styling. Use only the common button components for consistency and theming, and import them from src/components/button/ as needed.
🤖 Prompt for AI Agents
In `@src/platform/models/browser/components/LocalModelInfoPanel.vue` around lines
31 - 42, Replace the raw primary <button> in LocalModelInfoPanel.vue with the
project's common button component (e.g., IconTextButton or IconButton) from
src/components/button/, import that component into the LocalModelInfoPanel
component, and bind the same props/events (preserve the click handler
emit('use', model), icon class i="icon-[lucide--plus]", text
$t('modelBrowser.use'), and accessibility/disabled behavior) so styling/theming
follows the design system; also audit accordion headers in this file and swap
plain buttons there to the common button components where applicable.
Comment on lines
+238
to
+244
| <textarea | ||
| disabled | ||
| :placeholder="model.description || 'No description set'" | ||
| rows="3" | ||
| class="w-full resize-y rounded-lg border border-transparent bg-transparent px-3 py-2 text-sm text-component-node-foreground outline-none transition-colors focus:bg-component-node-widget-background cursor-not-allowed" | ||
| :value="model.description" | ||
| /> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "LocalModelInfoPanel.vue" -type fRepository: Comfy-Org/ComfyUI_frontend
Length of output: 134
🏁 Script executed:
fd LocalModelInfoPanel.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 132
🏁 Script executed:
cat -n src/platform/models/browser/components/LocalModelInfoPanel.vue | sed -n '235,250p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 738
🏁 Script executed:
wc -l src/platform/models/browser/components/LocalModelInfoPanel.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 136
🏁 Script executed:
grep -n "modelBrowser" src/locales/en/main.jsonRepository: Comfy-Org/ComfyUI_frontend
Length of output: 94
🏁 Script executed:
grep -n "noDescription\|No description" src/locales/en/main.jsonRepository: Comfy-Org/ComfyUI_frontend
Length of output: 176
🏁 Script executed:
sed -n '2829,2850p' src/locales/en/main.jsonRepository: Comfy-Org/ComfyUI_frontend
Length of output: 829
🏁 Script executed:
sed -n '2590,2600p' src/locales/en/main.jsonRepository: Comfy-Org/ComfyUI_frontend
Length of output: 439
🏁 Script executed:
head -30 src/platform/models/browser/components/LocalModelInfoPanel.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 872
🏁 Script executed:
sed -n '2590,2610p' src/locales/en/main.jsonRepository: Comfy-Org/ComfyUI_frontend
Length of output: 850
Localize the description placeholder using the existing translation key.
The placeholder is hardcoded; use the existing i18n translation key that matches this text.
🛠️ Suggested fix
- :placeholder="model.description || 'No description set'"
+ :placeholder="model.description || $t('modelBrowser.descriptionNotSet')"🤖 Prompt for AI Agents
In `@src/platform/models/browser/components/LocalModelInfoPanel.vue` around lines
238 - 244, The textarea currently uses a hardcoded placeholder string; update
the placeholder binding to use the i18n translation instead of the literal text
(keep the existing model.description fallback). Replace the placeholder
expression on the <textarea> (the element binding referencing model.description)
with a call to the app's translation helper (e.g. $t or i18n.t) using the
existing translation key that corresponds to "No description set" so it reads
something like :placeholder="model.description ||
$t('existing_translation_key')" while leaving the element, disabled attribute,
and :value="model.description" intact.
Comment on lines
+29
to
+71
| beforeEach(() => { | ||
| // Mock scrollIntoView | ||
| Element.prototype.scrollIntoView = vi.fn() | ||
| // window.getComputedStyle for grid tests | ||
| vi.spyOn(window, 'getComputedStyle').mockReturnValue({ | ||
| gridTemplateColumns: '1fr 1fr 1fr' | ||
| } as CSSStyleDeclaration) | ||
| }) | ||
|
|
||
| describe('Search and Filter Flow', () => { | ||
| it('should filter models by search query', async () => { | ||
| const models = createMockModels() | ||
| const modelsList = ref(models) | ||
|
|
||
| const { searchQuery, filteredModels } = | ||
| useModelBrowserFiltering(modelsList) | ||
|
|
||
| // Perform search | ||
| searchQuery.value = 'dream' | ||
| await nextTick() | ||
|
|
||
| // Should filter to models matching "dream" | ||
| expect(filteredModels.value.length).toBeGreaterThan(0) | ||
| expect( | ||
| filteredModels.value.some((m) => | ||
| m.fileName.toLowerCase().includes('dream') | ||
| ) | ||
| ).toBe(true) | ||
| }) | ||
|
|
||
| it('should handle empty search results', async () => { | ||
| const models = createMockModels() | ||
| const modelsList = ref(models) | ||
|
|
||
| const { searchQuery, filteredModels } = | ||
| useModelBrowserFiltering(modelsList) | ||
|
|
||
| // Search for non-existent model | ||
| searchQuery.value = 'xyznonexistent123' | ||
|
|
||
| // Wait for debounce (300ms default) | ||
| await new Promise((resolve) => setTimeout(resolve, 350)) | ||
| await nextTick() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "ModelBrowserDialog.test.ts" | head -5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 137
🏁 Script executed:
find . -type f -name "*useModelBrowserFiltering*" | head -5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 214
🏁 Script executed:
rg -l "useModelBrowserFiltering" --type ts --type tsx | head -10Repository: Comfy-Org/ComfyUI_frontend
Length of output: 97
🏁 Script executed:
cat -n src/platform/models/browser/components/ModelBrowserDialog.test.ts | head -120Repository: Comfy-Org/ComfyUI_frontend
Length of output: 5090
🏁 Script executed:
cat -n src/platform/models/browser/composables/useModelBrowserFiltering.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 6260
Use fake timers and cleanup to stabilize debounced search tests.
Global mocks are not restored, and real setTimeout in tests causes flakiness. The first search test never waits for debounce, so it doesn't validate filtering properly. Introduce vi.useFakeTimers() in beforeEach, add afterEach to restore global state and timers, and use vi.advanceTimersByTime() instead of real setTimeout promises.
🧪 Proposed test stabilization
describe('ModelBrowserDialog - Integration Tests', () => {
+ const originalScrollIntoView = Element.prototype.scrollIntoView
+
function createMockModels() {
@@
beforeEach(() => {
+ vi.useFakeTimers()
// Mock scrollIntoView
Element.prototype.scrollIntoView = vi.fn()
// window.getComputedStyle for grid tests
vi.spyOn(window, 'getComputedStyle').mockReturnValue({
gridTemplateColumns: '1fr 1fr 1fr'
} as CSSStyleDeclaration)
})
+
+ afterEach(() => {
+ Element.prototype.scrollIntoView = originalScrollIntoView
+ vi.useRealTimers()
+ vi.restoreAllMocks()
+ })
@@
// Perform search
searchQuery.value = 'dream'
+ vi.advanceTimersByTime(300)
await nextTick()
@@
// Wait for debounce (300ms default)
- await new Promise((resolve) => setTimeout(resolve, 350))
+ vi.advanceTimersByTime(350)
await nextTick()
@@
// Wait for debounce (300ms default)
- await new Promise((resolve) => setTimeout(resolve, 350))
+ vi.advanceTimersByTime(350)
await nextTick()🤖 Prompt for AI Agents
In `@src/platform/models/browser/components/ModelBrowserDialog.test.ts` around
lines 29 - 71, Enable fake timers and restore mocks/timers to stabilize the
debounced search tests: call vi.useFakeTimers() in beforeEach (alongside the
existing Element.prototype.scrollIntoView and getComputedStyle spy setup),
replace any real setTimeout-awaits in tests with vi.advanceTimersByTime(300) (or
the debounce duration) and await nextTick() to let reactive updates run (e.g.,
in tests using useModelBrowserFiltering/searchQuery/filteredModels), and add an
afterEach that calls vi.useRealTimers() and vi.restoreAllMocks() to clean up
global mocks and timers between tests.
Comment on lines
+7
to
+14
| const modelCache = { | ||
| data: null as EnrichedModel[] | null, | ||
| timestamp: 0, | ||
| CACHE_TTL: 5 * 60 * 1000 | ||
| } | ||
|
|
||
| let loadingPromise: Promise<void> | null = null | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Module-level state is intentional but worth documenting.
The module-level modelCache and loadingPromise enable cross-instance cache sharing and deduplication. This is the intended behavior but could surprise future maintainers.
Consider adding a brief comment explaining this design choice, especially since clearCache() affects all instances.
📝 Suggested documentation
+// Module-level cache shared across all useModelLoader() instances
+// Enables cache sharing and deduplication of concurrent load requests
const modelCache = {
data: null as EnrichedModel[] | null,
timestamp: 0,
CACHE_TTL: 5 * 60 * 1000
}
+// Singleton promise to deduplicate concurrent loadModels() calls
let loadingPromise: Promise<void> | null = null🤖 Prompt for AI Agents
In `@src/platform/models/browser/composables/useModelLoader.ts` around lines 7 -
14, Add a short module-level comment above the modelCache and loadingPromise
declarations explaining that these are intentionally shared module-level state
to enable cross-instance caching, request deduplication, and TTL-based
invalidation (referencing modelCache.CACHE_TTL), and that clearCache() will
clear this shared cache for all instances; keep the comment concise and mention
intended behavior to avoid surprising future maintainers.
Comment on lines
+48
to
+57
| const enrichedModels: EnrichedModel[] = [] | ||
|
|
||
| for (const folder of modelStore.modelFolders) { | ||
| const folderModels = Object.values(folder.models) | ||
| for (const model of folderModels) { | ||
| enrichedModels.push( | ||
| transformToEnrichedModel(model, folder.directory) | ||
| ) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using flatMap for cleaner model transformation.
The nested loops could be simplified with flatMap for improved readability.
♻️ Suggested refactor
- const enrichedModels: EnrichedModel[] = []
-
- for (const folder of modelStore.modelFolders) {
- const folderModels = Object.values(folder.models)
- for (const model of folderModels) {
- enrichedModels.push(
- transformToEnrichedModel(model, folder.directory)
- )
- }
- }
+ const enrichedModels = modelStore.modelFolders.flatMap((folder) =>
+ Object.values(folder.models).map((model) =>
+ transformToEnrichedModel(model, folder.directory)
+ )
+ )📝 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
| const enrichedModels: EnrichedModel[] = [] | |
| for (const folder of modelStore.modelFolders) { | |
| const folderModels = Object.values(folder.models) | |
| for (const model of folderModels) { | |
| enrichedModels.push( | |
| transformToEnrichedModel(model, folder.directory) | |
| ) | |
| } | |
| } | |
| const enrichedModels = modelStore.modelFolders.flatMap((folder) => | |
| Object.values(folder.models).map((model) => | |
| transformToEnrichedModel(model, folder.directory) | |
| ) | |
| ) |
🤖 Prompt for AI Agents
In `@src/platform/models/browser/composables/useModelLoader.ts` around lines 48 -
57, Replace the manual nested loops that build enrichedModels with a
flatMap-based transformation: iterate modelStore.modelFolders and for each
folder map its Object.values(folder.models) to transformToEnrichedModel(model,
folder.directory), then flatten into the enrichedModels array (e.g., const
enrichedModels: EnrichedModel[] = modelStore.modelFolders.flatMap(folder =>
Object.values(folder.models).map(m => transformToEnrichedModel(m,
folder.directory)))); this keeps the same logic but simplifies the code around
enrichedModels, modelStore.modelFolders, and transformToEnrichedModel.
Comment on lines
+49
to
+56
| if (!result.success) { | ||
| // Map generic error to model-specific error format | ||
| return { | ||
| success: false, | ||
| error: { | ||
| ...result.error, | ||
| modelId: result.error.itemId | ||
| } as NodeCreationError |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid type assertion by remapping itemId explicitly.
Remove the assertion and drop itemId while mapping to modelId so the error shape is consistent.
♻️ Proposed fix
if (!result.success) {
// Map generic error to model-specific error format
- return {
- success: false,
- error: {
- ...result.error,
- modelId: result.error.itemId
- } as NodeCreationError
- }
+ const { itemId, ...rest } = result.error
+ return {
+ success: false,
+ error: {
+ ...rest,
+ modelId: itemId
+ }
+ }
}📝 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
| if (!result.success) { | |
| // Map generic error to model-specific error format | |
| return { | |
| success: false, | |
| error: { | |
| ...result.error, | |
| modelId: result.error.itemId | |
| } as NodeCreationError | |
| if (!result.success) { | |
| // Map generic error to model-specific error format | |
| const { itemId, ...rest } = result.error | |
| return { | |
| success: false, | |
| error: { | |
| ...rest, | |
| modelId: itemId | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/platform/models/browser/utils/createModelNode.ts` around lines 49 - 56,
The error mapping in createModelNode currently uses a type assertion and
preserves itemId; instead, construct a new NodeCreationError object explicitly
by copying needed fields from result.error, assign modelId: result.error.itemId,
and omit itemId (do not use "as NodeCreationError"); return { success: false,
error: newError } so the shape matches NodeCreationError without type
assertions.
Comment on lines
88
to
106
| async function loadModelMetadata( | ||
| model: EnrichedModel | ||
| ): Promise<EnrichedModel> { | ||
| await model.original.load() | ||
|
|
||
| // Return new enriched model with loaded metadata (immutable) | ||
| return { | ||
| ...model, | ||
| description: model.original.description || undefined, | ||
| tags: model.original.tags.length > 0 ? model.original.tags : undefined, | ||
| author: model.original.author || undefined, | ||
| architectureId: model.original.architecture_id || undefined, | ||
| resolution: model.original.resolution || undefined, | ||
| usageHint: model.original.usage_hint || undefined, | ||
| triggerPhrase: model.original.trigger_phrase || undefined, | ||
| displayName: model.original.title || model.simplifiedName, | ||
| previewUrl: model.original.image || model.previewUrl | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'loadModelMetadata' --type=ts --type=vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 97
🏁 Script executed:
rg -n 'loadModelMetadata' --type=tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 158
Remove unused loadModelMetadata function.
This function is defined but never called or exported. The repository guidelines recommend keeping the public API surface minimal and avoiding dead code. Since there are no usages of this function anywhere in the codebase, it should be removed.
🤖 Prompt for AI Agents
In `@src/platform/models/browser/utils/modelTransform.ts` around lines 88 - 106,
Remove the dead function loadModelMetadata from
src/platform/models/browser/utils/modelTransform.ts since it is neither called
nor exported; delete the entire async function declaration and its body, and
then run a quick build/TS check to remove or adjust any now-unused imports/types
(e.g., EnrichedModel) and remove the function from any exports or test stubs if
present.
Comment on lines
+5
to
+26
| export function getModelTypeIcon(type: string): string { | ||
| const icons: Record<string, string> = { | ||
| CHECKPOINT: 'icon-[lucide--box]', | ||
| LORA: 'icon-[lucide--layers]', | ||
| VAE: 'icon-[lucide--cpu]', | ||
| CONTROLNET: 'icon-[lucide--sliders-horizontal]', | ||
| EMBEDDING: 'icon-[lucide--type]', | ||
| UPSCALE: 'icon-[lucide--arrow-up]', | ||
| 'CLIP VISION': 'icon-[lucide--eye]', | ||
| 'IP-ADAPTER': 'icon-[lucide--image]', | ||
| SAM: 'icon-[lucide--scissors]', | ||
| DIFFUSION: 'icon-[lucide--sparkles]', | ||
| ANIMATEDIFF: 'icon-[lucide--film]', | ||
| 'MOTION LORA': 'icon-[lucide--move]', | ||
| AUDIO: 'icon-[lucide--music]', | ||
| CLIP: 'icon-[lucide--text]', | ||
| UNET: 'icon-[lucide--network]', | ||
| GLIGEN: 'icon-[lucide--grid-3x3]', | ||
| STYLE: 'icon-[lucide--palette]', | ||
| PHOTOMAKER: 'icon-[lucide--camera]' | ||
| } | ||
| return icons[type] || 'icon-[comfy--ai-model]' |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hoist the icon map to module scope to avoid per-call allocations.
The map is constant and recreated on every call; hoisting keeps it immutable and cheaper.
♻️ Proposed refactor
+const MODEL_TYPE_ICONS: Record<string, string> = {
+ CHECKPOINT: 'icon-[lucide--box]',
+ LORA: 'icon-[lucide--layers]',
+ VAE: 'icon-[lucide--cpu]',
+ CONTROLNET: 'icon-[lucide--sliders-horizontal]',
+ EMBEDDING: 'icon-[lucide--type]',
+ UPSCALE: 'icon-[lucide--arrow-up]',
+ 'CLIP VISION': 'icon-[lucide--eye]',
+ 'IP-ADAPTER': 'icon-[lucide--image]',
+ SAM: 'icon-[lucide--scissors]',
+ DIFFUSION: 'icon-[lucide--sparkles]',
+ ANIMATEDIFF: 'icon-[lucide--film]',
+ 'MOTION LORA': 'icon-[lucide--move]',
+ AUDIO: 'icon-[lucide--music]',
+ CLIP: 'icon-[lucide--text]',
+ UNET: 'icon-[lucide--network]',
+ GLIGEN: 'icon-[lucide--grid-3x3]',
+ STYLE: 'icon-[lucide--palette]',
+ PHOTOMAKER: 'icon-[lucide--camera]'
+}
+
export function getModelTypeIcon(type: string): string {
- const icons: Record<string, string> = {
- CHECKPOINT: 'icon-[lucide--box]',
- LORA: 'icon-[lucide--layers]',
- VAE: 'icon-[lucide--cpu]',
- CONTROLNET: 'icon-[lucide--sliders-horizontal]',
- EMBEDDING: 'icon-[lucide--type]',
- UPSCALE: 'icon-[lucide--arrow-up]',
- 'CLIP VISION': 'icon-[lucide--eye]',
- 'IP-ADAPTER': 'icon-[lucide--image]',
- SAM: 'icon-[lucide--scissors]',
- DIFFUSION: 'icon-[lucide--sparkles]',
- ANIMATEDIFF: 'icon-[lucide--film]',
- 'MOTION LORA': 'icon-[lucide--move]',
- AUDIO: 'icon-[lucide--music]',
- CLIP: 'icon-[lucide--text]',
- UNET: 'icon-[lucide--network]',
- GLIGEN: 'icon-[lucide--grid-3x3]',
- STYLE: 'icon-[lucide--palette]',
- PHOTOMAKER: 'icon-[lucide--camera]'
- }
- return icons[type] || 'icon-[comfy--ai-model]'
+ return MODEL_TYPE_ICONS[type] || 'icon-[comfy--ai-model]'
}📝 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
| export function getModelTypeIcon(type: string): string { | |
| const icons: Record<string, string> = { | |
| CHECKPOINT: 'icon-[lucide--box]', | |
| LORA: 'icon-[lucide--layers]', | |
| VAE: 'icon-[lucide--cpu]', | |
| CONTROLNET: 'icon-[lucide--sliders-horizontal]', | |
| EMBEDDING: 'icon-[lucide--type]', | |
| UPSCALE: 'icon-[lucide--arrow-up]', | |
| 'CLIP VISION': 'icon-[lucide--eye]', | |
| 'IP-ADAPTER': 'icon-[lucide--image]', | |
| SAM: 'icon-[lucide--scissors]', | |
| DIFFUSION: 'icon-[lucide--sparkles]', | |
| ANIMATEDIFF: 'icon-[lucide--film]', | |
| 'MOTION LORA': 'icon-[lucide--move]', | |
| AUDIO: 'icon-[lucide--music]', | |
| CLIP: 'icon-[lucide--text]', | |
| UNET: 'icon-[lucide--network]', | |
| GLIGEN: 'icon-[lucide--grid-3x3]', | |
| STYLE: 'icon-[lucide--palette]', | |
| PHOTOMAKER: 'icon-[lucide--camera]' | |
| } | |
| return icons[type] || 'icon-[comfy--ai-model]' | |
| const MODEL_TYPE_ICONS: Record<string, string> = { | |
| CHECKPOINT: 'icon-[lucide--box]', | |
| LORA: 'icon-[lucide--layers]', | |
| VAE: 'icon-[lucide--cpu]', | |
| CONTROLNET: 'icon-[lucide--sliders-horizontal]', | |
| EMBEDDING: 'icon-[lucide--type]', | |
| UPSCALE: 'icon-[lucide--arrow-up]', | |
| 'CLIP VISION': 'icon-[lucide--eye]', | |
| 'IP-ADAPTER': 'icon-[lucide--image]', | |
| SAM: 'icon-[lucide--scissors]', | |
| DIFFUSION: 'icon-[lucide--sparkles]', | |
| ANIMATEDIFF: 'icon-[lucide--film]', | |
| 'MOTION LORA': 'icon-[lucide--move]', | |
| AUDIO: 'icon-[lucide--music]', | |
| CLIP: 'icon-[lucide--text]', | |
| UNET: 'icon-[lucide--network]', | |
| GLIGEN: 'icon-[lucide--grid-3x3]', | |
| STYLE: 'icon-[lucide--palette]', | |
| PHOTOMAKER: 'icon-[lucide--camera]' | |
| } | |
| export function getModelTypeIcon(type: string): string { | |
| return MODEL_TYPE_ICONS[type] || 'icon-[comfy--ai-model]' | |
| } |
🤖 Prompt for AI Agents
In `@src/platform/models/browser/utils/modelTypeIcons.ts` around lines 5 - 26, The
icons map is being recreated on every call inside getModelTypeIcon causing
unnecessary allocations; move the const icons object out to module scope (e.g.,
define a top-level ICONS constant) and have getModelTypeIcon(type: string)
simply reference that ICONS constant and return ICONS[type] ||
'icon-[comfy--ai-model]'; keep the same key names and function signature to
avoid other changes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/sidebar/SideToolbar.vue (1)
122-141: MoveuseModelBrowserDialog()instantiation to setup instead of calling it inside the event handler.Calling the composable inside the click handler creates a new instance on each invocation. Instantiate it once in the component setup to avoid repeated composable initialization.
Note:
show()is synchronous and does not return a Promise, so no await is needed.
🤖 Fix all issues with AI agents
In `@packages/design-system/src/css/style.css`:
- Around line 320-325: The table CSS currently hard-codes hex values for
--table-row-even-background, --table-row-odd-background,
--table-row-hover-background, and --table-header-background; replace those
literal colors by mapping them to the project's palette tokens (e.g., use the
existing gray/neutral palette variables) so global theming is respected — update
the declarations for the four variables to reference the appropriate palette
variables (use the closest matching tokens for even/odd/hover/header states)
instead of hex values.
In `@src/platform/assets/utils/createModelNodeFromAsset.ts`:
- Around line 119-128: The returned error construction in
createModelNodeFromAsset.ts currently spreads result.error and casts to
NodeCreationError, which leaks itemId and relies on a type assertion; instead
explicitly build and return a NodeCreationError object using known fields from
result.error (e.g., code, message, details) and set assetId from
result.error.itemId (or the correct source) without including itemId, and remove
the "as NodeCreationError" cast so the shape is concrete and typesafe.
In `@src/platform/models/browser/components/LocalModelInfoPanel.vue`:
- Around line 35-56: Replace the raw <button> elements with the repo’s shared
button components: use IconTextButton (or IconButton+TextButton if split) from
src/components/button/ for the primary "use" action (replace the button that
calls `@click`="emit('use', model)" and contains the plus icon and {{
$t('modelBrowser.use') }}), and replace the section toggle button (the one
toggling basicInfoOpen via `@click`="basicInfoOpen = !basicInfoOpen") with the
appropriate TextButton/IconTextButton so styling/behavior is consistent; ensure
you pass the same click handlers/props, slot the icon (icon-[lucide--plus]) and
translated label into the shared component, and preserve reactive bindings
(emit('use', model) and basicInfoOpen toggle) and classes/aria attributes as
props on the shared components.
- Around line 7-28: The imageError state isn't reset when switching models so
previous errors hide new previews; add a watcher that resets imageError to false
whenever the model's previewUrl changes (watch the reactive value
model.previewUrl or model.id) in the LocalModelInfoPanel.vue script setup so the
<img> can attempt to load the new URL; reference the imageError variable and the
model.previewUrl reactive property and ensure the watcher runs on component
mount/update to clear imageError before the new image load.
In `@src/platform/models/browser/components/ModelBrowserDialog.test.ts`:
- Around line 29-35: The test globally mutates Element.prototype.scrollIntoView
and window.getComputedStyle without teardown; change the scrollIntoView
assignment to use vi.spyOn(Element.prototype, 'scrollIntoView') with a mock
implementation and keep the existing vi.spyOn(window, 'getComputedStyle')
pattern, then add an afterEach that calls vi.restoreAllMocks() to restore both
spies/mocks after each test (referencing the beforeEach,
Element.prototype.scrollIntoView usage and the existing vi.spyOn(window,
'getComputedStyle') so you update those spots).
In `@src/platform/models/browser/components/ModelBrowserDialog.vue`:
- Around line 392-399: The failure branch in ModelBrowserDialog.vue that
currently only console.error when createModelNode fails should surface the error
to the user: replace or augment the console.error in the else branch that
references result.error.message and model.displayName by emitting a descriptive
error event (e.g. this.$emit('model-create-error', { modelId: model.id, message:
result.error.message })) so the parent can handle it, or call the app’s
user-notification/toast API to show an actionable message (e.g. "Failed to
create model {model.displayName}: {errorMessage}") and include the original
error object for debugging; update callers/listeners to handle the new
'model-create-error' event if you choose the emit approach.
- Around line 355-392: The dialog isn't closed when a model is selected because
handleClose() returns early if isRightPanelOpen is true; in handleModelSelect
(after successful create and emit('select', model.original)) bypass
handleClose's early-return by explicitly setting isRightPanelOpen.value = false
(to close the panel) and then calling emit('close') directly so the dialog
always closes; update handleModelSelect to perform these steps instead of
calling handleClose().
In `@src/platform/models/browser/components/ModelBrowserSortButton.vue`:
- Around line 5-17: The button elements are using a hardcoded aria-label that
overrides the visible, non-localized text; remove the :aria-label binding from
the button in ModelBrowserSortButton.vue (the v-for over sortOptions) so the
visible label remains the accessible name, and if you must announce sort
direction add a localized, visually-hidden element keyed to sortBy/sortDirection
(or use a localized aria-live region) instead of the current :aria-label; keep
handleSortClick, sortOptions, sortBy and sortDirection logic unchanged.
- Around line 5-32: Replace the raw <button> elements in
ModelBrowserSortButton.vue with the repo’s shared Button component (e.g.,
Button/IconButton) while preserving behavior: iterate over sortOptions and use
:key="option.value", bind the active classes/variants based on sortBy and
sortDirection via the shared component's class/variant props, keep the `@click`
calling handleSortClick(option.value), preserve the :aria-label string, and
render getDisplayLabel(option) and the conditional icon inside the button slot
(or via the component's icon prop) so sorting state and visuals remain
identical; update references to sortBy, sortDirection, handleSortClick,
getDisplayLabel and sortOptions accordingly.
In `@src/platform/models/browser/components/ModelCard.test.ts`:
- Around line 115-128: The test in ModelCard.test.ts is selecting the "Use"
button by index which is fragile; update the test that mounts ModelCard (using
createMockModel and wrapper) to find the "Use" button via a stable selector
instead (e.g., query by accessible name/visible text "Use", an aria-label, or a
data-test attribute on the button) and then trigger the click and assert the
'select' event as before; ensure you update the template or test to add/target
the stable identifier so the test no longer relies on buttons[buttons.length -
1].
- Around line 178-192: The test for ModelCard ('should show gradient fallback
when image fails to load') currently asserts on a Tailwind utility class
(.bg-gradient-to-br); update it to a behavioral assertion instead: after
triggering img.trigger('error') and awaiting wrapper.vm.$nextTick(), assert that
the <img> element is no longer present (e.g., wrapper.find('img').exists() is
false) or that a semantic fallback element is rendered (e.g., check for an
element with a specific role, test id, or alt text), so the test checks behavior
rather than a CSS utility class.
In `@src/platform/models/browser/components/ModelListItem.vue`:
- Around line 2-23: The element currently uses role="button" together with
aria-selected and only handles Enter activation; change semantics to ARIA option
semantics by replacing role="button" with role="option", keep
:aria-selected="focused" and ensure tabindex remains (e.g., :tabindex="focused ?
0 : -1"); add a keydown handler on the root that calls the existing handleSelect
for both Enter and Space (check/extend the `@keydown` handler that currently calls
emit('show-info', model) to also call handleSelect on Space/Enter), and remove
any contradictory semantics (e.g., if you prefer a real <button>, then remove
aria-selected and implement Space/Enter handling there instead). Ensure the
handlers referenced are handleClick, handleSelect and the emit calls so the
behavior and accessibility state stay in sync.
- Around line 81-111: Replace the raw <button> controls with the shared
IconButton component (IconButton) for both the bookmark and details actions:
keep the reactive bindings (isBookmarked) and attributes (v-tooltip.left,
:aria-label) and preserve the event handlers (`@click.stop` calling
handleToggleBookmark and emitting 'show-info' with model). For the bookmark
action, conditionally set the IconButton's icon/class to match the two states
(bookmark-check vs bookmark) driven by isBookmarked; for the details action, use
the file-text icon. Ensure the IconButton receives the same accessibility and
behavior (disabled, focus, hover) props and that click propagation remains
stopped.
- Around line 25-45: Resetting imageError when the model changes: add logic in
ModelListItem.vue to clear the imageError reactive flag whenever the component's
model prop/reference changes (e.g., watch the model prop or its id and set
imageError = false). This ensures that when a recycled component receives a new
model, any previous error state is cleared so valid model.previewUrl can show;
reference the imageError variable, the model prop (or model.id), and the
existing `@error/`@load handlers on the <img>.
In `@src/platform/models/browser/components/ViewModeToggle.vue`:
- Around line 5-31: Replace the raw <button> elements in ViewModeToggle.vue with
the shared IconButton component (import IconButton from
src/components/button/IconButton.vue) so styling, sizing and focus/hover states
are consistent; keep the current logic that checks modelValue ('grid' vs 'list')
to determine active vs inactive styling, forward the localized aria-label
($t('modelBrowser.viewGrid') / $t('modelBrowser.viewList')), and emit the same
update:modelValue events ('grid' and 'list') on click—pass the appropriate icon
into IconButton (the grid and list lucide icons) via its slot/props and map the
existing conditional classes/state to the IconButton's active/inactive variants
or class prop to preserve behavior.
- Around line 39-45: Replace the separate defineProps and defineEmits with
defineModel to implement the v-model: remove the defineProps<{ modelValue:
'grid' | 'list' }>() and defineEmits<{ 'update:modelValue': [value: 'grid' |
'list'] }>() calls and instead call defineModel<'grid' | 'list'>('modelValue')
(assigning its return to a ref like modelValue); update any code that read
props.modelValue to use modelValue.value and replace explicit emits of
'update:modelValue' with direct assignments to modelValue.value.
In `@src/platform/models/browser/composables/useModelBookmarks.ts`:
- Around line 66-116: Convert the composable helper arrow functions to function
declarations: replace the const arrow functions isBookmarked, toggleBookmark,
addBookmark, removeBookmark, bookmarkedIds, bookmarkCount, and
filterBookmarkedModels with equivalent function declarations (e.g., function
isBookmarked(modelId: string): boolean { ... }) while preserving the internal
logic, return types, and references to bookmarkedModelIds and EnrichedModel;
ensure computed helpers bookmarkedIds and bookmarkCount are exposed as functions
if required by the composable API surface and keep exported names unchanged.
In `@src/platform/models/browser/composables/useModelBrowserDialog.ts`:
- Around line 19-27: Convert the two arrow function expressions to function
declarations: replace the const handleModelSelected = (model: ComfyModelDef) =>
{ onModelSelected?.(model); close(); } with function handleModelSelected(model:
ComfyModelDef) { onModelSelected?.(model); close(); } and replace const
handleClose = () => { onClose?.(); close(); } with function handleClose() {
onClose?.(); close(); }; keep the same bodies and optional chaining for
onModelSelected/onClose and the call to close() so behavior is unchanged.
In `@src/platform/models/browser/composables/useModelBrowserFiltering.test.ts`:
- Around line 78-110: The test uses vi.waitFor with callbacks that return
booleans which can short-circuit; update the waitFor usages in this file
(including the block around the it('should filter by search query with fuzzy
search') test and the other ranges mentioned) to call expect(...) assertions
inside the waitFor callback instead of returning booleans—specifically adjust
the waitFor that currently checks debouncedSearchQuery.value and
filteredModels.value to use expect(debouncedSearchQuery.value).toBe('...') and
expect(filteredModels.value.length).toBeGreaterThanOrEqual(1) (and similarly for
the other searchQuery checks), so the waitFor keeps retrying until assertions
pass; apply the same pattern to all vi.waitFor occurrences in this file that
return booleans.
In `@src/platform/models/browser/composables/useModelFilterOptions.ts`:
- Around line 32-42: The computed availableModelTypes currently deduplicates
model.type values using uniqWith(types, (a, b) => a === b); change this to use
uniq(types) instead for simpler, clearer deduplication: in the
availableModelTypes computed (which reads toValue(models) and maps to
model.type), replace uniqWith(...) with uniq(...) and keep the rest of the
mapping to SelectOption objects (name/value) intact.
- Around line 17-29: Replace the manual extension extraction and uniqWith usage
by using each EnrichedModel.format directly from modelList and the simpler uniq
utility: map modelList to model.format, filter out falsy values, call
uniq(formats) instead of uniqWith(...), then sort and map to the { name:
`.${format}`, value: format } shape; also update the imports to use uniq from
es-toolkit instead of uniqWith.
In `@src/platform/models/browser/composables/useModelKeyboardNav.ts`:
- Around line 46-55: The getGridColumns function uses a fragile
document.querySelector('.grid') selector; change it to target the specific grid
container by either accepting a container ref/element parameter (e.g., pass in
gridContainerRef or HTMLElement to getGridColumns) or querying a specific data
attribute like document.querySelector('[data-model-browser-grid]'); update the
component that renders the grid to add the data-model-browser-grid attribute (or
pass its ref) and use that ref/attribute in getGridColumns to compute
gridTemplateColumns so the function always targets the correct element.
In `@src/platform/models/browser/composables/useModelLoader.ts`:
- Around line 1-11: The module-level singleton modelCache (symbols: modelCache,
CACHE_TTL) can leak state across tests; add and export a test-friendly reset
function (e.g., clearModelCache or resetModelCache) that sets modelCache.data =
null and modelCache.timestamp = 0, and update usage/tests to call it in
teardown, or alternatively document the shared-cache behavior near the
modelCache declaration so callers/tests know to clear it between runs.
In `@src/platform/models/browser/utils/createModelNode.ts`:
- Around line 49-58: The failure branch currently returns result.error spread
and then casts to NodeCreationError to rename itemId to modelId; instead
explicitly construct the NodeCreationError object (e.g., newError or errorObj)
by mapping the relevant fields from result.error (mapping itemId -> modelId) and
only include the NodeCreationError fields, omitting itemId, then return {
success: false, error: errorObj } so you can remove the type assertion; update
the code around the result handling in createModelNode to build the error shape
explicitly.
In `@src/platform/models/browser/utils/modelTypeIcons.ts`:
- Around line 5-26: The icons map is being recreated on every call inside
getModelTypeIcon causing unnecessary allocations; hoist the Record to module
scope as a const (e.g., MODEL_TYPE_ICONS) and have getModelTypeIcon(type:
string) simply return MODEL_TYPE_ICONS[type] || 'icon-[comfy--ai-model]'; update
references to use the new module-level constant and keep the exported function
signature the same.
In `@src/utils/nodeCreation/createNodeFromModel.test.ts`:
- Around line 300-316: The test currently asserts widget value and that
graph.add was called but not that the value was set before the add call; change
the test for createNodeFromModel so the addSpy captures the widget value at call
time (e.g., set addSpy = vi.fn((node) => { capturedValue =
node.widgets![0].value })) and assert that capturedValue ===
'test-model.safetensors' (in addition to the existing expect on
mockNode.widgets![0].value and expect(addSpy).toHaveBeenCalledWith(mockNode)),
ensuring you still mock LiteGraph.createNode and use mockNode and addSpy to
verify ordering.
In `@src/utils/nodeCreation/createNodeFromModel.ts`:
- Around line 8-25: Extract the shared types CreateNodeOptions and Result<T, E>
into a new shared types module and replace the local definitions in
createNodeFromModel.ts, createModelNode.ts, and createModelNodeFromAsset.ts with
imports from that module; keep NodeCreationErrorCode and NodeCreationError local
(do not extract) because they vary by domain, and ensure every file updates its
imports to use the new exported type names (CreateNodeOptions, Result) so
existing references compile without renaming.
Comment on lines
+320
to
+325
|
|
||
| /* Table tokens */ | ||
| --table-row-even-background: #f9fafb; | ||
| --table-row-odd-background: #ffffff; | ||
| --table-row-hover-background: #f3f4f6; | ||
| --table-header-background: #f5f5f5; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider reusing existing palette tokens for table backgrounds.
Hard-coded hex values duplicate palette intent and make global theming harder. Consider mapping these to existing palette variables for consistency.
🤖 Prompt for AI Agents
In `@packages/design-system/src/css/style.css` around lines 320 - 325, The table
CSS currently hard-codes hex values for --table-row-even-background,
--table-row-odd-background, --table-row-hover-background, and
--table-header-background; replace those literal colors by mapping them to the
project's palette tokens (e.g., use the existing gray/neutral palette variables)
so global theming is respected — update the declarations for the four variables
to reference the appropriate palette variables (use the closest matching tokens
for even/odd/hover/header states) instead of hex values.
Comment on lines
+119
to
128
| if (!result.success) { | ||
| // Map generic error to asset-specific error format | ||
| return { | ||
| success: false, | ||
| error: { | ||
| code: 'NODE_CREATION_FAILED', | ||
| message: `Failed to create node for type: ${provider.nodeDef.name}`, | ||
| assetId: validAsset.id, | ||
| details: { nodeType: provider.nodeDef.name } | ||
| } | ||
| ...result.error, | ||
| assetId: result.error.itemId | ||
| } as NodeCreationError | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Same type assertion pattern as createModelNode.ts.
Similar to createModelNode.ts, the as NodeCreationError assertion could be avoided by explicitly constructing the error object. This would also prevent the extra itemId property from being included.
♻️ Proposed refactor
if (!result.success) {
- // Map generic error to asset-specific error format
return {
success: false,
error: {
- ...result.error,
- assetId: result.error.itemId
- } as NodeCreationError
+ code: result.error.code,
+ message: result.error.message,
+ assetId: result.error.itemId,
+ details: result.error.details
+ }
}
}📝 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
| if (!result.success) { | |
| // Map generic error to asset-specific error format | |
| return { | |
| success: false, | |
| error: { | |
| code: 'NODE_CREATION_FAILED', | |
| message: `Failed to create node for type: ${provider.nodeDef.name}`, | |
| assetId: validAsset.id, | |
| details: { nodeType: provider.nodeDef.name } | |
| } | |
| ...result.error, | |
| assetId: result.error.itemId | |
| } as NodeCreationError | |
| } | |
| } | |
| if (!result.success) { | |
| return { | |
| success: false, | |
| error: { | |
| code: result.error.code, | |
| message: result.error.message, | |
| assetId: result.error.itemId, | |
| details: result.error.details | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/platform/assets/utils/createModelNodeFromAsset.ts` around lines 119 -
128, The returned error construction in createModelNodeFromAsset.ts currently
spreads result.error and casts to NodeCreationError, which leaks itemId and
relies on a type assertion; instead explicitly build and return a
NodeCreationError object using known fields from result.error (e.g., code,
message, details) and set assetId from result.error.itemId (or the correct
source) without including itemId, and remove the "as NodeCreationError" cast so
the shape is concrete and typesafe.
Comment on lines
+7
to
+28
| v-if="model.previewUrl" | ||
| class="relative w-full aspect-square bg-modal-panel-background border-t border-border-default" | ||
| > | ||
| <img | ||
| v-if="!imageError" | ||
| :src="model.previewUrl" | ||
| :alt="model.displayName" | ||
| loading="lazy" | ||
| decoding="async" | ||
| class="size-full object-cover" | ||
| @error="imageError = true" | ||
| /> | ||
| <div | ||
| v-else | ||
| class="flex size-full items-center justify-center bg-gradient-to-br from-smoke-400 via-smoke-800 to-charcoal-400" | ||
| > | ||
| <i | ||
| :class=" | ||
| cn(getModelTypeIcon(model.type), 'size-16 text-muted-foreground') | ||
| " | ||
| /> | ||
| </div> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/platform/models/browser/components/LocalModelInfoPanel.vue | head -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4497
🏁 Script executed:
cat -n src/platform/models/browser/components/LocalModelInfoPanel.vue | tail -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4065
🏁 Script executed:
# Check if there are shared button components in the codebase
fd -t f "Button\.vue" src/Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2159
🏁 Script executed:
cat -n src/components/ui/button/Button.vue | head -80Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1205
🏁 Script executed:
# Verify that the model prop type and behavior
grep -A 5 "defineProps" src/platform/models/browser/components/LocalModelInfoPanel.vueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 187
🏁 Script executed:
# Check the EnrichedModel type definition
rg "type EnrichedModel" src/platform/models/browser/types/ -A 15Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
rg "EnrichedModel" src/platform/models/browser/types/ -lRepository: Comfy-Org/ComfyUI_frontend
Length of output: 124
🏁 Script executed:
cat -n src/platform/models/browser/types/modelBrowserTypes.ts | grep -A 30 "EnrichedModel"Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1335
🏁 Script executed:
# Check if "No description set" placeholder should be localized
rg "No description set" src/Repository: Comfy-Org/ComfyUI_frontend
Length of output: 275
🏁 Script executed:
# Check if there are other similar watch patterns for resetting state in components
rg "watch\(" src/platform/models/browser/components/ -B 2 -A 3Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1753
🏁 Script executed:
# Check where LocalModelInfoPanel is used to confirm model can change
rg "LocalModelInfoPanel" src/ -B 2 -A 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1080
Reset preview error state when switching models.
When the panel displays a different model, imageError remains true from the previous model and hides valid previews. Reset it when the preview URL changes.
🛠️ Suggested fix
- import { computed, ref } from 'vue'
+ import { computed, ref, watch } from 'vue'
@@
const imageError = ref(false)
+watch(
+ () => model.previewUrl,
+ () => {
+ imageError.value = false
+ }
+)🤖 Prompt for AI Agents
In `@src/platform/models/browser/components/LocalModelInfoPanel.vue` around lines
7 - 28, The imageError state isn't reset when switching models so previous
errors hide new previews; add a watcher that resets imageError to false whenever
the model's previewUrl changes (watch the reactive value model.previewUrl or
model.id) in the LocalModelInfoPanel.vue script setup so the <img> can attempt
to load the new URL; reference the imageError variable and the model.previewUrl
reactive property and ensure the watcher runs on component mount/update to clear
imageError before the new image load.
Comment on lines
+35
to
+56
| <button | ||
| type="button" | ||
| class="flex-1 inline-flex items-center justify-center gap-2 cursor-pointer whitespace-nowrap appearance-none border-none font-medium font-inter transition-colors focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring disabled:pointer-events-none disabled:opacity-50 [&_svg]:pointer-events-none [&_svg]:size-4 [&_svg]:shrink-0 bg-primary-background text-primary-foreground hover:bg-primary-background-hover h-10 rounded-lg px-4 py-2 text-sm" | ||
| @click="emit('use', model)" | ||
| > | ||
| <i class="icon-[lucide--plus]" /> | ||
| {{ $t('modelBrowser.use') }} | ||
| </button> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div | ||
| class="flex flex-col bg-modal-panel-background border-t border-border-default" | ||
| > | ||
| <div | ||
| class="sticky top-0 z-10 flex items-center justify-between backdrop-blur-xl bg-inherit" | ||
| > | ||
| <button | ||
| type="button" | ||
| class="group min-h-12 bg-transparent border-0 outline-0 ring-0 w-full text-left flex items-center justify-between pl-4 pr-3 cursor-pointer" | ||
| @click="basicInfoOpen = !basicInfoOpen" | ||
| > |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use shared button components for the primary action and section toggles.
Replace the raw <button> elements with the repo’s shared button components to keep behavior and styling consistent.
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.
🤖 Prompt for AI Agents
In `@src/platform/models/browser/components/LocalModelInfoPanel.vue` around lines
35 - 56, Replace the raw <button> elements with the repo’s shared button
components: use IconTextButton (or IconButton+TextButton if split) from
src/components/button/ for the primary "use" action (replace the button that
calls `@click`="emit('use', model)" and contains the plus icon and {{
$t('modelBrowser.use') }}), and replace the section toggle button (the one
toggling basicInfoOpen via `@click`="basicInfoOpen = !basicInfoOpen") with the
appropriate TextButton/IconTextButton so styling/behavior is consistent; ensure
you pass the same click handlers/props, slot the icon (icon-[lucide--plus]) and
translated label into the shared component, and preserve reactive bindings
(emit('use', model) and basicInfoOpen toggle) and classes/aria attributes as
props on the shared components.
Comment on lines
+29
to
+35
| beforeEach(() => { | ||
| // Mock scrollIntoView | ||
| Element.prototype.scrollIntoView = vi.fn() | ||
| // window.getComputedStyle for grid tests | ||
| vi.spyOn(window, 'getComputedStyle').mockReturnValue({ | ||
| gridTemplateColumns: '1fr 1fr 1fr' | ||
| } as CSSStyleDeclaration) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Restore global mocks to avoid cross-test leakage.
scrollIntoView and getComputedStyle are mocked globally without teardown. Use vi.spyOn + vi.restoreAllMocks() in afterEach to keep mocks contained.
🧪 Suggested fix
-import { beforeEach, describe, expect, it, vi } from 'vitest'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
@@
beforeEach(() => {
- // Mock scrollIntoView
- Element.prototype.scrollIntoView = vi.fn()
+ vi.spyOn(Element.prototype, 'scrollIntoView').mockImplementation(() => {})
// window.getComputedStyle for grid tests
vi.spyOn(window, 'getComputedStyle').mockReturnValue({
gridTemplateColumns: '1fr 1fr 1fr'
} as CSSStyleDeclaration)
})
+
+ afterEach(() => {
+ vi.restoreAllMocks()
+ })📝 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
| beforeEach(() => { | |
| // Mock scrollIntoView | |
| Element.prototype.scrollIntoView = vi.fn() | |
| // window.getComputedStyle for grid tests | |
| vi.spyOn(window, 'getComputedStyle').mockReturnValue({ | |
| gridTemplateColumns: '1fr 1fr 1fr' | |
| } as CSSStyleDeclaration) | |
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' | |
| beforeEach(() => { | |
| vi.spyOn(Element.prototype, 'scrollIntoView').mockImplementation(() => {}) | |
| // window.getComputedStyle for grid tests | |
| vi.spyOn(window, 'getComputedStyle').mockReturnValue({ | |
| gridTemplateColumns: '1fr 1fr 1fr' | |
| } as CSSStyleDeclaration) | |
| }) | |
| afterEach(() => { | |
| vi.restoreAllMocks() | |
| }) |
🤖 Prompt for AI Agents
In `@src/platform/models/browser/components/ModelBrowserDialog.test.ts` around
lines 29 - 35, The test globally mutates Element.prototype.scrollIntoView and
window.getComputedStyle without teardown; change the scrollIntoView assignment
to use vi.spyOn(Element.prototype, 'scrollIntoView') with a mock implementation
and keep the existing vi.spyOn(window, 'getComputedStyle') pattern, then add an
afterEach that calls vi.restoreAllMocks() to restore both spies/mocks after each
test (referencing the beforeEach, Element.prototype.scrollIntoView usage and the
existing vi.spyOn(window, 'getComputedStyle') so you update those spots).
Comment on lines
+1
to
+11
| import { ref, shallowRef } from 'vue' | ||
|
|
||
| import type { EnrichedModel } from '@/platform/models/browser/types/modelBrowserTypes' | ||
| import { useModelStore } from '@/stores/modelStore' | ||
| import { transformToEnrichedModel } from '@/platform/models/browser/utils/modelTransform' | ||
|
|
||
| const modelCache = { | ||
| data: null as EnrichedModel[] | null, | ||
| timestamp: 0, | ||
| CACHE_TTL: 5 * 60 * 1000 | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Module-level cache state may cause test isolation issues.
The modelCache object is a module-level singleton shared across all usages. While this is intentional for global caching, it can cause test pollution if tests don't explicitly call clearCache() between runs.
Consider documenting this behavior or exposing a test-only reset mechanism.
🧰 Tools
🪛 ESLint
[error] 1-1: Resolve error: EACCES: permission denied, open '/XNGeBYHOiD'
at Object.writeFileSync (node:fs:2409:20)
at l (file:///home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.mjs:7:13670)
at createFilesMatcher (file:///home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.mjs:7:14422)
at resolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-import-resolver-typescript@4.4.4_eslint-plugin-import-x@4.16.1_@typescript-eslin_44eddb5b99ae4bce470e6fb9a90221ee/node_modules/eslint-import-resolver-typescript/lib/index.js:70:65)
at Object.resolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-import-resolver-typescript@4.4.4_eslint-plugin-import-x@4.16.1_@typescript-eslin_44eddb5b99ae4bce470e6fb9a90221ee/node_modules/eslint-import-resolver-typescript/lib/index.js:147:20)
at file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:170:69
at setRuleContext (/home/jailuser/git/node_modules/.pnpm/eslint-import-context@0.1.9_unrs-resolver@1.11.1/node_modules/eslint-import-context/lib/index.js:23:20)
at fullResolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:170:30)
at relative (file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:215:12)
at resolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:220:16)
(import-x/no-unresolved)
[error] 1-1: Resolve error: EACCES: permission denied, open '/BpekTtgSCX'
at Object.writeFileSync (node:fs:2409:20)
at l (file:///home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.mjs:7:13670)
at createFilesMatcher (file:///home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.mjs:7:14422)
at resolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-import-resolver-typescript@4.4.4_eslint-plugin-import-x@4.16.1_@typescript-eslin_44eddb5b99ae4bce470e6fb9a90221ee/node_modules/eslint-import-resolver-typescript/lib/index.js:70:65)
at Object.resolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-import-resolver-typescript@4.4.4_eslint-plugin-import-x@4.16.1_@typescript-eslin_44eddb5b99ae4bce470e6fb9a90221ee/node_modules/eslint-import-resolver-typescript/lib/index.js:147:20)
at file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:170:69
at setRuleContext (/home/jailuser/git/node_modules/.pnpm/eslint-import-context@0.1.9_unrs-resolver@1.11.1/node_modules/eslint-import-context/lib/index.js:23:20)
at fullResolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:170:30)
at relative (file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:215:12)
at resolve (file:///home/jailuser/git/node_modules/.pnpm/eslint-plugin-import-x@4.16.1_@typescript-eslint+utils@8.50.0_eslint@9.39.1_jiti@2.6.1__eacac87f98d760f1781d40e8519857dc/node_modules/eslint-plugin-import-x/lib/utils/resolve.js:220:16)
(import-x/no-relative-packages)
[error] 1-1: Unable to resolve path to module 'vue'.
(import-x/no-unresolved)
[error] 4-4: Unable to resolve path to module '@/stores/modelStore'.
(import-x/no-unresolved)
[error] 5-5: Unable to resolve path to module '@/platform/models/browser/utils/modelTransform'.
(import-x/no-unresolved)
🤖 Prompt for AI Agents
In `@src/platform/models/browser/composables/useModelLoader.ts` around lines 1 -
11, The module-level singleton modelCache (symbols: modelCache, CACHE_TTL) can
leak state across tests; add and export a test-friendly reset function (e.g.,
clearModelCache or resetModelCache) that sets modelCache.data = null and
modelCache.timestamp = 0, and update usage/tests to call it in teardown, or
alternatively document the shared-cache behavior near the modelCache declaration
so callers/tests know to clear it between runs.
Comment on lines
+49
to
+58
| if (!result.success) { | ||
| // Map generic error to model-specific error format | ||
| return { | ||
| success: false, | ||
| error: { | ||
| ...result.error, | ||
| modelId: result.error.itemId | ||
| } as NodeCreationError | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type assertion could be avoided with explicit error construction.
The as NodeCreationError assertion on line 56 is needed because spreading result.error includes itemId, but the local NodeCreationError type expects modelId. Consider constructing the error explicitly to avoid the assertion and the extra itemId property.
♻️ Proposed refactor
if (!result.success) {
- // Map generic error to model-specific error format
return {
success: false,
error: {
- ...result.error,
- modelId: result.error.itemId
- } as NodeCreationError
+ code: result.error.code,
+ message: result.error.message,
+ modelId: result.error.itemId,
+ details: result.error.details
+ }
}
}🤖 Prompt for AI Agents
In `@src/platform/models/browser/utils/createModelNode.ts` around lines 49 - 58,
The failure branch currently returns result.error spread and then casts to
NodeCreationError to rename itemId to modelId; instead explicitly construct the
NodeCreationError object (e.g., newError or errorObj) by mapping the relevant
fields from result.error (mapping itemId -> modelId) and only include the
NodeCreationError fields, omitting itemId, then return { success: false, error:
errorObj } so you can remove the type assertion; update the code around the
result handling in createModelNode to build the error shape explicitly.
Comment on lines
+5
to
+26
| export function getModelTypeIcon(type: string): string { | ||
| const icons: Record<string, string> = { | ||
| CHECKPOINT: 'icon-[lucide--box]', | ||
| LORA: 'icon-[lucide--layers]', | ||
| VAE: 'icon-[lucide--cpu]', | ||
| CONTROLNET: 'icon-[lucide--sliders-horizontal]', | ||
| EMBEDDING: 'icon-[lucide--type]', | ||
| UPSCALE: 'icon-[lucide--arrow-up]', | ||
| 'CLIP VISION': 'icon-[lucide--eye]', | ||
| 'IP-ADAPTER': 'icon-[lucide--image]', | ||
| SAM: 'icon-[lucide--scissors]', | ||
| DIFFUSION: 'icon-[lucide--sparkles]', | ||
| ANIMATEDIFF: 'icon-[lucide--film]', | ||
| 'MOTION LORA': 'icon-[lucide--move]', | ||
| AUDIO: 'icon-[lucide--music]', | ||
| CLIP: 'icon-[lucide--text]', | ||
| UNET: 'icon-[lucide--network]', | ||
| GLIGEN: 'icon-[lucide--grid-3x3]', | ||
| STYLE: 'icon-[lucide--palette]', | ||
| PHOTOMAKER: 'icon-[lucide--camera]' | ||
| } | ||
| return icons[type] || 'icon-[comfy--ai-model]' |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hoist the icon map to module scope to avoid per-call allocations.
This avoids recreating the same Record on every lookup.
♻️ Proposed refactor
+const MODEL_TYPE_ICONS: Record<string, string> = {
+ CHECKPOINT: 'icon-[lucide--box]',
+ LORA: 'icon-[lucide--layers]',
+ VAE: 'icon-[lucide--cpu]',
+ CONTROLNET: 'icon-[lucide--sliders-horizontal]',
+ EMBEDDING: 'icon-[lucide--type]',
+ UPSCALE: 'icon-[lucide--arrow-up]',
+ 'CLIP VISION': 'icon-[lucide--eye]',
+ 'IP-ADAPTER': 'icon-[lucide--image]',
+ SAM: 'icon-[lucide--scissors]',
+ DIFFUSION: 'icon-[lucide--sparkles]',
+ ANIMATEDIFF: 'icon-[lucide--film]',
+ 'MOTION LORA': 'icon-[lucide--move]',
+ AUDIO: 'icon-[lucide--music]',
+ CLIP: 'icon-[lucide--text]',
+ UNET: 'icon-[lucide--network]',
+ GLIGEN: 'icon-[lucide--grid-3x3]',
+ STYLE: 'icon-[lucide--palette]',
+ PHOTOMAKER: 'icon-[lucide--camera]'
+}
+
export function getModelTypeIcon(type: string): string {
- const icons: Record<string, string> = {
- CHECKPOINT: 'icon-[lucide--box]',
- LORA: 'icon-[lucide--layers]',
- VAE: 'icon-[lucide--cpu]',
- CONTROLNET: 'icon-[lucide--sliders-horizontal]',
- EMBEDDING: 'icon-[lucide--type]',
- UPSCALE: 'icon-[lucide--arrow-up]',
- 'CLIP VISION': 'icon-[lucide--eye]',
- 'IP-ADAPTER': 'icon-[lucide--image]',
- SAM: 'icon-[lucide--scissors]',
- DIFFUSION: 'icon-[lucide--sparkles]',
- ANIMATEDIFF: 'icon-[lucide--film]',
- 'MOTION LORA': 'icon-[lucide--move]',
- AUDIO: 'icon-[lucide--music]',
- CLIP: 'icon-[lucide--text]',
- UNET: 'icon-[lucide--network]',
- GLIGEN: 'icon-[lucide--grid-3x3]',
- STYLE: 'icon-[lucide--palette]',
- PHOTOMAKER: 'icon-[lucide--camera]'
- }
- return icons[type] || 'icon-[comfy--ai-model]'
+ return MODEL_TYPE_ICONS[type] || 'icon-[comfy--ai-model]'
}🤖 Prompt for AI Agents
In `@src/platform/models/browser/utils/modelTypeIcons.ts` around lines 5 - 26, The
icons map is being recreated on every call inside getModelTypeIcon causing
unnecessary allocations; hoist the Record to module scope as a const (e.g.,
MODEL_TYPE_ICONS) and have getModelTypeIcon(type: string) simply return
MODEL_TYPE_ICONS[type] || 'icon-[comfy--ai-model]'; update references to use the
new module-level constant and keep the exported function signature the same.
Comment on lines
+300
to
+316
| describe('widget value setting', () => { | ||
| it('should set widget value before adding to graph', async () => { | ||
| const mockNode = await createMockNode() | ||
| const addSpy = vi.fn() | ||
| vi.mocked(LiteGraph.createNode).mockReturnValue(mockNode) | ||
| if (app.canvas.graph) { | ||
| app.canvas.graph.add = addSpy | ||
| } | ||
|
|
||
| createNodeFromModel('checkpoints', 'test-model.safetensors', 'model-123') | ||
|
|
||
| // Widget value should be set | ||
| expect(mockNode.widgets![0].value).toBe('test-model.safetensors') | ||
| // And node should be added to graph | ||
| expect(addSpy).toHaveBeenCalledWith(mockNode) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test doesn't verify widget value is set before graph add.
The test name "should set widget value before adding to graph" implies ordering verification, but it only asserts both happened. To verify ordering, you could track call order or use a spy that captures the widget value at the time add is called.
♻️ Example: Verify ordering with call-time capture
it('should set widget value before adding to graph', async () => {
const mockNode = await createMockNode()
- const addSpy = vi.fn()
+ let widgetValueAtAddTime: string | undefined
+ const addSpy = vi.fn(() => {
+ widgetValueAtAddTime = mockNode.widgets![0].value as string
+ })
vi.mocked(LiteGraph.createNode).mockReturnValue(mockNode)
if (app.canvas.graph) {
app.canvas.graph.add = addSpy
}
createNodeFromModel('checkpoints', 'test-model.safetensors', 'model-123')
- // Widget value should be set
- expect(mockNode.widgets![0].value).toBe('test-model.safetensors')
- // And node should be added to graph
+ // Widget value should have been set BEFORE add was called
+ expect(widgetValueAtAddTime).toBe('test-model.safetensors')
expect(addSpy).toHaveBeenCalledWith(mockNode)
})📝 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
| describe('widget value setting', () => { | |
| it('should set widget value before adding to graph', async () => { | |
| const mockNode = await createMockNode() | |
| const addSpy = vi.fn() | |
| vi.mocked(LiteGraph.createNode).mockReturnValue(mockNode) | |
| if (app.canvas.graph) { | |
| app.canvas.graph.add = addSpy | |
| } | |
| createNodeFromModel('checkpoints', 'test-model.safetensors', 'model-123') | |
| // Widget value should be set | |
| expect(mockNode.widgets![0].value).toBe('test-model.safetensors') | |
| // And node should be added to graph | |
| expect(addSpy).toHaveBeenCalledWith(mockNode) | |
| }) | |
| }) | |
| describe('widget value setting', () => { | |
| it('should set widget value before adding to graph', async () => { | |
| const mockNode = await createMockNode() | |
| let widgetValueAtAddTime: string | undefined | |
| const addSpy = vi.fn(() => { | |
| widgetValueAtAddTime = mockNode.widgets![0].value as string | |
| }) | |
| vi.mocked(LiteGraph.createNode).mockReturnValue(mockNode) | |
| if (app.canvas.graph) { | |
| app.canvas.graph.add = addSpy | |
| } | |
| createNodeFromModel('checkpoints', 'test-model.safetensors', 'model-123') | |
| // Widget value should have been set BEFORE add was called | |
| expect(widgetValueAtAddTime).toBe('test-model.safetensors') | |
| expect(addSpy).toHaveBeenCalledWith(mockNode) | |
| }) | |
| }) |
🤖 Prompt for AI Agents
In `@src/utils/nodeCreation/createNodeFromModel.test.ts` around lines 300 - 316,
The test currently asserts widget value and that graph.add was called but not
that the value was set before the add call; change the test for
createNodeFromModel so the addSpy captures the widget value at call time (e.g.,
set addSpy = vi.fn((node) => { capturedValue = node.widgets![0].value })) and
assert that capturedValue === 'test-model.safetensors' (in addition to the
existing expect on mockNode.widgets![0].value and
expect(addSpy).toHaveBeenCalledWith(mockNode)), ensuring you still mock
LiteGraph.createNode and use mockNode and addSpy to verify ordering.
Comment on lines
+8
to
+25
| interface CreateNodeOptions { | ||
| position?: Point | ||
| } | ||
|
|
||
| type NodeCreationErrorCode = | ||
| | 'NO_PROVIDER' | ||
| | 'NODE_CREATION_FAILED' | ||
| | 'MISSING_WIDGET' | ||
| | 'NO_GRAPH' | ||
|
|
||
| interface NodeCreationError { | ||
| code: NodeCreationErrorCode | ||
| message: string | ||
| itemId: string | ||
| details?: Record<string, unknown> | ||
| } | ||
|
|
||
| type Result<T, E> = { success: true; value: T } | { success: false; error: E } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Searching for NodeCreationErrorCode type definitions ==="
rg -n "type NodeCreationErrorCode" --type=ts -B2 -A8
echo ""
echo "=== Searching for NodeCreationError interface definitions ==="
rg -n "interface NodeCreationError" --type=ts -B2 -A6
echo ""
echo "=== Searching for Result<T, E> type definitions ==="
rg -n "type Result<T, E>" --type=ts -B2 -A2
echo ""
echo "=== Searching for CreateNodeOptions interface definitions ==="
rg -n "interface CreateNodeOptions" --type=ts -B2 -A4
echo ""
echo "=== List all node creation files ==="
fd "createNode" --type=f --extension=tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 7396
Extract CreateNodeOptions and Result<T, E> to a shared types file.
The types CreateNodeOptions and Result<T, E> are identical across src/utils/nodeCreation/createNodeFromModel.ts, src/platform/models/browser/utils/createModelNode.ts, and src/platform/assets/utils/createModelNodeFromAsset.ts and are good candidates for extraction to a shared types module.
However, NodeCreationErrorCode and NodeCreationError have context-specific variations: the asset version includes an additional INVALID_ASSET error code, and each file uses different identifier field names (itemId, modelId, assetId) to reflect domain semantics. Extracting these would require generics or base types and should be evaluated separately.
🤖 Prompt for AI Agents
In `@src/utils/nodeCreation/createNodeFromModel.ts` around lines 8 - 25, Extract
the shared types CreateNodeOptions and Result<T, E> into a new shared types
module and replace the local definitions in createNodeFromModel.ts,
createModelNode.ts, and createModelNodeFromAsset.ts with imports from that
module; keep NodeCreationErrorCode and NodeCreationError local (do not extract)
because they vary by domain, and ensure every file updates its imports to use
the new exported type names (CreateNodeOptions, Result) so existing references
compile without renaming.
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
2 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
Add model browser dialog for exploring and managing AI models in ComfyUI with search, filtering, and keyboard navigation.
Changes
Review Focus
Implementation Details
Components:
ModelBrowserDialog.vue- Main dialog with BaseModalLayoutModelCard.vue/ModelListItem.vue- Grid and list view itemsModelBrowserFilterBar.vue- Multi-select filters and sort controlsModelBrowserStates.vue- Loading, error, and empty statesLocalModelInfoPanel.vue- Right panel with model detailsComposables:
useModelLoader.ts- Fetches and transforms ComfyModelDef to EnrichedModeluseModelBrowserFiltering.ts- Search/filter/sort logic with debouncinguseModelKeyboardNav.ts- Arrow navigation, enter/escape handlersuseModelFilterOptions.ts- Extracts available formats and typesFeatures Implemented:
Test Coverage:
┆Issue is synchronized with this Notion page by Unito