[C+ Checklist Needs Completion] [$500] "Failed to load PDF file" message is not rendering in message preview

[kavimuru]

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.36-1 Reproducible in staging?: y Reproducible in production?: y 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: @ishpaul777 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1706234706816449

Action Performed:

Upload a Corrupted pdf in any report

Expected Result:

if pdf preview fails to load "Failed to load PDF file" render

Actual Result:

Nothing renders

Workaround:

unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0163673ea563ca697f
• Upwork Job ID: 1754473111912144896
• Last Price Increase: 2024-02-19
• Automatic offers:
• getusha | Reviewer | 0

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[ikevin127]

Can't reproduce - on my side on Android it shows an error if I try to upload a corrupted PDF.

[ishpaul777]

Can you try reproducing with this one Corrupted.PDF.pdf

[kaushiktd]

Proposal

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

"Failed to load PDF file" message is not rendering in message preview.

What is the root cause of that problem?

Three is a issue with mention file here. It seems there is an issue with the provided file (index.android.js) where the PDF content is still being rendered even when attachmentCarouselPagerContext is null

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

To address this issue, you can modify the code to check for the null value of attachmentCarouselPagerContext and display an error message accordingly. Here is an example of how you can update the code:

{attachmentCarouselPagerContext === null ? (
    <View style={[themeStyles.flex1, themeStyles.justifyContentCenter]}>
        <Text style={props.errorLabelStyles}>{translate('attachmentView.failedToLoadPDF')}</Text>
    </View>
) : (
    // same as existing code
)}

include components on top of the function

import useLocalize from '@hooks/useLocalize';
import Text from '@components/Text';

and mention below const variable on line#20

const {translate} = useLocalize();
const themeStyles = useThemeStyles();

What alternative solutions did you explore? (Optional)

You can remove the style [this.props.themeStyles.flexRow] from here

Screenshots:

[ikevin127]

Proposal

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

When successfully uploading a corrupt PDF file, the message is not being rendered in the message preview.

What is the root cause of that problem?

[!NOTE] The issue is happening on botn iOS / Android native platforms.

We don't see anything on native platforms but we see "Failed to load PDF file." on web. The difference between the native / web platforms is that on native we're applying different styles to the child container View that's rendered within the Pressable when an error exists.

Where is the problem exactly ?

If we take a look within the PDFView/index.native.js we can see that the Pressable that wraps the {this.renderPDFView()} has the following styles applied: flex1, flexRow and alignSelfStretch.

While the container View rendered by renderPDFView has the following styles applied when the error is being rendered: alignItemsCenter and flex1.

These styles have a conflict which make the error View invisible due to the following reasons:

1. flex1 on the container View within the Pressable:

   • The use of flex1 is intended to make the View expand to fill the available space in the primary axis of its parent (the Pressable). However, because the Pressable has a flexRow style, its primary axis is horizontal. This means that flex1 would only affect the width of the container View. If there's no explicit height specified for the View or the Pressable, and no other child dictates the height, the View does not have enough height to be visible.

2. alignItemsCenter on the child View within the Pressable:

   • Being inside a parent with flexRow (the Pressable), the alignItemsCenter applied to the container View doesn't affect its width but rather its vertical alignment. This leads to the container View being centered vertically with no defined height, resulting in a zero-height View that does not display its contents.

3. flexRow on the parent Pressable:

   • This style sets the children of the Pressable to be laid out in a horizontal line. Since flex1 on the child is trying to fill space horizontally, it doesn't provide information about how the View should expand vertically (along the cross axis).

Together, these styles result in the container View taking up all available width due to flex1 while it does not have a defined height to make it visible within the layout. The View needs distinct rules to ensure it takes up space along the vertical axis as well since flexRow on the Pressable causes the main axis to be horizontal.

Without an explicit height (or minimum height), the container View will collapse vertically and not display any of its contents, including the error View / Text when the error is present.

[!IMPORTANT] All platforms (web / native) DO render the "Failed to load PDF file." from a code POV but we're not seeing it on native due to the conflicting styles.

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

https://github.com/Expensify/App/blob/06c7dac74ad5bcc8009c614570625afe4006f621/src/components/PDFView/index.native.js#L188

In order to make sure, as mentioned in the RCA that the Pressable's children -> the container View takes up space along the vertical axis as well since flexRow is present on the Pressable, we need to conditionally remove the flexRow style from the Pressable based on whether the error failedToLoadPDF is true.

