fix(artist-info): id-gate fetchers, share ArtistCard, square hero (#739)

* fix(artist-info): id-gate fetcher tuples, reuse ArtistCard on ArtistDetail

The cache-mismatch bug PR #732 fixed in `NowPlayingInfo.tsx` had the same
shape inside `ArtistCard` on the NowPlaying page: `useNowPlayingFetchers`
returned `artistInfo` for the previously-current artist for one render
after `artistId` changed, and `CachedImage` persisted that mismatched
blob under the new `artistInfo:<new-id>:hero` key in IndexedDB —
sticky "previous artist" image on every subsequent track.

Apply the same `{ id, value }` tuple pattern from PR #732 inside the
hooks themselves so every consumer is safe by construction:

- `useNowPlayingFetchers`: gate `artistInfo`, `songMeta`, `albumData`,
  `discography` on id-match at the return. Late-arriving resolves for
  a stale id can no longer overwrite the displayed value.
- `useArtistDetailData`: same for `info`. Required because the
  `ArtistDetail` bio card now uses `CachedImage` via the shared
  `ArtistCard` (previously raw `<img>`, no persistence hazard).

Unify `ArtistDetail`'s inline "About the Artist" block onto the same
shared `ArtistCard` so there is one source of truth for hero / bio /
similar rendering. New optional props: `onNavigate?` (omitted on
`/artist/:id` since the user is already there), `coverFallback`
(coverArt fallback when artistInfo has no hero image),
`hideArtistName` (avoid duplicating the hero name), `hideSimilar`
(ArtistDetail has its own similar-artists section).

Tests cover the gating contract for both hooks (incl. stale-resolve
race) and the `ArtistCard` prop matrix.

* fix(artist-image): square queue info hero, drop artist-avatar glow

- Queue Info bar's artist hero was rendered in a 16:10 wrap with
  `object-fit: cover`, so portrait photos lost top/bottom equally
  while landscape ones lost the sides — perceived as cropped even
  on roughly square sources. Set the wrap to 1:1 so the crop is
  symmetric and matches the typical square framing of artist
  photography.

- `ArtistDetail` extracted the cover's accent colour on every image
  load and rendered a 36px / 8px-spread `boxShadow` ring around the
  avatar. Drop the glow, the state, the one-shot reset effect, the
  prop-passing through `ArtistDetailHero`, and the now-orphaned
  `extractCoverColors` import on this page. `extractCoverColors`
  itself stays in place (still used by `useFsDynamicAccent`).

* docs(changelog): artist-info image fix extension + UI tweaks (PR #739)
This commit is contained in:
Frank Stellmacher
2026-05-17 00:43:05 +02:00
committed by GitHub
parent 97957df310
commit 6cc227d761
10 changed files with 493 additions and 88 deletions
+8
View File
@@ -603,6 +603,14 @@ Foundational work: faster reviews, narrower diffs, and a safety net under the pa
* The **Artist Biography** modal lived under the album page tree, where an ancestor broke **`position: fixed`** on the overlay — opening a long bio scrolled the whole page instead of staying pinned to the viewport, and the modal itself stretched past the visible area. It now mounts via **`createPortal(..., document.body)`** (same approach as **`RadioEditModal`** / **`CoverLightbox`**), so the overlay always pins to the viewport.
* A new **`.modal-content.bio-modal`** variant turns the modal into a flex column with **`overflow: hidden`** and an inner **`.bio-modal-body`** that handles the scrolling. The existing **`max-height: 80vh`** cap is now honored, and the title + close button stay pinned while the bio scrolls.
### Artist info — image-mismatch fix extended; square Queue Info hero; ArtistDetail glow removed
**By [@Psychotoxical](https://github.com/Psychotoxical), PR [#739](https://github.com/Psychotoxical/psysonic/pull/739)**
* The cache-mismatch shape PR [#732](https://github.com/Psychotoxical/psysonic/pull/732) fixed inside the Queue Info panel was latent in the **About the Artist** card on the **NowPlaying** page and would have surfaced there the moment that card used **`CachedImage`**. Fixed at the source: **`useNowPlayingFetchers`** and **`useArtistDetailData`** hold id-keyed entities as **`{ id, value }`** tuples and gate them on id-match at the return — every consumer is safe by construction. ArtistDetail's inline bio block is now the shared **`ArtistCard`** so there is a single rendering path.
* The artist hero in the **Queue Info** panel was rendered in a **16:10** wrap with **`object-fit: cover`**, so portrait photos always lost top and bottom equally — perceived as cropped even on roughly square sources. Now **1:1**, symmetric crop.
* The **ArtistDetail** avatar extracted the cover's accent colour on every image load and painted a 36px **`boxShadow`** ring around the photo. The glow is gone (the state, the reset effect, and the per-page **`extractCoverColors`** import are dropped too; the utility itself stays in place for **`useFsDynamicAccent`**).
## [1.45.0] - 2026-05-04
## Added
@@ -10,7 +10,6 @@ import { useOfflineStore } from '../../store/offlineStore';
import { useOfflineJobStore } from '../../store/offlineJobStore';
import { useAuthStore } from '../../store/authStore';
import { useIsMobile } from '../../hooks/useIsMobile';
import { extractCoverColors } from '../../utils/ui/dynamicColors';
import CachedImage from '../CachedImage';
import CoverLightbox from '../CoverLightbox';
import LastfmIcon from '../LastfmIcon';
@@ -42,8 +41,6 @@ interface Props {
coverRevision: number;
headerCoverFailed: boolean;
setHeaderCoverFailed: React.Dispatch<React.SetStateAction<boolean>>;
avatarGlow: string;
setAvatarGlow: React.Dispatch<React.SetStateAction<string>>;
lightboxOpen: boolean;
setLightboxOpen: React.Dispatch<React.SetStateAction<boolean>>;
}
@@ -55,7 +52,7 @@ export default function ArtistDetailHero({
openedLink, openLink,
coverId, artistCover300Src, artistCover300Key, artistCover2000Src,
coverRevision, headerCoverFailed, setHeaderCoverFailed,
avatarGlow, setAvatarGlow, lightboxOpen, setLightboxOpen,
lightboxOpen, setLightboxOpen,
}: Props) {
const { t } = useTranslation();
const navigate = useNavigate();
@@ -88,14 +85,7 @@ export default function ArtistDetailHero({
)}
<div className="artist-detail-header">
<div
className="artist-detail-avatar"
style={{
position: 'relative',
boxShadow: avatarGlow ? `0 0 36px 8px ${avatarGlow.replace('rgb(', 'rgba(').replace(')', ', 0.55)')}` : undefined,
transition: 'box-shadow 0.6s ease',
}}
>
<div className="artist-detail-avatar" style={{ position: 'relative' }}>
{coverId ? (
<button
className="artist-detail-avatar-btn"
@@ -109,7 +99,6 @@ export default function ArtistDetailHero({
cacheKey={artistCover300Key}
alt={artist.name}
style={{ width: '100%', height: '100%', objectFit: 'cover' }}
onLoad={e => extractCoverColors(e.currentTarget.src).then(({ accent }) => { if (accent) setAvatarGlow(accent); })}
onError={() => setHeaderCoverFailed(true)}
/>
) : (
@@ -0,0 +1,80 @@
/**
* `ArtistCard` characterization — covers the prop surface added when the
* card became the shared "About the Artist" component for both the
* NowPlaying page and ArtistDetail (replacing the inline bio block on
* /artist/:id and the previously-divergent rendering).
*/
import { describe, it, expect, vi } from 'vitest';
import { renderWithProviders } from '@/test/helpers/renderWithProviders';
import ArtistCard from './ArtistCard';
import type { SubsonicArtistInfo } from '../../api/subsonicTypes';
const infoWithImage: SubsonicArtistInfo = {
biography: 'Some bio text here.',
largeImageUrl: 'https://example.test/a-large.jpg',
similarArtist: [{ id: 'sim-1', name: 'Sister Act' }],
} as unknown as SubsonicArtistInfo;
describe('ArtistCard — optional onNavigate', () => {
it('hides the "Go to Artist" link when onNavigate is omitted', () => {
const { container } = renderWithProviders(
<ArtistCard artistName="A" artistId="art-1" artistInfo={infoWithImage} />,
);
expect(container.querySelector('.np-card-link')).toBeNull();
});
it('renders the "Go to Artist" link when onNavigate is provided', () => {
const { container } = renderWithProviders(
<ArtistCard artistName="A" artistId="art-1" artistInfo={infoWithImage} onNavigate={vi.fn()} />,
);
expect(container.querySelector('.np-card-link')).not.toBeNull();
});
});
describe('ArtistCard — hideArtistName / hideSimilar', () => {
it('does not render the artist name row when hideArtistName is set', () => {
const { container } = renderWithProviders(
<ArtistCard artistName="A" artistId="art-1" artistInfo={infoWithImage} hideArtistName />,
);
expect(container.querySelector('.np-dash-artist-name')).toBeNull();
});
it('does not render the similar-artists chip row when hideSimilar is set', () => {
const { container } = renderWithProviders(
<ArtistCard artistName="A" artistId="art-1" artistInfo={infoWithImage} hideSimilar />,
);
expect(container.querySelector('.np-dash-similar')).toBeNull();
});
});
describe('ArtistCard — coverFallback', () => {
it('uses coverFallback src + cacheKey when artistInfo has no hero image', () => {
const noImageInfo = { biography: 'b', similarArtist: [] } as unknown as SubsonicArtistInfo;
const { container } = renderWithProviders(
<ArtistCard
artistName="A"
artistId="art-1"
artistInfo={noImageInfo}
coverFallback={{ src: 'https://fallback.test/cover.jpg', cacheKey: 'fb:art-1:cover' }}
/>,
);
const img = container.querySelector<HTMLImageElement>('img.np-dash-artist-image');
expect(img).not.toBeNull();
// CachedImage swaps src to a blob URL asynchronously; the initial render
// shows the fetch URL — assert the *configured* fallback src is in play.
expect(img!.getAttribute('src') || '').toContain('fallback.test');
});
it('prefers info.largeImageUrl over the fallback when both are present', () => {
const { container } = renderWithProviders(
<ArtistCard
artistName="A"
artistId="art-1"
artistInfo={infoWithImage}
coverFallback={{ src: 'https://fallback.test/cover.jpg', cacheKey: 'fb:art-1:cover' }}
/>,
);
const img = container.querySelector<HTMLImageElement>('img.np-dash-artist-image');
expect(img!.getAttribute('src') || '').toContain('a-large.jpg');
});
});
+21 -8
View File
@@ -9,10 +9,20 @@ interface ArtistCardProps {
artistName: string;
artistId?: string;
artistInfo: SubsonicArtistInfo | null;
onNavigate: (path: string) => void;
/** When omitted the "Go to Artist" link and similar-artist chip click handlers do nothing — used on /artist/:id where the user is already there. */
onNavigate?: (path: string) => void;
/** Render fallback cover when artistInfo has no hero image (ArtistDetail's coverArt fallback). */
coverFallback?: { src: string; cacheKey: string };
/** Suppress the artist-name row — ArtistDetail shows the name in its hero already. */
hideArtistName?: boolean;
/** Suppress the similar-artists chip row — ArtistDetail has its own similar section. */
hideSimilar?: boolean;
}
const ArtistCard = memo(function ArtistCard({ artistName, artistId, artistInfo, onNavigate }: ArtistCardProps) {
const ArtistCard = memo(function ArtistCard({
artistName, artistId, artistInfo, onNavigate, coverFallback,
hideArtistName = false, hideSimilar = false,
}: ArtistCardProps) {
const { t } = useTranslation();
const [bioExpanded, setBioExpanded] = useState(false);
const [bioOverflows, setBioOverflows] = useState(false);
@@ -28,13 +38,16 @@ const ArtistCard = memo(function ArtistCard({ artistName, artistId, artistInfo,
setBioOverflows(el.scrollHeight - el.clientHeight > 1);
}, [bioHtml]);
const similar = artistInfo?.similarArtist ?? [];
const similar = hideSimilar ? [] : (artistInfo?.similarArtist ?? []);
const rawLarge = artistInfo?.largeImageUrl;
const rawMed = artistInfo?.mediumImageUrl;
const heroImage = isRealArtistImage(rawLarge)
const heroFromInfo = isRealArtistImage(rawLarge)
? rawLarge!
: isRealArtistImage(rawMed) ? rawMed! : '';
const heroCacheKey = artistId ? `artistInfo:${artistId}:hero` : '';
const heroImage = heroFromInfo || coverFallback?.src || '';
const heroCacheKey = heroFromInfo
? (artistId ? `artistInfo:${artistId}:hero` : '')
: (coverFallback?.cacheKey ?? '');
if (!bioHtml && similar.length === 0 && !heroImage) return null;
@@ -42,7 +55,7 @@ const ArtistCard = memo(function ArtistCard({ artistName, artistId, artistInfo,
<div className="np-info-card np-dash-card">
<div className="np-card-header">
<h3 className="np-card-title">{t('nowPlaying.aboutArtist')}</h3>
{artistId && (
{artistId && onNavigate && (
<button className="np-card-link" onClick={() => onNavigate(`/artist/${artistId}`)}>
{t('nowPlaying.goToArtist')} <ExternalLink size={12} />
</button>
@@ -60,7 +73,7 @@ const ArtistCard = memo(function ArtistCard({ artistName, artistId, artistInfo,
/>
)}
<div className="np-dash-artist-text">
<div className="np-dash-artist-name">{artistName}</div>
{!hideArtistName && <div className="np-dash-artist-name">{artistName}</div>}
{bioHtml && (
<>
<div
@@ -83,7 +96,7 @@ const ArtistCard = memo(function ArtistCard({ artistName, artistId, artistInfo,
<div className="np-dash-chip-row">
{similar.slice(0, 12).map((a, idx) => (
<span key={`${a.id}-${idx}`} className="np-chip"
onClick={() => a.id && onNavigate(`/artist/${a.id}`)}
onClick={() => a.id && onNavigate?.(`/artist/${a.id}`)}
data-tooltip={t('nowPlaying.goToArtist')}>
{a.name}
</span>
+98
View File
@@ -0,0 +1,98 @@
/**
* Regression tests for the id-gating tuple pattern in `useArtistDetailData`.
*
* `info` (the SubsonicArtistInfo returned by getArtistInfo) is held as a
* `{ id, value }` tuple internally and gated on id-match at the return
* statement. Without this gate, navigating between /artist/A → /artist/B
* would render one frame with A's `largeImageUrl` paired with B's id —
* exactly the cache-mismatch shape that PR #732 fixed for the queue info
* panel and that the shared ArtistCard (now used on this page) would
* otherwise persist into IndexedDB.
*/
import { renderHook, act, waitFor } from '@testing-library/react';
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import type { SubsonicArtistInfo } from '../api/subsonicTypes';
vi.mock('../api/subsonicArtists');
vi.mock('../api/subsonicSearch');
import { getArtist, getArtistInfo, getTopSongs } from '../api/subsonicArtists';
import { search } from '../api/subsonicSearch';
import { useArtistDetailData } from './useArtistDetailData';
const mockArtistInfo = vi.mocked(getArtistInfo) as unknown as {
mockImplementation: (impl: (id: string) => Promise<SubsonicArtistInfo | null>) => void;
};
beforeEach(() => {
vi.mocked(getTopSongs).mockResolvedValue([]);
vi.mocked(search).mockResolvedValue({ songs: [], albums: [], artists: [] } as any);
});
afterEach(() => {
vi.clearAllMocks();
});
function deferred<T>() {
let resolve!: (v: T) => void;
let reject!: (e?: unknown) => void;
const promise = new Promise<T>((res, rej) => { resolve = res; reject = rej; });
return { promise, resolve, reject };
}
describe('useArtistDetailData — id-gated info', () => {
it('returns null info when id changes before the new fetch resolves', async () => {
vi.mocked(getArtist).mockImplementation(async (id) => (
{ artist: { id, name: id }, albums: [] } as any
));
const a = deferred<SubsonicArtistInfo | null>();
const b = deferred<SubsonicArtistInfo | null>();
mockArtistInfo.mockImplementation(async (id) => {
if (id === 'A') return a.promise;
if (id === 'B') return b.promise;
return null;
});
const { result, rerender } = renderHook(
({ id }: { id: string }) => useArtistDetailData(id),
{ initialProps: { id: 'A' } },
);
await act(async () => { a.resolve({ largeImageUrl: 'A.jpg' } as SubsonicArtistInfo); });
await waitFor(() => expect(result.current.info).toEqual({ largeImageUrl: 'A.jpg' }));
// Switch to artist B. info must flip to null until B's fetch resolves —
// it must never carry A's largeImageUrl paired with B's id, since the
// shared ArtistCard would build a `coverArtCacheKey(B, 80)` from `id`
// and pair it with A's URL inside CachedImage.
rerender({ id: 'B' });
expect(result.current.info).toBeNull();
await act(async () => { b.resolve({ largeImageUrl: 'B.jpg' } as SubsonicArtistInfo); });
await waitFor(() => expect(result.current.info).toEqual({ largeImageUrl: 'B.jpg' }));
});
it('ignores a late-arriving resolve for a stale id', async () => {
vi.mocked(getArtist).mockImplementation(async (id) => (
{ artist: { id, name: id }, albums: [] } as any
));
const a = deferred<SubsonicArtistInfo | null>();
mockArtistInfo.mockImplementation(async (id) => {
if (id === 'A') return a.promise;
if (id === 'B') return { largeImageUrl: 'B.jpg' } as SubsonicArtistInfo;
return null;
});
const { result, rerender } = renderHook(
({ id }: { id: string }) => useArtistDetailData(id),
{ initialProps: { id: 'A' } },
);
rerender({ id: 'B' });
await waitFor(() => expect(result.current.info).toEqual({ largeImageUrl: 'B.jpg' }));
// A's late resolve must not overwrite B's info.
await act(async () => { a.resolve({ largeImageUrl: 'A.jpg' } as SubsonicArtistInfo); });
expect(result.current.info).toEqual({ largeImageUrl: 'B.jpg' });
});
});
+9 -4
View File
@@ -30,7 +30,10 @@ export function useArtistDetailData(id: string | undefined): ArtistDetailDataRes
const [albums, setAlbums] = useState<SubsonicAlbum[]>([]);
const [featuredAlbums, setFeaturedAlbums] = useState<SubsonicAlbum[]>([]);
const [topSongs, setTopSongs] = useState<SubsonicSong[]>([]);
const [info, setInfo] = useState<SubsonicArtistInfo | null>(null);
// Tuple gates `info` on id-match so a CachedImage-style consumer (shared
// ArtistCard) can never see info from a previously-viewed artist paired
// with the current `id`. Same pattern as `useNowPlayingFetchers`.
const [infoEntry, setInfoEntry] = useState<{ id: string; value: SubsonicArtistInfo | null } | null>(null);
const [loading, setLoading] = useState(true);
const [isStarred, setIsStarred] = useState(false);
const [artistInfoLoading, setArtistInfoLoading] = useState(false);
@@ -40,7 +43,7 @@ export function useArtistDetailData(id: string | undefined): ArtistDetailDataRes
if (!id) return;
let cancelled = false;
setLoading(true);
setInfo(null);
setInfoEntry(null);
setTopSongs([]);
setFeaturedAlbums([]);
getArtist(id).then(artistData => {
@@ -66,10 +69,10 @@ export function useArtistDetailData(id: string | undefined): ArtistDetailDataRes
setArtistInfoLoading(true);
getArtistInfo(id, { similarArtistCount: audiomuseNavidromeEnabled ? 24 : undefined })
.then(artistInfo => {
if (!cancelled) setInfo(artistInfo ?? null);
if (!cancelled) setInfoEntry({ id, value: artistInfo ?? null });
})
.catch(() => {
if (!cancelled) setInfo(null);
if (!cancelled) setInfoEntry({ id, value: null });
})
.finally(() => {
if (!cancelled) setArtistInfoLoading(false);
@@ -113,6 +116,8 @@ export function useArtistDetailData(id: string | undefined): ArtistDetailDataRes
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [artist?.id, musicLibraryFilterVersion]);
const info = infoEntry && infoEntry.id === id ? infoEntry.value : null;
return {
artist, setArtist, albums, topSongs, info, featuredAlbums,
loading, artistInfoLoading, featuredLoading,
+211
View File
@@ -0,0 +1,211 @@
/**
* Regression tests for the id-gating tuple pattern in `useNowPlayingFetchers`.
*
* Each id-keyed slot (`artistInfo`, `songMeta`, `albumData`, `discography`) is
* held as a `{ id, value }` tuple internally and gated on id-match at the
* return statement. This guarantees that consumers building a `cacheKey` from
* the current id can never receive a value paired with a previously-current
* id — the bug that PR #732 fixed inside `NowPlayingInfo.tsx` and that this
* hook would otherwise leak into every other consumer (e.g. ArtistCard on the
* NowPlaying page).
*/
import { renderHook, act, waitFor } from '@testing-library/react';
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import type { SubsonicArtistInfo, SubsonicSong, SubsonicAlbum } from '../api/subsonicTypes';
vi.mock('../api/subsonicArtists');
vi.mock('../api/subsonicLibrary');
vi.mock('../api/bandsintown');
vi.mock('../api/lastfm');
import { getArtist, getArtistInfo, getTopSongs } from '../api/subsonicArtists';
import { getAlbum, getSong } from '../api/subsonicLibrary';
import { fetchBandsintownEvents } from '../api/bandsintown';
import { lastfmGetArtistStats, lastfmGetTrackInfo, lastfmIsConfigured } from '../api/lastfm';
import { useNowPlayingFetchers, type NowPlayingFetchersDeps } from './useNowPlayingFetchers';
// The real getArtistInfo signature returns `Promise<SubsonicArtistInfo>`, but
// the hook treats `null` as the "no info available" case and stores it as
// such in its tuple. The tests mirror that — cast to a nullable-returning
// shape so we can mock the empty case without `as any` at every site.
const mockArtistInfo = vi.mocked(getArtistInfo) as unknown as {
mockImplementation: (impl: (id: string) => Promise<SubsonicArtistInfo | null>) => void;
mockResolvedValue: (v: SubsonicArtistInfo | null) => void;
};
const baseDeps: NowPlayingFetchersDeps = {
songId: undefined,
artistId: undefined,
albumId: undefined,
artistName: '',
enableBandsintown: false,
audiomuseNavidromeEnabled: false,
lastfmUsername: '',
currentTrack: null,
subsonicServerId: 'srv1',
fetchEnabled: true,
};
beforeEach(() => {
vi.mocked(getTopSongs).mockResolvedValue([]);
vi.mocked(getArtist).mockResolvedValue({ albums: [] } as any);
vi.mocked(fetchBandsintownEvents).mockResolvedValue([]);
vi.mocked(lastfmIsConfigured).mockReturnValue(false);
vi.mocked(lastfmGetTrackInfo).mockResolvedValue(null);
vi.mocked(lastfmGetArtistStats).mockResolvedValue(null);
});
afterEach(() => {
vi.clearAllMocks();
});
/** Deferred promise helper — lets a test step the fetch resolution manually. */
function deferred<T>() {
let resolve!: (v: T) => void;
let reject!: (e?: unknown) => void;
const promise = new Promise<T>((res, rej) => { resolve = res; reject = rej; });
return { promise, resolve, reject };
}
describe('useNowPlayingFetchers — id-gated artistInfo', () => {
it('returns null artistInfo while the previously-resolved info belongs to a different artistId', async () => {
const a = deferred<SubsonicArtistInfo | null>();
const b = deferred<SubsonicArtistInfo | null>();
mockArtistInfo.mockImplementation(async (id) => {
if (id === 'art-A') return a.promise;
if (id === 'art-B') return b.promise;
return null;
});
const { result, rerender } = renderHook(
({ artistId }: { artistId: string }) =>
useNowPlayingFetchers({ ...baseDeps, artistId, artistName: artistId }),
{ initialProps: { artistId: 'art-A' } },
);
// Before any resolve, no info yet.
expect(result.current.artistInfo).toBeNull();
// Resolve A — artistInfo becomes A's info.
await act(async () => { a.resolve({ largeImageUrl: 'A.jpg' } as SubsonicArtistInfo); });
await waitFor(() => {
expect(result.current.artistInfo).toEqual({ largeImageUrl: 'A.jpg' });
});
// Switch to artistId B. Without id-gating, artistInfo would still be A's
// info paired with the new B id — the bug that PR #732 fixed in the queue
// info panel. With gating, artistInfo flips to null immediately.
rerender({ artistId: 'art-B' });
expect(result.current.artistInfo).toBeNull();
// Resolve B — artistInfo now becomes B's info, never paired with A.
await act(async () => { b.resolve({ largeImageUrl: 'B.jpg' } as SubsonicArtistInfo); });
await waitFor(() => {
expect(result.current.artistInfo).toEqual({ largeImageUrl: 'B.jpg' });
});
});
it('does not leak a late-arriving resolve for a stale artistId', async () => {
// Race: artist A's fetch resolves AFTER the consumer switched to B.
const a = deferred<SubsonicArtistInfo | null>();
mockArtistInfo.mockImplementation(async (id) => {
if (id === 'art-A') return a.promise;
if (id === 'art-B') return { largeImageUrl: 'B.jpg' } as SubsonicArtistInfo;
return null;
});
const { result, rerender } = renderHook(
({ artistId }: { artistId: string }) =>
useNowPlayingFetchers({ ...baseDeps, artistId, artistName: artistId }),
{ initialProps: { artistId: 'art-A' } },
);
// Switch to B before A resolves.
rerender({ artistId: 'art-B' });
await waitFor(() => {
expect(result.current.artistInfo).toEqual({ largeImageUrl: 'B.jpg' });
});
// Late A resolve must not overwrite the displayed value for B.
await act(async () => { a.resolve({ largeImageUrl: 'A.jpg' } as SubsonicArtistInfo); });
expect(result.current.artistInfo).toEqual({ largeImageUrl: 'B.jpg' });
});
});
describe('useNowPlayingFetchers — id-gated songMeta / albumData / discography', () => {
it('gates songMeta on songId match', async () => {
const s1 = deferred<SubsonicSong | null>();
const s2 = deferred<SubsonicSong | null>();
vi.mocked(getSong).mockImplementation(async (id) => {
if (id === 's1') return s1.promise;
if (id === 's2') return s2.promise;
return null;
});
const { result, rerender } = renderHook(
({ songId }: { songId: string }) =>
useNowPlayingFetchers({ ...baseDeps, songId }),
{ initialProps: { songId: 's1' } },
);
await act(async () => { s1.resolve({ id: 's1', title: 'Track 1' } as SubsonicSong); });
await waitFor(() => expect(result.current.songMeta?.title).toBe('Track 1'));
rerender({ songId: 's2' });
expect(result.current.songMeta).toBeNull();
await act(async () => { s2.resolve({ id: 's2', title: 'Track 2' } as SubsonicSong); });
await waitFor(() => expect(result.current.songMeta?.title).toBe('Track 2'));
});
it('gates albumData on albumId match', async () => {
const al1 = deferred<{ album: SubsonicAlbum; songs: SubsonicSong[] } | null>();
const al2 = deferred<{ album: SubsonicAlbum; songs: SubsonicSong[] } | null>();
vi.mocked(getAlbum).mockImplementation(async (id) => {
if (id === 'alb1') return al1.promise as any;
if (id === 'alb2') return al2.promise as any;
return null as any;
});
const { result, rerender } = renderHook(
({ albumId }: { albumId: string }) =>
useNowPlayingFetchers({ ...baseDeps, albumId }),
{ initialProps: { albumId: 'alb1' } },
);
await act(async () => { al1.resolve({ album: { id: 'alb1', name: 'A1' } as SubsonicAlbum, songs: [] }); });
await waitFor(() => expect(result.current.albumData?.album.name).toBe('A1'));
rerender({ albumId: 'alb2' });
expect(result.current.albumData).toBeNull();
await act(async () => { al2.resolve({ album: { id: 'alb2', name: 'A2' } as SubsonicAlbum, songs: [] }); });
await waitFor(() => expect(result.current.albumData?.album.name).toBe('A2'));
});
it('gates discography on artistId match (empty fallback while gated)', async () => {
const d1 = deferred<{ artist: any; albums: SubsonicAlbum[] }>();
const d2 = deferred<{ artist: any; albums: SubsonicAlbum[] }>();
vi.mocked(getArtist).mockImplementation(async (id) => {
if (id === 'art-D1') return d1.promise as any;
if (id === 'art-D2') return d2.promise as any;
return { albums: [] } as any;
});
mockArtistInfo.mockResolvedValue(null);
const { result, rerender } = renderHook(
({ artistId }: { artistId: string }) =>
useNowPlayingFetchers({ ...baseDeps, artistId, artistName: artistId }),
{ initialProps: { artistId: 'art-D1' } },
);
await act(async () => { d1.resolve({ artist: {}, albums: [{ id: 'al-D1' } as SubsonicAlbum] }); });
await waitFor(() => expect(result.current.discography.map(a => a.id)).toEqual(['al-D1']));
rerender({ artistId: 'art-D2' });
expect(result.current.discography).toEqual([]);
await act(async () => { d2.resolve({ artist: {}, albums: [{ id: 'al-D2' } as SubsonicAlbum] }); });
await waitFor(() => expect(result.current.discography.map(a => a.id)).toEqual(['al-D2']));
});
});
+53 -25
View File
@@ -50,62 +50,81 @@ function subsonicCacheKey(serverId: string, id: string): string {
return serverId ? `${serverId}:${id}` : id;
}
// id-keyed slots are held as `{ id, value }` tuples and gated on id-match in
// the return statement. Without the gate, a track switch renders one frame
// with the previous track's value paired with the new id — consumers that
// build a cacheKey from the new id (e.g. CachedImage) would persist a
// mismatched blob in IndexedDB and never recover. See PR #732 for the same
// fix inside `NowPlayingInfo.tsx`.
type IdSlot<T> = { id: string; value: T } | null;
function seedSlot<T>(id: string, lookup: (id: string) => T | undefined): IdSlot<T> {
if (!id) return null;
const cached = lookup(id);
return cached === undefined ? null : { id, value: cached };
}
export function useNowPlayingFetchers(deps: NowPlayingFetchersDeps): NowPlayingFetchersResult {
const {
songId, artistId, albumId, artistName, enableBandsintown, audiomuseNavidromeEnabled,
lastfmUsername, currentTrack, subsonicServerId, fetchEnabled = true,
} = deps;
// Entity state, seeded from TTL cache so same-artist song switches are instant
const [songMeta, setSongMeta] = useState<SubsonicSong | null>(() =>
songId && subsonicServerId ? songMetaCache.get(subsonicCacheKey(subsonicServerId, songId)) ?? null : null);
const [artistInfo, setArtistInfo] = useState<SubsonicArtistInfo | null>(() =>
artistId && subsonicServerId ? artistInfoCache.get(subsonicCacheKey(subsonicServerId, artistId)) ?? null : null);
const [albumData, setAlbumData] = useState<{ album: SubsonicAlbum; songs: SubsonicSong[] } | null>(() =>
albumId && subsonicServerId ? albumCache.get(subsonicCacheKey(subsonicServerId, albumId)) ?? null : null);
// id-keyed entity state seeded from TTL cache so same-artist song switches
// are instant. Held as `{ id, value }` tuples and gated below.
const [songMetaEntry, setSongMetaEntry] = useState<IdSlot<SubsonicSong | null>>(() =>
seedSlot(songId && subsonicServerId ? songId : '', k => songMetaCache.get(subsonicCacheKey(subsonicServerId, k))));
const [artistInfoEntry, setArtistInfoEntry] = useState<IdSlot<SubsonicArtistInfo | null>>(() =>
seedSlot(artistId && subsonicServerId ? artistId : '', k => artistInfoCache.get(subsonicCacheKey(subsonicServerId, k))));
const [albumDataEntry, setAlbumDataEntry] = useState<IdSlot<{ album: SubsonicAlbum; songs: SubsonicSong[] } | null>>(() =>
seedSlot(albumId && subsonicServerId ? albumId : '', k => albumCache.get(subsonicCacheKey(subsonicServerId, k))));
const [discographyEntry, setDiscographyEntry] = useState<IdSlot<SubsonicAlbum[]>>(() =>
seedSlot(artistId && subsonicServerId ? artistId : '', k => discographyCache.get(subsonicCacheKey(subsonicServerId, k))));
// Name-keyed / global state — no cacheKey/persistence hazard, kept as plain state.
const [topSongs, setTopSongs] = useState<SubsonicSong[]>(() =>
artistName && subsonicServerId ? topSongsCache.get(subsonicCacheKey(subsonicServerId, artistName)) ?? [] : []);
const [tourEvents, setTourEvents] = useState<BandsintownEvent[]>(() => artistName ? tourCache.get(artistName) ?? [] : []);
const [tourLoading, setTourLoading] = useState(false);
const [discography, setDiscography] = useState<SubsonicAlbum[]>(() =>
artistId && subsonicServerId ? discographyCache.get(subsonicCacheKey(subsonicServerId, artistId)) ?? [] : []);
const [lfmTrack, setLfmTrack] = useState<LastfmTrackInfo | null>(null);
const [lfmArtist, setLfmArtist] = useState<LastfmArtistStats | null>(null);
// Fetch batch per entity change (not per song switch — same-artist songs share artist/top/tour fetches)
useEffect(() => {
if (!fetchEnabled || !subsonicServerId || !songId) { setSongMeta(null); return; }
if (!fetchEnabled || !subsonicServerId || !songId) { setSongMetaEntry(null); return; }
const cacheKey = subsonicCacheKey(subsonicServerId, songId);
const cached = songMetaCache.get(cacheKey);
if (cached !== undefined) { setSongMeta(cached); return; }
if (cached !== undefined) { setSongMetaEntry({ id: songId, value: cached }); return; }
setSongMetaEntry(null);
let cancelled = false;
getSong(songId)
.then(v => { if (!cancelled) { songMetaCache.set(cacheKey, v ?? null); setSongMeta(v ?? null); } })
.catch(() => { if (!cancelled) { songMetaCache.set(cacheKey, null); setSongMeta(null); } });
.then(v => { if (!cancelled) { songMetaCache.set(cacheKey, v ?? null); setSongMetaEntry({ id: songId, value: v ?? null }); } })
.catch(() => { if (!cancelled) { songMetaCache.set(cacheKey, null); setSongMetaEntry({ id: songId, value: null }); } });
return () => { cancelled = true; };
}, [fetchEnabled, subsonicServerId, songId]);
useEffect(() => {
if (!fetchEnabled || !subsonicServerId || !artistId) { setArtistInfo(null); return; }
if (!fetchEnabled || !subsonicServerId || !artistId) { setArtistInfoEntry(null); return; }
const cacheKey = subsonicCacheKey(subsonicServerId, artistId);
const cached = artistInfoCache.get(cacheKey);
if (cached !== undefined) { setArtistInfo(cached); return; }
if (cached !== undefined) { setArtistInfoEntry({ id: artistId, value: cached }); return; }
setArtistInfoEntry(null);
let cancelled = false;
getArtistInfo(artistId, { similarArtistCount: audiomuseNavidromeEnabled ? 24 : undefined })
.then(v => { if (!cancelled) { artistInfoCache.set(cacheKey, v ?? null); setArtistInfo(v ?? null); } })
.catch(() => { if (!cancelled) { artistInfoCache.set(cacheKey, null); setArtistInfo(null); } });
.then(v => { if (!cancelled) { artistInfoCache.set(cacheKey, v ?? null); setArtistInfoEntry({ id: artistId, value: v ?? null }); } })
.catch(() => { if (!cancelled) { artistInfoCache.set(cacheKey, null); setArtistInfoEntry({ id: artistId, value: null }); } });
return () => { cancelled = true; };
}, [fetchEnabled, subsonicServerId, artistId, audiomuseNavidromeEnabled]);
useEffect(() => {
if (!fetchEnabled || !subsonicServerId || !albumId) { setAlbumData(null); return; }
if (!fetchEnabled || !subsonicServerId || !albumId) { setAlbumDataEntry(null); return; }
const cacheKey = subsonicCacheKey(subsonicServerId, albumId);
const cached = albumCache.get(cacheKey);
if (cached !== undefined) { setAlbumData(cached); return; }
if (cached !== undefined) { setAlbumDataEntry({ id: albumId, value: cached }); return; }
setAlbumDataEntry(null);
let cancelled = false;
getAlbum(albumId)
.then(v => { if (!cancelled) { albumCache.set(cacheKey, v); setAlbumData(v); } })
.catch(() => { if (!cancelled) { albumCache.set(cacheKey, null); setAlbumData(null); } });
.then(v => { if (!cancelled) { albumCache.set(cacheKey, v); setAlbumDataEntry({ id: albumId, value: v }); } })
.catch(() => { if (!cancelled) { albumCache.set(cacheKey, null); setAlbumDataEntry({ id: albumId, value: null }); } });
return () => { cancelled = true; };
}, [fetchEnabled, subsonicServerId, albumId]);
@@ -135,14 +154,15 @@ export function useNowPlayingFetchers(deps: NowPlayingFetchersDeps): NowPlayingF
// Discography via getArtist
useEffect(() => {
if (!fetchEnabled || !subsonicServerId || !artistId) { setDiscography([]); return; }
if (!fetchEnabled || !subsonicServerId || !artistId) { setDiscographyEntry(null); return; }
const cacheKey = subsonicCacheKey(subsonicServerId, artistId);
const cached = discographyCache.get(cacheKey);
if (cached !== undefined) { setDiscography(cached); return; }
if (cached !== undefined) { setDiscographyEntry({ id: artistId, value: cached }); return; }
setDiscographyEntry(null);
let cancelled = false;
getArtist(artistId)
.then(v => { if (!cancelled) { discographyCache.set(cacheKey, v.albums); setDiscography(v.albums); } })
.catch(() => { if (!cancelled) { discographyCache.set(cacheKey, []); setDiscography([]); } });
.then(v => { if (!cancelled) { discographyCache.set(cacheKey, v.albums); setDiscographyEntry({ id: artistId, value: v.albums }); } })
.catch(() => { if (!cancelled) { discographyCache.set(cacheKey, []); setDiscographyEntry({ id: artistId, value: [] }); } });
return () => { cancelled = true; };
}, [fetchEnabled, subsonicServerId, artistId]);
@@ -172,5 +192,13 @@ export function useNowPlayingFetchers(deps: NowPlayingFetchersDeps): NowPlayingF
return () => { cancelled = true; };
}, [lfmArtistKey, artistName, lastfmUsername]);
// Gate id-keyed slots on id-match so consumers never see a value paired
// with the wrong id, even on the single render between an id change and
// the next effect run.
const songMeta = songMetaEntry && songMetaEntry.id === songId ? songMetaEntry.value : null;
const artistInfo = artistInfoEntry && artistInfoEntry.id === artistId ? artistInfoEntry.value : null;
const albumData = albumDataEntry && albumDataEntry.id === albumId ? albumDataEntry.value : null;
const discography = discographyEntry && discographyEntry.id === artistId ? discographyEntry.value : [];
return { songMeta, artistInfo, albumData, topSongs, tourEvents, tourLoading, discography, lfmTrack, lfmArtist };
}
+10 -37
View File
@@ -24,11 +24,9 @@ import LastfmIcon from '../components/LastfmIcon';
import { invalidateCoverArt } from '../utils/imageCache';
import { showToast } from '../utils/ui/toast';
import { copyEntityShareLink } from '../utils/share/copyEntityShareLink';
import { extractCoverColors } from '../utils/ui/dynamicColors';
import StarRating from '../components/StarRating';
import { useArtistLayoutStore, type ArtistSectionId } from '../store/artistLayoutStore';
import { sanitizeHtml } from '../utils/sanitizeHtml';
import { useArtistDetailData } from '../hooks/useArtistDetailData';
import { useArtistSimilarArtists } from '../hooks/useArtistSimilarArtists';
import {
@@ -40,6 +38,7 @@ import {
import ArtistDetailHero from '../components/artistDetail/ArtistDetailHero';
import ArtistDetailTopTracks from '../components/artistDetail/ArtistDetailTopTracks';
import ArtistDetailSimilarArtists from '../components/artistDetail/ArtistDetailSimilarArtists';
import ArtistCard from '../components/nowPlaying/ArtistCard';
import { usePerfProbeFlags } from '../utils/perf/perfFlags';
import { VirtualCardGrid } from '../components/VirtualCardGrid';
@@ -59,12 +58,10 @@ export default function ArtistDetail() {
const [openedLink, setOpenedLink] = useState<string | null>(null);
const { similarArtists, similarLoading } = useArtistSimilarArtists(artist, info, artistInfoLoading);
const [lightboxOpen, setLightboxOpen] = useState(false);
const [bioExpanded, setBioExpanded] = useState(false);
const [uploading, setUploading] = useState(false);
const [similarCollapsed, setSimilarCollapsed] = useState(true);
const isMobile = useIsMobile();
const [coverRevision, setCoverRevision] = useState(0);
const [avatarGlow, setAvatarGlow] = useState('');
/** True after header CachedImage onError — avoid `display:none` on the img (breaks recovery). */
const [headerCoverFailed, setHeaderCoverFailed] = useState(false);
const imageInputRef = useRef<HTMLInputElement>(null);
@@ -94,10 +91,6 @@ export default function ArtistDetail() {
const [artistEntityRating, setArtistEntityRating] = useState(0);
useEffect(() => {
setAvatarGlow('');
}, [id]);
useEffect(() => {
if (!id) return;
if (artist && artist.id === id) setArtistEntityRating(artist.userRating ?? 0);
@@ -295,8 +288,6 @@ export default function ArtistDetail() {
coverRevision={coverRevision}
headerCoverFailed={headerCoverFailed}
setHeaderCoverFailed={setHeaderCoverFailed}
avatarGlow={avatarGlow}
setAvatarGlow={setAvatarGlow}
lightboxOpen={lightboxOpen}
setLightboxOpen={setLightboxOpen}
/>
@@ -307,33 +298,15 @@ export default function ArtistDetail() {
{renderableSectionIds.map(sectionId => {
switch (sectionId) {
case 'bio': return (
<div
key="bio"
className="np-info-card artist-bio-card"
style={{ marginTop: sectionMt('bio') }}
>
<div className="np-card-header">
<h3 className="np-card-title">{t('nowPlaying.aboutArtist')}</h3>
</div>
<div className="np-artist-bio-row">
{(info?.largeImageUrl || coverId) && (
<img
src={info?.largeImageUrl || artistCover80FallbackSrc}
alt={artist.name}
className="np-artist-thumb"
onError={e => { (e.target as HTMLImageElement).style.display = 'none'; }}
/>
)}
<div className="np-bio-wrap">
<div
className={`np-bio-text${bioExpanded ? ' expanded' : ''}`}
dangerouslySetInnerHTML={{ __html: sanitizeHtml(info!.biography!) }}
/>
<button className="np-bio-toggle" onClick={() => setBioExpanded(v => !v)}>
{bioExpanded ? t('nowPlaying.showLess') : t('nowPlaying.readMore')}
</button>
</div>
</div>
<div key="bio" style={{ marginTop: sectionMt('bio') }}>
<ArtistCard
artistName={artist.name}
artistId={id}
artistInfo={info}
hideArtistName
hideSimilar
coverFallback={coverId ? { src: artistCover80FallbackSrc, cacheKey: coverArtCacheKey(coverId, 80) } : undefined}
/>
</div>
);
+1 -1
View File
@@ -576,7 +576,7 @@ html[data-perf-disable-home-artwork-clip="true"] .home-lite-artwork .song-card-c
/* Artist card */
.np-info-artist-image-wrap {
width: 100%;
aspect-ratio: 16 / 10;
aspect-ratio: 1 / 1;
border-radius: 12px;
overflow: hidden;
background: var(--bg-elevated);