Merged
Conversation
…or late detection - Remove all createCheckRun/updateCheckRun API calls from webhook, create-submission, and submit-feedback - Make repository_check_runs.check_run_id nullable, add unique on (repository_id, sha) - Use decoded.actor (GitHub username) for role lookup instead of profile_id - Use repository_check_runs.created_at for late detection instead of current time - Drop fallback path when check run is missing; fail fast with UserVisibleError
Contributor
WalkthroughAdds org/method circuit-breaker checks to GitHub webhook and check_run flows to fast-fail when circuits are open, classifies GitHub rate-limit errors (including installationId), makes Changes
Sequence DiagramsequenceDiagram
participant Webhook as GitHub Webhook
participant Handler as Push Handler
participant CB as Circuit Breaker RPC
participant DB as Database
participant Sentry as Sentry
participant GitHub as GitHub API
Webhook->>Handler: POST push event / check_run trigger
Handler->>Handler: extract org from repo
Handler->>CB: checkCircuitBreakerOpen(adminSupabase, org, method)
CB->>DB: RPC get_github_circuit_breaker_statuses(org, method)
DB-->>CB: circuit records (isOpen?, open_until, scope, reason)
CB-->>Handler: {isOpen, reason, openUntil, circuitScope}
alt Circuit Open
Handler->>Sentry: set tags (org, method, circuitScope, openUntil)
Handler-->>Webhook: fail fast with circuit details
else Circuit Closed
Handler->>GitHub: triggerWorkflow / other API calls
GitHub-->>Handler: success / rate-limit error
alt Rate-limit error
Handler->>Sentry: fingerprint ["github-rate-limit", type, method], add installationId in context
Handler->>DB: only update local DB state (no mid-flight check_run completions)
Handler-->>Webhook: surface rate-limit error
else Success
Handler->>DB: insert/update repository_check_runs, submissions
DB-->>Handler: ack
Handler-->>Webhook: success
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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.
🧹 Nitpick comments (1)
supabase/functions/github-repo-webhook/index.ts (1)
287-301: Circuit breaker check runs N times inside the loop — move it before theforblock.
checkCircuitBreakerOpenis called once per commit, issuing up to two RPC round-trips on each iteration. Since circuit state is stable for the lifetime of the request, all iterations after the first redundantly re-query the same result. Move the check (and its throw) to before the loop.♻️ Proposed fix
+ // Check circuit breaker once before processing commits - fail fast if rate limited + const circuitStatus = await checkCircuitBreakerOpen(adminSupabase, org, "create_repo", scope); + if (circuitStatus.isOpen) { + const openUntil = circuitStatus.openUntil ? new Date(circuitStatus.openUntil).toLocaleString() : "unknown"; + const scopeInfo = circuitStatus.circuitScope === "org_method" ? ` (method: create_repo)` : ""; + throw new Error( + `Circuit breaker open for org ${org}${scopeInfo}: GitHub API operations temporarily unavailable. ` + + `Reason: ${circuitStatus.reason || "Rate limit or error threshold exceeded"}. ` + + `Open until: ${openUntil}` + ); + } + for (const commit of payload.commits) { maybeCrash("push.student.for_each_commit.before_lookup"); - - // Check circuit breaker before making GitHub API calls - fail fast if rate limited - // Check both org-level and create_repo method-specific circuit breakers - const circuitStatus = await checkCircuitBreakerOpen(adminSupabase, org, "create_repo", scope); - if (circuitStatus.isOpen) { - const openUntil = circuitStatus.openUntil ? new Date(circuitStatus.openUntil).toLocaleString() : "unknown"; - const scopeInfo = circuitStatus.circuitScope === "org_method" ? ` (method: create_repo)` : ""; - throw new Error( - `Circuit breaker open for org ${org}${scopeInfo}: GitHub API operations temporarily unavailable. ` + - `Reason: ${circuitStatus.reason || "Rate limit or error threshold exceeded"}. ` + - `Open until: ${openUntil}` - ); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/github-repo-webhook/index.ts` around lines 287 - 301, The circuit breaker check using checkCircuitBreakerOpen(adminSupabase, org, "create_repo", scope) is being executed inside the for (const commit of payload.commits) loop causing redundant RPCs; move the entire check (including the isOpen handling and the throw that builds openUntil/scopeInfo) to just before the for-loop so the circuit state is fetched once per request and the loop only processes commits; keep the same variables (adminSupabase, org, scope) and preserve the exact error message construction when extracting circuitStatus.isOpen, circuitStatus.openUntil and circuitStatus.reason.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@supabase/functions/github-repo-webhook/index.ts`:
- Around line 287-301: The circuit breaker check using
checkCircuitBreakerOpen(adminSupabase, org, "create_repo", scope) is being
executed inside the for (const commit of payload.commits) loop causing redundant
RPCs; move the entire check (including the isOpen handling and the throw that
builds openUntil/scopeInfo) to just before the for-loop so the circuit state is
fetched once per request and the loop only processes commits; keep the same
variables (adminSupabase, org, scope) and preserve the exact error message
construction when extracting circuitStatus.isOpen, circuitStatus.openUntil and
circuitStatus.reason.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
supabase/functions/_shared/SupabaseTypes.d.ts (1)
6156-6205:⚠️ Potential issue | 🟡 MinorAdd
| nulltorepository_check_runs.Update.check_run_idto match DB nullability.
RowandInsertpermitcheck_run_id: number | null, butUpdaterestricts it tonumber, preventing callers from clearing the field. This type inconsistency forces unnecessary casts or workarounds.Fix
Update: { assignment_group_id?: number | null; auto_promote_result?: boolean | null; - check_run_id?: number; + check_run_id?: number | null; class_id?: number;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_shared/SupabaseTypes.d.ts` around lines 6156 - 6205, The Update type for repository_check_runs currently declares check_run_id as number which contradicts Row and Insert nullability; update repository_check_runs.Update.check_run_id to be number | null so callers can clear the field (mirror Row/Insert), and ensure any generated types or converters that produce Update reflect this nullable change.supabase/functions/autograder-submit-feedback/index.ts (1)
766-780:⚠️ Potential issue | 🟡 MinorMissing error handling on the DB update for check run completion.
The
await adminSupabase.from("repository_check_runs").update(...)at lines 773–778 discards its result without checking for errors. Every other DB operation in this function checks for errors and either throws or logs to Sentry. If this update fails silently, the check run will appear stuck in a non-completed state with no indication of what went wrong.Proposed fix
- await adminSupabase + const { error: checkRunUpdateError } = await adminSupabase .from("repository_check_runs") .update({ status: newStatus }) .eq("id", checkRun.id); + if (checkRunUpdateError) { + console.error(checkRunUpdateError); + Sentry.captureException(checkRunUpdateError, scope); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/autograder-submit-feedback/index.ts` around lines 766 - 780, The DB update that sets repository_check_runs.status to newStatus (inside the block gated by submission_id && !isRegressionRerun && !isE2ERun when checkRun is present) currently awaits adminSupabase.from("repository_check_runs").update(...).eq("id", checkRun.id) but ignores the result; modify this to capture the response, check for response.error (or !response.data), and handle failures consistently with other DB calls in this file—either throw an Error or log to Sentry (and include context like checkRun.id, submission_id and newStatus) so failures aren’t silent.
🧹 Nitpick comments (1)
supabase/functions/github-repo-webhook/index.ts (1)
282-294: Circuit breaker check is inside the per-commit loop — consider hoisting it.
checkCircuitBreakerOpenmakes 1–2 RPC calls each invocation. Placing it inside thefor (const commit of payload.commits)loop means those DB round-trips run for every commit in the push. A push with 20 commits will fire 20–40 extra RPCs even though the circuit state is unlikely to change within a single webhook invocation.Moving the check before the loop (right after line 278) retains the same protection without the repeated overhead.
Proposed fix
// Extract org for circuit breaker check const org = repoName.split("/")[0]; + // Check circuit breaker before making GitHub API calls - fail fast if rate limited + const circuitStatus = await checkCircuitBreakerOpen(adminSupabase, org, "create_repo", scope); + if (circuitStatus.isOpen) { + const openUntil = circuitStatus.openUntil ? new Date(circuitStatus.openUntil).toLocaleString() : "unknown"; + const scopeInfo = circuitStatus.circuitScope === "org_method" ? ` (method: create_repo)` : ""; + throw new Error( + `Circuit breaker open for org ${org}${scopeInfo}: GitHub API operations temporarily unavailable. ` + + `Reason: ${circuitStatus.reason || "Rate limit or error threshold exceeded"}. ` + + `Open until: ${openUntil}` + ); + } + for (const commit of payload.commits) { maybeCrash("push.student.for_each_commit.before_lookup"); - - // Check circuit breaker before making GitHub API calls - fail fast if rate limited - // Check both org-level and create_repo method-specific circuit breakers - const circuitStatus = await checkCircuitBreakerOpen(adminSupabase, org, "create_repo", scope); - if (circuitStatus.isOpen) { - const openUntil = circuitStatus.openUntil ? new Date(circuitStatus.openUntil).toLocaleString() : "unknown"; - const scopeInfo = circuitStatus.circuitScope === "org_method" ? ` (method: create_repo)` : ""; - throw new Error( - `Circuit breaker open for org ${org}${scopeInfo}: GitHub API operations temporarily unavailable. ` + - `Reason: ${circuitStatus.reason || "Rate limit or error threshold exceeded"}. ` + - `Open until: ${openUntil}` - ); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/github-repo-webhook/index.ts` around lines 282 - 294, The circuit breaker check (checkCircuitBreakerOpen) is being executed inside the per-commit loop (for (const commit of payload.commits)), causing redundant RPCs; move the call to run once before iterating commits—immediately after you set org/scope (right after where payload is parsed around line ~278) and keep the existing error handling that throws if circuitStatus.isOpen; remove the check from inside the loop so the loop only processes commits and relies on the single pre-loop circuitStatus result (use the same variables adminSupabase, org, scope, and circuitStatus).
🤖 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/functions/autograder-create-submission/index.ts`:
- Around line 604-636: The code performs user role DB lookups using
initialCheckRun/classId before verifying the upstream query result; move the
check for checkRunError and the guard that throws when initialCheckRun is null
to occur immediately after fetching initialCheckRun/checkRunError and before
computing classId or querying user_roles; then only compute classId (use
initialCheckRun?.class_id) and call
adminSupabase.from("users")/.from("user_roles") when there is no checkRunError
and initialCheckRun is present, assigning userRoles otherwise leave undefined —
update the block around initialCheckRun, checkRunError, classId, userRoles so
unnecessary DB calls are avoided and errors are handled first.
---
Outside diff comments:
In `@supabase/functions/_shared/SupabaseTypes.d.ts`:
- Around line 6156-6205: The Update type for repository_check_runs currently
declares check_run_id as number which contradicts Row and Insert nullability;
update repository_check_runs.Update.check_run_id to be number | null so callers
can clear the field (mirror Row/Insert), and ensure any generated types or
converters that produce Update reflect this nullable change.
In `@supabase/functions/autograder-submit-feedback/index.ts`:
- Around line 766-780: The DB update that sets repository_check_runs.status to
newStatus (inside the block gated by submission_id && !isRegressionRerun &&
!isE2ERun when checkRun is present) currently awaits
adminSupabase.from("repository_check_runs").update(...).eq("id", checkRun.id)
but ignores the result; modify this to capture the response, check for
response.error (or !response.data), and handle failures consistently with other
DB calls in this file—either throw an Error or log to Sentry (and include
context like checkRun.id, submission_id and newStatus) so failures aren’t
silent.
---
Nitpick comments:
In `@supabase/functions/github-repo-webhook/index.ts`:
- Around line 282-294: The circuit breaker check (checkCircuitBreakerOpen) is
being executed inside the per-commit loop (for (const commit of
payload.commits)), causing redundant RPCs; move the call to run once before
iterating commits—immediately after you set org/scope (right after where payload
is parsed around line ~278) and keep the existing error handling that throws if
circuitStatus.isOpen; remove the check from inside the loop so the loop only
processes commits and relies on the single pre-loop circuitStatus result (use
the same variables adminSupabase, org, scope, and circuitStatus).
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
Contributor
Author
|
@coderabbitai full-review |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
supabase/functions/_shared/SupabaseTypes.d.ts (1)
6173-6194:⚠️ Potential issue | 🟠 MajorAdd
| nulltoUpdate.check_run_idfor schema consistency.At Line 6193,
Update.check_run_idis typed asnumberbut should benumber | nullto match theRow(Line 6159) andInsert(Line 6176) definitions. This type mismatch prevents valid null assignments likeupdate({ check_run_id: null })even though the database schema allows null.Proposed fix
Update: { assignment_group_id?: number | null; auto_promote_result?: boolean | null; - check_run_id?: number; + check_run_id?: number | null; class_id?: number;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_shared/SupabaseTypes.d.ts` around lines 6173 - 6194, Update the Update type for the relevant table so check_run_id allows nulls like the Row and Insert types; change the type of Update.check_run_id from number to number | null (the same nullable signature as Row.check_run_id and Insert.check_run_id) to permit calling update({ check_run_id: null }) without a type error.supabase/functions/github-repo-webhook/index.ts (1)
387-403:⚠️ Potential issue | 🟠 MajorHandle unique-conflict insert as idempotent success.
Between Line 361 and Line 387, concurrent deliveries can race. With the new unique key, one insert will hit
23505; treating that as fatal causes avoidable webhook failures.Idempotent conflict handling
const { error: checkRunError } = await adminSupabase.from("repository_check_runs").insert({ repository_id: studentRepo.id, check_run_id: null, class_id: studentRepo.class_id, assignment_group_id: studentRepo.assignment_group_id, commit_message: commit.message, sha: commit.id, profile_id: studentRepo.profile_id, status: status as unknown as Json }); if (checkRunError) { + if (checkRunError.code === "23505") { + // Duplicate (repository_id, sha): already processed by another delivery. + continue; + } console.error(checkRunError); scope.setTag("error_source", "repository_check_run_insert_failed"); scope.setTag("error_context", "Could not create repository_check_run"); Sentry.captureException(checkRunError, scope); throw checkRunError; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/github-repo-webhook/index.ts` around lines 387 - 403, When inserting into repository_check_runs via adminSupabase.from("repository_check_runs").insert(...) treat a Postgres unique-constraint conflict (error code "23505") as an idempotent success instead of a fatal error: in the existing error handling for checkRunError, detect if checkRunError.code === "23505" (or equivalent Postgres unique violation identifier returned by Supabase) and swallow it (optionally log an info/debug-level message with context like repository_id/sha), but only call Sentry.captureException and throw for other error codes; keep the existing scope tags and error path for non-23505 cases.supabase/functions/autograder-submit-feedback/index.ts (1)
856-862:⚠️ Potential issue | 🟠 MajorCheck the DB update result when marking check runs completed.
At Line 856, the update response is ignored. If this write fails, the API still returns success with inconsistent state.
Safe error handling
- await adminSupabase + const { error: checkRunUpdateError } = await adminSupabase .from("repository_check_runs") .update({ status: newStatus }) .eq("id", checkRun.id); + if (checkRunUpdateError) { + Sentry.captureException(checkRunUpdateError, scope); + throw new UserVisibleError( + `Internal error: Failed to finalize check run status: ${checkRunUpdateError.message}` + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/autograder-submit-feedback/index.ts` around lines 856 - 862, The update to repository_check_runs using adminSupabase (.update({...}).eq("id", checkRun.id)) ignores the response; change it to capture the result (data, error) and handle failures: check for error or affected row count, log the failure with context (checkRun.id and newStatus) and propagate or return an error/non-200 response instead of continuing; ensure this check is applied in the same function where adminSupabase and checkRun are used so the API does not report success when the DB write failed.
🤖 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/functions/autograder-create-submission/index.ts`:
- Around line 726-727: The code currently sets pushTime using
checkRun?.created_at or TZDate.tz(timeZone), which falls back to now and
reintroduces "now"-based late-detection; instead, fail fast when created_at is
missing: remove the TZDate.tz(timeZone) fallback and throw or return an error
(or reject the request) when checkRun?.created_at is undefined so pushTime is
only constructed from a real push timestamp (use the existing
TZDate(checkRun.created_at, timeZone) path and handle the missing created_at by
raising an explicit error with context).
- Around line 643-655: The actor-role lookup uses case-sensitive .eq and ignores
query errors, which can misclassify staff as students; update the users query to
use a case-insensitive match (e.g., .ilike("github_username", decoded.actor))
instead of .eq, and check the Supabase response error fields for both the users
and user_roles queries (the objects returned from
adminSupabase.from("users").select(...).maybeSingle() and
adminSupabase.from("user_roles").select(...).maybeSingle()); if either query
returns an error, throw or return the error (do not treat it as "no role"), and
only set userRoles = userRolesData ?? undefined when there was no error and the
data is valid. Ensure you reference the adminSupabase calls, the
user/userRolesData variables, and the user_roles lookup in your fix.
In `@supabase/functions/github-repo-webhook/index.ts`:
- Around line 347-353: The circuit-breaker check is using the wrong method key
("create_repo") before a GitHub call that actually triggers a workflow; update
the checkCircuitBreakerOpen invocation to use the workflow-specific method key
(e.g., "triggerWorkflow") and update the scopeInfo string generation to
reference the correct method name so method-level rate-limiting applies to
triggerWorkflow flows; specifically modify the call to
checkCircuitBreakerOpen(adminSupabase, org, "create_repo", scope) and the
related scope/messaging that uses `create_repo` to use `triggerWorkflow` and its
matching display text.
In `@supabase/migrations/20260218120000_make_check_run_id_nullable.sql`:
- Around line 11-12: The ALTER TABLE adding UNIQUE (repository_id, sha) on
repository_check_runs can fail if duplicate historical rows exist; before adding
the constraint (in the same migration, prior to
repository_check_runs_repository_id_sha_key creation) run a dedupe step that
identifies duplicates by (repository_id, sha) and deletes all but a single
canonical row (e.g., keep the row with the smallest id or earliest created_at) —
implement this with a CTE using ROW_NUMBER() OVER (PARTITION BY repository_id,
sha ORDER BY id ASC) and delete where rn > 1, then proceed to ADD CONSTRAINT
repository_check_runs_repository_id_sha_key UNIQUE (repository_id, sha).
---
Outside diff comments:
In `@supabase/functions/_shared/SupabaseTypes.d.ts`:
- Around line 6173-6194: Update the Update type for the relevant table so
check_run_id allows nulls like the Row and Insert types; change the type of
Update.check_run_id from number to number | null (the same nullable signature as
Row.check_run_id and Insert.check_run_id) to permit calling update({
check_run_id: null }) without a type error.
In `@supabase/functions/autograder-submit-feedback/index.ts`:
- Around line 856-862: The update to repository_check_runs using adminSupabase
(.update({...}).eq("id", checkRun.id)) ignores the response; change it to
capture the result (data, error) and handle failures: check for error or
affected row count, log the failure with context (checkRun.id and newStatus) and
propagate or return an error/non-200 response instead of continuing; ensure this
check is applied in the same function where adminSupabase and checkRun are used
so the API does not report success when the DB write failed.
In `@supabase/functions/github-repo-webhook/index.ts`:
- Around line 387-403: When inserting into repository_check_runs via
adminSupabase.from("repository_check_runs").insert(...) treat a Postgres
unique-constraint conflict (error code "23505") as an idempotent success instead
of a fatal error: in the existing error handling for checkRunError, detect if
checkRunError.code === "23505" (or equivalent Postgres unique violation
identifier returned by Supabase) and swallow it (optionally log an
info/debug-level message with context like repository_id/sha), but only call
Sentry.captureException and throw for other error codes; keep the existing scope
tags and error path for non-23505 cases.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
supabase/functions/_shared/SupabaseTypes.d.tssupabase/functions/autograder-create-submission/index.tssupabase/functions/autograder-submit-feedback/index.tssupabase/functions/github-async-worker/index.tssupabase/functions/github-repo-webhook/index.tssupabase/migrations/20260218120000_make_check_run_id_nullable.sql
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
supabase/functions/github-repo-webhook/index.ts (1)
33-87: Third copy ofdetectRateLimitType- consolidation recommended.This is the third implementation of
detectRateLimitTypeacross the codebase (also inautograder-submit-feedback/index.tsandautograder-create-submission/index.ts). Consider extracting to_shared/GitHubWrapper.tsalongside the existingSecondaryRateLimitErrorandPrimaryRateLimitErrorclasses.Suggested approach
// In _shared/GitHubWrapper.ts +export function detectRateLimitType(error: unknown): { + type: "secondary" | "primary" | "extreme" | null; + retryAfter?: number; + installationId?: string; +} { + // ... consolidated implementation +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/github-repo-webhook/index.ts` around lines 33 - 87, detectRateLimitType is duplicated in multiple functions; extract and reuse a single implementation by moving the function into a shared module (e.g., _shared/GitHubWrapper.ts) alongside SecondaryRateLimitError and PrimaryRateLimitError, export it, and import it from github-repo-webhook/index.ts (and from autograder-submit-feedback/index.ts and autograder-create-submission/index.ts) so each file calls the centralized detectRateLimitType; ensure the exported function signature and behavior (handling AggregateError messages, headers, status checks, and retryAfter logic) remain unchanged and update imports/uses accordingly.supabase/functions/autograder-submit-feedback/index.ts (1)
28-82: Consider extractingdetectRateLimitTypeto a shared module.This function is duplicated across multiple files (
autograder-submit-feedback/index.ts,autograder-create-submission/index.ts, andgithub-repo-webhook/index.ts). While the implementations have minor variations, centralizing this logic in_shared/GitHubWrapper.tswould reduce maintenance burden and ensure consistent rate-limit detection behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/autograder-submit-feedback/index.ts` around lines 28 - 82, Extract the duplicated detectRateLimitType function into a shared module (e.g., _shared/GitHubWrapper.ts): move the full implementation of detectRateLimitType along with any type exports it needs, ensure it exports the function (and any helper types) and imports or references SecondaryRateLimitError and PrimaryRateLimitError from their canonical source; then update each location that currently defines its own copy (the functions named detectRateLimitType in the three modules) to import and call the shared detectRateLimitType instead, preserving the exact behavior (including returned shape: type | retryAfter | installationId) and keep handling of AggregateError/err.message, headers parsing, and status checks identical so behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@supabase/functions/autograder-submit-feedback/index.ts`:
- Around line 28-82: Extract the duplicated detectRateLimitType function into a
shared module (e.g., _shared/GitHubWrapper.ts): move the full implementation of
detectRateLimitType along with any type exports it needs, ensure it exports the
function (and any helper types) and imports or references
SecondaryRateLimitError and PrimaryRateLimitError from their canonical source;
then update each location that currently defines its own copy (the functions
named detectRateLimitType in the three modules) to import and call the shared
detectRateLimitType instead, preserving the exact behavior (including returned
shape: type | retryAfter | installationId) and keep handling of
AggregateError/err.message, headers parsing, and status checks identical so
behavior remains unchanged.
In `@supabase/functions/github-repo-webhook/index.ts`:
- Around line 33-87: detectRateLimitType is duplicated in multiple functions;
extract and reuse a single implementation by moving the function into a shared
module (e.g., _shared/GitHubWrapper.ts) alongside SecondaryRateLimitError and
PrimaryRateLimitError, export it, and import it from
github-repo-webhook/index.ts (and from autograder-submit-feedback/index.ts and
autograder-create-submission/index.ts) so each file calls the centralized
detectRateLimitType; ensure the exported function signature and behavior
(handling AggregateError messages, headers, status checks, and retryAfter logic)
remain unchanged and update imports/uses accordingly.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
supabase/functions/autograder-create-submission/index.tssupabase/functions/autograder-submit-feedback/index.tssupabase/functions/github-repo-webhook/index.tssupabase/migrations/20260218120000_make_check_run_id_nullable.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
New Features
Bug Fixes
Chores