This is the only way to have both the error comment and the error within the attachment modal render correctly, without affecting the Pressable styles when the error is not present, by keeping the flexRow style.

Changed code:

    style={[themeStyles.flex1, themeStyles.alignSelfStretch, !failedToLoadPDF && themeStyles.flexRow]}

Videos

Android: Native

https://github.com/Expensify/App/assets/56457735/ad5cb45b-b707-4908-a73e-6af593ed1601

[getusha]

Reviewing proposals, i will update today.

[ikevin127]

[!NOTE] @kaushiktd Updated their proposal using the styles solution suggested by me after my proposal was posted - without calling it out in an Updated proposal comment ✌️

[kaushiktd]

@ikevin127 That was my optional solution, and unlike your idea, you are removing all styles properties, whilst I am removing only one stylesheet.

Also, my principal solution differed from yours.

[ikevin127]

1. I didn't mention exactly in my solution which style is to be removed. Obviously we're not going to remove styles which don't cause any issue - that's to be sorted later during PR, if selected.

2. Don't act like you had any idea about the styles causing the issue before I posted my proposal - it's clear from your RCA and solution that you didn't✌️

3. Your RCA and main solution only targets android - while the issue is reproducible on both native platforms.

[kaushiktd]

Looks like I pressed the nerve point! I knew about the styles so don't jump around to conclusions. Cintributors can see the proposals so let's leave on them. Besides I have a feeling that this issues is going to be auto close soon.

About the targeting android, I know it also happens in iOS but the issue suggests this is for android so I prefer to do iOS part when the same issue will be reposted in iOS, separately. If contributor has checked iOS as platform , I would have done that as well. So I would rather do it separately instead doing double job in one payment.

[getusha]

Which style property is causing the text to disappear, and any explanation why?

[ikevin127]

Looks like the flexRow is causing the issue here, not sure why other than the way react native styles apply between web / native platforms might be slightly different.

I checked the pressable styles together with the Failed to Load PDF wrapping View container styles and normally there shouldn't be any issue from a styles pov.

It looks like there's nothing rendered as Pressable children with that style, whereas when removing it - the children are rendered.

[kaushiktd]

@getusha Styles properties are not the main cause of this issue. When uploading a corrupted PDF file, the backend will attempt to generate an image preview for the PDF. However, if the PDF is corrupted, it won't be converted to an image and will remain in the link tag.

In the existing code, there is no context to check whether the preview is available or not on the native side. Therefore, you must check whether the preview is available or not, as mentioned in my proposal.

In addition, if you want to bypass all the check processes, you can remove the flexrow styles.

