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` -