fix: XYNE-157 attachement removal while it is in uploading state #1096
Conversation
Ravishekhar7870
requested review from
devesh-juspay,
junaid-shirur,
kalpadhwaryu,
shivamashtikar and
zereraz
as code owners
WalkthroughUpdates ChatBox upload/removal logic to clamp uploading file count at zero, conditionally decrement on removal only when applicable, and enable remove-file buttons. Public API changes: adds optional agentIdFromChatData and requires a user (PublicUser) prop. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CB as ChatBox
participant FS as FileState
U->>CB: Remove file
CB->>FS: Check file.uploading?
alt uploading === true
note over CB,FS: Decrement uploadingFilesCount with clamp
CB->>CB: uploadingFilesCount = max(prev - 1, 0)
else uploading === false
note over CB: No decrement
end
CB->>FS: Remove file from list
CB-->>U: UI updates (remove button enabled)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 implements a fix for issue XYNE-157, which previously prevented users from removing attachments while they were still uploading. The changes ensure that the count of uploading files is correctly maintained and never goes negative, and crucially, it enables the removal of files from the UI regardless of their current upload status. This improves the overall robustness and user experience of the attachment handling feature within the chat interface. 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 correctly enables the removal of attachments while they are in an uploading state by un-disabling the remove button. However, the implementation for updating the uploadingFilesCount introduces a race condition, which could lead to an incorrect count and subsequent issues with message sending logic. I've provided a detailed comment with a suggested fix to resolve this race condition.
| } finally { | ||
| setUploadingFilesCount((prev) => prev - 1) | ||
| setUploadingFilesCount((prev) => Math.max(prev - 1, 0)) | ||
| } |
There was a problem hiding this comment.
While adding Math.max is a good defensive measure, there's a more critical race condition here. The new logic in removeFile (around line 1005) decrements uploadingFilesCount when an uploading file is removed. However, this finally block will also run when that same file's upload promise completes, causing the count to be decremented twice.
To fix this, we should only decrement the count here if the file has not been removed by the user in the meantime. We can use selectedFilesRef to check the latest state without adding a dependency to uploadFiles.
} finally {
// Only decrement the count if the file hasn't been removed by the user.
if (selectedFilesRef.current.some(f => f.id === selectedFile.id)) {
setUploadingFilesCount((prev) => Math.max(prev - 1, 0))
}
}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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)
2058-2065: Remove dead code: Synchronous check can't respond to async state changes.The code inside the promise constructor (lines 2061-2064) attempts to check
uploadingFilesCountsynchronously, but this can't reactively respond to state changes:if (uploadingFilesCount > 0) { await new Promise<void>((resolve) => { uploadCompleteResolver.current = resolve if(uploadingFilesCount==0){ // This check is effectively dead code resolve(); uploadCompleteResolver.current=null } }) }Why this doesn't work:
- The promise constructor executes synchronously when the promise is created
- At that moment,
uploadingFilesCount > 0(verified by the outerifon line 2058)- The inner check
uploadingFilesCount==0will always be false- Even if it were true, this is a one-time synchronous check - it can't respond to
uploadingFilesCountchanging laterThe
useEffecton lines 906-911 is the correct mechanism for resolving the promise when the count reaches zero.Apply this diff to remove the dead code:
if (uploadingFilesCount > 0) { await new Promise<void>((resolve) => { uploadCompleteResolver.current = resolve - if(uploadingFilesCount==0){ - resolve(); - uploadCompleteResolver.current=null - } }) - }
🧹 Nitpick comments (1)
frontend/src/components/ChatBox.tsx (1)
1004-1006: Code style: Formatting inconsistencies.This code segment has minor formatting issues:
- Missing space after
ifkeyword (should beif (fileToRemove))- Inconsistent indentation (single space instead of standard indentation)
- Redundant optional chaining:
fileToRemove?.uploading- the?.is unnecessary since we already verifiedfileToRemoveexists in theifconditionSuggested formatting (note: this code should be removed per the critical issue comment above):
- if(fileToRemove){ - setUploadingFilesCount((prev) => Math.max(prev - (fileToRemove?.uploading ? 1 : 0), 0)) - } + if (fileToRemove) { + setUploadingFilesCount((prev) => Math.max(prev - (fileToRemove.uploading ? 1 : 0), 0)) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ChatBox.tsx(5 hunks)
🔇 Additional comments (1)
frontend/src/components/ChatBox.tsx (1)
868-868: Good change: Enables file removal during upload.Commenting out the
disabledattribute correctly allows users to remove files while they're uploading, which aligns with the PR objective (XYNE-157).However, ensure the critical double-decrement bug flagged in the earlier comment is fixed before merging, as it will cause
uploadingFilesCountto become incorrect when users remove uploading files.Also applies to: 924-924
| return null | ||
| } finally { | ||
| setUploadingFilesCount((prev) => prev - 1) | ||
| setUploadingFilesCount((prev) => Math.max(prev - 1, 0)) |
There was a problem hiding this comment.
Critical: Double-decrement bug when removing files during upload.
When a file is removed while uploading, uploadingFilesCount is decremented twice:
- In
removeFile(line 1005): decrements by 1 iffileToRemove.uploadingis true - Here in the
finallyblock: decrements by 1 when the upload promise completes
This causes the count to drop by 2 for a single file, leading to incorrect state and potentially allowing premature message sending.
Solution: Only decrement in the finally block, not in removeFile. The upload promise should be the single source of truth for decrementing the counter.
Apply this diff to fix the issue:
const removeFile = useCallback(async (id: string) => {
const fileToRemove = selectedFiles.find((f) => f.id === 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
- if(fileToRemove){
- setUploadingFilesCount((prev) => Math.max(prev - (fileToRemove?.uploading ? 1 : 0), 0))
- }
setSelectedFiles((prev) => {
if (fileToRemove?.preview) {
URL.revokeObjectURL(fileToRemove.preview)
}
return prev.filter((f) => f.id !== id)
})
}, [selectedFiles])Note: The Math.max(..., 0) clamping here masks the double-decrement issue but doesn't fix the root cause. The upload promise's finally block is sufficient to manage the counter correctly.
📝 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.
| setUploadingFilesCount((prev) => Math.max(prev - 1, 0)) | |
| const removeFile = useCallback(async (id: string) => { | |
| const fileToRemove = selectedFiles.find((f) => f.id === 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) | |
| }) | |
| }, [selectedFiles]) |
🤖 Prompt for AI Agents
In frontend/src/components/ChatBox.tsx around line 892 (and related code at
removeFile on line 1005), the uploadingFilesCount is being decremented twice for
a single file: once in removeFile when fileToRemove.uploading is true and again
in the upload promise finally block; remove the decrement in removeFile so the
upload promise's finally is the single source of truth for decrementing
uploadingFilesCount, keep the Math.max clamp in the finally block as a safety
measure, and ensure removeFile still clears the file/abort logic without
touching uploadingFilesCount.
1 participant
Testing
tested locally
Summary by CodeRabbit
Bug Fixes
UI
Reliability