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

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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:

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

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-10

Issue Owner

Current Issue Owner: @rayane-djouah

[melvin-bot]

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]

Unable to reproduce. Can you confirm @IuliiaHerets?

[IuliiaHerets]

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

Recording.341.mp4

[melvin-bot]

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

[melvin-bot]

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

[github-actions]

⚠️ @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]

🚨 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

App/src/components/PDFView/index.tsx

Line 96 in d0b4943

<PDFPreviewer

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

App/src/components/PDFView/index.tsx

Line 104 in d0b4943

LoadingComponent={

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

fixworks.mp4

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]

@rayane-djouah to review!

[melvin-bot]

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

[rayane-d]

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

@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
Expensify/react-fast-pdf@c0fad98#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519

(and also Expensify/react-fast-pdf@27427a4#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]

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

[rayane-d]

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

[melvin-bot]

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

[rayane-d]

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

• When removing the empty loading prop (thereby using the default "Loading page..." text), the page displays this text but with incorrect page dimensions. This explains the grey line bug—the empty loading page likely has zero width but retains shadows.

• Before the page fully loads, a blank page with incorrect dimensions is briefly displayed also:

Proposed Fixes:

• Can we resolve this in by ensuring proper width/height calculations for both the loading and blank pages? This way we will have a blank page while it's loading.

• Alternative Approach: Instead of tightly coupling the loading states of PDFPreviewer and PageRenderer (as suggested in your proposal), we could implement a dedicated loading spinner component for PageRenderer and pass it via the loading prop. This would keep the loading states of PDFPreviewer and PageRenderer decoupled

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

[rayane-d]

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

[rezkiy37]

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 (#55176). However, I've already opened Expensify/react-fast-pdf#45. It has been approved and I will return the modern PDF.js worker for browsers. So yeah, I will manage it there.

[Kalydosos]

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

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]

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

[dylanexpensify]

@rayane-d to confirm

[rayane-d]

@rezkiy37 Thanks for reviewing! I tried resetting the history locally to the commit 23c4e87, which is just before f71f36e (where #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?

Screen.Recording.2025-02-05.at.11.52.56.PM.mov

[melvin-bot]

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

Let me take a look 👀

[rezkiy37]

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

Recording.mp4

[rayane-d]

@rezkiy37 Thanks for confirming!

[rayane-d]

@Kalydosos Any thoughts on #55671 (comment)?

[Kalydosos]

Can we resolve this in by ensuring proper width/height calculations for both the loading and blank pages?

@rayane-d the grey outlines seems related to the viewports of the currently displayed pages. Their widths are adjusted progressively in the rendering process. i think maybe @rezkiy37 can tell us best if we can ensuring proper width/height calculations

we could implement a dedicated loading spinner component for PageRenderer and pass it via the loading prop

trynotemptyloaderprop.mp4

I've tried that but the issue still occur. I've passed to PageRenderer the same LoadingComponent (a modified version) passed to Document in PDFPreviewer https://github.com/Expensify/react-fast-pdf/blob/a66e51945117e89956e5aea9be5cf7d3cc29d269/src/PDFPreviewer.tsx#L232

[rezkiy37]

...i think maybe @rezkiy37 can tell us best if we can ensuring proper width/height calculations...

If you don't mind, I can work on this issue and figure it out.

[rezkiy37]

Hi, I am Michael (Mykhailo) from Callstack, an expert agency and I can work on this issue.

[Kalydosos]

If you don't mind, I can work on this issue and figure it out.

that will be nice @rezkiy37 but i will somehow advise that for consistency we set up a loading spinner managment in App instead of in react-fast-pdf like in my proposal so we can anticipate changes and constraints (like offline status) better and in the same manner as for other types of attachments. Per example, we display the default attachment (an icon and the filename), when offline and cache is disabled, instead of the offline status notice as for other types of attachments. What do you think @rayane-d ?

[melvin-bot]

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

[melvin-bot]

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

[rayane-d]

i will somehow advise that for consistency we set up a loading spinner managment in App instead of in react-fast-pdf like in my proposal so we can anticipate changes and constraints (like offline status) better and in the same manner as for other types of attachments

As I mentioned in this comment, my preference would be to address this issue within react-fast-pdf itself, based on the root cause analysis in your proposal and my initial investigation. However, if we are unable to find a suitable solution, this alternative approach could serve as a viable option.

Per example, we display the default attachment (an icon and the filename), when offline and cache is disabled, instead of the offline status notice as for other types of attachments.

I think this was a design choice and is currently intended behavior.

[rayane-d]

@dylanexpensify Friendly bump on #55671 (comment)
@rezkiy37 Feel free to post a proposal

[rezkiy37]

Nice, I am working on the issue tomorrow.