diff --git a/backend/database/sql/queries/audio_files.sql b/backend/database/sql/queries/audio_files.sql index de88294..4dc7a95 100644 --- a/backend/database/sql/queries/audio_files.sql +++ b/backend/database/sql/queries/audio_files.sql @@ -117,6 +117,9 @@ WHERE library_id = COALESCE(NULLIF(CAST(sqlc.arg(library_id) AS INTEGER), 0), li -- name: GetTrackByPath :one SELECT * FROM track_metadata WHERE file_path = ? LIMIT 1; +-- name: GetTracksByPaths :many +SELECT * FROM track_metadata WHERE file_path IN (sqlc.slice('paths')); + -- name: GetTracksByAlbum :many SELECT * FROM track_metadata WHERE album_id = sqlc.arg(album_id) diff --git a/backend/database/sql/sqlcgen/audio_files.sql.go b/backend/database/sql/sqlcgen/audio_files.sql.go index df3dabf..846e3ee 100644 --- a/backend/database/sql/sqlcgen/audio_files.sql.go +++ b/backend/database/sql/sqlcgen/audio_files.sql.go @@ -830,6 +830,71 @@ func (q *Queries) GetTracksByGenre(ctx context.Context, arg GetTracksByGenrePara return items, nil } +const getTracksByPaths = `-- name: GetTracksByPaths :many +SELECT id, file_path, length_milliseconds, title, artist_name, track_number, disc_number, album, genre, year, release_year, composer, file_type, sample_rate, bit_depth, channels, bitrate, file_size, library_id, play_count, last_played, cover_art_path, artist_mbid, release_group_mbid, recording_mbid, album_id, artist_id FROM track_metadata WHERE file_path IN (/*SLICE:paths*/?) +` + +func (q *Queries) GetTracksByPaths(ctx context.Context, paths []string) ([]TrackMetadatum, error) { + query := getTracksByPaths + var queryParams []interface{} + if len(paths) > 0 { + for _, v := range paths { + queryParams = append(queryParams, v) + } + query = strings.Replace(query, "/*SLICE:paths*/?", strings.Repeat(",?", len(paths))[1:], 1) + } else { + query = strings.Replace(query, "/*SLICE:paths*/?", "NULL", 1) + } + rows, err := q.db.QueryContext(ctx, query, queryParams...) + if err != nil { + return nil, err + } + defer rows.Close() + var items []TrackMetadatum + for rows.Next() { + var i TrackMetadatum + if err := rows.Scan( + &i.ID, + &i.FilePath, + &i.LengthMilliseconds, + &i.Title, + &i.ArtistName, + &i.TrackNumber, + &i.DiscNumber, + &i.Album, + &i.Genre, + &i.Year, + &i.ReleaseYear, + &i.Composer, + &i.FileType, + &i.SampleRate, + &i.BitDepth, + &i.Channels, + &i.Bitrate, + &i.FileSize, + &i.LibraryID, + &i.PlayCount, + &i.LastPlayed, + &i.CoverArtPath, + &i.ArtistMbid, + &i.ReleaseGroupMbid, + &i.RecordingMbid, + &i.AlbumID, + &i.ArtistID, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const lookupTrackMetaByPaths = `-- name: LookupTrackMetaByPaths :many SELECT id, file_path, title, artist_name, album, cover_art_path, artist_mbid, release_group_mbid, recording_mbid diff --git a/backend/library/query.go b/backend/library/query.go index efc05e0..b1574da 100644 --- a/backend/library/query.go +++ b/backend/library/query.go @@ -193,6 +193,48 @@ func (l *Library) GetTracks(libraryID int64) ([]Track, error) { return tracksFromRows(rows), nil } +// pathLookupChunk bounds the paths bound into one IN (...) query, well +// under SQLite's bind-variable limit. +const pathLookupChunk = 500 + +// GetTracksByPaths returns whole tracks for the given file paths, in +// the order asked, dropping any path that is not in the library. +// +// It is how the frontend resolves the tracks a surface is actually +// showing (#279). Track details from the queue, a playlist or a smart +// playlist used to look the path up in the whole library's track +// array, which had to be loaded first — so it was fetched eagerly at +// startup, 20.5 MB at 26k tracks, to answer questions about a handful +// of rows. +func (l *Library) GetTracksByPaths(paths []string) ([]Track, error) { + byPath := make(map[string]Track, len(paths)) + + for start := 0; start < len(paths); start += pathLookupChunk { + chunk := paths[start:min(start+pathLookupChunk, len(paths))] + + rows, err := l.db.ReadQueries.GetTracksByPaths(l.ctx, chunk) + if err != nil { + return nil, fmt.Errorf("could not get tracks by path: %w", err) + } + + for _, row := range rows { + byPath[row.FilePath] = trackFromRow(row) + } + } + + tracks := make([]Track, 0, len(byPath)) + + for _, path := range paths { + if t, ok := byPath[path]; ok { + tracks = append(tracks, t) + // A path asked twice is answered once. + delete(byPath, path) + } + } + + return tracks, nil +} + // SearchTracks runs the library's FTS index and returns whole tracks. func (l *Library) SearchTracks(query string, libraryID int64) ([]Track, error) { rows, err := l.db.SearchFTSTracks(query, libraryID, searchTrackLimit) diff --git a/backend/library/trackpaths_test.go b/backend/library/trackpaths_test.go new file mode 100644 index 0000000..69226e1 --- /dev/null +++ b/backend/library/trackpaths_test.go @@ -0,0 +1,64 @@ +package library + +import ( + "fmt" + "testing" +) + +// #279: the track-details openers resolve the rows a surface is showing +// by path, instead of finding them in the whole library's array. The +// answer must keep the caller's order (a batch dialog lists them as +// selected), drop what is not in the library rather than invent it, and +// survive more paths than one IN (...) can bind. +func TestGetTracksByPaths(t *testing.T) { + t.Parallel() + + lib, _ := setupTestLibrary(t) + seedAlbumsAndGenres(t, lib) + + got, err := lib.GetTracksByPaths([]string{ + "/other/b1.mp3", "/music/missing.mp3", "/music/a1.mp3", "/other/b1.mp3", + }) + if err != nil { + t.Fatalf("GetTracksByPaths: %v", err) + } + + paths := make([]string, 0, len(got)) + for _, tr := range got { + paths = append(paths, tr.FilePath) + } + + if want := "[/other/b1.mp3 /music/a1.mp3]"; fmt.Sprint(paths) != want { + t.Fatalf("paths = %v, want %s (caller order, missing dropped, duplicate once)", paths, want) + } + + // Whole tracks, not just the key: details render every field. + if got[1].TrackName != "A1" || len(got[1].Genre) != 2 { + t.Errorf("a1 = %+v, want title A1 with two genres", got[1]) + } +} + +func TestGetTracksByPathsSpansChunks(t *testing.T) { + t.Parallel() + + lib, _ := setupTestLibrary(t) + seedAlbumsAndGenres(t, lib) + + // The real path last, behind more misses than one chunk binds, so a + // loop that only ran the first chunk would return nothing. + paths := make([]string, 0, pathLookupChunk+2) + for i := range pathLookupChunk + 1 { + paths = append(paths, fmt.Sprintf("/nowhere/%d.mp3", i)) + } + + paths = append(paths, "/music/a2.mp3") + + got, err := lib.GetTracksByPaths(paths) + if err != nil { + t.Fatalf("GetTracksByPaths: %v", err) + } + + if len(got) != 1 || got[0].FilePath != "/music/a2.mp3" { + t.Fatalf("got %d tracks (%v), want only /music/a2.mp3", len(got), got) + } +} diff --git a/frontend/bindings/yellowjacket/backend/library/library.ts b/frontend/bindings/yellowjacket/backend/library/library.ts index 0591049..349c325 100644 --- a/frontend/bindings/yellowjacket/backend/library/library.ts +++ b/frontend/bindings/yellowjacket/backend/library/library.ts @@ -222,6 +222,21 @@ export function GetTracksByGenre(genre: string, libraryID: number): $Cancellable return $Call.ByID(1674220245, genre, libraryID); } +/** + * GetTracksByPaths returns whole tracks for the given file paths, in + * the order asked, dropping any path that is not in the library. + * + * It is how the frontend resolves the tracks a surface is actually + * showing (#279). Track details from the queue, a playlist or a smart + * playlist used to look the path up in the whole library's track + * array, which had to be loaded first — so it was fetched eagerly at + * startup, 20.5 MB at 26k tracks, to answer questions about a handful + * of rows. + */ +export function GetTracksByPaths(paths: string[] | null): $CancellablePromise<$models.Track[] | null> { + return $Call.ByID(3966945290, paths); +} + /** * IsScanActive returns whether a scan is currently running. */ diff --git a/frontend/src/components/playlist-details/playlist-details.ts b/frontend/src/components/playlist-details/playlist-details.ts index f38f359..4614f6b 100644 --- a/frontend/src/components/playlist-details/playlist-details.ts +++ b/frontend/src/components/playlist-details/playlist-details.ts @@ -56,12 +56,9 @@ import { createTrackCardDragImage, removeDragImage, } from '@utils/drag-image'; -import { libraryStore } from '@store/library-store'; import '@components/playlist-picker/playlist-picker.js'; -import { loadTrackDetails } from '@utils/lazy-track-details.js'; -import { tracksByFilePath, tracksForPaths } from '@utils/track-index.js'; +import { showTrackDetailsForPath, showBatchTrackDetailsForPaths } from '@utils/track-details-opener.js'; import type { TrackDetails } from '@components/track-details/track-details.js'; -import type { CoverArtUrls } from '@components/track-details/track-details.js'; import '@components/phantom-resolver/phantom-resolver.js'; import type { PhantomResolver } from '@components/phantom-resolver/phantom-resolver.js'; import '@components/duplicate-tracks-dialog/duplicate-tracks-dialog.js'; @@ -758,76 +755,21 @@ export class PlaylistDetails } private async openTrackDetails(filePath: string) { - const tracks = libraryStore.getCachedTracks(); - const track = tracks - ? tracksByFilePath(tracks).get(filePath) - : undefined; - - if (!track) return; - - const ready = await loadTrackDetails( + await showTrackDetailsForPath( + () => this.trackDetailsDialog, + filePath, () => void this.openTrackDetails(filePath), ); - - if (!ready) return; - - const coverArt = track.CoverArtPath - ? { - coverArtPath: track.CoverArtPath, - coverArtSmall: track.CoverArtSmall, - coverArtMedium: track.CoverArtMedium, - coverArtLarge: track.CoverArtLarge, - } - : undefined; - - this.trackDetailsDialog?.show( - track, - coverArt, - ); } private async openBatchTrackDetails( filePaths: string[], ) { - const cachedTracks = - libraryStore.getCachedTracks(); - - if (!cachedTracks) return; - - const tracks = tracksForPaths( - cachedTracks, + await showBatchTrackDetailsForPaths( + () => this.trackDetailsDialog, filePaths, - ); - - if (tracks.length === 0) return; - - const ready = await loadTrackDetails( () => void this.openBatchTrackDetails(filePaths), ); - - if (!ready) return; - - const first = tracks[0]!; - const albumNames = new Set(tracks.map((t) => t.Album)); - let coverArt: CoverArtUrls | null = null; - let coverArtMixed = false; - - if (albumNames.size === 1 && first.CoverArtPath) { - coverArt = { - coverArtPath: first.CoverArtPath, - coverArtSmall: first.CoverArtSmall, - coverArtMedium: first.CoverArtMedium, - coverArtLarge: first.CoverArtLarge, - }; - } else if (albumNames.size > 1) { - coverArtMixed = true; - } - - this.trackDetailsDialog?.showBatch( - tracks, - coverArt, - coverArtMixed, - ); } /** diff --git a/frontend/src/components/queue-panel/queue-panel.ts b/frontend/src/components/queue-panel/queue-panel.ts index 990ce2e..1bb1c2f 100644 --- a/frontend/src/components/queue-panel/queue-panel.ts +++ b/frontend/src/components/queue-panel/queue-panel.ts @@ -51,12 +51,8 @@ import { createTrackCardDragImage, removeDragImage, } from '@utils/drag-image'; -import { libraryStore } from '@store/library-store'; -import type * as library from '@go/library/models.js'; -import { loadTrackDetails } from '@utils/lazy-track-details.js'; -import { tracksByFilePath } from '@utils/track-index.js'; +import { showTrackDetailsForPath, showBatchTrackDetailsForPaths } from '@utils/track-details-opener.js'; import type { TrackDetails } from '@components/track-details/track-details.js'; -import type { CoverArtUrls } from '@components/track-details/track-details.js'; import { creditLink, trackLink, @@ -1542,90 +1538,30 @@ export class QueuePanel } private async openTrackDetails(index: number) { - const queueTrack = - this.queue.tracks[index]; + const queueTrack = this.queue.tracks[index]; if (!queueTrack) return; - const tracks = - libraryStore.getCachedTracks(); - const track = tracks - ? tracksByFilePath(tracks).get( - queueTrack.filePath, - ) - : undefined; - - if (!track) return; - - const ready = await loadTrackDetails( + await showTrackDetailsForPath( + () => this.trackDetailsDialog, + queueTrack.filePath, () => void this.openTrackDetails(index), ); - - if (!ready) return; - - const coverArt = track.CoverArtPath - ? { - coverArtPath: track.CoverArtPath, - coverArtSmall: track.CoverArtSmall, - coverArtMedium: track.CoverArtMedium, - coverArtLarge: track.CoverArtLarge, - } - : undefined; - - this.trackDetailsDialog?.show( - track, - coverArt, - ); } private async openBatchTrackDetails( indices: number[], ) { const queueTracks = this.queue.tracks; - const cachedTracks = - libraryStore.getCachedTracks(); + const filePaths = indices + .map((i) => queueTracks[i]?.filePath) + .filter((fp): fp is string => fp != null); - if (!cachedTracks) return; - - const byPath = tracksByFilePath(cachedTracks); - const tracks = indices - .map((i) => queueTracks[i]) - .filter((qt) => qt != null) - .map((qt) => byPath.get(qt.filePath)) - .filter( - (t): t is library.Track => - t != null, - ); - - if (tracks.length === 0) return; - - const ready = await loadTrackDetails( + await showBatchTrackDetailsForPaths( + () => this.trackDetailsDialog, + filePaths, () => void this.openBatchTrackDetails(indices), ); - - if (!ready) return; - - const first = tracks[0]!; - const albumNames = new Set(tracks.map((t) => t.Album)); - let coverArt: CoverArtUrls | null = null; - let coverArtMixed = false; - - if (albumNames.size === 1 && first.CoverArtPath) { - coverArt = { - coverArtPath: first.CoverArtPath, - coverArtSmall: first.CoverArtSmall, - coverArtMedium: first.CoverArtMedium, - coverArtLarge: first.CoverArtLarge, - }; - } else if (albumNames.size > 1) { - coverArtMixed = true; - } - - this.trackDetailsDialog?.showBatch( - tracks, - coverArt, - coverArtMixed, - ); } private onContextPlaylistActionComplete = () => { diff --git a/frontend/src/components/smart-playlist-details/smart-playlist-details.ts b/frontend/src/components/smart-playlist-details/smart-playlist-details.ts index 5dd008b..049bc7c 100644 --- a/frontend/src/components/smart-playlist-details/smart-playlist-details.ts +++ b/frontend/src/components/smart-playlist-details/smart-playlist-details.ts @@ -52,11 +52,8 @@ import '@lit-labs/virtualizer'; import type { LitVirtualizer } from '@lit-labs/virtualizer'; import { flow } from '@lit-labs/virtualizer/layouts/flow.js'; import '@components/playlist-picker/playlist-picker.js'; -import { loadTrackDetails } from '@utils/lazy-track-details.js'; -import { tracksByFilePath, tracksForPaths } from '@utils/track-index.js'; +import { showTrackDetailsForPath, showBatchTrackDetailsForPaths } from '@utils/track-details-opener.js'; import type { TrackDetails } from '@components/track-details/track-details.js'; -import type { CoverArtUrls } from '@components/track-details/track-details.js'; -import { libraryStore } from '@store/library-store'; import { formatMilliseconds } from '@utils/time'; import { creditLink, @@ -1145,76 +1142,21 @@ export class SmartPlaylistDetails } private async openTrackDetails(filePath: string) { - const tracks = libraryStore.getCachedTracks(); - const track = tracks - ? tracksByFilePath(tracks).get(filePath) - : undefined; - - if (!track) return; - - const ready = await loadTrackDetails( + await showTrackDetailsForPath( + () => this.trackDetailsDialog, + filePath, () => void this.openTrackDetails(filePath), ); - - if (!ready) return; - - const coverArt = track.CoverArtPath - ? { - coverArtPath: track.CoverArtPath, - coverArtSmall: track.CoverArtSmall, - coverArtMedium: track.CoverArtMedium, - coverArtLarge: track.CoverArtLarge, - } - : undefined; - - this.trackDetailsDialog?.show( - track, - coverArt, - ); } private async openBatchTrackDetails( filePaths: string[], ) { - const cachedTracks = - libraryStore.getCachedTracks(); - - if (!cachedTracks) return; - - const tracks = tracksForPaths( - cachedTracks, + await showBatchTrackDetailsForPaths( + () => this.trackDetailsDialog, filePaths, - ); - - if (tracks.length === 0) return; - - const ready = await loadTrackDetails( () => void this.openBatchTrackDetails(filePaths), ); - - if (!ready) return; - - const first = tracks[0]!; - const albumNames = new Set(tracks.map((t) => t.Album)); - let coverArt: CoverArtUrls | null = null; - let coverArtMixed = false; - - if (albumNames.size === 1 && first.CoverArtPath) { - coverArt = { - coverArtPath: first.CoverArtPath, - coverArtSmall: first.CoverArtSmall, - coverArtMedium: first.CoverArtMedium, - coverArtLarge: first.CoverArtLarge, - }; - } else if (albumNames.size > 1) { - coverArtMixed = true; - } - - this.trackDetailsDialog?.showBatch( - tracks, - coverArt, - coverArtMixed, - ); } // ================================================================= diff --git a/frontend/src/components/track-details/track-details.ts b/frontend/src/components/track-details/track-details.ts index f63d6b2..d229aa5 100644 --- a/frontend/src/components/track-details/track-details.ts +++ b/frontend/src/components/track-details/track-details.ts @@ -22,7 +22,8 @@ import { import { GetTrackMBIDs } from '@go/library/library.js'; type TrackMBIDs = library.TrackMBIDs; import { ImageFilePicker, ReadFile } from '@go/frontendutil/frontendutil.js'; -import { libraryStore } from '../../store/library-store'; +import { trackCache } from '../../store/track-cache'; +import { batchCoverArt } from '@utils/track-details-opener.js'; import { EventsOn } from '@runtime/runtime'; import { Events } from '../../events'; @@ -1317,10 +1318,13 @@ export class TrackDetails extends LitElement { const container = img.parentElement; if (container) { - container.innerHTML = - '
' + - '' + - '
'; + const placeholder = document.createElement('div'); + const icon = document.createElement('wa-icon'); + + placeholder.className = 'cover-placeholder'; + icon.setAttribute('name', 'music'); + placeholder.append(icon); + container.replaceChildren(placeholder); } }; @@ -1690,20 +1694,11 @@ export class TrackDetails extends LitElement { // invalidation, which refreshes all other views. this.exitEditMode(); - // Re-fetch track and album data so the dialog - // shows updated values and cover art. The store - // invalidation is already in-flight from the event; - // these calls await the pending fetch or start one. - // Awaited for the side effect of refreshing the - // store; the album list is consumed elsewhere. - const [tracks] = await Promise.all([ - libraryStore.getTracks(), - libraryStore.getAlbums(), - ]); - - const updated = tracks.find( - (t) => t.FilePath === filePath, - ); + // Re-read this one track so the dialog shows the + // values and cover art just written. Not the whole + // library (#279): the views that hold it refetch on + // the event, and only if something is showing them. + const [updated] = await trackCache.refresh([filePath]); if (updated) { this.track = updated; @@ -1828,39 +1823,18 @@ export class TrackDetails extends LitElement { this.errorMessage = ''; this.cleanupPendingCoverArt(); - // Refresh data from library store; album list is - // refreshed for side effects only. - const [tracks] = await Promise.all([ - libraryStore.getTracks(), - libraryStore.getAlbums(), - ]); + // Re-read the tracks just written, in the order the batch + // holds them (#279) — not the whole library to filter. + // `refresh`, because the write's event may not have reached + // the cache before its own result did. + const refreshed = await trackCache.refresh(this.batchFilePaths); - // Re-resolve batch tracks. - const pathSet = new Set(this.batchFilePaths); - const refreshed = tracks.filter((t) => - pathSet.has(t.FilePath), - ); this.batchTracks = refreshed; - // Re-resolve cover art state. - const first = refreshed[0]; - const albumNames = new Set(refreshed.map((t) => t.Album)); + const { coverArt, mixed } = batchCoverArt(refreshed); - if (albumNames.size === 1 && first?.CoverArtPath) { - this.coverArt = { - coverArtPath: first.CoverArtPath, - coverArtSmall: first.CoverArtSmall, - coverArtMedium: first.CoverArtMedium, - coverArtLarge: first.CoverArtLarge, - }; - this.batchCoverArtMixed = false; - } else if (albumNames.size > 1) { - this.coverArt = null; - this.batchCoverArtMixed = true; - } else { - this.coverArt = null; - this.batchCoverArtMixed = false; - } + this.coverArt = coverArt; + this.batchCoverArtMixed = mixed; }; // -- Shared edit logic -- @@ -2103,6 +2077,8 @@ export class TrackDetails extends LitElement { // as a base64-encoded string (standard encoding/json // behaviour for []byte). const result = await ReadFile(filePath); + // SAFETY: the binding is typed as the Go []byte, but the + // wire value is the base64 string encoding/json made of it. const b64 = result as unknown as string; const binary = atob(b64); const bytes = new Uint8Array(binary.length); diff --git a/frontend/src/store/track-cache.ts b/frontend/src/store/track-cache.ts new file mode 100644 index 0000000..6e496ce --- /dev/null +++ b/frontend/src/store/track-cache.ts @@ -0,0 +1,240 @@ +/** + * Whole tracks, looked up by file path, for the rows a surface is + * actually showing (#279). + * + * Track details from the queue, a playlist or a smart playlist used to + * find their track in `libraryStore`'s whole-library array. That made + * the array a dependency of every surface that can open details, so it + * was fetched eagerly at startup — 20.5 MB of JSON at 26 138 tracks — + * and when it had *not* landed yet, the openers that read it + * synchronously did nothing at all. This asks the backend for the + * paths in hand instead. + * + * Three things are load-bearing, the same three as `credit-store`: + * + * **Lookups are coalesced.** Every `get()` made in the same task joins + * one `GetTracksByPaths` call. A timer rather than a frame: these are + * user actions, not row renders, and a frame never fires in a hidden + * window, which would leave the caller waiting on nothing. + * + * **It is bounded.** A batch details dialog over "Select all" asks for + * every track in the library; it gets every one of them back, but the + * cache keeps only the most recent `TRACK_CACHE_LIMIT`. + * + * **It forgets what changed.** The events that make `library-store` + * refetch drop the affected entries here, and a play count is patched + * in place, so a cached track is never older than the last event about + * it. An answer that was in flight across an invalidation is still + * returned to its caller (it was correct when asked) but not cached. + */ + +import { EventsOn } from '@runtime/runtime'; +import { GetTracksByPaths } from '@go/library/library.js'; +import type * as library from '@go/library/models.js'; +import { list } from '@utils/binding'; +import { LRUMap } from '@utils/lru-map'; +import { registerCacheProbe } from '@utils/cache-stats'; +import { Events } from '../events'; + +/** + * Tracks retained. A track is ~25 short fields, so this is a couple of + * megabytes at most — sized above any batch a person edits by hand, + * far below a library. + */ +export const TRACK_CACHE_LIMIT = 2_000; + +interface Waiter { + paths: readonly string[]; + resolve: (tracks: library.Track[]) => void; + reject: (err: unknown) => void; +} + +class TrackCache { + private cache = new LRUMap(TRACK_CACHE_LIMIT); + + /** Requests collected in this task, answered by one binding call. */ + private waiting: Waiter[] = []; + + private flushHandle: ReturnType | null = null; + + /** Bumped by every invalidation; an older answer is not cached. */ + private gen = 0; + + constructor() { + EventsOn(Events.LibraryScanComplete, () => this.clear()); + EventsOn(Events.LibraryRemoved, () => this.clear()); + EventsOn(Events.TrackMetadataChanged, (payload: unknown) => { + const p = payload as { filePath?: string } | null; + + // A batch write names no paths: forget everything. + if (p?.filePath) this.forget([p.filePath]); + else this.clear(); + }); + EventsOn(Events.TracksRemovedFromLibrary, (payload: unknown) => { + const p = payload as { filePaths?: string[] } | null; + + this.forget(p?.filePaths ?? []); + }); + EventsOn(Events.TrackPlayCountChanged, (payload: unknown) => { + this.applyPlayCount(payload); + }); + + registerCacheProbe('tracks-by-path', () => ({ + entries: this.cache.size, + chars: this.retainedChars(), + limit: TRACK_CACHE_LIMIT, + })); + } + + /** + * The tracks at `paths`, in the order given, without the ones that + * are not in the library. Cached tracks are answered without a call. + */ + get(paths: readonly string[]): Promise { + const answered = this.fromCache(paths); + + if (answered) return Promise.resolve(answered); + + return new Promise((resolve, reject) => { + this.waiting.push({ paths, resolve, reject }); + this.flushHandle ??= setTimeout(() => void this.flush(), 0); + }); + } + + /** One track, or undefined when the path is not in the library. */ + async getOne(path: string): Promise { + return (await this.get([path]))[0]; + } + + /** + * Ask the backend again for `paths`, ignoring the cache. + * + * For a caller that has just written to these files and must not be + * answered from before the write, whether or not the event that + * invalidates them has arrived yet. + */ + refresh(paths: readonly string[]): Promise { + this.forget(paths); + + return this.get(paths); + } + + /** Every path cached, or null if any is missing. */ + private fromCache(paths: readonly string[]): library.Track[] | null { + const tracks: library.Track[] = []; + + for (const path of paths) { + const track = this.cache.get(path); + + if (!track) return null; + + tracks.push(track); + } + + return tracks; + } + + private async flush(): Promise { + this.flushHandle = null; + + const waiting = this.waiting; + + this.waiting = []; + + const missing = new Set(); + + for (const w of waiting) { + for (const path of w.paths) { + if (!this.cache.has(path)) missing.add(path); + } + } + + const gen = this.gen; + let fetched: library.Track[]; + + try { + fetched = missing.size > 0 + ? await list(GetTracksByPaths([...missing])) + : []; + } catch (err) { + for (const w of waiting) w.reject(err); + + return; + } + + // Answer from what this call returned plus what was cached when + // it was made — not from the cache afterwards, which an LRU + // eviction or an invalidation may have emptied in between. + const answer = new Map(); + + for (const w of waiting) { + for (const path of w.paths) { + const hit = this.cache.get(path); + + if (hit) answer.set(path, hit); + } + } + + for (const track of fetched) { + answer.set(track.FilePath, track); + + if (gen === this.gen) this.cache.set(track.FilePath, track); + } + + for (const w of waiting) { + const tracks: library.Track[] = []; + + for (const path of w.paths) { + const track = answer.get(path); + + if (track) tracks.push(track); + } + + w.resolve(tracks); + } + } + + private forget(paths: readonly string[]): void { + for (const path of paths) this.cache.delete(path); + + this.gen++; + } + + private clear(): void { + this.cache.clear(); + this.gen++; + } + + private applyPlayCount(payload: unknown): void { + const p = payload as { + filePath?: string; + playCount?: number; + lastPlayed?: string; + } | null; + + if (!p?.filePath) return; + + const existing = this.cache.get(p.filePath); + + if (!existing) return; + + this.cache.set(p.filePath, { + ...existing, + PlayCount: p.playCount ?? existing.PlayCount, + LastPlayed: p.lastPlayed ?? existing.LastPlayed, + }); + } + + private retainedChars(): number { + let total = 0; + + for (const t of this.cache.values()) { + total += t.FilePath.length + t.TrackName.length + + t.ArtistName.length + t.Album.length; + } + + return total; + } +} + +export const trackCache = new TrackCache(); diff --git a/frontend/src/utils/track-details-opener.ts b/frontend/src/utils/track-details-opener.ts index bb21e5a..af02150 100644 --- a/frontend/src/utils/track-details-opener.ts +++ b/frontend/src/utils/track-details-opener.ts @@ -8,12 +8,13 @@ * one key both sides share, and turning it back into a track is the * work this does. * - * `libraryStore.getTracks()` is awaited rather than - * `getCachedTracks()`-and-bail (which is what `queue-panel` does): - * Explore is reachable without ever opening the library views, so a - * cold cache is ordinary here rather than a symptom, and silently doing - * nothing on a menu item the user just clicked is not an option. The - * fetch is the store's own, shared with every other reader. + * 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. */ import type * as library from '@go/library/models.js'; @@ -21,9 +22,8 @@ import type { CoverArtUrls, TrackDetails, } from '@components/track-details/track-details.js'; -import { libraryStore } from '@store/library-store.js'; +import { trackCache } from '@store/track-cache.js'; import { loadTrackDetails } from '@utils/lazy-track-details.js'; -import { tracksByFilePath } from '@utils/track-index.js'; /** The cover art the dialog shows, or nothing when the track has none. */ function coverArtOf(track: library.Track): CoverArtUrls | undefined { @@ -37,6 +37,22 @@ function coverArtOf(track: library.Track): CoverArtUrls | undefined { : undefined; } +/** + * The cover a batch dialog shows: the album's, when every track is on + * one album; none and `mixed` when they span several. + */ +export function batchCoverArt( + tracks: readonly library.Track[], +): { coverArt: CoverArtUrls | null; mixed: boolean } { + const albums = new Set(tracks.map((t) => t.Album)); + + if (albums.size > 1) return { coverArt: null, mixed: true }; + + const first = tracks[0]; + + return { coverArt: first ? coverArtOf(first) ?? null : null, mixed: false }; +} + /** * What became of the attempt. * @@ -62,8 +78,7 @@ export async function showTrackDetailsForPath( filePath: string, retry: () => void, ): Promise { - const tracks = await libraryStore.getTracks(); - const track = tracksByFilePath(tracks).get(filePath); + const track = await trackCache.getOne(filePath); if (!track) return 'not-in-library'; @@ -75,3 +90,28 @@ export async function showTrackDetailsForPath( return 'shown'; } + +/** + * Show the batch details dialog for the library tracks at `filePaths`, + * in that order. Paths not in the library are left out; when none are, + * nothing is shown. + */ +export async function showBatchTrackDetailsForPaths( + dialog: () => TrackDetails | undefined, + filePaths: readonly string[], + retry: () => void, +): Promise { + const tracks = await trackCache.get(filePaths); + + if (tracks.length === 0) return 'not-in-library'; + + const ready = await loadTrackDetails(retry); + + if (!ready) return 'chunk-failed'; + + const { coverArt, mixed } = batchCoverArt(tracks); + + dialog()?.showBatch(tracks, coverArt, mixed); + + return 'shown'; +} diff --git a/frontend/test/components/explore-track-details.test.ts b/frontend/test/components/explore-track-details.test.ts index aab73a9..c051ee2 100644 --- a/frontend/test/components/explore-track-details.test.ts +++ b/frontend/test/components/explore-track-details.test.ts @@ -28,7 +28,7 @@ import type { TrackDetails } from '@components/track-details/track-details'; const ALBUM_TRACKS = 'library.Library.GetAlbumTracks'; const COMPLETENESS = 'library.Library.GetAlbumCompleteness'; const FILE_PATHS = 'library.Library.GetFilePathsByRecordingMBIDs'; -const ALL_TRACKS = 'library.Library.GetTracks'; +const TRACKS_BY_PATH = 'library.Library.GetTracksByPaths'; const LOOKUP_RG = 'explore.Service.LookupReleaseGroup'; const BROWSE_RELEASES = 'explore.Service.BrowseReleases'; @@ -127,14 +127,13 @@ describe('Explore track details', () => { stub(ALBUM_TRACKS, [albumTrack]); stub(COMPLETENESS, { known: true, complete: true, owned: 1, expected: 1 }); stub(FILE_PATHS, { [MBID]: [PATH] }); - stub(ALL_TRACKS, [libraryTrack]); + stub(TRACKS_BY_PATH, [libraryTrack]); }); /** - * `libraryStore` fetches at import and caches the empty list the - * shared setup stubs, for the life of the browser session — so a test - * that wants tracks in it has to say so. A scan-complete event is how - * the app itself invalidates that cache. + * `trackCache` keeps what it was answered for the life of the browser + * session, so a test that changes the answer has to drop it. A + * scan-complete event is how the app itself invalidates that cache. */ async function primeLibrary() { emit(Events.LibraryScanComplete); @@ -204,7 +203,7 @@ describe('Explore track details', () => { items[details]!.dispatchEvent(new MouseEvent('click', { bubbles: true })); // Polled rather than counted: the opener is three awaits deep — the - // path lookup, the store's tracks, and the dynamic `import()` of + // path lookup, the track by path, and the dynamic `import()` of // the dialog chunk — and a chunk fetch is the one of the three // whose cost depends on what else the suite is doing. const shown = await dialogTrack(el); @@ -230,7 +229,7 @@ describe('Explore track details', () => { }); it('reports a path with no library track rather than opening empty', async () => { - stub(ALL_TRACKS, []); + stub(TRACKS_BY_PATH, []); await primeLibrary(); const outcome = await showTrackDetailsForPath( diff --git a/frontend/test/components/lazy-track-details.test.ts b/frontend/test/components/lazy-track-details.test.ts index 1c8de60..78cd979 100644 --- a/frontend/test/components/lazy-track-details.test.ts +++ b/frontend/test/components/lazy-track-details.test.ts @@ -51,8 +51,12 @@ describe('track-details stays out of the startup chunk', () => { }, ); + // Directly, or through `utils/track-details-opener`, which awaits + // it and is what the path-keyed openers share (#279). it.each(OPENERS)('%s loads it at the point of use', (_name, source) => { - expect(source).toContain('loadTrackDetails'); + expect(source).toMatch( + /loadTrackDetails|showTrackDetailsForPath|showBatchTrackDetailsForPaths/, + ); }); }); diff --git a/frontend/test/stores/track-cache.test.ts b/frontend/test/stores/track-cache.test.ts new file mode 100644 index 0000000..8804e19 --- /dev/null +++ b/frontend/test/stores/track-cache.test.ts @@ -0,0 +1,209 @@ +/** + * `trackCache` answers "which track is at this path" for the rows a + * surface is showing (#279), instead of every surface reaching into the + * whole library's array — which had to be fetched eagerly at startup + * for that to work, and silently did nothing when it had not been. + */ +import { describe, expect, it, beforeEach } from 'vitest'; + +import { trackCache } from '@store/track-cache'; +import { libraryStore } from '@store/library-store'; +import { showBatchTrackDetailsForPaths } from '@utils/track-details-opener'; +import type { TrackDetails } from '@components/track-details/track-details'; +import { Events } from '../../src/events'; +import { + calls, + emit, + flush, + resetHarness, + stub, + stubFailure, +} from '@test/support/harness'; + +const BY_PATHS = 'library.Library.GetTracksByPaths'; + +function track(path: string, album = 'Album') { + return { + FilePath: path, + TrackName: path, + ArtistName: 'Artist', + Album: album, + PlayCount: 0, + LastPlayed: '', + CoverArtPath: '', + }; +} + +/** The backend: answers every path that starts with /lib/. */ +function stubLibrary(): void { + stub(BY_PATHS, (paths: string[]) => + paths.filter((p) => p.startsWith('/lib/')).map((p) => track(p)), + ); +} + +describe('track cache', () => { + beforeEach(() => { + // The cache is a singleton; a scan completing is how the app empties it. + emit(Events.LibraryScanComplete); + resetHarness(); + stubLibrary(); + }); + + it('answers in the order asked and leaves out paths not in the library', async () => { + const got = await trackCache.get(['/lib/b', '/gone', '/lib/a']); + + expect(got.map((t) => t.FilePath)).toEqual(['/lib/b', '/lib/a']); + }); + + it('coalesces lookups made together into one call', async () => { + await Promise.all([ + trackCache.get(['/lib/a']), + trackCache.get(['/lib/b', '/lib/a']), + trackCache.getOne('/lib/c'), + ]); + + expect(calls(BY_PATHS)).toHaveLength(1); + expect((calls(BY_PATHS)[0]!.args[0] as string[]).sort()).toEqual([ + '/lib/a', + '/lib/b', + '/lib/c', + ]); + }); + + it('answers a repeat from cache, and asks again after a retag of that file', async () => { + await trackCache.get(['/lib/a', '/lib/b']); + await trackCache.get(['/lib/a', '/lib/b']); + expect(calls(BY_PATHS)).toHaveLength(1); + + emit(Events.TrackMetadataChanged, { filePath: '/lib/a' }); + await trackCache.get(['/lib/b']); + expect(calls(BY_PATHS), 'an unchanged file is still cached').toHaveLength(1); + + await trackCache.get(['/lib/a']); + expect(calls(BY_PATHS)).toHaveLength(2); + }); + + it('patches a play count in place without a call', async () => { + await trackCache.getOne('/lib/a'); + emit(Events.TrackPlayCountChanged, { + filePath: '/lib/a', + playCount: 7, + lastPlayed: '2026-10-05 10:00:00', + }); + + const got = await trackCache.getOne('/lib/a'); + + expect([got?.PlayCount, got?.LastPlayed, calls(BY_PATHS).length]).toEqual([ + 7, + '2026-10-05 10:00:00', + 1, + ]); + }); + + it('does not cache an answer that crossed an invalidation', async () => { + let answer: (v: unknown) => void = () => undefined; + + stub(BY_PATHS, () => new Promise((resolve) => { + answer = resolve; + })); + + const pending = trackCache.getOne('/lib/a'); + + // Sent, not yet answered: the invalidation lands in between. + await flush(); + expect(calls(BY_PATHS)).toHaveLength(1); + emit(Events.TrackMetadataChanged, { batch: true }); + answer([track('/lib/a')]); + expect((await pending)?.FilePath, 'the caller still gets it').toBe('/lib/a'); + + stubLibrary(); + + await trackCache.getOne('/lib/a'); + expect(calls(BY_PATHS)).toHaveLength(2); + }); + + it('rejects every waiter in a batch when the call fails', async () => { + stubFailure(BY_PATHS); + + const results = await Promise.allSettled([ + trackCache.getOne('/lib/a'), + trackCache.getOne('/lib/b'), + ]); + + expect(results.map((r) => r.status)).toEqual(['rejected', 'rejected']); + }); +}); + +/* + * The bug half of #279: the queue, playlist and smart-playlist openers + * read `libraryStore.getCachedTracks()` and returned silently when it + * was null. The batch opener is what all three call now, so it is + * asserted against a library store that has nothing loaded. + */ +describe('opening details with no library list loaded', () => { + beforeEach(() => { + emit(Events.LibraryScanComplete); + resetHarness(); + stubLibrary(); + stub('library.Library.GetTracks', []); + }); + + it('shows the dialog with the tracks asked for', async () => { + await flush(); + expect(libraryStore.getCachedTracks()?.length ?? 0).toBe(0); + + let shown: unknown[] | null = null; + const dialog = { + showBatch: (tracks: unknown[]) => { + shown = tracks; + }, + } as unknown as TrackDetails; + + const outcome = await showBatchTrackDetailsForPaths( + () => dialog, + ['/lib/a', '/lib/b'], + () => undefined, + ); + + expect(outcome).toBe('shown'); + expect((shown ?? []).map((t) => (t as { FilePath: string }).FilePath)).toEqual([ + '/lib/a', + '/lib/b', + ]); + }); +}); + +/** Every source file, as text. */ +const SOURCES = import.meta.glob('../../src/**/*.ts', { + eager: true, + query: '?raw', + import: 'default', +}); + +/** + * Who may read the whole library's track array. The store owns it and + * the Tracks view draws it; anything else that needs a track by path + * asks `trackCache`, or the array becomes a startup dependency again. + */ +const MAY_READ_ALL_TRACKS = [ + 'store/library-store.ts', + 'store/controllers/library-controller.ts', + 'components/track-list/track-list.ts', +]; + +const ALL_TRACKS_READ = /getCachedTracks\(|libraryStore\.getTracks\(|\bcachedTracks\b/; + +describe('the whole-library track array has one reader', () => { + it('reads the sources it sweeps', () => { + expect(Object.keys(SOURCES).length).toBeGreaterThan(100); + }); + + it('is read only by the store and the Tracks view', () => { + const readers = Object.entries(SOURCES) + .filter(([, src]) => ALL_TRACKS_READ.test(src)) + .map(([path]) => path.replace('../../src/', '')) + .sort(); + + expect(readers).toEqual([...MAY_READ_ALL_TRACKS].sort()); + }); +});