feat(frontend): render a multi-artist credit as one link per artist
Every artist name in the app went through `artistLink(name, mbid)`, so a track credited to several artists rendered one link and the rest as punctuation — "2Pac feat. Snoop Dogg" linked 2Pac and left Snoop Dogg as text inside it. `creditLink(parts, fallbackName, fallbackMbid)` renders the credit from its parts: one link per credited artist, join phrases as plain text between them. The link boundaries are known by construction, which is the point — locating a name inside the stored credit string would reintroduce the mismatch the catalog exists to avoid, since that string may come from the file's tags while the parts come from MusicBrainz and the two disagree for ~1 in 3 multi-artist credits. Fewer than two parts falls through to the previous behaviour exactly, so a single-artist credit, a file with no recording MBID and a catalog that has not answered yet all render as they did before. Nothing tries to split the fallback string: "Simon & Garfunkel" is one artist, which is why primaryArtist() does not split on "&" either. The lookup is keyed on the recording MBID, which both sides already carry — a catalog row has one and so does a local file — so one binding serves Explore and the library's own lists, and no local table is needed for this. credit-store.ts, and three things in it are load-bearing: - A miss is cached as an empty array. The backend returns nothing for a single-artist credit, which is ~87% of tracks, and caching only the hits would re-request the rest on every render forever. - request() is per-row and coalesces into one call per frame. A virtualized list cannot hand over "the whole list": 50,000 rows would be 100 queries for the ~30 on screen. - It is an LRU with a counted retainedChars probe, because a cache that grows with use is a leak with a schedule. The virtualized lists push requestUpdate() into the virtualizer rather than only the host, since its rows come from its own properties — a host update alone would leave them exactly as they were. now-playing marks its geometry dirty instead, because the marquee measures the text it is about to scroll. track-list keeps the single link while a search term is active: the highlight spans are computed against the flat credit string, and mapping them onto decomposed parts is a different problem. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AfVYUVExXsx1nSWrXN8mAh
This commit is contained in:
Claude Opus 5
@@ -52,7 +52,8 @@ import {
|
||||
} from '@utils/context-menu-controller.js';
|
||||
import type { ContextMenuHost } from '@utils/context-menu-controller.js';
|
||||
import { FavoritesController } from '@store/controllers/favorites-controller';
|
||||
import { artistLink, exploreLinkStyles } from '../../utils/explore-link';
|
||||
import { creditLink, exploreLinkStyles } from '../../utils/explore-link';
|
||||
import { creditStore } from '@store/credit-store';
|
||||
import {
|
||||
createAlbumArtDragImage,
|
||||
createDragImage,
|
||||
@@ -425,8 +426,18 @@ export class CoverGrid
|
||||
* Lifecycle
|
||||
* ==================================================================== */
|
||||
|
||||
/** Unsubscribes the credit-arrival repaint. */
|
||||
private creditsUnsub?: () => void;
|
||||
|
||||
override connectedCallback() {
|
||||
super.connectedCallback();
|
||||
|
||||
this.creditsUnsub = creditStore.subscribe(() => {
|
||||
this.requestUpdate();
|
||||
// Two virtualizers when the grid is split; both draw rows.
|
||||
this.renderRoot?.querySelectorAll('lit-virtualizer')
|
||||
.forEach((v) => (v as unknown as { requestUpdate(): void }).requestUpdate());
|
||||
});
|
||||
this.restoreSortPreferences();
|
||||
this.loadAlbums();
|
||||
|
||||
@@ -441,6 +452,8 @@ export class CoverGrid
|
||||
|
||||
override disconnectedCallback() {
|
||||
super.disconnectedCallback();
|
||||
this.creditsUnsub?.();
|
||||
this.creditsUnsub = undefined;
|
||||
|
||||
this.removeEventListener(
|
||||
'error',
|
||||
@@ -1820,7 +1833,7 @@ export class CoverGrid
|
||||
class="artist-name"
|
||||
title="${album.ArtistName}"
|
||||
>
|
||||
${artistLink(album.ArtistName, album.ArtistMBID ?? '')}
|
||||
${creditLink(creditStore.credits(album.MBID), album.ArtistName, album.ArtistMBID ?? '')}
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
@@ -20,7 +20,8 @@ type MBRelease = explore.MBRelease;
|
||||
type MBTrack = explore.MBTrack;
|
||||
import { exploreCache } from '../../store/explore-cache';
|
||||
import { libraryStore } from '../../store/library-store';
|
||||
import { artistLink, exploreLinkStyles } from '../../utils/explore-link';
|
||||
import { creditLink, exploreLinkStyles } from '../../utils/explore-link';
|
||||
import { creditStore } from '@store/credit-store';
|
||||
import { describeError } from '../../utils/describe-error';
|
||||
import { EventsOn } from '@runtime/runtime';
|
||||
import { Events } from '../../events';
|
||||
@@ -704,8 +705,15 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost {
|
||||
* BrowseReleases fetch never signals readiness. */
|
||||
private releasesFallbackTimer?: number;
|
||||
|
||||
/** Unsubscribes the credit-arrival repaint. */
|
||||
private creditsUnsub?: () => void;
|
||||
|
||||
override connectedCallback() {
|
||||
super.connectedCallback();
|
||||
|
||||
this.creditsUnsub = creditStore.subscribe(() => {
|
||||
this.requestUpdate();
|
||||
});
|
||||
if (this.releaseGroupMBID || this.localAlbumId) {
|
||||
void this.loadAllData();
|
||||
}
|
||||
@@ -760,6 +768,8 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost {
|
||||
|
||||
override disconnectedCallback() {
|
||||
super.disconnectedCallback();
|
||||
this.creditsUnsub?.();
|
||||
this.creditsUnsub = undefined;
|
||||
this.downloadUnsub?.();
|
||||
this.downloadUnsub = null;
|
||||
this.unsubReleasesReady?.();
|
||||
@@ -2850,7 +2860,11 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost {
|
||||
return html`
|
||||
${artist
|
||||
? html`<div class="album-artist">
|
||||
${artistLink(artist, artistMbid)}
|
||||
${creditLink(
|
||||
creditStore.credits(this.releaseGroupMBID),
|
||||
artist,
|
||||
artistMbid,
|
||||
)}
|
||||
</div>`
|
||||
: nothing}
|
||||
${metaParts.length > 0
|
||||
|
||||
@@ -16,7 +16,8 @@ import { exploreCache, ARTIST_IMAGE_CACHE_LIMIT } from '../../store/explore-cach
|
||||
import { queueStore } from '../../store/queue-store';
|
||||
import { notificationStore } from '../../store/notification-store';
|
||||
import '../notifications/inline-notice';
|
||||
import { artistLink, trackLink, exploreLinkStyles } from '../../utils/explore-link';
|
||||
import { creditLink, trackLink, exploreLinkStyles } from '../../utils/explore-link';
|
||||
import { creditStore } from '@store/credit-store';
|
||||
import { describeError } from '../../utils/describe-error';
|
||||
import '@awesome.me/webawesome/dist/components/icon/icon.js';
|
||||
import '../library-status-indicator/library-status-indicator.js';
|
||||
@@ -774,6 +775,14 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
|
||||
}
|
||||
|
||||
protected override onViewActivate(): void {
|
||||
// A cached primary view, so this is torn down on the way out
|
||||
// rather than on disconnect — which never fires here.
|
||||
this.whileActive(
|
||||
creditStore.subscribe(() => {
|
||||
this.requestUpdate();
|
||||
}),
|
||||
);
|
||||
|
||||
// Fetched on arrival rather than on connect: this is a cached
|
||||
// primary view, created and warmed at startup, so a fetch there
|
||||
// is three catalog queries every user pays for whether or not
|
||||
@@ -2193,7 +2202,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
|
||||
<div class="album-title" title="${rg.title}">
|
||||
${rg.title}
|
||||
</div>
|
||||
<div class="album-artist">${artistLink(rg.artistCredit, rg.artistMbid ?? '')}</div>
|
||||
<div class="album-artist">${creditLink(creditStore.credits(rg.mbid), rg.artistCredit, rg.artistMbid ?? '')}</div>
|
||||
<div class="album-meta">
|
||||
<div class="album-meta-text">
|
||||
${rg.primaryType
|
||||
@@ -2255,7 +2264,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
|
||||
${trackLink(r.title, r.releaseName ?? '', r.releaseGroupMbid ?? '', r.mbid)}
|
||||
</div>
|
||||
<div class="track-artist">
|
||||
${artistLink(r.artistCredit, r.artistMbid ?? '')}
|
||||
${creditLink(creditStore.credits(r.mbid), r.artistCredit, r.artistMbid ?? '')}
|
||||
</div>
|
||||
</div>
|
||||
<div class="track-meta">
|
||||
|
||||
@@ -4,7 +4,7 @@ import '@awesome.me/webawesome/dist/components/icon/icon.js';
|
||||
import '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
import {
|
||||
artistLink,
|
||||
creditLink,
|
||||
trackLink,
|
||||
exploreLinkStyles,
|
||||
} from '@utils/explore-link';
|
||||
@@ -14,6 +14,7 @@ import {
|
||||
navigateToQueueSource,
|
||||
} from '@utils/queue-source-link';
|
||||
import { PlayerController } from '@store/controllers/player-controller';
|
||||
import { creditStore } from '@store/credit-store';
|
||||
import { QueueController } from '@store/controllers/queue-controller';
|
||||
import { FavoritesController } from '@store/controllers/favorites-controller';
|
||||
import { designTokens } from '../../styles/tokens.css';
|
||||
@@ -290,6 +291,9 @@ export class NowPlaying extends LitElement {
|
||||
}
|
||||
`];
|
||||
|
||||
/** Unsubscribes the credit-arrival repaint. */
|
||||
private creditsUnsub?: () => void;
|
||||
|
||||
override connectedCallback() {
|
||||
super.connectedCallback();
|
||||
this.loadScrollMode();
|
||||
@@ -306,10 +310,21 @@ export class NowPlaying extends LitElement {
|
||||
this.geometryDirty = true;
|
||||
this.requestUpdate();
|
||||
});
|
||||
|
||||
// A credit arriving changes the rendered text, and the marquee
|
||||
// measures that text — so this is a geometry change, not just a
|
||||
// repaint. Saying so is what stops the bar scrolling to the
|
||||
// old width.
|
||||
this.creditsUnsub = creditStore.subscribe(() => {
|
||||
this.geometryDirty = true;
|
||||
this.requestUpdate();
|
||||
});
|
||||
}
|
||||
|
||||
override disconnectedCallback() {
|
||||
super.disconnectedCallback();
|
||||
this.creditsUnsub?.();
|
||||
this.creditsUnsub = undefined;
|
||||
// A drag interrupted by the bar going away still has to clean up.
|
||||
this.attachDragListeners(false);
|
||||
window.removeEventListener(SCROLL_CHANGE_EVENT, this.handleScrollModeEvent);
|
||||
@@ -441,7 +456,7 @@ export class NowPlaying extends LitElement {
|
||||
@mouseleave=${this.handleArtistMouseLeave}
|
||||
@transitionend=${() => this.onScrollCycleEnd('artist')}
|
||||
>
|
||||
<span class="scroll-content">${artistLink(track.artist, track.artistMbid) || 'Unknown Artist'}</span>
|
||||
<span class="scroll-content">${creditLink(creditStore.credits(track.recordingMbid), track.artist, track.artistMbid) || 'Unknown Artist'}</span>
|
||||
</span>
|
||||
${describeQueueSource(this.queue.source)
|
||||
? html`
|
||||
|
||||
@@ -24,6 +24,7 @@ import type * as playlist from '@go/playlist/models.js';
|
||||
import { EventsOn } from '@runtime/runtime';
|
||||
import { Events } from '../../events';
|
||||
import { queueStore } from '@store/queue-store';
|
||||
import { creditStore } from '@store/credit-store';
|
||||
import { PlayerController } from '@store/controllers/player-controller';
|
||||
import { SearchController } from '@store/controllers/search-controller';
|
||||
import { SelectionController } from '@utils/selection-controller';
|
||||
@@ -61,7 +62,8 @@ import '@components/duplicate-tracks-dialog/duplicate-tracks-dialog.js';
|
||||
import type { DuplicateTracksDialog } from '@components/duplicate-tracks-dialog/duplicate-tracks-dialog.js';
|
||||
import { formatMilliseconds } from '@utils/time';
|
||||
import {
|
||||
artistLink,
|
||||
creditLink,
|
||||
creditText,
|
||||
albumLink,
|
||||
trackLink,
|
||||
exploreLinkStyles,
|
||||
@@ -106,6 +108,9 @@ export class PlaylistDetails
|
||||
* rather than guessed. Without the hint the flow layout's 100 px
|
||||
* default drives constant scroll-error correction, which reads as
|
||||
* the list jumping under the pointer. */
|
||||
/** Unsubscribes the credit-arrival repaint. */
|
||||
private creditsUnsub?: () => void;
|
||||
|
||||
@query('lit-virtualizer')
|
||||
private virtualizer?: LitVirtualizer;
|
||||
|
||||
@@ -224,6 +229,14 @@ export class PlaylistDetails
|
||||
|
||||
override connectedCallback() {
|
||||
super.connectedCallback();
|
||||
|
||||
// Credits arrive after the rows that asked for them, and a
|
||||
// virtualizer repaints from its *own* properties — a host
|
||||
// update alone leaves the rows exactly as they were.
|
||||
this.creditsUnsub = creditStore.subscribe(() => {
|
||||
this.requestUpdate();
|
||||
this.virtualizer?.requestUpdate();
|
||||
});
|
||||
this.loadTracks();
|
||||
|
||||
this.tracksChangedCleanup = EventsOn(
|
||||
@@ -248,6 +261,8 @@ export class PlaylistDetails
|
||||
|
||||
override disconnectedCallback() {
|
||||
super.disconnectedCallback();
|
||||
this.creditsUnsub?.();
|
||||
this.creditsUnsub = undefined;
|
||||
|
||||
if (this.tracksChangedCleanup) {
|
||||
this.tracksChangedCleanup();
|
||||
@@ -1575,7 +1590,7 @@ export class PlaylistDetails
|
||||
: nothing}
|
||||
</div>
|
||||
<span class="cell col-title" title="${track.Title || track.FilePath}">${trackLink(track.Title, track.Album, track.ReleaseGroupMBID, track.RecordingMBID, undefined, track.Artist) || track.FilePath}</span>
|
||||
<span class="cell col-artist" title="${track.Artist}">${artistLink(track.Artist, track.ArtistMBID)}</span>
|
||||
<span class="cell col-artist" title="${creditText(creditStore.credits(track.RecordingMBID), track.Artist)}">${creditLink(creditStore.credits(track.RecordingMBID), track.Artist, track.ArtistMBID)}</span>
|
||||
<span class="cell col-album" title="${track.Album}">${albumLink(track.Album, track.ReleaseGroupMBID, undefined, track.Artist)}</span>
|
||||
<span class="cell col-duration">${formatMilliseconds(track.Duration)}</span>`}
|
||||
</div>
|
||||
|
||||
@@ -12,6 +12,7 @@ import '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js';
|
||||
import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js';
|
||||
import { QueueController } from '@store/controllers/queue-controller';
|
||||
import { creditStore } from '@store/credit-store';
|
||||
import {
|
||||
describeQueueSource,
|
||||
isQueueSourceNavigable,
|
||||
@@ -55,7 +56,7 @@ import { tracksByFilePath } from '@utils/track-index.js';
|
||||
import type { TrackDetails } from '@components/track-details/track-details.js';
|
||||
import type { CoverArtUrls } from '@components/track-details/track-details.js';
|
||||
import {
|
||||
artistLink,
|
||||
creditLink,
|
||||
trackLink,
|
||||
exploreLinkStyles,
|
||||
} from '@utils/explore-link';
|
||||
@@ -108,6 +109,9 @@ export class QueuePanel
|
||||
@query('#playlist-submenu')
|
||||
private playlistSubmenuPopup!: WaPopup;
|
||||
|
||||
/** Unsubscribes the credit-arrival repaint. */
|
||||
private creditsUnsub?: () => void;
|
||||
|
||||
@query('lit-virtualizer')
|
||||
private virtualizer!: LitVirtualizer;
|
||||
|
||||
@@ -635,6 +639,14 @@ export class QueuePanel
|
||||
|
||||
override connectedCallback() {
|
||||
super.connectedCallback();
|
||||
|
||||
// Credits arrive after the rows that asked for them, and a
|
||||
// virtualizer repaints from its *own* properties — a host
|
||||
// update alone leaves the rows exactly as they were.
|
||||
this.creditsUnsub = creditStore.subscribe(() => {
|
||||
this.requestUpdate();
|
||||
this.virtualizer?.requestUpdate();
|
||||
});
|
||||
this.style.setProperty(
|
||||
'--queue-width',
|
||||
`${this.panelWidth}px`,
|
||||
@@ -671,6 +683,8 @@ export class QueuePanel
|
||||
|
||||
override disconnectedCallback() {
|
||||
super.disconnectedCallback();
|
||||
this.creditsUnsub?.();
|
||||
this.creditsUnsub = undefined;
|
||||
document.removeEventListener(
|
||||
'mousemove',
|
||||
this.handleMouseMove,
|
||||
@@ -1675,7 +1689,7 @@ export class QueuePanel
|
||||
${trackLink(title, track.album, track.releaseGroupMbid, track.recordingMbid, undefined, track.artist)}
|
||||
</span>
|
||||
<span class="track-artist" title=${artist}>
|
||||
${artistLink(track.artist, track.artistMbid) || 'Unknown Artist'}
|
||||
${creditLink(creditStore.credits(track.recordingMbid), track.artist, track.artistMbid) || 'Unknown Artist'}
|
||||
</span>
|
||||
</div>
|
||||
<button
|
||||
|
||||
@@ -15,6 +15,7 @@ import {
|
||||
import { EventsOn } from '@runtime/runtime';
|
||||
import { Events } from '../../events';
|
||||
import { queueStore } from '@store/queue-store';
|
||||
import { creditStore } from '@store/credit-store';
|
||||
import { PlayerController } from '@store/controllers/player-controller';
|
||||
import { SearchController } from '@store/controllers/search-controller';
|
||||
import { SelectionController } from '@utils/selection-controller';
|
||||
@@ -51,7 +52,8 @@ import type { CoverArtUrls } from '@components/track-details/track-details.js';
|
||||
import { libraryStore } from '@store/library-store';
|
||||
import { formatMilliseconds } from '@utils/time';
|
||||
import {
|
||||
artistLink,
|
||||
creditLink,
|
||||
creditText,
|
||||
albumLink,
|
||||
trackLink,
|
||||
exploreLinkStyles,
|
||||
@@ -136,6 +138,9 @@ export class SmartPlaylistDetails
|
||||
* rather than guessed: without the hint the flow layout's 100 px
|
||||
* default drives constant scroll-error correction, which reads as
|
||||
* the list jumping under the pointer. */
|
||||
/** Unsubscribes the credit-arrival repaint. */
|
||||
private creditsUnsub?: () => void;
|
||||
|
||||
@query('lit-virtualizer')
|
||||
private virtualizer?: LitVirtualizer;
|
||||
|
||||
@@ -609,6 +614,14 @@ export class SmartPlaylistDetails
|
||||
override connectedCallback() {
|
||||
super.connectedCallback();
|
||||
|
||||
// Credits arrive after the rows that asked for them, and a
|
||||
// virtualizer repaints from its *own* properties — a host
|
||||
// update alone leaves the rows exactly as they were.
|
||||
this.creditsUnsub = creditStore.subscribe(() => {
|
||||
this.requestUpdate();
|
||||
this.virtualizer?.requestUpdate();
|
||||
});
|
||||
|
||||
if (this.autoEdit) {
|
||||
// Skip evaluation for new playlists — go straight to editor.
|
||||
this.autoEdit = false;
|
||||
@@ -649,6 +662,8 @@ export class SmartPlaylistDetails
|
||||
|
||||
override disconnectedCallback() {
|
||||
super.disconnectedCallback();
|
||||
this.creditsUnsub?.();
|
||||
this.creditsUnsub = undefined;
|
||||
|
||||
if (this.playlistDeletedCleanup) {
|
||||
this.playlistDeletedCleanup();
|
||||
@@ -1423,7 +1438,7 @@ export class SmartPlaylistDetails
|
||||
: nothing}
|
||||
</div>
|
||||
<span class="cell col-title" title="${track.Title || track.FilePath}">${trackLink(track.Title, track.Album, track.ReleaseGroupMBID, track.RecordingMBID, undefined, track.Artist) || track.FilePath}</span>
|
||||
<span class="cell col-artist" title="${track.Artist}">${artistLink(track.Artist, track.ArtistMBID)}</span>
|
||||
<span class="cell col-artist" title="${creditText(creditStore.credits(track.RecordingMBID), track.Artist)}">${creditLink(creditStore.credits(track.RecordingMBID), track.Artist, track.ArtistMBID)}</span>
|
||||
<span class="cell col-album" title="${track.Album}">${albumLink(track.Album, track.ReleaseGroupMBID, undefined, track.Artist)}</span>
|
||||
<span class="cell col-duration">${formatMilliseconds(track.Duration)}</span>`}
|
||||
</div>
|
||||
|
||||
@@ -9,7 +9,8 @@ import {
|
||||
} from '@go/explore/service.js';
|
||||
import '../library-status-indicator/library-status-indicator.js';
|
||||
import type { LibraryStatus } from '../library-status-indicator/library-status-indicator.js';
|
||||
import { artistLink, exploreLinkStyles } from '../../utils/explore-link';
|
||||
import { creditLink, exploreLinkStyles } from '../../utils/explore-link';
|
||||
import { creditStore } from '@store/credit-store';
|
||||
import { libraryStatusFor } from '../../utils/library-status';
|
||||
import { downloadStore } from '../../store/download-store';
|
||||
|
||||
@@ -61,14 +62,23 @@ export class TopResultsRow extends LitElement {
|
||||
* the property and never updates this element. One subscription for
|
||||
* the row, not one per card.
|
||||
*/
|
||||
/** Unsubscribes the credit-arrival repaint. */
|
||||
private creditsUnsub?: () => void;
|
||||
|
||||
override connectedCallback(): void {
|
||||
super.connectedCallback();
|
||||
|
||||
this.creditsUnsub = creditStore.subscribe(() => {
|
||||
this.requestUpdate();
|
||||
});
|
||||
this.unsubRequests = downloadStore.subscribe(() =>
|
||||
this.requestUpdate(),
|
||||
);
|
||||
}
|
||||
|
||||
override disconnectedCallback(): void {
|
||||
this.creditsUnsub?.();
|
||||
this.creditsUnsub = undefined;
|
||||
this.unsubRequests?.();
|
||||
this.unsubRequests = undefined;
|
||||
super.disconnectedCallback();
|
||||
@@ -327,7 +337,7 @@ export class TopResultsRow extends LitElement {
|
||||
${artistPart || metaPart
|
||||
? html`<span class="card-subtitle"
|
||||
>${artistPart
|
||||
? artistLink(artistPart, r.artistMbid ?? '')
|
||||
? creditLink(creditStore.credits(r.mbid), artistPart, r.artistMbid ?? '')
|
||||
: nothing}${artistPart && metaPart
|
||||
? ' · '
|
||||
: ''}${metaPart}</span
|
||||
|
||||
@@ -25,6 +25,7 @@ import type { SortOption } from '@components/page-header/page-header';
|
||||
import { TrackListController } from '@store/controllers/tracklist-controller';
|
||||
import { FavoritesController } from '@store/controllers/favorites-controller';
|
||||
import { queueStore } from '@store/queue-store';
|
||||
import { creditStore } from '@store/credit-store';
|
||||
import type { QueueSource } from '@store/queue-store';
|
||||
import { LibraryController } from '@store/controllers/library-controller';
|
||||
import {
|
||||
@@ -39,6 +40,7 @@ import {
|
||||
} from './search-ranking';
|
||||
import {
|
||||
artistLink,
|
||||
creditLink,
|
||||
albumLink,
|
||||
trackLink,
|
||||
exploreLinkStyles,
|
||||
@@ -1220,6 +1222,17 @@ export class TrackList
|
||||
'shortcut:tracklist-delete',
|
||||
this.handleShortcutDelete,
|
||||
);
|
||||
|
||||
// Credits arrive after the rows that asked for them. The
|
||||
// virtualizer produces its rows from its *own* properties, so a
|
||||
// host re-render alone repaints nothing — the same reason a
|
||||
// selection change pushes requestUpdate() into it.
|
||||
this.whileActive(
|
||||
creditStore.subscribe(() => {
|
||||
this.requestUpdate();
|
||||
this.virtualizer?.requestUpdate();
|
||||
}),
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -2031,7 +2044,15 @@ export class TrackList
|
||||
if (col.id === 'trackName') {
|
||||
display = trackLink(track.TrackName, track.Album, track.ReleaseGroupMBID, track.RecordingMBID, display as any, track.ArtistName);
|
||||
} else if (col.id === 'artistName') {
|
||||
display = artistLink(track.ArtistName, track.ArtistMBID, display as any);
|
||||
// A search term highlights the *flat* credit string,
|
||||
// and mapping those spans onto decomposed parts is a
|
||||
// different problem from rendering the credit. While
|
||||
// filtering, the single link is the honest answer.
|
||||
creditStore.request(track.RecordingMBID);
|
||||
const parts = term ? undefined : creditStore.get(track.RecordingMBID);
|
||||
display = parts && parts.length > 1
|
||||
? creditLink(parts, track.ArtistName, track.ArtistMBID)
|
||||
: artistLink(track.ArtistName, track.ArtistMBID, display as any);
|
||||
} else if (col.id === 'album') {
|
||||
display = albumLink(track.Album, track.ReleaseGroupMBID, display as any, track.ArtistName);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,226 @@
|
||||
/**
|
||||
* Multi-artist credits, keyed by recording MBID.
|
||||
*
|
||||
* A credit is ordered parts and the credit *string* is derived from
|
||||
* them. This store holds the parts for entities that have more than
|
||||
* one credited artist; everything else renders the single link it
|
||||
* always did.
|
||||
*
|
||||
* Three things about it are load-bearing.
|
||||
*
|
||||
* **Absence is an answer, and it is cached as one.** The backend
|
||||
* returns nothing for a single-artist credit, which is the common case
|
||||
* by a wide margin — measured on a real library, 13% of tracks are
|
||||
* multi-artist. Caching only the hits would re-request the other 87%
|
||||
* on every render, forever, which is the same shape as the bug that
|
||||
* made `explore-album-details` ask the backend on hover. A miss is
|
||||
* stored as an empty array: *asked*, not *answered*.
|
||||
*
|
||||
* **The lookup is batched, and coalesced across callers.** Every row
|
||||
* of every tracklist asks this question, and one IPC round trip per row
|
||||
* is how a 5,000-row list becomes unusable. A virtualized list cannot
|
||||
* hand over "the whole list" either — 50,000 rows is 100 queries for
|
||||
* the ~30 on screen. So `request()` is per-row and cheap: it collects
|
||||
* into a pending set and flushes once on the next frame, which turns a
|
||||
* screenful of rows into exactly one call. `ensure()` remains for a
|
||||
* caller that genuinely has a bounded list in hand.
|
||||
*
|
||||
* **It is bounded.** A cache that grows with use is a leak with a
|
||||
* schedule; a browsing afternoon touches far more credits than a
|
||||
* screenful. The cap is entries rather than bytes because a credit is
|
||||
* a handful of short strings, unlike the art caches next door.
|
||||
*/
|
||||
|
||||
import { GetCredits } from '@go/explore/service.js';
|
||||
import type { CreditPart } from '../utils/explore-link';
|
||||
import { LRUMap } from '../utils/lru-map';
|
||||
import { compact } from '../utils/binding';
|
||||
import { registerCacheProbe } from '../utils/cache-stats';
|
||||
|
||||
/**
|
||||
* Entries retained. A credit is ~4 short strings, so this is well
|
||||
* under a megabyte — sized to comfortably exceed any single list the
|
||||
* app renders, because a cap below the visible count evicts rows that
|
||||
* are still on screen and the re-render fetches them straight back.
|
||||
*/
|
||||
export const CREDIT_CACHE_LIMIT = 20_000;
|
||||
|
||||
/** An empty parts array is the negative marker: asked, no decomposition. */
|
||||
type CachedParts = readonly CreditPart[];
|
||||
|
||||
class CreditStore {
|
||||
private cache = new LRUMap<string, CachedParts>(CREDIT_CACHE_LIMIT);
|
||||
|
||||
/** MBIDs with a request in flight, so a re-render does not refetch. */
|
||||
private inFlight = new Set<string>();
|
||||
|
||||
private listeners = new Set<() => void>();
|
||||
|
||||
/** Collected by request(), flushed as one batch on the next frame. */
|
||||
private pending = new Set<string>();
|
||||
|
||||
private flushHandle: number | null = null;
|
||||
|
||||
constructor() {
|
||||
registerCacheProbe('credits', () => ({
|
||||
entries: this.cache.size,
|
||||
chars: this.retainedChars(),
|
||||
limit: CREDIT_CACHE_LIMIT,
|
||||
}));
|
||||
}
|
||||
|
||||
/**
|
||||
* The strings actually retained, counted rather than estimated —
|
||||
* a bound that is only checkable against a guess is not checkable.
|
||||
*/
|
||||
private retainedChars(): number {
|
||||
let total = 0;
|
||||
|
||||
for (const parts of this.cache.values()) {
|
||||
for (const part of parts) {
|
||||
total +=
|
||||
part.creditedName.length +
|
||||
part.joinPhrase.length +
|
||||
part.artistMbid.length;
|
||||
}
|
||||
}
|
||||
|
||||
return total;
|
||||
}
|
||||
|
||||
/**
|
||||
* Subscribe to "some credits arrived".
|
||||
*
|
||||
* Deliberately not per-MBID: a list fetches its rows in one call and
|
||||
* re-renders once, so a fine-grained signal would buy nothing and
|
||||
* cost a listener per row.
|
||||
*/
|
||||
subscribe(fn: () => void): () => void {
|
||||
this.listeners.add(fn);
|
||||
|
||||
return () => this.listeners.delete(fn);
|
||||
}
|
||||
|
||||
/**
|
||||
* The parts for one entity, or undefined when it has not been asked
|
||||
* about yet.
|
||||
*
|
||||
* An entity with a single-artist credit returns an empty array, and
|
||||
* `creditLink` treats fewer than two parts as the fallback — so a
|
||||
* caller does not have to distinguish "not asked" from "one artist"
|
||||
* to render correctly, only to decide whether to ask.
|
||||
*/
|
||||
get(mbid: string | undefined): readonly CreditPart[] | undefined {
|
||||
if (!mbid) return undefined;
|
||||
|
||||
return this.cache.get(mbid);
|
||||
}
|
||||
|
||||
/**
|
||||
* Ask about one entity, joining whatever batch is forming.
|
||||
*
|
||||
* Safe to call from a render: it is a set insert and a scheduled
|
||||
* flush, and an entity already cached or in flight is dropped. The
|
||||
* loop it looks like it might cause does not happen — after a flush
|
||||
* every requested MBID is cached, so the re-render's requests are
|
||||
* all dropped and nothing notifies again.
|
||||
*/
|
||||
request(mbid: string | undefined): void {
|
||||
if (!mbid) return;
|
||||
if (this.cache.has(mbid)) return;
|
||||
if (this.inFlight.has(mbid)) return;
|
||||
if (this.pending.has(mbid)) return;
|
||||
|
||||
this.pending.add(mbid);
|
||||
|
||||
if (this.flushHandle !== null) return;
|
||||
|
||||
// A frame, not a microtask: the point is to collect every row a
|
||||
// virtualizer renders in this pass, and those happen across the
|
||||
// whole update, not within one microtask checkpoint.
|
||||
this.flushHandle = requestAnimationFrame(() => {
|
||||
this.flushHandle = null;
|
||||
|
||||
const batch = [...this.pending];
|
||||
|
||||
this.pending.clear();
|
||||
|
||||
void this.ensure(batch);
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Ask and read in one call, for use inside a template.
|
||||
*
|
||||
* A getter with a side effect, deliberately: the alternative is
|
||||
* every call site writing `request(x)` beside `get(x)` and one of
|
||||
* them eventually forgetting, which renders a permanently
|
||||
* single-artist credit that looks exactly like an entity with one
|
||||
* artist. Making the request the same act as the read is what
|
||||
* stops the two drifting apart.
|
||||
*/
|
||||
credits(mbid: string | undefined): readonly CreditPart[] | undefined {
|
||||
this.request(mbid);
|
||||
|
||||
return this.get(mbid);
|
||||
}
|
||||
|
||||
/**
|
||||
* Fetch the credits for a list, skipping anything already known or
|
||||
* already being fetched.
|
||||
*
|
||||
* `has` rather than `get` for the membership test: probing must not
|
||||
* mark an entry recently-used, or scrolling past a row would keep
|
||||
* it alive ahead of one actually being rendered.
|
||||
*/
|
||||
async ensure(mbids: readonly (string | undefined)[]): Promise<void> {
|
||||
const wanted = new Set<string>();
|
||||
|
||||
for (const mbid of mbids) {
|
||||
if (!mbid) continue;
|
||||
if (this.cache.has(mbid)) continue;
|
||||
if (this.inFlight.has(mbid)) continue;
|
||||
|
||||
wanted.add(mbid);
|
||||
}
|
||||
|
||||
if (wanted.size === 0) return;
|
||||
|
||||
const batch = [...wanted];
|
||||
|
||||
for (const mbid of batch) this.inFlight.add(mbid);
|
||||
|
||||
try {
|
||||
const found = compact(await GetCredits(batch));
|
||||
|
||||
for (const mbid of batch) {
|
||||
// Every MBID asked for gets an entry, present or not:
|
||||
// the absent ones are the answer "one artist", and not
|
||||
// recording that is what would re-ask forever.
|
||||
this.cache.set(mbid, found[mbid] ?? []);
|
||||
}
|
||||
|
||||
this.notify();
|
||||
} catch (err) {
|
||||
// A credit is an enrichment: without it every name renders
|
||||
// as the single link it did before, which is a worse answer
|
||||
// rather than a broken one. Nothing user-facing is worth
|
||||
// interrupting for, so this stays in the console.
|
||||
console.error('Failed to load artist credits', err);
|
||||
} finally {
|
||||
for (const mbid of batch) this.inFlight.delete(mbid);
|
||||
}
|
||||
}
|
||||
|
||||
/** Drop everything. The tags on disk changed, so credits may have. */
|
||||
invalidate(): void {
|
||||
this.cache = new LRUMap<string, CachedParts>(CREDIT_CACHE_LIMIT);
|
||||
this.notify();
|
||||
}
|
||||
|
||||
private notify(): void {
|
||||
for (const fn of this.listeners) fn();
|
||||
}
|
||||
}
|
||||
|
||||
export const creditStore = new CreditStore();
|
||||
@@ -294,3 +294,81 @@ async function openAlbum(
|
||||
|
||||
navigate(target, detail);
|
||||
}
|
||||
|
||||
/**
|
||||
* One credited artist within a multi-artist credit.
|
||||
*
|
||||
* Mirrors `artist_credit_part` / `file_artists`: the name **as
|
||||
* credited** (which is not the artist's own name — MusicBrainz credits
|
||||
* "Snoop Dogg" on a track by the artist called "Snoop Doggy Dogg"), the
|
||||
* MBID to navigate to, and the literal connector that follows this
|
||||
* part.
|
||||
*/
|
||||
export interface CreditPart {
|
||||
/** The name as credited. Display uses this. */
|
||||
creditedName: string;
|
||||
/** The artist's MusicBrainz ID. Navigation uses this. */
|
||||
artistMbid: string;
|
||||
/** The connector following this part: " feat. ", " & ", ", ", "". */
|
||||
joinPhrase: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* Render a credit as links, one per credited artist, with the join
|
||||
* phrases as plain text between them.
|
||||
*
|
||||
* Join phrases are **assembly instructions, not disassembly
|
||||
* instructions**. This concatenates parts; it never searches for a
|
||||
* name inside a credit string. That distinction is the whole point:
|
||||
* the stored credit text may have come from a file's tags while the
|
||||
* parts come from the catalog, and measured on a real library those
|
||||
* disagree for about one in three multi-artist credits ("Skrillex
|
||||
* feat. Swae Lee" tagged against "Skrillex & Swae Lee" upstream). A
|
||||
* search would miss, or match the wrong span. Building from parts,
|
||||
* the link boundaries are known by construction.
|
||||
*
|
||||
* Falls back to `artistLink(fallbackName, fallbackMbid)` — today's
|
||||
* behaviour exactly — when there are no parts. That is the common
|
||||
* case and not a degraded one: a single-artist credit *is* one link,
|
||||
* and a file with no recording MBID or no catalog row has nothing to
|
||||
* decompose. Do not try to split the fallback string; there is
|
||||
* genuinely no information in it to split on.
|
||||
*
|
||||
* @param parts - The credit's parts in position order, if known.
|
||||
* @param fallbackName - The credit as a single string.
|
||||
* @param fallbackMbid - The primary artist's MBID.
|
||||
*/
|
||||
export function creditLink(
|
||||
parts: readonly CreditPart[] | undefined,
|
||||
fallbackName: string,
|
||||
fallbackMbid: string,
|
||||
): TemplateResult | string {
|
||||
// One part is one link, so it is the fallback rather than a special
|
||||
// case — and a zero-part credit reaching here would otherwise
|
||||
// render as nothing at all, which is worse than the single-artist
|
||||
// answer it replaced.
|
||||
if (!parts || parts.length < 2) {
|
||||
return artistLink(fallbackName, fallbackMbid);
|
||||
}
|
||||
|
||||
return html`${parts.map(
|
||||
(part) =>
|
||||
html`${artistLink(part.creditedName, part.artistMbid)}${part.joinPhrase}`,
|
||||
)}`;
|
||||
}
|
||||
|
||||
/**
|
||||
* The plain-text form of a credit, for `title=` attributes and any
|
||||
* other place that needs a string rather than a template.
|
||||
*
|
||||
* Rendered from the same parts by the same concatenation, so the
|
||||
* tooltip cannot disagree with the links beneath it.
|
||||
*/
|
||||
export function creditText(
|
||||
parts: readonly CreditPart[] | undefined,
|
||||
fallbackName: string,
|
||||
): string {
|
||||
if (!parts || parts.length < 2) return fallbackName;
|
||||
|
||||
return parts.map((p) => p.creditedName + p.joinPhrase).join('');
|
||||
}
|
||||
|
||||
@@ -0,0 +1,121 @@
|
||||
/**
|
||||
* A track credited to more than one artist has one navigable artist in
|
||||
* this app and the rest are punctuation. `creditLink` is the fix: it
|
||||
* renders a credit as one link per credited artist with the join
|
||||
* phrases as plain text between them.
|
||||
*
|
||||
* The rule these tests exist to pin is that join phrases are
|
||||
* **assembly** instructions, not disassembly instructions — the credit
|
||||
* is built from its parts, never found by searching a name inside a
|
||||
* credit string. Measured on a real library, the stored credit text and
|
||||
* the catalog's parts disagree for about one in three multi-artist
|
||||
* credits, so a search would miss or match the wrong span.
|
||||
*/
|
||||
import { describe, expect, it } from 'vitest';
|
||||
import { html, render } from 'lit';
|
||||
|
||||
import { creditLink, creditText, type CreditPart } from '@utils/explore-link';
|
||||
|
||||
const TUPAC = '11111111-1111-4111-8111-111111111111';
|
||||
const SNOOP = '22222222-2222-4222-8222-222222222222';
|
||||
|
||||
const parts: CreditPart[] = [
|
||||
{ creditedName: '2Pac', artistMbid: TUPAC, joinPhrase: ' feat. ' },
|
||||
{ creditedName: 'Snoop Dogg', artistMbid: SNOOP, joinPhrase: '' },
|
||||
];
|
||||
|
||||
function renderToEl(value: unknown): HTMLElement {
|
||||
const host = document.createElement('div');
|
||||
render(html`${value}`, host);
|
||||
|
||||
return host;
|
||||
}
|
||||
|
||||
describe('creditLink', () => {
|
||||
it('renders one link per credited artist', () => {
|
||||
const el = renderToEl(creditLink(parts, '2Pac feat. Snoop Dogg', TUPAC));
|
||||
const links = el.querySelectorAll('a.explore-link');
|
||||
|
||||
expect(links).toHaveLength(2);
|
||||
expect(links[0]?.textContent).toBe('2Pac');
|
||||
expect(links[1]?.textContent).toBe('Snoop Dogg');
|
||||
});
|
||||
|
||||
it('puts the join phrase between the links as plain text', () => {
|
||||
const el = renderToEl(creditLink(parts, '2Pac feat. Snoop Dogg', TUPAC));
|
||||
|
||||
// The whole credit reads correctly...
|
||||
expect(el.textContent?.replace(/\s+/g, ' ').trim()).toBe(
|
||||
'2Pac feat. Snoop Dogg',
|
||||
);
|
||||
|
||||
// ...and " feat. " is not inside either link, which is the
|
||||
// difference between a credit and a link with punctuation in it.
|
||||
for (const link of el.querySelectorAll('a.explore-link')) {
|
||||
expect(link.textContent).not.toMatch(/feat/);
|
||||
}
|
||||
});
|
||||
|
||||
it('falls back to a single link when there are no parts', () => {
|
||||
const el = renderToEl(creditLink(undefined, 'Alina Baraz & Galimatias', TUPAC));
|
||||
const links = el.querySelectorAll('a.explore-link');
|
||||
|
||||
expect(links).toHaveLength(1);
|
||||
expect(links[0]?.textContent).toBe('Alina Baraz & Galimatias');
|
||||
});
|
||||
|
||||
it('does not split the fallback string on its separators', () => {
|
||||
// "&" and "with" appear inside real artist names — "Simon &
|
||||
// Garfunkel" is one artist — so a credit with no parts is one
|
||||
// link, always. This is the whole reason primaryArtist() does
|
||||
// not split on them either.
|
||||
const el = renderToEl(creditLink(undefined, 'Simon & Garfunkel', TUPAC));
|
||||
|
||||
expect(el.querySelectorAll('a.explore-link')).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('treats a one-part credit as the single-link case', () => {
|
||||
// A zero- or one-part credit reaching the multi-artist branch
|
||||
// would render as nothing, or as a link with a dangling join
|
||||
// phrase after it.
|
||||
const one: CreditPart[] = [
|
||||
{ creditedName: 'Solo', artistMbid: TUPAC, joinPhrase: '' },
|
||||
];
|
||||
const el = renderToEl(creditLink(one, 'Solo', TUPAC));
|
||||
|
||||
expect(el.querySelectorAll('a.explore-link')).toHaveLength(1);
|
||||
expect(el.textContent?.trim()).toBe('Solo');
|
||||
});
|
||||
|
||||
it('renders the credited name, not the artist name', () => {
|
||||
// MusicBrainz credits "Snoop Dogg" on a track by the artist
|
||||
// called "Snoop Doggy Dogg". Display follows the credit;
|
||||
// navigation follows the MBID.
|
||||
const el = renderToEl(creditLink(parts, 'anything', TUPAC));
|
||||
|
||||
expect(el.textContent).toContain('Snoop Dogg');
|
||||
expect(el.textContent).not.toContain('Snoop Doggy Dogg');
|
||||
});
|
||||
});
|
||||
|
||||
describe('creditText', () => {
|
||||
it('reassembles the credit as a string', () => {
|
||||
expect(creditText(parts, 'ignored')).toBe('2Pac feat. Snoop Dogg');
|
||||
});
|
||||
|
||||
it('is the fallback string when there are no parts', () => {
|
||||
expect(creditText(undefined, 'Alina Baraz & Galimatias')).toBe(
|
||||
'Alina Baraz & Galimatias',
|
||||
);
|
||||
});
|
||||
|
||||
it('agrees with what creditLink renders', () => {
|
||||
// The tooltip and the links come from the same parts by the same
|
||||
// concatenation, so they cannot disagree.
|
||||
const el = renderToEl(creditLink(parts, 'ignored', TUPAC));
|
||||
|
||||
expect(el.textContent?.replace(/\s+/g, ' ').trim()).toBe(
|
||||
creditText(parts, 'ignored'),
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user