Conversation
* fix: prevent Vercel Blob upload conflicts and improve error handling - Add addRandomSuffix and allowOverwrite to blob upload token options, preventing "blob already exists" errors on retry uploads - Use Promise.allSettled for resilient multi-file blob uploads - Surface meaningful error messages when blob uploads fail Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix: remove redundant allowOverwrite and surface blob upload errors - Remove allowOverwrite: true (redundant with addRandomSuffix) - Surface rejected blob uploads in the errors array so callers can report partial failures to the user Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: CTO Agent <cto@recoupable.com> Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: Sweets Sweetman <sweetmantech@gmail.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 27 minutes and 58 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR enhances sandbox file uploads by configuring random suffix generation for upload tokens and implementing batch-resilient error handling. It replaces Changes
Sequence DiagramsequenceDiagram
participant Client
participant TokenAPI as Upload API
participant BlobHandler as Blob Handler
participant FileAPI as /api/sandboxes/files
Client->>TokenAPI: Request upload token
TokenAPI->>TokenAPI: Generate token<br/>with addRandomSuffix: true
TokenAPI-->>Client: Return token
Client->>BlobHandler: Upload files (batch)<br/>with Promise.allSettled
BlobHandler->>BlobHandler: Process each file<br/>(continue on failure)
BlobHandler-->>Client: Successful uploads +<br/>per-file errors
alt All uploads failed
Client->>Client: Throw early with<br/>first error
else Partial/full success
Client->>FileAPI: POST aggregated results
FileAPI-->>Client: Combine blob errors +<br/>server errors
Client-->>Client: Return errors only<br/>if non-empty
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 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: 1
🧹 Nitpick comments (1)
lib/sandboxes/uploadSandboxFiles.ts (1)
28-92: Extract the blob-upload phase into a helper.
uploadSandboxFilesnow owns blob uploads, settlement reduction, the GitHub commit call, and final error merging in one 65-line utility. Pulling thePromise.allSettledblock into a small private helper would keep this easier to test and back inside the repo’s utility-function guideline.As per coding guidelines,
**/*.{js,jsx,ts,tsx}: Each function should have a single responsibility and clear naming, andlib/**/*.ts: For utility functions, ensure: ... Keep functions under 50 lines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sandboxes/uploadSandboxFiles.ts` around lines 28 - 92, Extract the Promise.allSettled blob-upload logic from uploadSandboxFiles into a small private helper (e.g., uploadFilesToBlobs or uploadFilesToBlobStorage) that accepts (files: File[], accessToken: string) and returns a Promise<{ blobFiles: {url:string;name:string}[]; blobErrors: string[] }>; move the mapping that calls upload(...) with handleUploadUrl and clientPayload into that helper, keep the results.forEach reduction (fulfilled -> blobFiles, rejected -> blobErrors) and the same error message formatting, and have uploadSandboxFiles call this helper and then continue with the existing POST to `${NEW_API_BASE_URL}/api/sandboxes/files`, response handling, and final error merging; ensure exported types/signatures remain unchanged and preserve current behavior (throw when no blobFiles) and local variable names (uploadSandboxFiles, upload) so callers and tests continue to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/sandboxes/uploadSandboxFiles.ts`:
- Around line 61-65: When all blob uploads fail the code currently throws only
the first blobErrors entry and later throws before merging blobErrors with
data.errors; update the failure paths in uploadSandboxFiles (the blobFiles
branch and the later throw) to aggregate all per-file diagnostics instead of
losing them. Construct and throw a single combined error (e.g., an
AggregateError or Error whose message is blobErrors.join("; ") plus any
data.errors joined) so callers receive the full list of per-file failures, and
ensure you include both blobErrors and data.errors when building that combined
error.
---
Nitpick comments:
In `@lib/sandboxes/uploadSandboxFiles.ts`:
- Around line 28-92: Extract the Promise.allSettled blob-upload logic from
uploadSandboxFiles into a small private helper (e.g., uploadFilesToBlobs or
uploadFilesToBlobStorage) that accepts (files: File[], accessToken: string) and
returns a Promise<{ blobFiles: {url:string;name:string}[]; blobErrors: string[]
}>; move the mapping that calls upload(...) with handleUploadUrl and
clientPayload into that helper, keep the results.forEach reduction (fulfilled ->
blobFiles, rejected -> blobErrors) and the same error message formatting, and
have uploadSandboxFiles call this helper and then continue with the existing
POST to `${NEW_API_BASE_URL}/api/sandboxes/files`, response handling, and final
error merging; ensure exported types/signatures remain unchanged and preserve
current behavior (throw when no blobFiles) and local variable names
(uploadSandboxFiles, upload) so callers and tests continue to work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 08f7de36-0ace-4587-846b-9205247f9c81
📒 Files selected for processing (2)
app/api/sandbox/upload/route.tslib/sandboxes/uploadSandboxFiles.ts
| if (blobFiles.length === 0) { | ||
| throw new Error( | ||
| blobErrors[0] || "Failed to upload files to temporary storage", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Don't drop the collected per-file errors on failure paths.
Line 63 only surfaces the first blobErrors entry when every blob upload fails, and Line 83 throws before the collected blobErrors/data.errors are combined. The batch diagnostics gathered above never reach callers in the failure cases where they matter most.
🛠️ Suggested fix
if (blobFiles.length === 0) {
throw new Error(
- blobErrors[0] || "Failed to upload files to temporary storage",
+ blobErrors.length > 0
+ ? blobErrors.join("\n")
+ : "Failed to upload files to temporary storage",
);
}
@@
- if (!response.ok || data.status === "error") {
- throw new Error(data.error || "Failed to upload files");
- }
-
const allErrors = [...blobErrors, ...(data.errors || [])];
+ if (!response.ok || data.status === "error") {
+ throw new Error(
+ [data.error, ...allErrors].filter(Boolean).join("\n") ||
+ "Failed to upload files",
+ );
+ }Also applies to: 82-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sandboxes/uploadSandboxFiles.ts` around lines 61 - 65, When all blob
uploads fail the code currently throws only the first blobErrors entry and later
throws before merging blobErrors with data.errors; update the failure paths in
uploadSandboxFiles (the blobFiles branch and the later throw) to aggregate all
per-file diagnostics instead of losing them. Construct and throw a single
combined error (e.g., an AggregateError or Error whose message is
blobErrors.join("; ") plus any data.errors joined) so callers receive the full
list of per-file failures, and ensure you include both blobErrors and
data.errors when building that combined error.
…m-create-endpoint chore: remove unused local /api/room/create route
3 participants
Merging test branch to main after PR #1606 (blob upload fix).
Summary by CodeRabbit
Bug Fixes
New Features