fix(media): keep avatar from blinking or failing

.changeset/fix-avatar-blinking.md

@@ -0,0 +1,5 @@
+---
+default: patch
+---
+
+Fix avatars flickering and sometimes staying broken: a retry now re-requests the image on the web, loads without dropping the fallback, and stays cached once it succeeds.

src/app/components/room-avatar/RoomAvatar.tsx

@@ -25,7 +25,8 @@ type RoomAvatarProps = {
 };
 
 export function RoomAvatar({ roomId, src, alt, renderFallback, uniformIcons }: RoomAvatarProps) {
-  const { mediaSrc, error, onError } = useAvatarMediaSource(src);
+  // Mirrors the crossOrigin AvatarImage puts on the rendered element.
+  const { mediaSrc, error, onError } = useAvatarMediaSource(src, { crossOrigin: 'anonymous' });
 
   if (!mediaSrc || error) {
     return (

src/app/hooks/useRenderableMediaUrl.test.tsx

@@ -29,6 +29,35 @@
 vi.mock('$utils/mediaTransport', () => mediaTransport);
 vi.mock('@tauri-apps/api/core', () => tauriApi);
 
+// The out-of-band retry preload never fires in jsdom, so tests drive it explicitly.
+class StubImage {
+  src = '';
+
+  crossOrigin: string | null = null;
+
+  private readonly handlers = new Map<string, Set<() => void>>();
+
+  constructor() {
+    imageProbes.push(this);
+  }
+
+  addEventListener(type: string, handler: () => void): void {
+    const existing = this.handlers.get(type) ?? new Set<() => void>();
+    existing.add(handler);
+    this.handlers.set(type, existing);
+  }
+
+  removeEventListener(type: string, handler: () => void): void {
+    this.handlers.get(type)?.delete(handler);
+  }
+
+  emit(type: string): void {
+    this.handlers.get(type)?.forEach((handler) => handler());
+  }
+}
+
+const imageProbes: StubImage[] = [];
+
 describe('useRenderableMediaUrl', () => {
   beforeEach(() => {
     vi.resetModules();
@@ -48,6 +77,8 @@
     tauriApi.convertFileSrc.mockImplementation(
       (url: string, protocol: string) => `${protocol}://${url}`
     );
+    imageProbes.length = 0;
+    vi.stubGlobal('Image', StubImage);
     vi.spyOn(URL, 'createObjectURL').mockReturnValue('blob:rendered-media');
     vi.spyOn(URL, 'revokeObjectURL').mockImplementation(() => undefined);
     Object.defineProperty(navigator, 'serviceWorker', {
@@ -62,6 +93,7 @@
   });
 
   afterEach(() => {
+    vi.unstubAllGlobals();
     vi.restoreAllMocks();
   });
 
@@ -268,7 +300,7 @@
   it('re-resolves the loopback url after the cache is cleared by a token rotation', async () => {
     tauriApi.isTauri.mockReturnValue(true);
     const freshLoopback = 'http://127.0.0.1:45678/capability-new-token';
     let resolveFresh: (url: string) => void = () => {};
     tauriApi.invoke
       .mockResolvedValueOnce('http://127.0.0.1:45678/capability-old-token')
       .mockImplementationOnce(
@@ -304,7 +336,7 @@
 
   it('withholds the raw source under Tauri until the loopback url resolves', async () => {
     tauriApi.isTauri.mockReturnValue(true);
     let resolveLoopback: (url: string) => void = () => {};
     tauriApi.invoke.mockReturnValue(
       new Promise<string>((resolve) => {
         resolveLoopback = resolve;
@@ -363,11 +395,60 @@
         await vi.advanceTimersByTimeAsync(500);
       });
 
-      expect(result.current.error).toBe(false);
+      // The revision advances, but the latch stays shut so the caller keeps rendering its
+      // fallback rather than blinking through an empty <img> for the duration of the load.
+      expect(result.current.error).toBe(true);
       expect(result.current.mediaSrc).toBe('http://127.0.0.1:45678/fresh-capability');
       expect(tauriApi.invoke).toHaveBeenCalledTimes(2);
       const retryUrl = tauriApi.invoke.mock.calls[1]?.[1].url ?? '';
       expect(retryUrl).toContain('__sable_media_retry=1');
+
+      const probe = imageProbes.at(-1);
+      expect(probe?.src).toBe('http://127.0.0.1:45678/fresh-capability');
+
+      act(() => {
+        probe?.emit('load');
+      });
+      expect(result.current.error).toBe(false);
+      vi.useRealTimers();
+    });
+
+    it('only reveals the image once the out-of-band retry decodes', async () => {
+      vi.useFakeTimers();
+      tauriApi.isTauri.mockReturnValue(true);
+      tauriApi.invoke.mockImplementation(
+        async (_cmd: string, args: { url: string }) =>
+          `http://127.0.0.1:45678/capability-${args.url.length}`
+      );
+      const { useAvatarMediaSource } = await import('./useRenderableMediaUrl');
+
+      const { result } = renderHook(() =>
+        useAvatarMediaSource(RAW_URL, { crossOrigin: 'anonymous' })
+      );
+
+      await act(async () => {
+        await vi.advanceTimersByTimeAsync(0);
+      });
+      act(() => {
+        result.current.onError();
+      });
+
+      // Two attempts elapse without the preload ever settling: the latch never opens, so
+      // the fallback is continuous instead of flashing once per attempt.
+      await act(async () => {
+        await vi.advanceTimersByTimeAsync(500);
+      });
+      expect(result.current.error).toBe(true);
+      await act(async () => {
+        await vi.advanceTimersByTimeAsync(1500);
+      });
+      expect(result.current.error).toBe(true);
+
+      // The preload must request exactly what the rendered element would, or it warms a
+      // different cache entry and the real <img> refetches.
+      const probe = imageProbes.at(-1);
+      expect(probe?.src).toBe(result.current.mediaSrc);
+      expect(probe?.crossOrigin).toBe('anonymous');
       vi.useRealTimers();
     });
 
@@ -386,28 +467,27 @@
       });
       expect(result.current.mediaSrc).toBeDefined();
 
-      const failAndExhaustRetry = async (delay: number) => {
-        act(() => {
-          result.current.onError();
-        });
-        expect(result.current.error).toBe(true);
+      act(() => {
+        result.current.onError();
+      });
+      expect(result.current.error).toBe(true);
+
+      // Nothing ever loads, so the latch holds and the ladder walks its whole schedule.
+      const advance = async (delay: number) => {
         await act(async () => {
           await vi.advanceTimersByTimeAsync(delay);
         });
-        expect(result.current.error).toBe(false);
+        expect(result.current.error).toBe(true);
       };
-      await failAndExhaustRetry(500);
-      await failAndExhaustRetry(1500);
-      await failAndExhaustRetry(4500);
+      await advance(500);
+      await advance(1500);
+      await advance(4500);
 
-      act(() => {
-        result.current.onError();
-      });
-      expect(result.current.error).toBe(true);
-      await act(async () => {
-        await vi.advanceTimersByTimeAsync(60_000);
-      });
-      expect(result.current.error).toBe(true);
+      // One resolve for the initial source plus one per scheduled retry.
+      expect(tauriApi.invoke).toHaveBeenCalledTimes(4);
+
+      await advance(60_000);
+      expect(tauriApi.invoke).toHaveBeenCalledTimes(4);
       vi.useRealTimers();
     });
 

src/app/hooks/useRenderableMediaUrl.ts

@@ -13,7 +13,7 @@ import {
   subscribeSWMediaAuthSupport,
 } from '$utils/swMediaAuth';
 import { rewriteAuthenticatedMediaUrl } from '$utils/matrix';
-import { addTauriMediaRetryRevision } from '$utils/mediaUrl';
+import { addMediaRetryRevision } from '$utils/mediaUrl';
 
 type ObjectUrlEntry = {
   refs: number;
@@ -334,8 +334,7 @@ export function useRenderableMediaSource(
   url: string | undefined,
   retryRevision = 0
 ): string | undefined {
-  const retriedUrl =
-    url && retryRevision > 0 ? addTauriMediaRetryRevision(url, retryRevision) : url;
+  const retriedUrl = url && retryRevision > 0 ? addMediaRetryRevision(url, retryRevision) : url;
   const resolvedUrl = useRenderableMediaUrl(retriedUrl, retryRevision);
   if (resolvedUrl) return resolvedUrl;
   return isTauri() ? undefined : retriedUrl;
@@ -349,7 +348,17 @@ type AvatarMediaSource = {
   onError: () => void;
 };
 
-export function useAvatarMediaSource(src: string | undefined): AvatarMediaSource {
+// `crossOrigin` must match what the caller puts on its own <img>, or the out-of-band retry
+// below warms a different cache entry and the rendered element requests the media again.
+type AvatarMediaSourceOptions = {
+  crossOrigin?: 'anonymous';
+};
+
+export function useAvatarMediaSource(
+  src: string | undefined,
+  options?: AvatarMediaSourceOptions
+): AvatarMediaSource {
+  const crossOrigin = options?.crossOrigin;
   const [error, setError] = useState(false);
   const [retryRevision, setRetryRevision] = useState(0);
   const mediaSrc = useRenderableMediaSource(src, retryRevision);
@@ -359,18 +368,38 @@ export function useAvatarMediaSource(src: string | undefined): AvatarMediaSource
     setRetryRevision(0);
   }, [src]);
 
+  // First attempt only: on Tauri the resolved url arrives after the initial render, so an
+  // error latched against the earlier (or absent) source must not outlive it. A retry
+  // deliberately does not clear the latch here — the preload below owns that, which is what
+  // keeps the fallback on screen instead of blinking through an empty <img>.
   useEffect(() => {
+    if (retryRevision > 0) return;
     setError(false);
-  }, [mediaSrc]);
+  }, [mediaSrc, retryRevision]);
+
+  // Rendering a retried <img> straight away shows its placeholder background until the load
+  // settles, which reads as the avatar blinking once per attempt. Loading out of band keeps
+  // the fallback up and swaps to the image only once it is decodable, by which point the
+  // rendered element resolves from cache.
+  useEffect(() => {
+    if (!error || retryRevision === 0 || !mediaSrc) return undefined;
+
+    const probe = new Image();
+    if (crossOrigin && !mediaSrc.startsWith('blob:')) {
+      probe.crossOrigin = crossOrigin;
+    }
+    const onLoad = () => setError(false);
+    probe.addEventListener('load', onLoad, { once: true });
+    probe.src = mediaSrc;
+
+    return () => probe.removeEventListener('load', onLoad);
+  }, [error, retryRevision, mediaSrc, crossOrigin]);
 
   useEffect(() => {
     if (!error) return undefined;
     const delay = AVATAR_RETRY_DELAYS_MS[retryRevision];
     if (delay === undefined) return undefined;
-    const timer = setTimeout(() => {
-      setError(false);
-      setRetryRevision((revision) => revision + 1);
-    }, delay);
+    const timer = setTimeout(() => setRetryRevision((revision) => revision + 1), delay);
     return () => clearTimeout(timer);
   }, [error, retryRevision]);
 

src/app/utils/mediaTransport.test.ts

@@ -546,4 +546,71 @@ describe('fetchMediaBlob', () => {
     expect(await blob.text()).toBe('ok');
     expect(headersSeen).toEqual(['Bearer token-1']);
   });
+
+  it(
+    'ignores the retry marker when keying the cache so a retried success is reused',
+    async () => {
+      const { fetchMediaBlob } = await import('./mediaTransport');
+      const canonical =
+        'https://matrix.example.org/_matrix/client/v1/media/thumbnail/example.org/abc123?width=96';
+      const retried = `${canonical}&__sable_media_retry=2`;
+
+      localStorage.setItem(
+        'matrixSessions',
+        JSON.stringify([
+          {
+            baseUrl: 'https://matrix.example.org',
+            userId: '@alice:example.org',
+            deviceId: 'DEVICE',
+            accessToken: 'token-1',
+          },
+        ])
+      );
+      localStorage.setItem('matrixActiveSession', '@alice:example.org');
+
+      const media = new Blob(['avatar'], { type: 'image/png' });
+      vi.mocked(fetch).mockResolvedValueOnce(new Response(media, { status: 200 }));
+
+      await expect(fetchMediaBlob(retried)).resolves.toEqual(media);
+      expect(fetch).toHaveBeenCalledOnce();
+      expect(mediaCache.putInMediaCache).toHaveBeenCalledWith(
+        '@alice:example.org:mxc://example.org/abc123:thumbnail?width=96',
+        expect.any(Blob)
+      );
+
+      // The next mount is back at revision 0, so it must hit the entry the retry wrote
+      // instead of downloading the same avatar again.
+      await expect(fetchMediaBlob(canonical)).resolves.toEqual(media);
+      expect(fetch).toHaveBeenCalledOnce();
+    },
+    TEST_TIMEOUT
+  );
+});
+
+describe('getStableMediaCacheKeyFragment', () => {
+  const CANONICAL =
+    'https://matrix.example.org/_matrix/client/v1/media/thumbnail/example.org/abc123?width=96';
+
+  beforeEach(() => {
+    vi.resetModules();
+  });
+
+  it('drops the retry marker in both its query and fragment forms', async () => {
+    const { getStableMediaCacheKeyFragment } = await import('./mediaTransport');
+
+    const expected = getStableMediaCacheKeyFragment(CANONICAL);
+    expect(getStableMediaCacheKeyFragment(`${CANONICAL}&__sable_media_retry=2`)).toBe(expected);
+    expect(getStableMediaCacheKeyFragment(`${CANONICAL}#__sable_media_retry=2`)).toBe(expected);
+  });
+
+  it('still separates different media and different thumbnail sizes', async () => {
+    const { getStableMediaCacheKeyFragment } = await import('./mediaTransport');
+
+    expect(getStableMediaCacheKeyFragment(CANONICAL)).not.toBe(
+      getStableMediaCacheKeyFragment(CANONICAL.replace('abc123', 'def456'))
+    );
+    expect(getStableMediaCacheKeyFragment(CANONICAL)).not.toBe(
+      getStableMediaCacheKeyFragment(CANONICAL.replace('width=96', 'width=32'))
+    );
+  });
 });

src/app/utils/mediaTransport.ts

@@ -229,10 +229,34 @@ function getMatrixMediaInfo(url: string): MatrixMediaInfo | undefined {
   };
 }
 
+// Kept in step with MEDIA_RETRY_MARKER in mediaUrl.ts, which imports from this module and
+// so cannot be imported back.
+const MEDIA_RETRY_MARKER = '__sable_media_retry';
+
+// A retry marker exists only to make the browser (or the native layer) re-request, so it must
+// never reach a cache key: a retry that finally succeeds would land under a key nothing looks
+// up again, and the media would re-download on every mount. Attempts stay distinct in memory
+// through getObjectUrlCacheKey, which appends the revision itself.
+function stripMediaRetryMarker(url: string): string {
+  if (!url.includes(MEDIA_RETRY_MARKER)) return url;
+
+  try {
+    const parsed = new URL(url);
+    parsed.searchParams.delete(MEDIA_RETRY_MARKER);
+    if (parsed.hash.startsWith(`#${MEDIA_RETRY_MARKER}=`)) {
+      parsed.hash = '';
+    }
+    return parsed.toString();
+  } catch {
+    return url;
+  }
+}
+
 function getStableMediaCacheKeyFragment(url: string): string {
-  const info = getMatrixMediaInfo(url);
+  const stableUrl = stripMediaRetryMarker(url);
+  const info = getMatrixMediaInfo(stableUrl);
   if (info) return `${info.mxcUrl}:${info.operation}${info.query}`;
-  return url;
+  return stableUrl;
 }
 
 export { getStableMediaCacheKeyFragment };