Add grading assignment defaults and reminder automation#722
Open
jon-bell wants to merge 17 commits into
Open
Conversation
Co-authored-by: Jonathan Bell <jon@jonbell.net>
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughAdds class-scoped grading-assignment default profiles and per-assignment overrides, DB schema and RPCs for auto-assignment/late-reminder scheduling, UI to manage/apply profiles and form integration, TypeScript Supabase type updates, and E2E tests validating automation and email queuing. ChangesGrading automation (single cohesive change DAG)
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser as Client
participant AssignmentForm
participant GradingAutomationSubform
participant RefineAPI
participant Database
rect rgba(120,160,200,0.5)
User->>Browser: Open assignment create/edit page
Browser->>AssignmentForm: Render form
AssignmentForm->>GradingAutomationSubform: Render profile selector & controls
GradingAutomationSubform->>RefineAPI: List grading default profiles
RefineAPI->>Database: SELECT grading_assignment_default_profiles
Database-->>RefineAPI: Return profiles
RefineAPI-->>GradingAutomationSubform: Profiles loaded
User->>GradingAutomationSubform: Select profile / toggle apply
GradingAutomationSubform->>AssignmentForm: setValue(apply profile values)
User->>AssignmentForm: Submit
AssignmentForm->>AssignmentForm: Normalize grading fields
AssignmentForm->>RefineAPI: Create/Update assignment (include grading fields)
RefineAPI->>Database: INSERT/UPDATE assignments row
Database-->>RefineAPI: Confirm
RefineAPI-->>AssignmentForm: Success
AssignmentForm-->>User: Saved
end
sequenceDiagram
participant Scheduler as pg_cron
participant DB
participant Runner as run_assignment_grading_automation
participant AutoAssign as auto_assign_grading_reviews_for_assignment
participant QueueEmails as queue_late_grading_reminders_for_assignment
participant bulkAssign as bulk_assign_reviews
rect rgba(200,160,120,0.5)
Scheduler->>DB: Trigger run_assignment_grading_automation()
DB->>Runner: Execute
Runner->>DB: Find due/non-archived assignments with automation enabled
alt Auto-assign needed
Runner->>AutoAssign: Call with assignment_id
AutoAssign->>DB: Select submissions & eligible staff
AutoAssign->>bulkAssign: Call bulk_assign_reviews(...)
bulkAssign-->>AutoAssign: Assigned count
AutoAssign-->>Runner: Return count
end
alt Late reminders needed
Runner->>QueueEmails: Call with assignment_id
QueueEmails->>DB: Find recipients, insert emails & batch
DB-->>QueueEmails: Return queued count
QueueEmails-->>Runner: Return count
end
Runner->>DB: Upsert automation state (timestamps/counts)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
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. 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 |
Co-authored-by: Jonathan Bell <jon@jonbell.net>
Co-authored-by: Jonathan Bell <jon@jonbell.net>
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/course/[course_id]/manage/assignments/[assignment_id]/edit/page.tsx (1)
37-75:⚠️ Potential issue | 🟠 MajorEdits to grading-automation fields can be clobbered when the self-review query resolves.
This effect depends on
selfReviewSetting?.data.*, so it re-runs when the self-review query resolves — which is typically after the initialreset(queryData)in the effect above. Whenever that happens, all nineform.setValue(...)calls here re-fire and re-seed the grading-automation fields fromqueryData, overwriting any edits the user made in the meantime. The previous code only set three self-review fields, so the race was benign; now that seven auto-assign/reminder fields are driven from this same effect, the blast radius is much larger (e.g., a user toggles "Auto assign at deadline" off right after the page loads, and it flips back on whenselfReviewSettinglands).Two reasonable options:
- Move the grading-automation seeding into the first effect (which only depends on
queryData/reset) by passing the full shape toreset(...).- Guard this effect so the grading-automation branch only runs once per
queryDataload (e.g., a ref or a separate effect keyed only onqueryData).🛡️ Sketch of option 1
useEffect(() => { if (queryData) { - reset(queryData); + const qd = queryData as AssignmentFormValues; + reset({ + ...qd, + grading_default_profile_id: qd.grading_default_profile_id ?? null, + auto_assign_at_deadline: qd.auto_assign_at_deadline ?? false, + auto_assign_assignee_pool: qd.auto_assign_assignee_pool ?? "graders", + auto_assign_review_due_hours: qd.auto_assign_review_due_hours ?? 72, + late_grading_reminders_enabled: qd.late_grading_reminders_enabled ?? false, + late_grading_reminder_interval_hours: qd.late_grading_reminder_interval_hours ?? 12, + late_grading_reply_to: qd.late_grading_reply_to ?? null, + late_grading_cc_emails: qd.late_grading_cc_emails ?? { emails: [] } + }); } }, [queryData, reset]);Then drop the grading-automation
setValuecalls from the self-review effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/course/`[course_id]/manage/assignments/[assignment_id]/edit/page.tsx around lines 37 - 75, The effect that reads selfReviewSetting is re-seeding grading-automation fields (multiple form.setValue calls) whenever selfReviewSetting resolves and clobbers user edits; move the grading-automation seeding into the initial queryData-driven reset so those fields are set only when queryData loads (i.e., include grading-automation keys in the object passed to reset(queryData) in the earlier effect that handles queryData) and remove the grading-automation form.setValue calls from the selfReviewSetting useEffect; alternatively, if you prefer minimal change, guard the selfReviewSetting useEffect with a ref or a secondary flag keyed only on queryData so the grading-automation branch runs at most once per queryData load.
🧹 Nitpick comments (4)
app/course/[course_id]/manage/assignments/new/form.tsx (2)
43-70: Duplicate local type — prefer the newly-added generated type fromDatabaseTypes.d.ts.
GradingAssignmentDefaultProfileis now exported from@/utils/supabase/DatabaseTypes. Re-declaring a hand-maintained shape here (and again ingrading-assignment-defaults/page.tsx) invites drift as columns are added/renamed. Swap to the generated alias and keep the narrowGradingAssigneePool/GradingCcEmailshelpers if they're handy for form typing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/course/`[course_id]/manage/assignments/new/form.tsx around lines 43 - 70, The local GradingAssignmentDefaultProfile type declaration duplicates the generated type and should be replaced with an import from the generated DatabaseTypes; update this file's type references (the GradingAssignmentDefaultProfile mentioned above and any usages inside AssignmentFormValues) to use the exported alias from "@/utils/supabase/DatabaseTypes" instead of re-declaring it, remove the duplicate declarations here and in grading-assignment-defaults/page.tsx, and keep/continue using the narrow helper types GradingAssigneePool and GradingCcEmails where needed for form typing.
666-679: Minor: controlled<select>value should be coerced to string.
value={selectedProfileId ?? ""}passes anumber(or"") to the DOM<select>. React tolerates this but the matching<option value={profile.id}>also yields a number, and under Strict Mode hydration with mismatched types you can see "Selected options mismatch" warnings. Cast both sides to string (String(profile.id)andString(selectedProfileId)) for consistency, and parse back in theonChange. Not breaking, just tidier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/course/`[course_id]/manage/assignments/new/form.tsx around lines 666 - 679, The select is passing mixed types—number and ""—causing potential hydration warnings; update the value prop on NativeSelectField to use String(selectedProfileId ?? "") and set each option's value to String(profile.id) so both sides are strings, and in the onChange handler parse the selected string back to a number (or null) before calling setValue("grading_default_profile_id", ...); locate symbols: NativeSelectField, selectedProfileId, profiles, onChange, and setValue to make these changes.app/course/[course_id]/manage/course/grading-assignment-defaults/page.tsx (1)
31-45: Prefer the generated type over a hand-rolled duplicate.This PR adds
GradingAssignmentDefaultProfiletoutils/supabase/DatabaseTypes.d.ts; importing from there keeps this page in lockstep with the database schema (and catches future column additions/renames at compile time). The localGradingAssigneePool/GradingCcEmailsaliases can stay as narrowed helpers, but the row shape shouldn't be maintained in two places.♻️ Proposed refactor
+import type { GradingAssignmentDefaultProfile as DbGradingAssignmentDefaultProfile } from "@/utils/supabase/DatabaseTypes"; + type GradingAssigneePool = "graders" | "instructors" | "instructors_and_graders"; type GradingCcEmails = { emails: string[] }; -type GradingAssignmentDefaultProfile = { - id: number; - class_id: number; - name: string; - description: string | null; - auto_assign_at_deadline: boolean; - auto_assign_assignee_pool: GradingAssigneePool; - auto_assign_review_due_hours: number; - late_grading_reminders_enabled: boolean; - late_grading_reminder_interval_hours: number | null; - late_grading_reply_to: string | null; - late_grading_cc_emails: GradingCcEmails; -}; +type GradingAssignmentDefaultProfile = DbGradingAssignmentDefaultProfile;(Same suggestion applies to the duplicate block in
app/course/[course_id]/manage/assignments/new/form.tsx.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/course/`[course_id]/manage/course/grading-assignment-defaults/page.tsx around lines 31 - 45, Replace the locally defined GradingAssignmentDefaultProfile with the generated DB row type from utils/supabase/DatabaseTypes.d.ts and import that type; keep the local GradingAssigneePool and GradingCcEmails helpers as-is, then redefine FormValues as Omit<ImportedGradingAssignmentDefaultProfile, "id" | "class_id"> so the page (and the similar block in the new assignment form) stays synchronized with the database schema.app/course/[course_id]/manage/assignments/new/page.tsx (1)
115-124: Minor: prefer??over||for nullable coercions and hoist repeatedgetValuesreads.
|| nullcoerces falsy but valid values (e.g., a numeric0) to null. While profile ids and hour counts aren't expected to be0,??more accurately expresses "fallback only on null/undefined" and is idiomatic for this kind of payload construction. Also,late_grading_reminders_enabledis read three times here — grab it once into a local so the ternary is clearer and you can't accidentally de-sync the branches.♻️ Proposed refactor
+ grading_default_profile_id: form.getValues("grading_default_profile_id") ?? null, - grading_default_profile_id: form.getValues("grading_default_profile_id") || null, - auto_assign_at_deadline: form.getValues("auto_assign_at_deadline") || false, - auto_assign_assignee_pool: form.getValues("auto_assign_assignee_pool") || "graders", + auto_assign_at_deadline: form.getValues("auto_assign_at_deadline") ?? false, + auto_assign_assignee_pool: form.getValues("auto_assign_assignee_pool") ?? "graders", auto_assign_review_due_hours: form.getValues("auto_assign_review_due_hours") ?? 72, - late_grading_reminders_enabled: form.getValues("late_grading_reminders_enabled") || false, - late_grading_reminder_interval_hours: form.getValues("late_grading_reminders_enabled") - ? (form.getValues("late_grading_reminder_interval_hours") ?? 12) - : null, - late_grading_reply_to: form.getValues("late_grading_reply_to") || null, + late_grading_reminders_enabled: remindersEnabled, + late_grading_reminder_interval_hours: remindersEnabled + ? (form.getValues("late_grading_reminder_interval_hours") ?? 12) + : null, + late_grading_reply_to: form.getValues("late_grading_reply_to") || null, late_grading_cc_emails: form.getValues("late_grading_cc_emails") ?? { emails: [] }(with
const remindersEnabled = form.getValues("late_grading_reminders_enabled") ?? false;hoisted just above the insert.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/course/`[course_id]/manage/assignments/new/page.tsx around lines 115 - 124, The object construction uses || which wrongly treats falsy-but-valid values as absent and repeatedly calls form.getValues("late_grading_reminders_enabled"); change each fallback to null/typed defaults using the nullish coalescing operator (??) for fields like grading_default_profile_id, auto_assign_at_deadline, auto_assign_assignee_pool, auto_assign_review_due_hours, late_grading_reply_to, and late_grading_cc_emails, and hoist form.getValues("late_grading_reminders_enabled") into a local constant (e.g., const remindersEnabled = form.getValues("late_grading_reminders_enabled") ?? false) so you use remindersEnabled in the ternary for late_grading_reminder_interval_hours and the boolean field late_grading_reminders_enabled to avoid repeated reads and ensure correct null/undefined fallback semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/course/`[course_id]/manage/assignments/new/form.tsx:
- Around line 617-651: The profileMap is rebuilt on every render causing the
useEffect that applies profile settings to re-run and overwrite user edits;
memoize profiles and profileMap using React's useMemo (import useMemo) so
profileMap is stable, and update the useEffect dependency array to remove
profileMap and include only selectedProfileId, profileExists,
applyProfileOnSelect, setValue (and any stable helpers like normalizeCcEmails if
needed); locate symbols profiles, profileMap, selectedProfileId,
applyProfileOnSelect, setValue, and the useEffect block and implement useMemo
around profiles.reduce to prevent continuous re-application.
In `@app/course/`[course_id]/manage/course/grading-assignment-defaults/page.tsx:
- Around line 68-94: GradingAssignmentDefaultsPage currently converts course_id
to classId with Number(course_id) and only gates useList with
Number.isFinite(classId); you must also guard form submission and payload
creation (onSubmit and any create/update calls that include class_id) so they
never send class_id: NaN. Fix by validating classId early in the component
(e.g., if (!Number.isFinite(classId)) return a small "Invalid course" message or
render the form disabled) and add a runtime check in the submit handler
(onSubmit) to abort if classId is not finite before building create/update
payloads, ensuring all references to classId in payloads use the validated
value.
- Around line 163-177: The handleDelete function currently calls deleteProfile
immediately; add a user confirmation step before invoking deleteProfile (e.g.,
use a Chakra Dialog or a window.confirm prompt) so destructive deletes are
gated—update the handleDelete handler to first show/await the confirmation, only
call deleteProfile({ resource: "grading_assignment_default_profiles", id }) and
proceed with toaster.success, clearForm (if editingId === id), and refetch()
when the user confirms, and abort with no-op when the user cancels.
In
`@supabase/migrations/20260418100000_grading_assignment_defaults_and_reminders.sql`:
- Around line 297-300: The code sets auto_assigned_at even when
bulk_assign_reviews failed; change the flow so auto_assigned_at is only set when
bulk_assign_reviews succeeded by checking (v_result->>'success')::boolean (same
check used with v_result and v_assignment) before updating the assignment.
Concretely, move or guard the update that writes auto_assigned_at (the block
that sets auto_assigned_at for v_assignment) so it executes only on success of
bulk_assign_reviews (i.e., when COALESCE((v_result->>'success')::boolean, false)
IS TRUE), and ensure the RETURN 0 path on failure does not reach the
auto_assigned_at update. Ensure references to bulk_assign_reviews, v_result,
v_assignment, and auto_assigned_at are used to locate and modify the logic.
- Around line 352-365: The reminder_recipients CTE currently selects all
incomplete review_assignments (ra) regardless of whether the individual review's
p_due_date is past; to only remind graders for overdue reviews, add a condition
in the WHERE of the CTE to filter by the review due datetime (e.g. AND
COALESCE(ra.p_due_date, v_assignment.due_date) < now() or simply AND
ra.p_due_date < now() if p_due_date is always set). Update the WHERE clause in
the reminder_recipients query (referencing reminder_recipients,
review_assignments/ra, and v_assignment) to include that overdue check so
auto-assigned reviews with future p_due_date are not included.
- Around line 414-456: The loop currently reads stale LEFT JOIN values then
upserts and performs side effects, allowing races; after the INSERT ... ON
CONFLICT (assignment_id) DO NOTHING into
public.assignment_grading_automation_state, re-select the state row for the
current v_assignment.id with SELECT ... FROM
public.assignment_grading_automation_state WHERE assignment_id = v_assignment.id
FOR UPDATE to atomically claim the work and refresh fields (auto_assigned_at,
last_reminder_sent_at, last_reminder_recipient_count) into local variables
before calling public.auto_assign_grading_reviews_for_assignment or
public.queue_late_grading_reminders_for_assignment so the subsequent IF checks
use the locked, up-to-date state to prevent duplicate work.
- Line 34: The foreign key on the assignment default profile currently only
references profiles.id, allowing cross-class profile links; update the FK to be
class-scoped by replacing the single-column FK on profile_id with a composite FK
(class_id, profile_id) that references profiles(class_id, id) (and ensure
profiles has class_id), and apply the same change for the other occurrence
(lines 104–108) so both constraints enforce tenant isolation; locate the
constraint(s) named or in the table(s) created in this migration (e.g.,
grading_assignment_default_profiles and the other similar constraint block) and
modify the reference to use (class_id, profile_id) -> profiles(class_id, id).
- Around line 154-162: The table assignment_grading_automation_state is created
without RLS which may allow clients to read/mutate automation timestamps; enable
row-level security on assignment_grading_automation_state and add restrictive
policies (e.g., enable RLS with ALTER TABLE ... ENABLE ROW LEVEL SECURITY and
then create policies that only allow access from server-side roles or functions
such as the supabase service_role or specific authenticated backend role) and
remove any PUBLIC grants so only the intended server/service roles and trusted
DB functions can SELECT/INSERT/UPDATE/DELETE; reference the table name
assignment_grading_automation_state and ensure policies explicitly permit only
the server/service context used by your backend.
- Around line 473-484: Wrap the pg_cron scheduling/unscheduling calls inside an
exception handler so failures don't abort the migration: inside the existing DO
$$ block surrounding the checks for pg_extension and cron.job, add a BEGIN ...
EXCEPTION WHEN OTHERS THEN ... END block around the PERFORM cron.unschedule(...)
and PERFORM cron.schedule(...) calls (the cron.unschedule and cron.schedule
invocations are the unique symbols to modify) and in the EXCEPTION branch log or
RAISE NOTICE with the error message so the migration continues when scheduling
fails due to privileges or misconfiguration.
In `@utils/supabase/SupabaseTypes.d.ts`:
- Around line 1016-1144: The PR contains manual edits to the auto-generated
Supabase types (e.g., assignment_grading_automation_state and
grading_assignment_default_profiles) — instead of hand-editing
SupabaseTypes.d.ts, revert your manual changes and regenerate the file by
running the local client generator (run npm run client-local against the current
DB/schema), verify the regenerated SupabaseTypes.d.ts includes the intended
table entries, and commit the regenerated file so the auto-generated types
remain in sync.
---
Outside diff comments:
In `@app/course/`[course_id]/manage/assignments/[assignment_id]/edit/page.tsx:
- Around line 37-75: The effect that reads selfReviewSetting is re-seeding
grading-automation fields (multiple form.setValue calls) whenever
selfReviewSetting resolves and clobbers user edits; move the grading-automation
seeding into the initial queryData-driven reset so those fields are set only
when queryData loads (i.e., include grading-automation keys in the object passed
to reset(queryData) in the earlier effect that handles queryData) and remove the
grading-automation form.setValue calls from the selfReviewSetting useEffect;
alternatively, if you prefer minimal change, guard the selfReviewSetting
useEffect with a ref or a secondary flag keyed only on queryData so the
grading-automation branch runs at most once per queryData load.
---
Nitpick comments:
In `@app/course/`[course_id]/manage/assignments/new/form.tsx:
- Around line 43-70: The local GradingAssignmentDefaultProfile type declaration
duplicates the generated type and should be replaced with an import from the
generated DatabaseTypes; update this file's type references (the
GradingAssignmentDefaultProfile mentioned above and any usages inside
AssignmentFormValues) to use the exported alias from
"@/utils/supabase/DatabaseTypes" instead of re-declaring it, remove the
duplicate declarations here and in grading-assignment-defaults/page.tsx, and
keep/continue using the narrow helper types GradingAssigneePool and
GradingCcEmails where needed for form typing.
- Around line 666-679: The select is passing mixed types—number and ""—causing
potential hydration warnings; update the value prop on NativeSelectField to use
String(selectedProfileId ?? "") and set each option's value to
String(profile.id) so both sides are strings, and in the onChange handler parse
the selected string back to a number (or null) before calling
setValue("grading_default_profile_id", ...); locate symbols: NativeSelectField,
selectedProfileId, profiles, onChange, and setValue to make these changes.
In `@app/course/`[course_id]/manage/assignments/new/page.tsx:
- Around line 115-124: The object construction uses || which wrongly treats
falsy-but-valid values as absent and repeatedly calls
form.getValues("late_grading_reminders_enabled"); change each fallback to
null/typed defaults using the nullish coalescing operator (??) for fields like
grading_default_profile_id, auto_assign_at_deadline, auto_assign_assignee_pool,
auto_assign_review_due_hours, late_grading_reply_to, and late_grading_cc_emails,
and hoist form.getValues("late_grading_reminders_enabled") into a local constant
(e.g., const remindersEnabled = form.getValues("late_grading_reminders_enabled")
?? false) so you use remindersEnabled in the ternary for
late_grading_reminder_interval_hours and the boolean field
late_grading_reminders_enabled to avoid repeated reads and ensure correct
null/undefined fallback semantics.
In `@app/course/`[course_id]/manage/course/grading-assignment-defaults/page.tsx:
- Around line 31-45: Replace the locally defined GradingAssignmentDefaultProfile
with the generated DB row type from utils/supabase/DatabaseTypes.d.ts and import
that type; keep the local GradingAssigneePool and GradingCcEmails helpers as-is,
then redefine FormValues as Omit<ImportedGradingAssignmentDefaultProfile, "id" |
"class_id"> so the page (and the similar block in the new assignment form) stays
synchronized with the database schema.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8dc8c29c-c4d3-4057-a337-f2dca088f751
📒 Files selected for processing (9)
app/course/[course_id]/dynamicCourseNav.tsxapp/course/[course_id]/manage/assignments/[assignment_id]/edit/page.tsxapp/course/[course_id]/manage/assignments/new/form.tsxapp/course/[course_id]/manage/assignments/new/page.tsxapp/course/[course_id]/manage/course/grading-assignment-defaults/page.tsxsupabase/functions/_shared/SupabaseTypes.d.tssupabase/migrations/20260418100000_grading_assignment_defaults_and_reminders.sqlutils/supabase/DatabaseTypes.d.tsutils/supabase/SupabaseTypes.d.ts
Co-authored-by: Jonathan Bell <jon@jonbell.net>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
app/course/[course_id]/manage/course/grading-assignment-defaults/page.tsx (3)
130-182: Referenced assignments aren't surfaced on delete.
handleDeleteprompts beforeuseDelete, but the migration defines the FK withON DELETE SET NULL, meaning silently detaching the profile from any assignment currently pointing at it. That's probably desired behaviour, but instructors might be surprised to see their assignment forms show "No profile selected" the next morning. Consider showing the referenced assignment count in the confirm (or post-delete toast) — a cheapuseListwithfilter grading_default_profile_id eq idbefore confirmation would do it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/course/`[course_id]/manage/course/grading-assignment-defaults/page.tsx around lines 130 - 182, The delete flow currently prompts and calls useDelete (handleDelete) without surfacing how many assignments reference the profile (FK ON DELETE SET NULL) so instructors are surprised when assignments lose the profile; before showing the confirmation (in handleDelete) call useList (or the existing list hook) with filter "grading_default_profile_id eq {id}" to count referencing assignments and include that count in the confirm message (or, if you prefer, fetch after delete and show a post-delete toaster with the count), ensuring you reference the profile id passed to handleDelete and keep behavior with useDelete unchanged.
55-76: Duplicate helpers — import from the form module instead.
normalizeCcEmailshere is a copy of the one inapp/course/[course_id]/manage/assignments/new/form.tsx. If the CC-shape contract ever evolves (multi-field, display name + address, etc.), having two definitions will drift. Easy win to export fromform.tsxand import both here and in the assignment pages.As per coding guidelines, use the
@/*path alias when resolving the import path (e.g.@/app/course/[course_id]/manage/assignments/new/form).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/course/`[course_id]/manage/course/grading-assignment-defaults/page.tsx around lines 55 - 76, The file duplicates the normalizeCcEmails helper (and related parseCcEmails) already defined in form.tsx; remove the local definitions (normalizeCcEmails and parseCcEmails) and instead export the canonical helper(s) from app/course/[course_id]/manage/assignments/new/form.tsx and import them into this page using the `@/`* path alias (e.g. import { normalizeCcEmails, parseCcEmails } from '@/app/course/[course_id]/manage/assignments/new/form'). Ensure the exported names match the originals and update any call sites in this file to use the imported functions.
184-203:window.confirmblocks the main thread and isn't stylable.Functionally fine and addresses the prior destructive-action feedback. For parity with the rest of the app's Chakra surface, a
Dialogwould render inside the SSR/hydration boundary and is easier to e2e-test than a native browser prompt. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/course/`[course_id]/manage/course/grading-assignment-defaults/page.tsx around lines 184 - 203, Replace the blocking window.confirm call in handleDelete with a Chakra UI Dialog: create a local state like pendingDeleteId (or pendingDeleteProfile) and open the Dialog when user initiates delete (instead of calling handleDelete directly), move the destructive call into a confirm handler (e.g., confirmDelete which calls deleteProfile, toaster, clearForm/refetch and closes the dialog) and add a cancel handler that clears pendingDeleteId to close the dialog; update UI buttons that currently call handleDelete(id) to instead set pendingDeleteId = id and render the Dialog with accessible title/description and Confirm/Cancel buttons wired to the confirmDelete and cancel handlers so deletion is non-blocking and stylable while preserving the existing error/toaster/refetch behavior found in handleDelete.app/course/[course_id]/manage/assignments/[assignment_id]/edit/page.tsx (2)
29-40: Normalizelate_grading_cc_emailsthrough the shared helper on load.
values.late_grading_cc_emails ?? { emails: [] }trusts the DB payload. The check constraint guarantees{ emails: [...] }shape, but element types aren't constrained to strings (JSONB arrays can mix types), and the form'sGradingCcEmailsis typed asstring[].form.tsxexportsnormalizeCcEmailsfor precisely this reason and is already used both in the profile-apply effect there and in the defaults page. Using it here keeps the edit-form input consistent with the create path.♻️ Suggested fix
-import AssignmentForm, { AssignmentFormValues } from "../../new/form"; +import AssignmentForm, { AssignmentFormValues, normalizeCcEmails } from "../../new/form"; @@ - late_grading_cc_emails: values.late_grading_cc_emails ?? { emails: [] } + late_grading_cc_emails: normalizeCcEmails(values.late_grading_cc_emails)(Requires exporting
normalizeCcEmailsfromapp/course/[course_id]/manage/assignments/new/form.tsx.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/course/`[course_id]/manage/assignments/[assignment_id]/edit/page.tsx around lines 29 - 40, The edit page currently sets late_grading_cc_emails directly from the DB payload (values.late_grading_cc_emails ?? { emails: [] }) without normalizing types; import and call the shared normalizeCcEmails used in form.tsx (export it if not already exported) and pass values.late_grading_cc_emails through normalizeCcEmails before calling reset (i.e., use normalizeCcEmails(values.late_grading_cc_emails) or fallback to normalizeCcEmails({ emails: [] })) so the form's GradingCcEmails input always receives a string[] shape.
62-134:useCallbackdep list referencesform.refineCorewithoutform.React-Hooks exhaustive-deps will flag this —
formis accessed viaform.refineCore.onFinish(values)inside the callback, but onlyform.refineCoreis listed. Sinceformis stable fromuseForm()anyway, adding it (and droppingform.refineCore) is the idiomatic fix. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/course/`[course_id]/manage/assignments/[assignment_id]/edit/page.tsx around lines 62 - 134, The callback passed to useCallback references form.refineCore.onFinish but the dependency array includes form.refineCore instead of form, which triggers exhaustive-deps; update the dependency array for the onFinish callback to include form (and remove form.refineCore) so it depends on the stable form object returned by useForm(), keeping other deps (assignment_id, course_id, data?.data.self_review_setting_id, update) unchanged.app/course/[course_id]/manage/assignments/new/form.tsx (2)
787-795:valueAsNumberon an optional hour field producesNaN, notnull.When
late_grading_reminders_enabledis toggled on, then off, then on again, an empty input coerces toNaN. The create/edit pages normalize to?? 12before submit, butNaN ?? 12 === NaN. ConsidersetValueAs: (v) => (v === "" || v === null ? null : Number(v))or similar, so the downstream nullish-coalescing default actually kicks in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/course/`[course_id]/manage/assignments/new/form.tsx around lines 787 - 795, The numeric input for late_grading_reminder_interval_hours currently uses valueAsNumber which coerces empty strings to NaN and breaks the downstream nullish-coalescing default; update the react-hook-form registration for late_grading_reminder_interval_hours (the register call) to use setValueAs that returns null for empty or null values (e.g., setValueAs: v => (v === "" || v === null ? null : Number(v))) so that when late_grading_reminders_enabled is toggled off/on the field normalizes to null and the existing ?? 12 fallback works as intended.
58-71: Consider exportingnormalizeCcEmailsfor reuse.Two sibling files (
edit/page.tsx,new/page.tsx) and thegrading-assignment-defaults/page.tsxeach re-derive or duplicate this logic. Exporting this helper keeps the CC-email contract in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/course/`[course_id]/manage/assignments/new/form.tsx around lines 58 - 71, The normalizeCcEmails helper is duplicated across multiple files; export it from this module so other pages can import the single source of truth: convert the local function normalizeCcEmails into a named export (export const normalizeCcEmails) and update callers in edit/page.tsx, new/page.tsx and grading-assignment-defaults/page.tsx to import { normalizeCcEmails } from this file, ensuring the exported type GradingCcEmails is also exported if needed so the contract remains consistent.supabase/migrations/20260418100000_grading_assignment_defaults_and_reminders.sql (2)
454-464: Log message implies success even when nothing was assigned.When
v_auto_assigned_countis0(no submissions / no-op) theRAISE LOGstill reads'Auto-assigned grading for assignment %, created/updated=%'. Minor phrasing nit; consider distinguishing the "nothing to do" path from the "actually created assignments" path so operator logs are easier to triage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260418100000_grading_assignment_defaults_and_reminders.sql` around lines 454 - 464, The RAISE LOG always reports "Auto-assigned grading..." even when v_auto_assigned_count = 0; update the logic after calling public.auto_assign_grading_reviews_for_assignment(v_assignment.id) to emit distinct messages: if v_auto_assigned_count > 0, log the existing success message with v_assignment.id and v_auto_assigned_count; if v_auto_assigned_count = 0, log a no-op message like "No grading assignments created for assignment % (count=0)"; and if v_auto_assigned_count < 0 (error path), log an error/warning indicating the function failed. Adjust the RAISE LOG block near the call and keep the existing update to assignment_grading_automation_state unchanged.
354-410: Queue function always creates anemail_batchesrow, even on rubric-mismatch / empty recipient runs.
INSERT INTO public.email_batchesat line 354 runs before the recipient CTE is evaluated, and the compensatingDELETEon line 409 only fires when zero emails were queued. On every cron tick after deadline where there are still some (but stable) overdue reviewers, you will happily recreate a batch. That is correct — I only want to flag that the batch-delete compensation is racy if concurrent cron runs both insert batches; sincerun_assignment_grading_automationnow holdsFOR UPDATEon the state row (line 452) the outer serialization protects you, so this is a nit. Worth a short comment explaining that invariant so future refactors don't drop the lock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260418100000_grading_assignment_defaults_and_reminders.sql` around lines 354 - 410, The INSERT into public.email_batches currently happens before computing recipients which can lead to transient duplicate batches if concurrent cron runs insert at the same time; add a brief explanatory comment next to the INSERT (or at the top of this block) stating that concurrency is protected by the outer FOR UPDATE lock on the state row held by run_assignment_grading_automation (the state row lock at the FOR UPDATE in run_assignment_grading_automation prevents concurrent inserts and makes the later DELETE safe), so future refactors know this invariant must be preserved or replaced with equivalent serialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@supabase/migrations/20260418100000_grading_assignment_defaults_and_reminders.sql`:
- Around line 194-316: The function auto_assign_grading_reviews_for_assignment
currently returns 0 for misconfiguration cases (no rubric, no instructor actor,
or no eligible staff) which the caller run_assignment_grading_automation treats
as success and marks auto_assigned_at; change the behavior so that when
v_assignment.grading_rubric_id IS NULL, when v_actor_user_id IS NULL, or when
v_staff_count = 0 the function returns -1 (or RAISE WARNING/EXCEPTION) instead
of 0, leaving auto_assigned_at NULL and allowing retries, while keeping the
existing 0 return for the legitimate "no submissions" case; update any RAISE
WARNING messages to include context (assignment id and reason) to aid debugging.
---
Nitpick comments:
In `@app/course/`[course_id]/manage/assignments/[assignment_id]/edit/page.tsx:
- Around line 29-40: The edit page currently sets late_grading_cc_emails
directly from the DB payload (values.late_grading_cc_emails ?? { emails: [] })
without normalizing types; import and call the shared normalizeCcEmails used in
form.tsx (export it if not already exported) and pass
values.late_grading_cc_emails through normalizeCcEmails before calling reset
(i.e., use normalizeCcEmails(values.late_grading_cc_emails) or fallback to
normalizeCcEmails({ emails: [] })) so the form's GradingCcEmails input always
receives a string[] shape.
- Around line 62-134: The callback passed to useCallback references
form.refineCore.onFinish but the dependency array includes form.refineCore
instead of form, which triggers exhaustive-deps; update the dependency array for
the onFinish callback to include form (and remove form.refineCore) so it depends
on the stable form object returned by useForm(), keeping other deps
(assignment_id, course_id, data?.data.self_review_setting_id, update) unchanged.
In `@app/course/`[course_id]/manage/assignments/new/form.tsx:
- Around line 787-795: The numeric input for
late_grading_reminder_interval_hours currently uses valueAsNumber which coerces
empty strings to NaN and breaks the downstream nullish-coalescing default;
update the react-hook-form registration for late_grading_reminder_interval_hours
(the register call) to use setValueAs that returns null for empty or null values
(e.g., setValueAs: v => (v === "" || v === null ? null : Number(v))) so that
when late_grading_reminders_enabled is toggled off/on the field normalizes to
null and the existing ?? 12 fallback works as intended.
- Around line 58-71: The normalizeCcEmails helper is duplicated across multiple
files; export it from this module so other pages can import the single source of
truth: convert the local function normalizeCcEmails into a named export (export
const normalizeCcEmails) and update callers in edit/page.tsx, new/page.tsx and
grading-assignment-defaults/page.tsx to import { normalizeCcEmails } from this
file, ensuring the exported type GradingCcEmails is also exported if needed so
the contract remains consistent.
In `@app/course/`[course_id]/manage/course/grading-assignment-defaults/page.tsx:
- Around line 130-182: The delete flow currently prompts and calls useDelete
(handleDelete) without surfacing how many assignments reference the profile (FK
ON DELETE SET NULL) so instructors are surprised when assignments lose the
profile; before showing the confirmation (in handleDelete) call useList (or the
existing list hook) with filter "grading_default_profile_id eq {id}" to count
referencing assignments and include that count in the confirm message (or, if
you prefer, fetch after delete and show a post-delete toaster with the count),
ensuring you reference the profile id passed to handleDelete and keep behavior
with useDelete unchanged.
- Around line 55-76: The file duplicates the normalizeCcEmails helper (and
related parseCcEmails) already defined in form.tsx; remove the local definitions
(normalizeCcEmails and parseCcEmails) and instead export the canonical helper(s)
from app/course/[course_id]/manage/assignments/new/form.tsx and import them into
this page using the `@/`* path alias (e.g. import { normalizeCcEmails,
parseCcEmails } from '@/app/course/[course_id]/manage/assignments/new/form').
Ensure the exported names match the originals and update any call sites in this
file to use the imported functions.
- Around line 184-203: Replace the blocking window.confirm call in handleDelete
with a Chakra UI Dialog: create a local state like pendingDeleteId (or
pendingDeleteProfile) and open the Dialog when user initiates delete (instead of
calling handleDelete directly), move the destructive call into a confirm handler
(e.g., confirmDelete which calls deleteProfile, toaster, clearForm/refetch and
closes the dialog) and add a cancel handler that clears pendingDeleteId to close
the dialog; update UI buttons that currently call handleDelete(id) to instead
set pendingDeleteId = id and render the Dialog with accessible title/description
and Confirm/Cancel buttons wired to the confirmDelete and cancel handlers so
deletion is non-blocking and stylable while preserving the existing
error/toaster/refetch behavior found in handleDelete.
In
`@supabase/migrations/20260418100000_grading_assignment_defaults_and_reminders.sql`:
- Around line 454-464: The RAISE LOG always reports "Auto-assigned grading..."
even when v_auto_assigned_count = 0; update the logic after calling
public.auto_assign_grading_reviews_for_assignment(v_assignment.id) to emit
distinct messages: if v_auto_assigned_count > 0, log the existing success
message with v_assignment.id and v_auto_assigned_count; if v_auto_assigned_count
= 0, log a no-op message like "No grading assignments created for assignment %
(count=0)"; and if v_auto_assigned_count < 0 (error path), log an error/warning
indicating the function failed. Adjust the RAISE LOG block near the call and
keep the existing update to assignment_grading_automation_state unchanged.
- Around line 354-410: The INSERT into public.email_batches currently happens
before computing recipients which can lead to transient duplicate batches if
concurrent cron runs insert at the same time; add a brief explanatory comment
next to the INSERT (or at the top of this block) stating that concurrency is
protected by the outer FOR UPDATE lock on the state row held by
run_assignment_grading_automation (the state row lock at the FOR UPDATE in
run_assignment_grading_automation prevents concurrent inserts and makes the
later DELETE safe), so future refactors know this invariant must be preserved or
replaced with equivalent serialization.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7148d0d3-4401-472b-ad34-ff34cd21df8d
📒 Files selected for processing (5)
app/course/[course_id]/manage/assignments/[assignment_id]/edit/page.tsxapp/course/[course_id]/manage/assignments/new/form.tsxapp/course/[course_id]/manage/assignments/new/page.tsxapp/course/[course_id]/manage/course/grading-assignment-defaults/page.tsxsupabase/migrations/20260418100000_grading_assignment_defaults_and_reminders.sql
Co-authored-by: Jonathan Bell <jon@jonbell.net>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/e2e/grading-assignment-defaults.test.tsx (2)
139-141: Replace the fixed 1.5s sleep with a deterministic wait so the clobber-regression guard doesn't go stale.This block is specifically guarding the "don't re-seed automation fields after the initial
reset(...)" fix called out in the PR objectives, which is great — butpage.waitForTimeout(1500)is both flaky (CI can easily delay an effect past 1.5s) and silently permissive (a regression that clobbers after, say, 2s would still let this test pass). A deterministic signal (e.g.expect.pollthe value for a short window, or wait on adata-form-ready/ network-idle marker tied to the initial reset) would both reduce flakes and keep the regression guard honest.♻️ Sketch of a poll-based guard
- await editDueHoursInput.fill("77"); - await page.waitForTimeout(1500); - await expect(editDueHoursInput).toHaveValue("77"); + await editDueHoursInput.fill("77"); + // Ensure no delayed reset() clobbers the edited value. + await expect + .poll(async () => editDueHoursInput.inputValue(), { timeout: 3000, intervals: [200, 300, 500] }) + .toBe("77"); + await expect(editDueHoursInput).toHaveValue("77");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/grading-assignment-defaults.test.tsx` around lines 139 - 141, Replace the flaky fixed sleep (page.waitForTimeout(1500)) used after filling editDueHoursInput with a deterministic wait: poll the element value (e.g., use expect.poll or expect(editDueHoursInput).toHaveValue within a short timeout) or wait on a reliable signal (network/form-ready event) that confirms the initial reset has completed; update the block around editDueHoursInput so the assertion only proceeds once the deterministic condition is met rather than using page.waitForTimeout.
78-83: Delete coverage only hops over the fence — the "confirm and actually delete" path is never exercised.The PR hardens the defaults page with a delete confirmation dialog, and this block nicely verifies the cancel/dismiss branch. However, the accept branch (dialog accepted → row disappears from the list → row removed from
grading_assignment_default_profiles) is the one that actually mutates data, and it's currently untested here. Since this test already owns a throwaway profile (and later seeds a separate one viasupabase.updateon the assignment, so deletion of this profile wouldn't break the rest of the flow if reordered), it would be cheap to add a secondpage.once("dialog", d => d.accept())pass and assert the row is gone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/grading-assignment-defaults.test.tsx` around lines 78 - 83, The test currently only covers dismissing the delete confirmation dialog; add the "accept" path by registering a second page.once("dialog", async dialog => dialog.accept()) before triggering the Delete button again (using the same page.getByRole("button", { name: 'Delete' }).first().click()), then assert the profile row identified by gradingProfileName is no longer visible via expect(page.getByText(gradingProfileName)).not.toBeVisible() and optionally verify the row was removed from the grading_assignment_default_profiles table; ensure you reuse the same throwaway profile (or reorder seeding) so deleting it won't break later steps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/grading-assignment-defaults.test.tsx`:
- Around line 139-141: Replace the flaky fixed sleep (page.waitForTimeout(1500))
used after filling editDueHoursInput with a deterministic wait: poll the element
value (e.g., use expect.poll or expect(editDueHoursInput).toHaveValue within a
short timeout) or wait on a reliable signal (network/form-ready event) that
confirms the initial reset has completed; update the block around
editDueHoursInput so the assertion only proceeds once the deterministic
condition is met rather than using page.waitForTimeout.
- Around line 78-83: The test currently only covers dismissing the delete
confirmation dialog; add the "accept" path by registering a second
page.once("dialog", async dialog => dialog.accept()) before triggering the
Delete button again (using the same page.getByRole("button", { name: 'Delete'
}).first().click()), then assert the profile row identified by
gradingProfileName is no longer visible via
expect(page.getByText(gradingProfileName)).not.toBeVisible() and optionally
verify the row was removed from the grading_assignment_default_profiles table;
ensure you reuse the same throwaway profile (or reorder seeding) so deleting it
won't break later steps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: eb8ac282-d207-4bbc-8c08-f232329339d4
📒 Files selected for processing (1)
tests/e2e/grading-assignment-defaults.test.tsx
Co-authored-by: Jonathan Bell <jon@jonbell.net>
Co-authored-by: Jonathan Bell <jon@jonbell.net>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/e2e/grading-assignment-defaults.test.tsx (2)
193-264: 💤 Low valueTest name suggests "lab leaders" pool but uses
"graders"— verify this is intentional.The test is named "deadline automation assigns grading to lab leaders" (line 193), but line 256 sets
auto_assign_assignee_pool: "graders". The test passes because the lab leaders happen to haverole: "grader", so they're included in the graders pool.If the intent is to test the
"lab_leaders"assignee pool feature specifically, consider updating line 256 to use that value. Otherwise, consider renaming the test to reflect that it's testing general grader assignment where graders happen to be lab leaders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/grading-assignment-defaults.test.tsx` around lines 193 - 264, The test name says it targets lab leaders but the assignment config uses auto_assign_assignee_pool: "graders"; decide whether to test the lab_leaders pool or general graders. If you mean lab leaders, change the assignment update to set auto_assign_assignee_pool: "lab_leaders" (refer to the autoAssignment variable and the .update call that sets auto_assign_assignee_pool), otherwise rename the test string ("deadline automation assigns grading to graders") to match the current behavior. Ensure the test data (createLabSectionWithStudents / lab_leaders variables) remains consistent with the chosen pool.
162-163: 💤 Low valueConsider replacing hardcoded wait with a condition-based assertion.
page.waitForTimeout(1500)can introduce flakiness if the input debounce timing changes. You could useexpect(editDueHoursInput).toHaveValue("77")with a built-in retry instead, since Playwright'sexpectalready polls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/grading-assignment-defaults.test.tsx` around lines 162 - 163, Replace the hardcoded sleep by removing the page.waitForTimeout(1500) and rely on Playwright's polling in the assertion: use await expect(editDueHoursInput).toHaveValue("77") (optionally with a custom timeout) so the test waits for the element value change instead of using a fixed delay; locate the code around page.waitForTimeout and editDueHoursInput to make this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/course/`[course_id]/manage/course/grading-assignment-defaults/page.tsx:
- Around line 116-118: The component references hooks useCourseController and
useListTableControllerValues (used in the gradersOnly and graderCourseRoles
logic) but they are not imported, causing a runtime ReferenceError; add import
statements for useCourseController and useListTableControllerValues at the top
of the file alongside the other hooks/modules so the symbols are defined when
gradersOnly and graderCourseRoles are evaluated. Ensure the imports match the
actual module exports in your codebase and place them with the other imports in
this file.
---
Nitpick comments:
In `@tests/e2e/grading-assignment-defaults.test.tsx`:
- Around line 193-264: The test name says it targets lab leaders but the
assignment config uses auto_assign_assignee_pool: "graders"; decide whether to
test the lab_leaders pool or general graders. If you mean lab leaders, change
the assignment update to set auto_assign_assignee_pool: "lab_leaders" (refer to
the autoAssignment variable and the .update call that sets
auto_assign_assignee_pool), otherwise rename the test string ("deadline
automation assigns grading to graders") to match the current behavior. Ensure
the test data (createLabSectionWithStudents / lab_leaders variables) remains
consistent with the chosen pool.
- Around line 162-163: Replace the hardcoded sleep by removing the
page.waitForTimeout(1500) and rely on Playwright's polling in the assertion: use
await expect(editDueHoursInput).toHaveValue("77") (optionally with a custom
timeout) so the test waits for the element value change instead of using a fixed
delay; locate the code around page.waitForTimeout and editDueHoursInput to make
this change.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e7852360-dfb5-4890-b373-82b2f322af1e
📒 Files selected for processing (10)
app/course/[course_id]/dynamicCourseNav.tsxapp/course/[course_id]/manage/assignments/[assignment_id]/edit/page.tsxapp/course/[course_id]/manage/assignments/new/form.tsxapp/course/[course_id]/manage/assignments/new/page.tsxapp/course/[course_id]/manage/course/grading-assignment-defaults/page.tsxsupabase/functions/_shared/SupabaseTypes.d.tssupabase/migrations/20260418100000_grading_assignment_defaults_and_reminders.sqltests/e2e/grading-assignment-defaults.test.tsxutils/supabase/DatabaseTypes.d.tsutils/supabase/SupabaseTypes.d.ts
✅ Files skipped from review due to trivial changes (3)
- app/course/[course_id]/dynamicCourseNav.tsx
- utils/supabase/DatabaseTypes.d.ts
- supabase/functions/_shared/SupabaseTypes.d.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- supabase/migrations/20260418100000_grading_assignment_defaults_and_reminders.sql
- app/course/[course_id]/manage/assignments/new/form.tsx
…assign - Fix CHECK-constraint subquery error (SQLSTATE 0A000) by wrapping the array-of-strings predicate in a new IMMUTABLE jsonb_is_string_array function and calling it from both grader_subset_ids CHECKs. - Bound run_assignment_grading_automation to a recent-deadline window and only select rows with actionable work (unrun auto-assign or elapsed reminder interval) so cron cost stays proportional to recent work rather than the lifetime of the assignments table. - Add submission-time auto-assignment for late submissions: AFTER INSERT trigger on submissions calls auto_assign_grading_review_for_submission for past-deadline insertions, with load-balanced staff picking and a re-submission guard. Errors are swallowed so a failed auto-assign cannot block the student's submission. - Add E2E coverage for the assignee-pool branches (instructors, instructors_and_graders, graders subset, lab_leaders with conflict exclusion, group_mentors with conflict exclusion), cron idempotency, reminder cadence (backdating last_reminder_sent_at), reminder filter predicates, and the late-submission trigger. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…brittleness
Reproduced and fixed the CI failure (run 25948945436) locally against a fresh
Supabase + prod build.
SQL fix: auto_assign_grading_reviews_for_assignment and the new
auto_assign_grading_review_for_submission both did ORDER BY ur.created_at
on user_roles, which has no created_at column. Use ORDER BY ur.id, which
gives the same stable "earliest enrolled" semantics.
Product fix: GradingAutomationSubform applied the saved profile's values
to the form via a useEffect on selectedProfile every time selectedProfile
became defined — including the initial form reset from the assignment's
saved DB state. On edit, this clobbered per-assignment overrides with
profile defaults. Move the apply-profile-values call into the Saved-profile
select's onChange so it only fires on user-initiated profile changes.
UI cleanup: removed the duplicate <Toaster /> on the grading-defaults page
(layout.tsx already mounts one globally), which had caused every
toaster.success(...) to render twice in the DOM and tripped Playwright's
strict-mode locator.
E2E robustness fixes around real CI artifacts of the page:
- Chakra Checkbox.Control intercepts pointer events; click the label text
instead (pattern used in gradebook.test.tsx).
- Lingering toast regions overlap the Delete buttons; use dispatchEvent("click")
to bypass DOM-level stacking, and drive the two window.confirms with a
single counter-based page.on("dialog") handler so order-of-arrival doesn't
matter.
- ManageAssignmentNav renders its children twice (one desktop layout, one
mobile layout) — both copies are in the DOM regardless of viewport, so
scope every edit-page locator to the first <form> wrapper.
- The reminder-filter test now upserts submission_review rows (existing
triggers may auto-create the grading-rubric one on submission insert)
instead of unconditionally inserting and failing on the unique key.
Local validation: with the fixes applied, all 10 non-UI tests in
grading-assignment-defaults.test.tsx pass; the UI test progresses cleanly
through every assertion until the post-save "Assignment Updated" toast,
which requires a real GitHub webhook configure call (AGENTS.md documents
this as expected for dummy-credentials local runs).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/e2e/grading-assignment-defaults.test.tsx`:
- Around line 209-211: The test clicks the Save button
(editForm.getByRole("button", { name: "Save" })) and immediately awaits the
"Assignment Updated" toaster without synchronizing the submission; add a
post-click synchronization step before asserting the toast by waiting for the
Save button to enter a loading/disabled state or for a network response that
indicates the update completed (e.g., waitForResponse matching the assignment
update endpoint), and also add an assertion branch that checks for the failure
toast title "Update Error" if the update doesn't finish; reference the existing
toaster title "Assignment Updated" (from
app/course/[course_id]/manage/assignments/[assignment_id]/edit/page.tsx) when
asserting success.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e5eba637-2625-43ce-8419-4fef8ef022df
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
app/course/[course_id]/manage/assignments/new/form.tsxapp/course/[course_id]/manage/course/grading-assignment-defaults/page.tsxsupabase/migrations/20260418100000_grading_assignment_defaults_and_reminders.sqltests/e2e/grading-assignment-defaults.test.tsx
Comment on lines
+209
to
+211
| await editForm.getByRole("button", { name: "Save" }).click(); | ||
|
|
||
| await expect(page.getByText("Assignment Updated")).toBeVisible({ timeout: 20_000 }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the exact toast message text used in the assignment edit flow
rg -n -i "assignment updated" --type=ts --type=tsx app/course 2>/dev/null || \
rg -n -i "assignment updated" -g '*.ts' -g '*.tsx' app/courseRepository: pawtograder/platform
Length of output: 479
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/e2e/grading-assignment-defaults.test.tsx"
echo "=== Test snippet around failing lines (190-225) ==="
nl -ba "$FILE" | sed -n '180,240p'
echo
echo "=== Locate toast/title strings in the test file ==="
rg -n --fixed-string "Assignment Updated" "$FILE" || true
rg -n -i "assignment updated" "$FILE" || true
echo
echo "=== Find where the UI string 'Assignment Updated' is defined/used ==="
rg -n --fixed-string "Assignment Updated" app tests || true
rg -n -i "assignment updated" app tests || true
echo
echo "=== Confirm whether a network/response wait exists near the Save click ==="
rg -n "waitForResponse|expect\\(.*response|waitUntil|route\\.|request" "$FILE" || true
rg -n "Save\"\\)\\.click|getByRole\\(\"button\", \\{ name: \"Save\"\\}\\)\\.click" "$FILE" || trueRepository: pawtograder/platform
Length of output: 177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/e2e/grading-assignment-defaults.test.tsx"
echo "=== Test snippet around failing lines (190-225) ==="
cat -n "$FILE" | sed -n '190,225p'
echo
echo "=== Locate toast/title strings in the test file ==="
rg -n --fixed-string "Assignment Updated" "$FILE" || true
rg -n -i "assignment updated" "$FILE" || true
echo
echo "=== Locate nearby Save click and subsequent waits/assertions ==="
rg -n 'getByRole\("button", \{ name: "Save"\}\)\.click\(\);' "$FILE" || true
rg -n 'getByText\("Assignment Updated"\)|getByText\(\x27Assignment Updated\x27\)|getByText' "$FILE" | head -n 50 || true
echo
echo "=== Confirm whether network/response waits exist near the Save click ==="
rg -n "waitForResponse|expect\\(.*response|waitUntil|route\\.|request|response" "$FILE" || trueRepository: pawtograder/platform
Length of output: 3793
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE_TEST="tests/e2e/grading-assignment-defaults.test.tsx"
echo "=== rg 'Assignment Updated' variants in test ==="
rg -n --fixed-strings "Assignment Updated" "$FILE_TEST" || true
rg -n -i "assignment updated" "$FILE_TEST" || true
echo
echo "=== rg Save click context: any loading/network waits nearby ==="
rg -n "getByRole\\(\"button\", \\{ name: \"Save\" \\}\\)\\.click\\(\\)" "$FILE_TEST"
rg -n "waitForResponse|route\\(|waitForURL|waitUntil|request|response|route\\.|supabase\\." "$FILE_TEST" || true
rg -n "toBeDisabled|loading|spinner|Saving|submit" "$FILE_TEST" || true
echo
echo "=== Show larger context around the failing assertion (200-220) ==="
sed -n '195,220p' "$FILE_TEST"Repository: pawtograder/platform
Length of output: 3908
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Use the path we already saw from the earlier rg result.
FILE_APP="app/course/[course_id]/manage/assignments/[assignment_id]/edit/page.tsx"
echo "=== Context around title: \"Assignment Updated\" ==="
# Print a small window around the match
line=$(rg -n --fixed-strings "title: \"Assignment Updated\"" "$FILE_APP" | head -n 1 | cut -d: -f1)
echo "Matched line: ${line}"
start=$((line-25))
end=$((line+25))
if [ "$start" -lt 1 ]; then start=1; fi
sed -n "${start},${end}p" "$FILE_APP"Repository: pawtograder/platform
Length of output: 1997
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE_TEST="tests/e2e/grading-assignment-defaults.test.tsx"
echo "=== All 'Assignment Updated' uses in the test file ==="
rg -n --fixed-strings "Assignment Updated" "$FILE_TEST" || true
echo
echo "=== Check for 'Update Error' toast assertions in the test file ==="
rg -n --fixed-strings "Update Error" "$FILE_TEST" || true
echo
echo "=== Search for 'toaster' / toast container usage in the test file ==="
rg -n "toaster|toast" "$FILE_TEST" || true
echo
echo "=== Show a bit more context around the Save click (280-360) to see other flows ==="
sed -n '240,360p' "$FILE_TEST"Repository: pawtograder/platform
Length of output: 4938
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE_TEST="tests/e2e/grading-assignment-defaults.test.tsx"
echo "=== Find if this flow sets template_repo (GitHub webhook) ==="
rg -n "template_repo|template repo|webhook|githubRepoConfigureWebhook|Watch" "$FILE_TEST" || true
echo
echo "=== Find label/select changes in the same test section as the Save click ==="
# print a broader window around the failing Save click to see what fields are set
sed -n '140,225p' "$FILE_TEST"Repository: pawtograder/platform
Length of output: 4576
Pipeline failure: add synchronization around Save before asserting the “Assignment Updated” toast
- The expected toast text matches the app implementation:
toaster.create({ title: "Assignment Updated", ... })inapp/course/[course_id]/manage/assignments/[assignment_id]/edit/page.tsx, so a capitalization/text mismatch is unlikely. - The test clicks Save and immediately waits up to 20s for the toast, with no assertion that the submission actually started/completed (no check for button loading/disabled, no network/response wait, and the existing
waitForTimeout(1500)occurs earlier). - Add a post-click synchronization step (e.g., wait for the Save button to enter a loading/disabled state or for the persisted assignment update to land) and/or assert the failure toast (“Update Error”) when the update doesn’t finish.
🧰 Tools
🪛 GitHub Actions: E2E Tests / 0_e2e-local.txt
[error] 211-211: Playwright expect(toBeVisible) timed out (20s). Missing UI text: getByText('Assignment Updated')
🤖 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 `@tests/e2e/grading-assignment-defaults.test.tsx` around lines 209 - 211, The
test clicks the Save button (editForm.getByRole("button", { name: "Save" })) and
immediately awaits the "Assignment Updated" toaster without synchronizing the
submission; add a post-click synchronization step before asserting the toast by
waiting for the Save button to enter a loading/disabled state or for a network
response that indicates the update completed (e.g., waitForResponse matching the
assignment update endpoint), and also add an assertion branch that checks for
the failure toast title "Update Error" if the update doesn't finish; reference
the existing toaster title "Assignment Updated" (from
app/course/[course_id]/manage/assignments/[assignment_id]/edit/page.tsx) when
asserting success.
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
2 participants
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
reset(...)and keeping self-review effect scoped to self-review fieldsremindersEnabledreadFOR UPDATEstate claim in cron runner, auto-assignment completion gating on function result, and pg_cron schedule exception handling-1soauto_assigned_atremains unset and cron retries after configuration is correctedtests/e2e/grading-assignment-defaults.test.tsxTargeted E2E Coverage Added
tests/e2e/grading-assignment-defaults.test.tsxnow covers two workflow tracks:Profile management + assignment create/edit wiring
Deadline automation execution path (submissions → deadline hit → assignment + reminders)
run_assignment_grading_automation()auto_assigned_at,last_reminder_sent_at, recipient count)reply_to,cc_emails, and intended recipient usersValidation
npm run formatnpx tsc --noEmitnpx eslint tests/e2e/grading-assignment-defaults.test.tsxTMPDIR=/dev/shm npx playwright test tests/e2e/grading-assignment-defaults.test.tsx --project=chromium --reporter=line --output=/dev/shm/pw-results --workers=1Notes
initdbwithNo space left on device(ENOSPC), which prevents test fixture creation (createClass) and end-to-end runtime execution in this VM.Summary by CodeRabbit
New Features
Tests