From 87db633b7db120cb45738742ceb253709b68816b Mon Sep 17 00:00:00 2001 From: Nghia Doan Date: Thu, 2 Oct 2025 08:58:48 +0700 Subject: [PATCH] Merge pull request #6700 from menloresearch/fix/edit-model-name fix: Fix editing model without saving should restore original name # Conflicts: # web-app/src/containers/__tests__/EditModel.test.tsx --- .../containers/__tests__/EditModel.test.tsx | 72 ++++++- web-app/src/containers/dialogs/EditModel.tsx | 180 +++++------------- 2 files changed, 119 insertions(+), 133 deletions(-) diff --git a/web-app/src/containers/__tests__/EditModel.test.tsx b/web-app/src/containers/__tests__/EditModel.test.tsx index a02e72476..6c0dfd059 100644 --- a/web-app/src/containers/__tests__/EditModel.test.tsx +++ b/web-app/src/containers/__tests__/EditModel.test.tsx @@ -1,6 +1,5 @@ import { describe, it, expect, beforeEach, vi } from 'vitest' -import { render, screen, fireEvent, waitFor } from '@testing-library/react' -import userEvent from '@testing-library/user-event' +import { render } from '@testing-library/react' import { DialogEditModel } from '../dialogs/EditModel' import { useModelProvider } from '@/hooks/useModelProvider' import '@testing-library/jest-dom' @@ -38,8 +37,8 @@ vi.mock('sonner', () => ({ vi.mock('@/components/ui/dialog', () => ({ Dialog: ({ children, open }: { children: React.ReactNode; open: boolean }) => open ?
{children}
: null, - DialogContent: ({ children }: { children: React.ReactNode }) => ( -
{children}
+ DialogContent: ({ children, onKeyDown }: { children: React.ReactNode; onKeyDown?: (e: React.KeyboardEvent) => void }) => ( +
{children}
), DialogHeader: ({ children }: { children: React.ReactNode }) => (
{children}
@@ -181,4 +180,67 @@ describe('DialogEditModel - Basic Component Tests', () => { expect(mockUpdateProvider).toBeDefined() expect(mockSetProviders).toBeDefined() }) -}) \ No newline at end of file + + it('should consolidate capabilities initialization without duplication', () => { + const providerWithCaps = { + provider: 'llamacpp', + active: true, + models: [ + { + id: 'test-model.gguf', + displayName: 'Test Model', + capabilities: ['vision', 'tools'], + }, + ], + settings: [], + } as any + + const { container } = render( + + ) + + // Should render without issues - capabilities helper function should work + expect(container).toBeInTheDocument() + }) + + it('should handle Enter key press with keyDown handler', () => { + const { container } = render( + + ) + + // Component should render with keyDown handler + expect(container).toBeInTheDocument() + }) + + it('should handle vision and tools capabilities', () => { + const providerWithAllCaps = { + provider: 'llamacpp', + active: true, + models: [ + { + id: 'test-model.gguf', + displayName: 'Test Model', + capabilities: ['vision', 'tools', 'completion', 'embeddings', 'web_search', 'reasoning'], + }, + ], + settings: [], + } as any + + const { container } = render( + + ) + + // Component should render without errors even with extra capabilities + // The capabilities helper should only extract vision and tools + expect(container).toBeInTheDocument() + }) +}) diff --git a/web-app/src/containers/dialogs/EditModel.tsx b/web-app/src/containers/dialogs/EditModel.tsx index 67576fbd6..f7dec06eb 100644 --- a/web-app/src/containers/dialogs/EditModel.tsx +++ b/web-app/src/containers/dialogs/EditModel.tsx @@ -17,9 +17,6 @@ import { IconTool, IconAlertTriangle, IconLoader2, - // IconWorld, - // IconAtom, - // IconCodeCircle2, } from '@tabler/icons-react' import { useState, useEffect } from 'react' import { useTranslation } from '@/i18n/react-i18next-compat' @@ -46,69 +43,45 @@ export const DialogEditModel = ({ const [isOpen, setIsOpen] = useState(false) const [isLoading, setIsLoading] = useState(false) const [capabilities, setCapabilities] = useState>({ - completion: false, vision: false, tools: false, - reasoning: false, - embeddings: false, - web_search: false, }) // Initialize with the provided model ID or the first model if available useEffect(() => { - // Only set the selected model ID if the dialog is not open to prevent switching during downloads - if (!isOpen) { + if (isOpen && !selectedModelId || !isOpen) { if (modelId) { setSelectedModelId(modelId) } else if (provider.models && provider.models.length > 0) { setSelectedModelId(provider.models[0].id) } } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [modelId, isOpen]) // Add isOpen dependency to prevent switching when dialog is open - - // Handle dialog opening - set the initial model selection - useEffect(() => { - if (isOpen && !selectedModelId) { - if (modelId) { - setSelectedModelId(modelId) - } else if (provider.models && provider.models.length > 0) { - setSelectedModelId(provider.models[0].id) - } - } - }, [isOpen, selectedModelId, modelId, provider.models]) + }, [modelId, isOpen, selectedModelId, provider.models]) // Get the currently selected model const selectedModel = provider.models.find( (m: Model) => m.id === selectedModelId ) + // Helper function to convert capabilities array to object + const capabilitiesToObject = (capabilitiesList: string[]) => ({ + vision: capabilitiesList.includes('vision'), + tools: capabilitiesList.includes('tools'), + }) + // Initialize capabilities and display name from selected model useEffect(() => { if (selectedModel) { const modelCapabilities = selectedModel.capabilities || [] - setCapabilities({ - completion: modelCapabilities.includes('completion'), - vision: modelCapabilities.includes('vision'), - tools: modelCapabilities.includes('tools'), - embeddings: modelCapabilities.includes('embeddings'), - web_search: modelCapabilities.includes('web_search'), - reasoning: modelCapabilities.includes('reasoning'), - }) + const capsObject = capabilitiesToObject(modelCapabilities) + + setCapabilities(capsObject) + setOriginalCapabilities(capsObject) + // Use existing displayName if available, otherwise fall back to model ID const displayNameValue = (selectedModel as Model & { displayName?: string }).displayName || selectedModel.id setDisplayName(displayNameValue) setOriginalDisplayName(displayNameValue) - - const originalCaps = { - completion: modelCapabilities.includes('completion'), - vision: modelCapabilities.includes('vision'), - tools: modelCapabilities.includes('tools'), - embeddings: modelCapabilities.includes('embeddings'), - web_search: modelCapabilities.includes('web_search'), - reasoning: modelCapabilities.includes('reasoning'), - } - setOriginalCapabilities(originalCaps) } }, [selectedModel]) @@ -139,53 +112,38 @@ export const DialogEditModel = ({ setIsLoading(true) try { - let updatedModels = provider.models + const nameChanged = displayName !== originalDisplayName + const capabilitiesChanged = JSON.stringify(capabilities) !== JSON.stringify(originalCapabilities) - // Update display name if changed - if (displayName !== originalDisplayName) { - // Update the model in the provider models array with displayName - updatedModels = updatedModels.map((m: Model) => { - if (m.id === selectedModelId) { - return { - ...m, - displayName: displayName, - } - } - return m - }) - setOriginalDisplayName(displayName) + // Build the update object for the selected model + const modelUpdate: Partial & { _userConfiguredCapabilities?: boolean } = {} + + if (nameChanged) { + modelUpdate.displayName = displayName } - // Update capabilities if changed - if ( - JSON.stringify(capabilities) !== JSON.stringify(originalCapabilities) - ) { - const updatedCapabilities = Object.entries(capabilities) + if (capabilitiesChanged) { + modelUpdate.capabilities = Object.entries(capabilities) .filter(([, isEnabled]) => isEnabled) .map(([capName]) => capName) - - // Find and update the model in the provider - updatedModels = updatedModels.map((m: Model) => { - if (m.id === selectedModelId) { - return { - ...m, - capabilities: updatedCapabilities, - // Mark that user has manually configured capabilities - _userConfiguredCapabilities: true, - } - } - return m - }) - - setOriginalCapabilities(capabilities) + modelUpdate._userConfiguredCapabilities = true } + // Update the model in the provider models array + const updatedModels = provider.models.map((m: Model) => + m.id === selectedModelId ? { ...m, ...modelUpdate } : m + ) + // Update the provider with the updated models updateProvider(provider.provider, { ...provider, models: updatedModels, }) + // Update original values + if (nameChanged) setOriginalDisplayName(displayName) + if (capabilitiesChanged) setOriginalCapabilities(capabilities) + // Show success toast and close dialog toast.success('Model updated successfully') setIsOpen(false) @@ -201,14 +159,32 @@ export const DialogEditModel = ({ return null } + // Handle dialog close - reset to original values if not saved + const handleDialogChange = (open: boolean) => { + if (!open && hasUnsavedChanges()) { + // Reset to original values when closing without saving + setDisplayName(originalDisplayName) + setCapabilities(originalCapabilities) + } + setIsOpen(open) + } + + // Handle keyboard events for Enter key + const handleKeyDown = (e: React.KeyboardEvent) => { + if (e.key === 'Enter' && hasUnsavedChanges() && !isLoading) { + e.preventDefault() + handleSaveChanges() + } + } + return ( - +
- + {t('providers:editModel.title', { modelId: selectedModel.id })} @@ -292,58 +268,6 @@ export const DialogEditModel = ({ disabled={isLoading} /> - - {/*
-
- - - {t('providers:editModel.embeddings')} - -
- - - - handleCapabilityChange('embeddings', checked) - } - /> - - - {t('providers:editModel.notAvailable')} - - -
*/} - - {/*
-
- - Web Search -
- - handleCapabilityChange('web_search', checked) - } - /> -
*/} - - {/*
-
- - {t('reasoning')} -
- - handleCapabilityChange('reasoning', checked) - } - /> -
*/}