From de4484da855f39cbfd5ac78886fa45d4b174b124 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B8rn=20A=2E=20Andersen?= Date: Fri, 21 Aug 2026 21:42:59 +0200 Subject: [PATCH] fix(ui): reload the playlist after rating or loving a track MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A playlistTrack id is a position in the playlist, not a stable key, so refetching a row by id after the annotation is saved can return a different song: in a smart playlist filtered on that annotation the track is gone and every later row has shifted up. The stale-keyed record then renders as a duplicate of its neighbour. Signed-off-by: Bjørn A. Andersen --- ui/src/common/useRating.jsx | 22 ++++++--------- ui/src/common/useRating.test.js | 43 +++++++++++++++-------------- ui/src/common/useToggleLove.jsx | 31 ++++++++++----------- ui/src/common/useToggleLove.test.js | 39 +++++++++++++------------- 4 files changed, 65 insertions(+), 70 deletions(-) diff --git a/ui/src/common/useRating.jsx b/ui/src/common/useRating.jsx index 2eb5d9eca..82e9bfc06 100644 --- a/ui/src/common/useRating.jsx +++ b/ui/src/common/useRating.jsx @@ -1,11 +1,12 @@ import { useState, useCallback, useEffect, useRef } from 'react' -import { useDataProvider, useNotify } from 'react-admin' +import { useDataProvider, useNotify, useRefresh } from 'react-admin' import subsonic from '../subsonic' export const useRating = (resource, record) => { const [loading, setLoading] = useState(false) const notify = useNotify() const dataProvider = useDataProvider() + const refresh = useRefresh() const mountedRef = useRef(false) const rating = record.rating @@ -17,23 +18,18 @@ export const useRating = (resource, record) => { }, []) const refreshRating = useCallback(() => { - // For playlist tracks, refresh both resources to keep data in sync if (record.mediaFileId) { - // This is a playlist track - refresh both the playlist track and the song - const promises = [ - dataProvider.getOne('song', { id: record.mediaFileId }), - dataProvider.getOne('playlistTrack', { - id: record.id, - filter: { playlist_id: record.playlistId }, - }), - ] - - Promise.all(promises) + // A playlistTrack id is a position, not a stable key: rating a song can drop it out + // of a smart playlist, and that position then holds a different track. Refetching + // the row by id would write the neighbour's data under this row, so reload the list. + dataProvider + .getOne('song', { id: record.mediaFileId }) .catch((e) => { // eslint-disable-next-line no-console console.log('Error encountered: ' + e) }) .finally(() => { + refresh() if (mountedRef.current) { setLoading(false) } @@ -52,7 +48,7 @@ export const useRating = (resource, record) => { } }) } - }, [dataProvider, record.id, record.mediaFileId, record.playlistId, resource]) + }, [dataProvider, record.id, record.mediaFileId, refresh, resource]) const rate = (val, id) => { setLoading(true) diff --git a/ui/src/common/useRating.test.js b/ui/src/common/useRating.test.js index b1353512e..ffe9abffd 100644 --- a/ui/src/common/useRating.test.js +++ b/ui/src/common/useRating.test.js @@ -4,6 +4,8 @@ import { useRating } from './useRating' import subsonic from '../subsonic' import { useDataProvider } from 'react-admin' +const mockRefresh = vi.fn() + vi.mock('../subsonic', () => ({ default: { setRating: vi.fn(() => Promise.resolve()), @@ -16,13 +18,16 @@ vi.mock('react-admin', async () => { ...actual, useDataProvider: vi.fn(), useNotify: vi.fn(() => vi.fn()), + useRefresh: vi.fn(() => mockRefresh), } }) describe('useRating', () => { let getOne beforeEach(() => { - getOne = vi.fn(() => Promise.resolve()) + getOne = vi.fn((resource, params) => + Promise.resolve({ data: { id: params.id } }), + ) useDataProvider.mockReturnValue({ getOne }) vi.clearAllMocks() }) @@ -56,9 +61,9 @@ describe('useRating', () => { }) describe('playlist track scenarios', () => { - it('refreshes both playlist track and song for playlist tracks', async () => { + it('refreshes the song and reloads the list for playlist tracks', async () => { const record = { - id: 'pt-1', + id: '1', mediaFileId: 'sg-1', playlistId: 'pl-1', rating: 2, @@ -71,18 +76,21 @@ describe('useRating', () => { // Should rate using the media file ID expect(subsonic.setRating).toHaveBeenCalledWith('sg-1', 5) - // Should refresh both the playlist track and the song - expect(getOne).toHaveBeenCalledTimes(2) - expect(getOne).toHaveBeenCalledWith('playlistTrack', { - id: 'pt-1', - filter: { playlist_id: 'pl-1' }, - }) + // The row is a position in the playlist, so it cannot be refetched by id: + // rating can drop the track out of a smart playlist and shift every row up + expect(getOne).toHaveBeenCalledTimes(1) expect(getOne).toHaveBeenCalledWith('song', { id: 'sg-1' }) + expect(getOne).not.toHaveBeenCalledWith( + 'playlistTrack', + expect.anything(), + ) + expect(mockRefresh).toHaveBeenCalled() }) - it('includes playlist_id filter when refreshing playlist tracks', async () => { + it('reloads the list even when the song refresh fails', async () => { + getOne.mockImplementation(() => Promise.reject(new Error('boom'))) const record = { - id: 'pt-5', + id: '5', mediaFileId: 'sg-10', playlistId: 'pl-123', rating: 1, @@ -92,16 +100,8 @@ describe('useRating', () => { await result.current[0](3, 'sg-10') }) - // Should rate using the media file ID expect(subsonic.setRating).toHaveBeenCalledWith('sg-10', 3) - - // Should refresh playlist track with correct playlist_id filter - expect(getOne).toHaveBeenCalledWith('playlistTrack', { - id: 'pt-5', - filter: { playlist_id: 'pl-123' }, - }) - // Should also refresh the underlying song - expect(getOne).toHaveBeenCalledWith('song', { id: 'sg-10' }) + expect(mockRefresh).toHaveBeenCalled() }) it('only refreshes original resource when no mediaFileId present', async () => { @@ -111,9 +111,10 @@ describe('useRating', () => { await result.current[0](2, 'sg-1') }) - // Should only refresh the original resource (song) + // Should only refresh the original resource (song), without reloading the list expect(getOne).toHaveBeenCalledTimes(1) expect(getOne).toHaveBeenCalledWith('song', { id: 'sg-1' }) + expect(mockRefresh).not.toHaveBeenCalled() }) it('does not include playlist_id filter for non-playlist resources', async () => { diff --git a/ui/src/common/useToggleLove.jsx b/ui/src/common/useToggleLove.jsx index 3f98a2e21..22468cbe1 100644 --- a/ui/src/common/useToggleLove.jsx +++ b/ui/src/common/useToggleLove.jsx @@ -1,5 +1,5 @@ import { useCallback, useEffect, useRef, useState } from 'react' -import { useDataProvider, useNotify } from 'react-admin' +import { useDataProvider, useNotify, useRefresh } from 'react-admin' import subsonic from '../subsonic' export const useToggleLove = (resource, record = {}) => { @@ -15,33 +15,32 @@ export const useToggleLove = (resource, record = {}) => { }, []) const dataProvider = useDataProvider() + const refresh = useRefresh() const refreshRecord = useCallback(() => { - const promises = [] + // A playlistTrack id is a position, not a stable key: loving a song can drop it out of + // a smart playlist, and that position then holds a different track. Refetching the row + // by id would write the neighbour's data under this row, so reload the list instead. + const isPlaylistTrack = !!record.mediaFileId + const target = isPlaylistTrack + ? { resource: 'song', params: { id: record.mediaFileId } } + : { resource, params: { id: record.id } } - // Always refresh the original resource - const params = { id: record.id } - if (record.playlistId) { - params.filter = { playlist_id: record.playlistId } - } - promises.push(dataProvider.getOne(resource, params)) - - // If we have a mediaFileId, also refresh the song - if (record.mediaFileId) { - promises.push(dataProvider.getOne('song', { id: record.mediaFileId })) - } - - Promise.all(promises) + dataProvider + .getOne(target.resource, target.params) .catch((e) => { // eslint-disable-next-line no-console console.log('Error encountered: ' + e) }) .finally(() => { + if (isPlaylistTrack) { + refresh() + } if (mountedRef.current) { setLoading(false) } }) - }, [dataProvider, record.mediaFileId, record.id, record.playlistId, resource]) + }, [dataProvider, record.mediaFileId, record.id, refresh, resource]) const toggleLove = () => { const toggle = record.starred ? subsonic.unstar : subsonic.star diff --git a/ui/src/common/useToggleLove.test.js b/ui/src/common/useToggleLove.test.js index 640e9ff89..10db1b3c3 100644 --- a/ui/src/common/useToggleLove.test.js +++ b/ui/src/common/useToggleLove.test.js @@ -4,6 +4,8 @@ import { useToggleLove } from './useToggleLove' import subsonic from '../subsonic' import { useDataProvider } from 'react-admin' +const mockRefresh = vi.fn() + vi.mock('../subsonic', () => ({ default: { star: vi.fn(() => Promise.resolve()), @@ -17,6 +19,7 @@ vi.mock('react-admin', async () => { ...actual, useDataProvider: vi.fn(), useNotify: vi.fn(() => vi.fn()), + useRefresh: vi.fn(() => mockRefresh), } }) @@ -58,9 +61,9 @@ describe('useToggleLove', () => { }) describe('playlist track scenarios', () => { - it('refreshes both playlist track and song for playlist tracks', async () => { + it('refreshes the song and reloads the list for playlist tracks', async () => { const record = { - id: 'pt-1', + id: '1', mediaFileId: 'sg-1', playlistId: 'pl-1', starred: false, @@ -75,18 +78,21 @@ describe('useToggleLove', () => { // Should star using the media file ID expect(subsonic.star).toHaveBeenCalledWith('sg-1') - // Should refresh both the playlist track and the song - expect(getOne).toHaveBeenCalledTimes(2) - expect(getOne).toHaveBeenCalledWith('playlistTrack', { - id: 'pt-1', - filter: { playlist_id: 'pl-1' }, - }) + // The row is a position in the playlist, so it cannot be refetched by id: + // loving can drop the track out of a smart playlist and shift every row up + expect(getOne).toHaveBeenCalledTimes(1) expect(getOne).toHaveBeenCalledWith('song', { id: 'sg-1' }) + expect(getOne).not.toHaveBeenCalledWith( + 'playlistTrack', + expect.anything(), + ) + expect(mockRefresh).toHaveBeenCalled() }) - it('includes playlist_id filter when refreshing playlist tracks', async () => { + it('reloads the list even when the song refresh fails', async () => { + getOne.mockImplementation(() => Promise.reject(new Error('boom'))) const record = { - id: 'pt-5', + id: '5', mediaFileId: 'sg-10', playlistId: 'pl-123', starred: true, @@ -98,16 +104,8 @@ describe('useToggleLove', () => { await result.current[0]() }) - // Should unstar using the media file ID expect(subsonic.unstar).toHaveBeenCalledWith('sg-10') - - // Should refresh playlist track with correct playlist_id filter - expect(getOne).toHaveBeenCalledWith('playlistTrack', { - id: 'pt-5', - filter: { playlist_id: 'pl-123' }, - }) - // Should also refresh the underlying song - expect(getOne).toHaveBeenCalledWith('song', { id: 'sg-10' }) + expect(mockRefresh).toHaveBeenCalled() }) it('only refreshes original resource when no mediaFileId present', async () => { @@ -117,9 +115,10 @@ describe('useToggleLove', () => { await result.current[0]() }) - // Should only refresh the original resource (song) + // Should only refresh the original resource (song), without reloading the list expect(getOne).toHaveBeenCalledTimes(1) expect(getOne).toHaveBeenCalledWith('song', { id: 'sg-1' }) + expect(mockRefresh).not.toHaveBeenCalled() }) it('does not include playlist_id filter for non-playlist resources', async () => {