feat: add metadata_only param to GET /api/sandboxes#215
feat: add metadata_only param to GET /api/sandboxes#215sweetmantech wants to merge 2 commits intotestfrom
Conversation
When an expired Vercel snapshot is stored in the database, Sandbox.create() fails. This adds a try-catch fallback that creates a fresh sandbox and clears the expired snapshot record. Includes test for the fallback behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR refactors the GET /organizations endpoint's validation flow to use request-based authentication context, replaces URL parameter handling with optional account_id filtering, introduces snapshot fallback logic for sandbox creation, and optimizes sandbox retrieval with a metadata_only parameter for performance. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant validateGetOrganizationsRequest
participant validateAuthContext
participant buildGetOrganizationsParams
participant Database
Client->>validateGetOrganizationsRequest: GET /api/organizations?account_id=<uuid>
validateGetOrganizationsRequest->>validateGetOrganizationsRequest: Parse & validate account_id query param
validateGetOrganizationsRequest->>validateAuthContext: Validate API key / Bearer token
alt Auth Success
validateAuthContext-->>validateGetOrganizationsRequest: Auth context (orgId, accountId, role)
validateGetOrganizationsRequest->>buildGetOrganizationsParams: Build params with targetAccountId, orgId
alt Has targetAccountId (account_id provided)
buildGetOrganizationsParams->>buildGetOrganizationsParams: Check access via canAccessAccount
alt Access Granted
buildGetOrganizationsParams-->>validateGetOrganizationsRequest: { accountId: targetAccountId, error: null }
else Access Denied
buildGetOrganizationsParams-->>validateGetOrganizationsRequest: { params: null, error: "Forbidden" }
end
else No targetAccountId
alt orgId === RECOUP_ORG_ID (admin)
buildGetOrganizationsParams-->>validateGetOrganizationsRequest: { params: {}, error: null } (all records)
else orgId present (org key)
buildGetOrganizationsParams-->>validateGetOrganizationsRequest: { organizationId: orgId, error: null }
else Personal key
buildGetOrganizationsParams-->>validateGetOrganizationsRequest: { accountId: accountId, error: null }
end
end
validateGetOrganizationsRequest->>Database: Query organizations with params
Database-->>Client: Organizations list
else Auth Failure
validateAuthContext-->>Client: 401 Unauthorized
end
sequenceDiagram
participant Client
participant validateGetSandboxesRequest
participant getSandboxesHandler
participant Database
Client->>validateGetSandboxesRequest: GET /api/sandboxes?metadata_only=true
validateGetSandboxesRequest->>validateGetSandboxesRequest: Parse metadata_only parameter
validateGetSandboxesRequest-->>getSandboxesHandler: Return params with metadataOnly: boolean
getSandboxesHandler->>getSandboxesHandler: Check metadataOnly flag
alt metadataOnly === true
getSandboxesHandler->>Database: Query snapshot info & filetree only
Database-->>getSandboxesHandler: Metadata (snapshot_id, github_repo, filetree)
getSandboxesHandler-->>Client: { sandboxes: [], snapshot_id, github_repo, filetree }
else metadataOnly === false (full data)
getSandboxesHandler->>Database: Query sandboxes + live status
Database-->>getSandboxesHandler: Full sandbox records
getSandboxesHandler-->>Client: { sandboxes: [...], filetree, ... }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…216) Replace unauthenticated accountId query param with auth-derived account using validateAuthContext + buildGetOrganizationsParams pattern. - Add buildGetOrganizationsParams with access control (canAccessAccount) - Add validateGetOrganizationsRequest (auth + Zod query validation) - Update getOrganizationsHandler to use new validation pipeline - Add 7 unit tests for buildGetOrganizationsParams - Delete replaced validateOrganizationsQuery.ts - Rename param from accountId to account_id (snake_case) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
sweetmantech
changed the title
![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/organizations/buildGetOrganizationsParams.ts`:
- Around line 49-53: The Recoup-admin branch in buildGetOrganizationsParams
returns empty params which getAccountOrganizations treats as "no filters" and
triggers the early guard that returns an empty array; change the contract so
Recoup admin signals unrestricted access explicitly (for example return a
payload like { params: null, error: null, unrestricted: true } from
buildGetOrganizationsParams when orgId === RECOUP_ORG_ID) and then update
getAccountOrganizations to check that unrestricted flag (instead of treating
empty params as the early-no-filter case) and fetch all organizations; ensure
you update the signatures/consumers of buildGetOrganizationsParams and the early
guard in getAccountOrganizations to use the new unrestricted indicator (or
equivalent explicit flag) so Recoup admin actually receives all orgs.
In `@lib/organizations/validateGetOrganizationsRequest.ts`:
- Around line 65-75: The destructured params variable can still be typed as
GetAccountOrganizationsParams | null after the error branch; explicitly narrow
it before returning by asserting non-null or using a type guard: in
validateGetOrganizationsRequest, after the existing if (error) return, convert
params to GetAccountOrganizationsParams (e.g., use a non-null assertion or cast
to that type) and return the narrowed value; apply the same fix pattern to the
other validators mentioned (validateGetSandboxesRequest,
validateGetPulsesRequest, validateGetChatsRequest) for consistency.
🧹 Nitpick comments (4)
lib/supabase/account_sandboxes/selectAccountSandboxes.ts (1)
8-8:metadataOnlydoesn't belong in the Supabase query params interface.This field is a handler-level concern (skip expensive API calls), not a database query parameter. Adding it here couples presentation logic to the data-access layer, violating the Single Responsibility Principle. Consider defining a separate validated-request type (e.g.,
GetSandboxesValidatedRequest) invalidateGetSandboxesRequest.tsthat extendsSelectAccountSandboxesParamswithmetadataOnly, keeping the Supabase interface focused purely on DB query filtering.As per coding guidelines, "For Supabase operations, ensure: Follow naming convention: select*, insert*, update*, delete*, get* (for complex queries)" — this interface should remain scoped to query concerns.
lib/sandbox/createSandboxPostHandler.ts (1)
41-42: Consider logging the actual error for debuggability.The bare
catchdiscards the error object. When diagnosing snapshot failures in production, the original error message/stack would be valuable.♻️ Suggested improvement
- } catch { - console.error("Snapshot creation failed, falling back to fresh sandbox", { snapshotId }); + } catch (error) { + console.error("Snapshot creation failed, falling back to fresh sandbox", { snapshotId, error });lib/sandbox/validateGetSandboxesRequest.ts (1)
77-77: Return type couples handler concerns into Supabase params — see related comment onselectAccountSandboxes.ts.This line adds
metadataOnlyinto theSelectAccountSandboxesParamsreturn type, which as noted in the other file, bleeds handler logic into the data-access interface. If you introduce a dedicated validated-request type, this would be the natural place to define and return it.app/api/organizations/route.ts (1)
43-45: Pre-existing guideline violation:accountIdin request body.The POST handler's JSDoc documents
accountIdas a required body parameter (Line 45). Per your coding guidelines, account ID should always be derived from authentication context, never passed in request bodies. This isn't introduced by this PR, but given the direction of this change (moving towardvalidateAuthContext()-based flows), it's worth a follow-up to align the POST handler as well.As per coding guidelines, "Never use
account_idin request bodies or tool schemas - always derive account ID from authentication".#!/bin/bash # Check if createOrganizationHandler uses accountId from request body rg -n -C5 'accountId|account_id' --type=ts -g '**/organizations/createOrganization*'
| // No account_id filter - determine what to return based on key type | ||
| if (orgId === RECOUP_ORG_ID) { | ||
| // Recoup admin: return empty params to indicate ALL records | ||
| return { params: {}, error: null }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate and examine getAccountOrganizations function
fd "getAccountOrganizations" --type fRepository: recoupable/api
Length of output: 122
🏁 Script executed:
#!/bin/bash
# Search for the function implementation
rg "export.*function getAccountOrganizations" -A 30Repository: recoupable/api
Length of output: 2780
Recoup admin path returns no organizations instead of all.
The early guard in getAccountOrganizations (if (!accountId && !organizationId) return [];) means passing empty params {} yields an empty array, not all organizations. The intent stated in the comment conflicts with the implementation. Either buildGetOrganizationsParams should pass different params for Recoup admin, or getAccountOrganizations needs separate logic to handle unrestricted access.
🤖 Prompt for AI Agents
In `@lib/organizations/buildGetOrganizationsParams.ts` around lines 49 - 53, The
Recoup-admin branch in buildGetOrganizationsParams returns empty params which
getAccountOrganizations treats as "no filters" and triggers the early guard that
returns an empty array; change the contract so Recoup admin signals unrestricted
access explicitly (for example return a payload like { params: null, error:
null, unrestricted: true } from buildGetOrganizationsParams when orgId ===
RECOUP_ORG_ID) and then update getAccountOrganizations to check that
unrestricted flag (instead of treating empty params as the early-no-filter case)
and fetch all organizations; ensure you update the signatures/consumers of
buildGetOrganizationsParams and the early guard in getAccountOrganizations to
use the new unrestricted indicator (or equivalent explicit flag) so Recoup admin
actually receives all orgs.
| if (error) { | ||
| return NextResponse.json( | ||
| { | ||
| status: "error", | ||
| error, | ||
| }, | ||
| { status: 403, headers: getCorsHeaders() }, | ||
| ); | ||
| } | ||
|
|
||
| return params; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the validateGetOrganizationsRequest.ts file
fd -t f "validateGetOrganizationsRequest.ts"Repository: recoupable/api
Length of output: 110
🏁 Script executed:
# Find buildGetOrganizationsParams to understand the return type
rg -n "buildGetOrganizationsParams" --type=tsRepository: recoupable/api
Length of output: 1526
🏁 Script executed:
# Check the similar validateGetSandboxesRequest pattern mentioned in scratchpad
fd -t f "validateGetSandboxesRequest.ts"Repository: recoupable/api
Length of output: 100
🏁 Script executed:
# Read the validateGetOrganizationsRequest.ts file
cat -n lib/organizations/validateGetOrganizationsRequest.tsRepository: recoupable/api
Length of output: 3025
🏁 Script executed:
# Read the buildGetOrganizationsParams file to understand the return type
cat -n lib/organizations/buildGetOrganizationsParams.tsRepository: recoupable/api
Length of output: 2769
🏁 Script executed:
# Read the validateGetSandboxesRequest for comparison
cat -n lib/sandbox/validateGetSandboxesRequest.tsRepository: recoupable/api
Length of output: 3213
🏁 Script executed:
# Also check the buildGetSandboxesParams to see the pattern
fd -t f "buildGetSandboxesParams.ts" | xargs cat -nRepository: recoupable/api
Length of output: 3269
🏁 Script executed:
# Check if there are any TypeScript errors in the build or tests
find . -name "*.json" -path "*tsconfig*" | head -5Repository: recoupable/api
Length of output: 73
🏁 Script executed:
# Verify the same pattern appears in both files and how common this pattern is
rg -n 'const { params, error } = await build' --type=tsRepository: recoupable/api
Length of output: 185
🏁 Script executed:
# Fixed regex - escape the braces
rg -n 'const.*params.*error.*await build' --type=tsRepository: recoupable/api
Length of output: 693
🏁 Script executed:
# Simpler search - look for destructuring of params and error
rg -n 'params.*error.*build' --type=ts | head -20Repository: recoupable/api
Length of output: 693
🏁 Script executed:
# Check one of the other validators to see if they have the same issue
cat -n lib/pulse/validateGetPulsesRequest.ts | sed -n '60,75p'Repository: recoupable/api
Length of output: 460
🏁 Script executed:
# Check the buildGetPulsesParams return type
cat -n lib/pulse/buildGetPulsesParams.ts | head -25Repository: recoupable/api
Length of output: 1205
🏁 Script executed:
# Check how the return statement looks in validateGetPulsesRequest to compare
cat -n lib/pulse/validateGetPulsesRequest.ts | sed -n '72,85p'Repository: recoupable/api
Length of output: 324
🏁 Script executed:
# Let's also check if there's a TypeScript configuration that might affect this
cat tsconfig.json | grep -A5 -B5 "strict"Repository: recoupable/api
Length of output: 308
Type-safety improvement recommended: explicitly narrow params after error check.
While the code compiles and tests pass (due to strict: false in tsconfig), the destructured params retains type GetAccountOrganizationsParams | null even after the if (error) check. TypeScript doesn't narrow destructured union members together. The code is functionally safe—the error branch always returns early—but a non-null assertion or explicit type guard improves clarity and future-proofs against stricter type checking:
♻️ Recommended approach
if (error) {
return NextResponse.json(
{ status: "error", error },
{ status: 403, headers: getCorsHeaders() },
);
}
- return params;
+ return params!;Note: This pattern appears consistently across multiple validators (validateGetSandboxesRequest, validateGetPulsesRequest, validateGetChatsRequest, and MCP tools). Consider applying the same fix across all occurrences for consistency.
🤖 Prompt for AI Agents
In `@lib/organizations/validateGetOrganizationsRequest.ts` around lines 65 - 75,
The destructured params variable can still be typed as
GetAccountOrganizationsParams | null after the error branch; explicitly narrow
it before returning by asserting non-null or using a type guard: in
validateGetOrganizationsRequest, after the existing if (error) return, convert
params to GetAccountOrganizationsParams (e.g., use a non-null assertion or cast
to that type) and return the narrowed value; apply the same fix pattern to the
other validators mentioned (validateGetSandboxesRequest,
validateGetPulsesRequest, validateGetChatsRequest) for consistency.
a8eb87b to
8136ce3
Compare
1 participant
Summary
metadata_only=truequery parameter toGET /api/sandboxesthat skips expensiveSandbox.get()API calls to Vercelmetadata_only=true, returns onlysnapshot_id,github_repo, andfiletree(emptysandboxesarray)setup-sandboxtaskRoot Cause
The Tasks repo's
getAccountSandboxes()only needssnapshot_idandgithub_repo, but the GET endpoint was makingSandbox.get()for every sandbox record (100+). This caused the newly created sandbox to become unavailable (HTTP 410) by the timesetupSandboxTasktried to use it.Test plan
getSandboxStatusshould NOT be called whenmetadataOnlyis truesetup-sandboxtask no longer fails with 410🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation