feat(knowledgemanagment): make global state for uploading in kb #986
Conversation
make global state for uploading files in kb and refactor ui for kb upload
rahul1841
requested review from
devesh-juspay,
junaid-shirur,
kalpadhwaryu,
shivamashtikar and
zereraz
as code owners
WalkthroughAdds a centralized UploadProgressContext with provider and widget, refactors route-level upload flows to use the context, revamps the file upload UI (queue, file-type icons, folder support, preview), makes skeleton headers optional, sets toast duration to 10s, and wires the provider/widget into the app main. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant ClFileUpload
participant KnowledgeRoute as KnowledgeRoute/UI
participant UploadCtx as UploadProgressProvider
participant Server
participant Widget as UploadProgressWidget
User->>ClFileUpload: select files / add folder / drag-drop
ClFileUpload->>KnowledgeRoute: submit files + metadata
KnowledgeRoute->>UploadCtx: startUpload(files, meta) → { uploadId, AbortController }
UploadCtx-->>Widget: publish currentUpload (isUploading=true)
loop per batch (<= configured limits)
KnowledgeRoute->>Server: uploadFileBatch(batchFiles, collectionId, parentId, abortSignal)
Server-->>KnowledgeRoute: batch response (per-file results)
KnowledgeRoute->>UploadCtx: updateFileStatus(fileId, status[, error])
KnowledgeRoute->>UploadCtx: updateProgress(batchIndex, fileIndex, totals)
UploadCtx-->>Widget: progress & status updates
end
alt user cancels
Widget->>UploadCtx: cancelUpload(uploadId)
UploadCtx->>UploadCtx: abort controller -> reset currentUpload
else completed
KnowledgeRoute->>UploadCtx: finishUpload(uploadId)
end
UploadCtx-->>Widget: currentUpload=null (widget hides)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)frontend/src/components/ClFileUpload.tsx (2)
frontend/src/lib/common.tsx (2)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 @rahul1841, 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 user experience for file and folder uploads within the Knowledge Base feature. By centralizing upload state management, users now benefit from a consistent, real-time progress display through a new dedicated widget. The file upload interface has also been modernized with a clearer layout, dynamic file type icons, and improved feedback mechanisms, making the upload process more intuitive and informative. 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 major refactoring of the file upload functionality in the knowledge management section. It replaces a localStorage-based progress tracking system with a much more robust and user-friendly global state managed by a React Context. This enables a new upload progress widget that provides detailed, real-time feedback. The file upload UI has also been significantly improved. The changes are well-structured and greatly enhance the user experience. I've included a few suggestions to improve responsiveness, maintainability, and performance.
![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
🧹 Nitpick comments (6)
frontend/src/components/ui/toast.tsx (1)
63-71: Set a default duration without shadowing consumer overrides.Right now
duration={10000}is overridden by{...props}. Make the 10s value a default while still allowing callers to override it.Apply this diff:
->({ className, variant, children, ...props }, ref) => { +>({ className, variant, children, duration = 10000, ...props }, ref) => { @@ - className={cn(toastVariants({ variant }), className)} - duration={10000} + className={cn(toastVariants({ variant }), className)} + duration={duration} {...props}frontend/src/components/ClFileUpload.tsx (2)
16-35: Deduplicate file type detection by reusing the shared utility.
getFileTypeis already implemented infrontend/src/utils/fileUtils.ts. Duplicating this logic here risks drift.Apply this diff to remove the local helper:
-// Helper function to determine FileType from a file -const getFileType = (file: File): FileType => { - // First check by MIME type - for (const [fileType, mimeTypes] of Object.entries(MIME_TYPE_MAPPINGS)) { - if ((mimeTypes as readonly string[]).includes(file.type)) { - return fileType as FileType - } - } - - // Fallback to extension checking - const fileName = file.name.toLowerCase() - for (const [fileType, extensions] of Object.entries(EXTENSION_MAPPINGS)) { - if ((extensions as readonly string[]).some(ext => fileName.endsWith(ext))) { - return fileType as FileType - } - } - - // Default fallback - return FileType.FILE -}And add this import near the top of the file:
import { getFileType } from "@/utils/fileUtils"Based on relevant code snippets.
224-231: Add a non-WebKit fallback for folder drops.Some browsers don’t support
webkitGetAsEntry. IffilePromisesis empty, fall back todataTransfer.filesso plain file drops still work.Apply this diff:
- Promise.all(filePromises) - .then((filesArrays) => { - onFilesSelect(filesArrays.flat()) - }) - .catch((err) => { - console.error("Error traversing file tree:", err) - }) + if (filePromises.length > 0) { + Promise.all(filePromises) + .then((filesArrays) => { + onFilesSelect(filesArrays.flat()) + }) + .catch((err) => { + console.error("Error traversing file tree:", err) + }) + } else { + // Fallback: treat as regular files drop + const droppedFiles = Array.from(e.dataTransfer.files).filter( + (file) => !file.name.startsWith("."), + ) + if (droppedFiles.length) onFilesSelect(droppedFiles) + }frontend/src/components/UploadProgressWidget.tsx (2)
19-21: Guard and clamp progress percentage.Avoid NaN/Infinity and overshoot; clamp to [0, 100] with a zero-total guard.
Apply this diff:
- const progressPercentage = Math.round((batchProgress.current / batchProgress.total) * 100) + const progressPercentage = batchProgress.total > 0 + ? Math.min(100, Math.max(0, Math.round((batchProgress.current / batchProgress.total) * 100))) + : 0
70-75: Optional: Add accessible progress semantics.Expose progress to assistive tech via aria attributes.
Example:
<div className="w-full bg-gray-200 dark:bg-gray-700 rounded-full h-2 mb-3" role="progressbar" aria-valuemin={0} aria-valuemax={100} aria-valuenow={progressPercentage} aria-label="Upload progress" > <div className="bg-gray-500 h-2 rounded-full transition-all duration-300 ease-out" style={{ width: `${progressPercentage}%` }} /> </div>frontend/src/routes/_authenticated/knowledgeManagement.tsx (1)
734-760: Duplicate pattern - consider extracting shared logic.The file status update logic is duplicated between new collection uploads (lines 553-576) and existing collection uploads (lines 734-760). Consider extracting this into a shared helper function to reduce code duplication.
+const updateBatchFileStatuses = (uploadId: string, batchFiles: File[], uploadResult: any, updateFileStatus: Function) => { + if (uploadResult.results) { + uploadResult.results.forEach((result: any, index: number) => { + const file = batchFiles[index] + if (result.success) { + updateFileStatus(uploadId, file.name, 'uploaded') + } else { + updateFileStatus(uploadId, file.name, 'failed', result.error || 'Upload failed') + } + }) + } else { + // Fallback: mark all as uploaded if no individual results available + batchFiles.forEach(file => { + updateFileStatus(uploadId, file.name, 'uploaded') + }) + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
frontend/src/assets/collectionIcons/document.svgis excluded by!**/*.svgfrontend/src/assets/collectionIcons/image.svgis excluded by!**/*.svgfrontend/src/assets/collectionIcons/pdf.svgis excluded by!**/*.svgfrontend/src/assets/collectionIcons/ppt.svgis excluded by!**/*.svgfrontend/src/assets/collectionIcons/spreadsheet.svgis excluded by!**/*.svgfrontend/src/assets/collectionIcons/text.svgis excluded by!**/*.svg
📒 Files selected for processing (7)
frontend/src/components/ClFileUpload.tsx(2 hunks)frontend/src/components/FileUploadSkeleton.tsx(1 hunks)frontend/src/components/UploadProgressWidget.tsx(1 hunks)frontend/src/components/ui/toast.tsx(1 hunks)frontend/src/contexts/UploadProgressContext.tsx(1 hunks)frontend/src/main.tsx(2 hunks)frontend/src/routes/_authenticated/knowledgeManagement.tsx(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/src/main.tsx (2)
frontend/src/contexts/UploadProgressContext.tsx (1)
UploadProgressProvider(52-147)frontend/src/components/UploadProgressWidget.tsx (1)
UploadProgressWidget(9-205)
frontend/src/components/UploadProgressWidget.tsx (2)
frontend/src/contexts/UploadProgressContext.tsx (1)
useUploadProgress(40-46)frontend/src/components/ui/confirmModal.tsx (1)
ConfirmModal(23-59)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (2)
frontend/src/contexts/UploadProgressContext.tsx (1)
useUploadProgress(40-46)frontend/src/utils/fileUtils.ts (1)
uploadFileBatch(205-252)
frontend/src/components/ClFileUpload.tsx (3)
frontend/src/utils/fileUtils.ts (1)
getFileType(9-31)server/shared/types.ts (2)
MIME_TYPE_MAPPINGS(168-191)EXTENSION_MAPPINGS(194-201)server/shared/fileUtils.ts (1)
isValidFile(10-46)
🔇 Additional comments (29)
frontend/src/components/ClFileUpload.tsx (1)
5-7: Verify shared imports across frontend/server boundaries build in all environments.Directly importing from
../../../server/shared/*in a frontend component relies on tooling allowing cross‑package/dir imports (Vite/TS config, aliasing). Please confirm CI/prod builds resolve these paths and don’t pull server-only code.If helpful, I can scan the repo for tsconfig/vite aliases and produce a quick check script to validate resolution.
frontend/src/main.tsx (1)
32-36: LGTM: Properly wires the provider and widget at the app root.Provider placement and widget rendering look correct alongside RouterProvider and Toaster.
frontend/src/components/FileUploadSkeleton.tsx (1)
18-20: Fix potential crash when processedFiles exceeds totalFiles.
skeletonCountcan become negative, causingArray.from({ length: skeletonCount })to throw. Clamp to 0–4.Apply this diff:
- const skeletonCount = Math.min(4, totalFiles - processedFiles) + const skeletonCount = Math.max(0, Math.min(4, totalFiles - processedFiles))Likely an incorrect or invalid review comment.
frontend/src/routes/_authenticated/knowledgeManagement.tsx (13)
60-60: LGTM! Clean integration with global upload context.The import statement properly adds the
useUploadProgresshook from the new global upload context.
382-390: Clean migration from localStorage to global context.The code correctly replaces localStorage-based upload state management with the global upload context. The derived state variables are properly extracted from
currentUpload.
553-576: Excellent per-file status tracking implementation.The code properly tracks individual file upload statuses through the global context, providing detailed feedback for each file in the batch. The fallback handling when individual results aren't available is also well implemented.
584-586: Progress updates correctly implemented.The progress calculation and update through the global context is accurate and provides real-time feedback to users.
675-675: Proper cleanup with finishUpload.The upload cleanup is correctly handled by calling
finishUploadwith the upload ID, maintaining consistency with the global state management.
717-720: Consistent pattern for existing collection uploads.The implementation follows the same pattern as new collection uploads, correctly differentiating between new and existing collection scenarios through the
isNewCollectionparameter.
861-861: Consistent cleanup pattern.The
finishUploadcall maintains consistency with the new collection upload flow.
1581-1606: Proper skeleton display for new collection uploads.The conditional rendering correctly shows the upload skeleton only for new collection uploads, with appropriate progress indicators and headers.
1836-1848: Proper skeleton display for existing collection uploads.The conditional rendering correctly shows the upload skeleton for existing collection uploads, properly filtering by collection ID and hiding headers for consistency with the existing UI.
1966-1967: Modal positioning updated appropriately.The modal positioning changes appear to be minor UI adjustments that align with the new upload flow design.
1991-1998: Label text updated for consistency.The label text changes from "Collection title" to "Collection name" provide better consistency throughout the UI.
534-537: createBatches enforces reduced limits
It uses MAX_PAYLOAD_SIZE = 5 MiB and MAX_FILES_PER_BATCH = 5, so batches adhere to the updated constraints.
2092-2093: Verify reduced batch limits against real-world uploads.
Reducing MAX_PAYLOAD_SIZE from 45 MB to 5 MB and MAX_FILES_PER_BATCH from 50 to 5 may degrade performance for users uploading many or large files—load-test with representative payloads and adjust limits based on observed throughput and timeout behavior.frontend/src/contexts/UploadProgressContext.tsx (13)
1-1: LGTM! Appropriate React and TypeScript imports.The imports correctly include all necessary React hooks and types for context implementation.
3-8: Well-structured interface for batch progress tracking.The
UploadBatchProgressinterface provides comprehensive tracking for batch upload operations with all necessary fields.
10-16: Comprehensive file status tracking interface.The
UploadFileStatusinterface properly tracks individual file upload states with appropriate status types and optional error handling.
18-26: Well-designed upload task structure.The
UploadTaskinterface effectively combines all upload-related information, supporting both new and existing collection scenarios through theisNewCollectionandtargetCollectionIdfields.
28-36: Comprehensive context interface design.The
UploadProgressContextTypeinterface provides all necessary methods for managing upload lifecycle, with appropriate parameter types and return values.
40-46: Standard React context hook pattern.The
useUploadProgresshook follows React best practices with proper error handling for usage outside the provider.
55-82: Robust upload initiation implementation.The
startUploadfunction properly generates unique IDs, initializes file statuses, and creates the upload task with all required fields. The use ofuseCallbackis appropriate for performance optimization.
84-97: Efficient progress update implementation.The
updateProgressfunction correctly updates batch progress while preserving other upload task properties. The null check ensures safe updates.
99-112: Proper file status update with immutable patterns.The
updateFileStatusfunction correctly updates individual file statuses using immutable update patterns, ensuring React re-renders properly.
114-119: Clean upload completion handling.The
finishUploadfunction properly clears the current upload, maintaining clean state management.
121-126: Consistent cancel upload implementation.The
cancelUploadfunction follows the same pattern asfinishUpload, providing consistent cleanup behavior.
128-130: Simple and effective upload retrieval.The
getUploadProgressfunction provides a straightforward way to retrieve upload progress by ID.
132-147: Complete provider implementation.The
UploadProgressProvidercomponent properly provides all context methods and maintains the upload state. The value object correctly exposes all required functionality.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
frontend/src/components/ui/toast.tsx (1)
70-70: Consider making the toast duration configurable.Hardcoding the toast duration to 10 seconds may not be appropriate for all types of toasts. Some messages (errors, warnings) might need longer display times while others (success confirmations) could be shorter.
Consider accepting the duration as a prop with 10000ms as the default:
-duration={10000} +duration={props.duration ?? 10000}frontend/src/components/FileUploadSkeleton.tsx (1)
35-35: Simplify the conditional class application.The empty string in the ternary operator is unnecessary.
-<div className={showHeaders ? "mt-2" : ""}> +<div className={showHeaders ? "mt-2" : undefined}>frontend/src/components/UploadProgressWidget.tsx (1)
159-168: Consider using icon components instead of inline SVGs.The inline SVGs for status indicators could be replaced with icon components from lucide-react for better consistency and maintainability. Based on learnings, the project already uses lucide-react v0.475.0.
-{file.status === 'uploaded' && ( - <svg width="14" height="10" viewBox="0 0 14 10" fill="none" xmlns="http://www.w3.org/2000/svg"> - <path d="M1 5L5 9L13 1" stroke="#10B981" strokeWidth="2" strokeLinecap="round" strokeLinejoin="round"/> - </svg> -)} -{file.status === 'failed' && ( - <svg width="14" height="14" viewBox="0 0 14 14" fill="none" xmlns="http://www.w3.org/2000/svg"> - <path d="M3 3L11 11M11 3L3 11" stroke="#EF4444" strokeWidth="2" strokeLinecap="round" strokeLinejoin="round"/> - </svg> -)} +{file.status === 'uploaded' && ( + <Check className="h-3.5 w-3.5 text-green-500" /> +)} +{file.status === 'failed' && ( + <X className="h-3.5 w-3.5 text-red-500" /> +)}Don't forget to import the Check icon at the top:
-import { X, ChevronUp, ChevronDown } from 'lucide-react' +import { X, ChevronUp, ChevronDown, Check } from 'lucide-react'frontend/src/routes/_authenticated/knowledgeManagement.tsx (3)
568-583: Consider extracting the file status update logic into a helper function.The logic for updating file statuses based on upload results is duplicated between
handleUploadandhandleAddFilesToCollection. This could lead to inconsistencies if one location is updated but not the other.Extract into a helper function:
const updateFileStatuses = ( uploadId: string, batchFiles: File[], uploadResult: any, updateFileStatus: (uploadId: string, fileName: string, status: 'pending' | 'uploading' | 'uploaded' | 'failed', error?: string) => void ) => { if (uploadResult.results) { uploadResult.results.forEach((result: any, index: number) => { const file = batchFiles[index] if (result.success) { updateFileStatus(uploadId, file.name, 'uploaded') } else { updateFileStatus(uploadId, file.name, 'failed', result.error || 'Upload failed') } }) } else { // Fallback: mark all as uploaded if no individual results available batchFiles.forEach(file => { updateFileStatus(uploadId, file.name, 'uploaded') }) } }Then use it in both places:
-// Update individual file statuses based on results -if (uploadResult.results) { - uploadResult.results.forEach((result: any, index: number) => { - const file = batchFiles[index] - if (result.success) { - updateFileStatus(uploadId, file.name, 'uploaded') - } else { - updateFileStatus(uploadId, file.name, 'failed', result.error || 'Upload failed') - } - }) -} else { - // Fallback: mark all as uploaded if no individual results available - batchFiles.forEach(file => { - updateFileStatus(uploadId, file.name, 'uploaded') - }) -} +updateFileStatuses(uploadId, batchFiles, uploadResult, updateFileStatus)
1989-1990: Inconsistent modal positioning between different modals.The upload modal is positioned differently (
pt-24 pr-24) compared to other modals in the component which are centered. This could create an inconsistent user experience.Consider using consistent positioning for all modals:
-<div className="fixed inset-0 bg-black bg-opacity-50 flex items-start justify-end pt-24 pr-24 z-50"> +<div className="fixed inset-0 bg-black bg-opacity-50 flex items-center justify-center z-50">
2123-2124: Document the batch configuration constants.The batch size limits are important configuration that affects upload behavior. These should be documented for future maintainability.
Add documentation:
const BATCH_CONFIG = { + // Maximum payload size per batch (5MB) MAX_PAYLOAD_SIZE: 5 * 1024 * 1024, + // Maximum number of files per batch MAX_FILES_PER_BATCH: 5, }frontend/src/components/ClFileUpload.tsx (5)
5-7: Avoid duplicating file-type detection logic; import the shared helper.You already have getFileType in frontend/src/utils/fileUtils.ts. Import it and drop local MIME/extension mappings here to reduce coupling to server/shared and keep one source of truth.
Apply this diff:
-import { FileType, MIME_TYPE_MAPPINGS, EXTENSION_MAPPINGS } from "../../../server/shared/types" -import { isValidFile } from "../../../server/shared/fileUtils" +import { FileType } from "../../../server/shared/types" +import { getFileType } from "@/utils/fileUtils" +import { isValidFile } from "../../../server/shared/fileUtils"
16-35: Remove in-file getFileType (now imported).Drops duplicated logic and relies on the centralized helper.
Apply this diff:
-// Helper function to determine FileType from a file -const getFileType = (file: File): FileType => { - // First check by MIME type - for (const [fileType, mimeTypes] of Object.entries(MIME_TYPE_MAPPINGS)) { - if ((mimeTypes as readonly string[]).includes(file.type)) { - return fileType as FileType - } - } - - // Fallback to extension checking - const fileName = file.name.toLowerCase() - for (const [fileType, extensions] of Object.entries(EXTENSION_MAPPINGS)) { - if ((extensions as readonly string[]).some(ext => fileName.endsWith(ext))) { - return fileType as FileType - } - } - - // Default fallback - return FileType.FILE -}
91-101: Stabilize drag-highlight to prevent flicker (nested dragenter/dragleave).Counting enter/leave events with a ref avoids the highlight toggling rapidly over children.
Apply this diff:
const [isDragging, setIsDragging] = useState(false) const fileInputRef = useRef<HTMLInputElement>(null) const folderInputRef = useRef<HTMLInputElement>(null) + const dragCounterRef = useRef(0) - const handleDragEnter = (e: React.DragEvent<HTMLDivElement>) => { - e.preventDefault() - e.stopPropagation() - if (!isUploading) { - setIsDragging(true) - } - } + const handleDragEnter = (e: React.DragEvent<HTMLDivElement>) => { + e.preventDefault() + e.stopPropagation() + if (isUploading) return + dragCounterRef.current += 1 + setIsDragging(true) + } - const handleDragLeave = (e: React.DragEvent<HTMLDivElement>) => { - e.preventDefault() - e.stopPropagation() - if (!isUploading) { - setIsDragging(false) - } - } + const handleDragLeave = (e: React.DragEvent<HTMLDivElement>) => { + e.preventDefault() + e.stopPropagation() + if (isUploading) return + dragCounterRef.current = Math.max(0, dragCounterRef.current - 1) + if (dragCounterRef.current === 0) setIsDragging(false) + } const handleDrop = useCallback( (e: React.DragEvent<HTMLDivElement>) => { e.preventDefault() e.stopPropagation() - setIsDragging(false) + setIsDragging(false) + dragCounterRef.current = 0Also applies to: 103-109, 116-123
214-231: Add cross-browser fallback when DataTransferItem.webkitGetAsEntry is unavailable.Firefox and some environments don’t expose webkitGetAsEntry. Fall back to e.dataTransfer.files so simple file drops still work.
Apply this diff:
- Promise.all(filePromises) + if (filePromises.length === 0) { + const files = Array.from(e.dataTransfer.files).filter( + (file) => !file.name.startsWith("."), + ) + onFilesSelect(files) + return + } + Promise.all(filePromises) .then((filesArrays) => { onFilesSelect(filesArrays.flat()) }) .catch((err) => { console.error("Error traversing file tree:", err) })
392-441: Minor: remove unused map index parameter.index isn’t used; drop it to satisfy stricter linters and reduce noise.
Apply this diff:
- {selectedFiles.map((selectedFile, index) => { + {selectedFiles.map((selectedFile) => {frontend/src/contexts/UploadProgressContext.tsx (1)
84-97: Clamp progress to avoid overflows.Defensive bounds prevent UI showing >100% or invalid batch numbers.
Apply this diff:
const updateProgress = useCallback((uploadId: string, current: number, batch: number) => { setCurrentUpload(prev => { if (!prev || prev.id !== uploadId) return prev - - return { + const nextCurrent = Math.min(Math.max(current, 0), prev.batchProgress.total) + const nextBatch = Math.min(Math.max(batch, 0), prev.batchProgress.totalBatches) + return { ...prev, batchProgress: { ...prev.batchProgress, - current, - batch + current: nextCurrent, + batch: nextBatch } } }) }, [])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
frontend/src/assets/collectionIcons/document.svgis excluded by!**/*.svgfrontend/src/assets/collectionIcons/image.svgis excluded by!**/*.svgfrontend/src/assets/collectionIcons/pdf.svgis excluded by!**/*.svgfrontend/src/assets/collectionIcons/ppt.svgis excluded by!**/*.svgfrontend/src/assets/collectionIcons/spreadsheet.svgis excluded by!**/*.svgfrontend/src/assets/collectionIcons/text.svgis excluded by!**/*.svg
📒 Files selected for processing (7)
frontend/src/components/ClFileUpload.tsx(2 hunks)frontend/src/components/FileUploadSkeleton.tsx(1 hunks)frontend/src/components/UploadProgressWidget.tsx(1 hunks)frontend/src/components/ui/toast.tsx(1 hunks)frontend/src/contexts/UploadProgressContext.tsx(1 hunks)frontend/src/main.tsx(2 hunks)frontend/src/routes/_authenticated/knowledgeManagement.tsx(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/src/components/UploadProgressWidget.tsx (2)
frontend/src/contexts/UploadProgressContext.tsx (1)
useUploadProgress(40-46)frontend/src/components/ui/confirmModal.tsx (1)
ConfirmModal(23-59)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (2)
frontend/src/contexts/UploadProgressContext.tsx (1)
useUploadProgress(40-46)frontend/src/utils/fileUtils.ts (1)
uploadFileBatch(205-252)
frontend/src/main.tsx (2)
frontend/src/contexts/UploadProgressContext.tsx (1)
UploadProgressProvider(52-147)frontend/src/components/UploadProgressWidget.tsx (1)
UploadProgressWidget(9-205)
frontend/src/components/ClFileUpload.tsx (3)
frontend/src/utils/fileUtils.ts (1)
getFileType(9-31)server/shared/types.ts (2)
MIME_TYPE_MAPPINGS(168-191)EXTENSION_MAPPINGS(194-201)server/shared/fileUtils.ts (1)
isValidFile(10-46)
🪛 GitHub Actions: TypeScript Build Check
frontend/src/routes/_authenticated/knowledgeManagement.tsx
[error] 389-389: TS6133: 'cancelUpload' is declared but its value is never read.
frontend/src/components/ClFileUpload.tsx
[error] 2-2: TS6133: 'Upload' is declared but its value is never read.
[error] 2-2: TS6133: 'Trash2' is declared but its value is never read.
🔇 Additional comments (5)
frontend/src/components/FileUploadSkeleton.tsx (1)
8-8: LGTM! Clean implementation of the conditional header display.The optional
showHeadersprop with a sensible default oftrueallows flexible usage of the skeleton component.Also applies to: 16-16
frontend/src/main.tsx (1)
10-11: LGTM! Clean integration of the global upload progress system.The upload progress provider and widget are correctly integrated at the app level, ensuring all components have access to the upload context.
Also applies to: 32-36
frontend/src/routes/_authenticated/knowledgeManagement.tsx (1)
1605-1605: No changes needed for skeleton loader logic. ThestartUploadcall initializesisNewCollectionon the upload task and all subsequent updates (updateProgress/updateFileStatus) spread the previous state, so the skeleton only appears for new-collection uploads.frontend/src/components/ClFileUpload.tsx (1)
315-347: LGTM: drag-and-drop surface and accessibility basics are solid.Visible labels, alt text for icons, and disabled states are handled well.
frontend/src/contexts/UploadProgressContext.tsx (1)
52-82: LGTM: simple, single-upload global state with immutable updates.API is coherent; startUpload returns an id and initializes per-file status cleanly.
Confirm the product requirement is “only one upload globally at a time.” If multiple uploads can start concurrently, we should guard startUpload (reject or queue).
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (2)
734-737: Apply the same progress fix in add-to-collection flow.- let totalSuccessful = 0 - let totalSkipped = 0 - let totalFailed = 0 + let totalSuccessful = 0 + let totalSkipped = 0 + let totalFailed = 0 + let processed = 0 @@ - // Update progress - const newCurrent = (i + 1) * batchFiles.length - updateProgress(uploadId, newCurrent, i + 1) + // Update progress + processed += batchFiles.length + updateProgress(uploadId, processed, i + 1)Also applies to: 776-779
686-693: Revoke object URLs to avoid memory leaks.URLs created for previews aren’t revoked on remove/clear/close.
const handleFilesSelect = (files: File[]) => { const newFiles: FileUploadSelectedFile[] = files.map((file) => ({ file, id: Math.random().toString(), preview: URL.createObjectURL(file), })) setSelectedFiles((prev) => [...prev, ...newFiles]) } const handleRemoveFile = (id: string) => { - setSelectedFiles((prev) => prev.filter((f) => f.id !== id)) + setSelectedFiles((prev) => { + const toRemove = prev.find((f) => f.id === id) + if (toRemove?.preview) URL.revokeObjectURL(toRemove.preview) + return prev.filter((f) => f.id !== id) + }) } const handleRemoveAllFiles = () => { - setSelectedFiles([]) + setSelectedFiles((prev) => { + prev.forEach((f) => f.preview && URL.revokeObjectURL(f.preview)) + return [] + }) }const handleCloseModal = () => { setShowNewCollection(false) setAddingToCollection(null) setTargetFolder(null) setCollectionName("") - setSelectedFiles([]) + setSelectedFiles((prev) => { + prev.forEach((f) => f.preview && URL.revokeObjectURL(f.preview)) + return [] + }) setOpenDropdown(null) }Also applies to: 695-701, 699-701, 510-517
🧹 Nitpick comments (5)
frontend/src/components/ClFileUpload.tsx (4)
16-35: DRY: Reuse centralized getFileType instead of redefining it here.This duplicates logic already available in frontend/src/utils/fileUtils.ts, increasing drift risk.
Apply this diff:
+import { getFileType } from "@/utils/fileUtils" -// Helper function to determine FileType from a file -const getFileType = (file: File): FileType => { - // First check by MIME type - for (const [fileType, mimeTypes] of Object.entries(MIME_TYPE_MAPPINGS)) { - if ((mimeTypes as readonly string[]).includes(file.type)) { - return fileType as FileType - } - } - - // Fallback to extension checking - const fileName = file.name.toLowerCase() - for (const [fileType, extensions] of Object.entries(EXTENSION_MAPPINGS)) { - if ((extensions as readonly string[]).some(ext => fileName.endsWith(ext))) { - return fileType as FileType - } - } - - // Default fallback - return FileType.FILE -}
5-7: Align imports to shared alias to avoid brittle relative paths.Use the same alias style as elsewhere (e.g., knowledgeManagement.tsx uses "shared/types").
-import { FileType, MIME_TYPE_MAPPINGS, EXTENSION_MAPPINGS } from "../../../server/shared/types" +import { FileType, MIME_TYPE_MAPPINGS, EXTENSION_MAPPINGS } from "shared/types"If available, also prefer importing isValidFile via the shared alias.
451-456: Prevent uploading unsupported files.Disable the Upload button if any selected file is invalid per isValidFile.
- disabled={ - selectedFiles.length === 0 || - isUploading || - !collectionName.trim() - } + disabled={ + selectedFiles.length === 0 || + isUploading || + !collectionName.trim() || + selectedFiles.some(sf => !isValidFile(sf.file)) + }
239-243: Optional: Filter out unsupported files at selection time.Drop hidden/unsupported files early to keep the queue clean (you already show “UNSUPPORTED FORMAT”, so treat this as optional).
- const filteredFiles = Array.from(e.target.files).filter( - (file) => !file.name.startsWith("."), - ) + const filteredFiles = Array.from(e.target.files).filter( + (file) => !file.name.startsWith(".") && isValidFile(file), + )- const filteredFiles = Array.from(e.target.files).filter( - (file: File) => !file.name.startsWith("."), - ) + const filteredFiles = Array.from(e.target.files).filter( + (file: File) => !file.name.startsWith(".") && isValidFile(file), + )Also applies to: 252-256
frontend/src/components/UploadProgressWidget.tsx (1)
20-21: Clamp progress to 0–100 as a guard.Defensive clamp avoids overshoot if current > total.
- const progressPercentage = batchProgress.total > 0 ? Math.round((batchProgress.current / batchProgress.total) * 100) : 0 + const progressPercentage = batchProgress.total > 0 + ? Math.min(100, Math.max(0, Math.round((batchProgress.current / batchProgress.total) * 100))) + : 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/ClFileUpload.tsx(2 hunks)frontend/src/components/UploadProgressWidget.tsx(1 hunks)frontend/src/contexts/UploadProgressContext.tsx(1 hunks)frontend/src/routes/_authenticated/knowledgeManagement.tsx(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (2)
frontend/src/contexts/UploadProgressContext.tsx (1)
useUploadProgress(40-46)frontend/src/utils/fileUtils.ts (1)
uploadFileBatch(205-252)
frontend/src/components/ClFileUpload.tsx (3)
frontend/src/utils/fileUtils.ts (1)
getFileType(9-31)server/shared/types.ts (2)
MIME_TYPE_MAPPINGS(168-191)EXTENSION_MAPPINGS(194-201)server/shared/fileUtils.ts (1)
isValidFile(10-46)
frontend/src/components/UploadProgressWidget.tsx (2)
frontend/src/contexts/UploadProgressContext.tsx (1)
useUploadProgress(40-46)frontend/src/components/ui/confirmModal.tsx (1)
ConfirmModal(23-59)
🔇 Additional comments (1)
frontend/src/contexts/UploadProgressContext.tsx (1)
28-36: Use fileId only for status updates (avoid name collisions and redundant params).File names collide across folders; id is sufficient and stable. Simplify API and internals.
interface UploadProgressContextType { currentUpload: UploadTask | null startUpload: (collectionName: string, files: { file: File; id: string }[], totalBatches: number, isNewCollection: boolean, targetCollectionId?: string) => string updateProgress: (uploadId: string, current: number, batch: number) => void - updateFileStatus: (uploadId: string, fileName: string, fileId: string, status: 'pending' | 'uploading' | 'uploaded' | 'failed', error?: string) => void + updateFileStatus: (uploadId: string, fileId: string, status: 'pending' | 'uploading' | 'uploaded' | 'failed', error?: string) => void finishUpload: (uploadId: string) => void cancelUpload: (uploadId: string) => void getUploadProgress: (uploadId: string) => UploadTask | null }- const updateFileStatus = useCallback((uploadId: string, fileName: string, fileId: string, status: 'pending' | 'uploading' | 'uploaded' | 'failed', error?: string) => { + const updateFileStatus = useCallback((uploadId: string, fileId: string, status: 'pending' | 'uploading' | 'uploaded' | 'failed', error?: string) => { setCurrentUpload(prev => { if (!prev || prev.id !== uploadId) return prev return { ...prev, - files: prev.files.map(file => - file.name === fileName && file.id === fileId - ? { ...file, status, error } - : file - ) + files: prev.files.map(file => file.id === fileId ? { ...file, status, error } : file) } }) }, [])Run to find and patch call sites:
#!/bin/bash rg -nP --type=ts -C2 '\bupdateFileStatus\s*\('Also applies to: 99-112
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/src/utils/fileUtils.ts (1)
209-210: Thread cancellation across helper APIs for consistency.
addFilesToExistingCollectiondoesn’t allow passing anAbortSignal, so any callers using it can’t cancel in-flight uploads. Consider adding an optionalabortSignalparam and threading it touploadFileBatch.Example:
export const addFilesToExistingCollection = async ( files: SelectedFile[], collectionId: string, parentId?: string | null, abortSignal?: AbortSignal, ): Promise<any> => { return uploadFileBatch( files.map((f) => f.file), collectionId, parentId, abortSignal, ) }frontend/src/routes/_authenticated/knowledgeManagement.tsx (1)
541-545: Propagate cancelation to collection creation.
createCollectionisn’t cancelable, so pressing Cancel during the “create” step won’t abort that request. Pass the sameAbortSignaltocreateCollection(and update its signature to accept it) to make the whole flow cancelable.Proposed call-site change:
-const cl = await createCollection(collectionName.trim(), "") +const cl = await createCollection(collectionName.trim(), "", abortController.signal)And update
createCollectionto acceptabortSignal?: AbortSignaland passsignal: abortSignaltoauthFetch(in fileUtils.ts).frontend/src/contexts/UploadProgressContext.tsx (1)
141-149: Memoize provider value to avoid needless rerenders.Wrap the
valueobject inuseMemowith[currentUpload, startUpload, updateProgress, updateFileStatus, finishUpload, cancelUpload, getUploadProgress].
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/contexts/UploadProgressContext.tsx(1 hunks)frontend/src/routes/_authenticated/knowledgeManagement.tsx(15 hunks)frontend/src/utils/fileUtils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (2)
frontend/src/contexts/UploadProgressContext.tsx (1)
useUploadProgress(41-47)frontend/src/utils/fileUtils.ts (1)
uploadFileBatch(205-254)
🔇 Additional comments (9)
frontend/src/routes/_authenticated/knowledgeManagement.tsx (6)
389-396: LGTM on wiring the global upload context.Destructure looks correct; derived state matches the provider shape.
562-567: Prefer fileId-only status updates to avoid filename collisions.Requiring both name and id couples UI updates to a mutable/display field. Using only
file.idis sufficient and robust when duplicate names exist.If adopting, first change the context API to
updateFileStatus(uploadId, fileId, status, error?), then dropfile.file.namehere:- updateFileStatus(uploadId, file.file.name, file.id, 'uploading') + updateFileStatus(uploadId, file.id, 'uploading')(similarly for 'uploaded' and 'failed')
Also applies to: 569-577, 579-583, 764-773, 775-779
592-595: Progress calc fix looks good.Switching to a running processed count addresses variable batch sizes correctly.
Also applies to: 788-791
734-738: Optional: short‑circuit loops on cancel to avoid extra UI work.Even with abort, add a guard to cease per-file status updates ASAP when canceled.
Example:
-const { currentUpload, startUpload, updateProgress, updateFileStatus, finishUpload } = useUploadProgress() +const { currentUpload, startUpload, updateProgress, updateFileStatus, finishUpload, getUploadProgress } = useUploadProgress() @@ -for (let i = 0; i < batches.length; i++) { +for (let i = 0; i < batches.length; i++) { + if (!getUploadProgress(uploadId)) break const batchFiles = batches[i].map((f) => ({ file: f.file, id: f.id })) // Mark batch files as uploading - batchFiles.forEach(file => { - updateFileStatus(uploadId, file.file.name, file.id, 'uploading') - }) + batchFiles.forEach(file => { + if (!getUploadProgress(uploadId)) return + updateFileStatus(uploadId, file.file.name, file.id, 'uploading') + })Also applies to: 749-756, 757-762
1625-1648: Progress skeleton UX: good conditional rendering.New/Existing collection gating and header toggling read well.
Also applies to: 1880-1892
2144-2146: Verify new batching limits with backend constraints.Dropping to 5MB/5 files per batch may impact throughput; ensure server accepts many small multipart posts and any reverse proxies (e.g., Nginx, CDN) won’t 413 on cumulative requests.
frontend/src/contexts/UploadProgressContext.tsx (2)
26-27: Cancellation via AbortController: solid implementation.Provider tracks an AbortController per upload and exposes it to callers;
cancelUploadaborts and clears state. Looks good.Also applies to: 31-37, 56-66, 67-85, 124-135
33-37: Key by fileId only inupdateFileStatus(avoid name coupling).Matching on both
nameandidcan fail if the name changes or duplicates exist. Id alone is sufficient.Apply:
- updateFileStatus: (uploadId: string, fileName: string, fileId: string, status: 'pending' | 'uploading' | 'uploaded' | 'failed', error?: string) => void + updateFileStatus: (uploadId: string, fileId: string, status: 'pending' | 'uploading' | 'uploaded' | 'failed', error?: string) => void @@ -const updateFileStatus = useCallback((uploadId: string, fileName: string, fileId: string, status: 'pending' | 'uploading' | 'uploaded' | 'failed', error?: string) => { +const updateFileStatus = useCallback((uploadId: string, fileId: string, status: 'pending' | 'uploading' | 'uploaded' | 'failed', error?: string) => { setCurrentUpload(prev => { if (!prev || prev.id !== uploadId) return prev return { ...prev, files: prev.files.map(file => - file.name === fileName && file.id === fileId + file.id === fileId ? { ...file, status, error } : file ) } }) }, [])Then update call sites accordingly.
Also applies to: 102-115
frontend/src/utils/fileUtils.ts (1)
209-210: Confirmed signal forwarding in authFetch.authFetchspreadsinitinto both fetch calls (includingsignal), so aborting the passedAbortSignalwill cancel the request as expected.
rahul1841
dismissed stale reviews from Himanshvarma and kalpadhwaryu
via
753cf41
|
Maybe we can start adding global state management instead of creating wrapper components |
5 participants
Description
Testing
Summary by CodeRabbit
New Features
Refactor
Style
Chores