[HOLD for payment 2024-01-04] [HOLD for payment 2024-01-03] [$500] Chat - PDF is not displayed in chat after uploading

[kbecciv]

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.17-1
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:

Pre-requisite: the user must be logged in.

1. Go to any chat.
2. Tap on the "+" button.
3. Tap on "Add attachment".
4. Tap on "Choose document".
5. Select any PDF.
6. Tap on "Send".
7. Wait for it to upload.

Expected Result:

PDF preview should be displayed in chat after the uploading.

Actual Result:

PDF preview is not displayed in chat after the uploading.

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6326434_1703625657812.Chox1648_1_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a3278ab046cf0de8
• Upwork Job ID: 1739768322479259648
• Last Price Increase: 2023-12-26
• Automatic offers:
• mkhutornyi | Contributor | 28070768

Issue Owner

Current Issue Owner: @parasharrajat

[github-actions]

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

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

[melvin-bot]

Triggered auto assignment to @Beamanator (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[kbecciv]

Issue is not reproduced on production

RPReplay_Final1703624381.MP4

[melvin-bot]

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

[melvin-bot]

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

[mountiny]

I think this might be related to the expo image pr #30905 and seems like a true blocker as PDF files cannot be used/ sent in native devices cc @WojtekBoman

[mountiny]

The PDF previews are missing even after coming back to the chat so definitely a blocker imho

[Beamanator]

holy monkeys that's a big diff 😅

Would be great if @WojtekBoman could help investigate this 🙏 I'll try to test locally soon, assuming my sims start up nicely

[mkhutornyi]

Proposal

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

PDF preview is not displayed in chat

What is the root cause of that problem?

This is html attribute:
<img src="https://www.expensify.com/chat-attachments/:reportID/test.jpg.1024.jpg" data-expensify-source="https://www.expensify.com/chat-attachments/:reportID/test.pdf" data-name="001.pdf" data-expensify-height="0" data-expensify-width="0" />

We check data-expensify-height and data-expensify-width here:

App/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js

Lines 48 to 49 in 4cfb8a2

	const imageWidth = htmlAttribs['data-expensify-width'] ? parseInt(htmlAttribs['data-expensify-width'], 10) : undefined;
	const imageHeight = htmlAttribs['data-expensify-height'] ? parseInt(htmlAttribs['data-expensify-height'], 10) : undefined;

As "0" is not falsy, it doesn't meet above condition and imageWidth, imageHeight are set to undefined.
And they're passed to ThumbnailImage component:

App/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js

Lines 83 to 89 in 4cfb8a2

	<ThumbnailImage
	previewSourceURL={previewSource}
	style={styles.webViewStyles.tagStyles.img}
	isAuthTokenRequired={isAttachmentOrReceipt}
	imageWidth={imageWidth}
	imageHeight={imageHeight}
	/>

In ThumbnailImage, there's default value of 200 but as it's already set to "0", the default value is ignored

App/src/components/ThumbnailImage.tsx

Line 73 in 4cfb8a2

function ThumbnailImage({previewSourceURL, style, isAuthTokenRequired, imageWidth = 200, imageHeight = 200, shouldDynamicallyResize = true}: ThumbnailImageProps) {

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

App/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js

Lines 48 to 49 in 4cfb8a2

	const imageWidth = htmlAttribs['data-expensify-width'] ? parseInt(htmlAttribs['data-expensify-width'], 10) : undefined;
	const imageHeight = htmlAttribs['data-expensify-height'] ? parseInt(htmlAttribs['data-expensify-height'], 10) : undefined;

We can update this condition so check "0" as well:

- const imageWidth = htmlAttribs['data-expensify-width'] ? parseInt(htmlAttribs['data-expensify-width'], 10) : undefined;
- const imageHeight = htmlAttribs['data-expensify-height'] ? parseInt(htmlAttribs['data-expensify-height'], 10) : undefined;
+ const imageWidth = +htmlAttribs['data-expensify-width'] ? parseInt(htmlAttribs['data-expensify-width'], 10) : undefined;
+ const imageHeight = +htmlAttribs['data-expensify-height'] ? parseInt(htmlAttribs['data-expensify-height'], 10) : undefined;

(+ operator converts string to number so "0" ? is changed to 0 ?)

[Beamanator]

Hmm @mkhutornyi I like your idea, but i think we need to know why this is now necessary - a.k.a. what's different in prod now & how did things change?

[mountiny]

I am afraid the SWM developers will be ooo mostly so we should not wait for them. @mkhutornyi could you pinpoint whats the rootcause of this being different in staging than in production? thanks!

[mkhutornyi]

Before, we used FastImage and onLoad callback was called even when width/height layout is not set.
But expo image doesn't seem to send onLoad callback when size is 0

App/src/components/ImageWithSizeCalculation.tsx

Line 89 in 881a83b

onLoad={imageLoadedSuccessfully}

This is supposed to call updateImageSize which resize image to correct width/height:

App/src/components/ThumbnailImage.tsx

Lines 86 to 94 in 6365aeb

	const updateImageSize = useCallback(
	({width, height}: UpdateImageSizeParams) => {
	const {thumbnailWidth, thumbnailHeight} = calculateThumbnailImageSize(width, height, windowHeight);
	setCurrentImageWidth(thumbnailWidth);
	setCurrentImageHeight(thumbnailHeight);
	},
	[windowHeight],
	);

[mkhutornyi]

My solution still has issue of showing default size (200, 200) briefly before getting correct size.
In production, it's not completely shown before getting correct size.
But, this should at least unblock deploy

[asahi0419]

It seems like you are facing an issue where the default size (200, 200) is briefly shown before getting the correct size when using the updateImageSize function. I think the solution is to set an initial state for thumbnailWidth and tumbnailHeight before calculating and updating them. In this way, the
component rendering will have the correct initial values before the asynchronous calculation completes.

Here is my answer.

const updateImageSize = useCallback(
  async ({ width, height }: UpdateImageSizeParams) => {
    // Set initial values to the default size
    let thumbnailWidth = 200;
    let thumbnailHeight = 200;

    // Calculate the thumbnail size asynchronously
    const { thumbnailWidth: calculatedWidth, thumbnailHeight: calculatedHeight } = await calculateThumbnailImageSize(
      width,
      height,
      windowHeight
    );

    // Update the state with the correct size
    thumbnailWidth = calculatedWidth;
    thumbnailHeight = calculatedHeight;

    setCurrentImageWidth(thumbnailWidth);
    setCurrentImageHeight(thumbnailHeight);
  },
  [windowHeight],
);

In this code, by setting the initial values before the asynchronous calculation, you ensure that the default size is set before the component renders. The await keyword is used to wait for the asynchronous calculation to complete before updating the state with the correct size.

(I have assume that calculate ThumbnailmageSize returns a Promise or is an asynchronous function. If it is a synchronous function, you can remove the async keyword and the await statement.)

if my answer makes any problem or make sense to you plz ping me.

[melvin-bot]

📣 @kevinpetersson! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[asahi0419]

It seems like you are facing an issue where the default size (200, 200) is briefly shown before getting the correct size when using the updateImageSize function. I think the solution is to set an initial state for thumbnailWidth and tumbnailHeight before calculating and updating them. In this way, the
component rendering will have the correct initial values before the asynchronous calculation completes.

Here is my answer.

const updateImageSize = useCallback(
  async ({ width, height }: UpdateImageSizeParams) => {
    // Set initial values to the default size
    let thumbnailWidth = 200;
    let thumbnailHeight = 200;

    // Calculate the thumbnail size asynchronously
    const { thumbnailWidth: calculatedWidth, thumbnailHeight: calculatedHeight } = await calculateThumbnailImageSize(
      width,
      height,
      windowHeight
    );

    // Update the state with the correct size
    thumbnailWidth = calculatedWidth;
    thumbnailHeight = calculatedHeight;

    setCurrentImageWidth(thumbnailWidth);
    setCurrentImageHeight(thumbnailHeight);
  },
  [windowHeight],
);

In this code, by setting the initial values before the asynchronous calculation, you ensure that the default size is set before the component renders. The await keyword is used to wait for the asynchronous calculation to complete before updating the state with the correct size.

(I have assume that calculate ThumbnailmageSize returns a Promise or is an asynchronous function. If it is a synchronous function, you can remove the async keyword and the await statement.)

if my answer makes any problem or make sense to you plz ping me.

[c3024]

With RNFastImage we had this issue only on Android which was fixed with a patch.

With Expo Image it is on iOS as well.

I think properly addressing the falsy values here

App/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js

Lines 48 to 49 in 6365aeb

	const imageWidth = htmlAttribs['data-expensify-width'] ? parseInt(htmlAttribs['data-expensify-width'], 10) : undefined;
	const imageHeight = htmlAttribs['data-expensify-height'] ? parseInt(htmlAttribs['data-expensify-height'], 10) : undefined;

like @mkhutornyi suggested is a good approach.

[melvin-bot]

📣 @mkhutornyi 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[mountiny]

@mkhutornyi can you raise the pr? I think that showing briefly wrong size is fine for now to unblock deploy

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[mountiny]

Fixed in staging

[melvin-bot]

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

[melvin-bot]

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

• [CP Staging] fix pdf not displaying on native #33642

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

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @parasharrajat requires payment through NewDot Manual Requests
• @mkhutornyi requires payment automatic offer (Contributor)

[melvin-bot]

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:

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

[parasharrajat]

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:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: Looks like a backend change caused the dimensions to return as 0 which triggered a buggy code from Fix resizes on image upload #8928 causing this issue.
• [@parasharrajat] 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/8928/files#r1437162294
• [@parasharrajat] 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: Not needed
• [@parasharrajat] Determine if we should create a regression test for this bug. NA. Already exists.
• [@parasharrajat] 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.

[melvin-bot]

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

• [CP Staging] fix pdf not displaying on native #33642

If no regressions arise, payment will be issued on 2024-01-04. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @parasharrajat requires payment through NewDot Manual Requests
• @mkhutornyi requires payment automatic offer (Contributor)

[melvin-bot]

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:

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

[parasharrajat]

Checklist is posted #33626 (comment)

[melvin-bot]

@peterdbarkerUK, @parasharrajat, @mountiny, @mkhutornyi Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@peterdbarkerUK, @parasharrajat, @mountiny, @mkhutornyi Still overdue 6 days?! Let's take care of this!

[peterdbarkerUK]

Payment summary:

• $500 to @mkhutornyi paid via upwork for PR
• $500 to @parasharrajat via newdot (please send a manual request, CC @JmillsExpensify )

[parasharrajat]

@peterdbarkerUK Feel free to close this. I will request it later.

[parasharrajat]

Payment requested as per #33626 (comment)

[JmillsExpensify]

$500 approved for @parasharrajat