Compare commits

...
Author SHA1 Message Date
yonluandClaude Opus 5.5 bb26d5f289 feat(explore): arrows on every sideways row, and one album card size
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 4m25s
CI / e2e (pull_request) Successful in 13m43s
The shelves, the top results and the artist page's discography and
similar-artists rows scrolled sideways only by a horizontal wheel or a
trackpad, so a plain mouse could not reach anything past the fold.
<scroll-row> wraps each of them with previous/next arrows that are
hidden at the end they cannot move from, revealed on hover where there
is hover, and always shown where there is not.

The album card is defined once, in albumCardStyles, instead of twice.
explore-view clamped its cards to 130-150px, and with square artwork
that made cards in one row different heights as well as widths.  The
width is fixed now and each line under the art reserves its own space.
Covers are inset rather than cropped (contain, not cover).

The ownership badge sits over the artwork for owned and unowned alike,
and catalog cards no longer dim: a grid of dimmed covers read as a page
that had failed to load.  The album page's tracklist still dims unowned
rows, which says something different about something different.

Every MusicBrainz link now goes through openMusicBrainz, which pins the
origin, including the one on the album page that still called
window.open directly.  solid/chevron-left is bundled for the left arrow;
without it the arrow drew the missing-icon fallback.

Closes #264

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT
2026-09-26 16:59:28 -04:00
yonluandClaude Opus 5.5 7b42b9ce56 feat(explore): sample the one-album-owned shelf instead of ranking it
"More from artists you own one album by" drew its artists and their
albums most-popular first, so it was a second leaderboard: the same
handful of big names every time the page opened, which is not what the
shelf is saying.  The pool is still bounded, but which artists and
which albums fill it is RANDOM(), so each visit is a different sample.
The test asserts the set rather than the order.

