fix editing custom url not changing default url for default provider,… - #156
Conversation
… and added custom model helper text and error indicator
📝 WalkthroughWalkthroughBackend changes reformat SettingsRouter.py endpoints with multi-line decorators/docstrings and add a defaultBaseUrl update in save_ai_key for the current default provider. Frontend changes add validation state for a custom-model input field in ApiKeysSettings.jsx, wiring error/helper text and reset logic. ChangesSettingsRouter Formatting and Default Provider Update
Custom Model Field Validation
Estimated code review effort: 2 (Simple) | ~12 minutes Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| content={ | ||
| "success": False, | ||
| "message": f"Error clearing login data: {str(e)}", | ||
| }, |
![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 (3)
src/Engine/lib/api/routers/SettingsRouter.py (1)
208-256: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
defaultBaseUrlcan be incorrectly reset toNonefor the default provider.At Line 252,
ai_settings.defaultBaseUrlis set directly fromdata.base_url, which is optional and commonlyNonewhen the user isn't using a custom URL. In that case:
- For
openai, the provider'sbaseUrlis resolved to"https://api.openai.com/v1"(Line 247), butdefaultBaseUrlis set toNone— inconsistent with the provider's actual URL.- For
anthropic/baseUrlretains its existing (non-null) value, yetdefaultBaseUrlstill gets overwritten withNone.This defeats the PR's stated goal of keeping the default URL correctly in sync, and can cause
GET /settings/andGET /settings/ai/keysto report adefaultBaseUrlofNoneeven though the provider has a valid URL configured. Use the resolved providerbaseUrlinstead of the raw request field.🐛 Proposed fix
# If this provider is the current default, update defaultModel as well if ai_settings.defaultProvider == provider_id: ai_settings.defaultModel = data.model - ai_settings.defaultBaseUrl = data.base_url + ai_settings.defaultBaseUrl = getattr(ai_settings.providers, provider_id).baseUrlAlso, the docstring (Lines 219-221) still only mentions updating "the default model" — update it to reflect the
defaultBaseUrlchange as well.🤖 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 `@src/Engine/lib/api/routers/SettingsRouter.py` around lines 208 - 256, The save_ai_key handler is setting defaultBaseUrl from the optional request field instead of the resolved provider URL, so the default provider can end up with None even when a valid base URL exists. Update save_ai_key to assign ai_settings.defaultBaseUrl from the effective baseUrl on ai_settings.providers[provider_id] after the openai fallback/custom URL handling, and keep defaultModel in sync in the same defaultProvider branch. Also revise the save_ai_key docstring to mention that both the default model and defaultBaseUrl are updated.src/UI/src/Components/Modals/Settings/Sections/ApiKeysSettings.jsx (2)
384-396: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winCustom-URL Model field doesn't display the new validation state.
The added
customModelError/customModelHelperTextstate is only wired to the "Custom Model" TextField at Lines 428-446 (shown whenmodel === 'other'and NOTisCustomUrl). The TextField actually rendered whenisCustomUrlis true (this block) — which is the field this PR's fix is targeting — has noerror/helperTextprops and itsonChangedoesn't reset the error state. As a result,validateInputs()'s isCustomUrl branch can setcustomModelError/customModelHelperText, but the user never sees any indication of it on the relevant field.🐛 Proposed fix
<TextField fullWidth variant="outlined" label="Model" placeholder="Enter model name" sx={inputProps} value={customModel} - onChange={(e) => setCustomModel(e.target.value)} + onChange={(e) => { + setCustomModel(e.target.value); + setCustomModelError(false); + setCustomModelHelperText(''); + }} + error={customModelError} + helperText={customModelHelperText} />🤖 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 `@src/UI/src/Components/Modals/Settings/Sections/ApiKeysSettings.jsx` around lines 384 - 396, The Custom URL model input in ApiKeysSettings.jsx is missing the new validation UI wiring, so the field rendered when isCustomUrl is true never shows customModelError/customModelHelperText. Update that TextField to use the same error and helperText state handled by validateInputs(), and make its onChange clear the validation state like the existing Custom Model field does. Reference the isCustomUrl branch and the customModelError/customModelHelperText state so the fix lands on the correct input.
157-179: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
finalModeluses stalemodelstate instead ofcustomModelin the isCustomUrl validation branch.
finalModel = model === 'other' ? customModel : modelis reused for the isCustomUrl branch, but whenisCustomUrlis true the model dropdown isn't rendered — the user only editscustomModeldirectly (Line 393). Sincemodelstays whatever it was previously (often'', or'other'only if previously loaded), this check validates the wrong field: a correctly-filledcustomModelcan still fail validation, or a stalemodelvalue can mask an emptycustomModel.handleSave(Line 198) already treatscustomModelas the source of truth forisCustomUrl— validation should match.🐛 Proposed fix
- // Validate Model - const finalModel = model === 'other' ? customModel : model; - if (!isCustomUrl) { + // Validate Model + if (!isCustomUrl) { + const finalModel = model === 'other' ? customModel : model; if (!finalModel || finalModel.trim() === "") { setModelError(true); setModelHelperText("Model is required."); isValid = false; } else { setModelError(false); setModelHelperText(''); } } else { // For custom URL, validate custom model - if (!finalModel || finalModel.trim() === "") { + if (!customModel || customModel.trim() === "") { setCustomModelError(true); setCustomModelHelperText("Model is required."); isValid = false; } else { setCustomModelError(false); setCustomModelHelperText(''); } }🤖 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 `@src/UI/src/Components/Modals/Settings/Sections/ApiKeysSettings.jsx` around lines 157 - 179, The validation in ApiKeysSettings’s save flow is using stale model state in the isCustomUrl branch because finalModel is derived from model even when the custom URL path should rely on customModel. Update the validation logic in the same handler that sets modelError/customModelError so the isCustomUrl path checks customModel directly, matching the source of truth already used by handleSave, and keep the non-custom path using model/other as before.
🤖 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 `@src/Engine/lib/api/routers/SettingsRouter.py`:
- Around line 105-114: The exception handlers in SettingsRouter are returning
raw exception text to clients via the response message, which can leak internal
details. Keep the existing logger.error calls for server-side diagnostics, but
update the client-facing JSONResponse in this handler to use a generic failure
message instead of str(e). Apply the same pattern consistently across the other
exception blocks in SettingsRouter, including get_settings, get_ai_keys,
save_ai_key, and set_default_provider.
---
Outside diff comments:
In `@src/Engine/lib/api/routers/SettingsRouter.py`:
- Around line 208-256: The save_ai_key handler is setting defaultBaseUrl from
the optional request field instead of the resolved provider URL, so the default
provider can end up with None even when a valid base URL exists. Update
save_ai_key to assign ai_settings.defaultBaseUrl from the effective baseUrl on
ai_settings.providers[provider_id] after the openai fallback/custom URL
handling, and keep defaultModel in sync in the same defaultProvider branch. Also
revise the save_ai_key docstring to mention that both the default model and
defaultBaseUrl are updated.
In `@src/UI/src/Components/Modals/Settings/Sections/ApiKeysSettings.jsx`:
- Around line 384-396: The Custom URL model input in ApiKeysSettings.jsx is
missing the new validation UI wiring, so the field rendered when isCustomUrl is
true never shows customModelError/customModelHelperText. Update that TextField
to use the same error and helperText state handled by validateInputs(), and make
its onChange clear the validation state like the existing Custom Model field
does. Reference the isCustomUrl branch and the
customModelError/customModelHelperText state so the fix lands on the correct
input.
- Around line 157-179: The validation in ApiKeysSettings’s save flow is using
stale model state in the isCustomUrl branch because finalModel is derived from
model even when the custom URL path should rely on customModel. Update the
validation logic in the same handler that sets modelError/customModelError so
the isCustomUrl path checks customModel directly, matching the source of truth
already used by handleSave, and keep the non-custom path using model/other as
before.
🪄 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: ea58f1d4-231c-44cc-8231-58a98089b1f9
📒 Files selected for processing (2)
src/Engine/lib/api/routers/SettingsRouter.pysrc/UI/src/Components/Modals/Settings/Sections/ApiKeysSettings.jsx
| except Exception as e: | ||
| logger.error(f"Error clearing login data: {e}") | ||
|
|
||
| return JSONResponse( | ||
| status_code=500, | ||
| content={"success": False, "message": f"Error clearing login data: {str(e)}"}, | ||
| content={ | ||
| "success": False, | ||
| "message": f"Error clearing login data: {str(e)}", | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Exception details leaked to API response.
str(e) is embedded directly into the client-facing message field. Static analysis (CodeQL) flags this as information exposure through an exception — internal paths or implementation details from rmtree/filesystem errors could leak to the client. Log the full exception server-side (already done via logger.error) and return a generic message to the caller.
🔒️ Proposed fix
except Exception as e:
logger.error(f"Error clearing login data: {e}")
return JSONResponse(
status_code=500,
content={
"success": False,
- "message": f"Error clearing login data: {str(e)}",
+ "message": "Error clearing login data. Please check the server logs for details.",
},
)Note: this same pattern (raw str(e) returned to the client) recurs in the other exception handlers in this file (e.g. get_settings, get_ai_keys, save_ai_key, set_default_provider); consider fixing consistently across the file.
📝 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.
| except Exception as e: | |
| logger.error(f"Error clearing login data: {e}") | |
| return JSONResponse( | |
| status_code=500, | |
| content={"success": False, "message": f"Error clearing login data: {str(e)}"}, | |
| content={ | |
| "success": False, | |
| "message": f"Error clearing login data: {str(e)}", | |
| }, | |
| ) | |
| except Exception as e: | |
| logger.error(f"Error clearing login data: {e}") | |
| return JSONResponse( | |
| status_code=500, | |
| content={ | |
| "success": False, | |
| "message": "Error clearing login data. Please check the server logs for details.", | |
| }, | |
| ) |
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 110-113: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🪛 Ruff (0.15.20)
[warning] 105-105: Do not catch blind exception: Exception
(BLE001)
[warning] 112-112: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 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 `@src/Engine/lib/api/routers/SettingsRouter.py` around lines 105 - 114, The
exception handlers in SettingsRouter are returning raw exception text to clients
via the response message, which can leak internal details. Keep the existing
logger.error calls for server-side diagnostics, but update the client-facing
JSONResponse in this handler to use a generic failure message instead of str(e).
Apply the same pattern consistently across the other exception blocks in
SettingsRouter, including get_settings, get_ai_keys, save_ai_key, and
set_default_provider.
Source: Linters/SAST tools
Ashif4354
changed the title
2 participants
… and added custom model helper text and error indicator
Summary by CodeRabbit
New Features
Bug Fixes