Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the desktop UI and provider configuration flow so misconfigured providers don’t silently fall back to showing default/known models, and instead surface a clear “could not contact provider” error when model listing fails.
Changes:
- Validate provider configuration by calling the “list provider models” endpoint and (attempt to) roll back config on failure.
- Update model selection UI to record per-provider model-fetch errors and display an error panel instead of default models.
- Simplify config/model fetching by removing the
getProviderModelshelper fromConfigContextand using the API client directly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/desktop/src/components/settings/providers/modal/subcomponents/handlers/DefaultSubmitHandler.tsx | Adds provider validation via model listing and introduces rollback logic on validation failure. |
| ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.tsx | Fetches models per provider via API, tracks provider-specific errors, and shows an error UI when a provider can’t be contacted. |
| ui/desktop/src/components/settings/models/subcomponents/LeadWorkerSettings.tsx | Updates provider model fetching to use the API directly and logs failures. |
| ui/desktop/src/components/ConfigContext.tsx | Removes the getProviderModels method from context since callers now use the API client directly. |
| crates/goose-server/src/routes/config_management.rs | Removes retry wrapper around fetch_recommended_models() for the provider models endpoint. |
| // Rollback to previous config values on failure | ||
| const rollbackPromises: Promise<void>[] = []; | ||
| for (const [key, { value, isSecret }] of Object.entries(previousConfigValues)) { | ||
| rollbackPromises.push(upsertFn(key, value, isSecret)); | ||
| } | ||
| await Promise.all(rollbackPromises); |
There was a problem hiding this comment.
Rollback only restores keys that had a previous value, so newly-added config keys will remain persisted after a failed validation; track keys with no prior value and remove them during rollback (you may need to pass a removeFn into this handler).
| const currentValue = await readConfig({ | ||
| body: { key: param.name, is_secret: param.secret || false }, | ||
| }); | ||
| if (currentValue.data) { | ||
| previousConfigValues[param.name] = { | ||
| value: currentValue.data, | ||
| isSecret: param.secret || false, | ||
| }; |
There was a problem hiding this comment.
readConfig returns masked values when is_secret is true (see backend masking), so storing currentValue.data and later calling upsertFn will overwrite secrets with the masked payload (corrupting the saved secret); skip secret keys in this rollback mechanism or implement a safe server-side rollback strategy that doesn’t rely on reading secrets back.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
ui/desktop/src/components/settings/models/modelInterface.ts:68
- The OpenAPI client throws non-
Errorvalues for non-2xx responses (often an object or empty string), soe instanceof Errorwill frequently be false and you'll lose the underlying error details; formateby handling string/object cases (e.g., stringify) so the UI can show a useful provider failure message.
} catch (e: unknown) {
const errorMessage = `Failed to fetch models for ${p.name}${e instanceof Error ? `: ${e.message}` : ''}`;
return {
provider: p,
models: null,
error: errorMessage,
};
| if (currentValue.data) { | ||
| previousConfigValues[param.name] = { | ||
| value: currentValue.data, | ||
| isSecret: false, | ||
| }; | ||
| } |
There was a problem hiding this comment.
if (currentValue.data) will skip valid previous values that are falsy (e.g., empty string/0/false), so rollback may not restore them; check for data !== undefined (and possibly distinguish between “missing key” vs explicit null) instead of relying on truthiness.
| const rollbackPromises: Promise<void>[] = []; | ||
| for (const [key, { value, isSecret }] of Object.entries(previousConfigValues)) { | ||
| rollbackPromises.push(upsertFn(key, value, isSecret)); | ||
| } | ||
| await Promise.all(rollbackPromises); | ||
|
|
There was a problem hiding this comment.
Rollback only re-upserts keys that previously existed, so newly-added non-secret keys (where readConfig threw) will remain set even after a failed getProviderModels check; consider also removing keys that were absent before (e.g., track missing keys and call removeConfig, or pass a removeFn into this handler).
2 participants
Summary
For misconfigured providers we would just show the default models. For Ollama etc, that's pretty confusing!
now it says this: