From 3ce4a018e088ef043c66bd9735792ee27d542ef3 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 30 Jul 2026 01:05:05 -0400 Subject: [PATCH] fix(ui): stop artwork refreshes reloading the whole page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolving an album broadcast song:["*"], because a track with no art of its own is served its album's and the dependent ids are unbounded server-side. useResourceRefresh checked for that wildcard across every resource in the payload, not just the ones a component shows, so the album grid called refresh() — a full page reload — for every drained batch. Upgrading a large library reloaded the grid thousands of times. The client knows what the server cannot: which tracks are loaded, and their albumId. So the fan-out moves there, the wildcard check is scoped to watched resources, and the backend now names only what it resolved. The playlist views watch playlistTrack too: their rows carry albumId but are keyed by playlist entry, so a song refresh never reached them — previously masked by the wildcard's page reload. Signed-off-by: Deluan --- core/artwork/worker.go | 7 -- core/artwork/worker_test.go | 11 +-- ui/src/common/useResourceRefresh.jsx | 53 +++++++++++---- ui/src/common/useResourceRefresh.test.js | 87 ++++++++++++++++++++++++ ui/src/playlist/PlaylistShow.jsx | 2 +- ui/src/playlist/PlaylistSongs.jsx | 3 +- 6 files changed, 135 insertions(+), 28 deletions(-) diff --git a/core/artwork/worker.go b/core/artwork/worker.go index 115c3ce87..d09889aeb 100644 --- a/core/artwork/worker.go +++ b/core/artwork/worker.go @@ -250,13 +250,6 @@ func (w *Worker) broadcastRefresh(ctx context.Context, found []model.ArtworkQueu for res, ids := range byResource { event = event.With(res, ids...) } - // A track with no art of its own is served its album's, so an album change moves the track's - // hash too. The dependent id list is unbounded, so refresh the resource as a whole. - if _, ok := byResource["album"]; ok { - if _, ok := byResource["song"]; !ok { - event = event.With("song") - } - } w.broker.SendBroadcastMessage(ctx, event) } diff --git a/core/artwork/worker_test.go b/core/artwork/worker_test.go index 5a282ccee..f548bb0e9 100644 --- a/core/artwork/worker_test.go +++ b/core/artwork/worker_test.go @@ -440,10 +440,9 @@ var _ = Describe("Worker", func() { Expect(data).To(ContainSubstring("al2")) Expect(data).ToNot(ContainSubstring("artist"), "a failed (unresolved) artist must not be refreshed") Expect(data).ToNot(ContainSubstring("ar1")) - Expect(data).To(ContainSubstring(`"song"`), "tracks with no art of their own are served the album's") }) - DescribeTable("only pairs songs with album refreshes", + DescribeTable("only lists the kinds it actually resolved", func(kinds []string, wantSong bool) { items := slice.Map(kinds, func(k string) model.ArtworkQueueItem { return model.ArtworkQueueItem{ItemKind: k, ItemID: k + "1"} @@ -459,11 +458,13 @@ var _ = Describe("Worker", func() { Expect(data).ToNot(ContainSubstring(`"song"`)) } }, - Entry("album alone drags songs along", []string{"al"}, true), + // An album's tracks inherit its art, but the dependent ids are unbounded: the client + // fans an album refresh out to the tracks it has loaded. + Entry("album alone does not name songs", []string{"al"}, false), Entry("artist alone does not", []string{"ar"}, false), Entry("playlist alone does not", []string{"pl"}, false), - Entry("album mixed with others still does", []string{"ar", "al"}, true), - Entry("songs resolving on their own stay single-listed", []string{"mf"}, true), + Entry("album mixed with others still does not", []string{"ar", "al"}, false), + Entry("songs resolving on their own are listed by id", []string{"mf"}, true), ) It("broadcasts a refresh when an item resolves to absent (removed cover)", func() { diff --git a/ui/src/common/useResourceRefresh.jsx b/ui/src/common/useResourceRefresh.jsx index 52ae88738..b59ae8e88 100644 --- a/ui/src/common/useResourceRefresh.jsx +++ b/ui/src/common/useResourceRefresh.jsx @@ -63,6 +63,9 @@ import { useRefresh, useDataProvider } from 'react-admin' * - Global refresh: { '*': '*' } or { someResource: ['*'] } * - Specific resources: { album: ['id1', 'id2'], song: ['id3'] } */ +// Resources whose records are media files, and so inherit their album's artwork. +const trackResources = ['song', 'playlistTrack'] + export const useResourceRefresh = (...visibleResources) => { const [lastTime, setLastTime] = useState(Date.now()) const refresh = useRefresh() @@ -78,27 +81,49 @@ export const useResourceRefresh = (...visibleResources) => { } setLastTime(lastReceived) - if ( + const isWatched = (r) => + visibleResources.length === 0 || visibleResources.includes(r) + // A wildcard on a resource this component does not show is somebody else's business: reloading + // the page for it throws away the list the user is looking at. + const hasWildcard = resources && (resources['*'] === '*' || - Object.values(resources).find((v) => v.find((v2) => v2 === '*'))) - ) { + Object.entries(resources).some( + ([r, ids]) => isWatched(r) && ids.includes?.('*'), + )) + + if (hasWildcard) { refresh() return } - if (resources) { - Object.keys(resources).forEach((r) => { - if (visibleResources.length === 0 || visibleResources?.includes(r)) { - if (resources[r]?.length > 0) { - // Only refetch records already in the store; ones the UI never loaded will - // arrive fresh (with the new artwork) when navigated to, so fetching them is wasteful. - const loaded = loadedResources?.[r]?.data || {} - const ids = resources[r].filter((id) => loaded[id] !== undefined) - if (ids.length > 0) { - dataProvider.getMany(r, { ids }) - } + if (!resources) { + return + } + Object.keys(resources).forEach((r) => { + if (isWatched(r)) { + if (resources[r]?.length > 0) { + // Only refetch records already in the store; ones the UI never loaded will + // arrive fresh (with the new artwork) when navigated to, so fetching them is wasteful. + const loaded = loadedResources?.[r]?.data || {} + const ids = resources[r].filter((id) => loaded[id] !== undefined) + if (ids.length > 0) { + dataProvider.getMany(r, { ids }) } } + } + }) + + // A track with no art of its own is served its album's, so an album's new coverArt id moves its + // tracks' too. The dependent ids are unbounded server-side, but the store knows which are loaded. + if (resources.album?.length > 0) { + const albumIds = new Set(resources.album) + trackResources.filter(isWatched).forEach((r) => { + const ids = Object.values(loadedResources?.[r]?.data || {}) + .filter((t) => albumIds.has(t?.albumId)) + .map((t) => t.id) + if (ids.length > 0) { + dataProvider.getMany(r, { ids }) + } }) } } diff --git a/ui/src/common/useResourceRefresh.test.js b/ui/src/common/useResourceRefresh.test.js index ac4ee5c6b..ccd729836 100644 --- a/ui/src/common/useResourceRefresh.test.js +++ b/ui/src/common/useResourceRefresh.test.js @@ -169,6 +169,93 @@ describe('useResourceRefresh', () => { expect(getMany).not.toHaveBeenCalled() }) + it('does not refresh the page when the wildcard is on a resource it does not watch', () => { + // Guards other senders: a wildcard on an unwatched resource must not reload this list. + mockStore({ + refresh: { + lastReceived: lastTime, + resources: { album: ['al-1', 'al-2'], song: ['*'] }, + }, + loaded: asStore({ album: ['al-1', 'al-2'] }), + }) + + useResourceRefresh('album') + + expect(refresh).not.toHaveBeenCalled() + expect(getMany).toHaveBeenCalledWith('album', { ids: ['al-1', 'al-2'] }) + }) + + it('refetches the loaded songs of a refreshed album', () => { + // A track with no art of its own is served its album's, so its coverArt id moves with the + // album's. The backend cannot know which tracks are loaded; the store can. + mockStore({ + refresh: { lastReceived: lastTime, resources: { album: ['al-1'] } }, + loaded: { + album: { data: { 'al-1': { id: 'al-1' } } }, + song: { + data: { + 'sg-1': { id: 'sg-1', albumId: 'al-1' }, + 'sg-2': { id: 'sg-2', albumId: 'al-2' }, + }, + }, + }, + }) + + useResourceRefresh('song') + + expect(refresh).not.toHaveBeenCalled() + expect(getMany).toHaveBeenCalledWith('song', { ids: ['sg-1'] }) + }) + + it('fans an album refresh out to loaded playlist tracks, which have their own ids', () => { + mockStore({ + refresh: { lastReceived: lastTime, resources: { album: ['al-1'] } }, + loaded: { + playlistTrack: { + data: { + 'pt-1': { id: 'pt-1', albumId: 'al-1' }, + 'pt-2': { id: 'pt-2', albumId: 'al-2' }, + }, + }, + }, + }) + + useResourceRefresh('playlistTrack', 'song', 'playlist') + + expect(refresh).not.toHaveBeenCalled() + expect(getMany).toHaveBeenCalledWith('playlistTrack', { ids: ['pt-1'] }) + }) + + it('does not fan out to track resources the component does not show', () => { + mockStore({ + refresh: { lastReceived: lastTime, resources: { album: ['al-1'] } }, + loaded: { + album: { data: { 'al-1': { id: 'al-1' } } }, + song: { data: { 'sg-1': { id: 'sg-1', albumId: 'al-1' } } }, + }, + }) + + useResourceRefresh('album') + + expect(refresh).not.toHaveBeenCalled() + expect(getMany).toHaveBeenCalledTimes(1) + expect(getMany).toHaveBeenCalledWith('album', { ids: ['al-1'] }) + }) + + it('does not refetch songs when no loaded song belongs to the refreshed album', () => { + mockStore({ + refresh: { lastReceived: lastTime, resources: { album: ['al-9'] } }, + loaded: { + song: { data: { 'sg-1': { id: 'sg-1', albumId: 'al-1' } } }, + }, + }) + + useResourceRefresh('song') + + expect(refresh).not.toHaveBeenCalled() + expect(getMany).not.toHaveBeenCalled() + }) + it('refetches the received resources if they are visible and loaded', () => { mockStore({ refresh: { diff --git a/ui/src/playlist/PlaylistShow.jsx b/ui/src/playlist/PlaylistShow.jsx index 4e269be18..4c2fea315 100644 --- a/ui/src/playlist/PlaylistShow.jsx +++ b/ui/src/playlist/PlaylistShow.jsx @@ -35,7 +35,7 @@ const PlaylistShowLayout = (props) => { const { loading, ...context } = useShowContext(props) const { record } = context const classes = useStyles() - useResourceRefresh('song') + useResourceRefresh('playlistTrack', 'song') return ( <> diff --git a/ui/src/playlist/PlaylistSongs.jsx b/ui/src/playlist/PlaylistSongs.jsx index bbe38b4d5..4718a7cf4 100644 --- a/ui/src/playlist/PlaylistSongs.jsx +++ b/ui/src/playlist/PlaylistSongs.jsx @@ -100,7 +100,8 @@ const PlaylistSongs = ({ playlistId, readOnly, actions, ...props }) => { const dataProvider = useDataProvider() const notify = useNotify() const version = useVersion() - useResourceRefresh('song', 'playlist') + // The rows are stored as playlistTrack, not song: their ids are playlist entries. + useResourceRefresh('playlistTrack', 'song', 'playlist') useEffect(() => { setPage(1)