Skip to content

Conversation

@qdaxb
Copy link
Contributor

@qdaxb qdaxb commented Nov 21, 2025

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

  • ✅ Added new "Models" tab in settings page (positioned between Integrations and Bots tabs)
  • ✅ Created ModelList component with Agent filtering and full CRUD operations
  • ✅ Created ModelEdit component with dynamic provider-specific configuration forms
  • ✅ Implemented 6 provider-specific forms: Claude, OpenAI, OpenRouter, DeepSeek, GLM, Qwen
  • ✅ Extended models API client with complete CRUD, test connection, and reference checking methods
  • ✅ Added comprehensive Chinese and English i18n translations
  • ✅ Integrated CubeIcon for consistent model representation

Backend Changes

  • ✅ Added POST /api/models/test endpoint for validating model credentials
  • ✅ Added GET /api/models/{id}/check-references to check Bot dependencies
  • ✅ Enhanced model deletion protection to prevent removing models in use

Key Features

  • Agent-based filtering: Filter models by Agent type (ClaudeCode, Agno)
  • Provider-specific forms: Dynamic forms adapt to selected provider
  • Connection testing: Validate credentials before saving
  • Dependency checking: Prevents deletion of models referenced by Bots
  • Responsive design: Mobile and desktop layouts with consistent UX
  • Real-time status: Active/inactive indicators for all models
  • Clone support: Duplicate existing model configurations

UI/UX Improvements

  • Consistent card-based layout matching Bot management
  • Inline editing with drawer-style forms
  • Clear visual hierarchy and action buttons
  • Comprehensive error handling and user feedback
  • Loading states for async operations

Test Plan

  • Verify new "Models" tab appears in settings page
  • Test creating models for each provider type
  • Test editing and updating existing models
  • Test connection validation functionality
  • Verify Bot reference checking before deletion
  • Test Agent-based filtering
  • Verify mobile responsive layout
  • Test i18n with both Chinese and English
  • Verify model cloning functionality

Summary by CodeRabbit

  • New Features
    • Added a dedicated Models section in settings for managing AI model integrations across multiple providers (Claude, OpenAI, OpenRouter, DeepSeek, GLM, Qwen).
    • Introduced test connection functionality to validate model credentials before saving.
    • Enabled create, edit, duplicate, and delete operations for models with reference checking to prevent breaking active bots.
    • Added model filtering by agent and status indicators for active/inactive models.

✏️ Tip: You can customize this high-level summary in your review settings.

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
@coderabbitai
Copy link

coderabbitai bot commented Nov 21, 2025

Walkthrough

A 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

