feat: drag-and-drop file upload for sandbox file tree#1601
Conversation
|
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 3 minutes and 37 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 (2)
📝 WalkthroughWalkthroughAdds drag-and-drop file upload to the sandbox file tree: new drop-zone UI and empty-state component, a hook to handle drops and auth, a client upload utility that uses Vercel Blob, and a new POST API route to produce presigned upload tokens. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as SandboxFileTree / SandboxDropZone / NoSandboxFiles
participant Hook as useSandboxFileDrop
participant Util as uploadSandboxFiles
participant TokenAPI as /api/sandbox/upload
participant Blob as Vercel Blob
participant BackendAPI as /api/sandboxes/files
User->>UI: Drop files
UI->>Hook: handleFilesDropped(files)
Hook->>Hook: resolve target path (findFileNode)
Hook->>Hook: get access token via usePrivy()
Hook->>Util: uploadSandboxFiles({accessToken, files, path})
Util->>TokenAPI: POST to /api/sandbox/upload (auth header)
TokenAPI->>Blob: handleUpload (generate presigned tokens)
Blob-->>TokenAPI: tokens
TokenAPI-->>Util: token response
par upload files concurrently
Util->>Blob: upload(fileN, clientPayload)
Blob-->>Util: blob URL/metadata
end
Util->>BackendAPI: POST /api/sandboxes/files (blob URLs + metadata)
BackendAPI-->>Util: {uploaded, errors?}
Util-->>Hook: response
Hook->>UI: refetch()
UI-->>User: updated file tree / toasts
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~30 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 229df71c71
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // If selected path looks like a file (has extension), use parent dir | ||
| const lastPart = parts[parts.length - 1]; | ||
| if (lastPart.includes(".")) { | ||
| targetPath = parts.slice(0, -1).join("/"); |
There was a problem hiding this comment.
This branch infers “file vs folder” by checking for . in the selected path segment, which misclassifies common repo entries (for example, Dockerfile is a file with no dot and .github is a folder with a dot). In those cases the computed path is wrong, so dropped files can be uploaded to the wrong directory or fail to upload. Use the selected node’s actual type from the file tree instead of filename heuristics.
Useful? React with 👍 / 👎.
| if (!accessToken) { | ||
| setError("Not authenticated. Please sign in."); | ||
| return; |
There was a problem hiding this comment.
generate exits early whenever accessToken is falsy, but this token is loaded asynchronously by useAccessToken, so a signed-in user can submit quickly after page load and hit a false “Not authenticated” error with no request sent. This also risks failures after token rotation because the callback uses cached state. Resolve the token at submit time (or disable submit until token load completes) to remove the auth race.
Useful? React with 👍 / 👎.
Integrates react-dropzone drag-and-drop with the /files page to upload files to GitHub sandbox repos via POST /api/sandboxes/files. Shows upload overlay, progress state, and toast notifications on completion. Co-Authored-By: Paperclip <noreply@paperclip.ing>
sweetmantech
force-pushed
the
feature/sandbox-file-upload-chat
branch
from
229df71 to
f11c49f
Compare
| const { getAccessToken } = usePrivy(); | ||
| const [uploading, setUploading] = useState(false); | ||
|
|
||
| const handleFilesDropped = async (files: File[]) => { |
There was a problem hiding this comment.
OCP: make a new hook for the useFileDrop functions / state.
| <div | ||
| {...getRootProps()} | ||
| className={`flex w-full flex-col gap-4 lg:flex-row rounded-lg transition-all relative ${ | ||
| isDragging ? "ring-2 ring-primary ring-offset-2 bg-primary/5" : "" | ||
| }`} | ||
| > | ||
| <input {...getInputProps()} /> | ||
|
|
||
| {isDragging && ( | ||
| <div className="absolute inset-0 flex items-center justify-center bg-primary/10 backdrop-blur-sm rounded-lg z-10 pointer-events-none"> | ||
| <div className="flex flex-col items-center gap-3 text-primary"> | ||
| <Upload className="h-12 w-12 animate-bounce" /> | ||
| <p className="text-lg font-medium">Drop files to upload to repository</p> | ||
| </div> | ||
| </div> | ||
| )} | ||
|
|
||
| {uploading && ( | ||
| <div className="absolute inset-0 flex items-center justify-center bg-background/50 backdrop-blur-sm rounded-lg z-10"> | ||
| <div className="flex items-center gap-2 text-muted-foreground"> | ||
| <Loader className="h-4 w-4 animate-spin" /> | ||
| <span>Uploading...</span> | ||
| </div> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
OCP - new component file for the drag-n-drop wrapper.
- Add /api/sandbox-upload route for generating presigned blob tokens - Update uploadSandboxFiles to upload to Vercel Blob first, then pass blob URLs to the API (bypasses 4.5MB serverless limit) - Add @vercel/blob dependency Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
app/api/sandbox-upload/route.ts (1)
23-23: EmptyonUploadCompletedcallback is acceptable but consider adding logging.The empty callback is valid since cleanup happens in the downstream API call. However, adding observability (e.g., structured logging) here could help with debugging upload issues in production.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/sandbox-upload/route.ts` at line 23, The empty onUploadCompleted callback currently provides no observability; update the onUploadCompleted implementation to emit a structured log when uploads complete (e.g., using the existing logger or processLogger), include key context such as upload id/filename/user/client IP and any status/metadata available, and ensure errors thrown inside the callback are caught and logged; modify the onUploadCompleted function reference in route.ts to call logger.info/logger.error with those fields to aid debugging.lib/sandboxes/uploadSandboxFiles.ts (2)
41-47: Sequential uploads create unnecessary latency - consider parallelizing.The
for...ofloop withawaituploads files one at a time. For multiple files, this significantly increases total upload time. UsingPromise.allorPromise.allSettledwould upload files concurrently.⚡ Proposed parallel upload
- const blobFiles: { url: string; name: string }[] = []; - - for (const file of files) { - const blob = await upload(file.name, file, { - access: "public", - handleUploadUrl: "/api/sandbox-upload", - }); - blobFiles.push({ url: blob.url, name: file.name }); - } + const blobFiles = await Promise.all( + files.map(async (file) => { + const blob = await upload(file.name, file, { + access: "public", + handleUploadUrl: "/api/sandbox-upload", + }); + return { url: blob.url, name: file.name }; + }) + );Note: If you need partial success handling (some uploads succeed, others fail), use
Promise.allSettledinstead and filter results.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sandboxes/uploadSandboxFiles.ts` around lines 41 - 47, The current sequential upload loop (for...of awaiting upload) in uploadSandboxFiles.ts causes unnecessary latency; change it to run uploads concurrently by mapping files to upload(file.name, file, {...}) promises and awaiting them with Promise.all or Promise.allSettled, then populate blobFiles from the resolved results (use Promise.allSettled if you need partial success handling and filter fulfilled results before pushing { url: result.value.url, name: file.name } into blobFiles). Ensure you still preserve the access and handleUploadUrl options passed to the upload function.
49-60: Add request timeout to prevent hanging on slow/unresponsive API.The
fetchcall has no timeout configured. If the external API is slow or unresponsive, this could leave users waiting indefinitely with the "Uploading..." overlay.⏱️ Proposed fix with AbortController
+ const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30000); // 30 second timeout + const response = await fetch(`${NEW_API_BASE_URL}/api/sandboxes/files`, { method: "POST", headers: { Authorization: `Bearer ${accessToken}`, "Content-Type": "application/json", }, body: JSON.stringify({ files: blobFiles, path, message, }), + signal: controller.signal, }); + + clearTimeout(timeoutId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sandboxes/uploadSandboxFiles.ts` around lines 49 - 60, The fetch in uploadSandboxFiles.ts that posts to `${NEW_API_BASE_URL}/api/sandboxes/files` (using accessToken, blobFiles, path, message) needs a timeout to avoid hanging; wrap the request with an AbortController, pass controller.signal into fetch, set a setTimeout to call controller.abort() after a chosen timeout (e.g., 15–30s), and clear that timeout on success or failure so you don't leak timers; ensure you handle the abort/timeout case (AbortError) and propagate/handle it the same way other errors are handled by uploadSandboxFiles.components/Sandboxes/SandboxFileTree.tsx (1)
97-99: Consider usingcn()utility for cleaner conditional class merging.Per coding guidelines, use the
cn()utility (combiningclsxandtailwind-merge) for class merging instead of template literals with ternaries.✨ Example refactor
+import { cn } from "@/lib/utils"; // Empty state (line 97-99) - className={`w-full max-w-md rounded-lg p-8 border-2 border-dashed transition-all ${ - isDragging ? "border-primary bg-primary/5" : "border-muted-foreground/25" - }`} + className={cn( + "w-full max-w-md rounded-lg p-8 border-2 border-dashed transition-all", + isDragging ? "border-primary bg-primary/5" : "border-muted-foreground/25" + )} // Main container (line 113-115) - className={`flex w-full flex-col gap-4 lg:flex-row rounded-lg transition-all relative ${ - isDragging ? "ring-2 ring-primary ring-offset-2 bg-primary/5" : "" - }`} + className={cn( + "flex w-full flex-col gap-4 lg:flex-row rounded-lg transition-all relative", + isDragging && "ring-2 ring-primary ring-offset-2 bg-primary/5" + )}Also applies to: 113-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Sandboxes/SandboxFileTree.tsx` around lines 97 - 99, Replace the template-literal className concatenation in the SandboxFileTree JSX with the cn() utility: import cn (if not already) and call cn("w-full max-w-md rounded-lg p-8 border-2 border-dashed transition-all", isDragging ? "border-primary bg-primary/5" : "border-muted-foreground/25") (or use conditional entries like isDragging && "border-primary bg-primary/5") to let clsx/tailwind-merge handle merging; apply the same refactor to the second occurrence referenced (the className around lines 113-115) so both usages in the SandboxFileTree component use cn() instead of template literals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/sandbox-upload/route.ts`:
- Around line 11-12: Move the JSON parsing into the existing try-catch inside
the POST handler so malformed JSON is handled gracefully: inside the POST
function's try block call await request.json() and cast to HandleUploadBody
(instead of parsing before the try), catch parsing errors (e.g., SyntaxError or
any thrown error) and return a 400 NextResponse with a clear message, and keep
the rest of the logic that uses the parsed body unchanged (refer to the POST
function and the HandleUploadBody type to locate where to move the parsing).
- Around line 11-32: The POST handler currently calls handleUpload and returns
presigned tokens without authenticating the caller; update the
onBeforeGenerateToken callback passed to handleUpload to extract and validate
the auth token (e.g., from request.headers), verify the caller's account
identity/permissions (same checks used by files/get-signed-url), and only return
the maximumSizeInBytes object when the accountId is validated; if validation
fails, throw or return an authentication error (HTTP 401/403) so unauthenticated
users cannot get presigned upload tokens—update SandboxFileTree.tsx to send the
same auth token (usePrivy()) in request headers when invoking this endpoint.
In `@components/Sandboxes/SandboxFileTree.tsx`:
- Around line 95-106: The dropzone lacks ARIA attributes—update the JSX where
getRootProps() and getInputProps() are used (in SandboxFileTree) to add a
descriptive aria-label (e.g., aria-label="File upload dropzone") and a role of
"button" or "region" as appropriate, and reflect the drag state with an
aria-live or aria-pressed/aria-busy attribute bound to isDragging so screen
readers get state changes; also add aria-hidden or hidden text for the
empty-state prompt (the paragraph and Upload icon) using
aria-hidden={isDragging} or an offscreen visually-hidden description and ensure
the <input {...getInputProps()} /> has an accessible id/aria-describedby
referencing that description; apply the same attribute additions to the other
dropzone markup used elsewhere in this component.
- Around line 32-41: The current selected-path handling in the SandboxFileTree
assumes any segment containing "." is a file which misclassifies dotfiles and
dotted directories; update the logic in the block that reads
fileContent.selectedPath (and sets targetPath) to resolve file vs directory by
consulting the sandbox file model instead of string matching — either use an
existing file-tree lookup (e.g. check the node for fileContent.selectedPath in
the tree or call a helper like findNode/lookupNode) or expose and consume an
explicit isFile/type flag from useSandboxFileContent; if the resolved node is a
file, set targetPath to its parent directory, otherwise leave targetPath as the
directory path.
In `@lib/sandboxes/uploadSandboxFiles.ts`:
- Around line 41-66: The current flow uploads files via the upload(...) call and
accumulates blob metadata in blobFiles, but if the POST to
`${NEW_API_BASE_URL}/api/sandboxes/files` (JSON parsed into
UploadSandboxFilesResponse) fails the blobs remain orphaned; wrap the fetch +
response.json() + error check in a try/catch (or a finally) and on any failure
iterate blobFiles and call the appropriate Vercel blob delete/cleanup operation
for each uploaded blob (use the blob object returned by upload, e.g., items in
blobFiles or the original upload result), then rethrow the error so callers
still see the failure—update code paths referencing upload, blobFiles,
NEW_API_BASE_URL, and UploadSandboxFilesResponse accordingly.
---
Nitpick comments:
In `@app/api/sandbox-upload/route.ts`:
- Line 23: The empty onUploadCompleted callback currently provides no
observability; update the onUploadCompleted implementation to emit a structured
log when uploads complete (e.g., using the existing logger or processLogger),
include key context such as upload id/filename/user/client IP and any
status/metadata available, and ensure errors thrown inside the callback are
caught and logged; modify the onUploadCompleted function reference in route.ts
to call logger.info/logger.error with those fields to aid debugging.
In `@components/Sandboxes/SandboxFileTree.tsx`:
- Around line 97-99: Replace the template-literal className concatenation in the
SandboxFileTree JSX with the cn() utility: import cn (if not already) and call
cn("w-full max-w-md rounded-lg p-8 border-2 border-dashed transition-all",
isDragging ? "border-primary bg-primary/5" : "border-muted-foreground/25") (or
use conditional entries like isDragging && "border-primary bg-primary/5") to let
clsx/tailwind-merge handle merging; apply the same refactor to the second
occurrence referenced (the className around lines 113-115) so both usages in the
SandboxFileTree component use cn() instead of template literals.
In `@lib/sandboxes/uploadSandboxFiles.ts`:
- Around line 41-47: The current sequential upload loop (for...of awaiting
upload) in uploadSandboxFiles.ts causes unnecessary latency; change it to run
uploads concurrently by mapping files to upload(file.name, file, {...}) promises
and awaiting them with Promise.all or Promise.allSettled, then populate
blobFiles from the resolved results (use Promise.allSettled if you need partial
success handling and filter fulfilled results before pushing { url:
result.value.url, name: file.name } into blobFiles). Ensure you still preserve
the access and handleUploadUrl options passed to the upload function.
- Around line 49-60: The fetch in uploadSandboxFiles.ts that posts to
`${NEW_API_BASE_URL}/api/sandboxes/files` (using accessToken, blobFiles, path,
message) needs a timeout to avoid hanging; wrap the request with an
AbortController, pass controller.signal into fetch, set a setTimeout to call
controller.abort() after a chosen timeout (e.g., 15–30s), and clear that timeout
on success or failure so you don't leak timers; ensure you handle the
abort/timeout case (AbortError) and propagate/handle it the same way other
errors are handled by uploadSandboxFiles.
🪄 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: fad60805-ae76-4c83-ae60-f8f9f9367cf6
⛔ Files ignored due to path filters (2)
package.jsonis excluded by none and included by nonepnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by none
📒 Files selected for processing (3)
app/api/sandbox-upload/route.tscomponents/Sandboxes/SandboxFileTree.tsxlib/sandboxes/uploadSandboxFiles.ts
| export async function POST(request: Request): Promise<NextResponse> { | ||
| const body = (await request.json()) as HandleUploadBody; |
There was a problem hiding this comment.
Move JSON parsing inside try-catch for robust error handling.
The request.json() call on line 12 is outside the try-catch block. If the request body is malformed JSON, this will throw an unhandled exception resulting in a 500 error instead of a controlled 400 response.
🛡️ Proposed fix
export async function POST(request: Request): Promise<NextResponse> {
- const body = (await request.json()) as HandleUploadBody;
-
try {
+ const body = (await request.json()) as HandleUploadBody;
+
const jsonResponse = await handleUpload({
body,
request,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/sandbox-upload/route.ts` around lines 11 - 12, Move the JSON parsing
into the existing try-catch inside the POST handler so malformed JSON is handled
gracefully: inside the POST function's try block call await request.json() and
cast to HandleUploadBody (instead of parsing before the try), catch parsing
errors (e.g., SyntaxError or any thrown error) and return a 400 NextResponse
with a clear message, and keep the rest of the logic that uses the parsed body
unchanged (refer to the POST function and the HandleUploadBody type to locate
where to move the parsing).
| for (const file of files) { | ||
| const blob = await upload(file.name, file, { | ||
| access: "public", | ||
| handleUploadUrl: "/api/sandbox-upload", | ||
| }); | ||
| blobFiles.push({ url: blob.url, name: file.name }); | ||
| } | ||
|
|
||
| const response = await fetch(`${NEW_API_BASE_URL}/api/sandboxes/files`, { | ||
| method: "POST", | ||
| headers: { | ||
| Authorization: `Bearer ${accessToken}`, | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ | ||
| files: blobFiles, | ||
| path, | ||
| message, | ||
| }), | ||
| }); | ||
|
|
||
| const data: UploadSandboxFilesResponse = await response.json(); | ||
|
|
||
| if (!response.ok || data.status === "error") { | ||
| throw new Error(data.error || "Failed to upload files"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's any blob cleanup logic elsewhere in the codebase
rg -n "del.*blob|delete.*blob|cleanup.*blob" --type ts -iRepository: recoupable/chat
Length of output: 270
🏁 Script executed:
fd uploadSandboxFiles.tsRepository: recoupable/chat
Length of output: 94
🏁 Script executed:
cat -n lib/sandboxes/uploadSandboxFiles.tsRepository: recoupable/chat
Length of output: 2439
🏁 Script executed:
fd -name "*sandbox*" -type f | grep -E "\.(ts|tsx)$"Repository: recoupable/chat
Length of output: 289
🏁 Script executed:
rg -n "sandbox-upload|/api/sandboxes" -A 5 -B 2Repository: recoupable/chat
Length of output: 3023
🏁 Script executed:
fd -path "*/api/sandboxes/files*" -type fRepository: recoupable/chat
Length of output: 227
🏁 Script executed:
rg "sandboxes.*files|/api/sandboxes/files" -A 20 --type tsRepository: recoupable/chat
Length of output: 1341
🏁 Script executed:
fd -path "*/app/api*" -type f | head -20Repository: recoupable/chat
Length of output: 227
🏁 Script executed:
find app/api -type d -name sandboxesRepository: recoupable/chat
Length of output: 41
🏁 Script executed:
find app/api -type f -name "route.ts" | grep sandboxRepository: recoupable/chat
Length of output: 90
🏁 Script executed:
cat -n app/api/sandboxes/files/route.ts 2>/dev/null || echo "File not found, searching..."Repository: recoupable/chat
Length of output: 87
🏁 Script executed:
fd "route.ts" | xargs grep -l "sandboxes.*files\|files.*blob" 2>/dev/nullRepository: recoupable/chat
Length of output: 43
🏁 Script executed:
find app/api -type f -name "route.ts" | head -20Repository: recoupable/chat
Length of output: 681
🏁 Script executed:
rg "sandboxes/files|del.*blob|cleanup" --type-add='api:*.ts' --type apiRepository: recoupable/chat
Length of output: 411
🏁 Script executed:
rg "NEW_API_BASE_URL" -A 2 -B 2Repository: recoupable/chat
Length of output: 21111
🏁 Script executed:
grep -r "delete.*blob\|cleanup" app/api/sandbox-upload/route.ts 2>/dev/null || echo "Checking for blob cleanup in upload handler..."Repository: recoupable/chat
Length of output: 105
🏁 Script executed:
cat -n app/api/sandbox-upload/route.tsRepository: recoupable/chat
Length of output: 1270
Add blob cleanup on API request failure to prevent orphaned storage.
Vercel Blobs uploaded at lines 41-47 are orphaned if the subsequent POST to /api/sandboxes/files fails (lines 49-65). While the JSDoc documents that cleanup is the API's responsibility, there's no client-side recovery if that endpoint fails, potentially accumulating storage costs over time.
Consider either:
- Wrapping the API call in a try-catch to clean up uploaded blobs on failure
- Verifying the external API endpoint has robust cleanup logic for its own failures
- Implementing scheduled cleanup for uncommitted blobs (time-based or manual purge)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sandboxes/uploadSandboxFiles.ts` around lines 41 - 66, The current flow
uploads files via the upload(...) call and accumulates blob metadata in
blobFiles, but if the POST to `${NEW_API_BASE_URL}/api/sandboxes/files` (JSON
parsed into UploadSandboxFilesResponse) fails the blobs remain orphaned; wrap
the fetch + response.json() + error check in a try/catch (or a finally) and on
any failure iterate blobFiles and call the appropriate Vercel blob
delete/cleanup operation for each uploaded blob (use the blob object returned by
upload, e.g., items in blobFiles or the original upload result), then rethrow
the error so callers still see the failure—update code paths referencing upload,
blobFiles, NEW_API_BASE_URL, and UploadSandboxFilesResponse accordingly.
- Extract file drop logic to useSandboxFileDrop hook (OCP) - Extract drag-n-drop wrapper to SandboxDropZone component (OCP) - Move route from sandbox-upload to sandbox/upload (KISS) - Add auth check to blob upload endpoint - Move JSON parsing inside try-catch - Fix file vs directory detection using FileNode.type instead of dot heuristic - Parallelize blob uploads with Promise.all - Add ARIA labels for accessibility - Use cn() utility for class merging Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| <div | ||
| {...getRootProps()} | ||
| role="region" | ||
| aria-label="File upload dropzone" | ||
| className={cn( | ||
| "w-full max-w-md rounded-lg p-8 border-2 border-dashed transition-all", | ||
| isDragging | ||
| ? "border-primary bg-primary/5" | ||
| : "border-muted-foreground/25", | ||
| )} | ||
| > | ||
| <input {...getInputProps()} aria-label="Upload files" /> | ||
| <div className="flex flex-col items-center gap-3 text-muted-foreground"> | ||
| <Upload className="h-8 w-8" /> | ||
| <p className="text-sm"> | ||
| No files yet. Drag and drop files here to upload. | ||
| </p> | ||
| </div> |
There was a problem hiding this comment.
SRP - new component file for NoSandboxFiles.tsx
There was a problem hiding this comment.
Done — extracted to components/Sandboxes/NoSandboxFiles.tsx.
| /** | ||
| * Finds a node in the file tree by path. | ||
| */ | ||
| function findNode(nodes: FileNode[], path: string): FileNode | undefined { |
There was a problem hiding this comment.
SRP - new lib file for findNode
There was a problem hiding this comment.
Done — extracted to lib/sandboxes/findFileNode.ts.
| const { getAccessToken } = usePrivy(); | ||
| const [uploading, setUploading] = useState(false); | ||
|
|
||
| const handleFilesDropped = useCallback( |
There was a problem hiding this comment.
Would it simplify to use tanstack useMutation here? Or, would that make this implementation more complex?
There was a problem hiding this comment.
Good call — switched to useMutation. It simplified things: removed manual useState for uploading, and onSuccess/onError callbacks handle toasts + refetch cleanly.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
hooks/useSandboxFileDrop.ts (1)
10-19:findNodeis a pure utility that could be co-located withFileNodetype.The recursive tree search is clean and correct. Since it has no dependencies on hook state, consider moving it to
lib/sandboxes/parseFileTree.tsalongside theFileNodetype definition for better discoverability and potential reuse.This is a minor organizational suggestion—the current placement is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useSandboxFileDrop.ts` around lines 10 - 19, Move the pure utility function findNode (which accepts nodes: FileNode[] and path: string and returns FileNode | undefined) out of hooks/useSandboxFileDrop.ts into the shared module alongside the FileNode type (e.g., lib/sandboxes/parseFileTree.ts); export findNode from that module and update imports where used so callers reference the new exported utility instead of the local definition.components/Sandboxes/SandboxFileTree.tsx (1)
23-28: Hook instantiated even when output is unused in non-empty state.The
useDragAndDrophook at lines 23-28 runs on every render per React's rules of hooks, but its output (getRootProps,getInputProps,isDragging) is only used in the empty state (lines 55-66). Whenfiletree.length > 0,SandboxDropZonecreates its own independent dropzone.This creates unnecessary event listener setup. Consider extracting the empty state into a separate component that owns its own hook, mirroring how
SandboxDropZoneencapsulates its dropzone:♻️ Optional refactor sketch
// components/Sandboxes/SandboxEmptyDropZone.tsx function SandboxEmptyDropZone({ onDrop, uploading }: { onDrop: (files: File[]) => void; uploading: boolean }) { const { getRootProps, getInputProps, isDragging } = useDragAndDrop({ onDrop, maxFiles: 100, maxSizeMB: 100, disabled: uploading, }); // ... empty state UI }Then in
SandboxFileTree:if (filetree.length === 0) { - return ( - <div {...getRootProps()} ... > + return <SandboxEmptyDropZone onDrop={handleFilesDropped} uploading={uploading} />; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Sandboxes/SandboxFileTree.tsx` around lines 23 - 28, The useDragAndDrop hook (returning getRootProps, getInputProps, isDragging) is currently instantiated inside SandboxFileTree even when filetree.length > 0 and those values are unused; extract the empty-state dropzone into its own component (e.g. SandboxEmptyDropZone) that receives onDrop (handleFilesDropped) and uploading as props and calls useDragAndDrop locally so the hook and its event listeners are only created for the empty state; update SandboxFileTree to render SandboxEmptyDropZone when filetree.length === 0 and keep the existing SandboxDropZone for the non-empty path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/sandbox/upload/route.ts`:
- Around line 14-20: The route currently checks for an Authorization header
(authHeader) which the client never sends; instead move validation into the
onBeforeGenerateToken callback that receives clientPayload. Update
onBeforeGenerateToken to parse clientPayload (JSON.parse), extract the token
field (sent as { token: accessToken } from uploadSandboxFiles.ts), verify
presence/validity of that token, and return/throw an authentication error (HTTP
401) from that callback when missing/invalid; remove or bypass the authHeader
check in the existing upload route handler (authHeader/Authorization lookup) so
authentication is performed where the token is actually provided.
In `@lib/sandboxes/uploadSandboxFiles.ts`:
- Around line 39-48: The current use of Promise.all in the blobFiles creation
(files.map -> upload(...)) will reject on the first upload failure and leave
prior successful uploads orphaned; change Promise.all to Promise.allSettled,
collect successful results (extract { url, name } from fulfilled entries) and
handle rejected entries explicitly: either proceed with only the fulfilled
uploads or perform cleanup by deleting the successfully uploaded blobs (use your
existing blob-delete helper or call the appropriate deletion API with the URLs
returned from successful entries) and then rethrow or return an aggregated
error; update any downstream code that expects blobFiles to handle the
partial-success shape or an explicit error.
---
Nitpick comments:
In `@components/Sandboxes/SandboxFileTree.tsx`:
- Around line 23-28: The useDragAndDrop hook (returning getRootProps,
getInputProps, isDragging) is currently instantiated inside SandboxFileTree even
when filetree.length > 0 and those values are unused; extract the empty-state
dropzone into its own component (e.g. SandboxEmptyDropZone) that receives onDrop
(handleFilesDropped) and uploading as props and calls useDragAndDrop locally so
the hook and its event listeners are only created for the empty state; update
SandboxFileTree to render SandboxEmptyDropZone when filetree.length === 0 and
keep the existing SandboxDropZone for the non-empty path.
In `@hooks/useSandboxFileDrop.ts`:
- Around line 10-19: Move the pure utility function findNode (which accepts
nodes: FileNode[] and path: string and returns FileNode | undefined) out of
hooks/useSandboxFileDrop.ts into the shared module alongside the FileNode type
(e.g., lib/sandboxes/parseFileTree.ts); export findNode from that module and
update imports where used so callers reference the new exported utility instead
of the local definition.
🪄 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: caf41b49-b97b-474a-9a25-b83ccb62788c
📒 Files selected for processing (5)
app/api/sandbox/upload/route.tscomponents/Sandboxes/SandboxDropZone.tsxcomponents/Sandboxes/SandboxFileTree.tsxhooks/useSandboxFileDrop.tslib/sandboxes/uploadSandboxFiles.ts
| const blobFiles = await Promise.all( | ||
| files.map(async (file) => { | ||
| const blob = await upload(file.name, file, { | ||
| access: "public", | ||
| handleUploadUrl: "/api/sandbox/upload", | ||
| clientPayload: JSON.stringify({ token: accessToken }), | ||
| }); | ||
| return { url: blob.url, name: file.name }; | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Promise.all fails fast, leaving successfully uploaded blobs orphaned on partial failure.
If any single file upload fails, Promise.all rejects immediately. Files that uploaded successfully before the failure become orphaned in Vercel Blob storage with no cleanup path.
Consider using Promise.allSettled to capture partial success and either:
- Proceed with successfully uploaded files
- Clean up successful uploads on partial failure
♻️ Suggested approach with Promise.allSettled
- const blobFiles = await Promise.all(
- files.map(async (file) => {
- const blob = await upload(file.name, file, {
- access: "public",
- handleUploadUrl: "/api/sandbox/upload",
- clientPayload: JSON.stringify({ token: accessToken }),
- });
- return { url: blob.url, name: file.name };
- }),
- );
+ const results = await Promise.allSettled(
+ files.map(async (file) => {
+ const blob = await upload(file.name, file, {
+ access: "public",
+ handleUploadUrl: "/api/sandbox/upload",
+ clientPayload: JSON.stringify({ token: accessToken }),
+ });
+ return { url: blob.url, name: file.name };
+ }),
+ );
+
+ const blobFiles = results
+ .filter((r): r is PromiseFulfilledResult<{ url: string; name: string }> =>
+ r.status === "fulfilled")
+ .map((r) => r.value);
+
+ const uploadErrors = results
+ .filter((r): r is PromiseRejectedResult => r.status === "rejected")
+ .map((r, i) => `${files[i].name}: ${r.reason}`);
+
+ if (blobFiles.length === 0) {
+ throw new Error("All file uploads failed");
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sandboxes/uploadSandboxFiles.ts` around lines 39 - 48, The current use of
Promise.all in the blobFiles creation (files.map -> upload(...)) will reject on
the first upload failure and leave prior successful uploads orphaned; change
Promise.all to Promise.allSettled, collect successful results (extract { url,
name } from fulfilled entries) and handle rejected entries explicitly: either
proceed with only the fulfilled uploads or perform cleanup by deleting the
successfully uploaded blobs (use your existing blob-delete helper or call the
appropriate deletion API with the URLs returned from successful entries) and
then rethrow or return an aggregated error; update any downstream code that
expects blobFiles to handle the partial-success shape or an explicit error.
- Extract NoSandboxFiles to its own component (SRP) - Extract findFileNode to lib/sandboxes/findFileNode.ts (SRP) - Use tanstack useMutation in useSandboxFileDrop for cleaner state management Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| }, | ||
| }); | ||
|
|
||
| const handleFilesDropped = useCallback( |
There was a problem hiding this comment.
KISS - is the useCallback wrapper really required with useMutation?
There was a problem hiding this comment.
You're right — removed the useCallback wrapper entirely. Now just returning mutation.mutate directly.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/Sandboxes/NoSandboxFiles.tsx (1)
24-33: Prefer semantic root +data-statefor drag visual state.This is a good candidate to use a semantic container (
section) anddata-state-driven styling instead of branching classes.♻️ Suggested refactor
- <div + <section {...getRootProps()} - role="region" aria-label="File upload dropzone" + data-state={isDragging ? "dragging" : "idle"} className={cn( - "w-full max-w-md rounded-lg p-8 border-2 border-dashed transition-all", - isDragging - ? "border-primary bg-primary/5" - : "border-muted-foreground/25", + "w-full max-w-md rounded-lg p-8 border-2 border-dashed transition-all border-muted-foreground/25 data-[state=dragging]:border-primary data-[state=dragging]:bg-primary/5", )} > @@ - </div> + </section>As per coding guidelines, "Use data-state attribute for visual component states (open/closed, loading, etc.)" and "Start with semantic HTML first, then augment with ARIA if needed".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Sandboxes/NoSandboxFiles.tsx` around lines 24 - 33, Replace the non-semantic div with a semantic section and switch from conditional class branching to a data-state attribute to drive visual state: keep the getRootProps() spread, role, and aria-label, set data-state based on isDragging (e.g., "dragging" vs "idle"), and simplify the className to a single static base class (on the element referenced by getRootProps()/className) while moving the drag-specific styles to CSS selectors that target [data-state="dragging"] so the component (NoSandboxFiles / the element using getRootProps(), isDragging, className) uses semantic HTML and data-state-driven styling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/Sandboxes/NoSandboxFiles.tsx`:
- Around line 24-33: Replace the non-semantic div with a semantic section and
switch from conditional class branching to a data-state attribute to drive
visual state: keep the getRootProps() spread, role, and aria-label, set
data-state based on isDragging (e.g., "dragging" vs "idle"), and simplify the
className to a single static base class (on the element referenced by
getRootProps()/className) while moving the drag-specific styles to CSS selectors
that target [data-state="dragging"] so the component (NoSandboxFiles / the
element using getRootProps(), isDragging, className) uses semantic HTML and
data-state-driven styling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ffb5104-066a-4771-bca5-379e0374904b
📒 Files selected for processing (4)
components/Sandboxes/NoSandboxFiles.tsxcomponents/Sandboxes/SandboxFileTree.tsxhooks/useSandboxFileDrop.tslib/sandboxes/findFileNode.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- components/Sandboxes/SandboxFileTree.tsx
- hooks/useSandboxFileDrop.ts
The blob client controls its own request headers, so we validate the access token passed via clientPayload in onBeforeGenerateToken. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
useDragAndDrophook intoSandboxFileTreecomponentPOST /api/sandboxes/filesendpoint for file commitsRelated
Test plan
Summary by CodeRabbit