fix: harden EduDesk API handlers and dashboard state#1
fix: harden EduDesk API handlers and dashboard state#1Deprasny wants to merge 48 commits intoJavaScript-Mastery-Pro:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 58 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThese changes enhance API security and validation across multiple endpoints. Resource update and deletion operations now verify teacher ownership via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 (2)
app/api/grades/route.ts (1)
69-79:⚠️ Potential issue | 🟠 MajorApply the default
maxMarksbefore validation and persistence.Line 73 treats omitted
maxMarksas100, but the schema still allowsmarks > 100whenmaxMarksis absent, and$setmay omit the default value. This can persist inconsistent grades.🐛 Proposed fix
const parsed = GradeSchema.safeParse(body) if (!parsed.success) return NextResponse.json({ error: parsed.error.flatten() }, { status: 400 }) - const data = parsed.data - const max = data.maxMarks ?? 100 + const maxMarks = parsed.data.maxMarks ?? 100 + if (parsed.data.marks > maxMarks) { + return NextResponse.json( + { + error: { + formErrors: [], + fieldErrors: { marks: ['marks must be less than or equal to maxMarks'] }, + }, + }, + { status: 400 } + ) + } + + const data = { ...parsed.data, maxMarks } const term = data.term ?? 'Term 1' const grade = await Grade.findOneAndUpdate( { teacherId: userId, studentId: data.studentId, subject: data.subject, term }, - { $set: { ...data, term, teacherId: userId, grade: calcGrade(data.marks, max) } }, + { $set: { ...data, term, teacherId: userId, grade: calcGrade(data.marks, maxMarks) } }, { upsert: true, new: true, runValidators: true, setDefaultsOnInsert: true } )🤖 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 - 79, The parsed input uses a computed max (max = data.maxMarks ?? 100) but never writes that default back into the data before validation/persistence, so marks can exceed intended max and the stored document may omit maxMarks; update the parsed data to set data.maxMarks = max (and data.term = term) before calling Grade.findOneAndUpdate and use that enriched data in the $set and in calcGrade so Grade.findOneAndUpdate (and validators) see the defaulted maxMarks; adjust references to data, max, term, Grade.findOneAndUpdate and calcGrade accordingly.app/api/grades/[id]/route.ts (1)
32-45:⚠️ Potential issue | 🟠 MajorGuard against non-object JSON bodies before iterating keys.
await req.json()can legitimately returnnull, a number, a string, or an array. On line 42,key in bodywill throwTypeError: Cannot use 'in' operator to search for '...' in null(or in a primitive), which is then caught by the outertry/catchand surfaced as a generic 500 instead of a clear 400.🛡️ Proposed fix
let body try { body = await req.json() } catch { return NextResponse.json({ error: 'Invalid JSON in request body' }, { status: 400 }) } + if (!body || typeof body !== 'object' || Array.isArray(body)) { + return NextResponse.json({ error: 'Request body must be an object' }, { status: 400 }) + } // Sanitize: only allow whitelisted fields const sanitizedBody: Record<string, unknown> = {} for (const key of ALLOWED_UPDATE_FIELDS) { if (key in body) { sanitizedBody[key] = body[key] } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/grades/`[id]/route.ts around lines 32 - 45, The code assumes the parsed request body (body from await req.json()) is an object and uses "key in body", which will throw for null/primitives/arrays; before iterating ALLOWED_UPDATE_FIELDS, validate that body is a plain object (e.g. typeof body === 'object' && body !== null && !Array.isArray(body')) and if not return NextResponse.json({ error: 'Invalid JSON object in request body' }, { status: 400 }); then proceed to build sanitizedBody by checking each key in ALLOWED_UPDATE_FIELDS against body; update the logic around the body variable and the sanitizedBody construction to perform this guard.
🤖 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/`[id]/route.ts:
- Line 7: ALLOWED_UPDATE_FIELDS currently includes studentId and studentName
which allows unauthorized reassignment; either remove 'studentId' and
'studentName' from ALLOWED_UPDATE_FIELDS so grades remain tied at creation, or
if reassignment is required, validate and derive server-side: when handling PUT
in route.ts, call Student.findOne({ _id: sanitizedBody.studentId, teacherId:
userId }) to ensure the target student belongs to the authenticated teacher and
then set studentName from that Student document instead of trusting client input
(use sanitizedBody.studentId only after successful lookup).
- Around line 52-57: The code can produce inconsistent persisted data because
sanitizedBody may still contain non-numeric marks/maxMarks while
nextMarks/nextMaxMarks use numeric fallbacks; either validate and reject
non-numeric input or coerce and normalize sanitizedBody before computing grade.
Update the handler around sanitizedBody/nextMarks/nextMaxMarks (the logic
referencing sanitizedBody, existingGrade, and calcGrade) to: 1) validate that if
sanitizedBody.marks or sanitizedBody.maxMarks are present they are numeric (and
that maxMarks > 0) and return 400 on invalid input, or 2) coerce values into
numbers (e.g. sanitizedBody.marks = Number(sanitizedBody.marks)) before
computing nextMarks/nextMaxMarks and before setting sanitizedBody so calcGrade
and the persisted marks/maxMarks stay in sync, then compute sanitizedBody.grade
= calcGrade(nextMarks, nextMaxMarks).
In `@components/dashboard/Navbar.tsx`:
- Around line 11-17: The current lazy initializer for the dark state causes a
hydration mismatch because on the server you return false (document undefined)
but on the client the initializer may read document and return true; change to
initialize dark to false and then synchronize with the DOM inside a useEffect
(call setDark(document.documentElement.classList.contains('dark'))), or keep the
lazy initializer but introduce a mounted flag (e.g., useState mounted +
useEffect to set mounted true) and only render theme-dependent UI (the toggle
title/SVG) when mounted (or use suppressHydrationWarning) so the server and
first client render match; update references to the dark state in Navbar (the
dark variable and setDark) and the toggle button rendering to respect the
mounted flag or post-hydration sync.
---
Outside diff comments:
In `@app/api/grades/`[id]/route.ts:
- Around line 32-45: The code assumes the parsed request body (body from await
req.json()) is an object and uses "key in body", which will throw for
null/primitives/arrays; before iterating ALLOWED_UPDATE_FIELDS, validate that
body is a plain object (e.g. typeof body === 'object' && body !== null &&
!Array.isArray(body')) and if not return NextResponse.json({ error: 'Invalid
JSON object in request body' }, { status: 400 }); then proceed to build
sanitizedBody by checking each key in ALLOWED_UPDATE_FIELDS against body; update
the logic around the body variable and the sanitizedBody construction to perform
this guard.
In `@app/api/grades/route.ts`:
- Around line 69-79: The parsed input uses a computed max (max = data.maxMarks
?? 100) but never writes that default back into the data before
validation/persistence, so marks can exceed intended max and the stored document
may omit maxMarks; update the parsed data to set data.maxMarks = max (and
data.term = term) before calling Grade.findOneAndUpdate and use that enriched
data in the $set and in calcGrade so Grade.findOneAndUpdate (and validators) see
the defaulted maxMarks; adjust references to data, max, term,
Grade.findOneAndUpdate and calcGrade 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: 3171898b-8b16-472e-9ea8-6d3c69429ee7
📒 Files selected for processing (11)
app/api/announcements/[id]/route.tsapp/api/assignments/[id]/route.tsapp/api/assignments/route.tsapp/api/grades/[id]/route.tsapp/api/grades/route.tsapp/api/profile/route.tsapp/api/students/[id]/route.tsapp/api/students/route.tscomponents/dashboard/Navbar.tsxlib/mongodb.tsproxy.ts
| const nextMarks = | ||
| typeof sanitizedBody.marks === 'number' ? sanitizedBody.marks : existingGrade.marks | ||
| const nextMaxMarks = | ||
| typeof sanitizedBody.maxMarks === 'number' ? sanitizedBody.maxMarks : existingGrade.maxMarks | ||
|
|
||
| sanitizedBody.grade = calcGrade(nextMarks, nextMaxMarks) |
There was a problem hiding this comment.
Recalculated grade can drift from the persisted marks/maxMarks.
The typeof … === 'number' guards make nextMarks/nextMaxMarks fall back to the existing values when the client sends a non-number (e.g. "90" as a string, or null). However, the raw value is still present in sanitizedBody and will be $set on the document — Mongoose may cast the string to a number, so grade gets computed from the old marks while the stored marks reflects the new value, producing an inconsistent record.
Either reject non-numeric marks/maxMarks with a 400, or normalize/coerce into sanitizedBody so the computation and the persisted fields stay in sync:
♻️ Suggested direction
- const nextMarks =
- typeof sanitizedBody.marks === 'number' ? sanitizedBody.marks : existingGrade.marks
- const nextMaxMarks =
- typeof sanitizedBody.maxMarks === 'number' ? sanitizedBody.maxMarks : existingGrade.maxMarks
+ if ('marks' in sanitizedBody && typeof sanitizedBody.marks !== 'number') {
+ return NextResponse.json({ error: '`marks` must be a number' }, { status: 400 })
+ }
+ if ('maxMarks' in sanitizedBody && typeof sanitizedBody.maxMarks !== 'number') {
+ return NextResponse.json({ error: '`maxMarks` must be a number' }, { status: 400 })
+ }
+ const nextMarks = (sanitizedBody.marks as number | undefined) ?? existingGrade.marks
+ const nextMaxMarks = (sanitizedBody.maxMarks as number | undefined) ?? existingGrade.maxMarks
+ if (!nextMaxMarks || nextMaxMarks <= 0) {
+ return NextResponse.json({ error: '`maxMarks` must be greater than 0' }, { status: 400 })
+ }The explicit maxMarks > 0 check also removes the theoretical divide-by-zero / NaN result from calcGrade if legacy data ever has maxMarks === 0.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/grades/`[id]/route.ts around lines 52 - 57, The code can produce
inconsistent persisted data because sanitizedBody may still contain non-numeric
marks/maxMarks while nextMarks/nextMaxMarks use numeric fallbacks; either
validate and reject non-numeric input or coerce and normalize sanitizedBody
before computing grade. Update the handler around
sanitizedBody/nextMarks/nextMaxMarks (the logic referencing sanitizedBody,
existingGrade, and calcGrade) to: 1) validate that if sanitizedBody.marks or
sanitizedBody.maxMarks are present they are numeric (and that maxMarks > 0) and
return 400 on invalid input, or 2) coerce values into numbers (e.g.
sanitizedBody.marks = Number(sanitizedBody.marks)) before computing
nextMarks/nextMaxMarks and before setting sanitizedBody so calcGrade and the
persisted marks/maxMarks stay in sync, then compute sanitizedBody.grade =
calcGrade(nextMarks, nextMaxMarks).
| const [dark, setDark] = useState(() => { | ||
| if (typeof document === 'undefined') { | ||
| return false | ||
| } | ||
| }, []); | ||
|
|
||
| return document.documentElement.classList.contains('dark') | ||
| }) |
There was a problem hiding this comment.
Potential hydration mismatch from lazy initializer.
During SSR, typeof document === 'undefined' so dark initializes to false, and the server-rendered HTML will always show the moon icon and "Switch to dark mode" title. On the client, the lazy initializer runs again during hydration and may return true (since the inline script in app/layout.tsx has already set the dark class on documentElement). This produces a React hydration mismatch on the toggle button's title attribute and rendered SVG whenever the user's resolved theme is dark.
Consider initializing to false and syncing from the DOM inside useEffect (accepting one extra render, which was the original intent), or keep the lazy initializer but guard the theme-dependent UI behind a mounted flag / suppressHydrationWarning.
Proposed fix
- const [dark, setDark] = useState(() => {
- if (typeof document === 'undefined') {
- return false
- }
-
- return document.documentElement.classList.contains('dark')
- })
+ const [dark, setDark] = useState(false)
+
+ // Sync from DOM after mount to avoid hydration mismatch with the
+ // pre-hydration theme script in app/layout.tsx.
+ useEffect(() => {
+ setDark(document.documentElement.classList.contains('dark'))
+ }, [])📝 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 [dark, setDark] = useState(() => { | |
| if (typeof document === 'undefined') { | |
| return false | |
| } | |
| }, []); | |
| return document.documentElement.classList.contains('dark') | |
| }) | |
| const [dark, setDark] = useState(false) | |
| // Sync from DOM after mount to avoid hydration mismatch with the | |
| // pre-hydration theme script in app/layout.tsx. | |
| useEffect(() => { | |
| setDark(document.documentElement.classList.contains('dark')) | |
| }, []) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/dashboard/Navbar.tsx` around lines 11 - 17, The current lazy
initializer for the dark state causes a hydration mismatch because on the server
you return false (document undefined) but on the client the initializer may read
document and return true; change to initialize dark to false and then
synchronize with the DOM inside a useEffect (call
setDark(document.documentElement.classList.contains('dark'))), or keep the lazy
initializer but introduce a mounted flag (e.g., useState mounted + useEffect to
set mounted true) and only render theme-dependent UI (the toggle title/SVG) when
mounted (or use suppressHydrationWarning) so the server and first client render
match; update references to the dark state in Navbar (the dark variable and
setDark) and the toggle button rendering to respect the mounted flag or
post-hydration sync.
1 participant
Summary
middleware.tstoproxy.tsfor Next 16Verification
npm run lintnpx tsc --noEmitnpm run buildSummary by CodeRabbit
Bug Fixes
New Features