feat: add AI code review assessment module - #4629
Closed
wangyaodujing123 wants to merge 8 commits into
Closed
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a full AI Code Review Assessment feature (DB models, services, controllers, API, frontend UI, routes, i18n, sidebar) and replaces the prior multi-arch Docker workflow with a simplified single-arch GitHub Container Registry build-and-push workflow. ChangesAssessment System Implementation
Docker Workflow Simplification
Sequence DiagramsequenceDiagram
participant Client as User/Client
participant API as API Server
participant DB as Database
participant Notify as Notification Service
rect rgba(100,150,200,0.5)
Note over Client,API: Submission flow
Client->>API: GET /api/assessment/active
API->>DB: Query active assessments
DB-->>API: Assessments list
API-->>Client: Return assessments
Client->>API: POST /api/assessment/submit (FormData + screenshots)
API->>API: Validate files & save screenshots
API->>DB: Insert AssessmentSubmission
DB-->>API: Confirmation
API-->>Client: Submission success
end
rect rgba(150,100,200,0.5)
Note over Client,Notify: Admin review flow
Admin->>API: GET /api/assessment/admin/submissions/:assessment_id
API->>DB: Fetch submissions (+ user)
DB-->>API: Submissions list
Admin->>API: POST /api/assessment/admin/review (status, score, comment)
API->>DB: Update submission record
DB-->>API: Updated record
API->>Notify: NotifyAssessmentReview(userId, ...)
Notify->>DB: Load user & settings
Notify-->>Client: Deliver notification
API-->>Admin: Review confirmed
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/docker-image.yml (1)
1-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove duplicate publish workflow to avoid double pushes/races
This workflow duplicates
.github/workflows/docker-build.yml(same event, same purpose), so pushes tomainwill run two image publishes and race onlatest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/docker-image.yml around lines 1 - 24, This workflow duplicates an existing Docker publish workflow and causes double pushes; remove or disable this workflow to avoid racing pushes to the same tag (the top-level name "Build and Push Docker Image" and the job "build" that uses docker/build-push-action@v6 with tag ghcr.io/wangyaodujing123/new-api:latest). Fix by deleting this workflow file or changing its trigger (e.g., from push on branch "main" to workflow_dispatch or adding a conditional to only run in non-overlapping scenarios) so only one publish workflow performs the push.
🧹 Nitpick comments (5)
web/default/src/features/assessment/index.tsx (3)
41-595: 🏗️ Heavy liftSplit this page into smaller components/hooks.
This file is very large and mixes multiple responsibilities (user list, submit dialog, admin CRUD, admin review). Extracting submodules/hooks will reduce complexity and improve maintainability.
As per coding guidelines, "Consider splitting components or extracting logic to custom Hooks when a single file exceeds approximately 200 lines".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/assessment/index.tsx` around lines 41 - 595, This file is too large and mixes UI and data logic; split it into smaller components and hooks: extract UserView (with its tabs) and AdminView into their own components, move the heavy pieces ActiveAssessments, MySubmissions, MyStatsPanel, AdminManage, and AdminReview into separate files, and pull data-fetching and mutation logic out of those components into custom hooks (e.g., useActiveAssessments for the queries and submitAssessment logic, useMySubmissions, useMyStats, useAdminManage for create/update/delete, and useAdminReview for fetching submissions/stats and reviewSubmission). Ensure each extracted component keeps its local UI state (e.g., submitTarget, content, files, dialogOpen, editItem, reviewForm, reviewingId) but uses the new hooks for API calls and query invalidation (queryClient interactions should be inside hooks), update imports/exports accordingly, and keep prop interfaces minimal (pass only ids/titles or callbacks where needed) so the top-level AssessmentPage just composes these smaller components.
208-208: ⚡ Quick winReplace nested ternaries for badge variants with a helper mapping.
These 2-level ternaries violate the project rule and hurt readability. Extract a small helper (status → variant) and reuse it in all three places.
As per coding guidelines, "Prohibit nested ternary expressions 2 levels or deeper; use if-else, early return, or extract functions instead; single-level ternary is acceptable but must be concise".
Also applies to: 376-376, 532-532
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/assessment/index.tsx` at line 208, The nested ternary used in Badge variant prop (variant={sub.status === 1 ? 'default' : sub.status === 2 ? 'destructive' : 'secondary'}) should be replaced with a small helper to improve readability and follow the rule against deep ternaries; add a function like statusToVariant(status: number): 'default' | 'destructive' | 'secondary' (or a const mapping object) that returns the correct variant for a given status and then use variant={statusToVariant(sub.status)} wherever the nested ternary appears (all Badge variant usages referencing sub.status in this file).
89-113: 🏗️ Heavy liftStandardize write operations with
useMutationinstead of ad-hoc async handlers.Create/update/delete/review/submit flows are all manual async calls. Converting these to React Query mutations will centralize pending/error states and keep cache invalidation patterns consistent.
As per coding guidelines, "Use
useQueryfor data fetching anduseMutationfor mutations in React Query; configure uniquequeryKey(preferably array form with consistent hierarchy)".Also applies to: 330-352, 354-362, 478-489
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/assessment/index.tsx` around lines 89 - 113, The submit flow uses an ad-hoc async handler (handleSubmit) calling submitAssessment and manually managing submitting state and cache invalidation; convert this to a React Query mutation using useMutation (e.g., create a mutation hook around submitAssessment) and replace handleSubmit's manual logic with mutation.mutateAsync or mutation.mutate, use the mutation's isLoading/isError/isSuccess instead of setSubmitting/toast, and perform cache updates via mutation's onSuccess (call queryClient.invalidateQueries with the existing ['assessment-active'] and ['assessment-my'] keys or better yet a shared hierarchical key) and onError for toasts; update any other manual flows (create/update/delete/review) mentioned to follow the same pattern to centralize state and invalidation.web/default/src/i18n/locales/ja.json (1)
3147-3147: ⚡ Quick winConsolidate
Select LanguagevsSelect languageto one canonical key.Having both casing variants increases key drift risk across locales; prefer a single source key and update call sites accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/i18n/locales/ja.json` at line 3147, There are two locale keys differing only by casing ("Select Language" vs "Select language"); pick one canonical key (e.g., "Select language"), remove the duplicate from ja.json (and other locale files), and update all call sites to use the chosen key (search for both "Select Language" and "Select language" in the codebase and replace usages to the canonical key); ensure tests or components that import or reference the former key names (e.g., any i18n.t("Select Language") calls) are updated accordingly to avoid runtime missing-key errors.web/default/src/features/assessment/types.ts (1)
1-55: ⚡ Quick winConvert interfaces to type exports to align with project guidelines.
All type definitions in this file should use
export typeinstead ofexport interface, as specified in the project's coding guidelines (Use PascalCase for type names and export withexport type). All type names are already correctly in PascalCase.Suggested refactor
-export interface Assessment { +export type Assessment = { id: number title: string description: string start_time: number end_time: number status: number max_score: number created_by: number created_at: number updated_at: number -} +} -export interface AssessmentSubmission { +export type AssessmentSubmission = { id: number assessment_id: number user_id: number content: string screenshots: string[] status: number score: number | null comment: string reviewed_by: number submitted_at: number reviewed_at: number -} +} -export interface AssessmentWithSubmission extends Assessment { +export type AssessmentWithSubmission = Assessment & { submitted: boolean score: number | null submission_status: number -} +} -export interface SubmissionWithAssessment extends AssessmentSubmission { +export type SubmissionWithAssessment = AssessmentSubmission & { assessment_title: string -} +} -export interface SubmissionWithUser extends AssessmentSubmission { +export type SubmissionWithUser = AssessmentSubmission & { username: string email: string -} +} -export interface AssessmentStats { +export type AssessmentStats = { total: number pending: number passed: number failed: number average_score: number -} +} -export interface MyStats { +export type MyStats = { total_submissions: number passed: number average_score: number -} +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/assessment/types.ts` around lines 1 - 55, Replace all exported interfaces with exported type aliases: change `export interface Assessment` -> `export type Assessment = { ... }`, `AssessmentSubmission`, `SubmissionWithUser`, `AssessmentStats`, and `MyStats` similarly; for the extended interfaces `AssessmentWithSubmission` and `SubmissionWithAssessment` convert to intersection type aliases using `Assessment & { ... }` and `AssessmentSubmission & { assessment_title: string }` respectively (preserve all field names and types like `screenshots: string[]`, nullable `score: number | null`, timestamps, etc.). Ensure every `interface` keyword is removed and each declaration uses `export type` with the same PascalCase names so existing imports remain valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/docker-image.yml:
- Line 23: The image tag in the workflow currently hardcodes the GHCR namespace
("ghcr.io/wangyaodujing123/new-api:latest"); update the tags value to use the
repository-aware variable (e.g., `${{ github.repository }}`) or a shared
workflow env so the tag becomes dynamic (something like `${{ github.repository
}}:latest`), ensuring the change is applied where the "tags:" key contains the
hardcoded "ghcr.io/wangyaodujing123/new-api:latest".
In `@controller/assessment.go`:
- Around line 81-115: Create validation in the request handling for
CreateAssessment (and the corresponding update handler) to reject invalid
schedules, scores, and statuses before calling Insert/Save: ensure StartTime is
strictly before EndTime (reject when StartTime >= EndTime), ensure MaxScore is a
positive integer (>0), and ensure Status is one of the known
model.AssessmentStatus values (e.g., AssessmentStatusPending,
AssessmentStatusActive, AssessmentStatusClosed) — perform these checks after
binding (where status and maxScore are defaulted) and return a clear ApiErrorMsg
if any fail, centralizing this logic immediately prior to
assessment.Insert()/assessment.Save().
- Around line 363-376: In ReviewSubmission, validate req.Status and req.Score
before calling model.ReviewSubmission: load the assessment for the submission
(e.g. call model.GetAssessmentByID(submission.AssessmentID) or the appropriate
GetAssessment function) and enforce that req.Status is one of "passed" or
"failed" and that req.Score is between 0 and assessment.MaxScore (inclusive); if
validation fails return common.ApiErrorMsg(c, ...) and do not call
model.ReviewSubmission. Ensure you use the existing submission from
model.GetSubmissionByID(req.Id) to obtain the assessment ID and perform these
checks right before invoking model.ReviewSubmission.
- Around line 253-299: The screenshot save loop currently ignores fh.Open,
os.Create and io.Copy errors and proceeds, leading to inconsistent Screenshots
and orphaned files if submission.Insert fails; change the loop in the handler to
treat any fh.Open/os.Create/io.Copy error as fatal: on the first error return an
API error, close any open handles, and remove any files already written (use
getAssessmentScreenshotPath to locate them) before aborting; likewise, after
calling submission.Insert(), if it returns an error delete any files written in
screenshots and return the API error; ensure you append filenames only after a
successful io.Copy and always close file handles on error paths.
In `@model/assessment_submission.go`:
- Around line 50-70: Add a DB-level unique constraint on (assessment_id,
user_id) by updating the AssessmentSubmission struct to mark AssessmentId and
UserId with a composite unique index (e.g.,
gorm:"uniqueIndex:idx_assessment_user") and ensure migrations create that index;
then update the AssessmentSubmission.Insert method to catch
duplicate-key/unique-constraint errors from the DB (check DB error
codes/messages for Postgres/MySQL/SQLite) and return a clear
duplicate-submission error instead of failing silently—refer to the
AssessmentSubmission struct fields AssessmentId and UserId and the Insert method
for the changes.
- Around line 3-48: The ScreenshotsJSON methods use encoding/json directly;
update ScreenshotsJSON.Value, ScreenshotsJSON.Scan, and
ScreenshotsJSON.MarshalJSON to call the repository wrapper functions (e.g.,
common.Marshal and common.Unmarshal) instead of json.Marshal/json.Unmarshal,
preserving the same return values and error handling: replace the two
json.Marshal calls in Value and the default branch of Scan, the json.Unmarshal
calls in Scan's []byte and string branches and the default branch, and the
json.Marshal in MarshalJSON with the corresponding common.* calls so behavior
and nil handling remain identical.
In `@model/assessment.go`:
- Around line 43-45: DeleteAssessmentByID currently hard-deletes an Assessment
and leaves related assessment_submissions and their screenshot files orphaned;
update DeleteAssessmentByID to run inside a DB transaction that first queries
for any AssessmentSubmission rows linked to the Assessment (e.g., model/struct
AssessmentSubmission), and either aborts with an error if any submissions exist
or deletes those submission rows and invokes the existing screenshot-file
cleanup logic (or call a helper to remove files) before deleting the Assessment;
ensure you use the same *DB transaction (Begin/Commit/Rollback) and return the
transaction error so the entire cleanup is atomic.
- Around line 91-114: GetUserAssessmentStats currently sums scores over all
non-NULL scores but divides by total (which includes pending), so pending
submissions are treated as zero; change the average to use only non-pending
submissions by computing a completed count (e.g., completed int64) with
DB.Model(&AssessmentSubmission{}).Where("user_id = ? AND status != ?", userId,
SubmissionStatusPending).Count(&completed) and use the same WHERE condition when
selecting COALESCE(SUM(score), 0) into totalScore; then divide totalScore by
float64(completed) (and only compute avgScore when completed > 0), leaving the
top-level total (total variable) unchanged for total_submissions. Ensure you
reference SubmissionStatusPending, totalScore, total, avgScore, and
GetUserAssessmentStats when making the change.
In `@router/api-router.go`:
- Line 28: The GET route apiRouter.GET("/assessment/screenshot/:filename",
controller.GetAssessmentScreenshot) exposes user screenshots publicly; wrap this
route with the UserAuth() middleware and update the
controller.GetAssessmentScreenshot handler to verify that the authenticated user
owns the requested file or has admin privileges (or alternatively return a
signed, time-limited URL instead of the file). Specifically, apply UserAuth() to
the route registration and add an ownership check inside
controller.GetAssessmentScreenshot (compare the authenticated user ID from the
request context to the owner metadata used when files are saved in
controller/assessment.go), returning 403 if not owner/admin or issuing a signed
URL when chosen.
- Around line 385-393: The assessment submit/upload POST endpoints lack the
write-rate limiter used elsewhere, allowing a user to spam uploads; update the
assessmentUserRoute group to attach the same critical/user write-rate limiting
middleware to POST "/submit" and POST "/upload" (i.e., add the existing
rate-limit middleware used on other write-heavy routes such as the critical/user
limiter — e.g., middleware.RateLimitCriticalUser or the project's equivalent) so
that controller.SubmitAssessment and controller.UploadAssessmentScreenshot are
protected by per-user write limits while preserving middleware.UserAuth().
In `@web/default/src/features/assessment/api.ts`:
- Around line 2-9: The import list at the top is missing the
AssessmentSubmission type used as the return data for submitAssessment; update
the import block that currently declares types like Assessment,
AssessmentWithSubmission, SubmissionWithAssessment, etc., to also include
AssessmentSubmission so submitAssessment's Promise<{ success: boolean; data:
AssessmentSubmission }> compiles correctly—add AssessmentSubmission to the same
import statement to resolve the type-check error.
In `@web/default/src/features/assessment/index.tsx`:
- Around line 146-147: The dialog close handler only clears submitTarget but
leaves the submit form state (content and files) intact; update the onOpenChange
callback for the Dialog (the one using submitTarget and setSubmitTarget) to also
reset the submit form state when closing—e.g., call the existing submit form
reset function or explicitly set content to '' and files to [] (or the
appropriate initial values) alongside setSubmitTarget(null) so content/files do
not persist between submissions.
In `@web/default/src/i18n/locales/vi.json`:
- Line 2179: The translation for the key "My Assessment Statistics" in vi.json
lost the possessive "My" — update the value for the JSON key "My Assessment
Statistics" so it preserves the user-owned meaning (e.g., use a Vietnamese
phrasing that explicitly includes "của tôi" or equivalent) ensuring the i18n key
"My Assessment Statistics" maps to a user-specific translation rather than a
generic title.
---
Outside diff comments:
In @.github/workflows/docker-image.yml:
- Around line 1-24: This workflow duplicates an existing Docker publish workflow
and causes double pushes; remove or disable this workflow to avoid racing pushes
to the same tag (the top-level name "Build and Push Docker Image" and the job
"build" that uses docker/build-push-action@v6 with tag
ghcr.io/wangyaodujing123/new-api:latest). Fix by deleting this workflow file or
changing its trigger (e.g., from push on branch "main" to workflow_dispatch or
adding a conditional to only run in non-overlapping scenarios) so only one
publish workflow performs the push.
---
Nitpick comments:
In `@web/default/src/features/assessment/index.tsx`:
- Around line 41-595: This file is too large and mixes UI and data logic; split
it into smaller components and hooks: extract UserView (with its tabs) and
AdminView into their own components, move the heavy pieces ActiveAssessments,
MySubmissions, MyStatsPanel, AdminManage, and AdminReview into separate files,
and pull data-fetching and mutation logic out of those components into custom
hooks (e.g., useActiveAssessments for the queries and submitAssessment logic,
useMySubmissions, useMyStats, useAdminManage for create/update/delete, and
useAdminReview for fetching submissions/stats and reviewSubmission). Ensure each
extracted component keeps its local UI state (e.g., submitTarget, content,
files, dialogOpen, editItem, reviewForm, reviewingId) but uses the new hooks for
API calls and query invalidation (queryClient interactions should be inside
hooks), update imports/exports accordingly, and keep prop interfaces minimal
(pass only ids/titles or callbacks where needed) so the top-level AssessmentPage
just composes these smaller components.
- Line 208: The nested ternary used in Badge variant prop (variant={sub.status
=== 1 ? 'default' : sub.status === 2 ? 'destructive' : 'secondary'}) should be
replaced with a small helper to improve readability and follow the rule against
deep ternaries; add a function like statusToVariant(status: number): 'default' |
'destructive' | 'secondary' (or a const mapping object) that returns the correct
variant for a given status and then use variant={statusToVariant(sub.status)}
wherever the nested ternary appears (all Badge variant usages referencing
sub.status in this file).
- Around line 89-113: The submit flow uses an ad-hoc async handler
(handleSubmit) calling submitAssessment and manually managing submitting state
and cache invalidation; convert this to a React Query mutation using useMutation
(e.g., create a mutation hook around submitAssessment) and replace
handleSubmit's manual logic with mutation.mutateAsync or mutation.mutate, use
the mutation's isLoading/isError/isSuccess instead of setSubmitting/toast, and
perform cache updates via mutation's onSuccess (call
queryClient.invalidateQueries with the existing ['assessment-active'] and
['assessment-my'] keys or better yet a shared hierarchical key) and onError for
toasts; update any other manual flows (create/update/delete/review) mentioned to
follow the same pattern to centralize state and invalidation.
In `@web/default/src/features/assessment/types.ts`:
- Around line 1-55: Replace all exported interfaces with exported type aliases:
change `export interface Assessment` -> `export type Assessment = { ... }`,
`AssessmentSubmission`, `SubmissionWithUser`, `AssessmentStats`, and `MyStats`
similarly; for the extended interfaces `AssessmentWithSubmission` and
`SubmissionWithAssessment` convert to intersection type aliases using
`Assessment & { ... }` and `AssessmentSubmission & { assessment_title: string }`
respectively (preserve all field names and types like `screenshots: string[]`,
nullable `score: number | null`, timestamps, etc.). Ensure every `interface`
keyword is removed and each declaration uses `export type` with the same
PascalCase names so existing imports remain valid.
In `@web/default/src/i18n/locales/ja.json`:
- Line 3147: There are two locale keys differing only by casing ("Select
Language" vs "Select language"); pick one canonical key (e.g., "Select
language"), remove the duplicate from ja.json (and other locale files), and
update all call sites to use the chosen key (search for both "Select Language"
and "Select language" in the codebase and replace usages to the canonical key);
ensure tests or components that import or reference the former key names (e.g.,
any i18n.t("Select Language") calls) are updated accordingly to avoid runtime
missing-key errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c19ed50a-f309-4731-8368-712f3e41ad73
📒 Files selected for processing (19)
.github/workflows/docker-build.yml.github/workflows/docker-image.ymlcontroller/assessment.godto/assessment.gomodel/assessment.gomodel/assessment_submission.gomodel/main.gorouter/api-router.goservice/assessment.goweb/default/src/features/assessment/api.tsweb/default/src/features/assessment/index.tsxweb/default/src/features/assessment/types.tsweb/default/src/i18n/locales/en.jsonweb/default/src/i18n/locales/fr.jsonweb/default/src/i18n/locales/ja.jsonweb/default/src/i18n/locales/ru.jsonweb/default/src/i18n/locales/vi.jsonweb/default/src/i18n/locales/zh.jsonweb/default/src/routes/_authenticated/assessment/index.tsx
| with: | ||
| context: . | ||
| push: true | ||
| tags: ghcr.io/wangyaodujing123/new-api:latest |
Contributor
There was a problem hiding this comment.
Avoid hardcoded GHCR namespace in image tag
ghcr.io/wangyaodujing123/new-api:latest is repo/user-specific and can break in org repos/forks. Use ${{ github.repository }} (or shared env) like the other workflow.
Proposed fix
+env:
+ REGISTRY: ghcr.io
+ IMAGE_NAME: ${{ github.repository }}
jobs:
build:
@@
- uses: docker/build-push-action@v6
with:
context: .
push: true
- tags: ghcr.io/wangyaodujing123/new-api:latest
+ tags: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:latest🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/docker-image.yml at line 23, The image tag in the workflow
currently hardcodes the GHCR namespace
("ghcr.io/wangyaodujing123/new-api:latest"); update the tags value to use the
repository-aware variable (e.g., `${{ github.repository }}`) or a shared
workflow env so the tag becomes dynamic (something like `${{ github.repository
}}:latest`), ensuring the change is applied where the "tags:" key contains the
hardcoded "ghcr.io/wangyaodujing123/new-api:latest".
Comment on lines
+81
to
+115
| func CreateAssessment(c *gin.Context) { | ||
| var req dto.CreateAssessmentRequest | ||
| if err := c.ShouldBindJSON(&req); err != nil { | ||
| common.ApiErrorMsg(c, "参数错误") | ||
| return | ||
| } | ||
| if req.Title == "" { | ||
| common.ApiErrorMsg(c, "标题不能为空") | ||
| return | ||
| } | ||
|
|
||
| status := model.AssessmentStatusPending | ||
| if req.Status != nil { | ||
| status = *req.Status | ||
| } | ||
| maxScore := 100 | ||
| if req.MaxScore != nil { | ||
| maxScore = *req.MaxScore | ||
| } | ||
|
|
||
| assessment := model.Assessment{ | ||
| Title: req.Title, | ||
| Description: req.Description, | ||
| StartTime: req.StartTime, | ||
| EndTime: req.EndTime, | ||
| Status: status, | ||
| MaxScore: maxScore, | ||
| CreatedBy: c.GetInt("id"), | ||
| } | ||
| if err := assessment.Insert(); err != nil { | ||
| common.ApiErrorMsg(c, "创建失败："+err.Error()) | ||
| return | ||
| } | ||
| common.ApiSuccess(c, assessment) | ||
| } |
Contributor
There was a problem hiding this comment.
Validate schedule/status/score bounds on create and update.
These handlers currently accept start_time >= end_time, non-positive max_score, and arbitrary status values. That lets admins persist assessments users can never complete or states the rest of the code does not understand. Centralize the checks before insert/save.
Also applies to: 117-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/assessment.go` around lines 81 - 115, Create validation in the
request handling for CreateAssessment (and the corresponding update handler) to
reject invalid schedules, scores, and statuses before calling Insert/Save:
ensure StartTime is strictly before EndTime (reject when StartTime >= EndTime),
ensure MaxScore is a positive integer (>0), and ensure Status is one of the
known model.AssessmentStatus values (e.g., AssessmentStatusPending,
AssessmentStatusActive, AssessmentStatusClosed) — perform these checks after
binding (where status and maxScore are defaulted) and return a clear ApiErrorMsg
if any fail, centralizing this logic immediately prior to
assessment.Insert()/assessment.Save().
Comment on lines
+253
to
+299
| form := c.Request.MultipartForm | ||
| var screenshots []string | ||
| files := form.File["screenshots"] | ||
| for _, fh := range files { | ||
| ext := strings.ToLower(filepath.Ext(fh.Filename)) | ||
| if ext != ".png" && ext != ".jpg" && ext != ".jpeg" && ext != ".gif" && ext != ".webp" { | ||
| common.ApiErrorMsg(c, "仅支持 png/jpg/jpeg/gif/webp 格式") | ||
| return | ||
| } | ||
| if fh.Size > 10*1024*1024 { | ||
| common.ApiErrorMsg(c, "单张图片不能超过10MB") | ||
| return | ||
| } | ||
|
|
||
| file, err := fh.Open() | ||
| if err != nil { | ||
| continue | ||
| } | ||
|
|
||
| filename := uuid.New().String() + ext | ||
| outPath := getAssessmentScreenshotPath(filename) | ||
| outFile, err := os.Create(outPath) | ||
| if err != nil { | ||
| file.Close() | ||
| continue | ||
| } | ||
|
|
||
| io.Copy(outFile, file) | ||
| outFile.Close() | ||
| file.Close() | ||
|
|
||
| screenshots = append(screenshots, filename) | ||
| } | ||
|
|
||
| submission := model.AssessmentSubmission{ | ||
| AssessmentId: assessmentId, | ||
| UserId: userId, | ||
| Content: content, | ||
| Screenshots: model.ScreenshotsJSON(screenshots), | ||
| Status: model.SubmissionStatusPending, | ||
| } | ||
| if len(screenshots) == 0 { | ||
| submission.Screenshots = model.ScreenshotsJSON{} | ||
| } | ||
| if err := submission.Insert(); err != nil { | ||
| common.ApiErrorMsg(c, "提交失败："+err.Error()) | ||
| return |
Contributor
There was a problem hiding this comment.
Fail the submission if any screenshot write fails.
The loop silently continues on fh.Open/os.Create errors and ignores io.Copy errors entirely, so truncated or missing files can still be recorded in Screenshots. Also, if submission.Insert() fails afterward, the already-written files are orphaned. Return an error on the first failed save and delete any files written earlier in the request before aborting.
Possible direction
- io.Copy(outFile, file)
- outFile.Close()
- file.Close()
-
- screenshots = append(screenshots, filename)
+ if _, err := io.Copy(outFile, file); err != nil {
+ outFile.Close()
+ file.Close()
+ _ = os.Remove(outPath)
+ common.ApiErrorMsg(c, "图片保存失败")
+ return
+ }
+ outFile.Close()
+ file.Close()
+ screenshots = append(screenshots, filename)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/assessment.go` around lines 253 - 299, The screenshot save loop
currently ignores fh.Open, os.Create and io.Copy errors and proceeds, leading to
inconsistent Screenshots and orphaned files if submission.Insert fails; change
the loop in the handler to treat any fh.Open/os.Create/io.Copy error as fatal:
on the first error return an API error, close any open handles, and remove any
files already written (use getAssessmentScreenshotPath to locate them) before
aborting; likewise, after calling submission.Insert(), if it returns an error
delete any files written in screenshots and return the API error; ensure you
append filenames only after a successful io.Copy and always close file handles
on error paths.
Comment on lines
+363
to
+376
| func ReviewSubmission(c *gin.Context) { | ||
| var req dto.ReviewSubmissionRequest | ||
| if err := c.ShouldBindJSON(&req); err != nil { | ||
| common.ApiErrorMsg(c, "参数错误") | ||
| return | ||
| } | ||
|
|
||
| submission, err := model.GetSubmissionByID(req.Id) | ||
| if err != nil { | ||
| common.ApiErrorMsg(c, "提交记录不存在") | ||
| return | ||
| } | ||
|
|
||
| if err := model.ReviewSubmission(req.Id, req.Status, req.Score, req.Comment, c.GetInt("id")); err != nil { |
Contributor
There was a problem hiding this comment.
Reject invalid review states before updating the submission.
req.Status and req.Score are written verbatim, so this endpoint can persist pending as a "reviewed" state or scores outside the assessment's max score. Load the assessment first and enforce {passed, failed} plus 0 <= score <= MaxScore before calling model.ReviewSubmission.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/assessment.go` around lines 363 - 376, In ReviewSubmission,
validate req.Status and req.Score before calling model.ReviewSubmission: load
the assessment for the submission (e.g. call
model.GetAssessmentByID(submission.AssessmentID) or the appropriate
GetAssessment function) and enforce that req.Status is one of "passed" or
"failed" and that req.Score is between 0 and assessment.MaxScore (inclusive); if
validation fails return common.ApiErrorMsg(c, ...) and do not call
model.ReviewSubmission. Ensure you use the existing submission from
model.GetSubmissionByID(req.Id) to obtain the assessment ID and perform these
checks right before invoking model.ReviewSubmission.
Comment on lines
+3
to
+48
| import ( | ||
| "database/sql/driver" | ||
| "encoding/json" | ||
| "math" | ||
| "time" | ||
| ) | ||
|
|
||
| const ( | ||
| SubmissionStatusPending = 0 | ||
| SubmissionStatusPassed = 1 | ||
| SubmissionStatusFailed = 2 | ||
| ) | ||
|
|
||
| type ScreenshotsJSON []string | ||
|
|
||
| func (s ScreenshotsJSON) Value() (driver.Value, error) { | ||
| if s == nil { | ||
| return nil, nil | ||
| } | ||
| return json.Marshal(s) | ||
| } | ||
|
|
||
| func (s *ScreenshotsJSON) Scan(value interface{}) error { | ||
| switch v := value.(type) { | ||
| case nil: | ||
| *s = nil | ||
| return nil | ||
| case []byte: | ||
| return json.Unmarshal(v, s) | ||
| case string: | ||
| return json.Unmarshal([]byte(v), s) | ||
| default: | ||
| b, err := json.Marshal(v) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return json.Unmarshal(b, s) | ||
| } | ||
| } | ||
|
|
||
| func (s ScreenshotsJSON) MarshalJSON() ([]byte, error) { | ||
| if s == nil { | ||
| return []byte("null"), nil | ||
| } | ||
| return json.Marshal([]string(s)) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
sed -n '1,220p' common/json.go
rg -n --type go 'encoding/json|json\.(Marshal|Unmarshal|NewDecoder|NewEncoder)' model/assessment_submission.goRepository: QuantumNous/new-api
Length of output: 1483
Replace direct encoding/json calls with common wrapper functions throughout ScreenshotsJSON.
The Value(), Scan(), and MarshalJSON() methods use json.Marshal and json.Unmarshal directly instead of the repository wrapper functions. Per coding guidelines, all JSON operations in Go business code must use common.Marshal(), common.Unmarshal(), etc. Replace the 6 direct calls across these methods with their wrapper equivalents.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@model/assessment_submission.go` around lines 3 - 48, The ScreenshotsJSON
methods use encoding/json directly; update ScreenshotsJSON.Value,
ScreenshotsJSON.Scan, and ScreenshotsJSON.MarshalJSON to call the repository
wrapper functions (e.g., common.Marshal and common.Unmarshal) instead of
json.Marshal/json.Unmarshal, preserving the same return values and error
handling: replace the two json.Marshal calls in Value and the default branch of
Scan, the json.Unmarshal calls in Scan's []byte and string branches and the
default branch, and the json.Marshal in MarshalJSON with the corresponding
common.* calls so behavior and nil handling remain identical.
| apiRouter.GET("/models", middleware.UserAuth(), controller.DashboardListModels) | ||
| apiRouter.GET("/status/test", middleware.AdminAuth(), controller.TestStatus) | ||
| apiRouter.GET("/notice", controller.GetNotice) | ||
| apiRouter.GET("/assessment/screenshot/:filename", controller.GetAssessmentScreenshot) |
Contributor
There was a problem hiding this comment.
Protect assessment screenshots behind authorization.
This route is public, but controller/assessment.go stores user-submitted screenshots in the same directory. Anyone who obtains a filename can fetch another user's uploaded artifact. Put this behind UserAuth() and enforce ownership/admin checks, or serve signed URLs instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/api-router.go` at line 28, The GET route
apiRouter.GET("/assessment/screenshot/:filename",
controller.GetAssessmentScreenshot) exposes user screenshots publicly; wrap this
route with the UserAuth() middleware and update the
controller.GetAssessmentScreenshot handler to verify that the authenticated user
owns the requested file or has admin privileges (or alternatively return a
signed, time-limited URL instead of the file). Specifically, apply UserAuth() to
the route registration and add an ownership check inside
controller.GetAssessmentScreenshot (compare the authenticated user ID from the
request context to the owner metadata used when files are saved in
controller/assessment.go), returning 403 if not owner/admin or issuing a signed
URL when chosen.
Comment on lines
+385
to
+393
| assessmentUserRoute := apiRouter.Group("/assessment") | ||
| assessmentUserRoute.Use(middleware.UserAuth()) | ||
| { | ||
| assessmentUserRoute.GET("/active", controller.GetActiveAssessmentsForUser) | ||
| assessmentUserRoute.GET("/my", controller.GetMySubmissions) | ||
| assessmentUserRoute.GET("/my/stats", controller.GetMyAssessmentStats) | ||
| assessmentUserRoute.POST("/submit", controller.SubmitAssessment) | ||
| assessmentUserRoute.POST("/upload", controller.UploadAssessmentScreenshot) | ||
| } |
Contributor
There was a problem hiding this comment.
Add write-rate limiting to the assessment submit/upload endpoints.
POST /assessment/submit parses multipart forms and writes DB rows plus files, and POST /assessment/upload writes files directly, but neither route has the critical/user rate limit used on other write-heavy endpoints. A single account can currently spam storage and the review queue.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/api-router.go` around lines 385 - 393, The assessment submit/upload
POST endpoints lack the write-rate limiter used elsewhere, allowing a user to
spam uploads; update the assessmentUserRoute group to attach the same
critical/user write-rate limiting middleware to POST "/submit" and POST
"/upload" (i.e., add the existing rate-limit middleware used on other
write-heavy routes such as the critical/user limiter — e.g.,
middleware.RateLimitCriticalUser or the project's equivalent) so that
controller.SubmitAssessment and controller.UploadAssessmentScreenshot are
protected by per-user write limits while preserving middleware.UserAuth().
Comment on lines
+2
to
+9
| import type { | ||
| Assessment, | ||
| AssessmentWithSubmission, | ||
| SubmissionWithAssessment, | ||
| SubmissionWithUser, | ||
| AssessmentStats, | ||
| MyStats, | ||
| } from './types' |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== api.ts imports =="
sed -n '1,60p' web/default/src/features/assessment/api.ts
echo
echo "== AssessmentSubmission definition in types.ts =="
rg -n "export (type|interface) AssessmentSubmission\\b" web/default/src/features/assessment/types.tsRepository: QuantumNous/new-api
Length of output: 2340
Add missing AssessmentSubmission type import to resolve type checking failure.
The submitAssessment function on line 26 returns Promise<{ success: boolean; data: AssessmentSubmission }>, but AssessmentSubmission is not imported. TypeScript compilation will fail.
Suggested fix
import type {
Assessment,
+ AssessmentSubmission,
AssessmentWithSubmission,
SubmissionWithAssessment,
SubmissionWithUser,
AssessmentStats,
MyStats,
} from './types'📝 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.
Suggested change
| import type { | |
| Assessment, | |
| AssessmentWithSubmission, | |
| SubmissionWithAssessment, | |
| SubmissionWithUser, | |
| AssessmentStats, | |
| MyStats, | |
| } from './types' | |
| import type { | |
| Assessment, | |
| AssessmentSubmission, | |
| AssessmentWithSubmission, | |
| SubmissionWithAssessment, | |
| SubmissionWithUser, | |
| AssessmentStats, | |
| MyStats, | |
| } from './types' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/default/src/features/assessment/api.ts` around lines 2 - 9, The import
list at the top is missing the AssessmentSubmission type used as the return data
for submitAssessment; update the import block that currently declares types like
Assessment, AssessmentWithSubmission, SubmissionWithAssessment, etc., to also
include AssessmentSubmission so submitAssessment's Promise<{ success: boolean;
data: AssessmentSubmission }> compiles correctly—add AssessmentSubmission to the
same import statement to resolve the type-check error.
Comment on lines
+146
to
+147
| <Dialog open={!!submitTarget} onOpenChange={(v) => { if (!v) setSubmitTarget(null) }}> | ||
| <DialogContent className="max-h-[90vh] overflow-auto"> |
Contributor
There was a problem hiding this comment.
Reset submit form state when closing the dialog.
Closing the submit dialog currently only clears submitTarget; content/files persist and can leak into the next submission.
💡 Suggested fix
-<Dialog open={!!submitTarget} onOpenChange={(v) => { if (!v) setSubmitTarget(null) }}>
+<Dialog
+ open={!!submitTarget}
+ onOpenChange={(v) => {
+ if (!v) {
+ setSubmitTarget(null)
+ setContent('')
+ setFiles([])
+ }
+ }}
+>📝 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.
Suggested change
| <Dialog open={!!submitTarget} onOpenChange={(v) => { if (!v) setSubmitTarget(null) }}> | |
| <DialogContent className="max-h-[90vh] overflow-auto"> | |
| <Dialog | |
| open={!!submitTarget} | |
| onOpenChange={(v) => { | |
| if (!v) { | |
| setSubmitTarget(null) | |
| setContent('') | |
| setFiles([]) | |
| } | |
| }} | |
| > | |
| <DialogContent className="max-h-[90vh] overflow-auto"> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/default/src/features/assessment/index.tsx` around lines 146 - 147, The
dialog close handler only clears submitTarget but leaves the submit form state
(content and files) intact; update the onOpenChange callback for the Dialog (the
one using submitTarget and setSubmitTarget) to also reset the submit form state
when closing—e.g., call the existing submit form reset function or explicitly
set content to '' and files to [] (or the appropriate initial values) alongside
setSubmitTarget(null) so content/files do not persist between submissions.
| "Multipliers for recharge pricing based on user groups.": "Hệ số nhân cho việc định giá nạp tiền dựa trên nhóm người dùng.", | ||
| "Must be a valid URL": "Phải là URL hợp lệ", | ||
| "Must be at least 8 characters": "Phải có ít nhất 8 ký tự", | ||
| "My Assessment Statistics": "Thống kê đánh giá", |
Contributor
There was a problem hiding this comment.
Keep possessive meaning in My Assessment Statistics.
Current translation drops “My”, which can read as a generic page title instead of user-specific stats.
Suggested fix
- "My Assessment Statistics": "Thống kê đánh giá",
+ "My Assessment Statistics": "Thống kê đánh giá của tôi",📝 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.
Suggested change
| "My Assessment Statistics": "Thống kê đánh giá", | |
| "My Assessment Statistics": "Thống kê đánh giá của tôi", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/default/src/i18n/locales/vi.json` at line 2179, The translation for the
key "My Assessment Statistics" in vi.json lost the possessive "My" — update the
value for the JSON key "My Assessment Statistics" so it preserves the user-owned
meaning (e.g., use a Vietnamese phrasing that explicitly includes "của tôi" or
equivalent) ensuring the i18n key "My Assessment Statistics" maps to a
user-specific translation rather than a generic title.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/default/src/hooks/use-sidebar-config.ts`:
- Line 90: The route map currently only contains the key '/assessment' so
isModuleEnabled (which performs exact lookups) will miss '/assessment/' and
allow the module to be shown; fix by either adding the trailing-slash variant to
the map (add '/assessment/': { section: 'console', module: 'assessment' }) or,
preferably, normalize the route before lookup in use-sidebar-config.ts /
isModuleEnabled (trim a trailing slash from the path or use path.replace(/\/$/,
'') so both '/assessment' and '/assessment/' resolve to the same map key).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0395e933-f735-4930-afc1-4f1f48ec0e13
📒 Files selected for processing (3)
web/default/src/features/system-settings/maintenance/config.tsweb/default/src/features/system-settings/maintenance/sidebar-modules-section.tsxweb/default/src/hooks/use-sidebar-config.ts
| '/usage-logs/common': { section: 'console', module: 'log' }, | ||
| '/usage-logs/drawing': { section: 'console', module: 'midjourney' }, | ||
| '/usage-logs/task': { section: 'console', module: 'task' }, | ||
| '/assessment': { section: 'console', module: 'assessment' }, |
Contributor
There was a problem hiding this comment.
Handle trailing-slash route variant for assessment gating.
Line 90 maps only '/assessment', but isModuleEnabled does exact key lookup. If the URL is '/assessment/', module gating is skipped and defaults to visible.
Suggested fix
const URL_TO_CONFIG_MAP: Record<string, { section: string; module: string }> = {
@@
'/assessment': { section: 'console', module: 'assessment' },
+ '/assessment/': { section: 'console', module: 'assessment' },
@@
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/default/src/hooks/use-sidebar-config.ts` at line 90, The route map
currently only contains the key '/assessment' so isModuleEnabled (which performs
exact lookups) will miss '/assessment/' and allow the module to be shown; fix by
either adding the trailing-slash variant to the map (add '/assessment/': {
section: 'console', module: 'assessment' }) or, preferably, normalize the route
before lookup in use-sidebar-config.ts / isModuleEnabled (trim a trailing slash
from the path or use path.replace(/\/$/, '') so both '/assessment' and
'/assessment/' resolve to the same map key).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/classic/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsx (1)
102-136:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
resetSidebarModulesis missing theassessmentflag.The reset handler defines a hardcoded
defaultModulesconfig but does not includeassessment: trueunderconsole. Clicking "Reset to default" will produce a config that disables (or omits) the new assessment module whileDEFAULT_ADMIN_CONFIGinuseSidebar.jskeeps it enabled, causing inconsistent admin state and a hidden module after reset.The same gap exists in the catch-fallback inside the
useEffectat lines 181-202.🛠️ Proposed fix
console: { enabled: true, detail: true, token: true, log: true, midjourney: true, task: true, + assessment: true, },Apply the same addition in the
useEffectcatch-block default at lines 183-190. Better still, import and reuseDEFAULT_ADMIN_CONFIGfrom../../../hooks/common/useSidebarinstead of duplicating the structure in three places.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/classic/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsx` around lines 102 - 136, resetSidebarModules builds a hardcoded defaultModules missing console.assessment which causes the assessment module to be disabled after reset and the same omission exists in the useEffect catch-fallback; fix by importing and reusing DEFAULT_ADMIN_CONFIG from the useSidebar hook (DEFAULT_ADMIN_CONFIG) instead of duplicating the object, or if you must keep a local default, add assessment: true under the console object, and update the useEffect catch-block to use the same DEFAULT_ADMIN_CONFIG (or the corrected local default) and call setSidebarModulesAdmin with it so both resetSidebarModules and the catch-path produce the same config.
🧹 Nitpick comments (5)
web/classic/src/pages/Assessment/index.jsx (2)
1-1: 💤 Low valueUnused import:
React.With React 17+ JSX runtime (the project uses React 18.2.0), the default
Reactimport is unnecessary unlessReact.*APIs are referenced directly (none are here). LikewiseuseEffectis not used at the top level (only insideuseData, which already imports it). Minor cleanup; not blocking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/classic/src/pages/Assessment/index.jsx` at line 1, Remove the unused top-level imports: drop the default React import and the unused useEffect from the import statement that currently reads "import React, { useState, useEffect, useCallback } from 'react'"; keep only the hooks actually used in this module (e.g., useState and useCallback) since the project uses the new JSX runtime and useEffect is already imported/used inside useData, which eliminates the unnecessary React import and the unused useEffect reference.
254-262: ⚡ Quick winForm is submitted without validating times or
max_score.
handleSavewill passdayjs('').unix()→NaNwhen the user hasn't filled date fields, and acceptsstart_time >= end_timeor non-positivemax_score. The backend may or may not enforce these (worth verifying), but the UI should validate before sending: required title, valid datetime ordering,max_score > 0. Consider using Semi UI'sFormwith rules instead of rawInputs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/classic/src/pages/Assessment/index.jsx` around lines 254 - 262, handleSave currently builds payload from form without validating date/time or max_score; add pre-submit validation in handleSave to check that form.title is non-empty, dayjs(form.start_time) and dayjs(form.end_time) are valid, start_time < end_time (use dayjs().isBefore), and form.max_score is a positive number (>0); if any check fails, stop the save (setSaving(false)), surface an inline/form error or notification and do not call the API. Alternatively refactor the page to use the Semi UI Form with validation rules for title, start_time/end_time ordering, and max_score so the UI prevents invalid submissions before handleSave runs. Ensure references: handleSave, form.start_time, form.end_time, form.max_score, setSaving.web/classic/src/components/layout/SiderBar.jsx (1)
109-113: ⚡ Quick winInconsistent i18n key convention.
All other navigation labels in this file use Chinese strings as the translation key (e.g.,
t('数据看板'),t('令牌管理'),t('任务日志')). The new entry uses an English stringt('AI Code Review Assessment'), which breaks the convention and means the Chinese locale will fall back to the raw English key unless explicitly mapped.Consider aligning with the existing convention by using a Chinese source key (e.g.,
t('AI 代码评审')or similar) and adding the English translation in the locale files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/classic/src/components/layout/SiderBar.jsx` around lines 109 - 113, The nav entry using t('AI Code Review Assessment') (itemKey 'assessment' in SiderBar.jsx) breaks the project's i18n convention of Chinese source keys; change the source key to a Chinese string like t('AI 代码评审') in the menu object and update the corresponding locale files (add the English translation for that Chinese key in the en locale and the Chinese text in the zh locale) so translations remain consistent and the Chinese locale won’t fall back to the raw English key.web/classic/src/pages/Setting/Personal/SettingsSidebarModulesUser.jsx (1)
321-321: ⚡ Quick winInconsistent i18n key convention.
Same issue as in
SettingsSidebarModulesAdmin.jsxline 237: this entry uses English source keys (t('AI Code Review Assessment'),t('Monthly AI development...')) while every other module entry in this file uses Chinese keys. This breaks the project's i18n convention; the Chinese locale will fall back to displaying the raw English key.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/classic/src/pages/Setting/Personal/SettingsSidebarModulesUser.jsx` at line 321, The title/description for the object with key 'assessment' in SettingsSidebarModulesUser.jsx uses raw English strings in t(...), breaking the file's i18n convention; replace t('AI Code Review Assessment') and t('Monthly AI development code assessment with submission and review.') with the same Chinese i18n keys/pattern used by other module entries in this file (follow the existing key naming convention used elsewhere in SettingsSidebarModulesUser.jsx and mirror the equivalent keys used for the admin variant), so the 'assessment' entry uses the consistent Chinese locale keys for title and description.web/classic/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsx (1)
237-237: ⚡ Quick winInconsistent i18n key convention.
All sibling entries use Chinese strings as translation keys (e.g.,
title: t('数据看板'),description: t('系统数据统计')). This new entry uses English as the key for bothtitleanddescription, which breaks the project's Chinese-first i18n convention and may cause the Chinese locale to render the raw English key.Consider aligning keys with the existing convention and providing English/other locale translations through the i18n locale files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/classic/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsx` at line 237, The new entry object with key 'assessment' in SettingsSidebarModulesAdmin.jsx uses English strings for title and description (title: t('AI Code Review Assessment'), description: t('Monthly AI development code assessment with submission and review.')), which breaks the Chinese-first i18n convention used by sibling entries; change those t(...) keys to Chinese keys consistent with other entries (e.g., replace the English literal keys with appropriate Chinese strings), and then add corresponding English translations into the i18n locale files so locales render correctly; ensure you edit the object for key 'assessment' and update the locale JSON/YAML entries for both zh and en.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/classic/src/pages/Assessment/index.jsx`:
- Around line 279-287: The delete handler handleDelete currently issues a
destructive API.delete immediately; wrap it with a confirmation prompt so users
must confirm before the DELETE is executed. Replace direct calls to handleDelete
from delete buttons with a Modal.confirm or AntD Popconfirm that displays a
clear warning (e.g., include assessment title/id) and only calls
handleDelete(id) if the user confirms; update any other delete usages (the other
delete call sites referenced) to use the same confirm flow to prevent accidental
deletions.
- Around line 254-287: handleSave and handleDelete assume API throws on failure
but API resolves with {data: {success, message}}; update both functions
(handleSave and handleDelete) to inspect the response (res.data.success) instead
of relying on catch: call API.post/put/delete and store the result, if
res.data.success true then call showSuccess(res.data.message || t('...
success')), close dialog/reload as appropriate, else call
showError(res.data.message || t('Operation failed')); preserve existing
try/catch for network errors and ensure setSaving(false) still runs in finally
for handleSave.
- Around line 75-96: handleSubmit currently only treats res.data.success ===
true as a success and swallows server-side failures; modify the non-success
branch after the API.post call to call showError with the server-provided
message (use res.data.message || t('Submission failed') as a fallback) so users
see backend validation/errors, and avoid changing the existing success behavior
(setSubmitTarget, setContent, setFiles, reload) — locate handleSubmit and the
API.post('/api/assessment/submit', fd) response handling to implement this.
- Around line 229-287: The admin API routes used in useData, handleSave,
handleDelete and any submission/stats/review calls are using the wrong paths
(missing the /admin prefix and some parameter ordering); update the calls in
useData('all', ...) (API.get), handleSave (API.post and API.put), handleDelete
(API.delete) and the submission/stats/review GET/POST calls to use the backend
admin routes—i.e. change GET /api/assessment/all -> GET /api/assessment/admin/,
POST /api/assessment -> POST /api/assessment/admin/, PUT /api/assessment -> PUT
/api/assessment/admin/, DELETE /api/assessment/${id} -> DELETE
/api/assessment/admin/${id}, GET /api/assessment/${id}/submissions -> GET
/api/assessment/admin/submissions/${id}, GET /api/assessment/${id}/stats -> GET
/api/assessment/admin/stats/${id}, and POST /api/assessment/review -> POST
/api/assessment/admin/review so functions use
API.get/API.post/API.put/API.delete with those updated paths (refer to useData,
handleSave, handleDelete and the review/submissions/stats call sites).
- Around line 23-39: useData currently double-unwraps the API envelope and
swallows errors and can capture a stale fetcher; fix it by (1) preserving the
full response envelope in state (call setData(res) instead of setData(res?.data
|| res)) so consumers that access data?.data continue to work, (2) replace the
empty catch with a logged error (e.g., console.error or processLogger.error
within the catch of load) and optionally set an error state, (3) include fetcher
in the dependency list used by useCallback (e.g., build depsForCallback =
[fetcher, ...deps]) to avoid stale closures, and (4) either remove the unused
key param or reference key in the useEffect dependency array so reloads respond
to key changes; update references to load/useEffect/useCallback accordingly.
- Around line 137-143: The file picker is using Semi UI's Input which passes
(value, event) not a native DOM event, so the handler (e) => e.target.files
fails and files never get set; replace the Semi <Input type="file"> with a
native <input type="file" multiple> (or use Semi's Upload component) and update
the onChange to read event.target.files and call
setFiles(Array.from(event.target.files)); also tighten the accept attribute to
".png,.jpg,.jpeg,.gif,.webp" to match backend validation (ensure you reference
the files state and setFiles setter used in this component).
---
Outside diff comments:
In `@web/classic/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsx`:
- Around line 102-136: resetSidebarModules builds a hardcoded defaultModules
missing console.assessment which causes the assessment module to be disabled
after reset and the same omission exists in the useEffect catch-fallback; fix by
importing and reusing DEFAULT_ADMIN_CONFIG from the useSidebar hook
(DEFAULT_ADMIN_CONFIG) instead of duplicating the object, or if you must keep a
local default, add assessment: true under the console object, and update the
useEffect catch-block to use the same DEFAULT_ADMIN_CONFIG (or the corrected
local default) and call setSidebarModulesAdmin with it so both
resetSidebarModules and the catch-path produce the same config.
---
Nitpick comments:
In `@web/classic/src/components/layout/SiderBar.jsx`:
- Around line 109-113: The nav entry using t('AI Code Review Assessment')
(itemKey 'assessment' in SiderBar.jsx) breaks the project's i18n convention of
Chinese source keys; change the source key to a Chinese string like t('AI 代码评审')
in the menu object and update the corresponding locale files (add the English
translation for that Chinese key in the en locale and the Chinese text in the zh
locale) so translations remain consistent and the Chinese locale won’t fall back
to the raw English key.
In `@web/classic/src/pages/Assessment/index.jsx`:
- Line 1: Remove the unused top-level imports: drop the default React import and
the unused useEffect from the import statement that currently reads "import
React, { useState, useEffect, useCallback } from 'react'"; keep only the hooks
actually used in this module (e.g., useState and useCallback) since the project
uses the new JSX runtime and useEffect is already imported/used inside useData,
which eliminates the unnecessary React import and the unused useEffect
reference.
- Around line 254-262: handleSave currently builds payload from form without
validating date/time or max_score; add pre-submit validation in handleSave to
check that form.title is non-empty, dayjs(form.start_time) and
dayjs(form.end_time) are valid, start_time < end_time (use dayjs().isBefore),
and form.max_score is a positive number (>0); if any check fails, stop the save
(setSaving(false)), surface an inline/form error or notification and do not call
the API. Alternatively refactor the page to use the Semi UI Form with validation
rules for title, start_time/end_time ordering, and max_score so the UI prevents
invalid submissions before handleSave runs. Ensure references: handleSave,
form.start_time, form.end_time, form.max_score, setSaving.
In `@web/classic/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsx`:
- Line 237: The new entry object with key 'assessment' in
SettingsSidebarModulesAdmin.jsx uses English strings for title and description
(title: t('AI Code Review Assessment'), description: t('Monthly AI development
code assessment with submission and review.')), which breaks the Chinese-first
i18n convention used by sibling entries; change those t(...) keys to Chinese
keys consistent with other entries (e.g., replace the English literal keys with
appropriate Chinese strings), and then add corresponding English translations
into the i18n locale files so locales render correctly; ensure you edit the
object for key 'assessment' and update the locale JSON/YAML entries for both zh
and en.
In `@web/classic/src/pages/Setting/Personal/SettingsSidebarModulesUser.jsx`:
- Line 321: The title/description for the object with key 'assessment' in
SettingsSidebarModulesUser.jsx uses raw English strings in t(...), breaking the
file's i18n convention; replace t('AI Code Review Assessment') and t('Monthly AI
development code assessment with submission and review.') with the same Chinese
i18n keys/pattern used by other module entries in this file (follow the existing
key naming convention used elsewhere in SettingsSidebarModulesUser.jsx and
mirror the equivalent keys used for the admin variant), so the 'assessment'
entry uses the consistent Chinese locale keys for title and description.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5d61076-7b0e-4639-8fbe-81a8f6aaac70
📒 Files selected for processing (7)
web/classic/src/App.jsxweb/classic/src/components/layout/SiderBar.jsxweb/classic/src/helpers/render.jsxweb/classic/src/hooks/common/useSidebar.jsweb/classic/src/pages/Assessment/index.jsxweb/classic/src/pages/Setting/Operation/SettingsSidebarModulesAdmin.jsxweb/classic/src/pages/Setting/Personal/SettingsSidebarModulesUser.jsx
Comment on lines
+23
to
+39
| const useData = (key, fetcher, deps = []) => { | ||
| const [data, setData] = useState(null); | ||
| const [loading, setLoading] = useState(false); | ||
| const load = useCallback(async () => { | ||
| setLoading(true); | ||
| try { | ||
| const res = await fetcher(); | ||
| setData(res?.data || res); | ||
| } catch { | ||
| /* ignore */ | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }, deps); | ||
| useEffect(() => { load(); }, [load]); | ||
| return { data, loading, reload: load }; | ||
| }; |
Contributor
There was a problem hiding this comment.
useData hook has multiple defects causing data to never render.
Several issues compound here:
- Double-unwrap bug. The fetcher returns
r.data(the full API envelope{success, message, data}), thensetData(res?.data || res)extracts the innerdata. Consumers then readdata?.data ?? [](e.g., lines 99, 160, 197, 290, 375-377), trying to unwrap a second time. Since the state is already the inner array/object,data?.datais alwaysundefinedand components will permanently render empty lists / empty stats. - Silent error swallowing.
catch { /* ignore */ }hides all failures from both the user and developers. Failed loads will appear identical to empty results. - Stale-closure risk.
useCallback(async () => { ... fetcher() ... }, deps)excludesfetcherfrom the dep list while building a newfetcherarrow function on every render. If a caller's fetcher closes over changing state, the captured one is stale. - Unused
keyparameter never used inside the hook.
🛠️ Proposed direction
Pick a single contract — either store the envelope and unwrap once at the consumer, or unwrap in the hook and have consumers read data directly. For example:
-const useData = (key, fetcher, deps = []) => {
- const [data, setData] = useState(null);
- const [loading, setLoading] = useState(false);
- const load = useCallback(async () => {
- setLoading(true);
- try {
- const res = await fetcher();
- setData(res?.data || res);
- } catch {
- /* ignore */
- } finally {
- setLoading(false);
- }
- }, deps);
- useEffect(() => { load(); }, [load]);
- return { data, loading, reload: load };
-};
+const useData = (fetcher, deps = []) => {
+ const [data, setData] = useState(null);
+ const [loading, setLoading] = useState(false);
+ const [error, setError] = useState(null);
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ const load = useCallback(async () => {
+ setLoading(true);
+ setError(null);
+ try {
+ const res = await fetcher();
+ // res is the full API envelope; expose its `.data` field
+ setData(res?.data ?? null);
+ } catch (e) {
+ setError(e);
+ console.error('useData fetch failed', e);
+ } finally {
+ setLoading(false);
+ }
+ }, deps);
+ useEffect(() => { load(); }, [load]);
+ return { data, loading, error, reload: load };
+};Then consumers use const items = data ?? [] (drop the second ?.data).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/classic/src/pages/Assessment/index.jsx` around lines 23 - 39, useData
currently double-unwraps the API envelope and swallows errors and can capture a
stale fetcher; fix it by (1) preserving the full response envelope in state
(call setData(res) instead of setData(res?.data || res)) so consumers that
access data?.data continue to work, (2) replace the empty catch with a logged
error (e.g., console.error or processLogger.error within the catch of load) and
optionally set an error state, (3) include fetcher in the dependency list used
by useCallback (e.g., build depsForCallback = [fetcher, ...deps]) to avoid stale
closures, and (4) either remove the unused key param or reference key in the
useEffect dependency array so reloads respond to key changes; update references
to load/useEffect/useCallback accordingly.
Comment on lines
+75
to
+96
| const handleSubmit = async () => { | ||
| if (!submitTarget) return; | ||
| setSubmitting(true); | ||
| try { | ||
| const fd = new FormData(); | ||
| fd.append('assessment_id', String(submitTarget.id)); | ||
| fd.append('content', content); | ||
| for (const f of files) fd.append('screenshots', f); | ||
| const res = await API.post('/api/assessment/submit', fd); | ||
| if (res.data.success) { | ||
| showSuccess(t('Submitted successfully')); | ||
| setSubmitTarget(null); | ||
| setContent(''); | ||
| setFiles([]); | ||
| reload(); | ||
| } | ||
| } catch { | ||
| showError(t('Submission failed')); | ||
| } finally { | ||
| setSubmitting(false); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
handleSubmit only shows success when res.data.success is true; failures are silent.
If the API responds with {success: false, message: '...'} (e.g., already-submitted, assessment not active, bad file type — all returned by the backend), no toast is shown and the modal stays open without feedback. The catch only fires on network errors. Surface the server-provided message on the non-success branch.
🛠️ Suggested change
const res = await API.post('/api/assessment/submit', fd);
if (res.data.success) {
showSuccess(t('Submitted successfully'));
setSubmitTarget(null);
setContent('');
setFiles([]);
reload();
+ } else {
+ showError(res.data.message || t('Submission failed'));
}
- } catch {
- showError(t('Submission failed'));
+ } catch (e) {
+ showError(e?.response?.data?.message || t('Submission failed'));
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/classic/src/pages/Assessment/index.jsx` around lines 75 - 96,
handleSubmit currently only treats res.data.success === true as a success and
swallows server-side failures; modify the non-success branch after the API.post
call to call showError with the server-provided message (use res.data.message ||
t('Submission failed') as a fallback) so users see backend validation/errors,
and avoid changing the existing success behavior (setSubmitTarget, setContent,
setFiles, reload) — locate handleSubmit and the
API.post('/api/assessment/submit', fd) response handling to implement this.
Comment on lines
+137
to
+143
| <Typography.Text strong>{t('Upload screenshots (png/jpg/gif/webp, optional)')}</Typography.Text> | ||
| <Input type="file" accept="image/*" multiple onChange={(e) => { | ||
| if (e.target.files) setFiles(Array.from(e.target.files)); | ||
| }} /> | ||
| {files.length > 0 && ( | ||
| <Typography.Text size="small" type="tertiary">{files.length} {t('file(s) selected')}</Typography.Text> | ||
| )} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does @douyinfe/semi-ui Input component's onChange callback receive a value or a synthetic event as its first argument?
💡 Result:
The @douyinfe/semi-ui Input component's onChange callback receives a value (string) as its first argument, followed by a synthetic event (e:event) as the second argument. The signature is documented as function(value:string, e:event) => void or (value:string, e:event) => void [1]. Example usage: const onChange = (val, e) => { console.log(val); setValue(val); }; [1].
Citations:
🏁 Script executed:
find . -name "assessment.go" -o -name "*assessment*.go" | head -20Repository: QuantumNous/new-api
Length of output: 189
🏁 Script executed:
cat -n ./controller/assessment.goRepository: QuantumNous/new-api
Length of output: 13180
🏁 Script executed:
sed -n '137,143p' web/classic/src/pages/Assessment/index.jsxRepository: QuantumNous/new-api
Length of output: 505
File picker is wired to Semi UI Input which does not expose the native event, breaking file selection entirely.
Input from @douyinfe/semi-ui calls onChange(value, event) with the field's string value as the first argument; the handler here uses (e) => { if (e.target.files) ... } and treats the first arg as a DOM event, so e.target.files is undefined and selected files are never captured. Additionally, accept="image/*" allows users to select formats (svg, bmp, etc.) that the backend rejects with "仅支持 png/jpg/jpeg/gif/webp 格式" (verified in controller/assessment.go lines 53–55, 257–260), creating a confusing failure mode.
Switch to a native <input type="file"> or Semi's Upload component and tighten the accept filter to .png,.jpg,.jpeg,.gif,.webp.
Suggested change
- <Input type="file" accept="image/*" multiple onChange={(e) => {
- if (e.target.files) setFiles(Array.from(e.target.files));
- }} />
+ <input
+ type="file"
+ accept=".png,.jpg,.jpeg,.gif,.webp"
+ multiple
+ onChange={(e) => {
+ if (e.target.files) setFiles(Array.from(e.target.files));
+ }}
+ />📝 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.
Suggested change
| <Typography.Text strong>{t('Upload screenshots (png/jpg/gif/webp, optional)')}</Typography.Text> | |
| <Input type="file" accept="image/*" multiple onChange={(e) => { | |
| if (e.target.files) setFiles(Array.from(e.target.files)); | |
| }} /> | |
| {files.length > 0 && ( | |
| <Typography.Text size="small" type="tertiary">{files.length} {t('file(s) selected')}</Typography.Text> | |
| )} | |
| <Typography.Text strong>{t('Upload screenshots (png/jpg/gif/webp, optional)')}</Typography.Text> | |
| <input | |
| type="file" | |
| accept=".png,.jpg,.jpeg,.gif,.webp" | |
| multiple | |
| onChange={(e) => { | |
| if (e.target.files) setFiles(Array.from(e.target.files)); | |
| }} | |
| /> | |
| {files.length > 0 && ( | |
| <Typography.Text size="small" type="tertiary">{files.length} {t('file(s) selected')}</Typography.Text> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/classic/src/pages/Assessment/index.jsx` around lines 137 - 143, The file
picker is using Semi UI's Input which passes (value, event) not a native DOM
event, so the handler (e) => e.target.files fails and files never get set;
replace the Semi <Input type="file"> with a native <input type="file" multiple>
(or use Semi's Upload component) and update the onChange to read
event.target.files and call setFiles(Array.from(event.target.files)); also
tighten the accept attribute to ".png,.jpg,.jpeg,.gif,.webp" to match backend
validation (ensure you reference the files state and setFiles setter used in
this component).
Comment on lines
+229
to
+287
| const { data, loading, reload } = useData('all', () => | ||
| API.get('/api/assessment/all').then(r => r.data) | ||
| ); | ||
| const [editItem, setEditItem] = useState(null); | ||
| const [form, setForm] = useState({ title: '', description: '', start_time: '', end_time: '', max_score: 100, status: 0 }); | ||
| const [dialogOpen, setDialogOpen] = useState(false); | ||
| const [saving, setSaving] = useState(false); | ||
|
|
||
| const openCreate = () => { | ||
| setEditItem(null); | ||
| setForm({ title: '', description: '', start_time: '', end_time: '', max_score: 100, status: 0 }); | ||
| setDialogOpen(true); | ||
| }; | ||
|
|
||
| const openEdit = (item) => { | ||
| setEditItem(item); | ||
| setForm({ | ||
| title: item.title, description: item.description, | ||
| start_time: dayjs.unix(item.start_time).format('YYYY-MM-DDTHH:mm'), | ||
| end_time: dayjs.unix(item.end_time).format('YYYY-MM-DDTHH:mm'), | ||
| max_score: item.max_score, status: item.status, | ||
| }); | ||
| setDialogOpen(true); | ||
| }; | ||
|
|
||
| const handleSave = async () => { | ||
| setSaving(true); | ||
| try { | ||
| const payload = { | ||
| title: form.title, description: form.description, | ||
| start_time: dayjs(form.start_time).unix(), | ||
| end_time: dayjs(form.end_time).unix(), | ||
| max_score: form.max_score, status: form.status, | ||
| }; | ||
| if (editItem) { | ||
| await API.put('/api/assessment', { ...payload, id: editItem.id }); | ||
| showSuccess(t('Updated successfully')); | ||
| } else { | ||
| await API.post('/api/assessment', payload); | ||
| showSuccess(t('Created successfully')); | ||
| } | ||
| setDialogOpen(false); | ||
| reload(); | ||
| } catch { | ||
| showError(t('Operation failed')); | ||
| } finally { | ||
| setSaving(false); | ||
| } | ||
| }; | ||
|
|
||
| const handleDelete = async (id) => { | ||
| try { | ||
| await API.delete(`/api/assessment/${id}`); | ||
| showSuccess(t('Deleted successfully')); | ||
| reload(); | ||
| } catch { | ||
| showError(t('Deletion failed')); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm backend admin routes and frontend calls align.
rg -nP --type=go -C2 '/assessment(/admin)?[^"]*' router/
echo '---'
rg -nP --type=jsx -C1 "API\.(get|post|put|delete)\(['\"]/api/assessment" web/classic/src/pages/Assessment/Repository: QuantumNous/new-api
Length of output: 1176
🏁 Script executed:
# Get complete assessment routes from backend
cat -n router/api-router.go | sed -n '385,420p'Repository: QuantumNous/new-api
Length of output: 1327
🏁 Script executed:
# Check actual API calls in the Assessment component
rg -n "API\.(get|post|put|delete)" web/classic/src/pages/Assessment/index.jsxRepository: QuantumNous/new-api
Length of output: 1016
🏁 Script executed:
# Also check the full context around the lines mentioned in the review
wc -l web/classic/src/pages/Assessment/index.jsxRepository: QuantumNous/new-api
Length of output: 110
Update API calls to match backend admin routes.
The frontend admin endpoints are missing the /admin prefix and have incorrect path patterns. All admin operations (create, update, delete, review) will fail with 404 errors:
| Frontend call | Backend route |
|---|---|
GET /api/assessment/all (lines 230, 347) |
GET /api/assessment/admin/ |
POST /api/assessment (line 267) |
POST /api/assessment/admin/ |
PUT /api/assessment (line 264) |
PUT /api/assessment/admin/ |
DELETE /api/assessment/${id} (line 281) |
DELETE /api/assessment/admin/${id} |
GET /api/assessment/${id}/submissions (line 351) |
GET /api/assessment/admin/submissions/${id} |
GET /api/assessment/${id}/stats (line 354) |
GET /api/assessment/admin/stats/${id} |
POST /api/assessment/review (line 364) |
POST /api/assessment/admin/review |
Update each call to include the /admin prefix and correct path parameter order where needed. Also applies to lines 346-364.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/classic/src/pages/Assessment/index.jsx` around lines 229 - 287, The admin
API routes used in useData, handleSave, handleDelete and any
submission/stats/review calls are using the wrong paths (missing the /admin
prefix and some parameter ordering); update the calls in useData('all', ...)
(API.get), handleSave (API.post and API.put), handleDelete (API.delete) and the
submission/stats/review GET/POST calls to use the backend admin routes—i.e.
change GET /api/assessment/all -> GET /api/assessment/admin/, POST
/api/assessment -> POST /api/assessment/admin/, PUT /api/assessment -> PUT
/api/assessment/admin/, DELETE /api/assessment/${id} -> DELETE
/api/assessment/admin/${id}, GET /api/assessment/${id}/submissions -> GET
/api/assessment/admin/submissions/${id}, GET /api/assessment/${id}/stats -> GET
/api/assessment/admin/stats/${id}, and POST /api/assessment/review -> POST
/api/assessment/admin/review so functions use
API.get/API.post/API.put/API.delete with those updated paths (refer to useData,
handleSave, handleDelete and the review/submissions/stats call sites).
Comment on lines
+254
to
+287
| const handleSave = async () => { | ||
| setSaving(true); | ||
| try { | ||
| const payload = { | ||
| title: form.title, description: form.description, | ||
| start_time: dayjs(form.start_time).unix(), | ||
| end_time: dayjs(form.end_time).unix(), | ||
| max_score: form.max_score, status: form.status, | ||
| }; | ||
| if (editItem) { | ||
| await API.put('/api/assessment', { ...payload, id: editItem.id }); | ||
| showSuccess(t('Updated successfully')); | ||
| } else { | ||
| await API.post('/api/assessment', payload); | ||
| showSuccess(t('Created successfully')); | ||
| } | ||
| setDialogOpen(false); | ||
| reload(); | ||
| } catch { | ||
| showError(t('Operation failed')); | ||
| } finally { | ||
| setSaving(false); | ||
| } | ||
| }; | ||
|
|
||
| const handleDelete = async (id) => { | ||
| try { | ||
| await API.delete(`/api/assessment/${id}`); | ||
| showSuccess(t('Deleted successfully')); | ||
| reload(); | ||
| } catch { | ||
| showError(t('Deletion failed')); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
handleSave/handleDelete rely on thrown exceptions but the API helper resolves on non-2xx.
In this codebase, API (helpers) typically resolves with {data: {success, message}} even on logical failure rather than throwing. As written, handleSave/handleDelete only surface errors from the catch branch, which means a backend-rejected save (e.g., end_time before start_time) will silently show "Updated successfully" because nothing throws. Mirror the success/failure handling used in handleSubmit (and improved per the comment above) — check res.data.success and surface res.data.message.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/classic/src/pages/Assessment/index.jsx` around lines 254 - 287,
handleSave and handleDelete assume API throws on failure but API resolves with
{data: {success, message}}; update both functions (handleSave and handleDelete)
to inspect the response (res.data.success) instead of relying on catch: call
API.post/put/delete and store the result, if res.data.success true then call
showSuccess(res.data.message || t('... success')), close dialog/reload as
appropriate, else call showError(res.data.message || t('Operation failed'));
preserve existing try/catch for network errors and ensure setSaving(false) still
runs in finally for handleSave.
Comment on lines
+279
to
+287
| const handleDelete = async (id) => { | ||
| try { | ||
| await API.delete(`/api/assessment/${id}`); | ||
| showSuccess(t('Deleted successfully')); | ||
| reload(); | ||
| } catch { | ||
| showError(t('Deletion failed')); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Destructive delete is performed without confirmation.
A single click on the "Delete" button immediately fires DELETE against the assessment, with no Modal.confirm / Popconfirm dialog. Together with the URL bug above, this is a foot-gun for admins who could nuke an assessment (and cascade affect submissions) by accident. Wrap handleDelete with a confirmation prompt.
🛠️ Suggested change
- <Button size="small" type="danger" onClick={() => handleDelete(item.id)}>{t('Delete')}</Button>
+ <Button size="small" type="danger" onClick={() => Modal.confirm({
+ title: t('Delete Assessment'),
+ content: t('Are you sure? This cannot be undone.'),
+ onOk: () => handleDelete(item.id),
+ })}>{t('Delete')}</Button>Also applies to: 304-305
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/classic/src/pages/Assessment/index.jsx` around lines 279 - 287, The
delete handler handleDelete currently issues a destructive API.delete
immediately; wrap it with a confirmation prompt so users must confirm before the
DELETE is executed. Replace direct calls to handleDelete from delete buttons
with a Modal.confirm or AntD Popconfirm that displays a clear warning (e.g.,
include assessment title/id) and only calls handleDelete(id) if the user
confirms; update any other delete usages (the other delete call sites
referenced) to use the same confirm flow to prevent accidental deletions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Localization
Chores