refactor(ProviderSettings): streamline provider cleanup logic

- Removed inline cleanup function and utilized a dedicated utility to manage provider data.
- Enhanced the cleanup process to return both cleaned providers and a change flag for better state management.
- Simplified the useEffect hook for improved readability and maintainability.

src/renderer/src/pages/settings/ProviderSettings/ProviderSetting.tsx

@@ -3,7 +3,7 @@ import { LoadingIcon } from '@renderer/components/Icons'
 import { HStack } from '@renderer/components/Layout'
 import { ApiKeyListPopup } from '@renderer/components/Popups/ApiKeyListPopup'
 import { isEmbeddingModel, isRerankModel } from '@renderer/config/models'
-import { PROVIDER_URLS, SYSTEM_PROVIDERS_CONFIG } from '@renderer/config/providers'
+import { PROVIDER_URLS } from '@renderer/config/providers'
 import { useTheme } from '@renderer/context/ThemeProvider'
 import { useAllProviders, useProvider, useProviders } from '@renderer/hooks/useProvider'
 import { useTimer } from '@renderer/hooks/useTimer'
@@ -11,10 +11,11 @@ import i18n from '@renderer/i18n'
 import { ModelList } from '@renderer/pages/settings/ProviderSettings/ModelList'
 import { checkApi } from '@renderer/services/ApiService'
 import { isProviderSupportAuth } from '@renderer/services/ProviderService'
-import { isSystemProvider, Provider, SystemProviderId, SystemProviderIds } from '@renderer/types'
+import { isSystemProvider } from '@renderer/types'
 import { ApiKeyConnectivity, HealthStatus } from '@renderer/types/healthCheck'
 import { formatApiHost, formatApiKeys, getFancyProviderName, isOpenAIProvider } from '@renderer/utils'
 import { formatErrorMessage } from '@renderer/utils/error'
+import { cleanupProviders } from '@renderer/utils/provider'
 import { Button, Divider, Flex, Input, Space, Switch, Tooltip } from 'antd'
 import Link from 'antd/es/typography/Link'
 import { debounce, isEmpty } from 'lodash'
