Conversation
📝 WalkthroughWalkthroughThree files updated to improve API error handling consistency, refine user profile resolution logic with explicit fallbacks, and increase precision of grade average calculations in the radar chart from whole numbers to two-decimal fixed format. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/grades/route.ts (1)
76-80:⚠️ Potential issue | 🟡 MinorGood catch adding
awaittofindOneAndUpdate.Without the
await, the handler previously returned a pendingQueryas the JSON body and DB/pre-hook errors escaped thetry/catch. One follow-up: thepre('findOneAndUpdate')hook inmodels/Grade.tsthrows"marks must be less than or equal to maxMarks"— withawaitin place this will now be caught and collapsed to a generic 500. The Zodrefineon lines 14–20 already covers this case whenmaxMarksis present, but note thatmaxMarksis optional in the schema andconst max = data.maxMarks!on line 73 uses a non-null assertion; if a client omitsmaxMarks,maxisundefined,calcGrade(data.marks, undefined)yieldsNaN, and the Zod refine short-circuits on!data.maxMarks, so the bad value silently reaches the DB. Consider requiringmaxMarks(or defaulting it) rather than asserting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/grades/route.ts` around lines 76 - 80, The handler currently asserts maxMarks with const max = data.maxMarks! and calls calcGrade(data.marks, max) which can produce NaN if maxMarks is omitted; update the Zod input schema to require or provide a default for maxMarks (modify the schema where the Zod refine is defined around lines 14–20) OR change the handler to derive max safely (e.g. use a default or throw a validation error before calling calcGrade) and remove the non-null assertion; ensure Grade.findOneAndUpdate usage remains awaited and that validation prevents invalid marks from reaching the pre('findOneAndUpdate') hook in models/Grade.ts.
🧹 Nitpick comments (2)
app/api/profile/route.ts (1)
38-38: Avoid JSON round-tripping Mongoose documents.
JSON.parse(JSON.stringify(newTeacher))works as a response-normalization shortcut, but it discards runtime types before the response boundary. PrefernewTeacher.toObject()or return a lean document from the create/upsert flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/profile/route.ts` at line 38, Replace the JSON round-trip on the Mongoose document: instead of assigning teacher = JSON.parse(JSON.stringify(newTeacher)), call the Mongoose instance method to convert the document to a plain object (e.g. use newTeacher.toObject()) or ensure your create/upsert returns a lean document; update the assignment to use newTeacher.toObject() (or the lean result) so runtime types and Mongoose behavior are preserved.app/dashboard/grades/GradesClient.tsx (1)
211-218: Two-decimal precision here is partially cosmetic and inconsistent withactiveAvg.A couple of notes on this change:
- The inner
pct(g.marks, g.maxMarks)(line 58–60) already appliesMath.round(...), so the averaged values being summed are whole-number integers. ThetoFixed(2)therefore only exposes fractional parts introduced by the divisionsum / sg.length, not the underlying precision of each subject's percentage. If the intent was truly higher precision, compute the raw ratio here instead of callingpct.activeAvg(lines 340–346) still usesMath.round(...)for the stats bar, so the radar chart and the "Avg: X%" stat will disagree for the same subject. Consider applying the same rounding policy in both places.- Minor: the reassignment pattern can collapse to a single
const.♻️ Suggested simplification (chart-only precision)
- let avg = - sg.length > 0 - ? - sg.reduce((s, g) => s + pct(g.marks, g.maxMarks), 0) / sg.length - : 0 - - avg = Number(avg.toFixed(2)); - + const avg = + sg.length > 0 + ? Number( + ( + sg.reduce((s, g) => s + (g.marks / g.maxMarks) * 100, 0) / + sg.length + ).toFixed(2) + ) + : 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/dashboard/grades/GradesClient.tsx` around lines 211 - 218, The avg calculation uses pct(...) (which applies Math.round) then divides, causing inconsistent precision vs activeAvg and a pointless toFixed reassign; instead compute the raw percentage ratio from each subject (use g.marks / g.maxMarks * 100 rather than pct(g.marks, g.maxMarks)), average those raw values, then apply the desired rounding policy consistently with activeAvg (either two-decimal for chart-only or Math.round for both); also collapse the reassignment into a single const avg, updating references to avg, pct, and activeAvg accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/grades/route.ts`:
- Around line 69-70: The validation response currently returns the raw ZodError
(parsed.error) from the GradeSchema safeParse; change the handler to return the
flattened error shape instead by calling parsed.error.flatten() and pass that
flattened result into NextResponse.json so the response uses
formErrors/fieldErrors rather than exposing Zod internals (update the code
around the GradeSchema safeParse and the NextResponse.json error return).
In `@app/api/profile/route.ts`:
- Around line 10-15: The code currently trusts queryUserId before confirming
authentication; call auth() first and if session is missing immediately return
401, then derive userId only from session.userId (do not allow queryUserId to
override it). Locate the auth() call and the variables queryUserId, session and
userId in route.ts and change the assignment so userId = session.userId (ignore
or validate queryUserId only after verifying the session and explicit
authorization, e.g. admin check in a separate branch).
- Around line 21-38: The current findOne → create flow can race and hit the
unique clerkId constraint; replace it with an atomic upsert or
retry-on-duplicate pattern: use Teacher.findOneAndUpdate({ clerkId: userId }, {
$setOnInsert: { clerkId: userId, name: ..., email: ..., department: '',
subjects: [] } }, { upsert: true, new: true, setDefaultsOnInsert: true }) (or
wrap Teacher.create in a try/catch that retries a findOne after E11000
duplicate-key error) so that the logic around Teacher.findOne, Teacher.create
and assignment to the teacher variable becomes atomic and race-safe while still
using currentUser() to populate defaults.
---
Outside diff comments:
In `@app/api/grades/route.ts`:
- Around line 76-80: The handler currently asserts maxMarks with const max =
data.maxMarks! and calls calcGrade(data.marks, max) which can produce NaN if
maxMarks is omitted; update the Zod input schema to require or provide a default
for maxMarks (modify the schema where the Zod refine is defined around lines
14–20) OR change the handler to derive max safely (e.g. use a default or throw a
validation error before calling calcGrade) and remove the non-null assertion;
ensure Grade.findOneAndUpdate usage remains awaited and that validation prevents
invalid marks from reaching the pre('findOneAndUpdate') hook in models/Grade.ts.
---
Nitpick comments:
In `@app/api/profile/route.ts`:
- Line 38: Replace the JSON round-trip on the Mongoose document: instead of
assigning teacher = JSON.parse(JSON.stringify(newTeacher)), call the Mongoose
instance method to convert the document to a plain object (e.g. use
newTeacher.toObject()) or ensure your create/upsert returns a lean document;
update the assignment to use newTeacher.toObject() (or the lean result) so
runtime types and Mongoose behavior are preserved.
In `@app/dashboard/grades/GradesClient.tsx`:
- Around line 211-218: The avg calculation uses pct(...) (which applies
Math.round) then divides, causing inconsistent precision vs activeAvg and a
pointless toFixed reassign; instead compute the raw percentage ratio from each
subject (use g.marks / g.maxMarks * 100 rather than pct(g.marks, g.maxMarks)),
average those raw values, then apply the desired rounding policy consistently
with activeAvg (either two-decimal for chart-only or Math.round for both); also
collapse the reassignment into a single const avg, updating references to avg,
pct, and activeAvg accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e15218e5-25fc-40bf-b522-358aa23f9b54
📒 Files selected for processing (3)
app/api/grades/route.tsapp/api/profile/route.tsapp/dashboard/grades/GradesClient.tsx
| const parsed = GradeSchema.safeParse(body) | ||
| if (!parsed.success) return NextResponse.json({ error: parsed.error.flatten() }, { status: 400 }) | ||
| if (!parsed.success) return NextResponse.json({ error: parsed.error }, { status: 400 }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Zod ZodError toJSON vs flatten() vs format() recommended error response shape
💡 Result:
For ZodError error handling in Zod (stable v4 as of 2026), use these methods to shape responses: - error.toJSON: Serializes the ZodError to JSON, producing {name: 'ZodError', issues: [...]}, where issues is the raw array of ZodIssue objects (code, path, message, etc.). Suitable for simple API responses but retains full structured details. Note: JSON.stringify(error) uses this via the Error prototype's message getter, which stringifies issues. - error.flatten or z.flattenError(error): Recommended for flat schemas (most common, e.g., forms, top-level API objects). Returns {formErrors: string[], fieldErrors: {[key: string]: string[]}}, grouping top-level errors and per-field error arrays. Ideal for client-side form error display or simple API field-error responses. - error.format or z.formatError(error): Deprecated; use z.treeifyError(error) instead. treeifyError produces a nested object mirroring the schema structure: {errors: string[], properties: {...nested errors...}}. Best for deeply nested schemas. Recommended error response shape depends on use case: - For API responses (e.g., HTTP 400), use flattened shape for simplicity: {formErrors: [...], fieldErrors: {field1: [...], ...}}. Easy to parse, matches common patterns (React Hook Form, etc.), avoids deep nesting. - Raw error.toJSON if full issue details needed (codes, expected types). - treeifyError only for complex nested data. Official docs emphasize flatten for flat schemas (majority) and treeify for nested. Examples show flatten for form handling. GitHub issue confirms JSON serialization via issues array, with caveats for client reconstruction (use new ZodError(issues)). Examples: const flattened = error.flatten; // {formErrors: ['...'], fieldErrors: {username: ['...']}} res.status(400).json(flattened); // API response JSON.stringify(error); // '{"name":"ZodError","issues":[...]}'
Citations:
- 1: https://zod.dev/error-formatting
- 2: https://zod.dev/ERROR_HANDLING?id=flattening-errors
- 3: https://v4.zod.dev/error-formatting?id=zflattenerror
- 4: https://v4.zod.dev/error-formatting
- 5: https://v4.zod.dev/packages/core?id=errors
- 6:
flatten()broken/useless when doingnew z.ZodError()with a JSON stringified/parsed ZodError coming from the server colinhacks/zod#2351
🏁 Script executed:
cat -n app/api/grades/route.ts | head -80 | tail -20Repository: JavaScript-Mastery-Pro/discord-challenge
Length of output: 903
🏁 Script executed:
git log --oneline -5 -- app/api/grades/route.tsRepository: JavaScript-Mastery-Pro/discord-challenge
Length of output: 121
🏁 Script executed:
rg "NextResponse.json.*error" app/api/grades/route.ts -A 1Repository: JavaScript-Mastery-Pro/discord-challenge
Length of output: 629
🏁 Script executed:
rg "GradeSchema" app/api/grades/route.ts -B 5 -A 10Repository: JavaScript-Mastery-Pro/discord-challenge
Length of output: 1257
🏁 Script executed:
git diff HEAD~ app/api/grades/route.ts 2>/dev/null || echo "No previous version"Repository: JavaScript-Mastery-Pro/discord-challenge
Length of output: 103
🏁 Script executed:
fd "GradeSchema" --type f --exec grep -l "GradeSchema" {} \;Repository: JavaScript-Mastery-Pro/discord-challenge
Length of output: 66
Validation error response uses raw ZodError instead of flattened shape.
Line 70 returns parsed.error directly, which serializes to {name: 'ZodError', issues: [...]} and exposes Zod internals. This breaks consistency: every other error in this file uses a plain string ({ error: 'string' }), and this ZodError shape will confuse clients expecting a uniform error contract.
Since GradeSchema is flat (no nested objects), use .flatten() to produce {formErrors: string[], fieldErrors: {field: string[]}}, which is both client-friendly and the documented Zod best practice for flat schemas.
♻️ Suggested fix
- if (!parsed.success) return NextResponse.json({ error: parsed.error }, { status: 400 })
+ if (!parsed.success) return NextResponse.json({ error: parsed.error.flatten() }, { status: 400 })📝 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 parsed = GradeSchema.safeParse(body) | |
| if (!parsed.success) return NextResponse.json({ error: parsed.error.flatten() }, { status: 400 }) | |
| if (!parsed.success) return NextResponse.json({ error: parsed.error }, { status: 400 }) | |
| const parsed = GradeSchema.safeParse(body) | |
| if (!parsed.success) return NextResponse.json({ error: parsed.error.flatten() }, { status: 400 }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/grades/route.ts` around lines 69 - 70, The validation response
currently returns the raw ZodError (parsed.error) from the GradeSchema
safeParse; change the handler to return the flattened error shape instead by
calling parsed.error.flatten() and pass that flattened result into
NextResponse.json so the response uses formErrors/fieldErrors rather than
exposing Zod internals (update the code around the GradeSchema safeParse and the
NextResponse.json error return).
| // 1. Identificazione utente | ||
| const session = await auth() | ||
| const userId = queryUserId || session?.userId | ||
|
|
||
| if (!userId) { | ||
| const session = await auth() | ||
| userId = session.userId | ||
| return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) |
There was a problem hiding this comment.
Do not let queryUserId bypass authentication or override the session user.
Line 12 trusts queryUserId before confirming an authenticated session. This allows /api/profile?userId=... to read another profile unauthenticated if it exists, and can also create mismatched records later by combining a supplied clerkId with the current Clerk user data.
🔒 Proposed fix
- const session = await auth()
- const userId = queryUserId || session?.userId
+ const { userId: sessionUserId } = await auth()
- if (!userId) {
+ if (!sessionUserId) {
return NextResponse.json({ error: 'Unauthorized' }, { status: 401 })
}
+
+ if (queryUserId && queryUserId !== sessionUserId) {
+ return NextResponse.json({ error: 'Forbidden' }, { status: 403 })
+ }
+
+ const userId = queryUserId ?? sessionUserId📝 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.
| // 1. Identificazione utente | |
| const session = await auth() | |
| const userId = queryUserId || session?.userId | |
| if (!userId) { | |
| const session = await auth() | |
| userId = session.userId | |
| return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) | |
| // 1. Identificazione utente | |
| const { userId: sessionUserId } = await auth() | |
| if (!sessionUserId) { | |
| return NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) | |
| } | |
| if (queryUserId && queryUserId !== sessionUserId) { | |
| return NextResponse.json({ error: 'Forbidden' }, { status: 403 }) | |
| } | |
| const userId = queryUserId ?? sessionUserId |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/profile/route.ts` around lines 10 - 15, The code currently trusts
queryUserId before confirming authentication; call auth() first and if session
is missing immediately return 401, then derive userId only from session.userId
(do not allow queryUserId to override it). Locate the auth() call and the
variables queryUserId, session and userId in route.ts and change the assignment
so userId = session.userId (ignore or validate queryUserId only after verifying
the session and explicit authorization, e.g. admin check in a separate branch).
| let teacher = await Teacher.findOne({ clerkId: userId }).lean() | ||
|
|
||
| if (!teacher) { | ||
| const clerkUser = await currentUser() | ||
| const created = await Teacher.create({ | ||
|
|
||
| if (!clerkUser) { | ||
| return NextResponse.json({ error: 'User data not found' }, { status: 404 }) | ||
| } | ||
|
|
||
| const newTeacher = await Teacher.create({ | ||
| clerkId: userId, | ||
| name: clerkUser?.fullName ?? '', | ||
| email: clerkUser?.emailAddresses[0]?.emailAddress ?? '', | ||
| name: `${clerkUser.firstName ?? ''} ${clerkUser.lastName ?? ''}`.trim() || 'Anonymous', | ||
| email: clerkUser.emailAddresses[0]?.emailAddress ?? '', | ||
| department: '', | ||
| subjects: [], | ||
| }) | ||
| teacher = created.toObject() | ||
|
|
||
| teacher = JSON.parse(JSON.stringify(newTeacher)) |
There was a problem hiding this comment.
Make first-profile creation atomic.
The findOne → create sequence can race for a new user. With the unique clerkId constraint in models/Teacher.ts, concurrent first GETs can produce a duplicate-key error and return 500.
Prefer an atomic upsert or handle duplicate-key retry:
🛠️ Proposed direction
- const newTeacher = await Teacher.create({
- clerkId: userId,
- name: `${clerkUser.firstName ?? ''} ${clerkUser.lastName ?? ''}`.trim() || 'Anonymous',
- email: clerkUser.emailAddresses[0]?.emailAddress ?? '',
- department: '',
- subjects: [],
- })
-
- teacher = JSON.parse(JSON.stringify(newTeacher))
+ teacher = await Teacher.findOneAndUpdate(
+ { clerkId: userId },
+ {
+ $setOnInsert: {
+ clerkId: userId,
+ name: `${clerkUser.firstName ?? ''} ${clerkUser.lastName ?? ''}`.trim() || 'Anonymous',
+ email: clerkUser.emailAddresses[0]?.emailAddress ?? '',
+ department: '',
+ subjects: [],
+ },
+ },
+ { new: true, upsert: true, setDefaultsOnInsert: true },
+ ).lean()📝 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.
| let teacher = await Teacher.findOne({ clerkId: userId }).lean() | |
| if (!teacher) { | |
| const clerkUser = await currentUser() | |
| const created = await Teacher.create({ | |
| if (!clerkUser) { | |
| return NextResponse.json({ error: 'User data not found' }, { status: 404 }) | |
| } | |
| const newTeacher = await Teacher.create({ | |
| clerkId: userId, | |
| name: clerkUser?.fullName ?? '', | |
| email: clerkUser?.emailAddresses[0]?.emailAddress ?? '', | |
| name: `${clerkUser.firstName ?? ''} ${clerkUser.lastName ?? ''}`.trim() || 'Anonymous', | |
| email: clerkUser.emailAddresses[0]?.emailAddress ?? '', | |
| department: '', | |
| subjects: [], | |
| }) | |
| teacher = created.toObject() | |
| teacher = JSON.parse(JSON.stringify(newTeacher)) | |
| let teacher = await Teacher.findOne({ clerkId: userId }).lean() | |
| if (!teacher) { | |
| const clerkUser = await currentUser() | |
| if (!clerkUser) { | |
| return NextResponse.json({ error: 'User data not found' }, { status: 404 }) | |
| } | |
| teacher = await Teacher.findOneAndUpdate( | |
| { clerkId: userId }, | |
| { | |
| $setOnInsert: { | |
| clerkId: userId, | |
| name: `${clerkUser.firstName ?? ''} ${clerkUser.lastName ?? ''}`.trim() || 'Anonymous', | |
| email: clerkUser.emailAddresses[0]?.emailAddress ?? '', | |
| department: '', | |
| subjects: [], | |
| }, | |
| }, | |
| { new: true, upsert: true, setDefaultsOnInsert: true }, | |
| ).lean() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/profile/route.ts` around lines 21 - 38, The current findOne → create
flow can race and hit the unique clerkId constraint; replace it with an atomic
upsert or retry-on-duplicate pattern: use Teacher.findOneAndUpdate({ clerkId:
userId }, { $setOnInsert: { clerkId: userId, name: ..., email: ..., department:
'', subjects: [] } }, { upsert: true, new: true, setDefaultsOnInsert: true })
(or wrap Teacher.create in a try/catch that retries a findOne after E11000
duplicate-key error) so that the logic around Teacher.findOne, Teacher.create
and assignment to the teacher variable becomes atomic and race-safe while still
using currentUser() to populate defaults.
1 participant
Summary by CodeRabbit