From 1bfe5accc8dec9d834949f3861b4e0472b212fb9 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 14 Sep 2026 21:45:46 -0400 Subject: [PATCH] refactor(ui): only re-fetch no-store artwork stand-ins Refresh events also name albums and songs for plays, stars and ratings, so versioning every cached cover re-downloaded final, immutable images on each of them. useImageUrl now marks a no-store response as a stand-in and re-fetches only those entries, on remount as well as on a version change; final images are never re-fetched, and cache entries no longer carry a version. useArtworkRefresh now selects a primitive, so an event naming other items does not re-render every mounted cover. --- ui/src/common/useArtworkRefresh.js | 16 +++--- ui/src/common/useImageUrl.js | 27 +++++----- ui/src/common/useImageUrl.test.js | 84 ++++++++++++++++-------------- 3 files changed, 69 insertions(+), 58 deletions(-) diff --git a/ui/src/common/useArtworkRefresh.js b/ui/src/common/useArtworkRefresh.js index 2e87d139c..7fb92586b 100644 --- a/ui/src/common/useArtworkRefresh.js +++ b/ui/src/common/useArtworkRefresh.js @@ -9,15 +9,15 @@ const namesId = (resources, id) => // Artwork can change behind an unchanged URL (a background conversion replacing its stand-in), so // this returns when the server last announced new artwork for id, for useImageUrl to version by. export const useArtworkRefresh = (id) => { - const refresh = useSelector((state) => state.activity?.refresh) + // A primitive, so an event naming other items does not re-render this cover. + const named = useSelector(({ activity }) => + id && namesId(activity?.refresh?.resources, id) + ? activity.refresh.lastReceived + : undefined, + ) const [refreshedAt, setRefreshedAt] = useState() - if ( - id && - refresh && - namesId(refresh.resources, id) && - refresh.lastReceived !== refreshedAt - ) { - setRefreshedAt(refresh.lastReceived) + if (named !== undefined && named !== refreshedAt) { + setRefreshedAt(named) } return refreshedAt } diff --git a/ui/src/common/useImageUrl.js b/ui/src/common/useImageUrl.js index f52d13352..71811dbed 100644 --- a/ui/src/common/useImageUrl.js +++ b/ui/src/common/useImageUrl.js @@ -34,8 +34,8 @@ const evictIfNeeded = () => { /** * Loads an image via fetch() with AbortController so that in-flight requests * are canceled on unmount (e.g., during pagination). Uses a module-level cache - * so remounting returns the cached blob URL instantly. A new version re-fetches a cached url whose - * image changed behind it, painting the old blob until the new one arrives. + * so remounting returns the cached blob URL instantly. A no-store response is a stand-in: it is + * re-fetched on remount and on a new version, and painted until its replacement arrives. */ export const useImageUrl = (url, version) => { const cached = url ? cache.get(url) : null @@ -69,7 +69,7 @@ export const useImageUrl = (url, version) => { // Re-check: another component's effect may have populated the cache // between this component's render and effect execution. const entry = cache.get(url) - if (entry && entry.version === version) { + if (entry && !entry.transient) { entry.refCount++ setImgUrl(entry.blobUrl) setLoading(false) @@ -78,11 +78,11 @@ export const useImageUrl = (url, version) => { entry.refCount-- } } - const stale = entry?.blobUrl ? entry : null + const standIn = !!entry const controller = new AbortController() let queued = true - if (!stale) { + if (!standIn) { setImgUrl(null) setLoading(true) setError(false) @@ -96,9 +96,12 @@ export const useImageUrl = (url, version) => { if (!res.ok) { throw new Error(`HTTP ${res.status}`) } - return res.blob() + const transient = !!res.headers + ?.get('Cache-Control') + ?.includes('no-store') + return res.blob().then((blob) => ({ blob, transient })) }) - .then((blob) => { + .then(({ blob, transient }) => { activeFetches-- processQueue() // Guard against late resolution after abort @@ -109,7 +112,7 @@ export const useImageUrl = (url, version) => { // Handle concurrent fetches: if another component already cached // this URL, use its entry and discard our blob. const existing = cache.get(url) - if (existing && existing.blobUrl && existing.version === version) { + if (existing?.blobUrl && !existing.transient) { existing.refCount++ URL.revokeObjectURL(objectUrl) setImgUrl(existing.blobUrl) @@ -117,7 +120,7 @@ export const useImageUrl = (url, version) => { if (existing?.blobUrl && existing.refCount <= 0) { URL.revokeObjectURL(existing.blobUrl) } - cache.set(url, { blobUrl: objectUrl, refCount: 1, version }) + cache.set(url, { blobUrl: objectUrl, refCount: 1, transient }) evictIfNeeded() setImgUrl(objectUrl) } @@ -129,11 +132,11 @@ export const useImageUrl = (url, version) => { if (err.name === 'AbortError') { return // Expected on unmount or URL change } - if (stale) { - return // The old image is still valid; keep showing it + if (standIn) { + return // Keep painting the stand-in } // Cache the error so repeated mounts don't re-fetch broken URLs - cache.set(url, { blobUrl: null, error: true, refCount: 0, version }) + cache.set(url, { blobUrl: null, error: true, refCount: 0 }) setError(true) setLoading(false) }) diff --git a/ui/src/common/useImageUrl.test.js b/ui/src/common/useImageUrl.test.js index bc6bc70fd..fc7b91ab8 100644 --- a/ui/src/common/useImageUrl.test.js +++ b/ui/src/common/useImageUrl.test.js @@ -235,89 +235,97 @@ describe('useImageUrl', () => { expect(result2.current.fromCache).toBe(false) }) - describe('version', () => { - const ok = (data) => + describe('no-store stand-ins', () => { + const image = (data, cacheControl = 'public, no-cache') => Promise.resolve({ ok: true, + headers: { + get: (name) => (name === 'Cache-Control' ? cacheControl : null), + }, blob: () => Promise.resolve(new Blob([data])), }) + const standIn = (data) => image(data, 'no-store') const renderVersioned = (version) => renderHook( ({ version }) => useImageUrl('http://example.com/img.jpg', version), { initialProps: { version } }, ) + const settle = () => + act(async () => { + await flushPromises() + }) - it('re-fetches a cached url when its version changes, showing the old image meanwhile', async () => { - let resolveNew + it('re-fetches a stand-in when its version changes, painting it meanwhile', async () => { + let resolveFinal global.URL.createObjectURL = vi .fn() - .mockReturnValueOnce('blob:old') - .mockReturnValueOnce('blob:new') + .mockReturnValueOnce('blob:stand-in') + .mockReturnValueOnce('blob:final') global.fetch = vi .fn() - .mockImplementationOnce(() => ok('old')) + .mockImplementationOnce(() => standIn('still')) .mockImplementationOnce( - () => new Promise((resolve) => (resolveNew = resolve)), + () => new Promise((resolve) => (resolveFinal = resolve)), ) const { result, rerender } = renderVersioned(undefined) - await act(async () => { - await flushPromises() - }) - expect(result.current.imgUrl).toBe('blob:old') + await settle() + expect(result.current.imgUrl).toBe('blob:stand-in') rerender({ version: 1 }) - await act(async () => { - await flushPromises() - }) + await settle() expect(global.fetch).toHaveBeenCalledTimes(2) - expect(result.current.imgUrl).toBe('blob:old') + expect(result.current.imgUrl).toBe('blob:stand-in') expect(result.current.loading).toBe(false) await act(async () => { - resolveNew({ ok: true, blob: () => Promise.resolve(new Blob(['new'])) }) + resolveFinal(image('animated')) await flushPromises() }) - expect(result.current.imgUrl).toBe('blob:new') + expect(result.current.imgUrl).toBe('blob:final') }) - it('keeps the old image when the re-fetch fails', async () => { + it('keeps the stand-in when the re-fetch fails', async () => { global.fetch = vi .fn() - .mockImplementationOnce(() => ok('old')) + .mockImplementationOnce(() => standIn('still')) .mockImplementationOnce(() => Promise.resolve({ ok: false, status: 500 }), ) const { result, rerender } = renderVersioned(undefined) - await act(async () => { - await flushPromises() - }) + await settle() rerender({ version: 1 }) - await act(async () => { - await flushPromises() - }) + await settle() expect(global.fetch).toHaveBeenCalledTimes(2) expect(result.current.imgUrl).toBe('blob:mock-url') expect(result.current.error).toBe(false) }) - it('does not re-fetch on remount with the version it already has', async () => { - global.fetch = vi.fn(() => ok('data')) + // Plays, stars and ratings also name items, and must not re-download final images. + it('does not re-fetch a final image when its version changes', async () => { + global.fetch = vi.fn(() => image('final')) - const { unmount } = renderVersioned(1) - await act(async () => { - await flushPromises() - }) - unmount() - const { result } = renderVersioned(1) - await act(async () => { - await flushPromises() - }) + const { rerender } = renderVersioned(undefined) + await settle() + rerender({ version: 1 }) + await settle() expect(global.fetch).toHaveBeenCalledTimes(1) - expect(result.current.fromCache).toBe(true) + }) + + it('re-fetches a stand-in on remount', async () => { + global.fetch = vi.fn(() => standIn('still')) + + const { unmount } = renderVersioned(undefined) + await settle() + unmount() + const { result } = renderVersioned(undefined) + await settle() + + expect(global.fetch).toHaveBeenCalledTimes(2) + expect(result.current.imgUrl).toBe('blob:mock-url') }) }) })