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

kavimuru · 2024-02-05T11:52:38Z

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

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 · 2024-02-05T11:52:42Z

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

melvin-bot · 2024-02-05T11:52:47Z

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

melvin-bot · 2024-02-05T11:52:49Z

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

ikevin127 · 2024-02-05T13:31:57Z

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

ishpaul777 · 2024-02-05T13:36:08Z

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

kaushiktd · 2024-02-06T09:24:04Z

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 · 2024-02-06T15:06:10Z

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:

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.
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.
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?

App/src/components/PDFView/index.native.js

Line 188 in 06c7dac

    
           style={[this.props.themeStyles.flex1, this.props.themeStyles.flexRow, this.props.themeStyles.alignSelfStretch]}

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

Screen.Recording.2024-02-19.at.14.58.35.mov

getusha · 2024-02-07T18:33:45Z

Reviewing proposals, i will update today.

ikevin127 · 2024-02-07T20:56:48Z

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 · 2024-02-08T02:57:31Z

@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 · 2024-02-08T03:31:17Z

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.
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✌️
Your RCA and main solution only targets android - while the issue is reproducible on both native platforms.

kaushiktd · 2024-02-08T03:39:30Z

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 · 2024-02-08T17:33:32Z

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

kaushiktd · 2024-02-09T05:07:12Z

@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 · 2024-02-11T18:04:53Z

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 · 2024-02-11T21:56:56Z

Updated proposal

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

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

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

Screenshot

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 · 2024-02-12T03:28:35Z

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 · 2024-02-12T10:42:48Z

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 · 2024-02-12T11:41:00Z

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

kaushiktd · 2024-02-26T05:16:53Z

@getusha @jasperhuangg Can one of you please send upwork offer manually?

Any update on this?

getusha · 2024-02-26T08:35:35Z

@kaushiktd there is no need to worry, offer can be sent after PR is merged and regression period passes.

greg-schroeder · 2024-02-26T22:47:28Z

Reviewing

greg-schroeder · 2024-02-26T22:50:27Z

Yeah, I mean we're still not even in the payment period yet, but I'll send an offer to both just so it's ready right away

melvin-bot · 2024-02-29T23:25:43Z

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

melvin-bot · 2024-02-29T23:25:46Z

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

Failed to load PDF #37136

If no regressions arise, payment will be issued on 2024-03-07. 🎊

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

@kaushiktd requires payment (Needs manual offer from BZ)
@getusha requires payment automatic offer (Reviewer)
@ishpaul777 requires payment (Needs manual offer from BZ)

melvin-bot · 2024-02-29T23:25:48Z

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:

