Skip to content

fix: XYNE-157 attachement removal while it is in uploading state #1096

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

fix: XYNE-157 attachement removal while it is in uploading state #1096

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions frontend/src/components/ChatBox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -889,7 +889,7 @@ export const ChatBox = React.forwardRef<ChatBoxRef, ChatBoxProps>(
})
return null
} finally {
setUploadingFilesCount((prev) => prev - 1)
setUploadingFilesCount((prev) => Math.max(prev - 1, 0))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Double-decrement bug when removing files during upload.

When a file is removed while uploading, uploadingFilesCount is decremented twice:

  1. In removeFile (line 1005): decrements by 1 if fileToRemove.uploading is true
  2. Here in the finally block: 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.

Suggested change
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.

}
Comment on lines 891 to 893
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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))
            }
          }

})

Expand Down Expand Up @@ -1001,6 +1001,9 @@ export const ChatBox = React.forwardRef<ChatBoxRef, ChatBoxProps>(
}

// Remove from UI
if(fileToRemove){
setUploadingFilesCount((prev) => Math.max(prev - (fileToRemove?.uploading ? 1 : 0), 0))
}
setSelectedFiles((prev) => {
if (fileToRemove?.preview) {
URL.revokeObjectURL(fileToRemove.preview)
Expand Down Expand Up @@ -2256,7 +2259,7 @@ export const ChatBox = React.forwardRef<ChatBoxRef, ChatBoxProps>(
handleSendMessage,
],
)

return (
<div className="relative flex flex-col w-full max-w-3xl pb-5">
{persistedAgentId && displayAgentName && (
Expand Down Expand Up @@ -2862,7 +2865,7 @@ export const ChatBox = React.forwardRef<ChatBoxRef, ChatBoxProps>(
<button
onClick={() => removeFile(selectedFile.id)}
className="absolute top-1 right-1 bg-black bg-opacity-60 text-white rounded-full p-1 hover:bg-opacity-80 transition-opacity"
disabled={selectedFile.uploading}
// disabled={selectedFile.uploading}
>
<X size={10} />
</button>
Expand Down Expand Up @@ -2918,7 +2921,7 @@ export const ChatBox = React.forwardRef<ChatBoxRef, ChatBoxProps>(
<button
onClick={() => removeFile(selectedFile.id)}
className="text-gray-400 hover:text-red-500 transition-colors p-1"
disabled={selectedFile.uploading}
// disabled={selectedFile.uploading}
>
<X size={12} />
</button>
Expand Down
Loading