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

[lanitochka17]

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 https://github.com/Expensify/App/pull/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
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/a44efc0b-4e34-4df1-88bf-d1ac53d9aa60

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 Owner

Current Issue Owner: @s77rt

[melvin-bot[bot]]

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]

@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]

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

[dominictb]

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](https://github.com/Expensify/App/blob/c5acf354eb6b443979ff23185fffca46a7438242/src/components/Image/BaseImage.tsx#L19), 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](https://github.com/Expensify/App/blob/c5acf354eb6b443979ff23185fffca46a7438242/src/components/Image/index.tsx#L44), 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[bot]]

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

[strepanier03]

Working on this now.

[strepanier03]

Repro'd all good here.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[s77rt]

@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]

@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]

@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]

@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]

@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]

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]

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

[dominictb]

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

@s77rt yes.

[s77rt]

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

[dominictb]

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

[s77rt]

@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]

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

[s77rt]

@dominictb Checking the existing cache is more reliable than maintaining a second one. I found this solution https://stackoverflow.com/a/50111407 that may work. Can you check it out?

[dominictb]

@s77rt I will check it and get back to you asap.

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[dominictb]

@s77rt updated as alternative solution in the proposal

[s77rt]

@dominictb Why we have a globally defined Image? Also please make sure if an image does not exist in cache we don't fetch it.

instead of check using ImageLoader.has, which under the hood use ImageUriCache to check, we can use ImageLoader.isCached

We don't want to replace ImageLoader.has. Instead that method should make use of new isCached mechanism

[dominictb]

@s77rt proposal updated with more info

[s77rt]

@dominictb ImageUriCache.has is what we should update.

Also please make sure if an image does not exist in cache we don't fetch it.

Can you look at that as well?

[dominictb]

@s77rt

ImageUriCache.has is what we should update.

I think ImageUriCache class is for different purpose. I prefer to keep the scope separated, hence modifying ImageLoader.has.

Also please make sure if an image does not exist in cache we don't fetch it.

https://github.com/necolas/react-native-web/blob/54c14d64dabd175e8055e1dc92e9196c821f9b7d/packages/react-native-web/src/exports/Image/index.js#L276 we can add an early return here if state is LOADED

if(state === LOADED) return;

[s77rt]

@dominictb

we can add an early return here if state is LOADED

This is unrelated to the new cache function. Notice that after creating a Image and setting the src, the browser fill fetch that image which is an unwanted behaviour

[dominictb]

looking into it today.

[melvin-bot[bot]]

@strepanier03 @s77rt 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!

[s77rt]

Still looking for proposals

[dominictb]

@s77rt

Notice that after creating a Image and setting the src, the browser fill fetch that image which is an unwanted behaviour

I think react-native-web's Image component doesn't handle it pretty well either. We can see in here it fires a request once, and when we render the hiddenImage here it fires the request the second time. This is based on a common assumption that browser will turn on the cache, so firing the image request twice will increase the chance that the second image request when rendering the img component will hit the cache (thus saving 1 network round trip). But when the cache is off, we will see that for each image, there are 2 network requests fired off.

Solution

So, the ideal solution for that is to re-use the window.Image that is used for fetching/cache-checking the image, for displaying. The idea looks like this

let imageRef = useRef(new Image());
let imageUri = props.source
let prevImageUri = usePrevious(imageUri);

[state, updateState] = useState(() => {
     imageRef.current.src = imageUri
     if(imageRef.current.complete) return LOADED
     return IDLED
}) // the initial state will be determined by checking if the image is already in the cache

React.useEffect(() => {
   if(imageUri !== prevImageUri) {
      // load the image uri here
      imageRef.current.onload = ...
      imageRef.current.onerror = ....
     imageRef.current.src = imageUri
      // if the image is in the cache we should set the state to LOADED immediately, this will allow the image to be displayed as soon as possible
      if(imageRef.current.complete) {
          updateState(LOADED)
      } else {
         updateState(LOADING)
     }
     //...handle other cases
   }
},[...])

// change the logic of the `hiddenImage` to display display default source 
// (displaying the alternative/placeholder image while waiting for the main image to be loaded)
var hiddenImage =  props.defaultSource && state !== LOADED ? createElement('img', {
    alt: ariaLabel || '',
    style: styles.accessibilityImage$raw,
    draggable: draggable || false,
    ref: hiddenImageRef,
    src: props.defaultSource
  }) : null;

We will insert the imageRef.current after this View (right at the place of the hiddenImage, as when the state is LOADED, the hiddenImage should already be null)

useLayoutEffect(() => {
    if(state === LOADED) {
      if(backgroundViewRef.current) {
        mainImageRef.current.style = styles.accessibilityImage$raw;
        mainImageRef.current.draggable = draggable || false;
        mainImageRef.current.alt = ariaLabel || '';
        backgroundViewRef.current.insertAdjacentElement('afterend', mainImageRef.current); // insert the image after the view
      }
    } else {
      mainImageRef.current.remove();
    }
  }, [state])

[s77rt]

@dominictb I don't see how your are proposing to fix the unnecessary fetch when checking for cache. Can you please restate your solution in plain English? - This should be related only to ImageLoader.has and not to the logic that loads the image, i.e. calling ImageLoader.has should not make a fetch request.

[dominictb]

@s77rt sorry it should have been clearer like this:

• My original proposal is to use the ImageUriCache in react-native-web. This is based on the following assumption: If the image is load once then it should be in the cache, and subsequent request will hit the cache (until the user expires/clears/disables the cache). This is a simple yet heuristic approach.

• The alternative approach is based on here. This is more reliable, but with a side effect : the browser will make an additional request. Currently I couldn't find other way to check whether an image is in the cache without this side effect. So one way to reduce the amount of fetch request is to re-use the image = new window.Image() variable that we're using to check if the image is in the cache. The idea would look like this:

 let image = new window.Image(); // create a new image component:
image.src = uri
 if(image.complete) {
     document.appendChild(image) // if the image is in the cache -> display the image using DOM manipulation
  } else {
     // wait until the image is loaded
    // display the placeholder component
    // once the image is loaded, unmount the placeholder component and add the `image` component using DOM manipulation
  }

With this way, by 1 single request, we can know if the image is in the cache or not, and if not in the cache then fetch the image from the network and display it properly.

Detail implementation on RNW is https://github.com/Expensify/App/issues/45853#issuecomment-2271132202

[s77rt]

@dominictb

document.appendChild(image) // if the image is in the cache -> display the image using DOM manipulation

Why are you trying to display the image? We just want to check if the image is in cache or not. Also isn't unsetting the src helpful?

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[dominictb]

Also isn't unsetting the src helpful?

Sorry I overlooked this. Proposal's alternative solution updated!

[s77rt]

@dominictb The solution looks good to me. Can you please raise a PR in RNW. Please provide a reproducible example and follow the repo's contribution guide.

[dominictb]

@s77rt shall I be assigned here first, or should I raise an issue/PR in the RNW repo now?

cc @strepanier03

[s77rt]

@dominictb At this point I don't think the order matters much.

🎀 👀 🎀 C+ reviewed Link to proposal

[melvin-bot[bot]]

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

[melvin-bot[bot]]

📣 @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]

Will work on the upstream issue and PR shortly.

[melvin-bot[bot]]

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

[s77rt]

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

[dominictb]

@s77rt https://github.com/necolas/react-native-web/pull/2709 upstream PR and linked issue are up

[melvin-bot[bot]]

@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]

Not overdue. Waiting on RNW PR feedback