From 5d9c3ab46291056f382307a8b68fd21b1d2f2344 Mon Sep 17 00:00:00 2001 From: lugnicca Date: Wed, 20 Aug 2025 13:35:02 +0200 Subject: [PATCH 01/15] feat: add model selector with fetching from /v1/models endpoints when adding models --- web-app/src/containers/ModelCombobox.tsx | 367 ++++++++++++++++++++ web-app/src/containers/dialogs/AddModel.tsx | 20 +- web-app/src/hooks/useProviderModels.ts | 89 +++++ web-app/src/locales/en/common.json | 2 + web-app/src/services/providers.ts | 37 +- 5 files changed, 504 insertions(+), 11 deletions(-) create mode 100644 web-app/src/containers/ModelCombobox.tsx create mode 100644 web-app/src/hooks/useProviderModels.ts diff --git a/web-app/src/containers/ModelCombobox.tsx b/web-app/src/containers/ModelCombobox.tsx new file mode 100644 index 000000000..34c95bf6d --- /dev/null +++ b/web-app/src/containers/ModelCombobox.tsx @@ -0,0 +1,367 @@ +import { useState, useMemo, useRef, useEffect, useCallback } from 'react' +import { createPortal } from 'react-dom' +import { Input } from '@/components/ui/input' +import { Button } from '@/components/ui/button' +import { IconChevronDown, IconLoader2, IconRefresh } from '@tabler/icons-react' +import { cn } from '@/lib/utils' +import { useTranslation } from '@/i18n/react-i18next-compat' + +type ModelComboboxProps = { + value: string + onChange: (value: string) => void + models: string[] + loading?: boolean + error?: string | null + onRefresh?: () => void + placeholder?: string + disabled?: boolean + className?: string +} + +export function ModelCombobox({ + value, + onChange, + models, + loading = false, + error = null, + onRefresh, + placeholder = 'Type or select a model...', + disabled = false, + className, +}: ModelComboboxProps) { + const [open, setOpen] = useState(false) + const [inputValue, setInputValue] = useState(value) + const [dropdownPosition, setDropdownPosition] = useState({ top: 0, left: 0, width: 0 }) + const [highlightedIndex, setHighlightedIndex] = useState(-1) + const inputRef = useRef(null) + const containerRef = useRef(null) + const dropdownRef = useRef(null) + const keyRepeatTimeoutRef = useRef(null) + const { t } = useTranslation() + + // Sync input value with prop value + useEffect(() => { + setInputValue(value) + }, [value]) + + // Simple position calculation + const updateDropdownPosition = useCallback(() => { + if (containerRef.current) { + const rect = containerRef.current.getBoundingClientRect() + setDropdownPosition({ + top: rect.bottom + window.scrollY + 4, + left: rect.left + window.scrollX, + width: rect.width, + }) + } + }, []) + + // Update position when opening + useEffect(() => { + if (open) { + // Use requestAnimationFrame to ensure DOM is ready + requestAnimationFrame(() => { + updateDropdownPosition() + }) + } + }, [open, updateDropdownPosition]) + + // Close dropdown when clicking outside + useEffect(() => { + if (!open) return + + const handleClickOutside = (event: Event) => { + const target = event.target as Node + // Check if click is inside our container or dropdown + const isInsideContainer = containerRef.current?.contains(target) + const isInsideDropdown = dropdownRef.current?.contains(target) + + // Only close if click is outside both container and dropdown + if (!isInsideContainer && !isInsideDropdown) { + setOpen(false) + setDropdownPosition({ top: 0, left: 0, width: 0 }) + setHighlightedIndex(-1) + } + } + + // Use multiple event types to ensure we catch all interactions + const events = ['mousedown', 'touchstart'] + events.forEach(eventType => { + document.addEventListener(eventType, handleClickOutside, { capture: true, passive: true }) + }) + + return () => { + events.forEach(eventType => { + document.removeEventListener(eventType, handleClickOutside, { capture: true }) + }) + } + }, [open]) + + // Cleanup: close dropdown when component unmounts + useEffect(() => { + const timeoutId = keyRepeatTimeoutRef.current + return () => { + setOpen(false) + setDropdownPosition({ top: 0, left: 0, width: 0 }) + setHighlightedIndex(-1) + if (timeoutId) { + clearTimeout(timeoutId) + } + } + }, []) + + // Filter models based on input + const filteredModels = useMemo(() => { + if (!inputValue.trim()) return models + + return models.filter((model) => + model.toLowerCase().includes(inputValue.toLowerCase()) + ) + }, [models, inputValue]) + + // Reset highlighted index when filtered models change + useEffect(() => { + setHighlightedIndex(-1) + }, [filteredModels]) + + // Scroll to highlighted item with debouncing to handle key repeat + useEffect(() => { + if (highlightedIndex >= 0 && dropdownRef.current && !loading && !error) { + // Use requestAnimationFrame to ensure smooth scrolling and avoid conflicts + requestAnimationFrame(() => { + // Find all model elements (they have the data-model attribute) + const modelElements = dropdownRef.current?.querySelectorAll('[data-model]') + const highlightedElement = modelElements?.[highlightedIndex] as HTMLElement + if (highlightedElement) { + highlightedElement.scrollIntoView({ + block: 'nearest', + behavior: 'auto' + }) + } + }) + } + }, [highlightedIndex, error, loading]) + + // Handle input change + const handleInputChange = (newValue: string) => { + setInputValue(newValue) + onChange(newValue) + + // Only open dropdown if user is actively typing and there are models + if (newValue.trim() && models.length > 0) { + setOpen(true) + } else { + // Don't auto-open on empty input - wait for user interaction + setOpen(false) + } + } + + // Handle keyboard navigation + const handleKeyDown = (e: React.KeyboardEvent) => { + if (!open && (e.key === 'ArrowDown' || e.key === 'ArrowUp')) { + // Open dropdown on arrow keys if there are models + if (models.length > 0) { + e.preventDefault() + setOpen(true) + setHighlightedIndex(0) + } + return + } + + if (!open) return + + switch (e.key) { + case 'ArrowDown': + e.preventDefault() + if (keyRepeatTimeoutRef.current) { + clearTimeout(keyRepeatTimeoutRef.current) + } + setHighlightedIndex((prev) => + filteredModels.length === 0 ? 0 : (prev < filteredModels.length - 1 ? prev + 1 : 0) + ) + break + case 'ArrowUp': + e.preventDefault() + if (keyRepeatTimeoutRef.current) { + clearTimeout(keyRepeatTimeoutRef.current) + } + setHighlightedIndex((prev) => + filteredModels.length === 0 ? 0 : (prev > 0 ? prev - 1 : filteredModels.length - 1) + ) + break + case 'Enter': + e.preventDefault() + if (highlightedIndex >= 0 && highlightedIndex < filteredModels.length) { + handleModelSelect(filteredModels[highlightedIndex]) + } + break + case 'ArrowRight': + case 'ArrowLeft': + setOpen(false) + setHighlightedIndex(-1) + break + case 'PageUp': + setHighlightedIndex(0) + break + case 'PageDown': + setHighlightedIndex(filteredModels.length - 1) + break + } + } + + // Handle model selection from dropdown + const handleModelSelect = (model: string) => { + setInputValue(model) + onChange(model) + setOpen(false) + setDropdownPosition({ top: 0, left: 0, width: 0 }) + setHighlightedIndex(-1) + inputRef.current?.focus() + } + + return ( +
+
+ handleInputChange(e.target.value)} + onKeyDown={handleKeyDown} + onClick={() => { + // Open dropdown on click if models are available + if (models.length > 0) { + setOpen(true) + } + }} + placeholder={placeholder} + disabled={disabled} + className="pr-8" + /> + + {/* Dropdown trigger button */} + + + {/* Custom dropdown rendered as portal */} + {open && dropdownPosition.width > 0 && createPortal( +
{ + // Prevent interaction with underlying elements + e.stopPropagation() + }} + onClick={(e) => { + // Prevent click from bubbling up and closing modal + e.stopPropagation() + }} + onMouseDown={(e) => { + // Allow default behavior for scrolling and selection + e.stopPropagation() + }} + onWheel={(e) => { + // Allow wheel events for scrolling + e.stopPropagation() + }} + > + {/* Error state */} + {error && ( +
+
+ {t('common:failedToLoadModels')} + {onRefresh && ( + + )} +
+
{error}
+
+ )} + + {/* Loading state */} + {loading && ( +
+ + {t('common:loading')} +
+ )} + + {/* Models list */} + {!loading && !error && ( + <> + {filteredModels.length === 0 ? ( +
+ {inputValue.trim() ? ( + {t('common:noModelsFoundFor', { searchValue: inputValue })} + ) : ( + {t('common:noModels')} + )} +
+ ) : ( + <> + {/* Available models */} + {filteredModels.map((model, index) => ( +
{ + e.stopPropagation() + handleModelSelect(model) + }} + onMouseEnter={() => setHighlightedIndex(index)} + className={cn( + 'cursor-pointer px-3 py-2 hover:bg-main-view-fg/15 hover:shadow-sm transition-all duration-200 text-main-view-fg', + value === model && 'bg-main-view-fg/12 shadow-sm', + highlightedIndex === index && 'bg-main-view-fg/20 shadow-md' + )} + > + {model} +
+ ))} + + )} + + )} +
, + document.body + )} +
+
+ ) +} diff --git a/web-app/src/containers/dialogs/AddModel.tsx b/web-app/src/containers/dialogs/AddModel.tsx index 248600212..dc0eac244 100644 --- a/web-app/src/containers/dialogs/AddModel.tsx +++ b/web-app/src/containers/dialogs/AddModel.tsx @@ -8,8 +8,9 @@ import { DialogFooter, } from '@/components/ui/dialog' import { Button } from '@/components/ui/button' -import { Input } from '@/components/ui/input' import { useModelProvider } from '@/hooks/useModelProvider' +import { useProviderModels } from '@/hooks/useProviderModels' +import { ModelCombobox } from '@/containers/ModelCombobox' import { IconPlus } from '@tabler/icons-react' import { useState } from 'react' import { getProviderTitle } from '@/lib/utils' @@ -26,6 +27,11 @@ export const DialogAddModel = ({ provider, trigger }: DialogAddModelProps) => { const [modelId, setModelId] = useState('') const [open, setOpen] = useState(false) + // Fetch models from provider API (API key is optional) + const { models, loading, error, refetch } = useProviderModels( + provider.base_url ? provider : undefined + ) + // Handle form submission const handleSubmit = () => { if (!modelId.trim()) { @@ -72,7 +78,7 @@ export const DialogAddModel = ({ provider, trigger }: DialogAddModelProps) => { - {/* Model ID field - required */} + {/* Model selection field - required */}
- setModelId(e.target.value)} + onChange={setModelId} + models={models} + loading={loading} + error={error} + onRefresh={refetch} placeholder={t('providers:addModel.enterModelId')} - required />
diff --git a/web-app/src/hooks/useProviderModels.ts b/web-app/src/hooks/useProviderModels.ts new file mode 100644 index 000000000..5c984d1ca --- /dev/null +++ b/web-app/src/hooks/useProviderModels.ts @@ -0,0 +1,89 @@ +import { useState, useEffect, useCallback, useRef } from 'react' +import { fetchModelsFromProvider } from '@/services/providers' +import type { ModelProvider } from '@/types/providers' + +type UseProviderModelsState = { + models: string[] + loading: boolean + error: string | null + refetch: () => void +} + +const modelsCache = new Map() +const CACHE_DURATION = 5 * 60 * 1000 // 5 minutes + +export const useProviderModels = (provider?: ModelProvider): UseProviderModelsState => { + const [models, setModels] = useState([]) + const [loading, setLoading] = useState(false) + const [error, setError] = useState(null) + const prevProviderKey = useRef('') + + const fetchModels = useCallback(async () => { + if (!provider || !provider.base_url) { + // Clear models if provider is invalid (base_url is required, api_key is optional) + setModels([]) + setError(null) + setLoading(false) + return + } + + // Clear any previous state when starting a new fetch for a different provider + const currentProviderKey = `${provider.provider}-${provider.base_url}` + if (currentProviderKey !== prevProviderKey.current) { + setModels([]) + setError(null) + setLoading(false) + prevProviderKey.current = currentProviderKey + } + + const cacheKey = `${provider.provider}-${provider.base_url}` + const cached = modelsCache.get(cacheKey) + + // Check cache first + if (cached && Date.now() - cached.timestamp < CACHE_DURATION) { + setModels(cached.models) + return + } + + setLoading(true) + setError(null) + + try { + const fetchedModels = await fetchModelsFromProvider(provider) + const sortedModels = fetchedModels.sort((a, b) => a.localeCompare(b)) + + setModels(sortedModels) + + // Cache the results + modelsCache.set(cacheKey, { + models: sortedModels, + timestamp: Date.now(), + }) + } catch (err) { + const errorMessage = err instanceof Error ? err.message : 'Failed to fetch models' + setError(errorMessage) + console.error(`Error fetching models from ${provider.provider}:`, err) + } finally { + setLoading(false) + } + }, [provider]) + + const refetch = useCallback(() => { + if (provider) { + const cacheKey = `${provider.provider}-${provider.base_url}` + modelsCache.delete(cacheKey) + fetchModels() + } + }, [provider, fetchModels]) + + useEffect(() => { + fetchModels() + }, [fetchModels]) + + return { + models, + loading, + error, + refetch, + } +} diff --git a/web-app/src/locales/en/common.json b/web-app/src/locales/en/common.json index 46f2d5a8a..e5f5aa9f7 100644 --- a/web-app/src/locales/en/common.json +++ b/web-app/src/locales/en/common.json @@ -75,6 +75,8 @@ "selectAModel": "Select a model", "noToolsAvailable": "No tools available", "noModelsFoundFor": "No models found for \"{{searchValue}}\"", + "failedToLoadModels": "Failed to load models", + "noModels": "No models found", "customAvatar": "Custom avatar", "editAssistant": "Edit Assistant", "jan": "Jan", diff --git a/web-app/src/services/providers.ts b/web-app/src/services/providers.ts index e9f05fd09..748aed322 100644 --- a/web-app/src/services/providers.ts +++ b/web-app/src/services/providers.ts @@ -135,9 +135,24 @@ export const fetchModelsFromProvider = async ( }) if (!response.ok) { - throw new Error( - `Failed to fetch models: ${response.status} ${response.statusText}` - ) + // Provide more specific error messages based on status code + if (response.status === 401) { + throw new Error( + `Authentication failed: API key is required or invalid for ${provider.provider}` + ) + } else if (response.status === 403) { + throw new Error( + `Access forbidden: Check your API key permissions for ${provider.provider}` + ) + } else if (response.status === 404) { + throw new Error( + `Models endpoint not found for ${provider.provider}. Check the base URL configuration.` + ) + } else { + throw new Error( + `Failed to fetch models from ${provider.provider}: ${response.status} ${response.statusText}` + ) + } } const data = await response.json() @@ -167,14 +182,26 @@ export const fetchModelsFromProvider = async ( } catch (error) { console.error('Error fetching models from provider:', error) - // Provide helpful error message + if (error instanceof Error && ( + error.message.includes('Authentication failed') || + error.message.includes('Access forbidden') || + error.message.includes('Models endpoint not found') || + error.message.includes('Failed to fetch models from') + )) { + throw error + } + + // Provide helpful error message for network issues if (error instanceof Error && error.message.includes('fetch')) { throw new Error( `Cannot connect to ${provider.provider} at ${provider.base_url}. Please check that the service is running and accessible.` ) } - throw error + // Generic fallback + throw new Error( + `Unexpected error while fetching models from ${provider.provider}: ${error instanceof Error ? error.message : 'Unknown error'}` + ) } } From 3339629747a891c0cd20d46ab50b2e64310c4a16 Mon Sep 17 00:00:00 2001 From: lugnicca Date: Wed, 20 Aug 2025 16:22:21 +0200 Subject: [PATCH 02/15] test: add unit tests for ModelCombobox, useProviderModels and providers --- .../__tests__/ModelCombobox.test.tsx | 197 ++++++++++++++++++ .../hooks/__tests__/useProviderModels.test.ts | 105 ++++++++++ .../src/services/__tests__/providers.test.ts | 40 +++- 3 files changed, 340 insertions(+), 2 deletions(-) create mode 100644 web-app/src/containers/__tests__/ModelCombobox.test.tsx create mode 100644 web-app/src/hooks/__tests__/useProviderModels.test.ts diff --git a/web-app/src/containers/__tests__/ModelCombobox.test.tsx b/web-app/src/containers/__tests__/ModelCombobox.test.tsx new file mode 100644 index 000000000..88158f268 --- /dev/null +++ b/web-app/src/containers/__tests__/ModelCombobox.test.tsx @@ -0,0 +1,197 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { render, screen, fireEvent, waitFor, act } from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import '@testing-library/jest-dom/vitest' +import { ModelCombobox } from '../ModelCombobox' +import React from 'react' + +describe('ModelCombobox', () => { + const defaultProps = { + value: '', + onChange: vi.fn(), + models: ['gpt-3.5-turbo', 'gpt-4', 'claude-3-haiku'], + } + + const mockUser = userEvent.setup() + + beforeEach(() => { + vi.clearAllMocks() + }) + + afterEach(() => { + vi.restoreAllMocks() + }) + + describe('Basic Rendering', () => { + it('should render input field with placeholder', () => { + render() + + const input = screen.getByRole('textbox') + expect(input).toBeInTheDocument() + expect(input).toHaveAttribute('placeholder', 'Type or select a model...') + }) + + it('should render custom placeholder', () => { + render() + + const input = screen.getByRole('textbox') + expect(input).toHaveAttribute('placeholder', 'Choose a model') + }) + + it('should render dropdown trigger button', () => { + render() + const button = screen.getByRole('button') + expect(button).toBeInTheDocument() + }) + + it('should display current value in input', () => { + render() + + const input = screen.getByDisplayValue('gpt-4') + expect(input).toBeInTheDocument() + }) + + it('should apply custom className', () => { + const { container } = render( + + ) + + const wrapper = container.firstChild as HTMLElement + expect(wrapper).toHaveClass('custom-class') + }) + }) + + describe('Disabled State', () => { + it('should disable input when disabled prop is true', () => { + render() + + const input = screen.getByRole('textbox') + const button = screen.getByRole('button') + + expect(input).toBeDisabled() + expect(button).toBeDisabled() + }) + + it('should not open dropdown when disabled', async () => { + render() + + const input = screen.getByRole('textbox') + await mockUser.click(input) + + expect(screen.queryByTestId('dropdown')).not.toBeInTheDocument() + }) + }) + + describe('Loading State', () => { + it('should show loading spinner in trigger button', () => { + render() + + const button = screen.getByRole('button') + const spinner = button.querySelector('.animate-spin') + expect(spinner).toBeInTheDocument() + }) + + it('should show loading spinner when loading prop is true', () => { + render() + + const spinner = screen.getByRole('button').querySelector('.animate-spin') + expect(spinner).toBeInTheDocument() + }) + }) + + describe('Input Interactions', () => { + it('should call onChange when typing', async () => { + const mockOnChange = vi.fn() + render() + + const input = screen.getByRole('textbox') + await mockUser.type(input, 'g') + + expect(mockOnChange).toHaveBeenCalledWith('g') + }) + + it('should update input value when typing', async () => { + const mockOnChange = vi.fn() + render() + + const input = screen.getByRole('textbox') + await mockUser.type(input, 'test') + + expect(input).toHaveValue('test') + }) + + it('should handle input focus', async () => { + render() + + const input = screen.getByRole('textbox') + await mockUser.click(input) + + expect(input).toHaveFocus() + }) + }) + + describe('Props Validation', () => { + it('should render with empty models array', () => { + render() + + const input = screen.getByRole('textbox') + expect(input).toBeInTheDocument() + }) + + it('should render with models array', () => { + render() + + const input = screen.getByRole('textbox') + expect(input).toBeInTheDocument() + }) + + it('should render with all props', () => { + render( + + ) + + const input = screen.getByRole('textbox') + expect(input).toBeInTheDocument() + expect(input).toBeDisabled() + }) + }) + + describe('Component Lifecycle', () => { + it('should handle mount and unmount without errors', () => { + const { unmount } = render() + + expect(screen.getByRole('textbox')).toBeInTheDocument() + + unmount() + + expect(screen.queryByRole('textbox')).not.toBeInTheDocument() + }) + + it('should handle props changes', () => { + const { rerender } = render() + + expect(screen.getByDisplayValue('')).toBeInTheDocument() + + rerender() + + expect(screen.getByDisplayValue('gpt-4')).toBeInTheDocument() + }) + + it('should handle models array changes', () => { + const { rerender } = render() + + expect(screen.getByRole('textbox')).toBeInTheDocument() + + rerender() + + expect(screen.getByRole('textbox')).toBeInTheDocument() + }) + }) +}) diff --git a/web-app/src/hooks/__tests__/useProviderModels.test.ts b/web-app/src/hooks/__tests__/useProviderModels.test.ts new file mode 100644 index 000000000..3d107b9f8 --- /dev/null +++ b/web-app/src/hooks/__tests__/useProviderModels.test.ts @@ -0,0 +1,105 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest' +import { renderHook, waitFor } from '@testing-library/react' +import { useProviderModels } from '../useProviderModels' + +// Mock the providers service +vi.mock('@/services/providers', () => ({ + fetchModelsFromProvider: vi.fn(), +})) + +import { fetchModelsFromProvider } from '@/services/providers' +const mockFetchModelsFromProvider = vi.mocked(fetchModelsFromProvider) + +// Mock ModelProvider type +type MockModelProvider = { + active: boolean + provider: string + base_url?: string + api_key?: string + settings: any[] + models: any[] +} + +describe('useProviderModels', () => { + const mockProvider: MockModelProvider = { + active: true, + provider: 'openai', + base_url: 'https://api.openai.com/v1', + api_key: 'test-api-key', + settings: [], + models: [], + } + + const mockModels = ['gpt-4', 'gpt-3.5-turbo', 'gpt-4-turbo'] + + beforeEach(() => { + vi.clearAllMocks() + // Reset the cache by clearing any previous state + mockFetchModelsFromProvider.mockClear() + }) + + it('should initialize with empty state', () => { + const { result } = renderHook(() => useProviderModels()) + + expect(result.current.models).toEqual([]) + expect(result.current.loading).toBe(false) + expect(result.current.error).toBe(null) + expect(typeof result.current.refetch).toBe('function') + }) + + it('should not fetch models when provider is undefined', () => { + renderHook(() => useProviderModels(undefined)) + expect(mockFetchModelsFromProvider).not.toHaveBeenCalled() + }) + + it('should not fetch models when provider has no base_url', () => { + const providerWithoutUrl = { ...mockProvider, base_url: undefined } + renderHook(() => useProviderModels(providerWithoutUrl)) + expect(mockFetchModelsFromProvider).not.toHaveBeenCalled() + }) + + it('should fetch and sort models', async () => { + mockFetchModelsFromProvider.mockResolvedValueOnce(mockModels) + + const { result } = renderHook(() => useProviderModels(mockProvider)) + + await waitFor(() => { + expect(result.current.loading).toBe(false) + }) + + // Should be sorted alphabetically + expect(result.current.models).toEqual(['gpt-3.5-turbo', 'gpt-4', 'gpt-4-turbo']) + expect(result.current.error).toBe(null) + expect(mockFetchModelsFromProvider).toHaveBeenCalledWith(mockProvider) + }) + + it('should clear models when switching to invalid provider', async () => { + mockFetchModelsFromProvider.mockResolvedValueOnce(mockModels) + + const { result, rerender } = renderHook( + ({ provider }) => useProviderModels(provider), + { initialProps: { provider: mockProvider } } + ) + + await waitFor(() => { + expect(result.current.loading).toBe(false) + }) + + expect(result.current.models).toEqual(['gpt-3.5-turbo', 'gpt-4', 'gpt-4-turbo']) + + // Switch to invalid provider + rerender({ provider: { ...mockProvider, base_url: undefined } }) + + expect(result.current.models).toEqual([]) + expect(result.current.error).toBe(null) + expect(result.current.loading).toBe(false) + }) + + it('should not refetch when provider is undefined', () => { + const { result } = renderHook(() => useProviderModels(undefined)) + + result.current.refetch() + + expect(mockFetchModelsFromProvider).not.toHaveBeenCalled() + }) +}) \ No newline at end of file diff --git a/web-app/src/services/__tests__/providers.test.ts b/web-app/src/services/__tests__/providers.test.ts index 6660ffa30..c7b041cd5 100644 --- a/web-app/src/services/__tests__/providers.test.ts +++ b/web-app/src/services/__tests__/providers.test.ts @@ -222,7 +222,7 @@ describe('providers service', () => { ) }) - it('should throw error when API response is not ok', async () => { + it('should throw error when API response is not ok (404)', async () => { const mockResponse = { ok: false, status: 404, @@ -236,7 +236,43 @@ describe('providers service', () => { } await expect(fetchModelsFromProvider(provider)).rejects.toThrow( - 'Cannot connect to custom at https://api.custom.com. Please check that the service is running and accessible.' + 'Models endpoint not found for custom. Check the base URL configuration.' + ) + }) + + it('should throw error when API response is not ok (403)', async () => { + const mockResponse = { + ok: false, + status: 403, + statusText: 'Forbidden', + } + vi.mocked(fetchTauri).mockResolvedValue(mockResponse as any) + + const provider = { + provider: 'custom', + base_url: 'https://api.custom.com', + } as ModelProvider + + await expect(fetchModelsFromProvider(provider)).rejects.toThrow( + 'Access forbidden: Check your API key permissions for custom' + ) + }) + + it('should throw error when API response is not ok (401)', async () => { + const mockResponse = { + ok: false, + status: 401, + statusText: 'Unauthorized', + } + vi.mocked(fetchTauri).mockResolvedValue(mockResponse as any) + + const provider = { + provider: 'custom', + base_url: 'https://api.custom.com', + } as ModelProvider + + await expect(fetchModelsFromProvider(provider)).rejects.toThrow( + 'Authentication failed: API key is required or invalid for custom' ) }) From f35e6cdae86973b7534c710cb3ae73bad807ae2d Mon Sep 17 00:00:00 2001 From: lugnicca Date: Fri, 22 Aug 2025 02:20:47 +0200 Subject: [PATCH 03/15] refactor: clean model selector and add more tests --- web-app/src/containers/ModelCombobox.tsx | 496 +++++++++------- .../__tests__/ModelCombobox.test.tsx | 548 ++++++++++++++---- 2 files changed, 708 insertions(+), 336 deletions(-) diff --git a/web-app/src/containers/ModelCombobox.tsx b/web-app/src/containers/ModelCombobox.tsx index 34c95bf6d..80fe0157c 100644 --- a/web-app/src/containers/ModelCombobox.tsx +++ b/web-app/src/containers/ModelCombobox.tsx @@ -6,6 +6,216 @@ import { IconChevronDown, IconLoader2, IconRefresh } from '@tabler/icons-react' import { cn } from '@/lib/utils' import { useTranslation } from '@/i18n/react-i18next-compat' +// Hook for the dropdown position +function useDropdownPosition(open: boolean, containerRef: React.RefObject) { + const [dropdownPosition, setDropdownPosition] = useState({ top: 0, left: 0, width: 0 }) + + const updateDropdownPosition = useCallback(() => { + if (containerRef.current) { + const rect = containerRef.current.getBoundingClientRect() + setDropdownPosition({ + top: rect.bottom + window.scrollY + 4, + left: rect.left + window.scrollX, + width: rect.width, + }) + } + }, [containerRef]) + + // Update the position when the dropdown opens + useEffect(() => { + if (open) { + requestAnimationFrame(() => { + updateDropdownPosition() + }) + } + }, [open, updateDropdownPosition]) + + // Update the position when the window is resized + useEffect(() => { + if (!open) return + + const handleResize = () => { + updateDropdownPosition() + } + + window.addEventListener('resize', handleResize) + window.addEventListener('scroll', handleResize) + + return () => { + window.removeEventListener('resize', handleResize) + window.removeEventListener('scroll', handleResize) + } + }, [open, updateDropdownPosition]) + + return { dropdownPosition, updateDropdownPosition } +} + +// Components for the different sections of the dropdown +const ErrorSection = ({ error, onRefresh, t }: { error: string; onRefresh?: () => void; t: (key: string) => string }) => ( +
+
+ {t('common:failedToLoadModels')} + {onRefresh && ( + + )} +
+
{error}
+
+) + +const LoadingSection = ({ t }: { t: (key: string) => string }) => ( +
+ + {t('common:loading')} +
+) + +const EmptySection = ({ inputValue, t }: { inputValue: string; t: (key: string, options?: Record) => string }) => ( +
+ {inputValue.trim() ? ( + {t('common:noModelsFoundFor', { searchValue: inputValue })} + ) : ( + {t('common:noModels')} + )} +
+) + +const ModelsList = ({ + filteredModels, + value, + highlightedIndex, + onModelSelect, + onHighlight +}: { + filteredModels: string[] + value: string + highlightedIndex: number + onModelSelect: (model: string) => void + onHighlight: (index: number) => void +}) => ( + <> + {filteredModels.map((model, index) => ( +
{ + e.stopPropagation() + onModelSelect(model) + }} + onMouseEnter={() => onHighlight(index)} + className={cn( + 'cursor-pointer px-3 py-2 hover:bg-main-view-fg/15 hover:shadow-sm transition-all duration-200 text-main-view-fg', + value === model && 'bg-main-view-fg/12 shadow-sm', + highlightedIndex === index && 'bg-main-view-fg/20 shadow-md' + )} + > + {model} +
+ ))} + +) + +// Custom hook for keyboard navigation +function useKeyboardNavigation( + open: boolean, + setOpen: React.Dispatch>, + models: string[], + filteredModels: string[], + highlightedIndex: number, + setHighlightedIndex: React.Dispatch>, + onModelSelect: (model: string) => void, + dropdownRef: React.RefObject +) { + const keyRepeatTimeoutRef = useRef(null) + + // Scroll to the highlighted element + useEffect(() => { + if (highlightedIndex >= 0 && dropdownRef.current) { + requestAnimationFrame(() => { + const modelElements = dropdownRef.current?.querySelectorAll('[data-model]') + const highlightedElement = modelElements?.[highlightedIndex] as HTMLElement + if (highlightedElement) { + highlightedElement.scrollIntoView({ + block: 'nearest', + behavior: 'auto' + }) + } + }) + } + }, [highlightedIndex, dropdownRef]) + + const handleKeyDown = useCallback((e: React.KeyboardEvent) => { + // Open the dropdown with the arrows if closed + if (!open && (e.key === 'ArrowDown' || e.key === 'ArrowUp')) { + if (models.length > 0) { + e.preventDefault() + setOpen(true) + setHighlightedIndex(0) + } + return + } + + if (!open) return + + switch (e.key) { + case 'ArrowDown': + e.preventDefault() + if (keyRepeatTimeoutRef.current) { + clearTimeout(keyRepeatTimeoutRef.current) + } + setHighlightedIndex((prev: number) => filteredModels.length === 0 ? 0 : (prev < filteredModels.length - 1 ? prev + 1 : 0)) + break + case 'ArrowUp': + e.preventDefault() + if (keyRepeatTimeoutRef.current) { + clearTimeout(keyRepeatTimeoutRef.current) + } + setHighlightedIndex((prev: number) => filteredModels.length === 0 ? 0 : (prev > 0 ? prev - 1 : filteredModels.length - 1)) + break + case 'Enter': + e.preventDefault() + if (highlightedIndex >= 0 && highlightedIndex < filteredModels.length) { + onModelSelect(filteredModels[highlightedIndex]) + } + break + case 'ArrowRight': + case 'ArrowLeft': + setOpen(false) + setHighlightedIndex(-1) + break + case 'PageUp': + setHighlightedIndex(0) + break + case 'PageDown': + setHighlightedIndex(filteredModels.length - 1) + break + } + }, [open, setOpen, models.length, filteredModels, highlightedIndex, setHighlightedIndex, onModelSelect]) + + // Cleanup the timeout + useEffect(() => { + const timeoutId = keyRepeatTimeoutRef.current + return () => { + if (timeoutId) { + clearTimeout(timeoutId) + } + } + }, []) + + return { handleKeyDown } +} + type ModelComboboxProps = { value: string onChange: (value: string) => void @@ -31,12 +241,10 @@ export function ModelCombobox({ }: ModelComboboxProps) { const [open, setOpen] = useState(false) const [inputValue, setInputValue] = useState(value) - const [dropdownPosition, setDropdownPosition] = useState({ top: 0, left: 0, width: 0 }) const [highlightedIndex, setHighlightedIndex] = useState(-1) const inputRef = useRef(null) const containerRef = useRef(null) - const dropdownRef = useRef(null) - const keyRepeatTimeoutRef = useRef(null) + const dropdownRef = useRef(null) const { t } = useTranslation() // Sync input value with prop value @@ -44,47 +252,36 @@ export function ModelCombobox({ setInputValue(value) }, [value]) - // Simple position calculation - const updateDropdownPosition = useCallback(() => { - if (containerRef.current) { - const rect = containerRef.current.getBoundingClientRect() - setDropdownPosition({ - top: rect.bottom + window.scrollY + 4, - left: rect.left + window.scrollX, - width: rect.width, - }) - } - }, []) + // Hook for the dropdown position + const { dropdownPosition } = useDropdownPosition(open, containerRef) - // Update position when opening + // Optimized model filtering + const filteredModels = useMemo(() => { + if (!inputValue.trim()) return models + const searchValue = inputValue.toLowerCase() + return models.filter((model) => model.toLowerCase().includes(searchValue)) + }, [models, inputValue]) + + // Reset highlighted index when filtered models change useEffect(() => { - if (open) { - // Use requestAnimationFrame to ensure DOM is ready - requestAnimationFrame(() => { - updateDropdownPosition() - }) - } - }, [open, updateDropdownPosition]) + setHighlightedIndex(-1) + }, [filteredModels]) - // Close dropdown when clicking outside + // Close the dropdown when clicking outside useEffect(() => { if (!open) return const handleClickOutside = (event: Event) => { const target = event.target as Node - // Check if click is inside our container or dropdown const isInsideContainer = containerRef.current?.contains(target) const isInsideDropdown = dropdownRef.current?.contains(target) - // Only close if click is outside both container and dropdown if (!isInsideContainer && !isInsideDropdown) { setOpen(false) - setDropdownPosition({ top: 0, left: 0, width: 0 }) setHighlightedIndex(-1) } } - // Use multiple event types to ensure we catch all interactions const events = ['mousedown', 'touchstart'] events.forEach(eventType => { document.addEventListener(eventType, handleClickOutside, { capture: true, passive: true }) @@ -97,127 +294,60 @@ export function ModelCombobox({ } }, [open]) - // Cleanup: close dropdown when component unmounts + // Cleanup: close the dropdown when the component is unmounted useEffect(() => { - const timeoutId = keyRepeatTimeoutRef.current return () => { setOpen(false) - setDropdownPosition({ top: 0, left: 0, width: 0 }) setHighlightedIndex(-1) - if (timeoutId) { - clearTimeout(timeoutId) - } } }, []) - // Filter models based on input - const filteredModels = useMemo(() => { - if (!inputValue.trim()) return models - - return models.filter((model) => - model.toLowerCase().includes(inputValue.toLowerCase()) - ) - }, [models, inputValue]) - - // Reset highlighted index when filtered models change - useEffect(() => { - setHighlightedIndex(-1) - }, [filteredModels]) - - // Scroll to highlighted item with debouncing to handle key repeat - useEffect(() => { - if (highlightedIndex >= 0 && dropdownRef.current && !loading && !error) { - // Use requestAnimationFrame to ensure smooth scrolling and avoid conflicts - requestAnimationFrame(() => { - // Find all model elements (they have the data-model attribute) - const modelElements = dropdownRef.current?.querySelectorAll('[data-model]') - const highlightedElement = modelElements?.[highlightedIndex] as HTMLElement - if (highlightedElement) { - highlightedElement.scrollIntoView({ - block: 'nearest', - behavior: 'auto' - }) - } - }) - } - }, [highlightedIndex, error, loading]) - - // Handle input change - const handleInputChange = (newValue: string) => { + // Handler for the input change + const handleInputChange = useCallback((newValue: string) => { setInputValue(newValue) onChange(newValue) - - // Only open dropdown if user is actively typing and there are models + + // Open the dropdown if the user types and there are models if (newValue.trim() && models.length > 0) { setOpen(true) } else { - // Don't auto-open on empty input - wait for user interaction setOpen(false) } - } + }, [onChange, models.length]) - // Handle keyboard navigation - const handleKeyDown = (e: React.KeyboardEvent) => { - if (!open && (e.key === 'ArrowDown' || e.key === 'ArrowUp')) { - // Open dropdown on arrow keys if there are models - if (models.length > 0) { - e.preventDefault() - setOpen(true) - setHighlightedIndex(0) - } - return - } - - if (!open) return - - switch (e.key) { - case 'ArrowDown': - e.preventDefault() - if (keyRepeatTimeoutRef.current) { - clearTimeout(keyRepeatTimeoutRef.current) - } - setHighlightedIndex((prev) => - filteredModels.length === 0 ? 0 : (prev < filteredModels.length - 1 ? prev + 1 : 0) - ) - break - case 'ArrowUp': - e.preventDefault() - if (keyRepeatTimeoutRef.current) { - clearTimeout(keyRepeatTimeoutRef.current) - } - setHighlightedIndex((prev) => - filteredModels.length === 0 ? 0 : (prev > 0 ? prev - 1 : filteredModels.length - 1) - ) - break - case 'Enter': - e.preventDefault() - if (highlightedIndex >= 0 && highlightedIndex < filteredModels.length) { - handleModelSelect(filteredModels[highlightedIndex]) - } - break - case 'ArrowRight': - case 'ArrowLeft': - setOpen(false) - setHighlightedIndex(-1) - break - case 'PageUp': - setHighlightedIndex(0) - break - case 'PageDown': - setHighlightedIndex(filteredModels.length - 1) - break - } - } - - // Handle model selection from dropdown - const handleModelSelect = (model: string) => { + // Handler for the model selection + const handleModelSelect = useCallback((model: string) => { setInputValue(model) onChange(model) setOpen(false) - setDropdownPosition({ top: 0, left: 0, width: 0 }) setHighlightedIndex(-1) inputRef.current?.focus() - } + }, [onChange]) + + // Hook for the keyboard navigation + const { handleKeyDown } = useKeyboardNavigation( + open, + setOpen, + models, + filteredModels, + highlightedIndex, + setHighlightedIndex, + handleModelSelect, + dropdownRef + ) + + // Handler for the dropdown opening + const handleDropdownToggle = useCallback(() => { + inputRef.current?.focus() + setOpen(!open) + }, [open]) + + // Handler for the input click + const handleInputClick = useCallback(() => { + if (models.length > 0) { + setOpen(true) + } + }, [models.length]) return (
@@ -227,12 +357,7 @@ export function ModelCombobox({ value={inputValue} onChange={(e) => handleInputChange(e.target.value)} onKeyDown={handleKeyDown} - onClick={() => { - // Open dropdown on click if models are available - if (models.length > 0) { - setOpen(true) - } - }} + onClick={handleInputClick} placeholder={placeholder} disabled={disabled} className="pr-8" @@ -243,14 +368,8 @@ export function ModelCombobox({ variant="link" size="sm" disabled={disabled} - onMouseDown={(e) => { - // Prevent losing focus from input - e.preventDefault() - }} - onClick={() => { - inputRef.current?.focus() - setOpen(!open) - }} + onMouseDown={(e) => e.preventDefault()} + onClick={handleDropdownToggle} className="absolute right-1 top-1/2 h-6 w-6 p-0 -translate-y-1/2 no-underline hover:bg-main-view-fg/10" > {loading ? ( @@ -274,89 +393,30 @@ export function ModelCombobox({ pointerEvents: 'auto', }} data-dropdown="model-combobox" - onPointerDown={(e) => { - // Prevent interaction with underlying elements - e.stopPropagation() - }} - onClick={(e) => { - // Prevent click from bubbling up and closing modal - e.stopPropagation() - }} - onMouseDown={(e) => { - // Allow default behavior for scrolling and selection - e.stopPropagation() - }} - onWheel={(e) => { - // Allow wheel events for scrolling - e.stopPropagation() - }} + onPointerDown={(e) => e.stopPropagation()} + onClick={(e) => e.stopPropagation()} + onMouseDown={(e) => e.stopPropagation()} + onWheel={(e) => e.stopPropagation()} > {/* Error state */} - {error && ( -
-
- {t('common:failedToLoadModels')} - {onRefresh && ( - - )} -
-
{error}
-
- )} + {error && } {/* Loading state */} - {loading && ( -
- - {t('common:loading')} -
- )} + {loading && } {/* Models list */} {!loading && !error && ( - <> - {filteredModels.length === 0 ? ( -
- {inputValue.trim() ? ( - {t('common:noModelsFoundFor', { searchValue: inputValue })} - ) : ( - {t('common:noModels')} - )} -
- ) : ( - <> - {/* Available models */} - {filteredModels.map((model, index) => ( -
{ - e.stopPropagation() - handleModelSelect(model) - }} - onMouseEnter={() => setHighlightedIndex(index)} - className={cn( - 'cursor-pointer px-3 py-2 hover:bg-main-view-fg/15 hover:shadow-sm transition-all duration-200 text-main-view-fg', - value === model && 'bg-main-view-fg/12 shadow-sm', - highlightedIndex === index && 'bg-main-view-fg/20 shadow-md' - )} - > - {model} -
- ))} - - )} - + filteredModels.length === 0 ? ( + + ) : ( + + ) )}
, document.body diff --git a/web-app/src/containers/__tests__/ModelCombobox.test.tsx b/web-app/src/containers/__tests__/ModelCombobox.test.tsx index 88158f268..29496a6aa 100644 --- a/web-app/src/containers/__tests__/ModelCombobox.test.tsx +++ b/web-app/src/containers/__tests__/ModelCombobox.test.tsx @@ -1,197 +1,509 @@ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { describe, it, expect, vi, beforeEach } from 'vitest' import { render, screen, fireEvent, waitFor, act } from '@testing-library/react' import userEvent from '@testing-library/user-event' import '@testing-library/jest-dom/vitest' -import { ModelCombobox } from '../ModelCombobox' import React from 'react' +import { ModelCombobox } from '../ModelCombobox' + +// Mock translation hook +vi.mock('@/i18n/react-i18next-compat', () => ({ + useTranslation: () => ({ + t: (key: string, options?: Record) => { + if (key === 'common:failedToLoadModels') return 'Failed to load models' + if (key === 'common:loading') return 'Loading' + if (key === 'common:noModelsFoundFor') return `No models found for "${options?.searchValue}"` + if (key === 'common:noModels') return 'No models available' + return key + }, + }), +})) describe('ModelCombobox', () => { + const mockOnChange = vi.fn() + const mockOnRefresh = vi.fn() + const defaultProps = { value: '', - onChange: vi.fn(), + onChange: mockOnChange, models: ['gpt-3.5-turbo', 'gpt-4', 'claude-3-haiku'], } - const mockUser = userEvent.setup() - beforeEach(() => { vi.clearAllMocks() + + Element.prototype.getBoundingClientRect = vi.fn(() => ({ + width: 300, + height: 40, + top: 100, + left: 50, + bottom: 140, + right: 350, + x: 50, + y: 100, + toJSON: () => {}, + })) + + Element.prototype.scrollIntoView = vi.fn() }) - afterEach(() => { - vi.restoreAllMocks() - }) - - describe('Basic Rendering', () => { - it('should render input field with placeholder', () => { + it('renders input field with default placeholder', () => { + act(() => { render() - - const input = screen.getByRole('textbox') - expect(input).toBeInTheDocument() - expect(input).toHaveAttribute('placeholder', 'Type or select a model...') }) + + const input = screen.getByRole('textbox') + expect(input).toBeInTheDocument() + expect(input).toHaveAttribute('placeholder', 'Type or select a model...') + }) - it('should render custom placeholder', () => { + it('renders custom placeholder', () => { + act(() => { render() - - const input = screen.getByRole('textbox') - expect(input).toHaveAttribute('placeholder', 'Choose a model') }) + + const input = screen.getByRole('textbox') + expect(input).toHaveAttribute('placeholder', 'Choose a model') + }) - it('should render dropdown trigger button', () => { + it('renders dropdown trigger button', () => { + act(() => { render() - const button = screen.getByRole('button') - expect(button).toBeInTheDocument() }) + + const button = screen.getByRole('button') + expect(button).toBeInTheDocument() + }) - it('should display current value in input', () => { + it('displays current value in input', () => { + act(() => { render() - - const input = screen.getByDisplayValue('gpt-4') - expect(input).toBeInTheDocument() - }) - - it('should apply custom className', () => { - const { container } = render( - - ) - - const wrapper = container.firstChild as HTMLElement - expect(wrapper).toHaveClass('custom-class') }) + + const input = screen.getByDisplayValue('gpt-4') + expect(input).toBeInTheDocument() }) - describe('Disabled State', () => { - it('should disable input when disabled prop is true', () => { + it('applies custom className', () => { + const { container } = render( + + ) + + const wrapper = container.firstChild as HTMLElement + expect(wrapper).toHaveClass('custom-class') + }) + + it('disables input when disabled prop is true', () => { + act(() => { render() - - const input = screen.getByRole('textbox') - const button = screen.getByRole('button') - - expect(input).toBeDisabled() - expect(button).toBeDisabled() }) + + const input = screen.getByRole('textbox') + const button = screen.getByRole('button') - it('should not open dropdown when disabled', async () => { - render() - - const input = screen.getByRole('textbox') - await mockUser.click(input) - - expect(screen.queryByTestId('dropdown')).not.toBeInTheDocument() - }) + expect(input).toBeDisabled() + expect(button).toBeDisabled() }) - describe('Loading State', () => { - it('should show loading spinner in trigger button', () => { + it('shows loading spinner in trigger button', () => { + act(() => { render() - - const button = screen.getByRole('button') - const spinner = button.querySelector('.animate-spin') - expect(spinner).toBeInTheDocument() }) + + const button = screen.getByRole('button') + const spinner = button.querySelector('.animate-spin') + expect(spinner).toBeInTheDocument() + }) - it('should show loading spinner when loading prop is true', () => { - render() - - const spinner = screen.getByRole('button').querySelector('.animate-spin') - expect(spinner).toBeInTheDocument() + it('shows loading section when dropdown is opened during loading', async () => { + const user = userEvent.setup() + render() + + // Click input to trigger dropdown opening + const input = screen.getByRole('textbox') + await user.click(input) + + // Wait for dropdown to appear and check loading section + await waitFor(() => { + const dropdown = document.querySelector('[data-dropdown="model-combobox"]') + expect(dropdown).toBeInTheDocument() + expect(screen.getByText('Loading')).toBeInTheDocument() }) }) - describe('Input Interactions', () => { - it('should call onChange when typing', async () => { - const mockOnChange = vi.fn() - render() + it('calls onChange when typing', async () => { + const user = userEvent.setup() + const localMockOnChange = vi.fn() + render() - const input = screen.getByRole('textbox') - await mockUser.type(input, 'g') + const input = screen.getByRole('textbox') + await user.type(input, 'g') - expect(mockOnChange).toHaveBeenCalledWith('g') - }) - - it('should update input value when typing', async () => { - const mockOnChange = vi.fn() - render() - - const input = screen.getByRole('textbox') - await mockUser.type(input, 'test') - - expect(input).toHaveValue('test') - }) - - it('should handle input focus', async () => { - render() - - const input = screen.getByRole('textbox') - await mockUser.click(input) - - expect(input).toHaveFocus() - }) + expect(localMockOnChange).toHaveBeenCalledWith('g') }) - describe('Props Validation', () => { - it('should render with empty models array', () => { + it('updates input value when typing', async () => { + const user = userEvent.setup() + render() + + const input = screen.getByRole('textbox') + await user.type(input, 'test') + + expect(input).toHaveValue('test') + }) + + it('handles input focus', async () => { + const user = userEvent.setup() + render() + + const input = screen.getByRole('textbox') + await user.click(input) + + expect(input).toHaveFocus() + }) + + it('renders with empty models array', () => { + act(() => { render() - - const input = screen.getByRole('textbox') - expect(input).toBeInTheDocument() }) + + const input = screen.getByRole('textbox') + expect(input).toBeInTheDocument() + }) - it('should render with models array', () => { + it('renders with models array', () => { + act(() => { render() + }) + + const input = screen.getByRole('textbox') + expect(input).toBeInTheDocument() + }) - const input = screen.getByRole('textbox') - expect(input).toBeInTheDocument() + it('handles mount and unmount without errors', () => { + const { unmount } = render() + + expect(screen.getByRole('textbox')).toBeInTheDocument() + + unmount() + + expect(screen.queryByRole('textbox')).not.toBeInTheDocument() + }) + + it('handles props changes', () => { + const { rerender } = render() + + expect(screen.getByDisplayValue('')).toBeInTheDocument() + + rerender() + + expect(screen.getByDisplayValue('gpt-4')).toBeInTheDocument() + }) + + it('handles models array changes', () => { + const { rerender } = render() + + expect(screen.getByRole('textbox')).toBeInTheDocument() + + rerender() + + expect(screen.getByRole('textbox')).toBeInTheDocument() + }) + + it('does not open dropdown when clicking input with no models', async () => { + const user = userEvent.setup() + render() + + const input = screen.getByRole('textbox') + await user.click(input) + + // Should focus but not open dropdown + expect(input).toHaveFocus() + const dropdown = document.querySelector('[data-dropdown="model-combobox"]') + expect(dropdown).not.toBeInTheDocument() + }) + + it('accepts error prop without crashing', () => { + act(() => { + render() }) - it('should render with all props', () => { + const input = screen.getByRole('textbox') + expect(input).toBeInTheDocument() + expect(input).toHaveAttribute('placeholder', 'Type or select a model...') + }) + + it('renders with all props', () => { + act(() => { render( ) + }) + + const input = screen.getByRole('textbox') + expect(input).toBeInTheDocument() + expect(input).toBeDisabled() + }) - const input = screen.getByRole('textbox') - expect(input).toBeInTheDocument() - expect(input).toBeDisabled() + it('opens dropdown when clicking trigger button', async () => { + const user = userEvent.setup() + render() + + const button = screen.getByRole('button') + await user.click(button) + + await waitFor(() => { + const dropdown = document.querySelector('[data-dropdown="model-combobox"]') + expect(dropdown).toBeInTheDocument() }) }) - describe('Component Lifecycle', () => { - it('should handle mount and unmount without errors', () => { - const { unmount } = render() + it('opens dropdown when clicking input', async () => { + const user = userEvent.setup() + render() - expect(screen.getByRole('textbox')).toBeInTheDocument() + const input = screen.getByRole('textbox') + await user.click(input) - unmount() + expect(input).toHaveFocus() + await waitFor(() => { + const dropdown = document.querySelector('[data-dropdown="model-combobox"]') + expect(dropdown).toBeInTheDocument() + }) + }) - expect(screen.queryByRole('textbox')).not.toBeInTheDocument() + it('filters models based on input value', async () => { + const user = userEvent.setup() + const localMockOnChange = vi.fn() + render() + + const input = screen.getByRole('textbox') + await user.type(input, 'gpt-4') + + expect(localMockOnChange).toHaveBeenCalledWith('gpt-4') + }) + + it('shows filtered models in dropdown when typing', async () => { + const user = userEvent.setup() + render() + + const input = screen.getByRole('textbox') + // Type 'gpt' to trigger dropdown opening + await user.type(input, 'gpt') + + await waitFor(() => { + // Dropdown should be open + const dropdown = document.querySelector('[data-dropdown="model-combobox"]') + expect(dropdown).toBeInTheDocument() + + // Should show GPT models + expect(screen.getByText('gpt-3.5-turbo')).toBeInTheDocument() + expect(screen.getByText('gpt-4')).toBeInTheDocument() + // Should not show Claude + expect(screen.queryByText('claude-3-haiku')).not.toBeInTheDocument() + }) + }) + + it('handles case insensitive filtering', async () => { + const user = userEvent.setup() + render() + + const input = screen.getByRole('textbox') + await user.type(input, 'GPT') + + expect(mockOnChange).toHaveBeenCalledWith('GPT') + }) + + it('shows empty state when no models match filter', async () => { + const user = userEvent.setup() + render() + + const input = screen.getByRole('textbox') + // Type something that doesn't match any model to trigger dropdown + empty state + await user.type(input, 'nonexistent') + + await waitFor(() => { + // Dropdown should be open + const dropdown = document.querySelector('[data-dropdown="model-combobox"]') + expect(dropdown).toBeInTheDocument() + // Should show empty state message + expect(screen.getByText('No models found for "nonexistent"')).toBeInTheDocument() + }) + }) + + it('selects model from dropdown when clicked', async () => { + const user = userEvent.setup() + const localMockOnChange = vi.fn() + render() + + const input = screen.getByRole('textbox') + await user.click(input) + + await waitFor(() => { + const modelOption = screen.getByText('gpt-4') + expect(modelOption).toBeInTheDocument() + }) + + const modelOption = screen.getByText('gpt-4') + await user.click(modelOption) + + expect(localMockOnChange).toHaveBeenCalledWith('gpt-4') + expect(input).toHaveValue('gpt-4') + }) + + it('submits input value with Enter key', async () => { + const user = userEvent.setup() + const localMockOnChange = vi.fn() + render() + + const input = screen.getByRole('textbox') + await user.type(input, 'gpt') + await user.keyboard('{Enter}') + + expect(localMockOnChange).toHaveBeenCalledWith('gpt') + }) + + it('updates input value when typing', () => { + render() + + const input = screen.getByRole('textbox') + fireEvent.change(input, { target: { value: 'gpt-4' } }) + + expect(input).toHaveValue('gpt-4') + }) + + it('displays error message in dropdown', async () => { + const user = userEvent.setup() + render() + + const input = screen.getByRole('textbox') + // Click input to open dropdown + await user.click(input) + + await waitFor(() => { + // Dropdown should be open + const dropdown = document.querySelector('[data-dropdown="model-combobox"]') + expect(dropdown).toBeInTheDocument() + // Error messages should be displayed + expect(screen.getByText('Failed to load models')).toBeInTheDocument() + expect(screen.getByText('Network connection failed')).toBeInTheDocument() + }) + }) + + it('calls onRefresh when refresh button is clicked', async () => { + const user = userEvent.setup() + const localMockOnRefresh = vi.fn() + render() + + const input = screen.getByRole('textbox') + // Click input to open dropdown + await user.click(input) + + await waitFor(() => { + // Dropdown should be open with error section + const dropdown = document.querySelector('[data-dropdown="model-combobox"]') + expect(dropdown).toBeInTheDocument() + const refreshButton = document.querySelector('[aria-label="Refresh models"]') + expect(refreshButton).toBeInTheDocument() }) - it('should handle props changes', () => { - const { rerender } = render() + const refreshButton = document.querySelector('[aria-label="Refresh models"]') + if (refreshButton) { + await user.click(refreshButton) + expect(localMockOnRefresh).toHaveBeenCalledTimes(1) + } + }) - expect(screen.getByDisplayValue('')).toBeInTheDocument() + it('opens dropdown when pressing ArrowDown', async () => { + const user = userEvent.setup() + render() - rerender() + const input = screen.getByRole('textbox') + input.focus() + await user.keyboard('{ArrowDown}') - expect(screen.getByDisplayValue('gpt-4')).toBeInTheDocument() + expect(input).toHaveFocus() + await waitFor(() => { + const dropdown = document.querySelector('[data-dropdown="model-combobox"]') + expect(dropdown).toBeInTheDocument() }) + }) - it('should handle models array changes', () => { - const { rerender } = render() + it('navigates through models with arrow keys', async () => { + const user = userEvent.setup() + render() - expect(screen.getByRole('textbox')).toBeInTheDocument() + const input = screen.getByRole('textbox') + input.focus() + + // ArrowDown should open dropdown + await user.keyboard('{ArrowDown}') + + await waitFor(() => { + // Dropdown should be open + const dropdown = document.querySelector('[data-dropdown="model-combobox"]') + expect(dropdown).toBeInTheDocument() + }) + + // Navigate to second item + await user.keyboard('{ArrowDown}') - rerender() + await waitFor(() => { + const secondModel = screen.getByText('gpt-4') + const modelElement = secondModel.closest('[data-model]') + expect(modelElement).toHaveClass('bg-main-view-fg/20') + }) + }) - expect(screen.getByRole('textbox')).toBeInTheDocument() + it('handles Enter key to select highlighted model', async () => { + const user = userEvent.setup() + const localMockOnChange = vi.fn() + render() + + const input = screen.getByRole('textbox') + // Type 'gpt' to open dropdown and filter models + await user.type(input, 'gpt') + + await waitFor(() => { + // Dropdown should be open with filtered models + const dropdown = document.querySelector('[data-dropdown="model-combobox"]') + expect(dropdown).toBeInTheDocument() + }) + + // Navigate to highlight first model and select it + await user.keyboard('{ArrowDown}') + await user.keyboard('{Enter}') + + expect(localMockOnChange).toHaveBeenCalledWith('gpt-3.5-turbo') + }) + + it('closes dropdown with ArrowLeft key', async () => { + const user = userEvent.setup() + render() + + const input = screen.getByRole('textbox') + input.focus() + + // ArrowDown should open dropdown + await user.keyboard('{ArrowDown}') + + await waitFor(() => { + const dropdown = document.querySelector('[data-dropdown="model-combobox"]') + expect(dropdown).toBeInTheDocument() + }) + + // ArrowLeft should close dropdown + await user.keyboard('{ArrowLeft}') + + await waitFor(() => { + const dropdown = document.querySelector('[data-dropdown="model-combobox"]') + expect(dropdown).not.toBeInTheDocument() }) }) }) From 9a68631d39ff2448ddafb4af68d0f15f6a85dee6 Mon Sep 17 00:00:00 2001 From: lugnicca Date: Fri, 22 Aug 2025 03:12:37 +0200 Subject: [PATCH 04/15] refactor: more modular error handling in fetchModelsFromProvider function --- web-app/src/services/providers.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/web-app/src/services/providers.ts b/web-app/src/services/providers.ts index 748aed322..a036c9148 100644 --- a/web-app/src/services/providers.ts +++ b/web-app/src/services/providers.ts @@ -182,13 +182,16 @@ export const fetchModelsFromProvider = async ( } catch (error) { console.error('Error fetching models from provider:', error) - if (error instanceof Error && ( - error.message.includes('Authentication failed') || - error.message.includes('Access forbidden') || - error.message.includes('Models endpoint not found') || - error.message.includes('Failed to fetch models from') - )) { - throw error + const structuredErrorPrefixes = [ + 'Authentication failed', + 'Access forbidden', + 'Models endpoint not found', + 'Failed to fetch models from' + ] + + if (error instanceof Error && + structuredErrorPrefixes.some(prefix => (error as Error).message.startsWith(prefix))) { + throw new Error(error.message) } // Provide helpful error message for network issues From 4e8dd9281fadb2b5e65b7ab754957eb6c1ea1449 Mon Sep 17 00:00:00 2001 From: lugnicca Date: Fri, 22 Aug 2025 03:12:58 +0200 Subject: [PATCH 05/15] refactor: simplify event handling and fix test setup in ModelCombobox --- web-app/src/containers/ModelCombobox.tsx | 12 ++---- .../__tests__/ModelCombobox.test.tsx | 37 +++++++++++-------- 2 files changed, 24 insertions(+), 25 deletions(-) diff --git a/web-app/src/containers/ModelCombobox.tsx b/web-app/src/containers/ModelCombobox.tsx index 80fe0157c..ae1af0900 100644 --- a/web-app/src/containers/ModelCombobox.tsx +++ b/web-app/src/containers/ModelCombobox.tsx @@ -61,7 +61,7 @@ const ErrorSection = ({ error, onRefresh, t }: { error: string; onRefresh?: () = size="sm" onClick={(e) => { e.stopPropagation() - onRefresh?.() + onRefresh() }} className="h-6 w-6 p-0 no-underline hover:bg-main-view-fg/10 text-main-view-fg" aria-label="Refresh models" @@ -171,16 +171,10 @@ function useKeyboardNavigation( switch (e.key) { case 'ArrowDown': e.preventDefault() - if (keyRepeatTimeoutRef.current) { - clearTimeout(keyRepeatTimeoutRef.current) - } setHighlightedIndex((prev: number) => filteredModels.length === 0 ? 0 : (prev < filteredModels.length - 1 ? prev + 1 : 0)) break case 'ArrowUp': e.preventDefault() - if (keyRepeatTimeoutRef.current) { - clearTimeout(keyRepeatTimeoutRef.current) - } setHighlightedIndex((prev: number) => filteredModels.length === 0 ? 0 : (prev > 0 ? prev - 1 : filteredModels.length - 1)) break case 'Enter': @@ -195,9 +189,11 @@ function useKeyboardNavigation( setHighlightedIndex(-1) break case 'PageUp': + e.preventDefault() setHighlightedIndex(0) break case 'PageDown': + e.preventDefault() setHighlightedIndex(filteredModels.length - 1) break } @@ -394,8 +390,6 @@ export function ModelCombobox({ }} data-dropdown="model-combobox" onPointerDown={(e) => e.stopPropagation()} - onClick={(e) => e.stopPropagation()} - onMouseDown={(e) => e.stopPropagation()} onWheel={(e) => e.stopPropagation()} > {/* Error state */} diff --git a/web-app/src/containers/__tests__/ModelCombobox.test.tsx b/web-app/src/containers/__tests__/ModelCombobox.test.tsx index 29496a6aa..1b91c3712 100644 --- a/web-app/src/containers/__tests__/ModelCombobox.test.tsx +++ b/web-app/src/containers/__tests__/ModelCombobox.test.tsx @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest' +import { describe, it, expect, vi, beforeEach, beforeAll, afterAll } from 'vitest' import { render, screen, fireEvent, waitFor, act } from '@testing-library/react' import userEvent from '@testing-library/user-event' import '@testing-library/jest-dom/vitest' @@ -28,10 +28,11 @@ describe('ModelCombobox', () => { models: ['gpt-3.5-turbo', 'gpt-4', 'claude-3-haiku'], } - beforeEach(() => { - vi.clearAllMocks() + let bcrSpy: ReturnType + let scrollSpy: ReturnType - Element.prototype.getBoundingClientRect = vi.fn(() => ({ + beforeAll(() => { + const mockRect = { width: 300, height: 40, top: 100, @@ -41,9 +42,22 @@ describe('ModelCombobox', () => { x: 50, y: 100, toJSON: () => {}, - })) - - Element.prototype.scrollIntoView = vi.fn() + } as unknown as DOMRect + + bcrSpy = vi + .spyOn(Element.prototype as any, 'getBoundingClientRect') + .mockReturnValue(mockRect) + + Element.prototype.scrollIntoView = () => {} + }) + + beforeEach(() => { + vi.clearAllMocks() + }) + + afterAll(() => { + bcrSpy?.mockRestore() + scrollSpy?.mockRestore() }) it('renders input field with default placeholder', () => { @@ -369,15 +383,6 @@ describe('ModelCombobox', () => { expect(localMockOnChange).toHaveBeenCalledWith('gpt') }) - it('updates input value when typing', () => { - render() - - const input = screen.getByRole('textbox') - fireEvent.change(input, { target: { value: 'gpt-4' } }) - - expect(input).toHaveValue('gpt-4') - }) - it('displays error message in dropdown', async () => { const user = userEvent.setup() render() From 1bf5802a68549e748a0aea9264bb293ea75317a8 Mon Sep 17 00:00:00 2001 From: lugnicca Date: Fri, 22 Aug 2025 03:13:34 +0200 Subject: [PATCH 06/15] refactor: update MockModelProvider type to use ModelProvider and clean up test setup --- .../hooks/__tests__/useProviderModels.test.ts | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/web-app/src/hooks/__tests__/useProviderModels.test.ts b/web-app/src/hooks/__tests__/useProviderModels.test.ts index 3d107b9f8..89bc9e26a 100644 --- a/web-app/src/hooks/__tests__/useProviderModels.test.ts +++ b/web-app/src/hooks/__tests__/useProviderModels.test.ts @@ -10,15 +10,13 @@ vi.mock('@/services/providers', () => ({ import { fetchModelsFromProvider } from '@/services/providers' const mockFetchModelsFromProvider = vi.mocked(fetchModelsFromProvider) +import type { ModelProvider } from '@/types/modelProviders' + // Mock ModelProvider type -type MockModelProvider = { - active: boolean - provider: string - base_url?: string - api_key?: string - settings: any[] - models: any[] -} +type MockModelProvider = Pick< + ModelProvider, + 'active' | 'provider' | 'base_url' | 'api_key' | 'settings' | 'models' +> describe('useProviderModels', () => { const mockProvider: MockModelProvider = { @@ -34,8 +32,6 @@ describe('useProviderModels', () => { beforeEach(() => { vi.clearAllMocks() - // Reset the cache by clearing any previous state - mockFetchModelsFromProvider.mockClear() }) it('should initialize with empty state', () => { From aa568e62902cdddf00c46a26c36e21a2a26fa63e Mon Sep 17 00:00:00 2001 From: lugnicca Date: Sat, 23 Aug 2025 15:07:42 +0200 Subject: [PATCH 07/15] fix: remove ModelProvider type --- web-app/src/hooks/useProviderModels.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/web-app/src/hooks/useProviderModels.ts b/web-app/src/hooks/useProviderModels.ts index 5c984d1ca..7e5ee6650 100644 --- a/web-app/src/hooks/useProviderModels.ts +++ b/web-app/src/hooks/useProviderModels.ts @@ -1,6 +1,5 @@ import { useState, useEffect, useCallback, useRef } from 'react' import { fetchModelsFromProvider } from '@/services/providers' -import type { ModelProvider } from '@/types/providers' type UseProviderModelsState = { models: string[] From 639bd5fb276e5a1afb6d957085519f04216f0490 Mon Sep 17 00:00:00 2001 From: lugnicca Date: Sat, 23 Aug 2025 18:08:29 +0200 Subject: [PATCH 08/15] fix: set Escape in keyboard navigation --- web-app/src/containers/ModelCombobox.tsx | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/web-app/src/containers/ModelCombobox.tsx b/web-app/src/containers/ModelCombobox.tsx index ae1af0900..29fc3ee50 100644 --- a/web-app/src/containers/ModelCombobox.tsx +++ b/web-app/src/containers/ModelCombobox.tsx @@ -196,6 +196,11 @@ function useKeyboardNavigation( e.preventDefault() setHighlightedIndex(filteredModels.length - 1) break + case 'Escape': + e.preventDefault() + setOpen(false) + setHighlightedIndex(-1) + break } }, [open, setOpen, models.length, filteredModels, highlightedIndex, setHighlightedIndex, onModelSelect]) From 6c0e6dce0671d4ee5b62177657a9dcb58d36bed6 Mon Sep 17 00:00:00 2001 From: lugnicca Date: Sat, 23 Aug 2025 18:32:12 +0200 Subject: [PATCH 09/15] fix: remove unused keyRepeatTimeoutRef --- web-app/src/containers/ModelCombobox.tsx | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/web-app/src/containers/ModelCombobox.tsx b/web-app/src/containers/ModelCombobox.tsx index 29fc3ee50..7bd6672e7 100644 --- a/web-app/src/containers/ModelCombobox.tsx +++ b/web-app/src/containers/ModelCombobox.tsx @@ -137,7 +137,6 @@ function useKeyboardNavigation( onModelSelect: (model: string) => void, dropdownRef: React.RefObject ) { - const keyRepeatTimeoutRef = useRef(null) // Scroll to the highlighted element useEffect(() => { @@ -204,16 +203,6 @@ function useKeyboardNavigation( } }, [open, setOpen, models.length, filteredModels, highlightedIndex, setHighlightedIndex, onModelSelect]) - // Cleanup the timeout - useEffect(() => { - const timeoutId = keyRepeatTimeoutRef.current - return () => { - if (timeoutId) { - clearTimeout(timeoutId) - } - } - }, []) - return { handleKeyDown } } From 1a6a37c0037a65fd6a92f2052066cf5bfa44c908 Mon Sep 17 00:00:00 2001 From: lugnicca Date: Sun, 24 Aug 2025 00:40:02 +0200 Subject: [PATCH 10/15] fix: escape key was closing modal instead of only combobox and remove arrow left/righ closing combobox --- web-app/src/containers/ModelCombobox.tsx | 17 +++++++------ .../__tests__/ModelCombobox.test.tsx | 24 ------------------- web-app/src/containers/dialogs/AddModel.tsx | 10 +++++++- 3 files changed, 19 insertions(+), 32 deletions(-) diff --git a/web-app/src/containers/ModelCombobox.tsx b/web-app/src/containers/ModelCombobox.tsx index 7bd6672e7..9b8144ed3 100644 --- a/web-app/src/containers/ModelCombobox.tsx +++ b/web-app/src/containers/ModelCombobox.tsx @@ -182,8 +182,9 @@ function useKeyboardNavigation( onModelSelect(filteredModels[highlightedIndex]) } break - case 'ArrowRight': - case 'ArrowLeft': + case 'Escape': + e.preventDefault() + e.stopPropagation() setOpen(false) setHighlightedIndex(-1) break @@ -195,11 +196,6 @@ function useKeyboardNavigation( e.preventDefault() setHighlightedIndex(filteredModels.length - 1) break - case 'Escape': - e.preventDefault() - setOpen(false) - setHighlightedIndex(-1) - break } }, [open, setOpen, models.length, filteredModels, highlightedIndex, setHighlightedIndex, onModelSelect]) @@ -216,6 +212,7 @@ type ModelComboboxProps = { placeholder?: string disabled?: boolean className?: string + onOpenChange?: (open: boolean) => void } export function ModelCombobox({ @@ -228,6 +225,7 @@ export function ModelCombobox({ placeholder = 'Type or select a model...', disabled = false, className, + onOpenChange, }: ModelComboboxProps) { const [open, setOpen] = useState(false) const [inputValue, setInputValue] = useState(value) @@ -242,6 +240,11 @@ export function ModelCombobox({ setInputValue(value) }, [value]) + // Notify parent when open state changes + useEffect(() => { + onOpenChange?.(open) + }, [open, onOpenChange]) + // Hook for the dropdown position const { dropdownPosition } = useDropdownPosition(open, containerRef) diff --git a/web-app/src/containers/__tests__/ModelCombobox.test.tsx b/web-app/src/containers/__tests__/ModelCombobox.test.tsx index 1b91c3712..38f9b97c8 100644 --- a/web-app/src/containers/__tests__/ModelCombobox.test.tsx +++ b/web-app/src/containers/__tests__/ModelCombobox.test.tsx @@ -487,28 +487,4 @@ describe('ModelCombobox', () => { expect(localMockOnChange).toHaveBeenCalledWith('gpt-3.5-turbo') }) - - it('closes dropdown with ArrowLeft key', async () => { - const user = userEvent.setup() - render() - - const input = screen.getByRole('textbox') - input.focus() - - // ArrowDown should open dropdown - await user.keyboard('{ArrowDown}') - - await waitFor(() => { - const dropdown = document.querySelector('[data-dropdown="model-combobox"]') - expect(dropdown).toBeInTheDocument() - }) - - // ArrowLeft should close dropdown - await user.keyboard('{ArrowLeft}') - - await waitFor(() => { - const dropdown = document.querySelector('[data-dropdown="model-combobox"]') - expect(dropdown).not.toBeInTheDocument() - }) - }) }) diff --git a/web-app/src/containers/dialogs/AddModel.tsx b/web-app/src/containers/dialogs/AddModel.tsx index dc0eac244..a633867f6 100644 --- a/web-app/src/containers/dialogs/AddModel.tsx +++ b/web-app/src/containers/dialogs/AddModel.tsx @@ -26,6 +26,7 @@ export const DialogAddModel = ({ provider, trigger }: DialogAddModelProps) => { const { updateProvider } = useModelProvider() const [modelId, setModelId] = useState('') const [open, setOpen] = useState(false) + const [isComboboxOpen, setIsComboboxOpen] = useState(false) // Fetch models from provider API (API key is optional) const { models, loading, error, refetch } = useProviderModels( @@ -68,7 +69,13 @@ export const DialogAddModel = ({ provider, trigger }: DialogAddModelProps) => { )} - + { + if (isComboboxOpen) { + e.preventDefault() + } + }} + > {t('providers:addModel.title')} @@ -95,6 +102,7 @@ export const DialogAddModel = ({ provider, trigger }: DialogAddModelProps) => { error={error} onRefresh={refetch} placeholder={t('providers:addModel.enterModelId')} + onOpenChange={setIsComboboxOpen} /> From 70bf257e751b6d09ca4988fe5577d65bfb2c43dd Mon Sep 17 00:00:00 2001 From: lugnicca Date: Tue, 2 Sep 2025 18:18:05 +0200 Subject: [PATCH 11/15] fix: put refresh button directly in input instead of in dropdown --- web-app/src/containers/ModelCombobox.tsx | 84 ++++++++++++++---------- 1 file changed, 48 insertions(+), 36 deletions(-) diff --git a/web-app/src/containers/ModelCombobox.tsx b/web-app/src/containers/ModelCombobox.tsx index 9b8144ed3..ea5b3d670 100644 --- a/web-app/src/containers/ModelCombobox.tsx +++ b/web-app/src/containers/ModelCombobox.tsx @@ -51,24 +51,10 @@ function useDropdownPosition(open: boolean, containerRef: React.RefObject void; t: (key: string) => string }) => ( +const ErrorSection = ({ error, t }: { error: string; t: (key: string) => string }) => (
{t('common:failedToLoadModels')} - {onRefresh && ( - - )}
{error}
@@ -83,11 +69,15 @@ const LoadingSection = ({ t }: { t: (key: string) => string }) => ( const EmptySection = ({ inputValue, t }: { inputValue: string; t: (key: string, options?: Record) => string }) => (
- {inputValue.trim() ? ( - {t('common:noModelsFoundFor', { searchValue: inputValue })} - ) : ( - {t('common:noModels')} - )} +
+
+ {inputValue.trim() ? ( + {t('common:noModelsFoundFor', { searchValue: inputValue })} + ) : ( + {t('common:noModels')} + )} +
+
) @@ -353,24 +343,46 @@ export function ModelCombobox({ onClick={handleInputClick} placeholder={placeholder} disabled={disabled} - className="pr-8" + className="pr-16" /> - {/* Dropdown trigger button */} - )} - + + {/* Custom dropdown rendered as portal */} {open && dropdownPosition.width > 0 && createPortal( @@ -390,7 +402,7 @@ export function ModelCombobox({ onWheel={(e) => e.stopPropagation()} > {/* Error state */} - {error && } + {error && } {/* Loading state */} {loading && } From 3d0ce15fe85bd9f044b00f5f1b3ef44e3ea95ee2 Mon Sep 17 00:00:00 2001 From: lugnicca Date: Tue, 2 Sep 2025 18:19:12 +0200 Subject: [PATCH 12/15] fix: prevent stale provider model requests from polluting UI state --- web-app/src/containers/dialogs/AddModel.tsx | 1 + web-app/src/hooks/useProviderModels.ts | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/web-app/src/containers/dialogs/AddModel.tsx b/web-app/src/containers/dialogs/AddModel.tsx index a633867f6..3ccdc6d65 100644 --- a/web-app/src/containers/dialogs/AddModel.tsx +++ b/web-app/src/containers/dialogs/AddModel.tsx @@ -95,6 +95,7 @@ export const DialogAddModel = ({ provider, trigger }: DialogAddModelProps) => { * (null) const prevProviderKey = useRef('') + const requestIdRef = useRef(0) const fetchModels = useCallback(async () => { if (!provider || !provider.base_url) { @@ -44,11 +45,13 @@ export const useProviderModels = (provider?: ModelProvider): UseProviderModelsSt return } + const currentRequestId = ++requestIdRef.current setLoading(true) setError(null) try { const fetchedModels = await fetchModelsFromProvider(provider) + if (currentRequestId !== requestIdRef.current) return const sortedModels = fetchedModels.sort((a, b) => a.localeCompare(b)) setModels(sortedModels) @@ -59,11 +62,12 @@ export const useProviderModels = (provider?: ModelProvider): UseProviderModelsSt timestamp: Date.now(), }) } catch (err) { + if (currentRequestId !== requestIdRef.current) return const errorMessage = err instanceof Error ? err.message : 'Failed to fetch models' setError(errorMessage) console.error(`Error fetching models from ${provider.provider}:`, err) } finally { - setLoading(false) + if (currentRequestId === requestIdRef.current) setLoading(false) } }, [provider]) From 66af5c7386a12f2efcbbe8837c48628dc71f75a8 Mon Sep 17 00:00:00 2001 From: lugnicca Date: Fri, 5 Sep 2025 19:56:28 +0200 Subject: [PATCH 13/15] fix: use webprovider services to fetch models --- .../hooks/__tests__/useProviderModels.test.ts | 42 ++++++++++--------- web-app/src/hooks/useProviderModels.ts | 9 ++-- web-app/src/services/providers/web.ts | 42 ++++++++++++++++--- 3 files changed, 63 insertions(+), 30 deletions(-) diff --git a/web-app/src/hooks/__tests__/useProviderModels.test.ts b/web-app/src/hooks/__tests__/useProviderModels.test.ts index 89bc9e26a..e6ca301f7 100644 --- a/web-app/src/hooks/__tests__/useProviderModels.test.ts +++ b/web-app/src/hooks/__tests__/useProviderModels.test.ts @@ -1,22 +1,19 @@ import { describe, it, expect, beforeEach, vi } from 'vitest' import { renderHook, waitFor } from '@testing-library/react' import { useProviderModels } from '../useProviderModels' +import { WebProvidersService } from '../../services/providers/web' -// Mock the providers service -vi.mock('@/services/providers', () => ({ - fetchModelsFromProvider: vi.fn(), -})) +let fetchModelsSpy: ReturnType -import { fetchModelsFromProvider } from '@/services/providers' -const mockFetchModelsFromProvider = vi.mocked(fetchModelsFromProvider) - -import type { ModelProvider } from '@/types/modelProviders' - -// Mock ModelProvider type -type MockModelProvider = Pick< - ModelProvider, - 'active' | 'provider' | 'base_url' | 'api_key' | 'settings' | 'models' -> +// Local minimal provider type for tests +type MockModelProvider = { + active: boolean + provider: string + base_url?: string + api_key?: string + settings: any[] + models: any[] +} describe('useProviderModels', () => { const mockProvider: MockModelProvider = { @@ -31,7 +28,12 @@ describe('useProviderModels', () => { const mockModels = ['gpt-4', 'gpt-3.5-turbo', 'gpt-4-turbo'] beforeEach(() => { + vi.restoreAllMocks() vi.clearAllMocks() + fetchModelsSpy = vi.spyOn( + WebProvidersService.prototype, + 'fetchModelsFromProvider' + ) }) it('should initialize with empty state', () => { @@ -45,17 +47,17 @@ describe('useProviderModels', () => { it('should not fetch models when provider is undefined', () => { renderHook(() => useProviderModels(undefined)) - expect(mockFetchModelsFromProvider).not.toHaveBeenCalled() + expect(fetchModelsSpy).not.toHaveBeenCalled() }) it('should not fetch models when provider has no base_url', () => { const providerWithoutUrl = { ...mockProvider, base_url: undefined } renderHook(() => useProviderModels(providerWithoutUrl)) - expect(mockFetchModelsFromProvider).not.toHaveBeenCalled() + expect(fetchModelsSpy).not.toHaveBeenCalled() }) it('should fetch and sort models', async () => { - mockFetchModelsFromProvider.mockResolvedValueOnce(mockModels) + fetchModelsSpy.mockResolvedValueOnce(mockModels) const { result } = renderHook(() => useProviderModels(mockProvider)) @@ -66,11 +68,11 @@ describe('useProviderModels', () => { // Should be sorted alphabetically expect(result.current.models).toEqual(['gpt-3.5-turbo', 'gpt-4', 'gpt-4-turbo']) expect(result.current.error).toBe(null) - expect(mockFetchModelsFromProvider).toHaveBeenCalledWith(mockProvider) + expect(fetchModelsSpy).toHaveBeenCalledWith(mockProvider) }) it('should clear models when switching to invalid provider', async () => { - mockFetchModelsFromProvider.mockResolvedValueOnce(mockModels) + fetchModelsSpy.mockResolvedValueOnce(mockModels) const { result, rerender } = renderHook( ({ provider }) => useProviderModels(provider), @@ -96,6 +98,6 @@ describe('useProviderModels', () => { result.current.refetch() - expect(mockFetchModelsFromProvider).not.toHaveBeenCalled() + expect(fetchModelsSpy).not.toHaveBeenCalled() }) }) \ No newline at end of file diff --git a/web-app/src/hooks/useProviderModels.ts b/web-app/src/hooks/useProviderModels.ts index ee5104133..1be17b706 100644 --- a/web-app/src/hooks/useProviderModels.ts +++ b/web-app/src/hooks/useProviderModels.ts @@ -1,5 +1,5 @@ -import { useState, useEffect, useCallback, useRef } from 'react' -import { fetchModelsFromProvider } from '@/services/providers' +import { useState, useEffect, useCallback, useRef, useMemo } from 'react' +import { WebProvidersService } from '../services/providers/web' type UseProviderModelsState = { models: string[] @@ -12,6 +12,7 @@ const modelsCache = new Map() const CACHE_DURATION = 5 * 60 * 1000 // 5 minutes export const useProviderModels = (provider?: ModelProvider): UseProviderModelsState => { + const providersService = useMemo(() => new WebProvidersService(), []) const [models, setModels] = useState([]) const [loading, setLoading] = useState(false) const [error, setError] = useState(null) @@ -50,7 +51,7 @@ export const useProviderModels = (provider?: ModelProvider): UseProviderModelsSt setError(null) try { - const fetchedModels = await fetchModelsFromProvider(provider) + const fetchedModels = await providersService.fetchModelsFromProvider(provider) if (currentRequestId !== requestIdRef.current) return const sortedModels = fetchedModels.sort((a, b) => a.localeCompare(b)) @@ -69,7 +70,7 @@ export const useProviderModels = (provider?: ModelProvider): UseProviderModelsSt } finally { if (currentRequestId === requestIdRef.current) setLoading(false) } - }, [provider]) + }, [provider, providersService]) const refetch = useCallback(() => { if (provider) { diff --git a/web-app/src/services/providers/web.ts b/web-app/src/services/providers/web.ts index 30fe71366..5ac1430da 100644 --- a/web-app/src/services/providers/web.ts +++ b/web-app/src/services/providers/web.ts @@ -138,9 +138,24 @@ export class WebProvidersService implements ProvidersService { }) if (!response.ok) { - throw new Error( - `Cannot connect to ${provider.provider} at ${provider.base_url}. Please check that the service is running and accessible.` - ) + // Provide more specific error messages based on status code + if (response.status === 401) { + throw new Error( + `Authentication failed: API key is required or invalid for ${provider.provider}` + ) + } else if (response.status === 403) { + throw new Error( + `Access forbidden: Check your API key permissions for ${provider.provider}` + ) + } else if (response.status === 404) { + throw new Error( + `Models endpoint not found for ${provider.provider}. Check the base URL configuration.` + ) + } else { + throw new Error( + `Failed to fetch models from ${provider.provider}: ${response.status} ${response.statusText}` + ) + } } const data = await response.json() @@ -170,13 +185,28 @@ export class WebProvidersService implements ProvidersService { } catch (error) { console.error('Error fetching models from provider:', error) + const structuredErrorPrefixes = [ + 'Authentication failed', + 'Access forbidden', + 'Models endpoint not found', + 'Failed to fetch models from' + ] + + if (error instanceof Error && + structuredErrorPrefixes.some(prefix => (error as Error).message.startsWith(prefix))) { + throw new Error(error.message) + } + // Provide helpful error message for any connection errors if (error instanceof Error && error.message.includes('Cannot connect')) { - throw error + throw new Error( + `Cannot connect to ${provider.provider} at ${provider.base_url}. Please check that the service is running and accessible.` + ) } - + + // Generic fallback throw new Error( - `Cannot connect to ${provider.provider} at ${provider.base_url}. Please check that the service is running and accessible.` + `Unexpected error while fetching models from ${provider.provider}: ${error instanceof Error ? error.message : 'Unknown error'}` ) } } From 9fcd9503e7b16399699d44954efdaa377d35ea9e Mon Sep 17 00:00:00 2001 From: lugnicca Date: Sat, 6 Sep 2025 17:42:54 +0200 Subject: [PATCH 14/15] fix: error on message with "fetch" --- web-app/src/services/providers/web.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web-app/src/services/providers/web.ts b/web-app/src/services/providers/web.ts index 5ac1430da..5ad426a11 100644 --- a/web-app/src/services/providers/web.ts +++ b/web-app/src/services/providers/web.ts @@ -198,7 +198,7 @@ export class WebProvidersService implements ProvidersService { } // Provide helpful error message for any connection errors - if (error instanceof Error && error.message.includes('Cannot connect')) { + if (error instanceof Error && error.message.includes('fetch')) { throw new Error( `Cannot connect to ${provider.provider} at ${provider.base_url}. Please check that the service is running and accessible.` ) From 2db9af94fa7321f98472577451ed3191bd85572e Mon Sep 17 00:00:00 2001 From: lugnicca Date: Mon, 8 Sep 2025 17:45:35 +0200 Subject: [PATCH 15/15] fix: use serviceHub to fetch models and fix error message on app --- .../hooks/__tests__/useProviderModels.test.ts | 25 ++++++----- web-app/src/hooks/useProviderModels.ts | 10 ++--- web-app/src/services/providers/tauri.ts | 43 ++++++++++++++++--- web-app/src/test/setup.ts | 1 + 4 files changed, 55 insertions(+), 24 deletions(-) diff --git a/web-app/src/hooks/__tests__/useProviderModels.test.ts b/web-app/src/hooks/__tests__/useProviderModels.test.ts index e6ca301f7..da9b60e07 100644 --- a/web-app/src/hooks/__tests__/useProviderModels.test.ts +++ b/web-app/src/hooks/__tests__/useProviderModels.test.ts @@ -1,9 +1,7 @@ import { describe, it, expect, beforeEach, vi } from 'vitest' import { renderHook, waitFor } from '@testing-library/react' import { useProviderModels } from '../useProviderModels' -import { WebProvidersService } from '../../services/providers/web' - -let fetchModelsSpy: ReturnType +import { useServiceHub } from '@/hooks/useServiceHub' // Local minimal provider type for tests type MockModelProvider = { @@ -27,13 +25,17 @@ describe('useProviderModels', () => { const mockModels = ['gpt-4', 'gpt-3.5-turbo', 'gpt-4-turbo'] + let fetchModelsSpy: ReturnType + beforeEach(() => { vi.restoreAllMocks() vi.clearAllMocks() - fetchModelsSpy = vi.spyOn( - WebProvidersService.prototype, - 'fetchModelsFromProvider' - ) + const hub = (useServiceHub as unknown as () => any)() + const mockedFetch = vi.fn() + vi.spyOn(hub, 'providers').mockReturnValue({ + fetchModelsFromProvider: mockedFetch, + } as any) + fetchModelsSpy = mockedFetch }) it('should initialize with empty state', () => { @@ -62,11 +64,9 @@ describe('useProviderModels', () => { const { result } = renderHook(() => useProviderModels(mockProvider)) await waitFor(() => { - expect(result.current.loading).toBe(false) + expect(result.current.models).toEqual(['gpt-3.5-turbo', 'gpt-4', 'gpt-4-turbo']) }) - // Should be sorted alphabetically - expect(result.current.models).toEqual(['gpt-3.5-turbo', 'gpt-4', 'gpt-4-turbo']) expect(result.current.error).toBe(null) expect(fetchModelsSpy).toHaveBeenCalledWith(mockProvider) }) @@ -80,10 +80,9 @@ describe('useProviderModels', () => { ) await waitFor(() => { + expect(result.current.models).toEqual(['gpt-3.5-turbo', 'gpt-4', 'gpt-4-turbo']) expect(result.current.loading).toBe(false) - }) - - expect(result.current.models).toEqual(['gpt-3.5-turbo', 'gpt-4', 'gpt-4-turbo']) + }, { timeout: 500 }) // Switch to invalid provider rerender({ provider: { ...mockProvider, base_url: undefined } }) diff --git a/web-app/src/hooks/useProviderModels.ts b/web-app/src/hooks/useProviderModels.ts index 1be17b706..3c51a7f70 100644 --- a/web-app/src/hooks/useProviderModels.ts +++ b/web-app/src/hooks/useProviderModels.ts @@ -1,5 +1,5 @@ -import { useState, useEffect, useCallback, useRef, useMemo } from 'react' -import { WebProvidersService } from '../services/providers/web' +import { useState, useEffect, useCallback, useRef } from 'react' +import { useServiceHub } from './useServiceHub' type UseProviderModelsState = { models: string[] @@ -12,7 +12,7 @@ const modelsCache = new Map() const CACHE_DURATION = 5 * 60 * 1000 // 5 minutes export const useProviderModels = (provider?: ModelProvider): UseProviderModelsState => { - const providersService = useMemo(() => new WebProvidersService(), []) + const serviceHub = useServiceHub() const [models, setModels] = useState([]) const [loading, setLoading] = useState(false) const [error, setError] = useState(null) @@ -51,7 +51,7 @@ export const useProviderModels = (provider?: ModelProvider): UseProviderModelsSt setError(null) try { - const fetchedModels = await providersService.fetchModelsFromProvider(provider) + const fetchedModels = await serviceHub.providers().fetchModelsFromProvider(provider) if (currentRequestId !== requestIdRef.current) return const sortedModels = fetchedModels.sort((a, b) => a.localeCompare(b)) @@ -70,7 +70,7 @@ export const useProviderModels = (provider?: ModelProvider): UseProviderModelsSt } finally { if (currentRequestId === requestIdRef.current) setLoading(false) } - }, [provider, providersService]) + }, [provider, serviceHub]) const refetch = useCallback(() => { if (provider) { diff --git a/web-app/src/services/providers/tauri.ts b/web-app/src/services/providers/tauri.ts index 5c5103b20..110794322 100644 --- a/web-app/src/services/providers/tauri.ts +++ b/web-app/src/services/providers/tauri.ts @@ -113,7 +113,7 @@ export class TauriProvidersService extends DefaultProvidersService { } return runtimeProviders.concat(builtinProviders as ModelProvider[]) - } catch (error) { + } catch (error: unknown) { console.error('Error getting providers in Tauri:', error) return [] } @@ -142,9 +142,24 @@ export class TauriProvidersService extends DefaultProvidersService { }) if (!response.ok) { - throw new Error( - `Failed to fetch models: ${response.status} ${response.statusText}` - ) + // Provide more specific error messages based on status code (aligned with web implementation) + if (response.status === 401) { + throw new Error( + `Authentication failed: API key is required or invalid for ${provider.provider}` + ) + } else if (response.status === 403) { + throw new Error( + `Access forbidden: Check your API key permissions for ${provider.provider}` + ) + } else if (response.status === 404) { + throw new Error( + `Models endpoint not found for ${provider.provider}. Check the base URL configuration.` + ) + } else { + throw new Error( + `Failed to fetch models from ${provider.provider}: ${response.status} ${response.statusText}` + ) + } } const data = await response.json() @@ -174,14 +189,30 @@ export class TauriProvidersService extends DefaultProvidersService { } catch (error) { console.error('Error fetching models from provider:', error) - // Provide helpful error message + // Preserve structured error messages thrown above + const structuredErrorPrefixes = [ + 'Authentication failed', + 'Access forbidden', + 'Models endpoint not found', + 'Failed to fetch models from' + ] + + if (error instanceof Error && + structuredErrorPrefixes.some(prefix => (error as Error).message.startsWith(prefix))) { + throw new Error(error.message) + } + + // Provide helpful error message for any connection errors if (error instanceof Error && error.message.includes('fetch')) { throw new Error( `Cannot connect to ${provider.provider} at ${provider.base_url}. Please check that the service is running and accessible.` ) } - throw error + // Generic fallback + throw new Error( + `Unexpected error while fetching models from ${provider.provider}: ${error instanceof Error ? error.message : 'Unknown error'}` + ) } } diff --git a/web-app/src/test/setup.ts b/web-app/src/test/setup.ts index a542ffec9..cf93dba6b 100644 --- a/web-app/src/test/setup.ts +++ b/web-app/src/test/setup.ts @@ -103,6 +103,7 @@ const mockServiceHub = { deleteProvider: vi.fn().mockResolvedValue(undefined), updateProvider: vi.fn().mockResolvedValue(undefined), getProvider: vi.fn().mockResolvedValue(null), + fetchModelsFromProvider: vi.fn().mockResolvedValue([]), }), models: () => ({ getModels: vi.fn().mockResolvedValue([]),