[HOLD for payment 2023-06-23] [$1000] Android/iOS - Chat - jpg image opens always with loading spinner in preview

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open the App
2. Login with any account
3. Go to any chat
4. Send a jpg image
5. Open the image you just sent in the preview

Expected Result:

jpg image opens the first time with the loading spinner, but the next times without it

Actual Result:

jpg image opens always with loading spinner

Workaround:

Unkown

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

Reproducible in staging?: Yes

Reproducible in production?: Yes

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: Any additional supporting documentation

Bug6075968_Record_2023-05-31-23-12-46_4f9154176b47c00da84e32064abf1c48__1_.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010b826cfb93c10d6b
• Upwork Job ID: 1665390224439746560
• Last Price Increase: 2023-06-04

[melvin-bot]

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

[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

[sakluger]

I was able to reproduce this consistently on both Android and iOS native apps, it's working fine on other platforms. Also, PDF works fine on native apps.

This may be a duplicate of #19369, I've asked over there if it's the same root cause and if we should fix them together.

[melvin-bot]

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

[sakluger]

Sounds like this bug is unique from the other one, adding the external label. @jliexpensify I've also added you as another BZ team member to watch over this since I'm out all next week.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~010b826cfb93c10d6b

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @bondydaa (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.

jpg image opens always with loading spinner

What is the root cause of that problem?

It's here

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

Line 107 in 96adb8e

this.state.interactionPromise = InteractionManager.runAfterInteractions(() => {

where we always wait until navigation animation completes before setting isLoading to false.

So it will always show loading indicator until the screen finish animating. We don't have such logic in Web.

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

We can remove that InteractionManager.runAfterInteractions logic and set isLoading to false directly after the image finishes loading. There's this comment https://github.com/Expensify/App/blob/96adb8e58e53fd76c7a316234dab322643dfd479/src/components/ImageView/index.native.js#LL106C1-L106C1 but it looks fine to me, no stutter so I think we can remove it. It also makes it consistent with Web.

What alternative solutions did you explore? (Optional)

There'll be the loading appearing for a very small time (<100ms) if the image is large since there still needs to be some time for the cached image to be loaded from fast-image. If we want to improve on this then can show isLoading loading indicator only after the screen finishes navigating, or inside debounce so that if the image's loading for a very short time (<100ms) we won't show the loading. This will make for a slightly better UX.

[melvin-bot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[s77rt]

@tienifr There seems to be some lag. Can you please attach another video that keeps the InteractionManager.runAfterInteractions delay.

[tienifr]

Screen.Recording.2023-06-08.at.11.25.03.mov

Here's the video that keeps the InteractionManager.runAfterInteractions
@s77rt what do you think?

[s77rt]

@tienifr I think we still have some lag in the second video as well. Do you see that?

[tienifr]

@s77rt Yes, I see the two videos are the same, even though we have some lag but I think it doesn't affect the UX too much

[s77rt]

@tienifr Since the lag is not a regression here we can move forward and handle the lag bug separately.

🎀 👀 🎀 C+ reviewed

cc @bondydaa

[bondydaa]

👍 sounds good, assigning

[melvin-bot]

📣 @tienifr You have been assigned to this job by @bondydaa!
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]

@s77rt The PR: #20545 is ready for review

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @tienifr got assigned: 2023-06-05 16:23:43 Z
• when the PR got merged: 2023-06-14 13:22:57 UTC
• days elapsed: 7

On to the next one 🚀

[jliexpensify]

Invited @tienifr and @s77rt

[tienifr]

@jliexpensify @s77rt I think Melvin's PR timeline calculation here is incorrect (assignment time is incorrect), I was assigned on 2023-06-09 16:17:00 UTC, and there're 2 days of weekend in between so merging is under 3 working days.

[jliexpensify]

Cool, good to know - cheers @tienifr . We'll add the 50% bonus then.

[sakluger]

Sent offers to @tienifr and @s77rt in Upwork.

[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.3.28-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: 19920 image opens always with loading spinner #20545

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

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 - @tienifr
• Contributor+ that helped on the issue and/or PR - @s77rt

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

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

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

[s77rt]

• The PR that introduced the bug has been identified: Fix: Blurred images in attachment view on mobile devices #6442
• The offending PR has been commented on: Fix: Blurred images in attachment view on mobile devices #6442 (comment)
• A discussion in #expensify-bugs has been started: https://expensify.slack.com/archives/C049HHMV9SM/p1687174694104909
• Determine if we should create a regression test for this bug: No, we have a test that covers that https://expensify.testrail.io/index.php?/cases/view/2583751

[sakluger]

All paid, thanks everyone!