From edb13a6f395b78d27f99e3f108252f6da0dd653c Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Fri, 14 Aug 2026 13:34:15 -0400 Subject: [PATCH] perf(explore): ask the disk once, prefetch once, and menu the releases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things on the Explore surfaces, all about not asking twice. A portrait already on disk costs no network call. explore-view seeded only from the library store — owned artists, which on a catalog search is nearly none of the results — and sent everything else to GetArtistImageURL, the resolving entry point, one await at a time. GetArtistImagesCachedPaths asks the disk about every unresolved artist in one call, and only what it does not answer reaches the resolver, in parallel. The artist page's two sections both wanted PrefetchReleases and each called it, so the most expensive call the app makes was issued twice for an overlapping set on a 1 req/s limiter. They are collected and sent once on a microtask, and prefetchRequested stops the cold-artist refetch re-asking for what it already asked for. The release cards — most of the artist page — had no context menu at all. They have one now on both release shapes, normalised to a ReleaseMenuTarget when the menu opens so the union does not reach the action handlers. It is a discriminated union rather than one nullable field per kind because the panel is shared with the track menu: that is what keeps aria-label moving with the target, which is the fault cover-grid shipped. Which items appear is three different questions — playback is gated on a local album id, not on "owned", and the request needs a catalog MBID, so it is absent for a library-only release. Note on the docs: the CLAUDE.md and NOTES.md prose here was reconstructed after a mishandled `git stash --keep-index` destroyed the uncommitted originals. One NOTES.md section is marked as incomplete where its text could not be recovered. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UDCbcCZQepnpSQYJ6SxxZm --- .planning/NOTES.md | 28 ++ .../011-owned-artists-full-discography.md | 146 ++++++ .../010-owned-album-catalog-offline.md | 12 + .planning/plans/pending/012-api-call-audit.md | 158 +++++++ CLAUDE.md | 169 +++++++ .../artist-details/artist-details.ts | 17 +- .../explore-artist-details.ts | 439 +++++++++++++++--- .../components/explore-view/explore-view.ts | 64 ++- frontend/src/components/jobs/job-format.ts | 5 + frontend/src/store/job-store.ts | 6 +- frontend/src/utils/explore-link.ts | 7 +- .../test/components/artist-images.test.ts | 89 ++++ .../test/components/artist-prefetch.test.ts | 119 +++++ .../components/artist-release-menu.test.ts | 172 +++++++ 14 files changed, 1361 insertions(+), 70 deletions(-) create mode 100644 .planning/plans/active/011-owned-artists-full-discography.md create mode 100644 .planning/plans/pending/012-api-call-audit.md create mode 100644 frontend/test/components/artist-images.test.ts create mode 100644 frontend/test/components/artist-prefetch.test.ts create mode 100644 frontend/test/components/artist-release-menu.test.ts diff --git a/.planning/NOTES.md b/.planning/NOTES.md index db33651..64172f2 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -157,6 +157,34 @@ would be two copies free to drift. Bulk loads must suspend them: measured on a real import, assembly runs at ~31 rows/s with the triggers attached and ~4,700 rows/s without. +They are now **dropped and recreated** on every open rather than created +with "already exists" tolerated, because a trigger is a definition: an +existing install would otherwise keep the first one it ever got, and +these definitions are where this table's write cost is decided. +`explore_index_au` is scoped to `UPDATE OF title, artist_name, aliases` +with a `WHEN` guard on them having changed, so the common write — an +upsert whose merge rules keep every existing value — re-indexes nothing. + +## The writer stall that broke playback is only half explained (2026-08-14) + +The user's report — play an album, the track changes, the transport +stays paused, nothing appears in the queue, the play button does +nothing — was diagnosed on the running app: 91% of its CPU was +`BackfillLibraryDiscographies` → `upsertBatch`, with four of its six +workers parked in `sql.(*DB).conn` waiting for the single write +connection, while the play path did its writes inline under `q.mu` and +`p.mu`. The locking half is fixed and tested (see CLAUDE.md, "Playing a +track does not wait for the database to hear about it"). + +**What is not explained is why those upserts were so expensive.** + +> **Recovered note, 2026-08-14.** The rest of this section was lost to a +> mishandled `git stash --keep-index` before it was ever committed; what +> survives above is verbatim, and the two arguments that followed +> "Two things argue against the obvious answer" are gone. Re-derive them +> from a profile before acting on this — do not treat the question as +> answered. + ## Explore "library only" toggle was removed (2026-08-06) The Explore UI used to have a "library only" mode toggle diff --git a/.planning/plans/active/011-owned-artists-full-discography.md b/.planning/plans/active/011-owned-artists-full-discography.md new file mode 100644 index 0000000..8caee96 --- /dev/null +++ b/.planning/plans/active/011-owned-artists-full-discography.md @@ -0,0 +1,146 @@ +# 011 — An owned artist's discography, whole and offline + +**Status:** built, **not yet verified against a real library**. Lint, +the three Go test configurations, `tsc` and the Vitest suite all pass; +what has *not* happened is a run against a seeded library with real +MusicBrainz traffic, which is the only thing that can show the pass +completing an artist end to end. Do that before moving this to +`completed/`. +**Branch:** main +**Created:** 2026-08-13 +**Depends on:** nothing +**Related:** 010 (owned albums, offline) — the same rate limiter, the +next layer down. 010 warms *tracklists*; this warms the *list of +albums*. Read 010's "the rate limiter is the whole design constraint" +section before building either. + +--- + +## The problem + +`BackfillLibraryDiscographies` sounds like it does this and does not. +Per owned artist, `indexOneArtist` (`searchindex.go:1910`) fetches from +ListenBrainz: + +- `fetchTopReleaseGroups` — capped at `indexMaxRGs` (50) +- `fetchTopRecordings` — capped at `indexMaxRecs` (200) + +and both drop anything under `indexMinPopularity` (50 listens). So what +an owned artist's page shows offline is **their fifty most-listened +release groups**, not their discography. For an artist with a long tail +— early EPs, live albums, splits, anything regional — the missing rows +are precisely the ones a user who owns that artist is most likely to be +looking for. + +**It is also untyped.** LB's `top-release-groups-for-artist` returns no +secondary types, so the first view of every backfilled artist has no +EP / Live / Compilation / Soundtrack distinction — the discography +renders as one undifferentiated list. + +MusicBrainz's browse-by-artist has both the full list and the types, +and `BrowseReleaseGroups` (`explore.go:589`) already knows it: on +finding no secondary types on any indexed row it fires the browse **in +a goroutine, for next time**, and `AddFromCache` writes the result into +the index. So the fix is not new machinery. It is running that call +deliberately, once per owned artist, at scan time instead of +accidentally, on view, one artist at a time. + +## What to build + +Extend the existing post-scan pass — it is already bounded, resumable, +idempotent and ordered by owned-track count, which is the shape this +needs and the proven one in this codebase. + +Per unenriched owned artist, in addition to today's LB fetches: + +1. **`BrowseReleaseGroups`, paged to exhaustion.** `musicbrainz.go:318` + issues a single `Paginator{Limit: MaxLimit}` with no offset loop, so + a prolific artist is silently truncated at 100 release groups. Page + until a short response. This is the one change that makes the word + *full* honest, and it is a change to a function the interactive path + also calls — which is a win, not a risk. +2. **`SimilarArtists`.** `similar_artist_map` is not in the shipped + artifact and is filled lazily on view (`explore.go:905`), so it is + empty for every artist nobody has opened. It is one LB labs call and + already persists; folding it in here costs a request and removes the + page's last routine network dependency. + +Deliberately **not** in scope: cover art for non-owned release groups. +It is roughly *RGs per artist* fetches rather than one — an order of +magnitude more requests than everything else here combined — and a +missing thumbnail degrades to a placeholder, where a missing release +group degrades to a page that is quietly wrong. Covers stay lazy. + +## Four things that bite + +**`discog_fetched` is one boolean and would now cover three fetches +with different failure modes.** Today it is set only if an LB fetch +returned rows (`indexOneArtist:1962`), which is the right rule for one +call and useless for three — an MB failure would either permanently +claim the artist as done or force the LB fetches to repeat. Track the +facets separately. Prefer **a new table keyed by artist MBID** over new +`explore_index` columns: `artifactimport.go:95` enumerates the columns +the artifact merge preserves, so a flag column added there is a second +place to remember, and forgetting it silently wipes every mark on the +next artifact update. A new table is also the single-file schema case +(`CREATE TABLE IF NOT EXISTS`, no migration) and needs a `datamap` +entry — `Cache` / `Swept`, since it is re-derivable. + +**The `hasSecondaryTypes` heuristic re-fires forever for an artist who +has none.** An artist whose discography is entirely plain albums writes +`secondary_types = ''` on every row, so the "we must be missing them" +test is true on every visit and browses again (cheaply — 7-day +`cacheTTLEntity` — but forever). An explicit per-artist "browsed at" +mark retires the heuristic, which is a second reason for the table +above. + +**Popularity is safe, and only because of the upsert rule.** +`AddFromCache` writes `Popularity: 0` for every browsed release group; +`upsertIndexConflictSQL:2180` is "highest wins", so it cannot clobber +the LB figures. The consequence is one to state rather than fix: +`TopReleaseGroupsByArtist` orders by popularity descending, so the deep +cuts this plan adds sort below the top fifty. That is the correct +order. + +**The MB limiter is shared — and the priority work this needed is +done.** ~~One `NewRateLimiter()` at 1 req/s serves this, +`PrefetchReleases`, and every interactive browse~~ — 010 says that and +it is wrong on the detail: `e.mb` runs on `mbSearchLimiter`, +`NewRateLimiterBurst(3, 1)`, while the 1/s `NewRateLimiter()` at +`explore.go:84` is the *artist image* limiter. Both were shared with +background work and both are FIFO, which was the real problem. + +Shipped ahead of this plan (same session it was written): + +- `RateLimiter.WithBackgroundLane(perSecond)` plus + `WithBackgroundPriority(ctx)` — a marked caller yields entirely while + any interactive wait is outstanding, and is paced at MB's own 1/s + rather than the interactive burst rate. The marker is a context value + so a backfill and a detail page can call the same + `MusicBrainzClient` method and be treated differently. +- Both existing backfills mark their context, including the artist + image resolution (`GetArtistImage` takes a `ctx` now for no reason + other than carrying that marking). +- `jobs.KindCatalogEnrich` and `startBackfillJob` — both backfills are + registered, cancellable, and show progress. No job is registered + when there is nothing to do, which is every launch once the library + is covered. + +So this plan inherits the lane: mark the new fetches background and add +them to the existing job's progress. What it must **not** do is treat +"a backfill is now polite" as licence to widen it without measuring — +the yield gate protects latency, not the origin's patience. + +## Done when + +- An owned artist's page, opened for the first time after a scan, + renders their complete typed discography with no network call — + including release groups under the popularity floor and beyond the + first 100. +- Similar artists render offline for an owned artist nobody has opened. +- An interactive browse issued while the backfill runs is not delayed + by it. +- The backfill appears in the jobs indicator and can be paused and + cancelled. +- A second run after a completed one does approximately nothing, and an + artifact update does not undo a completed one. diff --git a/.planning/plans/pending/010-owned-album-catalog-offline.md b/.planning/plans/pending/010-owned-album-catalog-offline.md index ca50dde..90d0252 100644 --- a/.planning/plans/pending/010-owned-album-catalog-offline.md +++ b/.planning/plans/pending/010-owned-album-catalog-offline.md @@ -110,6 +110,18 @@ Sketch: ### The rate limiter is the whole design constraint +> **Update (2026-08-13): the priority half is built, and the sentence +> below is wrong on a detail.** `e.mb` runs on `mbSearchLimiter` +> (`NewRateLimiterBurst(3, 1)`); the 1 req/s `NewRateLimiter()` cited +> here is the *artist image* limiter. Both are shared and both were +> FIFO. `RateLimiter.WithBackgroundLane` + `WithBackgroundPriority(ctx)` +> now make a marked caller yield to interactive work and pace at 1/s, +> and `jobs.KindCatalogEnrich` + `startBackfillJob` give the existing +> backfills progress and cancel. **"Do not start until the priority +> question has an answer" is satisfied** — mark this backfill's context +> and register it the way `BackfillLibraryDiscographies` now is. +> `PrefetchReleases`' cap of 8 is still unrevisited. + One shared `NewRateLimiter()` at 1 req/s (`explore.go:84`) serves this, `PrefetchReleases`, and every interactive browse. A backfill over a few thousand owned albums is *hours* of wall clock at that rate — which diff --git a/.planning/plans/pending/012-api-call-audit.md b/.planning/plans/pending/012-api-call-audit.md new file mode 100644 index 0000000..cf65f80 --- /dev/null +++ b/.planning/plans/pending/012-api-call-audit.md @@ -0,0 +1,158 @@ +# 012 — What we ask the network for, and what we already had + +**Status:** all four findings fixed. Lint (3 configs), Go tests (3 +configs), `tsc` and 752 Vitest tests pass; **not driven against the +real app**, so the numbers below are read off the code, not measured. + +One claim in the audit was wrong and is corrected in finding 3: +`CheckLibraryMBIDs` is *not* dead — `downloadcatalog.go:152` calls it. +It has no *frontend* caller, which is what was checked and not what was +written. +**Branch:** none yet +**Created:** 2026-08-13 +**Related:** 010 (owned albums offline), 011 (owned artists' discography) + +--- + +## Scope + +Every frontend call site that can reach the network, and the backend +method behind it. The question asked of each: *is there a local answer +first, and if we do go out, do we go out once for many things or many +times for one?* + +## What is already right, and is the standard the rest is measured against + +- **Every catalog read is index-first.** `LookupArtist`, + `LookupReleaseGroup`, `BrowseReleaseGroups`, + `TopRecordingsForArtist`, `TopReleaseGroupsForArtist`, + `SimilarArtists` and `ResolveReleaseGroupMBIDs` all answer from + `explore_index` / `similar_artist_map` and only fall through on a + miss — several kick a background fetch and return empty rather than + blocking, with a `*Ready` event to re-read. +- **Album art has the right shape:** seed from the library, one + `GetThumbnails` batch that is *cached-only by contract*, then + per-item `GetThumbnail` calls that stream in + (`explore-view.ts:1445`). Nothing waits on a batch of network + fetches. +- **Artist art has the right shape in exactly one place:** + `seedSimilarArtistImagesFromLibrary` + (`explore-artist-details.ts:1627`) — library store, then disk-only + `GetArtistImageCachedPath`, fired in parallel, zero network calls. + It is the model for finding 1. + +## Finding 1 — Explore's artist images: no disk check, and serial + +`explore-view.ts:1526-1546`. `loadArtistImages` seeds from +`libraryStore.cachedArtists` — i.e. **owned artists only**, which on a +catalog search is a small minority of results — and then, for every +remaining artist: + +```ts +const url = await GetArtistImageURL(a.mbid); // in a for loop +``` + +Two faults, both fixed by patterns already in the codebase: + +- **No cached-path pass.** `GetArtistImageCachedPath` and + `GetArtistImageCached` are disk-only and free, and neither is used + here. An artist whose portrait is already on disk from a previous + search still takes the resolution path. +- **`await` in a loop.** `GetArtistImageURL` is the *resolving* entry + point: on a miss it does MB artist-rels (on the 1/s artist-image + limiter) → Wikidata → Wikipedia → a Wikimedia image download. Serial + awaits mean 8 unresolved artists are 8 of those end to end, each + blocking the next, while the equivalent album-art path fires all of + them at once. + +The same "resolver used where a cache check belongs" appears at +`top-results-row.ts:218` and `artist-details.ts:207` (both fire in +parallel, so only the first fault applies, and both are small-N). + +**Fix:** disk-cached pass first, then network in parallel. A +`GetArtistImagesCached(mbids []string) map[string]string` mirroring +`GetThumbnails` would make it one IPC call instead of N — see finding 4 +for why that is not `GetArtistImages`. + +## Finding 2 — The artist page prefetches tracklists twice, or four times + +`prefetchReleases` (`explore-artist-details.ts:1531`) is called from +**both** `fetchTopReleaseGroups` (:1467) and `fetchReleaseGroups` +(:1506), and `PrefetchReleases` fires up to **8** `BrowseReleases` per +call — the most expensive request the app makes (every version of a +release group, with `recordings` and `media`). + +The top release groups are a subset of the discography, so the two +calls are asking about overlapping sets; the backend's +`BrowseReleasesCached` guard stops a *literal* repeat, which means the +second call spends its 8 slots on the next 8 uncached albums rather +than doing nothing. One page view is therefore up to 16 browses — and +on a cold artist, `ArtistDiscographyReady` re-runs both fetchers +(:945, :948), taking it to 32. + +Worse, some of that is now provably wasted: since tag-derived +completeness landed (`dcc40b1`), **a complete, MBID-matched album opens +with no catalog call at all**, so warming its tracklist buys nothing. + +**Fix, in order of value:** + +1. Prefetch once, from the union of both lists, after both resolve. +2. Skip release groups that are owned and complete — + `GetAlbumCompleteness` already answers this locally. +3. Revisit the cap of 8 with the other two in place. Plan 010 flags + the same number from the other direction. + +## Finding 3 — Batch helpers with no caller (one of which was live) + +`CheckLibraryMBIDs`, `GetPopularityBatch` and `GetArtistImages` are +bound to the frontend and have **no call site in `frontend/src`**. +They are the batch shapes a future N+1 would want, and their existence +is presumably why the N+1s above were not noticed. + +**`CheckLibraryMBIDs` is not dead** — `downloadcatalog.go:152` calls +it from Go, one MBID at a time. Deleting it broke the build, which is +how that was found; it is kept, with a comment saying who its consumer +is. Read "no frontend caller" as exactly that, and grep both languages +before removing a bound method. + +Note `GetArtistImages` is not the helper finding 1 needs: it resolves +names through `libMBID.AllArtistMBIDs()`, so it only answers for +artists **in the library** — the exact set Explore's search results are +not. Either give it an MBID-keyed sibling or replace it. + +Also bound with no caller, and worth a separate decision about whether +the feature is live at all: `GetTrackLyrics`, `GenerateMix`, +`GetArtistPlayCount`, `GetLibrarySimilarArtists`, +`GetCandidateThumbnail`. + +## Finding 4 — One more background pass with no job and no priority + +`BackfillLibraryLyrics` (`lyrics.go:129`) is a bare `go` call: bounded +by passes and per-track (LRCLIB has no batch endpoint, so per-track is +correct), but with no `jobs` registration and no +`WithBackgroundPriority` marking. It runs on its own limiter, so it +starves nothing today — but it is invisible and uncancellable, which is +the gap 011 just closed for the other two backfills. + +## Not a finding, recorded so it is not re-audited + +- `GetThumbnails` returning only cached entries is deliberate and + documented; the per-item follow-up is the streaming half, not an + N+1. +- `explore-artist-details` calling both `TopReleaseGroupsForArtist` + (50) and `BrowseReleaseGroups` (200) reads overlapping rows from the + index twice, but both are local queries feeding two different + sections. Not worth merging. +- The newest components (`home-view`, `catalog-scope-notice`, + `page-header`, the notification stack, `shortcuts-overlay`) make no + network calls at all. `home-view` is `GetShelves` + `GetAlbumTracks`, + both local. + +## Done when + +- An Explore search with no owned artists in it makes zero artist-image + network calls for portraits already on disk, and resolves the rest + concurrently. +- Opening an artist page issues one prefetch pass, over albums that are + not already fully owned. +- The bound-but-uncalled batch helpers are either wired or removed. diff --git a/CLAUDE.md b/CLAUDE.md index 36d85f7..bce4f9c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -327,6 +327,120 @@ work happens **once, centrally**, and users download the result: (`dumpincremental.go`), and resolves artists outside the artifact's coverage lazily on first view. +**Background work yields, and says so in the context.** The post-scan +backfills share MusicBrainz's rate limiters with every page the user +can open, and both were FIFO — so a thousand-artist enrichment put an +album page behind an hour of queued work. +`RateLimiter.WithBackgroundLane(perSecond)` adds a second, slower lane +and `WithBackgroundPriority(ctx)` marks a caller as belonging to it: a +marked wait takes no token at all while any interactive wait is +outstanding, and is then paced at MB's own 1/s rather than the +interactive burst rate. It is a **context marker rather than a +parameter** because a backfill calls the same `MusicBrainzClient` +methods a detail page does — `GetArtistImage` takes a `ctx` for no +other reason than to carry it. One request of slippage is accepted and +documented at `waitBackground`: cancelling an already-granted +reservation is not something a token bucket can express, and the cost +is one request-time. + +The other half is that a long backfill has to be **visible and +stoppable**: `jobs.KindCatalogEnrich` and `startBackfillJob` +(`backfilljob.go`) register both backfills with progress and cancel. +Two rules in it are load-bearing. The job is registered *after* the +work is counted, because these passes are a no-op on every launch once +the library is covered and an empty job in the indicator is noise. And +the kind is distinct from `index-build` rather than reused, because +`job-controls.ts` keys its "you will discard hours of downloading" +confirmation on that kind — wrong prompt for a pass that is resumable +per artist and free to stop. + +**An owned artist's discography is fetched, not sampled.** +`BackfillLibraryDiscographies` is two fetches per owned artist, each +skipped by its own persistent mark, because they fail independently and +one boolean covering both either over-claims or forces repeats: +the ListenBrainz top release groups and recordings +(`explore_index.discog_fetched`) and the **full** MusicBrainz browse +(`artist_enrichment.browsed_at`). + +**What it does not fetch is the point.** It ran for hours against a +900-artist library and marked nothing, because three of the four things +it did per artist were work nobody had asked for. Similar artists were +fetched for every owned artist, when the artist page already resolves +them on view through `SimilarArtists` → `ensureSimilarArtistsAsync` — +which is what stamps `similar_at` now. And `indexOneArtist` reached the +MB artist lookup it wants (`GetArtistDetails` reads that cache) by +calling `GetArtistImage`, which additionally queried fanart.tv, +TheAudioDB, Wikidata and Wikipedia and downloaded up to ten full-size +portraits; `EnsureArtistRels` is the lookup on its own. The corollary +is written into the query: **`similar_at` must not be one of the +conditions** in `unenrichedLibraryArtistMBIDs`, because testing a mark +this pass no longer sets makes every owned artist a candidate on every +run, forever. + +The rest is that the pass was **serial across artists** while every +limiter that keeps us polite is per-host and idle — so one artist's +slowest upstream set the pace for the whole run. It runs +`discogBackfillWorkers` artists at once (concurrency here raises no +origin's request rate), each under `discogBackfillArtistTimeout`, +because the MB client retries a 503 five times honouring Retry-After +and one throttled artist could otherwise outlast a hundred healthy +ones. A timed-out artist goes unmarked and is retried next run, which +is what every other failure here already does. SQLite's writer pool is +`MaxOpenConns(1)`, so the workers queue at the Go level rather than +racing for the file. + +Four things about the marks are load-bearing. The marks are **a table, not +more `explore_index` columns**, because `artifactimport.go` merges the +downloaded catalog by column list — a flag added there is a second +place to remember, and forgetting it silently wipes every mark on the +next catalog update. `discog_fetched` stays in `explore_index` for the +opposite reason: the artifact legitimately answers it for artists it +covers. **The artifact answering it is not "we have their +discography"** — its per-artist coverage is graded, so an artist can +arrive `discog_fetched = 1` and never have been browsed, which is why +the unenriched query ORs its conditions instead of testing the +first. **`BrowseReleaseGroupsAll` pages to exhaustion** where +`BrowseReleaseGroups` asks for `MaxLimit` once and takes what comes +back — a prolific artist was silently cut at 100 release groups, and a +hundred albums looks like a complete answer unless you count. And the +per-artist mark **replaced a heuristic that could never be satisfied**: +`BrowseReleaseGroups` used to re-browse whenever no indexed row carried +a secondary type, which is permanently true for an artist whose +releases are all plain albums. + +**One artist portrait is downloaded; the rest are remembered as URLs.** +`resolveAllSources` asked five upstreams what images they had for an +artist and then downloaded **every** candidate, up to ten, full size, +serially — while nothing in the app has ever read anything but +`primary.jpg` and its three tiers. Measured on a real cache: 5.3 GB, +of which 4.1 GB was candidates no code path can reach, ~940 kB per +artist against the ~217 kB that is actually used. + +It is split now. `resolveCandidates` does the metadata lookups and +returns an ordered list; `fetchPrimary` walks that list downloading +until one **succeeds**, makes that the primary, and records the rest +with an empty `file_path` — known, not fetched — so replacing a +portrait later is one download rather than five lookups again. Taking +the first that succeeds rather than the first outright is also a fix: +the old loop keyed `is_primary` on the index, so a failed candidate 0 +left the artist with a stored image, no `primary.jpg`, and a `.miss` +marker claiming there was no artwork at all. The winning candidate is +no longer also written under its own name, since `setPrimary` writes +the same bytes to `primary.jpg`. + +Two janitor jobs go with it, and the first is why the waste survived. +`OrphanedArtistImagesJob` joined the bare MBID onto the images +directory — but artist directories are **sharded** under a two-character +prefix, so it named a path that has never existed, `RemoveAll` +succeeded on it, and the job deleted the rows that were the only record +of the files it left behind. `explore.ArtistImageDir` is that layout's +one definition, passed in the way `OrphanedCoverFilesJob` takes +`expandVariants`, and the test lays its fixtures out with it — a test +that invents its own flat layout agrees with the bug. +`StrayArtistImageFilesJob` reclaims what earlier versions downloaded, +keeping only `explore.ArtistImageKeepNames()` and refusing an empty +keep set for the reason the covers sweep refuses an empty live set. + **Frontend** (`frontend/`): Lit 3.2 web components + Web Awesome UI library + HTMX. State management via singleton reactive stores in `src/store/`. Wails bindings auto-generated in `frontend/wailsjs/` — don't edit by hand. **A view is a chunk, and three components are not.** `index.ts` holds a @@ -506,6 +620,29 @@ its equivalent predates this, carries selection semantics (shift-extend, ctrl-toggle) the other three do not have, and is pinned by its own tests. +**A shared panel computes its own name, and its items ask what the +target can do.** `explore-artist-details` had a menu for its top +*tracks* and none on the release cards, which are most of the page. It +has one now on both release shapes — the top section's +`LBTopReleaseGroup` and the discography's `MBReleaseGroup` — normalised +to a `ReleaseMenuTarget` at the moment the menu opens, so the union +does not reach five action handlers. `ctxMenuTarget` is a discriminated +union rather than one nullable field per kind **because the panel is +shared**: that is what keeps `aria-label` moving with the target, which +is the fault `cover-grid` shipped (every menu announced as "Album +actions"). + +Which items appear is decided by what the release can actually do, and +those are three different questions. Playback is gated on a **local +album id**, not on the badge's "owned": a release matched by MBID with +no local album behind it has nothing to queue, and `GetFilePathsBy‐ +Albums` is keyed on the id for the reason the album page is — an owned +but untagged release has no recording MBIDs, so an MBID-keyed lookup +returns nothing while looking entirely correct. The request item needs +the opposite, a catalog MBID, so it is absent for a library-only +release (`local:`, unwrapped the same way `navigateToAlbum` does) — +which is also the one case where wanting it makes no sense. + **Async surfaces say what they are doing.** `styles/sr-only.css.ts` carries the visually-hidden class and the rule that comes with it: a live region must be **in the DOM before the text it announces is**, @@ -1071,6 +1208,38 @@ reader and is created lazily, so neither the invalidation nor — more expensively — the singleton's own construction warms a cache for a page that may never open. +**Playing a track does not wait for the database to hear about it.** +Every write in this app goes through one connection — +`database.DB` is `MaxOpenConns(1)`, because SQLite has one writer — and +a background pass can hold it for a long time. The player and the queue +used to write inline from paths that hold their own mutexes, so a +contended writer did not merely slow persistence down: `SetQueue` +blocked in `LoadFile`'s `saveState` and then in `persistState`, **while +holding `q.mu` and `p.mu`**. The user's report is the exact shape of +that — the track changed and the transport sat at paused (`LoadFile` +had emitted `TrackChanged` and `PlaybackStateChanged(paused)`, and +`p.Play()` is *after* the writes), nothing appeared in the queue +(`emitQueueChanged` is after them too), and the play button did nothing +because `Queue.Play` was waiting on the same held `q.mu`. It was +diagnosed by profiling the running app: 91% of its CPU was +`explore.BackfillLibraryDiscographies` → `upsertBatch`, with four more +of its six workers parked in `sql.(*DB).conn`. + +So a write is **submitted, not performed** +(`queue/persistwriter.go`, `player/persistwriter.go`): jobs run in +submission order on one goroutine per component, each carrying its own +snapshot — which is what keeps "clear and rewrite the queue" and +"insert three tracks at 4" meaning what they meant when they were +called. Two rules come with it. A job **must not touch the component's +fields**: it holds no lock and the state has moved on, which is why +`persistTracks` clones. And `SaveState` — shutdown, and the tests — +still flushes and waits, because that is the one caller for which the +row has to exist on return. + +The corollary for anything new: **a durability write is not a step in a +user action**. If a mutation path needs the database to have finished +before it returns, that is a claim worth arguing for, not a default. + **"Remove from library" removes the row and excludes the path, and never touches the file.** `RemoveFromLibrary(filePaths)` deletes the `audio_files` rows the way the scan's own orphan cleanup does (tagging diff --git a/frontend/src/components/artist-details/artist-details.ts b/frontend/src/components/artist-details/artist-details.ts index b75c30c..e829125 100644 --- a/frontend/src/components/artist-details/artist-details.ts +++ b/frontend/src/components/artist-details/artist-details.ts @@ -6,7 +6,11 @@ import { } from 'lit/decorators.js'; import { library } from '@go/models'; import { LibraryController } from '@store/controllers/library-controller'; -import { GetArtistImageURL, GetArtistMBID } from '@go/explore/Service'; +import { + GetArtistImageURL, + GetArtistImageCachedPath, + GetArtistMBID, +} from '@go/explore/Service'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; import '@components/cover-grid/cover-grid.js'; import { designTokens } from '../../styles/tokens.css'; @@ -204,6 +208,17 @@ export class ArtistDetails extends LitElement { if (!mbid) return; try { + // Disk cache first — the resolving call below is MB → + // Wikidata → Wikipedia → Wikimedia, and most artists this + // page renders have been resolved once already. + const cached = await GetArtistImageCachedPath(mbid); + + if (cached) { + this.artistImageURL = cached; + + return; + } + const url = await GetArtistImageURL(mbid); if (url) { 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 ef9578d..91af887 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,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 { libraryStatusFor, toggleRequest } from '@utils/library-status'; 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'; @@ -61,6 +61,29 @@ import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js'; /** The region the artist header's own failures are rendered in. */ export const ExploreArtistRegion = 'explore-artist'; +/** + * A release the context menu can act on, whichever shape it came from. + * + * The page shows release groups in two forms — the top section's + * `LBTopReleaseGroup` and the discography's `MBReleaseGroup` — and the + * three questions the menu asks are not the same question: playback + * needs a *local album id*, a request needs a *catalog MBID*, and + * "owned" is neither on its own. + */ +interface ReleaseMenuTarget { + /** The catalog release-group MBID, or '' for a library-only release. */ + mbid: string; + /** The local album id, or 0 when nothing local backs it. */ + localId: number; + title: string; + owned: boolean; +} + +/** What the shared context menu panel is currently about. */ +type ContextMenuTarget = + | { kind: 'track'; track: LBTopRecording } + | { kind: 'release'; release: ReleaseMenuTarget }; + /** Desired section order for grouping release types. */ const TYPE_ORDER = ['Albums', 'EP', 'Single', 'Other Albums']; @@ -147,14 +170,32 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost @state() private similarExpanded = false; private libraryMBIDs = new Set(); - /* ── Track context menu ── */ + /* ── Release prefetch ── */ + + /** Top-section mbids awaiting the next coalesced prefetch. */ + private pendingTopPrefetch = new Set(); + /** Discography mbids awaiting the next coalesced prefetch. */ + private pendingPrefetch = new Set(); + private prefetchScheduled = false; + /** Every mbid already sent, so a refetch does not re-ask. */ + private prefetchRequested = new Set(); + + /* ── Context menu ── */ private ctxMenu = new ContextMenuController(this); - /** The top track the open context menu applies to. */ - @state() private ctxMenuTrack: LBTopRecording | null = null; + /** + * What the open context menu applies to. + * + * A discriminated union rather than one nullable field per kind, + * because the panel is shared between the top-tracks list and the + * release cards: that is what keeps `aria-label` moving with the + * target, which is the fault `cover-grid` shipped — every menu + * announced as "Album actions". + */ + @state() private ctxMenuTarget: ContextMenuTarget | null = null; - @query('#track-context-menu') + @query('#context-menu') private contextMenuPopup!: WaPopup; // -- ContextMenuHost interface -- @@ -170,7 +211,21 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost } onContextMenuClose(): void { - this.ctxMenuTrack = null; + this.ctxMenuTarget = null; + } + + /** The open menu's track, or null when it is not a track menu. */ + private get ctxMenuTrack(): LBTopRecording | null { + return this.ctxMenuTarget?.kind === 'track' + ? this.ctxMenuTarget.track + : null; + } + + /** The open menu's release, or null when it is not a release menu. */ + private get ctxMenuRelease(): ReleaseMenuTarget | null { + return this.ctxMenuTarget?.kind === 'release' + ? this.ctxMenuTarget.release + : null; } /* ── Styles ── */ @@ -1464,7 +1519,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost // Warm the release/tracklist cache for the top albums — these // are the most likely to be clicked from the artist page. - this.prefetchReleases(rgs?.map((r) => r.releaseGroupMbid) ?? []); + this.prefetchReleases(rgs?.map((r) => r.releaseGroupMbid) ?? [], true); } catch (err) { console.error('[explore-artist] TopReleaseGroupsForArtist error', err); this.topReleaseGroups = []; @@ -1525,14 +1580,56 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost /** * Warm the backend's release/tracklist cache for a set of release - * groups so opening an album from this page is instant. Fire-and-forget. + * groups so opening an album from this page is instant. + * Fire-and-forget. + * + * The page's two sections resolve independently and both want this, + * so the mbids are collected and sent once on a microtask rather + * than once per section — `BrowseReleases` is the most expensive + * call the app makes, on a 1 req/s limiter, and asking twice for an + * overlapping set spends that limiter on nothing. + * + * `prefetchRequested` is what stops the cold-artist refetch — which + * re-runs every fetch on `ArtistDiscographyReady` — asking again for + * everything it already asked for. */ - private prefetchReleases(mbids: string[]) { - const filtered = mbids.filter((m) => m); - if (filtered.length === 0) return; + private prefetchReleases(mbids: string[], top = false) { + const pending = top ? this.pendingTopPrefetch : this.pendingPrefetch; - void PrefetchReleases(filtered).catch(() => { - /* best-effort cache warming — ignore failures */ + for (const mbid of mbids) { + if (mbid) pending.add(mbid); + } + + if (this.pendingTopPrefetch.size + this.pendingPrefetch.size === 0) { + return; + } + + if (this.prefetchScheduled) return; + + this.prefetchScheduled = true; + + queueMicrotask(() => { + this.prefetchScheduled = false; + + // Top releases lead: they are what the page shows first, and + // the backend takes the list in order. + const batch = [ + ...new Set([ + ...this.pendingTopPrefetch, + ...this.pendingPrefetch, + ]), + ].filter((mbid) => !this.prefetchRequested.has(mbid)); + + this.pendingTopPrefetch.clear(); + this.pendingPrefetch.clear(); + + if (batch.length === 0) return; + + for (const mbid of batch) this.prefetchRequested.add(mbid); + + void PrefetchReleases(batch).catch(() => { + /* best-effort cache warming — ignore failures */ + }); }); } @@ -1950,7 +2047,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost private onTrackRowKeydown(e: KeyboardEvent, track: LBTopRecording): void { if (isContextMenuKey(e)) { e.preventDefault(); - this.ctxMenuTrack = track; + this.ctxMenuTarget = { kind: 'track', track }; this.ctxMenu.openFrom(e.currentTarget as HTMLElement); return; @@ -1966,10 +2063,169 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost e.preventDefault(); e.stopPropagation(); - this.ctxMenuTrack = track; + this.ctxMenuTarget = { kind: 'track', track }; this.ctxMenu.openAt(e.clientX, e.clientY); } + /* ── Release context menu ── */ + + // The two release shapes on this page are normalised at the moment + // the menu opens, so the union does not reach the action handlers. + + /** Normalise a top-section release group. */ + private topReleaseTarget(rg: LBTopReleaseGroup): ReleaseMenuTarget { + return { + mbid: rg.releaseGroupMbid || '', + localId: rg.localId ?? 0, + title: rg.title, + owned: Boolean(rg.inLibrary || rg.localId), + }; + } + + /** + * Normalise a discography release group. + * + * A library-only release arrives as `local:`, which names nothing + * upstream — so its catalog MBID is empty and the local id is + * unwrapped from it, the same way `navigateToAlbum` does. + */ + private albumTarget(rg: MBReleaseGroup): ReleaseMenuTarget { + const isLocal = + typeof rg.mbid === 'string' && rg.mbid.startsWith('local:'); + const localId = rg.localId || (isLocal ? Number(rg.mbid.slice(6)) : 0); + + return { + mbid: isLocal ? '' : rg.mbid || '', + localId: Number.isFinite(localId) ? localId : 0, + title: rg.title, + owned: + this.libraryMBIDs.has(rg.mbid) || + Boolean(rg.inLibrary) || + localId > 0, + }; + } + + private onReleaseContextMenu(e: MouseEvent, release: ReleaseMenuTarget): void { + e.preventDefault(); + e.stopPropagation(); + + this.ctxMenuTarget = { kind: 'release', release }; + this.ctxMenu.openAt(e.clientX, e.clientY); + } + + private onReleaseKeydown( + e: KeyboardEvent, + release: ReleaseMenuTarget, + activate: () => void, + ): void { + if (isContextMenuKey(e)) { + e.preventDefault(); + this.ctxMenuTarget = { kind: 'release', release }; + this.ctxMenu.openFrom(e.currentTarget as HTMLElement); + + return; + } + + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + activate(); + } + } + + /** + * The release's files, keyed on the local album id. + * + * Keyed on the id rather than the MBID for the reason the album page + * is: an owned but untagged release has no recording MBIDs, so an + * MBID-keyed lookup returns nothing while looking entirely correct. + */ + private async releaseFilePaths( + release: ReleaseMenuTarget, + ): Promise { + if (release.localId <= 0) return []; + + const libraryID = libraryStore.getSelectedLibraryId() ?? 0; + const byAlbum = await GetFilePathsByAlbums([release.localId], libraryID); + + return byAlbum[release.localId] ?? []; + } + + private async onReleaseAction( + action: 'play' | 'add-to-queue' | 'play-next', + ): Promise { + const release = this.ctxMenuRelease; + + this.ctxMenu.close(); + + if (!release) return; + + try { + const paths = await this.releaseFilePaths(release); + + if (paths.length === 0) { + notificationStore.inline(ExploreArtistRegion, { + text: `No files for ${release.title} were found in your library.`, + }); + + return; + } + + switch (action) { + case 'play': + queueStore.setQueue(paths, 0, false, this.queueSource()); + break; + case 'add-to-queue': + for (const path of paths) queueStore.addToQueue(path); + break; + case 'play-next': + for (const path of [...paths].reverse()) + queueStore.playNext(path); + break; + } + } catch (err) { + console.error('Could not queue release:', err); + notificationStore.inline(ExploreArtistRegion, { + text: describeError(err, `Could not play ${release.title}.`), + }); + } + } + + private async onReleaseRequestToggle(): Promise { + const release = this.ctxMenuRelease; + + this.ctxMenu.close(); + + if (!release?.mbid) return; + + try { + await toggleRequest({ + mbid: release.mbid, + entity: 'album', + title: release.title, + artist: this.artist?.name ?? this.artistName, + }); + } catch (err) { + console.error('Could not change the request:', err); + notificationStore.inline(ExploreArtistRegion, { + text: describeError(err, `Could not request ${release.title}.`), + }); + } + } + + private viewReleaseOnMusicBrainz(): void { + const release = this.ctxMenuRelease; + + this.ctxMenu.close(); + + if (!release?.mbid) return; + + window.open( + `https://musicbrainz.org/release-group/${release.mbid}`, + '_blank', + 'noopener', + ); + } + private onContextMenuAction(action: 'play' | 'add-to-queue' | 'play-next'): void { const track = this.ctxMenuTrack; @@ -2196,7 +2452,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost region=${ExploreArtistRegion} testid="artist-action-message" > - ${this.renderTrackContextMenu()} + ${this.renderContextMenu()} `; } @@ -2233,40 +2489,37 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost `; } - private renderTrackContextMenu() { - const track = this.ctxMenuTrack; + /** + * The one context menu panel, shared by the top tracks and the + * release cards. + * + * The label is computed from the target rather than written down, + * which is the fault `cover-grid` shipped — a shared panel that + * announced every menu as "Album actions". + */ + private renderContextMenu() { + const target = this.ctxMenuTarget; return html` - ${this.ctxMenu.contextMenuOpen && track + ${this.ctxMenu.contextMenuOpen && target ? html` -