Expose submission limit to students #364
Conversation
Tested locally, should work to expose the autograder table to students & allow for reading submission limits
WalkthroughImplements an RPC-based fetch of per-assignment autograder submission limits, updates the assignment page to render an alert based on fetched limits, adds a secure SQL function for student-scoped access, and updates Supabase TypeScript definitions to include the new function and related types. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Page as Assignment Page
participant SB as Supabase Client
participant RPC as get_submissions_limits()
participant DB as Postgres
User->>Page: Open assignment
activate Page
Page->>SB: createClient()
Page->>SB: rpc("get_submissions_limits", { p_assignment_id })
activate SB
SB->>RPC: Invoke (SECURITY DEFINER)
activate RPC
RPC->>DB: SELECT autograder limits WITH student privilege check
DB-->>RPC: Rows (0 or 1)
deactivate RPC
SB-->>Page: Result rows
deactivate SB
Page->>Page: Derive submissionsPeriod, maxSubmissions
Page-->>User: Render assignment + conditional Alert
deactivate Page
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
app/course/[course_id]/assignments/[assignment_id]/page.tsx (3)
166-177: Simplify conditional rendering.Rendering an empty fragment
<></>whensubmissionsPeriod <= 0is unnecessary. You can returnnullor simply omit the else clause entirely.- {submissionsPeriod > 0 ? ( - <Box w="925px"> + {submissionsPeriod > 0 && ( + <Box w="925px"> <Alert.Root status="info" flexDirection="column" size="md"> <Alert.Title>Submission Limit for this assignment</Alert.Title> <Alert.Description> This assignment has a submission limit of {maxSubmissions} submissions per every {submissionsPeriod} hour(s). </Alert.Description> </Alert.Root> </Box> - ) : ( - <></> - )} + )}
167-167: Replace hardcoded width with responsive design.The hardcoded width of "925px" for the Alert box may not work well on smaller screens. Consider using a responsive width like
maxW="4xl"(used elsewhere in this file at line 155) or a percentage-based width.- <Box w="925px"> + <Box maxW="4xl">
81-97: Simplify autograder query - remove unnecessary pagination and sorting.The autograder query uses pagination with
pageSize: 1000and sorting bycreated_at, but since there should be only one autograder row per assignment (filtered by assignment_id), these parameters are unnecessary and add overhead.const { data: autograderData } = useList<AutograderWithAssignment>({ resource: "autograder", meta: { - select: "*", - order: "created_at, { ascending: false }" + select: "*" }, - pagination: { - pageSize: 1000 - }, - filters: autograderFilters, - sorters: [ - { - field: "created_at", - order: "desc" - } - ] + filters: autograderFilters });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/course/[course_id]/assignments/[assignment_id]/page.tsx(5 hunks)hooks/useAssignment.tsx(1 hunks)supabase/migrations/20251002165014_student_autograder_read.sql(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
hooks/useAssignment.tsx (1)
lib/TableController.ts (1)
TableController(461-2222)
app/course/[course_id]/assignments/[assignment_id]/page.tsx (1)
utils/supabase/DatabaseTypes.d.ts (1)
AutograderWithAssignment(586-592)
🪛 GitHub Actions: Lint
app/course/[course_id]/assignments/[assignment_id]/page.tsx
[error] 31-31: 'id' is assigned a value but never used. @typescript-eslint/no-unused-vars
🔇 Additional comments (2)
hooks/useAssignment.tsx (1)
312-317: Use ofidfilter is correct. The autograder table’sidcolumn stores the assignment ID (per schema comment), so.eq("id", assignment_id)is the proper filter.Likely an incorrect or invalid review comment.
app/course/[course_id]/assignments/[assignment_id]/page.tsx (1)
58-62: Verify autograder filter field
The filter currently uses{ field: "id", operator: "eq", … }, which targets the primary key. Confirm whether the Autograder table uses a foreign key column (e.g.assignment_id) for assignments and update the filter accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/course/[course_id]/assignments/[assignment_id]/page.tsx(5 hunks)hooks/useAssignment.tsx(3 hunks)supabase/migrations/20251002165014_student_autograder_read.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- supabase/migrations/20251002165014_student_autograder_read.sql
- hooks/useAssignment.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
app/course/[course_id]/assignments/[assignment_id]/page.tsx (1)
utils/supabase/DatabaseTypes.d.ts (1)
AutograderWithAssignment(586-592)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/course/[course_id]/assignments/[assignment_id]/page.tsx (1)
126-126: Add fallback value for maxSubmissions.Line 126 uses optional chaining but doesn't provide a fallback with nullish coalescing. If
max_submissions_countisnullorundefined, thenmaxSubmissionswill beundefined, causing "undefined" to render in the UI text at line 172.- const maxSubmissions = autograder?.[0]?.max_submissions_count; + const maxSubmissions = autograder?.[0]?.max_submissions_count ?? 0;
🧹 Nitpick comments (1)
app/course/[course_id]/assignments/[assignment_id]/page.tsx (1)
167-167: Consider also checking maxSubmissions in the condition.The condition only checks
submissionsPeriod > 0, but ifmaxSubmissionsis 0 or falsy, showing the limit alert may not be meaningful. Consider adding a check formaxSubmissions.- {submissionsPeriod > 0 ? ( + {submissionsPeriod > 0 && maxSubmissions ? (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/course/[course_id]/assignments/[assignment_id]/page.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/course/[course_id]/assignments/[assignment_id]/page.tsx (1)
utils/supabase/DatabaseTypes.d.ts (1)
AutograderWithAssignment(586-592)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/course/[course_id]/assignments/[assignment_id]/page.tsx (1)
58-62: Fix autograder filter column.Filtering on
idstill mismatches the autograder schema, so every request returns an empty set and students never see their limits. Filter onassignment_idinstead.- filters.push({ field: "id", operator: "eq", value: assignment_id }); + filters.push({ field: "assignment_id", operator: "eq", value: assignment_id });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/course/[course_id]/assignments/[assignment_id]/page.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/course/[course_id]/assignments/[assignment_id]/page.tsx (1)
utils/supabase/DatabaseTypes.d.ts (1)
AutograderWithAssignment(586-592)
|
Very nice contribution, thank you! I am sorry but I added an incorrect detail in the blueprint for this issue: I think it is best to avoid making the autograder rows visible to students. The table also contains data that might be sensitive - the Thanks! |
|
I have coded myself into a corner I feel, I've been working on fixing the autograder exposure for an unhealthy amount of hours but I haven't been able to get it working like I did previously. I'm going to commit my changes and maybe CodeRabbit will be able to help. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
app/course/[course_id]/assignments/[assignment_id]/page.tsx (4)
82-82: Move client creation outside the component.Creating a new Supabase client on every render is inefficient and could lead to connection issues. Move the
createClient()call outside the component or useuseMemo.Apply this diff:
+const supabaseClient = createClient(); + export default function AssignmentPage() { const { course_id, assignment_id } = useParams(); ... - const client = createClient(); useEffect(() => { async function fetchSubmissionLimits() { - const { data, error } = await client + const { data, error } = await supabaseClient .rpc('get_submissions_limits');
92-92: Fix useEffect dependency array.The dependency array
[autograderData]includes a ref that never changes identity, which is misleading. Since the effect should run once on mount to fetch data, use an empty array[]. Alternatively, if you need it to refetch whenassignment_idchanges, includeassignment_idinstead.Apply this diff:
fetchSubmissionLimits(); - }, [autograderData]); + }, [assignment_id]);
59-63: Remove unused autograderFilters.The
autograderFiltersvariable is defined but never used (the commented-out code at lines 94-110 would have used it). Remove it to reduce clutter.Apply this diff:
}, [assignment_id, assignmentGroup, private_profile_id]); - const autograderFilters = useMemo(() => { - const filters: CrudFilter[] = []; - filters.push({ field: "id", operator: "eq", value: assignment_id }); - return filters; - }, [assignment_id]); const { data: submissionsData } = useList<SubmissionWithGraderResultsAndReview>({
137-140: Add explicit null check for autograder data.While the optional chaining handles undefined cases, it's clearer to explicitly check if
autograderexists and has data before accessing. This also prevents displaying a "0 hours" message when no data is available.Consider this approach for clarity:
const autograderRow = autograder?.[0]; - const submissionsPeriod = autograderRow?.max_submissions_period_secs != null - ? secondsToHours(autograderRow.max_submissions_period_secs) : 0; - const maxSubmissions = autograderRow?.max_submissions_count; + const submissionsPeriod = autograderRow?.max_submissions_period_secs + ? secondsToHours(autograderRow.max_submissions_period_secs) + : null; + const maxSubmissions = autograderRow?.max_submissions_count ?? null;Then adjust line 181 to check for
submissionsPeriod != null && maxSubmissions != nullinstead ofsubmissionsPeriod > 0.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/course/[course_id]/assignments/[assignment_id]/page.tsx(6 hunks)supabase/migrations/20251002165014_student_autograder_read.sql(1 hunks)utils/supabase/DatabaseTypes.d.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T02:47:03.315Z
Learnt from: Ari-S-123
PR: pawtograder/platform#168
File: app/course/[course_id]/manage/course/due-date-extensions/tables/rosterTokensTable.tsx:0-0
Timestamp: 2025-08-29T02:47:03.315Z
Learning: The assignment_groups_members table already has RLS enabled and proper policies in place that restrict access based on class authorization. Users should not suggest adding RLS when it already exists.
Applied to files:
supabase/migrations/20251002165014_student_autograder_read.sql
🧬 Code graph analysis (2)
utils/supabase/DatabaseTypes.d.ts (1)
utils/supabase/SupabaseTypes.d.ts (1)
Database(3-9741)
app/course/[course_id]/assignments/[assignment_id]/page.tsx (1)
utils/supabase/client.ts (1)
createClient(6-17)
🔇 Additional comments (3)
app/course/[course_id]/assignments/[assignment_id]/page.tsx (2)
186-186: LGTM: UI text grammar addressed.The pluralization logic and grammar have been corrected based on previous review feedback. The text now correctly handles singular/plural forms.
113-113: Critical: Fix ref property access.
autograderDatais a ref (created withuseRef(null)), so you must access its value via.current. The current codeautograderData?.datawill always be undefined because refs don't have adataproperty.Apply this diff:
const submissions = submissionsData?.data; - const autograder = autograderData?.data; + const autograder = autograderData.current;Likely an incorrect or invalid review comment.
utils/supabase/DatabaseTypes.d.ts (1)
602-608: Remove or implement StudentAutograder type
StudentAutograderreferences a non-existentstudents_autogradertable/view and appears unused. Remove this type or add the corresponding view/table to your schema.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
app/course/[course_id]/assignments/[assignment_id]/page.tsx (1)
25-25: Remove unused useState import.The
useStatehook is imported but never used; remove it to clean up the imports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/course/[course_id]/assignments/[assignment_id]/page.tsx(6 hunks)supabase/migrations/20251002165014_student_autograder_read.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T02:47:03.315Z
Learnt from: Ari-S-123
PR: pawtograder/platform#168
File: app/course/[course_id]/manage/course/due-date-extensions/tables/rosterTokensTable.tsx:0-0
Timestamp: 2025-08-29T02:47:03.315Z
Learning: The assignment_groups_members table already has RLS enabled and proper policies in place that restrict access based on class authorization. Users should not suggest adding RLS when it already exists.
Applied to files:
supabase/migrations/20251002165014_student_autograder_read.sql
🧬 Code graph analysis (1)
app/course/[course_id]/assignments/[assignment_id]/page.tsx (1)
utils/supabase/client.ts (1)
createClient(6-17)
🔇 Additional comments (2)
app/course/[course_id]/assignments/[assignment_id]/page.tsx (2)
120-123: Safe value derivation with proper guards.The autograder value derivation uses appropriate optional chaining and nullish coalescing to handle undefined/null safely. Once the ref access issue on line 96 is fixed, this code will work correctly.
164-175: Well-structured alert with proper pluralization.The conditional alert is properly implemented with correct pluralization logic for both "submission(s)" and "hour(s)", and only displays when there's an actual submission period configured.
It should work now without exposing the entirety of the autograder table to the student
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/course/[course_id]/assignments/[assignment_id]/page.tsx (1)
159-170: Use responsive width and simplify conditional rendering.The hardcoded width
w="925px"(line 160) is not responsive and may cause layout issues on different screen sizes. UsemaxWwith a logical breakpoint instead.Additionally, the empty fragment
<></>in the else branch is unnecessary and can be removed.Apply this diff:
{submissionsPeriod > 0 ? ( - <Box w="925px"> + <Box maxW="4xl"> <Alert.Root status="info" flexDirection="column" size="md"> <Alert.Title>Submission Limit for this assignment</Alert.Title> <Alert.Description> This assignment has a submission limit of {maxSubmissions} submission{maxSubmissions !== 1 ? 's' : ''} per {submissionsPeriod} hour{submissionsPeriod !== 1 ? 's' : ''}. </Alert.Description> </Alert.Root> </Box> - ) : ( - <></> - )} + ) : null}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/course/[course_id]/assignments/[assignment_id]/page.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/course/[course_id]/assignments/[assignment_id]/page.tsx (1)
utils/supabase/client.ts (1)
createClient(6-17)
🔇 Additional comments (1)
app/course/[course_id]/assignments/[assignment_id]/page.tsx (1)
115-118: LGTM!The null-safety checks and optional chaining correctly handle cases where the autograder data might be missing or incomplete. The computed values are derived safely.
|
Looks like it's almost there!
|
|
@jon-bell I think that was already taken care of? The RPC is being used in a |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/course/[course_id]/assignments/[assignment_id]/page.tsx(5 hunks)supabase/functions/_shared/SupabaseTypes.d.ts(1 hunks)utils/supabase/SupabaseTypes.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/course/[course_id]/assignments/[assignment_id]/page.tsx
🔇 Additional comments (1)
utils/supabase/SupabaseTypes.d.ts (1)
9355-9362: Ensure RPC output nullability is accurate.
max_submissions_countandmax_submissions_period_secscome straight frompublic.autograder, where both columns are nullable. Unless the SQL function filters out rows with nulls (or casts toNOT NULL), Supabase can returnnullhere, which would violate this type definition. Please confirm the RPC guarantees non-null values for these fields; otherwise the type should permitnullto avoid runtime surprises.
|
To get this small change in faster, I have made the following improvements directly:
|
|
Thank you very much for the contribution! |
|
@jon-bell Of course! Thank you for fixing it up and merging the PR. This PR fixes a problem I had run into myself when using Pawtograder for a class. I'm looking forward to contributing more to Pawtograder in the future when my schedule allows! |
Fixes issue #222 . Tested locally, should work to expose the autograder table to students & allow for reading submission limits. I haven't worked with SQL in 2 years so the RLS policy may be rough around the edges & I can fix if need be. Otherwise everything should be fine!
Summary by CodeRabbit
New Features
Chores