Open
Conversation
Contributor
WalkthroughAdds a timezone-aware Review due date input to the bulk-assignment UI (by_lab_leaders mode) and introduces a database trigger that marks sibling review_assignments and submission_reviews complete when appropriate. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DB_ReviewAssignment as ReviewAssignment (table)
participant Trigger as check_and_complete_submission_review()
participant DB_SubmissionReview as SubmissionReview (table)
participant DB_SiblingAssignments as SiblingAssignments (table)
User->>DB_ReviewAssignment: update completed_at (set)
DB_ReviewAssignment->>Trigger: AFTER UPDATE trigger fires
Trigger->>DB_SubmissionReview: acquire advisory lock, read rubric_id
alt rubric_id is present
Trigger->>DB_SiblingAssignments: query sibling assignments for submission_review_id
Trigger->>DB_SiblingAssignments: update siblings whose parts ⊆ completer.parts to completed
Trigger->>DB_SiblingAssignments: evaluate remaining uncovered parts and related comments/checks
alt no blocking uncovered parts
Trigger->>DB_SubmissionReview: update submission_review.completed_at = now()
end
end
Trigger-->>DB_ReviewAssignment: trigger ends
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 2
🧹 Nitpick comments (1)
app/course/[course_id]/manage/assignments/[assignment_id]/reviews/bulk-assign/page.tsx (1)
1684-1722: Extract this due-date control into a shared helper/component.This logic is duplicated in two branches with near-identical parse/format code, which makes timezone behavior easy to desynchronize.
🤖 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]/reviews/bulk-assign/page.tsx around lines 1684 - 1722, Extract the date-time input and TZDate parsing/formatting into a reusable component (e.g., DueDateControl) that accepts props {value: string, onChange: (isoString:string)=>void, timeZone?:string}. Move the current value formatting logic (convert dueDate -> new Date(dueDate).toLocaleString("sv-SE", {timeZone}).replace(" ", "T")) and the onChange parsing logic (split "YYYY-MM-DDTHH:MM", build new TZDate(...) with timeZone, call onChange(tzDate.toString()) or onChange("") when empty) into that component, keep the same defaults for course.time_zone ("America/New_York"), and export it for reuse; then replace the duplicated blocks with <DueDateControl value={dueDate} onChange={setDueDate} timeZone={course.time_zone} /> ensuring you import/export the new component where needed and preserve use of TZDate, Field.Label, and Input 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/[assignment_id]/reviews/bulk-assign/page.tsx:
- Around line 1690-1717: The dueDate state is being set with tzDate.toString(),
causing format inconsistency with the ISO string initialization and breaking new
TZDate(dueDate, course.time_zone) parsing; update the setters (the setDueDate
calls where TZDate instance tzDate is used, e.g., in the onChange handler and
the other occurrence later) to call tzDate.toISOString() instead of
tzDate.toString() so dueDate remains a stable ISO string for downstream new
TZDate(dueDate, course.time_zone) parsing.
In `@supabase/migrations/20260225000000_complete_sibling_review_assignments.sql`:
- Around line 21-22: This SECURITY DEFINER function references unqualified
tables (submission_reviews, review_assignments, review_assignment_rubric_parts)
and must declare an explicit search_path to prevent schema shadowing; update the
function declaration to include a SET search_path clause (for example SET
search_path = public, pg_temp) in the SECURITY DEFINER header (i.e., add the SET
search_path before AS $$ in the function declaration, matching the pattern used
by other functions).
---
Nitpick comments:
In
`@app/course/`[course_id]/manage/assignments/[assignment_id]/reviews/bulk-assign/page.tsx:
- Around line 1684-1722: Extract the date-time input and TZDate
parsing/formatting into a reusable component (e.g., DueDateControl) that accepts
props {value: string, onChange: (isoString:string)=>void, timeZone?:string}.
Move the current value formatting logic (convert dueDate -> new
Date(dueDate).toLocaleString("sv-SE", {timeZone}).replace(" ", "T")) and the
onChange parsing logic (split "YYYY-MM-DDTHH:MM", build new TZDate(...) with
timeZone, call onChange(tzDate.toString()) or onChange("") when empty) into that
component, keep the same defaults for course.time_zone ("America/New_York"), and
export it for reuse; then replace the duplicated blocks with <DueDateControl
value={dueDate} onChange={setDueDate} timeZone={course.time_zone} /> ensuring
you import/export the new component where needed and preserve use of TZDate,
Field.Label, and Input semantics.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/course/[course_id]/manage/assignments/[assignment_id]/reviews/bulk-assign/page.tsxsupabase/migrations/20260225000000_complete_sibling_review_assignments.sql
app/course/[course_id]/manage/assignments/[assignment_id]/reviews/bulk-assign/page.tsx
Show resolved
Hide resolved
supabase/migrations/20260225000000_complete_sibling_review_assignments.sql
Show resolved
Hide resolved
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
Contributor
Author
|
@coderabbitai full-review |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
Author
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Author
|
@CodeRabbit-ai full-review |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/course/[course_id]/manage/assignments/[assignment_id]/reviews/bulk-assign/page.tsx (1)
1684-1722: Deduplicate due-date parse/format logic to prevent mode drift.The same timezone parse/format logic is implemented twice. Consider extracting shared helpers (or a small
ReviewDueDateFieldcomponent) so future changes stay consistent across both paths.♻️ Proposed refactor sketch
+ const formatDueDateForInput = useCallback((iso: string) => { + if (!iso) return ""; + return new Date(iso) + .toLocaleString("sv-SE", { timeZone: course.time_zone ?? "America/New_York" }) + .replace(" ", "T"); + }, [course.time_zone]); + + const parseDueDateFromInput = useCallback((value: string) => { + if (!value) return ""; + const [date, time] = value.split("T"); + const [year, month, day] = date.split("-").map(Number); + const [hour, minute] = time.split(":").map(Number); + return new TZDate( + year, month - 1, day, hour, minute, 0, 0, + course.time_zone ?? "America/New_York" + ).toISOString(); + }, [course.time_zone]);- value={/* inline conversion */} + value={formatDueDateForInput(dueDate)} ... - onChange={(e) => { /* inline parsing */ }} + onChange={(e) => setDueDate(parseDueDateFromInput(e.target.value))}Also applies to: 2114-2152
🤖 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]/reviews/bulk-assign/page.tsx around lines 1684 - 1722, Duplicate date parse/format logic around the Review due date input (the Input onChange and the value formatter that both convert between ISO and course.time_zone using TZDate) should be consolidated: extract a small helper pair (e.g., parseCourseLocalToISOString(value, timeZone) and formatISOStringToCourseLocal(iso, timeZone)) or a ReviewDueDateField component that encapsulates the value computation and onChange using TZDate, and replace the inline logic in the value prop and the onChange handler (references: dueDate, setDueDate, TZDate, course.time_zone, Field.Root, Input) with calls to those helpers/component so both paths use the same implementation and avoid drift.supabase/migrations/20260225000000_complete_sibling_review_assignments.sql (1)
153-167: UseUNION ALLinEXISTSprobes to avoid unnecessary dedup work.In these boolean existence checks,
UNIONperforms deduplication that is not needed.⚡ Suggested SQL tweak
- union + union all select 1 from submission_file_comments sfc ... - union + union all select 1 from submission_artifact_comments sac ... - union + union all select 1 from submission_file_comments sfc ... - union + union all select 1 from submission_artifact_comments sacAlso applies to: 184-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260225000000_complete_sibling_review_assignments.sql` around lines 153 - 167, The EXISTS probe is using UNION which forces unnecessary deduplication; change each UNION to UNION ALL in the three-part subquery that checks submission_comments, submission_file_comments, and submission_artifact_comments (the predicates referencing target_submission_review_id and rc.id) so the EXISTS can short-circuit without de-duplicating, and apply the same UNION -> UNION ALL change to the equivalent block around lines referenced (the other occurrence that queries those same tables).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/course/`[course_id]/manage/assignments/[assignment_id]/reviews/bulk-assign/page.tsx:
- Around line 1684-1722: Duplicate date parse/format logic around the Review due
date input (the Input onChange and the value formatter that both convert between
ISO and course.time_zone using TZDate) should be consolidated: extract a small
helper pair (e.g., parseCourseLocalToISOString(value, timeZone) and
formatISOStringToCourseLocal(iso, timeZone)) or a ReviewDueDateField component
that encapsulates the value computation and onChange using TZDate, and replace
the inline logic in the value prop and the onChange handler (references:
dueDate, setDueDate, TZDate, course.time_zone, Field.Root, Input) with calls to
those helpers/component so both paths use the same implementation and avoid
drift.
In `@supabase/migrations/20260225000000_complete_sibling_review_assignments.sql`:
- Around line 153-167: The EXISTS probe is using UNION which forces unnecessary
deduplication; change each UNION to UNION ALL in the three-part subquery that
checks submission_comments, submission_file_comments, and
submission_artifact_comments (the predicates referencing
target_submission_review_id and rc.id) so the EXISTS can short-circuit without
de-duplicating, and apply the same UNION -> UNION ALL change to the equivalent
block around lines referenced (the other occurrence that queries those same
tables).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/course/[course_id]/manage/assignments/[assignment_id]/reviews/bulk-assign/page.tsxsupabase/migrations/20260225000000_complete_sibling_review_assignments.sql
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 by CodeRabbit
Note
Medium Risk
Updates database trigger logic that auto-completes
review_assignments/submission_reviews, which can change completion semantics and affect grading workflows if the subset/equality checks are wrong. UI changes are straightforward but touch timezone/date serialization, which is prone to edge cases.Overview
Adds a review due-date input to the lab-leader bulk-assignment flow and standardizes due-date handling to store ISO timestamps (course-timezone aware) instead of locale-dependent strings.
Updates the
check_and_complete_submission_review()trigger to auto-complete siblingreview_assignmentswhen a completed assignment covers the same or a superset of rubric parts, and then marks thesubmission_reviewcomplete once no incomplete assignments remain; also adds an index to speed up incomplete-assignment lookup.Written by Cursor Bugbot for commit 48058d1. This will update automatically on new commits. Configure here.