Implement multiple File uploading #9814
Conversation
WalkthroughThe pull request introduces multi-file upload support for the file upload functionality. A new Changes
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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
| 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
| 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.
🛠️ Refactor suggestion
Enhance PDF generation robustness and quality
The PDF generation implementation has several areas for improvement:
- No validation of image types before processing
- Hard-coded image dimensions (190, 0) may cause distortion
- Large files could cause memory issues
Consider this enhanced implementation:
const generatePDF = async (files: File[]): Promise<File | null> => {
try {
const pdf = new jsPDF();
+ const maxWidth = 190;
+ const margin = 10;
for (const [index, file] of files.entries()) {
+ if (!file.type.startsWith('image/')) {
+ throw new Error(`File ${file.name} is not an image`);
+ }
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);
+ // Get image dimensions
+ const img = new Image();
+ await new Promise((resolve, reject) => {
+ img.onload = resolve;
+ img.onerror = reject;
+ img.src = imgData;
+ });
+
+ // Calculate aspect ratio
+ const aspectRatio = img.width / img.height;
+ const width = Math.min(maxWidth, pdf.internal.pageSize.width - 2 * margin);
+ const height = width / aspectRatio;
+
+ pdf.addImage(imgData, "JPEG", margin, margin, width, height);
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;
}
};📝 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.
| 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; | |
| } | |
| }; | |
| const generatePDF = async (files: File[]): Promise<File | null> => { | |
| try { | |
| const pdf = new jsPDF(); | |
| const maxWidth = 190; | |
| const margin = 10; | |
| for (const [index, file] of files.entries()) { | |
| if (!file.type.startsWith('image/')) { | |
| throw new Error(`File ${file.name} is not an image`); | |
| } | |
| 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); | |
| }); | |
| // Get image dimensions | |
| const img = new Image(); | |
| await new Promise((resolve, reject) => { | |
| img.onload = resolve; | |
| img.onerror = reject; | |
| img.src = imgData; | |
| }); | |
| // Calculate aspect ratio | |
| const aspectRatio = img.width / img.height; | |
| const width = Math.min(maxWidth, pdf.internal.pageSize.width - 2 * margin); | |
| const height = width / aspectRatio; | |
| pdf.addImage(imgData, "JPEG", margin, margin, width, height); | |
| if (index < files.length - 1) pdf.addPage(); | |
| } | |
| const pdfBlob = pdf.output("blob"); | |
| const pdfFile = new File([pdfBlob], "combined.pdf", { | |
| type: "application/pdf", | |
| }); | |
| return pdfFile; | |
| } catch (error) { | |
| console.error("Error generating PDF:", error); | |
| return null; | |
| } | |
| }; |
🧰 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}
src/components/Auth/Login.tsx
Outdated
| window.location.href = "/patient/home"; | ||
| localStorage.setItem( | ||
| LocalStorageKeys.patientTokenKey, | ||
| JSON.stringify(tokenData), |
Check failure
Code scanning / CodeQL
Clear text storage of sensitive information High
There was a problem hiding this comment.
Should only be adding jspdf 👍
There was a problem hiding this comment.
Not relevant to the issue 🤔
| @@ -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.
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Summary by CodeRabbit
New Features
Improvements
jspdf).input-otpdependency.