feat(server): custom ArtistJoiner config (#3873)

* feat(server): custom ArtistJoiner config

Signed-off-by: Deluan <deluan@navidrome.org>

* refactor(ui): organize ArtistLinkField, add tests

Signed-off-by: Deluan <deluan@navidrome.org>

* feat(ui): use display artist

* feat(ui): use display artist

Signed-off-by: Deluan <deluan@navidrome.org>

---------

Signed-off-by: Deluan <deluan@navidrome.org>

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)
 

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[singularTagName], conf.Server.Scanner.ArtistJoiner),
-		strings.Join(md.tags[pluralTagName], consts.ArtistJoiner),
+		strings.Join(md.tags[pluralTagName], conf.Server.Scanner.ArtistJoiner),
 	)
 }
 

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
+  // Get artists array with fallback
-  if (
+  let artists = record?.participants?.[role] || []
-    role === 'artist' &&
+  const remixers =
-    record['participants'] &&
+    role === 'artist' && record?.participants?.remixer
-    record['participants']['remixer']
+      ? record.participants.remixer.slice(0, 2)
-  ) {
+      : []
-    record['participants']['remixer'].forEach((remixer) => {
-      artists.push(remixer)
-    })
-  }
 
-  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(
+              <ALink
+                artist={remixer}
+                className={className}
+                key={`remixer-${remixer.id}`}
+              />,
+            )
+          })
+        }
+      }
+
       return <div className={className}>{artistsLinks}</div>
     }
   }
 
-  // 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)
-
+      const existing = dedupedArtists[position]
-      if (position !== -1) {
+      if (artist.subRole && !existing.subroles.includes(artist.subRole)) {
-        const existing = dedupedArtists[position]
+        existing.subroles.push(artist.subRole)
-        if (artist.subRole && !existing.subroles.includes(artist.subRole)) {
-          existing.subroles.push(artist.subRole)
-        }
       }
     }
   }
 
+  // Create artist links
   const artistsList = dedupedArtists.map((artist) => (
-    <ALink artist={artist} className={className} key={artist?.id} />
+    <ALink artist={artist} className={className} key={artist.id} />
   ))
 
   if (limitedShow) {
-    artistsList.push(<span>...</span>)
+    artistsList.push(<span key="more">...</span>)
   }
 
   return <>{intersperse(artistsList, ' • ')}</>

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) => <Component {...props} width="md" />
+    WithWidthComponent.displayName = `WithWidth(${Component.displayName || Component.name || 'Component'})`
+    return WithWidthComponent
+  },
+}))
+
+vi.mock('react-admin', () => ({
+  Link: ({ children, to, ...props }) => (
+    <a href={to} {...props}>
+      {children}
+    </a>
+  ),
+}))
+
+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(<ArtistLinkField record={record} source="artist" />)
+
+      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(<ArtistLinkField record={record} source="artist" />)
+
+      expect(screen.getByText('Fallback Artist')).toBeInTheDocument()
+    })
+
+    it('handles empty artists array', () => {
+      const record = {
+        participants: {
+          artist: [],
+        },
+      }
+
+      render(<ArtistLinkField record={record} source="artist" />)
+
+      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(<ArtistLinkField record={record} source="artist" />)
+
+      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(<ArtistLinkField record={record} source="artist" />)
+
+      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(<ArtistLinkField record={record} source="artist" />)
+
+      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(<ArtistLinkField record={record} source="albumArtist" />)
+
+      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(<ArtistLinkField record={record} source="artist" />)
+
+      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(<ArtistLinkField record={record} source="artist" />)
+
+      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(<ArtistLinkField record={record} source="artist" />)
+
+      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(<ArtistLinkField record={record} source="artist" />)
+
+      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(<ArtistLinkField record={record} source="artist" limit={3} />)
+
+      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()
+    })
+  })
+})