From cd194e50eb81b96dc394fd58566992d8bc43d1f9 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 24 Jul 2026 14:04:30 -0400 Subject: [PATCH] fix(ui): address CoverImage review findings Restructure CoverImage so the size/shape lives on the root and the blurhash + image are absolute fills: the mounts only once its blob is ready, so an unresolved cover never flashes a broken . Add a fit prop (default cover) so album/playlist detail keep their letterbox instead of being cropped by a hardcoded object-fit. Remove the orphaned coverLoading styles and an unused subsonic import; add a CoverImage unit test. --- ui/src/album/AlbumDetails.jsx | 4 +- ui/src/album/AlbumGridView.jsx | 3 -- ui/src/artist/DesktopArtistDetails.jsx | 3 -- ui/src/artist/MobileArtistDetails.jsx | 3 -- ui/src/common/CoverArtAvatar.jsx | 10 +++- ui/src/common/CoverImage.jsx | 54 ++++++++++++-------- ui/src/common/CoverImage.test.jsx | 68 ++++++++++++++++++++++++++ ui/src/playlist/PlaylistDetails.jsx | 11 +---- ui/src/radio/RadioList.jsx | 8 ++- 9 files changed, 118 insertions(+), 46 deletions(-) create mode 100644 ui/src/common/CoverImage.test.jsx diff --git a/ui/src/album/AlbumDetails.jsx b/ui/src/album/AlbumDetails.jsx index c1dc7d7c5..534b176c0 100644 --- a/ui/src/album/AlbumDetails.jsx +++ b/ui/src/album/AlbumDetails.jsx @@ -87,9 +87,6 @@ const useStyles = makeStyles( backgroundColor: 'transparent', transition: 'opacity 0.3s ease-in-out', }, - coverLoading: { - opacity: 0.5, - }, loveButton: { top: theme.spacing(-0.2), left: theme.spacing(0.5), @@ -253,6 +250,7 @@ const AlbumDetails = (props) => {
setLightboxOpen(true)} diff --git a/ui/src/album/AlbumGridView.jsx b/ui/src/album/AlbumGridView.jsx index 6e35e4b23..69583597d 100644 --- a/ui/src/album/AlbumGridView.jsx +++ b/ui/src/album/AlbumGridView.jsx @@ -106,9 +106,6 @@ const useCoverStyles = makeStyles({ height: (props) => props.height, transition: 'opacity 0.3s ease-in-out', }, - coverLoading: { - opacity: 0, - }, }) const getColsForWidth = (width) => { diff --git a/ui/src/artist/DesktopArtistDetails.jsx b/ui/src/artist/DesktopArtistDetails.jsx index 64f2c6b10..0e5bd1401 100644 --- a/ui/src/artist/DesktopArtistDetails.jsx +++ b/ui/src/artist/DesktopArtistDetails.jsx @@ -44,9 +44,6 @@ const useStyles = makeStyles( transition: 'opacity 0.3s ease-in-out', objectFit: 'cover', }, - coverLoading: { - opacity: 0.5, - }, artistImage: { maxHeight: '12rem', minHeight: '12rem', diff --git a/ui/src/artist/MobileArtistDetails.jsx b/ui/src/artist/MobileArtistDetails.jsx index 1b3d68559..0d5fba156 100644 --- a/ui/src/artist/MobileArtistDetails.jsx +++ b/ui/src/artist/MobileArtistDetails.jsx @@ -55,9 +55,6 @@ const useStyles = makeStyles( transition: 'opacity 0.3s ease-in-out', objectFit: 'cover', }, - coverLoading: { - opacity: 0.5, - }, artistImage: { marginLeft: '1em', maxHeight: '7rem', diff --git a/ui/src/common/CoverArtAvatar.jsx b/ui/src/common/CoverArtAvatar.jsx index d932c9fb2..aab5d566d 100644 --- a/ui/src/common/CoverArtAvatar.jsx +++ b/ui/src/common/CoverArtAvatar.jsx @@ -17,7 +17,10 @@ const useStyles = makeStyles({ }, }) -export const CoverArtAvatar = ({ record: recordProp, variant = 'circular' }) => { +export const CoverArtAvatar = ({ + record: recordProp, + variant = 'circular', +}) => { const classes = useStyles() const recordContext = useRecordContext() const record = recordProp || recordContext @@ -28,7 +31,10 @@ export const CoverArtAvatar = ({ record: recordProp, variant = 'circular' }) => record={record} size={config.uiCoverArtSize} square={square} - className={clsx(classes.avatar, square ? classes.square : classes.circular)} + className={clsx( + classes.avatar, + square ? classes.square : classes.circular, + )} title={record.name} /> ) diff --git a/ui/src/common/CoverImage.jsx b/ui/src/common/CoverImage.jsx index b9ceb03b3..1de52bce1 100644 --- a/ui/src/common/CoverImage.jsx +++ b/ui/src/common/CoverImage.jsx @@ -7,18 +7,31 @@ import { useImageUrl } from './useImageUrl' import { BlurHashCanvas } from './BlurHashCanvas' const useStyles = makeStyles({ - root: { position: 'relative', display: 'inline-flex' }, - blur: { position: 'absolute', top: 0, left: 0, zIndex: 0 }, - img: { position: 'relative', zIndex: 1 }, + // className supplies the size and shape; overflow:hidden clips the fills to a rounded shape. + root: { + position: 'relative', + display: 'inline-flex', + overflow: 'hidden', + }, + fill: { + position: 'absolute', + top: 0, + left: 0, + width: '100%', + height: '100%', + }, + '@keyframes fadeIn': { from: { opacity: 0 }, to: { opacity: 1 } }, + img: { animation: '$fadeIn 0.3s ease-in-out' }, }) // CoverImage renders an entity's cover through the shared useImageUrl blob cache, so it survives -// React remounts without re-fetching, with the blurhash as the loading placeholder. `className` -// supplies the size (and any transition); the fade opacity is applied here. +// React remounts without re-fetching. The blurhash is the loading placeholder; the image is only +// mounted once its blob is ready, so an unresolved cover never renders as a broken . export const CoverImage = ({ record, size = config.uiCoverArtSize, square = false, + fit = 'cover', className, title, onClick, @@ -31,25 +44,23 @@ export const CoverImage = ({ const showBlurHash = loading && record.blurHash const handleClick = imgUrl && onClick ? onClick : undefined return ( -
+
{showBlurHash && ( - + )} + {imgUrl && ( + {title} )} - {title}
) } @@ -58,6 +69,7 @@ CoverImage.propTypes = { record: PropTypes.object, size: PropTypes.number, square: PropTypes.bool, + fit: PropTypes.oneOf(['cover', 'contain']), className: PropTypes.string, title: PropTypes.string, onClick: PropTypes.func, diff --git a/ui/src/common/CoverImage.test.jsx b/ui/src/common/CoverImage.test.jsx new file mode 100644 index 000000000..56459d5a5 --- /dev/null +++ b/ui/src/common/CoverImage.test.jsx @@ -0,0 +1,68 @@ +import { render } from '@testing-library/react' +import { describe, it, expect, vi, beforeEach } from 'vitest' + +vi.mock('./useImageUrl', () => ({ useImageUrl: vi.fn() })) +vi.mock('../subsonic', () => ({ + default: { getCoverArtUrl: () => '/rest/getCoverArt?id=al-1' }, +})) +vi.mock('../config', () => ({ default: { uiCoverArtSize: 300 } })) + +import { useImageUrl } from './useImageUrl' +import { CoverImage } from './CoverImage' + +const withArt = { + id: 'al-1', + name: 'Album', + blurHash: 'LEHV6nWB2yk8pyo0adR*.7kCMdnj', +} + +describe('CoverImage', () => { + beforeEach(() => { + vi.clearAllMocks() + // jsdom has no 2D context; stub it so BlurHashCanvas bails cleanly without console noise + HTMLCanvasElement.prototype.getContext = vi.fn(() => null) + }) + + it('renders nothing without a record', () => { + useImageUrl.mockReturnValue({ imgUrl: null, loading: false }) + const { container } = render() + expect(container.firstChild).toBeNull() + }) + + it('shows the blurhash and no while loading', () => { + useImageUrl.mockReturnValue({ imgUrl: null, loading: true }) + const { container } = render() + expect(container.querySelector('canvas')).not.toBeNull() + expect(container.querySelector('img')).toBeNull() + }) + + it('shows neither a broken nor a canvas while loading a record with no blurhash', () => { + useImageUrl.mockReturnValue({ imgUrl: null, loading: true }) + const { container } = render() + expect(container.querySelector('img')).toBeNull() + expect(container.querySelector('canvas')).toBeNull() + }) + + it('mounts the image only once its blob is ready', () => { + useImageUrl.mockReturnValue({ imgUrl: 'blob:abc', loading: false }) + const { container } = render() + const img = container.querySelector('img') + expect(img).not.toBeNull() + expect(img.getAttribute('src')).toBe('blob:abc') + }) + + it('fires onClick only when the image is loaded', () => { + const onClick = vi.fn() + useImageUrl.mockReturnValue({ imgUrl: null, loading: true }) + const { container, rerender } = render( + , + ) + container.firstChild.click() + expect(onClick).not.toHaveBeenCalled() + + useImageUrl.mockReturnValue({ imgUrl: 'blob:abc', loading: false }) + rerender() + container.firstChild.click() + expect(onClick).toHaveBeenCalledTimes(1) + }) +}) diff --git a/ui/src/playlist/PlaylistDetails.jsx b/ui/src/playlist/PlaylistDetails.jsx index 692c0572d..800a7e354 100644 --- a/ui/src/playlist/PlaylistDetails.jsx +++ b/ui/src/playlist/PlaylistDetails.jsx @@ -1,10 +1,5 @@ import { useState } from 'react' -import { - Card, - CardContent, - Typography, - useMediaQuery, -} from '@material-ui/core' +import { Card, CardContent, Typography, useMediaQuery } from '@material-ui/core' import { makeStyles } from '@material-ui/core/styles' import { useTranslate } from 'react-admin' import Lightbox from 'react-image-lightbox' @@ -74,9 +69,6 @@ const useStyles = makeStyles( backgroundColor: 'transparent', transition: 'opacity 0.3s ease-in-out', }, - coverLoading: { - opacity: 0.5, - }, title: { overflow: 'hidden', textOverflow: 'ellipsis', @@ -117,6 +109,7 @@ const PlaylistDetails = (props) => { setLightboxOpen(true)} diff --git a/ui/src/radio/RadioList.jsx b/ui/src/radio/RadioList.jsx index 2dd00feef..b127e84a8 100644 --- a/ui/src/radio/RadioList.jsx +++ b/ui/src/radio/RadioList.jsx @@ -22,7 +22,6 @@ import { useSelectedFields, } from '../common' import { CoverImage } from '../common/CoverImage' -import subsonic from '../subsonic' import { StreamField } from './StreamField' import { setTrack } from '../actions' import { songFromRadio } from './helper' @@ -103,7 +102,12 @@ const CoverArtField = ({ record }) => { ) } return ( - + ) } CoverArtField.defaultProps = { label: '' }