From 88fc50afb87f33543d11881a50067904748287ec Mon Sep 17 00:00:00 2001 From: Logan Date: Wed, 19 Aug 2026 00:38:22 -0400 Subject: [PATCH] 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(); + }); +});