Bugfix cmgoz1sdq00009mh3txngg3fm #1094
Conversation
…16a6d24610bcf84b12bea54549
…b58d0b4c58ac27d857f46d6fb9
…5ecf4d4f5aad30e7e43e575b8e
…fd91d44038a30f765857735513
…40d9ec4a59a72f5a541f1b1cc9
…7b8b3e448f834aa5e97b2491af
…6806814fb59e4f781eb6aefcd7
…835d28495e88bcd4f74ee921c8
…9f01f34e89b6693fa2f3fe0402
…3a22c24a6bba7ce545bfd18c5f
…dfd00f4a04afae77a97bc11fa4
…0bc6a84feb9b088d2c6118b0bf
Ravishekhar7870
requested review from
devesh-juspay,
junaid-shirur,
kalpadhwaryu,
shivamashtikar and
zereraz
as code owners
WalkthroughAdds abortable file uploads across frontend and server: introduces AbortController-based per-file cancellation in ChatBox, augments upload progress Zustand store with removeProgress, adds tests for cancellation flows, updates server to return 499 on aborted attachments, and adds the zustand dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant C as ChatBox (React)
participant S as UploadStore (Zustand)
participant B as fetch()/AbortController
participant API as Server /files
participant T as Toast
U->>C: Select file(s) to upload
C->>S: set uploading state per file
C->>B: Create AbortController per file
C->>API: POST /files (signal=controller.signal)
alt Upload completes
API-->>C: 200 OK
C->>S: mark uploaded
else User cancels
U-->>C: Click X (Cancel)
C->>B: controller.abort()
API-->>C: 499 Upload cancelled
C->>S: mark uploadAborted & cleanup
C--x T: (no error toast on AbortError)
end
sequenceDiagram
autonumber
participant C as ChatBox (Unmount)
participant S as UploadStore
participant B as AbortController Map
C->>B: Iterate controllers
loop each in-flight upload
C->>B: controller.abort()
end
C->>B: clear()
C->>S: removeProgress / cleanup state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 significantly enhances the file upload experience by introducing the capability to cancel ongoing 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 valuable feature: the ability to cancel file uploads in the chat box. The implementation correctly uses AbortController on the frontend and handles AbortError on the backend. The UI is also updated to reflect the cancellation action, which is great for user experience.
I've identified a high-severity bug related to state management where the count of uploading files is decremented twice upon cancellation, which could lead to UI inconsistencies. I've also provided feedback on the new test file for ChatBox, suggesting improvements to make the tests more robust by testing the component's behavior directly rather than just the underlying concepts.
| } | ||
| return prev.filter((f) => f.id !== id) | ||
| }) | ||
| setUploadingFilesCount((prev) => Math.max(0, prev - 1)) |
There was a problem hiding this comment.
There's a bug here where uploadingFilesCount is decremented twice for a cancelled upload. It's decremented here, and also in the finally block of the uploadFiles function (line 914), which will execute after the upload promise is rejected due to the abort signal.
This can lead to an incorrect count and potential UI bugs. The finally block is a more reliable place to handle the decrement as it covers success, failure, and abort cases. I suggest removing this line to fix the double-decrement issue.
| describe('ChatBox Upload Cancellation', () => { | ||
| let mockAuthFetch: any | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| mockAuthFetch = vi.fn() | ||
| vi.doMock('@/utils/authFetch', () => ({ | ||
| authFetch: mockAuthFetch | ||
| })) | ||
| }) | ||
|
|
||
| it('should handle upload cancellation correctly', async () => { | ||
| // Create a mock AbortController | ||
| const abortController = new AbortController() | ||
|
|
||
| // Mock a file upload that can be aborted | ||
| const uploadPromise = new Promise((resolve, reject) => { | ||
| abortController.signal.addEventListener('abort', () => { | ||
| const error = new Error('Upload aborted') | ||
| error.name = 'AbortError' | ||
| reject(error) | ||
| }) | ||
| }) | ||
|
|
||
| mockAuthFetch.mockReturnValue(uploadPromise) | ||
|
|
||
| // Test that aborting the upload triggers the correct behavior | ||
| act(() => { | ||
| abortController.abort() | ||
| }) | ||
|
|
||
| // Verify the upload was cancelled | ||
| await expect(uploadPromise).rejects.toThrow('Upload aborted') | ||
| }) | ||
|
|
||
| it('should clean up upload controllers on component unmount', () => { | ||
| // This test would require more complex setup with actual component rendering | ||
| // For now, we'll just verify the concept | ||
| const controllers = new Map<string, AbortController>() | ||
| const controller1 = new AbortController() | ||
| const controller2 = new AbortController() | ||
|
|
||
| controllers.set('file1', controller1) | ||
| controllers.set('file2', controller2) | ||
|
|
||
| // Simulate cleanup | ||
| controllers.forEach((controller) => { | ||
| controller.abort() | ||
| }) | ||
| controllers.clear() | ||
|
|
||
| expect(controllers.size).toBe(0) | ||
| expect(controller1.signal.aborted).toBe(true) | ||
| expect(controller2.signal.aborted).toBe(true) | ||
| }) | ||
|
|
||
| it('should handle AbortError gracefully in upload completion', async () => { | ||
| const abortError = new Error('Request aborted') | ||
| abortError.name = 'AbortError' | ||
|
|
||
| // Mock the upload function to handle AbortError | ||
| const mockUploadFile = async (file: File, signal: AbortSignal) => { | ||
| if (signal.aborted) { | ||
| throw abortError | ||
| } | ||
| return { fileId: 'test-id', fileName: file.name } | ||
| } | ||
|
|
||
| const testFile = new File(['test'], 'test.txt', { type: 'text/plain' }) | ||
| const abortController = new AbortController() | ||
|
|
||
| // Abort before upload | ||
| abortController.abort() | ||
|
|
||
| // Verify AbortError is thrown | ||
| await expect(mockUploadFile(testFile, abortController.signal)).rejects.toThrow('Request aborted') | ||
| }) | ||
| }) No newline at end of file |
There was a problem hiding this comment.
These tests verify the concepts of upload cancellation (like how AbortController works) but they don't actually test the ChatBox component's implementation. The component itself is imported but never rendered or tested.
To make these tests more effective and ensure the cancellation feature works as expected from a user's perspective, I recommend using @testing-library/react to render the ChatBox component, simulate file selection, trigger the cancellation by clicking the 'X' button, and then assert that the upload was aborted and the UI was updated correctly.
Here's an example of how such a test could be structured:
import { render, screen, fireEvent, waitFor } from '@testing-library/react';
it('should cancel an ongoing upload when the cancel button is clicked', async () => {
// 1. Mock authFetch to simulate a pending upload that can be aborted
const abortSpy = vi.fn();
mockAuthFetch.mockImplementation((url, options) => {
options.signal.addEventListener('abort', () => {
abortSpy();
const error = new Error('Upload aborted');
error.name = 'AbortError';
// This would normally be handled by fetch, but for test we reject a promise
});
// Return a promise that never resolves to simulate a long upload
return new Promise(() => {});
});
// 2. Render the ChatBox component with necessary props
render(<ChatBox {...mockProps} />);
// 3. Simulate file selection
const fileInput = screen.getByTitle(/Attach files/); // Use a more accessible selector
const file = new File(['content'], 'test.png', { type: 'image/png' });
await act(async () => {
fireEvent.change(fileInput, { target: { files: [file] } });
});
// 4. Find and click the cancel button
const cancelButton = await screen.findByTitle('Cancel upload');
fireEvent.click(cancelButton);
// 5. Assert that the abort function was called and the file is removed from UI
await waitFor(() => {
expect(abortSpy).toHaveBeenCalled();
expect(screen.queryByText('test.png')).not.toBeInTheDocument();
});
});![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
frontend/src/store/useUploadProgressStore.ts (1)
48-49: removeProgress addition looks goodAPI surface and behavior align with cancel/finish patterns.
You could dedupe finishUpload/cancelUpload/removeProgress via a small helper (set to null by id) to avoid repetition.
Also applies to: 145-149
server/api/files.ts (1)
401-407: Correct: return 499 on aborted uploadsGraceful AbortError handling with 499 is appropriate and matches client cancellation semantics.
- Broaden detection to include DOMException-based aborts:
const isAbort = (e: unknown) => typeof e === 'object' && e !== null && 'name' in (e as any) && (e as any).name === 'AbortError' if (isAbort(error)) { loggerWithChild({ email }).info("Attachment upload was cancelled by client") throw new HTTPException(499, { message: "Upload cancelled" }) }Also consider adding the same 499-on-abort handling in handleFileUpload for consistency across endpoints.
frontend/package.json (1)
67-69: Zustand addition aligns with new storeDependency looks correct for the introduced store patterns.
- Optional: align vitest packages to latest 3.2.x when convenient and verify @testing-library peer deps as per their latest guidance. Based on learnings
frontend/src/components/ChatBox.tsx (1)
847-851: Abortable upload flow: solid; a couple of minor polish ideasPer-file AbortController usage, signal wiring, UI affordances, and unmount cleanup look good.
- Use a tiny
isAbortErrorhelper for consistency:const isAbortError = (e: unknown) => typeof e === 'object' && e !== null && (e as any).name === 'AbortError'Then
if (isAbortError(error)) { /* ... */ }
- Consider marking the file as
uploading: falseimmediately upon abort before removal to avoid transient spinner flicker.Also applies to: 859-859, 884-895, 912-914, 2258-2263, 2911-2917, 2971-2977, 2989-2989
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (6)
frontend/package.json(1 hunks)frontend/src/components/ChatBox.tsx(11 hunks)frontend/src/components/__tests__/ChatBox.test.tsx(1 hunks)frontend/src/store/__tests__/useUploadProgressStore.test.ts(1 hunks)frontend/src/store/useUploadProgressStore.ts(2 hunks)server/api/files.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/store/__tests__/useUploadProgressStore.test.ts (1)
frontend/src/store/useUploadProgressStore.ts (1)
useUploadProgress(52-155)
🪛 GitHub Actions: TypeScript Build Check
frontend/src/store/__tests__/useUploadProgressStore.test.ts
[error] 1-1: bunx tsc -b: TS6133: 'vi' is declared but its value is never read.
frontend/src/components/__tests__/ChatBox.test.tsx
[error] 1-1: bunx tsc -b: TS6133: 'renderHook' is declared but its value is never read.
[error] 3-3: bunx tsc -b: TS6133: 'ChatBox' is declared but its value is never read.
| import { renderHook, act } from '@testing-library/react' | ||
| import { describe, it, expect, vi, beforeEach } from 'vitest' | ||
| import { ChatBox } from '../ChatBox' |
There was a problem hiding this comment.
Fix TS6133: remove unused imports
Build fails due to unused renderHook and ChatBox.
Apply:
-import { renderHook, act } from '@testing-library/react'
+import { act } from '@testing-library/react'
-import { ChatBox } from '../ChatBox'📝 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.
| import { renderHook, act } from '@testing-library/react' | |
| import { describe, it, expect, vi, beforeEach } from 'vitest' | |
| import { ChatBox } from '../ChatBox' | |
| import { act } from '@testing-library/react' | |
| import { describe, it, expect, vi, beforeEach } from 'vitest' |
🧰 Tools
🪛 GitHub Actions: TypeScript Build Check
[error] 1-1: bunx tsc -b: TS6133: 'renderHook' is declared but its value is never read.
[error] 3-3: bunx tsc -b: TS6133: 'ChatBox' is declared but its value is never read.
🤖 Prompt for AI Agents
In frontend/src/components/__tests__/ChatBox.test.tsx around lines 1-3, the test
file imports unused symbols causing TS6133; remove unused imports by deleting
renderHook from the first import and removing the ChatBox import (or import only
needed symbols) so the file only imports what it actually uses (e.g., keep act
and vitest imports) and update any references accordingly.
| // If the file is currently uploading, abort the upload | ||
| if (fileToRemove?.uploading) { | ||
| const controller = uploadControllers.current.get(id) | ||
| if (controller) { | ||
| controller.abort() | ||
| uploadControllers.current.delete(id) | ||
| } | ||
|
|
||
| // Remove from UI immediately for aborted uploads | ||
| setSelectedFiles((prev) => { | ||
| if (fileToRemove?.preview) { | ||
| URL.revokeObjectURL(fileToRemove.preview) | ||
| } | ||
| return prev.filter((f) => f.id !== id) | ||
| }) | ||
| setUploadingFilesCount((prev) => Math.max(0, prev - 1)) | ||
| return | ||
| } |
There was a problem hiding this comment.
Bug: double-decrement of uploadingFilesCount on abort
When canceling an in-flight upload, removeFile decrements the counter, and the upload promise finally decrements again. This can take the count negative and prematurely resolve send gating.
Fix by removing the early decrement and relying on the promise finally:
if (controller) {
controller.abort()
uploadControllers.current.delete(id)
}
// Remove from UI immediately for aborted uploads
setSelectedFiles((prev) => {
if (fileToRemove?.preview) {
URL.revokeObjectURL(fileToRemove.preview)
}
return prev.filter((f) => f.id !== id)
})
- setUploadingFilesCount((prev) => Math.max(0, prev - 1))
return📝 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.
| // If the file is currently uploading, abort the upload | |
| if (fileToRemove?.uploading) { | |
| const controller = uploadControllers.current.get(id) | |
| if (controller) { | |
| controller.abort() | |
| uploadControllers.current.delete(id) | |
| } | |
| // Remove from UI immediately for aborted uploads | |
| setSelectedFiles((prev) => { | |
| if (fileToRemove?.preview) { | |
| URL.revokeObjectURL(fileToRemove.preview) | |
| } | |
| return prev.filter((f) => f.id !== id) | |
| }) | |
| setUploadingFilesCount((prev) => Math.max(0, prev - 1)) | |
| return | |
| } | |
| // If the file is currently uploading, abort the upload | |
| if (fileToRemove?.uploading) { | |
| const controller = uploadControllers.current.get(id) | |
| if (controller) { | |
| controller.abort() | |
| uploadControllers.current.delete(id) | |
| } | |
| // Remove from UI immediately for aborted uploads | |
| setSelectedFiles((prev) => { | |
| if (fileToRemove?.preview) { | |
| URL.revokeObjectURL(fileToRemove.preview) | |
| } | |
| return prev.filter((f) => f.id !== id) | |
| }) | |
| return | |
| } |
🤖 Prompt for AI Agents
In frontend/src/components/ChatBox.tsx around lines 1005-1022, the abort branch
of removeFile calls setUploadingFilesCount decrement immediately and the upload
promise's finally also decrements, causing double-decrement and possible
negative counts; remove the early setUploadingFilesCount((prev) => Math.max(0,
prev - 1)) from this abort branch (still revoke preview URL, remove file from
UI, abort and delete the controller) and rely on the upload promise's finally to
decrement the uploadingFilesCount so the counter stays consistent.
| @@ -0,0 +1,118 @@ | |||
| import { describe, it, expect, beforeEach, vi } from 'vitest' | |||
There was a problem hiding this comment.
Fix TS6133: remove unused vi import
Build fails due to unused import.
Apply:
-import { describe, it, expect, beforeEach, vi } from 'vitest'
+import { describe, it, expect, beforeEach } from 'vitest'📝 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.
| import { describe, it, expect, beforeEach, vi } from 'vitest' | |
| import { describe, it, expect, beforeEach } from 'vitest' |
🧰 Tools
🪛 GitHub Actions: TypeScript Build Check
[error] 1-1: bunx tsc -b: TS6133: 'vi' is declared but its value is never read.
🤖 Prompt for AI Agents
In frontend/src/store/__tests__/useUploadProgressStore.test.ts around line 1,
the import list includes an unused `vi` from 'vitest' causing TS6133; remove
`vi` from the import statement (or replace the import with only the used
symbols) so the file imports only describe, it, expect, beforeEach to eliminate
the unused-import TypeScript error.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests