search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.57k stars 2.91k forks source link

[$250] Web - [AU] Attachment - Console errors appear when uploading a PDF attachment to a 1:1 DM #51313

Open IuliiaHerets opened 1 month ago

IuliiaHerets commented 1 month ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 9.0.52-5 Reproducible in staging?: Y Reproducible in production?: Y *If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5120680&group_by=cases:section_id&group_id=292107&group_order=asc Issue reported by:** Applause Internal Team

Action Performed:

  1. Navigate to https://staging.new.expensify.com/ and open Safari console
  2. Log in with a Gmail account
  3. Open any 1:1 DM
  4. Navigate to + button - Add attachment
  5. Send any PDF attachment

Expected Result:

There shouldn't be any console errors.

Actual Result:

Console errors appear when uploading a PDF attachment to a 1:1 DM.

Workaround:

Unknown

Platforms:

Screenshots/Videos

Bug6642816_1729659401962!Capture 2310.txt

https://github.com/user-attachments/assets/06963836-08f7-4bed-8154-93573665e2dd

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021851679624000395857
  • Upwork Job ID: 1851679624000395857
  • Last Price Increase: 2024-11-20
Issue OwnerCurrent Issue Owner: @allgandalf
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @abekkala (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

IuliiaHerets commented 1 month ago

@abekkala FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

melvin-bot[bot] commented 3 weeks ago

@abekkala Eep! 4 days overdue now. Issues have feelings too...

abekkala commented 3 weeks ago

I was able to reproduce in Safari

melvin-bot[bot] commented 3 weeks ago

Job added to Upwork: https://www.upwork.com/jobs/~021851679624000395857

melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif (External)

melvin-bot[bot] commented 2 weeks ago

@abekkala, @aimane-chnaif Huh... This is 4 days overdue. Who can take care of this?

aimane-chnaif commented 2 weeks ago

No proposals yet

aimane-chnaif commented 2 weeks ago

I am out sick at the moment. Please reassign C+ if needed

melvin-bot[bot] commented 2 weeks ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

VickyStash commented 2 weeks ago

Hi, I'm Viktoryia from Callstack - expert contributor group - and I would like to work on this issue.

VickyStash commented 2 weeks ago

Updates as for today:

image

I'll continue my investigations tomorrow

VickyStash commented 2 weeks ago

Updates: I was able to identify the place which triggers the second part of the errors. The problem is in the attachment optimistic report action, specifically in the attachment source attribute which is passed in the data attributes to the pdf attachment: https://github.com/Expensify/App/blob/346bb370add9c7225bec8b5b499a1aab93476dec/src/libs/ReportUtils.ts#L4307 And the way it's used for the report action display. I still need to find the way to handle it without errors, I haven't had enough time for it today unfortunately. After that I should be able to combine formal proposal.

melvin-bot[bot] commented 1 week ago

@abekkala Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @allgandalf (External)

VickyStash commented 1 week ago

I was OOO yesterday. I'm not sure I'll have enough time today to post the proposal, but I think it should be possible tomorrow.

allgandalf commented 1 week ago

Thanks for the update :)

wildan-m commented 1 week ago

Is this issue available for proposals or has it already been assigned?

wildan-m commented 1 week ago

No response. Assuming the task has been assigned, it's best to remove the help wanted tag if that is the case.

VickyStash commented 1 week ago

Okay, I've done more in-depth research about the second part of the console errors - the one that appears when you send the PDF attachment. And it's turned out:

  1. There is a similar issue opened https://github.com/Expensify/App/issues/46615, and it's held till https://github.com/Expensify/App/issues/9402 is resolved.
  2. There is an open Draft PR created by @kidroca , which seems to fix that part of the errors.

So maybe we should skip this part in favor of mentioned about issues/PRs. @allgandalf @abekkala

Regarding the first part of the console errors - the one that appears during selected pdf file preview in the Safari web. As I mentioned above, changing the pdfWorkerSource import to be imported not from pdfjs-dist/legacy/build/pdf.worker.mjs but from pdfjs-dist/build/pdf.worker.mjs in both E/App and react-fast-pdf lib fixes the errors.

But I've noticed that @hurali97 changed it from non-legacy to legacy in one of his PR some time ago. Was there any specific reason for it? Should we be aware of something if decide to update the pdfWorkerSource?

melvin-bot[bot] commented 1 week ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

allgandalf commented 1 week ago

Thanks for the summary @VickyStash , I looked the linked issue of the PR, there is a short description there:

Screenshot 2024-11-13 at 11 43 53 PM

maybe this adds a bit info, now lets wait for @hurali97 for some inputs from his side, I don't see them on Slack, but maybe you can bump them internally for faster response , thanks

VickyStash commented 1 week ago

I've confirmed internally that updating the pdfWorkerSource was out of the scope of the mentioned PR, so it was decided not to change it at that point. So, I guess there are no blockers as for now.

VickyStash commented 1 week ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Console errors appear:

  1. During pdf file attachment preview in the Safari web browser. image

  2. During pdf file attachment sending, for optimistically generated report action. image

What is the root cause of that problem?

  1. Legacy PDF worker source usage in both
  1. The file attached in the optimistic report action isn't considered as a local file in the app. The app always adds the encrypted authToken to it, which later affects the file loading and display. https://github.com/Expensify/App/blob/512ae9f1a1c6d840065f377264bc1ab009356f0f/src/components/AnchorForAttachmentsOnly/BaseAnchorForAttachmentsOnly.tsx#L26

What changes do you think we should make in order to solve the problem?

  1. Update pdfWorkerSource in both E/App and react-fast-pdf from pdfjs-dist/legacy/build/pdf.worker.mjs to pdfjs-dist/build/pdf.worker.mjs.

  2. Ignore this part of the console errors in this issue, since there are several issues and PR that are handling it:

allgandalf commented 1 week ago

okay cool, this makes sense to me, i will get and internal engineer assigned for final though and then we can probably get a PR up next week 🤝

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @tylerkaraszewski, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] commented 4 days ago

@tylerkaraszewski, @abekkala, @VickyStash, @allgandalf Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

VickyStash commented 4 days ago

@tylerkaraszewski Could you take a look at the proposal?

tylerkaraszewski commented 4 days ago

That seems OK to me.

allgandalf commented 4 days ago

dope!, @VickyStash lets get the PR going 💪

VickyStash commented 3 days ago

Updates: I've started to work on the PR but unfortunately, I've noticed some issues in the react-fast-pdf lib after getting rid of legacy. I'm going to take a closer look tomorrow to make sure all works correctly.

melvin-bot[bot] commented 2 days ago

@tylerkaraszewski @abekkala @VickyStash @allgandalf this issue is now 4 weeks old, please consider:

Thanks!

allgandalf commented 2 days ago

we are working through it melv!

melvin-bot[bot] commented 2 days ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

VickyStash commented 2 days ago

I'll provide the updates in 1-2h!

VickyStash commented 2 days ago

Updates: Today, I've decided to start with the reproduction of the console errors in the example app in the react-fast-pdf lib and fixing it there.

  1. The difference between pdfWorkerSource legacy and not legacy is the supported browsers: https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#which-browsersenvironments-are-supported While legacy version supports outdated browsers it can have some minor issues. Do we want to keep the legacy version for outdated browsers compatibility?

  2. The example app in the react-fast-pdf had the non existing path for the worker inside webpack config. Due to this the fake worker always was used, it confused me a little during the testing. image

I've corrected the path to be the same as in the code and started to see the same console errors as in the E/App:

https://github.com/user-attachments/assets/d835dcf8-2d23-4cab-bc39-c9615324a2a6

  1. After some experiments it's turned out it's fixed by using pdf.worker.min.mjs as a worker source:

image

The updates can be found in this branch.

VickyStash commented 1 day ago

Updates: I've opened the PR for the react-fast-pdf lib.

VickyStash commented 8 hours ago

No updates for today, I'll continue to work on it next week.