[HOLD for payment 2023-05-05] [$2000] Uploading gif image seems to fluctuate the image while previewing it immediately after uploading

[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!

---

Action Performed:

1. Open web chrome and open any chat and send a message
2. Now click on profile icon and upload a gif image. Crop the image to full.
3. From the chat, click on your profile picture and preview it.
4. First Bug : Notice that the image fluctuates
5. Second Bug : Also, sometimes an empty attachment also gets displayed (This is not easily reproducible) For this, upload a large jpg/png file without cropping and try to preview the image immediately. It displays an empty attachment. - Not able to reproduce for me

Expected Result:

The image shouldn’t fluctuate while trying to preview the image

Actual Result:

The image fluctuates while trying to preview the image

Workaround:

unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.2.89-0
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
Notes/Photos/Videos:

Recording.62.mp4

photoerror-2023-03-26_14.16.52.mp4

photoerror-2023-03-26_14.09.59.mp4

Expensify/Expensify Issue URL:
Issue reported by: @priya-zha
Slack conversation:
https://expensify.slack.com/archives/C049HHMV9SM/p1679819714964549
View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01afe7983a04d30b23
• Upwork Job ID: 1643073372561469440
• Last Price Increase: 2023-04-25

[MelvinBot]

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

[MelvinBot]

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

[marcaaron]

Feels questionable whether we should allow uploading an animated gif as a profile pic.

[priya-zha]

video-2023-03-27_16.54.41.mp4

@marcaaron It happens with normal images too. Can you please try to view any profile picture of the user. It seems to fluctuate with normal images too, not only with gifs.

[stephanieelliott]

Hm, I'm not seeing that behavior occur on static images, like .jpg. For me, it's only happening with .gifs.

Agree with @marcaaron that we probably shouldn't support .gifs in that field, posted in Slack to discuss: https://expensify.slack.com/archives/C01SKUP7QR0/p1680106537787609

[stephanieelliott]

OK it sounds like we had previously agreed to allow a GIF and convert it to JPG (context). So I suppose this does need to be fixed.

[MelvinBot]

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

[MelvinBot]

Current assignee @stephanieelliott is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

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

[MelvinBot]

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

[tienifr]

Proposal

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

The image fluctuates while trying to preview the image

What is the root cause of that problem?

The issue is in this line where we're setting the width and height of the Pressable that wraps the Image to 100% if the image width and height are 0 (a.k.a the image is still loading).

When the images finish loading, the image shows as full size (100%) while the image is very small (less than the full size), so it immediately resizes to its actual small size, causing the flicker.

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

We should not set width and height of the image container if the image is still loading. This can be done by return undefined in here

App/src/styles/StyleUtils.js

Line 93 in 3c41171

function getZoomSizingStyle(isZoomed, imgWidth, imgHeight, zoomScale, containerHeight) {

if the image is loading.

We can pass the loading state in by using the this.state.isLoading of the ImageView component.

Please note we're already trying to do the same here

App/src/components/ImageView/index.js

Line 235 in 6bfc8bb

style={this.state.isLoading ? undefined : [

with the Image component, but it doesn't work well in this case since the Image and its container renders in different times so there is race condition. That logic should be applied to the container of the image instead, as suggested above. We can safely remove this check for web if the container check above is applied (however that check in case of mWeb here should still be kept since there's no Pressable container on mWeb).

What alternative solutions did you explore? (Optional)

NA

[danieldoglas]

@tienifr don't know if you saw this comment, but the expectation here is to convert the GIF to a JPG. I don't think your proposal is doing that.

[tienifr]

convert the GIF to a JPG

@danieldoglas yes I saw that commend, this is an existing feature on staging, the Avatar upload is already doing this for uploaded GIFs.
I think what @stephanieelliott means here is that since uploading GIFs is a valid operation, this bug of flickering images should be fixed, my proposal addresses that.

Also just to clarify the issue occurs even for regular png/jpg images (not just GIFs), the key here is to use an image with width and height smaller than that of the desktop web container width and height, you can try with an image of just 100px by 100px.

[PrashantMangukiya]

Proposal

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

Small Image flicker on web and desktop when opened via attachment modal, this is applicable for gif or regular image.

What is the root cause of that problem?

Within ImageView/index.js we are using getZoomSizingStyle() function at line 277

App/src/components/ImageView/index.js

Line 277 in 4ee21e9

...StyleUtils.getZoomSizingStyle(this.state.isZoomed, this.state.imgWidth, this.state.imgHeight, this.state.zoomScale,

This getZoomSizingStyle() function is as under.

App/src/styles/StyleUtils.js

Lines 201 to 207 in 4ee21e9

	function getZoomSizingStyle(isZoomed, imgWidth, imgHeight, zoomScale, containerHeight, containerWidth) {
	if (imgWidth === 0 || imgHeight === 0) {
	return {
	height: isZoomed ? '250%' : '100%',
	width: isZoomed ? '250%' : '100%',
	};
	}

When component load at that time isZoomed is false, so it will return height and width 100% as shown on line 204, 205 so image show full size until correct dimension received. This is the root cause.

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

Within StyleUtils.js update getZoomSizingStyle() function as shown below.
i.e. When image dimension not decided return undefined.

if (imgWidth === 0 || imgHeight === 0) {
   return {
       //height: isZoomed ? '250%' : '100%',    // Old Code
       height: isZoomed ? '250%' : undefined,   // Updated Code
       //width: isZoomed ? '250%' : '100%',       // Old Code
       width: isZoomed ? '250%' : undefined,   // Updated Code
   };
}

What alternative solutions did you explore? (Optional)

Just return undefined object as shown below:

  if (imgWidth === 0 || imgHeight === 0) {
      return undefined;
  }

Result

Flicker-Solved.mp4

[eVoloshchak]

Reviewing proposals above tomorrow

[eVoloshchak]

I like @tienifr's proposal, Hide image until finished loading to prevent showing preview with wrong dimensions is a reasonable approach.

Second Bug : Also, sometimes an empty attachment also gets displayed (This is not easily reproducible) For this, upload a large jpg/png file without cropping and try to preview the image immediately. It displays an empty attachment. - Not able to reproduce for me

I've managed to reproduce this (it's easier if you have a slow connection):

1. Click on profile icon and upload a gif image (can be any size)
2. From the chat, click on your profile picture and preview it (you have to do this immediately, so using slow 3G preset helps)

@tienifr, are you able to reproduce this?

[tienifr]

you have to do this immediately, so using slow 3G preset helps

@eVoloshchak thanks for the explanation on the reproduction steps, I can reproduce it now, here's my proposal for that issue (it's a very different issue with different root cause)

Proposal

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

Sometimes an empty attachment gets displayed when uploading a large avatar in a slow network connection, and then view the avatar immediately in the chat detail page.

What is the root cause of that problem?

The root cause is here

App/src/components/AttachmentView.js

Line 84 in 64f4a78

if (Str.isImage(props.source) || (props.file && Str.isImage(props.file.name))) {

where we're checking if the attachment is an image or not.

When we upload the image and view it immediately, the image URL is a blob blob:http://localhost:8080/74a3cc86-0cc8-4e09-a9cb-509e23979e90 so it doesn't have the image extension for the image check. The file here is also empty because it will only be passed in the attachment view if you just uploaded it in the avatar upload page, here we're accessing the avatar in a completely different component (chat details).

So the component is unable to check that the source passed in is an image, thus it displays the paper clip icon as a fallback.

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

There're certain scenarios where we know exactly that the AttachmentView will display an image, for example here

App/src/pages/DetailsPage.js

Line 122 in 6ccfc4b

source={ReportUtils.getFullSizeAvatar(details.avatar, details.login)}

where we display the avatar.

We can just pass an isImage prop to the AttachmentModal on that page (since it's displaying an avatar, we know it is an image), then pass isImage down to the AttachmentView.

And update the condition here

App/src/components/AttachmentView.js

Line 84 in 64f4a78

if (Str.isImage(props.source) || (props.file && Str.isImage(props.file.name))) {

so that if isImage is true, it will render an image, otherwise it will depend on the checking of the source and file as usual

What alternative solutions did you explore? (Optional)

• Another solution we can use is to pass the file name (aside from the avatar which will be the blob URL) to the optimistic data when we upload the avatar, then pass the avatar file name to the AttachmentModal and use it as the default file state of the AttachmentModal. This will also work and quite straight-forward.
• Or we can fetch the blob when we check the file type of the blob, but I think this will be the last option to evaluate since it affects the app performance.

[MelvinBot]

@danieldoglas @eVoloshchak @stephanieelliott 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!

[MelvinBot]

📣 @tienifr You have been assigned to this job by @danieldoglas!
Please apply to this job in Upwork 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 📖

[tienifr]

@danieldoglas @eVoloshchak This PR: #17454 is ready for review. Thanks

[stephanieelliott]

PR is under review!

[MelvinBot]

Upwork job price has been updated to $2000

[danieldoglas]

Manually increasing the bounty since we found another bug while testing that will be fixed together in this PR.

[stephanieelliott]

PR is on staging

[MelvinBot]

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

[MelvinBot]

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

• fix: 16528 upload gif image fluctuate #17454

If no regressions arise, payment will be issued on 2023-05-05. 🎊

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

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[MelvinBot]

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:

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

[danieldoglas]

@eVoloshchak can you please fill the checklist above?

@stephanieelliott I think this is ready for payment

[eVoloshchak]

• The PR that introduced the bug has been identified. Link to the PR: Add feature to view Profile Picture in full screen #8483

• 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: Add feature to view Profile Picture in full screen #8483 (comment)

• 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: This is an edge case since it requires you to preview the image immediately after uploading, so it's hard to reproduce unless on a slow connection. I've asked here whether we should add "I throttled my network connection and tested it with slow connection to ensure it matches the expected behaviour" item to checklist

• Determine if we should create a regression test for this bug.
  Is it easy to test for this bug? Yes
  Is the bug related to an important user flow? (For example, adding a bank account) No
  Is it an impactful bug? No

  I don't think regression test is needed here. This is reproducible using only a slow connection and has a very low impact

[stephanieelliott]

Getting ready to pay (just waiting on offer acceptance in Upwork) - looks like the job was assigned on 4/13, all reviewed and all changes resolved on 4/18. Then on 4/18 another bug surfaced, we discussed and eventually decided to fix it in the same PR on 4/21. PR was finally merged on 4/16.

For the purposes of calculating the speed bonus/penalty, I am going to use the date that comments were resolved on the original bug (4/18) as the "merge date". 4/13 assignment vs 4/18 merge = 5 days, so this one is not eligible for the speed bonus nor will it face a penalty, so the payment will be $2,000.

[tienifr]

@stephanieelliott hi, I think there're 2 days of weekends between 4/13 and 4/18, kindly consider that in the calculations as well.
Thanks you!

[stephanieelliott]

Oh, thanks @tienifr I hadn't thought about that -- thanks for calling that out! Everyone is paid out, C and C+ w/ bonus