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 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/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/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/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; } /** 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. */ 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..c981310 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 ── */ /** @@ -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( @@ -2063,7 +2066,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 +2109,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 +2129,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 +2949,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 +2987,18 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost ${formatListenCount(t.totalListenCount)} plays - + ${owned + ? nothing + : html``}
- `, - )} + `; + })}
${canExpandTracks ? html` @@ -3057,10 +3069,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 +3110,18 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
${rg.date ? html`${extractYear(rg.date)}` : nothing}
- + ${badge.status === 'in-library' + ? nothing + : html``}
@@ -3189,14 +3211,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 +3248,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..e3d4c63 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; @@ -1229,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); } @@ -1665,42 +1680,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 +2086,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 +2154,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 +2217,17 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte : nothing} ${year ? html`${year}` : nothing}
- + ${badge.status === 'in-library' + ? nothing + : html``}
`; @@ -2249,12 +2242,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 +2292,18 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte ? html`${formatDuration(r.length)}` : nothing}
- + ${owned + ? nothing + : html``}
- `, - )} + `; + })}
`; 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/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/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'); + }); +}); 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(); + }); +});