feat(admin): add system administration module with user and model management #246
Conversation
…agement Add comprehensive admin module for managing users and public models: Backend changes: - Add 'role' field to User model (admin/user) - Create Alembic migration to add role column and set existing admin user - Update get_admin_user to check role instead of username - Add admin schemas for user and model management - Extend admin API with endpoints for: - User CRUD operations with role management - Password reset functionality - User status toggle (enable/disable) - Public model CRUD operations - System statistics Frontend changes: - Create admin page at /admin route - Add UserList component for user management - Add PublicModelList component for public model management - Create admin API client with all admin endpoints - Add role and auth_source to User type - Add admin navigation entry in UserMenu (visible to admins only) - Add i18n translations for admin module (en/zh-CN) Security: - Route guard: non-admins redirected to access denied page - API protection: all endpoints require admin role - Self-protection: admins cannot delete/disable themselves - Role protection: system must have at least one admin
WalkthroughAdds role-based admin features: DB migration and User.role field; backend admin schemas and many admin endpoints; security check switched to role-based; frontend admin API, types, UI pages/components, menu entry, and i18n for en and zh-CN. Changes
Sequence Diagram(s)sequenceDiagram
participant AdminUI as Frontend Admin UI
participant AdminAPI as Backend /admin endpoints
participant Auth as Backend /security
participant DB as Database
AdminUI->>AdminAPI: GET /admin/users?page,limit,include_inactive (with token)
AdminAPI->>Auth: validate token -> get current_user
Auth-->>AdminAPI: current_user (role)
alt current_user.role == "admin"
AdminAPI->>DB: SELECT users ... (apply filters/pagination)
DB-->>AdminAPI: users list
AdminAPI-->>AdminUI: 200 AdminUserListResponse
else not admin
AdminAPI-->>AdminUI: 403 Forbidden
end
AdminUI->>AdminAPI: POST /admin/users (create payload)
AdminAPI->>Auth: validate token -> get current_user
Auth-->>AdminAPI: current_user (role)
alt current_user.role == "admin"
AdminAPI->>AdminAPI: hash password
AdminAPI->>DB: INSERT user (role default 'user') / RETURNING id
DB-->>AdminAPI: created user
AdminAPI-->>AdminUI: 201 AdminUserResponse
else not admin
AdminAPI-->>AdminUI: 403 Forbidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/app/core/security.py (1)
207-212: Update test fixture to explicitly set role="admin" for admin user.The test fixture at
backend/tests/conftest.pycreates an admin user without setting therolefield. Since the User model defaultsrole="user", the test user hasrole="user"instead ofrole="admin", causingget_admin_user()to reject it with HTTP 403.Set
role="admin"in thetest_admin_userfixture:admin = User( user_name="admin", password_hash=get_password_hash("adminpassword123"), email="admin@example.com", is_active=True, role="admin" # Add this line )The security check logic itself is correct; the issue is only in the test fixture setup.
backend/app/api/endpoints/admin.py (2)
584-596: Admin token expiration of 500 years is a security risk.A token valid for 500 years (262,800,000 minutes) is effectively permanent with no revocation mechanism. If compromised, this token provides indefinite admin access. Consider implementing token rotation or a reasonable expiration with refresh capability.
At minimum, document this security trade-off clearly or consider:
- Adding a token revocation mechanism
- Using a shorter expiration with refresh tokens
- Logging token generation for audit purposes
834-836: DELETE endpoint should return empty body, not JSON.The
admin_delete_user_resourceendpoint is decorated withstatus_code=status.HTTP_204_NO_CONTENTbut returns a JSON message body. HTTP 204 responses must have no content. This is inconsistent withdelete_useranddelete_public_modelwhich correctly returnNone.- return { - "message": f"Successfully deleted {kind} resource '{name}' for user {user_id}" - } + return None
🧹 Nitpick comments (13)
backend/app/models/user.py (1)
23-24: Consider extracting role values to constants.The role values "admin" and "user" are used as magic strings here and throughout the codebase (security.py, schemas, etc.). Extracting them to named constants or a Python
Enumwould improve maintainability and reduce typo risks.# Example: Add at module level or in a constants file class UserRole: ADMIN = "admin" USER = "user"Based on coding guidelines requiring extraction of magic numbers/strings to named constants.
backend/app/schemas/user.py (1)
65-66: LGTM with optional improvement.The new fields correctly expose role and auth_source from the database model.
For stricter validation, consider using
Literaltypes:from typing import Literal role: Literal["admin", "user"] = "user" auth_source: Literal["password", "oidc", "unknown"] = "unknown"This would catch invalid values at the Pydantic validation layer.
frontend/src/types/api.ts (1)
8-9: Duplicate type definition detected.
UserRoleis also defined infrontend/src/apis/admin.ts(line 7). Consider consolidating these type definitions to a single source of truth inapi.tsand re-exporting fromadmin.tsto avoid drift.// In frontend/src/apis/admin.ts, replace local definition with: +import { UserRole, AuthSource } from '@/types/api'; -export type UserRole = 'admin' | 'user';frontend/src/app/admin/page.tsx (2)
70-71: Preferinoperator or direct lookup overhasOwnProperty.Using
hasOwnPropertydirectly on an object literal is safe here, but the pattern could be simplified:- if (tabParam && tabNameToIndex.hasOwnProperty(tabParam)) { + if (tabParam && tabParam in tabNameToIndex) { return tabNameToIndex[tabParam];
79-90: Consider reusing existing responsive detection hook.The codebase already has
useIsDesktop(seen inTopNavigation.tsximports). This duplicates that logic with a manual resize listener.+import { useIsDesktop } from '@/hooks/useMediaQuery'; // or wherever it's defined function AdminContent() { // ... - const [isDesktop, setIsDesktop] = useState(false); - - useEffect(() => { - const checkScreenSize = () => { - setIsDesktop(window.innerWidth >= 1024); - }; - - checkScreenSize(); - window.addEventListener('resize', checkScreenSize); - return () => window.removeEventListener('resize', checkScreenSize); - }, []); + const isDesktop = useIsDesktop();frontend/src/features/admin/components/UserList.tsx (2)
221-233: Toggle status lacks confirmation dialog.
handleToggleStatusperforms an immediate state change without user confirmation, while delete has a confirmation dialog. Consider adding confirmation for consistency, especially for disabling users which is a potentially disruptive action.
1-3: File naming convention.As per coding guidelines, component files in
**/components/**/*.{ts,tsx}should use kebab-case naming (e.g.,user-list.tsxinstead ofUserList.tsx). This applies to this file and likelyPublicModelList.tsxas well.Based on coding guidelines, component files MUST use kebab-case naming convention.
frontend/src/features/admin/components/PublicModelList.tsx (3)
293-297: Hardcoded English strings should use i18n translations.Several UI strings are hardcoded instead of using the translation hook: "Active", "Inactive", "Cancel", "Create", "Save", "Delete". This breaks consistency with the rest of the component which uses
t()for localization.- <Tag variant="success">Active</Tag> + <Tag variant="success">{t('public_models.status.active')}</Tag> ) : ( - <Tag variant="error">Inactive</Tag> + <Tag variant="error">{t('public_models.status.inactive')}</Tag>Similarly for dialog buttons:
- <Button variant="outline" onClick={() => setIsCreateDialogOpen(false)}> - Cancel - </Button> - <Button onClick={handleCreateModel} disabled={saving}> - {saving && <Loader2 className="mr-2 h-4 w-4 animate-spin" />} - Create - </Button> + <Button variant="outline" onClick={() => setIsCreateDialogOpen(false)}> + {t('common.cancel')} + </Button> + <Button onClick={handleCreateModel} disabled={saving}> + {saving && <Loader2 className="mr-2 h-4 w-4 animate-spin" />} + {t('common.create')} + </Button>Also applies to: 389-395, 441-447, 462-468
236-247: Helper functions for extracting model metadata are fragile.
getModelProviderandgetModelIdrely on a specific JSON structure (json.env.modelandjson.env.model_id). If the JSON config schema changes or uses different keys, these will silently return "Unknown" or "N/A". Consider documenting the expected JSON structure or making these more resilient.
53-54: Unused pagination state variables.
totalandsetPageare declared buttotalis set and never used for display (no pagination UI), andpageis never updated. Either implement pagination controls or remove these unused variables.- const [total, setTotal] = useState(0); - const [page, setPage] = useState(1); + const [page] = useState(1);And update
fetchModels:- setTotal(response.total);backend/app/schemas/admin.py (1)
66-66: Consider using stricter type annotation forjsonfield.The
json: dicttype is very permissive. UsingDict[str, Any]would be more explicit, matching the frontend'sRecord<string, unknown>type, and aligns with Python typing conventions.+from typing import Optional, List, Literal, Dict, Any -from typing import Optional, List, Literal ... class PublicModelCreate(BaseModel): """Public model creation model""" name: str = Field(..., min_length=1, max_length=100) namespace: str = Field(default="default", max_length=100) - json: dict + json: Dict[str, Any]Apply the same to
PublicModelUpdateandPublicModelResponse.frontend/src/apis/admin.ts (1)
163-165: Inconsistent query parameter construction style.
getPublicModelsuses template literal interpolation whilegetUsersusesURLSearchParams. While both work, usingURLSearchParamsconsistently is safer as it properly encodes parameter values.async getPublicModels(page: number = 1, limit: number = 20): Promise<AdminPublicModelListResponse> { - return apiClient.get(`/admin/public-models?page=${page}&limit=${limit}`); + const params = new URLSearchParams(); + params.append('page', String(page)); + params.append('limit', String(limit)); + return apiClient.get(`/admin/public-models?${params.toString()}`); },backend/app/api/endpoints/admin.py (1)
517-517: Move import to top of file.The
Taskmodel is imported inside the function body. Per PEP 8 and Python conventions, imports should be at the top of the file unless there's a circular import issue.Move to the top imports section:
from app.models.user import User from app.models.public_model import PublicModel +from app.models.task import TaskThen remove line 517.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/alembic/versions/b2c3d4e5f6a7_add_role_to_users.py(1 hunks)backend/app/api/endpoints/admin.py(4 hunks)backend/app/core/security.py(1 hunks)backend/app/models/user.py(1 hunks)backend/app/schemas/admin.py(1 hunks)backend/app/schemas/user.py(2 hunks)frontend/src/apis/admin.ts(1 hunks)frontend/src/app/admin/page.tsx(1 hunks)frontend/src/features/admin/components/PublicModelList.tsx(1 hunks)frontend/src/features/admin/components/UserList.tsx(1 hunks)frontend/src/features/layout/UserMenu.tsx(3 hunks)frontend/src/i18n/locales/en/admin.json(1 hunks)frontend/src/i18n/locales/en/common.json(1 hunks)frontend/src/i18n/locales/zh-CN/admin.json(1 hunks)frontend/src/i18n/locales/zh-CN/common.json(1 hunks)frontend/src/types/api.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
All code comments, inline comments, block comments, docstrings, TODO/FIXME annotations, and type hints descriptions MUST be written in English
Files:
backend/alembic/versions/b2c3d4e5f6a7_add_role_to_users.pyfrontend/src/features/admin/components/UserList.tsxbackend/app/models/user.pyfrontend/src/apis/admin.tsfrontend/src/types/api.tsbackend/app/schemas/user.pyfrontend/src/features/admin/components/PublicModelList.tsxfrontend/src/app/admin/page.tsxfrontend/src/features/layout/UserMenu.tsxbackend/app/schemas/admin.pybackend/app/core/security.pybackend/app/api/endpoints/admin.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code MUST be PEP 8 compliant with Black formatter (line length: 88) and isort for import organization
Python code MUST include type hints for all functions and variables
Python functions SHOULD NOT exceed 50 lines (preferred maximum)
Python functions and classes MUST have descriptive names and public functions/classes MUST include docstrings
Python code MUST extract magic numbers to named constants
Files:
backend/alembic/versions/b2c3d4e5f6a7_add_role_to_users.pybackend/app/models/user.pybackend/app/schemas/user.pybackend/app/schemas/admin.pybackend/app/core/security.pybackend/app/api/endpoints/admin.py
**/backend/alembic/versions/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend Alembic database migrations MUST be reviewed before applying, especially auto-generated migrations
Files:
backend/alembic/versions/b2c3d4e5f6a7_add_role_to_users.py
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: TypeScript code MUST use strict mode with type checking enabled
TypeScript/React code MUST use Prettier formatter with single quotes, no semicolons
TypeScript/React code MUST pass ESLint with Next.js configuration
React component names MUST use PascalCase convention
Files:
frontend/src/features/admin/components/UserList.tsxfrontend/src/apis/admin.tsfrontend/src/types/api.tsfrontend/src/features/admin/components/PublicModelList.tsxfrontend/src/app/admin/page.tsxfrontend/src/features/layout/UserMenu.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: React components MUST use functional components with hooks instead of class-based components
Useconstoverlet, never usevarin TypeScript/JavaScript code
Files:
frontend/src/features/admin/components/UserList.tsxfrontend/src/apis/admin.tsfrontend/src/types/api.tsfrontend/src/features/admin/components/PublicModelList.tsxfrontend/src/app/admin/page.tsxfrontend/src/features/layout/UserMenu.tsx
**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
React component files MUST use kebab-case naming convention
Files:
frontend/src/features/admin/components/UserList.tsxfrontend/src/features/admin/components/PublicModelList.tsx
**/src/**/*.{tsx,jsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
Frontend Tailwind CSS MUST use provided CSS variables for color system (e.g., --color-bg-base, --color-text-primary, --color-primary)
Files:
frontend/src/features/admin/components/UserList.tsxfrontend/src/features/admin/components/PublicModelList.tsxfrontend/src/app/admin/page.tsxfrontend/src/features/layout/UserMenu.tsx
**/src/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.{tsx,jsx}: Frontend responsive design MUST follow mobile-first approach with Tailwind breakpoints
Frontend React forms MUST use react-hook-form with zod validation schema
Frontend components MUST use shadcn/ui component library fromfrontend/src/components/ui/
Files:
frontend/src/features/admin/components/UserList.tsxfrontend/src/features/admin/components/PublicModelList.tsxfrontend/src/app/admin/page.tsxfrontend/src/features/layout/UserMenu.tsx
**/backend/app/models/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend SQLAlchemy models MUST be created in
app/models/directory
Files:
backend/app/models/user.py
**/types/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript type definitions MUST be organized in
src/types/directory
Files:
frontend/src/types/api.ts
**/backend/app/schemas/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend Pydantic schemas MUST be created in
app/schemas/directory
Files:
backend/app/schemas/user.pybackend/app/schemas/admin.py
**/backend/app/api/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Backend API route handlers MUST be created in
app/api/directory
Files:
backend/app/api/endpoints/admin.py
🧠 Learnings (3)
📚 Learning: 2025-12-01T06:44:24.499Z
Learnt from: CR
Repo: wecode-ai/Wegent PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T06:44:24.499Z
Learning: Applies to **/backend/alembic/versions/*.py : Backend Alembic database migrations MUST be reviewed before applying, especially auto-generated migrations
Applied to files:
backend/alembic/versions/b2c3d4e5f6a7_add_role_to_users.py
📚 Learning: 2025-12-01T06:44:24.499Z
Learnt from: CR
Repo: wecode-ai/Wegent PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T06:44:24.499Z
Learning: Applies to **/backend/app/schemas/**/*.py : Backend Pydantic schemas MUST be created in `app/schemas/` directory
Applied to files:
backend/app/schemas/admin.py
📚 Learning: 2025-12-01T06:44:24.499Z
Learnt from: CR
Repo: wecode-ai/Wegent PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T06:44:24.499Z
Learning: Applies to **/backend/app/models/**/*.py : Backend SQLAlchemy models MUST be created in `app/models/` directory
Applied to files:
backend/app/schemas/admin.py
🧬 Code graph analysis (6)
frontend/src/apis/admin.ts (3)
frontend/src/types/api.ts (2)
UserRole(8-8)AuthSource(9-9)backend/app/schemas/admin.py (3)
AdminUserCreate(12-19)AdminUserUpdate(22-28)SystemStats(101-108)frontend/src/apis/client.ts (1)
apiClient(105-105)
frontend/src/types/api.ts (2)
frontend/src/apis/admin.ts (1)
UserRole(8-8)backend/app/models/user.py (1)
User(14-37)
frontend/src/features/admin/components/PublicModelList.tsx (1)
frontend/src/apis/admin.ts (4)
AdminPublicModel(51-59)adminApis(89-199)AdminPublicModelCreate(66-70)AdminPublicModelUpdate(72-77)
frontend/src/app/admin/page.tsx (5)
frontend/src/components/ui/button.tsx (1)
Button(55-55)frontend/src/features/common/UserContext.tsx (2)
useUser(148-148)UserProvider(28-146)frontend/src/features/layout/TopNavigation.tsx (1)
TopNavigation(23-86)frontend/src/features/layout/GithubStarButton.tsx (1)
GithubStarButton(19-137)frontend/src/features/layout/UserMenu.tsx (1)
UserMenu(21-83)
backend/app/schemas/admin.py (2)
frontend/src/apis/admin.ts (4)
AdminUserCreate(27-33)AdminUserUpdate(35-40)AdminUserListResponse(22-25)SystemStats(80-86)backend/app/schemas/user.py (1)
Config(70-71)
backend/app/api/endpoints/admin.py (6)
backend/app/core/security.py (2)
get_admin_user(194-213)get_password_hash(67-77)backend/app/models/kind.py (1)
Kind(25-42)backend/app/models/user.py (1)
User(14-37)backend/app/models/public_model.py (1)
PublicModel(19-38)backend/app/schemas/admin.py (11)
AdminUserCreate(12-19)AdminUserUpdate(22-28)AdminUserResponse(37-50)AdminUserListResponse(53-57)PasswordReset(31-34)PublicModelCreate(61-66)PublicModelUpdate(69-75)PublicModelResponse(78-90)PublicModelListResponse(93-97)SystemStats(101-108)RoleUpdate(112-115)backend/app/api/dependencies.py (1)
get_db(12-21)
🪛 GitHub Actions: Tests
backend/app/core/security.py
[error] 209-209: TestGetAdminUser.get_admin_user_with_admin failed: admin user should have access but get_admin_user raises HTTP 403 Forbidden.
🪛 Ruff (0.14.7)
backend/app/api/endpoints/admin.py
56-56: 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)
57-57: Unused function argument: current_user
(ARG001)
57-57: 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)
64-64: Avoid equality comparisons to True; use User.is_active: for truth checks
Replace with User.is_active
(E712)
112-112: 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)
113-113: Unused function argument: current_user
(ARG001)
113-113: 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)
163-163: 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)
164-164: 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)
181-181: Avoid equality comparisons to True; use User.is_active: for truth checks
Replace with User.is_active
(E712)
225-225: 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)
226-226: 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)
251-251: 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)
252-252: Unused function argument: current_user
(ARG001)
252-252: 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)
287-287: 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)
288-288: 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)
322-322: 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)
323-323: 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)
332-332: Avoid equality comparisons to True; use User.is_active: for truth checks
Replace with User.is_active
(E712)
361-361: 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)
362-362: Unused function argument: current_user
(ARG001)
362-362: 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)
391-391: 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)
392-392: Unused function argument: current_user
(ARG001)
392-392: 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)
434-434: 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)
435-435: Unused function argument: current_user
(ARG001)
435-435: 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)
488-488: 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)
489-489: Unused function argument: current_user
(ARG001)
489-489: 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)
511-511: 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)
512-512: Unused function argument: current_user
(ARG001)
512-512: 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)
520-520: Avoid equality comparisons to True; use User.is_active: for truth checks
Replace with User.is_active
(E712)
521-521: Avoid equality comparisons to True; use User.is_active: for truth checks
Replace with User.is_active
(E712)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (20)
frontend/src/i18n/locales/en/common.json (1)
45-46: LGTM!The new
navigation.admintranslation key is correctly added and aligns with the UserMenu component usage.frontend/src/i18n/locales/zh-CN/common.json (1)
45-46: LGTM!Chinese translation for admin navigation is correctly added and consistent with the English locale structure.
frontend/src/i18n/locales/zh-CN/admin.json (1)
1-146: LGTM!Comprehensive and well-structured Chinese translations for the admin module. The key organization mirrors expected admin UI sections (users, public_models, access control) and translations are contextually appropriate.
frontend/src/features/layout/UserMenu.tsx (2)
45-64: Admin menu item correctly integrates role-based access.The conditional rendering based on
user?.role === 'admin'is correct. The implementation follows existing patterns in the component.Minor consideration: Other menu items like DocsButton and ThemeToggle receive a
closeoronTogglecallback to close the menu after interaction. The admin Link navigates to a new page, so the menu will close via page navigation anyway—this is acceptable.
25-25: Consider defensive handling for unexpected role values.Currently
isAdminis only true for exact match'admin'. This is correct, but if role values are ever extended or have case variations, this could silently fail. The current implementation is fine given the backend enforces role values.backend/app/schemas/user.py (1)
89-89: Consistent with UserInDB.The role field addition to UserInfo aligns with the UserInDB changes and supports the admin list functionality.
frontend/src/types/api.ts (1)
16-17: Optional fields are appropriate for backward compatibility.The
roleandauth_sourcefields are correctly marked as optional since existing API responses from older backends may not include these fields. This aligns with the backend model where these have defaults.backend/alembic/versions/b2c3d4e5f6a7_add_role_to_users.py (1)
60-62: Downgrade is clean.The downgrade correctly uses standard Alembic
op.drop_column, which is portable across databases.frontend/src/i18n/locales/en/admin.json (1)
1-146: Comprehensive and well-structured localization.The translation file covers all admin UI scenarios with proper key organization, interpolation placeholders, and error handling messages. Structure aligns well with the admin components.
frontend/src/app/admin/page.tsx (1)
246-254: Good composition with UserProvider and Suspense.The page correctly wraps
AdminContentinUserProviderfor auth context andSuspenseforuseSearchParams()requirements in Next.js 15.frontend/src/features/admin/components/UserList.tsx (1)
86-100: Well-structured data fetching with proper dependencies.The
fetchUserscallback correctly includes all dependencies and handles loading/error states appropriately.frontend/src/features/admin/components/PublicModelList.tsx (2)
49-75: Component structure and state management look good.The component is well-organized with clear separation of concerns: fetch logic, dialog states, and form states. The use of
useCallbackforfetchModelswith proper dependencies is appropriate.
96-113: JSON validation implementation is solid.The
validateConfigfunction properly handles edge cases: empty strings, non-object JSON, arrays, and parse errors. The error state management is clear and user-friendly.backend/app/schemas/admin.py (3)
12-19: Schema definitions are well-structured with appropriate validation.The
AdminUserCreateschema has proper field constraints (min/max lengths) and sensible defaults. The use ofLiteralforroleandauth_sourceprovides type safety at validation time.
49-50: Pydantic v2 configuration is correct.Using
from_attributes = TrueinConfigclass is the correct approach for Pydantic v2 (replacing the olderorm_mode = True). This enables ORM model serialization.
111-115: RoleUpdate schema is appropriately minimal.The schema correctly uses
Literal["admin", "user"]to constrain valid role values. This is consistent with the role definitions in other schemas.frontend/src/apis/admin.ts (1)
89-107: Well-structured API methods with proper typing and documentation.The
getUsersmethod has clear parameter typing with defaults, proper query string construction, and good JSDoc documentation. The pattern is consistent across all API methods.backend/app/api/endpoints/admin.py (3)
64-64: SQLAlchemy boolean comparisons are correct despite linter warning.The static analysis hints flag
User.is_active == Trueas E712, but this is required SQLAlchemy syntax for building SQL WHERE clauses. Using justUser.is_activewould not work correctly in this ORM context. These are false positives.Also applies to: 181-181, 332-332, 520-521
109-156: User creation endpoint is well-implemented with proper validation.The endpoint correctly:
- Checks for duplicate usernames
- Validates password requirement for password auth source
- Uses proper password hashing
- Returns appropriate HTTP status (201 CREATED)
171-186: Good self-protection logic for admin operations.The endpoint correctly prevents admins from:
- Deactivating themselves
- Demoting themselves when they're the only admin
This aligns with the PR's security requirements.
| op.execute(""" | ||
| SET @column_exists = ( | ||
| SELECT COUNT(*) | ||
| FROM INFORMATION_SCHEMA.COLUMNS | ||
| WHERE TABLE_SCHEMA = DATABASE() | ||
| AND TABLE_NAME = 'users' | ||
| AND COLUMN_NAME = 'role' | ||
| ); | ||
| """) | ||
|
|
||
| op.execute(""" | ||
| SET @query = IF(@column_exists = 0, | ||
| 'ALTER TABLE users ADD COLUMN role VARCHAR(20) NOT NULL DEFAULT ''user'' AFTER is_active', | ||
| 'SELECT 1' | ||
| ); | ||
| """) | ||
|
|
||
| op.execute("PREPARE stmt FROM @query;") | ||
| op.execute("EXECUTE stmt;") | ||
| op.execute("DEALLOCATE PREPARE stmt;") |
There was a problem hiding this comment.
Migration uses MySQL-specific syntax, breaking portability.
The migration relies on MySQL session variables (SET @variable), PREPARE/EXECUTE/DEALLOCATE, and INFORMATION_SCHEMA queries. The backend model's __table_args__ suggests SQLite support (sqlite_autoincrement), which means this migration will fail on SQLite.
Consider using Alembic's dialect-aware operations or the op.get_bind().dialect.name check:
def upgrade() -> None:
"""Add role column to users table."""
conn = op.get_bind()
inspector = sa.inspect(conn)
columns = [col['name'] for col in inspector.get_columns('users')]
if 'role' not in columns:
op.add_column('users', sa.Column(
'role',
sa.String(20),
nullable=False,
server_default='user'
))
# Set admin role for existing admin users
op.execute("UPDATE users SET role = 'admin' WHERE user_name = 'admin'")| # Set admin role for users with user_name='admin' | ||
| op.execute(""" | ||
| UPDATE users SET role = 'admin' WHERE user_name = 'admin'; | ||
| """) |
There was a problem hiding this comment.
Hardcoded username for admin role assignment.
Setting role='admin' for user_name='admin' assumes a specific naming convention. If the initial admin was created with a different username, they won't receive admin privileges after migration.
Consider documenting this assumption or providing a separate mechanism (e.g., environment variable or config) to specify the initial admin username.
🤖 Prompt for AI Agents
In backend/alembic/versions/b2c3d4e5f6a7_add_role_to_users.py around lines
54–57, the migration hardcodes user_name='admin' when granting the admin role;
change it to read an initial-admin username from configuration (e.g., an
environment variable like INITIAL_ADMIN_USERNAME with a sensible default of
'admin') and use that variable in a parameterized op.execute call so the
migration grants role to the configured user rather than a fixed string; also
add a short comment documenting the assumption and, if desired, a fallback log
or warning when the configured username is not present so operators know to
verify admin assignment after migration.
| ) | ||
|
|
||
| # Create user | ||
| password_hash = get_password_hash(user_data.password) if user_data.password else get_password_hash("oidc_placeholder") |
There was a problem hiding this comment.
Placeholder password for OIDC users may cause confusion.
When auth_source is "oidc" and no password is provided, a hashed "oidc_placeholder" is stored. While this prevents null in the password_hash column, it creates a non-obvious security concern: if the auth_source is later changed to "password", this placeholder becomes a valid (guessable) password.
Consider using a random, non-guessable placeholder or documenting this behavior clearly:
- password_hash = get_password_hash(user_data.password) if user_data.password else get_password_hash("oidc_placeholder")
+ # For OIDC users, generate a random placeholder that cannot be used for password login
+ import secrets
+ password_hash = get_password_hash(user_data.password) if user_data.password else get_password_hash(secrets.token_urlsafe(32))Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/app/api/endpoints/admin.py around line 134, the code hashes the
literal "oidc_placeholder" when no password is provided for OIDC users; replace
this with a secure approach: either set password_hash to None (update DB column
to nullable and add a migration) or generate a cryptographically random secret
(e.g., 32+ bytes from a secure RNG) and store its hash so the placeholder is
unguessable; additionally add validation to prevent switching auth_source to
"password" without requiring a real password reset.
| # Check name uniqueness if being changed | ||
| if model_data.name and model_data.name != model.name: | ||
| namespace = model_data.namespace or model.namespace | ||
| existing_model = ( | ||
| db.query(PublicModel) | ||
| .filter(PublicModel.name == model_data.name, PublicModel.namespace == namespace) | ||
| .first() | ||
| ) | ||
| if existing_model: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail=f"Public model '{model_data.name}' already exists in namespace '{namespace}'", | ||
| ) |
There was a problem hiding this comment.
Namespace uniqueness check has a subtle bug.
When updating a public model, if only the namespace changes (not the name), the uniqueness check is skipped because it's gated by if model_data.name and model_data.name != model.name. A model with the same name in the new namespace could already exist.
- # Check name uniqueness if being changed
- if model_data.name and model_data.name != model.name:
- namespace = model_data.namespace or model.namespace
+ # Check name+namespace uniqueness if either is being changed
+ new_name = model_data.name if model_data.name is not None else model.name
+ new_namespace = model_data.namespace if model_data.namespace is not None else model.namespace
+ if new_name != model.name or new_namespace != model.namespace:
existing_model = (
db.query(PublicModel)
- .filter(PublicModel.name == model_data.name, PublicModel.namespace == namespace)
+ .filter(
+ PublicModel.name == new_name,
+ PublicModel.namespace == new_namespace,
+ PublicModel.id != model_id
+ )
.first()
)
if existing_model:
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
- detail=f"Public model '{model_data.name}' already exists in namespace '{namespace}'",
+ detail=f"Public model '{new_name}' already exists in namespace '{new_namespace}'",
)🤖 Prompt for AI Agents
In backend/app/api/endpoints/admin.py around lines 447 to 459, the uniqueness
check only runs when the name changes, so changing only the namespace can let a
duplicate (same name in new namespace) slip through; update the logic to compute
the intended new_name = model_data.name or model.name and new_namespace =
model_data.namespace or model.namespace, then query PublicModel for an existing
record with that name and namespace excluding the current model (e.g., filter
PublicModel.id != model.id) and raise the same HTTPException if any exists.
| export type UserRole = 'admin' | 'user'; | ||
| export type AuthSource = 'password' | 'oidc' | 'unknown'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate type definitions with frontend/src/types/api.ts.
UserRole and AuthSource are already defined in frontend/src/types/api.ts (lines 7-9). Re-exporting them here creates potential drift between the definitions. Import and re-export from the central types file instead.
-export type UserRole = 'admin' | 'user';
-export type AuthSource = 'password' | 'oidc' | 'unknown';
+import { UserRole, AuthSource } from '@/types/api';
+export type { UserRole, AuthSource };📝 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.
| export type UserRole = 'admin' | 'user'; | |
| export type AuthSource = 'password' | 'oidc' | 'unknown'; | |
| import { UserRole, AuthSource } from '@/types/api'; | |
| export type { UserRole, AuthSource }; |
🤖 Prompt for AI Agents
In frontend/src/apis/admin.ts around lines 8-9, remove the local duplicate type
definitions for UserRole and AuthSource and instead import them from the central
file frontend/src/types/api.ts and re-export them (e.g. add an import {
UserRole, AuthSource } from 'frontend/src/types/api' and export or re-export as
needed), ensuring there are no duplicate type declarations left in this file.
| <Tab.Group | ||
| selectedIndex={tabIndex} | ||
| onChange={handleTabChange} | ||
| className={isDesktop ? 'flex' : 'block'} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Headless UI 2.x Tab.Group className prop support documentation
💡 Result:
Short answer: The official Headless UI v2 docs for Tabs (Tab.Group) do not list a built-in className prop in the Tab.Group prop table. Tab.Group documents props like as, defaultIndex, selectedIndex, onChange, vertical and manual — so className is not listed explicitly. If you need classes on the group wrapper, render Tab.Group as an element that accepts className (via the as prop) or put classes on Tab.List / Tab.Panels instead. [1][2]
Sources:
- Headless UI — Tabs (Tab.Group props table). [1]
- Headless UI — Tabs usage examples showing as + styling on Tab / Tab.List. [2]
Remove className prop from Tab.Group—it's not supported in Headless UI 2.x.
Tab.Group doesn't accept className directly. Either wrap it with a div for styling, or use the as prop to render as a styled element (e.g., as="div").
🤖 Prompt for AI Agents
In frontend/src/app/admin/page.tsx around lines 138-141, the Tab.Group is passed
a className prop which Headless UI v2 doesn't support; remove the className from
Tab.Group and instead either wrap the Tab.Group with a div that has
className={isDesktop ? 'flex' : 'block'} or set Tab.Group's as prop to render a
div and move the className there (e.g., <Tab.Group as="div" className=...>),
keeping selectedIndex and onChange unchanged.
| const handleUpdateModel = async () => { | ||
| if (!selectedModel) return; | ||
|
|
||
| const config = validateConfig(formData.config); | ||
| if (!config) { | ||
| toast({ | ||
| variant: 'destructive', | ||
| title: t('public_models.errors.config_invalid_json'), | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Missing name validation in update handler.
Unlike handleCreateModel, the update handler doesn't validate that the name field is non-empty before proceeding. While the backend may reject empty names, client-side validation provides better UX.
const handleUpdateModel = async () => {
if (!selectedModel) return;
+ if (!formData.name.trim()) {
+ toast({
+ variant: 'destructive',
+ title: t('public_models.errors.name_required'),
+ });
+ return;
+ }
+
const config = validateConfig(formData.config);📝 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.
| const handleUpdateModel = async () => { | |
| if (!selectedModel) return; | |
| const config = validateConfig(formData.config); | |
| if (!config) { | |
| toast({ | |
| variant: 'destructive', | |
| title: t('public_models.errors.config_invalid_json'), | |
| }); | |
| return; | |
| } | |
| const handleUpdateModel = async () => { | |
| if (!selectedModel) return; | |
| if (!formData.name.trim()) { | |
| toast({ | |
| variant: 'destructive', | |
| title: t('public_models.errors.name_required'), | |
| }); | |
| return; | |
| } | |
| const config = validateConfig(formData.config); | |
| if (!config) { | |
| toast({ | |
| variant: 'destructive', | |
| title: t('public_models.errors.config_invalid_json'), | |
| }); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In frontend/src/features/admin/components/PublicModelList.tsx around lines 156
to 166, the handleUpdateModel function lacks validation that the name field is
non-empty; add the same client-side name check used in handleCreateModel before
proceeding: get the name from formData.name (trimmed), if it's empty call toast
with a destructive variant and the same t('public_models.errors.name_required')
(or the equivalent key used in create), then return; only continue to validate
config and call the update logic when name is present.
| onChange={(e) => setIncludeInactive(e.target.checked)} | ||
| className="rounded border-border" | ||
| /> | ||
| Show inactive |
There was a problem hiding this comment.
Hardcoded text should use translation.
"Show inactive" is hardcoded. Use t('users.show_inactive') for consistency and i18n support.
- Show inactive
+ {t('users.show_inactive')}Add the key to the admin.json locale files:
"show_inactive": "Show inactive"🤖 Prompt for AI Agents
In frontend/src/features/admin/components/UserList.tsx around line 287, the
literal "Show inactive" is hardcoded; replace it with the translation call
t('users.show_inactive') where the text is rendered (ensure the component has
access to the t function from your i18n hook/context), and add the key
"show_inactive": "Show inactive" to the admin.json locale files for all
supported languages so the string is available for i18n.
| {getStatusTag(user.is_active)} | ||
| </div> | ||
| <div className="flex items-center gap-2 mt-1 text-xs text-text-muted"> | ||
| <span>{user.email || 'No email'}</span> |
There was a problem hiding this comment.
Hardcoded fallback text.
"No email" should use a translation key for i18n consistency.
- <span>{user.email || 'No email'}</span>
+ <span>{user.email || t('users.no_email')}</span>📝 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.
| <span>{user.email || 'No email'}</span> | |
| <span>{user.email || t('users.no_email')}</span> |
🤖 Prompt for AI Agents
In frontend/src/features/admin/components/UserList.tsx around line 329, the
hardcoded fallback string "No email" must be replaced with a translation key to
keep i18n consistent; update the span to use the existing i18n function or
component in the file (e.g., t('user.noEmail') or <Trans>), ensure the
appropriate key (user.noEmail) is added to your locale JSONs, and keep the same
falsy check (user.email || t('user.noEmail')) so behavior is unchanged.
| <Button variant="outline" onClick={() => setIsCreateDialogOpen(false)}> | ||
| Cancel | ||
| </Button> | ||
| <Button onClick={handleCreateUser} disabled={saving}> | ||
| {saving && <Loader2 className="mr-2 h-4 w-4 animate-spin" />} | ||
| Create | ||
| </Button> |
There was a problem hiding this comment.
Multiple hardcoded button labels.
Button labels "Cancel", "Create", "Save", "Reset", "Delete" throughout the dialogs (lines 477, 481, 531, 535, 563, 568, 584, 589) are hardcoded instead of using translations.
Consider adding these to the locale file:
"buttons": {
"cancel": "Cancel",
"create": "Create",
"save": "Save",
"reset": "Reset",
"delete": "Delete"
}And using them: {t('users.buttons.cancel')}, etc.
🤖 Prompt for AI Agents
In frontend/src/features/admin/components/UserList.tsx around lines 476 to 482
(and other occurrences at ~531, 535, 563, 568, 584, 589), multiple button labels
("Cancel", "Create", "Save", "Reset", "Delete") are hardcoded; replace each
hardcoded string with translation lookups (e.g., t('users.buttons.cancel'),
t('users.buttons.create'), t('users.buttons.save'), t('users.buttons.reset'),
t('users.buttons.delete')) and ensure the i18n hook (useTranslation or t) is
imported/available in this component; also add the corresponding keys under
"users.buttons" in the locale JSON files (e.g., "buttons": {"cancel": "Cancel",
"create": "Create", "save": "Save", "reset": "Reset", "delete": "Delete"}) so
translations resolve.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/i18n/locales/en/common.json(1 hunks)frontend/src/i18n/locales/zh-CN/common.json(1 hunks)frontend/src/types/api.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/i18n/locales/zh-CN/common.json
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
All code comments, inline comments, block comments, docstrings, TODO/FIXME annotations, and type hints descriptions MUST be written in English
Files:
frontend/src/types/api.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: TypeScript code MUST use strict mode with type checking enabled
TypeScript/React code MUST use Prettier formatter with single quotes, no semicolons
TypeScript/React code MUST pass ESLint with Next.js configuration
React component names MUST use PascalCase convention
Files:
frontend/src/types/api.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: React components MUST use functional components with hooks instead of class-based components
Useconstoverlet, never usevarin TypeScript/JavaScript code
Files:
frontend/src/types/api.ts
**/types/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript type definitions MUST be organized in
src/types/directory
Files:
frontend/src/types/api.ts
🧬 Code graph analysis (1)
frontend/src/types/api.ts (3)
frontend/src/apis/admin.ts (2)
UserRole(8-8)AuthSource(9-9)backend/app/models/user.py (1)
User(14-37)shared/models/task.py (1)
User(10-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (1)
frontend/src/i18n/locales/en/common.json (1)
40-47: Admin navigation label looks consistent
navigation.adminfollows existing naming and style ("Admin"), so this entry is fine as-is. Just ensure all supported locales define the same key.
| export type UserRole = 'admin' | 'user'; | ||
| export type AuthSource = 'password' | 'oidc' | 'unknown'; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "export type UserRole" frontend/src -g'*.ts' -g'*.tsx' -C2Repository: wecode-ai/Wegent
Length of output: 585
🏁 Script executed:
rg -n "export type AuthSource" frontend/src -g'*.ts' -g'*.tsx' -C2Repository: wecode-ai/Wegent
Length of output: 638
🏁 Script executed:
cat -n frontend/src/types/api.ts | head -25Repository: wecode-ai/Wegent
Length of output: 755
🏁 Script executed:
cat -n frontend/src/apis/admin.ts | head -30Repository: wecode-ai/Wegent
Length of output: 895
🏁 Script executed:
rg -n "import.*UserRole|import.*AuthSource" frontend/src -g'*.ts' -g'*.tsx'Repository: wecode-ai/Wegent
Length of output: 42
🏁 Script executed:
rg -n "from.*admin|from.*types/api" frontend/src -g'*.ts' -g'*.tsx' | grep -E "(UserRole|AuthSource|User|AdminUser)"Repository: wecode-ai/Wegent
Length of output: 562
🏁 Script executed:
rg -n "UserRole|AuthSource" frontend/src -g'*.ts' -g'*.tsx' --type-list | wc -lRepository: wecode-ai/Wegent
Length of output: 468
🏁 Script executed:
rg -n "type UserRole|type AuthSource" frontend/src -g'*.ts' -g'*.tsx'Repository: wecode-ai/Wegent
Length of output: 371
Centralize UserRole/AuthSource type definitions to frontend/src/types/api.ts
The UserRole and AuthSource types are defined identically in both frontend/src/types/api.ts (lines 8–9) and frontend/src/apis/admin.ts (lines 8–9), creating two sources of truth and risking divergence. This violates the coding guideline that TypeScript type definitions MUST be organized in src/types/.
Remove the duplicate type definitions from admin.ts and import them from types/api.ts instead. The AdminUser interface in admin.ts should use the imported UserRole and AuthSource types (lines 15–16).
Once centralized, the optional role? and auth_source? fields in the User interface (lines 16–17) can be made required in a future change, provided all API responses reliably include these fields.
🤖 Prompt for AI Agents
In frontend/src/types/api.ts around lines 8–10 you have the canonical UserRole
and AuthSource types; remove the duplicate definitions from
frontend/src/apis/admin.ts (the duplicates at admin.ts lines 8–9) and replace
them with imports from frontend/src/types/api.ts. Then update the AdminUser
interface in admin.ts (around lines 15–16) to reference the imported UserRole
and AuthSource types instead of redeclaring them; leave the optional role? and
auth_source? fields as-is for now.
- Add role="admin" to test_admin_user fixture for security check - Fix unused variable warnings in PublicModelList.tsx and UserList.tsx - Prefix unused variables with underscore (_total, _setPage, _error) - Remove unused ShieldCheckIcon import Co-authored-by: qdaxb <4157870+qdaxb@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
frontend/src/features/admin/components/UserList.tsx (3)
286-286: Hardcoded text should use translation.The label "Show inactive" is hardcoded. Use
t('users.show_inactive')for i18n consistency.- Show inactive + {t('users.show_inactive')}
328-328: Hardcoded fallback text should use translation.The "No email" fallback should use a translation key for i18n consistency.
- <span>{user.email || 'No email'}</span> + <span>{user.email || t('users.no_email')}</span>
475-481: Multiple hardcoded button labels throughout dialogs.Button labels "Cancel", "Create", "Save", "Reset", "Delete" are hardcoded in multiple dialogs (lines 476, 481, 530, 534, 562, 567, 583, 588) instead of using translation keys.
Consider adding these to your locale file and using translation lookups:
t('users.buttons.cancel')t('users.buttons.create')t('users.buttons.save')t('users.buttons.reset')t('users.buttons.delete')frontend/src/features/admin/components/PublicModelList.tsx (1)
156-166: Missing name validation in update handler.Unlike
handleCreateModel, the update handler doesn't validate that the name field is non-empty before proceeding. This creates inconsistent validation behavior between create and update operations.const handleUpdateModel = async () => { if (!selectedModel) return; + if (!formData.name.trim()) { + toast({ + variant: 'destructive', + title: t('public_models.errors.name_required'), + }); + return; + } + const config = validateConfig(formData.config);
🧹 Nitpick comments (3)
backend/tests/conftest.py (1)
96-103: Admin fixture correctly sets role; consider making roles explicit across fixturesThe explicit
role="admin"on thetest_admin_userfixture aligns with the new role-based auth and looks correct. To make tests more robust against future changes to theUser.roledefault, you might also explicitly setrole="user"on thetest_userandtest_inactive_userfixtures instead of relying on the model default. If you have a central enum/constant for roles, using it here would further reduce drift risk, but that’s optional for tests.frontend/src/features/admin/components/UserList.tsx (1)
245-254: Fragile auth_source handling in edit dialog.Line 251 assumes the backend will only return
'password','oidc', or'unknown'for auth_source. If the backend introduces a new auth source type in the future, this type assertion will fail silently and potentially cause runtime errors.Consider using a type guard or explicit validation:
const openEditDialog = (user: AdminUser) => { setSelectedUser(user); + const authSource = ['password', 'oidc'].includes(user.auth_source) + ? user.auth_source + : 'password'; setFormData({ user_name: user.user_name, email: user.email || '', role: user.role, - auth_source: user.auth_source === 'unknown' ? 'password' : user.auth_source as 'password' | 'oidc', + auth_source: authSource as 'password' | 'oidc', }); setIsEditDialogOpen(true); };frontend/src/features/admin/components/PublicModelList.tsx (1)
170-177: Always sending JSON in updates may be inefficient.The update handler always includes the
jsonfield inupdateData(line 177), even when the configuration hasn't changed. While the backend likely handles this gracefully, it's more efficient to only send changed fields.Consider checking if config changed before including it:
const updateData: AdminPublicModelUpdate = {}; if (formData.name !== selectedModel.name) { updateData.name = formData.name; } if (formData.namespace !== selectedModel.namespace) { updateData.namespace = formData.namespace; } -updateData.json = config; +const currentConfigString = JSON.stringify(selectedModel.json); +const newConfigString = JSON.stringify(config); +if (currentConfigString !== newConfigString) { + updateData.json = config; +}Note: This adds a bit of complexity, so only apply if update performance is a concern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/tests/conftest.py(1 hunks)frontend/src/features/admin/components/PublicModelList.tsx(1 hunks)frontend/src/features/admin/components/UserList.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
All code comments, inline comments, block comments, docstrings, TODO/FIXME annotations, and type hints descriptions MUST be written in English
Files:
frontend/src/features/admin/components/UserList.tsxbackend/tests/conftest.pyfrontend/src/features/admin/components/PublicModelList.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: TypeScript code MUST use strict mode with type checking enabled
TypeScript/React code MUST use Prettier formatter with single quotes, no semicolons
TypeScript/React code MUST pass ESLint with Next.js configuration
React component names MUST use PascalCase convention
Files:
frontend/src/features/admin/components/UserList.tsxfrontend/src/features/admin/components/PublicModelList.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: React components MUST use functional components with hooks instead of class-based components
Useconstoverlet, never usevarin TypeScript/JavaScript code
Files:
frontend/src/features/admin/components/UserList.tsxfrontend/src/features/admin/components/PublicModelList.tsx
**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
React component files MUST use kebab-case naming convention
Files:
frontend/src/features/admin/components/UserList.tsxfrontend/src/features/admin/components/PublicModelList.tsx
**/src/**/*.{tsx,jsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
Frontend Tailwind CSS MUST use provided CSS variables for color system (e.g., --color-bg-base, --color-text-primary, --color-primary)
Files:
frontend/src/features/admin/components/UserList.tsxfrontend/src/features/admin/components/PublicModelList.tsx
**/src/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.{tsx,jsx}: Frontend responsive design MUST follow mobile-first approach with Tailwind breakpoints
Frontend React forms MUST use react-hook-form with zod validation schema
Frontend components MUST use shadcn/ui component library fromfrontend/src/components/ui/
Files:
frontend/src/features/admin/components/UserList.tsxfrontend/src/features/admin/components/PublicModelList.tsx
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code MUST be PEP 8 compliant with Black formatter (line length: 88) and isort for import organization
Python code MUST include type hints for all functions and variables
Python functions SHOULD NOT exceed 50 lines (preferred maximum)
Python functions and classes MUST have descriptive names and public functions/classes MUST include docstrings
Python code MUST extract magic numbers to named constants
Files:
backend/tests/conftest.py
🧬 Code graph analysis (1)
frontend/src/features/admin/components/UserList.tsx (2)
frontend/src/apis/admin.ts (4)
AdminUser(11-20)AdminUserCreate(27-33)AdminUserUpdate(35-40)UserRole(8-8)frontend/src/types/api.ts (1)
UserRole(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
| {model.is_active ? ( | ||
| <Tag variant="success">Active</Tag> | ||
| ) : ( | ||
| <Tag variant="error">Inactive</Tag> | ||
| )} |
There was a problem hiding this comment.
Hardcoded status labels should use translations.
Status tag labels "Active" and "Inactive" are hardcoded instead of using translation keys.
{model.is_active ? (
- <Tag variant="success">Active</Tag>
+ <Tag variant="success">{t('public_models.status.active')}</Tag>
) : (
- <Tag variant="error">Inactive</Tag>
+ <Tag variant="error">{t('public_models.status.inactive')}</Tag>
)}📝 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.
| {model.is_active ? ( | |
| <Tag variant="success">Active</Tag> | |
| ) : ( | |
| <Tag variant="error">Inactive</Tag> | |
| )} | |
| {model.is_active ? ( | |
| <Tag variant="success">{t('public_models.status.active')}</Tag> | |
| ) : ( | |
| <Tag variant="error">{t('public_models.status.inactive')}</Tag> | |
| )} |
🤖 Prompt for AI Agents
In frontend/src/features/admin/components/PublicModelList.tsx around lines 293
to 297, the status Tag labels "Active" and "Inactive" are hardcoded; replace
them with i18n translation calls (e.g., use the existing translation hook or
i18n util in the project) so the labels become t('status.active') and
t('status.inactive') (or the project's equivalent keys), ensure the translation
hook is imported/initialized in this component, and add the corresponding keys
to the locale files if they don't already exist.
| <div className="flex items-center gap-2 mt-1 text-xs text-text-muted"> | ||
| <span>Model ID: {getModelId(model.json)}</span> | ||
| <span>•</span> | ||
| <span>Namespace: {model.namespace}</span> | ||
| </div> |
There was a problem hiding this comment.
Hardcoded metadata labels should use translations.
The labels "Model ID:" and "Namespace:" are hardcoded. Use translation keys for i18n consistency.
<div className="flex items-center gap-2 mt-1 text-xs text-text-muted">
- <span>Model ID: {getModelId(model.json)}</span>
+ <span>{t('public_models.model_id')}: {getModelId(model.json)}</span>
<span>•</span>
- <span>Namespace: {model.namespace}</span>
+ <span>{t('public_models.namespace')}: {model.namespace}</span>
</div>📝 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.
| <div className="flex items-center gap-2 mt-1 text-xs text-text-muted"> | |
| <span>Model ID: {getModelId(model.json)}</span> | |
| <span>•</span> | |
| <span>Namespace: {model.namespace}</span> | |
| </div> | |
| <div className="flex items-center gap-2 mt-1 text-xs text-text-muted"> | |
| <span>{t('public_models.model_id')}: {getModelId(model.json)}</span> | |
| <span>•</span> | |
| <span>{t('public_models.namespace')}: {model.namespace}</span> | |
| </div> |
🤖 Prompt for AI Agents
In frontend/src/features/admin/components/PublicModelList.tsx around lines
299-303, the labels "Model ID:" and "Namespace:" are hardcoded; replace them
with translation keys via the component's i18n function (e.g., use the existing
useTranslation/t hook in this file) such as t('admin.modelId') and
t('admin.namespace') (pick appropriate key names consistent with the project),
update the locale JSON files with those keys and translations, and ensure the
translated strings include any punctuation/spacing (or add separators in JSX) so
the rendered output matches the current UI.
| <Button variant="outline" onClick={() => setIsCreateDialogOpen(false)}> | ||
| Cancel | ||
| </Button> | ||
| <Button onClick={handleCreateModel} disabled={saving}> | ||
| {saving && <Loader2 className="mr-2 h-4 w-4 animate-spin" />} | ||
| Create | ||
| </Button> |
There was a problem hiding this comment.
Multiple hardcoded button labels throughout dialogs.
Button labels "Cancel", "Create", "Save", and "Delete" are hardcoded in multiple dialogs (lines 390, 394, 442, 446, 462, 467) instead of using translation keys.
Consider using translation lookups:
t('public_models.buttons.cancel')t('public_models.buttons.create')t('public_models.buttons.save')t('public_models.buttons.delete')
🤖 Prompt for AI Agents
In frontend/src/features/admin/components/PublicModelList.tsx around lines
389-395 (and related dialog button uses at ~442, 446, 462, 467), several button
labels are hardcoded ("Cancel", "Create", "Save", "Delete"); replace them with
i18n translation lookups using the t function (e.g.
t('public_models.buttons.cancel'), t('public_models.buttons.create'),
t('public_models.buttons.save'), t('public_models.buttons.delete')) by
importing/using the existing translation hook/context in the component and
substituting the hardcoded strings with the appropriate t(...) calls so all
dialog buttons use translation keys.
Summary
/adminroute with tabbed interface for Users and Public Models managementChanges
Backend
rolefield (admin/user) with Alembic migrationget_admin_userto check role instead of usernameFrontend
/adminroute with role-based access controlSecurity Features
Test Plan
/adminpageSummary by CodeRabbit
New Features
Chores
Security
✏️ Tip: You can customize this high-level summary in your review settings.