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

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 { render } from '@testing-library/react'
-import userEvent from '@testing-library/user-event'
 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 ? <div data-testid="dialog">{children}</div> : null,
-  DialogContent: ({ children }: { children: React.ReactNode }) => (
+  DialogContent: ({ children, onKeyDown }: { children: React.ReactNode; onKeyDown?: (e: React.KeyboardEvent) => void }) => (
-    <div data-testid="dialog-content">{children}</div>
+    <div data-testid="dialog-content" onKeyDown={onKeyDown}>{children}</div>
   ),
   DialogHeader: ({ children }: { children: React.ReactNode }) => (
     <div data-testid="dialog-header">{children}</div>
@@ -181,4 +180,67 @@ describe('DialogEditModel - Basic Component Tests', () => {
     expect(mockUpdateProvider).toBeDefined()
     expect(mockSetProviders).toBeDefined()
   })
+
+  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(
+      <DialogEditModel
+        provider={providerWithCaps}
+        modelId="test-model.gguf"
+      />
+    )
+
+    // Should render without issues - capabilities helper function should work
+    expect(container).toBeInTheDocument()
+  })
+
+  it('should handle Enter key press with keyDown handler', () => {
+    const { container } = render(
+      <DialogEditModel
+        provider={mockProvider}
+        modelId="test-model.gguf"
+      />
+    )
+
+    // 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(
+      <DialogEditModel
+        provider={providerWithAllCaps}
+        modelId="test-model.gguf"
+      />
+    )
+
+    // Component should render without errors even with extra capabilities
+    // The capabilities helper should only extract vision and tools
+    expect(container).toBeInTheDocument()
+  })
 })

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<Record<string, boolean>>({
-    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 && !selectedModelId || !isOpen) {
-    if (!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, selectedModelId, provider.models])
-  }, [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])
 
   // 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({
+      const capsObject = capabilitiesToObject(modelCapabilities)
-        completion: modelCapabilities.includes('completion'),
+
-        vision: modelCapabilities.includes('vision'),
+      setCapabilities(capsObject)
-        tools: modelCapabilities.includes('tools'),
+      setOriginalCapabilities(capsObject)
-        embeddings: modelCapabilities.includes('embeddings'),
+
-        web_search: modelCapabilities.includes('web_search'),
-        reasoning: modelCapabilities.includes('reasoning'),
-      })
       // 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
+      // Build the update object for the selected model
-      if (displayName !== originalDisplayName) {
+      const modelUpdate: Partial<Model> & { _userConfiguredCapabilities?: boolean } = {}
-        // Update the model in the provider models array with displayName
+
-        updatedModels = updatedModels.map((m: Model) => {
+      if (nameChanged) {
-          if (m.id === selectedModelId) {
+        modelUpdate.displayName = displayName
-            return {
-              ...m,
-              displayName: displayName,
-            }
-          }
-          return m
-        })
-        setOriginalDisplayName(displayName)
       }
 
-      // Update capabilities if changed
+      if (capabilitiesChanged) {
-      if (
+        modelUpdate.capabilities = Object.entries(capabilities)
-        JSON.stringify(capabilities) !== JSON.stringify(originalCapabilities)
-      ) {
-        const updatedCapabilities = Object.entries(capabilities)
           .filter(([, isEnabled]) => isEnabled)
           .map(([capName]) => capName)
-
+        modelUpdate._userConfiguredCapabilities = true
-        // 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)
       }
 
+      // 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 (
-    <Dialog open={isOpen} onOpenChange={setIsOpen}>
+    <Dialog open={isOpen} onOpenChange={handleDialogChange}>
       <DialogTrigger asChild>
         <div className="size-6 cursor-pointer flex items-center justify-center rounded hover:bg-main-view-fg/10 transition-all duration-200 ease-in-out">
           <IconPencil size={18} className="text-main-view-fg/50" />
         </div>
       </DialogTrigger>
-      <DialogContent>
+      <DialogContent onKeyDown={handleKeyDown}>
         <DialogHeader>
           <DialogTitle className="line-clamp-1" title={selectedModel.id}>
             {t('providers:editModel.title', { modelId: selectedModel.id })}
@@ -292,58 +268,6 @@ export const DialogEditModel = ({
                 disabled={isLoading}
               />
             </div>
-
-            {/* <div className="flex items-center justify-between">
-              <div className="flex items-center space-x-2">
-                <IconCodeCircle2 className="size-4 text-main-view-fg/70" />
-                <span className="text-sm">
-                  {t('providers:editModel.embeddings')}
-                </span>
-              </div>
-              <Tooltip>
-                <TooltipTrigger>
-                  <Switch
-                    id="embedding-capability"
-                    disabled={true}
-                    checked={capabilities.embeddings}
-                    onCheckedChange={(checked) =>
-                      handleCapabilityChange('embeddings', checked)
-                    }
-                  />
-                </TooltipTrigger>
-                <TooltipContent>
-                  {t('providers:editModel.notAvailable')}
-                </TooltipContent>
-              </Tooltip>
-            </div> */}
-
-            {/* <div className="flex items-center justify-between">
-              <div className="flex items-center space-x-2">
-                <IconWorld className="size-4 text-main-view-fg/70" />
-                <span className="text-sm">Web Search</span>
-              </div>
-              <Switch
-                id="web_search-capability"
-                checked={capabilities.web_search}
-                onCheckedChange={(checked) =>
-                  handleCapabilityChange('web_search', checked)
-                }
-              />
-            </div> */}
-
-            {/* <div className="flex items-center justify-between">
-              <div className="flex items-center space-x-2">
-                <IconAtom className="size-4 text-main-view-fg/70" />
-                <span className="text-sm">{t('reasoning')}</span>
-              </div>
-              <Switch
-                id="reasoning-capability"
-                checked={capabilities.reasoning}
-                onCheckedChange={(checked) =>
-                  handleCapabilityChange('reasoning', checked)
-                }
-              />
-            </div> */}
           </div>
         </div>