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
4.02k stars 3.01k forks source link

[$250] Attachments-PDF preview loads with delay, the grey line shown first #55671

Open IuliiaHerets opened 2 weeks ago

IuliiaHerets commented 2 weeks 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.89-2 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/cases/view/2654655 Issue reported by: Applause Internal Team Device used: MacBook Air 14.1 Chrome App Component: Chat Report View

Action Performed:

  1. Navigate to staging.new.expensify and sign in
  2. Open any chat and send PDF files
  3. Click on PDF thumbnail to open a preview

Expected Result:

PDF preview is opened without delays

Actual Result:

PDF preview loads with delay, the grey line shown first

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/26996e5f-8eb4-4e66-8ad0-1890d76162b4 https://github.com/user-attachments/assets/ac4dc41b-73f7-4ad8-9d29-dd1ee94bb6fa

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021883791423079955100
  • Upwork Job ID: 1883791423079955100
  • Last Price Increase: 2025-02-03
Issue OwnerCurrent Issue Owner: @rayane-djouah
melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @dylanexpensify (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.

dylanexpensify commented 1 week ago

Unable to reproduce. Can you confirm @IuliiaHerets?

IuliiaHerets commented 1 week ago

Hi, @dylanexpensify. QA team can still repro this issue, build - 89-5. Tester used Mac Chrome

https://github.com/user-attachments/assets/e9114d1c-c906-4d1e-9d1b-34520d5f5b4b

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

github-actions[bot] commented 1 week ago

⚠️ @Kalydosos Thanks for your proposal. Please update it to follow the proposal template, as proposals are only reviewed if they follow that format (note the mandatory sections).

Kalydosos commented 1 week ago

🚨 Edited by proposal-police: This proposal was edited at 2025-01-30 06:47:19 UTC.

Proposal

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

PDF attachments previews load with apparent delay, some grey lines are shown first.

What is the root cause of that problem?

The root cause is that the attachment display process is not hidden behind a loading spinner long enough to hide the display process

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

We should use the same loading indication mechanism as for the other types of attachments

For that, we should define a loading state in https://github.com/Expensify/App/tree/9.0.89-2/src/components/PDFView/index.tsx , meaning

const [isLoading, setIsLoading] = useState(true);

We then define a function to manage the loading completion status

const onLoadFullyComplete = useCallback( () => { setIsLoading(false); }, [], );

We then add the above function to the call to PDFPreviewer here

https://github.com/Expensify/App/blob/d0b494313adf0c1960a16e24c2114ccf9c8f5fe8/src/components/PDFView/index.tsx#L96

as well as a condtionned display of a loading spinner, as such

{isLoading && FullscreenLoadingIndicator style={[styles.opacity1]}} <PDFPreviewer onLoadComplete={onLoadFullyComplete}

and remove its LoadingComponent here

https://github.com/Expensify/App/blob/d0b494313adf0c1960a16e24c2114ccf9c8f5fe8/src/components/PDFView/index.tsx#L104

In https://github.com/Expensify/react-fast-pdf/blob/1.0.22/src/PDFPreviewer.tsx we could then use onLoadComplete function like this

const hideGlobalLoader = useCallback(() => { onLoadComplete?.(); }, []);

which will then be passed to the PageRenderer here https://github.com/Expensify/react-fast-pdf/blob/a66e51945117e89956e5aea9be5cf7d3cc29d269/src/PDFPreviewer.tsx#L245

as following

itemData={{pageWidth, estimatedPageHeight, calculatePageHeight, getDevicePixelRatio, containerHeight, numPages, hideGlobalLoader}}

https://github.com/Expensify/react-fast-pdf/blob/1.0.22/src/PageRenderer.tsx will then use it as following

const informPageRendered = useCallback(() => { // we call hideGlobalLoader as soon as the first page is rendered (usually as last) if (index == 0){ hideGlobalLoader?.(); } }, []);

That function will then be called when the display of each page is done by adding it as onRenderSuccess prop here

https://github.com/Expensify/react-fast-pdf/blob/a66e51945117e89956e5aea9be5cf7d3cc29d269/src/PageRenderer.tsx#L32

as following

onRenderSuccess={informPageRendered}

RESULT

It works like a charm

https://github.com/user-attachments/assets/756b3a01-8308-41fa-a7d4-94098ed8ea74

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

None

What alternative solutions did you explore? (Optional)

None

dylanexpensify commented 1 week ago

@rayane-djouah to review!

melvin-bot[bot] commented 1 week ago

@dylanexpensify, @rayane-djouah Whoops! This issue is 2 days overdue. Let's get this updated quick!

rayane-d commented 1 week ago

@Kalydosos, the root cause isn’t fully clear from your proposal. Could you clarify what’s specifically causing the delay in the PDF preview loading and the appearance of the grey line? Also, could you explain what the current loading indicator logic is doing and how it leads to this issue?

Kalydosos commented 1 week ago

@rayane-djouah there is no delay in fact, it's the normal rendering process except that there is no loading spinner mask hidding the process. And the original cause is that because of a previous bug we dont provide a spinner to react-pdf to hide the rendering process the way it expected as you can see below :

the spinner provided to react-pdf here https://github.com/Expensify/react-fast-pdf/blob/a66e51945117e89956e5aea9be5cf7d3cc29d269/src/PDFPreviewer.tsx#L232

should be provided also through pagerenderer here
https://github.com/Expensify/react-fast-pdf/blob/a66e51945117e89956e5aea9be5cf7d3cc29d269/src/PageRenderer.tsx#L37-L39

but it's not due to the mentionned issue.

but react-pdf version have been changed here https://github.com/Expensify/react-fast-pdf/commit/c0fad98555ec097201178e2260608890659fd7ca#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519

(and also https://github.com/Expensify/react-fast-pdf/commit/27427a4dca4bcbb7d45af7876a9459b9264112f4#diff-8ed1ff335cb4e3611d2bdb586f374710c998391940a7f421d894c04de3a3ac39)

which triggers this spinner issue again, the bottom line being that the current loading spinner mechanism relying on react-pdf (and due to the aforementioned issue) is not suffiscient to hide the whole rendering process and maybe we dont wish to tweak react-pdf so we should use instead our classical loading spinner mechanism like for other types of attachments

melvin-bot[bot] commented 4 days ago

@dylanexpensify, @rayane-djouah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

rayane-d commented 3 days ago

Sorry for the delay; I was sick at the end of last week. I'll review the issue today.

melvin-bot[bot] commented 3 days ago

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

rayane-d commented 2 days ago

@Kalydosos Thanks for the explanation. I investigated this issue, and as you mentioned, the root cause appears to be related to passing an empty loading prop here.

Observations:

Image Image

Proposed Fixes:

What do you think? Could you update your proposal accordingly?

rayane-d commented 2 days ago

Also cc-ing @rezkiy37 for thoughts as he worked on multiple react-fast-pdf issues

rezkiy37 commented 2 days ago

Hi,

PDF preview loads with delay, the grey line shown first

This is expected behavior now. We have reverted a couple of PRs recently for react-fast-pdf. There is a problem with the previous version of IOS Safari. Therefore, the app now utilizes the legacy PDF.js worker. I am working on this issue (https://github.com/Expensify/App/issues/55176). However, I've already opened https://github.com/Expensify/react-fast-pdf/pull/45. It has been approved and I will return the modern PDF.js worker for browsers. So yeah, I will manage it there.

Kalydosos commented 1 day ago

@rezkiy37 thanks for your feedback. So you will still use the legacy version for users under 18, the problem wont persist in that case ? FYI, i did the tests on Chrome which seems also impacted

rezkiy37 commented 1 day ago

the problem wont persist in that case ?

Yes.

FYI, i did the tests on Chrome which seems also impacted

Yes, it’s correct because it’s not separated by Chrome or Safari. Currently, the app utilizes the legacy one for all browsers and the desktop.

Kalydosos commented 1 day ago

@rezkiy37 Ok, thanks for the feedback 👍. @rayane-d any thoughts ?

dylanexpensify commented 1 day ago

@rayane-d to confirm

rayane-d commented 1 day ago

@rezkiy37 Thanks for reviewing! I tried resetting the history locally to the commit https://github.com/Expensify/App/commit/23c4e879666ceb1e6bfd5ab648164067bf3ef572, which is just before https://github.com/Expensify/App/commit/f71f36e81a50333eb8a801b328b9a83ad0260d17 (where https://github.com/Expensify/App/pull/55421 was merged). However, I’m still able to reproduce the issue. It seems the issue persists even with the latest PDF.js worker. Am I missing something?

https://github.com/user-attachments/assets/c98e3166-304a-4b5a-97d9-cd4742e5881f

melvin-bot[bot] commented 1 day ago

@dylanexpensify @rayane-d this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

rezkiy37 commented 1 day ago

Let me take a look 👀

rezkiy37 commented 16 hours ago

@rayane-d Yeah, you are right. It does not care about the worker. Therefore this issue makes sense.

https://github.com/user-attachments/assets/56eb0b76-8a14-4022-a490-fbbac566d3f6

rayane-d commented 13 hours ago

@rezkiy37 Thanks for confirming!

rayane-d commented 13 hours ago

@Kalydosos Any thoughts on https://github.com/Expensify/App/issues/55671#issuecomment-2635134947?