Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Implement multiple File uploading #9814

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
+33,119 −5
Open

Implement multiple File uploading #9814

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fbbd353
Implement multiple File uploading
DonXavierdev Jan 7, 2025
f136465
Added jspdf to package.json
DonXavierdev Jan 7, 2025
21cd51d
Implement multiple File uploading
DonXavierdev Jan 7, 2025
a757ad5
Added jspdf to package.json
DonXavierdev Jan 7, 2025
f478af1
Merge branch 'issues/7417/File-upload-enhancement' of https://github.…
DonXavierdev Jan 7, 2025
1c20535
Merge branch 'develop' into issues/7417/File-upload-enhancement
DonXavierdev Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
146 changes: 144 additions & 2 deletions package-lock.json
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Should only be adding jspdf 👍

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@
"i18next-browser-languagedetector": "^8.0.2",
"i18next-http-backend": "^3.0.1",
"input-otp": "^1.4.1",
"jspdf": "^2.5.2",
"lodash-es": "^4.17.21",
"lucide-react": "^0.469.0",
"markdown-it": "^14.1.0",
Expand Down
41 changes: 40 additions & 1 deletion src/hooks/useFileUpload.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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";
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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

import {
ChangeEvent,
DetailedHTMLProps,
Expand Down Expand Up @@ -97,6 +98,33 @@ export default function useFileUpload(
const [files, setFiles] = useState<File[]>([]);
const queryClient = useQueryClient();

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;
}
};
Comment on lines +101 to +127
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance PDF generation robustness and quality

The PDF generation implementation has several areas for improvement:

  1. No validation of image types before processing
  2. Hard-coded image dimensions (190, 0) may cause distortion
  3. 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.

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

const onFileChange = (e: ChangeEvent<HTMLInputElement>): any => {
if (!e.target.files?.length) {
return;
Expand Down Expand Up @@ -238,8 +266,19 @@ export default function useFileUpload(
if (!validateFileUpload()) return;

setProgress(0);
let filesToUpload = files;

if (files.length > 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

const pdfFile = await generatePDF(files);
if (pdfFile) {
filesToUpload = [pdfFile];
} else {
console.error("Failed to generate PDF from multiple files.");
return;
}
}
Comment on lines +269 to +279
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve error handling and user feedback for multiple file uploads

The multiple file upload implementation needs better error handling and user feedback:

  1. No progress indication during PDF generation
  2. Silent failure if PDF generation fails
  3. 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


for (const [index, file] of files.entries()) {
for (const [index, file] of filesToUpload.entries()) {
const filename =
allowNameFallback && uploadFileNames[index] === "" && file
? file.name
Expand Down
9 changes: 7 additions & 2 deletions src/pages/Encounters/tabs/EncounterFilesTab.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ export const EncounterFilesTab = (props: EncounterTabProps) => {
],
allowNameFallback: false,
onUpload: () => refetch(),
multiple: true,
});

useEffect(() => {
Expand Down Expand Up @@ -543,8 +544,12 @@ const FileUploadDialog = ({
</DialogHeader>
<div className="mb-1 flex items-center justify-between gap-2 rounded-md bg-secondary-300 px-4 py-2">
<span>
<CareIcon icon="l-paperclip" className="mr-2" />
{fileUpload.files?.[0]?.name}
{fileUpload.files?.map((file, index) => (
<div key={index} className="flex items-center mb-2">
<CareIcon icon="l-paperclip" className="mr-2" />
{file.name}
</div>
))}
Comment on lines +547 to +552
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance multiple file upload UI/UX

The current UI for multiple files has several limitations:

  1. No file size information
  2. No way to remove individual files
  3. 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.

</span>
</div>
<TextFormField
Expand Down
Loading