Uh oh!
There was an error while loading. Please reload this page.
fix(study): keep the exam selected when opening a recent guide (#476) - #499
Conversation
Opening a guide from the "Recent guides" rail left Regenerate permanently
disabled. The cause is not the open path — openRecent sets courseId AND
examId together. It's the courseId-keyed exams effect, which opened with an
unconditional setExamId(""): a scope reset that cannot tell "the user
switched course" (selection now invalid) from "we just opened a specific
guide" (selection deliberate and valid).
Both effects run in the same commit, so the loader still saw the intact pair
and the guide loaded; only the NEXT render lost the exam. Hence the symptom —
a guide on screen above a dead Regenerate button — rather than "nothing
opens". It needs a course CHANGE to reproduce, which is why a rail entry for
the already-selected course always worked (pinned as a control test).
The reset now happens at the two events that mean it: the course picker's
onChange, and a term switch. The term case adjusts state during render (the
StudyModePanel pattern already in this file) rather than in an effect,
because an effect-time reset lands a render late — the loader would commit
one read of the old exam under the new term first. That was the same defect's
second trigger, and it now has a test.
Making Regenerate reachable on the rail path exposed a term hazard: it sent
the ACTIVE selector's term, while a recent entry opens under its OWN term
(#475 F1). Regenerating a Fall guide as Spring would rebuild against an
offering the displayed guide never came from. Regenerate now replays the term
the displayed guide was loaded with.
Also seeds a CACHED study guide in the rich local dataset so the e2e journey
can open the rail without generating (the study_guide agent has no
function-mode handler).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reached
Next review available in:43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR seeds a cached CS101 midterm guide and updates Study to preserve the guide’s course, exam, and semester context. Course and semester changes clear stale state. Unit and Playwright tests cover recent-guide navigation and regeneration. ChangesStudy guide context
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 1778215 | Commit Preview URL Branch Preview URL | Jul 31 2026, 04:18 PM |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/screens/Study.tsx (1)
288-329: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
loadGuideandregenerateagainst stale async responses.Neither
loadGuidenorregeneratechecks whether the selection is still current when their request resolves. If a user switches term or course while agetStudyGuide/regenerateStudyGuidecall is in flight,clearSelection()clearsexamId/guideduring render, but the pending.thenstill runssetGuide(r.guide)andsetLoadedTerm(...)unconditionally afterward. That resurrects a guide (and its term) the user just navigated away from, under a course/term combination that no longer matches the pickers. This directly undercuts the invariant this PR adds ("switching term drops the selection instead of reloading it").Add a staleness check before applying the response, for example by comparing the captured
cid/eidagainst refs updated whenevercourseId/examIdchange, and bailing out if they no longer match.🔧 Proposed staleness guard
+ const courseIdRef = React.useRef(courseId);+ const examIdRef = React.useRef(examId);+ React.useEffect(() => { courseIdRef.current = courseId; }, [courseId]);+ React.useEffect(() => { examIdRef.current = examId; }, [examId]);+ const loadGuide = React.useCallback(async (cid: string, eid: string, termOverride?: string) => { if (!userId) return; const term = termOverride !== undefined ? (termOverride || undefined) : (semester || undefined); setLoadingGuide(true); setGuideProblem(null); try { const r = await getStudyGuide(userId, cid, eid, term); + // Bail if the selection changed while the request was in flight — a+ // cleared or reselected exam should not be resurrected by a late response.+ if (cid !== courseIdRef.current || eid !== examIdRef.current) return; setGuide(r.guide);Apply the same
cid/eidcheck inregeneratebeforesetGuide(r.guide).Also applies to: 351-362
🤖 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/Study.tsx` around lines 288 - 329, Guard the async response handling in loadGuide and regenerate with the current courseId/examId selection, using refs updated whenever those values change. Before applying guide data or related state such as loadedTerm, return without updates when the captured cid/eid no longer match the current selection, while preserving normal updates for the active request.
🧹 Nitpick comments (1)
frontend/e2e/study-recent-guides.spec.ts (1)
22-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a stable testid for
CustomSelecttriggers.
CustomSelecthas nodata-testidprop and its trigger button is selector-only, so this regression journey relies onaria-haspopup="listbox". Use a documented route, such as givingCustomSelecta testid prop and wiring the Study course/exam pickers into it, before relying on ARIA markup here.🤖 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/e2e/study-recent-guides.spec.ts` around lines 22 - 27, Add a documented testid prop to the CustomSelect trigger and pass distinct stable test IDs to the Study course and exam pickers. Update the examPicker locator in the regression test to use the exam picker test ID instead of relying on aria-haspopup and DOM order.Source: Path instructions
🤖 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 `@backend/db/seed_local_rich.py`:
- Around line 585-620: Update seed_study_guides to encrypt each guide’s content
before passing it to h.insert_if_absent, using the existing encrypt_json or
encrypt_if_present helper and matching the encryption behavior in the
study_guide route. Keep the stored study_guides row structure and
cross-references unchanged.
---
Outside diff comments:
In `@frontend/src/components/screens/Study.tsx`:
- Around line 288-329: Guard the async response handling in loadGuide and
regenerate with the current courseId/examId selection, using refs updated
whenever those values change. Before applying guide data or related state such
as loadedTerm, return without updates when the captured cid/eid no longer match
the current selection, while preserving normal updates for the active request.
---
Nitpick comments:
In `@frontend/e2e/study-recent-guides.spec.ts`:
- Around line 22-27: Add a documented testid prop to the CustomSelect trigger
and pass distinct stable test IDs to the Study course and exam pickers. Update
the examPicker locator in the regression test to use the exam picker test ID
instead of relying on aria-haspopup and DOM order.
🪄 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: 24187e41-dbab-4240-a235-8efe5889d8af
📒 Files selected for processing (5)
backend/db/seed_local_rich.pyfrontend/e2e/study-recent-guides.spec.tsfrontend/src/components/screens/Study.recentGuides.test.tsxfrontend/src/components/screens/Study.semester.test.tsxfrontend/src/components/screens/Study.tsx
| _STUDY_GUIDES = [ | ||
| ("rich-guide-cs-f25-mid", USER_ACTIVE, OFF_CS_F25, "rich-asg-cs-f25-mid", | ||
| "2026-03-01T12:00:00Z", | ||
| { | ||
| "exam": "Midterm Exam", | ||
| "due_date": "2025-10-15", | ||
| "overview": "Covers variables, control flow, and functions.", | ||
| "topics": [ | ||
| { | ||
| "name": "Variables", | ||
| "importance": "Every later topic builds on binding names to values.", | ||
| "concepts": ["Assignment", "Scope"], | ||
| }, | ||
| { | ||
| "name": "Recursion", | ||
| "importance": "The midterm's hardest questions are recursive traces.", | ||
| "concepts": ["Base case", "Call stack"], | ||
| }, | ||
| ], | ||
| }), | ||
| ] | ||
| def seed_study_guides() -> None: | ||
| for guide_id, user_id, off_id, exam_id, generated_at, content in _STUDY_GUIDES: | ||
| h.insert_if_absent( | ||
| "study_guides", | ||
| guide_id, | ||
| { | ||
| "user_id": user_id, | ||
| "offering_id": off_id, | ||
| "exam_id": exam_id, | ||
| "generated_at": generated_at, | ||
| "content": content, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"--- assignment id/title match ---"
rg -n "rich-asg-cs-f25-mid" backend/db/seed_local_rich.py
rg -n "_ASSIGNMENTS = \[" -A 40 backend/db/seed_local_rich.py | rg -n "rich-asg-cs-f25-mid|Midterm"echo"--- encryption precedent ---"
rg -n "encrypt_if_present" backend/db/seed_local_rich.py
rg -n "study_guides" backend -g '*.py' -g '!backend/db/seed_local_rich.py'Repository: SaplingLearn/Sapling
Length of output: 2909
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo"--- seed_local_rich study_guide section ---"
sed -n '560,635p' backend/db/seed_local_rich.py
echoecho"--- assignments section around exam id ---"
sed -n '340,385p' backend/db/seed_local_rich.py
echoecho"--- generate/insert study guide path ---"
sed -n '160,220p' backend/routes/study_guide.py
sed -n '300,370p' backend/routes/study_guide.py
echoecho"--- frontend e2e references ---"if [ -f frontend/e2e/study-recent-guides.spec.ts ];then
rg -n "Midterm Exam|study guide|guides|rich-guide-cs-f25-mid|Midterm" frontend/e2e/study-recent-guides.spec.ts
elseecho"frontend/e2e/study-recent-guides.spec.ts not found"fiechoecho"--- schema/table model for study_guides if present ---"
rg -n "study_guides|content.*study" backend/db backend/services backend/tests backend/routes -g '*.py'| head -200Repository: SaplingLearn/Sapling
Length of output: 13088
Encrypt the generated study_guides.content before inserting it.
"rich-asg-cs-f25-mid" exists and matches "Midterm Exam" in _ASSIGNMENTS, so the cross-reference is correct. For writes, backend/routes/study_guide.py inserts content = result.output.model_dump() directly into study_guides; route that value through encrypt_json/encrypt_if_present before table("study_guides").insert(row).
🤖 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 `@backend/db/seed_local_rich.py` around lines 585 - 620, Update
seed_study_guides to encrypt each guide’s content before passing it to
h.insert_if_absent, using the existing encrypt_json or encrypt_if_present helper
and matching the encryption behavior in the study_guide route. Keep the stored
study_guides row structure and cross-references unchanged.
Source: Coding guidelines
Code review caught a regression this PR introduced. Moving the reset out of the courseId-keyed effect and onto the picker's onChange dropped a guard the effect had for free: setCourseId(sameValue) bails out, so the effect never re-ran. CustomSelect.commit() fires onChange for the already-selected option too, so re-confirming the course you were already on wiped the guide you were reading — the same class of bug as #476 itself. Guard selectCourse on an actual change, and pin it with a test. Also records the known cross-term edge the fix leaves standing: the exam OPTIONS follow the active selector by #475's design, so a rail entry opened under a different term is absent from that list and the picker shows its placeholder while the guide and Regenerate are live and correctly aimed at loadedTerm. Squaring it means tracking "the term I'm viewing" across list and loads, which is a #475 change rather than part of this fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AndresL230
commented
Jul 31, 2026
Code reviewFound 1 blocking issue, now fixed in 1778215, plus two notes recorded rather than fixed.
Moving the reset out of the Sapling/frontend/src/components/screens/Study.tsx Lines 252 to 256 in 065b325 Fixed by guarding on an actual change, with a regression test ( Sapling/frontend/src/components/screens/Study.tsx Lines 252 to 262 in 1778215 Notes, not blocking:
Sapling/frontend/src/components/screens/Study.tsx Lines 275 to 290 in 1778215
Also checked and cleared: a review pass flagged a captured Playwright failure of this PR's own journey as evidence the fix doesn't work. That artifact is from a deliberate pre-fix run — 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Part of #476.
The issue body misattributes the cause
It doesn't.
openRecent(Study.tsx) sets both ids:The clearing comes from the courseId-keyed exams effect, whose first
statement was an unconditional
setExamId(""). That reset cannot tell"the user switched course" (selection now invalid) from "we just opened a
specific guide" (selection deliberate and valid) — it only sees that
courseIddiffers from its last value, whichopenRecentalso causes.Both effects run in the same commit, so the loader still saw the intact
pair and the guide loaded; only the next render lost the exam. That is why
the symptom is "a guide on screen above a dead Regenerate button" rather than
"nothing opens" — and why
disabled={… || !examId}pins the button off.Discriminator (pinned as a control test): it needs a course change to
reproduce. A rail entry whose course is already selected never trips the
effect and worked fine on main. That test passed before the fix and still
passes after — it's what proves the mechanism is the reset, not the open path.
The fix
The reset now happens at the two events that actually mean it:
onChange— the event that genuinely invalidates the exam.old term need not exist in the new one.
The term case adjusts state during render (the
StudyModePanelpatternalready in this file) rather than in an effect. That matters: an effect-time
reset lands a render late, so the loader commits one read of the old exam
under the new term first. That was the same defect's second trigger — it
reproduced on main and now has a test.
The exams effect is now purely a fetch.
One consequence worth flagging
Making Regenerate reachable on the rail path exposed a term hazard.
regeneratesent the active selector's term, while a recent entry opensunder its own term (#475 F1). On main that was unreachable — the button
was dead on the one path where the two differ — so it was latent. Enabling
the button would have made it live: regenerating a Fall guide while the
selector says Spring rebuilds against an offering the displayed guide never
came from, which is exactly the mismatch #475 F1 fixed for reads.
Regenerate now replays the term the displayed guide was loaded with.
Tests
Study.recentGuides.test.tsx(new, 7 cases) — written red first:4 failed on the
Regenerate/picker assertions, the term-switch case failedon the stale re-read, and the same-course control passed throughout.
Study.semester.test.tsx— updated a comment that documented the bug aspermanent ("Regenerate isn't drivable from the recent-guides rail").
e2e/study-recent-guides.spec.ts(new) — promoted regression journey.db/seed_local_rich.py— seeds a cached guide so the journey can openthe rail without generating; the
study_guideagent has no function-modehandler, and the guide GET returns the cache hit before reaching it. The
journey asserts Regenerate is enabled and deliberately never clicks it.
Gates
tsc --noEmitclean;eslint src0 errors;vitest run422/422.pytest1529 passed / 32 skipped;ruff checkclean.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes