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.

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 29 commits into
base: develop
Choose a base branch
from

Conversation

DonXavierdev
Copy link
Contributor

@DonXavierdev DonXavierdev commented Jan 7, 2025

Proposed Changes

@ohcnetwork/care-fe-code-reviewers

Summary by CodeRabbit

  • New Features

    • Added ability to combine multiple image files into a single PDF during upload.
    • Introduced PDF generation functionality for file uploads.
    • New option "Combine Files To PDF" available in the user interface.
    • New messages for file conversion progress and success, along with an error message for PDF generation failures.
  • Dependencies

    • Added jspdf library for PDF creation.
    • Downgraded input-otp library version.

@DonXavierdev DonXavierdev requested a review from a team as a code owner January 7, 2025 06:57
Copy link
Contributor

coderabbitai bot commented Jan 7, 2025

Walkthrough

The pull request introduces an enhancement to the file upload functionality by enabling multi-file support. A new generatePDF function is added to the useFileUpload hook, allowing multiple image files to be combined into a single PDF file during the upload process. This change simplifies the document upload workflow by automatically merging multiple files, addressing the current limitation of uploading individual pages separately.

Changes

File Change Summary
src/hooks/useFileUpload.tsx Added generatePDF method to convert multiple image files into a single PDF; updated handleFileUpload to include optional combineToPDF parameter.
package.json Added jspdf dependency (v^2.5.2) and adjusted input-otp version from ^1.4.2 to ^1.4.1.
public/locale/en.json Added new entries for "combine_files_pdf", "file_conversion_in_progress", "file_conversion_success", and error message for PDF generation failure.
src/components/Files/FilesTab.tsx Introduced isPdf state variable and modified file upload behavior to incorporate PDF generation option.

Assessment against linked issues

Objective Addressed Explanation
Multi-file upload support [#7417]
Automatic PDF generation
Simplify document upload for non-tech-savvy users

Possibly related PRs

  • Patient files #9787: The changes in this PR involve modifications to the public/locale/en.json file, which are related to the new functionality of combining files into a PDF, as it adds localization strings that support this feature.
  • Fix FileUploadDialog overflow | Use ShadCN component #10047: This PR includes changes to the FilesTab.tsx component, specifically in the FileUploadDialog, which is relevant to the main PR's modifications in the useFileUpload hook and the handling of file uploads, including the generation of PDFs.

Suggested labels

tested, reviewed

Suggested reviewers

  • Jacobjeevan
  • rithviknishad

Poem

🐰 Hop, hop, upload with glee,
Images dance, now PDF-free!
No more pages scattered wide,
One file now, with rabbit pride! 📄✨
Merging docs has never been so neat! 🎉


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 91306f2 and 2bc7574.

📒 Files selected for processing (3)
  • public/locale/en.json (3 hunks)
  • src/components/Files/FilesTab.tsx (5 hunks)
  • src/hooks/useFileUpload.tsx (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/hooks/useFileUpload.tsx
  • src/components/Files/FilesTab.tsx
  • public/locale/en.json

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

netlify bot commented Jan 7, 2025

Deploy Preview for care-ohc ready!

Name Link
🔨 Latest commit 2bc7574
🔍 Latest deploy log https://app.netlify.com/sites/care-ohc/deploys/67973b20344cb00008a98b0d
😎 Deploy Preview https://deploy-preview-9814--care-ohc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added the Deploy-Failed Deplyment is not showing preview label Jan 7, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (1)
src/hooks/useFileUpload.tsx (1)

Line range hint 132-146: Optimize image compression workflow

The 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

📥 Commits

Reviewing files that changed from the base of the PR and between e6b2596 and fbbd353.

📒 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";
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

Comment on lines 269 to 279
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;
}
}
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

Comment on lines 547 to 552
{fileUpload.files?.map((file, index) => (
<div key={index} className="flex items-center mb-2">
<CareIcon icon="l-paperclip" className="mr-2" />
{file.name}
</div>
))}
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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
package.json (1)

104-104: Consider browser-side PDF generation performance impact

Since PDF generation will happen in the browser, consider implementing the following optimizations:

  1. Add a loading state during PDF generation
  2. Use the existing browser-image-compression library to optimize images before PDF generation
  3. Consider implementing lazy loading if handling large numbers of images
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fbbd353 and f136465.

⛔ Files ignored due to path filters (1)
  • package-lock.json is 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 advisories

The 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.2 is 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

@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Jan 7, 2025
Copy link

github-actions bot commented Jan 7, 2025

👋 Hi, @DonXavierdev,
Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@github-actions github-actions bot removed the merge conflict pull requests with merge conflict label Jan 7, 2025
Copy link
Contributor

@Jacobjeevan Jacobjeevan left a comment

Choose a reason for hiding this comment

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

Can't test this yet due to permissions being restricted with encounters (and patient files isn't merged yet), so will wait till then.

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 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@Jacobjeevan Jacobjeevan Jan 13, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not relevant to the issue 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@@ -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.

@Jacobjeevan Jacobjeevan added the waiting for related PR a co-related detail PR is under construction label Jan 11, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
package.json (1)

104-104: Consider performance implications of PDF generation

Since this library will be used for merging multiple images into a PDF:

  1. Consider implementing a maximum file size limit
  2. Add client-side image compression before PDF generation
  3. Consider showing a loading indicator during PDF generation
  4. Test with various image formats (JPEG, PNG, etc.) and sizes

The existing browser-image-compression dependency (line 40) could be utilized for image optimization before PDF generation.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1c20535 and 5fe1f64.

⛔ Files ignored due to path filters (1)
  • package-lock.json is 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 compatibility

Please 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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (3)
src/hooks/useFileUpload.tsx (2)

4-4: ⚠️ Potential issue

Enhance PDF generation robustness and error handling

The PDF generation implementation needs improvements in several areas:

  1. No validation of file types before processing
  2. Hard-coded image dimensions (190, 0) may cause image distortion
  3. No progress indication during PDF generation
  4. Basic error handling without specific error messages

Previous review suggestions for improving the implementation are still valid. Additionally:

  1. Add file type validation before processing
  2. Consider memory usage for large files
  3. Add progress indication during PDF generation
  4. Improve error handling with specific error messages

Also applies to: 101-127


265-275: ⚠️ Potential issue

Improve error handling and user feedback for multiple file uploads

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

  1. No size limit checks for multiple files
  2. Silent failure if PDF generation fails
  3. 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 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

Previous review suggestions for improving the UI/UX are still valid.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5fe1f64 and 5d65539.

⛔ Files ignored due to path filters (1)
  • package-lock.json is 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,
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

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.

@github-actions github-actions bot added the question Further information is requested label Jan 22, 2025
<Checkbox
id="file_upload_patient"
checked={isPdf}
onCheckedChange={(checked) => setIsPdf(!!checked)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onCheckedChange={(checked) => setIsPdf(!!checked)}
onCheckedChange={(checked: boolean) => setIsPdf(checked)}

Copy link
Contributor

Choose a reason for hiding this comment

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

Try to think from user's perspective; when the checkbox is clicked, does it make sense to continue displaying individual textboxes for each file included?

Instead display all the files names, with only one textbox field.

Copy link
Contributor

Choose a reason for hiding this comment

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

While you are at it, go ahead and replace the TextFormField with Shadcn's Input. Feel free to ping me here or on slack if you have any questions :)

@Jacobjeevan Jacobjeevan removed question Further information is requested needs-triage labels Jan 23, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/components/Files/FilesTab.tsx (2)

599-633: Consider improving the combined PDF preview UI.

The current implementation shows a simple list of filenames. Consider enhancing the user experience by:

  1. Adding file size information
  2. Showing thumbnails for image files
  3. Adding a preview of how the combined PDF might look
 <div className="space-y-2">
   <ul className="list-disc list-inside space-y-1">
     {fileUpload.files.map((file, index) => (
-      <li key={index} className="truncate">
-        {file.name}
-      </li>
+      <li key={index} className="flex items-center gap-2">
+        <img
+          src={URL.createObjectURL(file)}
+          alt={file.name}
+          className="w-8 h-8 object-cover rounded"
+        />
+        <span className="truncate">{file.name}</span>
+        <span className="text-sm text-gray-500">
+          ({(file.size / 1024).toFixed(1)} KB)
+        </span>
+      </li>
     ))}
   </ul>
 </div>

697-697: Add loading state feedback during PDF conversion.

When isPdf is true, the upload process includes PDF conversion which might take longer. Consider adding a separate loading state or progress indicator for the conversion process.

 onClick={() => fileUpload.handleFileUpload(associatingId, isPdf)}
-disabled={fileUpload.uploading}
+disabled={fileUpload.uploading || fileUpload.converting}
 className="w-full"
 id="upload_file_button"
>
 <CareIcon icon="l-check" className="mr-1" />
-{t("upload")}
+{isPdf && fileUpload.converting ? t("converting") : t("upload")}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 677fd46 and 66ddf42.

📒 Files selected for processing (2)
  • public/locale/en.json (4 hunks)
  • src/components/Files/FilesTab.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • public/locale/en.json
🔇 Additional comments (3)
src/components/Files/FilesTab.tsx (3)

10-10: LGTM! Proper imports added.

The required imports for Checkbox and Input components from shadcn/ui have been correctly added.

Also applies to: 23-23


574-580: LGTM! Proper cleanup in dialog close handler.

The handler correctly resets the PDF state and clears files when the dialog is closed.


679-692: Verify PDF conversion capabilities.

The PDF conversion feature needs to be properly guarded:

  1. Verify device capabilities for PDF generation
  2. Add file size limits
  3. Handle conversion errors gracefully
✅ Verification successful

PDF conversion capabilities are properly guarded ✓

The codebase already implements the necessary safeguards for PDF conversion:

  • File size limit of 100MB is enforced
  • Error handling for PDF generation failures is implemented
  • File type validation is in place
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for PDF-related error handling
rg -A 5 "catch.*Error.*pdf|pdf.*error" .

# Look for file size validation
rg -A 5 "maxFileSize|file.*size.*limit" .

Length of output: 86


Script:

#!/bin/bash
# Search for PDF-related code
ast-grep --pattern 'function $_($$_) { $$$ pdf $$$ }'
ast-grep --pattern 'const $_ = async ($$$) => { $$$ pdf $$$ }'

# Search for file upload related code
rg -i "upload.*file|file.*upload" -A 5

# Search for file validation
rg -i "validate.*file|file.*validation" -A 5

# Search for any PDF-related imports or configurations
rg -i "pdf" -A 2

Length of output: 68498

Comment on lines 617 to 625
<Input
name="combined_file_name"
type="text"
id="combined-file-name"
required
value={fileUpload.fileNames[0] || ""}
disabled={fileUpload.uploading}
onChange={(e) => fileUpload.setFileName(e.target.value)}
/>
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

Add file name validation for PDF output.

The combined PDF file name input needs proper validation:

  1. Check for valid PDF file name characters
  2. Ensure .pdf extension
  3. Prevent overwriting existing files
 <Input
   name="combined_file_name"
   type="text"
   id="combined-file-name"
   required
   value={fileUpload.fileNames[0] || ""}
   disabled={fileUpload.uploading}
-  onChange={(e) => fileUpload.setFileName(e.target.value)}
+  onChange={(e) => {
+    const value = e.target.value.replace(/[^a-zA-Z0-9-_\s]/g, '');
+    fileUpload.setFileName(
+      value.endsWith('.pdf') ? value : `${value}.pdf`
+    );
+  }}
+  pattern="^[a-zA-Z0-9-_\s]+\.pdf$"
 />
📝 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
<Input
name="combined_file_name"
type="text"
id="combined-file-name"
required
value={fileUpload.fileNames[0] || ""}
disabled={fileUpload.uploading}
onChange={(e) => fileUpload.setFileName(e.target.value)}
/>
<Input
name="combined_file_name"
type="text"
id="combined-file-name"
required
value={fileUpload.fileNames[0] || ""}
disabled={fileUpload.uploading}
onChange={(e) => {
const value = e.target.value.replace(/[^a-zA-Z0-9-_\s]/g, '');
fileUpload.setFileName(
value.endsWith('.pdf') ? value : `${value}.pdf`
);
}}
pattern="^[a-zA-Z0-9-_\s]+\.pdf$"
/>

@github-actions github-actions bot added the Deploy-Failed Deplyment is not showing preview label Jan 24, 2025
@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Jan 25, 2025
Copy link

👋 Hi, @DonXavierdev,
Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/components/Files/FilesTab.tsx (1)

583-583: Consider moving the state declaration above the handler.

The state declaration should be moved above the handleDialogClose function since the handler depends on it.

+  const [isPdf, setIsPdf] = useState(false);
   const handleDialogClose = (open: boolean) => {
     if (!open) {
       setIsPdf(false);
       fileUpload.clearFiles();
     }
     onOpenChange(open);
   };
   const { t } = useTranslation();
-  const [isPdf, setIsPdf] = useState(false);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cec95d7 and eb5b3c2.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (1)
  • src/components/Files/FilesTab.tsx (5 hunks)
🔇 Additional comments (4)
src/components/Files/FilesTab.tsx (4)

10-10: LGTM!

The import of the Checkbox component from shadcn/ui is correctly added.


575-581: LGTM!

The dialog close handler properly cleans up the state by resetting isPdf and clearing files.


619-625: 🛠️ Refactor suggestion

Add file name validation for PDF output.

The combined PDF file name input needs proper validation to ensure valid characters and proper extension.

 <Input
   name="combined_file_name"
   type="text"
   id="combined-file-name"
   required
   value={fileUpload.fileNames[0] || ""}
   disabled={fileUpload.uploading}
-  onChange={(e) => fileUpload.setFileName(e.target.value)}
+  onChange={(e) => {
+    const value = e.target.value.replace(/[^a-zA-Z0-9-_\s]/g, '');
+    fileUpload.setFileName(
+      value.endsWith('.pdf') ? value : `${value}.pdf`
+    );
+  }}
+  pattern="^[a-zA-Z0-9-_\s]+\.pdf$"
 />

Likely invalid or redundant comment.


698-698: Verify PDF generation capability before upload.

The upload handler needs to verify device capabilities for PDF generation.

Comment on lines 600 to 693
<Label
htmlFor={`upload-file-name-${index}`}
className="text-sm font-medium text-gray-700"
htmlFor="upload-file-name-0"
className="block text-sm font-medium text-gray-700"
>
{t("enter_file_name")}
{t("enter_combined_file_name")}
</Label>
<Input
name={`file_name_${index}`}
name="combined_file_name"
type="text"
id={`upload-file-name-${index}`}
id="combined-file-name"
required
className={
index === 0 && fileUpload.error
? "border-red-500 focus:ring-red-500"
: "border-gray-300 focus:ring-primary"
}
value={fileUpload.fileNames[index] || ""}
value={fileUpload.fileNames[0] || ""}
disabled={fileUpload.uploading}
onChange={(e) =>
fileUpload.setFileName(e.target.value, index)
}
onChange={(e) => fileUpload.setFileName(e.target.value)}
/>
{index === 0 && fileUpload.error && (
<p className="mt-1 text-sm text-red-500">
{fileUpload.error && (
<p className="mt-2 text-sm text-red-600">
{fileUpload.error}
</p>
)}
</div>
</div>
))}
</>
) : (
fileUpload.files.map((file, index) => (
<div key={index} className="space-y-2">
<div className="flex items-center justify-between gap-2 rounded-md bg-secondary-300 px-4 py-2">
<span className="flex items-center truncate">
<CareIcon icon="l-paperclip" className="mr-2 shrink-0" />
<span className="truncate">{file.name}</span>
</span>
<Button
variant="ghost"
size="icon"
onClick={() => fileUpload.removeFile(index)}
disabled={fileUpload.uploading}
>
<CareIcon icon="l-times" />
</Button>
</div>
<div>
<Label
htmlFor={`upload-file-name-${index}`}
className="block text-sm font-medium text-gray-700"
>
{t("enter_file_name")}
</Label>

<Input
name={`file_name_${index}`}
type="text"
id={`upload-file-name-${index}`}
required
value={fileUpload.fileNames[index] || ""}
disabled={fileUpload.uploading}
onChange={(e) =>
fileUpload.setFileName(e.target.value, index)
}
/>
{index === 0 && fileUpload.error && (
<p className="mt-2 text-sm text-red-600">
{fileUpload.error}
</p>
)}
</div>
</div>
))
)}
</div>
{fileUpload.files.length > 1 && (
<div className="flex items-center gap-2 mt-4">
<Checkbox
id="file_upload_patient"
checked={isPdf}
onCheckedChange={(checked: boolean) => setIsPdf(checked)}
disabled={fileUpload.uploading}
className="cursor-pointer"
/>
<Label htmlFor="file_upload_patient" className="cursor-pointer">
{t("combine_files_pdf")}
</Label>
</div>
)}
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 validation for image files when PDF combination is enabled.

The UI correctly handles the PDF combination mode, but there's no validation to ensure only image files are selected when combining to PDF.

   {fileUpload.files.length > 1 && (
     <div className="flex items-center gap-2 mt-4">
       <Checkbox
         id="file_upload_patient"
         checked={isPdf}
-        onCheckedChange={(checked: boolean) => setIsPdf(checked)}
+        onCheckedChange={(checked: boolean) => {
+          const allImages = fileUpload.files.every(file => 
+            /\.(jpg|jpeg|png|gif|bmp|tiff)$/i.test(file.name)
+          );
+          if (checked && !allImages) {
+            fileUpload.setError(t("only_images_allowed_for_pdf"));
+            return;
+          }
+          setIsPdf(checked);
+        }}
         disabled={fileUpload.uploading}
         className="cursor-pointer"
       />
📝 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
{isPdf ? (
<>
<div className="space-y-2">
<ul className="list-disc list-inside space-y-1">
{fileUpload.files.map((file, index) => (
<li key={index} className="truncate">
{file.name}
</li>
))}
</ul>
</div>
<div>
<Label
htmlFor={`upload-file-name-${index}`}
className="text-sm font-medium text-gray-700"
htmlFor="upload-file-name-0"
className="block text-sm font-medium text-gray-700"
>
{t("enter_file_name")}
{t("enter_combined_file_name")}
</Label>
<Input
name={`file_name_${index}`}
name="combined_file_name"
type="text"
id={`upload-file-name-${index}`}
id="combined-file-name"
required
className={
index === 0 && fileUpload.error
? "border-red-500 focus:ring-red-500"
: "border-gray-300 focus:ring-primary"
}
value={fileUpload.fileNames[index] || ""}
value={fileUpload.fileNames[0] || ""}
disabled={fileUpload.uploading}
onChange={(e) =>
fileUpload.setFileName(e.target.value, index)
}
onChange={(e) => fileUpload.setFileName(e.target.value)}
/>
{index === 0 && fileUpload.error && (
<p className="mt-1 text-sm text-red-500">
{fileUpload.error && (
<p className="mt-2 text-sm text-red-600">
{fileUpload.error}
</p>
)}
</div>
</div>
))}
</>
) : (
fileUpload.files.map((file, index) => (
<div key={index} className="space-y-2">
<div className="flex items-center justify-between gap-2 rounded-md bg-secondary-300 px-4 py-2">
<span className="flex items-center truncate">
<CareIcon icon="l-paperclip" className="mr-2 shrink-0" />
<span className="truncate">{file.name}</span>
</span>
<Button
variant="ghost"
size="icon"
onClick={() => fileUpload.removeFile(index)}
disabled={fileUpload.uploading}
>
<CareIcon icon="l-times" />
</Button>
</div>
<div>
<Label
htmlFor={`upload-file-name-${index}`}
className="block text-sm font-medium text-gray-700"
>
{t("enter_file_name")}
</Label>
<Input
name={`file_name_${index}`}
type="text"
id={`upload-file-name-${index}`}
required
value={fileUpload.fileNames[index] || ""}
disabled={fileUpload.uploading}
onChange={(e) =>
fileUpload.setFileName(e.target.value, index)
}
/>
{index === 0 && fileUpload.error && (
<p className="mt-2 text-sm text-red-600">
{fileUpload.error}
</p>
)}
</div>
</div>
))
)}
</div>
{fileUpload.files.length > 1 && (
<div className="flex items-center gap-2 mt-4">
<Checkbox
id="file_upload_patient"
checked={isPdf}
onCheckedChange={(checked: boolean) => setIsPdf(checked)}
disabled={fileUpload.uploading}
className="cursor-pointer"
/>
<Label htmlFor="file_upload_patient" className="cursor-pointer">
{t("combine_files_pdf")}
</Label>
</div>
)}
{isPdf ? (
<>
<div className="space-y-2">
<ul className="list-disc list-inside space-y-1">
{fileUpload.files.map((file, index) => (
<li key={index} className="truncate">
{file.name}
</li>
))}
</ul>
</div>
<div>
<Label
htmlFor="upload-file-name-0"
className="block text-sm font-medium text-gray-700"
>
{t("enter_combined_file_name")}
</Label>
<Input
name="combined_file_name"
type="text"
id="combined-file-name"
required
value={fileUpload.fileNames[0] || ""}
disabled={fileUpload.uploading}
onChange={(e) => fileUpload.setFileName(e.target.value)}
/>
{fileUpload.error && (
<p className="mt-2 text-sm text-red-600">
{fileUpload.error}
</p>
)}
</div>
</>
) : (
fileUpload.files.map((file, index) => (
<div key={index} className="space-y-2">
<div className="flex items-center justify-between gap-2 rounded-md bg-secondary-300 px-4 py-2">
<span className="flex items-center truncate">
<CareIcon icon="l-paperclip" className="mr-2 shrink-0" />
<span className="truncate">{file.name}</span>
</span>
<Button
variant="ghost"
size="icon"
onClick={() => fileUpload.removeFile(index)}
disabled={fileUpload.uploading}
>
<CareIcon icon="l-times" />
</Button>
</div>
<div>
<Label
htmlFor={`upload-file-name-${index}`}
className="block text-sm font-medium text-gray-700"
>
{t("enter_file_name")}
</Label>
<Input
name={`file_name_${index}`}
type="text"
id={`upload-file-name-${index}`}
required
value={fileUpload.fileNames[index] || ""}
disabled={fileUpload.uploading}
onChange={(e) =>
fileUpload.setFileName(e.target.value, index)
}
/>
{index === 0 && fileUpload.error && (
<p className="mt-2 text-sm text-red-600">
{fileUpload.error}
</p>
)}
</div>
</div>
))
)}
</div>
{fileUpload.files.length > 1 && (
<div className="flex items-center gap-2 mt-4">
<Checkbox
id="file_upload_patient"
checked={isPdf}
onCheckedChange={(checked: boolean) => {
const allImages = fileUpload.files.every(file =>
/\.(jpg|jpeg|png|gif|bmp|tiff)$/i.test(file.name)
);
if (checked && !allImages) {
fileUpload.setError(t("only_images_allowed_for_pdf"));
return;
}
setIsPdf(checked);
}}
disabled={fileUpload.uploading}
className="cursor-pointer"
/>
<Label htmlFor="file_upload_patient" className="cursor-pointer">
{t("combine_files_pdf")}
</Label>
</div>
)}

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (2)
src/hooks/useFileUpload.tsx (2)

103-135: 🛠️ Refactor suggestion

⚠️ Potential issue

Add essential safeguards to PDF generation.

The PDF generation needs additional safeguards:

  1. Validate file types before starting PDF generation
  2. Add size limit checks for multiple files
  3. Handle memory constraints

Additionally, the caught error in the catch block is unused. Add proper error logging.

   } catch (error) {
+    console.error('PDF generation failed:', error);
     toast.error(t("file_error__generate_pdf"));
     setError(t("file_error__generate_pdf"));
     setProgress(0);
     return null;
   }
🧰 Tools
🪛 eslint

[error] 129-129: 'error' is defined but never used. Allowed unused caught errors must match /^_/u.

(@typescript-eslint/no-unused-vars)

🪛 GitHub Actions: Deploy Care Fe

[error] 129-129: 'error' is defined but never used. Allowed unused caught errors must match /^_/u


295-304: 🛠️ Refactor suggestion

Revise the PDF generation workflow.

Several issues with the current implementation:

  1. As per previous discussions, combining any multi-uploads is not the desired behavior. Add a specific UI option for PDF generation.
  2. Clearing all files on PDF generation failure is too aggressive. Consider preserving the original files.
  3. Missing progress indication during PDF generation.
🧹 Nitpick comments (1)
src/hooks/useFileUpload.tsx (1)

55-58: Follow TypeScript naming conventions.

The parameter CombineToPDF should be in camelCase as per TypeScript naming conventions.

   handleFileUpload: (
     associating_id: string,
-    CombineToPDF?: boolean,
+    combineToPdf?: boolean,
   ) => Promise<void>;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eb5b3c2 and 0c77702.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (2)
  • package.json (1 hunks)
  • src/hooks/useFileUpload.tsx (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • package.json
🧰 Additional context used
🪛 eslint
src/hooks/useFileUpload.tsx

[error] 129-129: 'error' is defined but never used. Allowed unused caught errors must match /^_/u.

(@typescript-eslint/no-unused-vars)

🪛 GitHub Actions: Deploy Care Fe
src/hooks/useFileUpload.tsx

[error] 129-129: 'error' is defined but never used. Allowed unused caught errors must match /^_/u

🔇 Additional comments (2)
src/hooks/useFileUpload.tsx (2)

4-4: Add jspdf to build configuration.

The jspdf import is causing pipeline failures. Add it to build.rollupOptions.external.


116-116: 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

♻️ Duplicate comments (3)
src/hooks/useFileUpload.tsx (3)

4-4: ⚠️ Potential issue

Add jspdf to build configuration.

The jspdf import needs to be added to build.rollupOptions.external to resolve pipeline failures.


103-135: 🛠️ Refactor suggestion

Enhance PDF generation with essential safeguards.

The PDF generation needs several improvements for robustness:

  1. Validate file types upfront before starting the process
  2. Add size limit checks for multiple files
  3. Calculate image dimensions based on aspect ratio
  4. Improve memory management

Additionally, the caught error variable should follow the naming convention:

-    } catch (error_) {
+    } catch (_error) {
🧰 Tools
🪛 eslint

[error] 129-129: 'error_' is defined but never used. Allowed unused caught errors must match /^_/u.

(@typescript-eslint/no-unused-vars)

🪛 GitHub Actions: Deploy Care Fe

[error] 129-129: 'error_' is defined but never used. Allowed unused caught errors must match /^_/u


286-304: 🛠️ Refactor suggestion

Revise the PDF generation workflow.

Several issues with the current implementation:

  1. As per previous discussions, combining any multi-uploads is not the desired behavior
  2. Clearing all files on PDF generation failure is too aggressive
  3. Missing progress indication during PDF generation

Additionally, maintain naming consistency:

   const handleUpload = async (
     associating_id: string,
-    CombineToPDF?: boolean,
+    combineToPdf?: boolean,
   ) => {
🧹 Nitpick comments (1)
src/hooks/useFileUpload.tsx (1)

55-58: Follow TypeScript naming conventions.

The parameter CombineToPDF should follow camelCase naming convention.

   handleFileUpload: (
     associating_id: string,
-    CombineToPDF?: boolean,
+    combineToPdf?: boolean,
   ) => Promise<void>;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0c77702 and e59ef78.

📒 Files selected for processing (1)
  • src/hooks/useFileUpload.tsx (4 hunks)
🧰 Additional context used
🪛 eslint
src/hooks/useFileUpload.tsx

[error] 129-129: 'error_' is defined but never used. Allowed unused caught errors must match /^_/u.

(@typescript-eslint/no-unused-vars)

🪛 GitHub Actions: Deploy Care Fe
src/hooks/useFileUpload.tsx

[error] 129-129: 'error_' is defined but never used. Allowed unused caught errors must match /^_/u

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/hooks/useFileUpload.tsx (1)

55-58: Use consistent camelCase naming.

The parameter CombineToPDF should follow JavaScript/TypeScript naming conventions and be renamed to combineToPdf.

   handleFileUpload: (
     associating_id: string,
-    CombineToPDF?: boolean,
+    combineToPdf?: boolean,
   ) => Promise<void>;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e59ef78 and 84d9bd7.

📒 Files selected for processing (1)
  • src/hooks/useFileUpload.tsx (4 hunks)
🔇 Additional comments (3)
src/hooks/useFileUpload.tsx (3)

4-4: Add jspdf to build configuration.

The jspdf import is causing pipeline failures. Add it to build.rollupOptions.external.


295-304: 🛠️ Refactor suggestion

Improve error handling and state management.

The current implementation has several issues:

  1. Clearing all files on PDF generation failure is too aggressive
  2. Direct array mutation with splice should be avoided
  3. Missing progress indication during PDF generation

Apply this enhancement:

-    if (CombineToPDF && files.length > 1) {
+    if (combineToPdf && files.length > 1) {
+      setProgress(-1); // Indeterminate progress during PDF generation
       const pdfFile = await generatePDF(files);
+      setProgress(0);
       if (pdfFile) {
-        files.splice(0, files.length, pdfFile);
+        setFiles([pdfFile]);
+        setUploadFileNames(['combined.pdf']);
       } else {
-        clearFiles();
         setError(t("file_error__generate_pdf"));
         return;
       }
     }

Likely invalid or redundant comment.


103-135: 🛠️ Refactor suggestion

Improve PDF generation robustness and performance.

Several improvements needed:

  1. Validate file types upfront before starting the PDF generation
  2. Add proper memory management
  3. Respect image aspect ratios
  4. Add size limit checks

Apply this enhancement:

 const generatePDF = async (files: File[]): Promise<File | null> => {
   try {
+    const totalSize = files.reduce((sum, file) => sum + file.size, 0);
+    const MAX_TOTAL_SIZE = 50 * 1024 * 1024; // 50MB
+    if (totalSize > MAX_TOTAL_SIZE) {
+      toast.error(t("file_error__total_size"));
+      setError(t("file_error__total_size"));
+      return null;
+    }
+    
+    if (!files.every(file => file.type.startsWith("image/"))) {
+      toast.error(t("file_error__file_type"));
+      setError(t("file_error__file_type"));
+      return null;
+    }
+    
     toast.info(t("file_conversion_in_progress"));
     const pdf = new jsPDF();
     const totalFiles = files.length;
+    const urls: string[] = [];
 
     for (const [index, file] of files.entries()) {
-      if (!file.type.startsWith("image/")) {
-        toast.error(t("file_error__file_type"));
-        setProgress(0);
-        return null;
-      }
       const imgData = URL.createObjectURL(file);
-      pdf.addImage(imgData, "JPEG", 10, 10, 190, 0);
-      URL.revokeObjectURL(imgData);
+      urls.push(imgData);
+      
+      // Load image to calculate dimensions
+      const img = new Image();
+      await new Promise((resolve, reject) => {
+        img.onload = resolve;
+        img.onerror = reject;
+        img.src = imgData;
+      });
+      
+      const aspectRatio = img.height / img.width;
+      const pageWidth = pdf.internal.pageSize.getWidth() - 20; // 10px margin
+      const pageHeight = pageWidth * aspectRatio;
+      
+      pdf.addImage(imgData, "JPEG", 10, 10, pageWidth, pageHeight);
       if (index < files.length - 1) pdf.addPage();
+      
       const progress = Math.round(((index + 1) / totalFiles) * 100);
       setProgress(progress);
     }
+    
     const pdfBlob = pdf.output("blob");
     const pdfFile = new File([pdfBlob], "combined.pdf", {
       type: "application/pdf",
     });
     setProgress(0);
     toast.success(t("file_conversion_success"));
     return pdfFile;
   } catch (error) {
     toast.error(t("file_error__generate_pdf"));
     setError(t("file_error__generate_pdf", { error: String(error) }));
     setProgress(0);
     return null;
   } finally {
+    // Clean up URLs
+    urls.forEach(URL.revokeObjectURL);
   }
 };

Likely invalid or redundant comment.

Comment on lines 286 to 304
const handleUpload = async (
associating_id: string,
CombineToPDF?: boolean,
) => {
if (!validateFileUpload()) return;

setProgress(0);
const errors: File[] = [];

if (CombineToPDF && files.length > 1) {
const pdfFile = await generatePDF(files);
if (pdfFile) {
files.splice(0, files.length, pdfFile);
} else {
clearFiles();
setError(t("file_error__generate_pdf"));
return;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

PDF combination needs explicit user control

The verification confirms that the current implementation only uses a boolean parameter without any dedicated UI controls for users to explicitly choose PDF combination. This validates the original concern about automatic PDF generation.

  • In src/hooks/useFileUpload.tsx, add UI controls (checkbox/toggle) to let users explicitly opt-in for PDF combination
  • Update the component's interface to expose this user preference
  • Consider adding a clear label explaining the PDF combination feature to users
🔗 Analysis chain

Verify the PDF generation approach.

As per previous discussions, automatically combining any multi-uploads into PDF wasn't the desired behavior. Instead, consider adding a specific UI option (like a checkbox) to trigger PDF generation only when explicitly requested by the user.

Run this script to check the current UI implementation:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check how the PDF generation option is presented in the UI
echo "Searching for PDF-related UI components..."
rg -A 10 "CombineToPDF|combineToPdf" --type tsx

Length of output: 172


Script:

#!/bin/bash
# Search for PDF-related UI components and their usage
echo "Searching for PDF-related UI components in TypeScript/React files..."
rg -t ts -t tsx -t js -t jsx -A 10 "CombineToPDF|combineToPdf|combinePdf"

# Also search for any PDF-related UI elements like checkboxes or toggles
echo -e "\nSearching for PDF-related UI elements..."
rg -t ts -t tsx -t js -t jsx "PDF.*(?:checkbox|toggle|switch|radio|select)" -A 5

Length of output: 458


Script:

#!/bin/bash
# Find TypeScript/React files and search for PDF-related code
echo "Searching for PDF-related components..."
fd -e ts -e tsx -e js -e jsx | xargs grep -l -i "combinetopdf\|pdf.*checkbox\|pdf.*toggle\|pdf.*switch" || true

# Show the content of files that contain matches
echo -e "\nShowing content of matching files:"
fd -e ts -e tsx -e js -e jsx | xargs grep -A 5 -B 5 -i "combinetopdf\|pdf.*checkbox\|pdf.*toggle\|pdf.*switch" || true

Length of output: 1956

@github-actions github-actions bot removed the merge conflict pull requests with merge conflict label Jan 25, 2025
@DonXavierdev
Copy link
Contributor Author

@Jacobjeevan Can you check the latest changes and let me know if theres anything more to add.

src/components/Files/FilesTab.tsx Outdated Show resolved Hide resolved
src/components/Files/FilesTab.tsx Outdated Show resolved Hide resolved
src/components/Files/FilesTab.tsx Outdated Show resolved Hide resolved
src/components/Files/FilesTab.tsx Outdated Show resolved Hide resolved
src/hooks/useFileUpload.tsx Show resolved Hide resolved
src/hooks/useFileUpload.tsx Outdated Show resolved Hide resolved
@DonXavierdev
Copy link
Contributor Author

@Jacobjeevan Done with the latest changes

<div className="space-y-2">
<ul className="list-disc list-inside space-y-1">
Copy link
Contributor

Choose a reason for hiding this comment

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

Add the paperclip + background to maintain consistent behavior. Do a self-review, check against existing behavior:

image

const handleDialogClose = (open: boolean) => {
if (!open) {
setIsPdf(false);
fileUpload.clearFiles();
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole block is unnecessary. Clearing files is already handled in parent component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to include setIsPdf(false) there because of this scenario:

If a user enables the checkbox to combine files into a PDF and then closes the dialog box, the isPdf state will remain true. When the user reopens the dialog, the checkbox will still be enabled, which isn’t an issue since they can manually disable it if needed.

However, if the user later selects only a single file and reopens the dialog, the checkbox won't be displayed (because it only appears for multiple files). In this case, the isPdf state will still be true, and the single file will be processed as a PDF due to the leftover state. This would create confusion, as the user has no way to disable the "combine to PDF" behavior in this situation.

Let me know if there is a workaround . I will remove the Clearing files part anyway.

@Jacobjeevan
Copy link
Contributor

Jacobjeevan commented Jan 27, 2025

Also you don't need to tag me and request for review; either one is enough. At this point, I'll get email updates to this thread regardless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes required Deploy-Failed Deplyment is not showing preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File Upload enhancement- multi-file support
4 participants