Cohort / File(s) Summary
Backend API Models & Endpoints
backend/app/api/endpoints/adapter/models.py
Added TestConnectionRequest, TestConnectionResponse, BotReference, and CheckReferencesResponse models. Introduced /test endpoint for provider-based connection validation and /{model_id}/check-references endpoint to identify active bot references to a model.
Frontend API Service Layer
frontend/src/apis/models.ts
Defined ModelDetail, ModelListResponse, ModelCreateRequest, TestConnectionRequest, and CheckReferencesResponse interfaces. Extended modelApis with seven methods: getModels, getModelById, createModel, updateModel, deleteModel, testConnection, and checkReferences.
Model Editor Component
frontend/src/features/settings/components/ModelEdit.tsx
Created ModelEdit React component with multi-provider support, form validation, test connection functionality, save/create operations, keyboard handling (Escape to close), and a two-pane responsive layout with help panel.
Model Provider Forms
frontend/src/features/settings/components/modelForms/*
Added six provider-specific form components (ClaudeModelForm, OpenAIModelForm, OpenRouterModelForm, DeepSeekModelForm, GLMModelForm, QwenModelForm), each rendering model_id, api_key, base_url fields with i18n translations and error styling.
Model List Component
frontend/src/features/settings/components/ModelList.tsx
Created ModelList client component with agent-based filtering, CRUD support via ModelEdit modal, delete confirmation with reference pre-check, optimistic UI updates, and rich status/provider display.
Settings Page Integration
frontend/src/app/settings/page.tsx
Imported ModelList component and CubeIcon. Added new "models" tab (index 1) between integrations and bots with adjusted tab mappings. Reordered subsequent tabs and included ModelList panel rendering.
Internationalization
frontend/src/i18n/locales/en/common.json, frontend/src/i18n/locales/zh-CN/common.json
Added comprehensive settings.model translation objects for both English and Chinese, covering UI labels, actions, field names, validation/error messages, provider names, and operation feedback.

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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~35 minutes

Areas requiring extra attention:

  • ModelList component (frontend/src/features/settings/components/ModelList.tsx): Complex state management with filtering, loading states, editing modes, and deletion flows. Verify optimistic UI updates and error handling edge cases.
  • ModelEdit component (frontend/src/features/settings/components/ModelEdit.tsx): Multi-provider form logic, validation error tracking, and provider-specific field handling. Check that form state resets correctly when switching providers.
  • Backend endpoints (backend/app/api/endpoints/adapter/models.py): Validate that reference checking logic correctly scans bot configurations and handles null-safety for model lookups.
  • Provider form components: Verify consistency in field naming, error styling, and i18n key usage across all six implementations (Claude, OpenAI, OpenRouter, DeepSeek, GLM, Qwen).

Suggested reviewers

  • feifei325

Poem

🐰 Six providers, forms so neat,
Models tested, connections sweet,
Tabs rearranged with a cubic touch,
Translation keys for languages much—
A settings feast, both broad and fleet!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding a model management feature to the settings page, which is reflected across all modified files.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch wegent/add-model-management-settings-page

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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_id and api_key presence) 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:

  1. Uses provider-specific SDK clients
  2. Makes minimal API calls (e.g., list models endpoint)
  3. Handles timeouts and provider-specific errors
  4. Returns meaningful error messages

49-49: Unused parameter should be removed or utilized.

The current_user parameter is declared but never used in the test_connection function. 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 == True which is flagged by static analysis. Use implicit truthiness or is True for 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 config

The new models tab is correctly threaded through the index↔name maps and both desktop/mobile Tab.List + Tab.Panels, so ?tab=models deep-linking should work as expected.

To reduce future drift as more tabs are added, consider deriving tabIndexToName / tabNameToIndex and the Tab.List/Tab.Panels from a single tabs config 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 conditions

The agent filter logic is functionally correct and falls back to all models on errors or empty results, which is good. However, since filterByAgent is async and depends on selectedAgent, rapid agent changes or refreshes could allow an older request to overwrite filteredModels after 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 around modelToDelete lifecycle

The delete flow is mostly solid (pre-check references, confirmation dialog, optimistic local removal). Two small robustness improvements:

  1. handleConfirmDelete uses if (!modelToDelete) return;, which would silently no-op if an ID of 0 is ever valid. A null check is safer.
  2. Dialog’s onOpenChange={setDeleteConfirmVisible} can close the dialog (e.g., overlay click) without clearing modelToDelete, 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 components

Other settings components (e.g., frontend/src/app/settings/page.tsx and ModelList.tsx) import useTranslation from @/hooks/useTranslation, but this file pulls it directly from react-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 avoid undefined messages

handleTestConnection currently assumes error is an Error and concatenates error.message, which can result in messages like "Connection test failed: undefined" if a non-Error is thrown.

You can mirror the pattern used in ModelList for 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 msg accordingly.)


172-212: Use an update-specific i18n key when edit save fails

handleSave falls back to t('settings.model.errors.create_failed') for all failures, including updates, even though settings.model.errors.update_failed exists 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

📥 Commits

Reviewing files that changed from the base of the PR and between cc3671a and f097f89.

📒 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 the bots parameter injected by the frontend component at frontend/src/features/settings/components/ModelList.tsx:146. The variable is constructed from botNames (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.json and frontend/src/i18n/locales/zh-CN/common.json are 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]: any in ModelDetail.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 good

The 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 models state and ModelEdit. 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 usage

The new settings.models label and the settings.model block (fields, actions, statuses, providers, and error messages) line up with all usages in ModelList and ModelEdit, including the {{bots}} interpolation for delete_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 extensible

The two-pane layout (left form, right help), required field indicators, provider selector, and the renderProviderForm switch 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.

Comment on lines +84 to +88
except Exception as e:
return TestConnectionResponse(
success=False,
message=f"Connection test failed: {str(e)}"
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Comment on lines +102 to +108
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=[]
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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
         pass

Also applies to: 127-129

🧰 Tools
🪛 Ruff (0.14.5)

104-104: Do not catch blind exception: Exception

(BLE001)

Comment on lines +66 to +77
<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>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Comment on lines +14 to +67
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>
);
};
Copy link

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.

@qdaxb qdaxb closed this Nov 29, 2025
@qdaxb qdaxb deleted the wegent/add-model-management-settings-page branch November 29, 2025 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants