feat: implement direct CSV upload support for cloud storage#1962
Open
Anchel123 wants to merge 7 commits into
Open
feat: implement direct CSV upload support for cloud storage#1962Anchel123 wants to merge 7 commits into
Anchel123 wants to merge 7 commits into
Conversation
- Added a new endpoint for direct CSV uploads to S3 and Vercel Blob storage. - Enhanced CSV storage provider to support direct upload target generation. - Updated local storage handling to reject direct uploads. - Improved error handling and user feedback for upload processes. - Added CORS support for direct upload requests. - Updated Docker configuration for MinIO with TLS support. - Enhanced upload error messages for better user experience.
Completed Working on "Code Review"✅ Review comments were successfully published from all chunks and the final review was submitted. Result: Review submitted: COMMENT. Total comments: 8 across 7 files. ✅ Workflow completed successfully. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughAdds cloud CSV direct uploads with S3 and Vercel Blob targets, storage-mode routing, MinIO TLS support, extension-aware Cypher uploads, clearer LOAD CSV validation errors, and multipart parsing diagnostics. ChangesCSV direct upload flow
Graph upload handling
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant UploadGraph
participant DirectUploadRoute
participant CsvStorageProvider
participant CloudStorage
UploadGraph->>DirectUploadRoute: Initialize CSV upload
DirectUploadRoute->>CsvStorageProvider: Create direct upload target
CsvStorageProvider-->>DirectUploadRoute: Return key and target
UploadGraph->>CloudStorage: Upload CSV bytes directly
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🔒 Trivy Security Scan Results |
![overcut-ai[bot]](https://avatars.githubusercontent.com/in/1367736?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the substantial work on direct CSV upload support and related storage/provider changes. I reviewed the posted findings and there are 8 MAJOR comments across this PR (0 BLOCKER, 0 CRITICAL, 8 MAJOR, 0 MINOR, 0 SUGGESTION, 0 PRAISE).
Key themes
- Direct upload robustness and fallback behavior: runtime payload validation and fallback logic need hardening to avoid user-facing failures in local/transient error scenarios.
- Storage endpoint/provider correctness under real environments: endpoint rewriting and provider caching may cause stale credentials or unreachable/read URL behavior depending on deployment setup.
- Error-surface clarity and semantic safety: some new mappings/normalization paths can alter intended behavior or mask root causes (CSV header diagnostics,
.txtdirective stripping, object existence handling).
Recommended next steps
- Harden direct-upload client flow with strict response schema guards and resilient local fallback for non-success direct-init paths.
- Revisit S3/Blob provider assumptions (endpoint rewrite defaults, credential-source cache invalidation, MIME acceptance policy) with explicit opt-in/configuration where behavior is environment-specific.
- Adjust parsing/error mapping paths to preserve user intent and diagnostic accuracy (directive handling, header parsing messages, missing-object precheck or equivalent deterministic API signaling).
…on for API routes
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
app/api/graph/[graph]/load-csv/route.ts (1)
172-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParse the URL once and use
hostnamefor a robust local endpoint check.Using a regular expression with a colon (
localhost:|127\.0\.0\.1:) assumes the local endpoint always includes a port and might inadvertently match the string elsewhere in the URL. Parsing the URL once and checking itshostnameis more accurate.♻️ Proposed refactor
- const csvUrlHost = (() => { - try { - return new URL(csvUrl).host; - } catch { - return "unknown"; - } - })(); - const isLikelyLocalEndpoint = /localhost:|127\.0\.0\.1:/i.test(csvUrl); + let csvUrlHost = "unknown"; + let isLikelyLocalEndpoint = false; + try { + const parsedUrl = new URL(csvUrl); + csvUrlHost = parsedUrl.host; + isLikelyLocalEndpoint = ["localhost", "127.0.0.1"].includes(parsedUrl.hostname); + } catch { + // fallback to defaults if URL parsing fails + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/graph/`[graph]/load-csv/route.ts around lines 172 - 185, Update the CSV error-message logic around isLikelyLocalEndpoint to parse csvUrl once, reuse the parsed URL for csvUrlHost, and determine local endpoints from the URL’s hostname rather than a port-dependent regular expression. Preserve the existing fallback behavior for invalid URLs and the current host value in the messages.app/lib/csv-storage.ts (1)
79-83: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake direct-upload target generation required by the provider contract.
Every supplied provider defines this behavior—cloud providers return a target and local returns
null. Keeping the method optional allows a cloud implementation to compile without it and fail only at runtime. Remove?and call it directly in the route.As per path instructions, review TypeScript code for type safety, proper error handling, and Next.js best practices.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/csv-storage.ts` around lines 79 - 83, Make createDirectUploadTarget in the provider contract required by removing its optional marker, then update the route to invoke it directly without optional-method checks. Ensure every provider implementation, including the local provider returning null, satisfies the required method signature.Source: Path instructions
app/components/graph/UploadGraph.tsx (1)
61-77: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winImport the shared direct-upload target type.
This duplicate can drift from
app/lib/csv-storage.ts; the response cast would then hide new variants and route them through the Blob branch. Use a type-only import so server contract changes produce client compile errors.Proposed refactor
+import type { CsvDirectUploadTarget } from "`@/app/lib/csv-storage`"; -interface S3DirectUploadTarget { - ... -} - -interface BlobDirectUploadTarget { - ... -} - -type CsvDirectUploadTarget = S3DirectUploadTarget | BlobDirectUploadTarget; +type S3DirectUploadTarget = Extract<CsvDirectUploadTarget, { type: "s3-presigned-put" }>;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/graph/UploadGraph.tsx` around lines 61 - 77, Remove the locally duplicated S3DirectUploadTarget, BlobDirectUploadTarget, and CsvDirectUploadTarget definitions in UploadGraph.tsx, and replace them with a type-only import of the shared CsvDirectUploadTarget from app/lib/csv-storage.ts. Update the response typing to use that imported type so newly added upload variants are caught by client compilation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/api/csv-temp/direct/route.ts`:
- Around line 118-124: Update the direct-upload flow before the success response
in the route handler to validate the uploaded object’s actual bytes and apply
the same BOM normalization used by streamCsvUpload. Only return the importable
key after validation succeeds; otherwise reject the upload using the route’s
existing error-handling conventions, while preserving the current CORS headers
and success response for valid CSV content.
- Around line 20-36: Update sanitizeDirectUploadRequest to first validate that
the decoded body is a non-null object before reading filename or sizeBytes, and
validate contentType is a string when provided before calling trim. Return null
for malformed values so the route produces its existing 400 response instead of
throwing; preserve the current defaults for an omitted or empty contentType.
In `@app/api/upload/route.ts`:
- Around line 64-71: Update the content-type validation in the upload route to
accept multipart/form-data headers where boundary appears after other
parameters, while still requiring a boundary parameter. Adjust the regex around
the existing contentType check to allow arbitrary semicolon-delimited directives
before boundary= and preserve the current 400 response for invalid headers.
In `@docker-compose.minio.yml`:
- Around line 20-23: Update the docker-compose service port mappings so host
ports 443, 444, and 9443 target Caddy’s actual HTTPS listener on container port
443; preserve the existing host-port exposure while replacing the incorrect 9443
container targets.
In `@docker/minio-tls/Caddyfile`:
- Around line 12-20: Update all CORS handler blocks in the Caddyfile, including
the preflight and corresponding response blocks, to validate the request Origin
against the trusted application-origin policy used by app/api/utils.ts. Only
emit Access-Control-Allow-Origin and related CORS headers for approved origins;
do not unconditionally reflect arbitrary Origin values.
---
Nitpick comments:
In `@app/api/graph/`[graph]/load-csv/route.ts:
- Around line 172-185: Update the CSV error-message logic around
isLikelyLocalEndpoint to parse csvUrl once, reuse the parsed URL for csvUrlHost,
and determine local endpoints from the URL’s hostname rather than a
port-dependent regular expression. Preserve the existing fallback behavior for
invalid URLs and the current host value in the messages.
In `@app/components/graph/UploadGraph.tsx`:
- Around line 61-77: Remove the locally duplicated S3DirectUploadTarget,
BlobDirectUploadTarget, and CsvDirectUploadTarget definitions in
UploadGraph.tsx, and replace them with a type-only import of the shared
CsvDirectUploadTarget from app/lib/csv-storage.ts. Update the response typing to
use that imported type so newly added upload variants are caught by client
compilation.
In `@app/lib/csv-storage.ts`:
- Around line 79-83: Make createDirectUploadTarget in the provider contract
required by removing its optional marker, then update the route to invoke it
directly without optional-method checks. Ensure every provider implementation,
including the local provider returning null, satisfies the required method
signature.
🪄 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: 88b9a2c6-7a87-45f8-add2-c3931ce3da80
📒 Files selected for processing (17)
app/api/csv-temp/[id]/route.tsapp/api/csv-temp/direct/route.tsapp/api/csv-temp/route.tsapp/api/graph/[graph]/load-csv/route.tsapp/api/graph/[graph]/upload/route.tsapp/api/graph/[graph]/upload/upload-utils.test.tsapp/api/graph/[graph]/upload/upload-utils.tsapp/api/upload/route.tsapp/components/graph/UploadGraph.tsxapp/lib/csv-storage-local.tsapp/lib/csv-storage-s3.tsapp/lib/csv-storage-vercel-blob.tsapp/lib/csv-storage.tsdocker-compose.minio.ymldocker/minio-tls/Caddyfilelib/utils.tsproxy.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/upload/route.ts (1)
350-373: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftRemove
request.clone()to prevent Out-Of-Memory (OOM) Denial of Service.Cloning a stream via
request.clone()relies on the standardtee()operation, which buffers all read chunks in memory for the unconsumed branch. WhenstreamToDiskprocesses a large upload (e.g., a 1GiB.dumpfile), the entire 1GiB payload will be buffered in memory for the unusedfallbackRequestbranch. This will exhaust the server's memory and cause an immediate OOM crash.Note: Relying on
Content-Lengthto conditionally clone the request is insecure, as an attacker can spoof a smallContent-Lengthwhile sending a 1GiB payload to trigger the same OOM exploit.Please remove the fallback logic entirely. The
streamToDiskimplementation usingbusboyis robust for streaming large files securely in a Node.js environment.🚨 Proposed fix to remove the unsafe fallback
- // Clone once up-front so we can retry multipart parsing with the built-in - // Request.formData() parser if Busboy fails on this runtime. - const fallbackRequest = request.clone(); const session = await getClient(request); if (session instanceof NextResponse) { return session; } // Uploads only feed graph-mutating flows (Cypher batch / dump restore), so // reject Read-Only users before staging anything to disk — matching every // other mutating graph route (resolveReadOnly). if (resolveReadOnly(request, session.user.role)) { return NextResponse.json( { error: "You do not have permission to upload files." }, { status: 403, headers: corsHeaders } ); } - let result = await streamToDisk(request, session.user.id); - - if (isMultipartParseFailure(result)) { - result = await formDataToDisk(fallbackRequest, session.user.id); - } + const result = await streamToDisk(request, session.user.id);(Note: Once applied, you can also safely remove the now-unused
formDataToDiskandisMultipartParseFailurehelper functions above to keep the code clean.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/upload/route.ts` around lines 350 - 373, Remove the request.clone() setup and eliminate the formDataToDisk/isMultipartParseFailure fallback from the upload handler. Call streamToDisk directly with the original request and preserve its result handling without retrying multipart parsing, then remove the now-unused helper functions.
🧹 Nitpick comments (3)
proxy.ts (1)
123-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the stale comment.
Since API routes now exit early to prevent request header mutation, the comment indicating that CSP is applied to "all routes" is no longer accurate. Consider updating it for clarity.
As a side note, a suitable PR title for these changes following the conventional commit format could be
feat: add direct CSV uploads to cloud storage.♻️ Proposed fix
if (pathname.startsWith("/api/")) { return NextResponse.next(); } - // --- CSP with nonce (all routes) --- + // --- CSP with nonce (non-API routes) --- const nonce = btoa(crypto.randomUUID()); const isDev = process.env.NODE_ENV === "development";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@proxy.ts` around lines 123 - 131, Update the stale CSP comment near the API-route early return to clarify that nonce/header mutation applies only to non-API routes, while preserving the existing control flow and API rate-limiting behavior.app/api/upload/route.ts (2)
230-236: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid logging a 500 error on expected size limit truncations.
When an upload exceeds the size limit,
writeStream.destroy()is intentionally called. This causes thepipelineto reject with aPrematureCloseError, which then gets logged here as a 500 "Stream pipeline error".Although the original 413 status correctly takes precedence (because
recordErrorignores subsequent errors), this creates unnecessary noise in the server error logs. You can safely skip logging if the stream was intentionally truncated.🧹 Proposed fix
.catch((err: unknown) => { fs.unlink(tempFilePath, () => {}); - console.error("Stream pipeline error:", err); - recordError(500, "Failed to store uploaded file."); + if (!sizeLimitHit) { + console.error("Stream pipeline error:", err); + recordError(500, "Failed to store uploaded file."); + } storeSettled = true; settleIfReady(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/upload/route.ts` around lines 230 - 236, Update the pipeline rejection handler in the upload route to detect intentional stream truncation caused by the size-limit path and skip console.error for that expected PrematureCloseError. Preserve cleanup, storeSettled assignment, settleIfReady(), and the existing 500 recordError behavior for all other pipeline failures.
307-316: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
stream()instead ofarrayBuffer()to prevent high memory usage.Calling
await file.arrayBuffer()buffers the entire file in memory before writing it to disk. If this function processes larger files, it can cause severe memory spikes or Out-Of-Memory (OOM) crashes.Since Web
Fileobjects expose astream()method, you can use Node'spipelineto stream the data directly to disk without holding it in memory.🚀 Proposed fix to stream directly to disk
try { - const buffer = Buffer.from(await file.arrayBuffer()); - await fs.promises.writeFile(filePath, buffer); + await pipeline( + Readable.fromWeb(file.stream() as NodeReadableStream<Uint8Array>), + fs.createWriteStream(filePath) + ); return { ok: true, filename }; } catch (err) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/upload/route.ts` around lines 307 - 316, Replace the whole-file Buffer conversion in the upload fallback try block with streaming from file.stream() to filePath via Node’s pipeline utility. Preserve the existing success result and catch cleanup behavior, including unlinking the partial file and returning the same error response.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/api/upload/route.ts`:
- Around line 350-373: Remove the request.clone() setup and eliminate the
formDataToDisk/isMultipartParseFailure fallback from the upload handler. Call
streamToDisk directly with the original request and preserve its result handling
without retrying multipart parsing, then remove the now-unused helper functions.
---
Nitpick comments:
In `@app/api/upload/route.ts`:
- Around line 230-236: Update the pipeline rejection handler in the upload route
to detect intentional stream truncation caused by the size-limit path and skip
console.error for that expected PrematureCloseError. Preserve cleanup,
storeSettled assignment, settleIfReady(), and the existing 500 recordError
behavior for all other pipeline failures.
- Around line 307-316: Replace the whole-file Buffer conversion in the upload
fallback try block with streaming from file.stream() to filePath via Node’s
pipeline utility. Preserve the existing success result and catch cleanup
behavior, including unlinking the partial file and returning the same error
response.
In `@proxy.ts`:
- Around line 123-131: Update the stale CSP comment near the API-route early
return to clarify that nonce/header mutation applies only to non-API routes,
while preserving the existing control flow and API rate-limiting behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5fb960d6-025a-4c54-96d5-b942d50740cc
📒 Files selected for processing (2)
app/api/upload/route.tsproxy.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/lib/csv-storage-s3.ts (1)
225-251: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winEnforce upload size limits via
ContentLength.To prevent clients from bypassing API-level size validations, consider including
ContentLength: request.sizeBytesin thePutObjectCommand. This ensures S3 will reject the upload if the actual file size differs from the requested size. Browsers automatically calculate and send the correctContent-Lengthheader based on the file object during thefetchrequest, so no frontend changes are needed to support this.♻️ Proposed refactor
async createDirectUploadTarget( owner: string, key: string, request: CsvDirectUploadRequest ): Promise<CsvDirectUploadTarget | null> { const s3Key = objectKey(owner, key); const contentType = request.contentType || "text/csv; charset=utf-8"; const url = await getSignedUrl( this.client, new PutObjectCommand({ Bucket: bucket(), Key: s3Key, ContentType: contentType, + ContentLength: request.sizeBytes, }), { expiresIn: WRITE_EXPIRES_IN } ); return { type: "s3-presigned-put", method: "PUT", url, headers: { "Content-Type": contentType, }, }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/csv-storage-s3.ts` around lines 225 - 251, Update createDirectUploadTarget’s PutObjectCommand to include ContentLength set from request.sizeBytes, so the presigned upload enforces the requested file size while preserving the existing content type and response behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/lib/csv-storage-s3.ts`:
- Around line 225-251: Update createDirectUploadTarget’s PutObjectCommand to
include ContentLength set from request.sizeBytes, so the presigned upload
enforces the requested file size while preserving the existing content type and
response behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 305ea3bf-d092-44b8-b58c-4ee5c9750e0b
📒 Files selected for processing (12)
app/api/csv-temp/direct/route.tsapp/api/graph/[graph]/load-csv/route.tsapp/api/graph/[graph]/upload/upload-utils.test.tsapp/api/graph/[graph]/upload/upload-utils.tsapp/api/upload/route.tsapp/components/graph/UploadGraph.tsxapp/lib/csv-storage-s3.tsapp/lib/csv-storage-vercel-blob.tsapp/lib/csv-storage.tsdocker-compose.minio.ymldocker/minio-tls/Caddyfileproxy.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- docker-compose.minio.yml
- app/api/graph/[graph]/upload/upload-utils.test.ts
- app/api/graph/[graph]/load-csv/route.ts
- docker/minio-tls/Caddyfile
- proxy.ts
- app/lib/csv-storage-vercel-blob.ts
- app/api/csv-temp/direct/route.ts
- app/components/graph/UploadGraph.tsx
- app/api/graph/[graph]/upload/upload-utils.ts
- app/lib/csv-storage.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/api/upload/route.ts (1)
94-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant type check.
Since
fileEntriesis already strictly filtered to only include items wherevalue instanceof Fileat line 81,fileValueis guaranteed to never be a string. This check and the subsequent intermediate variable can be safely simplified.♻️ Proposed refactor
- const [, fileValue] = fileEntries[0]; - if (typeof fileValue === "string") { - return { ok: false, error: "No file uploaded.", status: 400 }; - } - const file = fileValue as File; + const file = fileEntries[0][1] as File;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/upload/route.ts` around lines 94 - 98, In the upload handler, simplify the first file extraction after the filtered fileEntries construction by removing the redundant string check and intermediate fileValue variable. Destructure the File value directly from fileEntries[0] and continue using it as file, relying on the existing instanceof File filtering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/api/upload/route.ts`:
- Line 125: Replace the synchronous fs.mkdirSync call in the upload route with
the asynchronous fs.promises.mkdir equivalent, preserving the recursive
directory-creation option and awaiting completion within the existing async
handler.
- Around line 72-74: Update the upload handler before request.formData() to
validate the request’s Content-Length against the configured maximum and reject
oversized bodies before buffering; retain the existing file.size validation
afterward, and rely on the upstream proxy/body limit for requests without a
usable Content-Length.
---
Nitpick comments:
In `@app/api/upload/route.ts`:
- Around line 94-98: In the upload handler, simplify the first file extraction
after the filtered fileEntries construction by removing the redundant string
check and intermediate fileValue variable. Destructure the File value directly
from fileEntries[0] and continue using it as file, relying on the existing
instanceof File filtering.
🪄 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: 8752a8c6-9efa-466e-a374-125a8e2473d9
📒 Files selected for processing (2)
app/api/upload/route.tslib/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/utils.ts
Comment on lines
+72
to
+74
| let formData: FormData; | ||
| try { | ||
| formData = await request.formData(); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Next.js request.formData() buffer the entire body into memory before returning in the Node runtime?
💡 Result:
In the Next.js Node runtime, request.formData typically buffers the entire request body into memory before it can be parsed [1][2]. Because parsing multipart/form-data requires processing the full body to extract boundaries and individual fields, Next.js must have access to the complete payload [3][4]. When middleware is involved, Next.js explicitly clones the request body and buffers it in memory to enable multiple reads, which consumes memory and is subject to configured size limits [2]. Key details regarding this behavior include: 1. Memory Usage and Limits: Since the body is buffered, large payloads can significantly increase memory usage per request [2]. To protect against excessive memory consumption, Next.js provides configuration options such as experimental.middlewareClientMaxBodySize, which limits the size of buffered request bodies [5][2]. 2. Middleware Impact: When middleware is present, it often triggers the body cloning/buffering process [2]. If the incoming request exceeds the configured limits for these buffers, the request may fail or result in truncated data [1][2]. 3. Server Actions: For Server Actions, there are specific body size limits (e.g., experimental.serverActions.bodySizeLimit) intended to constrain memory usage for multipart submissions [3]. If you are dealing with large file uploads, it is generally recommended to avoid relying on standard request.formData if possible, or ensure that your server-side configurations are appropriately tuned for the expected payload sizes to prevent memory exhaustion or request rejection [5][3][2].
Citations:
- 1: https://github.com/vercel/next.js/blob/1b45163bce3dab30d7a79dff95238b84016b54c2/packages/next/src/server/body-streams.ts
- 2: https://heydev.us/blog/nextjs-middleware-empty-post-body-netlify-2026
- 3: Edge Server Actions do not enforce serverActions.bodySizeLimit for multipart form submissions vercel/next.js#95172
- 4: formData with app router: Upload error: TypeError: Failed to parse body as FormData. vercel/next.js#73220
- 5: https://nextjs.org/docs/15/pages/api-reference/config/next-config-js/middlewareClientMaxBodySize
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files app/api/upload/route.ts
wc -l app/api/upload/route.ts
cat -n app/api/upload/route.ts | sed -n '1,220p'Repository: FalkorDB/falkordb-browser
Length of output: 9743
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "content-length|MAX_FILE_SIZE|MAX_DUMP_SIZE|formData\(|uploadSmallFileFromFormData|streamToDisk|request\.body|Busboy|sizeLimit|bodySizeLimit" app/api/upload/route.ts next.config.* . 2>/dev/nullRepository: FalkorDB/falkordb-browser
Length of output: 6878
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n app/api/upload/route.ts | sed -n '340,390p'
printf '\n---\n'
cat -n app/lib/graphUpload.ts | sed -n '1,220p'Repository: FalkorDB/falkordb-browser
Length of output: 2441
Prevent oversized multipart bodies before request.formData()
request.formData() still buffers the full multipart body before file.size runs, so a large upload can exhaust memory in this path. Add an early Content-Length gate here or switch this branch to streaming parsing; keep an upstream proxy/body limit for chunked requests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/upload/route.ts` around lines 72 - 74, Update the upload handler
before request.formData() to validate the request’s Content-Length against the
configured maximum and reject oversized bodies before buffering; retain the
existing file.size validation afterward, and rely on the upstream proxy/body
limit for requests without a usable Content-Length.
Source: Path instructions
| return { ok: false, error: "Invalid file name.", status: 400 }; | ||
| } | ||
|
|
||
| fs.mkdirSync(uploadsDir, { recursive: true }); |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Replace synchronous filesystem call with its asynchronous equivalent.
fs.mkdirSync blocks the Node.js event loop, stalling all other concurrent requests while the directory is created. Since this is an asynchronous Route Handler, use the non-blocking fs.promises equivalent.
⚡ Proposed fix
- fs.mkdirSync(uploadsDir, { recursive: true });
+ await fs.promises.mkdir(uploadsDir, { recursive: true });📝 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.
Suggested change
| fs.mkdirSync(uploadsDir, { recursive: true }); | |
| await fs.promises.mkdir(uploadsDir, { recursive: true }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/upload/route.ts` at line 125, Replace the synchronous fs.mkdirSync
call in the upload route with the asynchronous fs.promises.mkdir equivalent,
preserving the recursive directory-creation option and awaiting completion
within the existing async handler.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a “direct upload” path for CSV temp storage so browsers can upload CSV bytes straight to S3 or Vercel Blob (instead of proxying multipart uploads through the backend), while tightening local-mode behavior and improving upload/import diagnostics.
Changes:
- Introduces
/api/csv-temp/directto mint provider-specific direct upload targets (S3 presigned PUT, Vercel Blob client token) and updates the UI to use them with a fallback flow. - Refactors CSV storage provider resolution/caching and updates providers to support optional direct-upload target generation.
- Improves user-facing upload and LOAD CSV error handling (multipart parsing, CSP/CORS hints, early CSV “head” validation), plus local MinIO TLS+CORS dev wiring.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| proxy.ts | Expands CSP connect-src for Vercel Blob control-plane; skips CSP nonce/header mutation for API routes to avoid multipart parsing issues. |
| lib/utils.ts | Extends allowlisted “user-readable” error patterns for improved upload failures. |
| docker/minio-tls/Caddyfile | Adds local TLS reverse proxy + CORS handling for MinIO. |
| docker-compose.minio.yml | Adds minio-tls Caddy sidecar and volumes/ports. |
| app/lib/csv-storage.ts | Adds storage mode/type definitions, direct-upload target contract, and provider caching signature. |
| app/lib/csv-storage-vercel-blob.ts | Updates blob pathname scheme and adds direct-upload token generation. |
| app/lib/csv-storage-s3.ts | Adds S3 presigned PUT direct-upload target and read-endpoint rewriting support. |
| app/lib/csv-storage-local.ts | Explicitly returns null for direct-upload targets (local mode). |
| app/components/graph/UploadGraph.tsx | Implements client-side direct upload to S3/Blob with progress + fallback to legacy upload. |
| app/api/upload/route.ts | Improves multipart parsing errors and switches small uploads to request.formData() in non-dump mode. |
| app/api/graph/[graph]/upload/upload-utils.ts | Adds .txt-specific normalization to strip obvious non-Cypher lines before splitting. |
| app/api/graph/[graph]/upload/upload-utils.test.ts | Adds tests for .txt normalization behavior and non-normalization for .cypher. |
| app/api/graph/[graph]/upload/route.ts | Passes source extension into executeCypherBatch. |
| app/api/graph/[graph]/load-csv/route.ts | Adds early HEAD-bytes validation and improves fetchability guidance in error messaging. |
| app/api/csv-temp/route.ts | Rejects proxied multipart CSV uploads when storage mode is cloud (requires direct uploads). |
| app/api/csv-temp/direct/route.ts | New endpoint to mint direct upload targets (CORS + auth + read-only enforcement + size checks). |
| app/api/csv-temp/[id]/route.ts | Hard-disables local CSV serving endpoint in non-local storage modes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+534
to
+536
| /^failed to parse upload\.?$/i, | ||
| /^malformed multipart body\b/i, | ||
| /^expected multipart\/form-data with a boundary\.?$/i, |
Comment on lines
+14
to
+26
| interface DirectUploadBody { | ||
| filename?: string; | ||
| contentType?: string; | ||
| sizeBytes?: number; | ||
| } | ||
|
|
||
| function sanitizeDirectUploadRequest(body: unknown): CsvDirectUploadRequest | null { | ||
| if (!body || typeof body !== "object" || Array.isArray(body)) return null; | ||
|
|
||
| const input = body as Record<string, unknown>; | ||
| if (typeof input.filename !== "string" || !input.filename.trim()) return null; | ||
| if (typeof input.sizeBytes !== "number" || !Number.isFinite(input.sizeBytes) || input.sizeBytes <= 0) return null; | ||
| if (input.contentType !== undefined && typeof input.contentType !== "string") return null; |
Comment on lines
+151
to
+156
| process.env.S3_READ_ENDPOINT ?? "", | ||
| process.env.S3_READ_URL_HOST ?? "", | ||
| process.env.S3_READ_URL_PORT ?? "", | ||
| process.env.S3_READ_URL_PROTOCOL ?? "", | ||
| process.env.S3_READ_URL_DOCKER_GATEWAY ?? "", | ||
| process.env.BLOB_READ_WRITE_TOKEN ?? "", |
Comment on lines
+225
to
+229
| async createDirectUploadTarget( | ||
| owner: string, | ||
| key: string, | ||
| request: CsvDirectUploadRequest | ||
| ): Promise<CsvDirectUploadTarget | null> { |
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
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