Bugfix cmgt1i14v0000kqlieevxyedw #1116
Conversation
…6ffde7456eb2d1e890b63f3e95
…b12d2543c5aac915373932f903
…c737c54ae0af14e3146dc64d20
…d60662425b87364f5d29025229
…d32eab4855a803deab1e3e568a
…f9bd7a452f939639d85104e1ff
…043632464fb7845561c24339f1
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 a bug by introducing comprehensive functionality for managing and cancelling file uploads. It allows users to abort ongoing uploads from the frontend, while simultaneously ensuring that any partially uploaded data or associated server-side resources are properly cleaned up. This enhancement improves the robustness of the file upload feature, prevents orphaned data, and provides better feedback to the user during the upload process. 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
|
WalkthroughThe changes introduce per-file upload abort control by creating individual AbortControllers for each file upload in ChatBox. A new server endpoint handles cleanup of aborted uploads by removing partial files and related Vespa documents. ChatBoxProps gains new optional properties for agent and RAG configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChatBox as ChatBox Component
participant Fetch as fetch()
participant Server as Server/Cleanup
User->>ChatBox: Removes file during upload
ChatBox->>ChatBox: Retrieve AbortController<br/>for file
ChatBox->>Fetch: abort() signal
Fetch-->>ChatBox: AbortError caught
ChatBox->>ChatBox: Update file state<br/>to cancelled
ChatBox->>Server: POST /files/upload-attachment/cleanup<br/>{fileId}
Server->>Server: Delete partial files<br/>& Vespa docs
Server-->>ChatBox: {success: true}
ChatBox->>User: Show cancellation toast
ChatBox->>ChatBox: Clean up controller ref
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The changes span multiple components with interconnected logic: per-file abort controller lifecycle management in frontend state, error-handling for AbortError detection, cleanup API integration with permission validation, and state consistency across the upload workflow. While patterns are consistent, careful review is needed for edge cases around concurrent uploads and state synchronization. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ 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 |
![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 the ability to cancel ongoing file uploads, which is a great enhancement for user experience. The implementation correctly uses AbortController on the frontend and adds a corresponding cleanup endpoint on the backend to prevent orphaned data. The backend cleanup logic is robust, checking for permissions and handling cases where partial data might not exist. My review includes a critical fix for a state management bug related to the upload counter and a suggestion to improve code maintainability. Overall, this is a well-implemented feature.
| // Check if document exists before trying to delete | ||
| const doc = await GetDocument(fileSchema, vespaId) | ||
| if (doc && doc.fields) { | ||
| const fields = doc.fields as any |
There was a problem hiding this comment.
Using as any here bypasses TypeScript's type checking, which can make the code less safe and harder to maintain. Since you are working with fileSchema from @xyne/vespa-ts/types, you could likely infer the type from the schema (e.g., type VespaFile = z.infer<typeof fileSchema>) and use that for doc.fields. This would provide better type safety and autocompletion.
![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)
846-859: Send a stable client fileId with the upload so server can clean up abortsAppend the client’s id to FormData. The server can then use it as the docId/dir and the cleanup endpoint will work.
Apply this diff:
const formData = new FormData() formData.append("attachment", selectedFile.file) + // Provide a stable client-generated id so the server can use it for docId/dir + formData.append("fileId", selectedFile.id) const response = await authFetch( "/api/v1/files/upload-attachment", { method: "POST", body: formData, signal: controller.signal, }, )
🧹 Nitpick comments (3)
server/server.ts (1)
898-898: Route wiring LGTM; consider schema validationPOST /files/upload-attachment/cleanup is correctly registered under authenticated routes. Add zValidator for a small JSON body (e.g.,
{ fileId: string }) to harden input.frontend/src/components/ChatBox.tsx (2)
904-907: Clamp the final decrement to prevent negative countsGuard against negatives in the finally block.
Apply this diff:
- setUploadingFilesCount((prev) => prev - 1) + setUploadingFilesCount((prev) => Math.max(0, prev - 1))
2273-2278: Optional: trigger server cleanup for in-flight uploads on unmountAfter aborting controllers, consider fire-and-forget cleanup calls for any uploading files to reduce residuals server-side (once IDs are aligned as suggested).
Example:
// After aborting: const inflightIds = Array.from(uploadControllersRef.current.keys()) inflightIds.forEach((id) => { authFetch("/api/v1/files/upload-attachment/cleanup", { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ fileId: id }), }).catch(() => void 0) }) uploadControllersRef.current.clear()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/ChatBox.tsx(8 hunks)server/api/files.ts(1 hunks)server/server.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/components/ChatBox.tsx (1)
frontend/src/utils/authFetch.ts (1)
authFetch(4-27)
server/server.ts (1)
server/api/files.ts (1)
handleAttachmentUploadCleanup(551-636)
server/api/files.ts (3)
server/search/utils.ts (1)
expandSheetIds(23-42)server/search/vespa.ts (2)
GetDocument(22-22)DeleteDocument(25-25)server/api/dataSource.ts (1)
DeleteImages(235-261)
🔇 Additional comments (1)
server/server.ts (1)
243-244: Import looks correct for new cleanup APINo issues with the import of handleAttachmentUploadCleanup.
| // Update the uploading count immediately | ||
| setUploadingFilesCount((prev) => Math.max(0, prev - 1)) | ||
|
|
||
| // Update file state to show it was cancelled |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid double-decrementing uploadingFilesCount on abort
You decrement here and again in the upload promise’s finally, which can drive the counter negative and break “wait for uploads” logic.
Apply this diff:
- // Update the uploading count immediately
- setUploadingFilesCount((prev) => Math.max(0, prev - 1))📝 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.
| // Update the uploading count immediately | |
| setUploadingFilesCount((prev) => Math.max(0, prev - 1)) | |
| // Update file state to show it was cancelled | |
| // Update file state to show it was cancelled |
🤖 Prompt for AI Agents
In frontend/src/components/ChatBox.tsx around lines 1004 to 1007, the code
immediately decrements uploadingFilesCount when an upload is aborted while the
upload promise’s finally block also decrements it, causing double-decrement and
possibly negative counts; remove the immediate setUploadingFilesCount((prev) =>
Math.max(0, prev - 1)) here and rely on the upload promise’s finally to
decrement once (or alternatively add a per-upload "didDecrement" flag so the
count is decremented exactly once), ensuring the counter cannot be decremented
twice.
| export const handleAttachmentUploadCleanup = async (c: Context) => { | ||
| const { sub } = c.get(JwtPayloadKey) | ||
| const email = sub | ||
|
|
||
| try { | ||
| const { fileId } = await c.req.json() | ||
| if (!fileId) { | ||
| throw new HTTPException(400, { message: "File ID is required for cleanup" }) | ||
| } | ||
|
|
||
| loggerWithChild({ email }).info( | ||
| `Cleaning up aborted upload for file ID: ${fileId}` | ||
| ) | ||
|
|
||
| // Validate fileId to prevent path traversal | ||
| if ( | ||
| fileId.includes("..") || | ||
| fileId.includes("/") || | ||
| fileId.includes("\\") | ||
| ) { | ||
| throw new HTTPException(400, { message: "Invalid file ID" }) | ||
| } | ||
|
|
||
| // Check if this is an image attachment that might have been partially written to disk | ||
| const imageBaseDir = path.resolve( | ||
| process.env.IMAGE_DIR || "downloads/xyne_images_db", | ||
| ) | ||
| const imageDir = path.join(imageBaseDir, fileId) | ||
|
|
||
| try { | ||
| // Try to remove the directory if it exists (for image uploads) | ||
| await fs.access(imageDir) | ||
| await fs.rm(imageDir, { recursive: true, force: true }) | ||
| loggerWithChild({ email }).info( | ||
| `Cleaned up image directory for aborted upload: ${imageDir}` | ||
| ) | ||
| } catch (error) { | ||
| // Directory might not exist, which is fine for aborted uploads | ||
| loggerWithChild({ email }).debug( | ||
| `No image directory to cleanup for file ID: ${fileId}` | ||
| ) | ||
| } | ||
|
|
||
| // Try to delete from Vespa if the document was partially created | ||
| try { | ||
| const vespaIds = expandSheetIds(fileId) | ||
| for (const vespaId of vespaIds) { | ||
| // Check if document exists before trying to delete | ||
| const doc = await GetDocument(fileSchema, vespaId) | ||
| if (doc && doc.fields) { | ||
| const fields = doc.fields as any | ||
| const permissions = Array.isArray(fields.permissions) ? fields.permissions as string[] : [] | ||
|
|
||
| // Only delete if user has permissions | ||
| if (permissions.includes(email)) { | ||
| await DeleteDocument(vespaId, fileSchema) | ||
| // Also try to delete any associated images | ||
| await DeleteImages(vespaId) | ||
| loggerWithChild({ email }).info( | ||
| `Cleaned up Vespa document for aborted upload: ${vespaId}` | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } catch (error) { | ||
| // Document might not exist in Vespa, which is fine for aborted uploads | ||
| loggerWithChild({ email }).debug( | ||
| `No Vespa document to cleanup for file ID: ${fileId}` | ||
| ) | ||
| } | ||
|
|
||
| return c.json({ | ||
| success: true, | ||
| message: "Cleanup completed for aborted upload" | ||
| }) | ||
| } catch (error) { | ||
| if (error instanceof HTTPException) { | ||
| throw error | ||
| } | ||
| loggerWithChild({ email }).error( | ||
| error, | ||
| "Error during attachment upload cleanup" | ||
| ) | ||
| throw new HTTPException(500, { message: "Internal server error during cleanup" }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Cleanup can’t find aborted uploads: wrong fileId is sent from the client
The cleanup endpoint expects the fileId used on server (attf_${uuid}), but the client calls it with selectedFile.id (client-only). On aborted uploads, the server never returned its fileId, so this endpoint will often no-op and leave partial files/docs behind.
Fix by aligning IDs:
- Frontend: include a stable client fileId in the upload form.
- Server: accept and use that client-provided fileId for the upload (docId, image dir). Then the cleanup can actually remove partial artifacts using the same id.
Additionally:
- Consider zod-validating
{ fileId: string }. - For parity with deletion code, optionally delete from KbItemsSchema when id starts with "att_".
To confirm the mismatch, run:
🏁 Script executed:
#!/bin/bash
# 1) Locate generateFileId and see its format
rg -nP --type=ts '(export\s+)?function\s+generateFileId|\bgenerateFileId\s*\(' -C3
# 2) Check if frontend already appends any client id to the upload form
rg -nP --type=tsx -C3 'formData\.append\([^)]*attachment|upload-attachment'
# 3) Confirm server constructs its own attf_ fileId
rg -nP --type=ts -C3 'upload-attachment|attf_' serverLength of output: 3353
🏁 Script executed:
#!/bin/bash
# 1. Show handleAttachmentUpload definition and context
rg -nP 'export const handleAttachmentUpload' -C50 server/api/files.ts
# 2. Show JSON response calls within that handler
rg -nP 'c\.json\(' -C5 server/api/files.ts
# 3. Search for client-side fetch to /files/upload-attachment
rg -nP 'fetch\([^)]*files/upload-attachment' -C10 frontend/srcLength of output: 9655
🏁 Script executed:
#!/bin/bash
# 1. Find client call to cleanup endpoint
rg -n "upload-attachment/cleanup" -C5 frontend/src
# 2. Search for any “cleanup” references in attachment components
rg -n "cleanup" -C5 frontend/src
# 3. Locate generateFileId implementation and its prefix
rg -nP "function\s+generateFileId" -C5 frontend/srcLength of output: 21777
Align cleanup ID with upload
- The client calls cleanup with its own
generateFileId()(ChatBox.tsx:1018), but the server usesattf_<uuid>and never returns it—aborted uploads aren’t cleaned up. - Persist the server’s fileId in the upload response (or switch to using the client-provided ID for both storage and cleanup).
- Add Zod validation for the cleanup body (
{ fileId: string }).
1 participant
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes