Phase 14: student portal to-do list + progress radar#248
Merged
Conversation
Adds two new sections to the student portal: - "My Work" — a combined essay+test to-do list grouped into Overdue/Planned/Completed, each item showing status (not started/in progress/submitted) and due date. - "My Progress" — a radar chart of the student's own per-criterion scores, combined across graded rubrics or filtered to one. Tests had no persisted assignment record at all (only a share-code URL the teacher had to hand out manually), so there was nothing to list. Adds `test_assignments` (migration 044), mirroring `essay_assignments`' RLS pattern, plus a read-only policy on `student_tests` for submission status and a scoped `tests` read policy so the portal can embed test content into a self-contained "Open" link. Also fixes a real bug found along the way: TestAssignmentModal shared one teacherKey across an entire class batch, so a DB-mode link pointed every student at the same row — now mints one per student. Docs updated per CLAUDE.md (DocsPage, README, LandingPage, all 5 locales). Deliberately left out: fixing StudentTestPage's own disconnected-client auth (a separate, pre-existing bug in the bare share-link flow, scoped out as its own follow-up in the wiki roadmap). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
Next review available in: 34 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds database-backed test assignment persistence, a teacher modal save-all flow, a unified student portal for essays and tests, and matching docs and locale updates for the new portal work/progress views. ChangesTest assignments and portal backend
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Teacher
participant TestAssignmentModal
participant AppContext
participant SupabaseAdapter
participant Database
Teacher->>TestAssignmentModal: enable DB embedding
TestAssignmentModal->>TestAssignmentModal: generate per-student teacherKeys
loop for each unsaved student
TestAssignmentModal->>AppContext: saveTestAssignment(assignment)
AppContext->>SupabaseAdapter: saveTestAssignment(assignment)
SupabaseAdapter->>Database: upsert test_assignments
Database-->>SupabaseAdapter: result
SupabaseAdapter-->>AppContext: SyncResult
AppContext-->>TestAssignmentModal: SyncResult
end
TestAssignmentModal-->>Teacher: show saved count / error indicator
sequenceDiagram
participant Student
participant StudentPortalPage
participant AppContext
participant SupabaseAdapter
Student->>StudentPortalPage: load portal
StudentPortalPage->>AppContext: fetchMyTestAssignments()
AppContext->>SupabaseAdapter: fetchMyTestAssignments()
SupabaseAdapter-->>AppContext: assignment summaries
AppContext-->>StudentPortalPage: test rows
StudentPortalPage->>StudentPortalPage: merge essay + test rows into allWork
StudentPortalPage-->>Student: render overdue/planned/completed groups
Student->>StudentPortalPage: click open test
StudentPortalPage->>AppContext: fetchAssignedTestContent(testId)
AppContext->>SupabaseAdapter: fetchAssignedTestContent(testId)
SupabaseAdapter-->>AppContext: test content
AppContext-->>StudentPortalPage: test content
StudentPortalPage-->>Student: navigate to test via hash route
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CI's format:check caught line-width violations prettier's own default config didn't flag locally until run explicitly. Whitespace only, no logic change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/Tests/__tests__/TestAssignmentModal.test.tsx (1)
71-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM on the added coverage. Consider also adding a regression test that changes
classIdmid-session and re-assertssaveTestAssignmentis called with the currently renderedteacherKeyfor each student — this would have caught the class-switch bug flagged inTestAssignmentModal.tsx.🤖 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 `@src/components/Tests/__tests__/TestAssignmentModal.test.tsx` around lines 71 - 115, Add a regression test around TestAssignmentModal that simulates changing classId during the same session and verifies saveTestAssignment uses the currently rendered teacherKey for each student. Update the existing TestAssignmentModal test suite to re-render with a new classId, then assert the saved assignments still match the visible per-student links from decodeTestAssignment and that each row uses the refreshed teacherKey rather than a stale one.src/components/Tests/TestAssignmentModal.tsx (1)
28-113: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftSwitching the class dropdown back and forth silently un-syncs DB-mode links from persisted rows.
teacherKeysregenerates freshnanoid()s wheneverclassStudentsgets a new array reference — which happens on everyclassIdchange, even re-selecting a class already visited (sinceuseMemoonly remembers the last computation, not per-input history). MeanwhilesavedStudentIdstracks raw student ids and is never reset, sohandleSaveAllToDb'sif (nowSaved.has(s.id)) continue;skips re-saving a student the first time their id was recorded — even though theirteacherKeyhas since changed. Net effect: the link shown/copied for that student (buildUrl) points at atest_assignmentsrow that was never actually persisted, so the student's link fails to resolve. The saved-count/disabled-button UI is similarly thrown off sincesavedStudentIds.sizeaccumulates across every class visited in the session, not just the current one.Track "saved" state by the actual
teacherKey(not raw student id), and/or maketeacherKeysstable across class switches by keying off the fullstudentslist instead of the filteredclassStudents.🐛 Sketch of a fix
- const teacherKeys = useMemo(() => { - const map: Record<string, string> = {}; - classStudents.forEach((s) => { - map[s.id] = nanoid(); - }); - return map; - }, [classStudents]); + const teacherKeys = useMemo(() => { + const map: Record<string, string> = {}; + students.forEach((s) => { + map[s.id] = nanoid(); + }); + return map; + }, [students]);- const [savedStudentIds, setSavedStudentIds] = useState<Set<string>>(new Set()); + const [savedTeacherKeys, setSavedTeacherKeys] = useState<Set<string>>(new Set()); ... const handleSaveAllToDb = useCallback(async () => { - const nowSaved = new Set(savedStudentIds); + const nowSaved = new Set(savedTeacherKeys); for (const s of classStudents) { - if (nowSaved.has(s.id)) continue; + const key = teacherKeys[s.id]; + if (nowSaved.has(key)) continue; ... - nowSaved.add(s.id); + nowSaved.add(key); } - setSavedStudentIds(nowSaved); + setSavedTeacherKeys(nowSaved);Want me to draft the full patch (including the disabled/count UI) and add a regression test that toggles
classIdA→B→A and assertssaveTestAssignmentwas called with the currently-displayedteacherKeyfor each student?🤖 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 `@src/components/Tests/TestAssignmentModal.tsx` around lines 28 - 113, The DB-mode save tracking in TestAssignmentModal is keyed by student id, but teacherKeys are regenerated when classStudents changes, so switching classes can leave the UI marked as saved while the current buildUrl points to an unpersisted row. Update the saving logic in handleSaveAllToDb and the saved state to track the actual teacherKey (or make teacherKeys stable across class switches using the full students list), and ensure the saved-count/disabled state reflects only the current class’s assignments rather than accumulating stale ids.
🤖 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 `@src/components/Tests/TestAssignmentModal.tsx`:
- Around line 76-103: The `handleSaveAllToDb` flow in `TestAssignmentModal` is
saving each student assignment sequentially because `saveTestAssignment` is
awaited inside the loop. Refactor this logic to build the pending saves for
`classStudents` (skipping `savedStudentIds`) and execute them in parallel with
`Promise.all` or `Promise.allSettled`, then aggregate successes/failures to
update `savedStudentIds`, `saveErrorCount`, and `saving` consistently. Keep the
existing `TestAssignment` construction and preserve the current error-count
behavior while removing the one-by-one network round trips.
In `@src/pages/__tests__/StudentPortalPage.test.tsx`:
- Around line 82-95: The radar test fixture only grades criterion c1, so the “3+
criteria have been graded” case is not actually exercised. Update the
StudentPortalPage.test fixture to add scored entries for c2 and c3 alongside c1,
using the existing rubric definitions and the radar data setup so the test
validates multiple graded criteria instead of missing/zero-filled entries.
In `@src/pages/StudentPortalPage.tsx`:
- Line 532: Remove the redundant JSX section comments in StudentPortalPage that
only restate the visible markup, including the “My Work: combined essay + test
to-do list” comment and the similar comment at the later section. Keep the JSX
structure and identifiers as-is, but delete comments that simply describe what
the headings/section names already communicate.
- Around line 268-288: The handleOpenTest flow in StudentPortalPage leaves the
loading state stuck if fetchAssignedTestContent rejects because
setOpeningTestKey(null) is only reached on success. Update handleOpenTest to
reset the opening state in a failure path as well, preferably by wrapping the
fetchAssignedTestContent call in try/catch or try/finally so
setOpeningTestKey(null) always runs and setTestOpenErrorKey(row.teacherKey) is
set when loading content fails.
- Around line 989-992: The button in StudentPortalPage’s row action should not
default to form submission; update the button used with onOpen(row) so it
explicitly uses a plain button type. Locate the button in the StudentPortalPage
component and set its type so it won’t submit if rendered inside a form, while
keeping the existing onClick and disabled behavior unchanged.
- Around line 212-219: The per-rubric radar selection in selectedRadarData only
uses the first matching history entry for a rubric, so later attempts are
dropped. Update the useMemo logic to collect all history rows matching
radarRubricId, then compute each criterion’s average across those rows before
building the CriterionRadarDataPoint list. Use selectedRadarData, history, and
criterionPct as the key symbols when adjusting this aggregation.
In `@supabase/migrations/044_test_assignments.sql`:
- Around line 19-29: The `tests_student_select` policy currently allows access
through `test_assignments` without verifying the test’s owner. Update the policy
to require that `test_assignments.owner_id` matches `tests.owner_id` when
joining assignments to tests, so only the assignment creator’s tests can be
exposed. Use the `tests_student_select` policy and the `test_assignments`
relationship in the migration to locate the change.
---
Outside diff comments:
In `@src/components/Tests/__tests__/TestAssignmentModal.test.tsx`:
- Around line 71-115: Add a regression test around TestAssignmentModal that
simulates changing classId during the same session and verifies
saveTestAssignment uses the currently rendered teacherKey for each student.
Update the existing TestAssignmentModal test suite to re-render with a new
classId, then assert the saved assignments still match the visible per-student
links from decodeTestAssignment and that each row uses the refreshed teacherKey
rather than a stale one.
In `@src/components/Tests/TestAssignmentModal.tsx`:
- Around line 28-113: The DB-mode save tracking in TestAssignmentModal is keyed
by student id, but teacherKeys are regenerated when classStudents changes, so
switching classes can leave the UI marked as saved while the current buildUrl
points to an unpersisted row. Update the saving logic in handleSaveAllToDb and
the saved state to track the actual teacherKey (or make teacherKeys stable
across class switches using the full students list), and ensure the
saved-count/disabled state reflects only the current class’s assignments rather
than accumulating stale ids.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dec81820-fa9c-403a-9871-fa080cbc8f06
📒 Files selected for processing (18)
README.mdsrc/components/Tests/TestAssignmentModal.tsxsrc/components/Tests/__tests__/TestAssignmentModal.test.tsxsrc/context/AppContext.tsxsrc/locales/de.jsonsrc/locales/en.jsonsrc/locales/es.jsonsrc/locales/fr.jsonsrc/locales/nl.jsonsrc/pages/DocsPage.tsxsrc/pages/LandingPage.tsxsrc/pages/StudentPortalPage.tsxsrc/pages/__tests__/StudentPortalPage.test.tsxsrc/services/database/StorageSync.tssrc/services/database/SupabaseAdapter.tssrc/types/index.tssupabase/CLAUDE.mdsupabase/migrations/044_test_assignments.sql
- security (critical): tests_student_select RLS policy didn't verify test_assignments.owner_id matched tests.owner_id, letting an assignment row leak another teacher's test content to a student - critical (found outside diff): TestAssignmentModal regenerated teacherKeys on every class-dropdown switch while savedStudentIds tracked by student id, silently un-syncing displayed links from persisted rows on a class revisit; keyed teacherKeys off the full students list instead so they stay stable across class switches - perf: parallelized per-student saveTestAssignment calls with Promise.all instead of a sequential await loop - correctness: per-rubric radar only used the first graded attempt of a rubric, dropping later re-grades; now averages every matching attempt per criterion, same as the combined view - robustness: handleOpenTest left the card stuck in a loading state if fetchAssignedTestContent rejected; wrapped in try/catch/finally - test quality: radar fixture only graded one of three criteria despite the test asserting "3+ criteria graded"; added real entries for all three - style: removed two comments that only restated their JSX section's heading, and added an explicit button type Added regression tests for the two critical/major bugs (class-switch teacherKey stability, per-rubric radar aggregation). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/StudentPortalPage.tsx (1)
317-320: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFilter fetched work rows to the portal student.
fetchMyTestAssignments()can return all rows allowed by the current session; under teacher preview that can include assignments for other students. SinceallWorkdoes not filter bystudent.id, Alice’s portal can show Bob’s test rows. Filter both assignment arrays before mapping.Proposed fix
const allWork: WorkEntry[] = [ - ...essayRows.map((row) => ({ kind: 'essay' as const, key: `essay-${row.teacherKey}`, row })), - ...testRows.map((row) => ({ kind: 'test' as const, key: `test-${row.teacherKey}`, row })), + ...essayRows + .filter((row) => row.studentId === student.id) + .map((row) => ({ kind: 'essay' as const, key: `essay-${row.teacherKey}`, row })), + ...testRows + .filter((row) => row.studentId === student.id) + .map((row) => ({ kind: 'test' as const, key: `test-${row.teacherKey}`, 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 `@src/pages/StudentPortalPage.tsx` around lines 317 - 320, The student portal work list is assembling rows without restricting them to the current student, so other students’ assignments can appear in `allWork`. Update `StudentPortalPage` to filter both `essayRows` and `testRows` by `student.id` before the existing `map` calls that build the `WorkEntry[]`, keeping the `allWork` construction localized to the current portal student.
🤖 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 `@src/components/Tests/TestAssignmentModal.tsx`:
- Around line 85-106: The batch save in TestAssignmentModal’s save flow can
leave the modal stuck in the saving state if any saveTestAssignment call rejects
before Promise.all completes. Update the async block that builds results and
calls setSaving so it uses try/finally around the batch, and switch the
Promise.all handling to Promise.allSettled (or equivalent rejection-safe logic)
so rejected saves are counted in errors while still updating saved IDs for
successful entries and always clearing saving at the end.
- Around line 83-106: The current save flow in TestAssignmentModal uses
savedStudentIds as a global skip list, which causes stale rows and incorrect
progress when the class changes or assignment metadata like expiresAt changes.
Update the auto-save logic around the Promise.all block to track a per-student
payload fingerprint (or otherwise invalidate/re-save when test assignment fields
change) so pending saves are based on the current assignment payload, not only
student IDs. Also keep savedStudentIds for progress display, but derive the
shown saved count from the current classStudents list intersected with that set
so the modal reflects the active class correctly.
In `@src/pages/__tests__/StudentPortalPage.test.tsx`:
- Around line 359-373: The regression test in StudentPortalPage.test.tsx only
verifies that the rubric view renders axis labels, so it would still pass with
the first-attempt-only bug. Mock CriterionRadarChart so the test can inspect its
data prop after selecting rubric r1 in renderAt('s1'). Then assert a criterion
value that can only be correct if both sr1 and sr2 are included in the average,
using the existing mocked student rubric data to locate the affected behavior in
the per-rubric radar flow.
---
Outside diff comments:
In `@src/pages/StudentPortalPage.tsx`:
- Around line 317-320: The student portal work list is assembling rows without
restricting them to the current student, so other students’ assignments can
appear in `allWork`. Update `StudentPortalPage` to filter both `essayRows` and
`testRows` by `student.id` before the existing `map` calls that build the
`WorkEntry[]`, keeping the `allWork` construction localized to the current
portal student.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a9163d0e-55e7-4bed-a1c5-c240a1f982d5
📒 Files selected for processing (5)
src/components/Tests/TestAssignmentModal.tsxsrc/components/Tests/__tests__/TestAssignmentModal.test.tsxsrc/pages/StudentPortalPage.tsxsrc/pages/__tests__/StudentPortalPage.test.tsxsupabase/migrations/044_test_assignments.sql
- major: savedStudentIds tracked save state globally across every
class visited this session, so the "saved N/total" display and the
save-button's disabled state read against the lifetime count instead
of the current class (e.g. "3/1 saved" after visiting a 2-student
then a 1-student class). Derive the displayed/disabled count from
classStudents intersected with the saved set instead.
- major: changing the deadline after TestAssignmentModal's initial
auto-save left already-persisted rows silently stuck on their
original expiry, since the skip check only looked at student id.
Re-keyed saved-state tracking to `${studentId}::${expiresAt}` so a
changed deadline is treated as a new payload and re-saved, and added
expiresAt to the auto-save effect's dependency array.
- major: a single rejected saveTestAssignment call would abort the
whole Promise.all batch, leaving the modal stuck in the saving
state. Switched to Promise.allSettled (rejections count as failures
rather than aborting the batch) wrapped in try/finally.
- minor: the per-rubric radar regression test only checked that
criteria rendered as axis labels, which would still pass under the
original bug. Mocked CriterionRadarChart to expose its data prop and
assert the actual averaged value (80%, not the single-attempt 90%).
Added a saved-count assertion to the existing class-switch regression
test to cover the first fix directly.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fetchMyEssayAssignments()/fetchMyTestAssignments() are scoped by the authenticated session's email via get_my_student_ids() (RLS), which can resolve to more than one Student record if two students share a login email (siblings, a roster data-entry mistake) — the portal page was trusting the fetch already matched the URL's studentId and never filtered client-side, so it could show one student's work on another's portal page. Filter both essayRows/testRows to student.id before building allWork. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
Re: "Filter fetched work rows to the portal student" (posted as an outside-diff-range finding, so it has no dedicated review thread to reply on inline) — fixed in 🤖 Addressed by Claude Code |
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
Why a new
test_assignmentstableTests had no persisted assignment record — a teacher-generated share link was the only way a student ever learned a test existed, so there was nothing to list. Added
supabase/migrations/044_test_assignments.sql:test_assignmentstable mirroringessay_assignments' proven RLS pattern (get_my_test_assignment_ids(), portal-scoped via student email → local student ID).student_testsso the portal can show a student's own submission status.testsso the portal can embed full test content into a self-contained "Open" link (the same offline/embedded-content URL shapeTestAssignmentModalalready produces), sidesteppingStudentTestPage's separate disconnected client.Bug fix bundled in
TestAssignmentModalshared a singleteacherKeyacross an entire class batch — a "DB mode" link would have pointed every student at the same row. Now mints oneteacherKey/DB row per student, matchingessay_assignments' 1:1-per-teacherKey constraint. Verified in the browser that all 9 generated links now decode to distinctteacherKeys.Docs
Updated per
CLAUDE.md's documentation-maintenance rule:DocsPage.tsx(new "Student Portal" section),README.md,LandingPage.tsx(new student feature card), and all 5 locale files (0 missing/extra key parity verified).Deliberately out of scope
StudentTestPage's own share-link flow still can't submit/fetch via DB mode — it opens a disconnected Supabase client and never authenticates, a pre-existing bug unrelated to this feature (the portal's "Open" button sidesteps it by embedding content directly). Planned as its own follow-up PR — full technical plan written into the wiki roadmap under "Deferred from Phase 14" (mirrors the essay flow'ssignInAnonymously()+get-essay-assignment/submit-essayedge-function pattern).Test plan
npm run typecheck— cleannpm run lint— clean (only pre-existing warnings elsewhere, unrelatede2e/fixtureserrors pre-date this branch)npm test— 2105/2105 tests pass (4 new)teacherKeyssupabase db resetto verify migration 044 applies cleanly — not run, no Docker daemon available in this sandbox; please verify locally before merging🤖 Generated with Claude Code
Summary by CodeRabbit