diff --git a/conf/configuration.go b/conf/configuration.go index 6f4b2d594..a75581fca 100644 --- a/conf/configuration.go +++ b/conf/configuration.go @@ -129,6 +129,7 @@ type scannerOptions struct { WatcherWait time.Duration ScanOnStartup bool Extractor string + ArtistJoiner string GenreSeparators string // Deprecated: Use Tags.genre.Split instead GroupAlbumReleases bool // Deprecated: Use PID.Album instead } @@ -495,6 +496,7 @@ func init() { viper.SetDefault("scanner.extractor", consts.DefaultScannerExtractor) viper.SetDefault("scanner.watcherwait", consts.DefaultWatcherWait) viper.SetDefault("scanner.scanonstartup", true) + viper.SetDefault("scanner.artistjoiner", consts.ArtistJoiner) viper.SetDefault("scanner.genreseparators", "") viper.SetDefault("scanner.groupalbumreleases", false) diff --git a/model/metadata/map_participants.go b/model/metadata/map_participants.go index a871f64fa..e8be6aaab 100644 --- a/model/metadata/map_participants.go +++ b/model/metadata/map_participants.go @@ -4,6 +4,7 @@ import ( "cmp" "strings" + "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/str" @@ -210,8 +211,8 @@ func (md Metadata) getArtistValues(single, multi model.TagName) []string { func (md Metadata) mapDisplayName(singularTagName, pluralTagName model.TagName) string { return cmp.Or( - strings.Join(md.tags[singularTagName], consts.ArtistJoiner), - strings.Join(md.tags[pluralTagName], consts.ArtistJoiner), + strings.Join(md.tags[singularTagName], conf.Server.Scanner.ArtistJoiner), + strings.Join(md.tags[pluralTagName], conf.Server.Scanner.ArtistJoiner), ) } diff --git a/ui/src/common/ArtistLinkField.jsx b/ui/src/common/ArtistLinkField.jsx index 053cd25aa..60832eb40 100644 --- a/ui/src/common/ArtistLinkField.jsx +++ b/ui/src/common/ArtistLinkField.jsx @@ -63,38 +63,70 @@ const parseAndReplaceArtists = ( export const ArtistLinkField = ({ record, className, limit, source }) => { const role = source.toLowerCase() - const artists = record['participants'] - ? record['participants'][role] - : [{ name: record[source], id: record[source + 'Id'] }] - // When showing artists for a track, add any remixers to the list of artists - if ( - role === 'artist' && - record['participants'] && - record['participants']['remixer'] - ) { - record['participants']['remixer'].forEach((remixer) => { - artists.push(remixer) - }) - } + // Get artists array with fallback + let artists = record?.participants?.[role] || [] + const remixers = + role === 'artist' && record?.participants?.remixer + ? record.participants.remixer.slice(0, 2) + : [] - if (role === 'albumartist') { + // Use parseAndReplaceArtists for artist and albumartist roles + if ((role === 'artist' || role === 'albumartist') && record[source]) { const artistsLinks = parseAndReplaceArtists( record[source], artists, className, ) + if (artistsLinks.length > 0) { + // For artist role, append remixers if available, avoiding duplicates + if (role === 'artist' && remixers.length > 0) { + // Track which artists are already displayed to avoid duplicates + const displayedArtistIds = new Set( + artists.map((artist) => artist.id).filter(Boolean), + ) + + // Only add remixers that aren't already in the artists list + const uniqueRemixers = remixers.filter( + (remixer) => remixer.id && !displayedArtistIds.has(remixer.id), + ) + + if (uniqueRemixers.length > 0) { + artistsLinks.push(' • ') + uniqueRemixers.forEach((remixer, index) => { + if (index > 0) artistsLinks.push(' • ') + artistsLinks.push( + , + ) + }) + } + } + return
{artistsLinks}
} } - // Dedupe artists, only shows the first 3 + // Fall back to regular handling + if (artists.length === 0 && record[source]) { + artists = [{ name: record[source], id: record[source + 'Id'] }] + } + + // For artist role, combine artists and remixers before deduplication + const allArtists = role === 'artist' ? [...artists, ...remixers] : artists + + // Dedupe artists and collect subroles const seen = new Map() const dedupedArtists = [] let limitedShow = false - for (const artist of artists ?? []) { + for (const artist of allArtists) { + if (!artist?.id) continue + if (!seen.has(artist.id)) { if (dedupedArtists.length < limit) { seen.set(artist.id, dedupedArtists.length) @@ -107,22 +139,20 @@ export const ArtistLinkField = ({ record, className, limit, source }) => { } } else { const position = seen.get(artist.id) - - if (position !== -1) { - const existing = dedupedArtists[position] - if (artist.subRole && !existing.subroles.includes(artist.subRole)) { - existing.subroles.push(artist.subRole) - } + const existing = dedupedArtists[position] + if (artist.subRole && !existing.subroles.includes(artist.subRole)) { + existing.subroles.push(artist.subRole) } } } + // Create artist links const artistsList = dedupedArtists.map((artist) => ( - + )) if (limitedShow) { - artistsList.push(...) + artistsList.push(...) } return <>{intersperse(artistsList, ' • ')} diff --git a/ui/src/common/ArtistLinkField.test.jsx b/ui/src/common/ArtistLinkField.test.jsx new file mode 100644 index 000000000..09fdf64a4 --- /dev/null +++ b/ui/src/common/ArtistLinkField.test.jsx @@ -0,0 +1,238 @@ +import React from 'react' +import { render, screen } from '@testing-library/react' +import { describe, it, expect, beforeEach, vi } from 'vitest' +import { ArtistLinkField } from './ArtistLinkField' +import { intersperse } from '../utils/index.js' + +// Mock dependencies +vi.mock('react-redux', () => ({ + useDispatch: vi.fn(() => vi.fn()), +})) + +vi.mock('./useGetHandleArtistClick', () => ({ + useGetHandleArtistClick: vi.fn(() => (id) => `/artist/${id}`), +})) + +vi.mock('../utils/index.js', () => ({ + intersperse: vi.fn((arr) => arr), +})) + +vi.mock('@material-ui/core', () => ({ + withWidth: () => (Component) => { + const WithWidthComponent = (props) => + WithWidthComponent.displayName = `WithWidth(${Component.displayName || Component.name || 'Component'})` + return WithWidthComponent + }, +})) + +vi.mock('react-admin', () => ({ + Link: ({ children, to, ...props }) => ( + + {children} + + ), +})) + +describe('ArtistLinkField', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + describe('when rendering artists', () => { + it('renders artists from participants when available', () => { + const record = { + participants: { + artist: [ + { id: '1', name: 'Artist 1' }, + { id: '2', name: 'Artist 2' }, + ], + }, + } + + render() + + expect(screen.getByText('Artist 1')).toBeInTheDocument() + expect(screen.getByText('Artist 2')).toBeInTheDocument() + }) + + it('falls back to record[source] when participants not available', () => { + const record = { + artist: 'Fallback Artist', + artistId: '123', + } + + render() + + expect(screen.getByText('Fallback Artist')).toBeInTheDocument() + }) + + it('handles empty artists array', () => { + const record = { + participants: { + artist: [], + }, + } + + render() + + expect(intersperse).toHaveBeenCalledWith([], ' • ') + }) + }) + + describe('when handling remixers', () => { + it('adds remixers when showing artist role', () => { + const record = { + participants: { + artist: [{ id: '1', name: 'Artist 1' }], + remixer: [{ id: '2', name: 'Remixer 1' }], + }, + } + + render() + + expect(screen.getByText('Artist 1')).toBeInTheDocument() + expect(screen.getByText('Remixer 1')).toBeInTheDocument() + }) + + it('limits remixers to maximum of 2', () => { + const record = { + participants: { + artist: [{ id: '1', name: 'Artist 1' }], + remixer: [ + { id: '2', name: 'Remixer 1' }, + { id: '3', name: 'Remixer 2' }, + { id: '4', name: 'Remixer 3' }, + ], + }, + } + + render() + + expect(screen.getByText('Artist 1')).toBeInTheDocument() + expect(screen.getByText('Remixer 1')).toBeInTheDocument() + expect(screen.getByText('Remixer 2')).toBeInTheDocument() + expect(screen.queryByText('Remixer 3')).not.toBeInTheDocument() + }) + + it('deduplicates artists and remixers', () => { + const record = { + participants: { + artist: [{ id: '1', name: 'Duplicate Person' }], + remixer: [{ id: '1', name: 'Duplicate Person' }], + }, + } + + render() + + const links = screen.getAllByRole('link') + expect(links).toHaveLength(1) + expect(links[0]).toHaveTextContent('Duplicate Person') + }) + }) + + describe('when using parseAndReplaceArtists', () => { + it('uses parseAndReplaceArtists when role is albumartist', () => { + const record = { + albumArtist: 'Group Artist', + participants: { + albumartist: [{ id: '1', name: 'Group Artist' }], + }, + } + + render() + + expect(screen.getByText('Group Artist')).toBeInTheDocument() + expect(screen.getByRole('link')).toHaveAttribute('href', '/artist/1') + }) + + it('uses parseAndReplaceArtists when role is artist', () => { + const record = { + artist: 'Main Artist', + participants: { + artist: [{ id: '1', name: 'Main Artist' }], + }, + } + + render() + + expect(screen.getByText('Main Artist')).toBeInTheDocument() + expect(screen.getByRole('link')).toHaveAttribute('href', '/artist/1') + }) + + it('adds remixers after parseAndReplaceArtists for artist role', () => { + const record = { + artist: 'Main Artist', + participants: { + artist: [{ id: '1', name: 'Main Artist' }], + remixer: [{ id: '2', name: 'Remixer 1' }], + }, + } + + render() + + const links = screen.getAllByRole('link') + expect(links).toHaveLength(2) + expect(links[0]).toHaveAttribute('href', '/artist/1') + expect(links[1]).toHaveAttribute('href', '/artist/2') + }) + }) + + describe('when handling artist deduplication', () => { + it('deduplicates artists with the same id', () => { + const record = { + participants: { + artist: [ + { id: '1', name: 'Duplicate Artist' }, + { id: '1', name: 'Duplicate Artist', subRole: 'Vocals' }, + ], + }, + } + + render() + + const links = screen.getAllByRole('link') + expect(links).toHaveLength(1) + expect(links[0]).toHaveTextContent('Duplicate Artist (Vocals)') + }) + + it('aggregates subroles for the same artist', () => { + const record = { + participants: { + artist: [ + { id: '1', name: 'Multi-Role Artist', subRole: 'Vocals' }, + { id: '1', name: 'Multi-Role Artist', subRole: 'Guitar' }, + ], + }, + } + + render() + + expect( + screen.getByText('Multi-Role Artist (Vocals, Guitar)'), + ).toBeInTheDocument() + }) + }) + + describe('when limiting displayed artists', () => { + it('limits the number of artists displayed', () => { + const record = { + participants: { + artist: [ + { id: '1', name: 'Artist 1' }, + { id: '2', name: 'Artist 2' }, + { id: '3', name: 'Artist 3' }, + { id: '4', name: 'Artist 4' }, + ], + }, + } + + render() + + expect(screen.getByText('Artist 1')).toBeInTheDocument() + expect(screen.getByText('Artist 2')).toBeInTheDocument() + expect(screen.getByText('Artist 3')).toBeInTheDocument() + expect(screen.queryByText('Artist 4')).not.toBeInTheDocument() + expect(screen.getByText('...')).toBeInTheDocument() + }) + }) +})