Remove heavy RAM-consuming maintenance functions #373
Conversation
|
Warning Rate limit exceeded@jon-bell has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 38 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (4)
WalkthroughReplaces a materialized-view summary with a new realtime Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Webhook
participant Edge as Webhook Function
participant Redis as Redis (TTL state)
participant PG as Postgres
GH->>Edge: Deliver event (delivery id, payload)
Edge->>Redis: GET/SETNX status key (TTL 3h)
alt Duplicate or Completed
Redis-->>Edge: status indicates processed
Edge-->>GH: 200 Early exit
else First delivery or retry
Edge->>PG: Persist event / enqueue processing
PG-->>Edge: OK
Edge->>Redis: UPDATE status (completed/attempts/errors)
Edge-->>GH: 200 OK
end
note right of PG: Trigger maintain_workflow_runs upserts per-run state
sequenceDiagram
autonumber
participant UI as Runs Page
participant TC as TableController
participant RPC as Supabase RPC
participant DB as workflow_runs
UI->>TC: Initialize (class_id, sort, pageSize)
TC->>RPC: call get_workflow_events_summary_for_class(class_id)
RPC->>DB: SELECT FROM workflow_runs (RLS)
DB-->>RPC: Rows
RPC-->>TC: Rows + computed filter options
TC-->>UI: Render rows, filters, links
UI->>TC: Click Refresh
TC->>RPC: Refetch
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/course/[course_id]/manage/workflow-runs/runs/page.tsx(13 hunks)supabase/functions/_shared/SupabaseTypes.d.ts(2 hunks)supabase/functions/github-repo-webhook/index.ts(5 hunks)supabase/migrations/20251006232741_reduce-db-ram-usage.sql(1 hunks)utils/supabase/SupabaseTypes.d.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
supabase/functions/github-repo-webhook/index.ts (1)
supabase/functions/_shared/Redis.ts (1)
Redis(12-488)
app/course/[course_id]/manage/workflow-runs/runs/page.tsx (4)
utils/supabase/client.ts (1)
createClient(6-17)hooks/useCourseController.tsx (2)
useCourseController(1387-1393)classRealTimeController(406-411)lib/TableController.ts (2)
TableController(461-2222)rows(2215-2217)hooks/useTableControllerTable.tsx (1)
useTableControllerTable(69-175)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint (22.x)
- GitHub Check: lint (22.x)
- GitHub Check: deploy
🔇 Additional comments (3)
supabase/functions/_shared/SupabaseTypes.d.ts (2)
7715-7842: Workflow run table typings look consistentSchema, optional fields, and FK mappings all line up with the new migration. 👍
9369-9390: Updated summary return type matches the new tableThe function signature now mirrors the workflow_runs columns and keeps nullability intact.
utils/supabase/SupabaseTypes.d.ts (1)
7715-7843: Schema typings look consistent
Theworkflow_runstable definition and the updatedget_workflow_events_summary_for_classreturn type line up with the migration plan—nullability, required Insert fields, and relationship metadata all look good.Also applies to: 9368-9390
There was a problem hiding this comment.
Actionable comments posted: 2
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/workflow-runs/runs/page.tsx (1)
332-356: Don’t hide zero durations0s currently renders as “-” due to falsy checks. Use null/undefined checks.
- const seconds = getValue() as number; - if (!seconds) return <Text fontSize="sm">-</Text>; + const seconds = getValue() as number | null | undefined; + if (seconds === null || seconds === undefined) return <Text fontSize="sm">-</Text>; @@ - {Math.round(seconds)}s + {Math.round(seconds)}s- const seconds = getValue() as number; - if (!seconds) return <Text fontSize="sm">-</Text>; + const seconds = getValue() as number | null | undefined; + if (seconds === null || seconds === undefined) return <Text fontSize="sm">-</Text>; @@ - {Math.round(seconds)}s + {Math.round(seconds)}sAlso applies to: 365-389
♻️ Duplicate comments (1)
app/course/[course_id]/manage/workflow-runs/runs/page.tsx (1)
433-475: Resolved: filter options recompute when rows changeThe memo now depends on workflowRuns, fixing the stale-options issue noted previously.
🧹 Nitpick comments (6)
app/course/[course_id]/manage/workflow-runs/runs/page.tsx (6)
480-489: Use IconButton’s icon prop (children are ignored/not styled consistently)Switch to the icon prop for correct sizing/accessibility.
- <IconButton - aria-label="Refresh workflow runs" - size="sm" - variant="outline" - onClick={async () => { - await controller?.refetchAll(); - }} - > - <LuRefreshCw /> - </IconButton> + <IconButton + aria-label="Refresh workflow runs" + size="sm" + variant="outline" + icon={<LuRefreshCw />} + onClick={async () => { + await controller?.refetchAll(); + }} + />Based on learnings
518-532: Chakra Icon prop: use boxSize, not sizeReplace size="md" with boxSize to avoid invalid DOM attrs and ensure consistent sizing.
- asc: ( - <Icon size="md"> + asc: ( + <Icon boxSize={4}> <FaSortUp /> </Icon> ), - desc: ( - <Icon size="md"> + desc: ( + <Icon boxSize={4}> <FaSortDown /> </Icon> ) }[header.column.getIsSorted() as string] ?? ( - <Icon size="md"> + <Icon boxSize={4}> <FaSort /> </Icon> )}Based on learnings
98-101: Avoid masking 0 for numeric cellsUse nullish coalescing instead of logical OR.
- <Text fontSize="sm">{(getValue() as number) || "-"}</Text> + <Text fontSize="sm">{(getValue() as number | null | undefined) ?? "-"}</Text>- <Text fontSize="sm">{(getValue() as number) || "-"}</Text> + <Text fontSize="sm">{(getValue() as number | null | undefined) ?? "-"}</Text>Also applies to: 315-317
406-412: Scope realtime updates to this class_idPass a realtimeFilter so the controller ignores events for other classes; reduces noise and memory.
return new TableController({ query: query, client: supabase, table: "workflow_runs", - classRealTimeController + classRealTimeController, + realtimeFilter: { class_id: Number(course_id) } });
273-283: Tighten status filtering to exact matchSubstring match can yield false positives; use equality against option values.
- const filterArray = Array.isArray(filterValue) ? filterValue : [filterValue]; - return filterArray.some((filter: string) => status.includes(filter.toLowerCase())); + const filterArray = Array.isArray(filterValue) ? filterValue : [filterValue]; + return filterArray.some((filter: string) => status === filter.toLowerCase());
542-549: Preserve option labels in the select chipsMap current filter values to the actual option objects so labels (e.g., “#123”) remain consistent.
- value={ - Array.isArray(header.column.getFilterValue()) - ? (header.column.getFilterValue() as string[]).map((v) => ({ - label: v, - value: v - })) - : [] - } + value={ + Array.isArray(header.column.getFilterValue()) + ? options.filter((opt) => + (header.column.getFilterValue() as string[]).includes(opt.value) + ) + : [] + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/course/[course_id]/manage/workflow-runs/runs/page.tsx(13 hunks)supabase/migrations/20251006232741_reduce-db-ram-usage.sql(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/course/[course_id]/manage/workflow-runs/runs/page.tsx (5)
components/ui/link.tsx (1)
Link(4-42)utils/supabase/client.ts (1)
createClient(6-17)hooks/useCourseController.tsx (2)
useCourseController(1387-1393)classRealTimeController(406-411)lib/TableController.ts (1)
TableController(461-2222)hooks/useTableControllerTable.tsx (1)
useTableControllerTable(69-175)
🪛 GitHub Actions: Lint
app/course/[course_id]/manage/workflow-runs/runs/page.tsx
[error] 1-1: Code style issues found by Prettier. Run 'prettier --write' to fix formatting.
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/course/[course_id]/manage/workflow-runs/runs/page.tsx (1)
1-1: Run Prettier to fix formatting and unblock CI.The Prettier formatting issue was flagged in a previous review. Please run the following command to fix formatting:
npx prettier --write "app/course/[course_id]/manage/workflow-runs/runs/page.tsx"
🧹 Nitpick comments (2)
app/course/[course_id]/manage/workflow-runs/runs/page.tsx (2)
56-84: Consider extracting GitHub link rendering into a helper function.The Run ID and Commit columns have similar link-rendering logic. If you anticipate adding more GitHub-linked columns, extracting this into a reusable helper function would reduce duplication.
Example refactor:
const renderGitHubLink = ( text: string, url: string, repoName: string | null | undefined ) => { if (!repoName) { return ( <Text fontFamily="mono" fontSize={text.startsWith('#') ? 'sm' : 'xs'} color={text.startsWith('#') ? undefined : 'fg.muted'}> {text} </Text> ); } return ( <Link href={url} target="_blank" rel="noopener noreferrer" color="blue.600" _hover={{ color: "blue.700", textDecoration: "underline" }} > <HStack gap={1}> <Text fontFamily="mono" fontSize={text.startsWith('#') ? 'sm' : 'xs'}> {text} </Text> <Icon boxSize={3}> <LuExternalLink /> </Icon> </HStack> </Link> ); };Then use:
cell: ({ getValue, row }) => { const runId = getValue() as number; const repoName = row.original.repository_name; return renderGitHubLink( `#${runId}`, `https://github.com/${repoName}/actions/runs/${runId}`, repoName ); }Also applies to: 166-194
395-395: Remove unnecessary memoization ofcreateClient().The
useMemoaroundcreateClient()doesn't provide any benefit sincecreateClient()returns a new Supabase client instance each time. The memoization prevents the reference from changing, but since the underlying client is new anyway, this doesn't optimize anything meaningful.Apply this diff:
- const supabase = useMemo(() => createClient(), []); + const supabase = createClient();Or, if you intend to reuse the same client instance across re-renders (which is the correct approach), move the client creation outside the component or use a singleton pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/course/[course_id]/manage/workflow-runs/runs/page.tsx(13 hunks)supabase/migrations/20251006232741_reduce-db-ram-usage.sql(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/course/[course_id]/manage/workflow-runs/runs/page.tsx (4)
utils/supabase/client.ts (1)
createClient(6-17)hooks/useCourseController.tsx (2)
useCourseController(1387-1393)classRealTimeController(406-411)lib/TableController.ts (1)
TableController(461-2222)hooks/useTableControllerTable.tsx (1)
useTableControllerTable(69-175)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (7)
app/course/[course_id]/manage/workflow-runs/runs/page.tsx (2)
433-474: LGTM! The filter option cache is now correctly reactive.The
columnUniqueValuesmemo now depends onworkflowRuns, which ensures filter options are recomputed when data arrives. This correctly addresses the previous issue where the memo captured an empty row model on first render and never updated.
478-490: LGTM! Refresh button, filters, and table rendering look solid.The refresh button correctly triggers
controller?.refetchAll()with optional chaining for safety. The dynamic filter UI usingCreatableSelectis properly configured with:
- Multi-select enabled
- Dynamic options from
columnUniqueValues- Correct value transformation and change handlers
- Appropriate styling
The header sorting indicators and row rendering are implemented correctly.
Also applies to: 505-577, 580-588
supabase/migrations/20251006232741_reduce-db-ram-usage.sql (5)
8-137: LGTM! Table schema, constraints, and RLS policies are well-designed.The
workflow_runstable schema is comprehensive with:
- Proper unique constraint on
(workflow_run_id, run_attempt, class_id)- Foreign keys with appropriate cascade behavior
- Indexes optimized for common query patterns
- RLS policies that correctly restrict access to instructors and service_role
The backfill from the materialized view preserves historical data safely.
139-322: LGTM! The trigger function correctly handles out-of-order webhook delivery.The
maintain_workflow_runs()function properly addresses both past review comments:
Requested event handling (lines 185-208): Uses
ON CONFLICT DO UPDATEwith COALESCE logic to backfillrequested_atand metadata fields when later events arrive first. This prevents data loss from out-of-order webhooks.In-progress event handling (lines 251-260): Correctly backfills
run_time_secondswhenin_progressarrives aftercompleted, ensuring duration metrics are preserved.All three event handlers (
requested,in_progress,completed) use consistent COALESCE-based backfilling for metadata fields and recalculate timing metrics when both required timestamps become available. The function is well-optimized with early exits for irrelevant events.
323-620: LGTM! Functions correctly migrated to useworkflow_runstable.All public functions have been properly updated:
get_workflow_events_summary_for_classnow returnsSETOF workflow_runswith efficient ordering and LIMITget_workflow_statisticscorrectly calculates metrics fromworkflow_runswith proper authorization checksget_all_class_metricsaggregates across all classes using the new table- Obsolete
refresh_workflow_events_summary()function and cron job are safely removed with defensive error handlingThe migration from materialized view to trigger-maintained table is complete and correct.
621-664: LGTM! Index cleanup achieves significant RAM reduction.The index management correctly:
- Drops 12 unused
workflow_eventsindexes (~210 MB recovery)- Drops unused
grader_result_outputandsubmissionsindexes (~60 MB recovery)- Adds targeted indexes to support the optimized
check_assignment_deadlines_passedfunction- Total RAM recovery: ~270 MB, directly addressing the PR objective
The new indexes (
idx_review_assignments_assignment_assignee_exists,idx_submissions_assignment_profile_group_active) are appropriately scoped to support specific query patterns.
666-808: LGTM! Highly optimized deadline processing function.The
check_assignment_deadlines_passed()function is well-optimized with:
- MATERIALIZED CTEs for better query planning and performance
- Time-based pre-filtering (30-day lookback) to dramatically reduce cross-join size
- Early Anti Join on
review_assignmentsbefore expensive submission lookups- Separated processing for submission_reviews and review_assignments creation
- Efficient index usage via the newly added composite indexes
The optimization strategy is clearly documented in the comment block, and the function correctly handles both individual and group submissions. The addition of
idx_submission_comments_updated_atat line 808 is appropriate for other query patterns.
Summary by CodeRabbit
New Features
Improvements
Performance
Bug Fixes