Config: fix Google provider models via dynamic multi-provider config#239
Conversation
…l backend providers
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR generalizes the prompt editor's completion config to support multiple providers (adding google-aistudio) instead of OpenAI-only. It introduces a shared ChangesMulti-provider completion config support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ConfigEditorPane
participant modelSchema
participant useConfigPersistence
User->>ConfigEditorPane: Select provider/type/model
ConfigEditorPane->>modelSchema: getCompletionTypesForProvider(provider)
modelSchema-->>ConfigEditorPane: available completion types
ConfigEditorPane->>modelSchema: reconcileParamsForModel(provider, model, params)
modelSchema-->>ConfigEditorPane: reconciled params
ConfigEditorPane->>User: Render updated Type/Model/Params UI
User->>useConfigPersistence: Save config
useConfigPersistence->>useConfigPersistence: Derive completionType, build payload
useConfigPersistence-->>User: Success or error toast
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/prompt-editor/ConfigEditorPane.tsx (1)
87-94: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGate
availableTypes/typeOptionsonschemasLoadedlikeproviderOptions.
providerOptionsexplicitly falls back to[]when!schemasLoaded(Line 87), butavailableTypes/typeOptionscallgetCompletionTypesForProvider(provider)unconditionally. Since that function reads from the same schema cache, the Type<select>can momentarily render with zero options before schemas finish loading, causing a mismatch between the controlledvalueand available options.🩹 Proposed fix
- const availableTypes = getCompletionTypesForProvider(provider); + const availableTypes = schemasLoaded + ? getCompletionTypesForProvider(provider) + : []; const typeOptions = PROVIDER_TYPES.filter((t) => availableTypes.includes(t.value as ConfigType), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/prompt-editor/ConfigEditorPane.tsx` around lines 87 - 94, `ConfigEditorPane` should gate the Type options on `schemasLoaded` the same way `providerOptions` does. Update the `availableTypes`/`typeOptions` logic so `getCompletionTypesForProvider(provider)` is only used after schemas are loaded, and fall back to an empty list before then. Keep the fix localized to the `ConfigEditorPane` render logic so the Type `<select>` doesn’t briefly show a controlled value with no matching options.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/apiClient.ts`:
- Around line 61-74: The error text assembly in extractErrorMessage is
duplicating plain-string body.detail because it is used both as msg and detail.
Update the logic so body.detail is only included once in the final message,
preferably by making the detail branch ignore plain strings or by removing the
string fallback from msg. Keep the combined output from extractErrorMessage and
its callers like useConfigPersistence from surfacing repeated text.
---
Outside diff comments:
In `@app/components/prompt-editor/ConfigEditorPane.tsx`:
- Around line 87-94: `ConfigEditorPane` should gate the Type options on
`schemasLoaded` the same way `providerOptions` does. Update the
`availableTypes`/`typeOptions` logic so
`getCompletionTypesForProvider(provider)` is only used after schemas are loaded,
and fall back to an empty list before then. Keep the fix localized to the
`ConfigEditorPane` render logic so the Type `<select>` doesn’t briefly show a
controlled value with no matching options.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aee6761f-5048-4c82-af44-5e945b5ece69
📒 Files selected for processing (7)
app/components/prompt-editor/ConfigEditorPane.tsxapp/hooks/useConfigPersistence.tsapp/lib/apiClient.tsapp/lib/chatClient.tsapp/lib/modelSchema.tsapp/lib/types/configs.tsapp/lib/utils.ts
💤 Files with no reviewable changes (1)
- app/lib/utils.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/lib/types/configs.ts (1)
19-19: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
SavedConfig.providerstill typed asstring, whileCompletionConfig.provideris nowProviderType.
configToBlob(app/lib/chatClient.ts) andapplyProviderTypeModel(ConfigEditorPane.tsx) both doconfig.provider as ProviderType/nextProvider as ProviderType, unsafely widening fromstring. SinceSavedConfigis the round-trip persisted shape that feeds directly intoCompletionConfig, narrowing it toProviderTypehere would let the compiler catch invalid provider values instead of relying on unchecked casts downstream.♻️ Proposed fix
export interface SavedConfig { ... - provider: string; + provider: ProviderType;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/types/configs.ts` at line 19, SavedConfig.provider is still typed too broadly as string, which forces unsafe casts later in configToBlob and applyProviderTypeModel. Update SavedConfig in configs.ts so provider uses ProviderType like CompletionConfig, and then remove the downstream as ProviderType casts in chatClient.ts and ConfigEditorPane.tsx where the persisted config is converted or updated.app/lib/modelSchema.ts (1)
28-58: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
max_output_tokensshould not be dropped from model schema config.acceptsToolsinConfigEditorPane.tsxanduseConfigPersistence.tsdepends on"max_output_tokens" in modelSchema.config, butflattenGroupedModels()filters it out because it isn’t inSUPPORTED_PARAMS. That makes tool support look unavailable for every loaded schema and preventsmax_num_resultsfrom being persisted. Addmax_output_tokensto the allowlist, or move this capability check to separate schema metadata.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/modelSchema.ts` around lines 28 - 58, The model schema flattening logic is dropping max_output_tokens from config, which breaks downstream capability checks. Update flattenGroupedModels() in modelSchema.ts so the SUPPORTED_PARAMS allowlist includes max_output_tokens, or otherwise preserve that field when copying entry.config into ModelSchema.config. Keep the existing filtering for other params intact so acceptsTools in ConfigEditorPane.tsx and persistence in useConfigPersistence.ts can still detect tool support correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/lib/modelSchema.ts`:
- Around line 28-58: The model schema flattening logic is dropping
max_output_tokens from config, which breaks downstream capability checks. Update
flattenGroupedModels() in modelSchema.ts so the SUPPORTED_PARAMS allowlist
includes max_output_tokens, or otherwise preserve that field when copying
entry.config into ModelSchema.config. Keep the existing filtering for other
params intact so acceptsTools in ConfigEditorPane.tsx and persistence in
useConfigPersistence.ts can still detect tool support correctly.
In `@app/lib/types/configs.ts`:
- Line 19: SavedConfig.provider is still typed too broadly as string, which
forces unsafe casts later in configToBlob and applyProviderTypeModel. Update
SavedConfig in configs.ts so provider uses ProviderType like CompletionConfig,
and then remove the downstream as ProviderType casts in chatClient.ts and
ConfigEditorPane.tsx where the persisted config is converted or updated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b387cae-72f6-4868-b75e-86bced374259
📒 Files selected for processing (6)
app/components/prompt-editor/ConfigEditorPane.tsxapp/lib/apiClient.tsapp/lib/chatClient.tsapp/lib/modelSchema.tsapp/lib/types/assessment/config.tsapp/lib/types/configs.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- app/lib/chatClient.ts
- app/lib/apiClient.ts
- app/components/prompt-editor/ConfigEditorPane.tsx
|
🎉 This PR is included in version 0.3.0-main.5 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 participants
Issue
Closes #240
Summary
Checklist
Before submitting a pull request, please ensure that you mark these task.
npm run devandnpm run buildin the repository root and test.Notes