From 4bf59b45b76b6b58cb9017c2c993d0901676f334 Mon Sep 17 00:00:00 2001 From: Logan Date: Wed, 19 Aug 2026 00:37:46 -0400 Subject: [PATCH 1/6] feat(library): answer album completeness for a screenful in one query MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A card grid has to know how much of an album is here — an album held 2 tracks of 10 wearing the same green tick as one held whole is the complaint the badge-accuracy work was filed about — and `GetAlbumCompleteness` is one query per album, which is fifty round trips for a grid of fifty. `GetAlbumsCompleteness` is the same question over a slice. It is two grouping levels rather than the single-album form's correlated subqueries, because a correlated subquery in the FROM clause is not something SQLite will reliably do, and because the slice may only be spelled once or sqlc expands it twice with independently numbered placeholders. An album with no files is absent from the result rather than zeroed: "I have none of this" and "I have no idea" are the third state `Known` exists to keep apart. The test that matters is that the two spellings never disagree — they are genuinely different SQL, so the risk is a drift in meaning (a disc's total counted once per file, a duplicate counted twice) rather than a typo. --- backend/database/sql/queries/albums.sql | 46 ++++++++ backend/database/sql/sqlcgen/albums.sql.go | 93 +++++++++++++++ backend/library/completeness_test.go | 109 ++++++++++++++++++ backend/library/query.go | 59 ++++++++++ .../yellowjacket/backend/library/library.ts | 20 ++++ 5 files changed, 327 insertions(+) diff --git a/backend/database/sql/queries/albums.sql b/backend/database/sql/queries/albums.sql index 6ee599c..ec7001f 100644 --- a/backend/database/sql/queries/albums.sql +++ b/backend/database/sql/queries/albums.sql @@ -135,3 +135,49 @@ SELECT ) AS INTEGER) AS known FROM audio_files a WHERE a.album_id = sqlc.arg(album_id); + +-- name: GetAlbumsCompleteness :many +-- The same question as GetAlbumCompleteness, asked of a screenful of +-- albums at once. +-- +-- A card grid cannot afford one query per card, and the answer it wants +-- is the one thing a badge cannot guess: an album held 9 tracks of 12 +-- must show the count, never a bare tick. So this is one query for the +-- whole grid, asked only of the cards that have a local album id. +-- +-- It is two grouping levels rather than the single-album form's +-- correlated subqueries, because a correlated subquery in the FROM +-- clause is not something SQLite will reliably do -- and because the +-- slice may only be spelled once, or sqlc expands it twice with +-- independently numbered placeholders. +-- +-- The per-disc level is where the meaning is, and it is the same +-- meaning as the single-album query. `owned` counts DISTINCT track +-- numbers within a disc (this app detects duplicates, and counting two +-- files of track 3 twice would report a short album as complete), with +-- a file that declares no track number falling back to its own id +-- because three untagged files are three tracks and not one. +-- `expected` takes each disc's declared total and sums over discs, +-- since a total is declared per disc and a release total written on +-- every file of a two-disc album would double its expectation. A disc +-- whose files declared nothing contributes a NULL that SUM ignores, +-- and `known` is what says the album is therefore unanswerable. +WITH per_disc AS ( + SELECT + album_id AS album_id, + COUNT(DISTINCT COALESCE(CAST(track_number AS TEXT), 'f' || id)) + AS owned_on_disc, + MAX(total_tracks) AS disc_total, + SUM(CASE WHEN total_tracks IS NULL THEN 1 ELSE 0 END) + AS discs_without_a_total + FROM audio_files + WHERE album_id IN (sqlc.slice('album_ids')) + GROUP BY album_id, COALESCE(disc_number, 1) +) +SELECT + CAST(album_id AS INTEGER) AS album_id, + CAST(SUM(owned_on_disc) AS INTEGER) AS owned, + CAST(COALESCE(SUM(disc_total), 0) AS INTEGER) AS expected, + CAST(SUM(discs_without_a_total) = 0 AS INTEGER) AS known +FROM per_disc +GROUP BY album_id; diff --git a/backend/database/sql/sqlcgen/albums.sql.go b/backend/database/sql/sqlcgen/albums.sql.go index 709eb20..3acb204 100644 --- a/backend/database/sql/sqlcgen/albums.sql.go +++ b/backend/database/sql/sqlcgen/albums.sql.go @@ -8,6 +8,7 @@ package sqlcgen import ( "context" "database/sql" + "strings" ) const deleteAlbum = `-- name: DeleteAlbum :exec @@ -234,6 +235,98 @@ func (q *Queries) GetAlbumsByArtistName(ctx context.Context, arg GetAlbumsByArti return items, nil } +const getAlbumsCompleteness = `-- name: GetAlbumsCompleteness :many +WITH per_disc AS ( + SELECT + album_id AS album_id, + COUNT(DISTINCT COALESCE(CAST(track_number AS TEXT), 'f' || id)) + AS owned_on_disc, + MAX(total_tracks) AS disc_total, + SUM(CASE WHEN total_tracks IS NULL THEN 1 ELSE 0 END) + AS discs_without_a_total + FROM audio_files + WHERE album_id IN (/*SLICE:album_ids*/?) + GROUP BY album_id, COALESCE(disc_number, 1) +) +SELECT + CAST(album_id AS INTEGER) AS album_id, + CAST(SUM(owned_on_disc) AS INTEGER) AS owned, + CAST(COALESCE(SUM(disc_total), 0) AS INTEGER) AS expected, + CAST(SUM(discs_without_a_total) = 0 AS INTEGER) AS known +FROM per_disc +GROUP BY album_id +` + +type GetAlbumsCompletenessRow struct { + AlbumID int64 + Owned int64 + Expected int64 + Known int64 +} + +// The same question as GetAlbumCompleteness, asked of a screenful of +// albums at once. +// +// A card grid cannot afford one query per card, and the answer it wants +// is the one thing a badge cannot guess: an album held 9 tracks of 12 +// must show the count, never a bare tick. So this is one query for the +// whole grid, asked only of the cards that have a local album id. +// +// It is two grouping levels rather than the single-album form's +// correlated subqueries, because a correlated subquery in the FROM +// clause is not something SQLite will reliably do -- and because the +// slice may only be spelled once, or sqlc expands it twice with +// independently numbered placeholders. +// +// The per-disc level is where the meaning is, and it is the same +// meaning as the single-album query. `owned` counts DISTINCT track +// numbers within a disc (this app detects duplicates, and counting two +// files of track 3 twice would report a short album as complete), with +// a file that declares no track number falling back to its own id +// because three untagged files are three tracks and not one. +// `expected` takes each disc's declared total and sums over discs, +// since a total is declared per disc and a release total written on +// every file of a two-disc album would double its expectation. A disc +// whose files declared nothing contributes a NULL that SUM ignores, +// and `known` is what says the album is therefore unanswerable. +func (q *Queries) GetAlbumsCompleteness(ctx context.Context, albumIds []sql.NullInt64) ([]GetAlbumsCompletenessRow, error) { + query := getAlbumsCompleteness + var queryParams []interface{} + if len(albumIds) > 0 { + for _, v := range albumIds { + queryParams = append(queryParams, v) + } + query = strings.Replace(query, "/*SLICE:album_ids*/?", strings.Repeat(",?", len(albumIds))[1:], 1) + } else { + query = strings.Replace(query, "/*SLICE:album_ids*/?", "NULL", 1) + } + rows, err := q.db.QueryContext(ctx, query, queryParams...) + if err != nil { + return nil, err + } + defer rows.Close() + var items []GetAlbumsCompletenessRow + for rows.Next() { + var i GetAlbumsCompletenessRow + if err := rows.Scan( + &i.AlbumID, + &i.Owned, + &i.Expected, + &i.Known, + ); 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 getAlbumsWithPendingReleaseMBID = `-- name: GetAlbumsWithPendingReleaseMBID :many SELECT id, pending_release_mbid FROM albums WHERE pending_release_mbid IS NOT NULL AND pending_release_mbid != '' diff --git a/backend/library/completeness_test.go b/backend/library/completeness_test.go index 3d7e601..f2a3c30 100644 --- a/backend/library/completeness_test.go +++ b/backend/library/completeness_test.go @@ -232,3 +232,112 @@ func TestGetAlbumCompleteness_EmptyAlbum(t *testing.T) { t.Errorf("empty album reported %+v, want zero and unknown", got) } } + +// The batch and the single-album query are two spellings of one +// question, and the thing worth pinning is that they never disagree. +// +// They are genuinely different SQL — the single-album form is +// correlated subqueries over one album, the batch is two grouping +// levels over a slice — so the risk is not a typo but a drift in +// meaning: a disc's total counted once per file, a duplicate counted +// twice, a disc with no total silently covered by one that had one. +// Every shape the table above cares about is staged here at once, +// because a batch that is only ever asked about one album is not being +// asked the question that can go wrong. +func TestGetAlbumsCompletenessAgreesWithTheSingleAlbumQuery(t *testing.T) { + t.Parallel() + + lib, _ := setupTestLibrary(t) + + shapes := map[int][]track{ + 1: disc(1, 100, 12, 12), + 2: disc(1, 200, 9, 12), + 3: disc(1, 300, 13, 12), + 4: {{recordingID: 400, disc: 1, number: 1}}, + 5: append(disc(1, 500, 10, 10), disc(2, 600, 2, 5)...), + 6: append( + disc(1, 700, 10, 10), + track{recordingID: 750, disc: 2, number: 1}, + ), + 7: append( + disc(1, 800, 5, 6), + track{recordingID: 899, disc: 1, number: 3, total: 6}, + ), + } + + ids := make([]int64, 0, len(shapes)) + + for albumID, tracks := range shapes { + stageAlbum(t, lib, albumID, tracks) + ids = append(ids, albumIDFor(t, lib, albumID)) + } + + batch, err := lib.GetAlbumsCompleteness(ids) + if err != nil { + t.Fatalf("GetAlbumsCompleteness: %v", err) + } + + if len(batch) != len(ids) { + t.Fatalf("batch answered for %d albums, want %d", len(batch), len(ids)) + } + + for _, id := range ids { + one, err := lib.GetAlbumCompleteness(id) + if err != nil { + t.Fatalf("GetAlbumCompleteness(%d): %v", id, err) + } + + if got := batch[id]; got != one { + t.Errorf("album %d: batch says %+v, single says %+v", id, got, one) + } + } +} + +// An album with no files is absent from the batch, not zeroed. +// +// "I have none of this" and "I have no idea" are the third state Known +// exists to keep apart, and a caller reading a missing key gets nothing +// rather than a confident zero it would have to know to distrust. +func TestGetAlbumsCompletenessOmitsAnAlbumWithNoFiles(t *testing.T) { + t.Parallel() + + lib, _ := setupTestLibrary(t) + + stageAlbum(t, lib, 1, disc(1, 100, 3, 3)) + + held := albumIDFor(t, lib, 1) + + got, err := lib.GetAlbumsCompleteness([]int64{held, 4242}) + if err != nil { + t.Fatalf("GetAlbumsCompleteness: %v", err) + } + + if _, ok := got[4242]; ok { + t.Errorf("an album with no files answered %+v, want absent", got[4242]) + } + + if !got[held].Complete { + t.Errorf("held album reported %+v, want complete", got[held]) + } +} + +// A caller with nothing to ask about must not issue a query at all — +// sqlc's empty-slice branch rewrites the placeholder to NULL, which is +// a perfectly valid query returning nothing, so this is about the round +// trip rather than the answer. +func TestGetAlbumsCompletenessAsksNothingForAnEmptyList(t *testing.T) { + t.Parallel() + + lib, _ := setupTestLibrary(t) + + for _, ids := range [][]int64{nil, {}, {0}, {-1, 0}} { + got, err := lib.GetAlbumsCompleteness(ids) + if err != nil { + t.Fatalf("GetAlbumsCompleteness(%v): %v", ids, err) + } + + if len(got) != 0 { + t.Errorf("GetAlbumsCompleteness(%v) = %+v, want empty", ids, got) + } + } +} diff --git a/backend/library/query.go b/backend/library/query.go index b137980..efc05e0 100644 --- a/backend/library/query.go +++ b/backend/library/query.go @@ -244,6 +244,65 @@ func (l *Library) GetAlbumCompleteness(albumID int64) (AlbumCompleteness, error) }, nil } +// GetAlbumsCompleteness answers the same question for a screenful of +// albums in one query, keyed by album id. +// +// A card grid asks this about every card that has a local album behind +// it, and one query per card is how a grid of fifty albums becomes +// fifty round trips. The answer matters there for the reason it +// matters on the album page: an album held 9 tracks of 12 has to show +// the count, and a bare tick saying "in your library" is the complaint +// this whole rule came from. +// +// An album with no row in the result is one with no files, and it is +// absent rather than zeroed — "I have none of this" and "I have no +// idea" are the same third state `Known` exists to keep apart, and a +// caller reading a missing key gets nothing rather than a confident 0. +func (l *Library) GetAlbumsCompleteness( + albumIDs []int64, +) (map[int64]AlbumCompleteness, error) { + out := make(map[int64]AlbumCompleteness, len(albumIDs)) + + if len(albumIDs) == 0 { + return out, nil + } + + keys := make([]sql.NullInt64, 0, len(albumIDs)) + + for _, id := range albumIDs { + if id <= 0 { + continue + } + + keys = append(keys, sql.NullInt64{Int64: id, Valid: true}) + } + + if len(keys) == 0 { + return out, nil + } + + rows, err := l.db.ReadQueries.GetAlbumsCompleteness(l.ctx, keys) + if err != nil { + l.logger.Error("could not get album completeness in batch", + "albums", len(keys), "error", err) + + return nil, fmt.Errorf("could not get album completeness: %w", err) + } + + for _, row := range rows { + known := row.Known != 0 && row.Expected > 0 + + out[row.AlbumID] = AlbumCompleteness{ + Owned: int(row.Owned), + Expected: int(row.Expected), + Known: known, + Complete: known && row.Owned >= row.Expected, + } + } + + return out, nil +} + // GetAlbumTracks returns one album's tracks in disc/track order. func (l *Library) GetAlbumTracks(albumID, libraryID int64) ([]Track, error) { rows, err := l.db.ReadQueries.GetTracksByAlbum( diff --git a/frontend/bindings/yellowjacket/backend/library/library.ts b/frontend/bindings/yellowjacket/backend/library/library.ts index 7d15a11..0591049 100644 --- a/frontend/bindings/yellowjacket/backend/library/library.ts +++ b/frontend/bindings/yellowjacket/backend/library/library.ts @@ -98,6 +98,26 @@ export function GetAlbumsByArtist(artist: string, libraryID: number): $Cancellab return $Call.ByID(1456840721, artist, libraryID); } +/** + * GetAlbumsCompleteness answers the same question for a screenful of + * albums in one query, keyed by album id. + * + * A card grid asks this about every card that has a local album behind + * it, and one query per card is how a grid of fifty albums becomes + * fifty round trips. The answer matters there for the reason it + * matters on the album page: an album held 9 tracks of 12 has to show + * the count, and a bare tick saying "in your library" is the complaint + * this whole rule came from. + * + * An album with no row in the result is one with no files, and it is + * absent rather than zeroed — "I have none of this" and "I have no + * idea" are the same third state `Known` exists to keep apart, and a + * caller reading a missing key gets nothing rather than a confident 0. + */ +export function GetAlbumsCompleteness(albumIDs: number[] | null): $CancellablePromise<{ [_ in `${number}`]?: $models.AlbumCompleteness } | null> { + return $Call.ByID(531636827, albumIDs); +} + /** * GetAllLibrariesWithTrackCounts lists the libraries and their sizes. */ From 41c41a860e12f87c3e4c043f9ddab89c15ab04b4 Mon Sep 17 00:00:00 2001 From: Logan Date: Wed, 19 Aug 2026 00:37:57 -0400 Subject: [PATCH 2/6] feat(explore): carry the local row id on a top result MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `TopResult` was the one projection here that shipped `inLibrary` and no local id, so the top-results cards had no choice but to read the weaker flag. Every sibling model — `MBArtist`, `MBReleaseGroup`, `MBRecording` — already carries `LocalID`, and the candidate builders had the value in hand at every construction site. `LocalID` is set and cleared by a test against `audio_files`, so it means "there is something of mine here". `InLibrary` is written by the same pass but is a one-way ratchet the prune can only clear alongside a local id; it stays for scoring, which is where an approximate answer is fine. --- backend/explore/explore.go | 9 +++++++++ backend/explore/types.go | 11 ++++++++++- .../bindings/yellowjacket/backend/explore/models.ts | 9 +++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/backend/explore/explore.go b/backend/explore/explore.go index d0ef82f..6b9304f 100644 --- a/backend/explore/explore.go +++ b/backend/explore/explore.go @@ -2212,6 +2212,7 @@ func (e *Service) gatherTopCandidates( ArtistType: a.Type, Country: a.Country, InLibrary: a.InLibrary, + LocalID: a.LocalID, }, category: "artist", qualityScore: quality, @@ -2243,6 +2244,7 @@ func (e *Service) gatherTopCandidates( ArtistType: a.Type, Country: a.Country, InLibrary: a.InLibrary, + LocalID: a.LocalID, }, category: "artist", qualityScore: quality, @@ -2275,6 +2277,7 @@ func (e *Service) gatherTopCandidates( PrimaryType: rg.PrimaryType, Year: year, InLibrary: rg.InLibrary, + LocalID: rg.LocalID, }, category: "release_group", qualityScore: quality, @@ -2320,6 +2323,7 @@ func (e *Service) gatherTopCandidates( PrimaryType: rg.PrimaryType, Year: year, InLibrary: rg.InLibrary, + LocalID: rg.LocalID, }, category: "release_group", qualityScore: quality, @@ -2347,6 +2351,7 @@ func (e *Service) gatherTopCandidates( CAAReleaseMBID: r.CAAReleaseMBID, ReleaseName: r.ReleaseName, InLibrary: r.InLibrary, + LocalID: r.LocalID, }, category: "recording", qualityScore: quality, @@ -2403,6 +2408,7 @@ func (e *Service) gatherTopCandidates( CAAReleaseMBID: r.CAAReleaseMBID, ReleaseName: r.ReleaseName, InLibrary: r.InLibrary, + LocalID: r.LocalID, }, category: "recording", qualityScore: quality, @@ -2425,6 +2431,7 @@ func (e *Service) gatherTopCandidates( ArtistType: m.ArtistType, Country: m.Country, InLibrary: m.InLibrary || m.LocalArtistID > 0, + LocalID: m.LocalArtistID, }, category: "artist", qualityScore: quality, @@ -2445,6 +2452,7 @@ func (e *Service) gatherTopCandidates( PrimaryType: m.PrimaryType, Year: year, InLibrary: m.InLibrary || m.LocalReleaseGroupID > 0, + LocalID: m.LocalReleaseGroupID, }, category: "release_group", qualityScore: quality, @@ -2459,6 +2467,7 @@ func (e *Service) gatherTopCandidates( ArtistMBID: m.ArtistMBID, Length: m.Duration, InLibrary: m.InLibrary || m.LocalRecordingID > 0, + LocalID: m.LocalRecordingID, }, category: "recording", qualityScore: quality, diff --git a/backend/explore/types.go b/backend/explore/types.go index a00de9e..7cd2c6c 100644 --- a/backend/explore/types.go +++ b/backend/explore/types.go @@ -41,7 +41,16 @@ type TopResult struct { ReleaseGroupMBID string `json:"releaseGroupMbid,omitempty"` ReleaseName string `json:"releaseName,omitempty"` // Library status — populated from index cross-reference columns. - InLibrary bool `json:"inLibrary"` + // + // LocalID is the one the cards read. It is the local row behind + // this entity — an album, a file, an artist — and it is set and + // cleared by a test against `audio_files`, so it means "there is + // something of mine here". InLibrary is written by the same pass + // but is a one-way ratchet the prune can only clear alongside a + // local id, so it is the weaker of the two and stays for scoring + // (`fwInLibrary`), which is where an approximate answer is fine. + InLibrary bool `json:"inLibrary"` + LocalID int64 `json:"localId,omitempty"` } // MBArtist is a Wails-friendly projection of a MusicBrainz artist. diff --git a/frontend/bindings/yellowjacket/backend/explore/models.ts b/frontend/bindings/yellowjacket/backend/explore/models.ts index 54646db..8af5467 100644 --- a/frontend/bindings/yellowjacket/backend/explore/models.ts +++ b/frontend/bindings/yellowjacket/backend/explore/models.ts @@ -431,8 +431,17 @@ export interface TopResult { /** * Library status — populated from index cross-reference columns. + * + * LocalID is the one the cards read. It is the local row behind + * this entity — an album, a file, an artist — and it is set and + * cleared by a test against `audio_files`, so it means "there is + * something of mine here". InLibrary is written by the same pass + * but is a one-way ratchet the prune can only clear alongside a + * local id, so it is the weaker of the two and stays for scoring + * (`fwInLibrary`), which is where an approximate answer is fine. */ "inLibrary": boolean; + "localId"?: number; } /** From 19c68d73a74e0e97d8f0f584d829df3cc7f35098 Mon Sep 17 00:00:00 2001 From: Logan Date: Wed, 19 Aug 2026 00:38:05 -0400 Subject: [PATCH 3/6] fix(ui): keep the count in a partial badge that can act MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A control is named after what activating it does, so an actionable badge said "Request album X" — and `partial` is actionable, because an album you hold nine of twelve tracks of has three left to ask for. That made the one state the ring exists for the one state whose name did not mention it. The argument the `partial` branch already carries does not stop applying because the badge became clickable: a ring says "some" to a sighted user and nothing to anyone else. The name is now the action and the count. --- .../library-status-indicator.ts | 20 +++++++++++--- .../library-status-indicator.test.ts | 27 +++++++++++++++++++ 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/frontend/src/components/library-status-indicator/library-status-indicator.ts b/frontend/src/components/library-status-indicator/library-status-indicator.ts index 1186c16..5fb8fbe 100644 --- a/frontend/src/components/library-status-indicator/library-status-indicator.ts +++ b/frontend/src/components/library-status-indicator/library-status-indicator.ts @@ -297,9 +297,23 @@ export class LibraryStatusIndicator extends LitElement { // row to one and nothing to the other, and "Add … to library" // was the old button's promise written into the copy. if (this.actionable) { - return this.status === 'queued' - ? `Cancel the request for ${kind}${name}` - : `Request ${kind}${name}`; + if (this.status === 'queued') { + return `Cancel the request for ${kind}${name}`; + } + + // A partly-held album is actionable *and* has a count, and + // the count does not survive being named after the action + // alone. The `partial` case below says why it matters — a + // ring says "some" to a sighted user and nothing to anyone + // else — and that argument does not stop applying because + // the badge became clickable. This branch used to say only + // "Request album X", so the one state the ring exists for + // was the one state whose name did not mention it. + if (this.status === 'partial') { + return `Request the rest of ${kind}${name} — ${this.owned} of ${this.expected} tracks are in your library`; + } + + return `Request ${kind}${name}`; } switch (this.status) { diff --git a/frontend/test/components/library-status-indicator.test.ts b/frontend/test/components/library-status-indicator.test.ts index fca6361..64aae0b 100644 --- a/frontend/test/components/library-status-indicator.test.ts +++ b/frontend/test/components/library-status-indicator.test.ts @@ -77,3 +77,30 @@ describe('the library status badge', () => { expect(name).toContain('Glass Harbour'); }); }); + +/** + * A partly-held album is the one state that is *actionable and + * counted*: there are tracks left to ask for, so the badge is a button + * — and a control is named after what activating it does, which is how + * the count came to be dropped from exactly the state the ring exists + * for. Both, or the ring says "some" to an eye and nothing to anyone + * else. + */ +describe('a partial badge that can act', () => { + it('names the action and keeps the count', async () => { + const el = await fixture('library-status-indicator', { + status: 'partial', + owned: 9, + expected: 12, + entityType: 'album', + label: 'Glass Harbour', + requestMbid: 'rg-1', + }); + + const name = shadow(el, '.badge')?.getAttribute('aria-label') ?? ''; + + expect(shadow(el, 'button.badge')).not.toBeNull(); + expect(name).toContain('Request the rest of'); + expect(name).toContain('9 of 12'); + }); +}); From 88fc50afb87f33543d11881a50067904748287ec Mon Sep 17 00:00:00 2001 From: Logan Date: Wed, 19 Aug 2026 00:38:22 -0400 Subject: [PATCH 4/6] feat(explore): mark what is not owned, everywhere it can be shown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `explore-album-details` had the rule right for one tracklist and nothing else did: Explore's cards, `top-results-row` and the artist page's three card shapes all mixed owned and unowned with a small badge as the only difference, and drew a green tick on the *common* case — which is the treatment that tracklist's own green ticks were removed for. `utils/ownership.ts` is the rule written once, so eight call sites stop each holding their own version: - owned is plain, and draws no badge at all; - unowned is dimmed *and* says so in its accessible name, because dimming is a colour and cannot be the only signal; - a partly-held album says how partly. **Ownership is a file, and `localId` is the flag that says so.** The album page answers with `filePaths`, a real file per displayed track; a card grid cannot afford that and does not need to, because `local_*_id` is built by queries that all join `audio_files` and cleared by a prune whose existence test is a file test in every case. `inLibrary` is written by the same pass, so the two agree in a healthy database — but it is a one-way ratchet (`MAX(in_library, excluded)`) whose only clearing pass is gated on a non-null local id, so it cannot be un-set on its own. Where they already diverged was the client. Both `explore-view` and `explore-artist-details` kept a `libraryMBIDs` set that accumulated every MBID ever seen with `inLibrary` and cleared it never, in views that never unmount. Both are deleted. And one card answered the question twice and got two answers: `renderReleaseMenuItems` gates Play on `localId > 0` while the badge and `albumTarget.owned` used `inLibrary`, so an album with the flag and no local row drew a tick saying it was in your library, offered no Play, and — the request item being gated on *not* owned — offered no way to ask for it either. The count comes from `completenessStore`, shaped like `credit-store`: `request()` is per-card and coalesces a screenful into one `GetAlbumsCompleteness`, absence is cached as an answer, and the whole cache is dropped on a scan, a retag or a removal rather than aged. `aria-disabled` goes on rows that cannot be activated and deliberately not on cards: an unowned card still navigates to the catalog page for it, which is a perfectly good thing to do with something you do not own. Audited and unchanged: `home-view`, `downloads-view`, `cover-grid`, `artist-details` and `genre-details` cannot show catalog content, so everything on them is owned and "owned is plain" is already what they do. The album page's own header badge stays, because that page is about one entity and the badge is its answer rather than a mark on one of many. Closes #38 --- .../explore-album-details.ts | 26 +- .../explore-artist-details.ts | 150 +++++--- .../components/explore-view/explore-view.ts | 122 +++--- .../top-results-row/top-results-row.ts | 57 ++- frontend/src/store/completeness-store.ts | 208 ++++++++++ frontend/src/utils/library-status.ts | 57 +++ frontend/src/utils/ownership.ts | 134 +++++++ .../components/unowned-everywhere.test.ts | 363 ++++++++++++++++++ 8 files changed, 968 insertions(+), 149 deletions(-) create mode 100644 frontend/src/store/completeness-store.ts create mode 100644 frontend/src/utils/ownership.ts create mode 100644 frontend/test/components/unowned-everywhere.test.ts diff --git a/frontend/src/components/explore-album-details/explore-album-details.ts b/frontend/src/components/explore-album-details/explore-album-details.ts index e55352b..a730b58 100644 --- a/frontend/src/components/explore-album-details/explore-album-details.ts +++ b/frontend/src/components/explore-album-details/explore-album-details.ts @@ -3,6 +3,7 @@ import { customElement, property, state, query } from 'lit/decorators.js'; import { classMap } from 'lit/directives/class-map.js'; import { designTokens } from '../../styles/tokens.css'; import { srOnly } from '../../styles/sr-only.css'; +import { unownedLabel, unownedStyles } from '@utils/ownership'; import { LookupReleaseGroup, BrowseReleases, @@ -316,6 +317,7 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { exploreLinkStyles, contextMenuStyles, srOnly, + unownedStyles, css` :host { display: flex; @@ -687,20 +689,14 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { white-space: nowrap; } - /* A track the library does not have, on the pattern a - * streaming service uses for something it cannot play: the - * row stays, dimmed, so the album reads as the album rather - * than as the subset that happens to be here. - * - * The dimming is a colour, so it cannot be the only signal - * — the row also carries aria-disabled, which is what - * reaches anyone not seeing it. Secondary rather than - * tertiary because the row's hover background is - * bgOverlay, which tertiary does not clear. */ - .track-row.unowned .track-title { - color: var(--yj-text-secondary, #b3b3b3); - font-weight: 400; - } + /* The dimming itself is unownedStyles, from + * utils/ownership.ts, imported above. It was written here + * first — this tracklist is where the treatment came from — + * and moved out when seven other surfaces had to draw the + * same thing, because two of them would otherwise have + * ended up drawing it slightly differently. (No backticks or + * apostrophes-as-quotes here: this is inside a tagged + * template literal.) */ /* The request control is offered on every row that has * something to request, and is not revealed on hover. @@ -3273,7 +3269,7 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { aria-disabled=${owned ? 'false' : 'true'} aria-label=${owned ? `Play “${track.title}”` - : `${track.title} — not in your library`} + : unownedLabel(track.title, 'track')} @dblclick=${() => this.onTrackRowDblClick(track)} @contextmenu=${(e: MouseEvent) => this.onTrackContextMenu(e, track)} @keydown=${(e: KeyboardEvent) => this.onTrackRowKeydown(e, track)} diff --git a/frontend/src/components/explore-artist-details/explore-artist-details.ts b/frontend/src/components/explore-artist-details/explore-artist-details.ts index 9c2389e..d7f11a8 100644 --- a/frontend/src/components/explore-artist-details/explore-artist-details.ts +++ b/frontend/src/components/explore-artist-details/explore-artist-details.ts @@ -39,7 +39,17 @@ import { EventsOn } from '@runtime/runtime'; import { Events } from '../../events'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; import '../library-status-indicator/library-status-indicator.js'; -import { libraryStatusFor, toggleRequest } from '@utils/library-status'; +import { + albumBadgeFor, + libraryStatusFor, + toggleRequest, +} from '@utils/library-status'; +import { + isOwned, + ownershipLabel, + unownedStyles, +} from '@utils/ownership'; +import { completenessStore } from '@store/completeness-store'; import '../catalog-scope-notice/catalog-scope-notice.js'; import type { CatalogScope } from '../catalog-scope-notice/catalog-scope-notice.js'; import { queueStore } from '../../store/queue-store'; @@ -178,7 +188,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost @state() private discoRowSize = 5; private discoObserver?: ResizeObserver; @state() private similarExpanded = false; - private libraryMBIDs = new Set(); /* ── Release prefetch ── */ @@ -257,6 +266,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost designTokens, exploreLinkStyles, contextMenuStyles, + unownedStyles, css` :host { display: flex; @@ -995,6 +1005,9 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost /** Unsubscribe handle for the requests list. */ private unsubRequests: (() => void) | null = null; + /** Unsubscribes the "how much of this album is here" repaint. */ + private unsubCompleteness: (() => void) | null = null; + override connectedCallback() { super.connectedCallback(); if (this.artistMBID || this.localArtistId) { @@ -1007,6 +1020,12 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost this.unsubRequests = downloadStore.subscribe(() => this.requestUpdate()); void downloadStore.init().then(() => this.requestUpdate()); + // The count behind a partly-held album lands a frame after the + // cards do, since the store batches a screenful into one query. + this.unsubCompleteness = completenessStore.subscribe(() => + this.requestUpdate(), + ); + // A background discography fetch (top tracks / top releases for an // artist that wasn't indexed yet) finished — re-fetch those two // sections, once per artist, so they fill in without the initial @@ -1045,6 +1064,8 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost super.disconnectedCallback(); this.unsubRequests?.(); this.unsubRequests = null; + this.unsubCompleteness?.(); + this.unsubCompleteness = null; this.unsubDiscogReady?.(); this.unsubSimilarReady?.(); if (this.discogFallbackTimer) clearTimeout(this.discogFallbackTimer); @@ -1573,10 +1594,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost this.catalogPending = false; } - // Populate libraryMBIDs from the inLibrary flag (already - // set by the backend via local_release_group_id cross-ref). - this.checkLibrary(); - // Batch-resolve cover art for discography (lower priority — loaded after top sections). void this.batchResolveThumbnails( rgs?.map((r) => ({ mbid: r.mbid, albumName: r.title, artistName: r.artistCredit })) @@ -1903,23 +1920,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost } } - private checkLibrary() { - // Backend now populates `inLibrary` directly on each MBReleaseGroup - // via the local_release_group_id cross-reference column. Just read it. - let updated = false; - - for (const rg of this.releaseGroups) { - if (rg.mbid && rg.inLibrary && !this.libraryMBIDs.has(rg.mbid)) { - this.libraryMBIDs.add(rg.mbid); - updated = true; - } - } - - if (updated) { - this.requestUpdate(); - } - } - /* ── Playback ── */ /** @@ -2063,7 +2063,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost } private isTrackOwned(track: LBTopRecording): boolean { - return Boolean(track.inLibrary || track.localId); + return isOwned(track); } private onTrackRowDblClick(track: LBTopRecording): void { @@ -2106,7 +2106,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost mbid: rg.releaseGroupMbid || '', localId: rg.localId ?? 0, title: rg.title, - owned: Boolean(rg.inLibrary || rg.localId), + owned: isOwned(rg), }; } @@ -2126,10 +2126,12 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost mbid: isLocal ? '' : rg.mbid || '', localId: Number.isFinite(localId) ? localId : 0, title: rg.title, - owned: - this.libraryMBIDs.has(rg.mbid) || - Boolean(rg.inLibrary) || - localId > 0, + // The same answer the menu gates Play on, which is the + // point: this used to be `inLibrary` too, so a card could + // report itself owned, be offered no Play (that item is + // gated on the local id) and be offered no request either + // (that one is gated on *not* owned). + owned: localId > 0, }; } @@ -2944,15 +2946,20 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost

