-
Notifications
You must be signed in to change notification settings - Fork 532
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.
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
base: develop
Are you sure you want to change the base?
Changes from 1 commit
fbbd353
f136465
21cd51d
a757ad5
f478af1
1c20535
1dbf18f
5fe1f64
ece5d70
467cf59
5d65539
8014bf8
74186e6
18bb454
da64eb4
5141210
127acde
14d397f
f1c77d2
677fd46
66ddf42
cec95d7
cc6e0d8
eb5b3c2
0c77702
e59ef78
84d9bd7
91306f2
2bc7574
4534c6f
6e76b95
eddcfc7
776ba36
6fff5bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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"; | ||
import { | ||
ChangeEvent, | ||
DetailedHTMLProps, | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This maybe unnecessary, but play around with larger number of image files (like say 10-15) and see how quickly it converts to pdf. If it does take a few seconds, might be worth using the progress bar here. You can use setProgress for it (and reset it to 0 once pdf file is created as it's also used for indicating file upload progress). You would also want to inform the user file is being converted through toast. Experiment and check if it fits within existing UI, if not we can discuss further. |
||
reader.onerror = () => reject("Error reading file"); | ||
reader.readAsDataURL(file); | ||
}); | ||
|
||
pdf.addImage(imgData, "JPEG", 10, 10, 190, 0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve PDF image layout. The hard-coded dimensions (10, 10, 190, 0) don't account for image aspect ratios and may result in distorted images. Consider calculating dimensions based on image aspect ratio. - pdf.addImage(imgData, "JPEG", 10, 10, 190, 0);
+ const img = new Image();
+ img.src = imgData;
+ const aspectRatio = img.height / img.width;
+ const pageWidth = pdf.internal.pageSize.getWidth() - 20; // 10px margin on each side
+ const pageHeight = pageWidth * aspectRatio;
+ pdf.addImage(imgData, "JPEG", 10, 10, pageWidth, pageHeight);
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 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 chainVerify file types before PDF generation Ensure that PDF generation is only attempted with image files. Let's verify the current file type handling: 🏁 Scripts executedThe 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) |
||
const onFileChange = (e: ChangeEvent<HTMLInputElement>): any => { | ||
if (!e.target.files?.length) { | ||
return; | ||
|
@@ -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. Choose a reason for hiding this commentThe 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; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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;
}
}
🧰 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -136,6 +136,7 @@ export const EncounterFilesTab = (props: EncounterTabProps) => { | |
], | ||
allowNameFallback: false, | ||
onUpload: () => refetch(), | ||
multiple: true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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>
|
||
}); | ||
|
||
useEffect(() => { | ||
|
@@ -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> | ||
))} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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}
+ />
+))}
|
||
</span> | ||
</div> | ||
<TextFormField | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add jspdf to build configuration
The jspdf import is causing pipeline failures. Add it to build.rollupOptions.external.
🧰 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