Refs #264

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT
2026-09-26 16:58:57 -04:00
yonlu a5c3990d12 Merge pull request 'Small-fix batch: artist_metadata sweep, and the three unbounded surfaces (#248, #249)' (#255) from batch/248-249 into main
CI / check (push) Skipped
CI / e2e (push) Skipped
Build & publish the Android APK / apk (push) Successful in 2m22s
Build & publish Arch package / arch-package (push) Successful in 3m23s
Attach the desktop build to the release / linux (push) Successful in 3m23s
Sync Homebrew formula / sync-formula (push) Successful in 9s
Closes #248
Closes #249
2026-09-21 02:01:54 +00:00
yonlu 8dbdb7ad75 Merge branch 'fix/249-unbounded-growth' into batch/248-249
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 3m42s
CI / e2e (pull_request) Successful in 11m12s
Both branches add to the same two registries, so the conflicts are
between the two fixes rather than with main:

- backend/app.go: both register a janitor job.  Both are registered.
- backend/maintenance/sweeps.go: both append a job at the end of the
  file.  Both are kept, each with its own closing tail.
- backend/maintenance/maintenance_test.go: both append a test.  Both are
  kept as separate functions.
- backend/library/library.go: #249's orphan-path lyrics delete was
  written against the loop variable before #250 renamed it, so its
  `audioFile.ID` no longer exists in that function.  Adapted to `f.ID`.

Closes #248
Closes #249
2026-09-20 21:40:49 -04:00
yonlu a205224a26 Merge branch 'fix/248-artist-metadata-sweep' into batch/248-249 2026-09-20 21:34:50 -04:00
yonlu a96cc9be1f Merge remote-tracking branch 'origin/main' into fix/248-artist-metadata-sweep
CI / e2e (push) Skipped
CI / check (push) Skipped
CI / check (pull_request) Successful in 3m22s
CI / e2e (pull_request) Successful in 11m43s
2026-09-20 21:15:14 -04:00
yonlu 8db19622b2 Merge pull request 'fix(library): sweep orphaned cover art when an album empties' (#251) from fix/247-cover-art-orphans into main
CI / check (push) Successful in 3m20s
CI / e2e (push) Successful in 12m5s
default
2026-09-21 01:14:58 +00:00
yonlu 53f480980f Merge remote-tracking branch 'origin/main' into fix/247-cover-art-orphans
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 3m35s
CI / e2e (pull_request) Successful in 11m41s
2026-09-20 20:59:14 -04:00
yonlu 1335572f0a Merge pull request 'fix(library): preserve playlist phantoms on incremental scan and removal' (#250) from fix/246-incremental-scan-phantoms into main
CI / check (push) Successful in 3m28s
CI / e2e (push) Successful in 12m53s
default
2026-09-21 00:41:05 +00:00
yonlu cf90030463 chore(bindings): regenerate for the queue's DropSourceForPlaylist
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 6m50s
CI / e2e (pull_request) Successful in 11m9s
frontend/bindings is generated by wails3 and is not covered by the
codegen pre-commit hook, so the new bound method went out without it and
make bindings-check failed on the PR.
2026-09-20 20:22:16 -04:00
yonlu 88f5524aa2 fix(maintenance): bound lyrics search and clicks, clear queue source
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Failing after 3m44s
CI / e2e (pull_request) Skipped
Three unbounded or stale surfaces, each small on its own:

- lyrics_index rows were never pruned on track removal, so the FTS index
  grew forever. Delete the entry where the library search FTS entry is
  already deleted, on the orphan and RemoveFromLibrary paths.
- search_clicks had no ceiling; age out ranking rows after a retention
  window via a daily janitor job.
- queue.source_* kept a "Playing from X" label after its playlist was
  deleted. Drop the source when the queue's own playlist goes, wired
  through a playlist-service hook like Library.SetRemovalHooks.

Closes #249
2026-09-11 17:14:41 -04:00
yonlu e745acf88a fix(maintenance): sweep artist_metadata rows nothing references
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 3m6s
CI / e2e (pull_request) Successful in 11m22s
artist_metadata was classified Cache/Swept but had no sweep and no
DELETE anywhere, so long-lived entity data (no TTL by design) grew for
the life of the install.  Sweep rows whose MBID is neither a library
artist nor holding cached artwork, and register the job with the
janitor.

Closes #248
2026-09-09 10:16:32 -04:00
yonlu 32bb64918c fix(library): sweep orphaned cover art when an album empties
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 3m19s
CI / e2e (pull_request) Successful in 10m53s
pruneEmptyEntities deleted empty albums but never the cover_art rows
they referenced, so removing the last track of an album leaked the row
and its files forever — the janitor's covers sweep computes its live set
from cover_art.file_path, which keeps the orphaned row's files exempt.

Extract sweepOrphanedCoverArt/removeCoverArtFiles as one implementation
and run it from pruneEmptyEntities (scan orphan path and
RemoveFromLibrary) and RemoveLibrary alike.

Closes #247
2026-09-09 10:14:07 -04:00
yonlu 1b9868ddd0 fix(library): preserve playlist phantoms on incremental scan and removal
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 3m33s
CI / e2e (pull_request) Successful in 11m12s
The incremental scan's orphan cleanup and RemoveFromLibrary deleted
audio_files rows without first filling the playlist phantom columns, so
a track removed from the library folder outside YellowJacket (or
removed from the library) became a permanently empty playlist row that
nothing could re-link — the same bug #183 fixed on the full-rescan and
retire paths, on the two paths it missed.

Add a scoped PreservePlaylistPhantomsForFiles and run it in the same
transaction as the deletes on both paths.

Closes #246
2026-09-09 10:09:17 -04:00
yonlu 6aeac42a46 Merge pull request 'fix(database): preserve playlist phantoms across a stale audio_files retire' (#245) from fix/183-phantom-across-retire into main
CI / check (push) Failing after 49s
CI / e2e (push) Skipped
Reviewed-on: #245
2026-09-09 13:54:36 +00:00
yonlu 68e7edb8c9 feat(database): listening-events log with skip counters
CI / check (push) Skipped
CI / e2e (push) Skipped
CI / check (pull_request) Successful in 5m23s
CI / e2e (pull_request) Successful in 11m9s
Replace play_history with listening_events — one row per track exit,
kind (complete/play/skip) plus raw position/duration — and add
skip_count/last_skipped to audio_files beside play_count/last_played.
The classifier that writes these lands later (plan 021); this is the
schema it records into.

Also drop the dead queue.source_playlist_id column and remove the stale
references to the squashed migration chain in download_*.sql and
tagging_items.sql, declaring the missing download-request indexes inline.
2026-09-09 09:19:36 -04:00
yonlu a3b5b43777 fix(database): preserve playlist phantoms across a stale audio_files retire
Retiring a stale audio_files dropped every playlist entry to an empty
row: ON DELETE SET NULL ran before the phantom_* columns were filled,
whereas the manual rescan path populates them first. Run the same
phantom population inside the retire transaction, before the drop, only
when audio_files is among the tables going, so
ResolvePhantomTracksAfterScan can re-link the entries.

Closes #183
2026-09-09 09:19:05 -04:00
logan 5fae61cdf1 Merge pull request 'fix(loop): document the model fallback chain and foreground launches' (#244) from fix/243-model-fallback into main
CI / check (push) Successful in 3m26s
CI / e2e (push) Successful in 11m7s
default
2026-09-04 03:37:29 +00:00
51 changed files with 3018 additions and 818 deletions
@@ -0,0 +1,263 @@
# 021 — Listening accounting: smart plays, skips, and a real history
**Issue:** none yet — open one before the first edit (tracker is the
source of truth; `./scripts/issue.sh search "skip play count"` comes
back empty as of this writing).
**Status:** plan — not started.
**Relates:** play-count rendering (`frontend/src/components/track-list/columns.ts`),
smart playlists (`backend/smartplaylist/`), the event contract
(`TrackPlayCountChanged`), and any future Wrapped / "minutes listened"
surface.
---
## What exists now
Three facts, all load-bearing.
**A "play" is recorded only on a natural finish.** `recordPlay`
(`backend/queue/playhistory.go:9`) is called from exactly one place —
`OnPlaybackFinished` (`backend/queue/handlers.go:14`), and only when
`srcErr == nil`. A track the user skips past at 90% is *not* a play;
neither is one they pause at 60% and abandon. `play_count` /
`last_played` on `audio_files` reflect "finished to the end," nothing
more.
**There is no skip concept at all.** Skipping is indistinguishable
from a natural finish, a pause, or a shutdown. Nothing records "the
user rejected this track," so no downstream feature (smart playlists,
shuffle, the revisit shelf, a future skip-rate heuristic) can ask
about it.
**`play_history` is a write-only log.** It holds
`(audio_file_id, played_at)` and nothing reads it — no sqlc query
touches it, no `PlayHistory` read path exists. Its only recorded
purpose is the timestamps a future "minutes listened over time"
feature would need. It is classified `Authored, Cascade` in
`backend/datamap/datamap.go:272` ("Listening history").
So the gaps are: (1) skips are invisible, and (2) "played" is
under-counted — the opposite of the usual over-counting fear. The
scrobble intuition (count a play once `min(50%, 4:00)` has been
*heard*, independent of how it ends) is the fix for both.
---
## What we're building
A single classification of every track *exit*, plus one row per exit in
a listening log, plus the existing denormalized `play_count` /
`last_played` updated to match the new meaning. Three exit kinds:
| kind | condition |
|---|---|
| `complete` | reached natural end, **or** abandoned with `remaining <= tail` |
| `play` | heard `>= playThreshold`, abandoned before the tail |
| `skip` | user moved to a *different* track before `playThreshold` |
Not counted, not any kind: decode failure, pause/stop/shutdown before
the threshold, and tracks shorter than `minTrackLength`.
### The thresholds — named judgements, one file
Follow the `PreviousRestartThreshold` precedent (`backend/queue/queue.go:28`,
a bare `const` with a comment). A new `backend/queue/listen.go` (or a
tiny `backend/listencount` package) declares:
```go
const (
// A track this short is deliberated jingle / interstitial and is
// never counted, either way.
minTrackLength = 30 * time.Second
// The scrobble rule: half the track, or four minutes, whichever
// comes first (Last.fm / ListenBrainz).
playThresholdMax = 4 * time.Minute
// "Finished enough": within 15s of the end, or the last 10%,
// whichever is larger. A 10:00 ambient track gets a 60s fade
// window; a 2:00 pop song gets 15s.
tailWindowFloor = 15 * time.Second
tailWindowFraction = 0.10
)
func playThreshold(d time.Duration) time.Duration {
return min(d/2, playThresholdMax)
}
func tailWindow(d time.Duration) time.Duration {
return max(d/10, tailWindowFloor)
}
```
Classification is a pure function of `(reason, position, duration)` and
*therefore unit-testable without a player*:
```go
func classify(reason ExitReason, pos, dur time.Duration) Kind
```
`ExitReason` is `finished | skipped | failed | abandoned`. `skipped`
means the queue moved to a different track by user action (Next,
Previous past the restart threshold, PlayIndex, queue replacement,
select-from-a-list). `failed` is the decode-error path. `abandoned` is
pause/stop/unload/shutdown — and in v1 is a no-op (see open question 3).
**"Heard" is approximated by the position at exit.** We read
`player.CurrentPositionSeconds()` at the moment of the transition, not
an accumulated listen-time ledger. A user who seeks to 80% and listens
5 seconds reads as "heard 80%." That is deliberately accepted for v1:
it is how most players actually behave, it is drastically simpler, and
the failure mode ("counted a track you skimmed as played") is mild and
exactly what the scrobble threshold already forgives. Written down
because "position is not listen time" is the one assumption that will
look like a bug if it is not.
**Fires once per listen.** Leaving a track already leaves it; the
`chainID` guard in `player.onPlaybackFinished` (`backend/player/player.go:633`)
already swallows a stale finish callback, and a transition advances
`currentIndex` past the finished track. The classifier needs the same
guard so a Next-then-stale-finish cannot produce two rows. Key it on the
`(audioFileID, chainID)` the transition was about.
---
## Schema — resolved: fresh design, no migration
The A/B migration agonizing is moot. This app has two users and both
are devs, and play counts are explicitly not worth preserving yet — so
the schema is written as if listening accounting had been designed in
from the start, and the existing two databases rebuild what they need
(see below). There is no migration step and none is re-introduced.
**`play_history` is renamed to `listening_events`** and grows the three
kinds, plus the raw position/duration the classification was made from:
```sql
CREATE TABLE IF NOT EXISTS listening_events (
id INTEGER PRIMARY KEY,
audio_file_id INTEGER NOT NULL,
kind TEXT NOT NULL DEFAULT 'complete'
CHECK (kind IN ('complete','play','skip')),
position_seconds INTEGER NOT NULL DEFAULT 0,
duration_seconds INTEGER NOT NULL DEFAULT 0,
occurred_at DATETIME NOT NULL DEFAULT (datetime('now')),
FOREIGN KEY(audio_file_id) REFERENCES audio_files(id) ON DELETE CASCADE
);
CREATE INDEX IF NOT EXISTS idx_listening_events_audio_file_id
ON listening_events(audio_file_id);
CREATE INDEX IF NOT EXISTS idx_listening_events_occurred_at
ON listening_events(occurred_at);
```
`position_seconds`/`duration_seconds` are kept raw so a future re-tune
of the threshold does not force the events to be re-recorded. `kind`
stays the write-time classification; the raw reading is evidence, not
a second copy of the rule.
**The counters are denormalized onto `audio_files`** — `skip_count` /
`last_skipped` join the existing `play_count` / `last_played`, because
that is where the hot read path already lives and a log join per track
row is not acceptable. This does grow the MIXED-KIND wart (see the
survey below for the structural answer), but it is the *continuation* of
the existing design, not a new leak: play counts sat on `audio_files`
from before this feature existed.
**What happens to the two real databases on next launch.**
`listening_events` is a new table, created verbatim. `audio_files`
gains two columns, which `retireStaleTables` treats as a stale Owned
table and rebuilds by rescan — dropping `play_count` / `last_played` /
`tag_status` with it, which is the accepted cost stated in the issue.
`play_history` is gone from the schema and the datamap, so
`obsoleteTables` drops it; its (natural-finish-only) timestamp rows go
with it. Nothing here is wrong on a fresh install, and on the two dev
machines the answer is the documented "delete and rescan."
---
## Wiring: where the classifier is called
The risk is not the classifier — it is that **every track-replacement
path must classify the outgoing track**, and there are many: `Next`,
`Previous` (past the 3s restart threshold), `PlayIndex`, `playFromStart`,
`SetQueue` / clear-and-play, remove-current, and select-from-a-list.
Miss one and that path silently never records a skip.
So the classification is centralized in one queue method —
```go
// leaveCurrent(reason) classifies the track at currentIndex as it is
// about to be replaced, and records exactly one listening event.
// Must be called without q.mu held (it writes to SQLite).
func (q *Queue) leaveCurrent(reason ExitReason)
```
— which reads position/duration from the player, calls `classify`, and
emits the play/skip row + `TrackPlayCountChanged` when `kind != skip`.
`OnPlaybackFinished(nil)` routes through `leaveCurrent(finished)`, the
navigation methods route through `leaveCurrent(skipped)` before they
advance, and `recordPlay` becomes the "did a play happen" half of it.
Because "one path forgot to call it" is the failure mode, a **source
sweep** pins it, on the pattern of `TestNoDirectRuntimeEmits`
(`backend/events/noemit_test.go`) and `TestCatalogCoversSchema`: a test
walks `backend/queue` for assignments to `currentIndex` (and the
`SetQueue` / remove paths) and fails if a mutation site does not sit
adjacent to a `leaveCurrent` call. The sweep is the enforcement; the
central method is the convenience.
`recordPlay` keeps its existing contract *when a play happens* —
`TrackPlayCountChanged` with `{audioFileId, filePath, playCount,
lastPlayed}` — so the frontend patch path and
`playhistory_test.go` keep passing. A skip emits no per-track event in
v1 (open question 4).
---
## Phases
1. **The classifier.** `listen.go`: the constants, `playThreshold`,
`tailWindow`, `classify`. Table-driven unit tests covering every
cell of the tristate, the <30s exemption, the tail window on both a
10:00 and a 2:00 track, and the clip at the 4:00 cap. No I/O.
2. **Schema.** *Done in this session.* `listening_events` replaces
`play_history`; `skip_count` / `last_skipped` added to
`audio_files`; datamap entry and `TestAuthoredCascadesAreDeliberate`
allow-list renamed; `recordPlay` writes `listening_events
('complete')`. `make generate` run; database / datamap / queue
tests green.
3. **Wiring.** `leaveCurrent`, the navigation/finish/error call sites,
the `fires once per listen` guard, and the source sweep. Extend
`playhistory_test.go` for skip/complete classification through the
queue rather than the pure function.
4. **Smart-playlist field.** `skip_count` (and optionally
`days_since_skipped`) in `smartplaylist.go` field/numeric maps and
the editor's field list, via subquery. A frontend event for skip —
if a UI wants a skip column — follows separately.
## Verification
- **Go:** the classifier is pure and exhaustively unit-tested; the
queue wiring is tested in-process with `events.WithSink`
(`backend/queue/emit_test.go` is the model), asserting a Next at 90%
emits a *play*, a Next at 10% emits a *skip and no play*, a natural
finish emits a *complete*.
- **Database:** schema + datamap tests fail-loud on any new or
reclassified table; `database_test.go`'s listening-events round-trip
asserts the new table and the four denormalized counter columns.
- **e2e:** `e2e/specs/play-count.spec.ts` already awaits
`TrackPlayCountChanged`; add the skip case (advance early, assert no
`TrackPlayCountChanged` and a `skip` row via the `__/test/sql`
endpoint if convenient, or via the playlist effect).
- No visual/component tier needed unless a skip column ships (phase 4).
## Open questions / decisions needed
1. **Migration mechanism.** Resolved — fresh design, no migration (see the schema section). Play counts are not worth preserving, both users are devs, and `audio_files` / `play_history` rebuild-or-drop on next launch.
2. **"Position is not listen time."** Accept the approximation for v1,
or track accumulated listen seconds (a real ledger on the player) now?
3. **Abandon on shutdown.** A track paused at 70% and then app-killed:
count a `play` (scrobble says heard) or leave it unrecorded? v1
proposes *unrecorded* — same as today — to keep the write path off
the shutdown critical path.
4. **Skip event to the frontend.** Emit now (parallel to
`TrackPlayCountChanged`) or only when a surface consumes it?
+6
View File
@@ -499,6 +499,10 @@ func (yj *YellowJacketApp) OnStartup(ctx context.Context) {
PostRemove: yj.explore.InvalidateLibrarySync,
})
// A deleted playlist must not leave the queue's "Playing from"
// label pointing at it.
yj.playlist.SetOnPlaylistDeleted(yj.queue.DropSourceForPlaylist)
// Register playback finished handler to drive queue auto-advance.
yj.player.SetPlaybackFinishedHandler(yj.queue.OnPlaybackFinished)
@@ -775,6 +779,8 @@ func (yj *YellowJacketApp) startJanitor() {
}
yj.janitor.Register(maintenance.ExpiredHTTPCacheJob(yj.database))
yj.janitor.Register(maintenance.StaleArtistMetadataJob(yj.database))
yj.janitor.Register(maintenance.StaleSearchClicksJob(yj.database))
yj.janitor.Register(maintenance.OrphanedCoverFilesJob(
yj.database, coversDir, library.CoverArtFileSet,
))
+28 -10
View File
@@ -662,19 +662,19 @@ func TestSmartPlaylistColumns(t *testing.T) {
}
// ---------------------------------------------------------------------------
// Migration 10 — play history tracking
// Listening events tracking
// ---------------------------------------------------------------------------
func TestPlayHistoryTable(t *testing.T) {
func TestListeningEventsTable(t *testing.T) {
t.Parallel()
db := NewTestDB(t)
// Verify play_history table exists.
// Verify listening_events table exists.
var tableCount int64
tblRows, err := db.QueryContext(
"SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name='play_history'",
"SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name='listening_events'",
)
if err != nil {
t.Fatalf("query sqlite_master: %v", err)
@@ -695,12 +695,14 @@ func TestPlayHistoryTable(t *testing.T) {
_ = tblRows.Close()
if tableCount != 1 {
t.Errorf("play_history table count = %d, want 1", tableCount)
t.Errorf("listening_events table count = %d, want 1", tableCount)
}
// Verify audio_files has play_count and last_played columns.
// Verify audio_files has the denormalized listening counters.
hasPlayCount := false
hasLastPlayed := false
hasSkipCount := false
hasLastSkipped := false
colRows, err := db.QueryContext("PRAGMA table_info(audio_files)")
if err != nil {
@@ -732,6 +734,14 @@ func TestPlayHistoryTable(t *testing.T) {
if name == "last_played" {
hasLastPlayed = true
}
if name == "skip_count" {
hasSkipCount = true
}
if name == "last_skipped" {
hasLastSkipped = true
}
}
_ = colRows.Close()
@@ -744,6 +754,14 @@ func TestPlayHistoryTable(t *testing.T) {
t.Error("audio_files missing last_played column")
}
if !hasSkipCount {
t.Error("audio_files missing skip_count column")
}
if !hasLastSkipped {
t.Error("audio_files missing last_skipped column")
}
// Verify track_metadata VIEW includes play_count and last_played.
viewCols := map[string]bool{}
@@ -783,7 +801,7 @@ func TestPlayHistoryTable(t *testing.T) {
t.Error("track_metadata VIEW missing last_played column")
}
// Round-trip: insert a play_history row and verify play_count update.
// Round-trip: insert a listening_events row and verify play_count update.
// First, set up test data. The test DB already has library id=0.
InsertTestTrack(t, db, TestTrack{
FilePath: "/test/play_history.mp3",
@@ -821,12 +839,12 @@ func TestPlayHistoryTable(t *testing.T) {
t.Errorf("initial play_count = %d, want 0", playCount)
}
// Insert a play_history row and update play_count.
// Insert a listening_events row (kind defaults to 'complete').
_, err = db.ExecContext(
"INSERT INTO play_history (audio_file_id) VALUES (1)",
"INSERT INTO listening_events (audio_file_id, kind) VALUES (1, 'complete')",
)
if err != nil {
t.Fatalf("insert play_history: %v", err)
t.Fatalf("insert listening_events: %v", err)
}
_, err = db.ExecContext(
+16 -4
View File
@@ -151,16 +151,28 @@ func (d *DB) SetLyrics(audioFileID int64, lyrics, source, recordingMBID string)
return d.upsertLyricsIndex(audioFileID, lyrics)
}
// upsertLyricsIndex refreshes a single file's entry in the contentless
// lyrics_index. contentless_delete=1 makes the DELETE valid; an empty
// lyrics string leaves the row deleted.
func (d *DB) upsertLyricsIndex(audioFileID int64, lyrics string) error {
// DeleteLyricsIndex removes one file's entry from the contentless
// lyrics_index. It is called wherever a file row is deleted — the
// `lyrics` table cascades with its file, but the FTS entry does not and
// would otherwise accumulate for the life of the install (#249).
func (d *DB) DeleteLyricsIndex(audioFileID int64) error {
if _, err := d.db.ExecContext(d.Ctx,
"DELETE FROM lyrics_index WHERE rowid = ?", audioFileID,
); err != nil {
return fmt.Errorf("could not delete lyrics_index row: %w", err)
}
return nil
}
// upsertLyricsIndex refreshes a single file's entry in the contentless
// lyrics_index. contentless_delete=1 makes the DELETE valid; an empty
// lyrics string leaves the row deleted.
func (d *DB) upsertLyricsIndex(audioFileID int64, lyrics string) error {
if err := d.DeleteLyricsIndex(audioFileID); err != nil {
return err
}
if strings.TrimSpace(lyrics) == "" {
return nil
}
+177
View File
@@ -0,0 +1,177 @@
package database
import (
"context"
"database/sql"
"fmt"
"log/slog"
"strings"
)
// Preserving a playlist entry across the loss of its track is two
// statements, not one, and the split is not tidiness -- it is what
// makes the important half work in the situation that needs it most.
//
// `playlist_tracks.audio_file_id` is ON DELETE SET NULL, so an entry
// outlives its file as an id-less row that says nothing about what the
// user put in the playlist. The phantom_* columns carry the answer
// across and ResolvePhantomTracksAfterScan re-links them afterwards --
// but only if something fills them *before* the rows go.
//
// The two halves are not equally important and are not equally
// available:
//
// - **phantom_file_path is the one that matters.**
// ResolvePhantomTracksAfterScan matches it against
// `audio_files.file_path`, so without it an entry can never be
// re-linked and the playlist is empty for good. It comes straight
// off `audio_files`, whose `file_path` is the table's natural key
// and has been present in every shape it has ever had -- including
// the pre-013 stub of `(id, file_path, recording_id)`.
// - The rest is *display* for a phantom entry before a rescan
// re-links it, and it comes from the `track_metadata` view, which
// is the one definition of a track row and not worth restating.
//
// Reading the view is what cannot be relied on here, and that is the
// whole reason for the split. This runs *before* applySchema, which is
// precisely the moment the schema is inconsistent: the view is whatever
// the last launch's schema declared, while `audio_files` is whatever
// the launch before that left behind. A view over columns the table no
// longer has is not merely empty -- `pragma_table_info` on it *errors*,
// and so does selecting from it. `cmd/indexbuild`'s fixture is exactly
// that shape and is what caught this.
//
// COALESCE keeps an existing phantom value in both halves: an entry
// already phantom is one whose file went missing in an earlier pass,
// and its recorded metadata is the only copy left. Overwriting that
// from a NULL join erases the rows this exists to protect.
const (
preservePhantomPathSQL = `
UPDATE playlist_tracks
SET phantom_file_path = COALESCE(phantom_file_path, (
SELECT af.file_path FROM audio_files af
WHERE af.id = playlist_tracks.audio_file_id
))
WHERE audio_file_id IS NOT NULL
`
preservePhantomDisplaySQL = `
UPDATE playlist_tracks
SET
phantom_title = COALESCE(phantom_title, (
SELECT tm.title FROM track_metadata tm
WHERE tm.id = playlist_tracks.audio_file_id
)),
phantom_artist = COALESCE(phantom_artist, (
SELECT tm.artist_name FROM track_metadata tm
WHERE tm.id = playlist_tracks.audio_file_id
)),
phantom_album = COALESCE(phantom_album, (
SELECT tm.album FROM track_metadata tm
WHERE tm.id = playlist_tracks.audio_file_id
)),
phantom_duration_ms = COALESCE(phantom_duration_ms, (
SELECT af.length_milliseconds FROM audio_files af
WHERE af.id = playlist_tracks.audio_file_id
)),
phantom_genre = COALESCE(phantom_genre, (
SELECT tm.genre FROM track_metadata tm
WHERE tm.id = playlist_tracks.audio_file_id
)),
phantom_cover_art_path = COALESCE(phantom_cover_art_path, (
SELECT tm.cover_art_path FROM track_metadata tm
WHERE tm.id = playlist_tracks.audio_file_id
))
WHERE audio_file_id IS NOT NULL
`
)
// PreservePlaylistPhantoms records every linked playlist entry's track
// metadata on the entry itself, so the entry survives the rows being
// deleted underneath it.
//
// Every path that empties `audio_files` must call this (or the scoped
// variant below) first, inside the same transaction as the delete.
// These paths have drifted before: the full rescan in backend/library
// did this and the stale-shape retire in this package did not, so the
// *documented* repair ("delete and rescan") preserved playlists while
// the automatic one that exists to spare the user that work silently
// emptied them (#183). The incremental scan's orphan cleanup and
// RemoveFromLibrary drifted the same way and are #246.
//
// The display half is skipped, with a warning, when `track_metadata`
// cannot answer -- see the note above. Skipping it costs a phantom
// entry its title until a rescan re-links it; skipping the path half
// would cost the entry outright, so that one is an error.
func PreservePlaylistPhantoms(
ctx context.Context, tx *sql.Tx, logger *slog.Logger,
) error {
return preservePlaylistPhantoms(ctx, tx, nil, logger)
}
// PreservePlaylistPhantomsForFiles is PreservePlaylistPhantoms scoped to
// the given audio file ids, for the two removal paths that delete a
// known subset of the table rather than all of it: the incremental
// scan's orphan cleanup and RemoveFromLibrary. A bulk pass there would
// rewrite every linked playlist row on every scan for nothing.
func PreservePlaylistPhantomsForFiles(
ctx context.Context, tx *sql.Tx, ids []int64, logger *slog.Logger,
) error {
return preservePlaylistPhantoms(ctx, tx, ids, logger)
}
func preservePlaylistPhantoms(
ctx context.Context, tx *sql.Tx, ids []int64, logger *slog.Logger,
) error {
clause, args, skip := phantomIDFilter(ids)
if skip {
return nil
}
if _, err := tx.ExecContext(
ctx, preservePhantomPathSQL+clause, args...,
); err != nil {
return fmt.Errorf(
"could not preserve playlist track file paths: %w", err,
)
}
if _, err := tx.ExecContext(
ctx, preservePhantomDisplaySQL+clause, args...,
); err != nil {
// A failed statement does not roll back a SQLite transaction,
// so the path half above stands and the entries remain
// re-linkable.
logger.Warn(
"could not record display metadata for playlist entries; "+
"they will be re-linked by the next scan but read as "+
"unknown until then",
"err", err,
)
}
return nil
}
// phantomIDFilter builds the extra WHERE terms and arguments that scope
// a preservation pass to a set of audio file ids. A nil ids returns the
// empty clause (a bulk run over every linked entry); an empty slice
// reports skip, since there is nothing to preserve.
func phantomIDFilter(ids []int64) (clause string, args []any, skip bool) {
switch {
case ids == nil:
return "", nil, false
case len(ids) == 0:
return "", nil, true
}
clause = " AND audio_file_id IN (" +
strings.Repeat("?,", len(ids)-1) + "?)"
args = make([]any, len(ids))
for i, id := range ids {
args[i] = id
}
return clause, args, false
}
+94
View File
@@ -0,0 +1,94 @@
package database
import (
"context"
"database/sql"
"testing"
)
// TestPreservePlaylistPhantomsForFilesScopesToTheRequestedIDs is the
// scoping half of the scoped variant: a run over one file's id must
// fill that file's playlist entries and leave every other entry alone,
// because the incremental scan calls this once per orphan batch and a
// pass that rewrote the whole table would touch every playlist row on
// every scan.
func TestPreservePlaylistPhantomsForFilesScopesToTheRequestedIDs(t *testing.T) {
ctx := context.Background()
db := openRaw(t, t.TempDir())
if _, err := db.ExecContext(ctx, "PRAGMA foreign_keys = ON"); err != nil {
t.Fatalf("pragma: %v", err)
}
if err := applySchema(ctx, db); err != nil {
t.Fatalf("applySchema: %v", err)
}
if _, err := db.ExecContext(ctx, `
INSERT INTO playlists (id, name) VALUES (1, 'keepme');
INSERT INTO libraries (id, name, path) VALUES (0, 'test', '/music');
INSERT INTO artists (id, name) VALUES (3, 'Aurora Fields');
INSERT INTO cover_art (id, file_path, mime_type)
VALUES (9, 'covers/7.jpg', 'image/jpeg');
INSERT INTO albums (id, name, artist_id, cover_art_id)
VALUES (4, 'Tideline', 3, 9);
INSERT INTO audio_files
(id, file_path, file_type_id, length_milliseconds,
title, artist_credit, artist_id, album_id)
VALUES
(7, '/music/a.flac', 1, 1000,
'Slack Water', 'Aurora Fields', 3, 4),
(8, '/music/b.flac', 1, 2000,
'Second Tide', 'Aurora Fields', 3, 4);
INSERT INTO playlist_tracks (playlist_id, audio_file_id, position)
VALUES (1, 7, 0), (1, 8, 1);
`); err != nil {
t.Fatalf("seed: %v", err)
}
tx, err := db.BeginTx(ctx, nil)
if err != nil {
t.Fatalf("begin: %v", err)
}
defer func() { _ = tx.Rollback() }()
if err := PreservePlaylistPhantomsForFiles(
ctx, tx, []int64{7}, testLogger(),
); err != nil {
t.Fatalf("preserve: %v", err)
}
if err := tx.Commit(); err != nil {
t.Fatalf("commit: %v", err)
}
var filled, untouched sql.NullString
if err := db.QueryRowContext(ctx,
"SELECT phantom_file_path FROM playlist_tracks WHERE audio_file_id = 7",
).Scan(&filled); err != nil {
t.Fatalf("read the requested entry: %v", err)
}
if filled.String != "/music/a.flac" {
t.Errorf(
"requested entry phantom_file_path = %q, want %q",
filled.String, "/music/a.flac",
)
}
if err := db.QueryRowContext(ctx,
"SELECT phantom_file_path FROM playlist_tracks WHERE audio_file_id = 8",
).Scan(&untouched); err != nil {
t.Fatalf("read the untouched entry: %v", err)
}
if untouched.Valid {
t.Errorf(
"untouched entry got phantom_file_path = %q, want NULL "+
"(a scoped run must not rewrite the whole table)",
untouched.String,
)
}
}
@@ -68,8 +68,14 @@ CREATE TABLE IF NOT EXISTS audio_files (
-- compared against the on-disk mtime during a scan to detect files
-- another application retagged in place.
modified_at INTEGER NOT NULL DEFAULT 0,
-- Listening counts, denormalized from listening_events so the hot
-- read path (track list sort, shelves, smart playlists) never joins
-- a log table. Authored: a rescan cannot rebuild them. This is the
-- "MIXED KIND" half of audio_files the datamap notes.
play_count INTEGER NOT NULL DEFAULT 0,
last_played DATETIME,
skip_count INTEGER NOT NULL DEFAULT 0,
last_skipped DATETIME,
tag_status TEXT NOT NULL DEFAULT 'untagged'
CHECK(tag_status IN (
'untagged', 'auto_matched', 'user_confirmed', 'user_skipped_permanent'
@@ -45,8 +45,6 @@ CREATE INDEX IF NOT EXISTS idx_download_items_live
CREATE INDEX IF NOT EXISTS idx_download_items_state
ON download_items(state);
-- idx_download_items_download is deliberately NOT declared here: on an
-- existing database this table already exists at schema-pass time with
-- its old column still named request_id, so an inline CREATE INDEX on
-- download_id would fail outright. See ensureDownloadIndexes in
-- backend/database/download_rename_migration.go.
-- ListDownloadItemsForDownload filters on the parent download.
CREATE INDEX IF NOT EXISTS idx_download_items_download
ON download_items(download_id);
@@ -66,14 +66,11 @@ CREATE TABLE IF NOT EXISTS download_requests (
FOREIGN KEY(parent_id) REFERENCES download_requests(id) ON DELETE CASCADE
);
-- idx_download_requests_{due,entity,parent} are deliberately NOT
-- declared here. This table name is reused from the old one-shot
-- attempt table (also called download_requests before the Want/Request
-- rename), so on an existing database this CREATE TABLE is a no-op
-- against a table that, at schema-pass time, is still shaped like the
-- OLD attempts table and lacks these columns entirely — an inline
-- CREATE INDEX here would fail outright rather than just no-op. See
-- migrateDownloadRename/ensureDownloadIndexes in
-- backend/database/download_rename_migration.go, which create these
-- once the rename has actually happened (or immediately, on a fresh
-- database where the columns exist from the start).
CREATE INDEX IF NOT EXISTS idx_download_requests_due
ON download_requests(state, next_try_at);
CREATE INDEX IF NOT EXISTS idx_download_requests_entity
ON download_requests(entity, state);
CREATE INDEX IF NOT EXISTS idx_download_requests_parent
ON download_requests(parent_id) WHERE parent_id IS NOT NULL;
@@ -0,0 +1,42 @@
-- One row per track *exit*, three ways a listen can end: it reached
-- the end, it was heard enough to count and then skipped past, or it
-- was abandoned for another track before anyone had really listened.
--
-- This is the source of truth for listening behaviour. The
-- denormalized `play_count` / `last_played` / `skip_count` /
-- `last_skipped` on audio_files are materialized from it, because the
-- hot read path (track-list sort, the shelves, smart playlists) must
-- not join a log that grows by one row per song forever.
--
-- `kind` is the classification, applied at write time:
--
-- complete the track reached its natural end, or was skipped in
-- its tail window (the last few seconds of a long fade).
-- play the scrobble threshold was heard — half the track or
-- four minutes, whichever is less — and the user moved on
-- before the end.
-- skip the user moved to a different track before that.
--
-- `position_seconds` / `duration_seconds` are the raw reading the
-- classification was made from, kept so a future re-tune of the
-- threshold does not need the events re-recorded. 0/0 on a row means
-- "not captured for this event" (e.g. a natural finish recorded before
-- these columns existed), not "a zero-second track".
CREATE TABLE IF NOT EXISTS listening_events (
id INTEGER PRIMARY KEY,
audio_file_id INTEGER NOT NULL,
kind TEXT NOT NULL DEFAULT 'complete'
CHECK (kind IN ('complete', 'play', 'skip')),
position_seconds INTEGER NOT NULL DEFAULT 0,
duration_seconds INTEGER NOT NULL DEFAULT 0,
occurred_at DATETIME NOT NULL DEFAULT (datetime('now')),
FOREIGN KEY(audio_file_id) REFERENCES audio_files(id) ON DELETE CASCADE
);
CREATE INDEX IF NOT EXISTS idx_listening_events_audio_file_id
ON listening_events(audio_file_id);
-- "What did I listen to this month" walks this, rather than the
-- per-track index above.
CREATE INDEX IF NOT EXISTS idx_listening_events_occurred_at
ON listening_events(occurred_at);
@@ -1,9 +0,0 @@
CREATE TABLE IF NOT EXISTS play_history (
id INTEGER PRIMARY KEY,
audio_file_id INTEGER NOT NULL,
played_at DATETIME NOT NULL DEFAULT (datetime('now')),
FOREIGN KEY(audio_file_id) REFERENCES audio_files(id) ON DELETE CASCADE
);
CREATE INDEX IF NOT EXISTS idx_play_history_audio_file_id
ON play_history(audio_file_id);
+4 -7
View File
@@ -1,18 +1,15 @@
CREATE TABLE IF NOT EXISTS queue (
id INTEGER PRIMARY KEY CHECK(id = 1),
source_playlist_id INTEGER,
current_position INTEGER NOT NULL DEFAULT 0,
shuffle_mode BOOLEAN NOT NULL DEFAULT false,
repeat_mode TEXT NOT NULL DEFAULT 'off',
shuffle_order TEXT,
-- source_playlist_id above is unused dead weight (nothing has ever
-- written it a nonzero value); source_type/source_id/source_label
-- below are its generalized replacement, covering albums, playlists,
-- smart playlists, genres and artists rather than playlists alone.
-- What the queue was built from ("Playing from: X"): an album,
-- playlist, smart playlist, genre or artist, identified by the id
-- that source_type's namespace gives it.
source_type TEXT NOT NULL DEFAULT '',
source_id INTEGER NOT NULL DEFAULT 0,
source_label TEXT NOT NULL DEFAULT '',
FOREIGN KEY(source_playlist_id) REFERENCES playlists(id) ON DELETE SET NULL
source_label TEXT NOT NULL DEFAULT ''
);
-- Singleton row: there is exactly one playback queue.
+4 -18
View File
@@ -26,17 +26,10 @@ CREATE TABLE IF NOT EXISTS tagging_items (
-- complete rip of their own directory. parent_group_key is the
-- original folder group they were split from.
--
-- These two columns are declared LAST, after created_at, even
-- though that reads oddly next to the rest of the table: sql/
-- migrations/0001 brings a pre-existing tagging_items up to date
-- with `ALTER TABLE ADD COLUMN`, which SQLite always appends at
-- the end of the column list. A fresh install (this file) and an
-- upgraded database (this file + the migration) must end up with
-- IDENTICAL column order, because sqlc-generated `SELECT *` scans
-- (e.g. GetTaggingItem) bind columns positionally — see the
-- schema/migration column-order test in database_test.go. Put
-- new columns wherever reads best when adding a table for the
-- first time; append-only from the second migration on.
-- These columns are appended after created_at rather than grouped
-- with the rest of the row: sqlc's `SELECT *` scans (GetTaggingItem)
-- bind column order positionally, so new columns always go at the
-- end.
synthetic INTEGER NOT NULL DEFAULT 0,
parent_group_key TEXT NOT NULL DEFAULT '',
-- album_artist_conflict latches to 1 the first time two tracks
@@ -58,10 +51,3 @@ CREATE INDEX IF NOT EXISTS idx_tagging_items_library_status
CREATE INDEX IF NOT EXISTS idx_tagging_items_status_pending
ON tagging_items(library_id) WHERE status = 'pending';
-- idx_tagging_items_parent_group_key is NOT declared here on
-- purpose: this file runs unconditionally, before migrations, even
-- against a database that hasn't run 0001 yet — an index predicate
-- referencing parent_group_key would fail on that table. It lives
-- solely in sql/migrations/0001_tagging_items_synthetic.sql, which
-- runs after the column exists either way (see database.go).
@@ -39,7 +39,7 @@ INSERT INTO audio_files (
?, ?, ?, ?, ?, ?,
?, ?, ?, ?, ?
)
RETURNING id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, tag_status
RETURNING id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, skip_count, last_skipped, tag_status
`
type CreateAudioFileParams struct {
@@ -135,6 +135,8 @@ func (q *Queries) CreateAudioFile(ctx context.Context, arg CreateAudioFileParams
&i.ModifiedAt,
&i.PlayCount,
&i.LastPlayed,
&i.SkipCount,
&i.LastSkipped,
&i.TagStatus,
)
return i, err
@@ -192,7 +194,7 @@ func (q *Queries) GetAllAudioFilePaths(ctx context.Context) ([]GetAllAudioFilePa
const getAudioFile = `-- name: GetAudioFile :one
SELECT id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, tag_status FROM audio_files WHERE id = ? LIMIT 1
SELECT id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, skip_count, last_skipped, tag_status FROM audio_files WHERE id = ? LIMIT 1
`
// ---------------------------------------------------------------------
@@ -228,13 +230,15 @@ func (q *Queries) GetAudioFile(ctx context.Context, id int64) (AudioFile, error)
&i.ModifiedAt,
&i.PlayCount,
&i.LastPlayed,
&i.SkipCount,
&i.LastSkipped,
&i.TagStatus,
)
return i, err
}
const getAudioFileByPath = `-- name: GetAudioFileByPath :one
SELECT id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, tag_status FROM audio_files WHERE file_path = ? LIMIT 1
SELECT id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, skip_count, last_skipped, tag_status FROM audio_files WHERE file_path = ? LIMIT 1
`
func (q *Queries) GetAudioFileByPath(ctx context.Context, filePath string) (AudioFile, error) {
@@ -267,6 +271,8 @@ func (q *Queries) GetAudioFileByPath(ctx context.Context, filePath string) (Audi
&i.ModifiedAt,
&i.PlayCount,
&i.LastPlayed,
&i.SkipCount,
&i.LastSkipped,
&i.TagStatus,
)
return i, err
@@ -334,7 +340,7 @@ func (q *Queries) GetAudioFilesByPaths(ctx context.Context, paths []string) ([]G
}
const getAudioFilesInLibrary = `-- name: GetAudioFilesInLibrary :many
SELECT id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, tag_status FROM audio_files WHERE library_id = ?
SELECT id, file_path, library_id, file_type_id, length_milliseconds, sample_rate, bit_depth, channels, bitrate, file_size, title, artist_credit, artist_id, album_id, track_number, disc_number, total_tracks, year, composer, comment, recording_mbid, basename, group_key, modified_at, play_count, last_played, skip_count, last_skipped, tag_status FROM audio_files WHERE library_id = ?
`
func (q *Queries) GetAudioFilesInLibrary(ctx context.Context, libraryID int64) ([]AudioFile, error) {
@@ -373,6 +379,8 @@ func (q *Queries) GetAudioFilesInLibrary(ctx context.Context, libraryID int64) (
&i.ModifiedAt,
&i.PlayCount,
&i.LastPlayed,
&i.SkipCount,
&i.LastSkipped,
&i.TagStatus,
); err != nil {
return nil, err
+11 -7
View File
@@ -94,6 +94,8 @@ type AudioFile struct {
ModifiedAt int64
PlayCount int64
LastPlayed sql.NullTime
SkipCount int64
LastSkipped sql.NullTime
TagStatus string
}
@@ -261,6 +263,15 @@ type Library struct {
AutotagWarningAcked int64
}
type ListeningEvent struct {
ID int64
AudioFileID int64
Kind string
PositionSeconds int64
DurationSeconds int64
OccurredAt time.Time
}
type Lyric struct {
AudioFileID int64
Text string
@@ -273,12 +284,6 @@ type LyricsIndex struct {
Lyrics string
}
type PlayHistory struct {
ID int64
AudioFileID int64
PlayedAt time.Time
}
type PlayerState struct {
ID int64
Volume int64
@@ -313,7 +318,6 @@ type PlaylistTrack struct {
type Queue struct {
ID int64
SourcePlaylistID sql.NullInt64
CurrentPosition int64
ShuffleMode bool
RepeatMode string
+55
View File
@@ -245,6 +245,13 @@ func dropDeferred(
ctx context.Context, db *sql.DB, logger *slog.Logger,
drop map[string]string,
) error {
// Asked before the transaction opens, because the answer is about
// which tables are live and that cannot change underneath us here.
preserve, err := shouldPreservePhantoms(ctx, db, drop)
if err != nil {
return err
}
tx, err := db.BeginTx(ctx, nil)
if err != nil {
return fmt.Errorf("could not begin the retire transaction: %w", err)
@@ -256,6 +263,22 @@ func dropDeferred(
return fmt.Errorf("could not defer foreign keys: %w", err)
}
// Before any drop, so every entry still has a track to read. It is
// in this transaction rather than beside it because the preservation
// and the delete have to succeed or fail together: a commit that
// dropped the files without the phantoms is the bug, and a commit
// that wrote phantoms without dropping anything is a lie about rows
// that are still there.
if preserve {
logger.Info(
"preserving playlist entries across the retire of audio_files",
)
if err := PreservePlaylistPhantoms(ctx, tx, logger); err != nil {
return err
}
}
// Sorted, so a failure is reproducible. Map order is random, and a
// bug that depends on which table happens to go first reproduces on
// one run in three and passes review on the other two -- which is
@@ -283,6 +306,38 @@ func dropDeferred(
return nil
}
// shouldPreservePhantoms reports whether this retire is about to take
// `audio_files` out from under the playlists.
//
// The `playlist_tracks` check is not defensive padding. This runs
// *before* applySchema, which is the moment the schema is by definition
// mid-repair, and the preservation reads a table it does not drop. A
// database old enough not to have it would otherwise fail here, and
// failing here means the app does not open at all -- while nothing is
// lost by skipping, since an absent `playlist_tracks` holds no
// playlists to save.
//
// It deliberately does *not* ask after `track_metadata`. Whether that
// view can answer is PreservePlaylistPhantoms's own business, because a
// view broken against an older `audio_files` is a state this function
// cannot detect without hitting the same error it is trying to avoid:
// pragma_table_info on such a view errors rather than reporting no
// columns.
func shouldPreservePhantoms(
ctx context.Context, db *sql.DB, drop map[string]string,
) (bool, error) {
if _, going := drop["audio_files"]; !going {
return false, nil
}
cols, err := liveColumns(ctx, db, "playlist_tracks")
if err != nil {
return false, err
}
return len(cols) > 0, nil
}
// staleReason reports why a live table disagrees with its declaration,
// or "" when it agrees. A column the live table does not have is the
// additive case; a column whose declared type changed is the one an
+177
View File
@@ -497,3 +497,180 @@ func TestParseCreateTablesReadsTheRealSchema(t *testing.T) {
}
}
}
// TestRetiringAudioFilesKeepsPlaylistContents is the symptom this
// repair exists for: a playlist survived the retire as a row count and
// nothing else.
//
// TestRetiringOwnedTablesDoesNotDangle already asserts the entry does
// not keep a stale id, which is the *dangerous* half. It is satisfied
// just as well by an entry that says nothing at all, which is the
// half that quietly emptied every playlist -- so this asserts what the
// entry still knows, and specifically phantom_file_path, because that
// is the column ResolvePhantomTracksAfterScan matches back against
// audio_files.file_path.
//
// Note the seed drops `comment`, not `artist_credit`: the mutation has
// to leave `track_metadata` standing, since a real launch reaches the
// retire with the view the previous launch created. A test that drops
// the view first is testing the skip path, not this one.
func TestRetiringAudioFilesKeepsPlaylistContents(t *testing.T) {
ctx := context.Background()
db := openRaw(t, t.TempDir())
if _, err := db.ExecContext(ctx, "PRAGMA foreign_keys = ON"); err != nil {
t.Fatalf("pragma: %v", err)
}
if err := applySchema(ctx, db); err != nil {
t.Fatalf("applySchema: %v", err)
}
if _, err := db.ExecContext(ctx, `
INSERT INTO playlists (id, name) VALUES (1, 'keepme');
INSERT INTO libraries (id, name, path) VALUES (0, 'test', '/music');
INSERT INTO artists (id, name) VALUES (3, 'Aurora Fields');
INSERT INTO cover_art (id, file_path, mime_type)
VALUES (9, 'covers/7.jpg', 'image/jpeg');
INSERT INTO genres (id, name) VALUES (5, 'Ambient');
INSERT INTO albums (id, name, artist_id, cover_art_id)
VALUES (4, 'Tideline', 3, 9);
INSERT INTO audio_files
(id, file_path, file_type_id, length_milliseconds,
title, artist_credit, artist_id, album_id)
VALUES (7, '/music/a.flac', 1, 1000,
'Slack Water', 'Aurora Fields', 3, 4);
INSERT INTO file_genres (audio_file_id, genre_id) VALUES (7, 5);
INSERT INTO playlist_tracks (playlist_id, audio_file_id, position)
VALUES (1, 7, 0);
ALTER TABLE audio_files DROP COLUMN comment;
`); err != nil {
t.Fatalf("seed: %v", err)
}
if err := retireStaleTables(ctx, db, testLogger()); err != nil {
t.Fatalf("retire: %v", err)
}
if err := applySchema(ctx, db); err != nil {
t.Fatalf("applySchema: %v", err)
}
var (
path, title, artist, album, genre, cover sql.NullString
duration sql.NullInt64
)
if err := db.QueryRowContext(ctx, `
SELECT phantom_file_path, phantom_title, phantom_artist,
phantom_album, phantom_duration_ms, phantom_genre,
phantom_cover_art_path
FROM playlist_tracks WHERE playlist_id = 1
`).Scan(&path, &title, &artist, &album, &duration, &genre, &cover); err != nil {
t.Fatalf("read the surviving entry: %v", err)
}
// The one that matters: without it the entry can never be re-linked
// by the rescan the retire itself provokes.
if path.String != "/music/a.flac" {
t.Fatalf(
"phantom_file_path is %q, want %q -- the playlist entry "+
"cannot be re-linked and the playlist is empty for good",
path.String, "/music/a.flac",
)
}
if title.String != "Slack Water" {
t.Errorf("phantom_title is %q, want %q", title.String, "Slack Water")
}
if artist.String != "Aurora Fields" {
t.Errorf("phantom_artist is %q, want %q", artist.String, "Aurora Fields")
}
if album.String != "Tideline" {
t.Errorf("phantom_album is %q, want %q", album.String, "Tideline")
}
if duration.Int64 != 1000 {
t.Errorf("phantom_duration_ms is %d, want 1000", duration.Int64)
}
if genre.String != "Ambient" {
t.Errorf("phantom_genre is %q, want %q", genre.String, "Ambient")
}
if cover.String != "covers/7.jpg" {
t.Errorf("phantom_cover_art_path is %q, want %q", cover.String, "covers/7.jpg")
}
}
// TestRetiringAudioFilesKeepsPathsWhenTheViewCannotAnswer is the case
// that broke cmd/indexbuild: this repair runs *before* applySchema, so
// `track_metadata` is whatever the last launch declared while
// `audio_files` is whatever the launch before that left behind, and a
// view over columns the table no longer has does not read as empty --
// it errors.
//
// The pre-013 stub shape below is the real one that fixture carries.
// What must survive is phantom_file_path, because `file_path` is the
// table's natural key and has been in every shape it ever had; the
// display columns are allowed to be absent, and the open must not fail.
func TestRetiringAudioFilesKeepsPathsWhenTheViewCannotAnswer(t *testing.T) {
ctx := context.Background()
db := openRaw(t, t.TempDir())
if _, err := db.ExecContext(ctx, "PRAGMA foreign_keys = ON"); err != nil {
t.Fatalf("pragma: %v", err)
}
if err := applySchema(ctx, db); err != nil {
t.Fatalf("applySchema: %v", err)
}
// The rows go in *after* the reshape: dropping audio_files with
// foreign keys on would fire the ON DELETE SET NULL and null the
// entry this test is about, which would pass for the wrong reason.
if _, err := db.ExecContext(ctx, `
DROP TABLE audio_files;
CREATE TABLE audio_files (
id INTEGER PRIMARY KEY,
file_path TEXT NOT NULL UNIQUE,
recording_id INTEGER
);
INSERT INTO playlists (id, name) VALUES (1, 'keepme');
INSERT INTO audio_files (id, file_path) VALUES (7, '/music/a.flac');
INSERT INTO playlist_tracks (playlist_id, audio_file_id, position)
VALUES (1, 7, 0);
`); err != nil {
t.Fatalf("seed: %v", err)
}
// The symptom this guards: the repair must not turn a recoverable
// database into one the app refuses to open.
if err := retireStaleTables(ctx, db, testLogger()); err != nil {
t.Fatalf(
"the retire failed on a view it could not read, so the app "+
"would not open at all: %v", err,
)
}
if err := applySchema(ctx, db); err != nil {
t.Fatalf("applySchema: %v", err)
}
var path sql.NullString
if err := db.QueryRowContext(ctx,
"SELECT phantom_file_path FROM playlist_tracks WHERE playlist_id = 1",
).Scan(&path); err != nil {
t.Fatalf("read the surviving entry: %v", err)
}
if path.String != "/music/a.flac" {
t.Fatalf(
"phantom_file_path is %q, want %q -- the display half being "+
"unavailable must not cost the entry its one re-link key",
path.String, "/music/a.flac",
)
}
}
+5 -4
View File
@@ -269,10 +269,11 @@ var tables = []Table{
"from owned files plus the LRCLIB backfill.",
},
{
Name: "play_history", Kind: Authored, Lifetime: Cascade,
Note: "Listening history. Authored, but intentionally cascades " +
"with its track — history for a file no longer in the library " +
"has nothing to point at.",
Name: "listening_events", Kind: Authored, Lifetime: Cascade,
Note: "Listening history, one row per track exit (complete, play " +
"or skip). Authored, but intentionally cascades with its " +
"track — history for a file no longer in the library has " +
"nothing to point at.",
},
{
Name: "player_state", Kind: Authored, Lifetime: Retained,
+2 -2
View File
@@ -212,13 +212,13 @@ func TestLifetimesMatchSchema(t *testing.T) {
// Authored data is unrecoverable, so it must never be removed as a side
// effect of deleting owned data. Cascade is allowed only where the
// catalog explains why (play_history, queue_tracks); this test pins the
// catalog explains why (listening_events, queue_tracks); this test pins the
// set so a new cascade onto authored data is a deliberate decision.
func TestAuthoredCascadesAreDeliberate(t *testing.T) {
t.Parallel()
allowed := map[string]bool{
"play_history": true,
"listening_events": true,
"queue_tracks": true,
// Download history is scoped to the library it imported into.
+11 -5
View File
@@ -383,9 +383,13 @@ func (si *SearchIndex) topByPopularity(
// MusicBrainz IDs, so this never touches the library tables and asks
// one query rather than one per artist.
//
// The artists are drawn most-popular-owned-album first, so a large
// library's pool is the part of it the user is likeliest to recognise
// rather than whichever artists sort first.
// **The row is drawn at random, and that is the whole point of it.**
// Ordered by popularity it was a second leaderboard: the same handful of
// big names appeared every time the page opened, which is not what "you
// own one album by these artists" is saying. The pool is still bounded
// to `pool` artists — a 4 000-artist library does not need all of them
// ranked — but which of them, and which of their albums, is `RANDOM()`,
// so the shelf is a different sample each visit.
func (si *SearchIndex) unownedAlbumsBySinglyOwnedArtists(
ctx context.Context,
pool, limit int,
@@ -396,15 +400,17 @@ func (si *SearchIndex) unownedAlbumsBySinglyOwnedArtists(
WHERE entity_type = 2 /* release_group */
AND in_library = 0
AND artist_mbid IN (
SELECT artist_mbid FROM (
SELECT artist_mbid FROM explore_index
WHERE entity_type = 2 /* release_group */
AND in_library = 1
AND artist_mbid != x''
GROUP BY artist_mbid
HAVING COUNT(*) = 1
ORDER BY MAX(popularity) DESC
)
ORDER BY RANDOM()
LIMIT ?)
ORDER BY popularity DESC
ORDER BY RANDOM()
LIMIT ?`,
pool, limit,
))
+6
View File
@@ -3,6 +3,7 @@ package explore
import (
"context"
"log/slog"
"sort"
"testing"
"yellowjacket/backend/database"
@@ -202,6 +203,11 @@ func TestShelves_MoreFromOwnedNeedsExactlyOneOwnedAlbum(t *testing.T) {
titles = append(titles, album.Title)
}
// The row is a random sample, so the *set* is what is asserted and
// not the order — see `unownedAlbumsBySinglyOwnedArtists` for why
// the ordering was given up.
sort.Strings(titles)
if len(titles) != 2 || titles[0] != "Second" || titles[1] != "Third" {
t.Fatalf("albums = %v, want [Second Third]", titles)
}
+72
View File
@@ -3,6 +3,7 @@ package library
import (
"bytes"
"crypto/sha256"
"database/sql"
"encoding/hex"
"fmt"
"image"
@@ -69,6 +70,77 @@ func CoverArtFileSet(coverPath string) []string {
return paths
}
// sweepOrphanedCoverArt deletes the cover_art rows no album references
// and returns their file paths, for the caller to remove from disk
// after the transaction commits. Cover art is referenced only by
// albums.cover_art_id, so an orphan is a cover whose album is gone —
// which is every album the caller just swept.
//
// One implementation because the scan path, RemoveFromLibrary and
// RemoveLibrary all reach this state, and the scan side used to skip it
// entirely while RemoveLibrary did it inline (#247).
func (l *Library) sweepOrphanedCoverArt(tx *sql.Tx) ([]string, error) {
const orphanSQL = `
SELECT file_path FROM cover_art WHERE id NOT IN (
SELECT DISTINCT cover_art_id FROM albums
WHERE cover_art_id IS NOT NULL
)`
rows, err := tx.QueryContext(l.ctx, orphanSQL)
if err != nil {
return nil, fmt.Errorf("could not query orphaned cover art: %w", err)
}
var paths []string
for rows.Next() {
var filePath string
if err := rows.Scan(&filePath); err != nil {
l.logger.Warn("could not scan cover art path", "err", err)
continue
}
paths = append(paths, filePath)
}
// Close before the DELETE: the two run on the one writer connection.
if err := rows.Close(); err != nil {
l.logger.Warn("could not close cover art rows", "err", err)
}
if len(paths) > 0 {
if _, err := tx.ExecContext(l.ctx, `
DELETE FROM cover_art WHERE id NOT IN (
SELECT DISTINCT cover_art_id FROM albums
WHERE cover_art_id IS NOT NULL
)`); err != nil {
return nil, fmt.Errorf("could not delete orphaned cover_art: %w", err)
}
}
return paths, nil
}
// removeCoverArtFiles removes a cover original and its derived size
// variants. Only the original is stored in cover_art.file_path; the
// _sm/_md/_lg tiers are derived filenames beside it, so they have to be
// removed by name or they accumulate forever.
func (l *Library) removeCoverArtFiles(coverPaths []string) {
for _, coverPath := range coverPaths {
for _, path := range CoverArtFileSet(coverPath) {
if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
l.logger.Warn(
"could not remove orphaned cover art file",
"path", path,
"err", err,
)
}
}
}
}
// saveCoverArt saves embedded cover art to the cache directory.
// Returns the file path where the art was saved, or empty string
// if no picture data. Timing is recorded in the provided metrics.
+8 -50
View File
@@ -320,43 +320,12 @@ func (l *Library) RemoveLibrary(id int64) (*RemovalSummary, error) {
genresRemoved, _ := result.RowsAffected()
// 15. Collect orphaned cover_art file paths for post-commit cleanup.
// SAFETY: Hand-crafted SELECT for orphaned cover art identification.
// Parameterless.
rows, err := tx.QueryContext(l.ctx,
`SELECT file_path FROM cover_art WHERE id NOT IN (
SELECT DISTINCT cover_art_id FROM albums
WHERE cover_art_id IS NOT NULL
)`)
// Collect and delete orphaned cover_art rows before the commit. The
// shared helper is the one place this sweep lives, so the scan path,
// RemoveFromLibrary and this removal cannot drift (#247).
orphanedCoverArtPaths, err := l.sweepOrphanedCoverArt(tx)
if err != nil {
return nil, fmt.Errorf("could not query orphaned cover art: %w", err)
}
var orphanedCoverArtPaths []string
for rows.Next() {
var filePath string
if err := rows.Scan(&filePath); err != nil {
l.logger.Warn("could not scan cover art path", "err", err)
continue
}
orphanedCoverArtPaths = append(orphanedCoverArtPaths, filePath)
}
if err := rows.Close(); err != nil {
l.logger.Warn("could not close cover art rows", "err", err)
}
// 16. Delete orphaned cover_art rows.
// SAFETY: Hand-crafted orphan cleanup SQL. Parameterless.
if _, err := tx.ExecContext(l.ctx,
`DELETE FROM cover_art WHERE id NOT IN (
SELECT DISTINCT cover_art_id FROM albums
WHERE cover_art_id IS NOT NULL
)`); err != nil {
return nil, fmt.Errorf("could not delete orphaned cover_art: %w", err)
return nil, err
}
// 17. Delete the library's tagging queue. tagging_items holds a
@@ -392,20 +361,9 @@ func (l *Library) RemoveLibrary(id int64) (*RemovalSummary, error) {
// avoids a costly full re-index of all remaining tracks (~10s for
// 25K tracks).
// 21. Post-commit: Delete orphaned cover art files and their sized
// variants. Only the original is stored in cover_art.file_path; the
// _sm/_md/_lg thumbnails are derived filenames beside it, so they
// have to be removed by name or they accumulate forever.
for _, coverPath := range orphanedCoverArtPaths {
for _, path := range CoverArtFileSet(coverPath) {
if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
l.logger.Warn("could not remove orphaned cover art file",
"path", path,
"err", err,
)
}
}
}
// Post-commit: remove the orphaned cover art files and their sized
// variants.
l.removeCoverArtFiles(orphanedCoverArtPaths)
// 22. Post-commit: Compact queue.
if l.removalHooks.CompactQueue != nil {
+116 -29
View File
@@ -911,30 +911,96 @@ func (l *Library) scanInternal(
orphanStart := time.Now()
existingPaths.Range(func(key, value any) bool {
path := key.(string)
audioFile := value.(sqlcgen.AudioFile)
l.logger.Debug(
"removing orphaned database entry",
"path", path, "id", audioFile.ID,
// Snapshot the orphan set first. The playlist-phantom
// preservation and the deletes are one transaction (the
// preservation has to land before the ON DELETE SET NULL, and
// both have to succeed or neither does), and the ids are what
// scope that preservation to just these files instead of
// rewriting every playlist row on a routine scan.
var (
orphans []sqlcgen.AudioFile
orphanPaths []string
)
if err := l.db.Queries.DeleteAudioFile(
l.ctx, audioFile.ID,
existingPaths.Range(func(key, value any) bool {
orphanPaths = append(orphanPaths, key.(string))
orphans = append(orphans, value.(sqlcgen.AudioFile))
return true
})
deleted := make([]bool, len(orphans))
if len(orphans) > 0 {
orphanIDs := make([]int64, len(orphans))
for i, f := range orphans {
orphanIDs[i] = f.ID
}
tx, beginErr := l.db.BeginTx()
if beginErr != nil {
metrics.addWarning("", "orphan", beginErr)
} else {
defer func() { _ = tx.Rollback() }() // no-op after commit
if err := database.PreservePlaylistPhantomsForFiles(
l.ctx, tx, orphanIDs, l.logger,
); err != nil {
l.logger.Warn(
"failed to delete orphaned audio file",
"path", path,
"id", audioFile.ID,
// Deleting without the phantoms is exactly the
// playlist-emptying bug the preservation exists to
// prevent, so leave the rows for the next scan
// rather than empty the playlists now.
l.logger.Error(
"skipping orphan deletion: could not preserve "+
"playlist entries",
"err", err,
)
metrics.addWarning(path, "orphan", err)
metrics.addWarning("", "orphan", err)
return true
_ = tx.Rollback()
} else {
txq := l.db.Queries.WithTx(tx)
for i, f := range orphans {
if err := txq.DeleteAudioFile(
l.ctx, f.ID,
); err != nil {
l.logger.Warn(
"failed to delete orphaned audio file",
"path", orphanPaths[i],
"id", f.ID,
"err", err,
)
metrics.addWarning(orphanPaths[i], "orphan", err)
continue
}
deleted[i] = true
}
if err := tx.Commit(); err != nil {
l.logger.Error(
"could not commit orphan deletion",
"err", err,
)
metrics.addWarning("", "orphan", err)
}
}
}
}
// Post-commit bookkeeping for the files that actually went.
for i, f := range orphans {
if !deleted[i] {
continue
}
path := orphanPaths[i]
// Keep the file's tagging group in sync: drop the group's
// track count and clear it out once empty, mirroring the
// bookkeeping maybeRebindTaggingGroup does for a group_key
@@ -942,25 +1008,25 @@ func (l *Library) scanInternal(
// and replaced leaves a stale tagging_items row behind —
// its track_count still counts the deleted files, and it
// never clears from the autotag queue.
if audioFile.GroupKey != "" {
if f.GroupKey != "" {
if err := l.db.Queries.DecrementTaggingItemTrackCount(
l.ctx, audioFile.GroupKey,
l.ctx, f.GroupKey,
); err != nil {
l.logger.Warn(
"failed to decrement tagging group for orphan",
"path", path,
"group_key", audioFile.GroupKey,
"group_key", f.GroupKey,
"err", err,
)
metrics.addWarning(path, "orphan", err)
} else if err := l.db.Queries.DeleteTaggingItemIfEmpty(
l.ctx, audioFile.GroupKey,
l.ctx, f.GroupKey,
); err != nil {
l.logger.Warn(
"failed to clean up emptied tagging group for orphan",
"path", path,
"group_key", audioFile.GroupKey,
"group_key", f.GroupKey,
"err", err,
)
@@ -968,13 +1034,21 @@ func (l *Library) scanInternal(
}
}
// Remove from FTS5 search index.
if err := l.db.DeleteSearchIndex(
audioFile.ID,
); err != nil {
// Remove from FTS5 search index and the lyrics index.
if err := l.db.DeleteSearchIndex(f.ID); err != nil {
l.logger.Warn(
"failed to delete FTS entry for orphan",
"id", audioFile.ID,
"id", f.ID,
"err", err,
)
metrics.addWarning(path, "orphan", err)
}
if err := l.db.DeleteLyricsIndex(f.ID); err != nil {
l.logger.Warn(
"failed to delete lyrics index entry for orphan",
"id", f.ID,
"err", err,
)
@@ -982,9 +1056,7 @@ func (l *Library) scanInternal(
}
removed.Add(1)
return true
})
}
metrics.OrphanCleanup = time.Since(orphanStart)
@@ -1193,17 +1265,32 @@ func (l *Library) pruneEmptyEntities() {
}
}
// Cover art after albums: a cover whose album just went is
// unreferenced, and leaving the row behind keeps its files exempt
// from the janitor's covers sweep forever (#247).
orphanedCovers, err := l.sweepOrphanedCoverArt(tx)
if err != nil {
l.logger.Warn("could not sweep orphaned cover art", "err", err)
return
}
if err := tx.Commit(); err != nil {
l.logger.Warn("could not commit entity cleanup", "err", err)
return
}
if len(albumIDs) > 0 || len(artistIDs) > 0 || len(genreIDs) > 0 {
// Post-commit: the rows are gone, so their files can go too.
l.removeCoverArtFiles(orphanedCovers)
if len(albumIDs) > 0 || len(artistIDs) > 0 || len(genreIDs) > 0 ||
len(orphanedCovers) > 0 {
l.logger.Info("pruned empty library entities",
"albums", len(albumIDs),
"artists", len(artistIDs),
"genres", len(genreIDs),
"covers", len(orphanedCovers),
)
}
}
+101
View File
@@ -94,6 +94,57 @@ func countRows(
return n
}
// RemoveFromLibrary is the one path that empties a track *deliberately*:
// the file stays on disk but is excluded, so nothing re-imports it. The
// playlist entry must still survive as a re-linkable phantom rather than
// an empty row, because a later full rescan clears the exclusion and is
// what re-links the entry then (#246).
func TestRemoveFromLibrary_PreservesPlaylistPhantoms(t *testing.T) {
t.Parallel()
lib, db := setupTestLibrary(t)
seedRemovableLibrary(t, lib, "/nonexistent/cover.jpg")
if _, err := lib.db.ExecContext(
`INSERT INTO playlists (name) VALUES ('keepme')`,
); err != nil {
t.Fatalf("seed playlist: %v", err)
}
playlistID := queryInt(
t, db, `SELECT id FROM playlists WHERE name = 'keepme'`,
)
if _, err := lib.db.ExecContext(
`INSERT INTO playlist_tracks (playlist_id, audio_file_id, position)
SELECT ?, id, 0 FROM audio_files
WHERE file_path = '/music/song.mp3'`,
playlistID,
); err != nil {
t.Fatalf("seed playlist_tracks: %v", err)
}
if _, err := lib.RemoveFromLibrary([]string{"/music/song.mp3"}); err != nil {
t.Fatalf("RemoveFromLibrary: %v", err)
}
phantomPath := queryString(
t, db,
`SELECT phantom_file_path FROM playlist_tracks
WHERE playlist_id = ?`,
playlistID,
)
if phantomPath != "/music/song.mp3" {
t.Fatalf(
"phantom_file_path is %q, want %q -- the entry cannot be "+
"re-linked after a later full rescan",
phantomPath, "/music/song.mp3",
)
}
}
// A library with tagging_items must still be removable. tagging_items
// FK-references libraries with no ON DELETE clause, so leaving those
// rows behind fails the DELETE and rolls back the entire removal.
@@ -198,3 +249,53 @@ func TestCoverArtFileSet(t *testing.T) {
}
}
}
// Removing the last track of an album must take the album's cover art
// with it — both the row and every derived file — or the row keeps its
// files exempt from the janitor's covers sweep forever (#247).
func TestRemoveFromLibrary_DeletesOrphanedCoverArt(t *testing.T) {
t.Parallel()
lib, _ := setupTestLibrary(t)
dir := t.TempDir()
// The largest tier is what cover_art.file_path names; write every
// variant so the sweep has a real set to remove.
for _, tier := range thumbnailTiers {
p := filepath.Join(dir, coverart.SizedFilename("abc123.jpg", tier.Suffix))
if err := os.WriteFile(p, []byte("img"), 0o600); err != nil {
t.Fatalf("write %s: %v", p, err)
}
}
cover := filepath.Join(dir, coverart.SizedFilename("abc123.jpg", "_lg"))
seedRemovableLibrary(t, lib, cover)
// Link the album to the cover so it is not orphaned until the track
// (and with it the album) goes.
if _, err := lib.db.ExecContext(
`UPDATE albums SET cover_art_id =
(SELECT id FROM cover_art WHERE file_path = ?)
WHERE name = 'Test Album'`,
cover,
); err != nil {
t.Fatalf("link cover art: %v", err)
}
if _, err := lib.RemoveFromLibrary([]string{"/music/song.mp3"}); err != nil {
t.Fatalf("RemoveFromLibrary: %v", err)
}
if n := countRows(t, lib, "cover_art"); n != 0 {
t.Errorf("cover_art has %d rows after removal, want 0", n)
}
for _, tier := range thumbnailTiers {
p := filepath.Join(dir, coverart.SizedFilename("abc123.jpg", tier.Suffix))
if _, err := os.Stat(p); !os.IsNotExist(err) {
t.Errorf("cover art file still present: %s", filepath.Base(p))
}
}
}
+25
View File
@@ -4,6 +4,7 @@ import (
"errors"
"fmt"
"yellowjacket/backend/database"
"yellowjacket/backend/events"
)
@@ -57,6 +58,25 @@ func (l *Library) RemoveFromLibrary(filePaths []string) (*RemovalResult, error)
var result RemovalResult
// Preserve the playlist entries before the rows go, so they survive
// as re-linkable phantoms rather than empty rows. The track is
// excluded and will not be re-imported on its own, but a later full
// rescan clears the exclusion and this is what lets the entry
// re-link then — the same preservation every other path that empties
// audio_files performs (#246).
rowIDs := make([]int64, len(rows))
for i, row := range rows {
rowIDs[i] = row.ID
}
if err := database.PreservePlaylistPhantomsForFiles(
l.ctx, tx, rowIDs, l.logger,
); err != nil {
return nil, fmt.Errorf(
"could not preserve playlist entries for removal: %w", err,
)
}
// Exclude every path the caller named, including one whose row has
// already gone: the user asked for that file to stay out, and a row
// that disappeared between the click and the commit is not a reason
@@ -127,6 +147,11 @@ func (l *Library) RemoveFromLibrary(filePaths []string) (*RemovalResult, error)
l.logger.Warn("could not delete FTS entry for removed track",
"path", row.FilePath, "id", row.ID, "err", err)
}
if err := l.db.DeleteLyricsIndex(row.ID); err != nil {
l.logger.Warn("could not delete lyrics index entry for removed track",
"path", row.FilePath, "id", row.ID, "err", err)
}
}
// Deleting an audio_files row cascades to queue_tracks, so the
+7 -28
View File
@@ -8,6 +8,7 @@ import (
"time"
"yellowjacket/backend/coverart"
"yellowjacket/backend/database"
)
var errNoLibrariesConfigured = errors.New(
@@ -137,34 +138,12 @@ func (l *Library) clearLibraryTables() error {
// metadata for all linked tracks before audio_files are deleted.
// ON DELETE SET NULL will null out audio_file_id, converting them
// to phantoms that ResolvePhantomTracksAfterScan can re-link.
if _, err := tx.ExecContext(l.ctx, `
UPDATE playlist_tracks
SET
phantom_title = COALESCE(phantom_title, (
SELECT tm.title FROM track_metadata tm
WHERE tm.id = playlist_tracks.audio_file_id
)),
phantom_artist = COALESCE(phantom_artist, (
SELECT tm.artist_name FROM track_metadata tm
WHERE tm.id = playlist_tracks.audio_file_id
)),
phantom_album = COALESCE(phantom_album, (
SELECT tm.album FROM track_metadata tm
WHERE tm.id = playlist_tracks.audio_file_id
)),
phantom_duration_ms = COALESCE(phantom_duration_ms, (
SELECT af.length_milliseconds FROM audio_files af
WHERE af.id = playlist_tracks.audio_file_id
)),
phantom_file_path = COALESCE(phantom_file_path, (
SELECT af.file_path FROM audio_files af
WHERE af.id = playlist_tracks.audio_file_id
))
WHERE audio_file_id IS NOT NULL
`); err != nil {
return fmt.Errorf(
"could not preserve playlist track metadata: %w", err,
)
//
// Shared with the stale-shape retire in backend/database, which is
// the other path that empties this table and which did not do this
// (#183): the statement lives there so the two cannot drift again.
if err := database.PreservePlaylistPhantoms(l.ctx, tx, l.logger); err != nil {
return err
}
// Phase 2: the files. file_genres cascades with them.
+95
View File
@@ -231,3 +231,98 @@ func TestScan_MultipleDirectoriesDoNotCrossContaminate(t *testing.T) {
t.Errorf("Album A and Album B must not share a group_key: %+v", keys)
}
}
// TestScan_OrphanCleanupPreservesPlaylistPhantoms guards #246: a file
// deleted from the library folder *outside* YellowJacket is discovered
// as an orphan by the next scan, and its playlist entry must survive as
// a re-linkable phantom — the same preservation the full rescan and
// stale-retire paths already perform, scoped here to just the orphaned
// file. Before the fix the entry became an empty row (audio_file_id
// NULL and no phantom_file_path), which nothing can ever re-link.
func TestScan_OrphanCleanupPreservesPlaylistPhantoms(t *testing.T) {
t.Parallel()
lib, db := setupTestLibrary(t)
root := t.TempDir()
track := filepath.Join(root, "gone.mp3")
writeTestTrack(t, track, 0)
library, err := db.Queries.CreateLibrary(lib.ctx, sqlcgen.CreateLibraryParams{
Name: "orphans",
Path: root,
})
if err != nil {
t.Fatalf("create library: %v", err)
}
if metrics := lib.scanInternal(library.ID, library.Name, library.Path); metrics == nil {
t.Fatal("first scan returned nil metrics")
}
trackID := queryInt(
t, db, "SELECT id FROM audio_files WHERE file_path = ?", track,
)
if trackID == 0 {
t.Fatal("first scan did not import the track")
}
if _, err := db.ExecContext(
`INSERT INTO playlists (name) VALUES ('keepme')`,
); err != nil {
t.Fatalf("seed playlist: %v", err)
}
playlistID := queryInt(
t, db, "SELECT id FROM playlists WHERE name = 'keepme'",
)
if _, err := db.ExecContext(
`INSERT INTO playlist_tracks (playlist_id, audio_file_id, position)
VALUES (?, ?, 0)`,
playlistID, trackID,
); err != nil {
t.Fatalf("seed playlist_tracks: %v", err)
}
// The file goes away outside the app.
if err := os.Remove(track); err != nil {
t.Fatalf("remove track: %v", err)
}
if metrics := lib.scanInternal(library.ID, library.Name, library.Path); metrics == nil {
t.Fatal("second scan returned nil metrics")
}
if n := queryInt(
t, db, "SELECT COUNT(*) FROM audio_files WHERE file_path = ?", track,
); n != 0 {
t.Fatalf("audio_files still holds the removed path: %d rows", n)
}
if n := queryInt(
t, db,
"SELECT COUNT(*) FROM playlist_tracks WHERE playlist_id = ? "+
"AND audio_file_id IS NULL",
playlistID,
); n != 1 {
t.Fatalf(
"playlist entry did not become a phantom: %d null-id rows, want 1",
n,
)
}
phantomPath := queryString(
t, db,
"SELECT phantom_file_path FROM playlist_tracks WHERE playlist_id = ?",
playlistID,
)
if phantomPath != track {
t.Fatalf(
"phantom_file_path = %q, want %q -- the entry cannot be "+
"re-linked if the file comes back",
phantomPath, track,
)
}
}
+117
View File
@@ -666,3 +666,120 @@ func TestExpiredHTTPCacheJob_TrimsToBudget(t *testing.T) {
t.Errorf("kept %q, want the longest-lived row", kept)
}
}
// TestStaleArtistMetadataJob pins the sweep's two keep rules: an owned
// artist's metadata survives, a browsed artist's survives while it still
// holds cached artwork, and everything else goes (#248).
func TestStaleArtistMetadataJob(t *testing.T) {
t.Parallel()
db := database.NewTestDB(t)
const (
ownedMBID = "11111111-1111-1111-1111-111111111111"
browsedMBID = "22222222-2222-2222-2222-222222222222"
staleMBID = "33333333-3333-3333-3333-333333333333"
)
// The owned artist is in the library - which means a *file* says
// so. An artists row on its own is not ownership.
database.InsertTestTrack(t, db, database.TestTrack{
FilePath: "/music/owned.mp3",
Artist: "Owned",
ArtistMBID: ownedMBID,
})
for _, mbid := range []string{ownedMBID, browsedMBID, staleMBID} {
if _, err := db.ExecContext(
`INSERT INTO artist_metadata (mbid, source, data, fetched_at)
VALUES (?, 'wikidata-p18', x'00', CURRENT_TIMESTAMP)`,
mbid,
); err != nil {
t.Fatalf("seed artist_metadata for %s: %v", mbid, err)
}
}
// The browsed artist holds cached artwork, so its metadata is still
// referenced and must survive.
if _, err := db.ExecContext(
`INSERT INTO artist_images
(artist_mbid, source, source_url, file_path)
VALUES (?, 'test', 'http://x', '/art/primary.jpg')`,
browsedMBID,
); err != nil {
t.Fatalf("seed artist_images: %v", err)
}
if _, err := StaleArtistMetadataJob(db).Run(context.Background()); err != nil {
t.Fatalf("run job: %v", err)
}
for _, tc := range []struct {
mbid string
want int
}{
{ownedMBID, 1},
{browsedMBID, 1},
{staleMBID, 0},
} {
var n int
if err := db.QueryRowWriter(
"SELECT COUNT(*) FROM artist_metadata WHERE mbid = ?", tc.mbid,
).Scan(&n); err != nil {
t.Fatalf("count %s: %v", tc.mbid, err)
}
if n != tc.want {
t.Errorf("artist_metadata rows for %s = %d, want %d", tc.mbid, n, tc.want)
}
}
}
// TestStaleSearchClicksJob deletes only the clicks old enough to have
// left the retention window (#249).
func TestStaleSearchClicksJob(t *testing.T) {
t.Parallel()
db := database.NewTestDB(t)
count := func(mbid string) int {
t.Helper()
var n int
if err := db.QueryRowWriter(
"SELECT COUNT(*) FROM search_clicks WHERE entity_mbid = ?", mbid,
).Scan(&n); err != nil {
t.Fatalf("count %s: %v", mbid, err)
}
return n
}
seed := func(query, mbid, lastClicked string) {
t.Helper()
if _, err := db.ExecContext(
`INSERT INTO search_clicks
(query, entity_mbid, entity_type, click_count, last_clicked)
VALUES (?, ?, 'recording', 1, ?)`,
query, mbid, lastClicked,
); err != nil {
t.Fatalf("seed search_clicks: %v", err)
}
}
seed("tide", "aaaa", "2024-01-01 00:00:00") // stale
seed("tide", "bbbb", "2999-01-01 00:00:00") // recent
if _, err := StaleSearchClicksJob(db).Run(context.Background()); err != nil {
t.Fatalf("run job: %v", err)
}
if n := count("bbbb"); n != 1 {
t.Errorf("recent click was deleted: %d rows, want 1", n)
}
if n := count("aaaa"); n != 0 {
t.Errorf("stale click survived: %d rows, want 0", n)
}
}
+66
View File
@@ -628,3 +628,69 @@ func dirSize(dir string) (bytes, files int64) {
return bytes, files
}
// StaleArtistMetadataJob evicts long-lived artist metadata (bios, wiki
// leads, relationships) for artists the user no longer has any reason
// to keep around: not owned and holding no cached artwork.
//
// artist_metadata has no TTL by design — entity data changes rarely and
// re-fetching spends someone else's rate limit — so without a sweep it
// grows for the life of the install. This is the "swept when the
// artist is no longer referenced" contract the datamap always declared
// for it and nothing ever performed (#248).
func StaleArtistMetadataJob(db *database.DB) Job {
return Job{
Name: "artist-metadata-sweep",
MinInterval: dailyInterval,
Run: func(_ context.Context) (Result, error) {
res, err := db.ExecContext(
`DELETE FROM artist_metadata
WHERE mbid NOT IN (` + ownedArtistMBIDs + `)
AND mbid NOT IN (
SELECT artist_mbid FROM artist_images
)`,
)
if err != nil {
return Result{}, fmt.Errorf(
"delete stale artist_metadata rows: %w", err,
)
}
rows, _ := res.RowsAffected()
return Result{RowsDeleted: rows}, nil
},
}
}
// searchClicksRetention is how long a search-click ranking signal stays
// useful. search_clicks is authored behavioural data — nothing that
// owns a row ever drops it — so age is the ceiling that keeps the table
// from growing without bound for the life of the install (#249).
const searchClicksRetention = "-180 days"
// StaleSearchClicksJob deletes search-click ranking rows older than the
// retention window. Rows are small and the table grows slowly, so this
// runs daily and does almost nothing most runs.
func StaleSearchClicksJob(db *database.DB) Job {
return Job{
Name: "search-clicks-sweep",
MinInterval: dailyInterval,
Run: func(_ context.Context) (Result, error) {
res, err := db.ExecContext(
`DELETE FROM search_clicks
WHERE last_clicked < datetime('now', ?)`,
searchClicksRetention,
)
if err != nil {
return Result{}, fmt.Errorf(
"delete stale search_clicks rows: %w", err,
)
}
rows, _ := res.RowsAffected()
return Result{RowsDeleted: rows}, nil
},
}
}
+28
View File
@@ -134,6 +134,12 @@ type Service struct {
libraryDir LibraryDirProvider
favoritesConf FavoritesConfigProvider
// onDeleted, when set, is called after a playlist is deleted so
// cross-cutting state that points at it (the queue's "Playing
// from" label) can stop pointing at a playlist that no longer
// exists. Wired from app.go, like Library.SetRemovalHooks.
onDeleted func(playlistID int64)
// dataDirOverride, when non-empty, replaces the OS user data
// directory as the base for the playlists folder. Set by tests to
// keep M3U writes out of the real user data directory.
@@ -166,6 +172,17 @@ func (s *Service) SetFavoritesConfig(
s.favoritesConf = provider
}
// SetOnPlaylistDeleted registers a callback invoked after a playlist is
// deleted, for cross-cutting invalidation.
//
//wails:ignore // internal wiring, not part of the app's IPC surface.
func (s *Service) SetOnPlaylistDeleted(onDeleted func(playlistID int64)) {
s.mu.Lock()
defer s.mu.Unlock()
s.onDeleted = onDeleted
}
// ServiceStartup is v3's service lifecycle hook: it runs once the
// runtime exists, and ctx is cancelled when the app shuts down. It
// replaces v2's SetContext, which had to be called by hand from
@@ -766,6 +783,17 @@ func (s *Service) DeletePlaylist(playlistID int64) error {
s.emitEvent(events.PlaylistDeleted, playlistID)
// Cross-cutting invalidation: the queue's "Playing from" label may
// point at this playlist, and a link to a playlist that no longer
// exists is worse than none.
s.mu.Lock()
onDeleted := s.onDeleted
s.mu.Unlock()
if onDeleted != nil {
onDeleted(playlistID)
}
// Recreate the default playlist if we just deleted it.
if s.defaultPlaylistID() == playlistID {
s.EnsureDefaultPlaylist()
+6 -4
View File
@@ -6,7 +6,7 @@ import (
"yellowjacket/backend/events"
)
// recordPlay inserts a play_history row and updates the denormalized
// recordPlay inserts a listening_events row and updates the denormalized
// play_count / last_played columns on audio_files. Called from
// OnPlaybackFinished for the track that just finished.
//
@@ -20,10 +20,12 @@ func (q *Queue) recordPlay(audioFileID int64) {
now := time.Now().UTC().Format(time.DateTime)
// Insert play_history row.
// Insert the listening event. A natural finish is a 'complete' by
// construction; position/duration are the classifier's to fill once
// skips are recorded (see .planning/plans/active/021).
_, err := q.db.ExecContext(
`INSERT INTO play_history (audio_file_id, played_at)
VALUES (?, ?)`,
`INSERT INTO listening_events (audio_file_id, kind, occurred_at)
VALUES (?, 'complete', ?)`,
audioFileID, now,
)
if err != nil {
+15
View File
@@ -1581,6 +1581,21 @@ func (q *Queue) dropSource() {
q.source = Source{}
}
// DropSourceForPlaylist clears the queue's "Playing from" label when
// its source playlist is deleted. A link back to a playlist that no
// longer exists is worse than none, and the label otherwise survives
// the deletion until the next SetQueue (#249).
func (q *Queue) DropSourceForPlaylist(playlistID int64) {
q.mu.Lock()
defer q.mu.Unlock()
if (q.source.Type == "playlist" || q.source.Type == "smartPlaylist") &&
q.source.ID == playlistID {
q.dropSource()
q.persistState()
}
}
// commitMutation persists the current queue state after a mutation.
// When reindex is true, track positions are renumbered first.
// The caller must hold q.mu.
+32
View File
@@ -535,3 +535,35 @@ func TestCycleRepeat_CyclesThroughModes(t *testing.T) {
t.Errorf("after third cycle: got %q, want %q", state.RepeatMode, RepeatOff)
}
}
// TestDropSourceForPlaylist clears the "Playing from" label when the
// queue's source playlist is deleted, and leaves it alone otherwise
// (#249).
func TestDropSourceForPlaylist(t *testing.T) {
t.Parallel()
q, db := setupTestQueue(t)
paths := seedAudioFiles(t, db, 2)
q.SetQueue(paths, 0, false, Source{Type: "playlist", ID: 42, Label: "Road Trip"})
q.DropSourceForPlaylist(42)
if got := q.GetState().Source; got != (Source{}) {
t.Errorf("source = %+v, want empty after playlist 42 deleted", got)
}
}
func TestDropSourceForPlaylistIgnoresOtherPlaylists(t *testing.T) {
t.Parallel()
q, db := setupTestQueue(t)
paths := seedAudioFiles(t, db, 2)
source := Source{Type: "smartPlaylist", ID: 42, Label: "Road Trip"}
q.SetQueue(paths, 0, false, source)
q.DropSourceForPlaylist(7)
if got := q.GetState().Source; got != source {
t.Errorf("source = %+v, want %+v unchanged for a different playlist", got, source)
}
}
@@ -56,6 +56,16 @@ export function CycleRepeat(): $CancellablePromise<void> {
return $Call.ByID(3510519482);
}
/**
* DropSourceForPlaylist clears the queue's "Playing from" label when
* its source playlist is deleted. A link back to a playlist that no
* longer exists is worse than none, and the label otherwise survives
* the deletion until the next SetQueue (#249).
*/
export function DropSourceForPlaylist(playlistID: number): $CancellablePromise<void> {
return $Call.ByID(1435106374, playlistID);
}
/**
* EmitCurrentState emits the current queue state to the frontend.
* This is called after the frontend DOM is ready.
@@ -0,0 +1 @@
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 320 512"><!--! Font Awesome Free 7.3.1 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL 1.1, Code: MIT License) Copyright 2026 Fonticons, Inc. --><path fill="currentColor" d="M9.4 233.4c-12.5 12.5-12.5 32.8 0 45.3l192 192c12.5 12.5 32.8 12.5 45.3 0s12.5-32.8 0-45.3L77.3 256 246.6 86.6c12.5-12.5 12.5-32.8 0-45.3s-32.8-12.5-45.3 0l-192 192z"/></svg>

After

Width:  |  Height:  |  Size: 476 B

@@ -60,6 +60,7 @@ import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js';
import { dictByName } from '@utils/binding';
import type { TrackDetails } from '@components/track-details/track-details.js';
import { showTrackDetailsForPath } from '@utils/track-details-opener.js';
import { openMusicBrainz } from '@utils/external-link';
import '@components/playlist-picker/playlist-picker.js';
import {
ICON_CAN_REQUEST,
@@ -2942,7 +2943,7 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost {
if (!track?.mbid) return;
window.open(`https://musicbrainz.org/recording/${track.mbid}`, '_blank', 'noopener');
openMusicBrainz(`/recording/${track.mbid}`);
}
/**
@@ -4,6 +4,8 @@ import { customElement, property, state, query } from 'lit/decorators.js';
import { classMap } from 'lit/directives/class-map.js';
import { designTokens } from '../../styles/tokens.css';
import { backButton } from '../../styles/back-button.css';
import { albumCardStyles } from '../../styles/album-card.css';
import '../scroll-row/scroll-row.js';
import {
LookupArtist,
BrowseReleaseGroups,
@@ -46,11 +48,8 @@ import {
libraryStatusFor,
toggleRequest,
} from '@utils/library-status';
import {
isOwned,
ownershipLabel,
unownedStyles,
} from '@utils/ownership';
import { isOwned, ownershipLabel } from '@utils/ownership';
import { openMusicBrainz } from '@utils/external-link';
import { completenessStore } from '@store/completeness-store';
import '../catalog-scope-notice/catalog-scope-notice.js';
import type { CatalogScope } from '../catalog-scope-notice/catalog-scope-notice.js';
@@ -62,6 +61,7 @@ import {
ContextMenuController,
contextMenuStyles,
isContextMenuKey,
MenuKeyboard,
} from '@utils/context-menu-controller.js';
import type { ContextMenuHost, MenuTarget } from '@utils/context-menu-controller.js';
import '@awesome.me/webawesome/dist/components/popup/popup.js';
@@ -187,11 +187,11 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
@state() private topReleasesExpanded = false;
private topSectionStacked = false;
private topSectionObserver?: ResizeObserver;
@state() private expandedDiscoGroups = new Set<string>();
/** Number of album cards that fit in one row of the discography grid. */
@state() private discoRowSize = 5;
private discoObserver?: ResizeObserver;
@state() private similarExpanded = false;
/** Whether the Play button's Shuffle dropdown is up. */
@state() private playMenuOpen = false;
private playMenuKeyboard = new MenuKeyboard(() => this.closePlayMenu());
private playOutsideAttached = false;
/* ── Release prefetch ── */
@@ -221,6 +221,12 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
@query('#context-menu')
private contextMenuPopup!: MenuSurface;
@query('.play-menu-button')
private playMenuButton?: HTMLButtonElement;
@query('#artist-play-menu')
private playMenuPanel?: HTMLElement;
@query('#playlist-submenu')
private playlistSubmenuPopup?: WaPopup;
@@ -271,7 +277,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
backButton,
exploreLinkStyles,
contextMenuStyles,
unownedStyles,
albumCardStyles,
css`
:host {
display: flex;
@@ -319,10 +325,45 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
object-fit: cover;
}
.artist-follow {
.artist-actions {
display: flex;
align-items: center;
gap: 8px;
flex-wrap: wrap;
margin-top: 10px;
}
/* The Play button and its caret are one control, so they
are one box: no gap between them, and the caret carries
the same filled appearance as the button it extends. */
.play-split {
display: inline-flex;
align-items: stretch;
}
.play-menu-button {
display: inline-flex;
align-items: center;
justify-content: center;
width: 28px;
padding: 0;
border: none;
border-left: 1px solid rgba(0, 0, 0, 0.25);
border-radius: 0 6px 6px 0;
background: var(--yj-accent, #ffd43b);
color: var(--yj-accent-fg, #000);
cursor: pointer;
}
.play-menu-button:hover {
filter: brightness(1.1);
}
.play-menu-button:focus-visible {
outline: 2px solid var(--yj-accent-text, #ffd43b);
outline-offset: 2px;
}
.artist-info {
display: flex;
flex-direction: column;
@@ -331,7 +372,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
}
.artist-title {
font-size: 24px;
font-size: 28px;
font-weight: 700;
color: var(--yj-text-primary, #fff);
white-space: nowrap;
@@ -359,6 +400,13 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
flex-wrap: wrap;
}
/* The listen count is a headline number, not metadata, so
it sits a size above the type/country line. */
.artist-listens {
font-size: var(--yj-text-lg);
color: var(--yj-text-secondary, #b3b3b3);
}
.meta-separator {
opacity: 0.4;
}
@@ -446,23 +494,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
outline-offset: -2px;
}
.artist-play-actions {
margin-top: 10px;
display: flex;
gap: 8px;
align-items: center;
flex-wrap: wrap;
}
.track-rank {
width: 24px;
text-align: right;
color: var(--yj-text-tertiary, #888);
font-size: var(--yj-text-md);
font-variant-numeric: tabular-nums;
flex-shrink: 0;
}
.track-art {
width: 32px;
height: 32px;
@@ -489,6 +520,52 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
opacity: 0.5;
}
/* Play where you own the track, the request badge where you
do not — over the artwork rather than at the end of the
row, where it was a badge beside a row you can already
double-click. */
.track-art-overlay {
position: absolute;
inset: 0;
display: flex;
align-items: center;
justify-content: center;
border-radius: 4px;
background: rgba(0, 0, 0, 0.55);
visibility: hidden;
opacity: 0;
transition: opacity 0.15s ease, visibility 0.15s ease;
}
.track-art-play {
display: flex;
align-items: center;
justify-content: center;
padding: 0;
border: none;
background: none;
color: #fff;
font-size: 14px;
cursor: pointer;
}
@media (hover: hover) and (pointer: fine) {
.track-item:hover .track-art-overlay,
.track-item:focus-within .track-art-overlay {
visibility: visible;
opacity: 1;
}
}
/* No hover means no double-click either, so the overlay is
the only route to playing a top track and must be there. */
@media not all and (hover: hover) {
.track-art-overlay {
visibility: visible;
opacity: 1;
}
}
.track-info {
flex: 1;
min-width: 0;
@@ -522,7 +599,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
.track-item library-status-indicator {
flex-shrink: 0;
}
/* ── Top section (tracks + releases side-by-side) ── */
.top-section-wrapper {
container-type: inline-size;
@@ -754,8 +830,30 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
white-space: nowrap;
}
.top-release-meta library-status-indicator {
flex-shrink: 0;
.top-release-art .album-card-badge {
position: absolute;
top: 4px;
left: 4px;
z-index: 1;
display: flex;
visibility: hidden;
opacity: 0;
transition: opacity 0.15s ease, visibility 0.15s ease;
}
@media (hover: hover) and (pointer: fine) {
.top-release-card:hover .album-card-badge,
.top-release-card:focus-within .album-card-badge {
visibility: visible;
opacity: 1;
}
}
@media not all and (hover: hover) {
.top-release-art .album-card-badge {
visibility: visible;
opacity: 1;
}
}
@@ -773,150 +871,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
margin: 0;
}
.album-grid {
display: grid;
grid-template-columns: repeat(auto-fill, 140px);
gap: 16px;
}
.album-grid.collapsed {
grid-template-rows: 1fr;
overflow: hidden;
}
.disco-toggle {
display: flex;
align-items: center;
justify-content: center;
gap: 6px;
padding: 4px 10px;
margin-top: 4px;
border: none;
border-radius: 6px;
background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.06));
color: var(--yj-text-secondary, #b3b3b3);
font-size: var(--yj-text-xs);
cursor: pointer;
transition: background 0.15s ease, color 0.15s ease;
width: 100%;
}
.disco-toggle:hover {
background: var(--yj-bg-hover, rgba(255, 255, 255, 0.1));
color: var(--yj-text-primary, #fff);
}
.disco-toggle wa-icon {
font-size: 11px;
transition: transform 0.2s ease;
}
.disco-toggle[aria-expanded='true'] wa-icon {
transform: rotate(180deg);
}
.album-card {
display: flex;
flex-direction: column;
gap: 6px;
padding: 8px;
border-radius: 8px;
cursor: pointer;
transition: background 0.15s ease;
}
.album-card:hover {
background: var(
--yj-bg-overlay,
rgba(255, 255, 255, 0.06)
);
}
.album-card:active {
transform: scale(0.97);
}
.album-art-container {
width: 100%;
aspect-ratio: 1;
border-radius: 4px;
overflow: hidden;
flex-shrink: 0;
position: relative;
background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.06));
}
.album-art-container img {
width: 100%;
height: 100%;
object-fit: cover;
display: block;
border-radius: 4px;
}
.album-art-fallback {
display: flex;
align-items: center;
justify-content: center;
width: 100%;
height: 100%;
position: absolute;
inset: 0;
}
.album-art-fallback wa-icon {
color: var(--yj-text-tertiary, #888);
font-size: 24px;
opacity: 0.5;
}
.album-title {
font-weight: 500;
color: var(--yj-text-primary, #fff);
font-size: var(--yj-text-sm);
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
}
.album-meta {
display: flex;
align-items: center;
justify-content: space-between;
gap: 6px;
color: var(--yj-text-tertiary, #888);
font-size: var(--yj-text-xs);
min-height: 20px;
}
.album-meta-text {
display: flex;
align-items: center;
gap: 6px;
min-width: 0;
overflow: hidden;
text-overflow: ellipsis;
white-space: nowrap;
}
.album-meta library-status-indicator {
flex-shrink: 0;
margin-left: auto;
}
/* ── Similar artists ── */
.similar-row {
display: grid;
grid-template-columns: repeat(auto-fill, 140px);
gap: 16px;
overflow: hidden;
}
.similar-row.collapsed {
grid-template-rows: 1fr;
overflow: hidden;
}
.similar-artist-card {
display: flex;
flex-direction: column;
@@ -926,6 +881,9 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
border-radius: 8px;
cursor: pointer;
text-align: center;
width: 120px;
box-sizing: border-box;
flex-shrink: 0;
transition: background 0.15s ease;
}
@@ -1056,7 +1014,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
this.unsubSimilarReady?.();
if (this.discogFallbackTimer) clearTimeout(this.discogFallbackTimer);
this.topSectionObserver?.disconnect();
this.discoObserver?.disconnect();
this.detachPlayOutsideClose();
}
/**
@@ -1083,17 +1041,14 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
protected override firstUpdated() {
this.observeTopSectionWidth();
this.observeDiscoWidth();
}
protected override updated() {
// Re-attach observers if elements appeared after initial render.
// Re-attach the observer if the section appeared after initial
// render.
if (!this.topSectionObserver) {
this.observeTopSectionWidth();
}
if (!this.discoObserver) {
this.observeDiscoWidth();
}
}
/**
@@ -1126,32 +1081,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
this.topSectionObserver.observe(wrapper);
}
/**
* Watch the .content width and compute how many album cards
* fit in one row of the discography grid.
* Grid uses: repeat(auto-fill, minmax(140px, 1fr)) with 16px gap
* and album-card has 8px padding on each side.
*/
private observeDiscoWidth() {
const content = this.renderRoot.querySelector('.content');
if (!content) return;
const CARD_MIN = 140;
const GAP = 16;
this.discoObserver = new ResizeObserver((entries) => {
for (const entry of entries) {
const width = entry.contentBoxSize?.[0]?.inlineSize ?? entry.contentRect.width;
const cols = Math.max(1, Math.floor((width + GAP) / (CARD_MIN + GAP)));
if (cols !== this.discoRowSize) {
this.discoRowSize = cols;
}
}
});
this.discoObserver.observe(content);
}
/* ── Data Loading ── */
private async loadAllData() {
@@ -1862,6 +1791,8 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
} catch {
// No image — letter avatar stays.
}
return undefined;
}),
);
}
@@ -1999,6 +1930,65 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
}
}
/* ── Play / Shuffle split button ── */
/**
* Open the Play button's Shuffle dropdown.
*
* `page-header`'s overflow menu one control over: the same
* `MenuKeyboard`, the same document-level outside-close, and the
* same `menu-surface`, so the phone gets the bottom sheet rather
* than a popup that Chrome 113 clips.
*/
private togglePlayMenu = (): void => {
if (this.playMenuOpen) {
this.closePlayMenu();
return;
}
this.playMenuOpen = true;
void this.updateComplete.then(() => {
if (!this.playMenuOpen) return;
this.playMenuKeyboard.open(
this.playMenuPanel ?? null,
this.playMenuButton ?? null,
);
this.attachPlayOutsideClose();
});
};
private closePlayMenu = (): void => {
if (!this.playMenuOpen) return;
this.detachPlayOutsideClose();
this.playMenuKeyboard.close();
this.playMenuOpen = false;
};
private onPlayOutsideDown = (e: Event): void => {
if (e.composedPath().includes(this.playMenuPanel as EventTarget)) return;
if (e.composedPath().includes(this.playMenuButton as EventTarget)) return;
this.closePlayMenu();
};
private attachPlayOutsideClose(): void {
if (this.playOutsideAttached) return;
this.playOutsideAttached = true;
document.addEventListener('mousedown', this.onPlayOutsideDown, true);
}
private detachPlayOutsideClose(): void {
if (!this.playOutsideAttached) return;
this.playOutsideAttached = false;
document.removeEventListener('mousedown', this.onPlayOutsideDown, true);
}
/**
* File path for one top track, resolved by recording MBID — the
* same key `localId` was set from. Works whether or not the
@@ -2240,11 +2230,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
if (!release?.mbid) return;
window.open(
`https://musicbrainz.org/release-group/${release.mbid}`,
'_blank',
'noopener',
);
openMusicBrainz(`/release-group/${release.mbid}`);
}
private onContextMenuAction(
@@ -2358,7 +2344,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
if (!track?.recordingMbid) return;
window.open(`https://musicbrainz.org/recording/${track.recordingMbid}`, '_blank', 'noopener');
openMusicBrainz(`/recording/${track.recordingMbid}`);
}
/* ── Navigation ── */
@@ -2538,12 +2524,14 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
: nothing}
${this.renderArtistMeta()}
${this.artist?.popularity && this.artist.popularity > 0
? html`<span class="artist-meta">${formatListenCount(this.artist.popularity)} plays on ListenBrainz</span>`
? html`<span class="artist-listens">${formatListenCount(this.artist.popularity)} plays on ListenBrainz</span>`
: nothing}
<div class="artist-actions">
${this.renderPlayLibraryAction()}
${this.renderFollowAction()}
</div>
</div>
</div>
<div class="content">
<catalog-scope-notice
scope=${this.catalogScope()}
@@ -2572,25 +2560,53 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
if (this.ownedLocalAlbumIds().length === 0) return nothing;
return html`
<div class="artist-play-actions">
<div class="play-split">
<wa-button
size="small"
appearance="filled"
data-testid="artist-play-library"
title="Play library tracks"
@click=${() => void this.playLibraryTracks(false)}
>
<wa-icon slot="start" name="play"></wa-icon>
Play library tracks
Play
</wa-button>
<wa-button
size="small"
appearance="outlined"
data-testid="artist-shuffle-library"
@click=${() => void this.playLibraryTracks(true)}
<menu-surface
placement="bottom-start"
.active=${this.playMenuOpen}
@menu-dismiss=${this.closePlayMenu}
>
<wa-icon slot="start" name="shuffle"></wa-icon>
<button
slot="anchor"
class="play-menu-button"
type="button"
data-testid="artist-play-menu"
aria-label="More play options"
aria-haspopup="menu"
aria-expanded=${this.playMenuOpen ? 'true' : 'false'}
aria-controls="artist-play-menu"
@click=${this.togglePlayMenu}
>
<wa-icon name="chevron-down"></wa-icon>
</button>
<div
id="artist-play-menu"
class="context-menu-panel"
role="menu"
aria-label="Play options"
>
<wa-dropdown-item
data-testid="artist-shuffle-library"
@click=${() => {
this.closePlayMenu();
void this.playLibraryTracks(true);
}}
>
<wa-icon slot="icon" name="shuffle"></wa-icon>
Shuffle
</wa-button>
</wa-dropdown-item>
</div>
</menu-surface>
</div>
`;
}
@@ -2786,10 +2802,13 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
const request = downloadStore.requestFor(this.artistMBID);
return html`
<div class="artist-follow">
<wa-button
size="small"
appearance=${request ? 'filled' : 'outlined'}
data-testid="artist-follow"
title=${request
? 'Following this artist'
: 'Follow this artist for new releases'}
@click=${() => void this.toggleFollow(request?.id)}
>
<!-- This was bookmark-check, which is not in
@@ -2802,9 +2821,8 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
slot="start"
name=${request ? ICON_REQUESTED : ICON_CAN_REQUEST}
></wa-icon>
${request ? 'Following' : 'Follow for new releases'}
${request ? 'Following' : 'Follow'}
</wa-button>
</div>
`;
}
@@ -2888,16 +2906,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
this.topReleasesExpanded = !this.topReleasesExpanded;
}
private toggleDiscoGroup(type: string) {
const next = new Set(this.expandedDiscoGroups);
if (next.has(type)) {
next.delete(type);
} else {
next.add(type);
}
this.expandedDiscoGroups = next;
}
private renderTopSection() {
const hasTracks = !this.loadingTracks && this.topTracks.length > 0;
const hasReleases = !this.loadingTopReleases && this.topReleaseGroups.length > 0;
@@ -2971,6 +2979,31 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
}} />`
: html`<wa-icon name="compact-disc"></wa-icon>`;
})()}
<!-- Over the artwork, not beside the
row: play where you own it, the
request badge where you do not. -->
<div class="track-art-overlay">
${owned
? html`<button
class="track-art-play"
type="button"
aria-label=${`Play ${t.trackName}`}
@click=${(e: Event) => {
e.stopPropagation();
void this.playTrack(t);
}}
>
<wa-icon name="play"></wa-icon>
</button>`
: html`<library-status-indicator
status=${libraryStatusFor(false, t.recordingMbid)}
entity-type="track"
label=${t.trackName}
request-mbid=${t.recordingMbid}
request-artist=${t.artistName ?? ''}
size="18"
></library-status-indicator>`}
</div>
</div>
<div class="track-info">
<div class="track-title">${trackLink(t.trackName, t.releaseName, t.releaseGroupMbid ?? '', t.recordingMbid)}</div>
@@ -2979,15 +3012,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
<span class="track-listens">
${formatListenCount(t.totalListenCount)} plays
</span>
${owned
? nothing
: html`<library-status-indicator
status=${libraryStatusFor(false, t.recordingMbid)}
entity-type="track"
label=${t.trackName}
request-mbid=${t.recordingMbid}
request-artist=${t.artistName ?? ''}
></library-status-indicator>`}
</div>
`;
})}
@@ -3093,6 +3117,18 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
<div class="album-art-fallback" style="${artURL ? 'display: none' : ''}">
<wa-icon name="compact-disc"></wa-icon>
</div>
<div class="album-card-badge">
<library-status-indicator
status=${badge.status}
owned=${badge.owned}
expected=${badge.expected}
entity-type="album"
label=${rg.title}
request-mbid=${rg.releaseGroupMbid}
request-artist=${this.artist?.name ?? ''}
size="21"
></library-status-indicator>
</div>
</div>
<div class="top-release-text">
<div class="top-release-title" title="${rg.title}">
@@ -3102,18 +3138,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
<div class="top-release-meta-text">
${rg.date ? html`<span>${extractYear(rg.date)}</span>` : nothing}
</div>
${badge.status === 'in-library'
? nothing
: html`<library-status-indicator
status=${badge.status}
owned=${badge.owned}
expected=${badge.expected}
entity-type="album"
label=${rg.title}
request-mbid=${rg.releaseGroupMbid}
request-artist=${this.artist?.name ?? ''}
size="18"
></library-status-indicator>`}
</div>
</div>
</div>
@@ -3164,37 +3188,16 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
<section>
<h3 class="section-header">Discography</h3>
${groups.map(
(g) => {
const isExpanded = this.expandedDiscoGroups.has(g.type);
const rowSize = this.discoRowSize;
const showToggle = g.items.length > rowSize;
const visibleItems = isExpanded ? g.items : g.items.slice(0, rowSize);
return html`
(g) => html`
<div class="disco-group">
<h4 class="disco-type-header">
${g.type === 'Other' ? 'Other Releases' : g.type.endsWith('s') ? g.type : `${g.type}s`}
</h4>
<div class="album-grid">
${visibleItems.map((rg) => this.renderAlbumCard(rg))}
<scroll-row>
${g.items.map((rg) => this.renderAlbumCard(rg))}
</scroll-row>
</div>
${showToggle
? html`
<button
class="disco-toggle"
aria-expanded="${isExpanded}"
@click=${() => this.toggleDiscoGroup(g.type)}
>
${isExpanded
? 'Show less'
: `Show all ${g.items.length}`}
<wa-icon name="chevron-down"></wa-icon>
</button>
`
: nothing}
</div>
`;
},
`,
)}
</section>
`;
@@ -3234,15 +3237,8 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
<div class="album-art-fallback" style="${artURL ? 'display: none' : ''}">
<wa-icon name="compact-disc"></wa-icon>
</div>
</div>
<div class="album-title" title="${rg.title}">${rg.title}</div>
<div class="album-meta">
<div class="album-meta-text">
${year ? html`<span>${year}</span>` : nothing}
</div>
${badge.status === 'in-library'
? nothing
: html`<library-status-indicator
<div class="album-card-badge">
<library-status-indicator
status=${badge.status}
owned=${badge.owned}
expected=${badge.expected}
@@ -3250,7 +3246,16 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
label=${rg.title}
request-mbid=${rg.mbid}
request-artist=${this.artist?.name ?? ''}
></library-status-indicator>`}
size="23"
></library-status-indicator>
</div>
</div>
<div class="album-title" title="${rg.title}">${rg.title}</div>
<div class="album-artist">${rg.artistCredit ?? ''}</div>
<div class="album-meta">
<div class="album-meta-text">
${year ? html`<span>${year}</span>` : nothing}
</div>
</div>
</div>
`;
@@ -3266,15 +3271,12 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
// Cap the similar-artists list at 10 to avoid a very long list.
const maxSimilar = 10;
const artists = this.similarArtists.slice(0, maxSimilar);
const showToggle = artists.length > this.discoRowSize;
const collapsed = !this.similarExpanded && showToggle;
const visible = collapsed ? artists.slice(0, this.discoRowSize) : artists;
return html`
<section>
<h3 class="section-header">Similar Artists</h3>
<div class="similar-row ${collapsed ? 'collapsed' : ''}">
${visible.map((a) => {
<scroll-row>
${artists.map((a) => {
const imgURL = this.similarImageURLs.get(a.artistMbid);
return html`
<div
@@ -3313,21 +3315,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
</div>
`;
})}
</div>
${showToggle
? html`
<button
class="disco-toggle"
aria-expanded="${this.similarExpanded}"
@click=${() => { this.similarExpanded = !this.similarExpanded; }}
>
${this.similarExpanded
? 'Show less'
: `Show all ${artists.length}`}
<wa-icon name="chevron-down"></wa-icon>
</button>
`
: nothing}
</scroll-row>
</section>
`;
}
@@ -1,10 +1,7 @@
import { avatarBackground } from '@utils/avatar-color';
import { albumBadgeFor, libraryStatusFor } from '@utils/library-status';
import {
isOwned,
ownershipLabel,
unownedStyles,
} from '@utils/ownership';
import { isOwned, ownershipLabel } from '@utils/ownership';
import { openMusicBrainz } from '@utils/external-link';
import { completenessStore } from '@store/completeness-store';
import { downloadStore } from '@store/download-store';
import { LitElement, html, css, nothing } from 'lit';
@@ -13,6 +10,8 @@ import { classMap } from 'lit/directives/class-map.js';
import '@components/page-header/page-header';
import { designTokens } from '../../styles/tokens.css';
import { srOnly } from '../../styles/sr-only.css';
import { albumCardStyles } from '../../styles/album-card.css';
import '../scroll-row/scroll-row.js';
import { SearchLocal, SearchLyrics, GetThumbnail, GetThumbnails, GetArtistImageURL, GetArtistImagesCachedPaths, GetExploreShelves, RecordSearchClick } from '@go/explore/service.js';
import { GetFilePathsByAlbums, GetFilePathsByRecordingMBIDs } from '@go/library/library.js';
import { EventsOn } from '@runtime/runtime';
@@ -253,7 +252,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
srOnly,
exploreLinkStyles,
contextMenuStyles,
unownedStyles,
albumCardStyles,
css`
:host {
display: block;
@@ -529,21 +528,9 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
line-height: 1.5;
}
/* ── Horizontal scroll rows ── */
.horizontal-row {
display: flex;
gap: 12px;
overflow-x: auto;
padding-bottom: 4px;
scrollbar-width: none;
}
.horizontal-row::-webkit-scrollbar {
display: none;
}
/* ── Top result cards ── */
/* ── Artist cards ── */
/* Fixed width, for the reason the album card is: a range
means two cards in one row are different sizes. */
.artist-card {
display: flex;
flex-direction: column;
@@ -552,8 +539,8 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
padding: 10px;
border-radius: 8px;
cursor: pointer;
min-width: 100px;
max-width: 120px;
width: 120px;
box-sizing: border-box;
flex-shrink: 0;
text-align: center;
transition: background 0.15s ease;
@@ -624,115 +611,6 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
font-size: var(--yj-text-xs);
}
/* ── Album cards ── */
.album-card {
display: flex;
flex-direction: column;
gap: 6px;
padding: 8px;
border-radius: 8px;
cursor: pointer;
min-width: 130px;
max-width: 150px;
flex-shrink: 0;
transition: background 0.15s ease;
}
.album-card:hover {
background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.06));
}
.album-card:active {
transform: scale(0.97);
}
.album-art-container {
width: 100%;
aspect-ratio: 1;
border-radius: 4px;
overflow: hidden;
background: linear-gradient(
135deg,
var(--yj-bg-overlay, #404040) 0%,
var(--yj-bg-surface, #282828) 100%
);
display: flex;
align-items: center;
justify-content: center;
position: relative;
}
.album-art-container img {
width: 100%;
height: 100%;
object-fit: cover;
display: block;
}
.album-art-fallback {
display: flex;
align-items: center;
justify-content: center;
width: 100%;
height: 100%;
position: absolute;
inset: 0;
}
.album-art-fallback wa-icon {
color: var(--yj-text-tertiary, #888);
font-size: 24px;
opacity: 0.5;
}
.album-title {
font-weight: 500;
color: var(--yj-text-primary, #fff);
font-size: var(--yj-text-sm);
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
}
.album-artist {
color: var(--yj-text-tertiary, #888);
font-size: var(--yj-text-xs);
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
}
.album-meta {
display: flex;
align-items: center;
justify-content: space-between;
gap: 6px;
color: var(--yj-text-tertiary, #888);
font-size: var(--yj-text-xs);
min-height: 20px;
}
.album-meta-text {
display: flex;
align-items: center;
gap: 6px;
min-width: 0;
overflow: hidden;
}
.album-meta library-status-indicator {
flex-shrink: 0;
margin-left: auto;
}
.type-badge {
background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.08));
padding: 1px 6px;
border-radius: 3px;
font-size: 10px;
white-space: nowrap;
}
/* ── Track list ── */
.track-list {
display: flex;
@@ -753,7 +631,6 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
cursor: pointer;
}
.album-card:focus-visible,
.track-item:focus-visible {
outline: 2px solid var(--yj-accent-text, #ffd43b);
outline-offset: -2px;
@@ -1371,7 +1248,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
const entity = target.kind === 'album' ? 'release-group' : 'recording';
window.open(`https://musicbrainz.org/${entity}/${target.mbid}`, '_blank', 'noopener');
openMusicBrainz(`/${entity}/${target.mbid}`);
}
private renderExploreContextMenu() {
@@ -1662,6 +1539,8 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
} catch {
// No image — leave empty string.
}
return undefined;
}),
);
@@ -2122,7 +2001,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
${subtitle
? html`<p class="section-reason">${subtitle}</p>`
: nothing}
<div class="horizontal-row">
<scroll-row>
${artists.map((a) => {
const owned = isOwned(a);
const name = a.englishName || a.name;
@@ -2171,7 +2050,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
</div>
`;
})}
</div>
</scroll-row>
</section>
`;
}
@@ -2187,7 +2066,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
${subtitle
? html`<p class="section-reason">${subtitle}</p>`
: nothing}
<div class="horizontal-row">
<scroll-row>
${releaseGroups.map((rg) => {
const artURL = this.thumbnailCache.get(rg.mbid) || '';
const year = extractYear(rg.firstReleaseDate);
@@ -2249,23 +2128,8 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
>
<wa-icon name="compact-disc"></wa-icon>
</div>
</div>
<div class="album-title" title="${rg.title}">
${rg.title}
</div>
<div class="album-artist">${creditLink(creditStore.credits(rg.mbid), rg.artistCredit, rg.artistMbid ?? '')}</div>
<div class="album-meta">
<div class="album-meta-text">
${rg.primaryType
? html`<span class="type-badge"
>${rg.primaryType}</span
>`
: nothing}
${year ? html`<span>${year}</span>` : nothing}
</div>
${badge.status === 'in-library'
? nothing
: html`<library-status-indicator
<div class="album-card-badge">
<library-status-indicator
status=${badge.status}
owned=${badge.owned}
expected=${badge.expected}
@@ -2273,12 +2137,28 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
label=${rg.title}
request-mbid=${rg.mbid}
request-artist=${rg.artistCredit ?? ''}
></library-status-indicator>`}
size="23"
></library-status-indicator>
</div>
</div>
<div class="album-title" title="${rg.title}">
${rg.title}
</div>
<div class="album-artist">${creditLink(creditStore.credits(rg.mbid), rg.artistCredit, rg.artistMbid ?? '')}</div>
<div class="album-meta">
<div class="album-meta-text">
${year ? html`<span>${year}</span>` : nothing}
${rg.primaryType
? html`<span class="type-badge"
>${rg.primaryType}</span
>`
: nothing}
</div>
</div>
</div>
`;
})}
</div>
</scroll-row>
</section>
`;
}
+5 -12
View File
@@ -12,6 +12,7 @@ import { libraryStore } from '@store/library-store';
import { EventsOn } from '@runtime/runtime';
import { Events } from '../../events';
import '@components/page-header/page-header';
import '../scroll-row/scroll-row.js';
import { designTokens } from '../../styles/tokens.css';
import { ViewLifecycleMixin } from '../../utils/view-lifecycle';
@@ -99,16 +100,6 @@ export class HomeView extends ViewLifecycleMixin(LitElement) {
color: var(--yj-text-tertiary, #888);
}
.row {
display: grid;
grid-auto-flow: column;
grid-auto-columns: 160px;
gap: 14px;
overflow-x: auto;
padding-bottom: 6px;
scrollbar-width: thin;
}
.card {
background: none;
border: none;
@@ -117,6 +108,8 @@ export class HomeView extends ViewLifecycleMixin(LitElement) {
cursor: pointer;
color: inherit;
display: block;
width: 160px;
flex-shrink: 0;
}
.art {
@@ -336,9 +329,9 @@ export class HomeView extends ViewLifecycleMixin(LitElement) {
<span class="shelf-title">${shelf.title}</span>
</div>
<p class="shelf-sub">${shelf.subtitle}</p>
<div class="row">
<scroll-row>
${(shelf.albums ?? []).map((album) => this.renderCard(album))}
</div>
</scroll-row>
</section>
`;
}
@@ -0,0 +1,213 @@
import { LitElement, css, html } from 'lit';
import { customElement, query, state } from 'lit/decorators.js';
import '@awesome.me/webawesome/dist/components/icon/icon.js';
/** How far one press moves the row — most of a screenful, not all of
* it, so the card that was at the edge stays as an anchor. */
const SCROLL_FRACTION = 0.8;
/**
* A horizontally scrolling row with arrow buttons.
*
* The shelves, the search results and (now) the artist page's
* discography and similar-artists rows are all "more than fits, scroll
* sideways". Until this existed the only way to see the rest was a
* mousewheel or a trackpad gesture, which is not an affordance — a
* mouse with no horizontal wheel simply could not reach the cards past
* the fold.
*
* It is a component rather than a rule on `.horizontal-row` for two
* reasons. The arrows are *state* — which way the row can still move —
* and that state has to be recomputed when the viewport resizes or a
* card arrives with its cover art; a stylesheet cannot do that. And
* every caller then gets the same arrows, the same reveal and the same
* keyboard labels without writing them again.
*
* **The arrows are `hidden`, not merely transparent, at the end they
* cannot move from** — a control that cannot act is worse than none,
* and an invisible one still holds a hit area and a tab stop. On a
* pointer device the pair fades in with the row's hover; where there is
* no hover they are always visible, because there is no other route to
* them there (a swipe is not an affordance a mouse-less keyboard user
* has either).
*
* The cards are light DOM children and stay in the *host's* shadow
* root, so the host's own `.album-card` / `.artist-card` styles apply
* unchanged — this component only owns the box they scroll inside.
*/
@customElement('scroll-row')
export class ScrollRow extends LitElement {
@query('.viewport') private viewport?: HTMLElement;
@state() private atStart = true;
@state() private atEnd = true;
@state() private overflowing = false;
private observer?: ResizeObserver;
static override styles = css`
:host {
display: block;
position: relative;
}
.viewport {
overflow-x: auto;
overflow-y: hidden;
scrollbar-width: none;
/* A swipe that reaches the row's end should not drag the
whole page sideways with it. */
overscroll-behavior-x: contain;
}
.viewport::-webkit-scrollbar {
display: none;
}
.track {
display: flex;
gap: 12px;
}
.arrow {
position: absolute;
top: 50%;
transform: translateY(-50%);
z-index: 2;
display: flex;
align-items: center;
justify-content: center;
width: 36px;
height: 36px;
padding: 0;
border-radius: 50%;
border: 1px solid var(--yj-border-subtle, rgba(255, 255, 255, 0.1));
background: var(--yj-bg-elevated, #343a40);
color: var(--yj-text-primary, #fff);
cursor: pointer;
opacity: 0;
transition: opacity 0.15s ease;
}
.arrow[hidden] {
display: none;
}
.arrow.prev {
left: 4px;
}
.arrow.next {
right: 4px;
}
.arrow:hover {
background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.12));
}
.arrow:focus-visible {
outline: 2px solid var(--yj-accent, #ffd43b);
outline-offset: 2px;
}
@media (hover: hover) and (pointer: fine) {
:host(:hover) .arrow,
.arrow:focus-visible {
opacity: 1;
}
}
@media not all and (hover: hover) {
.arrow {
opacity: 1;
}
}
`;
override firstUpdated(): void {
const viewport = this.viewport;
if (!viewport) return;
this.observer = new ResizeObserver(() => this.measure());
this.observer.observe(viewport);
// The track's own size is what changes when a card arrives with
// its cover art, and a ResizeObserver on the viewport alone
// never fires for that.
const track = viewport.firstElementChild;
if (track) this.observer.observe(track);
this.measure();
}
override disconnectedCallback(): void {
super.disconnectedCallback();
this.observer?.disconnect();
this.observer = undefined;
}
private measure(): void {
const viewport = this.viewport;
if (!viewport) return;
this.overflowing = viewport.scrollWidth > viewport.clientWidth + 1;
this.atStart = viewport.scrollLeft <= 1;
this.atEnd =
viewport.scrollLeft + viewport.clientWidth >=
viewport.scrollWidth - 1;
}
private onScroll = (): void => this.measure();
private scrollStep(direction: -1 | 1): void {
const viewport = this.viewport;
if (!viewport) return;
viewport.scrollBy({
left: direction * viewport.clientWidth * SCROLL_FRACTION,
behavior: 'smooth',
});
}
override render() {
const showPrev = this.overflowing && !this.atStart;
const showNext = this.overflowing && !this.atEnd;
return html`
<button
class="arrow prev"
type="button"
aria-label="Scroll left"
?hidden=${!showPrev}
@click=${() => this.scrollStep(-1)}
>
<wa-icon name="chevron-left"></wa-icon>
</button>
<div class="viewport" @scroll=${this.onScroll}>
<div class="track"><slot></slot></div>
</div>
<button
class="arrow next"
type="button"
aria-label="Scroll right"
?hidden=${!showNext}
@click=${() => this.scrollStep(1)}
>
<wa-icon name="chevron-right"></wa-icon>
</button>
`;
}
}
declare global {
interface HTMLElementTagNameMap {
'scroll-row': ScrollRow;
}
}
@@ -1,6 +1,7 @@
import { LitElement, html, css, nothing } from 'lit';
import { customElement, property } from 'lit/decorators.js';
import { designTokens } from '../../styles/tokens.css';
import '../scroll-row/scroll-row.js';
import type * as explore from '@go/explore/models.js';
import {
GetArtistImageURL,
@@ -15,7 +16,6 @@ import { albumBadgeFor, libraryStatusFor } from '../../utils/library-status';
import {
isOwned,
ownershipLabel,
unownedStyles,
type OwnableKind,
} from '../../utils/ownership';
import { completenessStore } from '../../store/completeness-store';
@@ -103,20 +103,12 @@ export class TopResultsRow extends LitElement {
static override styles = [
designTokens,
exploreLinkStyles,
unownedStyles,
css`
:host {
display: block;
margin-bottom: 16px;
}
.row {
display: flex;
gap: 12px;
overflow-x: auto;
padding-bottom: 4px;
}
.card {
flex: 0 0 auto;
width: 200px;
@@ -285,9 +277,9 @@ export class TopResultsRow extends LitElement {
return html`
<div class="section-label">Top Results</div>
<div class="row">
<scroll-row>
${this.results.map((r) => this.renderCard(r))}
</div>
</scroll-row>
`;
}
+1
View File
@@ -31,6 +31,7 @@ solid/bookmark
solid/box-open
solid/check
solid/chevron-down
solid/chevron-left
solid/chevron-right
solid/circle-check
solid/circle-exclamation
+184
View File
@@ -0,0 +1,184 @@
import { css } from 'lit';
/**
* The Explore album card, once.
*
* Two components draw one — `explore-view`'s shelves and search
* results, and `explore-artist-details`'s discography — and they had
* grown two copies of the same rules. That is how the size came apart:
* `explore-view` clamped its cards to a 130–150px range so two cards in
* one row could be different widths, and since the artwork is square
* that made them different *heights* as well. A row of covers with
* ragged bottoms is the whole complaint.
*
* So the width is a fixed `--yj-album-card-width` and the lines below
* the art each reserve their own space, which is what makes every card
* the same size no matter what a given album happens to carry —
* `album-card-size.test.ts` measures that rather than trusting it.
*
* Three rules here are the parts that changed rather than moved.
*
* **The artwork is inset in the square, not cropped to it.** The
* container was already `aspect-ratio: 1` but the image was
* `object-fit: cover`, so a non-square cover lost its edges. It is
* `contain` now and the container's own background is transparent, so
* a tall or wide cover sits in the middle of the square with the page
* showing through beside it.
*
* **The badge lives on the artwork, top-left, and only under the
* pointer.** It used to sit in the metadata line and only for the
* unowned case. It draws for every card now — an owned album's tick is
* the answer to the same question — and it is revealed by hover on a
* pointer device. Where there is no hover it is *always* visible rather
* than never, because on those devices it is the only route to its
* action: `explore-view`'s card menu carries no request item, so a
* phone with the badge hidden could not ask for an album at all.
*
* **Nothing dims an unowned card.** `unownedStyles` was removed from
* the catalog surfaces on the rule that the badge is the mark; the
* album page's *tracklist* still dims unowned rows, which is a
* different statement about a different thing.
*/
export const albumCardStyles = css`
.album-card {
width: var(--yj-album-card-width, 150px);
display: flex;
flex-direction: column;
gap: 6px;
padding: 8px;
border-radius: 8px;
box-sizing: border-box;
flex-shrink: 0;
cursor: pointer;
transition: background 0.15s ease;
}
.album-card:hover {
background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.06));
}
.album-card:active {
transform: scale(0.97);
}
.album-card:focus-visible {
outline: 2px solid var(--yj-accent-text, #ffd43b);
outline-offset: -2px;
}
.album-art-container {
position: relative;
width: 100%;
aspect-ratio: 1;
border-radius: 4px;
overflow: hidden;
background: transparent;
display: flex;
align-items: center;
justify-content: center;
}
.album-art-container img {
width: 100%;
height: 100%;
object-fit: contain;
display: block;
}
/* The placeholder is the one case that *is* a full square, so it
carries the background the container gave up. */
.album-art-fallback {
display: flex;
align-items: center;
justify-content: center;
width: 100%;
height: 100%;
position: absolute;
inset: 0;
background: linear-gradient(
135deg,
var(--yj-bg-overlay, #404040) 0%,
var(--yj-bg-surface, #282828) 100%
);
}
.album-art-fallback wa-icon {
color: var(--yj-text-tertiary, #888);
font-size: 24px;
opacity: 0.5;
}
.album-card-badge {
position: absolute;
top: 6px;
left: 6px;
z-index: 1;
display: flex;
visibility: hidden;
opacity: 0;
transition: opacity 0.15s ease, visibility 0.15s ease;
}
@media (hover: hover) and (pointer: fine) {
.album-card:hover .album-card-badge,
.album-card:focus-within .album-card-badge {
visibility: visible;
opacity: 1;
}
}
@media not all and (hover: hover) {
.album-card-badge {
visibility: visible;
opacity: 1;
}
}
.album-title {
font-weight: 500;
color: var(--yj-text-primary, #fff);
font-size: var(--yj-text-sm);
line-height: 1.3;
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
}
/* Reserved even where a surface has no artist to draw, so a card
in a row is never shorter than its neighbour. */
.album-artist {
color: var(--yj-text-tertiary, #888);
font-size: var(--yj-text-xs);
line-height: 1.3;
min-height: 1.3em;
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
}
.album-meta {
display: flex;
align-items: center;
justify-content: space-between;
gap: 6px;
color: var(--yj-text-tertiary, #888);
font-size: var(--yj-text-xs);
height: 20px;
}
.album-meta-text {
display: flex;
align-items: center;
gap: 6px;
min-width: 0;
overflow: hidden;
}
.type-badge {
background: var(--yj-bg-overlay, rgba(255, 255, 255, 0.08));
padding: 1px 6px;
border-radius: 3px;
font-size: 10px;
white-space: nowrap;
}
`;
+29
View File
@@ -0,0 +1,29 @@
/**
* Opening an external page, with the destination pinned.
*
* Every external link this app opens is a MusicBrainz entity page built
* from an MBID that came from the catalog. Constructing the URL by
* string concatenation leaves the destination to whatever is in that
* string, so this parses it against the one origin the app means and
* refuses anything else — an MBID cannot change the host, and if it
* somehow did, nothing would open.
*
* It navigates through a real anchor rather than `window.open`: the
* same top-level `_blank` navigation with `noopener`, and it keeps the
* destination an ordinary link rather than an argument to a function
* whose first parameter is a URL.
*/
const MUSICBRAINZ_ORIGIN = 'https://musicbrainz.org';
export function openMusicBrainz(path: string): void {
const url = new URL(path, MUSICBRAINZ_ORIGIN);
if (url.origin !== MUSICBRAINZ_ORIGIN) return;
const link = document.createElement('a');
link.href = url.toString();
link.target = '_blank';
link.rel = 'noopener noreferrer';
link.click();
}
+12 -1
View File
@@ -10,6 +10,16 @@
* badge as the only difference. This is that rule, written once, so
* eight surfaces cannot each keep their own version of it.
*
* **The catalog's *cards* no longer dim.** A grid of dimmed covers read
* as a page that had failed to load rather than as a page of things you
* could ask for, so on Explore the mark is the badge alone — over the
* artwork, on hover, drawn for owned and unowned alike. The album
* page's *tracklist* still dims unowned rows: that is a different
* statement ("this one is not here") about a different thing, and the
* `aria-disabled` row that cannot be played is what it is for. So
* `unownedStyles` survives for that one surface and the cards simply do
* not include it.
*
* ## Ownership is a file, and `localId` is the flag that says so
*
* The album page answers "do I own this row" with `filePaths`, a map
@@ -96,7 +106,8 @@ export function ownershipLabel(
}
/**
* The dimming, shared so it cannot drift across surfaces.
* The dimming, shared so it cannot drift across surfaces — and now
* used by exactly one of them.
*
* Two things about it are load-bearing.
*
@@ -0,0 +1,174 @@
/**
* Every album card is the same size, and its artwork is a square.
*
* The size came apart because `explore-view` clamped its cards to a
* 130–150px range, so two cards in one row could be different widths —
* and since the artwork is square, different *heights* as well. A row
* of covers with ragged bottoms is what that looks like.
*
* What makes the fix hold is that the lines below the art each reserve
* their own space (`album-card.css.ts`), so an album with no year, no
* release type or a one-character title is not shorter than its
* neighbour. This measures that rather than trusting it, because the
* next component to format a card is the way it comes back.
*
* The artwork half is the other change: the container was already
* square but the image was `object-fit: cover`, so a non-square cover
* was cropped to it. It is `contain` now, and the container has no
* background of its own, so a tall cover is inset with the page
* showing through beside it.
*/
import { beforeEach, describe, expect, it } from 'vitest';
import type { LitElement } from 'lit';
import '@components/explore-view/explore-view';
import { flush, stub, resetHarness } from '@test/support/harness';
import { fixture, shadow, shadowAll, update } from '@test/support/render';
import { completenessStore } from '@store/completeness-store';
const SEARCH = 'explore.Service.SearchLocal';
const SHELVES = 'explore.Service.GetExploreShelves';
/** A 1x1 transparent gif, so the `<img>` branch renders. */
const TINY_IMAGE =
'data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7';
/** Release groups chosen so every optional line is present on one and
* absent on another — that is what a size regression hides behind. */
const ALBUMS = [
{
mbid: 'rg-1',
title: 'A',
artistCredit: '',
artistMbid: 'ar-1',
primaryType: '',
firstReleaseDate: '',
popularity: 1,
listenerCount: 1,
secondaryTypes: [],
inLibrary: false,
localId: 0,
},
{
mbid: 'rg-2',
title: 'A Very Long Album Name That Will Certainly Be Truncated By The Card',
artistCredit: 'An Artist With A Long Name',
artistMbid: 'ar-2',
primaryType: 'Album',
firstReleaseDate: '1994-05-01',
popularity: 1,
listenerCount: 1,
secondaryTypes: [],
inLibrary: false,
localId: 0,
},
{
mbid: 'rg-3',
title: 'Three',
artistCredit: 'Another',
artistMbid: 'ar-3',
primaryType: 'EP',
firstReleaseDate: '2001-01-01',
popularity: 1,
listenerCount: 1,
secondaryTypes: [],
inLibrary: false,
localId: 0,
},
];
async function exploreWithAlbums(): Promise<LitElement> {
stub(SHELVES, { shelves: [], state: 'ready' });
stub(SEARCH, {
artists: [],
releaseGroups: ALBUMS,
recordings: [],
});
stub('explore.Service.GetThumbnails', Object.fromEntries(
ALBUMS.map((a) => [a.mbid, TINY_IMAGE]),
));
stub('explore.Service.GetThumbnail', TINY_IMAGE);
const el = await fixture<LitElement>('explore-view');
(el as unknown as { onViewActivate: () => void }).onViewActivate?.();
await update(el, {
results: { artists: [], releaseGroups: ALBUMS, recordings: [] },
});
await flush();
await el.updateComplete;
return el;
}
beforeEach(() => {
resetHarness();
stub('library.Library.GetAlbumsCompleteness', {});
completenessStore.invalidate();
});
describe('the album card size', () => {
it('is the same width and height for every card in a row', async () => {
const el = await exploreWithAlbums();
const cards = shadowAll(el, '.album-card');
expect(cards.length).toBe(ALBUMS.length);
const boxes = cards.map((c) => c.getBoundingClientRect());
// The first card is the reference; every other one must match it.
for (const box of boxes) {
expect(box.width).toBe(boxes[0]!.width);
expect(box.height).toBe(boxes[0]!.height);
}
// …and the reference is a real box, or the loop above is vacuous.
expect(boxes[0]!.width).toBeGreaterThan(0);
expect(boxes[0]!.height).toBeGreaterThan(0);
});
it('keeps the artwork square', async () => {
const el = await exploreWithAlbums();
for (const art of shadowAll(el, '.album-art-container')) {
const box = art.getBoundingClientRect();
expect(Math.round(box.width)).toBe(Math.round(box.height));
}
});
it('insets a non-square cover rather than cropping it', async () => {
const el = await exploreWithAlbums();
// Read from the parsed stylesheet rather than from a rendered
// `<img>`: the search path is what calls `loadThumbnails`, and
// setting `results` directly skips it, so there is no image to
// measure. The regression worth catching is the rule going back to
// `cover`, which is a stylesheet fact.
const rules = (el.shadowRoot?.adoptedStyleSheets ?? []).flatMap((sheet) =>
Array.from(sheet.cssRules).map((rule) => rule.cssText),
);
const art = rules.find(
(text) =>
text.startsWith('.album-art-container img') &&
text.includes('object-fit'),
);
expect(art, 'no object-fit rule for the cover image').toBeDefined();
expect(art).toContain('object-fit: contain');
});
it('draws the badge over the artwork, and not in the metadata line', async () => {
const el = await exploreWithAlbums();
const card = shadow(el, '.album-card')!;
const badge = card.querySelector('.album-art-container .album-card-badge');
expect(badge).not.toBeNull();
// The badge is positioned inside the art box, so its parent is the
// square rather than the row underneath it.
expect(badge?.parentElement?.classList.contains('album-art-container')).toBe(
true,
);
});
});
@@ -0,0 +1,168 @@
/**
* The artist page's header and its top tracks.
*
* Two cleanups, asserted together because they are one screen:
*
* - the Play/Shuffle pair became one split button ("Play" with the
* words on its title, Shuffle behind the caret), the Follow button
* moved onto the same line, and the name and listen count went up a
* size;
* - a top track's play/request affordance moved onto its artwork,
* where a hover reveals it, instead of a badge at the end of the
* row beside a row that already plays on a double-click.
*/
import { describe, expect, it, beforeEach } from 'vitest';
import type { LitElement } from 'lit';
import '@components/explore-artist-details/explore-artist-details';
import { stub, flush, emit, resetHarness } from '@test/support/harness';
import { Events } from '../../src/events';
import { fixture, shadow, shadowAll } from '@test/support/render';
const ARTIST = 'artist-0001';
const track = (name: string, localId = 0) => ({
recordingMbid: `rec-${name}`,
artistName: 'Tideline',
trackName: name,
totalListenCount: 100,
caaReleaseMbid: '',
releaseName: 'Foreshore',
releaseGroupMbid: 'rg-owned',
length: 200000,
inLibrary: localId > 0,
localId,
});
beforeEach(() => {
resetHarness();
stub('explore.Service.LookupArtist', {
mbid: ARTIST,
name: 'Tideline',
popularity: 1200,
type: 'Group',
country: 'GB',
});
stub('explore.Service.TopReleaseGroupsForArtist', []);
stub('explore.Service.TopRecordingsForArtist', [
track('Owned Song', 7),
track('Absent Song'),
]);
stub('explore.Service.SimilarArtists', []);
stub('explore.Service.PrefetchReleases', undefined);
stub('explore.Service.BrowseReleaseGroups', [
{
mbid: 'rg-owned',
title: 'Foreshore',
artistCredit: 'Tideline',
primaryType: 'Album',
inLibrary: true,
localId: 7,
},
]);
stub('library.Library.GetAlbumsCompleteness', {});
stub('download.Service.ListRequests', []);
});
async function mount(): Promise<LitElement> {
const el = await fixture<LitElement>('explore-artist-details', {
artistMBID: ARTIST,
artistName: 'Tideline',
});
await flush();
return el;
}
describe('the artist header', () => {
it('offers Play, with Shuffle behind its caret', async () => {
const el = await mount();
const play = shadow<HTMLElement>(el, '[data-testid="artist-play-library"]')!;
// The words moved to the title, which is where "Play library
// tracks" can still be read without taking the width of a button.
expect(play.textContent?.trim()).toBe('Play');
expect(play.getAttribute('title')).toBe('Play library tracks');
const menuButton = shadow(el, '[data-testid="artist-play-menu"]');
expect(menuButton).not.toBeNull();
const menu = shadow(el, '#artist-play-menu');
expect(menu?.textContent).toContain('Shuffle');
});
it('puts Follow on the same line as Play', async () => {
const el = await mount();
const actions = shadow(el, '.artist-actions')!;
expect(actions.querySelector('[data-testid="artist-play-library"]')).not.toBeNull();
const follow = actions.querySelector('[data-testid="artist-follow"]') as HTMLElement;
expect(follow).not.toBeNull();
expect(follow.textContent?.trim()).toBe('Follow');
});
it('says Following once the artist is on the request list', async () => {
const el = await mount();
// The store is a singleton and caches its list, so the change is
// announced the way the backend announces one.
stub('download.Service.ListRequests', [
{ id: 3, mbid: ARTIST, state: 'queued' },
]);
emit(Events.RequestsChanged);
await flush();
await el.updateComplete;
const follow = shadow<HTMLElement>(el, '[data-testid="artist-follow"]')!;
expect(follow.textContent?.trim()).toBe('Following');
});
it('sizes the name and the listen count above the metadata line', async () => {
const el = await mount();
const title = shadow<HTMLElement>(el, '.artist-title')!;
const listens = shadow<HTMLElement>(el, '.artist-listens')!;
const meta = shadow<HTMLElement>(el, '.artist-meta')!;
expect(listens.textContent).toContain('plays on ListenBrainz');
const titleSize = parseFloat(getComputedStyle(title).fontSize);
const listensSize = parseFloat(getComputedStyle(listens).fontSize);
const metaSize = parseFloat(getComputedStyle(meta).fontSize);
expect(titleSize).toBeGreaterThan(24);
expect(listensSize).toBeGreaterThan(metaSize);
});
});
describe('a top track’s affordance', () => {
it('plays from the artwork when it is owned', async () => {
const el = await mount();
const rows = shadowAll<HTMLElement>(el, '.track-item');
const owned = rows.find((r) => r.textContent?.includes('Owned Song'))!;
expect(owned.querySelector('.track-art-overlay .track-art-play')).not.toBeNull();
// Nothing beside the row any more.
expect(owned.querySelector(':scope > library-status-indicator')).toBeNull();
});
it('requests from the artwork when it is not', async () => {
const el = await mount();
const rows = shadowAll<HTMLElement>(el, '.track-item');
const absent = rows.find((r) => r.textContent?.includes('Absent Song'))!;
expect(
absent.querySelector('.track-art-overlay library-status-indicator'),
).not.toBeNull();
expect(absent.querySelector('.track-art-overlay .track-art-play')).toBeNull();
});
});
@@ -25,7 +25,9 @@ const ARTIST = 'artist-0001';
/** The labels of the open menu's items, trimmed. */
function menuItems(el: LitElement): string[] {
const panel = shadow(el, '.context-menu-panel');
// Scoped to the context menu: the artist page also has a Play/Shuffle
// dropdown, and its panel carries the same class.
const panel = shadow(el, '#context-menu .context-menu-panel');
if (!panel) return [];
@@ -100,7 +102,7 @@ describe('the context menu on an artist page release', () => {
await openMenuOnAlbum(el, 0);
const panel = shadow(el, '.context-menu-panel');
const panel = shadow(el, '#context-menu .context-menu-panel');
expect(panel).toBeTruthy();
// The panel is shared with the track menu, so a label that does not
+131
View File
@@ -0,0 +1,131 @@
/**
* A horizontally scrolling row can be moved without a wheel.
*
* Until this existed the only way to see the cards past the fold on the
* shelves, the search results and the artist page's discography was a
* mousewheel or a trackpad gesture — which is not an affordance. A
* mouse with no horizontal wheel simply could not reach them.
*
* What is asserted here is the state that makes the arrows honest: an
* arrow is `hidden` at the end it cannot move from, because a control
* that cannot act is worse than none, and an invisible one still holds
* a hit area and a tab stop.
*/
import { beforeEach, describe, expect, it } from 'vitest';
import type { LitElement } from 'lit';
import '@components/scroll-row/scroll-row';
import { fixture } from '@test/support/render';
/** Six 100px cards in a 320px row — comfortably overflowing. */
function content(el: Element): void {
for (let i = 0; i < 6; i += 1) {
const card = document.createElement('div');
card.style.cssText = 'flex: 0 0 100px; height: 40px';
card.textContent = String(i);
el.append(card);
}
}
function arrows(el: LitElement): { prev: HTMLButtonElement; next: HTMLButtonElement } {
const root = el.shadowRoot!;
return {
prev: root.querySelector('.arrow.prev') as HTMLButtonElement,
next: root.querySelector('.arrow.next') as HTMLButtonElement,
};
}
function viewport(el: LitElement): HTMLElement {
return el.shadowRoot!.querySelector('.viewport') as HTMLElement;
}
async function row(): Promise<LitElement> {
const el = await fixture<LitElement>('scroll-row');
el.style.display = 'block';
el.style.width = '320px';
content(el);
await el.updateComplete;
// The observer reports on a later frame than a microtask drain.
await new Promise((r) => setTimeout(r, 60));
await el.updateComplete;
return el;
}
describe('<scroll-row>', () => {
beforeEach(() => {
document.body.style.margin = '0';
});
it('draws an arrow for each direction it can still move', async () => {
const el = await row();
const { prev, next } = arrows(el);
expect(prev).not.toBeNull();
expect(next).not.toBeNull();
// At the start there is nothing behind, so only the forward arrow is
// offered.
expect(prev.hasAttribute('hidden')).toBe(true);
expect(next.hasAttribute('hidden')).toBe(false);
});
it('offers the way back once the row has moved', async () => {
const el = await row();
const vp = viewport(el);
vp.scrollLeft = 120;
vp.dispatchEvent(new Event('scroll'));
await el.updateComplete;
expect(arrows(el).prev.hasAttribute('hidden')).toBe(false);
});
it('stands the forward arrow down at the end', async () => {
const el = await row();
const vp = viewport(el);
vp.scrollLeft = vp.scrollWidth;
vp.dispatchEvent(new Event('scroll'));
await el.updateComplete;
expect(arrows(el).next.hasAttribute('hidden')).toBe(true);
expect(arrows(el).prev.hasAttribute('hidden')).toBe(false);
});
it('moves the row when the arrow is pressed', async () => {
const el = await row();
const vp = viewport(el);
expect(vp.scrollLeft).toBe(0);
arrows(el).next.click();
await expect.poll(() => vp.scrollLeft).toBeGreaterThan(0);
});
it('shows nothing to scroll when the content fits', async () => {
const el = await fixture<LitElement>('scroll-row');
el.style.cssText = 'display: block; width: 320px';
const only = document.createElement('div');
only.style.cssText = 'flex: 0 0 100px; height: 40px';
only.textContent = 'one';
el.append(only);
await el.updateComplete;
await new Promise((r) => setTimeout(r, 60));
await el.updateComplete;
await new Promise((r) => requestAnimationFrame(() => r(null)));
await el.updateComplete;
const { prev, next } = arrows(el);
expect(prev.hasAttribute('hidden')).toBe(true);
expect(next.hasAttribute('hidden')).toBe(true);
});
});
@@ -1,24 +1,28 @@
/**
* Owned is plain; unowned is what gets marked.
* The catalog's cards are not dimmed; the badge is the mark.
*
* `explore-album-details` had this right for one tracklist and nothing
* else did: Explore's cards, the top-results row and the artist page's
* three card shapes all mixed owned and unowned with a small badge as
* the only difference — and drew a green tick on the *common* case,
* which is the treatment the album page's own green ticks were removed
* for.
* The rule this replaced had every unowned card dimmed *and* badged,
* which on a shelf of mostly-unowned covers read as a page that had
* failed to load rather than a page of things you could ask for. So the
* dimming is gone from the catalog surfaces and the badge carries the
* whole statement — over the artwork, on hover, drawn for owned and
* unowned alike.
*
* What is pinned here is the rule rather than any one surface, because
* the fault this replaced was eight call sites each holding their own
* version of it:
* What is still pinned here is the half that was never about dimming:
*
* - an owned thing draws **no badge at all**;
* - an unowned one is dimmed *and* says so in its accessible name,
* because dimming is a colour and cannot be the only signal;
* - ownership is a **file** (`localId`), never the catalog's
* `inLibrary` ratchet, which is a flag that happens to agree;
* - and a partly-held album says *how* partly, which is the one thing
* a tick cannot.
* - a row that cannot be played is `aria-disabled`, while a card that
* still navigates is not;
* - a partly-held album says *how* partly, which is the one thing a
* tick cannot;
* - and an unowned thing still says so in its accessible name, because
* with the dimming gone that name is the whole signal for anyone not
* seeing the badge.
*
* The album page's *tracklist* still dims unowned rows — a different
* statement about a different thing — and is covered by
* `album-request-badge-visibility.test.ts`.
*/
import { beforeEach, describe, expect, it } from 'vitest';
import { page } from 'vitest/browser';
@@ -26,7 +30,7 @@ import { page } from 'vitest/browser';
import '@components/explore-view/explore-view';
import '@components/top-results-row/top-results-row';
import { flush, stub, resetHarness } from '@test/support/harness';
import { fixture, shadow, shadowAll, update } from '@test/support/render';
import { fixture, shadow, update } from '@test/support/render';
import { completenessStore } from '@store/completeness-store';
const SEARCH = 'explore.Service.SearchLocal';
@@ -105,27 +109,44 @@ beforeEach(() => {
// absent one — which is the point, or 87% of a grid re-asks forever.
// Two tests in one file are two sessions as far as it is concerned,
// so a stale entry from the test above would otherwise decide the
// one below. Found by writing the assertion the wrong way round.
// one below.
completenessStore.invalidate();
});
describe('an owned thing is plain', () => {
it('draws no badge on an album card it has files for', async () => {
describe('an unowned card is marked by its badge alone', () => {
it('does not dim the artwork', async () => {
const el = await exploreShowing({
releaseGroups: [album('Absent', {})],
});
const art = shadow(el, '.album-card .album-art-container')!;
// The dimming was an opacity on this box. With it gone the cover is
// at full strength, and the badge is what says the card is not
// yours.
expect(getComputedStyle(art).opacity).toBe('1');
expect(shadow(el, '.album-card library-status-indicator')).not.toBeNull();
});
it('still says so in the name the browser computes', async () => {
await exploreShowing({ releaseGroups: [album('Absent', {})] });
await expect
.element(page.getByRole('button', { name: /Absent — not in your library/ }))
.toBeInTheDocument();
});
});
describe('an owned card is plain except for its badge', () => {
it('draws the in-library badge rather than nothing', async () => {
const el = await exploreShowing({
releaseGroups: [album('Held', { localId: 7 })],
});
expect(shadowAll(el, '.album-card')).toHaveLength(1);
expect(shadow(el, '.album-card library-status-indicator')).toBeNull();
});
const badge = shadow(el, '.album-card library-status-indicator');
it('draws no badge on a track row it has a file for', async () => {
const el = await exploreShowing({
recordings: [recording('Held', { localId: 9 })],
});
expect(shadowAll(el, '.track-item')).toHaveLength(1);
expect(shadow(el, '.track-item library-status-indicator')).toBeNull();
expect(badge).not.toBeNull();
expect(badge?.getAttribute('status')).toBe('in-library');
});
it('does not dim it', async () => {
@@ -139,56 +160,6 @@ describe('an owned thing is plain', () => {
});
});
describe('an unowned thing is marked', () => {
it('dims the card and keeps its request badge', async () => {
const el = await exploreShowing({
releaseGroups: [album('Absent', {})],
});
expect(shadow(el, '.album-card')?.classList.contains('unowned')).toBe(true);
expect(shadow(el, '.album-card library-status-indicator')).not.toBeNull();
});
/**
* The name is the half of this that reaches anyone not seeing the
* dimming, so it has to be the browser's own answer — a shadow-root
* query cannot compute a name, and this repo has shipped a nameless
* control three times.
*/
it('says so in the name the browser computes', async () => {
await exploreShowing({ releaseGroups: [album('Absent', {})] });
await expect
.element(page.getByRole('button', { name: /Absent — not in your library/ }))
.toBeInTheDocument();
});
/**
* A track row is `aria-disabled` and a card is not, and the
* difference is not cosmetic: activating an unowned row does nothing
* (`onRecordingRowDblClick` returns early), while a card navigates to
* the catalog page for it, which is a perfectly good thing to do with
* something you do not own.
*/
it('marks a row that cannot be played as disabled', async () => {
const el = await exploreShowing({
recordings: [recording('Absent', {})],
});
expect(shadow(el, '.track-item')?.getAttribute('aria-disabled')).toBe(
'true',
);
});
it('leaves a card that still navigates enabled', async () => {
const el = await exploreShowing({
releaseGroups: [album('Absent', {})],
});
expect(shadow(el, '.album-card')?.getAttribute('aria-disabled')).toBeNull();
});
});
/**
* The decision this issue turned on.
*
@@ -206,7 +177,9 @@ describe('ownership is a file, not a flag', () => {
});
expect(shadow(el, '.album-card')?.classList.contains('unowned')).toBe(true);
expect(shadow(el, '.album-card library-status-indicator')).not.toBeNull();
expect(
shadow(el, '.album-card library-status-indicator')?.getAttribute('status'),
).not.toBe('in-library');
});
it('does the same for a track row', async () => {
@@ -220,6 +193,26 @@ describe('ownership is a file, not a flag', () => {
});
});
describe('a track row that cannot be played is disabled', () => {
it('marks an unowned row', async () => {
const el = await exploreShowing({
recordings: [recording('Absent', {})],
});
expect(shadow(el, '.track-item')?.getAttribute('aria-disabled')).toBe(
'true',
);
});
it('leaves a card that still navigates enabled', async () => {
const el = await exploreShowing({
releaseGroups: [album('Absent', {})],
});
expect(shadow(el, '.album-card')?.getAttribute('aria-disabled')).toBeNull();
});
});
/**
* The count, which is what `#16`'s deferred third step asked for: an
* album held 2 tracks of 10 wore the same green tick as one held whole,
@@ -248,9 +241,13 @@ describe('a partly-held album says how partly', () => {
// A partly-held album is *actionable* — it has three tracks left to
// ask for — so the badge is a button, and the name has to carry the
// action and the count. Naming it after the action alone left the
// one state the ring exists for as the one state whose name did not
// mention it.
// action and the count. The badge is revealed by the card's focus
// (`:focus-within`), and `visibility: hidden` is what takes it out
// of the accessibility tree until then, so the card is focused
// first — which is exactly the route a keyboard user takes.
shadow<HTMLElement>(el, '.album-card')?.focus();
await el.updateComplete;
await expect
.element(
page.getByRole('button', {
@@ -266,7 +263,7 @@ describe('a partly-held album says how partly', () => {
* state, and a ring drawn from its absence would mark all of it
* incomplete on no evidence. That is the rule `Known` exists for.
*/
it('says nothing when the total was never declared', async () => {
it('falls back to the plain in-library badge when the total was never declared', async () => {
stub(COMPLETENESS, {
'7': { owned: 3, expected: 0, known: false, complete: false },
});
@@ -279,7 +276,9 @@ describe('a partly-held album says how partly', () => {
await flush();
await el.updateComplete;
expect(shadow(el, '.album-card library-status-indicator')).toBeNull();
expect(
shadow(el, '.album-card library-status-indicator')?.getAttribute('status'),
).toBe('in-library');
});
it('asks about the owned albums only, in one call', async () => {
@@ -329,11 +328,13 @@ describe('the top-results row follows the same rule', () => {
query: 'held',
});
// A top-result card is a mixed bag — artist, album or track — and
// its badge is a corner mark rather than the cover overlay the
// album cards grew, so an owned one stays plain.
expect(shadow(el, '.card library-status-indicator')).toBeNull();
expect(shadow(el, '.card')?.classList.contains('unowned')).toBe(false);
});
it('dims and names something it does not', async () => {
it('names something it does not own', async () => {
const el = await fixture('top-results-row', {
results: [result('Absent', 'release_group')],
query: 'absent',
@@ -349,7 +350,7 @@ describe('the top-results row follows the same rule', () => {
/**
* An artist card has never had a badge — a discography subscription
* is the artist page's Follow button, which can say what it commits
* to — so the dimming and the name are the whole signal there.
* to — so the name is the whole signal there.
*/
it('marks an unowned artist without offering a request', async () => {
const el = await fixture('top-results-row', {