fix(model-list): add model fetching with fallback for Anthropic an…#1560
Merged
Conversation
…d Gemini - Add suppressErrors option to fetchModels in baseProvider - Implement fetchAnthropicModelsWithFallback and fetchGeminiModelsWithFallback - Update ProviderApiConfig component to handle new provider types - Add comprehensive tests for Anthropic and Gemini providers - Add tests for ProviderApiConfig component
Contributor
📝 WalkthroughWalkthroughChanges introduce an optional error suppression parameter to Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Component<br/>(ProviderApiConfig)
participant Presenter as BaseProvider<br/>(fetchModels)
participant Strategy as AiSdkProvider<br/>(fetchProviderModelsByStrategy)
participant API as Remote API<br/>(Anthropic/Gemini)
participant ConfigDB as Config DB
UI->>Presenter: refreshModels()<br/>(safeCall: false)
Presenter->>Presenter: fetchModels<br/>({ suppressErrors: false })
Presenter->>Strategy: fetchProviderModelsByStrategy('config-db')
alt Custom Provider with API Key
Strategy->>API: GET /models<br/>(runtime-specific headers)
alt Fetch Success
API-->>Strategy: model list response
Strategy->>Strategy: normalize & map to MODEL_META
Strategy-->>Presenter: models array
else Fetch Failure
API-->>Strategy: error response
Strategy->>Strategy: extract error message
Strategy-->>Presenter: throw error
end
else No API Key or Fallback
Strategy->>ConfigDB: lookup models
ConfigDB-->>Strategy: config-db models
Strategy-->>Presenter: models array
end
alt Success
Presenter-->>UI: MODEL_META[]
UI->>UI: display models
else Error & suppressErrors: false
Presenter-->>UI: throw error
UI->>UI: extract error message
UI->>UI: show toast<br/>with error details
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/llmProviderPresenter/baseProvider.ts (1)
238-267:⚠️ Potential issue | 🔴 CriticalAsync rejections bypass error handling in
fetchModels().Returning
this.fetchProviderModels().then(...)at line 241 causes rejections to skip the catch block entirely, making thesuppressErrorsoption ineffective and preventing error logging.✅ Suggested fix
public async fetchModels(options?: { suppressErrors?: boolean }): Promise<MODEL_META[]> { const suppressErrors = options?.suppressErrors ?? true try { - return this.fetchProviderModels().then((models) => { + const models = await this.fetchProviderModels() logger.info( `[Provider] fetchModels: fetched ${models?.length || 0} models for provider "${this.provider.id}"` ) // Validate that all models have correct providerId const validatedModels = models.map((model) => { @@ }) this.models = validatedModels this.configPresenter.setProviderModels(this.provider.id, validatedModels) return validatedModels - }) } catch (e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/llmProviderPresenter/baseProvider.ts` around lines 238 - 267, The current fetchModels method returns this.fetchProviderModels().then(...), which lets promise rejections bypass the surrounding try/catch and breaks suppressErrors and logging; change the implementation to await the provider call (e.g. const models = await this.fetchProviderModels()) inside the try block, keep the same validation/assignment logic (models -> validatedModels -> this.models and this.configPresenter.setProviderModels(this.provider.id, validatedModels)), and in the catch block log the error via logger.error and rethrow only when options?.suppressErrors is false to preserve the original behavior.
🧹 Nitpick comments (1)
test/main/presenter/llmProviderPresenter/anthropicProvider.test.ts (1)
171-228: Restorefetchglobals between tests to avoid leakage.These tests stub
global.fetchbut never un-stub it. Add cleanup inafterEachto prevent cross-test coupling.♻️ Suggested cleanup
afterEach(() => { + vi.unstubAllGlobals() if (originalEnvKey === undefined) { delete process.env.ANTHROPIC_API_KEY return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/llmProviderPresenter/anthropicProvider.test.ts` around lines 171 - 228, Add an afterEach cleanup to restore the stubbed global fetch between tests: the tests currently call vi.stubGlobal('fetch') (see occurrences in anthropicProvider.test.ts around the AiSdkProvider/createProvider tests) and never restore it, so add an afterEach that calls vi.restoreAllMocks() and removes or resets global.fetch so subsequent tests don't leak the stubbed fetch; ensure the cleanup runs in this test file so both the successful fetch (fetchModels) and failing fetch (refreshModels) tests are isolated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/llmProviderPresenter/providers/aiSdkProvider.ts`:
- Around line 894-895: Both fetchAnthropicModelsWithFallback and
fetchGeminiModelsWithFallback call this.createModelRequestSignal(null) which
leaves fetches unbounded; change both calls to pass a sensible timeout (e.g., a
configured ms value or a constant) instead of null, ensure the returned signal
is wired into the fetch request options and that dispose() is still called in
finally to cancel on timeout; update other identical call sites (the second
occurrence around the other fetch path) to match and use the same timeout
constant or config value so remote model fetches are protected from hanging.
In `@src/renderer/settings/components/ProviderApiConfig.vue`:
- Around line 328-354: extractRefreshErrorMessage currently converts non-Error
rejections to String(error) which yields “[object Object]” and loses message
fields; update the function (extractRefreshErrorMessage) to first detect
plain-object rejections (e.g., typeof error === 'object' && error !== null) and
extract a string from error.message or error.error?.message when present (trim
and validate), then fall back to the Error instance message or String(error)
only if no message field exists; keep the existing JSON.parse attempt for
stringified objects and preserve the final null/normalizedMessage behavior.
---
Outside diff comments:
In `@src/main/presenter/llmProviderPresenter/baseProvider.ts`:
- Around line 238-267: The current fetchModels method returns
this.fetchProviderModels().then(...), which lets promise rejections bypass the
surrounding try/catch and breaks suppressErrors and logging; change the
implementation to await the provider call (e.g. const models = await
this.fetchProviderModels()) inside the try block, keep the same
validation/assignment logic (models -> validatedModels -> this.models and
this.configPresenter.setProviderModels(this.provider.id, validatedModels)), and
in the catch block log the error via logger.error and rethrow only when
options?.suppressErrors is false to preserve the original behavior.
---
Nitpick comments:
In `@test/main/presenter/llmProviderPresenter/anthropicProvider.test.ts`:
- Around line 171-228: Add an afterEach cleanup to restore the stubbed global
fetch between tests: the tests currently call vi.stubGlobal('fetch') (see
occurrences in anthropicProvider.test.ts around the AiSdkProvider/createProvider
tests) and never restore it, so add an afterEach that calls vi.restoreAllMocks()
and removes or resets global.fetch so subsequent tests don't leak the stubbed
fetch; ensure the cleanup runs in this test file so both the successful fetch
(fetchModels) and failing fetch (refreshModels) tests are isolated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fed5655-1d89-4406-8982-5322a1c1c8d8
📒 Files selected for processing (6)
src/main/presenter/llmProviderPresenter/baseProvider.tssrc/main/presenter/llmProviderPresenter/providers/aiSdkProvider.tssrc/renderer/settings/components/ProviderApiConfig.vuetest/main/presenter/llmProviderPresenter/anthropicProvider.test.tstest/main/presenter/llmProviderPresenter/geminiProvider.test.tstest/renderer/components/ProviderApiConfig.test.ts
Comment on lines
+894
to
+895
| const { signal, dispose } = this.createModelRequestSignal(null) | ||
|
|
Contributor
There was a problem hiding this comment.
Remote model fetches are missing timeout protection.
Both new fetch paths pass null into createModelRequestSignal, so requests can hang indefinitely under network issues.
⏱️ Suggested fix
- const { signal, dispose } = this.createModelRequestSignal(null)
+ const { signal, dispose } = this.createModelRequestSignal({
+ timeout: this.getModelFetchTimeout()
+ })Apply in both methods:
fetchAnthropicModelsWithFallbackfetchGeminiModelsWithFallback
Also applies to: 959-961
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/presenter/llmProviderPresenter/providers/aiSdkProvider.ts` around
lines 894 - 895, Both fetchAnthropicModelsWithFallback and
fetchGeminiModelsWithFallback call this.createModelRequestSignal(null) which
leaves fetches unbounded; change both calls to pass a sensible timeout (e.g., a
configured ms value or a constant) instead of null, ensure the returned signal
is wired into the fetch request options and that dispose() is still called in
finally to cancel on timeout; update other identical call sites (the second
occurrence around the other fetch path) to match and use the same timeout
constant or config value so remote model fetches are protected from hanging.
Comment on lines
+328
to
+354
| const extractRefreshErrorMessage = (error: unknown): string | null => { | ||
| const rawMessage = error instanceof Error ? error.message : String(error) | ||
| const normalizedMessage = rawMessage.trim() | ||
|
|
||
| if (!normalizedMessage) { | ||
| return null | ||
| } | ||
|
|
||
| try { | ||
| const parsed = JSON.parse(normalizedMessage) as { | ||
| error?: { message?: string } | ||
| message?: string | ||
| } | ||
|
|
||
| if (typeof parsed.error?.message === 'string' && parsed.error.message.trim()) { | ||
| return parsed.error.message.trim() | ||
| } | ||
|
|
||
| if (typeof parsed.message === 'string' && parsed.message.trim()) { | ||
| return parsed.message.trim() | ||
| } | ||
| } catch { | ||
| // ignore JSON parse errors and fall back to the original message | ||
| } | ||
|
|
||
| return normalizedMessage | ||
| } |
Contributor
There was a problem hiding this comment.
Handle plain-object rejections to avoid [object Object] in toasts.
If IPC rejects with a non-Error object (e.g. { message: '...' }), current logic drops the message. Parse object-shaped errors before String(error).
🩹 Suggested hardening
const extractRefreshErrorMessage = (error: unknown): string | null => {
- const rawMessage = error instanceof Error ? error.message : String(error)
+ const objectMessage =
+ error &&
+ typeof error === 'object' &&
+ 'message' in error &&
+ typeof (error as { message?: unknown }).message === 'string'
+ ? (error as { message: string }).message
+ : null
+
+ const rawMessage = error instanceof Error ? error.message : objectMessage ?? String(error)
const normalizedMessage = rawMessage.trim()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const extractRefreshErrorMessage = (error: unknown): string | null => { | |
| const rawMessage = error instanceof Error ? error.message : String(error) | |
| const normalizedMessage = rawMessage.trim() | |
| if (!normalizedMessage) { | |
| return null | |
| } | |
| try { | |
| const parsed = JSON.parse(normalizedMessage) as { | |
| error?: { message?: string } | |
| message?: string | |
| } | |
| if (typeof parsed.error?.message === 'string' && parsed.error.message.trim()) { | |
| return parsed.error.message.trim() | |
| } | |
| if (typeof parsed.message === 'string' && parsed.message.trim()) { | |
| return parsed.message.trim() | |
| } | |
| } catch { | |
| // ignore JSON parse errors and fall back to the original message | |
| } | |
| return normalizedMessage | |
| } | |
| const extractRefreshErrorMessage = (error: unknown): string | null => { | |
| const objectMessage = | |
| error && | |
| typeof error === 'object' && | |
| 'message' in error && | |
| typeof (error as { message?: unknown }).message === 'string' | |
| ? (error as { message: string }).message | |
| : null | |
| const rawMessage = error instanceof Error ? error.message : objectMessage ?? String(error) | |
| const normalizedMessage = rawMessage.trim() | |
| if (!normalizedMessage) { | |
| return null | |
| } | |
| try { | |
| const parsed = JSON.parse(normalizedMessage) as { | |
| error?: { message?: string } | |
| message?: string | |
| } | |
| if (typeof parsed.error?.message === 'string' && parsed.error.message.trim()) { | |
| return parsed.error.message.trim() | |
| } | |
| if (typeof parsed.message === 'string' && parsed.message.trim()) { | |
| return parsed.message.trim() | |
| } | |
| } catch { | |
| // ignore JSON parse errors and fall back to the original message | |
| } | |
| return normalizedMessage | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/settings/components/ProviderApiConfig.vue` around lines 328 -
354, extractRefreshErrorMessage currently converts non-Error rejections to
String(error) which yields “[object Object]” and loses message fields; update
the function (extractRefreshErrorMessage) to first detect plain-object
rejections (e.g., typeof error === 'object' && error !== null) and extract a
string from error.message or error.error?.message when present (trim and
validate), then fall back to the Error instance message or String(error) only if
no message field exists; keep the existing JSON.parse attempt for stringified
objects and preserve the final null/normalizedMessage behavior.
zhangmo8
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…d Gemini
Summary by CodeRabbit
New Features
Bug Fixes
Tests