Phase 8: Collaboration & Department Workflow#160
Merged
Conversation
Adds the four Phase 8 roadmap items: co-grading/moderation (8.1, reuses the peer-review data model end-to-end), department rubric/comment-bank sharing (8.2, live read-only visibility via schools/school_members RLS), grading task assignment (8.3, batch-assign ungraded submissions to a colleague from the Activity Dashboard), and drag-to-reorder rubrics/tests/essays (8.4, via the existing @hello-pangea/dnd pattern from RubricBuilder). Includes Supabase migrations for school sharing and grading tasks, i18n across all 5 locales, doc updates (DocsPage/README/LandingPage), and new Playwright e2e coverage (23, 25, 26 verified locally across chromium/ firefox/webkit; 24 is Supabase-backed and CI-only, same constraint as the existing 14/15/16/17/18/20 specs). Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 34 minutes and 59 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements Phase 8 of RubricMaker, adding: (1) department-wide read-only rubric and comment-bank sharing via new Supabase RLS policies; (2) a co-grading second-marker workflow with a new ChangesPhase 8: Co-grading, Department Sharing, Grading Tasks & Reordering
Sequence Diagram(s)sequenceDiagram
rect rgba(100, 149, 237, 0.5)
Note over Teacher,ModerationQueuePage: Co-grading workflow
end
participant Teacher
participant GradeStudent
participant PeerReviewPage
participant ModerationQueuePage
participant getModerationQueue
participant AppContext
Teacher->>GradeStudent: click "Co-grade" (existingSR present)
GradeStudent->>GradeStudent: open modal, enter colleague name
GradeStudent->>PeerReviewPage: navigate /peer-review/?reviewerId=colleague
PeerReviewPage->>AppContext: save peer-review StudentRubric (isPeerReview=true, gradedBy=colleague)
Teacher->>ModerationQueuePage: open /moderation, set threshold
ModerationQueuePage->>getModerationQueue: rubrics, peerReviews, students, threshold
getModerationQueue-->>ModerationQueuePage: ModerationQueueItem[] sorted by totalAbsDelta desc
Teacher->>ModerationQueuePage: keepOriginal
ModerationQueuePage->>AppContext: deleteStudentRubric(secondMarkerId)
Teacher->>ModerationQueuePage: acceptSecondMarker
ModerationQueuePage->>AppContext: updateStudentRubric(baseline with second-marker data)
ModerationQueuePage->>AppContext: deleteStudentRubric(secondMarkerId)
sequenceDiagram
rect rgba(144, 238, 144, 0.5)
Note over Teacher,SupabaseDB: Grading task assignment and persistence
end
participant Teacher
participant ActivityDashboard
participant AppContext
participant StorageSync
participant SupabaseDB
Teacher->>ActivityDashboard: click Assign on activity row
ActivityDashboard->>ActivityDashboard: openAssignTaskModal, fill teacher + due date
Teacher->>ActivityDashboard: confirm assign
ActivityDashboard->>AppContext: addGradingTasks(tasks[]) for ungraded students
AppContext->>AppContext: dispatch ADD_GRADING_TASKS → localStorage
AppContext->>StorageSync: pushOne('gradingTask', 'upsert', task) per task
StorageSync->>SupabaseDB: upsert grading_tasks row
ActivityDashboard->>ActivityDashboard: render pendingTasks card
Teacher->>ActivityDashboard: delete pending task
ActivityDashboard->>AppContext: deleteGradingTask(id)
AppContext->>StorageSync: pushOne('gradingTask', 'delete', null, id)
StorageSync->>SupabaseDB: delete grading_tasks row
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
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)
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 |
![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: 22
🤖 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 `@e2e/fixtures/supabase.fixture.ts`:
- Line 365: The fixture callback in the supabase.fixture.ts file is triggering
lint violations due to the empty destructuring pattern and the parameter name
`use`. Rename the callback parameters: replace the empty destructuring pattern
`{}` with either an underscore `_` or a descriptive name, and rename the `use`
parameter to something like `useFixture` or `utilizeFixture` to avoid triggering
the React hooks lint rule. Apply this same fix to both occurrences in the
fixture file (lines 365 and 389).
- Around line 174-193: The Promise.all block containing the three fetch calls to
school_members, profiles, and user_settings endpoints does not validate the
response status codes. Add response validation for each fetch call by checking
the ok property or status code of each response, and throw an error if any
request fails so that fixture setup halts immediately rather than continuing
with potentially inconsistent backend state.
In `@src/components/Comments/CommentBankModal.tsx`:
- Around line 238-254: The button element in the share-with-department
functionality (the one that toggles sharedWithSchool and displays the Users2
icon) is missing an explicit type attribute. Add type="button" to this button
element to prevent accidental form submission and align with HTML best
practices, even if there is no form ancestor currently.
In `@src/context/AppContext.tsx`:
- Around line 571-574: In the ADD_GRADING_TASKS case, the current implementation
blindly appends incoming tasks to the existing gradingTasks array, which creates
duplicates when a task with the same id already exists in local state while the
cloud sync uses upsert semantics (one record per id). Instead of using
[...state.gradingTasks, ...action.payload], implement id-based upsert logic by
creating a map or filter-based approach that updates existing tasks by id or
adds new ones, ensuring local state maintains one record per task id and stays
consistent with the cloud sync behavior.
In `@src/pages/ActivityDashboardPage.tsx`:
- Around line 657-672: The label elements in the teacher assignment and due date
input fields are not programmatically associated with their corresponding
inputs, which impacts accessibility for screen readers. Add htmlFor attributes
to both labels (for the teacher name label with gradingTasks.teacher_label
translation and the due date label with gradingTasks.due_date_label translation)
and add matching id attributes to their corresponding input elements (the input
with assignTeacherName value and the input with assignDueDate value) to create
the proper association between labels and form controls.
- Around line 72-91: The handleAssignTasks function filters for ungraded
students but only excludes completed grades from studentRubrics, not
already-assigned pending tasks. When the modal reopens and assign is clicked
again, duplicate tasks are created with new IDs for the same student/rubric
pair. Expand the ungraded filter condition to also check for existing grading
tasks (pending assignments) for the same rubricId and studentId combination, in
addition to the existing studentRubrics check. This ensures that students with
both completed grades and already-pending assignments are excluded before
calling addGradingTasks.
- Around line 158-163: In the handleDragEnd function, the code derives the
destination kind from result.destination.droppableId but uses
result.source.index which refers to the item's position in the source section's
ordering. When a user drags across different sections (e.g., from rubric to
test), these indexes are misaligned, causing reorderDisplayOrder to corrupt the
operation. Extract the source kind from result.source.droppableId by applying
the same replace logic used for the destination, then compare both kinds to
ensure they match before proceeding with the reorder logic. If the kinds differ,
return early to reject the cross-section drop.
In `@src/pages/DocsPage.tsx`:
- Around line 80-81: The hardcoded English text in the `description` field and
similar user-visible strings in DocsPage.tsx (at the referenced line ranges)
must be moved to i18n keys. First, add appropriate translation keys to
src/locales/en.json for each hardcoded string. Then, ensure the useTranslation
hook is imported in DocsPage.tsx, and replace all hardcoded English strings with
t(key_name) calls referencing the new i18n keys. Apply this consistently across
all mentioned line ranges (267-276, 715-734, 869-889, 1127-1139) to maintain
multilingual behavior.
In `@src/pages/EssayListPage.tsx`:
- Around line 140-157: The button elements with className "btn btn-ghost
btn-icon btn-sm" containing the Edit2 icon (for navigation) and the Trash2 icon
(for handleDelete), as well as the additional button at line 185, are missing
explicit type attributes. Add type="button" to all three button elements to
prevent them from defaulting to type="submit" and triggering unintended form
submission behavior when used within form contexts.
- Around line 34-38: The handleDragEnd function needs an additional guard
condition to prevent unnecessary updateEssayGroup calls when an item is dropped
at the same position it was dragged from. After the existing check for missing
destinations, add a condition to return early if result.source.index equals
result.destination.index. This will prevent the reorderDisplayOrder function
from being called and avoid dispatching redundant updates when no actual
reordering has occurred.
In `@src/pages/GradeStudent.tsx`:
- Around line 1686-1693: The label with text coGrading.colleague_label is not
semantically linked to the input element that sets coGraderName, making it
inaccessible to screen readers. Add a unique id attribute to the input element
and then add a corresponding htmlFor attribute to the label element that matches
that id. This will ensure assistive technologies properly announce the field
name when the input is focused.
- Around line 1702-1705: The navigate call in GradeStudent.tsx uses free-text
coGraderName.trim() as the reviewerId parameter, which creates inconsistent
reviewer identities due to minor variations in how names/emails are entered.
Replace the coGraderName value with a stable, unique identifier for the reviewer
(such as a user ID or account identifier) that remains consistent regardless of
how the user's name or email is formatted. This ensures co-grade history and
moderation matching work correctly without fragmenting records based on text
variations.
In `@src/pages/LandingPage.tsx`:
- Around line 95-100: The feature card object containing the Department
Collaboration feature has hardcoded English text in the title and desc
properties, as well as a hardcoded hex color value. Extract the title
"Department Collaboration" and the description text to i18n translation keys
using the useTranslation hook pattern followed elsewhere in the file, replace
the hardcoded color '`#f59e0b`' with an appropriate global CSS custom property
instead of the hex value, and shorten the description to be concise and
user-focused rather than detailed and verbose.
In `@src/pages/ModerationQueuePage.tsx`:
- Around line 37-44: The label element displaying the threshold label text is
not programmatically associated with the input element of type number. Add a
unique id attribute to the number input element (the one with min={0},
step={0.5}, value={threshold}), and then add an htmlFor attribute to the label
element with the same id value to create the proper association. This will
ensure screen readers correctly announce the label when the input receives
focus.
- Around line 24-29: The resolveAcceptSecondMarker function is incomplete when
copying data from the second marker to the baseline. In the saveStudentRubric
call, you are spreading baseline properties and overriding entries and
overallComment from secondMarker, but you are missing the globalModifier
property. Add globalModifier from secondMarker to the object passed to
saveStudentRubric to ensure the accepted baseline grade remains consistent with
the accepted review, similar to how you are handling entries and overallComment.
- Around line 133-153: The three buttons in the ModerationQueuePage.tsx file are
missing explicit type attributes, which causes them to default to type="submit"
and can trigger accidental form submissions. Add type="button" to each of the
three buttons: the one with onClick navigating to the rubrics grading page, the
one calling resolveKeepBaseline, and the one calling resolveAcceptSecondMarker.
This ensures these action buttons do not trigger form submission behavior.
In `@src/pages/RubricList.tsx`:
- Around line 721-733: The clickable div element that navigates to
`/rubrics/${r.id}` on the onClick event lacks keyboard accessibility. Add the
`role="button"` attribute to semantically indicate this div is clickable, add
`tabIndex={0}` to make it keyboard focusable, and implement an `onKeyDown`
handler that triggers the same navigation logic when the Enter or Space key is
pressed. This ensures keyboard users can interact with the department-shared
rubric items the same way mouse users can.
In `@src/pages/TestListPage.tsx`:
- Around line 203-235: Add an explicit type="button" attribute to all button
elements in TestListPage.tsx that are currently missing it. This applies to the
action buttons including the duplicate button with the Copy icon, the edit
button with the Edit2 icon, and the delete button with the Trash2 icon, as well
as all other button elements mentioned in the file (lines 203, 214, 225, 275,
281, 288, 303, 309, 342, 439, 453). Without the explicit type attribute, buttons
default to type="submit" which can cause accidental form submission if these
controls are placed inside a form boundary.
- Around line 37-41: In the handleDragEnd function, add an early return check
after verifying the destination exists but before processing the reorder. Check
if the source index equals the destination index, and if they match, return
immediately since the item was dropped in its original position. This prevents
unnecessary iteration through the reordered list and avoids triggering
updateTest calls when the display order hasn't actually changed.
In `@src/store/storage.ts`:
- Around line 515-518: The validation for gradingTasks in the importFullBackup
function uses only isObjectArray() which checks if data is an object array but
does not validate the actual shape or required properties of each GradingTask
(such as required IDs and timestamps). Replace or enhance the isObjectArray
check with a more thorough validation function that verifies each item in the
array conforms to the GradingTask schema by checking for required fields. Only
call saveGradingTasks if all items pass this stricter shape validation,
otherwise log a warning and skip restoration as currently done.
In `@src/utils/activityDashboardAggregator.ts`:
- Line 23: The inline comment at line 23 in the activityDashboardAggregator.ts
file describes the implementation behavior (grouping essays by teacherKey and
using the first assignment's properties) rather than explaining the reasoning
behind this approach. Either remove the comment entirely since the code is
self-explanatory through its identifiers and logic, or rewrite it to explain the
business rationale or design decision for why this grouping and property
selection strategy is necessary.
In `@supabase/migrations/041_school_sharing.sql`:
- Around line 12-13: The RLS policies are using unsafe JSONB casting that can
cause query failures if malformed data exists. In both the rubrics_school_select
and comment_bank_school_select policies, replace the unsafe boolean cast
(data->>'sharedWithSchool')::boolean IS TRUE with a non-throwing JSONB predicate
that safely handles malformed values. Use the JSONB contains operator (@>) or a
CASE statement to check for the true value without throwing errors on invalid
data, ensuring the entire query doesn't fail just because one row has an invalid
sharedWithSchool value. Apply this same change at both locations mentioned in
the diff.
🪄 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: 4b6ec5db-e424-4d47-a3e1-7285ba4e7f6d
📒 Files selected for processing (43)
README.mde2e/fixtures/supabase.fixture.tse2e/pages/ActivityDashboardPage.tse2e/pages/BasePage.tse2e/pages/EssayListPage.tse2e/pages/ModerationQueuePage.tse2e/pages/RubricListPage.tse2e/pages/TestListPage.tse2e/specs/23-co-grading-moderation.spec.tse2e/specs/24-department-sharing.spec.tse2e/specs/25-grading-task-assignment.spec.tse2e/specs/26-list-reordering.spec.tsplaywright.config.tssrc/App.tsxsrc/components/Comments/CommentBankModal.tsxsrc/components/Layout/Sidebar.tsxsrc/components/__tests__/components.smoke.test.tsxsrc/context/AppContext.tsxsrc/locales/de.jsonsrc/locales/en.jsonsrc/locales/es.jsonsrc/locales/fr.jsonsrc/locales/nl.jsonsrc/pages/ActivityDashboardPage.tsxsrc/pages/DocsPage.tsxsrc/pages/EssayListPage.tsxsrc/pages/GradeStudent.tsxsrc/pages/LandingPage.tsxsrc/pages/ModerationQueuePage.tsxsrc/pages/RubricList.tsxsrc/pages/TestListPage.tsxsrc/pages/__tests__/pages-phase4.a11y.test.tsxsrc/services/database/StorageSync.tssrc/services/database/SupabaseAdapter.tssrc/store/storage.tssrc/types/index.tssrc/utils/activityDashboardAggregator.tssrc/utils/coGradingModerationQueue.test.tssrc/utils/coGradingModerationQueue.tssrc/utils/displayOrder.test.tssrc/utils/displayOrder.tssupabase/migrations/041_school_sharing.sqlsupabase/migrations/042_grading_tasks.sql
- e2e/fixtures/supabase.fixture.ts: validate the three school-linking write responses before returning the colleague magic link; fix no-empty-pattern/ rules-of-hooks lint violations in the new fixture callbacks - AppContext: ADD_GRADING_TASKS now upserts by id instead of blind-appending - ActivityDashboardPage: exclude already-pending tasks (not just graded ones) when assigning; guard handleDragEnd against cross-section/no-op drops; associate modal labels with their inputs - EssayListPage/TestListPage: guard handleDragEnd against no-op drops; add explicit type="button" to action buttons - GradeStudent/ModerationQueuePage: associate labels with inputs; add type="button"; copy globalModifier when accepting a second marker's grade - RubricList: keyboard support (role/tabIndex/onKeyDown) for the department-shared rubric list items - CommentBankModal: add type="button" to the department-share toggle - storage.ts: validate GradingTask shape (not just "is an object array") on backup restore - 041_school_sharing.sql: use non-throwing jsonb equality instead of a boolean cast in the RLS policies - activityDashboardAggregator.ts: drop a comment that narrated the code Two flagged items left as-is with an explanation on the thread: the co-grader identity stays free-text (matches the existing gradedBy heuristic, already documented as a known ceiling; a stable-identity picker needs a teacher directory that doesn't exist yet), and DocsPage/LandingPage hardcoded English strings, which match the existing convention in those files everywhere else. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced Jun 22, 2026
Playwright statically parses a fixture callback's first parameter to detect
declared dependencies, requiring it to be a destructuring pattern (even an
empty {}) — renaming it to a plain identifier (_fixtures) in the previous
commit broke fixture resolution at runtime ("First argument must use the
object destructuring pattern"), failing the whole supabase e2e project in CI.
Restored {} for colleagueEmail's first param and suppressed the no-empty-
pattern lint rule there instead, with a comment explaining why. The use->run
rename (second param) was unaffected and stays.
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…rmat)
The colleaguePage fixture's user_settings only had {theme, language,
hasSeenTutorial} — missing defaultFormat (and a few other fields)
crashed the colleague's app on hydration whenever a page touching
rubric format rendered, which is exactly what 24-department-sharing.spec.ts
hits when the colleague navigates to /rubrics. createUserAndGetMagicLink's
own comment already warned about this; the new fixture just hadn't followed
it.
Extracted the settings payload into a shared FULL_TEST_SETTINGS constant
used by both createUserAndGetMagicLink and createColleagueAndGetMagicLink
so they can't drift apart again.
Verified via `playwright test --project=supabase --list` (parses cleanly)
and the full local chromium e2e suite (99/99); the supabase project itself
still can't run locally (no Docker), so CI is the next real signal.
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pages/RubricList.tsx (2)
721-741: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider using
<button>instead ofrole="button"for native semantics.The keyboard accessibility fix is correct. The static analysis tool suggests that
<button>provides more reliable semantics for screen readers than a<div>withrole="button". This is optional since the current implementation works correctly.♿ Optional: native button approach
{schoolShared.map((r) => ( - <div + <button key={r.id} - role="button" - tabIndex={0} style={{ padding: '12px 14px', background: 'var(--bg-elevated)', borderRadius: 10, border: '1px solid var(--border)', display: 'flex', alignItems: 'center', gap: 12, cursor: 'pointer', + width: '100%', + textAlign: 'left', }} onClick={() => navigate(`/rubrics/${r.id}`)} - onKeyDown={(e) => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault(); - navigate(`/rubrics/${r.id}`); - } - }} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/RubricList.tsx` around lines 721 - 741, Replace the `<div>` element with `role="button"` with a native `<button>` element to improve screen reader semantics. Remove the `role="button"` attribute since it is now redundant, and keep all the styling applied to the button. The onKeyDown handler can be simplified since native buttons automatically handle Enter and Space key presses to trigger onClick, though keeping it does not hurt. Make sure the button still receives the same onClick handler that calls navigate and retains the tabIndex={0} if needed for focus management.Source: Linters/SAST tools
266-266:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHardcoded English strings violate i18n guidelines.
Multiple user-visible strings in the topbar actions and button titles are hardcoded instead of using
t():
- Line 266:
"Import from code"- Line 423:
"Copy share code (for other teachers)"- Line 443:
"Share preview with students (copy link)"Similar issues exist in the code import modal (lines 901, 912–913, 922, 948, 955) and differentiate modal (line 876).
As per coding guidelines: "All user-visible strings must use the
useTranslationhook and a key fromsrc/locales/en.json. Never hardcode English text in JSX."Also applies to: 423-423, 443-443
🤖 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/RubricList.tsx` at line 266, Replace all hardcoded English strings with translation keys using the t() function from the useTranslation hook in RubricList.tsx. Specifically, wrap the strings "Import from code", "Copy share code (for other teachers)", and "Share preview with students (copy link)" with t() calls, and also apply this to the hardcoded strings in the code import modal and differentiate modal sections. For each hardcoded string, add a corresponding translation key to the locales configuration file following the naming convention used in the existing keys. Ensure the useTranslation hook is already imported and available in the component before making these changes.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/pages/RubricList.tsx`:
- Around line 721-741: Replace the `<div>` element with `role="button"` with a
native `<button>` element to improve screen reader semantics. Remove the
`role="button"` attribute since it is now redundant, and keep all the styling
applied to the button. The onKeyDown handler can be simplified since native
buttons automatically handle Enter and Space key presses to trigger onClick,
though keeping it does not hurt. Make sure the button still receives the same
onClick handler that calls navigate and retains the tabIndex={0} if needed for
focus management.
- Line 266: Replace all hardcoded English strings with translation keys using
the t() function from the useTranslation hook in RubricList.tsx. Specifically,
wrap the strings "Import from code", "Copy share code (for other teachers)", and
"Share preview with students (copy link)" with t() calls, and also apply this to
the hardcoded strings in the code import modal and differentiate modal sections.
For each hardcoded string, add a corresponding translation key to the locales
configuration file following the naming convention used in the existing keys.
Ensure the useTranslation hook is already imported and available in the
component before making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c085d818-96dc-4ad5-bc86-76ee195eae7d
📒 Files selected for processing (12)
e2e/fixtures/supabase.fixture.tssrc/components/Comments/CommentBankModal.tsxsrc/context/AppContext.tsxsrc/pages/ActivityDashboardPage.tsxsrc/pages/EssayListPage.tsxsrc/pages/GradeStudent.tsxsrc/pages/ModerationQueuePage.tsxsrc/pages/RubricList.tsxsrc/pages/TestListPage.tsxsrc/store/storage.tssrc/utils/activityDashboardAggregator.tssupabase/migrations/041_school_sharing.sql
💤 Files with no reviewable changes (1)
- src/utils/activityDashboardAggregator.ts
Array.prototype.find() returns undefined (not null) when nothing matches, but the gate checked `linkedStudent !== null`. For any non-admin, non-student teacher with no linked student record, this evaluated to `undefined !== null && role !== 'admin'` = true, incorrectly routing them into the "No student account linked to this email" dead end instead of the normal teacher app. This was dormant in every existing e2e-supabase test because they only ever exercise an admin-role account (the first signup in a fresh DB always becomes admin) or the same account on a second device — never a genuine non-admin teacher session. The new colleaguePage fixture (24-department-sharing.spec.ts) is the first to do that, which is how this surfaced: the colleague got role='teacher' correctly, but then hit this gate and got stuck on the student-portal screen instead of /rubrics. Fix: coalesce the find() result to null so the check means what it says. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
… row The previous policy joined two school_members rows (the colleague's own + the rubric owner's) to confirm they're in the same school — mirroring marketplace_listings_select's join shape. But that policy only ever reads the QUERYING user's own school_members row; a second school_members row for someone else (the owner, looked up by a non-admin colleague) is exactly what school_members_select (migration 016) hides via RLS. So the EXISTS always evaluated false for anyone but the owner themselves — the colleague's fetchSchoolSharedRubrics() call kept returning [] even though the rubric was genuinely shared (confirmed via the CI trace: the owner's upsert with sharedWithSchool:true landed ~1s before the colleague's read, ruling out a timing race). Fixed by reading the owner's school_id off `profiles` instead, which any teacher/admin can already read in full (profiles_read_users_and_admins, migration 037) — so only the colleague's own school_members row needs to be visible, which it always is. Edited migration 041 in place rather than stacking a fix-up migration: it has only ever run in this PR's own ephemeral CI sandboxes, never in a shared/deployed environment. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
… list fetchRubrics() and fetchCommentBank() select with no owner filter, relying entirely on RLS to scope results. That was already latent for rubrics_shared_select (individual sharing, Phase 1) but never surfaced — RLS additively grants visibility, so once a row is visible via ANY policy it comes back from this same unscoped query and lands in state.rubrics/state.commentBank with full owner-only edit/delete controls, duplicated alongside the dedicated read-only "Shared with me"/"Shared with your department" sections that already query it separately. Migration 041's new rubrics_school_select/comment_bank_school_select policies made this newly reachable in a second way and is what the CI trace caught: the colleague's "My Rubrics" grid showed a fully-editable copy of the owner's shared rubric (Share/Duplicate/Edit/Delete buttons and all) right next to the correct read-only entry in "Shared with your department" — same row, two renderings, hence the e2e test's strict-mode locator violation on "Department Shared Rubric". Fixed by scoping both queries to owner_id = current user, matching what fetchSharedRubrics()/fetchSchoolSharedRubrics()/ fetchSchoolSharedCommentBank() already do as separate, explicit calls. Also tightened the e2e assertion to target the heading directly instead of a bare text match, now that there's only one row to find either way. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
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
Implements Phase 8 of the roadmap (Collaboration & Department Workflow), per the wiki roadmap's themes:
StudentRubricwithisPeerReview: true,gradedByset to the colleague's name/email).peerReviewAggregator.ts's existing delta math needed no changes since it's already generic over reviewer identity. Newsrc/utils/coGradingModerationQueue.tsflags disputes above a configurable point threshold; a "Co-grade" button on Grade Student opens the same Peer Review screen with a colleague as reviewer; a new/moderationpage lists disputes with a per-criterion delta and keep/accept resolution.sharedWithSchoolflag reused inside the existing rubric/comment-bankdatajsonb column (no new table), with a new RLS SELECT policy (041_school_sharing.sql) mirroring the Phase 6 marketplace's school_members join pattern. Unlike the Marketplace (clone a frozen snapshot), this shares the live rubric read-only with every teacher in the same school.GradingTasktype/table (042_grading_tasks.sql). From the Activity Dashboard, batch-assign a class's ungraded submissions for a rubric to a specific colleague. Completion is derived (no separate "done" flag) — a task disappears once a matching grade exists.src/utils/displayOrder.tshelper wired into RubricList, TestListPage, EssayListPage, and the Activity Dashboard via the existing@hello-pangea/dndpattern already used in RubricBuilder for criteria/levels.Also updates the wiki roadmap (pushed directly, separate repo) marking Phase 7 done and fleshing out 8.5 (cohort-based filtering) into a concrete "Proposed — not yet scheduled" plan, per CLAUDE.md's documentation-maintenance rule plus
DocsPage.tsx,README.md, andLandingPage.tsx. i18n keys added across all 5 locales (en/nl/fr/de/es).Test plan
npm run typecheck— cleannpm run lint— 0 errors (150 pre-existingno-explicit-anywarnings, +1 from a new test file matching existing style)npx prettier --check— clean on all touched/new filesnpm test— full unit suite green (1788 tests), including new tests fordisplayOrder.tsandcoGradingModerationQueue.ts23-co-grading-moderation.spec.ts(5 tests),25-grading-task-assignment.spec.ts(3 tests),26-list-reordering.spec.ts(3 tests, real@hello-pangea/dnddrag simulation) — all verified passing locally on chromium, firefox, and webkit, plus folded into the full existing chromium suite (99/99 passing) with no regressions24-department-sharing.spec.ts(2 tests) — requires a real Supabase stack (added acolleaguePagefixture tosupabase.fixture.tsfor a second distinct teacher in the same school). Not run locally (no Docker in this environment) — same constraint as the existing 14/15/16/17/18/20 specs; wired intoplaywright.config.ts'ssupabaseproject, runs in CI vianpm run e2e:supabaseReviewer notes
041_school_sharing.sqland042_grading_tasks.sqlhave not been applied against a live Supabase instance in this environment (no Docker available) — recommend runningnpm run db:resetbefore merge to confirm they apply cleanly.24-department-sharing.spec.tsis similarly unverified pending a CI run with the local Supabase stack.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes