From 76d48f9ccb40d5e4a76e54533971240e999c8d78 Mon Sep 17 00:00:00 2001 From: suyao Date: Sat, 29 Nov 2025 19:39:15 +0800 Subject: [PATCH] fix: address PR review issues for Volcengine integration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix region field being ignored: pass user-configured region to listFoundationModels and listEndpoints - Add user notification before silent fallback when API fails - Throw error on credential corruption instead of returning null - Remove redundant credentials (accessKeyId, secretAccessKey) from Redux store (they're securely stored via safeStorage) - Add warnings field to ListModelsResult for partial API failures - Fix Redux/IPC order: save to secure storage first, then update Redux on success - Update related tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/main/services/VolcengineService.ts | 49 ++++++++++++------ src/preload/index.ts | 6 ++- .../clients/volcengine/VolcengineAPIClient.ts | 15 +++++- src/renderer/src/hooks/useVolcengine.ts | 17 +------ .../ProviderSettings/VolcengineSettings.tsx | 51 +++++-------------- .../src/services/__tests__/ApiService.test.ts | 2 - src/renderer/src/store/llm.ts | 12 ----- 7 files changed, 66 insertions(+), 86 deletions(-) diff --git a/src/main/services/VolcengineService.ts b/src/main/services/VolcengineService.ts index 8f67383f65..041e9cf650 100644 --- a/src/main/services/VolcengineService.ts +++ b/src/main/services/VolcengineService.ts @@ -118,6 +118,7 @@ interface ModelInfo { interface ListModelsResult { models: ModelInfo[] total?: number + warnings?: string[] } // Custom error class @@ -401,19 +402,23 @@ class VolcengineService { /** * Load credentials from encrypted storage + * @throws VolcengineServiceError if credentials file exists but is corrupted */ private async loadCredentials(): Promise { - try { - if (!fs.existsSync(this.credentialsFilePath)) { - return null - } + if (!fs.existsSync(this.credentialsFilePath)) { + return null + } + try { const encryptedData = await fs.promises.readFile(this.credentialsFilePath) const decryptedJson = safeStorage.decryptString(Buffer.from(encryptedData)) return JSON.parse(decryptedJson) as VolcengineCredentials } catch (error) { logger.error('Failed to load Volcengine credentials:', error as Error) - return null + throw new VolcengineServiceError( + 'Credentials file exists but could not be loaded. Please re-enter your credentials.', + error + ) } } @@ -521,7 +526,7 @@ class VolcengineService { /** * List foundation models from Volcengine ARK */ - private async listFoundationModels(): Promise { + private async listFoundationModels(region: string = CONFIG.DEFAULT_REGION): Promise { const requestBody: ListFoundationModelsRequest = { PageNumber: 1, PageSize: CONFIG.DEFAULT_PAGE_SIZE @@ -536,7 +541,7 @@ class VolcengineService { {}, requestBody, CONFIG.SERVICE_NAME, - CONFIG.DEFAULT_REGION + region ) return ListFoundationModelsResponseSchema.parse(response) @@ -545,7 +550,10 @@ class VolcengineService { /** * List user-created endpoints from Volcengine ARK */ - private async listEndpoints(projectName?: string): Promise { + private async listEndpoints( + projectName?: string, + region: string = CONFIG.DEFAULT_REGION + ): Promise { const requestBody: ListEndpointsRequest = { ProjectName: projectName || 'default', PageNumber: 1, @@ -561,7 +569,7 @@ class VolcengineService { {}, requestBody, CONFIG.SERVICE_NAME, - CONFIG.DEFAULT_REGION + region ) return ListEndpointsResponseSchema.parse(response) @@ -571,14 +579,20 @@ class VolcengineService { * List all available models from Volcengine ARK * Combines foundation models and user-created endpoints */ - public listModels = async (_?: Electron.IpcMainInvokeEvent, projectName?: string): Promise => { + public listModels = async ( + _?: Electron.IpcMainInvokeEvent, + projectName?: string, + region?: string + ): Promise => { try { + const effectiveRegion = region || CONFIG.DEFAULT_REGION const [foundationModelsResult, endpointsResult] = await Promise.allSettled([ - this.listFoundationModels(), - this.listEndpoints(projectName) + this.listFoundationModels(effectiveRegion), + this.listEndpoints(projectName, effectiveRegion) ]) const models: ModelInfo[] = [] + const warnings: string[] = [] if (foundationModelsResult.status === 'fulfilled') { const foundationModels = foundationModelsResult.value @@ -591,7 +605,9 @@ class VolcengineService { } logger.info(`Found ${foundationModels.Result.Items.length} foundation models`) } else { - logger.warn('Failed to fetch foundation models:', foundationModelsResult.reason) + const errorMsg = `Failed to fetch foundation models: ${foundationModelsResult.reason}` + logger.warn(errorMsg) + warnings.push(errorMsg) } // Process endpoints @@ -618,7 +634,9 @@ class VolcengineService { } logger.info(`Found ${endpoints.Result.Items.length} endpoints`) } else { - logger.warn('Failed to fetch endpoints:', endpointsResult.reason) + const errorMsg = `Failed to fetch endpoints: ${endpointsResult.reason}` + logger.warn(errorMsg) + warnings.push(errorMsg) } // If both failed, throw error @@ -634,7 +652,8 @@ class VolcengineService { return { models, - total + total, + warnings: warnings.length > 0 ? warnings : undefined } } catch (error) { logger.error('Failed to list Volcengine models:', error as Error) diff --git a/src/preload/index.ts b/src/preload/index.ts index 7a6ab3a38e..f686500069 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -579,11 +579,13 @@ const api = { hasCredentials: (): Promise => ipcRenderer.invoke(IpcChannel.Volcengine_HasCredentials), clearCredentials: (): Promise => ipcRenderer.invoke(IpcChannel.Volcengine_ClearCredentials), listModels: ( - projectName?: string + projectName?: string, + region?: string ): Promise<{ models: Array<{ id: string; name: string; description?: string; created?: number }> total?: number - }> => ipcRenderer.invoke(IpcChannel.Volcengine_ListModels, projectName), + warnings?: string[] + }> => ipcRenderer.invoke(IpcChannel.Volcengine_ListModels, projectName, region), getAuthHeaders: (params: { method: 'GET' | 'POST' host: string diff --git a/src/renderer/src/aiCore/legacy/clients/volcengine/VolcengineAPIClient.ts b/src/renderer/src/aiCore/legacy/clients/volcengine/VolcengineAPIClient.ts index fc08f68060..5b394c17ec 100644 --- a/src/renderer/src/aiCore/legacy/clients/volcengine/VolcengineAPIClient.ts +++ b/src/renderer/src/aiCore/legacy/clients/volcengine/VolcengineAPIClient.ts @@ -1,6 +1,6 @@ import type OpenAI from '@cherrystudio/openai' import { loggerService } from '@logger' -import { getVolcengineProjectName } from '@renderer/hooks/useVolcengine' +import { getVolcengineProjectName, getVolcengineRegion } from '@renderer/hooks/useVolcengine' import type { Provider } from '@renderer/types' import { OpenAIAPIClient } from '../openai/OpenAIApiClient' @@ -35,13 +35,22 @@ export class VolcengineAPIClient extends OpenAIAPIClient { logger.info('Fetching models from Volcengine API using signed request') const projectName = getVolcengineProjectName() - const response = await window.api.volcengine.listModels(projectName) + const region = getVolcengineRegion() + const response = await window.api.volcengine.listModels(projectName, region) if (!response || !response.models) { logger.warn('Empty response from Volcengine listModels') return [] } + // Notify user of any partial failures + if (response.warnings && response.warnings.length > 0) { + for (const warning of response.warnings) { + logger.warn(warning) + } + window.toast?.warning('Some Volcengine models could not be fetched. Check logs for details.') + } + const models: OpenAI.Models.Model[] = response.models.map((model) => ({ id: model.id, object: 'model' as const, @@ -55,6 +64,8 @@ export class VolcengineAPIClient extends OpenAIAPIClient { return models } catch (error) { logger.error('Failed to list Volcengine models:', error as Error) + // Notify user before falling back + window.toast?.warning('Failed to fetch Volcengine models. Check credentials if this persists.') // Fall back to standard OpenAI-compatible API on error logger.info('Falling back to OpenAI-compatible model list') return super.listModels() diff --git a/src/renderer/src/hooks/useVolcengine.ts b/src/renderer/src/hooks/useVolcengine.ts index 635fa6912f..b38c62ace2 100644 --- a/src/renderer/src/hooks/useVolcengine.ts +++ b/src/renderer/src/hooks/useVolcengine.ts @@ -1,10 +1,5 @@ import store, { useAppSelector } from '@renderer/store' -import { - setVolcengineAccessKeyId, - setVolcengineProjectName, - setVolcengineRegion, - setVolcengineSecretAccessKey -} from '@renderer/store/llm' +import { setVolcengineProjectName, setVolcengineRegion } from '@renderer/store/llm' import { useDispatch } from 'react-redux' export function useVolcengineSettings() { @@ -13,8 +8,6 @@ export function useVolcengineSettings() { return { ...settings, - setAccessKeyId: (accessKeyId: string) => dispatch(setVolcengineAccessKeyId(accessKeyId)), - setSecretAccessKey: (secretAccessKey: string) => dispatch(setVolcengineSecretAccessKey(secretAccessKey)), setRegion: (region: string) => dispatch(setVolcengineRegion(region)), setProjectName: (projectName: string) => dispatch(setVolcengineProjectName(projectName)) } @@ -24,14 +17,6 @@ export function getVolcengineSettings() { return store.getState().llm.settings.volcengine } -export function getVolcengineAccessKeyId() { - return store.getState().llm.settings.volcengine.accessKeyId -} - -export function getVolcengineSecretAccessKey() { - return store.getState().llm.settings.volcengine.secretAccessKey -} - export function getVolcengineRegion() { return store.getState().llm.settings.volcengine.region } diff --git a/src/renderer/src/pages/settings/ProviderSettings/VolcengineSettings.tsx b/src/renderer/src/pages/settings/ProviderSettings/VolcengineSettings.tsx index d9890ed53e..61480f68a6 100644 --- a/src/renderer/src/pages/settings/ProviderSettings/VolcengineSettings.tsx +++ b/src/renderer/src/pages/settings/ProviderSettings/VolcengineSettings.tsx @@ -10,22 +10,13 @@ import { SettingHelpLink, SettingHelpText, SettingHelpTextRow, SettingSubtitle } const VolcengineSettings: FC = () => { const { t } = useTranslation() - const { - accessKeyId, - secretAccessKey, - region, - projectName, - setAccessKeyId, - setSecretAccessKey, - setRegion, - setProjectName - } = useVolcengineSettings() + const { region, projectName, setRegion, setProjectName } = useVolcengineSettings() const providerConfig = PROVIDER_URLS['doubao'] const apiKeyWebsite = providerConfig?.websites?.apiKey - const [localAccessKeyId, setLocalAccessKeyId] = useState(accessKeyId) - const [localSecretAccessKey, setLocalSecretAccessKey] = useState(secretAccessKey) + const [localAccessKeyId, setLocalAccessKeyId] = useState('') + const [localSecretAccessKey, setLocalSecretAccessKey] = useState('') const [localRegion, setLocalRegion] = useState(region) const [localProjectName, setLocalProjectName] = useState(projectName) const [saving, setSaving] = useState(false) @@ -36,13 +27,11 @@ const VolcengineSettings: FC = () => { window.api.volcengine.hasCredentials().then(setHasCredentials) }, []) - // Sync local state with store + // Sync local state with store (only for region and projectName) useEffect(() => { - setLocalAccessKeyId(accessKeyId) - setLocalSecretAccessKey(secretAccessKey) setLocalRegion(region) setLocalProjectName(projectName) - }, [accessKeyId, secretAccessKey, region, projectName]) + }, [region, projectName]) const handleSaveCredentials = useCallback(async () => { if (!localAccessKeyId || !localSecretAccessKey) { @@ -52,38 +41,28 @@ const VolcengineSettings: FC = () => { setSaving(true) try { - // Save to Redux store - setAccessKeyId(localAccessKeyId) - setSecretAccessKey(localSecretAccessKey) + // Save credentials to secure storage via IPC first + await window.api.volcengine.saveCredentials(localAccessKeyId, localSecretAccessKey) + + // Only update Redux after IPC success (for region and projectName only) setRegion(localRegion) setProjectName(localProjectName) - // Save to secure storage via IPC - await window.api.volcengine.saveCredentials(localAccessKeyId, localSecretAccessKey) setHasCredentials(true) + // Clear local credential state after successful save (they're now in secure storage) + setLocalAccessKeyId('') + setLocalSecretAccessKey('') window.toast.success(t('settings.provider.volcengine.credentials_saved')) } catch (error) { window.toast.error(String(error)) } finally { setSaving(false) } - }, [ - localAccessKeyId, - localSecretAccessKey, - localRegion, - localProjectName, - setAccessKeyId, - setSecretAccessKey, - setRegion, - setProjectName, - t - ]) + }, [localAccessKeyId, localSecretAccessKey, localRegion, localProjectName, setRegion, setProjectName, t]) const handleClearCredentials = useCallback(async () => { try { await window.api.volcengine.clearCredentials() - setAccessKeyId('') - setSecretAccessKey('') setLocalAccessKeyId('') setLocalSecretAccessKey('') setHasCredentials(false) @@ -91,7 +70,7 @@ const VolcengineSettings: FC = () => { } catch (error) { window.toast.error(String(error)) } - }, [setAccessKeyId, setSecretAccessKey, t]) + }, [t]) return ( <> @@ -103,7 +82,6 @@ const VolcengineSettings: FC = () => { value={localAccessKeyId} placeholder="Access Key ID" onChange={(e) => setLocalAccessKeyId(e.target.value)} - onBlur={() => setAccessKeyId(localAccessKeyId)} style={{ marginTop: 5 }} spellCheck={false} /> @@ -116,7 +94,6 @@ const VolcengineSettings: FC = () => { value={localSecretAccessKey} placeholder="Secret Access Key" onChange={(e) => setLocalSecretAccessKey(e.target.value)} - onBlur={() => setSecretAccessKey(localSecretAccessKey)} style={{ marginTop: 5 }} spellCheck={false} /> diff --git a/src/renderer/src/services/__tests__/ApiService.test.ts b/src/renderer/src/services/__tests__/ApiService.test.ts index 243fb556e5..976e64a86c 100644 --- a/src/renderer/src/services/__tests__/ApiService.test.ts +++ b/src/renderer/src/services/__tests__/ApiService.test.ts @@ -244,8 +244,6 @@ vi.mock('@renderer/store/llm.ts', () => { region: '' }, volcengine: { - accessKeyId: '', - secretAccessKey: '', region: 'cn-beijing', projectName: 'default' } diff --git a/src/renderer/src/store/llm.ts b/src/renderer/src/store/llm.ts index 592e3adf19..2138300b0b 100644 --- a/src/renderer/src/store/llm.ts +++ b/src/renderer/src/store/llm.ts @@ -32,8 +32,6 @@ type LlmSettings = { region: string } volcengine: { - accessKeyId: string - secretAccessKey: string region: string projectName: string } @@ -83,8 +81,6 @@ export const initialState: LlmState = { region: '' }, volcengine: { - accessKeyId: '', - secretAccessKey: '', region: 'cn-beijing', projectName: 'default' } @@ -228,12 +224,6 @@ const llmSlice = createSlice({ setAwsBedrockRegion: (state, action: PayloadAction) => { state.settings.awsBedrock.region = action.payload }, - setVolcengineAccessKeyId: (state, action: PayloadAction) => { - state.settings.volcengine.accessKeyId = action.payload - }, - setVolcengineSecretAccessKey: (state, action: PayloadAction) => { - state.settings.volcengine.secretAccessKey = action.payload - }, setVolcengineRegion: (state, action: PayloadAction) => { state.settings.volcengine.region = action.payload }, @@ -281,8 +271,6 @@ export const { setAwsBedrockSecretAccessKey, setAwsBedrockApiKey, setAwsBedrockRegion, - setVolcengineAccessKeyId, - setVolcengineSecretAccessKey, setVolcengineRegion, setVolcengineProjectName, updateModel