From 3d9828b847ba5b099458b31ebbff0b2f3cdd8696 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Mon, 5 Oct 2026 23:55:54 -0400 Subject: [PATCH] perf(library): hand the batch dialog the rows the view already has MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fetching whole tracks back for a dialog that reads only fields the rows in hand already carry cost 3 s and 700 ms of blocked main thread on a "select all" over 50 000 tracks — the regression #281 introduced by dropping four fields from the list. `showBatchTrackDetails` takes the rows; the Tracks view passes its own, and the views that hold no library rows (Explore, the queue, playlists) keep the path form over `trackCache`. The batch view's merged-fields extractors read only list fields already, and a cover tier the list does not carry falls back to the one it does. Measured on 50 000 tracks: 50.4 ms, from 87.9 ms before #281. Refs #281 --- .../components/track-details/track-details.ts | 17 ++-- .../src/components/track-list/track-list.ts | 10 ++- frontend/src/utils/track-details-opener.ts | 77 +++++++++++++------ 3 files changed, 70 insertions(+), 34 deletions(-) diff --git a/frontend/src/components/track-details/track-details.ts b/frontend/src/components/track-details/track-details.ts index d229aa5..37695e4 100644 --- a/frontend/src/components/track-details/track-details.ts +++ b/frontend/src/components/track-details/track-details.ts @@ -24,6 +24,7 @@ type TrackMBIDs = library.TrackMBIDs; import { ImageFilePicker, ReadFile } from '@go/frontendutil/frontendutil.js'; import { trackCache } from '../../store/track-cache'; import { batchCoverArt } from '@utils/track-details-opener.js'; +import type { ListTrack } from '@utils/track-table'; import { EventsOn } from '@runtime/runtime'; import { Events } from '../../events'; @@ -88,7 +89,11 @@ export class TrackDetails extends LitElement { // -- Batch-specific state -- @state() private batchMode = false; - @state() private batchTracks: library.Track[] = []; + // `ListTrack`, not `Track`: every field the merged-values view reads + // is on the list's rows, so a caller that is already showing them + // can open this dialog without fetching anything (#281). The + // openers that hold no rows ask `trackCache` instead. + @state() private batchTracks: ListTrack[] = []; @state() private batchFilePaths: string[] = []; @state() private batchCoverArtMixed = false; @state() private batchProgress: { @@ -136,12 +141,12 @@ export class TrackDetails extends LitElement { /** Open the dialog for batch editing multiple tracks. */ showBatch( - tracks: library.Track[], + tracks: readonly ListTrack[], coverArt: CoverArtUrls | null, coverArtMixed: boolean, ): void { this.batchMode = true; - this.batchTracks = tracks; + this.batchTracks = [...tracks]; this.batchFilePaths = tracks.map( (t) => t.FilePath, ); @@ -2133,7 +2138,7 @@ export class TrackDetails extends LitElement { key: string; label: string; type: 'text' | 'number'; - extract: (t: library.Track) => string; + extract: (t: ListTrack) => string; }> = [ { key: 'title', @@ -2217,7 +2222,7 @@ export class TrackDetails extends LitElement { private countDistinctValues(key: string): number { const extractMap: Record< string, - (t: library.Track) => string + (t: ListTrack) => string > = { title: (t) => t.TrackName ?? '', artist: (t) => t.ArtistName ?? '', @@ -2241,7 +2246,7 @@ export class TrackDetails extends LitElement { if (!extract) return 0; const unique = new Set( - this.batchTracks.map(extract), + this.batchTracks.map((t) => extract(t)), ); return unique.size; diff --git a/frontend/src/components/track-list/track-list.ts b/frontend/src/components/track-list/track-list.ts index e874619..df9d315 100644 --- a/frontend/src/components/track-list/track-list.ts +++ b/frontend/src/components/track-list/track-list.ts @@ -80,8 +80,8 @@ import { describeError } from '@utils/describe-error'; import { notificationStore } from '@store/notification-store'; import { confirmAction } from '@components/confirm-dialog/confirm-dialog'; import { RemoveFromLibrary } from '@go/library/library.js'; -import { showTrackDetailsForPath, showBatchTrackDetailsForPaths } from '@utils/track-details-opener.js'; -import { tracksByFilePath } from '@utils/track-index.js'; +import { showTrackDetailsForPath, showBatchTrackDetails } from '@utils/track-details-opener.js'; +import { tracksByFilePath, tracksForPaths } from '@utils/track-index.js'; import '@components/playlist-picker/playlist-picker.js'; import type { TrackDetails } from '@components/track-details/track-details.js'; import { @@ -2141,9 +2141,11 @@ export class TrackList private async openBatchTrackDetails( filePaths: string[], ) { - await showBatchTrackDetailsForPaths( + // The rows in hand are what the dialog reads, so a "select all" + // on 50 000 tracks opens it without asking the backend for them. + await showBatchTrackDetails( () => this.trackDetailsDialog, - filePaths, + tracksForPaths(this.tracks, filePaths), () => void this.openBatchTrackDetails(filePaths), ); } diff --git a/frontend/src/utils/track-details-opener.ts b/frontend/src/utils/track-details-opener.ts index af02150..5e1aad0 100644 --- a/frontend/src/utils/track-details-opener.ts +++ b/frontend/src/utils/track-details-opener.ts @@ -1,40 +1,54 @@ /** * Open `` for a file path. * - * The five library-side hosts already hold the `library.Track` the - * dialog wants — they render it. Explore's rows do not: a tracklist row - * is an `MBTrack`/`LBTopRecording` from the catalog, and all it can say + * Explore's rows carry no library metadata at all: a tracklist row is + * an `MBTrack`/`LBTopRecording` from the catalog, and all it can say * about the library is *which file is behind it*. So the path is the * one key both sides share, and turning it back into a track is the - * work this does. + * work this does. The queue, playlists and smart playlists are the same + * shape — they render their own row type. * - * The queue, playlists and smart playlists are in the same position: - * they render their own row type, not a `library.Track`. All of them - * used to find the track in `libraryStore`'s whole-library array, which - * meant that array had to be loaded for details to open — and the ones - * that read it synchronously silently did nothing when it was not - * (#279). They ask `trackCache` for the paths in hand instead: one - * small call, coalesced, answered from cache the second time. + * All of them used to find the track in `libraryStore`'s whole-library + * array, which meant that array had to be loaded for details to open — + * and the ones that read it synchronously silently did nothing when it + * was not (#279). They ask `trackCache` for the paths in hand instead: + * one small call, coalesced, answered from cache the second time. + * + * A caller that *is* showing the rows passes them + * (`showBatchTrackDetails`), because fetching back what is already in + * hand is how a "select all" over 50 000 tracks came to cost 3 s. */ -import type * as library from '@go/library/models.js'; import type { CoverArtUrls, TrackDetails, } from '@components/track-details/track-details.js'; import { trackCache } from '@store/track-cache.js'; import { loadTrackDetails } from '@utils/lazy-track-details.js'; +import type * as library from '@go/library/models.js'; +import type { ListTrack } from '@utils/track-table'; -/** The cover art the dialog shows, or nothing when the track has none. */ -function coverArtOf(track: library.Track): CoverArtUrls | undefined { - return track.CoverArtPath - ? { - coverArtPath: track.CoverArtPath, - coverArtSmall: track.CoverArtSmall, - coverArtMedium: track.CoverArtMedium, - coverArtLarge: track.CoverArtLarge, - } - : undefined; +/** + * The cover art the dialog shows, or nothing when the track has none. + * + * A whole track carries every tier; the list's rows carry only the + * small one (#281), and every tier is derived from the same file — so a + * missing one falls back to the tier that is there rather than the + * dialog showing nothing. + */ +function coverArtOf(track: ListTrack): CoverArtUrls | undefined { + const small = track.CoverArtSmall; + + if (!small) return undefined; + + const tiers = track as Partial; + + return { + coverArtPath: tiers.CoverArtPath ?? small, + coverArtSmall: small, + coverArtMedium: tiers.CoverArtMedium ?? small, + coverArtLarge: tiers.CoverArtLarge ?? small, + }; } /** @@ -42,7 +56,7 @@ function coverArtOf(track: library.Track): CoverArtUrls | undefined { * one album; none and `mixed` when they span several. */ export function batchCoverArt( - tracks: readonly library.Track[], + tracks: readonly ListTrack[], ): { coverArt: CoverArtUrls | null; mixed: boolean } { const albums = new Set(tracks.map((t) => t.Album)); @@ -101,8 +115,23 @@ export async function showBatchTrackDetailsForPaths( filePaths: readonly string[], retry: () => void, ): Promise { - const tracks = await trackCache.get(filePaths); + return showBatchTrackDetails(dialog, await trackCache.get(filePaths), retry); +} +/** + * Show the batch details dialog for tracks the caller already holds. + * + * The Tracks view is why this exists rather than only the path form: a + * selection there can be the whole library, and fetching every track of + * it back to read fields the rows in hand already carry cost 3 s and + * 700 ms of blocked main thread on 50 000 tracks (#281's own regression, + * measured). + */ +export async function showBatchTrackDetails( + dialog: () => TrackDetails | undefined, + tracks: readonly ListTrack[], + retry: () => void, +): Promise { if (tracks.length === 0) return 'not-in-library'; const ready = await loadTrackDetails(retry);