Uh oh!
There was an error while loading. Please reload this page.
feat(gradebook): term-aware course links + transcript & GPA (B6: #139) - #468
Merged
Conversation
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 1831442 | Commit Preview URL Branch Preview URL | Jul 30 2026, 07:51 AM |
📝 WalkthroughWalkthroughGradebook now supports semester-scoped course loading and mutations, displays term and cumulative GPA transcript data, preserves selected terms in course links, adds stable E2E test IDs, and includes unit, component, and browser-level coverage. ChangesGradebook semester and transcript flow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
AndresL230
commented
Jul 30, 2026
CollaboratorAuthor
Code reviewFound 1 issue:
Sapling/frontend/src/components/screens/Gradebook/Course.tsx Lines 662 to 698 in 2e50986 Sub-threshold notes (verified, fixed in a follow-up commit, scores <80): transcript rows render 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
The gradebook's term switcher was half-wired: the landing had semester chips, but the course-card link and getGradebookCourse dropped the selected term, so _resolve_enrollment(user, course, None) resolved the CURRENT term — a course also taken in an archived term (rich seed: CS101 in both fall-2025 and spring-2026) 404'd from that term's chip. And the backend's GPA endpoints had no frontend consumer at all. - CourseCard carries the selected term (?semester=<label>) on the card href; the course screen reads it off location (same pattern as Landing's deep-link param) and passes it through getGradebookCourse. - GradebookSummary stops discarding the summary's gpa/semester; the landing surfaces "Term GPA x.xx" next to the chips (gradebook-term-gpa, hidden while null). - New TranscriptModal (gradebook-transcript-open) over the previously unconsumed GET /api/gradebook/gpa: cumulative GPA (gradebook-transcript-gpa) + per-semester sections, in-progress rows listed but excluded from the math. Load failure toasts + inline retry (#463 pattern). - New pure lib/transcript.ts (buildTranscript/weightedGpa) mirroring backend gradebook_service.weighted_gpa exactly (null grade_points skipped, null/zero credits count as 1, empty -> null); term ordering reuses lib/semesters (new compareTermLabels export). - Testid surface `gradebook` registered in docs/frontend-testids.md + the four owning files joined the eslint no-restricted-syntax block; suppressions baseline regenerated for the pre-existing untagged elements (Course.tsx 9, Landing.tsx 1). - e2e/gradebook.spec.ts (authored, not run here): the promoted #139 regression journey (DB-truth precondition via queryRaw, then both chips' CS101 cards resolve their own term's categories) + the transcript journey. - vitest: lib/transcript.test.ts, TranscriptModal.test.tsx (dialog contract + loading/error/retry), Course.semester.test.tsx (param plumbing), Landing.test.tsx extended (term GPA, real CourseCard href under test via a next/link stub). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lay (#468 review) Closes the three findings from the #468 5-agent review of the #139 work. 1 (major) — read/write term split: getGradebookCourse was semester-aware but every course-keyed mutation from the Course screen still resolved term-blind, so editing a Fall 2025 CS101 page wrote into the Spring 2026 enrollment silently (backend _resolve_enrollment(course, None) = current term). The backend body models already accept `semester`; now the frontend sends it: optional `semester` on createCategory, bulkUpdateCategories, createGradedAssignment, setLetterScale and setCurveSettings in lib/api.ts (body field, JSON.stringify drops it when unset), passed at every Course.tsx call site (curve toggle + curve settings + weights + create-assignment + letter scale). The id-keyed calls (deleteCategory, update/deleteGradedAssignment) resolve by row ownership — verified against routes/gradebook.py — and stay as-is. - Course.semester.test.tsx: 4 new tests drive the REAL EditWeightsModal/AssignmentModal down to Save and assert bulkUpdateCategories/createGradedAssignment get the URL's semester when ?semester= is set, and undefined when absent. - e2e/gradebook.spec.ts: the Fall leg now also creates an assignment through the UI and queryRaw-polls that the new assignments row hangs off rich-enr-active-cs101-f25, not the spring enrollment. New testids gradebook-add-assignment / gradebook-assignment-title / gradebook-assignment-save; AssignmentList.tsx + AssignmentModal.tsx joined the eslint enforcement array (pre-existing untagged elements baselined: 7 + 12) and the docs inventory. 2 — transcript credits display: the modal showed `credits ?? 1` while the GPA math treated 0/negative as 1. New effectiveCredits() export in lib/transcript.ts is now the ONE place the rule lives, used by both weightedGpa and the "N cr" display; tests cover 0/negative/null and the rendered "1 cr" for a zero-credit row. 3 — ?semester= is read at FETCH time: the mount-frozen useState initializer became currentSemesterParam(), read inside refresh AND inside every mutation callback (keeps the no-Suspense plain-location pattern). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ing contract (ToastProvider stub, getGpa mock, summary gpa/semester fields) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/components/screens/Gradebook/Landing.tsx (1)
159-184: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGuard against stale
getGradebookSummaryresponses on rapid term switches.Switching
selectedquickly fires overlapping requests with no sequencing; a slower response for an earlier term can resolve after a newer one and overwritecourses/termGpawith stale data for the wrong term. This affects the newly-addedtermGpadisplay as well.♻️ Proposed fix: ignore stale responses
React.useEffect(() => { if (!termsReady) return; + let cancelled = false; if (!selected) { setCourses([]); setTermGpa(null); setLoading(false); return; } if (!userId) { setCourses(SAMPLE_COURSES[selected] ?? []); setTermGpa(null); setLoading(false); return; } setLoading(true); getGradebookSummary(userId, selected) .then((res) => { + if (cancelled) return; setCourses(res.courses.length ? res.courses : []); setTermGpa(res.gpa ?? null); }) .catch(() => { + if (cancelled) return; setCourses([]); setTermGpa(null); }) - .finally(() => setLoading(false));+ .finally(() => { if (!cancelled) setLoading(false); });+ return () => { cancelled = true; }; }, [userId, selected, termsReady]);🤖 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 `@frontend/src/components/screens/Gradebook/Landing.tsx` around lines 159 - 184, Update the useEffect that calls getGradebookSummary to ignore responses from requests started for an earlier selected term. Track the active request or selected-term identity and only apply courses, termGpa, error, and loading updates when the response belongs to the current request; preserve the existing reset, sample-course, and cleanup behavior.
🤖 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 `@frontend/src/components/screens/Gradebook/Course.tsx`:
- Around line 98-109: Make the semester selection reactive in
GradebookCourseScreen instead of relying only on currentSemesterParam() at call
time: derive it from a reactive URL/search-parameter source, use that same value
for reads and course-keyed mutations, and include it in the relevant fetch/mount
dependencies. When the semester changes for the same courseId, clear or reload
the existing course state so stale term data is not displayed.
---
Nitpick comments:
In `@frontend/src/components/screens/Gradebook/Landing.tsx`:
- Around line 159-184: Update the useEffect that calls getGradebookSummary to
ignore responses from requests started for an earlier selected term. Track the
active request or selected-term identity and only apply courses, termGpa, error,
and loading updates when the response belongs to the current request; preserve
the existing reset, sample-course, and cleanup behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d910b0c7-0c93-4b8c-b3cc-b86db7f13777
📒 Files selected for processing (19)
docs/frontend-testids.mdfrontend/e2e/gradebook.spec.tsfrontend/eslint-suppressions.jsonfrontend/eslint.config.mjsfrontend/src/components/Gradebook/AssignmentList.tsxfrontend/src/components/Gradebook/AssignmentModal.tsxfrontend/src/components/Gradebook/CourseCard.tsxfrontend/src/components/Gradebook/TranscriptModal.test.tsxfrontend/src/components/Gradebook/TranscriptModal.tsxfrontend/src/components/screens/Gradebook/Course.semester.test.tsxfrontend/src/components/screens/Gradebook/Course.tsxfrontend/src/components/screens/Gradebook/Landing.test.tsxfrontend/src/components/screens/Gradebook/Landing.testmode.test.tsxfrontend/src/components/screens/Gradebook/Landing.tsxfrontend/src/lib/api.tsfrontend/src/lib/semesters.tsfrontend/src/lib/transcript.test.tsfrontend/src/lib/transcript.tsfrontend/src/lib/types.ts
Comment on lines
+98
to
+109
| // /gradebook/<id>?semester=<label> — the landing's cards carry the selected | ||
| // term so a course enrolled in several terms resolves to that term's | ||
| // enrollment instead of 404ing off the current one (#139). Read straight off | ||
| // location (useSearchParams() would need a Suspense boundary in the route | ||
| // shell, which this screen doesn't own), and read it at CALL time — every | ||
| // fetch AND every course-keyed mutation below must see the URL the user is | ||
| // actually on, not a mount-frozen copy. Writes that drop the term would | ||
| // silently land on the CURRENT term's enrollment (#468 review). | ||
| function currentSemesterParam(): string | undefined { | ||
| if (typeof window === "undefined") return undefined; | ||
| return new URLSearchParams(window.location.search).get("semester") || undefined; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"Locate file"
git ls-files | rg 'frontend/src/components/screens/Gradebook/Course\.tsx$|Course\.tsx$'||trueechoecho"Outline"
ast-grep outline frontend/src/components/screens/Gradebook/Course.tsx --view compact ||trueechoecho"Relevant sections"
sed -n '1,320p' frontend/src/components/screens/Gradebook/Course.tsx
echo"---"
sed -n '430,720p' frontend/src/components/screens/Gradebook/Course.tsx
echoecho"Search for currentSemesterParam and refresh dependencies"
rg -n "currentSemesterParam|useEffect|currentSemester|semester|CourseEnrollment|student|record|mutation|useMutation|refresh" frontend/src/components/screens/Gradebook/Course.tsxRepository: SaplingLearn/Sapling
Length of output: 25705
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"Inspect API helpers and mutation signatures"
fd -a 'api\.(ts|tsx|js)$|types\.(ts|tsx|js)$' frontend/src | sed 's#^./##'echo
rg -n "export async function getGradebookCourse|const getGradebookCourse|function getGradebookCourse|getGradebookCourse" frontend/src/lib frontend/src -g '*.ts' -g '*.tsx'echo
sed -n '1,220p' frontend/src/lib/api.ts
echo"---"
rg -n "createGradedAssignment|updateGradedAssignment|deleteGradedAssignment|bulkUpdateCategories|setLetterScale|setCurveSettings" frontend/src/lib/api.tsRepository: SaplingLearn/Sapling
Length of output: 9818
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"API route/path parameters"
sed -n '1160,1300p' frontend/src/lib/api.ts
echoecho"GradebookCourseScreen effects/mutations around line 220-245"
sed -n '220,245p' frontend/src/components/screens/Gradebook/Course.tsx
echoecho"Semester-specific tests"
sed -n '1,180p' frontend/src/components/screens/Gradebook/Course.semester.test.tsxRepository: SaplingLearn/Sapling
Length of output: 13171
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"Find gradebook route files"
git ls-files | rg 'frontend/src/(app|routes|pages).*(gradebook|gradebook/.*course|gradebook|Course.page|page\.(tsx|ts|js))'| head -80
echoecho"Search route declarations for /gradebook/[id]"
rg -n 'gradebook/.*/course|gradebook/\[|gradebook/\\\[|courseId|GradebookCourseScreen|semester=' frontend/src -g '*.tsx' -g '*.ts'echo
git ls-files 'frontend/src/app/**/page.tsx''frontend/src/pages/**'| xargs rg -n "courseId|GradebookCourseScreen|semester="||trueRepository: SaplingLearn/Sapling
Length of output: 24931
Make the semester param a reactive dependency before relying on it.
For the same course ID, navigating /gradebook/c1?semester=Fall+2025 to /gradebook/c1?semester=Spring+2026 reuses GradebookCourseScreen because query changes don’t trigger the existing courseId-only fetch/mount effect. That leaves Fall grade state on screen while course-keyed mutations send Spring to the backend. Derive the selected term reactively (read and write with the same value) and reload/clear course data when it changes.
🤖 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 `@frontend/src/components/screens/Gradebook/Course.tsx` around lines 98 - 109,
Make the semester selection reactive in GradebookCourseScreen instead of relying
only on currentSemesterParam() at call time: derive it from a reactive
URL/search-parameter source, use that same value for reads and course-keyed
mutations, and include it in the relevant fetch/mount dependencies. When the
semester changes for the same courseId, clear or reload the existing course
state so stale term data is not displayed.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Bundle B6 — Semesters completion (#139)
Closes#139.
The issue body predates the terms/offerings redesign; built against shipped reality (epic #142's model is stale). The Landing term switcher already existed (#140); what this PR adds:
The bug fix (term switcher half)
?semester=<label>andgetGradebookCoursepasses it through — fixing a live 404: with multiple offerings of one course (seeded CS101 is in fall-2025 AND spring-2026) and no term hint,_resolve_enrollmentresolves the current term and 404s ("Course not in your gradebook") from either chip.Course.tsxreads the param exactly like Landing's deep-link handling.Transcript / GPA half
GradebookSummarystops discarding thegpa/semesterfields the backend already returns; Landing shows "Term GPA" (gradebook-term-gpa).TranscriptModal(gradebook-transcript-open) consuming the existing-but-unconsumedGET /api/gradebook/gpa: cumulative GPA + per-semester sections with course rows (letter or "in progress", credits). Per-semester GPA computed in a purelib/transcript.tsthat mirrorsgradebook_service.weighted_gpaexactly (lock-step comment; null grade_points listed-but-excluded, null credits → 1).Surface registration + journey
docs/frontend-testids.mdgains thegradebooksurface; the four owning files joined the eslint testid enforcement block;eslint-suppressions.jsonregenerated for the pre-existing untagged elements (delta: Course.tsx 9, Landing.tsx 1).e2e/gradebook.spec.ts(first gradebook journey): (a) the multi-term regression with a DB-truth precondition viaqueryRaw(asserts the two CS101 enrollments' term ids), Fall→"Exams" / Spring→"Projects" discriminators from the rich seed; (b) transcript open → GPA + both term sections.Gates
1102093(feat(observability): instrument the capture seams — events flow end to end (#117) #465).vitest: 39 files / 305 tests green;tsc --noEmitclean;eslint .0 errors.Note: sibling issue #141 got a corrected-state STOP-note instead of code (design conflict with the shipped #360 contract + file collision with #465/B5) — see the issue comment.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes