From 4172d2332ab9ae9e25ff6f865e0c0f5ece2122c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Wed, 4 Jun 2025 20:38:28 -0400 Subject: [PATCH] feat(ui): add song Love and Rating functionality to playlist view (#4134) * feat(ui): add playlist track love button Signed-off-by: Deluan * feat(ui): add star rating feature for playlist tracks Signed-off-by: Deluan * fix(ui): handle loading state and error logging in toggle love and rating components Signed-off-by: Deluan --------- Signed-off-by: Deluan --- persistence/playlist_track_repository.go | 4 +- server/nativeapi/native_api.go | 3 + server/nativeapi/playlists.go | 17 +++ ui/src/common/RatingField.jsx | 7 +- ui/src/common/useRating.jsx | 48 +++++-- ui/src/common/useRating.test.js | 165 +++++++++++++++++++++++ ui/src/common/useToggleLove.jsx | 34 ++++- ui/src/common/useToggleLove.test.js | 136 +++++++++++++++++++ ui/src/playlist/PlaylistSongs.jsx | 21 ++- 9 files changed, 409 insertions(+), 26 deletions(-) create mode 100644 ui/src/common/useRating.test.js create mode 100644 ui/src/common/useToggleLove.test.js diff --git a/persistence/playlist_track_repository.go b/persistence/playlist_track_repository.go index d33bd511..80925aa8 100644 --- a/persistence/playlist_track_repository.go +++ b/persistence/playlist_track_repository.go @@ -99,10 +99,10 @@ func (r *playlistTrackRepository) Read(id string) (interface{}, error) { "playlist_tracks.*", ). Join("media_file f on f.id = media_file_id"). - Where(And{Eq{"playlist_id": r.playlistId}, Eq{"id": id}}) + Where(And{Eq{"playlist_id": r.playlistId}, Eq{"playlist_tracks.id": id}}) var trk dbPlaylistTrack err := r.queryOne(sel, &trk) - return trk.PlaylistTrack.MediaFile, err + return trk.PlaylistTrack, err } func (r *playlistTrackRepository) GetAll(options ...model.QueryOptions) (model.PlaylistTracks, error) { diff --git a/server/nativeapi/native_api.go b/server/nativeapi/native_api.go index 34c887b2..3586a86a 100644 --- a/server/nativeapi/native_api.go +++ b/server/nativeapi/native_api.go @@ -133,6 +133,9 @@ func (n *Router) addPlaylistTrackRoute(r chi.Router) { }) r.Route("/{id}", func(r chi.Router) { r.Use(server.URLParamsMiddleware) + r.Get("/", func(w http.ResponseWriter, r *http.Request) { + getPlaylistTrack(n.ds)(w, r) + }) r.Put("/", func(w http.ResponseWriter, r *http.Request) { reorderItem(n.ds)(w, r) }) diff --git a/server/nativeapi/playlists.go b/server/nativeapi/playlists.go index 9feac718..17af1947 100644 --- a/server/nativeapi/playlists.go +++ b/server/nativeapi/playlists.go @@ -45,6 +45,23 @@ func getPlaylist(ds model.DataStore) http.HandlerFunc { } } +func getPlaylistTrack(ds model.DataStore) http.HandlerFunc { + // Add a middleware to capture the playlistId + wrapper := func(handler restHandler) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + constructor := func(ctx context.Context) rest.Repository { + plsRepo := ds.Playlist(ctx) + plsId := chi.URLParam(r, "playlistId") + return plsRepo.Tracks(plsId, true) + } + + handler(constructor).ServeHTTP(w, r) + } + } + + return wrapper(rest.Get) +} + func createPlaylistFromM3U(playlists core.Playlists) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() diff --git a/ui/src/common/RatingField.jsx b/ui/src/common/RatingField.jsx index 1b440c51..b29c1eee 100644 --- a/ui/src/common/RatingField.jsx +++ b/ui/src/common/RatingField.jsx @@ -38,15 +38,16 @@ export const RatingField = ({ const handleRating = useCallback( (e, val) => { - rate(val ?? 0, e.target.name) + const targetId = record.mediaFileId || record.id + rate(val ?? 0, targetId) }, - [rate], + [rate, record.mediaFileId, record.id], ) return ( stopPropagation(e)}> { }, []) const refreshRating = useCallback(() => { - dataProvider - .getOne(resource, { id: record.id }) - .then(() => { - if (mountedRef.current) { - setLoading(false) - } - }) - .catch((e) => { - // eslint-disable-next-line no-console - console.log('Error encountered: ' + e) - }) - }, [dataProvider, record, resource]) + // 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) + .catch((e) => { + // eslint-disable-next-line no-console + console.log('Error encountered: ' + e) + }) + .finally(() => { + if (mountedRef.current) { + setLoading(false) + } + }) + } else { + // Regular song or other resource + dataProvider + .getOne(resource, { id: record.id }) + .catch((e) => { + // eslint-disable-next-line no-console + console.log('Error encountered: ' + e) + }) + .finally(() => { + if (mountedRef.current) { + setLoading(false) + } + }) + } + }, [dataProvider, record.id, record.mediaFileId, record.playlistId, resource]) const rate = (val, id) => { setLoading(true) diff --git a/ui/src/common/useRating.test.js b/ui/src/common/useRating.test.js new file mode 100644 index 00000000..b1353512 --- /dev/null +++ b/ui/src/common/useRating.test.js @@ -0,0 +1,165 @@ +import { renderHook, act } from '@testing-library/react-hooks' +import { vi, describe, it, expect, beforeEach } from 'vitest' +import { useRating } from './useRating' +import subsonic from '../subsonic' +import { useDataProvider } from 'react-admin' + +vi.mock('../subsonic', () => ({ + default: { + setRating: vi.fn(() => Promise.resolve()), + }, +})) + +vi.mock('react-admin', async () => { + const actual = await vi.importActual('react-admin') + return { + ...actual, + useDataProvider: vi.fn(), + useNotify: vi.fn(() => vi.fn()), + } +}) + +describe('useRating', () => { + let getOne + beforeEach(() => { + getOne = vi.fn(() => Promise.resolve()) + useDataProvider.mockReturnValue({ getOne }) + vi.clearAllMocks() + }) + + it('returns rating value from record', () => { + const record = { id: 'sg-1', rating: 3 } + const { result } = renderHook(() => useRating('song', record)) + const [rate, rating, loading] = result.current + expect(rating).toBe(3) + expect(loading).toBe(false) + expect(typeof rate).toBe('function') + }) + + it('sets rating using targetId and calls setRating API', async () => { + const record = { id: 'sg-1', rating: 0 } + const { result } = renderHook(() => useRating('song', record)) + await act(async () => { + await result.current[0](4, 'sg-1') + }) + expect(subsonic.setRating).toHaveBeenCalledWith('sg-1', 4) + expect(getOne).toHaveBeenCalledWith('song', { id: 'sg-1' }) + }) + + it('handles zero rating (unrate)', async () => { + const record = { id: 'sg-1', rating: 5 } + const { result } = renderHook(() => useRating('song', record)) + await act(async () => { + await result.current[0](0, 'sg-1') + }) + expect(subsonic.setRating).toHaveBeenCalledWith('sg-1', 0) + }) + + describe('playlist track scenarios', () => { + it('refreshes both playlist track and song for playlist tracks', async () => { + const record = { + id: 'pt-1', + mediaFileId: 'sg-1', + playlistId: 'pl-1', + rating: 2, + } + const { result } = renderHook(() => useRating('playlistTrack', record)) + await act(async () => { + await result.current[0](5, 'sg-1') + }) + + // 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' }, + }) + expect(getOne).toHaveBeenCalledWith('song', { id: 'sg-1' }) + }) + + it('includes playlist_id filter when refreshing playlist tracks', async () => { + const record = { + id: 'pt-5', + mediaFileId: 'sg-10', + playlistId: 'pl-123', + rating: 1, + } + const { result } = renderHook(() => useRating('playlistTrack', record)) + await act(async () => { + 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' }) + }) + + it('only refreshes original resource when no mediaFileId present', async () => { + const record = { id: 'sg-1', rating: 4 } + const { result } = renderHook(() => useRating('song', record)) + await act(async () => { + await result.current[0](2, 'sg-1') + }) + + // Should only refresh the original resource (song) + expect(getOne).toHaveBeenCalledTimes(1) + expect(getOne).toHaveBeenCalledWith('song', { id: 'sg-1' }) + }) + + it('does not include playlist_id filter for non-playlist resources', async () => { + const record = { id: 'sg-1', rating: 0 } + const { result } = renderHook(() => useRating('song', record)) + await act(async () => { + await result.current[0](5, 'sg-1') + }) + + // Should refresh without any filter + expect(getOne).toHaveBeenCalledWith('song', { id: 'sg-1' }) + }) + }) + + describe('component integration scenarios', () => { + it('handles mediaFileId fallback correctly for playlist tracks', async () => { + const record = { + id: 'pt-1', + mediaFileId: 'sg-1', + playlistId: 'pl-1', + rating: 0, + } + const { result } = renderHook(() => useRating('playlistTrack', record)) + + // Simulate RatingField component behavior: uses mediaFileId || record.id + const targetId = record.mediaFileId || record.id + await act(async () => { + await result.current[0](4, targetId) + }) + + expect(subsonic.setRating).toHaveBeenCalledWith('sg-1', 4) + }) + + it('handles regular song rating without mediaFileId', async () => { + const record = { id: 'sg-1', rating: 2 } + const { result } = renderHook(() => useRating('song', record)) + + // Simulate RatingField component behavior: uses mediaFileId || record.id + const targetId = record.mediaFileId || record.id + await act(async () => { + await result.current[0](5, targetId) + }) + + expect(subsonic.setRating).toHaveBeenCalledWith('sg-1', 5) + expect(getOne).toHaveBeenCalledTimes(1) + expect(getOne).toHaveBeenCalledWith('song', { id: 'sg-1' }) + }) + }) +}) diff --git a/ui/src/common/useToggleLove.jsx b/ui/src/common/useToggleLove.jsx index 6379d587..3f98a2e2 100644 --- a/ui/src/common/useToggleLove.jsx +++ b/ui/src/common/useToggleLove.jsx @@ -17,18 +17,38 @@ export const useToggleLove = (resource, record = {}) => { const dataProvider = useDataProvider() const refreshRecord = useCallback(() => { - dataProvider.getOne(resource, { id: record.id }).then(() => { - if (mountedRef.current) { - setLoading(false) - } - }) - }, [dataProvider, record.id, resource]) + const promises = [] + + // 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) + .catch((e) => { + // eslint-disable-next-line no-console + console.log('Error encountered: ' + e) + }) + .finally(() => { + if (mountedRef.current) { + setLoading(false) + } + }) + }, [dataProvider, record.mediaFileId, record.id, record.playlistId, resource]) const toggleLove = () => { const toggle = record.starred ? subsonic.unstar : subsonic.star + const id = record.mediaFileId || record.id setLoading(true) - toggle(record.id) + toggle(id) .then(refreshRecord) .catch((e) => { // eslint-disable-next-line no-console diff --git a/ui/src/common/useToggleLove.test.js b/ui/src/common/useToggleLove.test.js new file mode 100644 index 00000000..640e9ff8 --- /dev/null +++ b/ui/src/common/useToggleLove.test.js @@ -0,0 +1,136 @@ +import { renderHook, act } from '@testing-library/react-hooks' +import { vi, describe, it, expect, beforeEach } from 'vitest' +import { useToggleLove } from './useToggleLove' +import subsonic from '../subsonic' +import { useDataProvider } from 'react-admin' + +vi.mock('../subsonic', () => ({ + default: { + star: vi.fn(() => Promise.resolve()), + unstar: vi.fn(() => Promise.resolve()), + }, +})) + +vi.mock('react-admin', async () => { + const actual = await vi.importActual('react-admin') + return { + ...actual, + useDataProvider: vi.fn(), + useNotify: vi.fn(() => vi.fn()), + } +}) + +describe('useToggleLove', () => { + let getOne + beforeEach(() => { + getOne = vi.fn(() => Promise.resolve()) + useDataProvider.mockReturnValue({ getOne }) + vi.clearAllMocks() + }) + + it('uses mediaFileId when present', async () => { + const record = { id: 'pt-1', mediaFileId: 'sg-1', starred: false } + const { result } = renderHook(() => useToggleLove('song', record)) + await act(async () => { + await result.current[0]() + }) + expect(subsonic.star).toHaveBeenCalledWith('sg-1') + expect(getOne).toHaveBeenCalledWith('song', { id: 'sg-1' }) + }) + + it('falls back to id when mediaFileId not present', async () => { + const record = { id: 'sg-1', starred: false } + const { result } = renderHook(() => useToggleLove('song', record)) + await act(async () => { + await result.current[0]() + }) + expect(subsonic.star).toHaveBeenCalledWith('sg-1') + expect(getOne).toHaveBeenCalledWith('song', { id: 'sg-1' }) + }) + + it('calls unstar when record is already loved', async () => { + const record = { id: 'sg-1', starred: true } + const { result } = renderHook(() => useToggleLove('song', record)) + await act(async () => { + await result.current[0]() + }) + expect(subsonic.unstar).toHaveBeenCalledWith('sg-1') + }) + + describe('playlist track scenarios', () => { + it('refreshes both playlist track and song for playlist tracks', async () => { + const record = { + id: 'pt-1', + mediaFileId: 'sg-1', + playlistId: 'pl-1', + starred: false, + } + const { result } = renderHook(() => + useToggleLove('playlistTrack', record), + ) + await act(async () => { + await result.current[0]() + }) + + // 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' }, + }) + expect(getOne).toHaveBeenCalledWith('song', { id: 'sg-1' }) + }) + + it('includes playlist_id filter when refreshing playlist tracks', async () => { + const record = { + id: 'pt-5', + mediaFileId: 'sg-10', + playlistId: 'pl-123', + starred: true, + } + const { result } = renderHook(() => + useToggleLove('playlistTrack', record), + ) + await act(async () => { + 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' }) + }) + + it('only refreshes original resource when no mediaFileId present', async () => { + const record = { id: 'sg-1', starred: false } + const { result } = renderHook(() => useToggleLove('song', record)) + await act(async () => { + await result.current[0]() + }) + + // Should only refresh the original resource (song) + expect(getOne).toHaveBeenCalledTimes(1) + expect(getOne).toHaveBeenCalledWith('song', { id: 'sg-1' }) + }) + + it('does not include playlist_id filter for non-playlist resources', async () => { + const record = { id: 'sg-1', starred: false } + const { result } = renderHook(() => useToggleLove('song', record)) + await act(async () => { + await result.current[0]() + }) + + // Should refresh without any filter + expect(getOne).toHaveBeenCalledWith('song', { id: 'sg-1' }) + }) + }) +}) diff --git a/ui/src/playlist/PlaylistSongs.jsx b/ui/src/playlist/PlaylistSongs.jsx index d9cbbbfd..4292562a 100644 --- a/ui/src/playlist/PlaylistSongs.jsx +++ b/ui/src/playlist/PlaylistSongs.jsx @@ -26,11 +26,13 @@ import { useResourceRefresh, DateField, ArtistLinkField, + RatingField, } from '../common' import { AlbumLinkField } from '../song/AlbumLinkField' import { playTracks } from '../actions' import PlaylistSongBulkActions from './PlaylistSongBulkActions' import ExpandInfoDialog from '../dialogs/ExpandInfoDialog' +import config from '../config' const useStyles = makeStyles( (theme) => ({ @@ -66,11 +68,17 @@ const useStyles = makeStyles( '& $contextMenu': { visibility: 'visible', }, + '& $ratingField': { + visibility: 'visible', + }, }, }, contextMenu: { visibility: (props) => (props.isDesktop ? 'hidden' : 'visible'), }, + ratingField: { + visibility: 'hidden', + }, }), { name: 'RaList' }, ) @@ -161,8 +169,16 @@ const PlaylistSongs = ({ playlistId, readOnly, actions, ...props }) => { quality: isDesktop && , channels: isDesktop && , bpm: isDesktop && , + rating: config.enableStarRating && ( + + ), } - }, [isDesktop, classes.draggable]) + }, [isDesktop, classes.draggable, classes.ratingField]) const columns = useSelectedFields({ resource: 'playlistTrack', @@ -174,6 +190,7 @@ const PlaylistSongs = ({ playlistId, readOnly, actions, ...props }) => { 'playCount', 'playDate', 'albumArtist', + 'rating', ], }) @@ -213,7 +230,7 @@ const PlaylistSongs = ({ playlistId, readOnly, actions, ...props }) => { {columns}