mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-18 07:30:29 +00:00
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.
This commit is contained in:
parent
362941fb5c
commit
233818659f
@ -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.
|
||||
|
||||
@ -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(
|
||||
<Artwork record={withArt} square fit="cover" title="Album" />,
|
||||
)
|
||||
@ -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(<Artwork record={withArt} title="Album" />)
|
||||
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(<Artwork record={withArt} />)
|
||||
|
||||
useImageUrl.mockReturnValue({ imgUrl: 'blob:abc', loading: false })
|
||||
useImageUrl.mockReturnValue({
|
||||
imgUrl: 'blob:abc',
|
||||
loading: false,
|
||||
fromCache: false,
|
||||
})
|
||||
rerender(<Artwork record={withArt} />)
|
||||
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(<Artwork record={withArt} />)
|
||||
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(<Artwork record={withArt} />)
|
||||
useImageUrl.mockReturnValue({ imgUrl: 'blob:abc', loading: false })
|
||||
useImageUrl.mockReturnValue({
|
||||
imgUrl: 'blob:abc',
|
||||
loading: false,
|
||||
fromCache: false,
|
||||
})
|
||||
rerender(<Artwork record={withArt} />)
|
||||
|
||||
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(<Artwork record={withArt} onClick={onClick} />)
|
||||
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(<Artwork record={cached} />)
|
||||
expect(container.querySelector('canvas')).toBeNull()
|
||||
|
||||
useImageUrl.mockReturnValue({
|
||||
imgUrl: null,
|
||||
loading: true,
|
||||
fromCache: false,
|
||||
})
|
||||
rerender(<Artwork record={{ ...withArt, imageHash: 'bbbb' }} />)
|
||||
|
||||
expect(container.querySelector('img')).toBeNull()
|
||||
expect(container.querySelector('canvas')).not.toBeNull()
|
||||
})
|
||||
})
|
||||
|
||||
@ -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 }
|
||||
}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user