Document Upload Progress Indicator & Upload Limit#109
Open
Document Upload Progress Indicator & Upload Limit#109
Conversation
📝 WalkthroughWalkthroughFrontend upload flow now streams uploads via a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / Browser
participant Modal as UploadDocumentModal
participant Page as Document Page
participant Client as uploadWithProgress
participant API as API Handler (POST /api/document)
participant Backend as Backend Service
User->>Modal: select file & start upload
Modal->>Page: submit file -> handleUpload
Page->>Page: set uploadProgress=0, uploadPhase="uploading"
Page->>Client: uploadWithProgress(url, apiKey, formData, onProgress)
Client->>API: POST (duplex: "half") streaming body
API->>Backend: forward buffered 64KB slices
loop streaming upload progress
API->>Client: { "phase":"uploading","progress": X } (NDJSON)
Client->>Page: onProgress(X,"uploading")
Page->>Modal: update progress bar (X%)
end
Backend->>API: processing complete
API->>Client: { "phase":"processing" } then { "done":true, "status":..., "data":... }
Client->>Page: onProgress(100,"processing") then resolve
Page->>Modal: set uploadPhase="done" / show completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/(main)/document/page.tsx (1)
110-114:⚠️ Potential issue | 🟡 MinorSkip the failure toast on intentional cancellation.
Closing the modal aborts the request at Lines 225-226, so this catch block currently reports every user-cancelled upload as an error. Return early for
AbortErrorand letfinallyhandle the cleanup.Suggested change
} catch (error) { + if (error instanceof Error && error.name === "AbortError") { + return; + } console.error("Upload error:", error); toast.error( `Failed to upload document: ${error instanceof Error ? error.message : "Unknown error"}`, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`(main)/document/page.tsx around lines 110 - 114, The catch block that currently logs and toasts all errors should skip intentional cancellations: detect an AbortError (e.g., check error.name === "AbortError" or error instanceof DOMException with name "AbortError") and return early before calling console.error or toast.error so the finally cleanup still runs; leave the existing toast.error and console.error for non-abort errors (reference the catch block handling error, the toast.error call, and the finally cleanup).
🤖 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/document/route.ts`:
- Around line 82-84: The current code emits the { phase: "processing" } event
after awaiting apiClient(...), so clients learn "processing" only after the
backend finished; move the writer.write(encoder.encode(JSON.stringify({ phase:
"processing" }) + "\n")) call so it is executed immediately after the last chunk
is pulled from uploadBody (i.e., right after the loop that reads uploadBody
completes) and before calling await apiClient(...), and keep the final { done:
true, ... } event to be written only when the apiClient response is received;
update references to writer.write, encoder.encode, uploadBody, and apiClient
accordingly.
- Around line 32-33: The code currently calls await request.arrayBuffer()
(creating fileBuffer and totalSize) which buffers the entire upload; change the
proxying logic in app/api/document/route.ts to stream request.body directly to
the upstream (or to the file sink) and track bytes forwarded by reading chunks
from the ReadableStream, incrementing a counter instead of using
fileBuffer/totalSize, so memory usage stays constant and backpressure is
preserved; replace usages of fileBuffer and totalSize with the streamed byte
counter and forward each chunk to the destination write stream or fetch body.
- Around line 74-80: The upstream POST via apiClient is not receiving the
browser abort signal, so forward the incoming request's AbortSignal to the
upstream call by passing signal: request.signal (or request.body?.signal if
wrapped) into the apiClient fetch/init options; update the call that constructs
the POST to include the signal alongside method, body, headers, and duplex so
the upstream request is aborted when the browser aborts (refer to apiClient and
the request/uploadBody variables to locate the call).
In `@app/lib/apiClient.ts`:
- Around line 56-63: Before starting the NDJSON line-reader, check the HTTP
response status and content-type on the Fetch response (res) and handle
non-stream error payloads by reading and parsing them as JSON/text; if res.ok is
false or content-type is not an NDJSON/stream type, await res.text() or
res.json() and throw a new Error that includes the server message so the real
error is surfaced instead of falling through to the NDJSON reader. Update the
code around the fetch response check (the res variable and the NDJSON parsing
block referenced in lines ~70-107) to gate entering the line-reader only when
the status and content-type indicate a streaming NDJSON response. Ensure the
controller.signal handling remains the same and surface the parsed server error
in the thrown exception.
---
Outside diff comments:
In `@app/`(main)/document/page.tsx:
- Around line 110-114: The catch block that currently logs and toasts all errors
should skip intentional cancellations: detect an AbortError (e.g., check
error.name === "AbortError" or error instanceof DOMException with name
"AbortError") and return early before calling console.error or toast.error so
the finally cleanup still runs; leave the existing toast.error and console.error
for non-abort errors (reference the catch block handling error, the toast.error
call, and the finally cleanup).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ed9e38f-8efe-4a6f-93ab-d83800aad5e0
📒 Files selected for processing (6)
app/(main)/document/page.tsxapp/(main)/knowledge-base/page.tsxapp/api/document/route.tsapp/components/document/UploadDocumentModal.tsxapp/lib/apiClient.tsapp/lib/constants.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/api/document/route.ts (2)
32-33:⚠️ Potential issue | 🟠 MajorStream the inbound request instead of buffering it.
await request.arrayBuffer()makes memory usage proportional to the full multipart payload and removes most of the backpressure benefit of this proxy. It also prevents the route from sending any progress until the browser → Next.js hop has already finished. Read fromrequest.bodydirectly and update the forwarded-byte counter as chunks are enqueued upstream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/document/route.ts` around lines 32 - 33, Replace the full-buffering logic that uses await request.arrayBuffer() and the fileBuffer/totalSize variables with streaming reads from request.body (the incoming ReadableStream); read chunks from request.body, increment the forwarded-byte counter as each chunk is enqueued to the upstream request, and forward chunks directly to the outbound fetch/stream instead of assembling a single Uint8Array. Update any logic that used totalSize to account for streamed progress updates, and remove the await request.arrayBuffer() and fileBuffer creation to avoid buffering the whole payload.
83-85:⚠️ Potential issue | 🟠 MajorEmit the
processingphase before awaiting the backend response.
app/lib/apiClient.ts:71-101only switches the UI to"processing"when this event arrives. Because it is written afterawait apiClient(...), the client effectively jumps fromuploadingtodone. Emit"processing"whenuploadBodycloses, then keep{ done: true, status, data }as the terminal event.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/document/route.ts` around lines 83 - 85, The code currently writes the {"phase":"processing"} event after awaiting the backend response so the client transitions from "uploading" to "done"; move the writer.write call that emits the processing phase to immediately after uploadBody closes (i.e., before calling apiClient) so the client receives the processing event before the backend roundtrip; keep the final terminal event shape ({ done: true, status, data }) as the last write after apiClient returns; look for the writer.write call in app/api/document/route.ts and the apiClient invocation in app/lib/apiClient.ts to implement this ordering change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/api/document/route.ts`:
- Around line 32-33: Replace the full-buffering logic that uses await
request.arrayBuffer() and the fileBuffer/totalSize variables with streaming
reads from request.body (the incoming ReadableStream); read chunks from
request.body, increment the forwarded-byte counter as each chunk is enqueued to
the upstream request, and forward chunks directly to the outbound fetch/stream
instead of assembling a single Uint8Array. Update any logic that used totalSize
to account for streamed progress updates, and remove the await
request.arrayBuffer() and fileBuffer creation to avoid buffering the whole
payload.
- Around line 83-85: The code currently writes the {"phase":"processing"} event
after awaiting the backend response so the client transitions from "uploading"
to "done"; move the writer.write call that emits the processing phase to
immediately after uploadBody closes (i.e., before calling apiClient) so the
client receives the processing event before the backend roundtrip; keep the
final terminal event shape ({ done: true, status, data }) as the last write
after apiClient returns; look for the writer.write call in
app/api/document/route.ts and the apiClient invocation in app/lib/apiClient.ts
to implement this ordering change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/document/UploadDocumentModal.tsx`:
- Around line 106-127: The UI doesn't handle the "done" UploadPhase so users
briefly see "Uploading... 100%" when onProgress(100, "done") is emitted; update
the conditional rendering in UploadDocumentModal (look for isUploading,
uploadPhase, uploadProgress) to explicitly handle uploadPhase === "done"
(separate from "processing" and "uploading") by showing a completion message
like "Done" or "Upload complete", rendering the progress bar at 100% without the
uploading transition/animation (no animate-pulse or transition) and optionally
keep the percentage as 100% or hide it; ensure this branch is used when
uploadPhase === "done" so the UI reflects completion until the parent clears
isUploading.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 218e1fa3-3153-4f2f-b0ca-3e432d941503
📒 Files selected for processing (6)
app/(main)/document/page.tsxapp/components/document/DocumentListing.tsxapp/components/document/DocumentPreview.tsxapp/components/document/UploadDocumentModal.tsxapp/lib/constants.tsapp/lib/types/document.ts
✅ Files skipped from review due to trivial changes (2)
- app/lib/types/document.ts
- app/lib/constants.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/(main)/document/page.tsx
Ayush8923
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/components/document/DocumentListing.tsx (1)
125-133: Add an explicit accessible name to the icon-only delete button.
titleis not a reliable accessible name across assistive tech. Addaria-labelfor consistent screen-reader support.♿ Suggested tweak
<button onClick={(e) => { e.stopPropagation(); onDelete(doc.id); }} className="p-1.5 rounded-md transition-colors shrink-0 border border-[hsl(8,86%,80%)] bg-[hsl(0,0%,100%)] text-[hsl(8,86%,40%)] hover:bg-[hsl(8,86%,95%)] cursor-pointer" + aria-label={`Delete ${doc.fname}`} title="Delete Document" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/document/DocumentListing.tsx` around lines 125 - 133, The delete button in DocumentListing (the button wrapping <TrashIcon /> and invoking onDelete(doc.id)) lacks an explicit accessible name; add an aria-label attribute to that button (e.g., aria-label="Delete document" or aria-label={`Delete ${doc.name}`} with a sensible fallback) so screen readers get a reliable name instead of relying on title; keep the existing onClick/onDelete and visual title if desired but ensure aria-label is present on the button element.app/components/document/DocumentPreview.tsx (2)
108-128: Consider collapsing duplicated download link branches.Both branches render the same anchor with only
downloadbehavior differing; this is a good candidate for a small cleanup.♻️ Simplify duplicated markup
- {(document.signed_url || document.object_store_url) && - (isPreviewable(document.fname) ? ( - <a - href={document.signed_url || document.object_store_url} - target="_blank" - rel="noopener noreferrer" - className="shrink-0 px-4 py-1.5 rounded-md text-sm font-medium transition-colors bg-[`#171717`] text-white hover:bg-accent-hover" - > - Download - </a> - ) : ( - <a - href={document.signed_url || document.object_store_url} - download={document.fname} - target="_blank" - rel="noopener noreferrer" - className="shrink-0 px-4 py-1.5 rounded-md text-sm font-medium transition-colors bg-[`#171717`] text-white hover:bg-accent-hover" - > - Download - </a> - ))} + {(document.signed_url || document.object_store_url) && ( + <a + href={document.signed_url || document.object_store_url} + download={isPreviewable(document.fname) ? undefined : document.fname} + target="_blank" + rel="noopener noreferrer" + className="shrink-0 px-4 py-1.5 rounded-md text-sm font-medium transition-colors bg-[`#171717`] text-white hover:bg-accent-hover" + > + Download + </a> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/document/DocumentPreview.tsx` around lines 108 - 128, The JSX in DocumentPreview duplicates the same <a> element in both branches; consolidate by computing href = document.signed_url || document.object_store_url and a conditional downloadProp = isPreviewable(document.fname) ? undefined : document.fname, then render one anchor with href, target, rel, className and download={downloadProp}; update references to isPreviewable, document.fname, document.signed_url/document.object_store_url and keep the existing attributes and text.
152-157: Replace raw<img>withnext/imagefor better optimization.This aligns with the
@next/next/no-img-elementlint rule from your Next.js ESLint config. However, implementing this requires configuring the signed URL domain innext.config.tsviaremotePatternsfirst, since Next.js Image requires explicit remote domain allowlisting. Consider the trade-off: signed URLs are typically ephemeral, so Next.image's image optimization benefits may be limited. If you proceed, ensure proper sizing constraints are set to avoid layout shifts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/document/DocumentPreview.tsx` around lines 152 - 157, Replace the raw <img> in DocumentPreview with Next.js' Image component: import Image from 'next/image', use <Image ...> in the DocumentPreview render (keep using document.signed_url and document.fname), provide explicit sizing (width/height) or use layout="fill" with a positioned container to prevent layout shifts, and wire Image's onError to call setImageLoadError(true) (or use onLoadingComplete fallback) so existing error handling remains. Also add the signed URL host to next.config.ts via images.remotePatterns (allowlist the domain/path pattern used by document.signed_url) so Next can load remote images, and adjust CSS/className for Image wrapper since Image doesn't accept all <img> props.
🤖 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/components/document/DocumentPreview.tsx`:
- Around line 45-47: The formatFileSize function treats zero as missing because
it uses a falsy check; change the existence check from if (!size) to a
null/undefined-only check (e.g., if (size == null) or typeof size ===
'undefined') so that 0 is handled and returns "0 B", keeping the remaining logic
(size < 1024, etc.) unchanged.
- Around line 169-171: The message in DocumentPreview.tsx uses the ext variable
inside the <p> element and renders "Preview is not available for .{ext} files",
which produces incorrect output when ext is empty; update the JSX in the
DocumentPreview component to check ext and render a fallback when it's falsy
(for example "Preview is not available for this file" or "Preview is not
available for files without an extension") and keep the original ".{ext} files"
text only when ext is non-empty.
---
Nitpick comments:
In `@app/components/document/DocumentListing.tsx`:
- Around line 125-133: The delete button in DocumentListing (the button wrapping
<TrashIcon /> and invoking onDelete(doc.id)) lacks an explicit accessible name;
add an aria-label attribute to that button (e.g., aria-label="Delete document"
or aria-label={`Delete ${doc.name}`} with a sensible fallback) so screen readers
get a reliable name instead of relying on title; keep the existing
onClick/onDelete and visual title if desired but ensure aria-label is present on
the button element.
In `@app/components/document/DocumentPreview.tsx`:
- Around line 108-128: The JSX in DocumentPreview duplicates the same <a>
element in both branches; consolidate by computing href = document.signed_url ||
document.object_store_url and a conditional downloadProp =
isPreviewable(document.fname) ? undefined : document.fname, then render one
anchor with href, target, rel, className and download={downloadProp}; update
references to isPreviewable, document.fname,
document.signed_url/document.object_store_url and keep the existing attributes
and text.
- Around line 152-157: Replace the raw <img> in DocumentPreview with Next.js'
Image component: import Image from 'next/image', use <Image ...> in the
DocumentPreview render (keep using document.signed_url and document.fname),
provide explicit sizing (width/height) or use layout="fill" with a positioned
container to prevent layout shifts, and wire Image's onError to call
setImageLoadError(true) (or use onLoadingComplete fallback) so existing error
handling remains. Also add the signed URL host to next.config.ts via
images.remotePatterns (allowlist the domain/path pattern used by
document.signed_url) so Next can load remote images, and adjust CSS/className
for Image wrapper since Image doesn't accept all <img> props.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59bb784e-78ce-4eb5-9d4e-ca617d3ae0f4
📒 Files selected for processing (6)
app/components/Modal.tsxapp/components/document/DocumentListing.tsxapp/components/document/DocumentPreview.tsxapp/components/document/UploadDocumentModal.tsxapp/components/icons/document/DocumentFileIcon.tsxapp/components/speech-to-text/ModelComparisonCard.tsx
💤 Files with no reviewable changes (1)
- app/components/speech-to-text/ModelComparisonCard.tsx
✅ Files skipped from review due to trivial changes (2)
- app/components/Modal.tsx
- app/components/icons/document/DocumentFileIcon.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/document/UploadDocumentModal.tsx
Comment on lines
+45
to
+47
| const formatFileSize = (size?: number) => { | ||
| if (!size) return "N/A"; | ||
| if (size < 1024) return `${size} B`; |
There was a problem hiding this comment.
formatFileSize mislabels zero-byte files.
if (!size) treats 0 as missing and returns "N/A" instead of "0 B".
✅ Minimal fix
const formatFileSize = (size?: number) => {
- if (!size) return "N/A";
+ if (size == null) return "N/A";
if (size < 1024) return `${size} B`;📝 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
| const formatFileSize = (size?: number) => { | |
| if (!size) return "N/A"; | |
| if (size < 1024) return `${size} B`; | |
| const formatFileSize = (size?: number) => { | |
| if (size == null) return "N/A"; | |
| if (size < 1024) return `${size} B`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/document/DocumentPreview.tsx` around lines 45 - 47, The
formatFileSize function treats zero as missing because it uses a falsy check;
change the existence check from if (!size) to a null/undefined-only check (e.g.,
if (size == null) or typeof size === 'undefined') so that 0 is handled and
returns "0 B", keeping the remaining logic (size < 1024, etc.) unchanged.
Comment on lines
+169
to
+171
| <p className="text-sm font-medium text-text-primary mb-1"> | ||
| Preview is not available for .{ext} files | ||
| </p> |
There was a problem hiding this comment.
Handle extension-less filenames in unsupported preview text.
For files without an extension, this currently renders ". files".
💬 Suggested fallback text
- <p className="text-sm font-medium text-text-primary mb-1">
- Preview is not available for .{ext} files
- </p>
+ <p className="text-sm font-medium text-text-primary mb-1">
+ {ext
+ ? `Preview is not available for .${ext} files`
+ : "Preview is not available for this file type"}
+ </p>📝 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
| <p className="text-sm font-medium text-text-primary mb-1"> | |
| Preview is not available for .{ext} files | |
| </p> | |
| <p className="text-sm font-medium text-text-primary mb-1"> | |
| {ext | |
| ? `Preview is not available for .${ext} files` | |
| : "Preview is not available for this file type"} | |
| </p> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/document/DocumentPreview.tsx` around lines 169 - 171, The
message in DocumentPreview.tsx uses the ext variable inside the <p> element and
renders "Preview is not available for .{ext} files", which produces incorrect
output when ext is empty; update the JSX in the DocumentPreview component to
check ext and render a fallback when it's falsy (for example "Preview is not
available for this file" or "Preview is not available for files without an
extension") and keep the original ".{ext} files" text only when ext is
non-empty.
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
1 participant
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.
Issue: #106
Summary:
ReadableStream, sending progress events asNDJSONback to the browser.uploadWithProgressclient utility that reads the streaming response and reports two phases: uploading (0–100%) and processing (pulsing animation while backend processes the file)AbortController- closing the modal aborts the in-flight requestUploadDocumentModalto use the sharedModalcomponent and extract accepted file types intoACCEPTED_DOCUMENT_TYPESconstantHow it's works:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Removed