fix(ui): stop artwork refreshes reloading the whole page

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 <deluan@navidrome.org>

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)
 }
 

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() {

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 })
+      }
     })
   }
 }

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: {

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 (
     <>

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)