Implement multiple File uploading #9814
Conversation
WalkthroughThe pull request introduces an enhancement to the file upload functionality by enabling multi-file support. A new Changes
Sequence DiagramsequenceDiagram
participant User
participant FileUploadHook
participant jsPDF
participant FileReader
User->>FileUploadHook: Select multiple files
FileUploadHook->>FileReader: Read image files
FileReader-->>FileUploadHook: Image data URLs
FileUploadHook->>jsPDF: Generate PDF
jsPDF-->>FileUploadHook: PDF File
FileUploadHook->>User: Upload single PDF
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
❌ Deploy Preview for care-ohc failed.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/hooks/useFileUpload.tsx (1)
Line range hint
132-146: Optimize image compression workflowThe current implementation compresses images after adding them to state, which could cause multiple re-renders. Consider compressing before updating state.
const onFileChange = (e: ChangeEvent<HTMLInputElement>): any => { if (!e.target.files?.length) { return; } const selectedFiles = Array.from(e.target.files); - setFiles((prev) => [...prev, ...selectedFiles]); + + const processFiles = async () => { + const processedFiles = await Promise.all( + selectedFiles.map(async (file) => { + const ext = file.name.split(".")[1]; + if (ExtImage.includes(ext)) { + const options = { + initialQuality: 0.6, + alwaysKeepResolution: true, + }; + return imageCompression(file, options); + } + return file; + }) + ); + setFiles((prev) => [...prev, ...processedFiles]); + }; - selectedFiles.forEach((file) => { - const ext: string = file.name.split(".")[1]; - if (ExtImage.includes(ext)) { - const options = { - initialQuality: 0.6, - alwaysKeepResolution: true, - }; - imageCompression(file, options).then((compressedFile: File) => { - setFiles((prev) => - prev.map((f) => (f.name === file.name ? compressedFile : f)), - ); - }); - } - }); + processFiles(); };🧰 Tools
🪛 GitHub Actions: Cypress Tests
[error] Failed to resolve import "jspdf". The module needs to be added to build.rollupOptions.external or properly installed.
🪛 GitHub Actions: Deploy Care Fe
[error] Failed to resolve import 'jspdf'. Module needs to be added to build.rollupOptions.external
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/hooks/useFileUpload.tsx(3 hunks)src/pages/Encounters/tabs/EncounterFilesTab.tsx(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Cypress Tests
src/hooks/useFileUpload.tsx
[error] Failed to resolve import "jspdf". The module needs to be added to build.rollupOptions.external or properly installed.
🪛 GitHub Actions: Deploy Care Fe
src/hooks/useFileUpload.tsx
[error] Failed to resolve import 'jspdf'. Module needs to be added to build.rollupOptions.external
🔇 Additional comments (1)
src/pages/Encounters/tabs/EncounterFilesTab.tsx (1)
139-139: LGTM! Multiple file upload configuration is correctly implemented.
| @@ -1,6 +1,7 @@ | |||
| import { useMutation, useQueryClient } from "@tanstack/react-query"; | |||
| import imageCompression from "browser-image-compression"; | |||
| import { t } from "i18next"; | |||
| import jsPDF from "jspdf"; | |||
There was a problem hiding this comment.
Add jspdf to build configuration
The jspdf import is causing pipeline failures. Add it to build.rollupOptions.external.
#!/bin/bash
# Verify if jspdf is properly configured in the build setup
fd -e js -e ts "vite.config" -x cat {} \; | grep -A 5 "rollupOptions"🧰 Tools
🪛 GitHub Actions: Cypress Tests
[error] Failed to resolve import "jspdf". The module needs to be added to build.rollupOptions.external or properly installed.
🪛 GitHub Actions: Deploy Care Fe
[error] Failed to resolve import 'jspdf'. Module needs to be added to build.rollupOptions.external
src/hooks/useFileUpload.tsx
Outdated
| let filesToUpload = files; | ||
|
|
||
| if (files.length > 1) { | ||
| const pdfFile = await generatePDF(files); | ||
| if (pdfFile) { | ||
| filesToUpload = [pdfFile]; | ||
| } else { | ||
| console.error("Failed to generate PDF from multiple files."); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and user feedback for multiple file uploads
The multiple file upload implementation needs better error handling and user feedback:
- No progress indication during PDF generation
- Silent failure if PDF generation fails
- Missing size limit checks for multiple files
Apply this enhancement:
let filesToUpload = files;
if (files.length > 1) {
+ const totalSize = files.reduce((sum, file) => sum + file.size, 0);
+ const MAX_TOTAL_SIZE = 50 * 1024 * 1024; // 50MB
+ if (totalSize > MAX_TOTAL_SIZE) {
+ setError(t("file_error__total_size"));
+ return;
+ }
+ setProgress(0);
const pdfFile = await generatePDF(files);
if (pdfFile) {
filesToUpload = [pdfFile];
} else {
- console.error("Failed to generate PDF from multiple files.");
+ setError(t("file_error__pdf_generation"));
+ setProgress(null);
return;
}
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Cypress Tests
[error] Failed to resolve import "jspdf". The module needs to be added to build.rollupOptions.external or properly installed.
🪛 GitHub Actions: Deploy Care Fe
[error] Failed to resolve import 'jspdf'. Module needs to be added to build.rollupOptions.external
| {fileUpload.files?.map((file, index) => ( | ||
| <div key={index} className="flex items-center mb-2"> | ||
| <CareIcon icon="l-paperclip" className="mr-2" /> | ||
| {file.name} | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance multiple file upload UI/UX
The current UI for multiple files has several limitations:
- No file size information
- No way to remove individual files
- Single filename input is confusing for multiple files
Consider this enhanced implementation:
-{fileUpload.files?.map((file, index) => (
- <div key={index} className="flex items-center mb-2">
- <CareIcon icon="l-paperclip" className="mr-2" />
- {file.name}
- </div>
-))}
+{fileUpload.files?.map((file, index) => (
+ <div key={index} className="flex items-center justify-between mb-2 bg-white p-2 rounded">
+ <div className="flex items-center">
+ <CareIcon icon="l-paperclip" className="mr-2" />
+ <div className="flex flex-col">
+ <span className="text-sm font-medium">{file.name}</span>
+ <span className="text-xs text-gray-500">
+ {(file.size / 1024).toFixed(1)}KB
+ </span>
+ </div>
+ </div>
+ <button
+ onClick={() => fileUpload.removeFile(index)}
+ className="text-gray-500 hover:text-red-500"
+ >
+ <CareIcon icon="l-times" />
+ </button>
+ </div>
+))}Also consider updating the filename input field to handle multiple files:
-<TextFormField
- name="consultation_file"
- type="text"
- label={t("enter_file_name")}
- id="upload-file-name"
- required
- value={fileUpload.fileNames[0] || ""}
- disabled={fileUpload.uploading}
- onChange={(e) => fileUpload.setFileName(e.value)}
- error={fileUpload.error || undefined}
-/>
+{fileUpload.files.map((file, index) => (
+ <TextFormField
+ key={index}
+ name={`file_name_${index}`}
+ type="text"
+ label={t("enter_file_name_for", { name: file.name })}
+ id={`upload-file-name-${index}`}
+ required
+ value={fileUpload.fileNames[index] || ""}
+ disabled={fileUpload.uploading}
+ onChange={(e) => fileUpload.setFileName(e.value, index)}
+ error={fileUpload.error || undefined}
+ />
+))}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
104-104: Consider browser-side PDF generation performance impactSince PDF generation will happen in the browser, consider implementing the following optimizations:
- Add a loading state during PDF generation
- Use the existing
browser-image-compressionlibrary to optimize images before PDF generation- Consider implementing lazy loading if handling large numbers of images
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
package.json (1)
104-104: Verify jspdf version and security advisoriesThe addition of jspdf for PDF generation is appropriate. However, let's verify the version and check for any security advisories.
Run the following script to check the latest version and security advisories:
✅ Verification successful
✓ jspdf dependency version and security verified
The specified version
^2.5.2is the latest available and is free from known security vulnerabilities. All previously reported issues were patched in earlier versions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories and latest versions of the jspdf library # Check NPM for latest versions curl -s https://registry.npmjs.org/jspdf | jq '.["dist-tags"].latest' # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "jspdf") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 1036
|
👋 Hi, @DonXavierdev, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
…com/DonXavierdev/care_fe into issues/7417/File-upload-enhancement
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
package-lock.json
Outdated
There was a problem hiding this comment.
Should only be adding jspdf 👍
There was a problem hiding this comment.
Even if i remove other dependencies like atob,canvg etc. When user installs using "npm install" jspdf automatically installs the necessary dependencies into the package-lock.json file. What should i do about it. @Jacobjeevan
There was a problem hiding this comment.
Even if i remove other dependencies like atob,canvg etc. When user installs using "npm install" jspdf automatically installs the necessary dependencies into the package-lock.json file. What should i do about it. @Jacobjeevan
Then it's fine (to include them) 👍 Make sure only those are affected though.
There was a problem hiding this comment.
Not relevant to the issue 🤔
src/hooks/useFileUpload.tsx
Outdated
| @@ -238,8 +266,19 @@ export default function useFileUpload( | |||
| if (!validateFileUpload()) return; | |||
|
|
|||
| setProgress(0); | |||
| let filesToUpload = files; | |||
|
|
|||
| if (files.length > 1) { | |||
There was a problem hiding this comment.
Hmm, right now it would combine any multi-uploads, let's not do that..instead let's add an option on FE (similar to "Upload From Device, Open Camera" etc) to trigger this.
Jacobjeevan
added
the
waiting for related PR
…com/DonXavierdev/care_fe into issues/7417/File-upload-enhancement
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
104-104: Consider performance implications of PDF generationSince this library will be used for merging multiple images into a PDF:
- Consider implementing a maximum file size limit
- Add client-side image compression before PDF generation
- Consider showing a loading indicator during PDF generation
- Test with various image formats (JPEG, PNG, etc.) and sizes
The existing
browser-image-compressiondependency (line 40) could be utilized for image optimization before PDF generation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
🔇 Additional comments (1)
package.json (1)
104-104: Verify jspdf version and compatibilityPlease verify the jspdf version and any known issues:
✅ Verification successful
✅ jspdf version is appropriate and secure
The specified version (^2.5.2) is the latest available and has no known security vulnerabilities. All previously reported issues were patched in earlier versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check latest version and security advisories for jspdf # Get latest version from npm echo "Latest version from npm:" npm view jspdf version # Check for security advisories echo -e "\nSecurity advisories:" gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "jspdf") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 1103
…o issues/7417/File-upload-enhancement
…com/DonXavierdev/care_fe into issues/7417/File-upload-enhancement
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/hooks/useFileUpload.tsx (2)
4-4:⚠️ Potential issueEnhance PDF generation robustness and error handling
The PDF generation implementation needs improvements in several areas:
- No validation of file types before processing
- Hard-coded image dimensions (190, 0) may cause image distortion
- No progress indication during PDF generation
- Basic error handling without specific error messages
Previous review suggestions for improving the implementation are still valid. Additionally:
- Add file type validation before processing
- Consider memory usage for large files
- Add progress indication during PDF generation
- Improve error handling with specific error messages
Also applies to: 101-127
265-275:⚠️ Potential issueImprove error handling and user feedback for multiple file uploads
The multiple file upload implementation needs better error handling and user feedback:
- No size limit checks for multiple files
- Silent failure if PDF generation fails
- No progress indication during PDF generation
Previous review suggestions for improving error handling and user feedback are still valid.
src/pages/Encounters/tabs/EncounterFilesTab.tsx (1)
547-552: 🛠️ Refactor suggestionEnhance multiple file upload UI/UX
The current UI for multiple files has several limitations:
- No file size information
- No way to remove individual files
- Single filename input is confusing for multiple files
Previous review suggestions for improving the UI/UX are still valid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(1 hunks)src/hooks/useFileUpload.tsx(3 hunks)src/pages/Encounters/tabs/EncounterFilesTab.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
| @@ -136,6 +136,7 @@ export const EncounterFilesTab = (props: EncounterTabProps) => { | |||
| ], | |||
| allowNameFallback: false, | |||
| onUpload: () => refetch(), | |||
| multiple: true, | |||
There was a problem hiding this comment.
Revise the automatic PDF generation approach
As per previous feedback from Jacobjeevan, automatically combining all multi-uploads into a PDF is not the desired behavior. Instead, consider adding a separate UI option to trigger PDF generation.
Consider adding a new option in the file upload dropdown menu:
<DropdownMenuContent
align="end"
className="w-[calc(100vw-2.5rem)] sm:w-full"
>
+ <DropdownMenuItem className="flex flex-row items-center text-primary-900">
+ <button
+ onClick={() => handleCombineIntoPDF()}
+ className="flex flex-row items-center"
+ >
+ <CareIcon icon="l-file-pdf" className="mr-1" />
+ <span>{t("combine_as_pdf")}</span>
+ </button>
+ </DropdownMenuItem>Committable suggestion skipped: line range outside the PR's diff.
|
Accidently Clicked on re-review. Can you please respond to this reply . |
|
👋 Hi, @DonXavierdev, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
…o issues/7417/File-upload-enhancement
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/hooks/useFileUpload.tsx (1)
101-127: Revise the multi-file upload approachAs per the reviewer's feedback, instead of automatically combining all multi-uploads into a PDF, add a specific UI option (similar to "Upload From Device" or "Open Camera") to trigger this functionality. This will give users more control over when to combine files into a PDF.
Consider these changes:
- Add a new prop to control PDF generation:
combineToPDF?: boolean- Only trigger PDF generation when this prop is true
- Add a new UI button/option specifically for PDF upload
🧰 Tools
🪛 eslint
[error] 101-101: 'generatePDF' is assigned a value but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(1 hunks)src/hooks/useFileUpload.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 eslint
src/hooks/useFileUpload.tsx
[error] 101-101: 'generatePDF' is assigned a value but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: cypress-run (1)
src/hooks/useFileUpload.tsx
[failure] 284-284:
Cannot find name 'filesToUpload'.
🪛 GitHub Actions: Cypress Tests
src/hooks/useFileUpload.tsx
[error] 284-284: TypeScript error: Cannot find name 'filesToUpload'
🪛 GitHub Actions: Deploy Care Fe
src/hooks/useFileUpload.tsx
[error] 284-284: Cannot find name 'filesToUpload'. TypeScript compilation error
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: CodeQL-Build
- GitHub Check: OSSAR-Scan
- GitHub Check: lint
🔇 Additional comments (3)
src/hooks/useFileUpload.tsx (3)
4-4: Add jspdf to build configurationThe jspdf import is causing pipeline failures. Add it to build.rollupOptions.external.
101-127: Enhance PDF generation robustness and qualityThe PDF generation implementation needs several critical improvements:
- No validation of image types before processing
- Hard-coded image dimensions (190, 0) may cause distortion
- Missing progress indication during PDF generation
- Basic error handling with console.log
🧰 Tools
🪛 eslint
[error] 101-101: 'generatePDF' is assigned a value but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
284-284:⚠️ Potential issueFix undefined variable and implement multi-file handling
The code uses an undefined variable
filesToUpload. This is causing TypeScript compilation errors and will fail at runtime.Add the following code before the for loop to handle multiple files:
+ let filesToUpload = files; + if (files.length > 1) { + const pdfFile = await generatePDF(files); + if (pdfFile) { + filesToUpload = [pdfFile]; + } else { + setError(t("file_error__pdf_generation")); + return; + } + } + for (const [index, file] of filesToUpload.entries()) {Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: cypress-run (1)
[failure] 284-284:
Cannot find name 'filesToUpload'.🪛 GitHub Actions: Cypress Tests
[error] 284-284: TypeScript error: Cannot find name 'filesToUpload'
🪛 GitHub Actions: Deploy Care Fe
[error] 284-284: Cannot find name 'filesToUpload'. TypeScript compilation error
| const generatePDF = async (files: File[]): Promise<File | null> => { | ||
| try { | ||
| const pdf = new jsPDF(); | ||
| for (const [index, file] of files.entries()) { | ||
| const imgData = await new Promise<string>((resolve, reject) => { | ||
| const reader = new FileReader(); | ||
| reader.onload = () => resolve(reader.result as string); | ||
| reader.onerror = () => reject("Error reading file"); | ||
| reader.readAsDataURL(file); | ||
| }); | ||
|
|
||
| pdf.addImage(imgData, "JPEG", 10, 10, 190, 0); | ||
| if (index < files.length - 1) pdf.addPage(); | ||
| } | ||
|
|
||
| const pdfBlob = pdf.output("blob"); | ||
| const pdfFile = new File([pdfBlob], "combined.pdf", { | ||
| type: "application/pdf", | ||
| }); | ||
|
|
||
| console.log("Generated PDF file:", pdfFile); // Log the generated file | ||
| return pdfFile; | ||
| } catch (error) { | ||
| console.error("Error generating PDF:", error); | ||
| return null; | ||
| } | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Add image type validation before PDF generation
The current implementation doesn't validate file types before attempting PDF generation, which could lead to errors when processing non-image files. Add a check using the supported image MIME types:
const isImageFile = (file: File) => {
const imageTypes = ['image/jpeg', 'image/png', 'image/gif', 'image/webp'];
return imageTypes.includes(file.type);
};
// In generatePDF function
if (!files.every(isImageFile)) {
console.error("Non-image files detected");
return null;
}🔗 Analysis chain
Verify file types before PDF generation
Ensure that PDF generation is only attempted with image files. Let's verify the current file type handling:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how file types are handled across the codebase
echo "Searching for file type checks..."
rg -A 5 "file.type" --type ts --type tsx
echo "Searching for image type constants..."
rg -A 5 "ExtImage" --type ts --type tsx
echo "Searching for PDF-related code..."
rg -A 5 "pdf" --type ts --type tsx
Length of output: 438
Script:
#!/bin/bash
# Search for file type checks and image-related code
echo "Searching for file type validations..."
rg "file\.type|fileType" -A 5
echo "Searching for image type checks..."
rg "image/(jpeg|png|gif|webp)" -A 5
echo "Searching for PDF generation context..."
rg "generatePDF|jsPDF" -A 5
Length of output: 8680
🧰 Tools
🪛 eslint
[error] 101-101: 'generatePDF' is assigned a value but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
Jacobjeevan
removed
the
waiting for related PR
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Summary by CodeRabbit
New Features
Dependencies
jspdflibrary for PDF creationinput-otplibrary version