From 233818659f58339ae4785524a6509ed707466dac Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 2 Aug 2026 00:59:36 -0400 Subject: [PATCH] fix(ui): show the placeholder when a refresh swaps in an uncached cover A cover whose hash changed under a mounted grid went blank for the whole fetch, instead of falling back to its thumbhash. Artwork decided 'this blob was already cached, skip the placeholder' once at mount and never revisited it, so every later url for that record inherited the answer. The fix has to live in useImageUrl. imgUrl is state cleared in an effect, so on the render where url changes it still holds the previous url's blob - any caller inspecting it reads the new url as already cached. Only the hook can see cache.get(url) for the url actually being rendered, so it now reports fromCache and resyncs its state during render rather than one render later. Two shapes were tried and rejected against a live server before this one: a ref recomputed per url, which a render-phase resync discards without rolling back the ref, and the same tracking in Artwork state, which still samples imgUrl before React re-runs the component. Verified on a running instance with cover-art responses held open: the thumbhash now covers the whole window and stays under the image until the fade ends. --- ui/src/common/Artwork.jsx | 12 +++---- ui/src/common/Artwork.test.jsx | 64 ++++++++++++++++++++++++++++++---- ui/src/common/useImageUrl.js | 14 +++++++- 3 files changed, 74 insertions(+), 16 deletions(-) diff --git a/ui/src/common/Artwork.jsx b/ui/src/common/Artwork.jsx index 92557920b..94c45e011 100644 --- a/ui/src/common/Artwork.jsx +++ b/ui/src/common/Artwork.jsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from 'react' +import { useEffect, useState } from 'react' import PropTypes from 'prop-types' import clsx from 'clsx' import { makeStyles } from '@material-ui/core/styles' @@ -47,13 +47,8 @@ export const Artwork = ({ }) => { const classes = useStyles() const url = record ? subsonic.getCoverArtUrl(record, size, square) : '' - const { imgUrl } = useImageUrl(url) + const { imgUrl, fromCache } = useImageUrl(url) - // A blob already cached at mount paints on the first frame, so it skips the fade. - const cachedOnMount = useRef(null) - if (cachedOnMount.current === null) { - cachedOnMount.current = !!imgUrl - } const [decoded, setDecoded] = useState(false) const [faded, setFaded] = useState(false) useEffect(() => { @@ -70,7 +65,8 @@ export const Artwork = ({ if (!record) return null - const instant = cachedOnMount.current + // A blob already cached paints on the first frame, so it skips the fade. + const instant = fromCache // Kept mounted until the fade ends; swapping on blob arrival would flash an empty container. const showPlaceholder = !!record.thumbHash && !instant && !faded // A square request is padded, not cropped, so `contain` keeps placeholder and image aligned. diff --git a/ui/src/common/Artwork.test.jsx b/ui/src/common/Artwork.test.jsx index c64153547..ed40823c8 100644 --- a/ui/src/common/Artwork.test.jsx +++ b/ui/src/common/Artwork.test.jsx @@ -3,7 +3,12 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' vi.mock('./useImageUrl', () => ({ useImageUrl: vi.fn() })) vi.mock('../subsonic', () => ({ - default: { getCoverArtUrl: () => '/rest/getCoverArt?id=al-1' }, + default: { + // Mirrors the real hash suffix, so a refreshed record yields a different URL. + getCoverArtUrl: (record) => + '/rest/getCoverArt?id=al-1' + + (record.imageHash ? `_${record.imageHash}` : ''), + }, })) vi.mock('../config', () => ({ default: { uiCoverArtSize: 300 } })) @@ -69,7 +74,11 @@ describe('Artwork', () => { // The padded square is aspect-fit, so `square` overrides fit="cover" as it does for the canvas. it('fits the image itself with contain when the server padded to a square', () => { - useImageUrl.mockReturnValue({ imgUrl: 'blob:abc', loading: false }) + useImageUrl.mockReturnValue({ + imgUrl: 'blob:abc', + loading: false, + fromCache: true, + }) const { container } = render( , ) @@ -95,7 +104,11 @@ describe('Artwork', () => { }) it('mounts the image only once its blob is ready', () => { - useImageUrl.mockReturnValue({ imgUrl: 'blob:abc', loading: false }) + useImageUrl.mockReturnValue({ + imgUrl: 'blob:abc', + loading: false, + fromCache: true, + }) const { container } = render() const img = container.querySelector('img') expect(img).not.toBeNull() @@ -108,7 +121,11 @@ describe('Artwork', () => { useImageUrl.mockReturnValue({ imgUrl: null, loading: true }) const { container, rerender } = render() - useImageUrl.mockReturnValue({ imgUrl: 'blob:abc', loading: false }) + useImageUrl.mockReturnValue({ + imgUrl: 'blob:abc', + loading: false, + fromCache: false, + }) rerender() const img = container.querySelector('img') expect(img).not.toBeNull() @@ -133,7 +150,11 @@ describe('Artwork', () => { }) it('does not fade an image that was already cached on mount', () => { - useImageUrl.mockReturnValue({ imgUrl: 'blob:abc', loading: false }) + useImageUrl.mockReturnValue({ + imgUrl: 'blob:abc', + loading: false, + fromCache: true, + }) const { container } = render() expect(container.querySelector('canvas')).toBeNull() expect(container.querySelector('img').className).toContain('imgInstant') @@ -142,7 +163,11 @@ describe('Artwork', () => { it('keeps the placeholder visible when the image never decodes', () => { useImageUrl.mockReturnValue({ imgUrl: null, loading: true }) const { container, rerender } = render() - useImageUrl.mockReturnValue({ imgUrl: 'blob:abc', loading: false }) + useImageUrl.mockReturnValue({ + imgUrl: 'blob:abc', + loading: false, + fromCache: false, + }) rerender() expect(container.querySelector('canvas')).not.toBeNull() @@ -157,9 +182,34 @@ describe('Artwork', () => { container.firstChild.click() expect(onClick).not.toHaveBeenCalled() - useImageUrl.mockReturnValue({ imgUrl: 'blob:abc', loading: false }) + useImageUrl.mockReturnValue({ + imgUrl: 'blob:abc', + loading: false, + fromCache: false, + }) rerender() container.firstChild.click() expect(onClick).toHaveBeenCalledTimes(1) }) + + it('falls back to the placeholder when a refresh swaps in an uncached hash', () => { + useImageUrl.mockReturnValue({ + imgUrl: 'blob:old', + loading: false, + fromCache: true, + }) + const cached = { ...withArt, imageHash: 'aaaa' } + const { container, rerender } = render() + expect(container.querySelector('canvas')).toBeNull() + + useImageUrl.mockReturnValue({ + imgUrl: null, + loading: true, + fromCache: false, + }) + rerender() + + expect(container.querySelector('img')).toBeNull() + expect(container.querySelector('canvas')).not.toBeNull() + }) }) diff --git a/ui/src/common/useImageUrl.js b/ui/src/common/useImageUrl.js index 9bcc70d74..736c599d6 100644 --- a/ui/src/common/useImageUrl.js +++ b/ui/src/common/useImageUrl.js @@ -43,6 +43,18 @@ export const useImageUrl = (url) => { const [error, setError] = useState(cached?.error || false) const abortedRef = useRef(false) + // Resync during render, not in the effect below: for one render otherwise imgUrl still holds the + // previous url's blob, so only fromCache can answer "was this url already cached when it started". + const [fromCache, setFromCache] = useState(!!cached) + const [trackedUrl, setTrackedUrl] = useState(url) + if (trackedUrl !== url) { + setTrackedUrl(url) + setImgUrl(cached?.blobUrl || null) + setLoading(!!url && !cached) + setError(cached?.error || false) + setFromCache(!!cached) + } + useEffect(() => { abortedRef.current = false @@ -140,5 +152,5 @@ export const useImageUrl = (url) => { } }, [url]) - return { imgUrl, loading, error } + return { imgUrl, loading, error, fromCache } }