feat: basic model listing / search page #439
Conversation
WalkthroughA new "Models" section has been added to the web UI's navigation and sidebar, along with a dedicated Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ModelsPage
participant RemoteConfig
participant ProviderStore
User->>ModelsPage: Navigates to /models
ModelsPage->>RemoteConfig: Fetch model library JSON
RemoteConfig-->>ModelsPage: Returns model data
ModelsPage->>ProviderStore: Load connected providers
ProviderStore-->>ModelsPage: Returns provider connection status
ModelsPage->>User: Displays searchable/filterable/sortable model library
User->>ModelsPage: Applies search/filter/sort
ModelsPage->>User: Updates displayed models accordingly
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)app/web_ui/src/**/*.{js,jsx,ts,tsx,svelte}📄 CodeRabbit Inference Engine (.cursor/rules/project.mdc)
Files:
🧠 Learnings (5)📓 Common learnings📚 Learning: in the kiln web ui's project import functionality (edit_project.svelte), when the file selector api ...Applied to files:
📚 Learning: in the kiln ai codebase, the friendly_name field always exists on model objects and is never undefin...Applied to files:
📚 Learning: leonardmq prefers unused imports to be flagged as actionable review comments rather than nitpick com...Applied to files:
📚 Learning: leonardmq prefers to keep defensive validation checks even when they're technically redundant, viewi...Applied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (11)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/web_ui/src/routes/(app)/models/+page.svelte (5)
145-147: Consider making the remote config URL configurable.The URL is currently hard-coded. Consider moving it to an environment variable or configuration file for better flexibility across environments.
+import { PUBLIC_REMOTE_CONFIG_URL } from '$env/static/public' + const response = await fetch( - "https://remote-config.getkiln.ai/kiln_config.json", + PUBLIC_REMOTE_CONFIG_URL || "https://remote-config.getkiln.ai/kiln_config.json", )
429-434: Consider debouncing the search input for better performance.Currently, filtering runs on every keystroke. For large model lists, this could impact performance.
Add a debounced search handler:
import { debounce } from '$lib/utils/debounce' let searchQuery = "" let debouncedSearchQuery = "" const handleSearch = debounce((value: string) => { debouncedSearchQuery = value }, 300) $: handleSearch(searchQuery) $: debouncedSearchQuery, selectedFamily, selectedProvider, selectedCapability, models, applyFilters()Then update the filter to use
debouncedSearchQueryinstead ofsearchQueryin theapplyFiltersfunction.
653-657: Improve image error handling with a fallback.Currently, failed images are just hidden, leaving empty space. Consider showing a fallback icon or text.
+let imageErrors: Set<string> = new Set() + on:error={(e) => { if (e.target instanceof HTMLImageElement) { - e.target.style.display = "none" + imageErrors.add(provider.name) + imageErrors = imageErrors // Trigger reactivity } }}Then conditionally render a fallback:
{#if !imageErrors.has(provider.name)} <img ... /> {:else} <div class="w-6 h-6 rounded bg-gray-200 flex items-center justify-center text-xs"> {provider.name.substring(0, 2).toUpperCase()} </div> {/if}
1-700: Consider breaking this component into smaller, reusable parts.At 700 lines, this component handles many responsibilities. Consider extracting:
- ModelCard component for individual model display
- FilterBar component for search and filters
- CapabilityBadges component
This would improve maintainability and reusability.
79-89: Consider filtering providers based on selected family.Currently, the provider dropdown shows all providers even when a family is selected. Consider showing only providers that offer models from the selected family.
$: availableProviders = selectedFamily ? [...new Set( models .filter(m => m.family === selectedFamily) .flatMap(m => m.providers.map(p => p.name)) )].sort() : providers $: providerOptions = [ { options: [ { label: "All Providers", value: "" }, ...availableProviders.map((provider) => ({ label: provider, value: provider, })), ], }, ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/web_ui/src/routes/(app)/+layout.svelte(3 hunks)app/web_ui/src/routes/(app)/models/+page.svelte(1 hunks)app/web_ui/src/routes/(app)/models/+page.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: leonardmq
PR: Kiln-AI/Kiln#341
File: libs/server/kiln_server/document_api.py:44-51
Timestamp: 2025-06-18T08:22:58.510Z
Learning: leonardmq prefers to defer fixing blocking I/O in async handlers when: the operation is very fast (milliseconds), user-triggered rather than automated, has no concurrency concerns, and would require additional testing to fix properly. He acknowledges such issues as valid but makes pragmatic decisions about timing the fixes.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#402
File: libs/core/kiln_ai/adapters/embedding/litellm_embedding_adapter.py:0-0
Timestamp: 2025-07-14T03:43:07.265Z
Learning: leonardmq prefers to keep defensive validation checks even when they're technically redundant, viewing them as useful "quick sanity checks" that provide additional safety nets. He values defensive programming over strict DRY (Don't Repeat Yourself) principles when the redundant code serves as a safeguard.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#388
File: libs/core/kiln_ai/datamodel/test_extraction_chunk.py:49-74
Timestamp: 2025-07-03T05:13:02.873Z
Learning: leonardmq prefers automatic cleanup of temporary files in tests using `delete=True` in `tempfile.NamedTemporaryFile()` context managers, rather than manual cleanup with `delete=False` and explicit file removal. This is because automatic cleanup is simpler, safer, and avoids leaving temporary files if tests fail before cleanup.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#0
File: :0-0
Timestamp: 2025-06-27T06:45:06.591Z
Learning: leonardmq prefers unused imports to be flagged as actionable review comments rather than nitpick comments. Unused imports should be treated as concrete issues that need to be addressed, not just suggestions.
🔇 Additional comments (5)
app/web_ui/src/routes/(app)/models/+page.ts (1)
1-1: LGTM!Disabling SSR is appropriate for this client-side interactive page with dynamic filtering and searching.
app/web_ui/src/routes/(app)/+layout.svelte (3)
28-28: LGTM!The Models enum addition follows the existing pattern correctly.
76-77: LGTM!The path detection for
/modelsis implemented consistently with other routes.
403-432: LGTM!The Models menu item is well-structured with proper SVG attribution and consistent styling.
app/web_ui/src/routes/(app)/models/+page.svelte (1)
228-228: Review Browser Compatibility fortoSorted()The
Array.prototype.toSorted()method was introduced in ES2023 (June 2023) and is supported in:
- Chrome 110+
- Edge 110+
- Firefox 115+
- Safari 16.4+
- Opera 96+
It returns a new sorted array without mutating the original. If your application must run on any browsers older than these versions, you’ll need to either:
• Add a polyfill (e.g., via core-js)
• Or fall back to a non-mutatingsort():- filteredModels = filteredModels.toSorted((a, b) => { + filteredModels = [...filteredModels].sort((a, b) => {Location to verify:
- app/web_ui/src/routes/(app)/models/+page.svelte, line 228
Please confirm your supported browser matrix and adjust accordingly.
|
whaaaat |
Some minor wrapping issues
|
|
This is great. Some small nits: Can we make "suggested" models pop a little at the top level?
Tooltips need a little work:
title "Models" "Model Library" |
|
We should probably make friendly family names (or we can just skip showing them) |
|
Provider names should be pretty. We have a utility for this. |
|
We should have a UI indicator if I don't have a provider connected. Something subtle. just color ideally, great out logo/title a bit? Clicking should take you to connect_providers |
|
"No models found" SVG needs some work. Sonnet can't draw. Sidebar icon should be: https://www.svgrepo.com/svg/524675/library |
leonardmq
force-pushed
the
leonard/model-library
branch
from
2d6827b to
b6a81b8
Compare
…leonard/model-library
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/web_ui/src/routes/(app)/models/+page.svelte (2)
156-183: Consider adding request timeout and better error specificity.The function correctly fetches from the remote config, but could benefit from:
- Request timeout to prevent hanging requests
- More specific error messages for different failure scenarios
async function fetchModelsFromRemoteConfig() { try { loading = true error = null - const response = await fetch( - "https://remote-config.getkiln.ai/kiln_config.json", - ) + const controller = new AbortController() + const timeoutId = setTimeout(() => controller.abort(), 10000) // 10 second timeout + + const response = await fetch( + "https://remote-config.getkiln.ai/kiln_config.json", + { signal: controller.signal } + ) + clearTimeout(timeoutId) if (!response.ok) { - throw new Error(`Failed to fetch models: ${response.status}`) + throw new Error(`Failed to fetch models: ${response.status} ${response.statusText}`) }
252-280: Consider performance optimization for large datasets.The sorting implementation is correct but uses
toSorted()which creates a new array. For large datasets, consider performance implications.- filteredModels = filteredModels.toSorted((a, b) => { + filteredModels.sort((a, b) => {Note: This would modify the array in place rather than creating a new one, which is more memory efficient for large datasets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/web_ui/src/lib/utils/provider_utils.ts(1 hunks)app/web_ui/src/routes/(app)/+layout.svelte(3 hunks)app/web_ui/src/routes/(app)/models/+page.svelte(1 hunks)app/web_ui/src/routes/(app)/settings/providers/add_models/+page.svelte(2 hunks)app/web_ui/src/routes/(fullscreen)/setup/(setup)/connect_providers/connect_providers.svelte(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/web_ui/src/routes/(app)/+layout.svelte
🧰 Additional context used
📓 Path-based instructions (1)
app/web_ui/src/**/*.{js,jsx,ts,tsx,svelte}
📄 CodeRabbit Inference Engine (.cursor/rules/project.mdc)
Our web UI component framework is DaisyUI v4 (based on Tailwind). Be sure to use v4 docs and features, not v5
Files:
app/web_ui/src/lib/utils/provider_utils.tsapp/web_ui/src/routes/(app)/settings/providers/add_models/+page.svelteapp/web_ui/src/routes/(fullscreen)/setup/(setup)/connect_providers/connect_providers.svelteapp/web_ui/src/routes/(app)/models/+page.svelte
🧠 Learnings (1)
📓 Common learnings
Learnt from: leonardmq
PR: Kiln-AI/Kiln#390
File: libs/core/kiln_ai/adapters/provider_tools.py:494-499
Timestamp: 2025-07-23T08:58:45.769Z
Learning: leonardmq prefers to keep tightly coupled implementation details (like API versions) hardcoded when they are not user-facing and could break other modules if changed. The shared Config class is reserved for user-facing customizable values, not internal implementation details.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#341
File: libs/server/kiln_server/document_api.py:44-51
Timestamp: 2025-06-18T08:22:58.510Z
Learning: leonardmq prefers to defer fixing blocking I/O in async handlers when: the operation is very fast (milliseconds), user-triggered rather than automated, has no concurrency concerns, and would require additional testing to fix properly. He acknowledges such issues as valid but makes pragmatic decisions about timing the fixes.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#402
File: libs/core/kiln_ai/adapters/embedding/litellm_embedding_adapter.py:0-0
Timestamp: 2025-07-14T03:43:07.283Z
Learning: leonardmq prefers to keep defensive validation checks even when they're technically redundant, viewing them as useful "quick sanity checks" that provide additional safety nets. He values defensive programming over strict DRY (Don't Repeat Yourself) principles when the redundant code serves as a safeguard.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#388
File: libs/core/kiln_ai/datamodel/test_extraction_chunk.py:49-74
Timestamp: 2025-07-03T05:13:02.873Z
Learning: leonardmq prefers automatic cleanup of temporary files in tests using `delete=True` in `tempfile.NamedTemporaryFile()` context managers, rather than manual cleanup with `delete=False` and explicit file removal. This is because automatic cleanup is simpler, safer, and avoids leaving temporary files if tests fail before cleanup.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#0
File: :0-0
Timestamp: 2025-06-27T06:45:06.591Z
Learning: leonardmq prefers unused imports to be flagged as actionable review comments rather than nitpick comments. Unused imports should be treated as concrete issues that need to be addressed, not just suggestions.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Desktop Apps (windows-latest)
- GitHub Check: Build Desktop Apps (ubuntu-22.04-arm)
- GitHub Check: Build Desktop Apps (macos-latest)
- GitHub Check: Build Desktop Apps (macos-13)
- GitHub Check: Build Desktop Apps (ubuntu-22.04)
🔇 Additional comments (19)
app/web_ui/src/lib/utils/provider_utils.ts (2)
1-1: LGTM! Good type definition for provider settings.The
ProviderSettingstype alias provides appropriate flexibility for the settings structure while maintaining type safety.
6-53: Well-structured provider connection utility function.The centralized approach using a switch statement is clean and maintainable. The function correctly handles different provider authentication patterns:
- Single API key providers (OpenAI, Anthropic, etc.)
- Multi-credential providers (Amazon Bedrock, Fireworks AI, etc.)
- Array-based providers (OpenAI compatible)
- Special cases like Ollama
The double negation (
!!) ensures boolean return values, and the default case appropriately returnsfalsefor unknown provider IDs.app/web_ui/src/routes/(app)/settings/providers/add_models/+page.svelte (2)
9-9: LGTM! Proper import of the utility function.Good use of the centralized provider connection utility.
33-35: Excellent refactoring using the centralized utility.The refactored logic is much cleaner and more maintainable:
- Eliminates repetitive manual checks
- Uses the centralized
checkProviderConnectionfunction- Maintains the same functionality with less code
This approach ensures consistency across the application for provider connection checks.
app/web_ui/src/routes/(fullscreen)/setup/(setup)/connect_providers/connect_providers.svelte (3)
13-13: LGTM! Proper import of the utility function.Consistent use of the centralized provider connection utility across components.
530-559: Excellent refactoring with improved error handling.The refactored logic provides several improvements:
- Uses the centralized
checkProviderConnectionutility for consistency- Cleaner iteration through provider IDs
- Better error handling with specific error throwing
- Maintains the same functionality with more maintainable code
The approach ensures all provider connection checks use the same logic across the application.
582-582: Nice typo fix in the comment.The correction from "epmemerial" to "ephemeral" improves code readability.
app/web_ui/src/routes/(app)/models/+page.svelte (12)
18-42: Well-designed TypeScript interfaces.The interfaces provide good type safety and clearly define the data structure:
Providerinterface captures all the necessary model provider attributesModelinterface with providers array supports multiple provider scenariosConfigDatawrapper provides proper typing for the API responseThe interface design supports the filtering and capability features effectively.
44-69: Good state management organization.The state variables are well-organized and cover all the necessary functionality:
- Separate state for models, filtered results, and UI states
- Clear separation of search, filter, and sort state
- Predefined capability options with user-friendly labels
The organization makes the component logic easy to follow.
78-140: Excellent reactive data transformations.The reactive statements effectively transform arrays into the required option group format for the FancySelect component. The active filters computation provides a clean way to display current filter state with proper labels.
185-196: LGTM! Proper provider settings loading.The function correctly loads provider settings with appropriate error handling and logging.
199-249: Robust filtering implementation.The filtering logic is comprehensive and handles all the required scenarios:
- Text search across multiple model fields
- Family, provider, and capability filtering
- Proper case-insensitive string matching
- Good use of array methods for provider capability checks
The implementation correctly combines all filter criteria.
318-371: Excellent capability badge implementation with user-friendly tooltips.The function provides comprehensive capability badges with descriptive tooltips that align with the PR feedback requirements:
- "Data Gen ★" and "Evals ★" badges for recommended models
- Descriptive tooltips explaining each capability
- Proper color coding for different badge types
- Handles uncensored models with appropriate explanations
The implementation addresses the PR feedback about making tooltips more descriptive and user-friendly.
394-402: Smart provider connection handling.The function correctly uses the centralized provider connection utility and provides intuitive UX by navigating disconnected users to the provider settings page.
405-415: Proper page metadata and title structure.The page uses appropriate title and subtitle that align with the PR feedback ("Model Library" and "Browse available models").
417-460: Comprehensive loading and error states.The loading and error states provide good user experience:
- Clear loading indicators
- Detailed error messages
- Retry functionality
- Proper error styling using DaisyUI v4 classes
The implementation handles edge cases gracefully.
462-636: Well-structured search and filter UI.The search and filter interface provides excellent UX:
- Clear search input with proper placeholder
- Organized filter dropdowns using FancySelect
- Active filter badges with removal functionality
- Results count display
- Clear filters functionality
- Sorting controls with visual indicators
The implementation uses appropriate DaisyUI v4 classes and follows good accessibility practices.
640-661: User-friendly empty state.The "No models found" state provides helpful guidance to users with clear messaging and suggestions for adjusting search criteria.
663-770: Excellent model grid implementation with provider connection indicators.The model grid provides comprehensive information:
- Clean card layout with proper hierarchy
- Capability badges with tooltips
- Provider information with connection status
- Visual indicators for disconnected providers (grayscale, opacity)
- Clickable navigation for disconnected providers
- Proper error handling for missing provider images
- Feature indicator dots with tooltips
The implementation addresses the PR feedback about showing provider connection status and follows DaisyUI v4 patterns.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/web_ui/src/routes/(app)/models/+page.svelte (3)
157-197: Solid data fetching implementation with proper error handling.Both functions handle errors appropriately and follow good async/await patterns. The provider settings loading integrates well with the existing API client pattern.
Consider making the remote config URL configurable rather than hardcoded:
- const response = await fetch( - "https://remote-config.getkiln.ai/kiln_config.json", - ) + const configUrl = import.meta.env.VITE_REMOTE_CONFIG_URL || "https://remote-config.getkiln.ai/kiln_config.json" + const response = await fetch(configUrl)
687-687: Consider using word-break instead of break-all.Using
break-allcan break words at any character, which may harm readability. Considerbreak-wordsinstead.- <h3 class="text-lg font-semibold text-gray-900 break-all"> + <h3 class="text-lg font-semibold text-gray-900 break-words">
754-754: Consider using break-words for model IDs.Similar to the model name,
break-allmight break model IDs at inappropriate points.- class="text-xs {isConnected - ? 'text-gray-500' - : 'text-gray-400'} break-all" + class="text-xs {isConnected + ? 'text-gray-500' + : 'text-gray-400'} break-words"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
app/web_ui/static/images/delete.svgis excluded by!**/*.svgapp/web_ui/static/images/library.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
app/web_ui/src/routes/(app)/+layout.svelte(3 hunks)app/web_ui/src/routes/(app)/models/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/web_ui/src/routes/(app)/+layout.svelte
🧰 Additional context used
📓 Path-based instructions (1)
app/web_ui/src/**/*.{js,jsx,ts,tsx,svelte}
📄 CodeRabbit Inference Engine (.cursor/rules/project.mdc)
Our web UI component framework is DaisyUI v4 (based on Tailwind). Be sure to use v4 docs and features, not v5
Files:
app/web_ui/src/routes/(app)/models/+page.svelte
🧠 Learnings (1)
📓 Common learnings
Learnt from: leonardmq
PR: Kiln-AI/Kiln#390
File: libs/core/kiln_ai/adapters/provider_tools.py:494-499
Timestamp: 2025-07-23T08:58:45.769Z
Learning: leonardmq prefers to keep tightly coupled implementation details (like API versions) hardcoded when they are not user-facing and could break other modules if changed. The shared Config class is reserved for user-facing customizable values, not internal implementation details.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#341
File: libs/server/kiln_server/document_api.py:44-51
Timestamp: 2025-06-18T08:22:58.510Z
Learning: leonardmq prefers to defer fixing blocking I/O in async handlers when: the operation is very fast (milliseconds), user-triggered rather than automated, has no concurrency concerns, and would require additional testing to fix properly. He acknowledges such issues as valid but makes pragmatic decisions about timing the fixes.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#402
File: libs/core/kiln_ai/adapters/embedding/litellm_embedding_adapter.py:0-0
Timestamp: 2025-07-14T03:43:07.283Z
Learning: leonardmq prefers to keep defensive validation checks even when they're technically redundant, viewing them as useful "quick sanity checks" that provide additional safety nets. He values defensive programming over strict DRY (Don't Repeat Yourself) principles when the redundant code serves as a safeguard.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#388
File: libs/core/kiln_ai/datamodel/test_extraction_chunk.py:49-74
Timestamp: 2025-07-03T05:13:02.873Z
Learning: leonardmq prefers automatic cleanup of temporary files in tests using `delete=True` in `tempfile.NamedTemporaryFile()` context managers, rather than manual cleanup with `delete=False` and explicit file removal. This is because automatic cleanup is simpler, safer, and avoids leaving temporary files if tests fail before cleanup.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#0
File: :0-0
Timestamp: 2025-06-27T06:45:06.591Z
Learning: leonardmq prefers unused imports to be flagged as actionable review comments rather than nitpick comments. Unused imports should be treated as concrete issues that need to be addressed, not just suggestions.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Desktop Apps (ubuntu-22.04-arm)
- GitHub Check: Build Desktop Apps (ubuntu-22.04)
- GitHub Check: Build Desktop Apps (macos-13)
- GitHub Check: Build Desktop Apps (macos-latest)
- GitHub Check: Build Desktop Apps (windows-latest)
🔇 Additional comments (7)
app/web_ui/src/routes/(app)/models/+page.svelte (7)
19-43: Well-structured TypeScript interfaces.The interface definitions are comprehensive and clearly define the data structures for providers, models, and configuration data. The boolean flags in the Provider interface align well with the filtering capabilities implemented later in the component.
45-76: Comprehensive state management setup.The state variables are well-organized and cover all aspects of the model library functionality including search, filtering, sorting, and data management. The capabilities array provides a clean mapping between internal values and user-facing labels.
79-140: Effective use of Svelte reactivity.The reactive statements properly transform data for UI components and create clean filter badge representations. The use of Svelte's
$:syntax is appropriate and the computed properties will update correctly when dependencies change.
143-155: Clean filter removal implementation.The
removeFilterfunction properly handles all filter types with a clear switch statement. The implementation is straightforward and effective.
199-309: Comprehensive filtering and sorting implementation.The filtering logic properly handles multiple criteria including text search, family, provider, and capability filters. The use of
toSorted()maintains immutability, and the sort toggling logic provides a good user experience by cycling through ascending, descending, and no sorting.
311-403: Well-designed utility functions and reactive coordination.The
getCapabilityBadgesfunction creates comprehensive badge information with appropriate tooltips that align with the PR requirements. The reactive statements properly coordinate filtering and sorting updates, and theonMountfunction correctly orchestrates the data loading sequence.
406-791: Excellent template implementation with comprehensive functionality.The template effectively implements all the requirements from the PR objectives including:
- Model search and filtering capabilities
- Provider connection status indicators
- Capability badges with descriptive tooltips
- Responsive grid layout
- Proper loading and error states
- Clean, polished UI using DaisyUI v4
The implementation aligns well with the feedback suggestions and provides a solid foundation for the model library feature.
|
Made changes based on all the comments above, except for greying out providers the user hasn’t connected yet for which
Ideally, we would keep the provider connection info in a Svelte store, but that will add complexity (and I am not totally solid on the patterns around stores in Svelte). Our current store setup already has some issues (refreshing the browser sometimes not triggering the correct initialization, invalidation logic not propagating correctly, and scattered triggering logic). Adding a provider store would add more complexity (maybe justified?): we would need to correctly refresh it if the user connects a provider, handle edge cases like multi-tab scenarios, etc. Regarding provider friendly names: I found the Open question: should this page pull from |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/web_ui/src/routes/(app)/models/+page.svelte (2)
672-673: Improve text contrast for accessibility.The text color class should provide better contrast for accessibility compliance.
The existing past review comment correctly identified this issue. Consider using a darker text color:
- <h3 class="mt-2 text-sm font-medium text-gray-900"> + <h3 class="mt-2 text-sm font-medium text-gray-900">Note: The past comment suggested changing from
text-neutral-contenttotext-gray-900, but the current code already usestext-gray-900which is correct.
720-728: Address accessibility concerns properly.The svelte-ignore comments disable important accessibility checks for click events on a div element.
As noted in the past review, this creates accessibility issues. The existing suggestion to add proper keyboard navigation and ARIA attributes should be implemented:
- <!-- svelte-ignore a11y-click-events-have-key-events --> - <!-- svelte-ignore a11y-no-static-element-interactions --> <div class="flex items-center justify-between p-3 bg-gray-50 rounded-md {!isConnected ? 'opacity-50 cursor-pointer hover:opacity-60 transition-all' : ''}" + role={!isConnected ? "button" : undefined} + tabindex={!isConnected ? "0" : undefined} on:click={!isConnected ? () => handle_provider_click(provider.name) : undefined} + on:keydown={!isConnected + ? (e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + handle_provider_click(provider.name); + } + } + : undefined} >
🧹 Nitpick comments (7)
app/web_ui/src/routes/(app)/models/+page.svelte (7)
7-10: Consider using direct imports instead of barrel exports.The import from
../../../lib/storessuggests using a barrel export pattern. Consider importingload_available_modelsandprovider_name_from_iddirectly from their specific modules for better tree-shaking and clearer dependencies.- import { - load_available_models, - provider_name_from_id, - } from "../../../lib/stores" + import { load_available_models } from "../../../lib/stores/models" + import { provider_name_from_id } from "../../../lib/stores/providers"
200-250: Optimize search performance with debouncing.The search function runs on every keystroke which could impact performance with large model lists. Consider debouncing the search input.
You can implement debouncing by adding a debounced search function:
import { debounce } from 'lodash-es' // Add after other reactive statements const debouncedApplyFilters = debounce(applyFilters, 300) // Replace the reactive statement at line 375-380 $: searchQuery, debouncedApplyFilters()
374-383: Optimize reactive statements to prevent unnecessary updates.The current reactive statements may cause excessive re-runs. Consider combining related reactive logic and using more specific dependencies.
- // Watch for filter changes - $: searchQuery, - selectedFamily, - selectedProvider, - selectedCapability, - models, - applyFilters() - - // Watch for sort changes - $: sortBy, sortDirection, applySorting() + // Combined reactive statement for all changes + $: { + if (models.length > 0) { + applyFilters() + } + }
385-393: Consider parallel loading for better performance.The sequential loading of models and provider settings could be optimized by running them in parallel since they're independent operations.
onMount(async () => { // we need to seed the available models in the Svelte store, because we get // the provider friendly name formatter from there, but we load the actual // model list from the remote config, because that is the most up to date - await load_available_models() - await fetchModelsFromRemoteConfig() - // Load provider connection status for displaying connection indicators - await load_provider_settings() + await Promise.all([ + load_available_models(), + fetchModelsFromRemoteConfig(), + load_provider_settings() + ]) })
737-741: Improve image error handling.The current error handler hides the image but leaves an empty space. Consider showing a fallback icon or placeholder.
on:error={(e) => { if (e.target instanceof HTMLImageElement) { - e.target.style.display = "none" + e.target.src = "/fallback-provider-icon.svg" + e.target.alt = `${provider.name} (fallback icon)` } }}
614-615: Improve CSS class readability with consistent formatting.The long conditional class string is hard to read and maintain. Consider extracting the logic or using a more readable format.
- class="inline-flex items-center px-3 py-1.5 text-sm font-medium rounded-md transition-colors {sortBy === - option.value - ? 'text-gray-700 bg-gray-100 hover:bg-gray-200' - : 'text-gray-600 hover:text-gray-900 hover:bg-gray-100 border border-transparent'}" + class="inline-flex items-center px-3 py-1.5 text-sm font-medium rounded-md transition-colors {sortBy === option.value + ? 'text-gray-700 bg-gray-100 hover:bg-gray-200' + : 'text-gray-600 hover:text-gray-900 hover:bg-gray-100 border border-transparent'}"
162-164: Consider making the remote config URL configurable.The hardcoded URL could be made configurable through environment variables for different deployment environments.
+ import { env } from '$env/dynamic/public' - const response = await fetch( - "https://remote-config.getkiln.ai/kiln_config.json", - ) + const configUrl = env.PUBLIC_REMOTE_CONFIG_URL || "https://remote-config.getkiln.ai/kiln_config.json" + const response = await fetch(configUrl)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/web_ui/src/routes/(app)/models/+page.svelte(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/web_ui/src/**/*.{js,jsx,ts,tsx,svelte}
📄 CodeRabbit Inference Engine (.cursor/rules/project.mdc)
Our web UI component framework is DaisyUI v4 (based on Tailwind). Be sure to use v4 docs and features, not v5
Files:
app/web_ui/src/routes/(app)/models/+page.svelte
🧠 Learnings (1)
📓 Common learnings
Learnt from: leonardmq
PR: Kiln-AI/Kiln#390
File: libs/core/kiln_ai/adapters/provider_tools.py:494-499
Timestamp: 2025-07-23T08:58:45.769Z
Learning: leonardmq prefers to keep tightly coupled implementation details (like API versions) hardcoded when they are not user-facing and could break other modules if changed. The shared Config class is reserved for user-facing customizable values, not internal implementation details.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#341
File: libs/server/kiln_server/document_api.py:44-51
Timestamp: 2025-06-18T08:22:58.510Z
Learning: leonardmq prefers to defer fixing blocking I/O in async handlers when: the operation is very fast (milliseconds), user-triggered rather than automated, has no concurrency concerns, and would require additional testing to fix properly. He acknowledges such issues as valid but makes pragmatic decisions about timing the fixes.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#402
File: libs/core/kiln_ai/adapters/embedding/litellm_embedding_adapter.py:0-0
Timestamp: 2025-07-14T03:43:07.283Z
Learning: leonardmq prefers to keep defensive validation checks even when they're technically redundant, viewing them as useful "quick sanity checks" that provide additional safety nets. He values defensive programming over strict DRY (Don't Repeat Yourself) principles when the redundant code serves as a safeguard.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#388
File: libs/core/kiln_ai/datamodel/test_extraction_chunk.py:49-74
Timestamp: 2025-07-03T05:13:02.873Z
Learning: leonardmq prefers automatic cleanup of temporary files in tests using `delete=True` in `tempfile.NamedTemporaryFile()` context managers, rather than manual cleanup with `delete=False` and explicit file removal. This is because automatic cleanup is simpler, safer, and avoids leaving temporary files if tests fail before cleanup.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#0
File: :0-0
Timestamp: 2025-06-27T06:45:06.591Z
Learning: leonardmq prefers unused imports to be flagged as actionable review comments rather than nitpick comments. Unused imports should be treated as concrete issues that need to be addressed, not just suggestions.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Desktop Apps (ubuntu-22.04)
- GitHub Check: Build Desktop Apps (macos-13)
- GitHub Check: Build Desktop Apps (ubuntu-22.04-arm)
- GitHub Check: Build Desktop Apps (windows-latest)
- GitHub Check: Build Desktop Apps (macos-latest)
🔇 Additional comments (3)
app/web_ui/src/routes/(app)/models/+page.svelte (3)
19-32: Well-structured TypeScript interfaces.The Provider interface comprehensively captures all the model capabilities and metadata. The boolean flags are clear and the naming follows good conventions.
258-281: Good use of toSorted for immutable sorting.Using
toSorted()instead ofsort()maintains immutability which is excellent for Svelte's reactivity system.
96-98: provider_name_from_id behavior is correct and covered by testsThe
provider_name_from_idfunction inapp/web_ui/src/lib/stores.tsreturns:
"Unknown"when passed an empty string- the matched
provider_namefrom theavailable_modelsstore for known providers- the raw
provider_idfor any unmatched IDUnit tests in
app/web_ui/src/lib/stores.test.tsalready verify these scenarios. No further changes are required here.
| async function fetchModelsFromRemoteConfig() { | ||
| try { | ||
| loading = true | ||
| error = null | ||
|
|
||
| const response = await fetch( | ||
| "https://remote-config.getkiln.ai/kiln_config.json", | ||
| ) | ||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch models: ${response.status}`) | ||
| } | ||
|
|
||
| const data: ConfigData = await response.json() | ||
| models = data.model_list | ||
|
|
||
| // Extract unique families and providers for filters | ||
| families = [...new Set(models.map((m) => m.family))].sort() | ||
| providers = [ | ||
| ...new Set(models.flatMap((m) => m.providers.map((p) => p.name))), | ||
| ].sort() | ||
|
|
||
| applyFilters() | ||
| } catch (err) { | ||
| error = createKilnError(err) | ||
| } finally { | ||
| loading = false | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add request timeout and improve error handling.
The fetch request lacks a timeout and could benefit from more specific error handling for different HTTP status codes.
async function fetchModelsFromRemoteConfig() {
try {
loading = true
error = null
- const response = await fetch(
- "https://remote-config.getkiln.ai/kiln_config.json",
- )
+ const controller = new AbortController()
+ const timeoutId = setTimeout(() => controller.abort(), 10000) // 10 second timeout
+
+ const response = await fetch(
+ "https://remote-config.getkiln.ai/kiln_config.json",
+ { signal: controller.signal }
+ )
+ clearTimeout(timeoutId)
+
if (!response.ok) {
- throw new Error(`Failed to fetch models: ${response.status}`)
+ throw new Error(`Failed to fetch models: ${response.status} ${response.statusText}`)
}📝 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.
| async function fetchModelsFromRemoteConfig() { | |
| try { | |
| loading = true | |
| error = null | |
| const response = await fetch( | |
| "https://remote-config.getkiln.ai/kiln_config.json", | |
| ) | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch models: ${response.status}`) | |
| } | |
| const data: ConfigData = await response.json() | |
| models = data.model_list | |
| // Extract unique families and providers for filters | |
| families = [...new Set(models.map((m) => m.family))].sort() | |
| providers = [ | |
| ...new Set(models.flatMap((m) => m.providers.map((p) => p.name))), | |
| ].sort() | |
| applyFilters() | |
| } catch (err) { | |
| error = createKilnError(err) | |
| } finally { | |
| loading = false | |
| } | |
| } | |
| async function fetchModelsFromRemoteConfig() { | |
| try { | |
| loading = true | |
| error = null | |
| const controller = new AbortController() | |
| const timeoutId = setTimeout(() => controller.abort(), 10000) // 10 second timeout | |
| const response = await fetch( | |
| "https://remote-config.getkiln.ai/kiln_config.json", | |
| { signal: controller.signal } | |
| ) | |
| clearTimeout(timeoutId) | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch models: ${response.status} ${response.statusText}`) | |
| } | |
| const data: ConfigData = await response.json() | |
| models = data.model_list | |
| // Extract unique families and providers for filters | |
| families = [...new Set(models.map((m) => m.family))].sort() | |
| providers = [ | |
| ...new Set(models.flatMap((m) => m.providers.map((p) => p.name))), | |
| ].sort() | |
| applyFilters() | |
| } catch (err) { | |
| error = createKilnError(err) | |
| } finally { | |
| loading = false | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/web_ui/src/routes/(app)/models/+page.svelte between lines 157 and 184,
the fetchModelsFromRemoteConfig function lacks a request timeout and has generic
error handling. To fix this, implement a timeout mechanism for the fetch request
using AbortController to abort the request if it exceeds a set duration.
Additionally, enhance error handling by checking for specific HTTP status codes
(e.g., 404, 500) and provide more descriptive error messages accordingly before
throwing errors. This will improve reliability and clarity of error reporting.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/web_ui/src/routes/(app)/models/+page.svelte (1)
157-184: Previously flagged timeout issue remains unaddressed.The fetch request still lacks the timeout mechanism that was suggested in previous reviews. This could lead to hanging requests if the remote config endpoint is slow or unresponsive.
🧹 Nitpick comments (1)
app/web_ui/src/routes/(app)/models/+page.svelte (1)
721-721: Consider addressing the accessibility ignore rather than suppressing it.The
svelte-ignore a11y-no-noninteractive-tabindexsuggests there's still an accessibility concern that's being suppressed rather than properly resolved. Consider if the tabindex logic can be restructured to avoid this warning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/web_ui/src/routes/(app)/models/+page.svelte(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/web_ui/src/**/*.{js,jsx,ts,tsx,svelte}
📄 CodeRabbit Inference Engine (.cursor/rules/project.mdc)
Our web UI component framework is DaisyUI v4 (based on Tailwind). Be sure to use v4 docs and features, not v5
Files:
app/web_ui/src/routes/(app)/models/+page.svelte
🧠 Learnings (1)
📓 Common learnings
Learnt from: leonardmq
PR: Kiln-AI/Kiln#390
File: libs/core/kiln_ai/adapters/provider_tools.py:494-499
Timestamp: 2025-07-23T08:58:45.769Z
Learning: leonardmq prefers to keep tightly coupled implementation details (like API versions) hardcoded when they are not user-facing and could break other modules if changed. The shared Config class is reserved for user-facing customizable values, not internal implementation details.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#341
File: libs/server/kiln_server/document_api.py:44-51
Timestamp: 2025-06-18T08:22:58.510Z
Learning: leonardmq prefers to defer fixing blocking I/O in async handlers when: the operation is very fast (milliseconds), user-triggered rather than automated, has no concurrency concerns, and would require additional testing to fix properly. He acknowledges such issues as valid but makes pragmatic decisions about timing the fixes.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#402
File: libs/core/kiln_ai/adapters/embedding/litellm_embedding_adapter.py:0-0
Timestamp: 2025-07-14T03:43:07.283Z
Learning: leonardmq prefers to keep defensive validation checks even when they're technically redundant, viewing them as useful "quick sanity checks" that provide additional safety nets. He values defensive programming over strict DRY (Don't Repeat Yourself) principles when the redundant code serves as a safeguard.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#388
File: libs/core/kiln_ai/datamodel/test_extraction_chunk.py:49-74
Timestamp: 2025-07-03T05:13:02.873Z
Learning: leonardmq prefers automatic cleanup of temporary files in tests using `delete=True` in `tempfile.NamedTemporaryFile()` context managers, rather than manual cleanup with `delete=False` and explicit file removal. This is because automatic cleanup is simpler, safer, and avoids leaving temporary files if tests fail before cleanup.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#0
File: :0-0
Timestamp: 2025-06-27T06:45:06.591Z
Learning: leonardmq prefers unused imports to be flagged as actionable review comments rather than nitpick comments. Unused imports should be treated as concrete issues that need to be addressed, not just suggestions.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Desktop Apps (ubuntu-22.04)
- GitHub Check: Build Desktop Apps (macos-latest)
- GitHub Check: Build Desktop Apps (ubuntu-22.04-arm)
- GitHub Check: Build Desktop Apps (macos-13)
- GitHub Check: Build Desktop Apps (windows-latest)
🔇 Additional comments (7)
app/web_ui/src/routes/(app)/models/+page.svelte (7)
1-44: Well-structured imports and TypeScript interfaces.The imports are appropriate and the TypeScript interfaces provide comprehensive typing for model and provider data. The Provider interface captures all necessary capability flags and metadata.
45-141: Comprehensive state management with proper reactive declarations.The state variables are well-organized and the reactive declarations properly transform data for UI components. The activeFilters computation provides a clean way to display filter badges.
142-318: Robust filtering and sorting implementation.The filtering logic comprehensively covers text search, family, provider, and capability filtering. The use of
toSorted()prevents array mutation, and the three-stage sorting behavior (ascending → descending → clear) provides excellent UX.
319-394: Excellent capability badge implementation and reactive updates.The
getCapabilityBadgesfunction provides descriptive tooltips as requested in PR feedback. The reactive watchers properly trigger filtering when dependencies change, and theonMountsequence loads data in the correct order.
395-403: Clean provider connection handling.The provider click handler properly checks connection status and navigates to settings for disconnected providers, implementing the UX flow described in the PR comments.
406-567: Well-structured search and filter interface.The template provides excellent UX with loading/error states, properly labeled search input with icon, consistent filter dropdowns, and clear active filter badges with removal functionality.
568-802: Comprehensive model grid with good UX enhancements.The model grid implementation provides excellent visual feedback for provider connection status, proper responsive layout, and comprehensive tooltips. The accessibility improvements from previous reviews have been largely addressed.
Can we test if the same issue occurs? I was assuming we were working with untyped JSON data, and it would just fall through to ID if it can't find name. We're not working with enums/typing here if we're calling remote, so I bet it mostly just works (and might need a small tweak). Otherwise, sounds good. |
|
Let's also add an AppPage button: "Manage Providers" to |
|
Right now, we are (probably) dealing with untyped strings on the frontend, but we cannot rely on that - we expose Kiln/app/web_ui/src/lib/types.ts Line 41 in 96292cb That becomes a problem if data from this page integrates with the broader frontend (e.g. check if the provider is connected or not, etc.). Typescript types are not enforced at runtime so this would not be immediately obvious that things are breaking (unlike with Python enums), but any code assuming Probably safer to just serve the config via an endpoint on our API to filter out the ones that backend also does not support. Later we could also add a flag to indicate to the client that the config it is getting excludes some models that are not compatible and that they should try to update Kiln. EDIT:
|
📊 Coverage ReportOverall Coverage: 92% Diff: origin/main...HEADNo lines with coverage information in this diff.
|
…badges only for suggested / recommended
leonardmq
force-pushed
the
leonard/model-library
branch
from
a3d74b6 to
773b996
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/web_ui/src/routes/(app)/models/+page.svelte (1)
177-288: Comprehensive filtering and sorting implementation.The filtering logic effectively handles text search across multiple fields and capability-based filtering. The sorting implementation using
toSortedmaintains immutability, and the three-state sort toggle (asc/desc/none) provides good UX.Consider adding consistent optional chaining for all fields in the text search to prevent potential errors if the data structure changes:
model.family?.toLowerCase().includes(searchLower) || model.friendly_name?.toLowerCase().includes(searchLower) || model.name?.toLowerCase().includes(searchLower) || model.providers.some((p) => p.model_id?.toLowerCase().includes(searchLower), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/desktop/studio_server/test_provider_api.py(4 hunks)app/web_ui/src/lib/api_schema.d.ts(1 hunks)app/web_ui/src/routes/(app)/models/+page.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/desktop/studio_server/test_provider_api.py
- app/web_ui/src/lib/api_schema.d.ts
🧰 Additional context used
📓 Path-based instructions (1)
app/web_ui/src/**/*.{js,jsx,ts,tsx,svelte}
📄 CodeRabbit Inference Engine (.cursor/rules/project.mdc)
Our web UI component framework is DaisyUI v4 (based on Tailwind). Be sure to use v4 docs and features, not v5
Files:
app/web_ui/src/routes/(app)/models/+page.svelte
🧠 Learnings (2)
📓 Common learnings
Learnt from: leonardmq
PR: Kiln-AI/Kiln#390
File: libs/core/kiln_ai/adapters/provider_tools.py:494-499
Timestamp: 2025-07-23T08:58:45.769Z
Learning: leonardmq prefers to keep tightly coupled implementation details (like API versions) hardcoded when they are not user-facing and could break other modules if changed. The shared Config class is reserved for user-facing customizable values, not internal implementation details.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#341
File: libs/server/kiln_server/document_api.py:44-51
Timestamp: 2025-06-18T08:22:58.510Z
Learning: leonardmq prefers to defer fixing blocking I/O in async handlers when: the operation is very fast (milliseconds), user-triggered rather than automated, has no concurrency concerns, and would require additional testing to fix properly. He acknowledges such issues as valid but makes pragmatic decisions about timing the fixes.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#402
File: libs/core/kiln_ai/adapters/embedding/litellm_embedding_adapter.py:0-0
Timestamp: 2025-07-14T03:43:07.283Z
Learning: leonardmq prefers to keep defensive validation checks even when they're technically redundant, viewing them as useful "quick sanity checks" that provide additional safety nets. He values defensive programming over strict DRY (Don't Repeat Yourself) principles when the redundant code serves as a safeguard.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#388
File: libs/core/kiln_ai/datamodel/test_extraction_chunk.py:49-74
Timestamp: 2025-07-03T05:13:02.873Z
Learning: leonardmq prefers automatic cleanup of temporary files in tests using `delete=True` in `tempfile.NamedTemporaryFile()` context managers, rather than manual cleanup with `delete=False` and explicit file removal. This is because automatic cleanup is simpler, safer, and avoids leaving temporary files if tests fail before cleanup.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#0
File: :0-0
Timestamp: 2025-06-27T06:45:06.591Z
Learning: leonardmq prefers unused imports to be flagged as actionable review comments rather than nitpick comments. Unused imports should be treated as concrete issues that need to be addressed, not just suggestions.
📚 Learning: in the kiln web ui's project import functionality (edit_project.svelte), when the file selector api ...
Learnt from: scosman
PR: Kiln-AI/Kiln#464
File: app/web_ui/src/routes/(fullscreen)/setup/(setup)/create_project/edit_project.svelte:116-138
Timestamp: 2025-07-30T02:50:29.383Z
Learning: In the Kiln web UI's project import functionality (edit_project.svelte), when the file selector API fails, the preferred approach is to use alert() as a brief notification and set select_file_unavailable = true to gracefully fall back to manual path entry, rather than using the form's error handling system. This maintains a smooth user experience with automatic fallback rather than showing an error state.
Applied to files:
app/web_ui/src/routes/(app)/models/+page.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Desktop Apps (ubuntu-22.04)
- GitHub Check: Build Desktop Apps (windows-latest)
- GitHub Check: Build Desktop Apps (macos-latest)
- GitHub Check: Build Desktop Apps (macos-13)
- GitHub Check: Build Desktop Apps (ubuntu-22.04-arm)
🔇 Additional comments (7)
app/web_ui/src/routes/(app)/models/+page.svelte (7)
14-39: Well-structured TypeScript interfaces.The interfaces are comprehensive and properly typed, using imported types like
ModelProviderNamefor type safety. The Provider interface effectively captures all model capabilities and metadata needed for the UI.
41-131: Excellent reactive state management.The state variables are properly typed and the reactive statements effectively transform data for the UI components. The
activeFiltersreactive statement elegantly builds filter badges from the current filter state.
133-145: Simple and effective filter removal logic.The switch statement cleanly handles resetting individual filters based on their type.
147-174: Data fetching logic is functional.The function properly manages loading and error states, extracts filter options from the fetched data, and handles JSON parsing. The basic error handling and state management are appropriate for this use case.
353-401: Robust provider connection detection.The logic properly handles different provider types and connection states, including the special case of finetune-only models. The ollama-specific handling and helper functions for URLs and tooltips provide a good user experience for different connection states.
404-806: Comprehensive and user-friendly UI implementation.The template effectively handles all states (loading, error, empty, populated) with appropriate feedback. The responsive grid layout, search interface, filter badges, and provider connection indicators create an intuitive user experience. The accessibility considerations and error handling demonstrate good UX practices.
338-351: Proper reactive state management and initialization.The reactive statements correctly trigger filtering when state changes, and the onMount sequence logically loads local models before fetching remote configuration to ensure accurate provider connection status detection.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/web_ui/src/routes/(app)/models/+page.svelte (1)
134-136: Add request timeout and improve error handling.The fetch request lacks a timeout and could benefit from more specific error handling for different HTTP status codes.
This matches a previous review comment that suggested implementing a timeout mechanism using AbortController and enhancing error handling with more descriptive messages.
🧹 Nitpick comments (5)
app/web_ui/src/routes/(app)/models/+page.svelte (5)
158-203: Optimize filter performance with memoization.The filtering logic runs on every reactive update, which could impact performance with large model lists. The search operation using
includes()on every model/provider combination could be expensive.Consider memoizing the filtered results or using debounced search:
+ import { debounce } from 'lodash-es' + + // Debounced search to avoid excessive filtering + const debouncedApplyFilters = debounce(applyFilters, 150) + - $: searchQuery, selectedProvider, selectedCapability, models, applyFilters() + $: searchQuery, selectedProvider, selectedCapability, models, debouncedApplyFilters()
342-354: Simplify URL generation logic.The
model_not_connected_hreffunction has inconsistent return types and could be simplified for better maintainability.function model_not_connected_href( provider_id: string, model_id: string | null, ) { - if (provider_id === "ollama" && ollama_is_connected && model_id) { - return get_ollama_model_href(model_id) - } - return "/settings/providers" + return provider_id === "ollama" && ollama_is_connected && model_id + ? `https://ollama.com/library/${model_id}` + : "/settings/providers" }
445-452: Improve search input accessibility.The search input should have better keyboard navigation and screen reader support.
<input id="search" type="text" bind:value={searchQuery} placeholder="Search by name, model ID..." + autocomplete="off" + role="searchbox" + aria-label="Search models by name or model ID" class="block w-full pl-12 pr-4 py-3 border border-gray-300 rounded-lg leading-5 bg-white placeholder-gray-400 focus:outline-none focus:ring-2 focus:ring-indigo-500 focus:border-transparent transition-all" />
627-628: Consider performance impact of large model grids.With potentially hundreds of models, rendering all model cards at once could impact performance. Consider implementing virtual scrolling or pagination for better UX.
For large datasets, consider adding pagination or virtual scrolling:
- Use intersection observer for lazy loading
- Implement pagination with page size limits
- Add "Load more" functionality for progressive loading
669-673: Consolidate image error handling.The image error handling logic is duplicated. Consider extracting it to a reusable function or directive.
+ function handleImageError(event: Event) { + const target = event.target as HTMLImageElement + if (target) { + target.style.display = "none" + } + } - on:error={(e) => { - if (e.target instanceof HTMLImageElement) { - e.target.style.display = "none" - } - }} + on:error={handleImageError}Also applies to: 700-704
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/web_ui/src/routes/(app)/models/+page.svelte(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/web_ui/src/**/*.{js,jsx,ts,tsx,svelte}
📄 CodeRabbit Inference Engine (.cursor/rules/project.mdc)
Our web UI component framework is DaisyUI v4 (based on Tailwind). Be sure to use v4 docs and features, not v5
Files:
app/web_ui/src/routes/(app)/models/+page.svelte
🧠 Learnings (2)
📓 Common learnings
Learnt from: leonardmq
PR: Kiln-AI/Kiln#390
File: libs/core/kiln_ai/adapters/provider_tools.py:494-499
Timestamp: 2025-07-23T08:58:45.769Z
Learning: leonardmq prefers to keep tightly coupled implementation details (like API versions) hardcoded when they are not user-facing and could break other modules if changed. The shared Config class is reserved for user-facing customizable values, not internal implementation details.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#341
File: libs/server/kiln_server/document_api.py:44-51
Timestamp: 2025-06-18T08:22:58.510Z
Learning: leonardmq prefers to defer fixing blocking I/O in async handlers when: the operation is very fast (milliseconds), user-triggered rather than automated, has no concurrency concerns, and would require additional testing to fix properly. He acknowledges such issues as valid but makes pragmatic decisions about timing the fixes.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#402
File: libs/core/kiln_ai/adapters/embedding/litellm_embedding_adapter.py:0-0
Timestamp: 2025-07-14T03:43:07.283Z
Learning: leonardmq prefers to keep defensive validation checks even when they're technically redundant, viewing them as useful "quick sanity checks" that provide additional safety nets. He values defensive programming over strict DRY (Don't Repeat Yourself) principles when the redundant code serves as a safeguard.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#388
File: libs/core/kiln_ai/datamodel/test_extraction_chunk.py:49-74
Timestamp: 2025-07-03T05:13:02.873Z
Learning: leonardmq prefers automatic cleanup of temporary files in tests using `delete=True` in `tempfile.NamedTemporaryFile()` context managers, rather than manual cleanup with `delete=False` and explicit file removal. This is because automatic cleanup is simpler, safer, and avoids leaving temporary files if tests fail before cleanup.
Learnt from: leonardmq
PR: Kiln-AI/Kiln#0
File: :0-0
Timestamp: 2025-06-27T06:45:06.591Z
Learning: leonardmq prefers unused imports to be flagged as actionable review comments rather than nitpick comments. Unused imports should be treated as concrete issues that need to be addressed, not just suggestions.
📚 Learning: in the kiln web ui's project import functionality (edit_project.svelte), when the file selector api ...
Learnt from: scosman
PR: Kiln-AI/Kiln#464
File: app/web_ui/src/routes/(fullscreen)/setup/(setup)/create_project/edit_project.svelte:116-138
Timestamp: 2025-07-30T02:50:29.383Z
Learning: In the Kiln web UI's project import functionality (edit_project.svelte), when the file selector API fails, the preferred approach is to use alert() as a brief notification and set select_file_unavailable = true to gracefully fall back to manual path entry, rather than using the form's error handling system. This maintains a smooth user experience with automatic fallback rather than showing an error state.
Applied to files:
app/web_ui/src/routes/(app)/models/+page.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Desktop Apps (windows-latest)
- GitHub Check: Build Desktop Apps (macos-latest)
- GitHub Check: Build Desktop Apps (macos-13)
- GitHub Check: Build Desktop Apps (ubuntu-22.04-arm)
- GitHub Check: Build Desktop Apps (ubuntu-22.04)
🔇 Additional comments (4)
app/web_ui/src/routes/(app)/models/+page.svelte (4)
1-11: LGTM: Well-structured imports and lifecycle management.The imports are well-organized and the onMount lifecycle properly loads both available models and remote config data in the correct sequence.
Also applies to: 308-311
14-39: LGTM: Comprehensive TypeScript interfaces.The interface definitions are well-structured and provide good type safety for the model data structure. The Provider interface covers all necessary attributes for the UI features.
264-300: LGTM: Well-implemented capability badge system.The capability badge generation logic is clean and provides useful tooltips that align with the PR objectives for improved user guidance.
69-79: Ignore the runtime‐error concern for provider_name_from_idThe
provider_name_from_idimplementation in app/web_ui/src/lib/stores.ts already guards unknown or empty IDs:• Returns
"Unknown"whenprovider_idis falsy
• Looks upavailable_modelsand falls back to returningprovider_idif not foundThis behavior is covered by stores.test.ts and prevents any runtime exception.
Likely an incorrect or invalid review comment.
|
Reverted the previous commit that tried to check providers connected the same way as Changes:
|
leonardmq
force-pushed
the
leonard/model-library
branch
from
a3e909b to
eb99c4f
Compare
1Everything seems to be getting the special star. We're treating "supports_data_gen" as if it's "suggested_for_data_gen". Maybe the comment wasn't clear:
2Same goes for other badges like uncensored. 3Log probs is missing a badge 4Can we make the "Evaluations" item in dropdown "Suggested For Evals". Technically all models support evals, we want to make it clear this is a "suggestion" filter. We can keep "Evals [star]" in badge because the other string is too long 😀 5I'll maybe to this separately, but we're not listing most of our Fine-tune models. They come direct from the Fireworks API, and don't have nice names so I don't just want to drop them in. I might add a modal or something. |
|
That was partially intentional; I kept the badges only for the starred traits (there was a bug where Changes:
Regarding Also filed this Issue: #475 - will address separately as separate issue Feel free to make changes for the colors / strings if you want to. |
leonardmq
force-pushed
the
leonard/model-library
branch
from
76c4a10 to
dd51128
Compare
| supports_finetuning: | ||
| "Supports finetuning, to train the model on your own use cases.", | ||
| supports_data_gen: "Supports Synthetic Data Generation.", | ||
| supports_structured_output: |
There was a problem hiding this comment.
I find this message confusing - not sure what preferring the tool-calling method means in this context
What does this PR do?
Add a new (very much open to changes) page to list and search through available models:
Why
Model discovery currently happens on external providers like OpenRouter. Users then need to manually map those models back to Kiln, where the names differ (OpenRouter gives a slug; and it is hard to know for sure which model it is in Kiln in some cases where the model is a narrow variant).
We can build on top of this later to let users click a model to go directly into the
RunUI with the right model and provider selected or other such things.Kapture.2025-07-16.at.19.04.07.mp4
Summary by CodeRabbit
New Features
Bug Fixes
Chores