[Avatar] Fix flashing when image is already cached

[rkkautsar]

Description

Fixes #2044 by initializing loadingStatus with loaded if the image.complete is true, based on @lauri865 suggestion in the linked issue.

Based on MDN, image.complete will be true if:

• Neither the src nor the srcset attribute is specified.
• The srcset attribute is absent and the src attribute, while specified, is the empty string ("").
• The image resource has been fully fetched and has been queued for rendering/compositing.
• The image element has previously determined that the image is fully available and ready for use.
• The image is "broken;" that is, the image failed to load due to an error or because image loading is disabled.

I added test case for all the cases, although I imagine the last case might be different from browser to browser. In Chrome, when image loading is disabled the load event will never fire and image.complete will always be false. To be safe I added another check for image.naturalWidth > 0 since image with no data will have naturalWidth = 0.

I also added test and story for the case when image src changing to make sure my change can handle the changing src.

Tests added:
✓ does not leak event listeners (7 ms)
✓ can handle changing src (627 ms)
✓ should render the image immediately after it is cached (327 ms)
✓ should not render image with no src (5 ms)
✓ should not render image with empty string as src (3 ms)
✓ should show fallback if image has no data (4 ms)

Storybook added:

• Changing image src

Before	After

[LeakedDave]

Awesome, I've been waiting for this I was actually about to hack my own Avatar together myself... Will this be useable directly into Shadcn and NextUI or will we need an update from NextUI directly?

[rkkautsar]

@LeakedDave the same avatar flicker issue in shadcn/ui should be fixed just by bumping the radix version if this fix is released. For nextui I think the same idea can be implemented to this hook: https://github.com/nextui-org/nextui/blob/canary/packages/hooks/use-image/src/index.ts

[chaance]

One minor change and I'll merge 👍

[chaance]

Can we rename this to resolveLoadingStatus?

[rkkautsar]

@chaance done! I also rebased with main since there's some conflict with the referrerPolicy change

[chaance]

Sorry, one more thing I noticed. This will error in SSR. We want to ensure that hydration is complete before initializing the image.

[chaance]

Here's my suggested fix:

• Check if the app is hydrated on the client (this can either be a new function in the utils package or included in this file for now)
• If the app is not hydrated (SSR + first client render) initialize the image ref as null
• Initialize the state as "loading" pre-hydration to prevent client/server mismatch

function resolveLoadingStatus(image: HTMLImageElement | null, src?: string): ImageLoadingStatus {
  if (!src) {
    return 'error';
  }

  if (!image) {
    // image is not initialized during SSR. Ensures the initial state is
    // 'loading' on both the server and the client to prevent hydration errors.
    return 'loading';
  }

  if (image.src !== src) {
    image.src = src;
  }
  return image.complete && image.naturalWidth > 0 ? 'loaded' : 'loading';
}

function useImageLoadingStatus(src?: string, referrerPolicy?: React.HTMLAttributeReferrerPolicy) {
  const isHydrated = useIsHydrated();
  const image = React.useRef<HTMLImageElement | null>(isHydrated ? new window.Image() : null);
  const [loadingStatus, setLoadingStatus] = React.useState<ImageLoadingStatus>(() =>
    resolveLoadingStatus(image.current, src)
  );

  useLayoutEffect(() => {
    setLoadingStatus(resolveLoadingStatus(image.current, src));
  }, [src]);

  useLayoutEffect(() => {
    // We can safely assume that image.current is not null because hydration is
    // complete by the time this effect runs
    const img = image.current!;

    const updateStatus = (status: ImageLoadingStatus) => () => {
      setLoadingStatus(status);
    };

    const handleLoad = updateStatus('loaded');
    const handleError = updateStatus('error');
    img.addEventListener('load', handleLoad);
    img.addEventListener('error', handleError);
    if (referrerPolicy) {
      img.referrerPolicy = referrerPolicy;
    }

    return () => {
      img.removeEventListener('load', handleLoad);
      img.removeEventListener('error', handleError);
    };
  }, [referrerPolicy]);

  return loadingStatus;
}

function subscribe() {
  return () => {};
}

function useIsHydrated() {
  return React.useSyncExternalStore(
    subscribe,
    () => true,
    () => false
  );
}

[rkkautsar]

Good catch! also TIL useSyncExternalStore is the recommended way to detect hydration. I'll make the change

[rkkautsar]

@chaance done, I added a test for SSR and make sure it works in the ssr-testing app too. Need to make some change from your suggestion but overall it works. One thing is that the SSR testing will log this error:

 Warning: useLayoutEffect does nothing on the server, because its effect cannot be encoded into the server renderer's output format. This will lead to a mismatch between the initial, non-hydrated UI and the intended UI. To avoid this, useLayoutEffect should only be used in components that render exclusively on the client. See https://reactjs.org/link/uselayouteffect-ssr for common fixes.

since we use useLayoutEffect for Avatar. It's just a warning from jest though, and the Avatar component server rendered to its fallback component just fine as tested with unit test and manually checked in the ssr-testing app

[mackermans]

Hey @chaance @rkkautsar, is this PR ready to be merged? 🙏

[quachthientai]

Hello, I just want to follow if this PR is ready?

[rkkautsar]

@mackermans @quachthientai need maintainer to review the change 😅

[chaance]

Please do not leave comments like "bump" and "+1", they are unhelpful. At the moment this project is just me, and I've got a large backlog of issues and need to prioritize accordingly.

[jvandenaardweg]

@rkkautsar do you see an opportunity to finish this PR with the last PR comment?

[rkkautsar]

@chaance sorry just got the chance to work on this again. I applied your suggestion and tested that everything still works.