Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$250][HOLDnecolas/react-native-web#2709] Tab - When switching tabs, the Custom Profile Avatar shows delayed #45853

Open
1 of 6 tasks
lanitochka17 opened this issue Jul 20, 2024 · 60 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Monthly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Jul 20, 2024

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


Version Number: 9.0.10-2
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Issue found when executing PR #45663

Action Performed:

PreCondition: User has a custom avatar added to their profile.

  1. Access staging.new.expensify.com
  2. Sign into a valid account (Beta enabled)
  3. Go to inbox tab
  4. Go to Search tab
  5. Go to Profile tab

Expected Result:

User expects the Avatar image to load instantly upon changing tabs

Actual Result:

The avatar image blinks a takes a couple of seconds to load

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

Bug6548102_1721483392280.Avatar_shows_in_a_delayed_manner_when_switching_tabs_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ec9d563c4ff0e779
  • Upwork Job ID: 1816230532262348738
  • Last Price Increase: 2024-08-07
  • Automatic offers:
    • dominictb | Contributor | 103496599
Issue OwnerCurrent Issue Owner: @s77rt
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 20, 2024
Copy link

melvin-bot bot commented Jul 20, 2024

Triggered auto assignment to @strepanier03 (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@lanitochka17
Copy link
Author

@strepanier03 FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@lanitochka17
Copy link
Author

We think that this bug might be related to #vip-vsp

@dominictb
Copy link
Contributor

dominictb commented Jul 23, 2024

Edited by proposal-police: This proposal was edited at 2024-08-08 03:11:24 UTC.

Proposal

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

The avatar image takes a couple of seconds to load

What is the root cause of that problem?

From here we can see that react-native-web's Image component won't show the image immediately if the image is not in the state LOADED, and in order to land in that state when the component mount, the image should be in the cache as dictated here

And in here, we never prefetch the avatar URL, hence the issue.

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

To solve this issue in react-native-web library, we can simply cache the image after load in here the same way in here, making sure that the image is in the LOADED state for subsequent access from the parent component

if(!ImageUriCache.has(uri)) {
   ImageUriCache.add(uri);
   ImageUriCache.remove(uri);
}

Some extra consideration: We should even add a param in load function as well as the respective prop in the parent components to control the caching behavior. For example, we might don't want to cache the image naively this way if it is loaded from local file or when it goes with defaultSource here. That can be ironed out later in the PR

OUTDATED solution

We should add a mechanism in the BaseImage.tsx component to prefetch the source, allowing the image to be displayed immediately in web: In here, add a call to prefetch the URI. We don't need to un-prefetch, or remove the URI from the cache as react-native-web only keep track of 256 URIs in the cache, and will remove least-accessed URI once the count has reached its limit

useEffect(() => {
        // shouldPrefetchUri is a new introduced prop, and we can control it from the parent component
        if(shouldPrefetchUri && props?.source?.['uri']) {
            // prefetch the image
            RNImage.prefetch(props.source?.['uri']);
        }
    }, [shouldPrefetchUri, props.source]);

In here, we can control that we only prefetch for non-authenticated source, but we can extend later for authenticated-required source.

What alternative solutions did you explore? (Optional)

Even more exact solution on web, suggested here https://stackoverflow.com/questions/2446740/post-loading-check-if-an-image-is-in-the-browser-cache/50111407#50111407, we can utilize this logic to check and set the initial state correctly. To achieve that, updateImageLoader.has function here

let globalImage = new window.Image(); // or we can initialize new image for every `ImageLoader.has` invocation.
....
has(uri) {
    globalImage.src = uri;
    let result = globalImage.complete;
    globalImage.src = '' // unset the src to prevent firing extra request
    return;
}

We can easily verify that this logic work on web by opening the devtools > network tab and try toggle disable cache option. If the cache is available, the image will load almost immediately from the second time.

@melvin-bot melvin-bot bot added the Overdue label Jul 23, 2024
Copy link

melvin-bot bot commented Jul 24, 2024

@strepanier03 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@strepanier03
Copy link
Contributor

Working on this now.

@melvin-bot melvin-bot bot removed the Overdue label Jul 24, 2024
@strepanier03
Copy link
Contributor

Repro'd all good here.

@strepanier03 strepanier03 added the External Added to denote the issue can be worked on by a contributor label Jul 24, 2024
@melvin-bot melvin-bot bot changed the title Tab - When switching tabs, the Custom Profile Avatar shows delayed [$250] Tab - When switching tabs, the Custom Profile Avatar shows delayed Jul 24, 2024
Copy link

melvin-bot bot commented Jul 24, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 24, 2024
Copy link

melvin-bot bot commented Jul 24, 2024

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

@s77rt
Copy link
Contributor

s77rt commented Jul 25, 2024

@dominictb

in order to land in that state when the component mount, the image should be in the cache

Isn't the image being cached?

@dominictb
Copy link
Contributor

dominictb commented Jul 25, 2024

@s77rt please check this logic, then you will see that by default the image URL is not in the cache, as nowhere in the code did we call Image.prefetch function

@s77rt
Copy link
Contributor

s77rt commented Jul 26, 2024

@dominictb After an image is loaded the first time it should be cached. Next time you render the same image it should load from the cache. Why this is not the case here?

@dominictb
Copy link
Contributor

@s77rt it is true that once the image is loaded the first time, it is in the browser cache. But that doesn't mean this condition is true, and that means, the image is not shown as implemented here

So, to show the image immediately, we need to make this condition to be true, i.e: Call Image.prefetch for the image URI.

@s77rt
Copy link
Contributor

s77rt commented Jul 26, 2024

@dominictb Sounds to me like a bug in RNW. If we overwrite the initial state to be LOADED. Will the image load correctly on tab switch?

@dominictb
Copy link
Contributor

If we overwrite the initial state to be LOADED. Will the image load correctly on tab switch?

@s77rt you can, but it might defeat the use case where we want to use image placeholder (check this out) while the image is loading

@s77rt
Copy link
Contributor

s77rt commented Jul 27, 2024

@dominictb Will the image load correctly (from cache) in that case? (regardless of the placeholder)

@dominictb
Copy link
Contributor

Will the image load correctly (from cache) in that case? (regardless of the placeholder)

@s77rt yes.

@s77rt
Copy link
Contributor

s77rt commented Jul 28, 2024

@dominictb In that case the bug should be fixed at RNW level

@dominictb
Copy link
Contributor

@s77rt proposal updated with fix in react-native-web lib itself.

@s77rt
Copy link
Contributor

s77rt commented Jul 29, 2024

@dominictb Adding the image uri to the cache entries makes sense to me but a better solution is to make use of the browser cache, that is to modify the ImageUriCache.has method to check if a uri is already cached. Can you look into that?

@dominictb
Copy link
Contributor

@s77rt If the URI has been fetched at least one using ImageLoader.load, it is highly likely that the browser has already cached it (that's also the assumption in react-native-web). There's no direct way to check if the browser has actually cached it or not (for example: if the user clear the cache or disable the cache then we don't really have the way to know). Adding the URI to the ImageUriCache is just to ensure the image component is rendered and displayed immediately since the inital state will be LOADED if the image has been fetched at least once.

Copy link

melvin-bot bot commented Aug 12, 2024

📣 @dominictb 🎉 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 📖

@dominictb
Copy link
Contributor

Will work on the upstream issue and PR shortly.

Copy link

melvin-bot bot commented Aug 15, 2024

@strepanier03, @s77rt, @stitesExpensify, @dominictb Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Aug 15, 2024
@s77rt
Copy link
Contributor

s77rt commented Aug 15, 2024

Not overdue. @dominictb is going to raise a PR upstream (RNW)

@dominictb
Copy link
Contributor

@s77rt necolas/react-native-web#2709 upstream PR and linked issue are up

Copy link

melvin-bot bot commented Aug 17, 2024

@strepanier03 @s77rt @stitesExpensify @dominictb this issue is now 4 weeks old, please consider:

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@s77rt
Copy link
Contributor

s77rt commented Aug 17, 2024

Not overdue. Waiting on RNW PR feedback

Copy link

melvin-bot bot commented Aug 21, 2024

@strepanier03, @s77rt, @stitesExpensify, @dominictb Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Aug 21, 2024
@s77rt
Copy link
Contributor

s77rt commented Aug 22, 2024

Same ^

@melvin-bot melvin-bot bot removed the Overdue label Aug 22, 2024
@s77rt
Copy link
Contributor

s77rt commented Aug 22, 2024

@stitesExpensify Let's make this Weekly and hold on necolas/react-native-web#2709

@stitesExpensify stitesExpensify added Weekly KSv2 and removed Daily KSv2 labels Aug 22, 2024
@stitesExpensify stitesExpensify changed the title [$250] Tab - When switching tabs, the Custom Profile Avatar shows delayed [$250][HOLDnecolas/react-native-web#2709] Tab - When switching tabs, the Custom Profile Avatar shows delayed Aug 22, 2024
@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Sep 16, 2024
Copy link

melvin-bot bot commented Sep 16, 2024

This issue has not been updated in over 15 days. @strepanier03, @s77rt, @stitesExpensify, @dominictb eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@s77rt
Copy link
Contributor

s77rt commented Sep 16, 2024

Still on hold

@strepanier03
Copy link
Contributor

Put this in #quality.

@melvin-bot melvin-bot bot added the Overdue label Oct 30, 2024
@s77rt
Copy link
Contributor

s77rt commented Oct 31, 2024

@dominictb Seems like the upstream PR needs your attention. Please provide a reproducer for the repo maintainers to test

@melvin-bot melvin-bot bot removed the Overdue label Oct 31, 2024
@dominictb
Copy link
Contributor

@s77rt thanks, I'll check and update the upstream PR as soon as possible

@dominictb
Copy link
Contributor

@s77rt this is a little bit weird, I couldn't reproduce the original bug in react-native-web. I checked for both case: when enable cache and when disable cache. I believe the original bug, I believe, is partly due to #47041, which has already solved.

So what should be the next step here?

@s77rt
Copy link
Contributor

s77rt commented Nov 1, 2024

@dominictb Is the bug no longer reproducible? Did you try with a custom avatar?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Monthly KSv2
Projects
Status: No status
Development

No branches or pull requests

5 participants