fix(explore): show a requested album as queued, not absent
`library-status-indicator` has had three states since it was written and produced two: all eight call sites were a two-way ternary between `in-library` and `not-in-library`, so the `queued` state it styles and labels was unreachable. The result was the app contradicting itself on one page. An album added to the request list showed a plus and announced "is not in your library", forty pixels from a filled button reading "Wanted". The rule was written at eight places, which is why none of them had all of it, so it is `utils/library-status.ts` now: owning outranks wanting, a satisfied request is not queued, and a request is by MBID — a track inside a requested album is not itself requested and still says so. `explore-view` gains the `downloadStore` subscription both detail views already had, registered `whileActive` because it is a cached view that never unmounts. `top-results-row` needs its own: its host re-rendering sets the same `results` array back, so Lit stops at the property and the row never hears about a change.
This commit is contained in:
@@ -24,6 +24,8 @@ 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 } from '@utils/library-status';
|
||||
import type { LibraryStatus } from '../library-status-indicator/library-status-indicator';
|
||||
import '../catalog-scope-notice/catalog-scope-notice.js';
|
||||
import type { CatalogScope } from '../catalog-scope-notice/catalog-scope-notice.js';
|
||||
import '@awesome.me/webawesome/dist/components/button/button.js';
|
||||
@@ -1443,7 +1445,7 @@ export class ExploreAlbumDetails extends LitElement {
|
||||
* - cachedAlbums has MBID match → owned
|
||||
* - any selected version has → owned (covers local-only albums
|
||||
* a track marked inLibrary where releaseGroup may be null)
|
||||
* - else → not owned
|
||||
* - else → whatever the request list says
|
||||
*
|
||||
* Four different claims of decreasing confidence, OR'd together and
|
||||
* reported as one tick — the last of which fires when a *single*
|
||||
@@ -1452,10 +1454,11 @@ export class ExploreAlbumDetails extends LitElement {
|
||||
* actions key off; this stays as it was, because the indicator's
|
||||
* job is "is any of this yours" and that is what it answers.
|
||||
*
|
||||
* No queued state for now — that's reserved for future
|
||||
* download-client integration.
|
||||
* When none of them hold the answer is not automatically "no":
|
||||
* the album may be on the request list, which the button directly
|
||||
* below this badge has reported as "Wanted" all along.
|
||||
*/
|
||||
private albumLibraryStatus(): 'in-library' | 'not-in-library' {
|
||||
private albumLibraryStatus(): LibraryStatus {
|
||||
if (this.localAlbumId > 0) return 'in-library';
|
||||
|
||||
if (this.releaseGroup?.inLibrary) return 'in-library';
|
||||
@@ -1477,7 +1480,11 @@ export class ExploreAlbumDetails extends LitElement {
|
||||
}
|
||||
}
|
||||
|
||||
return 'not-in-library';
|
||||
// None of the five ownership claims held, so the badge falls
|
||||
// through to the one thing this page already knew and never
|
||||
// said: whether the album is on the request list. The button
|
||||
// below it has read "Wanted" all along.
|
||||
return libraryStatusFor(false, this.releaseGroupMBID);
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -2280,7 +2287,7 @@ export class ExploreAlbumDetails extends LitElement {
|
||||
)}</span
|
||||
>
|
||||
<library-status-indicator
|
||||
status=${track.inLibrary ? 'in-library' : 'not-in-library'}
|
||||
status=${libraryStatusFor(Boolean(track.inLibrary), track.mbid)}
|
||||
entity-type="track"
|
||||
label=${track.title}
|
||||
></library-status-indicator>
|
||||
|
||||
@@ -34,6 +34,7 @@ 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 } from '@utils/library-status';
|
||||
import '../catalog-scope-notice/catalog-scope-notice.js';
|
||||
import type { CatalogScope } from '../catalog-scope-notice/catalog-scope-notice.js';
|
||||
|
||||
@@ -2113,7 +2114,7 @@ export class ExploreArtistDetails extends LitElement {
|
||||
${formatListenCount(t.totalListenCount)} plays
|
||||
</span>
|
||||
<library-status-indicator
|
||||
status=${t.inLibrary || t.localId ? 'in-library' : 'not-in-library'}
|
||||
status=${libraryStatusFor(Boolean(t.inLibrary || t.localId), t.recordingMbid)}
|
||||
entity-type="track"
|
||||
label=${t.trackName}
|
||||
></library-status-indicator>
|
||||
@@ -2225,7 +2226,7 @@ export class ExploreArtistDetails extends LitElement {
|
||||
${rg.date ? html`<span>${extractYear(rg.date)}</span>` : nothing}
|
||||
</div>
|
||||
<library-status-indicator
|
||||
status=${rg.inLibrary || rg.localId ? 'in-library' : 'not-in-library'}
|
||||
status=${libraryStatusFor(Boolean(rg.inLibrary || rg.localId), rg.releaseGroupMbid)}
|
||||
entity-type="album"
|
||||
label=${rg.title}
|
||||
size="18"
|
||||
@@ -2320,9 +2321,7 @@ export class ExploreArtistDetails extends LitElement {
|
||||
const artURL = this.thumbnailURLs.get(rg.mbid) || '';
|
||||
const year = extractYear(rg.firstReleaseDate);
|
||||
const inLibrary = this.libraryMBIDs.has(rg.mbid) || Boolean(rg.inLibrary);
|
||||
const status: 'in-library' | 'not-in-library' = inLibrary
|
||||
? 'in-library'
|
||||
: 'not-in-library';
|
||||
const status = libraryStatusFor(inLibrary, rg.mbid);
|
||||
|
||||
return html`
|
||||
<div
|
||||
|
||||
@@ -1,4 +1,6 @@
|
||||
import { avatarBackground } from '@utils/avatar-color';
|
||||
import { libraryStatusFor } from '@utils/library-status';
|
||||
import { downloadStore } from '@store/download-store';
|
||||
import { LitElement, html, css, nothing } from 'lit';
|
||||
import { customElement, state, query as litQuery } from 'lit/decorators.js';
|
||||
import '@components/page-header/page-header';
|
||||
@@ -724,6 +726,19 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) {
|
||||
this.cancelIndexStatus = EventsOn(Events.IndexStatusChanged, () => {
|
||||
if (this.shelves?.state !== 'ready') void this.loadShelves();
|
||||
});
|
||||
|
||||
// The badges on every result card say whether something is
|
||||
// already requested, which a background reconcile pass changes
|
||||
// without this page doing anything. Registered `whileActive`
|
||||
// rather than on connect: this view is cached and never
|
||||
// unmounts, so a connect-time subscription would run for the
|
||||
// life of the session from pages it is not on.
|
||||
//
|
||||
// `init()` is four fetches, and it happens on arrival for the
|
||||
// same reason `loadShelves()` does — a user who never opens
|
||||
// Explore should not pay for it.
|
||||
this.whileActive(downloadStore.subscribe(() => this.requestUpdate()));
|
||||
void downloadStore.init().then(() => this.requestUpdate());
|
||||
}
|
||||
|
||||
/** A debounced search that lands after the user has left the page is
|
||||
@@ -1836,7 +1851,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) {
|
||||
${year ? html`<span>${year}</span>` : nothing}
|
||||
</div>
|
||||
<library-status-indicator
|
||||
status=${this.libraryMBIDs.has(rg.mbid) || rg.inLibrary ? 'in-library' : 'not-in-library'}
|
||||
status=${libraryStatusFor(this.libraryMBIDs.has(rg.mbid) || Boolean(rg.inLibrary), rg.mbid)}
|
||||
entity-type="album"
|
||||
label=${rg.title}
|
||||
></library-status-indicator>
|
||||
@@ -1874,7 +1889,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) {
|
||||
: nothing}
|
||||
</div>
|
||||
<library-status-indicator
|
||||
status=${this.libraryMBIDs.has(r.mbid) || r.inLibrary ? 'in-library' : 'not-in-library'}
|
||||
status=${libraryStatusFor(this.libraryMBIDs.has(r.mbid) || Boolean(r.inLibrary), r.mbid)}
|
||||
entity-type="track"
|
||||
label=${r.title}
|
||||
></library-status-indicator>
|
||||
|
||||
@@ -10,6 +10,8 @@ import {
|
||||
import '../library-status-indicator/library-status-indicator.js';
|
||||
import type { LibraryStatus } from '../library-status-indicator/library-status-indicator.js';
|
||||
import { artistLink, exploreLinkStyles } from '../../utils/explore-link';
|
||||
import { libraryStatusFor } from '../../utils/library-status';
|
||||
import { downloadStore } from '../../store/download-store';
|
||||
|
||||
/** Format milliseconds as m:ss. */
|
||||
function formatDuration(ms: number | undefined): string {
|
||||
@@ -50,6 +52,28 @@ export class TopResultsRow extends LitElement {
|
||||
// Per-card state: cover images.
|
||||
private images = new Map<string, string>();
|
||||
|
||||
private unsubRequests?: () => void;
|
||||
|
||||
/**
|
||||
* The badges here say whether something is already requested, and
|
||||
* this row will not hear about a change from its host: `explore-view`
|
||||
* re-rendering sets the same `results` array back, so Lit stops at
|
||||
* the property and never updates this element. One subscription for
|
||||
* the row, not one per card.
|
||||
*/
|
||||
override connectedCallback(): void {
|
||||
super.connectedCallback();
|
||||
this.unsubRequests = downloadStore.subscribe(() =>
|
||||
this.requestUpdate(),
|
||||
);
|
||||
}
|
||||
|
||||
override disconnectedCallback(): void {
|
||||
this.unsubRequests?.();
|
||||
this.unsubRequests = undefined;
|
||||
super.disconnectedCallback();
|
||||
}
|
||||
|
||||
static override styles = [
|
||||
designTokens,
|
||||
exploreLinkStyles,
|
||||
@@ -255,7 +279,10 @@ export class TopResultsRow extends LitElement {
|
||||
? r.year || ''
|
||||
: formatDuration(r.length) || '';
|
||||
|
||||
const status: LibraryStatus = r.inLibrary ? 'in-library' : 'not-in-library';
|
||||
const status: LibraryStatus = libraryStatusFor(
|
||||
Boolean(r.inLibrary),
|
||||
r.mbid,
|
||||
);
|
||||
const entityType: 'artist' | 'album' | 'track' =
|
||||
r.entityType === 'artist'
|
||||
? 'artist'
|
||||
|
||||
@@ -0,0 +1,45 @@
|
||||
import { downloadStore } from '@store/download-store';
|
||||
import type { LibraryStatus } from '../components/library-status-indicator/library-status-indicator';
|
||||
|
||||
/**
|
||||
* What the tick/hourglass/plus badge should say about one entity.
|
||||
*
|
||||
* This exists because the rule was written at all eight call sites and
|
||||
* so none of them had the whole of it: every one was a two-way ternary
|
||||
* between `in-library` and `not-in-library`, and the badge's third
|
||||
* state — `queued`, styled and labelled since it was written — was
|
||||
* produced by nothing. An album the user had already asked for through
|
||||
* the "Want this" button showed a plus and said it was not in their
|
||||
* library, on the same page, forty pixels from a filled button reading
|
||||
* "Wanted".
|
||||
*
|
||||
* Two rules decide the answer, and both are about honesty rather than
|
||||
* precedence for its own sake:
|
||||
*
|
||||
* - **Owning outranks wanting.** A request that has been satisfied by
|
||||
* any route — downloaded here, ripped, bought elsewhere — is not
|
||||
* news; what the user has is.
|
||||
* - **A request is by MBID, and the badge answers about the entity it
|
||||
* is on.** A track inside a requested album is not itself requested,
|
||||
* so it stays a plus. Saying otherwise would promise that clicking
|
||||
* it later would find *that* recording.
|
||||
*
|
||||
* A `satisfied` request is deliberately not `queued`: nothing is coming.
|
||||
* A `paused` one is, because the user did ask for it and it is still on
|
||||
* the list — "queued" is a slight overstatement of a paused request and
|
||||
* a much smaller one than "not in your library".
|
||||
*/
|
||||
export function libraryStatusFor(
|
||||
owned: boolean,
|
||||
mbid?: string | null,
|
||||
): LibraryStatus {
|
||||
if (owned) return 'in-library';
|
||||
|
||||
if (!mbid) return 'not-in-library';
|
||||
|
||||
const request = downloadStore.requestFor(mbid);
|
||||
|
||||
if (request && request.state !== 'satisfied') return 'queued';
|
||||
|
||||
return 'not-in-library';
|
||||
}
|
||||
@@ -0,0 +1,185 @@
|
||||
/**
|
||||
* Plan 009 phase 1: the badge tells the truth.
|
||||
*
|
||||
* `library-status-indicator` has had three states since it was written
|
||||
* — a tick, an hourglass and a plus — and the hourglass was produced by
|
||||
* nothing. All eight call sites were a two-way ternary, so an album the
|
||||
* user had already asked for through "Want this" showed a plus and said
|
||||
* it was not in their library, on the same page as a filled button
|
||||
* reading "Wanted".
|
||||
*
|
||||
* Two tiers of assertion here, and the second is the one that would
|
||||
* have failed:
|
||||
*
|
||||
* - the rule itself, which is now written once, and
|
||||
* - a rendered Explore result whose release group is requested,
|
||||
* because a helper nobody calls is a rule nobody follows.
|
||||
*/
|
||||
import { beforeEach, describe, expect, it } from 'vitest';
|
||||
|
||||
import '@components/explore-view/explore-view';
|
||||
import type { Request } from '@store/download-store';
|
||||
import { libraryStatusFor } from '@utils/library-status';
|
||||
import { Events } from '../../src/events';
|
||||
import { emit, flush, stub } from '@test/support/harness';
|
||||
import { fixture, shadow, shadowAll } from '@test/support/render';
|
||||
|
||||
const SEARCH = 'explore.Service.SearchLocal';
|
||||
|
||||
function request(overrides: Partial<Request>): Request {
|
||||
return {
|
||||
id: 1,
|
||||
mbid: 'rg-wanted',
|
||||
entity: 'release-group',
|
||||
libraryId: 1,
|
||||
artist: 'An Artist',
|
||||
title: 'Wanted Album',
|
||||
scope: 'future',
|
||||
secondary: false,
|
||||
state: 'wanted',
|
||||
attempts: 0,
|
||||
...overrides,
|
||||
} as Request;
|
||||
}
|
||||
|
||||
/** Put a request list into the store the way the backend does. */
|
||||
async function withRequests(rows: Request[]): Promise<void> {
|
||||
stub('download.Service.ListRequests', rows);
|
||||
emit(Events.RequestsChanged);
|
||||
await flush();
|
||||
}
|
||||
|
||||
const releaseGroup = (mbid: string, title: string) => ({
|
||||
mbid,
|
||||
title,
|
||||
artistCredit: 'An Artist',
|
||||
artistMbid: 'ar-1',
|
||||
primaryType: 'Album',
|
||||
firstReleaseDate: '1994-05-01',
|
||||
popularity: 100,
|
||||
listenerCount: 10,
|
||||
inLibrary: false,
|
||||
secondaryTypes: [],
|
||||
});
|
||||
|
||||
describe('libraryStatusFor', () => {
|
||||
beforeEach(async () => {
|
||||
await withRequests([]);
|
||||
});
|
||||
|
||||
it('says nothing about an entity with no MBID to ask about', () => {
|
||||
expect(libraryStatusFor(false, '')).toBe('not-in-library');
|
||||
expect(libraryStatusFor(false, undefined)).toBe('not-in-library');
|
||||
});
|
||||
|
||||
it('reports a request as queued', async () => {
|
||||
await withRequests([request({ mbid: 'rg-wanted' })]);
|
||||
|
||||
expect(libraryStatusFor(false, 'rg-wanted')).toBe('queued');
|
||||
});
|
||||
|
||||
it('lets owning outrank wanting', async () => {
|
||||
// Both are true of an album that has arrived but whose request has
|
||||
// not been retired yet. What the user has is not news; what they
|
||||
// have is.
|
||||
await withRequests([request({ mbid: 'rg-wanted' })]);
|
||||
|
||||
expect(libraryStatusFor(true, 'rg-wanted')).toBe('in-library');
|
||||
});
|
||||
|
||||
it('does not call a satisfied request queued', async () => {
|
||||
// Nothing is coming: the request is history. An unowned entity with
|
||||
// a satisfied request is a stale row, not a download in flight.
|
||||
await withRequests([request({ mbid: 'rg-done', state: 'satisfied' })]);
|
||||
|
||||
expect(libraryStatusFor(false, 'rg-done')).toBe('not-in-library');
|
||||
});
|
||||
|
||||
it('does count a paused request, which the user did ask for', async () => {
|
||||
await withRequests([request({ mbid: 'rg-paused', state: 'paused' })]);
|
||||
|
||||
expect(libraryStatusFor(false, 'rg-paused')).toBe('queued');
|
||||
});
|
||||
|
||||
it('answers about the entity it is on, not the one containing it', () => {
|
||||
// A request is by MBID. A track inside a requested album is not
|
||||
// itself requested, and saying otherwise promises that clicking it
|
||||
// would find that recording.
|
||||
expect(libraryStatusFor(false, 'recording-inside-rg-wanted')).toBe(
|
||||
'not-in-library',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('<explore-view> badges', () => {
|
||||
beforeEach(async () => {
|
||||
stub('explore.Service.GetThumbnails', []);
|
||||
stub('explore.Service.GetThumbnail', '');
|
||||
stub('explore.Service.GetArtistImageURL', '');
|
||||
stub('explore.Service.GetExploreShelves', { shelves: [], state: 'ready' });
|
||||
stub('library.Library.GetAllAlbums', []);
|
||||
stub('library.Library.GetAllTracks', []);
|
||||
await withRequests([]);
|
||||
});
|
||||
|
||||
async function searchFor(rows: ReturnType<typeof releaseGroup>[]) {
|
||||
stub(SEARCH, {
|
||||
artists: [],
|
||||
releaseGroups: rows,
|
||||
recordings: [],
|
||||
topResults: [],
|
||||
});
|
||||
|
||||
const el = await fixture('explore-view');
|
||||
(el as unknown as { viewActivated(): void }).viewActivated();
|
||||
await flush();
|
||||
|
||||
const input = shadow<HTMLInputElement>(el, 'input');
|
||||
if (input) {
|
||||
input.value = 'anything';
|
||||
input.dispatchEvent(new Event('input', { bubbles: true }));
|
||||
}
|
||||
|
||||
// The search box debounces, so waiting a frame measures the input
|
||||
// echoing its own character.
|
||||
await new Promise((resolve) => setTimeout(resolve, 300));
|
||||
await flush();
|
||||
await el.updateComplete;
|
||||
|
||||
return el;
|
||||
}
|
||||
|
||||
it('shows a requested album as queued rather than as absent', async () => {
|
||||
await withRequests([request({ mbid: 'rg-wanted' })]);
|
||||
|
||||
const el = await searchFor([
|
||||
releaseGroup('rg-wanted', 'Wanted Album'),
|
||||
releaseGroup('rg-other', 'Some Other Album'),
|
||||
]);
|
||||
|
||||
const badges = shadowAll(el, 'library-status-indicator');
|
||||
|
||||
expect(badges.length).toBeGreaterThanOrEqual(2);
|
||||
expect(badges.map((b) => b.getAttribute('status'))).toEqual([
|
||||
'queued',
|
||||
'not-in-library',
|
||||
]);
|
||||
});
|
||||
|
||||
it('re-renders when the request list changes underneath it', async () => {
|
||||
// A background reconcile pass expands an artist or retires a want
|
||||
// without this page doing anything, so the badge has to be told.
|
||||
const el = await searchFor([releaseGroup('rg-wanted', 'Wanted Album')]);
|
||||
|
||||
expect(shadow(el, 'library-status-indicator')?.getAttribute('status')).toBe(
|
||||
'not-in-library',
|
||||
);
|
||||
|
||||
await withRequests([request({ mbid: 'rg-wanted' })]);
|
||||
await el.updateComplete;
|
||||
|
||||
expect(shadow(el, 'library-status-indicator')?.getAttribute('status')).toBe(
|
||||
'queued',
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user