@@ -227,47 +228,11 @@ const ProviderSetting: FC<Props> = ({ providerId }) => {
 
   // Clean up provider data on component mount - remove duplicates and add missing system providers
   useEffect(() => {
-    const cleanupProviders = () => {
-      const currentProviders = allProviders
-      const systemProviderIds = Object.keys(SystemProviderIds) as SystemProviderId[]
+    const { cleanedProviders, hasChanges } = cleanupProviders(allProviders)
 
-      // Find duplicates (same id appears multiple times)
-      const seenIds = new Set<string>()
-      const duplicateIds = new Set<string>()
-      currentProviders.forEach((p) => {
-        if (seenIds.has(p.id)) {
-          duplicateIds.add(p.id)
-        }
-        seenIds.add(p.id)
-      })
-
-      const cleanedProviders: Provider[] = []
-      const processedIds = new Set<string>()
-
-      currentProviders.forEach((p) => {
-        if (!processedIds.has(p.id)) {
-          cleanedProviders.push(p)
-          processedIds.add(p.id)
-        }
-      })
-
-      // Find missing system providers
-      const existingProviderIds = cleanedProviders.map((p) => p.id)
-      const missingSystemProviderIds = systemProviderIds.filter((id) => !existingProviderIds.includes(id))
-
-      // Add missing system providers
-      missingSystemProviderIds.forEach((id: SystemProviderId) => {
-        const systemProvider = SYSTEM_PROVIDERS_CONFIG[id]
-        cleanedProviders.push({ ...systemProvider })
-      })
-
-      // Update providers if there were changes
-      if (duplicateIds.size > 0 || missingSystemProviderIds.length > 0) {
-        updateProviders(cleanedProviders)
-      }
+    if (hasChanges) {
+      updateProviders(cleanedProviders)
     }
-
-    cleanupProviders()
     // eslint-disable-next-line react-hooks/exhaustive-deps
   }, []) // Empty dependency array to run only on mount
 

src/renderer/src/utils/__tests__/provider.test.ts

@@ -0,0 +1,200 @@
+import { OpenAIServiceTiers, Provider, SystemProvider } from '@renderer/types'
+import { describe, expect, it, vi } from 'vitest'
+
+import { cleanupProviders } from '../provider'
+
+// Mock the config to avoid dependency issues in tests
+vi.mock('@renderer/config/providers', () => ({
+  SYSTEM_PROVIDERS_CONFIG: {
+    openai: {
+      id: 'openai',
+      name: 'OpenAI',
+      type: 'openai-response',
+      apiKey: '',
+      apiHost: 'https://api.openai.com',
+      models: [],
+      isSystem: true,
+      enabled: false,
+      serviceTier: 'auto'
+    },
+    anthropic: {
+      id: 'anthropic',
+      name: 'Anthropic',
+      type: 'anthropic',
+      apiKey: '',
+      apiHost: 'https://api.anthropic.com/',
+      models: [],
+      isSystem: true,
+      enabled: false
+    }
+  }
+}))
+
+// Mock SystemProviderIds to avoid dependency issues
+vi.mock('@renderer/types', async () => {
+  const actual = await vi.importActual('@renderer/types')
+  return {
+    ...actual,
+    SystemProviderIds: {
+      openai: 'openai',
+      anthropic: 'anthropic'
+    }
+  }
+})
+
+// Mock system providers for testing
+const mockSystemProvider1: SystemProvider = {
+  id: 'openai',
+  name: 'OpenAI',
+  type: 'openai-response',
+  apiKey: '',
+  apiHost: 'https://api.openai.com',
+  models: [],
+  isSystem: true,
+  enabled: false,
+  serviceTier: OpenAIServiceTiers.auto
+}
+
+const mockSystemProvider2: SystemProvider = {
+  id: 'anthropic',
+  name: 'Anthropic',
+  type: 'anthropic',
+  apiKey: '',
+  apiHost: 'https://api.anthropic.com/',
+  models: [],
+  isSystem: true,
+  enabled: false
+}
+
+const mockCustomProvider: Provider = {
+  id: 'custom-provider',
+  name: 'Custom Provider',
+  type: 'openai',
+  apiKey: 'custom-key',
+  apiHost: 'https://custom.com',
+  models: [],
+  enabled: true
+}
+
+describe('cleanupProviders', () => {
+  it('should return original providers when no duplicates or missing providers', () => {
+    const providers = [mockCustomProvider]
+    const result = cleanupProviders(providers)
+
+    expect(result.cleanedProviders).toHaveLength(3) // 1 custom + 2 mocked system providers
+    expect(result.hasChanges).toBe(true) // Should be true because missing system providers were added
+  })
+
+  it('should remove duplicate providers', () => {
+    const duplicate1 = { ...mockSystemProvider1, apiKey: '' }
+    const duplicate2 = { ...mockSystemProvider1, apiKey: 'test-key' }
+    const providers = [duplicate1, duplicate2]
+
+    const result = cleanupProviders(providers)
+
+    // Should keep the one with apiKey
+    const openaiProvider = result.cleanedProviders.find((p) => p.id === 'openai')
+    expect(openaiProvider?.apiKey).toBe('test-key')
+    expect(result.hasChanges).toBe(true)
+  })
+
+  it('should prefer enabled provider when both have empty apiKey', () => {
+    const duplicate1 = { ...mockSystemProvider1, apiKey: '', enabled: false }
+    const duplicate2 = { ...mockSystemProvider1, apiKey: '', enabled: true }
+    const providers = [duplicate1, duplicate2]
+
+    const result = cleanupProviders(providers)
+
+    const openaiProvider = result.cleanedProviders.find((p) => p.id === 'openai')
+    expect(openaiProvider?.enabled).toBe(true)
+    expect(result.hasChanges).toBe(true)
+  })
+
+  it('should keep first occurrence when both have same priority', () => {
+    const duplicate1 = { ...mockSystemProvider1, apiKey: '', enabled: false, name: 'First' }
+    const duplicate2 = { ...mockSystemProvider1, apiKey: '', enabled: false, name: 'Second' }
+    const providers = [duplicate1, duplicate2]
+
+    const result = cleanupProviders(providers)
+
+    const openaiProvider = result.cleanedProviders.find((p) => p.id === 'openai')
+    expect(openaiProvider?.name).toBe('First')
+    expect(result.hasChanges).toBe(true)
+  })
+
+  it('should add missing system providers', () => {
+    const providers: Provider[] = []
+    const result = cleanupProviders(providers)
+
+    expect(result.cleanedProviders.length).toBe(2) // 2 mocked system providers
+    expect(result.hasChanges).toBe(true)
+  })
+
+  it('should preserve custom providers', () => {
+    const providers = [mockCustomProvider]
+    const result = cleanupProviders(providers)
+
+    const customProvider = result.cleanedProviders.find((p) => p.id === 'custom-provider')
+    expect(customProvider).toEqual(mockCustomProvider)
+    expect(result.hasChanges).toBe(true) // Because system providers were added
+  })
+
+  it('should handle mixed duplicates correctly', () => {
+    const systemDuplicate1 = { ...mockSystemProvider1, apiKey: '', enabled: false }
+    const systemDuplicate2 = { ...mockSystemProvider1, apiKey: 'test-key', enabled: false }
+    const systemDuplicate3 = { ...mockSystemProvider2, apiKey: '', enabled: true }
+    const systemDuplicate4 = { ...mockSystemProvider2, apiKey: '', enabled: false }
+
+    const providers = [mockCustomProvider, systemDuplicate1, systemDuplicate2, systemDuplicate3, systemDuplicate4]
+
+    const result = cleanupProviders(providers)
+
+    // Should keep custom provider
+    expect(result.cleanedProviders.find((p) => p.id === 'custom-provider')).toEqual(mockCustomProvider)
+
+    // Should keep system provider with apiKey
+    const openaiProvider = result.cleanedProviders.find((p) => p.id === 'openai')
+    expect(openaiProvider?.apiKey).toBe('test-key')
+
+    // Should keep enabled system provider
+    const anthropicProvider = result.cleanedProviders.find((p) => p.id === 'anthropic')
+    expect(anthropicProvider?.enabled).toBe(true)
+
+    expect(result.hasChanges).toBe(true)
+  })
+
+  it('should handle non-system provider duplicates', () => {
+    const custom1 = { ...mockCustomProvider, name: 'First Custom' }
+    const custom2 = { ...mockCustomProvider, name: 'Second Custom' }
+    const providers = [custom1, custom2]
+
+    const result = cleanupProviders(providers)
+
+    // Should keep first occurrence of custom provider
+    const customProvider = result.cleanedProviders.find((p) => p.id === 'custom-provider')
+    expect(customProvider?.name).toBe('First Custom')
+    expect(result.hasChanges).toBe(true)
+  })
+
+  it('should return hasChanges false when no changes needed', () => {
+    const providers = [mockSystemProvider1, mockSystemProvider2]
+
+    const result = cleanupProviders(providers)
+
+    expect(result.hasChanges).toBe(false)
+    expect(result.cleanedProviders).toHaveLength(2)
+  })
+
+  it('should prioritize apiKey over enabled status', () => {
+    const duplicate1 = { ...mockSystemProvider1, apiKey: '', enabled: true }
+    const duplicate2 = { ...mockSystemProvider1, apiKey: 'test-key', enabled: false }
+    const providers = [duplicate1, duplicate2]
+
+    const result = cleanupProviders(providers)
+
+    const openaiProvider = result.cleanedProviders.find((p) => p.id === 'openai')
+    expect(openaiProvider?.apiKey).toBe('test-key')
+    expect(openaiProvider?.enabled).toBe(false)
+    expect(result.hasChanges).toBe(true)
+  })
+})

src/renderer/src/utils/provider.ts

@@ -0,0 +1,87 @@
+import { SYSTEM_PROVIDERS_CONFIG } from '@renderer/config/providers'
+import { isSystemProvider, Provider, SystemProviderId, SystemProviderIds } from '@renderer/types'
+
+/**
+ * Clean up provider data by removing duplicates and adding missing system providers
+ *
+ * Priority rules for duplicate system providers:
+ * 1. Provider with non-empty apiKey takes precedence
+ * 2. If both have empty apiKey, enabled provider takes precedence
+ * 3. Otherwise, keep the first occurrence
+ *
+ * @param providers - Array of providers to clean up
+ * @returns Object containing cleaned providers and whether changes were made
+ */
+export function cleanupProviders(providers: Provider[]): {
+  cleanedProviders: Provider[]
+  hasChanges: boolean
+} {
+  const systemProviderIds = Object.keys(SystemProviderIds) as SystemProviderId[]
+  const cleanedProviders: Provider[] = []
+  const processedIds = new Set<string>()
+  let hasChanges = false
+
+  // Remove duplicates with priority logic
+  providers.forEach((p) => {
+    if (!processedIds.has(p.id)) {
+      cleanedProviders.push(p)
+      processedIds.add(p.id)
+      return
+    }
+
+    // Handle duplicate system providers
+    if (!isSystemProvider(p)) {
+      hasChanges = true // Found duplicate non-system provider (should not happen, but mark as change)
+      return
+    }
+
+    const existingIndex = cleanedProviders.findIndex((existing) => existing.id === p.id)
+    if (existingIndex === -1) return
+
+    const existingProvider = cleanedProviders[existingIndex]
+    if (shouldReplaceProvider(p, existingProvider)) {
+      cleanedProviders[existingIndex] = p
+      hasChanges = true
+    } else {
+      hasChanges = true // Found duplicate but didn't replace
+    }
+  })
+
+  // Add missing system providers
+  const existingProviderIds = cleanedProviders.map((p) => p.id)
+  const missingSystemProviderIds = systemProviderIds.filter((id) => !existingProviderIds.includes(id))
+
+  missingSystemProviderIds.forEach((id: SystemProviderId) => {
+    const systemProvider = SYSTEM_PROVIDERS_CONFIG[id]
+    cleanedProviders.push({ ...systemProvider })
+    hasChanges = true
+  })
+
+  return {
+    cleanedProviders,
+    hasChanges
+  }
+}
+
+/**
+ * Determine if current provider should replace existing provider
+ *
+ * @param current - Current provider being evaluated
+ * @param existing - Existing provider in the list
+ * @returns true if current should replace existing
+ */
+function shouldReplaceProvider(current: Provider, existing: Provider): boolean {
+  const currentHasApiKey = current.apiKey && current.apiKey.trim() !== ''
+  const existingHasApiKey = existing.apiKey && existing.apiKey.trim() !== ''
+
+  // Priority: 1) has apiKey, 2) is enabled
+  if (currentHasApiKey && !existingHasApiKey) {
+    return true
+  }
+
+  if (!currentHasApiKey && !existingHasApiKey && current.enabled && !existing.enabled) {
+    return true
+  }
+
+  return false
+}