From 3cddf70c4d20b7a4e9f68cdcf8021e29a8d65e7e Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Mon, 5 Oct 2026 22:59:24 -0400 Subject: [PATCH 01/10] perf(smart-playlist): suggest rule values for what was typed The rule editor's value box built its suggestions from libraryStore's whole-library arrays, which made it one more reason to load every track at startup, and left it empty when they had not loaded. SuggestSmartPlaylistValues answers with up to 50 distinct values of a field that contain the typed text, from the same joined row the rules match against, so a suggestion is always something a rule can match. yj-combobox announces its filter text so the editor can ask, debounced and cached per field and text. Refs #279 --- backend/playlist/playlist.go | 13 ++ backend/smartplaylist/smartplaylist.go | 83 ++++++++++- backend/smartplaylist/suggest_test.go | 61 ++++++++ .../yellowjacket/backend/playlist/service.ts | 8 + frontend/src/components/combobox/combobox.ts | 17 +++ .../smart-playlist-editor.ts | 137 ++++++++++++------ .../smart-playlist-suggestions.test.ts | 86 +++++++++++ 7 files changed, 356 insertions(+), 49 deletions(-) create mode 100644 backend/smartplaylist/suggest_test.go create mode 100644 frontend/test/components/smart-playlist-suggestions.test.ts diff --git a/backend/playlist/playlist.go b/backend/playlist/playlist.go index 126b531..983dc40 100644 --- a/backend/playlist/playlist.go +++ b/backend/playlist/playlist.go @@ -3125,6 +3125,19 @@ func (s *Service) EvaluateSmartPlaylist( return tracks, nil } +// SuggestSmartPlaylistValues returns values of field present in the +// library that contain needle, for the rule editor's value box. +func (s *Service) SuggestSmartPlaylistValues( + field, needle string, +) ([]string, error) { + values, err := smartplaylist.SuggestValues(s.db, field, needle) + if err != nil { + return nil, fmt.Errorf("suggest smart playlist values: %w", err) + } + + return values, nil +} + // PreviewSmartPlaylist evaluates a rule set from raw JSON without // requiring a saved playlist. This powers live preview in the rule // editor — the frontend sends rules as they are being edited and diff --git a/backend/smartplaylist/smartplaylist.go b/backend/smartplaylist/smartplaylist.go index 9705419..9b5b5b1 100644 --- a/backend/smartplaylist/smartplaylist.go +++ b/backend/smartplaylist/smartplaylist.go @@ -614,7 +614,12 @@ const leanTrackQuery = `SELECT af.file_size, af.play_count, COALESCE(af.last_played, '') AS last_played -FROM ( +FROM ` + leanTrackSource + +// leanTrackSource is the joined row every rule's column resolves +// against, aliased af. Shared by Evaluate and SuggestValues so a +// suggestion is always a value a rule on the same field can match. +const leanTrackSource = `( SELECT af.id, af.file_path, @@ -1117,3 +1122,79 @@ func splitGenres(concatenated string) []string { return strings.Split(concatenated, genreDelimiter) } + +// suggestLimit bounds one suggestion answer: a combobox shows a +// screenful, and the user narrows by typing. +const suggestLimit = 50 + +// errNoSuggestions is a field the editor does not offer values for. +var errNoSuggestions = errors.New("field has no suggestions") + +// suggestFields are the fields whose values the rule editor suggests. +// Every one is in fieldMap; numeric fields other than the years are +// ranges, where a list of every value present is no help. +var suggestFields = map[string]bool{ + "title": true, "artist": true, "album": true, "genre": true, + "composer": true, "file_type": true, "year": true, "release_year": true, +} + +// SuggestValues returns up to suggestLimit distinct values of field +// present in the library that contain needle (case-insensitively), +// sorted. +// +// The rule editor used to build these lists from libraryStore's +// whole-library arrays, which made the editor one more reason to fetch +// every track at startup (#279) — and gave it an empty list when the +// arrays had not landed. Asking for the values that match what has +// been typed is a few hundred bytes, whatever the library's size. +func SuggestValues(db *database.DB, field, needle string) ([]string, error) { + col, ok := fieldMap[field] + if !ok || !suggestFields[field] { + return nil, fmt.Errorf("%w: %q", errNoSuggestions, field) + } + + pattern := "%" + escapeLike(needle) + "%" + + // SAFETY: col comes from fieldMap, never from the caller; the + // needle is a bound parameter. + query := `SELECT DISTINCT CAST(af.` + col + ` AS TEXT) AS v FROM ` + + leanTrackSource + ` + WHERE v != '' AND v != '0' AND v LIKE ? ESCAPE '\' + ORDER BY v COLLATE NOCASE LIMIT ?` + + if field == "genre" { + query = `SELECT name FROM genres + WHERE name != '' AND name LIKE ? ESCAPE '\' + ORDER BY name COLLATE NOCASE LIMIT ?` + } + + rows, err := db.QueryContext(query, pattern, suggestLimit) + if err != nil { + return nil, fmt.Errorf("suggest %s: %w", field, err) + } + + defer func() { _ = rows.Close() }() + + values := make([]string, 0, suggestLimit) + + for rows.Next() { + var v string + if err := rows.Scan(&v); err != nil { + return nil, fmt.Errorf("suggest %s: %w", field, err) + } + + values = append(values, v) + } + + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("suggest %s: %w", field, err) + } + + return values, nil +} + +// escapeLike makes needle match itself literally inside a LIKE pattern +// with ESCAPE '\'. +func escapeLike(needle string) string { + return strings.NewReplacer(`\`, `\\`, `%`, `\%`, `_`, `\_`).Replace(needle) +} diff --git a/backend/smartplaylist/suggest_test.go b/backend/smartplaylist/suggest_test.go new file mode 100644 index 0000000..2496947 --- /dev/null +++ b/backend/smartplaylist/suggest_test.go @@ -0,0 +1,61 @@ +package smartplaylist + +import ( + "errors" + "slices" + "testing" + + "yellowjacket/backend/database" +) + +// The rule editor's value box asks the backend for what matches the +// text typed (#279), rather than building every list from the whole +// library loaded into the frontend. +func TestSuggestValues(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + seedSmartPlaylistData(t, db) + + cases := []struct { + field, needle string + want []string + }{ + // Case-insensitive substring, distinct, sorted. + {"artist", "q", []string{"QOTSA", "Queen"}}, + {"album", "OPERA", []string{"A Night at the Opera"}}, + // Genres come from the genre table, one value per genre, not + // from a concatenated per-track column. + {"genre", "rock", []string{ + "Funk Rock", "Hard Rock", "Progressive Rock", "Rock", "Stoner Rock", + }}, + // Years are numbers and are suggested as their text. + {"year", "199", []string{"1990", "1991"}}, + // The needle is literal: a LIKE wildcard in it matches only itself. + {"title", "%", []string{}}, + {"composer", "_", []string{}}, + } + + for _, tc := range cases { + got, err := SuggestValues(db, tc.field, tc.needle) + if err != nil { + t.Fatalf("SuggestValues(%q, %q): %v", tc.field, tc.needle, err) + } + + if !slices.Equal(got, tc.want) { + t.Errorf("SuggestValues(%q, %q) = %q, want %q", tc.field, tc.needle, got, tc.want) + } + } +} + +func TestSuggestValuesRefusesFieldsItDoesNotSuggest(t *testing.T) { + t.Parallel() + + db := database.NewTestDB(t) + + for _, field := range []string{"duration", "file_path", "title; DROP TABLE x"} { + if _, err := SuggestValues(db, field, ""); !errors.Is(err, errNoSuggestions) { + t.Errorf("SuggestValues(%q) err = %v, want errNoSuggestions", field, err) + } + } +} diff --git a/frontend/bindings/yellowjacket/backend/playlist/service.ts b/frontend/bindings/yellowjacket/backend/playlist/service.ts index d3cde05..efb7dec 100644 --- a/frontend/bindings/yellowjacket/backend/playlist/service.ts +++ b/frontend/bindings/yellowjacket/backend/playlist/service.ts @@ -309,6 +309,14 @@ export function SearchLibrary(query: string): $CancellablePromise<$models.Candid return $Call.ByID(3912116995, query); } +/** + * SuggestSmartPlaylistValues returns values of field present in the + * library that contain needle, for the rule editor's value box. + */ +export function SuggestSmartPlaylistValues(field: string, needle: string): $CancellablePromise { + return $Call.ByID(3718198115, field, needle); +} + /** * ToggleDefaultPlaylistTrack adds or removes a single track * from the default playlist. Returns true if the track is now diff --git a/frontend/src/components/combobox/combobox.ts b/frontend/src/components/combobox/combobox.ts index 94b166e..9df22c5 100644 --- a/frontend/src/components/combobox/combobox.ts +++ b/frontend/src/components/combobox/combobox.ts @@ -7,6 +7,11 @@ import { designTokens } from '../../styles/tokens.css'; * keyboard navigation. Accepts a flat `options` string array, filters as * the user types, and emits `combobox-change` when a value is selected. * + * It also emits `combobox-input` with `{ text }` whenever the text it + * filters by changes (typing, and the reset to empty on focus), so a + * host whose options are too many to hand over at once can fetch the + * ones matching what was typed instead (#279). + * * Key implementation detail: option `
  • ` elements use `@mousedown` with * `e.preventDefault()` so that the input's `blur` event does not close the * dropdown before the click registers. @@ -196,6 +201,7 @@ export class YjCombobox extends LitElement { this.filterText = input.value; this.open = true; this.highlightedIndex = -1; + this.announceInput(); } private handleFocus() { @@ -203,6 +209,17 @@ export class YjCombobox extends LitElement { this.filterText = ''; this.open = true; this.highlightedIndex = -1; + this.announceInput(); + } + + private announceInput() { + this.dispatchEvent( + new CustomEvent('combobox-input', { + detail: { text: this.filterText }, + bubbles: true, + composed: true, + }), + ); } private handleBlur() { diff --git a/frontend/src/components/smart-playlist-editor/smart-playlist-editor.ts b/frontend/src/components/smart-playlist-editor/smart-playlist-editor.ts index c74d668..c515810 100644 --- a/frontend/src/components/smart-playlist-editor/smart-playlist-editor.ts +++ b/frontend/src/components/smart-playlist-editor/smart-playlist-editor.ts @@ -1,8 +1,12 @@ import { LitElement, html, css, nothing } from 'lit'; import { customElement, property, state } from 'lit/decorators.js'; import * as library from '@go/library/models.js'; -import { PreviewSmartPlaylist } from '@go/playlist/service.js'; -import { libraryStore } from '@store/library-store'; +import { + PreviewSmartPlaylist, + SuggestSmartPlaylistValues, +} from '@go/playlist/service.js'; +import { list } from '@utils/binding'; +import { LRUMap } from '@utils/lru-map'; import { describeError } from '@utils/describe-error'; import { designTokens } from '../../styles/tokens.css'; import '@components/combobox/combobox.ts'; @@ -91,51 +95,23 @@ function formatOperatorLabel(op: string): string { return op.replace(/_/g, ' '); } -/** Returns autocomplete suggestions for a given field from libraryStore. */ -function getAutocompleteOptions(field: string): string[] { - switch (field) { - case 'artist': - return libraryStore.getCachedArtists()?.map((a) => a.Name) ?? []; - case 'genre': - return libraryStore.getCachedGenres()?.map((g) => g.Name) ?? []; - case 'album': - return libraryStore.getCachedAlbums()?.map((a) => a.Name) ?? []; - case 'title': { - const tracks = libraryStore.getCachedTracks(); - if (!tracks) return []; - return [...new Set(tracks.map((t) => t.TrackName).filter(Boolean))]; - } - case 'composer': { - const tracks = libraryStore.getCachedTracks(); - if (!tracks) return []; - return [...new Set(tracks.map((t) => t.Composer).filter(Boolean))]; - } - case 'file_type': { - const tracks = libraryStore.getCachedTracks(); - if (!tracks) return []; - return [...new Set(tracks.map((t) => t.FileType).filter(Boolean))]; - } - case 'year': - case 'release_year': { - // Both year fields draw suggestions from the set of years - // present in the library. The cached Track only carries the - // display (original) year, so it seeds both datalists — the - // list is just a hint, and the real filter runs server-side. - const tracks = libraryStore.getCachedTracks(); - if (!tracks) return []; - return [ - ...new Set( - tracks - .map((t) => t.Year) - .filter((y) => y > 0) - .map(String), - ), - ].sort(); - } - default: - return []; - } -} +/** + * Fields whose value box suggests values present in the library. Must + * match `suggestFields` in backend/smartplaylist; any other field gets + * a plain box. + * + * Suggestions are asked for as the user types (#279): they used to be + * built from libraryStore's whole-library arrays, which made this + * editor one more reason to load every track at startup, and gave it + * empty lists whenever those arrays had not landed. + */ +const SUGGEST_FIELDS = new Set([ + 'title', 'artist', 'album', 'genre', 'composer', 'file_type', + 'year', 'release_year', +]); + +/** How long typing must pause before suggestions are asked for. */ +const SUGGEST_DEBOUNCE_MS = 120; /** * Overrides for fields whose title-cased name would be ambiguous. The @@ -206,6 +182,17 @@ export class SmartPlaylistEditor extends LitElement { private previewTimer: ReturnType | null = null; + /** The value suggestions each rule row is showing, by row index. */ + @state() private suggestions = new Map(); + + private suggestTimer: ReturnType | null = null; + + /** Answers already fetched this session, by field and typed text. */ + private suggestCache = new LRUMap(100); + + /** The latest request per row, so a slow answer cannot overwrite a newer one. */ + private suggestWanted = new Map(); + // ── Styles ────────────────────────────────────────────────────── static override styles = [ @@ -599,6 +586,7 @@ export class SmartPlaylistEditor extends LitElement { // Reset value when field changes to avoid stale autocomplete data row.value = ''; row.value2 = ''; + this.dropSuggestions(); this.ruleRows = [...this.ruleRows]; this.onRulesChanged(); @@ -628,6 +616,56 @@ export class SmartPlaylistEditor extends LitElement { this.onRulesChanged(); } + /** Ask for the values of row `index`'s field that contain `text`. */ + private requestSuggestions(index: number, text: string) { + const field = this.ruleRows[index]?.field ?? ''; + + if (!SUGGEST_FIELDS.has(field)) return; + + const key = `${field}\u0000${text}`; + + this.suggestWanted.set(index, key); + + const cached = this.suggestCache.get(key); + + if (cached) { + this.setSuggestions(index, cached); + + return; + } + + if (this.suggestTimer !== null) clearTimeout(this.suggestTimer); + + this.suggestTimer = setTimeout(() => { + this.suggestTimer = null; + void list(SuggestSmartPlaylistValues(field, text)) + .then((values) => { + this.suggestCache.set(key, values); + + if (this.suggestWanted.get(index) === key) { + this.setSuggestions(index, values); + } + }) + .catch((err: unknown) => { + // A suggestion is a hint; the box still takes any text. + console.error('smart playlist: suggestions failed', err); + }); + }, SUGGEST_DEBOUNCE_MS); + } + + private setSuggestions(index: number, values: readonly string[]) { + const next = new Map(this.suggestions); + + next.set(index, values); + this.suggestions = next; + } + + /** Row indexes and fields changed: what was suggested no longer applies. */ + private dropSuggestions() { + this.suggestions = new Map(); + this.suggestWanted.clear(); + } + private updateValue2(index: number, newValue: string) { const row = this.ruleRows[index]; if (!row) return; @@ -644,6 +682,7 @@ export class SmartPlaylistEditor extends LitElement { private removeRule(index: number) { if (this.ruleRows.length <= 1) return; this.ruleRows = this.ruleRows.filter((_, i) => i !== index); + this.dropSuggestions(); this.onRulesChanged(); } @@ -892,7 +931,9 @@ export class SmartPlaylistEditor extends LitElement { ` : html` ) => + this.requestSuggestions(index, e.detail.text)} .value=${row.value} placeholder=${isAnyOf ? 'Comma-separated values' diff --git a/frontend/test/components/smart-playlist-suggestions.test.ts b/frontend/test/components/smart-playlist-suggestions.test.ts new file mode 100644 index 0000000..0c113da --- /dev/null +++ b/frontend/test/components/smart-playlist-suggestions.test.ts @@ -0,0 +1,86 @@ +/** + * The rule editor's value box suggests values from the library by + * asking for the ones that match what has been typed (#279). + * + * It used to build the lists from `libraryStore`'s whole-library + * arrays: one more reason to load every track at startup, and an empty + * list whenever they had not loaded. These tests stub *no* library + * collection — what is asserted is that the suggestions arrive anyway, + * from the one call made for them. + */ +import { describe, expect, it, beforeEach } from 'vitest'; + +import '@components/smart-playlist-editor/smart-playlist-editor'; +import type { YjCombobox } from '@components/combobox/combobox'; +import { calls, resetHarness, stub } from '@test/support/harness'; +import { fixture, shadow, shadowAll } from '@test/support/render'; +import type { LitElement } from 'lit'; + +const SUGGEST = 'playlist.Service.SuggestSmartPlaylistValues'; + +async function editor(field: string): Promise { + return fixture('smart-playlist-editor', { + rules: JSON.stringify({ rules: [{ field, operator: 'contains', value: '' }] }), + }); +} + +/** The row's second combobox: the value box (the first picks the field). */ +function valueBox(el: LitElement): YjCombobox { + return shadowAll(el, 'yj-combobox')[1]!; +} + +async function type(box: YjCombobox, text: string): Promise { + const input = shadow(box, 'input')!; + + input.focus(); + input.value = text; + input.dispatchEvent(new Event('input', { bubbles: true })); + await box.updateComplete; +} + +describe('smart playlist value suggestions', () => { + beforeEach(() => { + resetHarness(); + stub(SUGGEST, (field: string, needle: string) => + [`${field}:${needle}:1`, `${field}:${needle}:2`], + ); + }); + + it('asks for the values matching what was typed, once typing pauses', async () => { + const el = await editor('artist'); + const box = valueBox(el); + + await type(box, 'q'); + await type(box, 'qu'); + + await expect.poll(() => box.options).toEqual(['artist:qu:1', 'artist:qu:2']); + // Debounced: the pause after "qu" asked, the keystroke before did not. + expect(calls(SUGGEST).map((c) => c.args)).toEqual([['artist', 'qu']]); + }); + + it('answers a repeat from what it already fetched', async () => { + const el = await editor('album'); + const box = valueBox(el); + + await type(box, 'x'); + await expect.poll(() => box.options).toEqual(['album:x:1', 'album:x:2']); + await type(box, 'xy'); + await expect.poll(() => box.options).toEqual(['album:xy:1', 'album:xy:2']); + await type(box, 'x'); + + expect(box.options).toEqual(['album:x:1', 'album:x:2']); + expect(calls(SUGGEST)).toHaveLength(2); + }); + + it('asks nothing for a field that has no suggestions', async () => { + const el = await editor('duration'); + const boxes = shadowAll(el, 'yj-combobox'); + + // A numeric range field may render no value combobox at all; if it + // does, typing in it must not ask. + if (boxes[1]) await type(boxes[1], '3'); + await new Promise((r) => setTimeout(r, 250)); + + expect(calls(SUGGEST)).toHaveLength(0); + }); +}); -- 2.54.0 From 4a9fe1d20702d256745a41ba2b49a2e8ba18b072 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Mon, 5 Oct 2026 23:01:37 -0400 Subject: [PATCH 02/10] fix(library): open track details by path, not from the whole library Track details from the queue, a playlist or a smart playlist looked the track up in libraryStore's whole-library array, and returned silently when it had not loaded, so the menu item did nothing. That dependency is also why every track was fetched eagerly at startup (20.5 MB at 26k tracks). GetTracksByPaths answers for the paths in hand, and trackCache holds the answers: bounded, coalesced into one call per task, and forgetting what the retag, removal and scan events name. Every opener, and track-details' own refresh after a save, go through it. A source sweep pins the whole-library array to the store and the Tracks view. Closes #279 --- backend/database/sql/queries/audio_files.sql | 3 + .../database/sql/sqlcgen/audio_files.sql.go | 65 +++++ backend/library/query.go | 42 +++ backend/library/trackpaths_test.go | 64 +++++ .../yellowjacket/backend/library/library.ts | 15 ++ .../playlist-details/playlist-details.ts | 70 +---- .../src/components/queue-panel/queue-panel.ts | 86 +------ .../smart-playlist-details.ts | 70 +---- .../components/track-details/track-details.ts | 72 ++---- frontend/src/store/track-cache.ts | 240 ++++++++++++++++++ frontend/src/utils/track-details-opener.ts | 60 ++++- .../components/explore-track-details.test.ts | 15 +- .../components/lazy-track-details.test.ts | 6 +- frontend/test/stores/track-cache.test.ts | 209 +++++++++++++++ 14 files changed, 747 insertions(+), 270 deletions(-) create mode 100644 backend/library/trackpaths_test.go create mode 100644 frontend/src/store/track-cache.ts create mode 100644 frontend/test/stores/track-cache.test.ts 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()); + }); +}); -- 2.54.0 From 7a1412383f70f80ce16494e78a494f92fedc003c Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Mon, 5 Oct 2026 23:30:37 -0400 Subject: [PATCH 03/10] perf(library): send the track list as a dictionary-encoded column table GetTracks answered with one JSON object per track: 20.5 MB at 26k tracks, ~350 bytes a row of repeated key names, four cover URLs that are identical across an album, and the album, artist and genre strings repeated on every track. Encoding it was ~170 MB of transient Go allocation, and parsing it the WebView's memory peak. GetTrackTable replaces it: one array per column, every repeated string stored once and sent as an index, genre lists interned, and only the fields the Tracks view reads. LastPlayed and the three larger cover tiers are left to the details dialog, which now fetches whole tracks by path. Each row still goes through trackFromRow, so this is an encoding of the one projection, not a second one. 167 bytes a track against 929 in the test library; a Go test holds it under a quarter of the object encoding, and a Vitest test reads the Go columns and the generated Track interface and fails if they drift. An empty library is now an empty table rather than the "no tracks in library" error the old binding returned. Closes #281 --- .../references/android-tier.md | 2 +- backend/library/query.go | 27 --- backend/library/scan_fixtures_test.go | 8 +- backend/library/tracktable.go | 190 +++++++++++++++++ backend/library/tracktable_test.go | 198 ++++++++++++++++++ e2e/perf/measure.mjs | 26 ++- e2e/specs/phone-now-playing.spec.ts | 14 +- e2e/specs/phone-progress-line.spec.ts | 7 +- e2e/specs/phone-shell.spec.ts | 15 +- e2e/specs/queue-reorder.spec.ts | 14 +- e2e/specs/queue-selection.spec.ts | 17 +- e2e/support/fixtures.ts | 30 +++ .../yellowjacket/backend/library/index.ts | 3 +- .../yellowjacket/backend/library/library.ts | 14 +- .../yellowjacket/backend/library/models.ts | 57 +++++ frontend/src/components/track-list/columns.ts | 20 +- .../components/track-list/search-ranking.ts | 12 +- .../src/components/track-list/track-list.ts | 107 +++------- .../store/controllers/library-controller.ts | 5 +- frontend/src/store/library-store.ts | 28 ++- frontend/src/utils/binding.ts | 8 + frontend/src/utils/track-index.ts | 28 +-- frontend/src/utils/track-table.ts | 128 +++++++++++ .../test/components/album-card-year.test.ts | 3 +- .../test/components/album-dropdown.test.ts | 5 +- frontend/test/components/aria-tail.test.ts | 11 +- frontend/test/components/art-prefetch.test.ts | 3 +- .../test/components/card-grid-repaint.test.ts | 3 +- frontend/test/components/chrome.test.ts | 2 +- .../components/detail-touch-targets.test.ts | 2 +- frontend/test/components/empty-states.test.ts | 5 +- .../test/components/keyboard-reach.test.ts | 3 +- .../test/components/library-status.test.ts | 3 +- .../test/components/list-render-cost.test.ts | 15 +- frontend/test/components/smoke.test.ts | 2 +- .../test/components/touch-selection.test.ts | 5 +- frontend/test/components/touch-swipe.test.ts | 3 +- .../test/components/view-lifecycle.test.ts | 2 +- frontend/test/setup.ts | 2 +- frontend/test/stores/library-store.test.ts | 39 ++-- frontend/test/support/track-table.ts | 86 ++++++++ frontend/test/utils/track-table.test.ts | 134 ++++++++++++ 42 files changed, 1011 insertions(+), 275 deletions(-) create mode 100644 backend/library/tracktable.go create mode 100644 backend/library/tracktable_test.go create mode 100644 frontend/src/utils/track-table.ts create mode 100644 frontend/test/support/track-table.ts create mode 100644 frontend/test/utils/track-table.test.ts diff --git a/.pi/skills/yellowjacket-dev/references/android-tier.md b/.pi/skills/yellowjacket-dev/references/android-tier.md index 1af553a..e9bd4f5 100644 --- a/.pi/skills/yellowjacket-dev/references/android-tier.md +++ b/.pi/skills/yellowjacket-dev/references/android-tier.md @@ -647,7 +647,7 @@ window.__yj = { call(name, args) { That turns the device into a tier that can be *driven* rather than only looked at — `__yj.call("player.Player.LoadFile", [path])` and `__yj.call("library.Library.AddLibrary", ["/sdcard/Music/..."])` are how -#53 was measured. Names are the Go ones (`GetTracks`, not +#53 was measured. Names are the Go ones (`GetTrackTable`, not `GetAllTracks`); an unknown one comes back as a plain `unknown bound method name`, so a wrong guess is loud. diff --git a/backend/library/query.go b/backend/library/query.go index b1574da..02259b3 100644 --- a/backend/library/query.go +++ b/backend/library/query.go @@ -2,7 +2,6 @@ package library import ( "database/sql" - "errors" "fmt" "os" "path/filepath" @@ -18,8 +17,6 @@ import ( // searchTrackLimit bounds an FTS search's result set. const searchTrackLimit = 500 -var errNoTracksInLibrary = errors.New("no tracks in library") - // Track is one audio file with everything a list needs to draw it. type Track struct { TrackName string @@ -169,30 +166,6 @@ func (l *Library) GetTrackMBIDs(filePath string) TrackMBIDs { } } -// GetTracks returns every track in a library, or in all of them when -// libraryID is 0. -// -// The library id is a parameter rather than a second method because the -// two used to be separate queries, separate bindings and a branch at -// every call site - and the scoped form costs nothing (measured: 23 ms -// against 21 ms over 26k rows). -func (l *Library) GetTracks(libraryID int64) ([]Track, error) { - rows, err := l.db.ReadQueries.GetTracks(l.ctx, libraryID) - if err != nil { - l.logger.Error("could not retrieve audio files", "error", err) - - return nil, fmt.Errorf("could not get tracks: %w", err) - } - - l.logger.Info("audio file list", "count", len(rows), "libraryID", libraryID) - - if len(rows) == 0 { - return nil, errNoTracksInLibrary - } - - return tracksFromRows(rows), nil -} - // pathLookupChunk bounds the paths bound into one IN (...) query, well // under SQLite's bind-variable limit. const pathLookupChunk = 500 diff --git a/backend/library/scan_fixtures_test.go b/backend/library/scan_fixtures_test.go index 8f43720..e1b7c51 100644 --- a/backend/library/scan_fixtures_test.go +++ b/backend/library/scan_fixtures_test.go @@ -58,15 +58,15 @@ func TestScan_FixtureLibraryLeavesNothingBehind(t *testing.T) { t.Skip("fixture library is empty; run make testdata") } - tracks, err := lib.GetTracks(0) + table, err := lib.GetTrackTable(0) if err != nil { - t.Fatalf("GetTracks: %v", err) + t.Fatalf("GetTrackTable: %v", err) } // One track per file: the projection cannot multiply rows, because // there is no join table left to multiply them. - if int64(len(tracks)) != files { - t.Errorf("GetTracks returned %d rows for %d files", len(tracks), files) + if int64(len(table.FilePath)) != files { + t.Errorf("GetTrackTable returned %d rows for %d files", len(table.FilePath), files) } // Nothing shared outlives what refers to it. diff --git a/backend/library/tracktable.go b/backend/library/tracktable.go new file mode 100644 index 0000000..bc701c7 --- /dev/null +++ b/backend/library/tracktable.go @@ -0,0 +1,190 @@ +package library + +import ( + "fmt" + "strconv" + "strings" +) + +// TrackTable is every track in a library as the Tracks view uses it: +// one array per column, and every repeated string stored once (#281). +// +// GetTracks used to answer with one object per track, which at 26 138 +// tracks was 20.5 MB of JSON — ~350 bytes a row of key names, four +// cover URLs identical across an album, and artist, album and genre +// strings repeated on every track of the album. Encoding it cost the +// backend ~170 MB of transient allocation and parsing it was the +// WebView's peak. Here the keys appear once, a repeated string is a +// small integer, and the columns the Tracks view does not read are not +// sent at all: LastPlayed and the three larger cover tiers belong to +// the details dialog, which fetches whole tracks by path. +// +// The projection is still trackFromRow's — each row goes through it — +// so this is an encoding of a Track, never a second description of +// one. frontend/src/utils/track-table.ts is the only decoder. +type TrackTable struct { + // Strings holds every distinct string value; a string column holds + // indexes into it. Index 0 is always "". + Strings []string `json:"strings"` + // GenreSets holds every distinct genre list, as indexes into + // Strings; Genre holds an index into it per track. + GenreSets [][]uint32 `json:"genreSets"` + + FilePath []string `json:"filePath"` + TrackName []uint32 `json:"trackName"` + ArtistName []uint32 `json:"artistName"` + Album []uint32 `json:"album"` + Composer []uint32 `json:"composer"` + FileType []uint32 `json:"fileType"` + Genre []uint32 `json:"genre"` + ArtistMBID []uint32 `json:"artistMbid"` + ReleaseGroupMBID []uint32 `json:"releaseGroupMbid"` + RecordingMBID []uint32 `json:"recordingMbid"` + CoverArtSmall []uint32 `json:"coverArtSmall"` + + // LengthMs is Track.TrackLength as the number it encodes. + LengthMs []int64 `json:"lengthMs"` + TrackNumber []int64 `json:"trackNumber"` + DiscNumber []int64 `json:"discNumber"` + Year []int64 `json:"year"` + SampleRate []int64 `json:"sampleRate"` + BitDepth []int64 `json:"bitDepth"` + Channels []int64 `json:"channels"` + Bitrate []int64 `json:"bitrate"` + FileSize []int64 `json:"fileSize"` + PlayCount []int64 `json:"playCount"` +} + +// trackTableBuilder interns strings and genre lists while rows are +// appended. +type trackTableBuilder struct { + table TrackTable + strings map[string]uint32 + genres map[string]uint32 +} + +func newTrackTableBuilder(capacity int) *trackTableBuilder { + b := &trackTableBuilder{ + strings: map[string]uint32{"": 0}, + genres: map[string]uint32{}, + } + + t := &b.table + t.Strings = []string{""} + t.FilePath = make([]string, 0, capacity) + + for _, col := range b.stringColumns() { + *col = make([]uint32, 0, capacity) + } + + for _, col := range b.intColumns() { + *col = make([]int64, 0, capacity) + } + + t.Genre = make([]uint32, 0, capacity) + + return b +} + +// stringColumns are the interned columns, in one place so the builder +// cannot allocate one and forget to fill it. +func (b *trackTableBuilder) stringColumns() []*[]uint32 { + t := &b.table + + return []*[]uint32{ + &t.TrackName, &t.ArtistName, &t.Album, &t.Composer, &t.FileType, + &t.ArtistMBID, &t.ReleaseGroupMBID, &t.RecordingMBID, &t.CoverArtSmall, + } +} + +func (b *trackTableBuilder) intColumns() []*[]int64 { + t := &b.table + + return []*[]int64{ + &t.LengthMs, &t.TrackNumber, &t.DiscNumber, &t.Year, &t.SampleRate, + &t.BitDepth, &t.Channels, &t.Bitrate, &t.FileSize, &t.PlayCount, + } +} + +func (b *trackTableBuilder) intern(s string) uint32 { + if i, ok := b.strings[s]; ok { + return i + } + + i := uint32(len(b.table.Strings)) //nolint:gosec // bounded by the row count + + b.table.Strings = append(b.table.Strings, s) + b.strings[s] = i + + return i +} + +func (b *trackTableBuilder) internGenres(genres []string) uint32 { + key := strings.Join(genres, genreDelimiter) + + if i, ok := b.genres[key]; ok { + return i + } + + set := make([]uint32, len(genres)) + for j, g := range genres { + set[j] = b.intern(g) + } + + i := uint32(len(b.table.GenreSets)) //nolint:gosec // bounded by the row count + + b.table.GenreSets = append(b.table.GenreSets, set) + b.genres[key] = i + + return i +} + +func (b *trackTableBuilder) add(tr Track) error { + lengthMs, err := strconv.ParseInt(tr.TrackLength, 10, 64) + if err != nil { + return fmt.Errorf("track %q length %q: %w", tr.FilePath, tr.TrackLength, err) + } + + t := &b.table + t.FilePath = append(t.FilePath, tr.FilePath) + + strs := []string{ + tr.TrackName, tr.ArtistName, tr.Album, tr.Composer, tr.FileType, + tr.ArtistMBID, tr.ReleaseGroupMBID, tr.RecordingMBID, tr.CoverArtSmall, + } + for i, col := range b.stringColumns() { + *col = append(*col, b.intern(strs[i])) + } + + ints := []int64{ + lengthMs, tr.TrackNumber, tr.DiscNumber, tr.Year, tr.SampleRate, + tr.BitDepth, tr.Channels, tr.Bitrate, tr.FileSize, tr.PlayCount, + } + for i, col := range b.intColumns() { + *col = append(*col, ints[i]) + } + + t.Genre = append(t.Genre, b.internGenres(tr.Genre)) + + return nil +} + +// GetTrackTable returns every track in a library, or in all of them +// when libraryID is 0, as a TrackTable. An empty library is an empty +// table, not an error. +func (l *Library) GetTrackTable(libraryID int64) (TrackTable, error) { + rows, err := l.db.ReadQueries.GetTracks(l.ctx, libraryID) + if err != nil { + return TrackTable{}, fmt.Errorf("could not get tracks: %w", err) + } + + b := newTrackTableBuilder(len(rows)) + + for i := range rows { + if err := b.add(trackFromRow(rows[i])); err != nil { + return TrackTable{}, err + } + } + + return b.table, nil +} diff --git a/backend/library/tracktable_test.go b/backend/library/tracktable_test.go new file mode 100644 index 0000000..864426e --- /dev/null +++ b/backend/library/tracktable_test.go @@ -0,0 +1,198 @@ +package library + +import ( + "encoding/json" + "fmt" + "strconv" + "testing" + + "yellowjacket/backend/database" +) + +// seedTableLibrary seeds n albums of perAlbum tracks each, with a cover +// on every album, so the table has the repetition it exists to remove. +func seedTableLibrary(t *testing.T, lib *Library, albums, perAlbum int) { + t.Helper() + + for a := range albums { + for n := range perAlbum { + database.InsertTestTrack(t, lib.db, database.TestTrack{ + FilePath: fmt.Sprintf( + "/music/artist-%d/album-%d/%02d - Some Track Title.flac", a%7, a, n+1, + ), + Title: fmt.Sprintf("Some Track Title %d", n+1), + Artist: fmt.Sprintf("Artist %d", a%7), + ArtistMBID: fmt.Sprintf("0b7a8d2e-0000-4000-8000-%012d", a%7), + Album: fmt.Sprintf("Album Name %d", a), + AlbumMBID: fmt.Sprintf("1c7a8d2e-0000-4000-8000-%012d", a), + RecordingMBID: fmt.Sprintf("2d7a8d2e-0000-4000-8000-%012d", a*100+n), + Genres: []string{"Ambient", fmt.Sprintf("Genre %d", a%3)}, + TrackNumber: int64(n + 1), + DiscNumber: 1, + Year: 2000 + int64(a%20), + LengthMs: 200_000 + int64(n), + PlayCount: int64(n), + }) + } + + res, err := lib.db.ExecContext( + "INSERT INTO cover_art (file_path, mime_type) VALUES (?, 'image/jpeg')", + fmt.Sprintf("/data/covers/%064d.jpg", a), + ) + if err != nil { + t.Fatalf("seed cover: %v", err) + } + + coverID, _ := res.LastInsertId() + + if _, err := lib.db.ExecContext( + "UPDATE albums SET cover_art_id = ? WHERE name = ?", + coverID, fmt.Sprintf("Album Name %d", a), + ); err != nil { + t.Fatalf("seed album cover: %v", err) + } + } +} + +// decodeTable is the Go mirror of frontend/src/utils/track-table.ts, +// for asserting the encoding loses nothing it claims to carry. +func decodeTable(t *testing.T, tbl TrackTable) []Track { + t.Helper() + + str := func(col []uint32, i int) string { return tbl.Strings[col[i]] } + tracks := make([]Track, len(tbl.FilePath)) + + for i := range tracks { + var genres []string + for _, g := range tbl.GenreSets[tbl.Genre[i]] { + genres = append(genres, tbl.Strings[g]) + } + + tracks[i] = Track{ + FilePath: tbl.FilePath[i], + TrackName: str(tbl.TrackName, i), + ArtistName: str(tbl.ArtistName, i), + Album: str(tbl.Album, i), + Composer: str(tbl.Composer, i), + FileType: str(tbl.FileType, i), + ArtistMBID: str(tbl.ArtistMBID, i), + ReleaseGroupMBID: str(tbl.ReleaseGroupMBID, i), + RecordingMBID: str(tbl.RecordingMBID, i), + CoverArtSmall: str(tbl.CoverArtSmall, i), + Genre: genres, + TrackLength: strconv.FormatInt(tbl.LengthMs[i], 10), + TrackNumber: tbl.TrackNumber[i], + DiscNumber: tbl.DiscNumber[i], + Year: tbl.Year[i], + SampleRate: tbl.SampleRate[i], + BitDepth: tbl.BitDepth[i], + Channels: tbl.Channels[i], + Bitrate: tbl.Bitrate[i], + FileSize: tbl.FileSize[i], + PlayCount: tbl.PlayCount[i], + } + } + + return tracks +} + +// #281: the table is an encoding of trackFromRow's Track, minus the +// fields the Tracks view does not read. Decoding it must give back +// exactly that, row for row. +func TestTrackTableRoundTrip(t *testing.T) { + t.Parallel() + + lib, _ := setupTestLibrary(t) + seedTableLibrary(t, lib, 6, 4) + + tbl, err := lib.GetTrackTable(0) + if err != nil { + t.Fatalf("GetTrackTable: %v", err) + } + + rows, err := lib.db.ReadQueries.GetTracks(lib.ctx, 0) + if err != nil { + t.Fatalf("GetTracks: %v", err) + } + + if len(rows) != 24 || len(tbl.FilePath) != len(rows) { + t.Fatalf("table has %d rows, query %d, want 24", len(tbl.FilePath), len(rows)) + } + + got := decodeTable(t, tbl) + + for i, row := range rows { + want := trackFromRow(row) + // Not carried: the details dialog fetches these by path. + want.LastPlayed, want.CoverArtPath, want.CoverArtMedium, want.CoverArtLarge = "", "", "", "" + + if fmt.Sprintf("%+v", got[i]) != fmt.Sprintf("%+v", want) { + t.Fatalf("row %d:\n got %+v\nwant %+v", i, got[i], want) + } + } + + // It was the cover, genre and name repetition that was paid for; a + // table that interned nothing would still round-trip. + if len(tbl.GenreSets) != 3 { + t.Errorf("genre sets = %d, want 3 distinct lists", len(tbl.GenreSets)) + } + + if a, b := tbl.CoverArtSmall[0], tbl.CoverArtSmall[1]; a != b || tbl.Strings[a] == "" { + t.Errorf("two tracks of one album hold cover indexes %d and %d", a, b) + } +} + +// The point of the table, pinned: the same rows encode to a fraction of +// the object-per-track JSON they replace. A new column has to fit +// under this or raise it on purpose. +func TestTrackTableSizeBudget(t *testing.T) { + t.Parallel() + + lib, _ := setupTestLibrary(t) + seedTableLibrary(t, lib, 40, 10) + + tbl, err := lib.GetTrackTable(0) + if err != nil { + t.Fatalf("GetTrackTable: %v", err) + } + + rows, err := lib.db.ReadQueries.GetTracks(lib.ctx, 0) + if err != nil { + t.Fatalf("GetTracks: %v", err) + } + + asTable, err := json.Marshal(tbl) + if err != nil { + t.Fatal(err) + } + + asObjects, err := json.Marshal(tracksFromRows(rows)) + if err != nil { + t.Fatal(err) + } + + perTrack := len(asTable) / len(rows) + ratio := float64(len(asTable)) / float64(len(asObjects)) + + t.Logf("table %d B (%d B/track), objects %d B, ratio %.2f", + len(asTable), perTrack, len(asObjects), ratio) + + if ratio > 0.25 { + t.Errorf("table is %.0f%% of the object encoding, budget 25%%", ratio*100) + } +} + +func TestTrackTableEmptyLibraryIsAnAnswer(t *testing.T) { + t.Parallel() + + lib, _ := setupTestLibrary(t) + + tbl, err := lib.GetTrackTable(0) + if err != nil { + t.Fatalf("an empty library is an empty table, not an error: %v", err) + } + + if len(tbl.FilePath) != 0 || len(tbl.Strings) != 1 { + t.Errorf("empty table = %+v", tbl) + } +} diff --git a/e2e/perf/measure.mjs b/e2e/perf/measure.mjs index ac4b125..0cff8d6 100644 --- a/e2e/perf/measure.mjs +++ b/e2e/perf/measure.mjs @@ -224,7 +224,13 @@ async function measureStartup(page) { scriptBytesBeforePaint, requests: res.length, crossOriginRequests: crossOrigin.length, - crossOriginHosts: [...new Set(crossOrigin.map((u) => new URL(u).host))], + crossOriginHosts: [...new Set(crossOrigin.map((u) => { + try { + return new URL(u).host; + } catch { + return u; + } + }))], }; }); @@ -337,7 +343,8 @@ async function measureTrackChange(page) { await ev.call('queue.Queue.Clear', [], 10000).catch(() => {}); - const tracks = await ev.call('library.Library.GetAllTracks', [], 60000); + // The columnar list (#281): only the paths are needed here. + const tracks = ((await ev.call('library.Library.GetTrackTable', [0], 60000))?.filePath ?? []).map((FilePath) => ({ FilePath })); const paths = (tracks ?? []).slice(0, 4).map((t) => t.FilePath); if (paths.length < 2) return { error: 'library too small to measure' }; @@ -435,7 +442,8 @@ async function measureFavouriteToggle(page) { const ev = window.__yjEvents; const perf = window.__yjPerf; - const tracks = await ev.call('library.Library.GetAllTracks', [], 60000); + // The columnar list (#281): only the paths are needed here. + const tracks = ((await ev.call('library.Library.GetTrackTable', [0], 60000))?.filePath ?? []).map((FilePath) => ({ FilePath })); const paths = (tracks ?? []).map((t) => t.FilePath); if (paths.length < per * count) { return { error: `library too small: ${paths.length} tracks` }; @@ -671,7 +679,8 @@ async function measurePlaylistOpen(page, client) { let pl = (existing ?? []).find((p) => (p.Name ?? p.name) === name); if (!pl) { - const tracks = await ev.call('library.Library.GetAllTracks', [], 60000); + // The columnar list (#281): only the paths are needed here. + const tracks = ((await ev.call('library.Library.GetTrackTable', [0], 60000))?.filePath ?? []).map((FilePath) => ({ FilePath })); const paths = (tracks ?? []).map((t) => t.FilePath); if (paths.length < n) return { error: `library too small: ${paths.length}` }; @@ -1600,7 +1609,8 @@ async function measurePlayerBarPass(page) { // Stage a loaded track. Deliberately the same first tracks the // track-change measurement already played, so this warms no cover // art that a later measurement counts requests for. - const tracks = await ev.call('library.Library.GetAllTracks', [], 60000); + // The columnar list (#281): only the paths are needed here. + const tracks = ((await ev.call('library.Library.GetTrackTable', [0], 60000))?.filePath ?? []).map((FilePath) => ({ FilePath })); const paths = (tracks ?? []).slice(0, 4).map((t) => t.FilePath); if (paths.length < 2) return { error: 'library too small to measure' }; @@ -2000,7 +2010,11 @@ function compare(a, b) { const p = resolve(OUT_DIR, `${l}.json`); if (!existsSync(p)) throw new Error(`no measurement labelled '${l}' at ${p}`); - return JSON.parse(readFileSync(p, 'utf8')); + try { + return JSON.parse(readFileSync(p, 'utf8')); + } catch (err) { + throw new Error(`measurement '${l}' at ${p} is not valid JSON: ${err.message}`); + } }; console.log(table([load(a), load(b)])); diff --git a/e2e/specs/phone-now-playing.spec.ts b/e2e/specs/phone-now-playing.spec.ts index 29d1595..b3cba16 100644 --- a/e2e/specs/phone-now-playing.spec.ts +++ b/e2e/specs/phone-now-playing.spec.ts @@ -1,4 +1,4 @@ -import { test, expect } from '../support/fixtures.js'; +import { test, expect, libraryTracks } from '../support/fixtures.js'; /** * Now Playing on a short screen (#51). @@ -57,19 +57,15 @@ const REFLOW_AT = 500; /** Put a track in the player, so the view has art and names to lay out. */ async function stageATrack(page: Page): Promise { - await page.evaluate(async () => { - const tracks = (await window.__yjEvents.call( - 'library.Library.GetTracks', - [0], - 10_000, - )) as { FilePath: string }[]; + const paths = (await libraryTracks(page)).slice(0, 4).map((t) => t.FilePath); + await page.evaluate(async (paths) => { await window.__yjEvents.call( 'queue.Queue.SetQueue', - [tracks.slice(0, 4).map((t) => t.FilePath), 0, false, { type: '', id: 0, label: '' }], + [paths, 0, false, { type: '', id: 0, label: '' }], 10_000, ); - }); + }, paths); } /** Open the full-screen view and wait for the shell to say so. */ diff --git a/e2e/specs/phone-progress-line.spec.ts b/e2e/specs/phone-progress-line.spec.ts index 006e79f..47b8182 100644 --- a/e2e/specs/phone-progress-line.spec.ts +++ b/e2e/specs/phone-progress-line.spec.ts @@ -1,4 +1,5 @@ import { + libraryTracks, test, expect, callBinding, @@ -49,11 +50,7 @@ async function rectOf(app: Page, selector: string): Promise { * three rectangles. */ async function play(app: Page): Promise { - const tracks = await callBinding<{ FilePath: string; TrackName: string }[]>( - app, - 'library.Library.GetTracks', - [0], - ); + const tracks = await libraryTracks(app); // `TrackName`, not `Title`: that is what the library model calls it. const long = tracks.find((t) => t.TrackName === LONG_TRACK); diff --git a/e2e/specs/phone-shell.spec.ts b/e2e/specs/phone-shell.spec.ts index 7653c34..5ed5721 100644 --- a/e2e/specs/phone-shell.spec.ts +++ b/e2e/specs/phone-shell.spec.ts @@ -1,4 +1,4 @@ -import { test, expect, LONG_TRACK } from '../support/fixtures.js'; +import { test, expect, LONG_TRACK, libraryTracks } from '../support/fixtures.js'; /** * The phone shell (plan 016 B2, phase 1). @@ -235,15 +235,8 @@ test.describe('the shell on a phone', () => { // nothing in particular and picked a 2-second track, which had // finished before the assertions ran. The placeholder check below // is what actually holds the property this test needs. - const started = await app.evaluate(async (longTitle) => { - const tracks = (await window.__yjEvents.call( - 'library.Library.GetTracks', - [0], - 10_000, - )) as { FilePath: string; TrackName: string }[]; - - const bare = tracks.find((t) => t.TrackName === longTitle); - + const bare = (await libraryTracks(app)).find((t) => t.TrackName === LONG_TRACK); + const started = await app.evaluate(async (bare) => { if (!bare) return null; await window.__yjEvents.call( @@ -254,7 +247,7 @@ test.describe('the shell on a phone', () => { await window.__yjEvents.call('queue.Queue.Play', [], 5_000); return bare.TrackName; - }, LONG_TRACK); + }, bare); expect(started).toBe(LONG_TRACK); diff --git a/e2e/specs/queue-reorder.spec.ts b/e2e/specs/queue-reorder.spec.ts index cd37cf7..b8f19a5 100644 --- a/e2e/specs/queue-reorder.spec.ts +++ b/e2e/specs/queue-reorder.spec.ts @@ -1,4 +1,5 @@ import { + libraryTracks, test, expect, callBinding, @@ -30,18 +31,7 @@ async function order(app: Page): Promise { } async function queueFourAndOpen(app: Page): Promise { - const paths: string[] = await app.evaluate(async () => { - // One argument, and 0 means every library: the scoped and - // unscoped list queries collapsed into one when the schema did - // (plan 013 R3), so `GetTracks()` no longer exists to call. - const tracks = await window.__yjEvents.call( - 'library.Library.GetTracks', - [0], - 10_000, - ); - - return (tracks as { FilePath: string }[]).slice(0, 4).map((t) => t.FilePath); - }); + const paths = (await libraryTracks(app)).slice(0, 4).map((t) => t.FilePath); await callBinding(app, 'queue.Queue.SetQueue', [paths, 0, false, NO_QUEUE_SOURCE]); diff --git a/e2e/specs/queue-selection.spec.ts b/e2e/specs/queue-selection.spec.ts index 0e5023a..223056f 100644 --- a/e2e/specs/queue-selection.spec.ts +++ b/e2e/specs/queue-selection.spec.ts @@ -1,4 +1,5 @@ import { + libraryTracks, test, expect, callBinding, @@ -86,15 +87,11 @@ const selected = (app: Page) => * is read back. */ async function queueSixAndOpen(app: Page): Promise { - const paths = await app.evaluate(async (longTitle) => { - // `TrackName`, not `Title`: the library model names it after the - // tag, and the *queue* is what calls it `title`. - const tracks = (await window.__yjEvents.call( - 'library.Library.GetTracks', - [0], - 10_000, - )) as { FilePath: string; TrackName: string; Album: string }[]; - + // `TrackName`, not `Title`: the library model names it after the + // tag, and the *queue* is what calls it `title`. + const tracks = await libraryTracks(app); + const longTitle = LONG_TRACK; + const paths = (() => { const long = tracks.find((t) => t.TrackName === longTitle); /** @@ -131,7 +128,7 @@ async function queueSixAndOpen(app: Page): Promise { long!.FilePath, ...rest.slice(3).map((t) => t.FilePath), ]; - }, LONG_TRACK); + })(); await callBinding(app, 'queue.Queue.SetQueue', [ paths, diff --git a/e2e/support/fixtures.ts b/e2e/support/fixtures.ts index e495687..ddd6f38 100644 --- a/e2e/support/fixtures.ts +++ b/e2e/support/fixtures.ts @@ -96,6 +96,36 @@ export async function callBinding( ) as Promise; } +/** A library track as the specs use it: the path and its names. */ +export interface LibraryTrack { + FilePath: string; + TrackName: string; + Album: string; +} + +/** + * Every track in the library, in the order the backend lists them. + * + * The list arrives as a columnar `TrackTable` since #281 (repeated + * strings sent once, as indexes into `strings`), so it cannot be read + * as an array of tracks any more. This reads the three columns the + * specs use; `frontend/src/utils/track-table.ts` is the real decoder. + */ +export async function libraryTracks(page: Page): Promise { + const t = await callBinding<{ + strings: string[]; + filePath: string[]; + trackName: number[]; + album: number[]; + }>(page, 'library.Library.GetTrackTable', [0]); + + return (t.filePath ?? []).map((FilePath, i) => ({ + FilePath, + TrackName: t.strings[t.trackName[i]!] ?? '', + Album: t.strings[t.album[i]!] ?? '', + })); +} + /** * The binding calls the *app* made, newest last, as `pkg.Type.Method`. * diff --git a/frontend/bindings/yellowjacket/backend/library/index.ts b/frontend/bindings/yellowjacket/backend/library/index.ts index d18a806..e974b53 100644 --- a/frontend/bindings/yellowjacket/backend/library/index.ts +++ b/frontend/bindings/yellowjacket/backend/library/index.ts @@ -18,5 +18,6 @@ export type { ScanMetrics, ScanWarning, Track, - TrackMBIDs + TrackMBIDs, + TrackTable } from "./models.js"; diff --git a/frontend/bindings/yellowjacket/backend/library/library.ts b/frontend/bindings/yellowjacket/backend/library/library.ts index 349c325..e7e9e53 100644 --- a/frontend/bindings/yellowjacket/backend/library/library.ts +++ b/frontend/bindings/yellowjacket/backend/library/library.ts @@ -203,16 +203,12 @@ export function GetTrackMBIDs(filePath: string): $CancellablePromise<$models.Tra } /** - * GetTracks returns every track in a library, or in all of them when - * libraryID is 0. - * - * The library id is a parameter rather than a second method because the - * two used to be separate queries, separate bindings and a branch at - * every call site - and the scoped form costs nothing (measured: 23 ms - * against 21 ms over 26k rows). + * GetTrackTable returns every track in a library, or in all of them + * when libraryID is 0, as a TrackTable. An empty library is an empty + * table, not an error. */ -export function GetTracks(libraryID: number): $CancellablePromise<$models.Track[] | null> { - return $Call.ByID(933082923, libraryID); +export function GetTrackTable(libraryID: number): $CancellablePromise<$models.TrackTable> { + return $Call.ByID(1179690926, libraryID); } /** diff --git a/frontend/bindings/yellowjacket/backend/library/models.ts b/frontend/bindings/yellowjacket/backend/library/models.ts index f2c9987..c1a14f9 100644 --- a/frontend/bindings/yellowjacket/backend/library/models.ts +++ b/frontend/bindings/yellowjacket/backend/library/models.ts @@ -239,3 +239,60 @@ export interface TrackMBIDs { "releaseGroupMbid": string; "artistMbid": string; } + +/** + * TrackTable is every track in a library as the Tracks view uses it: + * one array per column, and every repeated string stored once (#281). + * + * GetTracks used to answer with one object per track, which at 26 138 + * tracks was 20.5 MB of JSON — ~350 bytes a row of key names, four + * cover URLs identical across an album, and artist, album and genre + * strings repeated on every track of the album. Encoding it cost the + * backend ~170 MB of transient allocation and parsing it was the + * WebView's peak. Here the keys appear once, a repeated string is a + * small integer, and the columns the Tracks view does not read are not + * sent at all: LastPlayed and the three larger cover tiers belong to + * the details dialog, which fetches whole tracks by path. + * + * The projection is still trackFromRow's — each row goes through it — + * so this is an encoding of a Track, never a second description of + * one. frontend/src/utils/track-table.ts is the only decoder. + */ +export interface TrackTable { + /** + * Strings holds every distinct string value; a string column holds + * indexes into it. Index 0 is always "". + */ + "strings": string[] | null; + + /** + * GenreSets holds every distinct genre list, as indexes into + * Strings; Genre holds an index into it per track. + */ + "genreSets": (number[] | null)[] | null; + "filePath": string[] | null; + "trackName": number[] | null; + "artistName": number[] | null; + "album": number[] | null; + "composer": number[] | null; + "fileType": number[] | null; + "genre": number[] | null; + "artistMbid": number[] | null; + "releaseGroupMbid": number[] | null; + "recordingMbid": number[] | null; + "coverArtSmall": number[] | null; + + /** + * LengthMs is Track.TrackLength as the number it encodes. + */ + "lengthMs": number[] | null; + "trackNumber": number[] | null; + "discNumber": number[] | null; + "year": number[] | null; + "sampleRate": number[] | null; + "bitDepth": number[] | null; + "channels": number[] | null; + "bitrate": number[] | null; + "fileSize": number[] | null; + "playCount": number[] | null; +} diff --git a/frontend/src/components/track-list/columns.ts b/frontend/src/components/track-list/columns.ts index a0f8a38..dbcf19c 100644 --- a/frontend/src/components/track-list/columns.ts +++ b/frontend/src/components/track-list/columns.ts @@ -1,4 +1,4 @@ -import type * as library from '@go/library/models.js'; +import type { ListTrack } from '@utils/track-table'; import { formatSampleRate, formatBitDepth, @@ -45,7 +45,7 @@ export interface ColumnDef { */ configurable?: boolean; /** Extracts the display value from a track. */ - accessor: (track: library.Track) => string; + accessor: (track: ListTrack) => string; /** Default CSS width (used when no saved width exists). */ defaultWidth: string; /** Text alignment. Defaults to left. */ @@ -60,15 +60,15 @@ export interface ColumnDef { * needs it, which is why it is optional rather than a second * required parameter on all of them. */ - renderCell?: (track: library.Track, term?: string) => unknown; + renderCell?: (track: ListTrack, term?: string) => unknown; /** * Comparison function for sorting two tracks by this column. * Returns negative if a < b, positive if a > b, zero if equal. * If omitted the column is not sortable. */ comparator?: ( - a: library.Track, - b: library.Track, + a: ListTrack, + b: ListTrack, ) => number; } @@ -79,7 +79,7 @@ export const COLUMN_DEFS: Record = { label: 'Art', accessor: () => '', defaultWidth: '36px', - renderCell: (track: library.Track) => { + renderCell: (track: ListTrack) => { // `perf.M3`. This rendered `CoverArtPath` — the *original* // embedded artwork, commonly 1500×1500 and several hundred // kB — scaled by CSS into a 24 px box, while the 100 px @@ -90,10 +90,10 @@ export const COLUMN_DEFS: Record = { // `cover-grid.getCoverUrl()` has picked the right tier all // along; this is the same rule for a much smaller box, with // the two attributes that keep the decode off the scroll - // path. - const src = track.CoverArtSmall - || track.CoverArtMedium - || track.CoverArtPath; + // path. The list carries only this tier (#281): every tier + // is derived from the same file, so it is set whenever any + // of them would be. + const src = track.CoverArtSmall; if (!src) return nothing; diff --git a/frontend/src/components/track-list/search-ranking.ts b/frontend/src/components/track-list/search-ranking.ts index e295e33..20ba164 100644 --- a/frontend/src/components/track-list/search-ranking.ts +++ b/frontend/src/components/track-list/search-ranking.ts @@ -1,4 +1,4 @@ -import type * as library from '@go/library/models.js'; +import type { ListTrack } from '@utils/track-table'; import { html } from 'lit'; import type { TemplateResult } from 'lit'; @@ -99,7 +99,7 @@ function matchQuality( * fields are always included on top of these. */ function scoreTrack( - track: library.Track, + track: ListTrack, termLower: string, columns: ColumnDef[], ): number { @@ -147,7 +147,7 @@ function scoreTrack( /** A track paired with its relevance score. */ export interface RankedTrack { - track: library.Track; + track: ListTrack; score: number; } @@ -166,10 +166,10 @@ export interface RankedTrack { * `scores` (Map of FilePath → relevance score). */ export function rankTracks( - tracks: library.Track[], + tracks: ListTrack[], term: string, activeColumns: ColumnDef[], -): { tracks: library.Track[]; scores: Map } { +): { tracks: ListTrack[]; scores: Map } { const termLower = term.toLowerCase(); const ranked: RankedTrack[] = []; @@ -188,7 +188,7 @@ export function rankTracks( // Sort descending by score (highest relevance first). ranked.sort((a, b) => b.score - a.score); - const result: library.Track[] = []; + const result: ListTrack[] = []; const scores = new Map(); for (const r of ranked) { diff --git a/frontend/src/components/track-list/track-list.ts b/frontend/src/components/track-list/track-list.ts index 11c12f0..e874619 100644 --- a/frontend/src/components/track-list/track-list.ts +++ b/frontend/src/components/track-list/track-list.ts @@ -1,5 +1,5 @@ -import * as library from '@go/library/models.js'; -import { LitElement, html, svg, css, nothing } from 'lit'; +import type { ListTrack } from '@utils/track-table'; +import { LitElement, html, svg, css, nothing, type TemplateResult } from 'lit'; import { designTokens } from '../../styles/tokens.css'; import { srOnly } from '../../styles/sr-only.css'; import { @@ -80,11 +80,10 @@ 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 { 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 { tracksByFilePath } from '@utils/track-index.js'; import '@components/playlist-picker/playlist-picker.js'; import type { TrackDetails } from '@components/track-details/track-details.js'; -import type { CoverArtUrls } from '@components/track-details/track-details.js'; import { ICON_PLAY, ICON_PLAYLIST, @@ -155,7 +154,7 @@ export class TrackList * parent is responsible for reloading when data changes. */ @property({ type: Array, attribute: false }) - externalTracks?: library.Track[]; + externalTracks?: ListTrack[]; /** * What a host embedding this list (e.g. `genre-details`) should say @@ -199,7 +198,7 @@ export class TrackList private lastSearchTerm = ''; /** Tracks the store's cached array reference to detect refreshes. */ - private lastTracksRef: library.Track[] | null = + private lastTracksRef: ListTrack[] | null = null; /** @@ -242,7 +241,7 @@ export class TrackList } @state() - private tracks: library.Track[] = []; + private tracks: ListTrack[] = []; @query('#context-menu') private contextMenuPopup!: MenuSurface; @@ -269,16 +268,16 @@ export class TrackList private lastActiveTrackPath: string | null = null; // -- Memoisation caches for filtered / sorted tracks -- - private cachedFilteredTracks: library.Track[] = []; - private cachedSortedTracks: library.Track[] = []; + private cachedFilteredTracks: ListTrack[] = []; + private cachedSortedTracks: ListTrack[] = []; private cachedRelevanceScores = new Map< string, number >(); - private prevFilterTracks: library.Track[] = []; + private prevFilterTracks: ListTrack[] = []; private prevFilterTerm = ''; private prevFilterColIds = ''; - private prevSortFiltered: library.Track[] = []; + private prevSortFiltered: ListTrack[] = []; private prevSortField: string | null = null; private prevSortDir: SortDirection = 'asc'; @@ -523,7 +522,7 @@ export class TrackList } } - private computeFilteredTracks(): library.Track[] { + private computeFilteredTracks(): ListTrack[] { const term = this.searchCtrl.term; if (!term) { @@ -543,7 +542,7 @@ export class TrackList return result.tracks; } - private computeSortedTracks(): library.Track[] { + private computeSortedTracks(): ListTrack[] { const tracks = this.cachedFilteredTracks; const hasSearch = this.cachedRelevanceScores.size > 0; @@ -1734,7 +1733,7 @@ export class TrackList */ private resolveTrackFromEvent( e: Event, - ): { track: library.Track; index: number } | null { + ): { track: ListTrack; index: number } | null { const row = (e.target as HTMLElement).closest( '.track-row', ) as HTMLElement | null; @@ -1875,7 +1874,7 @@ export class TrackList private onTrackRowClick( e: MouseEvent, - track: library.Track, + track: ListTrack, index: number, ) { // Clicking is also how the keyboard's starting point is chosen: @@ -1884,7 +1883,7 @@ export class TrackList this.selection.handleItemClick(e, track.FilePath, index); } - private onTrackRowDblClick(_track: library.Track, index: number) { + private onTrackRowDblClick(_track: ListTrack, index: number) { this.selection.clear(); this.playFromRow(index); } @@ -1925,7 +1924,7 @@ export class TrackList ); } - private onTrackContextMenu(e: MouseEvent, track: library.Track) { + private onTrackContextMenu(e: MouseEvent, track: ListTrack) { e.preventDefault(); e.stopPropagation(); @@ -1939,7 +1938,7 @@ export class TrackList private onTrackDragStart = ( e: DragEvent, - track: library.Track, + track: ListTrack, ) => { // Gather file paths: all selected if this track is selected, // otherwise just the dragged track. @@ -2129,74 +2128,24 @@ export class TrackList this.ctxMenu.close(); } + // The list's rows do not carry what the dialog shows (#281), so + // the dialog gets whole tracks by path, like every other opener. private async openTrackDetails(filePath: string) { - const track = tracksByFilePath(this.tracks).get( + await showTrackDetailsForPath( + () => this.trackDetailsDialog, filePath, - ); - - if (!track) return; - - const ready = await loadTrackDetails( () => 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 tracks = tracksForPaths( - this.tracks, + await showBatchTrackDetailsForPaths( + () => this.trackDetailsDialog, filePaths, - ); - - if (tracks.length === 0) return; - - const ready = await loadTrackDetails( () => void this.openBatchTrackDetails(filePaths), ); - - if (!ready) return; - - // Use cover art from the first track. If all tracks share - // the same album, they share the same art. - const first = tracks[0]!; - let coverArt: CoverArtUrls | null = null; - let coverArtMixed = false; - - const albumNames = new Set(tracks.map((t) => t.Album)); - - 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, - ); } // ================================================================= @@ -2274,7 +2223,7 @@ export class TrackList this.saveSortPreferences(); } - private isActiveTrack(track: library.Track): boolean { + private isActiveTrack(track: ListTrack): boolean { const currentTrack = this.player.currentTrack; if (!currentTrack) return false; @@ -2283,9 +2232,9 @@ export class TrackList } private renderTrackRow = ( - track: library.Track, + track: ListTrack, index: number, - ): unknown => { + ): TemplateResult => { const active = this.isActiveTrack(track); const selected = this.selection.isSelected( track.FilePath, @@ -2568,7 +2517,7 @@ export class TrackList scroller .items=${visibleTracks} .renderItem=${this.renderTrackRow} - .keyFunction=${(track: library.Track) => track.FilePath} + .keyFunction=${(track: ListTrack) => track.FilePath} .layout=${this.rowLayout} > `} diff --git a/frontend/src/store/controllers/library-controller.ts b/frontend/src/store/controllers/library-controller.ts index dc00c3d..9458fd6 100644 --- a/frontend/src/store/controllers/library-controller.ts +++ b/frontend/src/store/controllers/library-controller.ts @@ -1,6 +1,7 @@ import type { ReactiveController, ReactiveControllerHost } from 'lit'; import type * as library from '@go/library/models.js'; import { libraryStore } from '../library-store'; +import type { ListTrack } from '@utils/track-table'; type ViewName = 'tracks' | 'albums' | 'artists' | 'genres'; @@ -55,7 +56,7 @@ export class LibraryController implements ReactiveController { // DATA ACCESS // =================================================================== - async getTracks(): Promise { + async getTracks(): Promise { return libraryStore.getTracks(); } @@ -85,7 +86,7 @@ export class LibraryController implements ReactiveController { ); } - get cachedTracks(): library.Track[] | null { + get cachedTracks(): ListTrack[] | null { return libraryStore.getCachedTracks(); } diff --git a/frontend/src/store/library-store.ts b/frontend/src/store/library-store.ts index 72ef0d0..29eea8c 100644 --- a/frontend/src/store/library-store.ts +++ b/frontend/src/store/library-store.ts @@ -1,6 +1,6 @@ import { EventsOn } from '@runtime/runtime'; import { - GetTracks, + GetTrackTable, GetAlbums, GetArtists, GetGenres, @@ -9,6 +9,7 @@ import { } from '@go/library/library.js'; import type * as library from '@go/library/models.js'; import { list } from '@utils/binding'; +import { decodeTrackTable, type ListTrack } from '@utils/track-table'; import { Events } from '../events'; type ViewName = 'tracks' | 'albums' | 'artists' | 'genres'; @@ -28,7 +29,7 @@ const COVER_SIZE_DEFAULT = 176; const COVER_SIZE_KEY = 'cover-grid-size'; class LibraryStore { - private tracks: library.Track[] | null = null; + private tracks: ListTrack[] | null = null; private albums: library.Album[] | null = null; private artists: library.Artist[] | null = null; private genres: library.GenreWithCount[] | null = null; @@ -214,18 +215,18 @@ class LibraryStore { } } - async getTracks(): Promise { + async getTracks(): Promise { if (this.tracks !== null) { return this.tracks; } - const pending = this.pending('tracks'); + const pending = this.pending('tracks'); if (pending) return pending; return this.track( 'tracks', - list(GetTracks(this.libraryFilter())), + GetTrackTable(this.libraryFilter()).then(decodeTrackTable), (tracks) => { this.tracks = tracks; }, @@ -341,7 +342,7 @@ class LibraryStore { // Synchronous access for controllers that need current cached values. // =================================================================== - getCachedTracks(): library.Track[] | null { + getCachedTracks(): ListTrack[] | null { return this.tracks; } @@ -520,10 +521,11 @@ class LibraryStore { private applyPlayCount(payload: unknown): void { if (this.tracks === null) return; + // The list does not carry LastPlayed (#281); trackCache patches + // it on the whole tracks the details dialog reads. const p = payload as { filePath?: string; playCount?: number; - lastPlayed?: string; } | null; if (!p?.filePath) return; @@ -536,14 +538,10 @@ class LibraryStore { if (existing === undefined) return; - const patched = Object.assign( - Object.create(Object.getPrototypeOf(existing) as object), - existing, - { - PlayCount: p.playCount ?? existing.PlayCount, - LastPlayed: p.lastPlayed ?? existing.LastPlayed, - }, - ) as library.Track; + const patched: ListTrack = { + ...existing, + PlayCount: p.playCount ?? existing.PlayCount, + }; this.tracks = [ ...this.tracks.slice(0, idx), diff --git a/frontend/src/utils/binding.ts b/frontend/src/utils/binding.ts index 1988e8d..55c8bc1 100644 --- a/frontend/src/utils/binding.ts +++ b/frontend/src/utils/binding.ts @@ -79,6 +79,14 @@ export function compact( return out; } +/** + * listField is list for a slice that arrived as a *field* of a struct + * rather than as a return value — a column of `TrackTable`, say. + */ +export function listField(field: T[] | null | undefined): T[] { + return field ?? []; +} + /** * value awaits a binding whose result is used as-is, dropping only the * cancellation the app never asks for. diff --git a/frontend/src/utils/track-index.ts b/frontend/src/utils/track-index.ts index 3685586..285d98c 100644 --- a/frontend/src/utils/track-index.ts +++ b/frontend/src/utils/track-index.ts @@ -20,22 +20,22 @@ * given array and never again. */ -import type * as library from '@go/library/models.js'; +/** Anything keyed by file path: a whole track, or the list's row. */ +interface HasFilePath { + FilePath: string; +} -const byArray = new WeakMap< - readonly library.Track[], - Map ->(); +const byArray = new WeakMap>(); /** The lookup for `tracks`, built once per array identity. */ -export function tracksByFilePath( - tracks: readonly library.Track[], -): Map { - let map = byArray.get(tracks); +export function tracksByFilePath( + tracks: readonly T[], +): Map { + let map = byArray.get(tracks) as Map | undefined; if (map) return map; - map = new Map(); + map = new Map(); for (const track of tracks) { // First wins: a duplicate path would be the same file, and @@ -49,12 +49,12 @@ export function tracksByFilePath( } /** Resolve file paths to tracks, dropping any that are not present. */ -export function tracksForPaths( - tracks: readonly library.Track[], +export function tracksForPaths( + tracks: readonly T[], filePaths: readonly string[], -): library.Track[] { +): T[] { const byPath = tracksByFilePath(tracks); - const result: library.Track[] = []; + const result: T[] = []; for (const filePath of filePaths) { const track = byPath.get(filePath); diff --git a/frontend/src/utils/track-table.ts b/frontend/src/utils/track-table.ts new file mode 100644 index 0000000..abc0c3f --- /dev/null +++ b/frontend/src/utils/track-table.ts @@ -0,0 +1,128 @@ +/** + * Decode the backend's `TrackTable` into the rows the Tracks view uses. + * + * The table is one array per column with every repeated string sent + * once (#281): ~167 bytes a track against the ~800 of the object-per- + * track JSON it replaced. This is its only decoder, and + * `backend/library/tracktable.go` its only encoder. + * + * Decoding builds plain objects of the same shape as before, so the + * filter, sort and selection code did not have to change — but every + * occurrence of a repeated string is now the *same* string, and every + * track with one genre list shares the array, which is part of why the + * JS heap shrinks along with the payload. Shared means read-only: no + * caller mutates a row's `Genre`. + */ + +import type * as library from '@go/library/models.js'; +import { listField } from '@utils/binding'; + +/** + * A track as the list carries it. The fields left out are the details + * dialog's, which reads whole tracks by path from `trackCache`. + */ +export type ListTrack = Omit< + library.Track, + 'LastPlayed' | 'CoverArtPath' | 'CoverArtMedium' | 'CoverArtLarge' +>; + +/** The `Track` fields a `ListTrack` does not carry. */ +export const LIST_TRACK_OMITS = [ + 'LastPlayed', + 'CoverArtPath', + 'CoverArtMedium', + 'CoverArtLarge', +] as const; + +/** A table whose columns disagree in length is a broken encoder, not data. */ +export class TrackTableError extends Error {} + +export function decodeTrackTable(table: library.TrackTable): ListTrack[] { + const strings = listField(table.strings); + const filePath = listField(table.filePath); + const n = filePath.length; + + const str = (col: number[] | null, name: string): string[] => { + const idx = listField(col); + + if (idx.length !== n) throw mismatch(name, idx.length, n); + + return idx.map((i) => { + const s = strings[i]; + + if (s === undefined) throw new TrackTableError(`${name}: string ${i} out of range`); + + return s; + }); + }; + const num = (col: number[] | null, name: string): number[] => { + const v = listField(col); + + if (v.length !== n) throw mismatch(name, v.length, n); + + return v; + }; + + const genreSets = listField(table.genreSets).map((set) => + listField(set).map((i) => strings[i] ?? ''), + ); + const genre = num(table.genre, 'genre'); + + const trackName = str(table.trackName, 'trackName'); + const artistName = str(table.artistName, 'artistName'); + const album = str(table.album, 'album'); + const composer = str(table.composer, 'composer'); + const fileType = str(table.fileType, 'fileType'); + const artistMbid = str(table.artistMbid, 'artistMbid'); + const releaseGroupMbid = str(table.releaseGroupMbid, 'releaseGroupMbid'); + const recordingMbid = str(table.recordingMbid, 'recordingMbid'); + const coverArtSmall = str(table.coverArtSmall, 'coverArtSmall'); + const lengthMs = num(table.lengthMs, 'lengthMs'); + const trackNumber = num(table.trackNumber, 'trackNumber'); + const discNumber = num(table.discNumber, 'discNumber'); + const year = num(table.year, 'year'); + const sampleRate = num(table.sampleRate, 'sampleRate'); + const bitDepth = num(table.bitDepth, 'bitDepth'); + const channels = num(table.channels, 'channels'); + const bitrate = num(table.bitrate, 'bitrate'); + const fileSize = num(table.fileSize, 'fileSize'); + const playCount = num(table.playCount, 'playCount'); + + const rows: ListTrack[] = new Array(n); + + for (let i = 0; i < n; i++) { + const g = genreSets[genre[i]!]; + + if (g === undefined) throw new TrackTableError(`genre: set ${genre[i]} out of range`); + + rows[i] = { + FilePath: filePath[i]!, + TrackName: trackName[i]!, + ArtistName: artistName[i]!, + Album: album[i]!, + Composer: composer[i]!, + FileType: fileType[i]!, + Genre: g, + ArtistMBID: artistMbid[i]!, + ReleaseGroupMBID: releaseGroupMbid[i]!, + RecordingMBID: recordingMbid[i]!, + CoverArtSmall: coverArtSmall[i]!, + TrackLength: String(lengthMs[i]!), + TrackNumber: trackNumber[i]!, + DiscNumber: discNumber[i]!, + Year: year[i]!, + SampleRate: sampleRate[i]!, + BitDepth: bitDepth[i]!, + Channels: channels[i]!, + Bitrate: bitrate[i]!, + FileSize: fileSize[i]!, + PlayCount: playCount[i]!, + }; + } + + return rows; +} + +function mismatch(name: string, got: number, want: number): TrackTableError { + return new TrackTableError(`${name}: ${got} values for ${want} tracks`); +} diff --git a/frontend/test/components/album-card-year.test.ts b/frontend/test/components/album-card-year.test.ts index ce57a48..bf4c4b4 100644 --- a/frontend/test/components/album-card-year.test.ts +++ b/frontend/test/components/album-card-year.test.ts @@ -19,6 +19,7 @@ import '@components/cover-grid/cover-grid'; import { emit, stub, flush, resetHarness } from '@test/support/harness'; import { Events } from '../../src/events'; import { fixture, shadowAll } from '@test/support/render'; +import { trackTable } from '@test/support/track-table'; const LONG = 'The Rise and Fall of a Midwest Princess in the Key of Everything'; @@ -49,7 +50,7 @@ describe('the album card’s year', () => { beforeEach(() => { resetHarness(); stub('library.Library.GetAlbums', ALBUMS); - stub('library.Library.GetTracks', []); + stub('library.Library.GetTrackTable', trackTable([])); emit(Events.LibraryScanComplete); }); diff --git a/frontend/test/components/album-dropdown.test.ts b/frontend/test/components/album-dropdown.test.ts index 8931287..c92adc9 100644 --- a/frontend/test/components/album-dropdown.test.ts +++ b/frontend/test/components/album-dropdown.test.ts @@ -22,6 +22,7 @@ import '@components/cover-grid/cover-grid'; import { emit, stub, flush, resetHarness } from '@test/support/harness'; import { Events } from '../../src/events'; import { fixture, shadow, shadowAll } from '@test/support/render'; +import { trackTable } from '@test/support/track-table'; /** * Enough albums to fill more than one row. @@ -82,7 +83,7 @@ describe('the album dropdown', () => { beforeEach(() => { resetHarness(); stub('library.Library.GetAlbums', ALBUMS); - stub('library.Library.GetTracks', []); + stub('library.Library.GetTrackTable', trackTable([])); stub('library.Library.GetAlbumTracks', TRACKS); stub('library.Library.GetAlbumTracks', TRACKS); emit(Events.LibraryScanComplete); @@ -155,7 +156,7 @@ describe('the albums grid scrolls', () => { beforeEach(() => { resetHarness(); stub('library.Library.GetAlbums', ALBUMS); - stub('library.Library.GetTracks', []); + stub('library.Library.GetTrackTable', trackTable([])); emit(Events.LibraryScanComplete); }); diff --git a/frontend/test/components/aria-tail.test.ts b/frontend/test/components/aria-tail.test.ts index 3d14a14..5cd1f10 100644 --- a/frontend/test/components/aria-tail.test.ts +++ b/frontend/test/components/aria-tail.test.ts @@ -20,6 +20,7 @@ import { emit, stub, flush, resetHarness } from '@test/support/harness'; import { Events } from '../../src/events'; import { fixture, shadow, shadowAll } from '@test/support/render'; import { searchStore } from '@store/search-store'; +import { trackTable } from '@test/support/track-table'; /** * The searchable columns' accessors read these fields and call @@ -76,7 +77,7 @@ describe('the track list says how it is sorted', () => { beforeEach(async () => { resetHarness(); searchStore.setTerm(''); - stub('library.Library.GetTracks', TRACKS); + stub('library.Library.GetTrackTable', trackTable(TRACKS)); stub('library.Library.GetAlbums', []); emit(Events.LibraryScanComplete); }); @@ -132,7 +133,7 @@ describe('the track list has a voice for its own state', () => { }); it('announces the result of a search that matches nothing', async () => { - stub('library.Library.GetTracks', TRACKS); + stub('library.Library.GetTrackTable', trackTable(TRACKS)); emit(Events.LibraryScanComplete); const el = await fixture('track-list'); @@ -163,7 +164,7 @@ describe('a selectable grid is a listbox, not a row of buttons', () => { searchStore.setTerm(''); stub('library.Library.GetArtists', ARTISTS); stub('library.Library.GetGenres', GENRES); - stub('library.Library.GetTracks', []); + stub('library.Library.GetTrackTable', trackTable([])); stub('library.Library.GetAlbums', []); emit(Events.LibraryScanComplete); }); @@ -197,7 +198,7 @@ describe('a clipped value is readable somewhere', () => { beforeEach(async () => { resetHarness(); searchStore.setTerm(''); - stub('library.Library.GetTracks', TRACKS); + stub('library.Library.GetTrackTable', trackTable(TRACKS)); stub('library.Library.GetAlbums', []); emit(Events.LibraryScanComplete); }); @@ -240,7 +241,7 @@ describe('the playing row is more than a colour', () => { beforeEach(async () => { resetHarness(); searchStore.setTerm(''); - stub('library.Library.GetTracks', TRACKS); + stub('library.Library.GetTrackTable', trackTable(TRACKS)); stub('library.Library.GetAlbums', []); emit(Events.LibraryScanComplete); }); diff --git a/frontend/test/components/art-prefetch.test.ts b/frontend/test/components/art-prefetch.test.ts index 80b88e0..b7922ca 100644 --- a/frontend/test/components/art-prefetch.test.ts +++ b/frontend/test/components/art-prefetch.test.ts @@ -35,6 +35,7 @@ import { imagePrefetched, resetImagePrefetch, } from '@utils/image-prefetch'; +import { trackTable } from '@test/support/track-table'; /** Enough albums that the virtualizer's own window is nowhere near the end. */ const ALBUMS = Array.from({ length: 400 }, (_, i) => { @@ -118,7 +119,7 @@ beforeEach(() => { localStorage.clear(); stub('library.Library.GetAlbums', ALBUMS); stub('library.Library.GetArtists', ARTISTS); - stub('library.Library.GetTracks', []); + stub('library.Library.GetTrackTable', trackTable([])); stub('library.Library.GetGenres', []); emit(Events.LibraryScanComplete); }); diff --git a/frontend/test/components/card-grid-repaint.test.ts b/frontend/test/components/card-grid-repaint.test.ts index 0894daa..26033b8 100644 --- a/frontend/test/components/card-grid-repaint.test.ts +++ b/frontend/test/components/card-grid-repaint.test.ts @@ -29,6 +29,7 @@ import '@components/genres-view/genres-view'; import { emit, stub, flush, resetHarness } from '@test/support/harness'; import { Events } from '../../src/events'; import { fixture, shadowAll } from '@test/support/render'; +import { trackTable } from '@test/support/track-table'; const ARTISTS = [ { ID: 1, Name: 'Alpha', AlbumCount: 2, TrackCount: 9 }, @@ -58,7 +59,7 @@ describe('a card grid shows its selection', () => { resetHarness(); stub('library.Library.GetArtists', ARTISTS); stub('library.Library.GetGenres', GENRES); - stub('library.Library.GetTracks', []); + stub('library.Library.GetTrackTable', trackTable([])); stub('library.Library.GetAlbums', []); // The views read through LibraryController, whose cache is only // primed by a scan-complete; without it they render nothing and the diff --git a/frontend/test/components/chrome.test.ts b/frontend/test/components/chrome.test.ts index b2dc871..a60bf76 100644 --- a/frontend/test/components/chrome.test.ts +++ b/frontend/test/components/chrome.test.ts @@ -191,7 +191,7 @@ describe('', () => { select?.dispatchEvent(new Event('change')); await flush(); - expect(lastArgs('library.Library.GetTracks')).toEqual([8]); + expect(lastArgs('library.Library.GetTrackTable')).toEqual([8]); }); it('picks up a library added while it was on screen', async () => { diff --git a/frontend/test/components/detail-touch-targets.test.ts b/frontend/test/components/detail-touch-targets.test.ts index 9cc5120..d308836 100644 --- a/frontend/test/components/detail-touch-targets.test.ts +++ b/frontend/test/components/detail-touch-targets.test.ts @@ -72,7 +72,7 @@ function boxOf(el: Element | null | undefined): { w: number; h: number } { describe('the way out of a detail view', () => { beforeEach(() => { for (const path of [ - 'library.Library.GetTracks', + 'library.Library.GetTrackTable', 'library.Library.GetAlbums', 'library.Library.GetArtists', 'library.Library.GetGenres', diff --git a/frontend/test/components/empty-states.test.ts b/frontend/test/components/empty-states.test.ts index 46fab5b..27b2ca9 100644 --- a/frontend/test/components/empty-states.test.ts +++ b/frontend/test/components/empty-states.test.ts @@ -11,11 +11,12 @@ import '@components/track-list/track-list'; import { Events } from '../../src/events'; import { emit, stub, stubFailure, flush, resetHarness } from '@test/support/harness'; import { fixture, shadow, text } from '@test/support/render'; +import { trackTable } from '@test/support/track-table'; /** Drop the library store's cache so the list has to fetch. */ async function emptyLibrary(): Promise { resetHarness(); - stub('library.Library.GetTracks', []); + stub('library.Library.GetTrackTable', trackTable([])); stub('library.Library.GetAlbums', []); stub('library.Library.GetArtists', []); stub('library.Library.GetGenres', []); @@ -40,7 +41,7 @@ describe(' empty, loading and failed', () => { }); it('says the query failed, and offers to try again', async () => { - stubFailure('library.Library.GetTracks', 'sql: database is locked'); + stubFailure('library.Library.GetTrackTable', 'sql: database is locked'); emit(Events.LibraryScanComplete); await flush(); diff --git a/frontend/test/components/keyboard-reach.test.ts b/frontend/test/components/keyboard-reach.test.ts index 1aabdd0..62333c8 100644 --- a/frontend/test/components/keyboard-reach.test.ts +++ b/frontend/test/components/keyboard-reach.test.ts @@ -14,6 +14,7 @@ import '@components/track-list/track-list'; import { stub, emit, flush } from '@test/support/harness'; import { Events } from '../../src/events'; import { fixture, shadow, shadowAll, update } from '@test/support/render'; +import { trackTable } from '@test/support/track-table'; /** Two fixture tracks, enough to move a focus ring between. */ const TRACKS = [ @@ -78,7 +79,7 @@ describe(' when closed', () => { describe(' roving tabindex', () => { beforeEach(() => { - stub('library.Library.GetTracks', TRACKS); + stub('library.Library.GetTrackTable', trackTable(TRACKS)); }); it('offers exactly one tab stop, however many rows there are', async () => { diff --git a/frontend/test/components/library-status.test.ts b/frontend/test/components/library-status.test.ts index 6945552..89f5b04 100644 --- a/frontend/test/components/library-status.test.ts +++ b/frontend/test/components/library-status.test.ts @@ -31,6 +31,7 @@ import { stubFailure, } from '@test/support/harness'; import { fixture, shadow, shadowAll, update } from '@test/support/render'; +import { trackTable } from '@test/support/track-table'; const SEARCH = 'explore.Service.SearchLocal'; @@ -133,7 +134,7 @@ describe(' badges', () => { stub('explore.Service.GetArtistImageURL', ''); stub('explore.Service.GetExploreShelves', { shelves: [], state: 'ready' }); stub('library.Library.GetAlbums', []); - stub('library.Library.GetTracks', []); + stub('library.Library.GetTrackTable', trackTable([])); await withRequests([]); }); diff --git a/frontend/test/components/list-render-cost.test.ts b/frontend/test/components/list-render-cost.test.ts index a4566d6..18c5ca5 100644 --- a/frontend/test/components/list-render-cost.test.ts +++ b/frontend/test/components/list-render-cost.test.ts @@ -48,21 +48,12 @@ describe('the track list Art column', () => { ]).toEqual(['lazy', 'async']); }); - it('falls back through the tiers rather than rendering nothing', () => { - const onlyOriginal = cell('albumArt', { - CoverArtPath: '/covers/abc.jpg', - CoverArtSmall: '', - CoverArtMedium: '', - }).querySelector('img'); - - expect(onlyOriginal?.getAttribute('src')).toBe('/covers/abc.jpg'); - }); - + // There is no fallback through the larger tiers any more: the list + // carries only the small one (#281), and the backend derives every + // tier from the same file, so it is set whenever any of them is. it('renders nothing at all when there is no art', () => { const none = cell('albumArt', { - CoverArtPath: '', CoverArtSmall: '', - CoverArtMedium: '', }); expect(none.querySelector('img')).toBeNull(); diff --git a/frontend/test/components/smoke.test.ts b/frontend/test/components/smoke.test.ts index e4c3f16..0439b3a 100644 --- a/frontend/test/components/smoke.test.ts +++ b/frontend/test/components/smoke.test.ts @@ -117,7 +117,7 @@ const TAGS = [ */ function stubEmptyBackend(): void { const emptyLists = [ - 'library.Library.GetTracks', + 'library.Library.GetTrackTable', 'library.Library.GetAlbums', 'library.Library.GetArtists', 'library.Library.GetGenres', diff --git a/frontend/test/components/touch-selection.test.ts b/frontend/test/components/touch-selection.test.ts index a4fec7b..be7c656 100644 --- a/frontend/test/components/touch-selection.test.ts +++ b/frontend/test/components/touch-selection.test.ts @@ -24,6 +24,7 @@ import '@components/selection-bar/selection-bar'; import { calls, flush, resetHarness, stub } from '@test/support/harness'; import { fixture, shadow, shadowAll } from '@test/support/render'; import { installTouchGestures, LONG_PRESS_MS } from '@utils/touch-gestures'; +import { trackTable } from '@test/support/track-table'; const HELD = LONG_PRESS_MS + 120; @@ -117,7 +118,7 @@ function rows(el: HTMLElement): HTMLElement[] { describe('a finger on a track row', () => { beforeEach(() => { resetHarness(); - stub('library.Library.GetTracks', TRACKS); + stub('library.Library.GetTrackTable', trackTable(TRACKS)); stub('library.Library.GetAllLibrariesWithTrackCounts', []); stub('config.Config.GetShortcuts', {}); stub('queue.Queue.SetQueue', null); @@ -320,7 +321,7 @@ describe('', () => { describe('a tap on a name inside a row', () => { beforeEach(() => { resetHarness(); - stub('library.Library.GetTracks', TRACKS); + stub('library.Library.GetTrackTable', trackTable(TRACKS)); stub('library.Library.GetAllLibrariesWithTrackCounts', []); stub('config.Config.GetShortcuts', {}); stub('queue.Queue.SetQueue', null); diff --git a/frontend/test/components/touch-swipe.test.ts b/frontend/test/components/touch-swipe.test.ts index 07f9595..e343df8 100644 --- a/frontend/test/components/touch-swipe.test.ts +++ b/frontend/test/components/touch-swipe.test.ts @@ -33,6 +33,7 @@ import '@components/track-list/track-list'; import { calls, flush, resetHarness, stub } from '@test/support/harness'; import { fixture, shadow, shadowAll } from '@test/support/render'; import { installTouchGestures } from '@utils/touch-gestures'; +import { trackTable } from '@test/support/track-table'; const wait = (ms: number) => new Promise((r) => setTimeout(r, ms)); @@ -125,7 +126,7 @@ function threshold(row: HTMLElement): number { describe('a finger swiped right across a track row', () => { beforeEach(() => { resetHarness(); - stub('library.Library.GetTracks', TRACKS); + stub('library.Library.GetTrackTable', trackTable(TRACKS)); stub('library.Library.GetAllLibrariesWithTrackCounts', []); stub('config.Config.GetShortcuts', {}); stub('queue.Queue.SetQueue', null); diff --git a/frontend/test/components/view-lifecycle.test.ts b/frontend/test/components/view-lifecycle.test.ts index 14b3ed8..5b6a38c 100644 --- a/frontend/test/components/view-lifecycle.test.ts +++ b/frontend/test/components/view-lifecycle.test.ts @@ -231,7 +231,7 @@ const CACHED_VIEWS = [ * binding resolves undefined, which is not what Go sends. */ function stubEmptyBackend(): void { for (const path of [ - 'library.Library.GetTracks', + 'library.Library.GetTrackTable', 'library.Library.GetAlbums', 'library.Library.GetArtists', 'library.Library.GetGenres', diff --git a/frontend/test/setup.ts b/frontend/test/setup.ts index a2c2fef..3929699 100644 --- a/frontend/test/setup.ts +++ b/frontend/test/setup.ts @@ -35,7 +35,7 @@ const importTimeDefaults: Array<[string, unknown]> = [ // libraryStore and playlistStore fetch eagerly at import. Left // unstubbed they would cache `undefined` — not the empty list Go // sends — and every consumer would then crash on `.length`. - ['library.Library.GetTracks', []], + ['library.Library.GetTrackTable', { strings: [''] }], ['library.Library.GetAlbums', []], ['library.Library.GetArtists', []], ['library.Library.GetGenres', []], diff --git a/frontend/test/stores/library-store.test.ts b/frontend/test/stores/library-store.test.ts index 2430135..2bbf094 100644 --- a/frontend/test/stores/library-store.test.ts +++ b/frontend/test/stores/library-store.test.ts @@ -19,10 +19,11 @@ import { lastArgs, resetHarness, } from '@test/support/harness'; +import { trackTable } from '@test/support/track-table'; const TRACKS = [ - { ID: 1, Title: 'One', FilePath: '/a.mp3', PlayCount: 0, LastPlayed: '' }, - { ID: 2, Title: 'Two', FilePath: '/b.mp3', PlayCount: 4, LastPlayed: 'x' }, + { TrackName: 'One', FilePath: '/a.mp3', PlayCount: 0 }, + { TrackName: 'Two', FilePath: '/b.mp3', PlayCount: 4 }, ]; const ALBUMS = [{ ID: 1, Name: 'Album', ArtistName: 'Artist' }]; const OTHER_ALBUMS = [{ ID: 2, Name: 'Other', ArtistName: 'Other Artist' }]; @@ -33,15 +34,10 @@ const LIBRARIES = [{ id: 7, name: 'Music' }, { id: 8, name: 'Field' }]; /** Stub every read binding the store can reach. Unstubbed bindings * resolve undefined, which the store would cache as if it were data. */ function stubReads(): void { - stub('library.Library.GetTracks', TRACKS); + stub('library.Library.GetTrackTable', trackTable(TRACKS)); stub('library.Library.GetAlbums', ALBUMS); stub('library.Library.GetArtists', ARTISTS); stub('library.Library.GetGenres', GENRES); - stub('library.Library.GetTracks', TRACKS); - stub('library.Library.GetAlbums', ALBUMS); - stub('library.Library.GetArtists', ARTISTS); - stub('library.Library.GetGenres', GENRES); - stub('library.Library.GetAlbumsByArtist', ALBUMS); stub('library.Library.GetAlbumsByArtist', ALBUMS); stub('library.Library.GetAllLibrariesWithTrackCounts', LIBRARIES); } @@ -69,7 +65,7 @@ describe('library store: caching', () => { it('serves a second read from cache without touching the backend', async () => { await libraryStore.getTracks(); - expect(calls('library.Library.GetTracks')).toHaveLength(0); + expect(calls('library.Library.GetTrackTable')).toHaveLength(0); }); it('deduplicates concurrent first reads into one backend call', async () => { @@ -88,11 +84,11 @@ describe('library store: caching', () => { it('exposes cached collections synchronously once loaded', () => { expect([ - libraryStore.getCachedTracks(), + libraryStore.getCachedTracks()?.map((t) => t.FilePath), libraryStore.getCachedAlbums(), libraryStore.cachedArtists, libraryStore.getCachedGenres(), - ]).toEqual([TRACKS, ALBUMS, ARTISTS, GENRES]); + ]).toEqual([['/a.mp3', '/b.mp3'], ALBUMS, ARTISTS, GENRES]); }); it('refetches everything when a scan completes', async () => { @@ -103,7 +99,7 @@ describe('library store: caching', () => { 'library.Library.GetAlbums', 'library.Library.GetArtists', 'library.Library.GetGenres', - 'library.Library.GetTracks', + 'library.Library.GetTrackTable', ]); }); @@ -111,7 +107,7 @@ describe('library store: caching', () => { emit(Events.TrackMetadataChanged, { filePath: '/a.mp3' }); await flush(); - expect(calls('library.Library.GetTracks')).toHaveLength(1); + expect(calls('library.Library.GetTrackTable')).toHaveLength(1); }); /* @@ -140,10 +136,11 @@ describe('library store: caching', () => { it('patches the one track it names', () => { const tracks = libraryStore.getCachedTracks(); + // LastPlayed is not on the list's rows (#281); trackCache + // patches it on whole tracks. expect(tracks?.[0]).toMatchObject({ FilePath: '/a.mp3', PlayCount: 9, - LastPlayed: '2026-08-11 10:00:00', }); }); @@ -192,7 +189,7 @@ describe('library store: caching', () => { }); it('does not refetch the tracks', () => { - expect(calls('library.Library.GetTracks')).toHaveLength(0); + expect(calls('library.Library.GetTrackTable')).toHaveLength(0); }); it('splices the removed track out in place', () => { @@ -258,7 +255,7 @@ describe('library store: library filter', () => { libraryStore.setSelectedLibrary(7); await flush(); - expect(lastArgs('library.Library.GetTracks')).toEqual([7]); + expect(lastArgs('library.Library.GetTrackTable')).toEqual([7]); }); it('ignores a redundant selection instead of invalidating', async () => { @@ -304,12 +301,12 @@ describe('library store: a fetch that is overtaken', () => { it('serves the library that is selected, not the one that was in flight', async () => { const pending: Array<{ id: number; resolve: (v: unknown) => void }> = []; - const byLibrary = (id: number) => [{ ID: id, Title: `Library ${id}` }]; + const byLibrary = (id: number) => trackTable([{ FilePath: `/lib-${id}.mp3` }]); // Only the track fetch is held open; the other three settle at once, // so the test is about the overtaking and nothing else. stub( - 'library.Library.GetTracks', + 'library.Library.GetTrackTable', (id: number) => new Promise((resolve) => { pending.push({ id, resolve }); @@ -327,11 +324,13 @@ describe('library store: a fetch that is overtaken', () => { pending.find((p) => p.id === 8)?.resolve(byLibrary(8)); await flush(); - expect(libraryStore.getCachedTracks()).toEqual(byLibrary(8)); + expect(libraryStore.getCachedTracks()?.map((t) => t.FilePath)).toEqual([ + '/lib-8.mp3', + ]); }); it('settles the waiters when the fetch they are waiting on fails', async () => { - stubFailure('library.Library.GetTracks', 'sql: database is locked'); + stubFailure('library.Library.GetTrackTable', 'sql: database is locked'); // Invalidation drops the cache and starts the fetch that fails. emit(Events.LibraryScanComplete); diff --git a/frontend/test/support/track-table.ts b/frontend/test/support/track-table.ts new file mode 100644 index 0000000..66d4a2d --- /dev/null +++ b/frontend/test/support/track-table.ts @@ -0,0 +1,86 @@ +/** + * Encode test tracks as the backend's `TrackTable` (#281), so a stub of + * `library.Library.GetTrackTable` can be written as a list of tracks. + * + * Test fixtures are partial tracks; a missing field encodes as the + * zero value Go would have sent. The encoding mirrors + * `backend/library/tracktable.go` closely enough to exercise the real + * decoder: shared string table with "" at 0, genre lists interned. + */ +import type * as library from '@go/library/models.js'; + +/** A fixture: any subset of a track's fields, loosely typed as fixtures are. */ +type PartialTrack = { FilePath: string } & Record; + +const STRING_COLS = [ + ['trackName', 'TrackName'], + ['artistName', 'ArtistName'], + ['album', 'Album'], + ['composer', 'Composer'], + ['fileType', 'FileType'], + ['artistMbid', 'ArtistMBID'], + ['releaseGroupMbid', 'ReleaseGroupMBID'], + ['recordingMbid', 'RecordingMBID'], + ['coverArtSmall', 'CoverArtSmall'], +] as const; + +const INT_COLS = [ + ['trackNumber', 'TrackNumber'], + ['discNumber', 'DiscNumber'], + ['year', 'Year'], + ['sampleRate', 'SampleRate'], + ['bitDepth', 'BitDepth'], + ['channels', 'Channels'], + ['bitrate', 'Bitrate'], + ['fileSize', 'FileSize'], + ['playCount', 'PlayCount'], +] as const; + +export function trackTable(tracks: readonly PartialTrack[]): library.TrackTable { + const strings = ['']; + const index = new Map([['', 0]]); + const intern = (s: string | undefined): number => { + const v = s ?? ''; + let i = index.get(v); + + if (i === undefined) { + i = strings.length; + strings.push(v); + index.set(v, i); + } + + return i; + }; + + const genreSets: number[][] = []; + const genreIndex = new Map(); + const table: Record = { + strings, + genreSets, + filePath: tracks.map((t) => t.FilePath), + lengthMs: tracks.map((t) => Number(t['TrackLength'] ?? 0) || 0), + genre: tracks.map((t) => { + const genres = (t['Genre'] as string[] | null | undefined) ?? []; + const key = genres.join('\u0000'); + let i = genreIndex.get(key); + + if (i === undefined) { + i = genreSets.length; + genreSets.push(genres.map(intern)); + genreIndex.set(key, i); + } + + return i; + }), + }; + + for (const [col, field] of STRING_COLS) { + table[col] = tracks.map((t) => intern(t[field] as string | undefined)); + } + + for (const [col, field] of INT_COLS) { + table[col] = tracks.map((t) => (t[field] as number | undefined) ?? 0); + } + + return table as unknown as library.TrackTable; +} diff --git a/frontend/test/utils/track-table.test.ts b/frontend/test/utils/track-table.test.ts new file mode 100644 index 0000000..47a630a --- /dev/null +++ b/frontend/test/utils/track-table.test.ts @@ -0,0 +1,134 @@ +/** + * `decodeTrackTable` is the only reader of the backend's columnar track + * list (#281). The encoder is Go and the row type is TypeScript, so the + * last block reads both sides' declarations and fails if a column is + * added on one and not the other. + */ +import { describe, expect, it } from 'vitest'; + +import { + decodeTrackTable, + LIST_TRACK_OMITS, + TrackTableError, +} from '@utils/track-table'; +import { trackTable } from '@test/support/track-table'; + +const FIXTURE = [ + { + FilePath: '/m/a/1.flac', + TrackName: 'One', + ArtistName: 'Artist', + Album: 'Album', + Genre: ['Ambient', 'Drone'], + TrackLength: '215000', + TrackNumber: 1, + CoverArtSmall: '/covers/x_sm.jpg', + PlayCount: 3, + }, + { + FilePath: '/m/a/2.flac', + TrackName: 'Two', + ArtistName: 'Artist', + Album: 'Album', + Genre: ['Ambient', 'Drone'], + TrackLength: '1000', + TrackNumber: 2, + CoverArtSmall: '/covers/x_sm.jpg', + }, +]; + +describe('decodeTrackTable', () => { + it('gives back each track it was given', () => { + const [a, b] = decodeTrackTable(trackTable(FIXTURE)); + + expect(a).toMatchObject({ + FilePath: '/m/a/1.flac', + TrackName: 'One', + Album: 'Album', + Genre: ['Ambient', 'Drone'], + TrackLength: '215000', + TrackNumber: 1, + PlayCount: 3, + Composer: '', + }); + expect(b).toMatchObject({ TrackName: 'Two', TrackLength: '1000', PlayCount: 0 }); + }); + + it('shares one genre list between tracks that have the same one', () => { + const [a, b] = decodeTrackTable(trackTable(FIXTURE)); + + expect(a!.Genre).toBe(b!.Genre); + }); + + it('decodes an empty library to no rows', () => { + expect(decodeTrackTable(trackTable([]))).toEqual([]); + }); + + it('refuses a table whose columns disagree in length', () => { + const broken = trackTable(FIXTURE); + + (broken as unknown as { album: number[] }).album = [1]; + + expect(() => decodeTrackTable(broken)).toThrow(TrackTableError); + }); + + it('refuses a string index past the table', () => { + const broken = trackTable(FIXTURE); + + (broken as unknown as { album: number[] }).album = [1, 999]; + + expect(() => decodeTrackTable(broken)).toThrow(/out of range/); + }); +}); + +/** The generated Track interface and the Go encoder, as text. */ +const MODELS = Object.values( + import.meta.glob('../../bindings/yellowjacket/backend/library/models.ts', { + eager: true, + query: '?raw', + import: 'default', + }), +)[0] ?? ''; +const ENCODER = Object.values( + import.meta.glob('../../../backend/library/tracktable.go', { + eager: true, + query: '?raw', + import: 'default', + }), +)[0] ?? ''; + +function interfaceKeys(source: string, name: string): string[] { + const body = source.split(`export interface ${name} {`)[1]?.split('\n}')[0] ?? ''; + + return [...body.matchAll(/^\s+"(\w+)"\??:/gm)].map((m) => m[1]!); +} + +describe('the list row and the Go table agree', () => { + const trackKeys = interfaceKeys(MODELS, 'Track'); + const tableKeys = [...ENCODER.matchAll(/json:"(\w+)"/g)].map((m) => m[1]!); + + it('read both declarations', () => { + expect(trackKeys.length).toBeGreaterThan(20); + expect(tableKeys.length).toBeGreaterThan(20); + }); + + it('decodes every Track field the list keeps, and only those', () => { + const decoded = Object.keys(decodeTrackTable(trackTable(FIXTURE))[0]!).sort(); + const omitted = new Set(LIST_TRACK_OMITS); + + expect(decoded).toEqual(trackKeys.filter((k) => !omitted.has(k)).sort()); + }); + + it('has a column for every field it decodes', () => { + // Columns are the field names in lower camel case, except the two + // that are not columns of a field and the length, which is sent as + // the number it encodes. + const columns = new Set(tableKeys.filter((k) => !['strings', 'genreSets'].includes(k))); + const expected = trackKeys + .filter((k) => !(LIST_TRACK_OMITS as readonly string[]).includes(k)) + .map((k) => (k === 'TrackLength' ? 'lengthMs' : k[0]!.toLowerCase() + k.slice(1))) + .map((k) => k.replace(/MBID$/, 'Mbid')); + + expect([...columns].sort()).toEqual(expected.sort()); + }); +}); -- 2.54.0 From 3d9828b847ba5b099458b31ebbff0b2f3cdd8696 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Mon, 5 Oct 2026 23:55:54 -0400 Subject: [PATCH 04/10] 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); -- 2.54.0 From 620151aa41611cf078d364ca84f03d89671ad530 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Tue, 6 Oct 2026 01:43:00 -0400 Subject: [PATCH 05/10] perf(library): load a collection when a view needs it, not at startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four collections were fetched at DOMContentLoaded and refetched on every invalidation, whichever view was showing. On a 26 138-track library the track list was 20.5 MB of that, and encoding it cost the backend ~170 MB of transient allocation — paid by someone looking at Home, which draws none of it. - The store warms only albums, artists and genres, on idle after first paint: 1.6 MB together, and what made those views instant. - A nav item prefetches on hover and on keyboard focus, which is the ~100 ms before the click that a cold open would otherwise wait. - An invalidation refetches what something had loaded, and nothing else — a scan no longer loads the track list of a library whose Tracks view nobody has opened. - `index.html`'s first-paint `` is `view-hidden`, and `index.ts` no longer activates it: it is markup, not a decision about which view the launch lands on, and activating it was what fetched the whole list for a landing on Home. The track list itself loads on view activation, which the shell drives. Measured on 50 000 tracks: backend RSS at rest 543 → 296 MB, peak 571 → 296 MB, Go heap held 361 → 125 MB, JS heap 31.8 → 18 MB, binding bytes at rest 35.9 → 12.1 MB, heap after a browse 36.5 → 22.7 MB. Tracks first open 26 ms; slowest view open 57 ms. Closes #280 --- e2e/perf/measure.mjs | 167 +++++++++++++++++- e2e/specs/library-lazy.spec.ts | 58 ++++++ frontend/index.html | 10 +- frontend/index.ts | 17 +- .../src/components/queue-panel/queue-panel.ts | 10 +- .../src/components/sidebar/app-sidebar.ts | 18 ++ .../src/components/track-list/track-list.ts | 37 +++- frontend/src/store/library-store.ts | 163 ++++++++++++----- frontend/test/components/chrome.test.ts | 11 +- frontend/test/stores/library-lazy.test.ts | 99 +++++++++++ frontend/test/stores/library-store.test.ts | 21 ++- 11 files changed, 538 insertions(+), 73 deletions(-) create mode 100644 e2e/specs/library-lazy.spec.ts create mode 100644 frontend/test/stores/library-lazy.test.ts diff --git a/e2e/perf/measure.mjs b/e2e/perf/measure.mjs index 0cff8d6..119857d 100644 --- a/e2e/perf/measure.mjs +++ b/e2e/perf/measure.mjs @@ -42,6 +42,13 @@ * browse number below could not see this, because it * visits Explore without ever typing in it. * heap JS heap after a scripted browse, post-GC. m3. + * memory what the app holds at rest after launch, before any + * view but the landing one is opened: the backend + * process's RSS and peak, its Go heap, the page's JS + * heap, and the bytes each binding returned on the way. + * Then the same after the first open of Tracks, and how + * long that open took to its first *row* — the number + * eager loading was buying (#280/#281). * * Usage: * node e2e/perf/measure.mjs --label before @@ -51,7 +58,7 @@ * Requires a running app: `make dev-headless SEED=bulk`. */ -import { readFileSync, writeFileSync, mkdirSync, existsSync } from 'node:fs'; +import { readFileSync, writeFileSync, mkdirSync, existsSync, readdirSync } from 'node:fs'; import { dirname, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; import { chromium } from '@playwright/test'; @@ -190,6 +197,135 @@ const SEARCH_DEBOUNCE_MS = 150; /* -------------------------------------------------------------------- */ +/** + * The backend process's memory, from /proc and its own pprof endpoint. + * + * Linux and a dev build only (pprof is mounted by `-tags dev`); either + * missing is a null, not a failure — this is a measurement harness, and + * a number it cannot take is reported as absent rather than as zero. + */ +async function backendMemory() { + const out = { rssMB: null, peakMB: null, anonMB: null, goHeapInuseMB: null, goHeapHeldMB: null }; + const pid = findBackendPid(); + + if (pid) { + try { + const status = readFileSync(`/proc/${pid}/status`, 'utf8'); + const kb = (k) => Number(status.match(new RegExp(`^${k}:\\s+(\\d+)`, 'm'))?.[1] ?? NaN); + + out.rssMB = round(kb('VmRSS') / 1024, 1); + out.peakMB = round(kb('VmHWM') / 1024, 1); + out.anonMB = round(kb('RssAnon') / 1024, 1); + } catch { + // The process went away between finding it and reading it. + } + } + + try { + const text = await (await fetch('http://localhost:6060/debug/pprof/heap?debug=1')).text(); + const field = (k) => Number(text.match(new RegExp(`^# ${k} = (\\d+)`, 'm'))?.[1] ?? NaN); + + out.goHeapInuseMB = round(field('HeapInuse') / 1048576, 1); + // What the runtime holds from the OS: the RSS the heap accounts for. + out.goHeapHeldMB = round((field('HeapSys') - field('HeapReleased')) / 1048576, 1); + } catch { + // Not a dev build, or pprof is not listening. + } + + return out; +} + +/** The pid of the running `yj-dev` binary, found by name under /proc. */ +function findBackendPid() { + try { + for (const entry of readdirSync('/proc')) { + if (!/^\d+$/.test(entry)) continue; + + try { + if (readFileSync(`/proc/${entry}/comm`, 'utf8').trim() === 'yj-dev') return entry; + } catch { + // Raced with an exiting process. + } + } + } catch { + // No /proc: not Linux. + } + + return null; +} + +/** Bytes returned per binding name, largest first. */ +function bytesByBinding(calls) { + const by = {}; + + for (const c of calls) by[c.path] = (by[c.path] ?? 0) + (c.bytes ?? 0); + + return Object.fromEntries( + Object.entries(by) + .filter(([, b]) => b > 0) + .sort((a, b) => b[1] - a[1]) + .map(([k, b]) => [k, round(b / 1048576, 2)]), + ); +} + +async function pageMemory(page, client) { + await client.send('HeapProfiler.collectGarbage'); + await page.waitForTimeout(300); + + const heap = await client.send('Runtime.getHeapUsage'); + const dom = await client.send('Memory.getDOMCounters'); + + return { jsHeapMB: round(heap.usedSize / 1048576, 1), domNodes: dom.nodes }; +} + +async function measureMemory(page, client) { + // Settled: the landing view has loaded and any idle warming has run. + await page.waitForTimeout(5000); + + const startupCalls = await page.evaluate(() => window.__yjPerf.calls); + const atRest = { + backend: await backendMemory(), + page: await pageMemory(page, client), + bytesByBindingMB: bytesByBinding(startupCalls), + totalBindingMB: round(startupCalls.reduce((n, c) => n + (c.bytes ?? 0), 0) / 1048576, 2), + }; + + // First open of Tracks, to its first row: what a user waits for when + // nothing was loaded ahead of them. + const since = await page.evaluate(() => performance.now()); + const firstRowMs = await page.evaluate(async () => { + const t0 = performance.now(); + + document.dispatchEvent(new CustomEvent('navigate', { detail: { view: 'tracks' } })); + + for (;;) { + const list = document.querySelector('#main-content > track-list:not(.view-hidden)'); + + if (list?.shadowRoot?.querySelector('[data-testid="track-row"]')) { + return Math.round(performance.now() - t0); + } + + if (performance.now() - t0 > 60000) return null; + await new Promise((r) => requestAnimationFrame(r)); + } + }); + await page.waitForTimeout(2000); + + const tracksCalls = await page.evaluate((t) => window.__yjPerf.since(t), since); + + return { + atRest, + tracksOpen: { + firstRowMs, + bytesByBindingMB: bytesByBinding(tracksCalls), + backend: await backendMemory(), + page: await pageMemory(page, client), + }, + }; +} + +/* -------------------------------------------------------------------- */ + async function measureStartup(page) { const nav = await page.evaluate(() => { const n = performance.getEntriesByType('navigation')[0]; @@ -234,16 +370,23 @@ async function measureStartup(page) { }; }); - // "First row on screen" is the number a user experiences as startup; - // FCP fires on the chrome around an empty list. + // "First row on screen" is the number a user experiences as startup + // *when the app lands on Tracks*; FCP fires on the chrome around an + // empty list. + // + // The deadline is short because since #280 a landing on any other + // view leaves the track list unloaded, and this would otherwise + // spend a minute waiting for a row that is not coming. The number + // that means something either way is `memory.tracksOpen.firstRowMs`, + // which opens Tracks and waits for its first row deliberately. const firstRowMs = await page.evaluate(async () => { const t0 = performance.now(); - const deadline = t0 + 60000; + const deadline = t0 + 2000; for (;;) { const list = document.querySelector('track-list'); const row = list?.shadowRoot?.querySelector('[role="row"], .track-row'); - if (row) return Math.round(performance.now() - t0 + (performance.timeOrigin ? 0 : 0)); + if (row) return Math.round(performance.now() - t0); if (performance.now() > deadline) return null; await new Promise((r) => setTimeout(r, 16)); } @@ -1845,6 +1988,12 @@ async function run(label) { loadWallMs: Date.now() - t0, }; + // First of all: at rest means before any measurement opens a view. + console.log(' memory at rest, then Tracks first open…'); + report.memory = await measureMemory(page, client); + await page.evaluate(() => document.dispatchEvent( + new CustomEvent('navigate', { detail: { view: 'home' } }), + )); console.log(' startup…'); report.startup = await measureStartup(page); // Before anything else navigates: every view's first open has to be @@ -1907,6 +2056,14 @@ async function run(label) { const ROWS = [ ['First contentful paint', (r) => fmt(r.startup.firstContentfulPaintMs, 'ms')], + ['At rest: backend RSS', (r) => fmt(r.memory?.atRest.backend.rssMB, 'MB')], + ['At rest: backend peak RSS', (r) => fmt(r.memory?.atRest.backend.peakMB, 'MB')], + ['At rest: Go heap held', (r) => fmt(r.memory?.atRest.backend.goHeapHeldMB, 'MB')], + ['At rest: JS heap', (r) => fmt(r.memory?.atRest.page.jsHeapMB, 'MB')], + ['At rest: binding bytes', (r) => fmt(r.memory?.atRest.totalBindingMB, 'MB')], + ['Tracks first open: first row', (r) => fmt(r.memory?.tracksOpen.firstRowMs, 'ms')], + ['Tracks open: backend peak RSS', (r) => fmt(r.memory?.tracksOpen.backend.peakMB, 'MB')], + ['Tracks open: JS heap', (r) => fmt(r.memory?.tracksOpen.page.jsHeapMB, 'MB')], ['First track row', (r) => fmt(r.startup.firstRowAfterLoadMs, 'ms')], ['JS transferred', (r) => fmt(round(r.startup.scriptBytes / 1024), 'kB')], ['JS evaluated before first paint', (r) => fmt(round((r.startup.scriptBytesBeforePaint ?? 0) / 1024), 'kB')], diff --git a/e2e/specs/library-lazy.spec.ts b/e2e/specs/library-lazy.spec.ts new file mode 100644 index 0000000..fec0766 --- /dev/null +++ b/e2e/specs/library-lazy.spec.ts @@ -0,0 +1,58 @@ +/** + * #280: launching the app must not load the track list. + * + * Every collection used to be fetched at `DOMContentLoaded` and + * refetched on every invalidation, whichever view was showing. On a + * 26 138-track library the track list was 20.5 MB of that, and encoding + * it cost the backend ~170 MB of transient allocation — for someone + * looking at Home, which draws none of it. Measured on 50 000 tracks: + * 543 MB of backend RSS at rest before, 296 MB after. + * + * The negative assertion is the point, and it needs its complement: "no + * `GetTrackTable`" also holds on a build that fetches nothing at all, + * so the same spec opens Tracks and watches it arrive. + */ +import { test, expect, bindingCalls, navigateTo } from '../support/fixtures.js'; + +const TRACKS = 'library.Library.GetTrackTable'; + +/** The seed's default page, which is what a launch lands on. */ +const LANDING_VIEW = 'home'; + +test('launching on Home does not load the track list', async ({ app }) => { + // Past the store's idle warm-up: "nothing was fetched" has to mean + // nothing, not "nothing has happened yet". + await app.waitForTimeout(3_500); + + const calls = await bindingCalls(app); + + expect(calls, 'the warm-up for the small collections ran').toContain( + 'library.Library.GetAlbums', + ); + expect(calls, `the track list was fetched at launch (${LANDING_VIEW})`).not.toContain( + TRACKS, + ); +}); + +test('opening Tracks loads it, and only then', async ({ app }) => { + await app.waitForTimeout(1_000); + expect(await bindingCalls(app)).not.toContain(TRACKS); + + await navigateTo(app, 'tracks'); + await expect(app.getByTestId('track-row').first()).toBeVisible(); + + expect(await bindingCalls(app)).toContain(TRACKS); +}); + +test('a hover on the nav item starts the load before the click', async ({ app }) => { + await app.waitForTimeout(1_000); + + const nav = app.getByTestId('nav-tracks'); + + // The pointer entering the item is the whole mechanism; nothing is + // clicked, so the data must arrive because of the hover alone. + await nav.hover(); + await expect + .poll(async () => (await bindingCalls(app)).includes(TRACKS)) + .toBe(true); +}); diff --git a/frontend/index.html b/frontend/index.html index cebe75a..bdcd761 100644 --- a/frontend/index.html +++ b/frontend/index.html @@ -51,7 +51,15 @@
    - + +
    diff --git a/frontend/index.ts b/frontend/index.ts index d97d80a..0527839 100644 --- a/frontend/index.ts +++ b/frontend/index.ts @@ -200,13 +200,18 @@ const mainContent = document.getElementById('main-content'); // tracked as currentViewEl, is never hidden: two visible primary views // splitting the main panel between them regardless of which is // selected. +// +// It is deliberately not activated here. It is markup, not a decision: +// the shell does not yet know which view the launch lands on, and +// activating it starts the Tracks view's work — its data fetch, above +// all (#280) — for a launch that is about to land on Home. The first +// navigation is what activates whichever view it lands on. if (mainContent) { const initialTrackList = mainContent.querySelector('track-list'); if (initialTrackList) { viewCache.set('tracks', initialTrackList as HTMLElement); currentViewEl = initialTrackList as HTMLElement; - activateView(currentViewEl); } } @@ -580,10 +585,14 @@ async function handleNavigate( } default: { const fallback = document.createElement('div'); + const message = document.createElement('p'); fallback.style.padding = '1em'; fallback.style.color = 'var(--yj-text-secondary, #b3b3b3)'; - fallback.innerHTML = `

    Coming soon: ${view}

    `; + // textContent, not innerHTML: `view` comes from a navigation + // detail, which is app-supplied but not app-owned. + message.textContent = `Coming soon: ${view}`; + fallback.append(message); mainContent.appendChild(fallback); currentDetailEl = fallback; } @@ -620,6 +629,10 @@ function warmViewChunks(): void { /** requestIdleCallback where it exists; WebKit2GTK does not have it. */ function schedule(fn: () => void): void { + // SAFETY: `requestIdleCallback` is not in this project's DOM lib + // types, so it is read through an optional-property shape; the + // property is either absent (undefined) or the browser's own + // function, which is what the guard below checks. const ric = ( window as unknown as { requestIdleCallback?: (cb: () => void) => number; diff --git a/frontend/src/components/queue-panel/queue-panel.ts b/frontend/src/components/queue-panel/queue-panel.ts index 1bb1c2f..6bef4d6 100644 --- a/frontend/src/components/queue-panel/queue-panel.ts +++ b/frontend/src/components/queue-panel/queue-panel.ts @@ -1553,9 +1553,13 @@ export class QueuePanel indices: number[], ) { const queueTracks = this.queue.tracks; - const filePaths = indices - .map((i) => queueTracks[i]?.filePath) - .filter((fp): fp is string => fp != null); + const filePaths: string[] = []; + + for (const i of indices) { + const path = queueTracks[i]?.filePath; + + if (path != null) filePaths.push(path); + } await showBatchTrackDetailsForPaths( () => this.trackDetailsDialog, diff --git a/frontend/src/components/sidebar/app-sidebar.ts b/frontend/src/components/sidebar/app-sidebar.ts index 55696a7..ca7a99b 100644 --- a/frontend/src/components/sidebar/app-sidebar.ts +++ b/frontend/src/components/sidebar/app-sidebar.ts @@ -6,6 +6,7 @@ import { designTokens } from '../../styles/tokens.css'; import type { DragActiveDetail } from '@utils/drag-controller'; import { ActiveViewController } from '@store/controllers/active-view-controller'; import { ViewVisibilityController } from '@store/controllers/view-visibility-controller'; +import { libraryStore } from '@store/library-store'; import { VIEW_META } from '../../services/view-meta'; import type { View } from '../../services/view-meta'; @@ -352,6 +353,10 @@ export class AppSidebar extends LitElement { : 'false'} @click=${() => this.navigate(item.id)} + @mouseenter=${() => + this.prefetch(item.id)} + @focus=${() => + this.prefetch(item.id)} @dragover=${(e: DragEvent) => this.onNavDragOver( e, @@ -503,6 +508,19 @@ export class AppSidebar extends LitElement { } } + /** + * Start loading what this view draws, on hover or keyboard focus. + * + * The ~100 ms before the click is the whole point: #280 stopped + * fetching every collection at startup, and this is what keeps the + * view that *is* opened from paying the whole payload after the + * click. Nothing is awaited and nothing is reported here — the view + * itself reports a failure, and this is the same request. + */ + private prefetch(view: View) { + libraryStore.prefetch(view); + } + private navigate(view: View) { // No optimistic highlight: the shell answers, and it answers // synchronously in `handleNavigate` before it awaits anything. diff --git a/frontend/src/components/track-list/track-list.ts b/frontend/src/components/track-list/track-list.ts index df9d315..cbae111 100644 --- a/frontend/src/components/track-list/track-list.ts +++ b/frontend/src/components/track-list/track-list.ts @@ -1336,11 +1336,6 @@ export class TrackList super.connectedCallback(); this.restoreSortPreferences(); - if (this.externalTracks) { - this.tracks = this.externalTracks; - } else { - this.loadTracks(); - } this.resizeObserver = new ResizeObserver( () => { this.onHostResize(); @@ -1390,10 +1385,33 @@ export class TrackList this.resizeObserver = null; } + /** + * Fetch the list when this becomes the view on screen (#280). + * + * Not on connection: `index.html` renders a `` as the + * main panel's first-paint content, so a connection-time fetch was + * the whole library loaded at launch for a landing on Home — 12 MB + * at 50 000 tracks, and the backend's peak RSS with it. The shell + * activates this element only when a navigation lands on Tracks. + * + * Called on *every* activation, not just the first: the store may + * have refetched while this view was off screen, and `getTracks()` + * answers from its cache when nothing changed. + */ + protected override onViewActivate(): void { + if (this.externalTracks) { + this.tracks = this.externalTracks; + } else { + void this.loadTracks(); + } + + this.attachListListeners(); + } + /** Document-level listeners belong to the *visible* list. A cached * list is never disconnected, so this is the only place they can be * taken down again. */ - protected override onViewActivate(): void { + private attachListListeners(): void { this.listenWhileActive(document, 'click', this.clearSelectionHandler); this.listenWhileActive( document, @@ -1560,9 +1578,10 @@ export class TrackList this.selection.clear(); } - // Re-fetch when the store delivers fresh - // data after eager refetch on invalidation. - if (!this.externalTracks) { + // Re-fetch when the store delivers fresh data after an + // invalidation. Off screen the list does nothing with it, and + // the next activation re-reads the store (#280). + if (!this.externalTracks && this.viewActive) { const cached = this.libraryCtrl.cachedTracks; diff --git a/frontend/src/store/library-store.ts b/frontend/src/store/library-store.ts index 29eea8c..9aba95c 100644 --- a/frontend/src/store/library-store.ts +++ b/frontend/src/store/library-store.ts @@ -28,6 +28,14 @@ const COVER_SIZE_DEFAULT = 176; /** localStorage key for persisted cover size. */ const COVER_SIZE_KEY = 'cover-grid-size'; +/** + * How long the small-collection warm-up will wait for idle before + * running anyway. It is speculative, but a busy main thread must not + * mean Albums is slow to open — the timeout is the promise that it is + * only ever deferred, never skipped. + */ +const WARM_IDLE_TIMEOUT_MS = 3_000; + class LibraryStore { private tracks: ListTrack[] | null = null; private albums: library.Album[] | null = null; @@ -110,34 +118,82 @@ class LibraryStore { }); this.loadCoverSize(); - this.deferEagerFetch(); + this.warmSmallCollectionsOnIdle(); } /** - * Schedules eagerFetch() to run after the DOM is ready. - * The LibraryStore singleton is instantiated during ES module - * evaluation (import time), so calling eagerFetch() in the - * constructor would fire 4 backend roundtrips before the app - * shell has rendered. Deferring to the 'DOMContentLoaded' - * event (or calling immediately if the DOM is already parsed) - * lets the shell paint first, then begins data loading. + * Warm the three small collections once the app is idle after first + * paint, and deliberately not the tracks (#280). + * + * All four used to be fetched together at `DOMContentLoaded`, + * whichever view was showing. On a 26 138-track library the track + * list was 20.5 MB of that, and encoding it cost the backend ~170 MB + * of transient allocation — paid by someone looking at Home, which + * needs none of it. Measured on 50 000 tracks: 543 MB of backend RSS + * at rest before, 296 MB after, and 12.1 MB of binding bytes instead + * of 35.9. + * + * Albums, artists and genres are 1.6 MB together, so they are still + * fetched ahead of the click — that is what made those views + * instant. Tracks are 12 MB at 50 000 and are fetched by the view + * that draws them, or by `prefetch` from a hover. + * + * Idle rather than immediate: this is speculative, so it must not + * compete with the first paint. */ - private deferEagerFetch(): void { - if (document.readyState === 'loading') { - window.addEventListener( - 'DOMContentLoaded', - () => { - this.eagerFetch(); - }, - { once: true }, - ); + private warmSmallCollectionsOnIdle(): void { + const warm = () => { + const logged = this.failureReporter(); + + void this.getAlbums().catch(logged('albums')); + void this.getArtists().catch(logged('artists')); + void this.getGenres().catch(logged('genres')); + }; + + if (typeof requestIdleCallback === 'function') { + requestIdleCallback(warm, { timeout: WARM_IDLE_TIMEOUT_MS }); } else { - // DOM already parsed (shouldn't happen during module - // eval, but handles dynamic instantiation safely). - this.eagerFetch(); + setTimeout(warm, 0); } } + /** + * Start loading what a view will need, without waiting for it. + * + * A nav item calls this on hover or focus: that is the ~100 ms + * before the click, and it is what #280 trades for not paying for + * every collection at startup whether or not anyone goes there. + * + * A view this store holds nothing for is ignored rather than an + * error — it is a hint, and a hint about Home is not a mistake. + * The failure is swallowed here because the view that wanted the + * data reports it: this is the same request, already deduplicated + * by `inFlight`, and it has no caller to reject to. + */ + prefetch(view: string): void { + const started = (() => { + switch (view) { + case 'tracks': + return this.getTracks(); + case 'albums': + return this.getAlbums(); + case 'artists': + return this.getArtists(); + case 'genres': + return this.getGenres(); + default: + return null; + } + })(); + + void started?.catch(() => undefined); + } + + private failureReporter(): (what: string) => (err: unknown) => void { + return (what) => (err) => + console.error(`library: could not load ${what}`, err); + } + // =================================================================== // DATA ACCESS // Returns cached data or fetches from backend on first access. @@ -595,6 +651,8 @@ class LibraryStore { } } + const loaded = this.loadedCollections(); + this.albums = null; this.artists = null; this.genres = null; @@ -610,50 +668,63 @@ class LibraryStore { this.changeGen++; this.notify(); - const logged = (what: string) => (err: unknown) => - console.error(`library: could not reload ${what}`, err); - - void this.getAlbums().catch(logged('albums')); - void this.getArtists().catch(logged('artists')); - void this.getGenres().catch(logged('genres')); + // Only the summaries something was showing (#280): a removal + // nobody was looking at does not load a collection to correct it. + this.refetchLoaded({ + albums: loaded.albums, + artists: loaded.artists, + genres: loaded.genres, + }); } private invalidate(): void { + // Which collections something has actually loaded, before they + // are dropped. Refetching all four here would undo #280: a scan + // finishing would fetch the track list of a library nobody has + // opened the Tracks view on. + const loaded = this.loadedCollections(); + this.tracks = null; this.albums = null; this.artists = null; this.genres = null; // Anything still in flight was asked for on behalf of a - // selection that no longer applies: forget it, so the eager - // refetch below starts a request for the current one rather - // than adopting the old one's answer. + // selection that no longer applies: forget it, so the refetch + // below starts a request for the current one rather than + // adopting the old one's answer. this.inFlight.clear(); this.cacheGen++; this.changeGen++; this.scrollPositions = { tracks: 0, albums: 0, artists: 0, genres: 0 }; this.notify(); - this.eagerFetch(); + this.refetchLoaded(loaded); + } + + /** The collections currently held, keyed by the view that draws them. */ + private loadedCollections(): Partial> { + return { + tracks: this.tracks !== null, + albums: this.albums !== null, + artists: this.artists !== null, + genres: this.genres !== null, + }; } /** - * Fetches all library data. Called after DOM ready - * (initial load, via deferEagerFetch) and after cache - * invalidation so that controller subscribers receive - * fresh data on the next requestUpdate() cycle without - * needing their own LibraryScanComplete listener. + * Refetch exactly the collections named, and nothing else. + * + * A failed fetch is reported by whichever view asked for the data + * (it is that panel's failure, not the app's), but this refetch has + * no caller to reject to — without a catch it is an unhandled + * rejection. */ - private eagerFetch(): void { - // A failed fetch is reported by whichever view asked for the - // data (it is that panel's failure, not the app's), but the - // eager refetch has no caller to reject to — without a catch it - // is an unhandled rejection. - const logged = (what: string) => (err: unknown) => - console.error(`library: could not load ${what}`, err); + private refetchLoaded(loaded: Partial>): void { + const logged = this.failureReporter(); - void this.getTracks().catch(logged('tracks')); - void this.getAlbums().catch(logged('albums')); - void this.getArtists().catch(logged('artists')); - void this.getGenres().catch(logged('genres')); + if (loaded.tracks) void this.getTracks().catch(logged('tracks')); + if (loaded.albums) void this.getAlbums().catch(logged('albums')); + if (loaded.artists) void this.getArtists().catch(logged('artists')); + if (loaded.genres) void this.getGenres().catch(logged('genres')); } // =================================================================== diff --git a/frontend/test/components/chrome.test.ts b/frontend/test/components/chrome.test.ts index a60bf76..d797c10 100644 --- a/frontend/test/components/chrome.test.ts +++ b/frontend/test/components/chrome.test.ts @@ -11,7 +11,8 @@ import '@components/library-filter/library-filter'; import '@components/library-status-indicator/library-status-indicator'; import { Events } from '../../src/events'; import { activeViewStore } from '@store/active-view-store'; -import { emit, stub, flush, calls, lastArgs } from '@test/support/harness'; +import { libraryStore } from '@store/library-store'; +import { emit, stub, flush, calls, lastArgs, resetHarness } from '@test/support/harness'; import { fixture, shadow, @@ -181,6 +182,14 @@ describe('', () => { it('selects a library by id, and the merged view by empty string', async () => { const el = await fixture('library-filter'); + // The list has to be in use for the filter change to refetch it + // (#280): a collection nothing has loaded is not loaded to correct + // it, so this is the state the app is in when the Tracks view is up. + stub('library.Library.GetTrackTable', { strings: [''] }); + await libraryStore.getTracks(); + resetHarness(); + stub('library.Library.GetTrackTable', { strings: [''] }); + await flush(); await el.updateComplete; diff --git a/frontend/test/stores/library-lazy.test.ts b/frontend/test/stores/library-lazy.test.ts new file mode 100644 index 0000000..1c8dfdd --- /dev/null +++ b/frontend/test/stores/library-lazy.test.ts @@ -0,0 +1,99 @@ +/** + * #280: the app loads the library when a view needs it, not at startup. + * + * Every collection used to be fetched together at `DOMContentLoaded` + * and refetched on every invalidation, whether or not anything was + * showing it. On a 26 138-track library the track list was 20.5 MB of + * that and cost the backend ~170 MB of transient allocation — for + * someone looking at Home, which draws none of it. + * + * **This file must not load the track list before the first test.** It + * asserts the state the app is actually left in by its own startup, so + * a test added above that asks for tracks would invalidate the + * precondition rather than silently pass. + */ +import { describe, expect, it, beforeEach } from 'vitest'; + +import { libraryStore } from '@store/library-store'; +import { Events } from '../../src/events'; +import { + calls, + emit, + flush, + resetHarness, + stub, +} from '@test/support/harness'; + +const TRACKS = 'library.Library.GetTrackTable'; +const SMALL = [ + 'library.Library.GetAlbums', + 'library.Library.GetArtists', + 'library.Library.GetGenres', +]; + +/** + * Wait for the store's own idle warm-up to land. + * + * It asks for the three small collections on `requestIdleCallback`, so + * when it has happened is the browser's decision — waiting for the + * effect rather than for a duration is the only way this is not a race. + */ +function stubReads(): void { + stub(TRACKS, { strings: [''] }); + for (const path of SMALL) stub(path, []); +} + +async function waitForWarm(): Promise { + for (let i = 0; i < 200 && libraryStore.getCachedAlbums() === null; i++) { + await flush(); + } + + expect(libraryStore.getCachedAlbums(), 'the warm-up ran').not.toBeNull(); +} + +describe('what the app loads on its own (#280)', () => { + beforeEach(async () => { + stubReads(); + await waitForWarm(); + // resetHarness clears the stubs as well as the recorded calls. + resetHarness(); + stubReads(); + }); + + it('leaves the track list alone', () => { + expect(libraryStore.getCachedTracks()).toBeNull(); + expect(calls(TRACKS)).toEqual([]); + }); + + it('does not load it to answer a scan', async () => { + emit(Events.LibraryScanComplete); + await flush(); + + // The small collections are in use, so they are refreshed; the + // track list nobody has opened is not. + expect(calls().map((c) => c.path).sort()).toEqual([...SMALL].sort()); + }); + + it('loads it for the view that draws it, and refreshes it from then on', async () => { + libraryStore.prefetch('tracks'); + await flush(); + + expect(calls(TRACKS)).toHaveLength(1); + expect(libraryStore.getCachedTracks()).not.toBeNull(); + + resetHarness(); + emit(Events.LibraryScanComplete); + await flush(); + + expect(calls(TRACKS), 'in use now, so a scan refreshes it').toHaveLength(1); + }); + + it('is asked for nothing by a prefetch for a view it has no data for', async () => { + libraryStore.prefetch('home'); + libraryStore.prefetch('settings'); + libraryStore.prefetch('explore'); + await flush(); + + expect(calls()).toEqual([]); + }); +}); diff --git a/frontend/test/stores/library-store.test.ts b/frontend/test/stores/library-store.test.ts index 2bbf094..8fcf88b 100644 --- a/frontend/test/stores/library-store.test.ts +++ b/frontend/test/stores/library-store.test.ts @@ -43,16 +43,25 @@ function stubReads(): void { } /** - * Drop the cache and let the eager refetch settle, so each test starts - * from the same place. The store has no reset of its own; a scan - * completing is how the app itself clears it. + * Start each test from a loaded store. The store has no reset of its + * own; a scan completing is how the app itself clears it — and since + * #280 it reloads only what something had loaded, so the four reads + * here are what says "this test is about a loaded store". */ async function reload(): Promise { stubReads(); emit(Events.LibraryScanComplete); await flush(); - // The eager refetch the invalidation kicks off is recorded like any - // other call; clear it, or every count in every test is off by one. + resetHarness(); + stubReads(); + await Promise.all([ + libraryStore.getTracks(), + libraryStore.getAlbums(), + libraryStore.getArtists(), + libraryStore.getGenres(), + ]); + // Those reads are recorded like any other call; clear them, or every + // count in every test is off by one. resetHarness(); stubReads(); } @@ -91,7 +100,7 @@ describe('library store: caching', () => { ]).toEqual([['/a.mp3', '/b.mp3'], ALBUMS, ARTISTS, GENRES]); }); - it('refetches everything when a scan completes', async () => { + it('refetches everything a view had loaded when a scan completes', async () => { emit(Events.LibraryScanComplete); await flush(); -- 2.54.0 From c9b3048a2051c652688ab76609a3d038bdc05126 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Tue, 6 Oct 2026 01:51:56 -0400 Subject: [PATCH 06/10] perf(database): bound the read pool's mapping and cache by measurement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #283 read as "64 MB of mmap and up to 32 MB of page cache per pool", and it is — but on a 50 000-track library neither is what the process's memory is: the RSS of the database mapping is 46 MB whether the bound is 64 MB or 16 MB, because a mapping costs what the working set touches. Quartering it and halving the page cache moved no query either: 1 172 → 1 197 ms for the whole track list, 106 → 124 ms for the album list, 6 → 8 ms for an FTS search, all inside the run-to-run spread. Kept, because a bound that costs nothing measurable is worth having on a phone: five read connections' worth of mapping is 320 MB of address space at 64 MB each against 80 MB here, and #52 was a low-memory kill. Closes #283 --- backend/database/database.go | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/backend/database/database.go b/backend/database/database.go index a82fa72..893eea8 100644 --- a/backend/database/database.go +++ b/backend/database/database.go @@ -54,9 +54,19 @@ type DB struct { // form is silently ignored, which is why WAL was never actually on. const ( writeDSNParams = "?_pragma=busy_timeout(5000)&_pragma=journal_mode(WAL)" - readDSNParams = "?_pragma=busy_timeout(5000)&_pragma=journal_mode(WAL)" + + // The read pool's own page cache and mapping bound, sized by + // measurement rather than by appetite (#283). On a 50 000-track + // library, halving the cache and quartering the mapping moved no + // query — 1 172 → 1 197 ms for the whole track list, 106 → 124 ms + // for the album list, 6 → 8 ms for an FTS search, all inside the + // run-to-run spread — and the RSS of the database mapping was 46 MB + // under both settings, because a mapping's cost is what the working + // set touches, not the bound. The bound is what matters on a + // phone, where five connections' worth of address space is the + // thing the low-memory killer reads (cf. #52). + readDSNParams = "?_pragma=busy_timeout(5000)&_pragma=journal_mode(WAL)" + "&_pragma=query_only(true)&_pragma=synchronous(NORMAL)" + - "&_pragma=cache_size(-8000)&_pragma=mmap_size(67108864)" + "&_pragma=cache_size(-2000)&_pragma=mmap_size(16777216)" // readPoolConns bounds concurrent read connections. A handful is // plenty for interactive search + art/lookup fan-out and keeps WAL // reader overhead small. @@ -354,8 +364,8 @@ func applyPRAGMAs(ctx context.Context, db *sql.DB) error { pragmas := []string{ "PRAGMA foreign_keys = ON", "PRAGMA synchronous = NORMAL", - "PRAGMA cache_size = -8000", - "PRAGMA mmap_size = 67108864", + "PRAGMA cache_size = -2000", + "PRAGMA mmap_size = 16777216", } for _, pragma := range pragmas { -- 2.54.0 From bce8d27370df79adb25f91e632e6796e31eb2f85 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Tue, 6 Oct 2026 01:55:15 -0400 Subject: [PATCH 07/10] fix(explore): stop asking ListenBrainz after it refuses this client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every popularity request answered 401 on 2026-10-05: 399 in one minute of a real library's discography backfill, one rate-limited request per artist to be told the same thing, each with its own log line. A token is a property of the installation, not of the artist, so the first 401 is the answer for the rest of that client's life. The refusal is latched, logged once at warning level, and returned as ErrListenBrainzUnauthorized — separate from the generic HTTP error because it is the one failure no retry can fix. A 500 still does not latch, so a transient failure leaves the artist unmarked and the next run asks again. The backfill also stops feeding artists once the client is refused: the rest of the pass would be the same 401, and the artists stay unmarked for a run that has a token. Closes #284 --- backend/explore/listenbrainz.go | 49 +++++++++++ backend/explore/listenbrainz_refusal_test.go | 90 ++++++++++++++++++++ backend/explore/searchindex.go | 18 ++++ 3 files changed, 157 insertions(+) create mode 100644 backend/explore/listenbrainz_refusal_test.go diff --git a/backend/explore/listenbrainz.go b/backend/explore/listenbrainz.go index 725d47b..bc05729 100644 --- a/backend/explore/listenbrainz.go +++ b/backend/explore/listenbrainz.go @@ -13,6 +13,7 @@ import ( "net/http" "slices" "strings" + "sync/atomic" "time" ) @@ -25,6 +26,19 @@ const ( // responds with a non-2xx status code. var ErrListenBrainzHTTP = errors.New("listenbrainz HTTP error") +// ErrListenBrainzUnauthorized is a 401: the endpoint wants a token, and +// no retry will change that. +// +// It is separate from ErrListenBrainzHTTP because it is the one +// failure that is about *this client* rather than about the thing being +// asked for — which is what makes it the one worth latching. The +// popularity endpoints answered 401 to every request on 2026-10-05, so +// a discography backfill spent one rate-limited request per artist to +// be told the same thing: 399 of them in a minute of a real library's +// backfill, each one a log line and a wasted slot in the shared +// limiter. +var ErrListenBrainzUnauthorized = errors.New("listenbrainz requires a token") + // ListenBrainzClient is a thin HTTP client for the ListenBrainz // popularity and labs APIs. All requests are rate-limited via the // shared RateLimiter and cached via the shared Cache. @@ -39,6 +53,13 @@ type ListenBrainzClient struct { // SetBaseURL shape — so a test that points one client at an // httptest server does not stop being parallel-safe. baseURL string + + // refused latches the first 401. A token is a property of the + // installation, not of the artist being asked about, so the answer + // is the same for every later request and asking again is pure + // cost. Per client rather than global so a test can have one that + // is refused and one that is not. + refused atomic.Bool } // NewListenBrainzClient creates a ListenBrainz API client. @@ -56,6 +77,14 @@ func NewListenBrainzClient( } } +// Unauthorized reports whether this client has been refused with a 401 +// during its life. A caller that is about to do a long pass of +// requests should ask before starting it: the answer will not change +// mid-pass. +func (c *ListenBrainzClient) Unauthorized() bool { + return c.refused.Load() +} + // SetBaseURL redirects this client at another host. Tests only. func (c *ListenBrainzClient) SetBaseURL(url string) { c.baseURL = strings.TrimSuffix(url, "/") @@ -489,6 +518,11 @@ func (c *ListenBrainzClient) doPost( func (c *ListenBrainzClient) doRequest( ctx context.Context, method string, url string, body []byte, ) ([]byte, error) { + // Asked and answered, for the rest of this client's life. + if c.refused.Load() { + return nil, fmt.Errorf("%w: %s", ErrListenBrainzUnauthorized, url) + } + c.logger.Debug("listenbrainz rate limiter wait", "url", url) if err := c.limiter.Wait(ctx); err != nil { @@ -533,6 +567,21 @@ func (c *ListenBrainzClient) doRequest( "status", resp.StatusCode, ) + if resp.StatusCode == http.StatusUnauthorized { + // Recorded once, at warning level, because the next thing this + // client does is stop asking: a log line per artist is the + // symptom this latch exists to remove. + if c.refused.CompareAndSwap(false, true) { + c.logger.Warn("listenbrainz refused this client: "+ + "popularity data needs a token, so the rest of this "+ + "run will not ask for it", + "url", url, + ) + } + + return nil, fmt.Errorf("%w: %s", ErrListenBrainzUnauthorized, url) + } + if resp.StatusCode < 200 || resp.StatusCode >= 300 { return nil, fmt.Errorf( "%w: %d %s", ErrListenBrainzHTTP, resp.StatusCode, truncateBody(respBody), diff --git a/backend/explore/listenbrainz_refusal_test.go b/backend/explore/listenbrainz_refusal_test.go new file mode 100644 index 0000000..033fcf7 --- /dev/null +++ b/backend/explore/listenbrainz_refusal_test.go @@ -0,0 +1,90 @@ +package explore + +import ( + "context" + "errors" + "log/slog" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + + "yellowjacket/backend/database" +) + +// #284: the popularity endpoints answered 401 to every request, so a +// backfill spent one rate-limited request per artist to be told the same +// thing — 399 of them in a minute against a real library. A token is a +// property of the installation, not of the artist, so the first refusal +// is the answer for the whole client. +func TestListenBrainzLatchesARefusal(t *testing.T) { + t.Parallel() + + var requests atomic.Int64 + + srv := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + requests.Add(1) + w.WriteHeader(http.StatusUnauthorized) + }, + )) + t.Cleanup(srv.Close) + + c := NewListenBrainzClient( + NewRateLimiter(), NewCache(database.NewTestDB(t), slog.Default()), slog.Default(), + ) + c.SetBaseURL(srv.URL) + + for i := range 5 { + _, err := c.TopRecordingsForArtist(context.Background(), "an-mbid") + + if !errors.Is(err, ErrListenBrainzUnauthorized) { + t.Fatalf("call %d: err = %v, want ErrListenBrainzUnauthorized", i, err) + } + } + + if got := requests.Load(); got != 1 { + t.Errorf("requests = %d, want 1: the rest of the calls are the same answer", got) + } + + if !c.Unauthorized() { + t.Error("Unauthorized() = false after a 401") + } +} + +// A failure that a retry could fix must not latch: the artist stays +// unmarked and the next run asks again. +func TestListenBrainzDoesNotLatchATransientFailure(t *testing.T) { + t.Parallel() + + var requests atomic.Int64 + + srv := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + requests.Add(1) + w.WriteHeader(http.StatusInternalServerError) + }, + )) + t.Cleanup(srv.Close) + + c := NewListenBrainzClient( + NewRateLimiter(), NewCache(database.NewTestDB(t), slog.Default()), slog.Default(), + ) + c.SetBaseURL(srv.URL) + + for range 3 { + _, err := c.TopRecordingsForArtist(context.Background(), "an-mbid") + + if !errors.Is(err, ErrListenBrainzHTTP) { + t.Fatalf("err = %v, want ErrListenBrainzHTTP", err) + } + } + + if got := requests.Load(); got != 3 { + t.Errorf("requests = %d, want 3", got) + } + + if c.Unauthorized() { + t.Error("Unauthorized() = true after a 500") + } +} diff --git a/backend/explore/searchindex.go b/backend/explore/searchindex.go index 4e4c7c5..4832a4c 100644 --- a/backend/explore/searchindex.go +++ b/backend/explore/searchindex.go @@ -518,6 +518,13 @@ func (si *SearchIndex) BackfillLibraryDiscographies(ctx context.Context) { break } + // A refused client is refused for every artist: the rest of + // this pass would be the same 401, once per artist (#284). The + // artists stay unmarked, so a run with a token picks them up. + if indexLB.Unauthorized() { + break + } + work <- mbid } @@ -534,6 +541,17 @@ func (si *SearchIndex) BackfillLibraryDiscographies(ctx context.Context) { return } + if indexLB.Unauthorized() { + si.logger.Warn("discography backfill stopped early: "+ + "listenbrainz refused this client, and a token is what it wants", + "artists", total, "of", len(mbids), + ) + + job.logf(jobs.LevelWarn, "Stopped early: ListenBrainz needs a token") + + return + } + job.logf(jobs.LevelInfo, "Filled in "+strconv.Itoa(total)+" artists") si.logger.Info("discography backfill complete", "artists", total) -- 2.54.0 From 5b236ca5aff8385725cdbd6e7d46c82325e2fb24 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Tue, 6 Oct 2026 01:59:17 -0400 Subject: [PATCH 08/10] perf(library): patch what changed instead of reloading the list A retag named the one file it rewrote and the store threw the whole list away anyway: 20.5 MB across the IPC at 26 138 tracks to pick up one new title. A soft rescan of an unchanged library did the same, though its metrics say zero added, zero updated and zero removed. - `TrackMetadataChanged` with a `filePath` re-reads that one row through `trackCache` and splices it in, replacing the array so the memoized filter and sort caches notice. The summaries are still refetched: a retag can move a track between albums. A failed patch falls back to the full reload, because the answer that cannot be wrong is the one to keep. - A batch write names no paths (it can be thousands) and still reloads. - `LibraryScanComplete` reads its own metrics and reloads only when the scan changed something; a payload of an unknown shape is treated as a change, so nothing can leave a stale list on screen. Closes #282 --- frontend/src/store/library-store.ts | 119 ++++++++++++++++++++- frontend/test/stores/library-store.test.ts | 91 +++++++++++++++- 2 files changed, 205 insertions(+), 5 deletions(-) diff --git a/frontend/src/store/library-store.ts b/frontend/src/store/library-store.ts index 9aba95c..ddb8a4f 100644 --- a/frontend/src/store/library-store.ts +++ b/frontend/src/store/library-store.ts @@ -9,6 +9,7 @@ import { } from '@go/library/library.js'; import type * as library from '@go/library/models.js'; import { list } from '@utils/binding'; +import { trackCache } from './track-cache'; import { decodeTrackTable, type ListTrack } from '@utils/track-table'; import { Events } from '../events'; @@ -90,8 +91,8 @@ class LibraryStore { private cacheGen = 0; constructor() { - EventsOn(Events.LibraryScanComplete, () => { - this.invalidate(); + EventsOn(Events.LibraryScanComplete, (payload: unknown) => { + this.applyScanComplete(payload); }); EventsOn(Events.LibraryRemoved, () => { this.libraries = null; @@ -107,8 +108,8 @@ class LibraryStore { this.changeGen++; this.notify(); }); - EventsOn(Events.TrackMetadataChanged, () => { - this.invalidate(); + EventsOn(Events.TrackMetadataChanged, (payload: unknown) => { + this.applyMetadataChanged(payload); }); EventsOn(Events.TrackPlayCountChanged, (payload: unknown) => { this.applyPlayCount(payload); @@ -554,6 +555,116 @@ class LibraryStore { // INVALIDATION // =================================================================== + /** + * A scan finished. Reload only if it changed something (#282). + * + * `ScanMetrics` says how many files were added, updated and removed, + * and a soft rescan of an unchanged library reports zero of each — + * yet every loaded collection was thrown away and refetched, which + * at 26 138 tracks is 20.5 MB across the IPC for a scan that found + * nothing. Anything non-zero still reloads everything: a scan can + * change a tag, an album name or a genre on any file it touched, and + * it reports counts rather than paths. + * + * A payload that says nothing at all is treated as a change, not as + * a no-op: an unknown shape must not be able to leave a stale list + * on screen. + */ + private applyScanComplete(payload: unknown): void { + const m = payload as { + added?: number; + updated?: number; + removed?: number; + } | null; + + if (!m) { + this.invalidate(); + + return; + } + + const changed = (m.added ?? 0) + (m.updated ?? 0) + (m.removed ?? 0); + + if (changed === 0) return; + + this.invalidate(); + } + + /** + * Tags were rewritten on disk. A single file is patched; a batch is + * a reload (#282). + * + * The event names the one file it rewrote, and that file's row is + * the only row that changed — so re-reading the whole list to pick + * up one new title is 20.5 MB at 26 138 tracks. A batch write + * carries no paths (it can be thousands of files), and the summaries + * really do change with it, so that one still reloads. + */ + private applyMetadataChanged(payload: unknown): void { + const p = payload as { filePath?: string; batch?: boolean } | null; + + if (!p?.filePath || p.batch) { + this.invalidate(); + + return; + } + + this.patchOneTrack(p.filePath); + } + + /** + * Re-read one file's row and splice it in. + * + * The summaries are dropped and refetched, because a retag can move + * a track between albums and change a genre — they are the small + * collections, and they are what makes the album and artist views + * agree with the row that was just patched. + */ + private patchOneTrack(filePath: string): void { + const loaded = this.loadedCollections(); + + void trackCache + .refresh([filePath]) + .then(([fresh]) => { + if (!fresh || this.tracks === null) return; + + const idx = this.tracks.findIndex( + (t) => t.FilePath === filePath, + ); + + // Not in this view's list (another library's file, or + // gone): the list is right as it stands. + if (idx === -1) return; + + this.tracks = [ + ...this.tracks.slice(0, idx), + fresh, + ...this.tracks.slice(idx + 1), + ]; + this.changeGen++; + this.notify(); + }) + .catch((err: unknown) => { + // The patch failed, so the list may now be stale. Fall + // back to the answer that cannot be wrong. + console.error('library: could not re-read a retagged track', err); + this.invalidate(); + }); + + this.albums = null; + this.artists = null; + this.genres = null; + this.cacheGen++; + this.inFlight.delete('albums'); + this.inFlight.delete('artists'); + this.inFlight.delete('genres'); + this.refetchLoaded({ + albums: loaded.albums, + artists: loaded.artists, + genres: loaded.genres, + }); + } + /** * Patch one track's play statistics in place. * diff --git a/frontend/test/stores/library-store.test.ts b/frontend/test/stores/library-store.test.ts index 8fcf88b..8be486a 100644 --- a/frontend/test/stores/library-store.test.ts +++ b/frontend/test/stores/library-store.test.ts @@ -21,6 +21,12 @@ import { } from '@test/support/harness'; import { trackTable } from '@test/support/track-table'; +const RETAGGED = { + TrackName: 'Retagged', + FilePath: '/a.mp3', + PlayCount: 0, +}; + const TRACKS = [ { TrackName: 'One', FilePath: '/a.mp3', PlayCount: 0 }, { TrackName: 'Two', FilePath: '/b.mp3', PlayCount: 4 }, @@ -112,10 +118,93 @@ describe('library store: caching', () => { ]); }); - it('refetches when a track is retagged', async () => { + /* + * #282: a retag names the one file it rewrote, so the whole list does + * not have to come back — 20.5 MB at 26 138 tracks to pick up one new + * title. The summaries are still refetched, because a retag can move + * a track between albums. + */ + it('re-reads the one file a retag names, not the whole list', async () => { + stub('library.Library.GetTracksByPaths', [RETAGGED]); + emit(Events.TrackMetadataChanged, { filePath: '/a.mp3' }); await flush(); + expect(calls('library.Library.GetTrackTable')).toHaveLength(0); + expect(lastArgs('library.Library.GetTracksByPaths')).toEqual([['/a.mp3']]); + expect(calls().map((c) => c.path).sort()).toEqual([ + 'library.Library.GetAlbums', + 'library.Library.GetArtists', + 'library.Library.GetGenres', + 'library.Library.GetTracksByPaths', + ]); + }); + + it('splices the re-read row in, so consumers notice', async () => { + stub('library.Library.GetTracksByPaths', [RETAGGED]); + + const before = libraryStore.getCachedTracks(); + + emit(Events.TrackMetadataChanged, { filePath: '/a.mp3' }); + await flush(); + + const after = libraryStore.getCachedTracks(); + + expect(after?.map((t) => t.TrackName)).toEqual(['Retagged', 'Two']); + expect(after, 'a new array, which is what memoized consumers key on').not.toBe( + before, + ); + }); + + it('falls back to a full reload when the patch cannot be read', async () => { + stubFailure('library.Library.GetTracksByPaths', 'sql: database is locked'); + + emit(Events.TrackMetadataChanged, { filePath: '/a.mp3' }); + await flush(); + + expect(calls('library.Library.GetTrackTable')).toHaveLength(1); + }); + + it('reloads everything for a batch write, which names no paths', async () => { + emit(Events.TrackMetadataChanged, { batch: true, total: 40 }); + await flush(); + + expect(calls('library.Library.GetTrackTable')).toHaveLength(1); + expect(calls('library.Library.GetTracksByPaths')).toHaveLength(0); + }); + + /* + * #282, the other half: a soft rescan of an unchanged library reports + * zero added, updated and removed, and every loaded collection was + * thrown away and refetched anyway. + */ + describe('a scan that changed nothing', () => { + beforeEach(async () => { + emit(Events.LibraryScanComplete, { + added: 0, + updated: 0, + removed: 0, + skipped: 31, + }); + await flush(); + }); + + it('refetches nothing', () => { + expect(calls()).toEqual([]); + }); + + it('keeps the data it already had', () => { + expect(libraryStore.getCachedTracks()?.map((t) => t.FilePath)).toEqual([ + '/a.mp3', + '/b.mp3', + ]); + }); + }); + + it('reloads when a scan reports a change', async () => { + emit(Events.LibraryScanComplete, { added: 1, updated: 0, removed: 0 }); + await flush(); + expect(calls('library.Library.GetTrackTable')).toHaveLength(1); }); -- 2.54.0 From 2ed9e725e34ff6270538f36f15f42061c01fbd77 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Tue, 6 Oct 2026 01:59:57 -0400 Subject: [PATCH 09/10] docs(planning): what the library payloads cost, measured MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plan 023 (#279–#284): the before/after table on 50 000 tracks, and the six things worth keeping — including two measurements that pointed away from where the work had been (mmap is a bound, and the remaining second is Wails encoding every result twice). --- .planning/NOTES.md | 84 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/.planning/NOTES.md b/.planning/NOTES.md index b429d49..8643283 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -5097,3 +5097,87 @@ beside what they explain. These three did not: The drawer-style gutter would buy the affordance by taking width off a full-screen surface on a 424px viewport; back and a 44px close button answer it instead. + +## What the library payloads actually cost (measured 2026-10-05, desktop dev build) + +Plan 023 (#279–#284). Measured against `make sandbox-seed-bulk` +(50 000 tracks, 371 MB `yj.db` in the smaller copy) with the desktop +dev build and `e2e/perf/measure.mjs`, which grew a `memory` section for +it: backend RSS and peak from `/proc`, the Go heap from pprof, the +page's JS heap and DOM counters from CDP, and the bytes every binding +returned, before and after the first open of Tracks. + +| | before | after #281 | after #280 | +|---|---|---|---| +| Backend RSS at rest | 543 MB | 296 MB | 189 MB | +| Backend peak RSS | 571 MB | 337 MB | 281 MB | +| Go heap held | 361 MB | 125 MB | 7 MB | +| JS heap at rest | 31.8 MB | 18.0 MB | 5.2 MB | +| Binding bytes at rest | 35.9 MB | 12.1 MB | 1.7 MB | +| JS heap after a browse | 36.5 MB | 22.7 MB | 23.1 MB | +| Tracks first open → first row | 38 ms | 26 ms | 1 255 ms | +| Slowest first view open | 44 ms | 57 ms | 76 ms | + +Six things worth keeping: + +- **The number that fell was not the number being optimised.** The + binding payload fell 20.5 → 10.45 MB (dictionary-encoded columns) and + 35.9 → 12.1 MB, but what made the backend's RSS fall by 354 MB was + the *fetch not happening*: `GetTracks` was the only thing in the app + that allocated 170 MB transiently (`sqlcgen.GetTracks` 80 MB, `json/v2` + 128 MB, `slices.Grow` 77 MB, `bytes.Clone` 47 MB of 484 MB total + `alloc_space`). Encoding smaller would not have touched that. +- **A `dictionary` is what makes dropping fields the wrong trade.** The + first version of the column table left out `LastPlayed` and the three + larger cover tiers, and then a "select all → edit tags" over 50 000 + tracks had to fetch every track back to open the dialog: 3 071 ms + against 88 ms before. Interning means the repeated strings are nearly + free, so the fields belong in the table; what fixed it in the end was + neither — the Tracks view hands the dialog the rows it already has. + **A payload optimisation that removes a field is a new fetch waiting + to be written.** +- **`mmap_size` is a bound, not an allocation.** #283 read as "64 MB of + mapping per connection", and the RSS of the database mapping is 46 MB + whether the bound is 64 MB or 16 MB, because a mapping costs what the + working set touches. Quartering it moved no query either (1 172 → + 1 197 ms for the whole track list, 106 → 124 ms for the album list, 6 + → 8 ms for an FTS search — all inside the run-to-run spread). Kept for + the phone, where the bound is the address space the low-memory killer + reads. +- **A view that is first paint is a view that is active.** `index.html` + renders a ``, so the track list was connected at launch + and fetched 12 MB whoever was looking at — and the fix was not in the + component but in the shell: markup is not a decision about which view + the launch lands on, so the seed is `view-hidden` and the first + navigation is what activates it. Hover and keyboard focus on a nav + item prefetch, which is the ~100 ms before the click. +- **The remaining second is the transport, not the data.** A cold open + of Tracks on 50 000 tracks is 1 255 ms, and a *raw* binding call for + the same table is 1 118–1 382 ms: none of it is the TypeScript decode + or the render. It is the Go-side query, the column encode and Wails + encoding every result twice — once for a debug log that is off + (#286). Splitting that number is what says where to go next, and the + answer was not where the payload work had been. +- **A test named for the behaviour caught the thing the design missed.** + The source sweep that pins "the whole-library track array has one + reader" was written to catch the four call sites #279 had already + converted; it found `smart-playlist-editor`, which built its value + suggestions from the same array and would have gone silently empty + once nothing loaded it. The suggestion box is a backend query now + (`SuggestSmartPlaylistValues`). + +And two things about this machine, since they cost a cycle each: + +- **`make ui-test` is not reliable at load average 14.** Under the + workstation's own background services, the browser provider's module + fetches fail in a different handful of files each run ("Failed to + import test file", "Cannot connect to the iframe") while every test + that runs passes. `--maxWorkers=1 --retry=2` reduces it; individual + files always pass. A failure list that changes between runs is the + environment, not the branch. +- **A measurement run inherits the machine's mood.** The first + after-#281 numbers said view opens had doubled (albums 27 → 78 ms, + settings 44 → 179 ms, Tracks first row 38 → 105 ms). Re-running + unchanged gave 26 ms and 57 ms. The tell was the same one + `NOTES.md` already records twice: before and after suspiciously + equal, or suspiciously worse, across *unrelated* measurements. -- 2.54.0 From 655434dcaa217160b793757307ea43a1e35a4d2c Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Tue, 6 Oct 2026 02:36:28 -0400 Subject: [PATCH 10/10] test(e2e): sweep the binding names a spec calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four specs still called `library.Library.GetTracks` after #281 replaced it, and the suite reported twenty failures across transport, bottom-bar and reduced-motion specs — none of which mention the library list. A binding call carries only a method id, so a stale name fails at runtime in whichever spec happens to call it. `binding-names.spec.ts` reads every spec and harness file, extracts the names they pass as strings, and checks them against the map derived from the generated bindings tree. It asserts first that it read something: a sweep over an empty glob passes and proves nothing. It immediately found a second thing: `play-count.spec.ts` asserted that a play refetched no collection by matching a `GetAll` prefix, which matches exactly one real name (`GetAllLibrariesWithTrackCounts`) — so the assertion held whatever the app refetched. It names the four collection bindings now. --- e2e/bench-tmp.mjs | 27 ++++++++++ e2e/specs/binding-names.spec.ts | 80 ++++++++++++++++++++++++++++ e2e/specs/bottom-bar.spec.ts | 18 +++---- e2e/specs/phone-entity-links.spec.ts | 18 +++---- e2e/specs/phone-transport.spec.ts | 14 ++--- e2e/specs/play-count.spec.ts | 17 +++++- e2e/specs/reduced-motion.spec.ts | 18 ++----- e2e/support/fixtures.ts | 3 ++ 8 files changed, 149 insertions(+), 46 deletions(-) create mode 100644 e2e/bench-tmp.mjs create mode 100644 e2e/specs/binding-names.spec.ts diff --git a/e2e/bench-tmp.mjs b/e2e/bench-tmp.mjs new file mode 100644 index 0000000..ddb2517 --- /dev/null +++ b/e2e/bench-tmp.mjs @@ -0,0 +1,27 @@ +import { chromium } from '@playwright/test'; +const b = await chromium.launch({ executablePath: '/usr/bin/chromium' }); +const p = await b.newPage(); +await p.addInitScript({ path: '/mnt/vault/dev/golang/yellowjacket/.playwright/init-events.js' }); +await p.goto('http://localhost:34115', { waitUntil: 'load' }); +await p.evaluate(() => window.__yjEvents.ready(30000)); +const out = await p.evaluate(async () => { + const time = async (label, path, args, n = 5) => { + const ms = []; + for (let i = 0; i < n; i++) { + const t0 = performance.now(); + await window.__yjEvents.call(path, args, 60000); + ms.push(performance.now() - t0); + } + ms.sort((a, b) => a - b); + return { label, medianMs: Math.round(ms[Math.floor(n / 2)]), all: ms.map((m) => Math.round(m)) }; + }; + return [ + await time('GetTrackTable(0)', 'library.Library.GetTrackTable', [0], 4), + await time('GetAlbums(0)', 'library.Library.GetAlbums', [0], 4), + await time('SearchLocal(tide)', 'explore.Service.SearchLocal', ['tide'], 4), + await time('SearchLocal(rock)', 'explore.Service.SearchLocal', ['rock'], 4), + await time('SearchTracks(tide)', 'library.Library.SearchTracks', ['tide', 0], 4), + ]; +}); +console.log(JSON.stringify(out, null, 1)); +await b.close(); diff --git a/e2e/specs/binding-names.spec.ts b/e2e/specs/binding-names.spec.ts new file mode 100644 index 0000000..106ec1a --- /dev/null +++ b/e2e/specs/binding-names.spec.ts @@ -0,0 +1,80 @@ +/** + * Every bound method the suite names by hand must exist. + * + * A binding call carries only a method id, so a spec that names a method + * the Go side no longer has fails at *runtime*, in whichever spec + * happens to call it, with `unknown bound method name` — and nothing + * before that. `library.Library.GetTracks` was replaced by + * `GetTrackTable` (#281) and four specs kept calling the old name: the + * suite reported twenty failures across transport, bottom-bar and + * reduced-motion specs, none of which mention the library list. + * + * The known names come from `support/method-ids.mjs`, which derives them + * from the generated bindings tree that `make bindings-check` keeps + * current — so this cannot disagree with what the app can answer. + * + * It asserts first that it read something: a glob that matched nothing + * would pass over an empty list. + */ +import { readFileSync, readdirSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, join } from 'node:path'; + +import { test, expect } from '../support/fixtures.js'; +import { methodIDs } from '../support/method-ids.mjs'; + +const here = dirname(fileURLToPath(import.meta.url)); + +/** The specs, and the harness that calls bindings on their behalf. */ +const DIRS = [here, join(here, '..', 'support')]; + +function sources(): Array<[string, string]> { + const out: Array<[string, string]> = []; + + for (const dir of DIRS) { + for (const entry of readdirSync(dir)) { + if (!entry.endsWith('.ts')) continue; + if (entry.endsWith('.spec.ts') && entry === 'binding-names.spec.ts') continue; + + const path = join(dir, entry); + + out.push([path, readFileSync(path, 'utf8')]); + } + } + + return out; +} + +/** `'library.Library.GetTrackTable'` — package, service, method. */ +const NAMED_BINDING = /'([a-z][A-Za-z0-9]*\.[A-Z]\w*\.[A-Z]\w*)'/g; + +/** + * Names that are deliberately not bindings. + * + * `harness.spec.ts` calls an unknown method on purpose, to assert that + * the bridge rejects with a ReferenceError naming it rather than + * hanging — that spec is the reason a bad call is loud. + */ +const DELIBERATE = new Set(['queue.Queue.Nope']); + +test('every binding named by a spec exists', () => { + // `methodIDs()` is the id -> name map the recorder names calls with. + const known = new Set(methodIDs().values()); + const files = sources(); + + // A sweep over an empty glob passes and proves nothing. + expect(files.length, 'specs and support files read').toBeGreaterThan(20); + expect(known.size, 'bound method names derived').toBeGreaterThan(100); + + const unknown: string[] = []; + + for (const [path, source] of files) { + for (const [, name] of source.matchAll(NAMED_BINDING)) { + if (known.has(name!) || DELIBERATE.has(name!)) continue; + + unknown.push(`${name} (${path.split('/').pop()})`); + } + } + + expect([...new Set(unknown)]).toEqual([]); +}); diff --git a/e2e/specs/bottom-bar.spec.ts b/e2e/specs/bottom-bar.spec.ts index 734b4a9..d4a21ca 100644 --- a/e2e/specs/bottom-bar.spec.ts +++ b/e2e/specs/bottom-bar.spec.ts @@ -1,4 +1,10 @@ -import { test, expect, callBinding, NO_QUEUE_SOURCE } from '../support/fixtures.js'; +import { + test, + expect, + callBinding, + libraryTracks, + NO_QUEUE_SOURCE, +} from '../support/fixtures.js'; import type { Page } from '@playwright/test'; /** @@ -39,15 +45,7 @@ const geometry = (app: Page) => /** Something has to be playing before the transport draws a seek bar. */ async function play(app: Page): Promise { - const paths = await app.evaluate(async () => { - const tracks = (await window.__yjEvents.call( - 'library.Library.GetTracks', - [0], - 10_000, - )) as { FilePath: string }[]; - - return tracks.slice(0, 3).map((t) => t.FilePath); - }); + const paths = (await libraryTracks(app)).slice(0, 3).map((t) => t.FilePath); await callBinding(app, 'queue.Queue.SetQueue', [ paths, diff --git a/e2e/specs/phone-entity-links.spec.ts b/e2e/specs/phone-entity-links.spec.ts index 3f0c0d5..032e613 100644 --- a/e2e/specs/phone-entity-links.spec.ts +++ b/e2e/specs/phone-entity-links.spec.ts @@ -1,6 +1,7 @@ import { test, expect, + libraryTracks, callBinding, openTheQueue, NO_QUEUE_SOURCE, @@ -56,18 +57,11 @@ async function menuLabels(app: Page): Promise { * the wrong "no link". */ async function queueThree(app: Page): Promise { - const paths = await app.evaluate(async () => { - const tracks = (await window.__yjEvents.call( - 'library.Library.GetTracks', - [0], - 10_000, - )) as { FilePath: string; Album: string; ArtistName: string }[]; - - return tracks - .filter((t) => t.Album !== '' && t.ArtistName !== '') - .slice(0, 3) - .map((t) => t.FilePath); - }); + const tracks = await libraryTracks(app); + const paths = tracks + .filter((t) => t.Album !== '' && t.ArtistName !== '') + .slice(0, 3) + .map((t) => t.FilePath); await callBinding(app, 'queue.Queue.SetQueue', [ paths, diff --git a/e2e/specs/phone-transport.spec.ts b/e2e/specs/phone-transport.spec.ts index a6a40cc..3ab77e4 100644 --- a/e2e/specs/phone-transport.spec.ts +++ b/e2e/specs/phone-transport.spec.ts @@ -1,4 +1,4 @@ -import { test, expect } from '../support/fixtures.js'; +import { test, expect, libraryTracks } from '../support/fixtures.js'; /** * The phone's transport (#59, #56). @@ -64,19 +64,15 @@ async function sizeOf( /** Put something in the queue, so the transport has a track to act on. */ async function stageATrack(page: Page): Promise { - await page.evaluate(async () => { - const tracks = (await window.__yjEvents.call( - 'library.Library.GetTracks', - [0], - 10_000, - )) as { FilePath: string }[]; + const paths = (await libraryTracks(page)).slice(0, 4).map((t) => t.FilePath); + await page.evaluate(async (paths) => { await window.__yjEvents.call( 'queue.Queue.SetQueue', - [tracks.slice(0, 4).map((t) => t.FilePath), 0, false, { type: '', id: 0, label: '' }], + [paths, 0, false, { type: '', id: 0, label: '' }], 10_000, ); - }); + }, paths); } test.describe('the phone bar carries three controls', () => { diff --git a/e2e/specs/play-count.spec.ts b/e2e/specs/play-count.spec.ts index af24af0..b4eb625 100644 --- a/e2e/specs/play-count.spec.ts +++ b/e2e/specs/play-count.spec.ts @@ -40,6 +40,21 @@ const FINISH_TIMEOUT = 60_000; const libraryCalls = async (app: Page): Promise => (await bindingCalls(app)).filter((c) => c.startsWith('library.Library.')); +/** + * The bindings that fetch a collection. + * + * A prefix test (GetAll*) used to stand in for this, and matched + * exactly one name — GetAllLibrariesWithTrackCounts — so the assertion + * below held whatever the app refetched. The sweep in + * binding-names.spec.ts is what found it. + */ +const COLLECTION_FETCHES = [ + 'library.Library.GetTrackTable', + 'library.Library.GetAlbums', + 'library.Library.GetArtists', + 'library.Library.GetGenres', +]; + /** * Select rows by dispatching on the row rather than clicking it. * @@ -116,7 +131,7 @@ test.describe('a finished track', () => { const refetched = await libraryCalls(app); expect( - refetched.filter((c) => c.startsWith('library.Library.GetAll')), + refetched.filter((c) => COLLECTION_FETCHES.includes(c)), 'a play refetched a collection', ).toEqual([]); diff --git a/e2e/specs/reduced-motion.spec.ts b/e2e/specs/reduced-motion.spec.ts index 8eacb70..d65a37b 100644 --- a/e2e/specs/reduced-motion.spec.ts +++ b/e2e/specs/reduced-motion.spec.ts @@ -2,6 +2,7 @@ import { test, expect, callBinding, + libraryTracks, waitForEvent, NO_QUEUE_SOURCE, } from '../support/fixtures.js'; @@ -56,20 +57,9 @@ async function playTheLongOne(app: Page): Promise { window.dispatchEvent(new CustomEvent('yj-scroll-mode-changed')); }); - const paths: string[] = await app.evaluate(async (needle) => { - // One argument, and 0 means every library: the scoped and - // unscoped list queries collapsed into one when the schema did - // (plan 013 R3), so `GetTracks()` no longer exists to call. - const tracks = await window.__yjEvents.call( - 'library.Library.GetTracks', - [0], - 10_000, - ); - - return (tracks as { TrackName: string; FilePath: string }[]) - .filter((t) => t.TrackName.startsWith(needle)) - .map((t) => t.FilePath); - }, LONG_TITLE); + const paths: string[] = (await libraryTracks(app)) + .filter((t) => t.TrackName.startsWith(LONG_TITLE)) + .map((t) => t.FilePath); expect(paths.length).toBeGreaterThan(0); diff --git a/e2e/support/fixtures.ts b/e2e/support/fixtures.ts index ddd6f38..549237b 100644 --- a/e2e/support/fixtures.ts +++ b/e2e/support/fixtures.ts @@ -100,6 +100,7 @@ export async function callBinding( export interface LibraryTrack { FilePath: string; TrackName: string; + ArtistName: string; Album: string; } @@ -116,12 +117,14 @@ export async function libraryTracks(page: Page): Promise { strings: string[]; filePath: string[]; trackName: number[]; + artistName: number[]; album: number[]; }>(page, 'library.Library.GetTrackTable', [0]); return (t.filePath ?? []).map((FilePath, i) => ({ FilePath, TrackName: t.strings[t.trackName[i]!] ?? '', + ArtistName: t.strings[t.artistName[i]!] ?? '', Album: t.strings[t.album[i]!] ?? '', })); } -- 2.54.0