In current Expensify code, the alignItemsCenter and flex1 styles are applied to the inner View component. here (https://github.com/Expensify/App/blob/main/src/components/PDFView/index.native.js#L148)

However, when using flexRow on the parent view, these styles will not behave as expected because they primarily affect the alignment of child components along the cross-axis, which is vertical in a row.

If you want to horizontally center the Text component within the row, you should either remove flexRow from the parent View (https://github.com/Expensify/App/blob/main/src/components/PDFView/index.native.js#L188) or remove flex1 from the child View (https://github.com/Expensify/App/blob/main/src/components/PDFView/index.native.js#L145).

[getusha]

Thanks everyone for the explanation, i think it makes sense to add the condition as @kaushiktd mentioned. will continue testing and i'll update.

[ikevin127]

Updated proposal

i think it makes sense to add the condition as @ kaushiktd mentioned

@getusha Not sure how you arrived to that conclusion 👀 because:

1. If we look into the code and test the proposed solution we can notice that it only "fixes" the problem on Android.

Screenshot

2. Why do I quote "fixes" ?

Because it's not an actual fix since the root cause is not understood and the solution redundantly copies a block of code which already exists within PDFView and replaces the Content component with that block, while the Content component already renders PDFView which contains that error View logic and more.

Note: This goes against our Contributing Guideline, precisely the DRY principle.

[!NOTE] The Content that's returned when attachmentCarouselPagerContext === null renders the BaseAttachmentViewPdf component which in turn renders the actual PDFView which already contains the error View code block when there's an error + other logic which should not be discarded.

Again: this is a simple styling conflict issue, as mentioned in my proposal - has nothing to do with the error View not being rendered - it is rendered, it's just not visible because of the styles. See the updated proposal for more details.

I think somebody from Expensify should take a look at the current state of this issue because with statements like:

About the targeting android, I know it also happens in iOS but the issue suggests this is for android so I prefer to do iOS part when the same issue will be reposted in iOS, separately. If contributor has checked iOS as platform , I would have done that as well. So I would rather do it separately instead doing double job in one payment.

especially that last sentence, I don't like where this is going 😅

[kaushiktd]

Note: This goes against our Contributing Guideline, precisely the DRY principle.

Consider me a friend while I say this, but don't push this, my dear fellow! The way you phrase this sounds like you're accusing them of not knowing what they're doing. They are "assignees" for a reason.

Also, it makes no difference to me or the contributors where my last sentences end because I had a conversation on Slack when I was in a similar circumstance. Since they discontinued bug reporting and made it purely internal, there is no purpose in registering the identical bug on iOS for this. They are coders, so if they believe this should also be done in iOS, they will let us know in a separate job. As of now only ANDROD is check marked. It is common sense for me that any bug reported for android, I should check them in iOS. I knew that. Stop assuming they are ignoring your suggestion or favoring mine and making such long comments. They inspect everything. Just because they do not put their views about every proposal, does not mean they do not check each comment here. Trust the process. They will make right decision.

[ikevin127]

don't push this

Don't push what exactly ? If you don't mind me asking. We're not friends nor dear fellows - I don't know you personally 🙂, all I know about you is that you are trying really hard to push redundant code to our codebase while not fully understanding the root cause of the issue.

Just because whomever reported this bug did not check on iOS, that doesn't mean we should completely ignore the fact that this happens on iOS and disregard the guidelines for a quick pay, regardless of making a dent within the codebase.

Stop assuming they are ignoring your suggestion or favoring mine and making such long comments. They inspect everything. Just because they do not put their views about every proposal, does not mean they do not check each comment here. Trust the process. They will make right decision.

That's partly true and shouldn't always be taken as absolute truth as based on recent events I can say that that's not always the case. Take this issue as an example.

As you can see from the mentioned issue, the reviewer already made their decision but upon further review from an Expensify team member, the decision was to go with the correct RCA and solution which was best for the codebase and I think we're dealing with the same situation here.

We have to acknowledge that sometimes the reviewer doesn't have as deep of an understanding of an issue as a Contributor that spend 1-2 hours debugging that issue has when it comes to the code involved. That's completely fine, but that doesn't mean I won't do anything in my power to bring that to their attention - especially when it comes to keeping the codebase clean.

I understand that some Contributors might have other motivations than keeping the codebase clean and adhering to the guidelines, but this does not mean they have the right to silence somebody who sees things differently ✌

I'm going to stop the back and forth here as given all the context provided I think it's enough for the reviewer / team to make their decision 🚀

[kaushiktd]

Again long msg and you were asking for pushing what!! I said "Consider" but nvm. Great talk and good luck!🙂

[melvin-bot[bot]]

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

[getusha]

thanks for the detailed RCA @ikevin127, will test your proposal too. i appreciate the patience.

[getusha]

@ikevin127 i just applied your solution and it doesn't seem to fix the issue.

[getusha]

@kaushiktd i found your proposal to be quite incomplete and a bit hard to test, could you add a bit more details on the implementation based on the component being class based currently?

[kaushiktd]

Sure, @getusha

[kaushiktd]

I have a question. In my proposal I have already given detailed explanation on class components for android. Are you requesting to do this for iOS as well? @getusha

[ikevin127]

i just applied your solution and it doesn't seem to fix the issue.

@getusha Are you sure you replaced [themeStyles.alignItemsCenter, themeStyles.flex1] with [themeStyles.w100, themeStyles.h100] within PDFView/index.native.js (native file) on this line ?

https://github.com/Expensify/App/blob/2e3840f2f94934b7937e9953133b32b1e3e81566/src/components/PDFView/index.native.js#L126

Video proof step by step

https://github.com/Expensify/App/assets/56457735/b3e3d772-fa11-40a7-b963-2e5dd1e94ac7

[getusha]

@kaushiktd my bad i applied your solution at the wrong place.

@ikevin127 this is the diff

diff --git a/src/components/PDFView/index.native.js b/src/components/PDFView/index.native.js
index bfdb80131a..af0d64431b 100644
--- a/src/components/PDFView/index.native.js
+++ b/src/components/PDFView/index.native.js
@@ -142,7 +142,7 @@ class PDFView extends Component {
         const containerStyles =
             this.state.shouldRequestPassword && this.props.isSmallScreenWidth
                 ? [this.props.themeStyles.w100, this.props.themeStyles.flex1]
-                : [this.props.themeStyles.alignItemsCenter, this.props.themeStyles.flex1];
+                : [themeStyles.w100, themeStyles.h100];

         return (
             <View style={containerStyles}>

[ikevin127]

@getusha That does not make any sense, can you make sure you pulled latest main and rebuild your android app ?

Android: step by step with latest main and fresh build

https://github.com/Expensify/App/assets/56457735/6c9466ea-1ba1-40e5-8d1c-7dfb1398cc55

Looks like there are some differences in your diff as the component changed from class to functional recently. The PDFView/index.js (non-native) is class component currently:

https://github.com/Expensify/App/blob/2e3840f2f94934b7937e9953133b32b1e3e81566/src/components/PDFView/index.js#L23

whereas the native file is using functional component.

Not sure how you're applying that diff, can you please try as I did in the step by step video and confirm whether or not it's working ?

In case that doesn't work, can you please ask in the C+ channel if anyone else can apply the changes and test this ? 👀

[kaushiktd]

@kaushiktd my bad i applied your solution at the wrong place.

So can I assume I do not have to do anything? @getusha

[getusha]

@ikevin127 looks like rebuilding the android app made it work.

So can I assume I do not have to do anything? @getusha

@kaushiktd yes

[ikevin127]

It happens sometimes for the apps to use cached old code and have weird bugs that cannot be explained if a rebuild wasn't performed in a while after many changes were pulled into the codebase ♻ I know that building on Android can be a pain 😪

Glad you managed to get it work and confirm the solution as working 👍

[getusha]

@kaushiktd your solution will cause a regression when you try to upload a new PDF it will show the error message no matter what. but i am convinced that, this is a styling issue since the component is already rendering.

[getusha]

@ikevin127 this is what i am seeing when i try to upload the corrupt pdf.

[getusha]

@kaushiktd's alternative solution seems to work, will test it further.

[ikevin127]

@getusha Hmm, weird - it looks good on my side 🧐

Most likely because of the h100, we just need to pass height: min-content or something similar that would only give it as much height as the view has.

Will get back and let you know exactly - but I think since the solution is correct (a styling issue) we can work this out during the PR, what do you think ?

[getusha]

but I think since the solution is correct (a styling issue) we can work this out during the PR, what do you think ?

I agree your RCA is correct and you're the first to point where the issue lies (styling). but it could create complications if we end up using @kaushiktd's alternative solution in the PR. imo its better to clear that up here.

[ikevin127]

@ kaushiktd's alternative solution seems to work, will test it further.

@getusha That was my main solution before I updated my proposal - they added it as the alternative solution AFTER I posted my initial proposal.

Please check comments edit history ✌️

I also mentioned in my proposal's alternative why that solution is not a good idea.

If that part of my initial solution ends up being used for solving this issue by somebody else other than me - I will request a 50/50 bounty split, as I was the first to identify the RCA and provide a working solution - and I think that's fair!

[kaushiktd]

but it could create complications if we end up using @kaushiktd's alternative solution in the PR.

Not sure what complications you are referring to here but I tested again and it works perfectly! https://drive.google.com/file/d/14mhmdLmoSGjlUAjFqkmwFnpFlyr7iyTS/view

Also I belive I provided the proposal, and then updated it multiple times. Unfortunately from my side I can not check exactly when was my proposal updated and in my proposal I have made everything clear so far. so I will not be pushy here.

Also, it doesn't imply I'm incorrect if I don't respond to every comment. I don't tend to be pushy or aggressive because I have complete faith in the assignee. Although I oppose splitting the amount, I will accept the assignee's decision in this regard.

[getusha]

Not sure what complications you are referring to here but I tested again and it works perfectly!

@kaushiktd did you test your main solution or alternative?

[kaushiktd]

@kaushiktd did you test your main solution or alternative?

@getusha I made that video after testing both solutions, main and alternative.

[getusha]

@getusha I made that video after testing both solutions, main and alternative.

I double confirmed unfortunately the main solution causes the issue i mentioned above.

[kaushiktd]

Ishpaul777's comment just caught my attention and as per that in latest code merge this issue may have been fixed. Have you checked that?

[ikevin127]

Updated proposal (second time)

• final update of proposal based on the latest context as the last update would create regression (#35808 (comment))
• this update takes care of both scenarios (chat comment and attachment modal open with "Failed to load PDF file" error)

cc @getusha

___

Ishpaul777's comment just caught my attention and as per that in latest code merge this issue may have been fixed. Have you checked that?

I can confirm that the issue is still present with latest main branch.

Also I belive I provided the proposal, and then updated it multiple times. Unfortunately from my side I can not check exactly when was my proposal updated and in my proposal I have made everything clear so far. so I will not be pushy here.

Also, it doesn't imply I'm incorrect if I don't respond to every comment. I don't tend to be pushy or aggressive because I have complete faith in the assignee. Although I oppose splitting the amount, I will accept the assignee's decision in this regard.

This is not being aggressive or pushy as you keep insinuating, this is about making sure the reviewer is aware of the proposal edit history and who came with the correct RCA / solution first - you don't have to make this a personal attack as this is an argument based on real data (timestamps) which can be verified by anyone by hovering the cursor over each edit timestamp (see videos below).

So here we go, below you can find timestamps / snapshoots of the proposals and their edits in chronological order to avoid any confusion when it comes to the reviewing / selection process:

Proposals edit history (spoiler)

@kaushiktd Feb 6, 11:24 AM - Initial proposal - watching the video we can see that there's no indication of a styling issue in either RCA or solution

Video of proposal

https://github.com/Expensify/App/assets/56457735/68b73cb6-0aed-4770-a857-94c313a096e5

@kaushiktd Between Feb 6, 11:24 AM - Feb 6, 2:30 PM - 2 minor edits adding to the initial RCA / solution - no mention of the styling issue yet @ikevin127 Feb 6, 5:06 PM - Initial proposal - first mention of the correct RCA / solution according to reviewer's comment ([#35808 (comment)](https://github.com/Expensify/App/issues/35808#issuecomment-1951370433))

Video of proposal

https://github.com/Expensify/App/assets/56457735/1f8b7482-cf6e-4d3f-ae65-0cba370ac0de

@kaushiktd Feb 7, 07:13 AM - 3rd edit of proposal, a day later after my initial proposal - watching the video we can see that the alternative was added a day after my initial proposal was posted, I will let the reviewers decide if this was inspired by mine or not - added the alternative quickly with permalink broken (line doesn't exist in that file - check video below)

Video of edited proposal

https://github.com/Expensify/App/assets/56457735/27b9cc3e-538f-4b9c-add7-0a9fd7c2238e

@kaushiktd Feb 7, 07:16 AM - 4rd edit of proposal, specified exactly what style should be removed but not from what file - permalink to the referenced code line still broken (line doesn't exist in that file - check video below)

Video of edited proposal

https://github.com/Expensify/App/assets/56457735/282829b7-1cd4-4505-823c-305f8643aab0

This is all I had to say with regards to the edit timestamps and I hope this will be taken into account when assigning the Contributor. In any case, this is the proof to support my case for the 50/50 split in case the RCA / solution I identified and proposed initially will be used by another Contributor 🙏

[melvin-bot[bot]]

@greg-schroeder @getusha 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!

[melvin-bot[bot]]

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

[getusha]

thanks @ikevin127 conditionally removing that styling makes sense! i'll test it a bit more and provide an update. i think splitting the bounty is fair, what do you guys think? @kaushiktd

[kaushiktd]

@getusha I will respect your decision without hesitation. 🙂

[ikevin127]

@getusha I disagree splitting the bounty if I'm assigned here, since based on the info provided in https://github.com/Expensify/App/issues/35808#issuecomment-1952482113 (spoiler) is clear at least from my side who came up with the correct RCA / solution first and who edited their proposal using that info 1 day later:

• without explaining RCA
• without posting a Proposal updated comment

having no idea of the correct RCA / solution in their initial proposal.

Just so we're clear here - I'm not going to do the work myself if we're going to split.

The only way we would split here, which I still think is unfair (no basis), would be for somebody else to do the work using my proposal.

If you disagree please support your case for a split using the proposal edits history - I'm curious how you came to that suggestion 👀

Otherwise we should have somebody else review this split situation once assigned, hopefully it will clear things up!

[getusha]

@ikevin127 the alternative solution @kaushiktd provided was the solution that was working without any regressions (removing a specific styling without having to remove all), and if one provides a clear RCA and a solution that'll cause a regression i don't think we should be selecting that. and splitting is fair because one provided the RCA one has a working solution.