Fix auth, validation, and dashboard stats bugs#3
Fix auth, validation, and dashboard stats bugs#3notedwin-dev wants to merge 25 commits intoJavaScript-Mastery-Pro:mainfrom
Conversation
📝 WalkthroughWalkthroughThe changes introduce authorization scoping to API endpoints, restrict field-level updates by removing sensitive or computed fields from whitelists, implement server-side grade calculation logic, standardize error handling to exclude stack traces, add input validation for bulk operations, and adjust grade-to-point weightings in CGPA calculations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
## Summary - Enforce ownership and auth checks for updates/deletes and profile access; validate student creation and grade updates. - Harden grade creation/update defaults (term/maxMarks), prevent manual grade overrides, and align CGPA point mapping. - Improve UX and data integrity: dashboard totals, assignment move rollback, edit date normalization, maxMarks form handling, and attendance bulk validation. - Remove stack-trace leaks from API error responses. ## Testing - Not run (not requested)
notedwin-dev
changed the title
|
@CodeRabbit please try again... |
|
✅ Actions performedFull review triggered. |
|
@CodeRabbit please do a full code review and try to find potential security issues as well as uncaught potential bugs |
|
✅ Actions performedFull review triggered. |
![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
🧹 Nitpick comments (1)
app/api/grades/[id]/route.ts (1)
7-18: ExtractcalcGradeinto a shared util to avoid drift.
calcGradeis now defined identically here and inapp/api/grades/route.ts. If thresholds change again (as they did in this PR for A+), keeping both in sync is error-prone. Consider extracting to e.g.lib/grading.tsand importing from both routes.♻️ Proposed extraction
// lib/grading.ts export function calcGrade(marks: number, max: number): string { const pct = (marks / max) * 100 if (pct >= 90) return 'A+' if (pct >= 80) return 'A' if (pct >= 70) return 'B+' if (pct >= 60) return 'B' if (pct >= 50) return 'C' if (pct >= 40) return 'D' return 'F' }Then in both routes:
import { calcGrade } from '@/lib/grading'🤖 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 7 - 18, The calcGrade function is duplicated and should be extracted to a shared utility to avoid drift; create a new module (e.g., lib/grading.ts) that exports calcGrade(marks: number, max: number): string using the existing threshold logic, then replace the local calcGrade definition in this file and the other route that duplicates it by importing calcGrade (e.g., import { calcGrade } from '@/lib/grading') so both routes use the single implementation.
🤖 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 72-81: The code currently defaults an empty term to "Term 1" which
lets upserts (Grade.findOneAndUpdate keyed by teacherId, studentId, subject,
term) silently collide; instead make term required in the request validation
(change the Zod schema that produces parsed.data so term is z.string().min(1)),
stop applying a silent default (remove the `term = data.term?.trim() || 'Term
1'` fallback), and return a 400 Bad Request when parsed.data.term is
missing/empty before calling Grade.findOneAndUpdate; this ensures clients must
supply term and prevents accidental overwrites of existing records.
In `@app/dashboard/OverviewClient.tsx`:
- Around line 198-224: The upcomingDeadlines calculation currently uses the
paginated assignments list from assignmentsData (fetched via
fetch("/api/assignments") which defaults to limit=20) and can miss active items
beyond page 1; change the data source so deadlines are computed from the active
set: either modify the initial fetch to request the active set with a
sufficiently large limit (e.g.,
fetch("/api/assignments?status=active&limit=100")) or reuse
assignmentsActiveRes/assignmentsActiveData and derive upcomingDeadlines from
assignmentsActiveData.assignments (or perform an additional fetch for active
assignments) so that upcomingDeadlines uses the full active list rather than the
mixed paginated assignments array.
In `@components/dashboard/Navbar.tsx`:
- Around line 11-14: The current lazy useState initializer for dark reads
document during initial render causing SSR/CSR mismatch; change the initial
state to false (useState(false)), add a mounted flag (e.g., const [mounted,
setMounted] = useState(false)) and in useEffect on mount call
setDark(document.documentElement.classList.contains("dark")) then
setMounted(true); finally, guard the theme-dependent UI (the icon/title
rendering that reads dark) to only render when mounted (or render a neutral
placeholder) so SSR markup matches the first client paint; update references to
dark and setDark accordingly.
---
Nitpick comments:
In `@app/api/grades/`[id]/route.ts:
- Around line 7-18: The calcGrade function is duplicated and should be extracted
to a shared utility to avoid drift; create a new module (e.g., lib/grading.ts)
that exports calcGrade(marks: number, max: number): string using the existing
threshold logic, then replace the local calcGrade definition in this file and
the other route that duplicates it by importing calcGrade (e.g., import {
calcGrade } from '@/lib/grading') so both routes use the single implementation.
🪄 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: 97e89224-a866-477d-a120-59593b564979
📒 Files selected for processing (17)
app/api/announcements/[id]/route.tsapp/api/assignments/[id]/route.tsapp/api/assignments/route.tsapp/api/attendance/route.tsapp/api/grades/[id]/route.tsapp/api/grades/route.tsapp/api/profile/route.tsapp/api/students/[id]/route.tsapp/api/students/route.tsapp/dashboard/OverviewClient.tsxapp/dashboard/assignments/AssignmentsClient.tsxapp/dashboard/grades/GradesClient.tsxapp/dashboard/students/StudentsClient.tsxcomponents/dashboard/Navbar.tsxlib/mongodb.tsnext.config.tsproxy.ts
| const data = parsed.data | ||
| const max = data.maxMarks! | ||
| const term = data.term ?? 'Term 1' | ||
| const maxMarks = data.maxMarks | ||
| const term = data.term?.trim() || 'Term 1' | ||
|
|
||
| const grade = Grade.findOneAndUpdate( | ||
| const grade = await Grade.findOneAndUpdate( | ||
| { teacherId: userId, studentId: data.studentId, subject: data.subject, term }, | ||
| { $set: { ...data, term, teacherId: userId, grade: calcGrade(data.marks, max) } }, | ||
| { upsert: true, new: true } | ||
| { $set: { ...data, term, maxMarks, teacherId: userId, grade: calcGrade(data.marks, maxMarks) } }, | ||
| { upsert: true, new: true, runValidators: true, context: 'query' } | ||
| ) | ||
| return NextResponse.json(grade, { status: 201 }) |
There was a problem hiding this comment.
Empty-term default + upsert can silently overwrite a prior record.
With term defaulted to "Term 1" and the upsert keyed on {teacherId, studentId, subject, term}, two submissions without a term for the same (student, subject) will collide and the second one will overwrite the first under Term 1. If clients commonly omit the term (the client already defaults term to "Term 1" in openAdd, so this is usually fine, but direct API users may not), consider either:
- Requiring
termexplicitly at the schema level (z.string().min(1)) and returning 400 if missing, or - Returning 200 vs 201 based on whether the upsert created a new document (via
rawResult: true/includeResultMetadata: true) so clients can distinguish create from update.
Minor: the response always returns 201 Created even when the upsert merely updates an existing document.
🤖 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 72 - 81, The code currently defaults an
empty term to "Term 1" which lets upserts (Grade.findOneAndUpdate keyed by
teacherId, studentId, subject, term) silently collide; instead make term
required in the request validation (change the Zod schema that produces
parsed.data so term is z.string().min(1)), stop applying a silent default
(remove the `term = data.term?.trim() || 'Term 1'` fallback), and return a 400
Bad Request when parsed.data.term is missing/empty before calling
Grade.findOneAndUpdate; this ensures clients must supply term and prevents
accidental overwrites of existing records.
| fetch("/api/assignments"), | ||
| fetch("/api/assignments?status=active&limit=1"), | ||
| fetch("/api/attendance"), | ||
| fetch("/api/grades"), | ||
| fetch("/api/announcements?limit=5"), | ||
| ]); | ||
|
|
||
| const [students, assignmentsData, attendance, grades, announcements] = | ||
| await Promise.all([ | ||
| studentsRes.json(), | ||
| assignmentsRes.json(), | ||
| attendanceRes.json(), | ||
| gradesRes.json(), | ||
| announcementsRes.json(), | ||
| ]); | ||
| const [ | ||
| students, | ||
| assignmentsData, | ||
| assignmentsActiveData, | ||
| attendance, | ||
| grades, | ||
| announcements, | ||
| ] = await Promise.all([ | ||
| studentsRes.json(), | ||
| assignmentsRes.json(), | ||
| assignmentsActiveRes.json(), | ||
| attendanceRes.json(), | ||
| gradesRes.json(), | ||
| announcementsRes.json(), | ||
| ]); | ||
|
|
||
| const assignments = assignmentsData.assignments ?? assignmentsData; | ||
| const assignmentsTotal = assignmentsData.total ?? assignments.length; | ||
| const pendingAssignments = assignmentsActiveData.total ?? | ||
| (assignmentsActiveData.assignments ?? assignmentsActiveData ?? []).length; |
There was a problem hiding this comment.
LGTM on the count fix — but upcomingDeadlines still relies on the paginated list.
assignmentsTotal and pendingAssignments are now correctly derived from server-side total. However, the same assignmentsData.assignments array (line 221) is later used at lines 310–328 to compute upcomingDeadlines, and fetch('/api/assignments') (line 198) uses the server default of limit=20. For teachers whose newest 20 records include closed assignments, active items beyond page 1 will silently disappear from the "Upcoming Deadlines" panel.
Consider either fetching with status=active (and a larger limit) specifically for the deadlines panel, or reusing assignmentsActiveRes with an appropriate limit:
🔧 Suggested adjustment
- fetch("/api/assignments"),
- fetch("/api/assignments?status=active&limit=1"),
+ fetch("/api/assignments?limit=100"),
+ fetch("/api/assignments?status=active&limit=100"),Then derive upcomingDeadlines from assignmentsActiveData.assignments instead of the mixed list.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/dashboard/OverviewClient.tsx` around lines 198 - 224, The
upcomingDeadlines calculation currently uses the paginated assignments list from
assignmentsData (fetched via fetch("/api/assignments") which defaults to
limit=20) and can miss active items beyond page 1; change the data source so
deadlines are computed from the active set: either modify the initial fetch to
request the active set with a sufficiently large limit (e.g.,
fetch("/api/assignments?status=active&limit=100")) or reuse
assignmentsActiveRes/assignmentsActiveData and derive upcomingDeadlines from
assignmentsActiveData.assignments (or perform an additional fetch for active
assignments) so that upcomingDeadlines uses the full active list rather than the
mixed paginated assignments array.
| 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 reading document during initial render.
The lazy initializer runs on both server and client: on the server it returns false, while on the client it reflects the dark class already set by the layout's theme init script. When a user's stored/system preference is dark, the server-rendered markup (icon at lines 64–92, title at line 62) will differ from the first client render, triggering a React hydration warning and a brief visual flip.
Consider initializing to false and deriving the real value in a mount effect, or gating the toggle UI behind a mounted flag so SSR and first client paint agree.
♻️ Suggested pattern
- const [dark, setDark] = useState(() => {
- if (typeof document === "undefined") return false;
- return document.documentElement.classList.contains("dark");
- });
+ const [dark, setDark] = useState(false);
+ const [mounted, setMounted] = useState(false);
+
+ useEffect(() => {
+ setDark(document.documentElement.classList.contains("dark"));
+ setMounted(true);
+ }, []);Then guard the icon render on mounted (or render a neutral placeholder) to avoid the mismatch.
📝 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); | |
| const [mounted, setMounted] = useState(false); | |
| useEffect(() => { | |
| setDark(document.documentElement.classList.contains("dark")); | |
| setMounted(true); | |
| }, []); |
🤖 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 - 14, The current lazy
useState initializer for dark reads document during initial render causing
SSR/CSR mismatch; change the initial state to false (useState(false)), add a
mounted flag (e.g., const [mounted, setMounted] = useState(false)) and in
useEffect on mount call
setDark(document.documentElement.classList.contains("dark")) then
setMounted(true); finally, guard the theme-dependent UI (the icon/title
rendering that reads dark) to only render when mounted (or render a neutral
placeholder) so SSR markup matches the first client paint; update references to
dark and setDark accordingly.
1 participant
Summary
Verification
Summary by CodeRabbit
Bug Fixes
New Features
Improvements