Bugfix cmgkqwns100001r2w9stk3a02 #1086
Conversation
…e6f5f54ddca65f7c0a2218f8f3
…4662da4a62bb6516fbf11d6757
…087ca6494ea3fa2e87e106c7f2
…5d953b435f99153d8840d93b28
…f6bddc408f82845c5c4cf39895
…4de8f34227a7707f4d6fdde5f8
…1e55b04531b620083161db40f1
…dc122840269ed274c9a8df96d6
…37bdf04391a76e8648e2b582a5
…2b181f42b3b7e2232e1fe4f497
Ravishekhar7870
requested review from
devesh-juspay,
junaid-shirur,
kalpadhwaryu,
shivamashtikar and
zereraz
as code owners
WalkthroughImplements cancellable file uploads end-to-end. Frontend adds per-file AbortControllers, consistent upload state/error handling, and UI hooks for cancel/removal. authFetch forwards AbortSignal to refresh-token requests. Server upload endpoints add early-abort checks and standardized 499 “Upload cancelled” responses across parsing and per-file processing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant C as ChatBox (Frontend)
participant AF as authFetch
participant S as Server /api/files
rect rgba(221,238,255,0.5)
U->>C: Select files
C->>C: Create AbortController per file
C->>S: POST /files (signal)
note over C,S: Upload with progress and per-file state
end
alt Upload succeeds
S-->>C: 200 + file metadata
C->>C: Update state (uploading=false, store metadata)
else Upload fails
S-->>C: 4xx/5xx error
C->>C: Set uploadError, toast, cleanup controller
else User cancels
U-->>C: Click cancel/remove
C->>C: controller.abort()
C->>S: Abort signal
S-->>C: 499 Upload cancelled
C->>C: Mark uploading=false, cleanup
end
rect rgba(232,252,239,0.5)
note over AF,S: Token refresh path
C->>AF: fetch(..., signal)
AF->>S: refresh-token (propagates signal)
opt Aborted during refresh
S-->>AF: Aborted/terminated
AF-->>C: Propagate abort
C->>C: Handle as cancellation
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
Summary of ChangesHello @Ravishekhar7870, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a bug by introducing comprehensive cancellation capabilities for file uploads. It integrates Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a file upload cancellation feature, which is a great addition. The implementation is solid, with changes on both the frontend to manage cancellation via AbortController and on the backend to handle aborted requests. I've identified a critical bug related to state management during cancellation that could lead to an inconsistent UI state. I've also included a few suggestions to improve performance, robustness, and code clarity. Overall, great work on this complex feature.
| setUploadControllers((prev) => { | ||
| const newMap = new Map(prev) | ||
| newMap.delete(id) | ||
| return newMap | ||
| }) | ||
| setUploadingFilesCount((prev) => Math.max(0, prev - 1)) |
There was a problem hiding this comment.
These state updates are redundant and introduce a bug. When an upload is aborted via controller.abort(), the finally block in the uploadFile function is also triggered, which cleans up uploadControllers and decrements uploadingFilesCount. By calling the state setters here as well, uploadingFilesCount is decremented twice, leading to an incorrect count and potential UI bugs. The cleanup logic should be centralized in the uploadFile function to ensure it's the single source of truth.
| const uploadPromises = files.map(async (selectedFile) => { | ||
| try { | ||
| const formData = new FormData() | ||
| formData.append("attachment", selectedFile.file) | ||
| const response = await authFetch( | ||
| "/api/v1/files/upload-attachment", | ||
| { | ||
| method: "POST", | ||
| body: formData, | ||
| }, | ||
| ) | ||
|
|
||
| if (!response.ok) { | ||
| const error = await response.json() | ||
| throw new Error(error.message || "Upload failed") | ||
| } | ||
|
|
||
| const result = await response.json() | ||
| const metadata = result.attachments?.[0] | ||
|
|
||
| if (metadata) { | ||
| setSelectedFiles((prev) => | ||
| prev.map((f) => | ||
| f.id === selectedFile.id | ||
| ? { ...f, uploading: false, metadata } | ||
| : f, | ||
| ), | ||
| ) | ||
| return metadata | ||
| } else { | ||
| throw new Error("No document ID returned from upload") | ||
| } | ||
| } catch (error) { | ||
| const errorMessage = | ||
| error instanceof Error ? error.message : "Upload failed" | ||
| setSelectedFiles((prev) => | ||
| prev.map((f) => | ||
| f.id === selectedFile.id | ||
| ? { ...f, uploading: false, uploadError: errorMessage } | ||
| : f, | ||
| ), | ||
| ) | ||
| toast.error({ | ||
| title: "Upload failed", | ||
| description: `Failed to upload ${selectedFile.file.name}: ${errorMessage}`, | ||
| }) | ||
| return null | ||
| } finally { | ||
| setUploadingFilesCount((prev) => prev - 1) | ||
| } | ||
| const controller = new AbortController() | ||
| setUploadControllers((prev) => new Map(prev).set(selectedFile.id, controller)) | ||
|
|
||
| return uploadFile(selectedFile, controller.signal) | ||
| }) |
There was a problem hiding this comment.
Calling a state setter (setUploadControllers) inside a .map() loop can trigger a state update for each file, which may lead to performance issues due to multiple re-renders. It's more efficient to batch these updates. You can create all the AbortController instances first, update the state once with all of them, and then map over the files to start the uploads.
const newControllers = new Map<string, AbortController>();
files.forEach((file) => newControllers.set(file.id, new AbortController()));
setUploadControllers((prev) => new Map([...prev, ...newControllers]));
const uploadPromises = files.map((selectedFile) => {
const controller = newControllers.get(selectedFile.id)!;
return uploadFile(selectedFile, controller.signal);
});
| // Abort any ongoing uploads | ||
| uploadControllers.forEach((controller) => { | ||
| controller.abort() | ||
| }) | ||
| } | ||
| }, []) | ||
| }, [uploadControllers]) |
There was a problem hiding this comment.
Aborting uploads on component unmount is good practice. However, this will cause the finally block in uploadFile to call state setters on an unmounted component, which will trigger a React warning in development: "Can't perform a React state update on an unmounted component."
To fix this, you can use a ref to track the component's mounted status.
-
Add a ref to your component to track its mounted state:
const mountedRef = React.useRef(true); React.useEffect(() => { return () => { mountedRef.current = false; }; }, []);
-
Then, guard the state updates in
uploadFile'sfinallyblock:// inside uploadFile } finally { if (mountedRef.current) { setUploadControllers((prev) => { const newMap = new Map(prev); newMap.delete(selectedFile.id); return newMap; }); setUploadingFilesCount((prev) => prev - 1); } }
| // Check if request was aborted | ||
| if (c.req.raw.signal?.aborted) { | ||
| return c.json({ error: 'Upload cancelled' }, 499) | ||
| } |
There was a problem hiding this comment.
This if (c.req.raw.signal?.aborted) check is repeated in multiple places within this function. To improve code clarity, reduce duplication, and make the cancellation logic easier to maintain, consider refactoring this into a reusable helper function.
For example, you could create a function that throws a specific error that your main try...catch block can handle:
const checkAborted = (c: Context) => {
if (c.req.raw.signal?.aborted) {
throw new HTTPException(499, { message: "Upload cancelled" });
}
};
// Then in your handler:
try {
checkAborted(c);
const formData = await c.req.formData();
checkAborted(c);
// ...
} catch (error) {
if (error instanceof HTTPException && error.status === 499) {
return c.json({ error: error.message }, 499);
}
// ... other error handling
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/ChatBox.tsx (1)
2246-2258: Don’t abort active uploads on state updatesThis effect depends on
uploadControllers, so every time the map changes (e.g., when you add or remove a controller), the cleanup from the previous render runs and aborts all controllers captured in that closure. That means finishing one file or adding another cancels the rest of the ongoing uploads, defeating the whole feature.Hold the controllers in a ref for cleanup and run the abort loop only on unmount, e.g.
- useEffect(() => { - return () => { - const previewUrls = selectedFilesRef.current - .map((f) => f.preview) - .filter(Boolean) as string[] - cleanupPreviewUrls(previewUrls) - - // Abort any ongoing uploads - uploadControllers.forEach((controller) => { - controller.abort() - }) - } - }, [uploadControllers]) + const uploadControllersRef = useRef(uploadControllers) + useEffect(() => { + uploadControllersRef.current = uploadControllers + }, [uploadControllers]) + + useEffect(() => { + return () => { + const previewUrls = selectedFilesRef.current + .map((f) => f.preview) + .filter(Boolean) as string[] + cleanupPreviewUrls(previewUrls) + + uploadControllersRef.current.forEach((controller) => { + controller.abort() + }) + } + }, [])This preserves the unmount cleanup without killing active uploads on every state change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/ChatBox.tsx(9 hunks)frontend/src/utils/authFetch.ts(1 hunks)server/api/files.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/ChatBox.tsx (2)
frontend/src/components/ClFileUpload.tsx (1)
SelectedFile(8-12)frontend/src/utils/authFetch.ts (1)
authFetch(4-28)
🪛 GitHub Actions: TypeScript Build Check
server/api/files.ts
[error] 202-202: TS2769: No overload matches this call. Argument of type '499' is not assignable to parameter of type 'ContentfulStatusCode | undefined'.
[error] 220-220: TS2769: No overload matches this call. Argument of type '499' is not assignable to parameter of type 'ContentfulStatusCode | undefined'.
[error] 243-243: TS2769: No overload matches this call. Argument of type '499' is not assignable to parameter of type 'ContentfulStatusCode | undefined'.
[error] 258-258: TS2769: No overload matches this call. Argument of type '499' is not assignable to parameter of type 'ContentfulStatusCode | undefined'.
[error] 422-422: TS2769: No overload matches this call. Argument of type '499' is not assignable to parameter of type 'ContentfulStatusCode | undefined'.
| setUploadControllers((prev) => { | ||
| const newMap = new Map(prev) | ||
| newMap.delete(selectedFile.id) | ||
| return newMap | ||
| }) | ||
| setUploadingFilesCount((prev) => prev - 1) | ||
| } |
There was a problem hiding this comment.
Prevent double decrement of uploading count
We’re decrementing uploadingFilesCount twice for the same upload: once in removeFile (Lines 1007-1016) and again in uploadFile’s finally (Lines 885-891). When a user cancels an upload, the manual decrement drops the counter immediately, and the finally runs shortly after and knocks it down again. With multiple files, this enables “Send” while other uploads are still active and can even drive the counter negative, breaking the uploadCompleteResolver logic.
Remove the manual decrement from removeFile and clamp the finally decrement so it only runs once per upload:
- if (fileToRemove?.uploading) {
- const controller = uploadControllers.get(id)
- controller?.abort()
- setUploadControllers((prev) => {
- const newMap = new Map(prev)
- newMap.delete(id)
- return newMap
- })
- setUploadingFilesCount((prev) => Math.max(0, prev - 1))
- }
+ if (fileToRemove?.uploading) {
+ const controller = uploadControllers.get(id)
+ controller?.abort()
+ setUploadControllers((prev) => {
+ const newMap = new Map(prev)
+ newMap.delete(id)
+ return newMap
+ })
+ }
...
- setUploadingFilesCount((prev) => prev - 1)
+ setUploadingFilesCount((prev) => Math.max(0, prev - 1))This keeps the counter accurate and the send button correctly disabled.
Also applies to: 1007-1016
🤖 Prompt for AI Agents
In frontend/src/components/ChatBox.tsx around lines 885-891 (and also apply
change to 1007-1016), remove the manual setUploadingFilesCount((prev) => prev -
1) call from removeFile, and instead ensure the finally block in uploadFile
performs an atomic check-and-decrement: update setUploadControllers via its
functional updater to detect whether the selectedFile.id is present in the map,
delete it only if present, and only then decrement setUploadingFilesCount once;
if the id is already gone, do not decrement. This guarantees the counter is
decremented exactly once per upload and prevents negative or double decrements.
| // Check if request was aborted | ||
| if (c.req.raw.signal?.aborted) { | ||
| return c.json({ error: 'Upload cancelled' }, 499) | ||
| } |
There was a problem hiding this comment.
Fix 499 status type mismatch
The TypeScript build is failing (TS2769 in the pipeline log) because Hono’s c.json second argument is typed as ContentfulStatusCode, which does not include 499. Every return c.json(..., 499) here (Lines 200, 219, 243, 258, and 421) breaks the build. Please either switch to a status that’s part of Hono’s union or construct the response manually, e.g.
- return c.json({ error: 'Upload cancelled' }, 499)
+ return new Response(JSON.stringify({ error: "Upload cancelled" }), {
+ status: 499,
+ headers: { "content-type": "application/json" },
+ })and mirror that change in the other call sites so the build passes while still emitting 499.
Also applies to: 219-221, 242-244, 257-259, 421-423
🧰 Tools
🪛 GitHub Actions: TypeScript Build Check
[error] 202-202: TS2769: No overload matches this call. Argument of type '499' is not assignable to parameter of type 'ContentfulStatusCode | undefined'.
🤖 Prompt for AI Agents
In server/api/files.ts around lines 200-203 (and similarly at 219-221, 242-244,
257-259, 421-423), the code returns c.json(..., 499) but Hono's c.json second
argument type doesn't include 499; replace those calls with a manually
constructed Response that sets status to 499 and content-type to
application/json, e.g. return new Response(JSON.stringify({ error: 'Upload
cancelled' }), { status: 499, headers: { 'Content-Type': 'application/json' }
}); apply the same replacement at each listed call site so the build type error
is resolved while preserving the 499 status.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes