[$500] PDF receipt preview loads each time visiting 1:1 DM, report and transaction thread

kavimuru commented 7 months 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: 1.4.48-0 Reproducible in staging?: y Reproducible in production?: n If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause internal team Slack conversation:

Action Performed:

Go to staging.new.expensify.com
Go offline.
Go to FAB > Request money > Scan.
Select a PDF receipt.
Select any participant and create the Scan request.
in 1:1 DM, click on the scan preview.
In IOU report, click on the scan preview.
Click on the header subtitle to return to report and 1:1 DM.

Expected Result:

The PDF receipt preview will not load each time going back and forth between 1:1 DM, IOU report and transaction thread.

Actual Result:

The PDF receipt preview loads each time going back and forth between 1:1 DM, IOU report and transaction thread. Sometimes it shows blue "loading page" briefly when the receipt is rendering.

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android: Native
[x] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/43996225/885644dc-0d8b-43ab-86a3-b288dfe8d92d

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~016df3671b30e63c80
Upwork Job ID: 1806060107499225786
Last Price Increase: 2024-07-24

Issue Owner

Current Issue Owner: @

github-actions[bot] commented 7 months ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

melvin-bot[bot] commented 7 months ago

Triggered auto assignment to @anmurali (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 7 months ago

Triggered auto assignment to @deetergp (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

kavimuru commented 7 months ago

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

deetergp commented 6 months ago

Is this our culprit PR? https://github.com/Expensify/App/pull/35255 (cc @eh2077 @s77rt @luacmartins)

youssef-lr commented 6 months ago

We have just added support for PDF receipts, so I'm not sure this would qualify as a deploy blocker because we don't support PDF receipts in production right now.

deetergp commented 6 months ago

Thanks @youssef-lr, that sounds reasonable. I'll remove the label and demote this back down to Daily.

s77rt commented 6 months ago

@eh2077 Can you please take a quick look on this?

eh2077 commented 6 months ago

@s77rt Sure. I'm looking into it.

eh2077 commented 6 months ago

It seems not easy to avoid reloading PDF when going back and forth between 1:1 DM, IOU report and transaction thread. This is because the browser by default can't cache the canvas of the PDFThumbnail but it's able to cache image by default.

While checking this issue, I also found another flaw that's easier to avoid - PDF thumbnail reloads when hovering on it

https://github.com/Expensify/App/assets/117511920/df575660-eba5-4a00-9dc1-08f9e0fda1dd

eh2077 commented 6 months ago

I think the reloading issue when hovering is a legit regression. I can make a quick PR to fix it.

But the reloading issue when re-visiting different pages seems a tricky one. Can this be a followup improvement instead of a regression?

@s77rt @luacmartins What're your opinions?

s77rt commented 6 months ago

@eh2077 Can you please raise a PR fixing the hover issue? I thought we fixed that here https://github.com/Expensify/App/pull/35255#discussion_r1469048195 Is this another case that we missed?

luacmartins commented 6 months ago

Yea, let's fix the hover issue and then investigate the reload one as a separate issue.

s77rt commented 6 months ago

@deetergp Please assign me here

melvin-bot[bot] commented 6 months ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 6 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.51-3 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

https://github.com/Expensify/App/pull/38031

If no regressions arise, payment will be issued on 2024-03-20. :confetti_ball:

For reference, here are some details about the assignees on this issue:

@s77rt requires payment (Needs manual offer from BZ)

melvin-bot[bot] commented 6 months ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[ ] [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@s77rt] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[ ] [@s77rt] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[ ] [@s77rt] Determine if we should create a regression test for this bug.
[ ] [@s77rt] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[ ] [@anmurali] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

s77rt commented 6 months ago

The PR that introduced the bug has been identified: https://github.com/Expensify/App/pull/35255
The offending PR has been commented on: N/A (Bug fixed by the author)
A discussion in #expensify-bugs has been started: Not needed.
Determine if we should create a regression test for this bug: No

s77rt commented 6 months ago

@luacmartins / @deetergp We still have the bug when switching reports; let's reopen this since we only fixed the hover case.

melvin-bot[bot] commented 6 months ago

Payment Summary

[Upwork Job]()

ROLE: @s77rt paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@anmurali)

[ ] I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
[ ] I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
[ ] I have paid out the Upwork contracts or cancelled the ones that are incorrect
[ ] I have verified the payment summary above is correct

anmurali commented 6 months ago

I think we should wait till the bug is fixed on switching reports as well before we pay this. Do you agree @s77rt ?

s77rt commented 6 months ago

Yes. In fact we should reopen this. Can you or @deetergp add Help Wanted label, remove Awaiting Payment and update the title

deetergp commented 6 months ago

Should be all set @s77rt.

deetergp commented 6 months ago

Going to make this a weekly till we have some proposals.

deetergp commented 6 months ago

Still waiting on proposals

ghost commented 5 months ago

Proposal

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

PDF receipt preview loads each time visting DM, report and transaction thread.

What is the root cause of that problem?

The react-pdf document is loading url using the file prop. According to react-pdf,

({ url: 'http://example.com/document.pdf' }) === ({ url: 'http://example.com/document.pdf' })
false

When the expected would have been true.

react-pdf takes the file as referentially unequal each time the file prop is set to previewSourceURL and causes re-render. https://github.com/Expensify/App/blob/b9bd07fcbf13ad517a0b42b5a4dc4abafefb91f7/src/components/PDFThumbnail/index.tsx#L22

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

Since only the thumbnail preview of the first page is needed and image receipt previews are loading correctly. I suggest to convert the first page of the pdf receipt to image for previewing purposes and render the first page as image rather than rendering the first page as pdf. The pdf approach would be difficult given the contraints posed by react-pdf.

The approach uses helper method to get the first page of the pdf receipt and render to canvas element and then get the DataURL (img in base64 format). The base64 img is stored in localstorage and uses fileName as key to the base64 img.

The PDFThumbnail component would need fileName (previewSourceURL) of the pdf receipt for mapping in localStorage. Using URl.createObjectURL returns different url each time which causes re-render so need to map using fileName. https://github.com/Expensify/App/blob/b9bd07fcbf13ad517a0b42b5a4dc4abafefb91f7/src/components/PDFThumbnail/index.tsx#L15

    const isImageLoadedRef = useRef(false);
    const [imageUrl, setImageUrl] = useState<string>(null);
    const data = localStorage.getItem(fileName);
    if(data && !isImageLoadedRef.current)
    {
        isImageLoadedRef.current = true;
        setImageUrl(data);
    }
    useEffect(() => {
        if (data) {
            return;
        }
        const canvas = document.createElement("canvas");
        pdfjs.getDocument(url).promise.then((pdf) => {
            for (let i = 0; i < 1; i++) {
                pdf.getPage(i + 1).then((page) => {
                    const viewport = page.getViewport({ scale: 5 });
                    const context = canvas.getContext("2d");
                    canvas.height = viewport.height;
                    canvas.width = viewport.width;
                    var renderContext = {
                        canvasContext: context,
                        viewport: viewport
                    };

                    page.render(renderContext).promise.then((e) => {
                        const data = canvas.toDataURL();
                        localStorage.setItem(fileName, data);
                        isImageLoadedRef.current = true;
                        setImageUrl(data);
                        canvas.remove();
                    });
                })
            }
        });
    }, [data])

In the PDFThumbnail component instead of loading react-pdf component load ReceiptImage component and pass the base64 imageURL. https://github.com/Expensify/App/blob/b9bd07fcbf13ad517a0b42b5a4dc4abafefb91f7/src/components/PDFThumbnail/index.tsx#L40

    {imageUrl &&
        <ReceiptImage
            isThumbnail={false}
            source={imageUrl}
            // AuthToken is required when retrieving the image from the server
            // but we don't need it to load the blob:// or file:// image when starting a money request / split bill
            // So if we have a thumbnail, it means we're retrieving the image from the server
            isAuthTokenRequired={isAuthTokenRequired}
        />
    }

What alternative solutions did you explore? (Optional)

s77rt commented 5 months ago

@jsdev2547 Thanks for the proposal. Regarding the RCA

The react-pdf document is loading url using the file prop. According to react-pdf,
({ url: 'http://example.com/document.pdf' }) === ({ url: 'http://example.com/document.pdf' })
false

Can you link to the relevant code? If the bug is in react-pdf then it should be fixed upstream

ghost commented 5 months ago

@s77rt I downgraded to react-pdf version 6.2.2 and the pdf thumbnail is still reloading when navigation from DM to report and transaction thread. I also checked if the route navigation was causing re-render when navigating from DM to report and transaction thread, but when image receipt is used even though the route navigates in reportPreview and MoneyRequestAction no reloading issue is observed, but the reloading issue is present when using pdf thumbnails. So the issue is definitely the third party lib react-pdf. But the important thing is not to try and solve the bug in third party lib but to correct approach of displaying pdf thumbnails.

Route navigation https://github.com/Expensify/App/blob/8f90c2567f020ed924a96593244727aace9a7d72/src/components/ReportActionItem/ReportPreview.tsx#L257

https://github.com/Expensify/App/blob/8f90c2567f020ed924a96593244727aace9a7d72/src/components/ReportActionItem/MoneyRequestAction.tsx#L96

More so what I have observed is the correct approach to solving the issue of pdf thumbnails is to generate thumbnail of pdf by rendering the pdf page to canvas and then converting the canvas to image using toDataURL() for preview.

pdfjs in the provided example of pdf thumbnail has suggested to convert the canvas to image and then display the image, rather than displaying the pdf page directly.

https://github.com/mozilla/pdf.js/blob/master/web/pdf_thumbnail_view.js#L216

s77rt commented 5 months ago

@jsdev2547 Let's fix the root cause. Rendering pdf then converting to image is more work than just rendering the pdf.

ghost commented 5 months ago

@s77rt The pdf would not be rendered, i.e. the react-pdf document component would not be used in pdf thumbnail. The poc is quite straight forward, get page 1 of pdf, render on canvas, get the image from canvas. The root cause would be to not to render pdf thumbnails as pdf but to render pdf thumbnails as image. I was trying to actually use pdfjs to render pdf thumbnail but then came across pdf_thumbnail example which suggests to convert pdf to thumbnail rather use pdf directly.

I have poc video of pdf image thumbnails in pdf receipts and how the functionality appears, or I can also provide branch, if needed.

deetergp commented 5 months ago

@jsdev2547 I'm pretty sure our back end creates a thumbnail of these uploaded PDFs automagically. If you replace the .pdf extension of the uploaded PDF with .jpg there you should have a JPG rendering of the fist page of the PDF. And if you add .320.jpg or .1024.jpg on to that you'll have 320px & 1024px thumbnails.

You may not have access to actually view these but I guarantee you they are there: https://staging.expensify.com/receipts/w_edee87a4d07915c51e6b58ed256da890177f2a3c.pdf https://staging.expensify.com/receipts/w_edee87a4d07915c51e6b58ed256da890177f2a3c.jpg https://staging.expensify.com/receipts/w_edee87a4d07915c51e6b58ed256da890177f2a3c.jpg.320.jpg https://staging.expensify.com/receipts/w_edee87a4d07915c51e6b58ed256da890177f2a3c.jpg.1024.jpg

s77rt commented 5 months ago

@jsdev2547

get page 1 of pdf, render on canvas, get the image from canvas

This is more work than what we have currently in place where we just render the pdf on canvas.

s77rt commented 5 months ago

@deetergp When offline we don't have access to those urls. We render the pdf instead

ghost commented 5 months ago

@deetergp Interesting, then the conversion step is already done only the thumbnail needs displaying.

ghost commented 5 months ago

@s77rt the issue is regarding the thumbnail reloading not offline, so @deetergp might have point on using the urls provided by server. I need to look into and get back.

ghost commented 5 months ago

@deetergp @s77rt The urls become available after the scan request is complete and the urls are available offline and online. The issue is regarding reloading of the pdf which happens either online or offline while the user is waiting for the scan request to complete. Once the scan request is complete the backend sends the thumbnail of the first page of the pdf and the pdfthumbnail component is no longer used instead the pdf image thumbnail is rendered using thumbnail component. The isPDFThumbnail variable is also set to false for pdf receipts and shouldUseThumbnailImage is set to true.

https://github.com/Expensify/App/blob/d4bc1b08daa1890829461ab464ef4894ce343d06/src/components/ReceiptImage.tsx#L113

The backend already has the correct approach of sending thumbnails of first page of pdf receipts after scan is complete, while the user is waiting for the scan to complete(online or offline), the front end should also correct the approach and generate image thumbnail of pdf. The thumbnail images generated on the front end would resolve the reloading issue and be available (online or offline) while the user is waiting for scan request to complete. Once the scan request is complete, the thumbnail would be served from the backend.

So what needs to be decided is to whether to push for and continue the use of pdfs in thumbnails while the user is waiting for the scan to complete, or whether the front end should be consistent with the backend and generate thumbnails for pdf while the scan is in progress. I vote for generating thumbnail of first page pdf on the front end because the effort is inlined with the backend and not hard to implement. I already having functional solution which solves the issue (I can provide video and branch for testing). Trying to get the pdf to directly render in thumbnail(also spending time to find bug in react-pdf) seems to be unnecessary effort when the server would eventually return thumbnails. I would have agreed more with the second approach if the backend was not sending thumbnails.

s77rt commented 5 months ago

@jsdev2547 Thanks for looking into this. Generating an image has to pass via the pdf rendering phase. This is not giving us any advantage and it's only resulting in more work. We should just fix the pdf rendering phase to not re-render if the source is not changed.

ghost commented 5 months ago

Generating an image has to pass via the pdf rendering phase.

@s77rt The current implementation is going to load the pdf document anyways on first render, the entire image generation can be done in onLoadSuccess event and is not difficult to implement. Please take moment to review the implementation and test the provided branch.

https://github.com/jsdev2547/App/tree/generatePdfThumbnail

Also added poc video,

poc.webm

We should just fix the pdf rendering phase to not re-render if the source is not changed.

Typically the right approach would be to correct the useEffect to not re-render if the source has not changed, but in the given issue, the useEffect is in third party lib and also I have concerns the approach of rendering pdf directly for thumbnails is incorrect. react-pdf is using pdf.js and in the pdf.js example I mentioned is also converting first page to image thumbnail and also the backend is generating image thumbnail. Also I find little advantage in trying to display the pdf thumbnail when after the scan is complete the backend would be sending image thumbnails anyways. so the correct approach should be to simply display image thumbnails (online and offline) while the user is waiting for the scan to complete.

@deetergp perhaps internal engineer can be requested to get second opinion on whether pdf should be used for thumbnails while the user is waiting for scan to complete (online & offline) or whether front end should generate image thumbnails and after the scan is complete display the thumbnails sent by backend.

s77rt commented 5 months ago

@jsdev2547

The current implementation is going to load the pdf document anyways on first render

Correct, and that should be all what we have to do here. No post-processing.

the entire image generation can be done in onLoadSuccess event and is not difficult to implement

This is workaround, we are doing more work just to avoid fixing the root cause. We should fix the root cause

but in the given issue, the useEffect is in third party lib

This is not a blocker. We should raise a PR in that library

s77rt commented 5 months ago

Not overdue. Still looking for proposals

s77rt commented 5 months ago

Same

deetergp commented 5 months ago

Making weekly while await additional proposals.

s77rt commented 5 months ago

Still looking for proposals

s77rt commented 4 months ago

Same ^

deetergp commented 4 months ago

MOTS

s77rt commented 4 months ago

Still looking for proposals

s77rt commented 3 months ago

Same ^

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

Current assignee @s77rt is eligible for the External assigner, not assigning anyone new.

deetergp commented 3 months ago

Still no proposals, but that may be because there wasn't an External label on this one. Added it now.

Expensify / App