Course ID schema migration + calendar assignment normalization#53
Merged
Jose-Gael-Cruz-Lopez merged 14 commits intoApr 15, 2026
Merged
Conversation
This is a major schema refactoring to support canonical courses: Backend Changes: - course_context_service.py: Complete rewrite to use new course_concept_stats and course_summary tables with Gemini-powered summary generation - graph_service.py: Updated to join user_courses->courses, use course_id FK - routes/graph.py: Endpoints now accept course_id instead of course_name - routes/learn.py: Sessions store course_id, pass through to graph updates - routes/calendar.py: Assignments use course_id, join with courses for display - calendar_service.py: Updated to use course_id instead of course_name - models/__init__.py: Added course_id fields to Pydantic models Frontend Changes: - api.ts: Updated all API functions to use course_id instead of course_name - getCourses returns enriched data: course_code, course_name, school, dept - addCourse, updateCourseColor, deleteCourse all use course_id - startSession accepts optional course_id parameter - saveAssignments uses course_id Database Schema Migration (already applied in Supabase): - courses table is now canonical (no user_id) - user_courses is the enrollment join table - All dependent tables use course_id FK: assignments, sessions, graph_nodes, documents, study_guides - New tables: course_concept_stats, course_summary - Removed: course_context table
…mes" This reverts commit 1ba4f7549504c64ff6358504fc9fdf38f1620888.
Made-with: Cursor
…ests with course_id Made-with: Cursor
…malization - Add course_code to Assignment type, normalizeAssignments, syllabus extract mapping - Dashboard/library: use EnrolledCourse and course_id for color/delete flows - Learn: pass course_id slot to startSession before shared-context flag - AssignmentTable: default row includes course_code; optional assignment_type for select Made-with: Cursor
This was referenced Apr 14, 2026
Deploying web with
|
| Latest commit: |
f23816c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://38228d2e.web-75h.pages.dev |
| Branch Preview URL: | https://combined-course-id-migration.web-75h.pages.dev |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrate canonical course identity from freeform Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Frontend
participant LearnRoute as Backend /learn
participant GraphSvc as graph_service.apply_graph_update
participant DB as Supabase
participant CourseCtx as course_context_service
participant Gemini as External AI
Client->>Frontend: beginSession(topic, mode, optional course_id)
Frontend->>LearnRoute: POST /start-session (topic, mode, course_id?)
LearnRoute->>DB: insert/select session (include course_id)
LearnRoute->>GraphSvc: apply_graph_update(user_id, graph_update, course_id)
GraphSvc->>DB: upsert graph_nodes with course_id
GraphSvc->>CourseCtx: update_course_context(course_id)
CourseCtx->>DB: aggregate graph_nodes by course_id -> upsert course_concept_stats/course_summary
alt summary changed
CourseCtx->>Gemini: request summary_text
Gemini-->>CourseCtx: summary_text
CourseCtx->>DB: store summary_text and summary_hash
end
LearnRoute-->>Frontend: 200 OK (session_id, course_id)
Frontend-->>Client: session started
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
frontend/src/components/AssignmentTable.tsx (1)
72-83:⚠️ Potential issue | 🟠 MajorRows added here cannot ever acquire a valid
course_id.New rows start with
course_id: '', and the table only lets the user editcourse_name. After the migration, that means a user-added row can look valid in the UI but still has no canonical FK when the calendar flow saves it. This needs a course selector or a name→id resolution step before persistence.Also applies to: 239-247
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/AssignmentTable.tsx` around lines 72 - 83, The add function in AssignmentTable.tsx creates new Assignment rows with course_id set to '' which means user-edited course_name never maps to the canonical FK; update the UI/logic so new rows either provide a course selector or perform a name→id resolution before persistence: modify the add handler (and the equivalent logic around lines 239-247) to populate course_id from the currently selected course or open a course-picker when creating the row, and/or add a resolveCourseNameToId step in the save/submit flow that looks up the canonical course_id for the edited course_name and sets assignment.course_id before sending to the backend. Ensure the change references the add function and the save/submit flow in AssignmentTable.tsx so no persisted row has an empty course_id.backend/services/calendar_service.py (1)
40-45:⚠️ Potential issue | 🟠 MajorDo not persist an empty string as
course_id.
course_idis now a canonical FK. Coercing a missing value to""here will either fail the insert or create assignments that can never join back to a course. Reject unresolved assignments at this boundary, or keep the field nullable until it has been resolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/calendar_service.py` around lines 40 - 45, The code in rows.append() (in calendar_service.py) must not coerce missing course_id to an empty string; change the behavior around "course_id": a.get("course_id") or "" — either validate and reject/skip unresolved assignments before appending (e.g., raise ValueError or return an error when a.get("course_id") is falsy) or preserve a nullable value (use None) and ensure downstream insert allows NULL; update the function that builds rows (the rows.append block and any callers that use variable a) to enforce this rule and add a clear failure path for missing course_id instead of persisting "".frontend/src/app/learn/page.tsx (1)
90-106:⚠️ Potential issue | 🟠 MajorSelected courses are no longer propagated to
startSession.This page collapses
getCourses()down to course names, and the changed call now hardcodescourseIdtoundefined. Sessions launched from the course picker therefore lose their canonical course association, so the backend cannot persist them against the migratedcourse_idmodel.Also applies to: 173-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/learn/page.tsx` around lines 90 - 106, The code reduces getCourses() to only course_name strings (setApiCourseNames) and builds a color map by name, which loses course_id so startSession is being called with courseId undefined; restore and propagate canonical IDs by changing getCourses handling to keep an array or map of course objects (or a Record course_name->course_id), update state (e.g., apiCourses or apiCourseMap instead of apiCourseNames) and adjust the courses useMemo and the course picker to use/display names but pass the corresponding course_id into startSession (also update the same logic referenced around lines 173-180) so backend sessions receive the correct course_id.frontend/src/app/library/page.tsx (1)
349-365:⚠️ Potential issue | 🟠 MajorThis add-course flow still enrolls by free-text name, not canonical id.
addCourse()now expects a catalogcourse_id, but this handler still sends the typed label fromnewCourseNameand then tries to recover the created enrollment by matchingcourse_name. Unless the typed value already equals the canonical id, enrollment will miss the catalog or select the wrong course. This UI needs a catalog-backed picker or an explicit name→id resolution step before calling the API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/library/page.tsx` around lines 349 - 365, The handler handleAddCourse is sending the free-text newCourseName to addCourse but addCourse now requires a canonical catalog course_id; insert a resolution step before calling addCourse: lookup the catalog id for the typed label (e.g., via a new helper like fetchCourseIdByLabel(name) or an existing catalog search API), handle the not-found case by setting course add error, and pass the resolved course_id to addCourse instead of the raw name; keep the rest of the flow (color selection, calling getCourses, setting selectedCourseId) unchanged and ensure you match created by course_id (not course_name) when choosing the selected course.
🧹 Nitpick comments (3)
backend/routes/learn.py (1)
514-518: Inefficient: Duplicate database query for topic.The
topicis fetched twice with identical queries. Consider storing the result of the first query.♻️ Refactor to single query
_ensure_session_ready(body.session_id, body.user_id) student_name = get_user_name(body.user_id).split()[0] - topic = table("sessions").select( - "topic", filters={"id": f"eq.{body.session_id}"}, limit=1 - )[0]["topic"] if table("sessions").select( - "topic", filters={"id": f"eq.{body.session_id}"}, limit=1 - ) else "this topic" + session_rows = table("sessions").select( + "topic", filters={"id": f"eq.{body.session_id}"}, limit=1 + ) + topic = session_rows[0]["topic"] if session_rows else "this topic"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/learn.py` around lines 514 - 518, The code makes the same table("sessions").select(...) call twice when computing topic; replace the duplicate by calling table("sessions").select(...) once (e.g., assign its result to a local variable like session_rows), then set topic = session_rows[0]["topic"] if session_rows else "this topic"; update any references that used the original expression to use the stored result and keep using body.session_id as the filter parameter.backend/routes/calendar.py (1)
180-191: Unused variable:course_labelis computed but never used.The
course_labelvariable at line 183 is constructed but not included in the study block dict. Was this intended to be used in thetopicfield?🔧 If intended, include course_label in the block
for a in assignments: course = a.get("courses", {}) if isinstance(a.get("courses"), dict) else {} course_label = f"[{course.get('course_code', '')}] " if course.get('course_code') else "" blocks.append({ - "topic": a["title"], + "topic": f"{course_label}{a['title']}" if course_label else a["title"], "suggested_date": a["due_date"],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/calendar.py` around lines 180 - 191, The code computes course_label but never uses it; update the study block creation in the function that builds blocks (the loop iterating over assignments) to include course_label in the "topic" field (e.g., replace "topic": a["title"] with "topic": f"{course_label}{a['title']}") so the course code is prepended when present, or alternatively remove the unused course_label if that information is intentionally omitted; keep the rest of the block keys (suggested_date, duration_minutes, reason, related_assignment_id) unchanged.backend/services/course_context_service.py (1)
55-63: Consider logging Gemini failures for observability.The bare
except Exceptionsilently falls back to a static summary. For production debugging, consider logging the exception before falling back.🔧 Add logging for Gemini failures
+import logging + +logger = logging.getLogger(__name__) + def _generate_summary_with_gemini(...) -> str: ... try: return call_gemini(prompt, retries=1) except Exception: + logger.warning("Gemini summary generation failed, using fallback", exc_info=True) # Fallback summary if Gemini fails return (...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/course_context_service.py` around lines 55 - 63, The except block that swallows errors from call_gemini should log the exception before returning the fallback summary: change the bare except to "except Exception as e" and call the module logger (e.g., logger.exception(...) or logger.error(..., exc_info=True)) with a concise message like "Gemini call failed" and the exception, then return the existing fallback using avg_class_mastery, top_struggling, and top_mastered; ensure you reference the existing logger instance or create one with logging.getLogger(...) if none exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/routes/calendar.py`:
- Around line 118-141: The assignments queries in calendar.py use PostgREST FK
join syntax ("courses!inner(course_code,course_name)") but the assignments table
lacks a course_id FK, so either add a DB migration to add assignments.course_id
referencing courses.id and update schema/migrations accordingly (then keep the
join and ensure insert_new_assignments in calendar_service.py writes course_id),
or change the code to stop using FK join: remove "courses!inner(...)" from
calendar.py, fetch course info by querying the courses table by course_name (or
perform an explicit join via a manual query) and update the transformation logic
to derive course_code/course_name from that lookup, and also update
insert_new_assignments to stop writing a non-existent course_id column (write
course_name only or resolve to a course_id via a lookup before insert). Ensure
these changes are applied consistently for the handlers used by
/upcoming/{user_id}, /all/{user_id}, /suggest-study-blocks and /sync-to-google
and reference functions/calendar.py and insert_new_assignments in
calendar_service.py when making edits.
In `@backend/routes/graph.py`:
- Around line 40-42: The create_course route currently returns the raw result
from add_course which can be {"error": "Course not found in catalog"} and thus
returns HTTP 200; update create_course to inspect the return value from
add_course (or the underlying canonical lookup response) and if it contains an
"error" key raise an HTTPException with an appropriate 4xx status (e.g., 404)
and detail set to the error message; ensure HTTPException is imported from
fastapi and keep successful add_course returns unchanged so clients only receive
200 for truly successful enrollments.
In `@backend/routes/learn.py`:
- Line 331: Replace the undefined bare session_id used in the
PENDING_SESSIONS.pop call with the correct property from the request body (use
body.session_id) so the pending session is removed using the provided session
identifier; update the PENDING_SESSIONS.pop(...) invocation to reference
body.session_id (the same body variable used elsewhere in the surrounding
function) to avoid the NameError.
In `@backend/services/course_context_service.py`:
- Around line 66-72: The caller in quiz.py is passing a subject name to
get_course_context(course_id: str), causing it to return {} because it filters
by course_id; update the caller to pass the actual course identifier (e.g., use
the node's course_id field or resolve the subject->course_id before calling) so
get_course_context receives the course_id it expects, or alternatively add a
short lookup inside get_course_context to resolve a subject name to the
corresponding course_id before querying; reference get_course_context and the
quiz.py call to get_course_context(subject) when making the change.
---
Outside diff comments:
In `@backend/services/calendar_service.py`:
- Around line 40-45: The code in rows.append() (in calendar_service.py) must not
coerce missing course_id to an empty string; change the behavior around
"course_id": a.get("course_id") or "" — either validate and reject/skip
unresolved assignments before appending (e.g., raise ValueError or return an
error when a.get("course_id") is falsy) or preserve a nullable value (use None)
and ensure downstream insert allows NULL; update the function that builds rows
(the rows.append block and any callers that use variable a) to enforce this rule
and add a clear failure path for missing course_id instead of persisting "".
In `@frontend/src/app/learn/page.tsx`:
- Around line 90-106: The code reduces getCourses() to only course_name strings
(setApiCourseNames) and builds a color map by name, which loses course_id so
startSession is being called with courseId undefined; restore and propagate
canonical IDs by changing getCourses handling to keep an array or map of course
objects (or a Record course_name->course_id), update state (e.g., apiCourses or
apiCourseMap instead of apiCourseNames) and adjust the courses useMemo and the
course picker to use/display names but pass the corresponding course_id into
startSession (also update the same logic referenced around lines 173-180) so
backend sessions receive the correct course_id.
In `@frontend/src/app/library/page.tsx`:
- Around line 349-365: The handler handleAddCourse is sending the free-text
newCourseName to addCourse but addCourse now requires a canonical catalog
course_id; insert a resolution step before calling addCourse: lookup the catalog
id for the typed label (e.g., via a new helper like fetchCourseIdByLabel(name)
or an existing catalog search API), handle the not-found case by setting course
add error, and pass the resolved course_id to addCourse instead of the raw name;
keep the rest of the flow (color selection, calling getCourses, setting
selectedCourseId) unchanged and ensure you match created by course_id (not
course_name) when choosing the selected course.
In `@frontend/src/components/AssignmentTable.tsx`:
- Around line 72-83: The add function in AssignmentTable.tsx creates new
Assignment rows with course_id set to '' which means user-edited course_name
never maps to the canonical FK; update the UI/logic so new rows either provide a
course selector or perform a name→id resolution before persistence: modify the
add handler (and the equivalent logic around lines 239-247) to populate
course_id from the currently selected course or open a course-picker when
creating the row, and/or add a resolveCourseNameToId step in the save/submit
flow that looks up the canonical course_id for the edited course_name and sets
assignment.course_id before sending to the backend. Ensure the change references
the add function and the save/submit flow in AssignmentTable.tsx so no persisted
row has an empty course_id.
---
Nitpick comments:
In `@backend/routes/calendar.py`:
- Around line 180-191: The code computes course_label but never uses it; update
the study block creation in the function that builds blocks (the loop iterating
over assignments) to include course_label in the "topic" field (e.g., replace
"topic": a["title"] with "topic": f"{course_label}{a['title']}") so the course
code is prepended when present, or alternatively remove the unused course_label
if that information is intentionally omitted; keep the rest of the block keys
(suggested_date, duration_minutes, reason, related_assignment_id) unchanged.
In `@backend/routes/learn.py`:
- Around line 514-518: The code makes the same table("sessions").select(...)
call twice when computing topic; replace the duplicate by calling
table("sessions").select(...) once (e.g., assign its result to a local variable
like session_rows), then set topic = session_rows[0]["topic"] if session_rows
else "this topic"; update any references that used the original expression to
use the stored result and keep using body.session_id as the filter parameter.
In `@backend/services/course_context_service.py`:
- Around line 55-63: The except block that swallows errors from call_gemini
should log the exception before returning the fallback summary: change the bare
except to "except Exception as e" and call the module logger (e.g.,
logger.exception(...) or logger.error(..., exc_info=True)) with a concise
message like "Gemini call failed" and the exception, then return the existing
fallback using avg_class_mastery, top_struggling, and top_mastered; ensure you
reference the existing logger instance or create one with logging.getLogger(...)
if none exists.
🪄 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 Plus
Run ID: 9724a29e-b14d-4462-9bec-16425dd1c515
📒 Files selected for processing (15)
backend/models/__init__.pybackend/routes/calendar.pybackend/routes/graph.pybackend/routes/learn.pybackend/services/calendar_service.pybackend/services/course_context_service.pybackend/services/graph_service.pyfrontend/src/__tests__/api.test.tsfrontend/src/app/calendar/page.tsxfrontend/src/app/dashboard/page.tsxfrontend/src/app/learn/page.tsxfrontend/src/app/library/page.tsxfrontend/src/components/AssignmentTable.tsxfrontend/src/lib/api.tsfrontend/src/lib/types.ts
…arg, and missing FK
- learn.py: fix NameError on PENDING_SESSIONS.pop(session_id) -> body.session_id
- graph.py: raise HTTPException(404) when add_course returns {"error": ...}
- quiz.py: pass node course_id to get_course_context instead of subject name
- calendar.py: use left join (courses!left) so assignments without course_id still appear
- calendar_service.py: insert None instead of "" for missing course_id (FK constraint)
- supabase_schema.sql: add course_id FK column to assignments table + migration comment
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/routes/quiz.py (1)
47-67:⚠️ Potential issue | 🟠 MajorUse the new
get_course_context()payload shape here.
backend/services/course_context_service.py:66-107now returns{"course_summary": ..., "concept_stats": ...}. Readingcommon_misconceptionsandweak_areasfrom the top level always yields empty lists, so shared course context never makes it into the quiz prompt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/quiz.py` around lines 47 - 67, The code reads common_misconceptions and weak_areas from the top-level course_ctx but the service now returns {"course_summary": ..., "concept_stats": ...}; update the block that calls get_course_context() to pull concept_stats = course_ctx.get("concept_stats", {}) and then set misconceptions = concept_stats.get("common_misconceptions", []) and weak_areas = concept_stats.get("weak_areas", []), leaving the rest of the logic that builds addendum_parts and appends to prompt unchanged (referencing get_course_context, course_ctx, misconceptions, weak_areas, and prompt).
🧹 Nitpick comments (1)
backend/routes/calendar.py (1)
118-125: Drop the commentary that just repeats the next statement.The added “join/transform” comments do not explain anything non-obvious and make these handlers noisier to scan.
As per coding guidelines, "
**/*.{ts,tsx,py}: No docstrings or comments unless the logic is non-obvious."Also applies to: 147-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/calendar.py` around lines 118 - 125, Remove the redundant “Join with courses to get course_code and course_name” and “Transform to include course info at top level” comments that simply restate the following code; in the handler around the rows assignment (the call to table("assignments").select and subsequent transformation of rows) delete those two comments and only add a brief comment if you need to explain non-obvious logic — otherwise leave the code self-documenting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/db/supabase_schema.sql`:
- Around line 111-120: The bootstrap schema is out of sync with the migrated
runtime model: update the SQL to match the current runtime tables by removing
the old user-scoped design for courses and adding the new columns/tables
referenced by the routes—specifically modify the courses table to include
course_code (and any non-user-scoped primary key structure used now), add the
user_courses join table, and add course_id columns to sessions and graph_nodes
(ensuring they reference courses(id)); also keep assignments.course_id as-is,
remove or update the stale migration comment, and ensure all FK constraints and
defaults align with the runtime model so a fresh DB init matches the codepaths
that read/write these fields.
In `@backend/routes/calendar.py`:
- Around line 127-141: The code that builds the assignments dict from DB rows
uses the joined courses object (variable course) for course_name/course_code and
loses legacy assignments that have course_name on the assignments row; update
the construction in the loop that creates assignments (the block that sets
"course_id", "course_code", "course_name") to fall back to the legacy column
when the join is missing (e.g., use course.get("course_name",
r.get("course_name", "")) and similarly for course_code), and make the same
fallback change to the other assignment-building loops referenced in this file
(the other blocks that construct assignments lists for /upcoming, /all, and the
Google sync/export code).
---
Outside diff comments:
In `@backend/routes/quiz.py`:
- Around line 47-67: The code reads common_misconceptions and weak_areas from
the top-level course_ctx but the service now returns {"course_summary": ...,
"concept_stats": ...}; update the block that calls get_course_context() to pull
concept_stats = course_ctx.get("concept_stats", {}) and then set misconceptions
= concept_stats.get("common_misconceptions", []) and weak_areas =
concept_stats.get("weak_areas", []), leaving the rest of the logic that builds
addendum_parts and appends to prompt unchanged (referencing get_course_context,
course_ctx, misconceptions, weak_areas, and prompt).
---
Nitpick comments:
In `@backend/routes/calendar.py`:
- Around line 118-125: Remove the redundant “Join with courses to get
course_code and course_name” and “Transform to include course info at top level”
comments that simply restate the following code; in the handler around the rows
assignment (the call to table("assignments").select and subsequent
transformation of rows) delete those two comments and only add a brief comment
if you need to explain non-obvious logic — otherwise leave the code
self-documenting.
🪄 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 Plus
Run ID: 4d1f2047-ace2-4deb-a3b3-806813c28c1a
📒 Files selected for processing (6)
backend/db/supabase_schema.sqlbackend/routes/calendar.pybackend/routes/graph.pybackend/routes/learn.pybackend/routes/quiz.pybackend/services/calendar_service.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/services/calendar_service.py
- backend/routes/graph.py
…e resolution - quiz: aggregate misconceptions and prerequisite_gaps from concept_stats rows - learn: match enrolled course by graph subject label; dedupe session topic query - calendar: fallback course_name/course_code from assignment row; study/sync labels - frontend: pass course_id from graph nodes and course picker; resolve course_id on syllabus save; library/dashboard addCourse errors and post-add selection - tests: update quiz augmentation mocks for course_id and concept_stats Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
backend/routes/learn.py (1)
75-76:⚠️ Potential issue | 🟠 MajorDon't swallow the DB lookup failures here.
These
except Exception: passblocks make the newcourse_idmigration fail silently, which is especially risky in the two helpers that decide course resolution and prompt context. Please log the exception plus the lookup key and then keep the current fallback behavior.Minimal fix
- except Exception: - pass + except Exception as e: + print(f"Failed to resolve course_id for topic={topic_trim!r} user_id={user_id!r}: {e}") @@ - except Exception: - pass + except Exception as e: + print(f"Failed to load course info for course_id={course_id!r}: {e}")Also applies to: 145-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/learn.py` around lines 75 - 76, Replace the silent except blocks that read "except Exception: pass" inside the helpers that resolve course_id and build prompt context with error-logging handlers: catch the exception as e, log the exception and the lookup key (e.g., the course lookup value or course_id being used) via the existing logger (or processLogger) including context like "course lookup failed for key=...", and then continue to return the current fallback value so behavior is unchanged; apply the same change to both occurrences mentioned (the course resolution helper and the prompt-context helper).
🧹 Nitpick comments (1)
backend/routes/learn.py (1)
232-233: Trim the restated inline comments.These comments only narrate the next line, so they add noise without clarifying anything non-obvious.
As per coding guidelines, "No docstrings or comments unless the logic is non-obvious".
Also applies to: 258-260, 307-309, 492-494
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/learn.py` around lines 232 - 233, Remove the redundant inline comments that simply restate the next line of code (e.g., the comment immediately above the session_data dict in backend/routes/learn.py) and the other similar narrating comments around the session creation, progress update, and response handling blocks; leave only comments that explain non-obvious logic or intent and ensure no docstring-style or line-by-line narration remains so the code is concise and follows the "no comments unless non-obvious" guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/routes/learn.py`:
- Around line 56-58: The queries in learn.py (e.g., the enrolled selection using
table("user_courses") and later queries/inserts referencing
graph_nodes.course_id and sessions.course_id) will fail because the DB schema
lacks course_code and course_id columns and silent except blocks mask these
errors; add the three schema changes (ALTER TABLE courses ADD COLUMN course_code
TEXT; ALTER TABLE graph_nodes ADD COLUMN course_id TEXT REFERENCES courses(id);
ALTER TABLE sessions ADD COLUMN course_id TEXT REFERENCES courses(id)) in your
migration, and replace the broad silent excepts around the Supabase calls (the
try/except blocks that currently do "except Exception: pass") with error
logging/raising so API errors are not swallowed and course resolution can
surface failures.
In `@backend/routes/quiz.py`:
- Around line 47-50: The graph_nodes schema is missing course_id so inserts in
backend/services/graph_service.py (the code that sets "course_id":
node_course_id) and reads in backend/routes/quiz.py (node.get("course_id", ""))
are ineffective; fix by adding course_id TEXT REFERENCES courses(id) to the
graph_nodes CREATE TABLE in backend/db/supabase_schema.sql and add a migration
comment/SQL to backfill or add the column for existing deployments, then verify
the insert path in the graph insert function and the read in routes/quiz.py
continue to use node_course_id/get("course_id") correctly; alternatively, if you
don't want schema changes, remove the course_id insert from the graph service
and the read/usage in routes/quiz.py (and any get_course_context usage) so the
code and schema are consistent.
In `@backend/tests/test_shared_course_context.py`:
- Around line 426-437: The test fixtures currently include both subject and
course_id so they won't detect a regression reverting to
get_course_context(node["subject"]); update the quiz-augmentation tests that set
mock_table/mock_ctx (the blocks around mock_table.return_value.select and
mock_ctx.return_value) to assert the new lookup by calling
mock_ctx.assert_called_once_with("c1") after generate_quiz() (or alternatively
remove the legacy "subject" field from the mocked row), ensuring the code uses
course_id rather than subject; apply the same change to the other two test
locations mentioned.
---
Duplicate comments:
In `@backend/routes/learn.py`:
- Around line 75-76: Replace the silent except blocks that read "except
Exception: pass" inside the helpers that resolve course_id and build prompt
context with error-logging handlers: catch the exception as e, log the exception
and the lookup key (e.g., the course lookup value or course_id being used) via
the existing logger (or processLogger) including context like "course lookup
failed for key=...", and then continue to return the current fallback value so
behavior is unchanged; apply the same change to both occurrences mentioned (the
course resolution helper and the prompt-context helper).
---
Nitpick comments:
In `@backend/routes/learn.py`:
- Around line 232-233: Remove the redundant inline comments that simply restate
the next line of code (e.g., the comment immediately above the session_data dict
in backend/routes/learn.py) and the other similar narrating comments around the
session creation, progress update, and response handling blocks; leave only
comments that explain non-obvious logic or intent and ensure no docstring-style
or line-by-line narration remains so the code is concise and follows the "no
comments unless non-obvious" guideline.
🪄 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 Plus
Run ID: 5ea9b5b0-f656-4543-8b62-59a02431ca2d
📒 Files selected for processing (9)
backend/routes/calendar.pybackend/routes/learn.pybackend/routes/quiz.pybackend/tests/test_shared_course_context.pyfrontend/src/app/calendar/page.tsxfrontend/src/app/dashboard/page.tsxfrontend/src/app/learn/page.tsxfrontend/src/app/library/page.tsxfrontend/src/lib/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/app/learn/page.tsx
- frontend/src/app/calendar/page.tsx
- backend/routes/calendar.py
…se_id model - GitHub Actions: continue-on-error on Claude Code steps so API limits do not block PRs - main.py: optional recost import for environments without the package - test_shared_course_context: course_summary/concept_stats mocks; learn build_system_prompt uses course_id - test_learn_routes: _get_course_id_for_topic coverage; graph apply_graph_update uses course_id - test_quiz_routes: patch routes.quiz.update_streak (import binding) to avoid real Supabase - test_graph_service: user_courses enrollment mocks; add_course/delete_course catalog semantics - test_calendar_routes: upcoming assignment mocks include user_id - test_ocr_pipeline: skip when GEMINI_API_KEY unset Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/tests/test_calendar_routes.py (1)
122-150: Strengthen this test to validate the expanded response contract.The fixture now includes additional fields, but the test still only checks
title. Add assertions for the newly mocked fields (and defaultedcourse_code/course_name) so mapping regressions are caught.Proposed test assertion update
assert r.status_code == 200 - assert len(r.json()["assignments"]) == 2 - assert r.json()["assignments"][0]["title"] == "HW1" + assignments = r.json()["assignments"] + assert len(assignments) == 2 + assert assignments[0]["title"] == "HW1" + assert assignments[0]["user_id"] == "user_andres" + assert assignments[0]["assignment_type"] == "homework" + assert assignments[0]["notes"] is None + assert assignments[0]["google_event_id"] is None + assert assignments[0]["course_id"] is None + assert assignments[0]["course_code"] == "" + assert assignments[0]["course_name"] == ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_calendar_routes.py` around lines 122 - 150, Update the test that patches routes.calendar.table and calls client.get("/api/calendar/upcoming/user_andres") to assert the full expanded response contract: after confirming r.status_code == 200 and len(r.json()["assignments"]) == 2, add assertions on r.json()["assignments"][0] and [1] for the mocked fields ("id", "user_id", "due_date", "assignment_type", "notes", "google_event_id", "course_id") and also assert the default-mapped fields "course_code" and "course_name" are present and have expected default values; locate the mock_rows in the test and add the new assertions immediately after the existing title check so mapping regressions in the calendar route are caught.backend/tests/test_quiz_routes.py (1)
207-244: Consider refactoring to reduce nesting depth.This test still uses 5 levels of nested
with patch(...)while other tests use_submit_quiz_mocks(). Since this test needs a custom table factory for the JSON-string scenario, consider parameterizing the context manager:♻️ Suggested refactor
`@contextmanager` -def _submit_quiz_mocks(): +def _submit_quiz_mocks(table_factory=None): + factory = table_factory or _make_table() with ( - patch("routes.quiz.table", side_effect=_make_table()), + patch("routes.quiz.table", side_effect=factory), patch("routes.quiz.update_streak"), patch("routes.quiz.get_graph", return_value={"nodes": [], "edges": [], "stats": {}}), patch("routes.quiz.get_quiz_context", return_value={}), patch("routes.quiz.call_gemini_json", return_value={}), ): yieldThen simplify the test:
def test_questions_json_as_string_is_parsed(self): """quiz_attempts.questions_json can arrive as a JSON string from the DB.""" import json as _json - - def factory(name): - ... - - with patch("routes.quiz.table", side_effect=factory): - with patch("routes.quiz.update_streak"): - with patch("routes.quiz.get_graph", return_value={"nodes": [], "edges": [], "stats": {}}): - with patch("routes.quiz.get_quiz_context", return_value={}): - with patch("routes.quiz.call_gemini_json", return_value={}): - r = client.post(...) + # define custom factory... + with _submit_quiz_mocks(table_factory=factory): + r = client.post(...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_quiz_routes.py` around lines 207 - 244, The test test_questions_json_as_string_is_parsed currently nests five with-patch blocks to inject a custom factory for routes.quiz.table; refactor by extending the existing _submit_quiz_mocks helper to accept an optional table_factory (or a flag) so you can reuse _submit_quiz_mocks() and provide the custom factory that returns serialized questions_json, then replace the nested with patch("routes.quiz.table", side_effect=factory) plus the other with patches with a single call to _submit_quiz_mocks(table_factory=factory) (or similar) inside the test; ensure the helper still patches update_streak, get_graph, get_quiz_context, and call_gemini_json so behavior and assertions in test_questions_json_as_string_is_parsed remain unchanged.backend/tests/test_graph_service.py (1)
162-170: Reuse_enrollment_row()for this fixture.This reintroduces the enrollment shape inline even though the file now has a shared helper for it. Using
_enrollment_row("c1", "", "Math")here will keep theuser_coursesmock schema from drifting across tests.♻️ Small cleanup
if name == "user_courses": - mock.select.return_value = [{ - "id": "e1", - "course_id": "c1", - "color": "#fff", - "nickname": None, - "enrolled_at": "2026-01-01", - "courses": {"course_code": "", "course_name": "Math", "school": "", "department": ""}, - }] + row = _enrollment_row("c1", "", "Math") + row["color"] = "#fff" + mock.select.return_value = [row]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_graph_service.py` around lines 162 - 170, Replace the inline enrollment dict in the user_courses branch with the shared helper to avoid schema drift: when name == "user_courses" set mock.select.return_value to a list containing _enrollment_row("c1", "", "Math") instead of the hard-coded dict; ensure you import/visible _enrollment_row in this test module and keep the surrounding keys (id, course_id, color, nickname, enrolled_at, courses) determined by that helper so the mocked shape stays consistent across tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/test_graph_service.py`:
- Around line 232-245: The test currently patches services.graph_service.table
to always return the same MagicMock so delete_course may be deleting from the
wrong table; change the test_unenroll_with_no_prior_nodes to patch
services.graph_service.table with a side_effect that returns distinct mocks for
the 'user_courses' and 'courses' table names (or otherwise return different
mocks based on the table argument), call delete_course("u1", "empty-course-id"),
and assert that the specific user_courses mock had its delete method called (and
optionally asserted with the expected key), while the courses mock was not used
— reference the delete_course function and services.graph_service.table to
locate where to apply the side_effect and assertions.
- Around line 196-227: Update the tests to assert the actual DB interactions
rather than only the return dict: when using the factory/patch for
services.graph_service.table in test_inserts_new_course, verify that the mock
for "courses" had select called and that the mock for "user_courses" had insert
called with the expected enrollment payload (and that no other table's insert
was called); in test_skips_insert_for_existing_course, assert that
"user_courses".insert was not called and that the "courses" lookup was still
exercised as expected. Reference the existing factory, the patched
services.graph_service.table, and the add_course call to locate where to add
these assertions.
---
Nitpick comments:
In `@backend/tests/test_calendar_routes.py`:
- Around line 122-150: Update the test that patches routes.calendar.table and
calls client.get("/api/calendar/upcoming/user_andres") to assert the full
expanded response contract: after confirming r.status_code == 200 and
len(r.json()["assignments"]) == 2, add assertions on r.json()["assignments"][0]
and [1] for the mocked fields ("id", "user_id", "due_date", "assignment_type",
"notes", "google_event_id", "course_id") and also assert the default-mapped
fields "course_code" and "course_name" are present and have expected default
values; locate the mock_rows in the test and add the new assertions immediately
after the existing title check so mapping regressions in the calendar route are
caught.
In `@backend/tests/test_graph_service.py`:
- Around line 162-170: Replace the inline enrollment dict in the user_courses
branch with the shared helper to avoid schema drift: when name == "user_courses"
set mock.select.return_value to a list containing _enrollment_row("c1", "",
"Math") instead of the hard-coded dict; ensure you import/visible
_enrollment_row in this test module and keep the surrounding keys (id,
course_id, color, nickname, enrolled_at, courses) determined by that helper so
the mocked shape stays consistent across tests.
In `@backend/tests/test_quiz_routes.py`:
- Around line 207-244: The test test_questions_json_as_string_is_parsed
currently nests five with-patch blocks to inject a custom factory for
routes.quiz.table; refactor by extending the existing _submit_quiz_mocks helper
to accept an optional table_factory (or a flag) so you can reuse
_submit_quiz_mocks() and provide the custom factory that returns serialized
questions_json, then replace the nested with patch("routes.quiz.table",
side_effect=factory) plus the other with patches with a single call to
_submit_quiz_mocks(table_factory=factory) (or similar) inside the test; ensure
the helper still patches update_streak, get_graph, get_quiz_context, and
call_gemini_json so behavior and assertions in
test_questions_json_as_string_is_parsed remain unchanged.
🪄 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 Plus
Run ID: 5ef4e8e8-e012-4fa0-a49c-4086e0b84a7b
📒 Files selected for processing (9)
.github/workflows/claude-code-review.yml.github/workflows/claude.ymlbackend/main.pybackend/tests/test_calendar_routes.pybackend/tests/test_graph_service.pybackend/tests/test_learn_routes.pybackend/tests/test_ocr_pipeline.pybackend/tests/test_quiz_routes.pybackend/tests/test_shared_course_context.py
Comment on lines
+232
to
+245
| def test_unenrolls_user_from_course(self): | ||
| mock = MagicMock() | ||
| mock.delete.return_value = [] | ||
| with patch("services.graph_service.table", return_value=mock): | ||
| result = delete_course("u1", "course-id-1") | ||
|
|
||
| assert result == {"deleted": True} | ||
| mock.delete.assert_called_once() | ||
|
|
||
| def test_deletes_course_with_no_nodes(self): | ||
| mock = _simple_mock(select_returns=[]) | ||
| def test_unenroll_with_no_prior_nodes(self): | ||
| mock = MagicMock() | ||
| mock.delete.return_value = [] | ||
| with patch("services.graph_service.table", return_value=mock): | ||
| result = delete_course("u1", "EmptyCourse") | ||
| result = delete_course("u1", "empty-course-id") |
There was a problem hiding this comment.
Pin the unenroll target, not just “some delete happened.”
Because table() always returns the same mock here, this still passes if delete_course() deletes from courses instead of user_courses. The migration coverage is much stronger if the test asserts the table name and the delete call on that specific mock.
🧪 Tighter assertion for the unenroll path
def test_unenrolls_user_from_course(self):
- mock = MagicMock()
- mock.delete.return_value = []
- with patch("services.graph_service.table", return_value=mock):
+ tables = {"user_courses": MagicMock()}
+ tables["user_courses"].delete.return_value = []
+ with patch("services.graph_service.table", side_effect=tables.__getitem__) as table_mock:
result = delete_course("u1", "course-id-1")
assert result == {"deleted": True}
- mock.delete.assert_called_once()
+ table_mock.assert_called_with("user_courses")
+ tables["user_courses"].delete.assert_called_once()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/test_graph_service.py` around lines 232 - 245, The test
currently patches services.graph_service.table to always return the same
MagicMock so delete_course may be deleting from the wrong table; change the
test_unenroll_with_no_prior_nodes to patch services.graph_service.table with a
side_effect that returns distinct mocks for the 'user_courses' and 'courses'
table names (or otherwise return different mocks based on the table argument),
call delete_course("u1", "empty-course-id"), and assert that the specific
user_courses mock had its delete method called (and optionally asserted with the
expected key), while the courses mock was not used — reference the delete_course
function and services.graph_service.table to locate where to apply the
side_effect and assertions.
…s course_id - course_context_service: aggregate quiz_context per concept (not one global list) - graph_service: scope apply_graph_update node lookups by course_id when provided - documents: set course_id on syllabus-extracted assignments from upload context - api.test: saveAssignments fixture uses course_id Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/routes/documents.py`:
- Around line 144-147: The loop in backend/routes/documents.py mutates every
item but still passes the original assignments list to save_assignments_to_db,
so non-dict items can cause calendar_service.py to crash (it expects dicts and
calls a.get). Filter the assignments list to only include dict items, set each
dict's "course_id" (e.g., inside the for a in assignments: block or via a new
list comprehension), and then call save_assignments_to_db(user_id,
filtered_assignments) so calendar_service.save_assignments_to_db only receives
well-formed dicts.
In `@backend/services/course_context_service.py`:
- Around line 267-279: The current selection always picks five concepts even
when pct_struggling or pct_mastered are 0.0; update the logic around
concept_metrics/sorted_by_struggling/top_struggling_concepts and
sorted_by_mastered/top_mastered_concepts to first filter out entries with
pct_struggling == 0.0 (and pct_mastered == 0.0 respectively) before sorting and
slicing, so only concepts with non-zero rates become “top” and an empty list
remains when none qualify (allowing the “None identified” fallback).
- Around line 76-89: The queries against the partitioned tables
table("course_summary") and table("course_concept_stats") in get_course_context
currently filter only by course_id and then take summary_rows[0], which can
return/merge data from different semesters; update both selects to also filter
by the semester (e.g. add a "semester": f"eq.{semester}" filter) and ensure you
pick the summary row that matches that semester rather than blindly using the
first element (i.e., scope summary_rows and stats_rows by semester and
validate/handle when no row exists for that semester).
- Around line 122-143: The aggregation currently returns early when
enrollment_rows or node_rows is empty and only upserts remaining concepts,
leaving stale course_summary and course_concept_stats records; update the logic
in the function that builds the summary (references: enrollment_rows, user_ids,
node_rows, course_info) to proactively purge stale rows: when enrollment_rows is
empty or node_rows is empty, delete any existing course_summary and all
course_concept_stats for the given course_id; when you compute the set of
current concept identifiers from node_rows before upserting, delete any
course_concept_stats rows for course_id whose concept identifier (concept_name
or id depending on your schema) is not in that current set so only present
concepts remain; ensure deletes run via the same table(...) helper that you use
elsewhere (targeting course_summary and course_concept_stats) before returning
or after upsert so get_course_context() no longer serves stale data.
- Line 108: The function update_course_context currently hard-codes
semester="Spring 2026"; remove this default so the semester must be explicit or
else compute it dynamically. Change the signature of
update_course_context(course_id: str, semester: str) (no hard-coded default) and
update all callers that call update_course_context(course_id) to pass the
correct semester, or alternatively implement a single-source helper (e.g.,
get_current_semester()) inside update_course_context to derive the current
semester when semester is None and use that value for writes; ensure the unique
symbol update_course_context and any callers that invoke it are updated
accordingly so writes go to the correct semester partition.
In `@backend/services/graph_service.py`:
- Around line 228-253: add_course (and likewise delete_course) mutates
user_courses but doesn't refresh the shared course context computed in
course_context_service, so shared summaries become stale; after successfully
inserting or removing a user_courses row in add_course and delete_course, import
and invoke the course_context_service function that recomputes/invalidates
shared context for that course (e.g. call
course_context_service.refresh_shared_context_for_course(course_id) or the
equivalent function used at Lines 118-121 in course_context_service), ensure the
call runs after the DB change and errors are handled/logged so the refresh won't
fail silently.
- Around line 396-403: The synchronous loop calling update_course_context(cid)
inside apply_graph_update is blocking the request; change it to run updates
asynchronously (e.g., dispatch background tasks or schedule jobs) so
apply_graph_update returns immediately. Specifically, replace the direct calls
to services.course_context_service.update_course_context in the touched_courses
loop with non-blocking dispatch (enqueue to your existing task queue, spawn a
background worker/future, or use an async fire-and-forget helper), ensuring
errors are logged but do not block; keep the retry/timeout behavior of
_generate_summary_with_gemini/gemini_service intact by running that logic in the
background worker rather than inline. Ensure the new code references the same
function name update_course_context so the background worker invokes it for each
cid.
🪄 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 Plus
Run ID: ace1f249-10a4-4d58-a915-31e9f863a321
📒 Files selected for processing (4)
backend/routes/documents.pybackend/services/course_context_service.pybackend/services/graph_service.pyfrontend/src/__tests__/api.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/tests/api.test.ts
Comment on lines
+122
to
+143
| if not enrollment_rows: | ||
| return # No students enrolled, nothing to aggregate | ||
|
|
||
| user_ids = [r["user_id"] for r in enrollment_rows] | ||
| student_count = len(user_ids) | ||
|
|
||
| # ── 2. Get course info for the summary ──────────────────────────────────── | ||
| course_rows = table("courses").select( | ||
| "course_code,course_name", | ||
| filters={"id": f"eq.{course_id}"}, | ||
| ) | ||
| course_info = course_rows[0] if course_rows else {"course_code": "", "course_name": ""} | ||
|
|
||
| # ── 3. All graph nodes for this course across every enrolled student ────── | ||
| # Build user_id filter for PostgREST | ||
| user_filter = ",".join(user_ids) | ||
| node_rows = table("graph_nodes").select( | ||
| "id,concept_name,mastery_score,mastery_tier,user_id", | ||
| filters={"subject": f"eq.{course_name}"}, | ||
| filters={"course_id": f"eq.{course_id}", "user_id": f"in.({user_filter})"}, | ||
| ) | ||
| if not node_rows: | ||
| return | ||
| return # No graph data yet for this course |
There was a problem hiding this comment.
Purge stale rows when the cohort or concept set shrinks.
This function returns early when there are no enrolled students or no graph_nodes, and later it only upserts the concepts that still exist. Nothing removes the old course_summary row or course_concept_stats rows that dropped out of the current aggregate, so get_course_context() can keep serving stale shared context after unenrollment or when a concept disappears from the course.
Also applies to: 239-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/services/course_context_service.py` around lines 122 - 143, The
aggregation currently returns early when enrollment_rows or node_rows is empty
and only upserts remaining concepts, leaving stale course_summary and
course_concept_stats records; update the logic in the function that builds the
summary (references: enrollment_rows, user_ids, node_rows, course_info) to
proactively purge stale rows: when enrollment_rows is empty or node_rows is
empty, delete any existing course_summary and all course_concept_stats for the
given course_id; when you compute the set of current concept identifiers from
node_rows before upserting, delete any course_concept_stats rows for course_id
whose concept identifier (concept_name or id depending on your schema) is not in
that current set so only present concepts remain; ensure deletes run via the
same table(...) helper that you use elsewhere (targeting course_summary and
course_concept_stats) before returning or after upsert so get_course_context()
no longer serves stale data.
Comment on lines
+396
to
403
| # Refresh shared course context for every course touched in this update | ||
| if touched_courses: | ||
| from services.course_context_service import update_course_context | ||
| for subj in touched_subjects: | ||
| for cid in touched_courses: | ||
| try: | ||
| update_course_context(subj) | ||
| update_course_context(cid) | ||
| except Exception: | ||
| pass # never block the main response for a context refresh |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the graph_service.py file around lines 396-403
find . -type f -name "graph_service.py" | head -5Repository: SaplingLearn/Sapling
Length of output: 99
🏁 Script executed:
# Get file size to determine if we can read it completely
wc -l backend/services/graph_service.pyRepository: SaplingLearn/Sapling
Length of output: 101
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '390,410p' backend/services/graph_service.pyRepository: SaplingLearn/Sapling
Length of output: 821
🏁 Script executed:
# Check gemini_service.py for the timeout configuration
find . -type f -name "gemini_service.py" | head -5Repository: SaplingLearn/Sapling
Length of output: 100
🏁 Script executed:
# Read lines 13-18 of gemini_service.py
sed -n '1,30p' backend/services/gemini_service.pyRepository: SaplingLearn/Sapling
Length of output: 777
🏁 Script executed:
# Find and examine the update_course_context function
rg "def update_course_context" -A 20Repository: SaplingLearn/Sapling
Length of output: 1767
🏁 Script executed:
# Get the full update_course_context function to see if it calls Gemini
wc -l backend/services/course_context_service.pyRepository: SaplingLearn/Sapling
Length of output: 110
🏁 Script executed:
# Search for Gemini calls within course_context_service
rg "gemini" backend/services/course_context_service.py -i -C 5Repository: SaplingLearn/Sapling
Length of output: 2308
🏁 Script executed:
# Search for summary generation which might use Gemini
rg "summary" backend/services/course_context_service.py -i -C 3Repository: SaplingLearn/Sapling
Length of output: 5015
🏁 Script executed:
# Check what apply_graph_update does and where it returns
rg "def apply_graph_update" -A 50 backend/services/graph_service.pyRepository: SaplingLearn/Sapling
Length of output: 2093
Move course context refresh to async/background task to prevent request blocking.
The inline update_course_context(cid) calls at lines 398-401 execute synchronously before apply_graph_update() returns. When triggered, _generate_summary_with_gemini() in course_context_service.py calls Gemini with retries=1. Given the 180-second timeout per attempt (gemini_service.py, line 13), a failed or slow summary refresh can block the learning request for multiple minutes, especially when touched_courses contains multiple course IDs.
The try/except suppresses errors but does not defer execution—the blocking still occurs. Consider deferring this refresh to a background queue or task scheduler.
🧰 Tools
🪛 Ruff (0.15.10)
[error] 402-403: try-except-pass detected, consider logging the exception
(S110)
[warning] 402-402: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/services/graph_service.py` around lines 396 - 403, The synchronous
loop calling update_course_context(cid) inside apply_graph_update is blocking
the request; change it to run updates asynchronously (e.g., dispatch background
tasks or schedule jobs) so apply_graph_update returns immediately. Specifically,
replace the direct calls to
services.course_context_service.update_course_context in the touched_courses
loop with non-blocking dispatch (enqueue to your existing task queue, spawn a
background worker/future, or use an async fire-and-forget helper), ensuring
errors are logged but do not block; keep the retry/timeout behavior of
_generate_summary_with_gemini/gemini_service intact by running that logic in the
background worker rather than inline. Ensure the new code references the same
function name update_course_context so the background worker invokes it for each
cid.
…rors, test alignment Production fixes: - quiz.py: read misconceptions/weak_areas from concept_stats list (new payload shape) - learn.py: log exceptions instead of silently swallowing in course_id lookup helpers - calendar.py: remove unused course_label variable in suggest_study_blocks - course_context_service.py: filter zero-rate concepts from top struggling/mastered; scope concept_stats query by semester - documents.py: filter non-dict assignments and attach course_id before saving - main.py: make recost import optional so tests can import app without the package Test alignment (all 177 tests now pass): - test_shared_course_context.py: update TestGetCourseContext, TestUpdateCourseContext, TestApplyGraphUpdateTriggersContext, TestLearnHelpers, TestQuizPromptAugmentation to match new schema (user_courses join, course_concept_stats/course_summary tables, course_id instead of subject string, renamed helper functions) - test_graph_service.py: update TestGetGraph, TestGetCourses, TestAddCourse to use user_courses enrollment model instead of legacy courses table - test_learn_routes.py: remove _resolve_course import, replace TestResolveCourse with TestGetCourseIdForTopic matching renamed function - test_calendar_routes.py: add full response contract fields to upcoming test fixture - test_quiz_routes.py: add update_streak mock to prevent real DB calls in submit tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ale purge, schema sync - Remove unused course_id_map from get_graph - Derive semester from courses table instead of hardcoding Spring 2026 - Purge stale course_concept_stats/course_summary rows when enrollment hits zero - Trigger update_course_context after add_course and delete_course - Rewrite supabase_schema.sql to match new canonical schema (user_courses, course_id FKs, course_concept_stats, course_summary; drop old course_context blob table) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| try: | ||
| from services.course_context_service import update_course_context | ||
| update_course_context(course_id) | ||
| except Exception: |
| try: | ||
| from services.course_context_service import update_course_context | ||
| update_course_context(course_id) | ||
| except Exception: |
AndresL230
deleted the
combined/course-id-migration-and-assignment-normalization
branch
2 tasks
9 tasks
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 free
to 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.
Summary
Combines the work from PR #51 and PR #52 into a single merge.
From PR #51 (schema migration)
course_idFKs across graph, learn, calendar, course context; Pydantic models updated.api.tsusescourse_idfor courses, sessions, assignments;getCoursesreturnsEnrolledCourseshape.From PR #52 (calendar robustness)
normalizeAssignments()on calendar: default fields, deterministic fallback ids (missing-id-${index}), applied on load / save / Google sync refresh.Combined follow-ups
course_idandcourse_codefrom API responses.EnrolledCourse/startSession(..., courseId?, useSharedContext).extractSyllabuserror path uses response status variable correctly; API tests aligned withcourse_id.Supersedes
Verification:
npm test -- --watchAll=falseandnpm run buildpass onfrontend/.Made with Cursor
Summary by CodeRabbit
New Features
Improvements
Bug Fixes