Uh oh!
There was an error while loading. Please reload this page.
feat(study): semester-scope the study-tool reads (#141 reframe) - #475
Conversation
The approved reframe: NO Archive toggle. The existing semester selector (Courses & Semesters hub -> lib/useActiveSemester, "" = All semesters DEFAULT — untouched, e2e-pinned per #360) now scopes the STUDY-TOOL reads the same way it already scopes the graph. The study endpoints used to hardcode current-term resolution (resolve_offering(course_id)), so under the frozen e2e clock (spring-2026 current) fall-2025 study material was unreachable regardless of the user's selection. Backend — optional `semester` (term LABEL via term_id_for_label) on the course-scoped READ paths, resolved STRICTLY: an unknown label or a term with no offering of the course degrades to each route's empty/404 behavior, never a silent fall-back to another term. New `fallback=False` mode on academics.resolve_offering carries that rule (the default create=False path used to silently resolve ANY offering of the course on a term miss). - study_guide: GET /{user}/guide + POST /regenerate (404 on a term miss, and never generates for an offering that isn't there); GET /{user}/exams scopes the enrollment set to the selected term. - flashcards: GET /user/{user} filters cards to the selected term's offerings (term-LESS cards stay visible under any selection); POST /generate grounds its docs context in the selected term's offering (a term miss contributes no docs — not all-docs, not current term). import/commit stays a CREATE path: current term by design. - notes: GET /user/{user} course-filtered read takes `semester` (API completeness — the notetaker UI carries no semester context and is deliberately NOT wired); the create/re-home paths stay current-term by design (commented). - quiz: untouched (no term resolution; scoping is client-side). Frontend — Study.tsx threads `activeSemester || undefined` into getStudyGuideExams/getStudyGuide/regenerateStudyGuide/getFlashcards/ generateFlashcards, gated on the useActiveSemester hydrated flag exactly like Dashboard (call-count pinned: one scoped fetch, never unscoped-then-scoped). Tests: backend route + resolver coverage for (a) no semester = existing current-term behavior, (b) explicit term threads (course, term, fallback=False), (c) unknown/no-offering term = empty/404 not 500; vitest Study.semester.test.tsx pins the scoped/unscoped fetch args and the single-fetch hydration gate. New journey e2e/study-semester.spec.ts (authored, not run here): All semesters shows the fall AND spring decks together; picking Fall 2025 in the hub surfaces the fall-2025 CS Basics deck and hides the spring-only one. No new agent tasks introduced; the journey triggers no generation. Co-Authored-By: Claude Fable 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:33 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 (9)
📝 WalkthroughWalkthroughChangesSemester scoping
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SemesterState
participant Study
participant ApiHelpers
participant BackendRoutes
participant OfferingResolver
SemesterState-->>Study: hydrated semester selection
Study->>ApiHelpers: request study data with semester
ApiHelpers->>BackendRoutes: send semester query or payload
BackendRoutes->>OfferingResolver: resolve course offering strictly
OfferingResolver-->>BackendRoutes: offering_id or no match
BackendRoutes-->>ApiHelpers: scoped data or empty/404 response
ApiHelpers-->>Study: render term-scoped study content
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 | 07ad94e | Commit Preview URL Branch Preview URL | Jul 30 2026, 05:09 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)
backend/routes/flashcards.py (1)
112-155: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSemester scope is dropped when the course name doesn't match any course.
The function's own docstring promises a term miss "never falls back... to all of the user's material," but that guarantee only holds inside the
if course_rows:branch. Whencourse_rowsis empty (no course row matchescourse_name), theelsebranch unconditionally returns every one of the user's documents across all terms — even whensemesterwas explicitly supplied. A semester-scoped generation request for a topic that doesn't exactly match a stored course name will silently leak material from every other term.🐛 Proposed fix
else: - docs = table("documents").select(- "file_name,category,summary,concept_notes",- filters={"user_id": f"eq.{user_id}", "deleted_at": "is.null"},- )+ if semester:+ # No course match at all — nothing to strictly scope to, so a+ # semester filter degrades to "no documents" instead of+ # leaking material from every term (`#141`).+ docs = []+ else:+ docs = table("documents").select(+ "file_name,category,summary,concept_notes",+ filters={"user_id": f"eq.{user_id}", "deleted_at": "is.null"},+ )Worth adding a regression test alongside
test_semester_with_no_offering_yields_no_docs_not_all_docsfor this "no course row at all" case inbackend/tests/test_flashcards_routes.py.🤖 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/routes/flashcards.py` around lines 112 - 155, Update _get_course_documents so an explicitly supplied semester never falls back to all user documents when course_rows is empty; return no documents for that scoped miss, while preserving the existing all-documents fallback only when semester is absent. Add a regression test in test_flashcards_routes.py alongside test_semester_with_no_offering_yields_no_docs_not_all_docs for a course name with no matching row.
🧹 Nitpick comments (2)
frontend/e2e/study-semester.spec.ts (1)
31-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a database assertion for the seeded semester contract.
The UI assertions alone cannot distinguish correct term filtering from incorrect seeded data. Assert the Fall/current offering-card setup through
support/db.tsas well. As per coding guidelines, “E2E journeys should use the fixtures-basedtestfromsupport/fixtures.ts, database assertions fromsupport/db.ts.”🤖 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-semester.spec.ts` around lines 31 - 53, Add database assertions to the semester-selection test using the fixtures-based test and helpers from support/db.ts, verifying the seeded Fall 2025/current offering-card setup before the UI flow runs. Keep the existing UI assertions and ensure the database checks confirm the expected Fall deck/cards and semester relationship.Source: Coding guidelines
backend/routes/flashcards.py (1)
286-305: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnscoped
course_offeringsquery for the term filter.The
course_offeringslookup fetches every offering in the target term platform-wide, rather than just the offering ids actually referenced by this user's already-fetchedrows. Scoping the filter to the offering ids present inrows(via anin.(...)filter) would keep this query bounded by the user's own data instead of the term's total size.♻️ Proposed refactor
if semester: term_id = term_id_for_label(semester) allowed: set[str] = set() if term_id: + offering_ids = {r["offering_id"] for r in rows if r.get("offering_id")}+ if offering_ids:+ offs = table("course_offerings").select(+ "id",+ filters={+ "term_id": f"eq.{term_id}",+ "id": f"in.({','.join(offering_ids)})",+ },+ ) or []+ allowed = {o["id"] for o in offs if o.get("id")}- offs = table("course_offerings").select(- "id", filters={"term_id": f"eq.{term_id}"}- ) or []- allowed = {o["id"] for o in offs if o.get("id")}🤖 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/routes/flashcards.py` around lines 286 - 305, Scope the `course_offerings` lookup in the semester-filtering block to offering IDs referenced by the user’s existing `rows`, using an `in.(...)` filter alongside the term filter. Build the ID set from non-null `rows` offering IDs and preserve the current term-less-card visibility and unknown-term 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/Study.semester.test.tsx`:
- Around line 132-148: Extend the Study screen tests around the regeneration
flow to cover the changed regenerateStudyGuide call. Set up selectable course
and exam state, click Regenerate, and assert the call includes "Fall 2025" when
the active semester is stored and undefined when it is absent, while preserving
the existing fetch assertions.
---
Outside diff comments:
In `@backend/routes/flashcards.py`:
- Around line 112-155: Update _get_course_documents so an explicitly supplied
semester never falls back to all user documents when course_rows is empty;
return no documents for that scoped miss, while preserving the existing
all-documents fallback only when semester is absent. Add a regression test in
test_flashcards_routes.py alongside
test_semester_with_no_offering_yields_no_docs_not_all_docs for a course name
with no matching row.
---
Nitpick comments:
In `@backend/routes/flashcards.py`:
- Around line 286-305: Scope the `course_offerings` lookup in the
semester-filtering block to offering IDs referenced by the user’s existing
`rows`, using an `in.(...)` filter alongside the term filter. Build the ID set
from non-null `rows` offering IDs and preserve the current term-less-card
visibility and unknown-term behavior.
In `@frontend/e2e/study-semester.spec.ts`:
- Around line 31-53: Add database assertions to the semester-selection test
using the fixtures-based test and helpers from support/db.ts, verifying the
seeded Fall 2025/current offering-card setup before the UI flow runs. Keep the
existing UI assertions and ensure the database checks confirm the expected Fall
deck/cards and semester relationship.
🪄 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: 7d7668eb-80c9-470e-98fb-b67a38b5c543
📒 Files selected for processing (13)
backend/routes/flashcards.pybackend/routes/notes.pybackend/routes/study_guide.pybackend/services/academics.pybackend/tests/test_academics.pybackend/tests/test_flashcards_routes.pybackend/tests/test_notes_routes.pybackend/tests/test_study_guide_routes.pyfrontend/e2e/study-semester.spec.tsfrontend/src/components/screens/Study.semester.test.tsxfrontend/src/components/screens/Study.test.tsxfrontend/src/components/screens/Study.tsxfrontend/src/lib/api.ts
Uh oh!
There was an error while loading. Please reload this page.
AndresL230
commented
Jul 30, 2026
Code reviewFound 1 issue:
Sapling/frontend/src/components/screens/Study.tsx Lines 239 to 271 in e6dfb0c Sub-threshold notes (verified, all being fixed in a follow-up commit, scores <80): 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…view findings F1 (major): the recent-guides rail lists guides from ANY term, but opening one resolved with the ACTIVE semester under #141's strict mode — a multi-term course + other-term entry cache-missed on (offering, exam) and silently generated-and-PERSISTED a mismatched row; a course absent from the active term 404'd for a guide visibly in the sidebar. Invariant now: a recent entry opens AS ITS OWN TERM. - backend GET /{user}/cached: each entry carries its own `semester` label (term_for_offering, lru-cached, offering ids deduped in the existing enrichment loop); ETag key bumped to guides.v2 so bodies cached under the old shape revalidate. - frontend: StudyGuideCacheEntry.semester; openRecent records the entry's term ("" = term-less entry -> explicitly unscoped) in a ref consumed by exactly one load; loadGuide takes a per-load term override; the failed state stores the term so retry replays the exact load. Picker-driven loads keep following the active selector (unit-tested by driving the real CustomSelects). The #476 emergent examId-clear behavior is untouched and its tests stay green. F3: _generate_and_insert scopes the exam lookup to the RESOLVED offering's enrollment (the #462 CodeRabbit fix) — a two-term user can no longer generate a guide keyed on one term's offering from another term's exam (regression test: 404, nothing persisted). F2: flashcards _get_course_documents — a course-name miss WITH an explicit semester now contributes no documents (an explicit term gives the all-docs fallback nothing to anchor to); without a semester the pre-existing all-docs fallback is byte-identical (both pinned). F4: the guide 404 branch no longer hardcodes the exam-deleted copy — the server's detail renders when it isn't the exam-deleted sentence (no-offering-in-term case), still guidance, never a toast. F5: comment truth fixes — notes PATCH re-home relabeled (re-home, not create; still deliberately current-term); api.ts study-guide block now documents the pre-existing exams-list asymmetry (omitted semester = current-term resolution for guide/regenerate but ALL terms for getStudyGuideExams); Study.semester.test.tsx attributes the exam-clear to the courseId-keyed effect racing openRecent (#476), not to openRecent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pill share the name under All semesters) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
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>
…#499) * fix(study): keep the exam selected when opening a recent guide (#476) 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> * fix(study): only reset the exam when the course actually changes 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> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
#141, reframed (approved by Andres on 2026-07-30)
Closes#141.
No Archive toggle. The existing semester selector (All-semesters default untouched — the #360 e2e-vetoed contract) now scopes the study-tool read paths the way it already scopes the graph. Full analysis + reframe rationale: the corrected-state comment on #141.
Backend — optional
semester(term label viaacademics.term_id_for_label), strict resolutionservices/academics.py:resolve_offering(..., fallback: bool = True)— new strict mode. The existingcreate=Falsepath silently falls back to any offering of the course; with an explicitsemesterthat would have silently served another term's content.fallback=FalsereturnsNoneon a term miss. Additive, default-preserving; resolver tests pin both modes.routes/study_guide.py:GET /{user}/guide?semester=,POST /regenerate(body),GET /{user}/exams?semester=— term miss → 404 (guide/regenerate) with the agent never invoked, exams filtered viaterm_for_offering.routes/flashcards.py:GET /user/{user}?semester=— cards filtered to the term's offerings; term-less cards (offering_id NULL) stay visible under any selection; unknown label degrades to term-less-only, never 500.POST /generategrounds its docs context in the selected term's offering.import/commitstays current-term (comment).routes/notes.py: course-filtered read takessemester; term miss → empty list. Create/re-home stay current-term by design (comments).routes/quiz.py: untouched — no term resolution exists; quiz scoping is already client-side via the graph picker.Frontend
lib/api.ts: optionalsemesterongetStudyGuideExams/getStudyGuide/regenerateStudyGuide/getFlashcards/generateFlashcards.Study.tsx:useActiveSemester()threaded into both modes, fetches gated on the hydrated flag (Dashboard pattern, call-count-pinned — no unscoped-then-scoped double fetch). Notetaker deliberately not wired (no semester context on that screen; the notes param is API-completeness).e2e/study-semester.spec.ts: All-semesters default shows the fall + spring decks together; hub → "Fall 2025" → only the fall deck serves. No generation triggered (function-mode-seam safe); no new interactive elements → no surface registration needed.Gates
Backend
pytest tests/ -q→ 1499 passed, 32 skipped;ruff check .clean. Frontend vitest 47 files / 353 tests;tsc --noEmitclean;eslint .0 errors. Based on main @9edfcf5. Pre-merge flock'd e2e cycle to follow.Note for a follow-up issue
Found pre-existing (not fixed here): opening a guide from the "Recent guides" rail clears the exam selection, leaving Regenerate permanently disabled on that path.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests