From 773d8dd4c3e598e4ee6583b1a28b7e71b2c380d5 Mon Sep 17 00:00:00 2001 From: suyao Date: Wed, 27 Aug 2025 14:33:53 +0800 Subject: [PATCH] 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. --- .../ProviderSettings/ProviderSetting.tsx | 47 +--- .../src/utils/__tests__/provider.test.ts | 200 ++++++++++++++++++ src/renderer/src/utils/provider.ts | 87 ++++++++ 3 files changed, 293 insertions(+), 41 deletions(-) create mode 100644 src/renderer/src/utils/__tests__/provider.test.ts create mode 100644 src/renderer/src/utils/provider.ts diff --git a/src/renderer/src/pages/settings/ProviderSettings/ProviderSetting.tsx b/src/renderer/src/pages/settings/ProviderSettings/ProviderSetting.tsx index 2fa9f7eaa7..50677d81f0 100644 --- a/src/renderer/src/pages/settings/ProviderSettings/ProviderSetting.tsx +++ b/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 = ({ 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() - const duplicateIds = new Set() - currentProviders.forEach((p) => { - if (seenIds.has(p.id)) { - duplicateIds.add(p.id) - } - seenIds.add(p.id) - }) - - const cleanedProviders: Provider[] = [] - const processedIds = new Set() - - 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 diff --git a/src/renderer/src/utils/__tests__/provider.test.ts b/src/renderer/src/utils/__tests__/provider.test.ts new file mode 100644 index 0000000000..9fd24ed573 --- /dev/null +++ b/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) + }) +}) diff --git a/src/renderer/src/utils/provider.ts b/src/renderer/src/utils/provider.ts new file mode 100644 index 0000000000..50d74b8308 --- /dev/null +++ b/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() + 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 +}