[@getusha] The PR that introduced the bug has been identified. Link to the PR:
[@getusha] 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:
[@getusha] 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:
[@getusha] Determine if we should create a regression test for this bug.
[@getusha] 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.
[@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

ishpaul777 · 2024-03-01T11:30:02Z

@ishpaul777 requires payment (Needs manual offer from BZ)

this is not correct i was not involved in the PR

greg-schroeder · 2024-03-02T00:31:45Z

Thanks @ishpaul777, noted

ikevin127 · 2024-03-07T19:45:59Z

@greg-schroeder Friendly bump for payments 🫡

greg-schroeder · 2024-03-08T16:10:29Z

Sorry about that, I was OOO yesterday, but I'll process this today!

greg-schroeder · 2024-03-09T02:18:23Z

Processing now

greg-schroeder · 2024-03-09T02:22:06Z

All payments complete. @getusha can you complete the checklist so we can close this out? Thanks!

getusha · 2024-03-11T10:49:01Z

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:

[@getusha] The PR that introduced the bug has been identified. Link to the PR: #21714
[@getusha] 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: https://github.com/Expensify/App/pull/21714/files#r1519516670
[@getusha] 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: N/a
[@getusha] Determine if we should create a regression test for this bug. Yes
[@getusha] 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.

Regression Test Proposal

Open any chat
Add attachment
Send corrupted pdf (https://github.com/Expensify/App/files/14166298/Corrupted.PDF.pdf)
Verify that Failed to load PDF file renders correctly.

Do we agree 👍 or 👎

greg-schroeder · 2024-03-13T20:25:06Z

Filing regression test and closing!

kavimuru added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 5, 2024

melvin-bot bot changed the title ~~"Failed to load PDF file" message is not rendering in message preview~~ [$500] "Failed to load PDF file" message is not rendering in message preview Feb 5, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 5, 2024

melvin-bot bot assigned getusha Feb 5, 2024

melvin-bot bot assigned greg-schroeder Feb 5, 2024

ishpaul777 mentioned this issue Feb 7, 2024

fixes blankArea-for-corruptedPDF #34302

Closed

50 tasks

melvin-bot bot added the Overdue label Feb 7, 2024

melvin-bot bot removed the Overdue label Feb 7, 2024

This comment was marked as outdated.

Sign in to view

melvin-bot bot added the Weekly KSv2 label Feb 23, 2024

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Feb 29, 2024

melvin-bot bot changed the title ~~[$500] "Failed to load PDF file" message is not rendering in message preview~~ [HOLD for payment 2024-03-07] [$500] "Failed to load PDF file" message is not rendering in message preview Feb 29, 2024

melvin-bot bot removed the Reviewing Has a PR in review label Feb 29, 2024

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Mar 7, 2024

greg-schroeder removed the Awaiting Payment Auto-added when associated PR is deployed to production label Mar 9, 2024

greg-schroeder changed the title ~~[HOLD for payment 2024-03-07] [$500] "Failed to load PDF file" message is not rendering in message preview~~ [C+ Checklist Needs Completion] [$500] "Failed to load PDF file" message is not rendering in message preview Mar 9, 2024

melvin-bot bot added the Overdue label Mar 11, 2024

melvin-bot bot added Overdue and removed Overdue labels Mar 11, 2024

greg-schroeder closed this as completed Mar 13, 2024

melvin-bot bot removed the Overdue label Mar 13, 2024

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

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

Comments

kavimuru commented Feb 5, 2024 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Feb 5, 2024

melvin-bot bot commented Feb 5, 2024

melvin-bot bot commented Feb 5, 2024

ikevin127 commented Feb 5, 2024

ishpaul777 commented Feb 5, 2024

kaushiktd commented Feb 6, 2024 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Screenshots:

ikevin127 commented Feb 6, 2024 • edited Loading

Proposal

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

What is the root cause of that problem?

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

Videos

getusha commented Feb 7, 2024

ikevin127 commented Feb 7, 2024

kaushiktd commented Feb 8, 2024

ikevin127 commented Feb 8, 2024

kaushiktd commented Feb 8, 2024

getusha commented Feb 8, 2024

This comment was marked as outdated.

kaushiktd commented Feb 9, 2024

getusha commented Feb 11, 2024

ikevin127 commented Feb 11, 2024

Updated proposal

kaushiktd commented Feb 12, 2024 • edited Loading

ikevin127 commented Feb 12, 2024

kaushiktd commented Feb 12, 2024

kaushiktd commented Feb 26, 2024

getusha commented Feb 26, 2024

greg-schroeder commented Feb 26, 2024

greg-schroeder commented Feb 26, 2024

melvin-bot bot commented Feb 29, 2024

melvin-bot bot commented Feb 29, 2024

melvin-bot bot commented Feb 29, 2024

ishpaul777 commented Mar 1, 2024

greg-schroeder commented Mar 2, 2024

ikevin127 commented Mar 7, 2024

greg-schroeder commented Mar 8, 2024 • edited Loading

greg-schroeder commented Mar 9, 2024

greg-schroeder commented Mar 9, 2024

getusha commented Mar 11, 2024

greg-schroeder commented Mar 13, 2024

kavimuru commented Feb 5, 2024 •

edited by melvin-bot bot

Loading

kaushiktd commented Feb 6, 2024 •

edited

Loading

ikevin127 commented Feb 6, 2024 •

edited

Loading

kaushiktd commented Feb 12, 2024 •

edited

Loading

greg-schroeder commented Mar 8, 2024 •

edited

Loading