-
Notifications
You must be signed in to change notification settings - Fork 30
Add model management feature to settings page #143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This commit implements a comprehensive model management system in the settings page, allowing users to configure and manage AI model providers.
Frontend changes:
- Added new "Models" tab in settings page (between Integrations and Bots)
- Created ModelList component with Agent filtering and CRUD operations
- Created ModelEdit component with provider-specific forms
- Implemented 6 provider forms: Claude, OpenAI, OpenRouter, DeepSeek, GLM, Qwen
- Extended models API with full CRUD, test connection, and reference checking
- Added Chinese and English i18n translations for all model-related texts
- Integrated CubeIcon for model representation
Backend changes:
- Added POST /api/models/test endpoint for connection testing
- Added GET /api/models/{id}/check-references to verify Bot dependencies
- Enhanced model deletion protection to prevent removing referenced models
Features:
- Filter models by Agent type (ClaudeCode, Agno)
- Create/Edit/Delete/Clone models with provider-specific configurations
- Test connection before saving to validate credentials
- Reference checking prevents deletion of models in use by Bots
- Responsive design for mobile and desktop layouts
- Real-time model status indicators
WalkthroughA model management system is introduced across the backend and frontend, adding API endpoints for connection testing and reference checking, new TypeScript service interfaces, a complete UI for CRUD operations supporting six AI providers, and comprehensive internationalization for English and Chinese. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant ModelList as ModelList Component
participant ModelEdit as ModelEdit Component
participant API as Model API Service
participant Backend as Backend
rect rgb(200, 220, 255)
Note over User,Backend: Create/Edit Model Flow
User->>ModelList: Click Edit/Create
ModelList->>ModelEdit: Open editor with model data
User->>ModelEdit: Enter name, select provider, fill fields
User->>ModelEdit: Click "Test Connection"
ModelEdit->>API: testConnection(provider, config)
API->>Backend: POST /test
Backend-->>API: { success, message }
API-->>ModelEdit: Show toast (success/failure)
end
rect rgb(200, 255, 220)
Note over User,Backend: Save Model
User->>ModelEdit: Click "Save"
ModelEdit->>API: createModel or updateModel
API->>Backend: POST /models or PUT /models/{id}
Backend-->>API: ModelDetail
API-->>ModelList: Updated model list
ModelList->>ModelList: Optimistic UI update
end
rect rgb(255, 220, 200)
Note over User,Backend: Delete Model with Reference Check
User->>ModelList: Click Delete on model
ModelList->>API: checkReferences(model_id)
API->>Backend: GET /models/{model_id}/check-references
Backend-->>API: { is_referenced, referenced_by }
alt Model is referenced
API-->>ModelList: Show error toast
else Model not referenced
ModelList->>User: Show confirmation dialog
User->>ModelList: Confirm delete
ModelList->>API: deleteModel(model_id)
API->>Backend: DELETE /models/{model_id}
Backend-->>API: Success
API-->>ModelList: Remove from list
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Areas requiring extra attention:
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (10)
frontend/src/features/settings/components/modelForms/DeepSeekModelForm.tsx (1)
8-12: Consider stricter typing for formData.
Record<string, any>loses type safety for provider-specific configuration fields. While this provides flexibility, defining a more specific interface would improve autocomplete and catch typos at compile time.Example:
interface DeepSeekConfig { model_id?: string; api_key?: string; base_url?: string; } interface DeepSeekModelFormProps { formData: DeepSeekConfig; onChange: (field: string, value: string) => void; errors?: Record<string, boolean>; }backend/app/api/endpoints/adapter/models.py (3)
46-89: Connection testing is not implemented.The endpoint only performs basic validation (checking for
model_idandapi_keypresence) but doesn't actually test the connection to the provider. The TODO comment on Line 77 confirms this. This means users will see "success" even with invalid credentials.Do you want me to generate a basic implementation for connection testing that makes actual API calls to validate credentials for each provider? I can provide a framework that:
- Uses provider-specific SDK clients
- Makes minimal API calls (e.g., list models endpoint)
- Handles timeouts and provider-specific errors
- Returns meaningful error messages
49-49: Unused parameter should be removed or utilized.The
current_userparameter is declared but never used in thetest_connectionfunction. If user context isn't needed for connection testing, remove the dependency. Otherwise, consider using it for audit logging.If not needed:
def test_connection( request: TestConnectionRequest = Body(...), - current_user: User = Depends(security.get_current_user), ):
116-116: Use implicit truthiness check for boolean field.Line 116 uses
Bot.is_active == Truewhich is flagged by static analysis. Use implicit truthiness oris Truefor boolean comparisons.Apply this diff:
- bots = db.query(Bot).filter(Bot.is_active == True).all() + bots = db.query(Bot).filter(Bot.is_active.is_(True)).all()Or more idiomatically:
- bots = db.query(Bot).filter(Bot.is_active == True).all() + bots = db.query(Bot).filter(Bot.is_active).all()frontend/src/app/settings/page.tsx (1)
12-18: Models tab wiring looks consistent; consider centralizing tab configThe new
modelstab is correctly threaded through the index↔name maps and both desktop/mobile Tab.List + Tab.Panels, so?tab=modelsdeep-linking should work as expected.To reduce future drift as more tabs are added, consider deriving
tabIndexToName/tabNameToIndexand the Tab.List/Tab.Panels from a singletabsconfig array (id, label key, icon, panel) instead of duplicating the mapping in multiple places.Also applies to: 32-35, 44-47, 122-133, 180-182, 213-224, 272-274
frontend/src/features/settings/components/ModelList.tsx (2)
52-66: Agent-based filtering is correct; guard against async race conditionsThe agent filter logic is functionally correct and falls back to all models on errors or empty results, which is good. However, since
filterByAgentis async and depends onselectedAgent, rapid agent changes or refreshes could allow an older request to overwritefilteredModelsafter a newer selection.You can cheaply guard against this by capturing the current agent and checking before setting state, e.g.:
useEffect(() => { if (!selectedAgent) { setFilteredModels(models); return; } async function filterByAgent() { + const currentAgent = selectedAgent; try { const response = await modelApis.getModelNames(selectedAgent); const modelNames = response.data.map(m => m.name); if (modelNames.length === 0) { setFilteredModels(models); } else { const filtered = models.filter(model => modelNames.includes(model.name)); - setFilteredModels(filtered); + // Ignore if selection changed while request was in flight + if (currentAgent === selectedAgent) { + setFilteredModels(filtered); + } } } catch (error) { console.error('Failed to filter models:', error); setFilteredModels(models); } } filterByAgent(); }, [selectedAgent, models]);Also applies to: 88-115
137-159: Tighten delete flow aroundmodelToDeletelifecycleThe delete flow is mostly solid (pre-check references, confirmation dialog, optimistic local removal). Two small robustness improvements:
handleConfirmDeleteusesif (!modelToDelete) return;, which would silently no-op if an ID of0is ever valid. A null check is safer.Dialog’sonOpenChange={setDeleteConfirmVisible}can close the dialog (e.g., overlay click) without clearingmodelToDelete, leaving stale state around.You can address both as follows:
- const handleConfirmDelete = async () => { - if (!modelToDelete) return; + const handleConfirmDelete = async () => { + if (modelToDelete == null) return; try { await modelApis.deleteModel(modelToDelete); setModels(prev => prev.filter(m => m.id !== modelToDelete)); - setDeleteConfirmVisible(false); - setModelToDelete(null); + handleCancelDelete(); toast({ title: t('settings.model.delete_success') || 'Model deleted successfully', }); } catch (e) { const errorMessage = e instanceof Error && e.message ? e.message : t('settings.model.errors.delete_failed'); toast({ variant: 'destructive', title: errorMessage, }); } };And centralize cleanup on dialog close:
- <Dialog open={deleteConfirmVisible} onOpenChange={setDeleteConfirmVisible}> + <Dialog + open={deleteConfirmVisible} + onOpenChange={open => { + setDeleteConfirmVisible(open); + if (!open) { + setModelToDelete(null); + } + }} + >Also applies to: 161-184, 336-352
frontend/src/features/settings/components/ModelEdit.tsx (3)
5-7: Unify translation hook source with other settings componentsOther settings components (e.g.,
frontend/src/app/settings/page.tsxandModelList.tsx) importuseTranslationfrom@/hooks/useTranslation, but this file pulls it directly fromreact-i18next. Mixing the two can make it harder to maintain a single i18n configuration and typing story.Consider switching to the project-level hook here for consistency:
-import { useTranslation } from 'react-i18next'; +import { useTranslation } from '@/hooks/useTranslation';Also applies to: 15-22
112-170: Harden test connection error handling to avoidundefinedmessages
handleTestConnectioncurrently assumeserroris anErrorand concatenateserror.message, which can result in messages like"Connection test failed: undefined"if a non-Error is thrown.You can mirror the pattern used in
ModelListfor deletes:- } catch (error) { - toast({ - variant: 'destructive', - title: t('settings.model.test_failed') + ': ' + (error as Error).message, - }); - } finally { + } catch (error) { + const msg = + error instanceof Error && error.message + ? error.message + : t('settings.model.test_failed'); + toast({ + variant: 'destructive', + title: msg, + }); + } finally { setTestingConnection(false); }(If you still want to prefix with the generic text, you can compose
msgaccordingly.)
172-212: Use an update-specific i18n key when edit save fails
handleSavefalls back tot('settings.model.errors.create_failed')for all failures, including updates, even thoughsettings.model.errors.update_failedexists in the i18n file.You can distinguish the messages based on whether you’re creating or updating:
- try { - const modelReq: ModelCreateRequest = { + try { + const modelReq: ModelCreateRequest = { name: modelName.trim(), config: { env: { model: provider, ...formData, }, }, is_active: true, }; - if (editingModelId && editingModelId > 0) { - // Edit existing model - const updated = await modelApis.updateModel(editingModelId, modelReq); - setModels(prev => prev.map(m => (m.id === editingModelId ? updated : m))); - } else { - // Create new model - const created = await modelApis.createModel(modelReq); - setModels(prev => [created, ...prev]); - } - onClose(); - } catch (error) { - toast({ - variant: 'destructive', - title: (error as Error)?.message || t('settings.model.errors.create_failed'), - }); - } finally { + if (editingModelId && editingModelId > 0) { + const updated = await modelApis.updateModel(editingModelId, modelReq); + setModels(prev => prev.map(m => (m.id === editingModelId ? updated : m))); + } else { + const created = await modelApis.createModel(modelReq); + setModels(prev => [created, ...prev]); + } + onClose(); + } catch (error) { + const fallbackKey = + editingModelId && editingModelId > 0 + ? 'settings.model.errors.update_failed' + : 'settings.model.errors.create_failed'; + const title = + error instanceof Error && error.message ? error.message : t(fallbackKey); + toast({ + variant: 'destructive', + title, + }); + } finally { setModelSaving(false); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/app/api/endpoints/adapter/models.py(2 hunks)frontend/src/apis/models.ts(1 hunks)frontend/src/app/settings/page.tsx(7 hunks)frontend/src/features/settings/components/ModelEdit.tsx(1 hunks)frontend/src/features/settings/components/ModelList.tsx(1 hunks)frontend/src/features/settings/components/modelForms/ClaudeModelForm.tsx(1 hunks)frontend/src/features/settings/components/modelForms/DeepSeekModelForm.tsx(1 hunks)frontend/src/features/settings/components/modelForms/GLMModelForm.tsx(1 hunks)frontend/src/features/settings/components/modelForms/OpenAIModelForm.tsx(1 hunks)frontend/src/features/settings/components/modelForms/OpenRouterModelForm.tsx(1 hunks)frontend/src/features/settings/components/modelForms/QwenModelForm.tsx(1 hunks)frontend/src/i18n/locales/en/common.json(1 hunks)frontend/src/i18n/locales/zh-CN/common.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
frontend/src/apis/models.ts (2)
backend/app/api/endpoints/adapter/models.py (2)
TestConnectionRequest(26-28)CheckReferencesResponse(41-43)frontend/src/apis/client.ts (1)
apiClient(105-105)
frontend/src/features/settings/components/ModelList.tsx (3)
frontend/src/apis/models.ts (2)
ModelDetail(12-27)modelApis(62-94)frontend/src/apis/agents.ts (1)
agentApis(23-32)frontend/src/components/common/UnifiedAddButton.tsx (1)
UnifiedAddButton(12-24)
frontend/src/features/settings/components/ModelEdit.tsx (1)
frontend/src/apis/models.ts (3)
ModelDetail(12-27)modelApis(62-94)ModelCreateRequest(38-44)
frontend/src/app/settings/page.tsx (1)
frontend/src/features/settings/components/ModelList.tsx (1)
ModelList(37-355)
backend/app/api/endpoints/adapter/models.py (3)
frontend/src/apis/models.ts (2)
TestConnectionRequest(46-51)CheckReferencesResponse(53-59)frontend/src/apis/client.ts (2)
post(86-91)request(22-80)backend/app/core/security.py (1)
get_current_user(27-50)
🪛 Ruff (0.14.5)
backend/app/api/endpoints/adapter/models.py
48-48: Do not perform function call Body in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
49-49: Unused function argument: current_user
(ARG001)
49-49: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
84-84: Do not catch blind exception: Exception
(BLE001)
87-87: Use explicit conversion flag
Replace with conversion flag
(RUF010)
94-94: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
95-95: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
104-104: Do not catch blind exception: Exception
(BLE001)
116-116: Avoid equality comparisons to True; use Bot.is_active: for truth checks
Replace with Bot.is_active
(E712)
127-129: try-except-pass detected, consider logging the exception
(S110)
127-127: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (7)
frontend/src/i18n/locales/zh-CN/common.json (2)
262-313: No issues found — template variables are correctly matched.The verification confirms that
{{bots}}in the i18n locale files (line 291 in zh-CN/common.json and line 288 in en/common.json) correctly maps to thebotsparameter injected by the frontend component atfrontend/src/features/settings/components/ModelList.tsx:146. The variable is constructed frombotNames(line 143) and passed to the translation function as{ bots: botNames }. Template substitution will work properly, and actual bot names will render in the UI rather than literal placeholder text.
258-258: Verification confirmed: English and Chinese i18n files have identical structure and keys.Both
frontend/src/i18n/locales/en/common.jsonandfrontend/src/i18n/locales/zh-CN/common.jsonare valid JSON with full parity. All 36 keys in the model management section match exactly between files, confirming consistency across locales and complete feature coverage in both languages.frontend/src/apis/models.ts (2)
12-59: LGTM!The TypeScript interfaces are well-structured and align with the backend Pydantic models. The use of
[key: string]: anyinModelDetail.config.env(Line 21) provides appropriate flexibility for provider-specific fields while maintaining type safety for common fields.
67-93: LGTM!The API methods are clean and follow consistent patterns. They correctly utilize the apiClient methods with proper typing and sensible defaults (page=1, limit=50 for pagination).
frontend/src/features/settings/components/ModelList.tsx (1)
191-329: Model list UI, status indicators, and parent state sync look goodThe list view cleanly surfaces name, provider, model ID, active/inactive status, and updated time, and the edit/clone/delete handlers are correctly wired into the shared
modelsstate andModelEdit. The scroll behavior and responsive layout also look consistent with the rest of the settings UI.frontend/src/i18n/locales/en/common.json (1)
255-310: Model management i18n additions align with component usageThe new
settings.modelslabel and thesettings.modelblock (fields, actions, statuses, providers, and error messages) line up with all usages inModelListandModelEdit, including the{{bots}}interpolation fordelete_referenced_error. This should give good coverage for the new Models tab without key mismatches.frontend/src/features/settings/components/ModelEdit.tsx (1)
239-345: Form layout and provider-specific handling look clean and extensibleThe two-pane layout (left form, right help), required field indicators, provider selector, and the
renderProviderFormswitch make it straightforward to add more providers later. Keyboard escape handling and the top action bar (Test + Save with spinners) provide a good UX for the editor.
| except Exception as e: | ||
| return TestConnectionResponse( | ||
| success=False, | ||
| message=f"Connection test failed: {str(e)}" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broad exception catching without logging.
Catching Exception without logging the error makes debugging difficult. Users will see a generic failure message but operators won't have visibility into the actual error.
Apply this diff to add logging:
+ import logging
+ logger = logging.getLogger(__name__)
+
except Exception as e:
+ logger.error(f"Connection test failed for provider {provider}: {str(e)}", exc_info=True)
return TestConnectionResponse(
success=False,
message=f"Connection test failed: {str(e)}"
)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.5)
84-84: Do not catch blind exception: Exception
(BLE001)
87-87: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In backend/app/api/endpoints/adapter/models.py around lines 84 to 88, the except
block currently swallows all Exceptions and returns a generic
TestConnectionResponse; modify it to log the caught exception before returning:
import or use the module logger (or existing app logger), call logger.exception
or logger.error with the exception and stack trace (including context like
adapter name/connection parameters where safe), then return the same
TestConnectionResponse so operators can see the full error details in logs while
clients still receive the failure message.
| model_dict = public_model_service.get_by_id(db=db, model_id=model_id, current_user=current_user) | ||
| model_name = model_dict.get("name") | ||
| except Exception: | ||
| return CheckReferencesResponse( | ||
| is_referenced=False, | ||
| referenced_by=[] | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silent exception handling may hide errors.
Both exception handlers on Lines 102-108 and 127-129 catch all exceptions and return empty results without logging. This could mask legitimate errors like database connection failures or permission issues.
Add logging to understand failures:
+import logging
+logger = logging.getLogger(__name__)
+
try:
model_dict = public_model_service.get_by_id(db=db, model_id=model_id, current_user=current_user)
model_name = model_dict.get("name")
except Exception:
+ logger.warning(f"Failed to fetch model {model_id} for reference check", exc_info=True)
return CheckReferencesResponse(
is_referenced=False,
referenced_by=[]
)
# ...
except Exception:
+ logger.error(f"Failed to query bots for model {model_id} references", exc_info=True)
# If bot model doesn't exist or query fails, return empty list
passAlso applies to: 127-129
🧰 Tools
🪛 Ruff (0.14.5)
104-104: Do not catch blind exception: Exception
(BLE001)
| <div className="flex flex-col"> | ||
| <label className="block text-base font-medium text-text-primary mb-1"> | ||
| Default Haiku Model | ||
| </label> | ||
| <input | ||
| type="text" | ||
| value={formData.default_haiku_model || ''} | ||
| onChange={e => onChange('default_haiku_model', e.target.value)} | ||
| placeholder="claude-3-5-haiku-20241022" | ||
| className="w-full px-4 py-2 bg-base rounded-md text-text-primary placeholder:text-text-muted focus:outline-none focus:ring-2 focus:ring-primary/40 border border-transparent text-base" | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded label should use i18n.
The label "Default Haiku Model" on Line 68 is hardcoded, unlike all other labels which use t() for translations. This breaks consistency and won't support localization for Chinese users.
Apply this diff:
<div className="flex flex-col">
<label className="block text-base font-medium text-text-primary mb-1">
- Default Haiku Model
+ {t('settings.model.default_haiku_model')}
</label>
<input
type="text"
value={formData.default_haiku_model || ''}
onChange={e => onChange('default_haiku_model', e.target.value)}
- placeholder="claude-3-5-haiku-20241022"
+ placeholder={t('settings.model.default_haiku_model_placeholder')}
className="w-full px-4 py-2 bg-base rounded-md text-text-primary placeholder:text-text-muted focus:outline-none focus:ring-2 focus:ring-primary/40 border border-transparent text-base"
/>
</div>You'll need to add the corresponding keys to the i18n files.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/features/settings/components/modelForms/ClaudeModelForm.tsx
around lines 66 to 77, replace the hardcoded label "Default Haiku Model" with a
translated string using the i18n t() function (e.g.
t('settings.models.defaultHaikuModel')) and ensure the t import is present if
not already; then add the key "settings.models.defaultHaikuModel": "Default
Haiku Model" to the English i18n file and corresponding translated entries to
other locale files (e.g. zh) so the label is localized.
| const DeepSeekModelForm: React.FC<DeepSeekModelFormProps> = ({ formData, onChange, errors = {} }) => { | ||
| const { t } = useTranslation('common'); | ||
|
|
||
| return ( | ||
| <div className="space-y-3"> | ||
| <div className="flex flex-col"> | ||
| <label className="block text-base font-medium text-text-primary mb-1"> | ||
| {t('settings.model.model_id')} <span className="text-red-400">*</span> | ||
| </label> | ||
| <input | ||
| type="text" | ||
| value={formData.model_id || ''} | ||
| onChange={e => onChange('model_id', e.target.value)} | ||
| placeholder={t('settings.model.model_id_placeholder')} | ||
| className={`w-full px-4 py-2 bg-base rounded-md text-text-primary placeholder:text-text-muted focus:outline-none focus:ring-2 text-base ${ | ||
| errors.model_id | ||
| ? 'border border-red-400 focus:ring-red-300' | ||
| : 'border border-transparent focus:ring-primary/40' | ||
| }`} | ||
| /> | ||
| </div> | ||
|
|
||
| <div className="flex flex-col"> | ||
| <label className="block text-base font-medium text-text-primary mb-1"> | ||
| {t('settings.model.api_key')} <span className="text-red-400">*</span> | ||
| </label> | ||
| <input | ||
| type="password" | ||
| value={formData.api_key || ''} | ||
| onChange={e => onChange('api_key', e.target.value)} | ||
| placeholder={t('settings.model.api_key_placeholder')} | ||
| className={`w-full px-4 py-2 bg-base rounded-md text-text-primary placeholder:text-text-muted focus:outline-none focus:ring-2 text-base ${ | ||
| errors.api_key | ||
| ? 'border border-red-400 focus:ring-red-300' | ||
| : 'border border-transparent focus:ring-primary/40' | ||
| }`} | ||
| /> | ||
| </div> | ||
|
|
||
| <div className="flex flex-col"> | ||
| <label className="block text-base font-medium text-text-primary mb-1"> | ||
| {t('settings.model.base_url')} | ||
| </label> | ||
| <input | ||
| type="text" | ||
| value={formData.base_url || ''} | ||
| onChange={e => onChange('base_url', e.target.value)} | ||
| placeholder="https://api.deepseek.com" | ||
| className="w-full px-4 py-2 bg-base rounded-md text-text-primary placeholder:text-text-muted focus:outline-none focus:ring-2 focus:ring-primary/40 border border-transparent text-base" | ||
| /> | ||
| </div> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
High code duplication across provider forms.
This component's structure is nearly identical to ClaudeModelForm, OpenAIModelForm, OpenRouterModelForm, GLMModelForm, and QwenModelForm. The only differences are the component name and static placeholder values (plus Claude's extra default_haiku_model field).
Consider extracting a shared base component or using a configuration-driven approach to reduce duplication and improve maintainability.
Example approach:
// Shared component
interface ProviderFormField {
name: string;
type: 'text' | 'password';
required?: boolean;
placeholder?: string;
}
interface ProviderFormConfig {
fields: ProviderFormField[];
}
const providerConfigs: Record<string, ProviderFormConfig> = {
deepseek: {
fields: [
{ name: 'model_id', type: 'text', required: true },
{ name: 'api_key', type: 'password', required: true },
{ name: 'base_url', type: 'text', placeholder: 'https://api.deepseek.com' }
]
},
// ... other providers
};
const ProviderModelForm: React.FC<Props> = ({ provider, formData, onChange, errors }) => {
const config = providerConfigs[provider];
return (
<div className="space-y-3">
{config.fields.map(field => (
<FormField key={field.name} field={field} value={formData[field.name]}
onChange={onChange} error={errors[field.name]} />
))}
</div>
);
};🤖 Prompt for AI Agents
In frontend/src/features/settings/components/modelForms/DeepSeekModelForm.tsx
around lines 14-67, this form duplicates structure used by other provider forms
(Claude, OpenAI, OpenRouter, GLM, Qwen); extract a generic ProviderModelForm
driven by a provider config to remove duplication: create a shared form
component that accepts a config (field name, type, required, placeholder) and
props (formData, onChange, errors), move the repeated input/label rendering into
that component, and replace DeepSeekModelForm with a thin wrapper that supplies
the deepseek config ({ model_id, api_key, base_url }) so placeholders and any
provider-specific extra fields can be handled via config or optional overrides.
Summary
This PR implements a comprehensive model management system in the settings page, allowing users to configure and manage AI model providers through a dedicated UI.
Frontend Changes
ModelListcomponent with Agent filtering and full CRUD operationsModelEditcomponent with dynamic provider-specific configuration formsBackend Changes
POST /api/models/testendpoint for validating model credentialsGET /api/models/{id}/check-referencesto check Bot dependenciesKey Features
UI/UX Improvements
Test Plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.