diff --git a/.changeset/fix-avatar-blinking.md b/.changeset/fix-avatar-blinking.md new file mode 100644 index 000000000..0940b6ff7 --- /dev/null +++ b/.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. diff --git a/src/app/components/room-avatar/RoomAvatar.tsx b/src/app/components/room-avatar/RoomAvatar.tsx index 68cf30f8b..c725da718 100644 --- a/src/app/components/room-avatar/RoomAvatar.tsx +++ b/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 ( diff --git a/src/app/hooks/useRenderableMediaUrl.test.tsx b/src/app/hooks/useRenderableMediaUrl.test.tsx index 3731aa9e6..c3ef8ad1e 100644 --- a/src/app/hooks/useRenderableMediaUrl.test.tsx +++ b/src/app/hooks/useRenderableMediaUrl.test.tsx @@ -29,6 +29,35 @@ vi.mock('$utils/swMediaAuth', () => swMediaAuth); 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 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 @@ describe('useRenderableMediaUrl', () => { 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 @@ describe('useRenderableMediaUrl', () => { }); afterEach(() => { + vi.unstubAllGlobals(); vi.restoreAllMocks(); }); @@ -363,11 +395,60 @@ describe('useRenderableMediaUrl', () => { 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 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 refetches. + const probe = imageProbes.at(-1); + expect(probe?.src).toBe(result.current.mediaSrc); + expect(probe?.crossOrigin).toBe('anonymous'); vi.useRealTimers(); }); @@ -386,28 +467,27 @@ describe('useRenderableMediaUrl', () => { }); 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(); }); diff --git a/src/app/hooks/useRenderableMediaUrl.ts b/src/app/hooks/useRenderableMediaUrl.ts index 6a59b4eba..a3c4710ae 100644 --- a/src/app/hooks/useRenderableMediaUrl.ts +++ b/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 , 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 . useEffect(() => { + if (retryRevision > 0) return; setError(false); - }, [mediaSrc]); + }, [mediaSrc, retryRevision]); + + // Rendering a retried 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]); diff --git a/src/app/utils/mediaTransport.test.ts b/src/app/utils/mediaTransport.test.ts index 30882db7a..9981ac510 100644 --- a/src/app/utils/mediaTransport.test.ts +++ b/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')) + ); + }); }); diff --git a/src/app/utils/mediaTransport.ts b/src/app/utils/mediaTransport.ts index 157ee4412..b7b00d6b7 100644 --- a/src/app/utils/mediaTransport.ts +++ b/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 }; diff --git a/src/app/utils/mediaUrl.test.ts b/src/app/utils/mediaUrl.test.ts index 6dd8a01e3..2d2027624 100644 --- a/src/app/utils/mediaUrl.test.ts +++ b/src/app/utils/mediaUrl.test.ts @@ -24,6 +24,7 @@ vi.mock('./mediaTransport', () => ({ })); import { + addMediaRetryRevision, addTauriMediaRetryRevision, getTauriMediaSourceUrl, getTauriMediaRetryTarget, @@ -202,6 +203,50 @@ describe('addTauriMediaRetryRevision', () => { }); }); +describe('addMediaRetryRevision', () => { + const WEB_URL = + 'https://matrix.example.com/_matrix/client/v1/media/thumbnail/example.com/abc123?width=96'; + const WRAPPED = `sable-media://${WEB_URL}?__sable_media_cache=3&__sable_media_session=session_abc`; + + it('is a no-op for the first attempt on either platform', () => { + hoistedIsTauri.mockReturnValue(false); + expect(addMediaRetryRevision(WEB_URL, 0)).toBe(WEB_URL); + hoistedIsTauri.mockReturnValue(true); + expect(addMediaRetryRevision(WRAPPED, 0)).toBe(WRAPPED); + }); + + // An identical src is never re-requested by the browser, so without this the retry + // fires no second load, no second error event, and the broken image sticks. + it('changes the requested url outside Tauri so the retry actually re-requests', () => { + hoistedIsTauri.mockReturnValue(false); + const revised = addMediaRetryRevision(WEB_URL, 1); + + expect(revised).not.toBe(WEB_URL); + const parsed = new URL(revised); + expect(parsed.searchParams.get('__sable_media_retry')).toBe('1'); + expect(parsed.searchParams.get('width')).toBe('96'); + expect(parsed.pathname).toBe('/_matrix/client/v1/media/thumbnail/example.com/abc123'); + }); + + it('replaces the revision outside Tauri instead of stacking parameters', () => { + hoistedIsTauri.mockReturnValue(false); + const second = addMediaRetryRevision(addMediaRetryRevision(WEB_URL, 1), 2); + expect(second).toBe(addMediaRetryRevision(WEB_URL, 2)); + expect(new URL(second).searchParams.getAll('__sable_media_retry')).toEqual(['2']); + }); + + it('leaves non-http(s) sources alone outside Tauri', () => { + hoistedIsTauri.mockReturnValue(false); + const blob = 'blob:https://app.local/0000-1111'; + expect(addMediaRetryRevision(blob, 1)).toBe(blob); + }); + + it('delegates to the Tauri fragment encoding inside Tauri', () => { + hoistedIsTauri.mockReturnValue(true); + expect(addMediaRetryRevision(WRAPPED, 1)).toBe(addTauriMediaRetryRevision(WRAPPED, 1)); + }); +}); + describe('getTauriMediaRetryTarget', () => { const WRAPPED = 'sable-media://https://matrix.example.com/_matrix/client/v1/media/download/example.com/abc123?__sable_media_cache=3&__sable_media_session=session_abc'; diff --git a/src/app/utils/mediaUrl.ts b/src/app/utils/mediaUrl.ts index bb6c6386a..e0c4982cc 100644 --- a/src/app/utils/mediaUrl.ts +++ b/src/app/utils/mediaUrl.ts @@ -40,7 +40,8 @@ export const rewriteAuthenticatedMediaUrl = (httpUrl: string | null): string | n return `${mediaUrl}${separator}${TAURI_MEDIA_CACHE_VERSION}&__sable_media_session=${sessionScope}`; }; -const TAURI_MEDIA_RETRY_FRAGMENT = '__sable_media_retry'; +// Kept in step with MEDIA_RETRY_MARKER in mediaTransport.ts, which strips it from cache keys. +const MEDIA_RETRY_MARKER = '__sable_media_retry'; const TAURI_MEDIA_OUTER_QUERY_PARAMS = ['__sable_media_cache', '__sable_media_session']; const TAURI_MEDIA_PROTOCOL = 'sable-media://'; const TAURI_MEDIA_LOCALHOST = 'localhost'; @@ -98,7 +99,7 @@ export const getTauriMediaRetryTarget = ( } if (parsedUrl.protocol !== 'http:' && parsedUrl.protocol !== 'https:') return undefined; TAURI_MEDIA_OUTER_QUERY_PARAMS.forEach((param) => parsedUrl.searchParams.delete(param)); - parsedUrl.hash = `${TAURI_MEDIA_RETRY_FRAGMENT}=${revision}`; + parsedUrl.hash = `${MEDIA_RETRY_MARKER}=${revision}`; return parsedUrl.toString(); }; @@ -108,6 +109,29 @@ export const addTauriMediaRetryRevision = (mediaUrl: string, revision: number): return rewriteAuthenticatedMediaUrl(target) ?? mediaUrl; }; +// Outside Tauri the revision rides as a query parameter. The media endpoints ignore it, but +// it makes the URL the browser requests distinct, which is the whole point of a retry: an +// identical `src` is never re-requested, so no second error event ever fires and a broken +// image sticks around instead of falling back. +const addWebMediaRetryRevision = (mediaUrl: string, revision: number): string => { + let parsedUrl: URL; + try { + parsedUrl = new URL(mediaUrl); + } catch { + return mediaUrl; + } + if (parsedUrl.protocol !== 'http:' && parsedUrl.protocol !== 'https:') return mediaUrl; + parsedUrl.searchParams.set(MEDIA_RETRY_MARKER, String(revision)); + return parsedUrl.toString(); +}; + +export const addMediaRetryRevision = (mediaUrl: string, revision: number): string => { + if (revision <= 0) return mediaUrl; + return isTauri() + ? addTauriMediaRetryRevision(mediaUrl, revision) + : addWebMediaRetryRevision(mediaUrl, revision); +}; + // A media element cannot play from the `sable-media` scheme (MEDIA_ERR_SRC_NOT_SUPPORTED), // so video/audio sources go through the loopback HTTP origin. export const prepareLoopbackMedia = async (source: string): Promise => { diff --git a/src/app/utils/swMediaAuth.test.ts b/src/app/utils/swMediaAuth.test.ts index d4f2b2929..3fff35fa1 100644 --- a/src/app/utils/swMediaAuth.test.ts +++ b/src/app/utils/swMediaAuth.test.ts @@ -96,7 +96,7 @@ describe('swMediaAuth', () => { expect(postMessage).toHaveBeenCalledTimes(2); }, 10_000); - it('notifies unsupported while a replacement controller probe is unresolved', async () => { + it('keeps listeners on the proven path while a replacement controller probe is unresolved', async () => { platform.hasServiceWorker.mockReturnValue(true); const firstController = { postMessage: vi.fn<(...args: unknown[]) => void>((...args: unknown[]) => { @@ -113,18 +113,48 @@ describe('swMediaAuth', () => { listener.mockClear(); vi.useFakeTimers(); - serviceWorker.controller = { postMessage: vi.fn<() => void>() }; + const replacement = { postMessage: vi.fn<() => void>() }; + serviceWorker.controller = replacement; const controllerChange = serviceWorker.addEventListener.mock.calls.find( ([type]) => type === 'controllerchange' )?.[1] as (() => void) | undefined; controllerChange?.(); expect(controllerChange).toBeTypeOf('function'); - expect(listener).toHaveBeenCalledWith(false); + // A speculative `false` here would flip every mounted media consumer to the blob + // path and back once the probe answers, blinking every avatar on screen. + expect(listener).not.toHaveBeenCalled(); + expect(replacement.postMessage).toHaveBeenCalledOnce(); expect(mod.getCachedSWMediaAuthSupport()).toBeUndefined(); await vi.advanceTimersByTimeAsync(1500); expect(mod.getCachedSWMediaAuthSupport()).toBeUndefined(); + expect(listener).not.toHaveBeenCalled(); + }); + + it('notifies unsupported when the page loses its controller', async () => { + platform.hasServiceWorker.mockReturnValue(true); + const serviceWorker = stubServiceWorker({ + postMessage: vi.fn<(...args: unknown[]) => void>((...args: unknown[]) => { + const [port] = args[1] as MessagePort[]; + port?.postMessage({ type: 'swMediaAuth', supported: true, version: 1 }); + }), + }); + const mod = await import('./swMediaAuth'); + const listener = vi.fn<(supported: boolean) => void>(); + mod.subscribeSWMediaAuthSupport(listener); + + await expect(mod.probeSWMediaAuthSupport()).resolves.toBe(true); + listener.mockClear(); + + serviceWorker.controller = null; + const controllerChange = serviceWorker.addEventListener.mock.calls.find( + ([type]) => type === 'controllerchange' + )?.[1] as (() => void) | undefined; + controllerChange?.(); + + expect(listener).toHaveBeenCalledWith(false); + expect(mod.getCachedSWMediaAuthSupport()).toBe(false); }); it('resolves false when posting the probe throws', async () => { diff --git a/src/app/utils/swMediaAuth.ts b/src/app/utils/swMediaAuth.ts index 8526e5275..d4f235b50 100644 --- a/src/app/utils/swMediaAuth.ts +++ b/src/app/utils/swMediaAuth.ts @@ -112,9 +112,13 @@ if (typeof window !== 'undefined' && typeof navigator !== 'undefined' && hasServ cachedSupport = controller ? undefined : false; inflightProbe = undefined; probedController = undefined; - notify(false); - if (controller) { - void probeSWMediaAuthSupport(); + if (!controller) { + notify(false); + return; } + // No speculative `false` here: it would flip every mounted media consumer to the + // blob path and back within the probe window, blinking every avatar on screen. + // The probe below notifies the real answer, and it is bounded by PROBE_TIMEOUT_MS. + void probeSWMediaAuthSupport(); }); }