[HOLD for payment 2022-06-16] [HOLD for payment 2022-06-15] [$500] Uploading an image causes the thumbnail to resize 3 times

[mvtglobally]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Open any chat
2. Upload a larger file to share

Expected Result:

Thumbnail should load once uploaded, or if it's a large file it should display a spinner until it's uploaded.

Actual Result:

If you share an image, the uploading thumbnail resizes 3 times. Then it finalizes the upload.

Workaround:

Just wait for the file to upload but it's a jarring experience.

Platform:

Where is this issue occurring?

• Web

Version Number: 1.1.52-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

If you click + > add attachment in a DM, and then select this attachment:

You'll notice it changes to this momentarily:

And then this:

Upwork job URL: https://www.upwork.com/jobs/~01ca59e5811fecf4d5
Issue reported by: @cead22
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1649352167230159

View all open jobs on GitHub

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @michaelhaxhiu (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (Exported)

[melvin-bot]

Triggered auto assignment to @chiragsalian (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[michaelhaxhiu]

Job link - https://www.upwork.com/jobs/~01ca59e5811fecf4d5

[michaelhaxhiu]

Cleaned up GH post a little for clarity.

[rushatgabhane]

I hope there's a way we can bring down the number of resize to 0, but here's a quick

Proposal

I can bring the resizing down to 1 time. This could be done by using the same thumbnail styles as the upload preview thumbnail. i.e thumbnailWidth: 200 and thumbnailHeight: 150

Changes will be made in this file.

App/src/components/ThumbnailImage.js

Lines 35 to 37 in fd0f83a

	this.state = {
	thumbnailWidth: 200,
	thumbnailHeight: 200,

How it'll look like

Screen.Recording.2022-04-12.at.4.57.34.AM.mov

[cead22]

This is still pretty janky, can we make it so:

1. The thumbnail that is shown immediately has the same dimensions as the image that's shown at the end of the flow
2. The thumbnail that is shown immediately doesn't have a spinner on top of it
3. The "blank thumbnail" is never shown

cc @JmillsExpensify

[JmillsExpensify]

Love all those suggestions!

[trjExpensify]

@rushatgabhane are you cool with those suggestions from @cead22 to update the proposal? Would be great to get going on this one if everyone is aligned!

[rushatgabhane]

@trjExpensify yes, those are good suggestions.
But I don't think we can get the dimensions of an image before it's downloaded. So implementing those suggestions might not be plausible.

I'd love to be proven wrong tho.

[chiragsalian]

Im unsure if we can get dimensions of an image before its downloaded. With that being said i'm not sure why we show the blank thumbnail. Can that be removed?

Besides that i think the rest is fine. Personally i feel like the first thumbnail i.e., the preview thumbnail with loading icon on top should be a smaller thumbnail, with maybe some opacity so that the loading icon is more obvious.

But i'll let @cead22 chime in here since he might have more stronger opinions.

[cead22]

This is still pretty janky, can we make it so:

1. The thumbnail that is shown immediately has the same dimensions as the image that's shown at the end of the flow
2. The thumbnail that is shown immediately doesn't have a spinner on top of it
3. The "blank thumbnail" is never shown

Can't we get the dimensions of the local image? If not, I think 2 & 3 are still worth doing

[chiragsalian]

Carlos, i didn't follow your point 2. The thumbnail thats shown immediately after "Send" is clicked does have a spinner on top of it. Did you mean something else?

[cead22]

Sorry, if you go to the comment above, I say "can we make it so:..." and list those points. I updated the previous comment to make it clearer

[michaelhaxhiu]

Started a discussion in #expensify-open-source here about further improvements we can make to the flow of image upload (let's not block this job on the discussion though).

[chiragsalian]

@allroundexperts, unfortunately we had to revert your PR due to this regression issue found by QA. Can you recreate your PR and address that issue as well.

[allroundexperts]

I'll check that.

…

On Wed, Jun 8, 2022 at 12:50 AM Chirag Chandrakant Salian < ***@***.***> wrote: @allroundexperts <https://github.com/allroundexperts>, unfortunately we had to revert <#9343> your PR due to this regression issue <#9334> found by QA. Can you recreate your PR and address that issue as well. — Reply to this email directly, view it on GitHub <#8590 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHFJUUEYZ56YKCZCFV3OMPDVN6RZDANCNFSM5TCA4OKQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[allroundexperts]

@chiragsalian Created a new PR:
#9346

[chiragsalian]

Nice, thank you for the quick fix. Shall review shortly.

[chiragsalian]

The new PR was created, reviewed and merged 🙂

[melvin-bot]

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

• Fix resizes on image upload #8928

If no regressions arise, payment will be issued on 2022-06-15. 🎊

[melvin-bot]

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

• Fix resizes on image upload regression issue #9346

If no regressions arise, payment will be issued on 2022-06-16. 🎊

[michaelhaxhiu]

Ok so payment is due today. I'm seeing a regression here and so the payout structure would be:

• $500 for @allroundexperts
• $500 for @Santhosh-Sellavel
• $brownie points for @cead22 the bug reporter

[michaelhaxhiu]

Awaiting confirmation from Chirag for this, as I'm alittle unsure about this regression 👆

[chiragsalian]

I don't feel a penalty for this regression is required for C+ because the PR was testing thumbnail loading and not really the attachment modal. So QA finding a regression with attachment modal would have been very hard to catch in the normal PR and hence i don't feel there should be a penalty for the C+.
(P.S i also tested the PR on different devices and would not have thought to test the attachment modal the way QA did)

[michaelhaxhiu]

Got it, so it seems you are saying this was out of scope of C+ responsibility.

Did we make the necessary changes to our process to ensure we don't miss this testing step again?

[chiragsalian]

I feel like the issue caught by QA was out of the scope of the C+ responsibility since its quite the edge case.

Also technically even QA tested and cleared the PR without realizing any issue with it and later found the regression issue. Most likely from their checklist of complete app testing steps.

Did we make the necessary changes to our process to ensure we don't miss this testing step again?

I'm not sure what can be done differently other than expecting our C+ to do the same regression testing as QA but this list is huge and not worth it for the C+ to go over so im unsure. Maybe we can recommend people to test a bit more beyond the PR's expectation. Not sure but thats a discussion for slack.

[michaelhaxhiu]

Got it! Thanks for the context.

In that case, it seems like the feedback is for QA so they don't miss it next time. Sometimes our regressions are QAs fault, I think that's going to happen from time to time!

[michaelhaxhiu]

@allroundexperts is paid.

@Santhosh-Sellavel can you apply for this upwork job? the OG link expired

[Santhosh-Sellavel]

@michaelhaxhiu Applied for the job!

[michaelhaxhiu]

All are paid.