Top Tracks

- ${tracks.map( - (t, i) => html` + ${tracks.map((t, i) => { + const owned = this.isTrackOwned(t); + + return html`
this.onTrackRowDblClick(t)} @contextmenu=${(e: MouseEvent) => this.onTrackContextMenu(e, t)} @keydown=${(e: KeyboardEvent) => this.onTrackRowKeydown(e, t)} @@ -2977,16 +2984,18 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost ${formatListenCount(t.totalListenCount)} plays - + ${owned + ? nothing + : html``}
- `, - )} + `; + })}
${canExpandTracks ? html` @@ -3057,10 +3066,16 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost private renderTopReleaseCard(rg: LBTopReleaseGroup) { const artURL = this.thumbnailURLs.get(rg.releaseGroupMbid) || ''; const target = this.topReleaseTarget(rg); + const owned = target.owned; + const badge = albumBadgeFor( + { localId: target.localId }, + rg.releaseGroupMbid, + ); return html`
this.navigateToTopRelease(rg)} role="button" tabindex="0" @@ -3092,14 +3107,18 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
${rg.date ? html`${extractYear(rg.date)}` : nothing}
- + ${badge.status === 'in-library' + ? nothing + : html``}
@@ -3189,14 +3208,15 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost private renderAlbumCard(rg: MBReleaseGroup) { const artURL = this.thumbnailURLs.get(rg.mbid) || ''; const year = extractYear(rg.firstReleaseDate); - const inLibrary = this.libraryMBIDs.has(rg.mbid) || Boolean(rg.inLibrary); - const status = libraryStatusFor(inLibrary, rg.mbid); const target = this.albumTarget(rg); + const owned = target.owned; + const badge = albumBadgeFor({ localId: target.localId }, target.mbid); return html`
this.navigateToAlbum(rg)} role="button" tabindex="0" @@ -3225,13 +3245,17 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
${year ? html`${year}` : nothing}
- + ${badge.status === 'in-library' + ? nothing + : html``}
`; diff --git a/frontend/src/components/explore-view/explore-view.ts b/frontend/src/components/explore-view/explore-view.ts index b67e003..0925d04 100644 --- a/frontend/src/components/explore-view/explore-view.ts +++ b/frontend/src/components/explore-view/explore-view.ts @@ -1,5 +1,11 @@ import { avatarBackground } from '@utils/avatar-color'; -import { libraryStatusFor } from '@utils/library-status'; +import { albumBadgeFor, libraryStatusFor } from '@utils/library-status'; +import { + isOwned, + ownershipLabel, + unownedStyles, +} from '@utils/ownership'; +import { completenessStore } from '@store/completeness-store'; import { downloadStore } from '@store/download-store'; import { LitElement, html, css, nothing } from 'lit'; import { customElement, state, query as litQuery } from 'lit/decorators.js'; @@ -171,7 +177,6 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte private searchDebounceTimer?: ReturnType; private thumbnailCache = new LRUMap(THUMBNAIL_CACHE_LIMIT); private artistImageCache = new LRUMap(ARTIST_IMAGE_CACHE_LIMIT); - private libraryMBIDs = new Set(); constructor() { super(); @@ -226,6 +231,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte srOnly, exploreLinkStyles, contextMenuStyles, + unownedStyles, css` :host { display: block; @@ -812,6 +818,13 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte // Explore should not pay for it. this.whileActive(downloadStore.subscribe(() => this.requestUpdate())); void downloadStore.init().then(() => this.requestUpdate()); + + // How much of an owned album is here arrives a frame after the + // cards do — the store coalesces a screenful into one query — + // so a card that turns out to be 9 of 12 repaints when the + // answer lands rather than showing a plain tick until something + // else happens to re-render the grid. + this.whileActive(completenessStore.subscribe(() => this.requestUpdate())); } /** A debounced search that lands after the user has left the page is @@ -1083,7 +1096,6 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte this.results?.artists ?? [], this.results?.releaseGroups ?? [], ); - this.checkLibrary(); } catch (err) { if (version !== this.searchVersion) return; @@ -1665,42 +1677,6 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte } } - /** - * Check which result MBIDs exist in the local library. - */ - private checkLibrary() { - if (!this.results) return; - - // Backend now populates `inLibrary` directly on each MB result - // via the local_*_id cross-reference columns. Just read those. - let updated = false; - - for (const a of this.results.artists ?? []) { - if (a.mbid && a.inLibrary && !this.libraryMBIDs.has(a.mbid)) { - this.libraryMBIDs.add(a.mbid); - updated = true; - } - } - - for (const rg of this.results.releaseGroups ?? []) { - if (rg.mbid && rg.inLibrary && !this.libraryMBIDs.has(rg.mbid)) { - this.libraryMBIDs.add(rg.mbid); - updated = true; - } - } - - for (const r of this.results.recordings ?? []) { - if (r.mbid && r.inLibrary && !this.libraryMBIDs.has(r.mbid)) { - this.libraryMBIDs.add(r.mbid); - updated = true; - } - } - - if (updated) { - this.requestUpdate(); - } - } - /* ── Navigation ── */ private navigateToArtist(artist: MBArtist) { @@ -2107,12 +2083,16 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte : nothing}
${artists.map((a) => { + const owned = isOwned(a); + const name = a.englishName || a.name; + return html`
this.navigateToArtist(a)} role="button" tabindex="0" + aria-label=${ownershipLabel(owned, 'Artist', name, 'artist')} @keydown=${(e: KeyboardEvent) => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); @@ -2171,11 +2151,17 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte const artURL = this.thumbnailCache.get(rg.mbid) || ''; const year = extractYear(rg.firstReleaseDate); - const owned = Boolean(rg.localId); + const owned = isOwned(rg); + const badge = albumBadgeFor(rg, rg.mbid); return html`
this.navigateToAlbum(rg)} @dblclick=${() => this.onAlbumCardDblClick(rg)} @contextmenu=${(e: MouseEvent) => @@ -2228,13 +2214,17 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte : nothing} ${year ? html`${year}` : nothing}
- + ${badge.status === 'in-library' + ? nothing + : html``}
`; @@ -2249,12 +2239,20 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte

Tracks

- ${recordings.map( - (r) => html` + ${recordings.map((r) => { + const owned = isOwned(r); + + return html`
this.onRecordingRowDblClick(r)} @contextmenu=${(e: MouseEvent) => this.onExploreContextMenu(e, { @@ -2291,16 +2289,18 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte ? html`${formatDuration(r.length)}` : nothing}
- + ${owned + ? nothing + : html``}
- `, - )} + `; + })}
`; diff --git a/frontend/src/components/top-results-row/top-results-row.ts b/frontend/src/components/top-results-row/top-results-row.ts index 50ed673..f25e6ad 100644 --- a/frontend/src/components/top-results-row/top-results-row.ts +++ b/frontend/src/components/top-results-row/top-results-row.ts @@ -11,8 +11,16 @@ import '../library-status-indicator/library-status-indicator.js'; import type { LibraryStatus } from '../library-status-indicator/library-status-indicator.js'; import { creditLink, exploreLinkStyles } from '../../utils/explore-link'; import { creditStore } from '@store/credit-store'; -import { libraryStatusFor } from '../../utils/library-status'; +import { albumBadgeFor, libraryStatusFor } from '../../utils/library-status'; +import { + isOwned, + ownershipLabel, + unownedStyles, + type OwnableKind, +} from '../../utils/ownership'; +import { completenessStore } from '../../store/completeness-store'; import { downloadStore } from '../../store/download-store'; +import { classMap } from 'lit/directives/class-map.js'; /** Format milliseconds as m:ss. */ function formatDuration(ms: number | undefined): string { @@ -65,6 +73,9 @@ export class TopResultsRow extends LitElement { /** Unsubscribes the credit-arrival repaint. */ private creditsUnsub?: () => void; + /** Unsubscribes the "how much of this album is here" repaint. */ + private unsubCompleteness?: () => void; + override connectedCallback(): void { super.connectedCallback(); @@ -74,6 +85,9 @@ export class TopResultsRow extends LitElement { this.unsubRequests = downloadStore.subscribe(() => this.requestUpdate(), ); + this.unsubCompleteness = completenessStore.subscribe(() => + this.requestUpdate(), + ); } override disconnectedCallback(): void { @@ -81,12 +95,15 @@ export class TopResultsRow extends LitElement { this.creditsUnsub = undefined; this.unsubRequests?.(); this.unsubRequests = undefined; + this.unsubCompleteness?.(); + this.unsubCompleteness = undefined; super.disconnectedCallback(); } static override styles = [ designTokens, exploreLinkStyles, + unownedStyles, css` :host { display: block; @@ -289,23 +306,41 @@ export class TopResultsRow extends LitElement { ? r.year || '' : formatDuration(r.length) || ''; - const status: LibraryStatus = libraryStatusFor( - Boolean(r.inLibrary), - r.mbid, - ); - const entityType: 'artist' | 'album' | 'track' = + const entityType: OwnableKind = r.entityType === 'artist' ? 'artist' : r.entityType === 'release_group' ? 'album' : 'track'; + // Ownership is the local row, not the catalog's flag — see + // `utils/ownership.ts`. An album additionally says *how much* + // of it is here, which is the one thing a tick cannot. + const owned = isOwned(r); + const badge = + entityType === 'album' + ? albumBadgeFor(r, r.mbid) + : { + status: libraryStatusFor(owned, r.mbid) as LibraryStatus, + owned: 0, + expected: 0, + }; + + // A card navigates whether or not the entity is owned, so it is + // not `aria-disabled` the way an unplayable track row is — the + // name is what carries the state to anyone not seeing the + // dimming. return html`
this.handleClick(r)} @keydown=${(e: KeyboardEvent) => { if (e.key !== 'Enter' && e.key !== ' ') return; @@ -345,10 +380,12 @@ export class TopResultsRow extends LitElement { : nothing}
- ${isArtist + ${isArtist || badge.status === 'in-library' ? nothing : html`(COMPLETENESS_CACHE_LIMIT); + + /** Ids with a request in flight, so a re-render does not refetch. */ + private inFlight = new Set(); + + private listeners = new Set<() => void>(); + + /** Collected by request(), flushed as one batch on the next frame. */ + private pending = new Set(); + + private flushHandle: number | null = null; + + constructor() { + registerCacheProbe('albumCompleteness', () => ({ + entries: this.cache.size, + chars: this.cache.size * 4, + limit: COMPLETENESS_CACHE_LIMIT, + })); + + EventsOn(Events.LibraryScanComplete, () => this.invalidate()); + EventsOn(Events.TrackMetadataChanged, () => this.invalidate()); + EventsOn(Events.TracksRemovedFromLibrary, () => this.invalidate()); + } + + /** + * Subscribe to "some answers arrived". + * + * Deliberately not per-album, for `credit-store`'s reason: a grid + * fetches its cards in one call and re-renders once, so a + * fine-grained signal would buy nothing and cost a listener a card. + */ + subscribe(fn: () => void): () => void { + this.listeners.add(fn); + + return () => this.listeners.delete(fn); + } + + /** The answer for one album, or undefined until it has been asked. */ + get(albumID: number | undefined | null): Completeness | undefined { + if (!albumID || albumID <= 0) return undefined; + + return this.cache.get(albumID); + } + + /** + * Ask about one album, joining whatever batch is forming. + * + * Safe from inside a render: a set insert and a scheduled flush, + * with anything cached or in flight dropped. It does not loop — + * after a flush every id asked for is cached, so the re-render's + * requests are all dropped and nothing notifies again. + */ + request(albumID: number | undefined | null): void { + if (!albumID || albumID <= 0) return; + if (this.cache.has(albumID)) return; + if (this.inFlight.has(albumID)) return; + if (this.pending.has(albumID)) return; + + this.pending.add(albumID); + + if (this.flushHandle !== null) return; + + this.flushHandle = requestAnimationFrame(() => { + this.flushHandle = null; + + const batch = [...this.pending]; + + this.pending.clear(); + + void this.ensure(batch); + }); + } + + /** + * Ask and read in one call, for use inside a template. + * + * A getter with a side effect, deliberately — `credit-store` makes + * the same trade and for the same reason: the alternative is every + * call site writing `request(x)` beside `get(x)` and one of them + * eventually forgetting, which renders a permanently unknown + * completeness that looks exactly like an album with no totals. + */ + completeness(albumID: number | undefined | null): Completeness | undefined { + this.request(albumID); + + return this.get(albumID); + } + + /** Fetch for a list, skipping anything known or already in flight. */ + async ensure(albumIDs: readonly number[]): Promise { + const wanted = new Set(); + + for (const id of albumIDs) { + if (!id || id <= 0) continue; + // `has` rather than `get`: probing must not mark an entry + // recently-used, or scrolling past a card would keep it + // alive ahead of one actually being rendered. + if (this.cache.has(id)) continue; + if (this.inFlight.has(id)) continue; + + wanted.add(id); + } + + if (wanted.size === 0) return; + + const batch = [...wanted]; + + for (const id of batch) this.inFlight.add(id); + + try { + const found = compact(await GetAlbumsCompleteness(batch)); + + for (const id of batch) { + this.cache.set(id, found[String(id)] ?? NOTHING_HERE); + } + + this.notify(); + } catch (err) { + // A count is an enrichment: without it a card shows the + // plain "you have this", which is what it showed before and + // is a weaker answer rather than a broken one. + console.error('Failed to load album completeness', err); + } finally { + for (const id of batch) this.inFlight.delete(id); + } + } + + /** Drop everything: the files on disk changed. */ + invalidate(): void { + this.cache = new LRUMap( + COMPLETENESS_CACHE_LIMIT, + ); + this.notify(); + } + + private notify(): void { + for (const fn of this.listeners) fn(); + } +} + +export const completenessStore = new CompletenessStore(); diff --git a/frontend/src/utils/library-status.ts b/frontend/src/utils/library-status.ts index d943d29..18233f5 100644 --- a/frontend/src/utils/library-status.ts +++ b/frontend/src/utils/library-status.ts @@ -1,7 +1,9 @@ +import { completenessStore } from '@store/completeness-store'; import { downloadStore } from '@store/download-store'; import { libraryStore } from '@store/library-store'; import type * as download from '@go/download/models.js'; import type { LibraryStatus } from '../components/library-status-indicator/library-status-indicator'; +import { isOwned, type Ownable } from './ownership'; /** * What the tick/hourglass/plus badge should say about one entity. @@ -46,6 +48,61 @@ export function libraryStatusFor( return 'not-in-library'; } +/** + * Everything a badge needs about one entity, decided in one place. + * + * `status` is the state; `owned`/`expected` are the counts behind + * `partial` and are zero for every other state, which is what the badge + * requires — it documents that a caller with no total must not pass a + * ring at 0%. + */ +export interface BadgeState { + status: LibraryStatus; + owned: number; + expected: number; +} + +/** + * What the badge on an album card should say. + * + * Three rules, and the middle one is the whole point of this issue. + * + * **Ownership is the local album id**, per `utils/ownership.ts` — a + * file, not the catalog's `inLibrary` ratchet. + * + * **A partly-held album says how partly.** The count comes from + * `completenessStore`, which batches a screenful into one query; + * reading it is what asks for it. Before this, an album held 2 tracks + * of 10 wore the same green tick as one held whole on every grid in + * the app. + * + * **A total that was never declared is not a total of zero.** Where + * `known` is false — most of an untagged library, and every album until + * a rescan repopulates `audio_files.total_tracks` — this is a plain + * `in-library` and says nothing, which is the rule the badge's own + * documentation states and the reason `Known` exists at all. + */ +export function albumBadgeFor( + album: Ownable | null | undefined, + mbid?: string | null, +): BadgeState { + if (!isOwned(album)) { + return { status: libraryStatusFor(false, mbid), owned: 0, expected: 0 }; + } + + const held = completenessStore.completeness(album?.localId); + + if (held?.known && !held.complete) { + return { + status: 'partial', + owned: held.owned, + expected: held.expected, + }; + } + + return { status: 'in-library', owned: 0, expected: 0 }; +} + /** What a badge can ask for. Artists are deliberately absent: a * discography subscription is `explore-artist-details`'s Follow * button, which can say what it is committing to. */ diff --git a/frontend/src/utils/ownership.ts b/frontend/src/utils/ownership.ts new file mode 100644 index 0000000..04b4ff9 --- /dev/null +++ b/frontend/src/utils/ownership.ts @@ -0,0 +1,134 @@ +/** + * What "I do not own this" looks like, and how the app decides it. + * + * The rule the user asked for, in their words: *owned content is the + * default, normal, unadorned presentation; unowned content is what gets + * marked*. `explore-album-details` implemented it for one tracklist — + * dimmed in place, `aria-disabled` because dimming is a colour and + * cannot be the only signal, and nothing at all drawn on the owned rows + * — and every other catalog surface still mixed the two with a small + * badge as the only difference. This is that rule, written once, so + * eight surfaces cannot each keep their own version of it. + * + * ## Ownership is a file, and `localId` is the flag that says so + * + * The album page answers "do I own this row" with `filePaths`, a map + * from a displayed track to a real file. A card grid cannot afford a + * lookup per card — and does not need one, because the answer is + * already on every model. + * + * `explore_index.local_artist_id` / `local_release_group_id` / + * `local_recording_id` are built by `collectLibraryEntities` from + * queries that every one join `audio_files`, and cleared by + * `pruneStaleLocalCrossReferences` whose existence test is a file test + * in all three cases. That is the same "ownership is a file" rule, + * computed once per scan instead of once per screenful. + * + * **`inLibrary` is the weaker one and is deliberately not consulted.** + * It is written by the same pass, so today the two agree — but it is a + * one-way ratchet (`in_library = MAX(in_library, excluded.in_library)`) + * whose only clearing pass is gated on a non-null `local_*_id`, so it + * cannot be un-set on its own. One of the two is a fact with an owner; + * the other is a flag that happens to agree with it. + * + * The divergence was observable before this: both `explore-view` and + * `explore-artist-details` kept a `libraryMBIDs` set that accumulated + * every MBID ever seen with `inLibrary` and cleared it never, in a view + * that never unmounts. And on one artist-detail card the two answers + * were used side by side — the context menu gated Play on + * `localId > 0` while the badge said "in your library" from + * `inLibrary`, so a card could claim to be owned, offer no Play, and + * (the request item being gated on *not* owned) offer no way to ask for + * it either. + */ + +import { css } from 'lit'; + +/** Anything a card or row can be drawn from, as far as this is concerned. */ +export interface Ownable { + /** The local row id behind this entity: an album, a file, an artist. */ + localId?: number | null; +} + +/** + * Whether there is something of the user's behind this entity. + * + * Deliberately narrow: a local id and nothing else. Passing the model + * straight in is the point — a call site that has to remember which of + * two fields to read is a call site that will eventually read the other + * one, which is exactly how the two answers came to sit on one card. + */ +export function isOwned(entity: Ownable | null | undefined): boolean { + return (entity?.localId ?? 0) > 0; +} + +/** The kinds of thing a catalog surface can draw. */ +export type OwnableKind = 'album' | 'track' | 'artist'; + +/** + * The sentence an unowned thing says, once. + * + * It reaches whoever is not seeing the dimming, so it has to name the + * thing as well as the state — "not in your library" alone, repeated + * down a grid, identifies nothing. The em dash matches the album + * tracklist's existing phrasing, which is where this came from. + */ +export function unownedLabel(name: string, kind: OwnableKind): string { + return `${name} — not in your library, ${ + kind === 'artist' ? 'browsing the catalog' : 'available to request' + }`; +} + +/** + * The accessible name for a card or row, owned or not. + * + * `activates` is what the thing does when it is yours: "Play", "Album", + * whatever the surface's own verb is. An unowned one does not get that + * verb, because it cannot do it. + */ +export function ownershipLabel( + owned: boolean, + activates: string, + name: string, + kind: OwnableKind, +): string { + return owned ? `${activates} ${name}` : unownedLabel(name, kind); +} + +/** + * The dimming, shared so it cannot drift across surfaces. + * + * Two things about it are load-bearing. + * + * **The text dims to a token, not with `opacity`.** `theme-store`'s + * ramps are checked by `theme-contrast.test.ts` against every surface + * text can sit on; an opacity multiplier is outside that check and + * would quietly drop a dimmed title under 4.5:1 on the light ramps. + * Secondary rather than tertiary for the reason the album tracklist + * gives: these rows and cards have a `bgOverlay` hover background, + * which tertiary does not clear. + * + * **Only the artwork takes an `opacity`.** A cover is not text, so it + * is outside the contrast rule entirely, and it is the part of a card + * that carries the most weight — dimming it is what makes a grid read + * as catalog at a glance rather than needing the badge to be found. + */ +export const unownedStyles = css` + .unowned .album-title, + .unowned .track-title, + .unowned .card-name, + .unowned .top-release-title, + .unowned .artist-name { + color: var(--yj-text-secondary, #b3b3b3); + font-weight: 400; + } + + .unowned .album-art-container, + .unowned .top-release-art, + .unowned .track-art, + .unowned .card-image, + .unowned .card-image-placeholder, + .unowned .artist-avatar { + opacity: 0.55; + } +`; diff --git a/frontend/test/components/unowned-everywhere.test.ts b/frontend/test/components/unowned-everywhere.test.ts new file mode 100644 index 0000000..5bee2de --- /dev/null +++ b/frontend/test/components/unowned-everywhere.test.ts @@ -0,0 +1,363 @@ +/** + * Owned is plain; unowned is what gets marked. + * + * `explore-album-details` had this right for one tracklist and nothing + * else did: Explore's cards, the top-results row and the artist page's + * three card shapes all mixed owned and unowned with a small badge as + * the only difference — and drew a green tick on the *common* case, + * which is the treatment the album page's own green ticks were removed + * for. + * + * What is pinned here is the rule rather than any one surface, because + * the fault this replaced was eight call sites each holding their own + * version of it: + * + * - an owned thing draws **no badge at all**; + * - an unowned one is dimmed *and* says so in its accessible name, + * because dimming is a colour and cannot be the only signal; + * - ownership is a **file** (`localId`), never the catalog's + * `inLibrary` ratchet, which is a flag that happens to agree; + * - and a partly-held album says *how* partly, which is the one thing + * a tick cannot. + */ +import { beforeEach, describe, expect, it } from 'vitest'; +import { page } from 'vitest/browser'; + +import '@components/explore-view/explore-view'; +import '@components/top-results-row/top-results-row'; +import { flush, stub, resetHarness } from '@test/support/harness'; +import { fixture, shadow, shadowAll, update } from '@test/support/render'; +import { completenessStore } from '@store/completeness-store'; + +const SEARCH = 'explore.Service.SearchLocal'; +const SHELVES = 'explore.Service.GetExploreShelves'; +const COMPLETENESS = 'library.Library.GetAlbumsCompleteness'; + +/** A release group as the backend projects one. */ +function album( + title: string, + { localId = 0, inLibrary = false }: { localId?: number; inLibrary?: boolean }, +) { + return { + mbid: `rg-${title}`, + title, + artistCredit: 'An Artist', + artistMbid: 'ar-1', + primaryType: 'Album', + firstReleaseDate: '1994-05-01', + popularity: 100, + listenerCount: 10, + secondaryTypes: [], + inLibrary, + localId, + }; +} + +/** A recording as the backend projects one. */ +function recording( + title: string, + { localId = 0, inLibrary = false }: { localId?: number; inLibrary?: boolean }, +) { + return { + mbid: `rec-${title}`, + title, + artistCredit: 'An Artist', + artistMbid: 'ar-1', + length: 200000, + popularity: 0, + listenerCount: 0, + inLibrary, + localId, + }; +} + +/** Mount Explore showing one page of results. */ +async function exploreShowing(results: { + releaseGroups?: unknown[]; + recordings?: unknown[]; + artists?: unknown[]; +}) { + stub(SHELVES, { shelves: [], state: 'ready' }); + stub(SEARCH, { + artists: [], + releaseGroups: [], + recordings: [], + ...results, + }); + + const el = await fixture('explore-view'); + + // A cached primary view only fetches on arrival, and the search is + // what these cards come from. + (el as unknown as { onViewActivate: () => void }).onViewActivate?.(); + await update(el, { results: { artists: [], releaseGroups: [], recordings: [], ...results } }); + await flush(); + await el.updateComplete; + + return el; +} + +beforeEach(() => { + resetHarness(); + stub(COMPLETENESS, {}); + + // The store is a singleton and caches an *answer*, including the + // absent one — which is the point, or 87% of a grid re-asks forever. + // Two tests in one file are two sessions as far as it is concerned, + // so a stale entry from the test above would otherwise decide the + // one below. Found by writing the assertion the wrong way round. + completenessStore.invalidate(); +}); + +describe('an owned thing is plain', () => { + it('draws no badge on an album card it has files for', async () => { + const el = await exploreShowing({ + releaseGroups: [album('Held', { localId: 7 })], + }); + + expect(shadowAll(el, '.album-card')).toHaveLength(1); + expect(shadow(el, '.album-card library-status-indicator')).toBeNull(); + }); + + it('draws no badge on a track row it has a file for', async () => { + const el = await exploreShowing({ + recordings: [recording('Held', { localId: 9 })], + }); + + expect(shadowAll(el, '.track-item')).toHaveLength(1); + expect(shadow(el, '.track-item library-status-indicator')).toBeNull(); + }); + + it('does not dim it', async () => { + const el = await exploreShowing({ + releaseGroups: [album('Held', { localId: 7 })], + }); + + expect(shadow(el, '.album-card')?.classList.contains('unowned')).toBe( + false, + ); + }); +}); + +describe('an unowned thing is marked', () => { + it('dims the card and keeps its request badge', async () => { + const el = await exploreShowing({ + releaseGroups: [album('Absent', {})], + }); + + expect(shadow(el, '.album-card')?.classList.contains('unowned')).toBe(true); + expect(shadow(el, '.album-card library-status-indicator')).not.toBeNull(); + }); + + /** + * The name is the half of this that reaches anyone not seeing the + * dimming, so it has to be the browser's own answer — a shadow-root + * query cannot compute a name, and this repo has shipped a nameless + * control three times. + */ + it('says so in the name the browser computes', async () => { + await exploreShowing({ releaseGroups: [album('Absent', {})] }); + + await expect + .element(page.getByRole('button', { name: /Absent — not in your library/ })) + .toBeInTheDocument(); + }); + + /** + * A track row is `aria-disabled` and a card is not, and the + * difference is not cosmetic: activating an unowned row does nothing + * (`onRecordingRowDblClick` returns early), while a card navigates to + * the catalog page for it, which is a perfectly good thing to do with + * something you do not own. + */ + it('marks a row that cannot be played as disabled', async () => { + const el = await exploreShowing({ + recordings: [recording('Absent', {})], + }); + + expect(shadow(el, '.track-item')?.getAttribute('aria-disabled')).toBe( + 'true', + ); + }); + + it('leaves a card that still navigates enabled', async () => { + const el = await exploreShowing({ + releaseGroups: [album('Absent', {})], + }); + + expect(shadow(el, '.album-card')?.getAttribute('aria-disabled')).toBeNull(); + }); +}); + +/** + * The decision this issue turned on. + * + * `inLibrary` is written by the same pass that writes the local ids, so + * the two agree in a healthy database — but it is a one-way ratchet + * (`MAX(in_library, excluded.in_library)`) whose only clearing pass is + * gated on a non-null `local_*_id`, so it cannot be un-set on its own. + * A row carrying it with no local id behind it is a claim of ownership + * with no file, which is exactly what the album page refuses to trust. + */ +describe('ownership is a file, not a flag', () => { + it('treats a card flagged inLibrary with no local row as unowned', async () => { + const el = await exploreShowing({ + releaseGroups: [album('Phantom', { inLibrary: true, localId: 0 })], + }); + + expect(shadow(el, '.album-card')?.classList.contains('unowned')).toBe(true); + expect(shadow(el, '.album-card library-status-indicator')).not.toBeNull(); + }); + + it('does the same for a track row', async () => { + const el = await exploreShowing({ + recordings: [recording('Phantom', { inLibrary: true, localId: 0 })], + }); + + expect(shadow(el, '.track-item')?.getAttribute('aria-disabled')).toBe( + 'true', + ); + }); +}); + +/** + * The count, which is what `#16`'s deferred third step asked for: an + * album held 2 tracks of 10 wore the same green tick as one held whole, + * on every grid in the app. + */ +describe('a partly-held album says how partly', () => { + it('draws the ring and puts the count in the badge name', async () => { + stub(COMPLETENESS, { + '7': { owned: 9, expected: 12, known: true, complete: false }, + }); + + const el = await exploreShowing({ + releaseGroups: [album('Partly', { localId: 7 })], + }); + + // The store batches into the next frame, so the answer lands one + // repaint after the cards do — which is the thing the subscription + // exists for. + await new Promise((r) => requestAnimationFrame(() => r(null))); + await flush(); + await el.updateComplete; + + const badge = shadow(el, '.album-card library-status-indicator'); + + expect(badge?.getAttribute('status')).toBe('partial'); + + // A partly-held album is *actionable* — it has three tracks left to + // ask for — so the badge is a button, and the name has to carry the + // action and the count. Naming it after the action alone left the + // one state the ring exists for as the one state whose name did not + // mention it. + await expect + .element( + page.getByRole('button', { + name: /Request the rest of album .*Partly.* — 9 of 12 tracks/, + }), + ) + .toBeInTheDocument(); + }); + + /** + * Where the tags never declared a total, `known` is false and the + * card must say nothing — most of an untagged library is in that + * state, and a ring drawn from its absence would mark all of it + * incomplete on no evidence. That is the rule `Known` exists for. + */ + it('says nothing when the total was never declared', async () => { + stub(COMPLETENESS, { + '7': { owned: 3, expected: 0, known: false, complete: false }, + }); + + const el = await exploreShowing({ + releaseGroups: [album('Untotalled', { localId: 7 })], + }); + + await new Promise((r) => requestAnimationFrame(() => r(null))); + await flush(); + await el.updateComplete; + + expect(shadow(el, '.album-card library-status-indicator')).toBeNull(); + }); + + it('asks about the owned albums only, in one call', async () => { + const seen: unknown[][] = []; + + stub(COMPLETENESS, (...args: unknown[]) => { + seen.push(args); + + return {}; + }); + + await exploreShowing({ + releaseGroups: [ + album('Held', { localId: 7 }), + album('Also held', { localId: 8 }), + album('Absent', {}), + album('Phantom', { inLibrary: true, localId: 0 }), + ], + }); + + await new Promise((r) => requestAnimationFrame(() => r(null))); + await flush(); + + expect(seen).toHaveLength(1); + expect(seen[0]?.[0]).toEqual([7, 8]); + }); +}); + +describe('the top-results row follows the same rule', () => { + const result = ( + name: string, + entityType: string, + extra: Record = {}, + ) => ({ + entityType, + mbid: `top-${name}`, + name, + artistCredit: 'An Artist', + intentScore: 1, + inLibrary: false, + ...extra, + }); + + it('draws no badge on something it owns', async () => { + const el = await fixture('top-results-row', { + results: [result('Held', 'release_group', { localId: 7 })], + query: 'held', + }); + + expect(shadow(el, '.card library-status-indicator')).toBeNull(); + expect(shadow(el, '.card')?.classList.contains('unowned')).toBe(false); + }); + + it('dims and names something it does not', async () => { + const el = await fixture('top-results-row', { + results: [result('Absent', 'release_group')], + query: 'absent', + }); + + expect(shadow(el, '.card')?.classList.contains('unowned')).toBe(true); + + await expect + .element(page.getByRole('button', { name: /Absent — not in your library/ })) + .toBeInTheDocument(); + }); + + /** + * An artist card has never had a badge — a discography subscription + * is the artist page's Follow button, which can say what it commits + * to — so the dimming and the name are the whole signal there. + */ + it('marks an unowned artist without offering a request', async () => { + const el = await fixture('top-results-row', { + results: [result('An Artist', 'artist')], + query: 'an artist', + }); + + expect(shadow(el, '.card')?.classList.contains('unowned')).toBe(true); + expect(shadow(el, '.card library-status-indicator')).toBeNull(); + }); +}); From 10eca353abe1dbcfe376c17af74a5f2efa0764b4 Mon Sep 17 00:00:00 2001 From: Logan Date: Wed, 19 Aug 2026 00:39:18 -0400 Subject: [PATCH 5/6] fix(explore): gate playback on the same answer the row is drawn from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two play paths still accepted `inLibrary`, so a row drawn dimmed and `aria-disabled` by the new rule would still attempt to play and fail with "this track could not be found in your library" — the disagreement this pass exists to remove, one layer down from the badge. --- .../explore-artist-details/explore-artist-details.ts | 9 ++++++--- frontend/src/components/explore-view/explore-view.ts | 7 +++++-- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/frontend/src/components/explore-artist-details/explore-artist-details.ts b/frontend/src/components/explore-artist-details/explore-artist-details.ts index d7f11a8..c981310 100644 --- a/frontend/src/components/explore-artist-details/explore-artist-details.ts +++ b/frontend/src/components/explore-artist-details/explore-artist-details.ts @@ -2015,11 +2015,14 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost /** * File path for one top track, resolved by recording MBID — the - * same key `inLibrary`/`localId` were set from. Works whether or - * not the containing release itself matched a local album. + * same key `localId` was set from. Works whether or not the + * containing release itself matched a local album. + * + * Gated on the same answer the row is drawn from, or a row drawn + * dimmed and `aria-disabled` would still try to play and fail. */ private async trackFilePath(track: LBTopRecording): Promise { - if (!(track.inLibrary || track.localId) || !track.recordingMbid) return null; + if (!isOwned(track) || !track.recordingMbid) return null; const libraryID = libraryStore.getSelectedLibraryId() ?? 0; const byMBID = await dictByName( diff --git a/frontend/src/components/explore-view/explore-view.ts b/frontend/src/components/explore-view/explore-view.ts index 0925d04..e3d4c63 100644 --- a/frontend/src/components/explore-view/explore-view.ts +++ b/frontend/src/components/explore-view/explore-view.ts @@ -1241,8 +1241,11 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte void this.playAlbum(rg, false); } - private onRecordingRowDblClick(r: { mbid: string; inLibrary: boolean; localId?: number }): void { - if (!r.inLibrary && !r.localId) return; + // The same answer the row is drawn from. It used to accept + // `inLibrary` as well, so a row drawn dimmed and `aria-disabled` + // would still try to play and fail with a notification. + private onRecordingRowDblClick(r: { mbid: string; localId?: number }): void { + if (!isOwned(r)) return; void this.playRecording(r.mbid); } From c4e055ce51f55f51df220737a1f07a8fc32c3ede Mon Sep 17 00:00:00 2001 From: Logan Date: Wed, 19 Aug 2026 00:50:53 -0400 Subject: [PATCH 6/6] docs: write down which of the two ownership columns to read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `localId` / `inLibrary` choice outlives #38 — every future catalog surface has to make it, and the code read them as an OR at eight call sites precisely because nothing said they were different kinds of thing. CLAUDE.md gets the rule and its four load-bearing details; NOTES.md gets the measurement, the card that used both answers at once, and the alternative that was rejected. --- .planning/NOTES.md | 46 ++++++++++++++++++++++++++++++++++++++++ CLAUDE.md | 53 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) diff --git a/.planning/NOTES.md b/.planning/NOTES.md index e2687de..f5b106b 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -3534,3 +3534,49 @@ silent.** `artifactHasTotals` and `artifactHasCredits` are both correct and both mean a feature can ship, pass every test, and produce nothing for anybody without a single failure anywhere. Checking the *published file* is one query and is not implied by any tick in CI. + +## "Do I own this" has two answers in the schema, and one of them is a flag (2026-08-19) + +Decided while doing #38, and it outlives it because every future +catalog surface has to pick one. + +`explore_index` carries both `in_library` and `local_artist_id` / +`local_release_group_id` / `local_recording_id`. They are written by +the same pass (`collectLibraryEntities`), so on a healthy database they +agree, and the code read them as an OR — `inLibrary || localId > 0` — +at eight call sites. + +They are not the same kind of thing: + +- **`local_*_id` is a fact with an owner.** Every query that sets one + joins `audio_files`, and `pruneStaleLocalCrossReferences` clears it + with an existence test that is a file test in all three cases. It is + the same rule `explore-album-details`'s `filePaths` implements, one + layer down and computed once per scan. +- **`in_library` is a ratchet.** `upsertBatch` raises it with + `MAX(in_library, excluded.in_library)` and the prune is the only + thing that lowers it — gated on the local id being non-null, so a row + holding the flag *without* an id is a fixed point nothing can clear. + Filed as #118; it still drives search scoring, the popularity-floor + bypass and two Explore shelves, so routing the UI around it was not a + fix. + +What made the choice concrete rather than theoretical: on +`explore-artist-details` the *same card* used both. The context menu +gated Play on `localId > 0`; the badge used `inLibrary`. An album with +the flag and no local row drew a green tick saying it was in your +library, offered no Play, and — the request item being gated on *not* +owned — offered no way to ask for it either. + +The rejected alternative is worth keeping: batching a real file lookup +per screenful, the way `credit-store` coalesces. It would have answered +for **recordings** (`GetFilePathsByRecordingMBIDs`) and most of the +cards on these surfaces are release groups, so it would have made track +rows strong, left album cards exactly where they were, and cost a new +store. The batch that *was* worth adding is a different question — +`GetAlbumsCompleteness`, "how much of this album is here", which no +per-card flag can answer at all. + +The general point: **two columns that agree today are not one column.** +Which of them a new surface reads should be decided by which one has +something that can un-set it. diff --git a/CLAUDE.md b/CLAUDE.md index f4d804e..9b408d5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1658,6 +1658,59 @@ not about plumbing — it says rows may be missing from the page altogether, which nothing on screen can show. (`explore-artist-details` still uses `loading`; it has no equivalent per-row signal.) +**And that treatment is the app's, not the page's.** +`utils/ownership.ts` is the rule written once, because it was written +at eight call sites and so none of them had the whole of it: Explore's +cards, `top-results-row` and the artist page's three card shapes all +mixed owned and unowned with a small badge as the only difference, and +the badge on the *owned* ones was a green tick — the mark on the common +case this tracklist removed. Owned is plain and draws no badge at all; +unowned is dimmed, says so in its accessible name, and keeps its +request affordance; a partly-held album says how partly. + +Four things about it are load-bearing. + +**Ownership is `localId`, and `inLibrary` is deliberately not +consulted.** The album page answers with `filePaths`, a real file per +displayed track, and a card grid cannot afford that — but it does not +need to, because `explore_index.local_*_id` is built by +`collectLibraryEntities` from queries that every one join `audio_files` +and cleared by `pruneStaleLocalCrossReferences`, whose existence test +is a file test in all three cases. That is the same "ownership is a +file" rule computed once per scan instead of once per screenful. +`in_library` is written by the same pass, so the two agree in a healthy +database, but it is a one-way ratchet +(`MAX(in_library, excluded.in_library)`) whose only clearing pass is +gated on a non-null local id: it cannot be un-set on its own (#118). +One is a fact with an owner; the other is a flag that happens to agree. +Both `explore-view` and `explore-artist-details` additionally kept a +`libraryMBIDs` set that accumulated every MBID ever seen with the flag +and cleared it never, in views that never unmount; both are gone. + +**The two answers used to sit on one card.** +`renderReleaseMenuItems` gates Play on `release.localId > 0` while the +badge used `inLibrary`, so an album with the flag and no local row drew +a tick saying it was in your library, offered no Play, and — the +request item being gated on *not* owned — offered no way to ask for it +either. Any new surface that asks the question twice will reproduce it. + +**`aria-disabled` goes on rows and not on cards.** An unowned *row* +cannot be activated; an unowned *card* still navigates to the catalog +page for it, which is a perfectly good thing to do with something you +do not own. The accessible name carries the state either way, which is +why it is one helper and not a class. + +**The count is batched, not looked up.** `store/completeness-store.ts` +is `credit-store` one question over: `request()` is per-card and +coalesces a screenful into one `GetAlbumsCompleteness`, absence is +cached as an answer (or the albums with no totals re-ask forever), and +the whole cache is dropped on a scan, a retag or a removal rather than +aged. `library-status.ts`'s `albumBadgeFor` is where that meets +`Known`: a total that was never declared is a plain `in-library`, never +a ring at 0%. One consequence in the badge itself — a `partial` badge +is *actionable*, and a control named after its action alone dropped the +count from the one state the ring exists for, so its name is both. + **A partly-owned album draws the release, not the part.** Once the tags say nine of twelve, `buildLibraryEntry` shows the *catalog's* twelve with three dimmed, rather than the nine on disk — the missing tracks