feat: add DELETE /api/sandboxes endpoint#211
Conversation
Deletes the GitHub repo and snapshot record for an account. Supports API key and Bearer auth with optional account_id for org keys. GitHub repo deletion is best-effort — snapshot is always deleted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a DELETE /api/sandboxes endpoint and supporting logic to validate/auth the request, discover and delete GitHub repos for an account, and remove the Supabase account_snapshot, returning structured JSON responses with CORS headers. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Route as "DELETE Route"
participant Validator as "validateDeleteSandboxBody"
participant Handler as "deleteSandboxHandler"
participant Snapshots as "Supabase (account_snapshots)"
participant GitHub as "GitHub API"
Client->>Route: DELETE /api/sandboxes (body w/ optional account_id)
Route->>Validator: validateDeleteSandboxBody(request)
Validator->>Validator: parse JSON & validate schema
alt validation fails
Validator-->>Route: NextResponse (400)
Route-->>Client: 400 error
else
Validator->>Handler: { accountId }
Handler->>Snapshots: fetch snapshots by accountId
Snapshots-->>Handler: snapshot[] or []
alt snapshot exists
Handler->>GitHub: findOrgReposByAccountId(accountId)
GitHub-->>Handler: list of repo URLs
loop for each repo
Handler->>GitHub: DELETE /repos/{owner}/{repo}
GitHub-->>Handler: 200/204 or 404
end
Handler->>Snapshots: deleteAccountSnapshot(accountId)
Snapshots-->>Handler: deleted snapshot row
Handler-->>Route: { status: "success", deleted_snapshot }
Route-->>Client: 200 success
else no snapshot
Handler-->>Route: { status: "success", deleted_snapshot: null }
Route-->>Client: 200 success
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/sandbox/deleteSandboxHandler.ts`:
- Around line 34-44: deleteSandboxHandler currently treats a null return from
deleteAccountSnapshot as success and always returns HTTP 200; update the handler
to distinguish "not found" vs "error" vs "deleted" using the richer return type
from deleteAccountSnapshot (or by propagating errors): call
deleteAccountSnapshot(validated.accountId) and if it indicates a deleted record
return 200 with deleted_snapshot, if it indicates "not found" return 404, and if
it indicates an error (or throws) log the error and return 500; also ensure any
failure from deleteGithubRepo is surfaced (log and return 500) rather than
letting the handler return snapshot as if deletion succeeded.
In `@lib/supabase/account_snapshots/deleteAccountSnapshot.ts`:
- Around line 10-25: The current deleteAccountSnapshot conflates Supabase errors
and "no row matched" by returning null for both; change deleteAccountSnapshot so
it surfaces real Supabase errors to callers and only returns null for the benign
"not found" case — e.g., after the supabase .delete().select().single() call, if
(error) throw or return a distinct error/result (so deleteSandboxHandler can
return 500), and only return null when !data (no row matched); update callers
accordingly to handle thrown errors or the new error/result shape. Ensure you
modify the function deleteAccountSnapshot and its caller deleteSandboxHandler to
stop treating null as a soft success.
🧹 Nitpick comments (1)
lib/sandbox/validateDeleteSandboxBody.ts (1)
8-14: Consider derivingDeleteSandboxBodyfrom the schema or documenting the intentional divergence.The
DeleteSandboxBodytype is manually defined withaccountId(camelCase) while the schema usesaccount_id(snake_case). This means the type and schema can silently drift apart. The manual type is understandable here since the field is renamed during extraction, but the coding guideline recommends exporting inferred types for validated data.At minimum, a brief comment explaining the snake_case → camelCase mapping would prevent future confusion.
As per coding guidelines, "
lib/**/validate*.ts: Export inferred types for validated data".
lib/sandbox/deleteSandboxHandler.ts
Outdated
| try { | ||
| if (snapshot.github_repo) { | ||
| await deleteGithubRepo(snapshot.github_repo); | ||
| } | ||
|
|
||
| const deletedSnapshot = await deleteAccountSnapshot(validated.accountId); | ||
|
|
||
| return NextResponse.json( | ||
| { status: "success", deleted_snapshot: deletedSnapshot ?? snapshot }, | ||
| { status: 200, headers: getCorsHeaders() }, | ||
| ); |
There was a problem hiding this comment.
deleteAccountSnapshot failure is silently treated as success.
This is the downstream effect of deleteAccountSnapshot returning null for both "not found" and "real error". If the Supabase delete fails (e.g., RLS policy, network issue), the handler still returns HTTP 200 with deleted_snapshot: snapshot, giving the client a false confirmation.
If you adopt the richer return type suggested in the deleteAccountSnapshot review, the handler can branch properly:
Sketch of error-aware handling
- const deletedSnapshot = await deleteAccountSnapshot(validated.accountId);
-
- return NextResponse.json(
- { status: "success", deleted_snapshot: deletedSnapshot ?? snapshot },
- { status: 200, headers: getCorsHeaders() },
- );
+ const result = await deleteAccountSnapshot(validated.accountId);
+
+ if (result.error) {
+ return NextResponse.json(
+ { status: "error", error: result.error },
+ { status: 500, headers: getCorsHeaders() },
+ );
+ }
+
+ return NextResponse.json(
+ { status: "success", deleted_snapshot: result.data ?? snapshot },
+ { status: 200, headers: getCorsHeaders() },
+ );🤖 Prompt for AI Agents
In `@lib/sandbox/deleteSandboxHandler.ts` around lines 34 - 44,
deleteSandboxHandler currently treats a null return from deleteAccountSnapshot
as success and always returns HTTP 200; update the handler to distinguish "not
found" vs "error" vs "deleted" using the richer return type from
deleteAccountSnapshot (or by propagating errors): call
deleteAccountSnapshot(validated.accountId) and if it indicates a deleted record
return 200 with deleted_snapshot, if it indicates "not found" return 404, and if
it indicates an error (or throws) log the error and return 500; also ensure any
failure from deleteGithubRepo is surfaced (log and return 500) rather than
letting the handler return snapshot as if deletion succeeded.
| export async function deleteAccountSnapshot( | ||
| accountId: string, | ||
| ): Promise<Tables<"account_snapshots"> | null> { | ||
| const { data, error } = await supabase | ||
| .from("account_snapshots") | ||
| .delete() | ||
| .eq("account_id", accountId) | ||
| .select("*") | ||
| .single(); | ||
|
|
||
| if (error || !data) { | ||
| return null; | ||
| } | ||
|
|
||
| return data; | ||
| } |
There was a problem hiding this comment.
Conflating "not found" with "delete failed" loses actionable error information.
deleteAccountSnapshot returns null for both a genuine Supabase error (e.g., permissions, network) and the benign "no row matched" case. The caller (deleteSandboxHandler) then treats null as a soft success by falling back to the previously-fetched snapshot — meaning a real delete failure is silently reported as successful to the client.
Consider distinguishing these two outcomes so the handler can respond with a 500 when the delete actually fails.
Proposed approach
-export async function deleteAccountSnapshot(
- accountId: string,
-): Promise<Tables<"account_snapshots"> | null> {
- const { data, error } = await supabase
+export async function deleteAccountSnapshot(
+ accountId: string,
+): Promise<{ data: Tables<"account_snapshots"> | null; error: string | null }> {
+ const { data, error } = await supabase
.from("account_snapshots")
.delete()
.eq("account_id", accountId)
.select("*")
.single();
- if (error || !data) {
- return null;
+ if (error) {
+ // PGRST116 = "no rows returned" — treat as not-found, not an error
+ if (error.code === "PGRST116") {
+ return { data: null, error: null };
+ }
+ return { data: null, error: error.message };
}
- return data;
+ return { data, error: null };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function deleteAccountSnapshot( | |
| accountId: string, | |
| ): Promise<Tables<"account_snapshots"> | null> { | |
| const { data, error } = await supabase | |
| .from("account_snapshots") | |
| .delete() | |
| .eq("account_id", accountId) | |
| .select("*") | |
| .single(); | |
| if (error || !data) { | |
| return null; | |
| } | |
| return data; | |
| } | |
| export async function deleteAccountSnapshot( | |
| accountId: string, | |
| ): Promise<{ data: Tables<"account_snapshots"> | null; error: string | null }> { | |
| const { data, error } = await supabase | |
| .from("account_snapshots") | |
| .delete() | |
| .eq("account_id", accountId) | |
| .select("*") | |
| .single(); | |
| if (error) { | |
| // PGRST116 = "no rows returned" — treat as not-found, not an error | |
| if (error.code === "PGRST116") { | |
| return { data: null, error: null }; | |
| } | |
| return { data: null, error: error.message }; | |
| } | |
| return { data, error: null }; | |
| } |
🤖 Prompt for AI Agents
In `@lib/supabase/account_snapshots/deleteAccountSnapshot.ts` around lines 10 -
25, The current deleteAccountSnapshot conflates Supabase errors and "no row
matched" by returning null for both; change deleteAccountSnapshot so it surfaces
real Supabase errors to callers and only returns null for the benign "not found"
case — e.g., after the supabase .delete().select().single() call, if (error)
throw or return a distinct error/result (so deleteSandboxHandler can return
500), and only return null when !data (no row matched); update callers
accordingly to handle thrown errors or the new error/result shape. Ensure you
modify the function deleteAccountSnapshot and its caller deleteSandboxHandler to
stop treating null as a soft success.
Don't delete the snapshot record if the GitHub repo can't be deleted, otherwise we lose the repo reference with no way to clean it up later. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
If the repo doesn't exist on GitHub (already deleted or never created), proceed with snapshot cleanup instead of aborting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When no github_repo is stored in the snapshot (or no snapshot exists), falls back to searching the recoupable GitHub org for repos whose name contains the account ID. Ensures robust cleanup even if the snapshot record was previously deleted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Logs the full error response from GitHub API to help diagnose 403 errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/github/findOrgReposByAccountId.ts`:
- Around line 16-17: The current call in findOrgReposByAccountId uses a single
fetch to "https://api.github.com/orgs/recoupable/repos?per_page=100" which only
returns the first page; update findOrgReposByAccountId to iterate GitHub
pagination: repeatedly fetch with a page=N query param (or follow the response
Link header) and accumulate results until no more pages (response length <
per_page or Link has no next); ensure you include per_page=100 on each request
and propagate auth headers/errors as the existing function does. Alternatively,
replace the org repos loop with a single GitHub Search API call (GET
/search/repositories?q={accountId}+org:recoupable) if you prefer one-request
behavior—pick one approach and remove the single-page fetch to avoid missing
repos beyond the first 100.
🧹 Nitpick comments (4)
lib/github/findOrgReposByAccountId.ts (2)
34-36: Substring match onrepo.name.includes(accountId)may over-match.If
accountIdis ever a short or common string, this could match unrelated repos. UUIDs are typically safe, but a defensive exact-suffix match (e.g.,repo.name.endsWith(accountId)) would better align with the documented naming pattern{sanitized-name}-{accountId}.Suggested tighter filter
return repos - .filter((repo) => repo.name.includes(accountId)) + .filter((repo) => repo.name.endsWith(`-${accountId}`)) .map((repo) => repo.html_url);
17-17: Hardcoded org name"recoupable".Consider extracting this to an environment variable or shared constant so it can be changed without a code deploy and stays consistent across all GitHub-related utilities.
lib/sandbox/deleteSandboxHandler.ts (2)
39-44: Sequential deletion creates a partial-delete risk.If the first repo is deleted successfully but the second fails, the handler returns
falseand the snapshot is preserved — but one repo is already gone. The caller sees an error and may retry, but the already-deleted repo is now a 404 (treated as success on retry), so this is self-healing on retry. Still, it's worth documenting this behavior or logging which repos succeeded vs. failed so operators can diagnose issues.Suggested: collect results and log partial failures
+ const failed: string[] = []; for (const url of repoUrls) { const deleted = await deleteGithubRepo(url); if (!deleted) { - return false; + failed.push(url); } } - return true; + if (failed.length > 0) { + console.error("Failed to delete repos:", failed); + return false; + } + return true;
18-47:deleteGithubReposhelper mixes repo discovery with deletion — consider splitting.This helper both aggregates URLs (from snapshot + org search) and deletes them. Per the Single Responsibility Principle, splitting into a pure aggregation step and a deletion step would make each independently testable and reusable.
That said, the function is short and readable as-is, so this is a nice-to-have.
| const response = await fetch( | ||
| `https://api.github.com/orgs/recoupable/repos?per_page=100`, |
There was a problem hiding this comment.
Pagination gap: only the first 100 repos are ever inspected.
The GitHub API paginates org repos. With per_page=100 and no page parameter or Link-header follow-up, any repo beyond the first page is invisible to this function. If the recoupable org grows past 100 repos, deletions will silently miss targets.
Consider iterating pages until the response contains fewer than per_page items, or using the GitHub search API (GET /search/repositories?q={accountId}+org:recoupable) which can match by name in a single call.
🤖 Prompt for AI Agents
In `@lib/github/findOrgReposByAccountId.ts` around lines 16 - 17, The current call
in findOrgReposByAccountId uses a single fetch to
"https://api.github.com/orgs/recoupable/repos?per_page=100" which only returns
the first page; update findOrgReposByAccountId to iterate GitHub pagination:
repeatedly fetch with a page=N query param (or follow the response Link header)
and accumulate results until no more pages (response length < per_page or Link
has no next); ensure you include per_page=100 on each request and propagate auth
headers/errors as the existing function does. Alternatively, replace the org
repos loop with a single GitHub Search API call (GET
/search/repositories?q={accountId}+org:recoupable) if you prefer one-request
behavior—pick one approach and remove the single-page fetch to avoid missing
repos beyond the first 100.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
DELETE /api/sandboxesendpoint that deletes the GitHub repo and snapshot record for an accountaccount_idfor org keys (same pattern as PATCH)New Files
lib/github/deleteGithubRepo.ts— GitHub API repo deletionlib/supabase/account_snapshots/deleteAccountSnapshot.ts— Supabase deletelib/sandbox/validateDeleteSandboxBody.ts— Zod validation + authlib/sandbox/deleteSandboxHandler.ts— Request handlerTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit