fix: XYNE-157 fixed attachement cancel while uploading #1119
Conversation
Ravishekhar7870
requested review from
devesh-juspay,
junaid-shirur,
kalpadhwaryu,
shivamashtikar and
zereraz
as code owners
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 an issue where users were unable to cancel attachment uploads once they had started in the chat interface. By introducing a mechanism to abort ongoing network requests, it significantly enhances the user experience, providing greater control over file uploads and preventing unnecessary resource consumption for unwanted transfers. 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
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds per-file cancellable uploads and abort-aware send/remove flows to ChatBox: each selected file gets an AbortController; uploads receive the signal; removing a file aborts its upload, optionally triggers server-side deletion, revokes preview URLs, and updates upload/error state with guarded counters. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChatBox as ChatBox UI
participant Upload as Upload Service
participant Server as Backend (delete)
participant State as File State Map
User->>ChatBox: Select file(s)
ChatBox->>State: create SelectedFile + AbortController (store in map)
User->>ChatBox: Send / upload
loop per-file
ChatBox->>Upload: POST file (with abort signal)
end
opt Upload success
Upload-->>ChatBox: 200 OK
ChatBox->>State: mark uploaded, clear abort ref, revoke preview
ChatBox->>ChatBox: decrement uploadingFilesCount (clamp ≥0)
end
alt Upload non-abort error
Upload-->>ChatBox: Error
ChatBox->>State: set uploadError, clear abort ref
ChatBox->>ChatBox: decrement uploadingFilesCount (clamp ≥0)
end
User->>ChatBox: Remove file (while uploading)
ChatBox->>State: fetch & abort controller.abort(), remove state
ChatBox->>Server: DELETE (if server metadata) -- note: errors logged but UI removed
Upload-->>ChatBox: AbortError
ChatBox->>ChatBox: treat as no-op, revoke preview, clamp counters
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)frontend/src/components/ChatBox.tsx (5)
🔇 Additional comments (7)
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 |
![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 successfully implements the functionality to cancel file uploads in the chat component. The use of AbortController is appropriate for this purpose. My review includes one high-severity suggestion to refactor the management of AbortController instances. Storing them in a useRef instead of React state would align better with React best practices by keeping the state serializable and separating concerns, leading to a more robust and maintainable implementation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
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)
2070-2078: Fix the upload completion logic.The inner condition at line 2073 is dead code and will never execute because we just verified
uploadingFilesCount > 0at line 2070. The synchronous Promise constructor cannot observe state changes between these checks.Additionally, line 2073 uses loose equality (
==) instead of strict equality (===).The current implementation has a theoretical race condition: if all uploads complete between checking
uploadingFilesCount > 0(line 2070) and setting the resolver (line 2072), the promise would never resolve. While unlikely due to the asynchronous nature of uploads, this can be fixed by removing the dead code and relying on theuseEffectat lines 918-923:if (uploadingFilesCount > 0) { await new Promise<void>((resolve) => { uploadCompleteResolver.current = resolve - if(uploadingFilesCount==0){ - resolve(); - uploadCompleteResolver.current=null - } }) - }The
useEffectat lines 918-923 will properly call the resolver whenuploadingFilesCountreaches 0, handling the race condition correctly.
🧹 Nitpick comments (1)
frontend/src/components/ChatBox.tsx (1)
903-903: Consider logging when clamping occurs.Using
Math.max(prev - 1, 0)is good defensive programming to prevent negative counts. However, if the count ever attempts to go negative, it indicates a logic error in the increment/decrement flow.Consider adding logging to detect if clamping ever occurs:
-setUploadingFilesCount(prev => Math.max(prev - 1, 0)); +setUploadingFilesCount(prev => { + if (prev <= 0) { + console.warn('Upload count attempted to go negative, clamping to 0') + } + return Math.max(prev - 1, 0) +});This helps identify potential bugs without changing behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ChatBox.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/ChatBox.tsx (1)
frontend/src/utils/authFetch.ts (1)
authFetch(4-27)
🔇 Additional comments (6)
frontend/src/components/ChatBox.tsx (6)
89-89: LGTM! Clean interface extension.Adding the optional
abortControllerproperty is the right approach for tracking per-file upload cancellation. The optional typing correctly reflects that controllers are only present during active uploads.
835-847: LGTM! Proper abort controller lifecycle management.The implementation correctly:
- Creates a dedicated
AbortControllerfor each file- Associates controllers with files via a Map for efficient lookup
- Updates file state with controllers before starting uploads
- Retrieves from the Map (line 850) to avoid relying on potentially stale state
855-862: LGTM! Correct signal propagation.Passing
abortController?.signalto the fetch request properly enables cancellation. The optional chaining provides defensive safety even though the controller should always exist at this point.
885-887: LGTM! Proper abort handling.Catching
AbortErrorand returningnullis the correct approach. The null values are filtered out later (lines 909-912), and thefinallyblock ensuresuploadingFilesCountis always decremented.
992-1025: LGTM! Clean abort-on-remove implementation.The removal flow is well-structured:
- Aborts the in-flight upload if the file is currently uploading (lines 994-996)
- Attempts server deletion if the file was already uploaded (lines 999-1016)
- Removes the file from the UI immediately (lines 1019-1024)
The abort happens before the UI update, which is correct. The
uploadingFilesCountis decremented when the aborted promise resolves in thefinallyblock.
873-879: LGTM! Proper controller cleanup.The implementation correctly clears the
abortControlleron both success (line 876) and error (line 893) paths by setting it toundefined. This ensures controllers are explicitly released after the upload completes, aiding garbage collection.On the abort path (line 886), the controller is implicitly cleaned up when the file is removed from
selectedFiles, which is acceptable.Also applies to: 890-896
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/src/components/ChatBox.tsx (1)
918-919: Delete AbortController after each upload completes (prevent leaks).Controllers remain in the map after success/error/abort. Delete them in
finally.- } finally { - setUploadingFilesCount((prev) => Math.max(prev - 1, 0)) - } + } finally { + // Cleanup controller for this file to prevent leaks + fileAbortControllers.current.delete(selectedFile.id) + setUploadingFilesCount((prev) => Math.max(prev - 1, 0)) + }
🧹 Nitpick comments (2)
frontend/src/components/ChatBox.tsx (2)
1006-1043: Abort cleanup + remove from UI immediately; server delete in background.
- Delete the controller from the ref after aborting.
- Remove the file from UI right away (better UX), then attempt server deletion. Currently UI removal waits on the server.
const removeFile = useCallback( async (id: string) => { const fileToRemove = selectedFiles.find((f) => f.id === id) const abortController = fileAbortControllers.current.get(id); if (fileToRemove?.uploading && abortController) { - abortController.abort(); + abortController.abort(); + fileAbortControllers.current.delete(id) } - // If the file has metadata with fileId (meaning it's already uploaded), delete it from the server - if (fileToRemove?.metadata?.fileId) { - try { - const response = await api.files.delete.$post({ - json: { - attachment: fileToRemove.metadata, - }, - }) - - if (!response.ok) { - const errorText = await response.text() - console.error(`Failed to delete attachment: ${errorText}`) - // Still remove from UI even if server deletion fails - } - } catch (error) { - console.error("Error deleting attachment:", error) - // Still remove from UI even if server deletion fails - } - } - - // Remove from UI - setSelectedFiles((prev) => { - if (fileToRemove?.preview) { - URL.revokeObjectURL(fileToRemove.preview) - } - return prev.filter((f) => f.id !== id) - }) + // Remove from UI immediately + setSelectedFiles((prev) => { + if (fileToRemove?.preview) { + URL.revokeObjectURL(fileToRemove.preview) + } + return prev.filter((f) => f.id !== id) + }) + + // Best-effort server deletion (non-blocking for UI) + if (fileToRemove?.metadata?.fileId) { + try { + const response = await api.files.delete.$post({ + json: { attachment: fileToRemove.metadata }, + }) + if (!response.ok) { + const errorText = await response.text() + console.error(`Failed to delete attachment: ${errorText}`) + } + } catch (error) { + console.error("Error deleting attachment:", error) + } + } }, [selectedFiles], )
427-427: Good move: controllers in a ref, not React state. Add unmount cleanup.Storing
AbortControllers in a ref is the right call. Add an unmount cleanup to abort any stragglers and clear the map.Add this effect near other cleanup effects:
useEffect(() => { return () => { fileAbortControllers.current.forEach((c) => c.abort()) fileAbortControllers.current.clear() } }, [])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ChatBox.tsx(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/ChatBox.tsx (5)
frontend/src/api.ts (1)
api(5-5)frontend/src/types.ts (1)
ToolsListItem(59-62)server/shared/types.ts (1)
AttachmentMetadata(709-709)frontend/src/utils/fileUtils.ts (1)
cleanupPreviewUrls(18-24)frontend/src/components/ui/smart-tooltip.tsx (1)
SmartTooltip(11-99)
🔇 Additional comments (1)
frontend/src/components/ChatBox.tsx (1)
865-866: The implementation correctly forwards thesignalthroughauthFetchtofetch.The
authFetchfunction spreads theinitparameter (which includes thesignalproperty) directly into thefetchcall via{ ...init, credentials: "include" }(line 8 offrontend/src/utils/authFetch.ts). AbortController abort signals will work correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/src/components/ChatBox.tsx (2)
895-909: AbortError branch uses undefined variable and marks abort as error.
- errorMessage is referenced before it’s defined → TS compile/runtime error.
- Aborts should clear uploading and not set an error.
Apply this fix:
- if (error instanceof Error && error.name === "AbortError") { - setSelectedFiles((prev) => - prev.map((f) => - f.id === selectedFile.id - ? { - ...f, - uploading: false, - uploadError: errorMessage, - - } - : f, - ), - ) - return null - } + if (error instanceof Error && error.name === "AbortError") { + // Clear spinner and do not treat abort as an error + setSelectedFiles((prev) => + prev.map((f) => + f.id === selectedFile.id + ? { ...f, uploading: false, uploadError: undefined } + : f, + ), + ) + return null + }
930-931: Leak: AbortControllers are never removed from the ref map.Delete the controller for this file in the finally block to avoid stale entries.
} finally { - setUploadingFilesCount((prev) => Math.max(prev - 1, 0)) + // Clean up per-file controller + fileAbortControllers.current.delete(selectedFile.id) + setUploadingFilesCount((prev) => Math.max(prev - 1, 0)) }
🧹 Nitpick comments (1)
frontend/src/components/ChatBox.tsx (1)
2178-2190: Optional cleanup: clear any leftover controllers after send.As a defensive measure after clearing selected files, also clear the controllers map.
cleanupPreviewUrls(previewUrls) setSelectedFiles([]) + // Defensive: ensure no dangling controllers remain + fileAbortControllers.current.clear()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ChatBox.tsx(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/ChatBox.tsx (5)
frontend/src/api.ts (1)
api(5-5)frontend/src/types.ts (1)
ToolsListItem(59-62)server/shared/types.ts (1)
AttachmentMetadata(709-709)frontend/src/utils/fileUtils.ts (1)
cleanupPreviewUrls(18-24)frontend/src/components/ui/smart-tooltip.tsx (1)
SmartTooltip(11-99)
🔇 Additional comments (1)
frontend/src/components/ChatBox.tsx (1)
426-426: Good fix: move AbortControllers to a ref map (not React state).This avoids non-serializable state and re-render churn. Clean approach.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/components/ChatBox.tsx (1)
2099-2107: Redundant check inside upload wait logic.Line 2102 checks
if (uploadingFilesCount == 0)inside a block that only executes whenuploadingFilesCount > 0. This condition can never be true at this point in the code flow.The actual Promise resolution happens via the
useEffectat lines 941-946 whenuploadingFilesCountreaches 0. This check appears to be a defensive fallback but is logically unreachable.Consider removing the redundant check for clarity:
if (uploadingFilesCount > 0) { await new Promise<void>((resolve) => { uploadCompleteResolver.current = resolve - if (uploadingFilesCount == 0) { - resolve() - uploadCompleteResolver.current = null - } }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ChatBox.tsx(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/ChatBox.tsx (5)
frontend/src/api.ts (1)
api(5-5)frontend/src/types.ts (1)
ToolsListItem(59-62)server/shared/types.ts (1)
AttachmentMetadata(709-709)frontend/src/utils/fileUtils.ts (1)
cleanupPreviewUrls(18-24)frontend/src/components/ui/smart-tooltip.tsx (1)
SmartTooltip(11-99)
🔇 Additional comments (6)
frontend/src/components/ChatBox.tsx (6)
426-426: LGTM! Proper use of useRef for AbortController storage.Using
useRefto store the AbortController map outside of React state is the correct approach, avoiding issues with state persistence and serialization.
648-656: LGTM! Correct disabled state logic.The conditions properly prevent sending messages while files are uploading or the document is not ready, providing a good user experience.
927-927: LGTM! Proper count decrement with safeguard.Using
Math.max(prev - 1, 0)prevents the counter from going negative, ensuring the UI state remains consistent.
1015-1053: LGTM! Comprehensive cleanup on file removal.The implementation correctly:
- Aborts ongoing uploads and removes the controller from the map (addressing previous review feedback)
- Attempts server-side deletion with graceful error handling
- Cleans up preview object URLs to free resources
- Removes files from UI even if server deletion fails
2182-2187: LGTM! Proper resource cleanup.Preview URLs are correctly revoked before clearing files, preventing memory leaks from blob URLs.
3984-4010: LGTM! Clean conditional tooltip implementation.The send button is wrapped with a tooltip only when disabled, providing helpful feedback to users about why they can't send yet (uploading files, processing document, etc.).
## [3.18.4](v3.18.3...v3.18.4) (2025-10-17) ### Bug Fixes * XYNE-157 fixed attachement cancel while uploading ([#1119](#1119)) ([d0ff988](d0ff988))
2 participants
Description
fixed attachment cancel while uploading in the chat
Testing
locally tested
Summary by CodeRabbit
New Features
Bug Fixes