Compare commits

...
2 Commits
Author SHA1 Message Date
logan f5621bf7c5 docs: record the fourth pass, and the bugs hiding behind three findings
Build & publish Arch package / arch-package (push) Successful in 2m4s
CI / check (push) Successful in 2m41s
Search index maintenance / maintain-index (push) Successful in 8s
CI / e2e (push) Successful in 4m55s
Each of perf.p2, H-13 and the dialog naming had a second defect behind
the one named, reachable only once the first fix made the code path run.
Also records two probe failures worth more than the fixes: the a11y
snapshot cannot see a dialog's accessible name at all, and a scroll
assertion that could not fail was hiding both a bug and a false claim.
2026-08-12 15:29:59 -04:00
logan f854076d95 feat(explore): give the album page a primary action that tells the truth
H-13: no Play, no Shuffle, no Add to queue on the album header. The
reason it is not just three buttons is that explore-album-details is a
catalog page — there is no library-side album detail page at all — so
the album shown may be wholly the user's, partly theirs, or not theirs.
A Play button that plays 7 of a 40-track release under a label saying
'Play' is the page lying about what is owned, so the button says which:
'Play' when all of it is owned, 'Play 7 of 12' when some is, and no
play button at all when none is.

albumLibraryStatus() stays as it was — four claims of decreasing
confidence OR'd into one tick, the weakest firing when a single
recording matches. That is a fine answer to 'is any of this mine' and a
useless basis for a button, so ownership() counts the displayed
tracklist instead.

GetFilePathsByRecordingMBIDs is the catalog-side sibling of
GetFilePathsByAlbums: one query, paths only, grouped so the caller
keeps the tracklist's order. It is keyed on recording MBID because that
is how the backend decides a track is inLibrary, and because
MBTrack.LocalID is declared and never written by anything. The local
album id is preferred where there is one — a library-only album has no
MBIDs at all, and keying on them alone queued nothing.

The ticks also get the legend H-13 asks for. They were never unlabelled
— the indicator has carried a title and aria-label all along — but a
sighted user got a column of green circles and no key.
2026-08-12 15:28:22 -04:00
12 changed files with 1180 additions and 2 deletions
+100
View File
@@ -1583,3 +1583,103 @@ Five things worth keeping:
written and had been carried forward through every subsequent read of
that file, including two this session. A measurement in a comment
needs the same expiry as one in a plan.
## A finding creates the conditions for the next one
Plan 007 phase 5, fourth pass: naming every `wa-dialog`, drawing
`cover-grid`'s album dropdown, and giving the album page a primary
action.
The generalisation this pass adds: **a finding is a door, and what is
behind it has never been looked at.** Three of the four items here had
a second bug hiding *behind* the one the audit named, and none of them
could have been found by reading — each was only reachable once the
first fix made the code path run for the first time.
- `perf.p2` files `renderSplitGrid` as "dead code carried in the
bundle", housekeeping, delete it. It was a **missing feature** whose
data path already worked, and behind it sat 1 463 lines that had
never executed: `scroll-manager.ts` (916) and `album-dropdown.ts`
(461). Drawing it revealed that **the albums grid could not scroll
at all** — `.grid-scroll-container` is the markup `artists-view`
uses and `cover-grid` had the class with no rule for it, so 186 984
px of albums sat in a 772 px box at 5 000 albums, unreachable by
wheel, keyboard or scrollbar — which in turn revealed that the
scroll manager had spent its whole life saving and restoring a
`scrollTop` that was permanently 0. And *that* revealed the shared
context menu labelled "Album actions" on a track row, which nothing
could observe while the only menu that could open on a track was
unreachable.
- `H-13` asks for a Play button. The button needs file paths; the
obvious key is `MBTrack.LocalID`, which is declared, is in the
generated bindings, and **is never written by anything in the
backend**. Ownership is decided by recording MBID. Keying on that
produced a button that was wired, labelled correctly, clicked
cleanly and **queued nothing** — a library-only album has no
recording MBIDs, because its tracks are synthesised with
`mbid: RecordingMBID || ''`. Every component test passed. It was
caught by clicking the button in the running app and reading the
queue.
Seven more things worth keeping:
- **A count of call sites in an audit is a count as of its date.**
`a11y.md` lists five `wa-dialog`s; last pass estimated eight; there
are eleven. Three of the six added since were added by *this plan*.
The fix was written as a helper called from each host rather than a
list, so the number stopped mattering — which is the right response
to a number that drifts.
- **Reaching into another library's shadow root is acceptable when the
failure is bounded.** `name-dialog.ts` queries Web Awesome's open
shadow root, which is not API. If the structure moves, the query
misses, nothing is written, and the dialog is exactly as unnamed as
it was — no state to get wrong, nothing to throw. The alternative,
patching `WaDialog.prototype`, fixes every call site for free and
fails loudly and strangely instead. The bound is the argument, not
the tidiness.
- **`aria-labelledby` beats `aria-label` when the label can change.**
Three call sites compute their label at render time. An IDREF to the
heading the component re-renders anyway stays correct with nothing
resyncing it, so the helper can be a one-shot call.
- **A web component's shadow root is populated in its own update, not
its host's.** Naming a `wa-dialog` from the host's `firstUpdated`
finds an element with an empty shadow root and names nothing. Same
lifecycle trap as `wa-dropdown-item`'s role two passes ago, which
suggests it is not a trap so much as a rule: **never query inside a
child custom element without awaiting its `updateComplete`.**
- **The a11y snapshot cannot see a dialog's accessible name.**
`playwright-cli snapshot` prints a bare `- dialog [ref=…]` whether
the dialog is named by `aria-labelledby`, by `aria-label`, or not at
all — checked all three ways against the running app. Twenty minutes
went into "the fix did not work" before the probe was suspected.
`getByRole('dialog', {name})` and CDP's
`Accessibility.getFullAXTree` both answer, and CDP additionally
reports *where* the name came from. The e2e spec was watched failing
on a probe-disabled build before it was believed, which is the only
reason the twenty minutes did not become an hour.
- **An assertion that cannot fail will pass a bug, and the fix is to
make the probe move.** "The scroll position is preserved when the
dropdown opens" passed against a `scrollTop` of 0 both times on an
eight-album fixture. Shrinking the viewport until the grid genuinely
scrolled turned it red — and the red was **correct**:
`scrollToShowDropdown` deliberately moves the scroll to reveal the
dropdown (80 → 4, with the content *taller* after, so not clamping).
The premise was wrong, not the app. Two lessons in one: a vacuous
assertion hides a bug *and* a false claim, and the way to tell them
apart is to make the number move before deciding what it means.
- **A parameter name is not a specification.** `Queue.SetQueue`'s
`shuffleStart` does not start a shuffle: it picks a random first
track *if shuffle mode is already on*. A Shuffle button written from
the name plays track 1 and looks broken. Reading the Go was thirty
seconds.
And one on the audit's own accounting: **`H-13`'s "unexplained ✓
badges" was half-aged before it was read.** The indicator has carried a
`title` and an `aria-label` reading "Album “X” is in your library" all
along, so a hover and a screen reader were both already answered; what
was missing was a key for a sighted user scanning a column of green
circles. The same element turns out to be a `<button>` whose click
handler is a comment saying "wire this up later" and a
`stopPropagation` — 30-odd keyboard stops per page that promise an
action and perform none. Not fixed here, and recorded rather than
implied.
@@ -1945,6 +1945,129 @@ is**: a step plays three seconds and fails if they take under two,
because otherwise the failure surfaces three steps later as "the
elapsed clock is 19 s adrift" and reads as an app bug.
---
### Phase 5 — the fourth pass: the dialogs get names, and the album page gets an action
The last of item 4, in three independently landable pieces. Each was
reproduced in the running app before it was fixed, and two of the three
changed shape when it was.
- **Every `wa-dialog` has an accessible name.** Eleven call sites (not
the eight the last pass estimated, and not the five `a11y.md` lists —
six have been added since it was written), one helper,
`utils/name-dialog.ts`. It points the native `<dialog>` at the `<h2
id="title">` Web Awesome renders `label` into and never links,
falling back to `aria-label` under `without-header`. Verified through
CDP's `Accessibility.getFullAXTree`, which reports the name and that
it came from `relatedElement`.
- **`perf.p2` — the album dropdown is drawn.** `renderSplitGrid` was
not dead code but a missing feature whose data path already worked:
Enter on an album card fetched the tracks and ran the whole split
state machine, and `render()` ignored `splitMode`. It is the only
route from the albums grid to `track-details`, since a plain click
navigates to the catalog page.
- **`H-13` — the album page has a primary action.** Play / Shuffle
album / Add to queue on `explore-album-details`, labelled by how much
of the release the user owns, plus the legend the ✓ badges never had.
Backed by a new `GetFilePathsByRecordingMBIDs` binding, the third
member of the `GetFilePathsBy…` family.
#### Where the plan was wrong — the fourth pass
Nine things, and three of them are findings that had a second bug
hiding behind them:
- **`perf.p2` is filed as housekeeping and is a two-bug feature.**
"Dead code carried in the bundle" is `renderSplitGrid` plus
`scroll-manager.ts` (916 lines) and `album-dropdown.ts` (461) — 1 463
lines that had never executed. Enabling the render exposed both of
the others.
- **The albums grid could not scroll at all.**
`.grid-scroll-container` is the same markup `artists-view` and
`genres-view` use, and `cover-grid` had the class with **no rule for
it**, so the container grew to its full content height inside an
`overflow: hidden` host: 186 984 px of albums in a 772 px box at
5 000 albums, unreachable by wheel, keyboard or scrollbar. Invisible
on the eight-album fixture, which is why nothing had ever caught it —
and it is the element the scroll manager saves and restores, so that
machinery had been aiming at a `scrollTop` that was permanently 0.
With a real scroller it works as designed (2891 preserved exactly
across an expand at 5 000 albums).
- **The shared context menu was labelled "Album actions"
unconditionally** — including on a track row, which nothing could
observe while the only menu that could open on one was unreachable. A
finding creates the conditions for the next one.
- **`H-13`'s "unexplained ✓ badges" has half aged.**
`library-status-indicator` carries a `title` *and* an `aria-label`
reading "Album “X” is in your library", so a hover and a screen
reader both get a full sentence. What was missing was a key for a
sighted user scanning a column of green circles. Also worth knowing:
it is a `<button>` whose click handler is a comment saying "wire this
up later" and a `stopPropagation` — a badge in a button's clothes.
- **`albumLibraryStatus()` is four claims OR'd into one tick**, the
weakest of which fires when *one* recording of a forty-track release
matches. Right for a badge, useless for a button — which is why the
header counts the tracklist instead of reading the status.
- **The obvious key for "play what I own" does not exist.**
`MBTrack.LocalID` is declared, is in the generated bindings, and
**nothing in the backend ever writes it**. Ownership is decided by
recording MBID (`markReleasesInLibrary` → `CheckMBIDs`), so that is
what the new binding is keyed on.
- **…and keying on it alone shipped a Play button that queued
nothing.** A library-only album has no recording MBIDs at all — its
tracks are synthesised with `mbid: RecordingMBID || ''` — so on the
fixture library the button was wired, labelled correctly, clicked
cleanly and queued 0 tracks. Every component test passed. Caught by
clicking it in the running app and reading the queue.
- **`shuffleStart` does not start a shuffle.** `Queue.SetQueue`'s third
argument picks a random first track *when shuffle mode is already
on*, so a Shuffle button has to set the mode first. Reading the Go
rather than the parameter name was the difference between a working
button and one that plays track 1.
- **"Shuffle" is still two controls with one name.** The first pass hit
this on Home; the album header would have hit it again, since the
transport's shuffle mode is on screen whenever this page is. It is
"Shuffle album".
And two about the probes rather than the findings:
- **The a11y snapshot cannot see a dialog's name.**
`playwright-cli snapshot` prints `- dialog [ref=…]` whether the
dialog is named by `aria-labelledby`, by `aria-label`, or not at all
— checked all three ways. A snapshot read as the oracle here reports
failure on a working build. `getByRole('dialog', {name})` and CDP
both answer correctly, and the e2e spec was watched failing on a
probe-disabled build before it was believed.
- **An e2e assertion about scroll position was vacuous and said so
under pressure.** "Wherever the scroll was, it stays" passed against
a `scrollTop` of 0 both times on an eight-album fixture. Shrinking
the viewport until the grid actually scrolled turned it red — and
the red was *correct*: `scrollToShowDropdown` deliberately moves the
scroll to reveal the dropdown (80 → 4, with the content taller
after, so not clamping). The premise was wrong, not the app; the
assertion is now "the dropdown is on screen".
#### Not done, and still worth doing (after the fourth pass)
Item 4 is complete. What remains from the inherited list is
`tracklist.delete`, which still needs a "remove from library" that does
not exist and a decision about what it removes.
Two things this pass found and did not fix:
- **`library-status-indicator` is a button that does nothing.** Every
tick and every "add to library" affordance in Explore is a `<button>`
whose handler stops propagation and returns. It should be a
non-interactive `role="img"` with its existing label until the
download-client integration it is waiting for exists — as written it
is a keyboard stop that promises an action on 30-odd elements per
page.
- **The split grid's roving tab stop was not re-examined.** The
dropdown path renders two virtualizers where there was one;
`roving-grid` is attached to the scroll container and keeps working,
but nobody has checked what Home/End mean across a split.
## Phase 6 — Explore starts the conversation
The only phase that adds rather than repairs.
+29
View File
@@ -544,6 +544,35 @@ is therefore **reported at runtime** to `window.__yjIconMisses` and
drawn as a fallback — an e2e sweep asserts there are none — since a
missing icon used to be impossible, the CDN having had everything.
**An album page says how much of the album is yours.**
`explore-album-details` is a *catalog* page and there is no
library-side album detail page at all, so the album on it may be
wholly the user's, partly theirs, or not theirs — and its primary
action has to mean the same thing in each case. It says which: **Play**
when the whole release is owned, **Play 7 of 12** when some of it is,
and **no play button at all** when none is, because a Play button that
plays nothing (or seven tracks of forty) is worse than none.
`albumLibraryStatus()` is deliberately *not* what decides that. It is
four claims of decreasing confidence OR'd into one tick — a local album
id, the backend's cross-reference, a cached MBID match, and finally
*any single track* marked `inLibrary` — which is a fine answer to "is
any of this mine" and a useless basis for a button. `ownership()`
counts the displayed tracklist instead, whose `inLibrary` flags the
backend sets per recording MBID.
**And the key it plays by is not the key it looks owned by.** The local
album id is used wherever there is one, because a library-only album
has *no* recording MBIDs — its tracks are synthesised from
`GetAlbumTracks` with `mbid: RecordingMBID || ''` — so an MBID-keyed
lookup on an untagged library resolves to nothing and Play queues
nothing while looking entirely correct.
`GetFilePathsByRecordingMBIDs` is the catalog-only fallback and the
third member of the `GetFilePathsBy…` family: one query, paths only,
grouped so the caller keeps the tracklist's order. It exists rather
than a lookup by track id because **`MBTrack.LocalID` is declared and
nothing in the backend ever writes it**.
**Ask for what the caller uses, once.** "Play this artist" resolved
file paths with one `GetAlbumTracks` per album, sequentially, and every
one of the four sites doing that asked for whole track rows to read
@@ -318,3 +318,28 @@ JOIN audio_files af ON af.recording_id = r.id
WHERE rgr.release_group_id IN (sqlc.slice('release_group_ids'))
AND af.library_id = ?
ORDER BY rgr.disc_number, rgr.track_number;
-- Same shape again, keyed on recording MBID, for the catalog side.
-- An Explore album page knows which of its tracks the user owns only
-- as a set of recording MBIDs -- that is exactly how the backend
-- decides `inLibrary` (markReleasesInLibrary -> CheckMBIDs) -- and
-- MBTrack.LocalID is declared but never written by anything, so there
-- is no id to ask by. Grouped by MBID because a recording can have
-- more than one file (the duplicate fixtures are precisely that) and
-- because the caller owns the order: the tracklist's, not the
-- database's.
-- name: GetFilePathsByRecordingMBIDs :many
SELECT r.mbid AS recording_mbid, af.file_path
FROM recordings r
JOIN audio_files af ON af.recording_id = r.id
WHERE r.mbid IN (sqlc.slice('mbids'))
ORDER BY af.file_path;
-- name: GetFilePathsByRecordingMBIDsByLibrary :many
SELECT r.mbid AS recording_mbid, af.file_path
FROM recordings r
JOIN audio_files af ON af.recording_id = r.id
WHERE r.mbid IN (sqlc.slice('mbids'))
AND af.library_id = ?
ORDER BY af.file_path;
@@ -907,6 +907,115 @@ func (q *Queries) GetAudioFilesNeedingMetadata(ctx context.Context) ([]AudioFile
return items, nil
}
const getFilePathsByRecordingMBIDs = `-- name: GetFilePathsByRecordingMBIDs :many
SELECT r.mbid AS recording_mbid, af.file_path
FROM recordings r
JOIN audio_files af ON af.recording_id = r.id
WHERE r.mbid IN (/*SLICE:mbids*/?)
ORDER BY af.file_path
`
type GetFilePathsByRecordingMBIDsRow struct {
RecordingMbid sql.NullString
FilePath string
}
// Same shape again, keyed on recording MBID, for the catalog side.
// An Explore album page knows which of its tracks the user owns only
// as a set of recording MBIDs -- that is exactly how the backend
// decides `inLibrary` (markReleasesInLibrary -> CheckMBIDs) -- and
// MBTrack.LocalID is declared but never written by anything, so there
// is no id to ask by. Grouped by MBID because a recording can have
// more than one file (the duplicate fixtures are precisely that) and
// because the caller owns the order: the tracklist's, not the
// database's.
func (q *Queries) GetFilePathsByRecordingMBIDs(ctx context.Context, mbids []sql.NullString) ([]GetFilePathsByRecordingMBIDsRow, error) {
query := getFilePathsByRecordingMBIDs
var queryParams []interface{}
if len(mbids) > 0 {
for _, v := range mbids {
queryParams = append(queryParams, v)
}
query = strings.Replace(query, "/*SLICE:mbids*/?", strings.Repeat(",?", len(mbids))[1:], 1)
} else {
query = strings.Replace(query, "/*SLICE:mbids*/?", "NULL", 1)
}
rows, err := q.db.QueryContext(ctx, query, queryParams...)
if err != nil {
return nil, err
}
defer rows.Close()
var items []GetFilePathsByRecordingMBIDsRow
for rows.Next() {
var i GetFilePathsByRecordingMBIDsRow
if err := rows.Scan(&i.RecordingMbid, &i.FilePath); err != nil {
return nil, err
}
items = append(items, i)
}
if err := rows.Close(); err != nil {
return nil, err
}
if err := rows.Err(); err != nil {
return nil, err
}
return items, nil
}
const getFilePathsByRecordingMBIDsByLibrary = `-- name: GetFilePathsByRecordingMBIDsByLibrary :many
SELECT r.mbid AS recording_mbid, af.file_path
FROM recordings r
JOIN audio_files af ON af.recording_id = r.id
WHERE r.mbid IN (/*SLICE:mbids*/?)
AND af.library_id = ?
ORDER BY af.file_path
`
type GetFilePathsByRecordingMBIDsByLibraryParams struct {
Mbids []sql.NullString
LibraryID int64
}
type GetFilePathsByRecordingMBIDsByLibraryRow struct {
RecordingMbid sql.NullString
FilePath string
}
func (q *Queries) GetFilePathsByRecordingMBIDsByLibrary(ctx context.Context, arg GetFilePathsByRecordingMBIDsByLibraryParams) ([]GetFilePathsByRecordingMBIDsByLibraryRow, error) {
query := getFilePathsByRecordingMBIDsByLibrary
var queryParams []interface{}
if len(arg.Mbids) > 0 {
for _, v := range arg.Mbids {
queryParams = append(queryParams, v)
}
query = strings.Replace(query, "/*SLICE:mbids*/?", strings.Repeat(",?", len(arg.Mbids))[1:], 1)
} else {
query = strings.Replace(query, "/*SLICE:mbids*/?", "NULL", 1)
}
queryParams = append(queryParams, arg.LibraryID)
rows, err := q.db.QueryContext(ctx, query, queryParams...)
if err != nil {
return nil, err
}
defer rows.Close()
var items []GetFilePathsByRecordingMBIDsByLibraryRow
for rows.Next() {
var i GetFilePathsByRecordingMBIDsByLibraryRow
if err := rows.Scan(&i.RecordingMbid, &i.FilePath); err != nil {
return nil, err
}
items = append(items, i)
}
if err := rows.Close(); err != nil {
return nil, err
}
if err := rows.Err(); err != nil {
return nil, err
}
return items, nil
}
const getFilePathsByReleaseGroups = `-- name: GetFilePathsByReleaseGroups :many
SELECT rgr.release_group_id, af.file_path
+101
View File
@@ -229,3 +229,104 @@ func TestGetFilePathsByGenres_Empty(t *testing.T) {
t.Errorf("got %v, want empty", got)
}
}
// seedRecordingMBIDs stamps recording MBIDs onto the tracks seeded by
// seedAlbumsAndGenres, in the shape the catalog side actually meets: two
// tracks tagged, one deliberately left untagged, and one MBID carried by
// two files in different libraries — which is what a duplicate is.
func seedRecordingMBIDs(t *testing.T, lib *Library) (tagged, shared string) {
t.Helper()
ctx := lib.ctx
q := lib.db.Queries
tagged = "11111111-1111-1111-1111-111111111111"
shared = "22222222-2222-2222-2222-222222222222"
byPath := map[string]string{
"/music/a1.mp3": tagged,
"/music/a2.mp3": shared,
"/other/b1.mp3": shared,
}
files, err := q.GetAllAudioFiles(ctx)
if err != nil {
t.Fatalf("get audio files: %v", err)
}
for _, f := range files {
mbid, ok := byPath[f.FilePath]
if !ok {
continue
}
if err := q.SetRecordingMBID(ctx, sqlcgen.SetRecordingMBIDParams{
Mbid: sql.NullString{String: mbid, Valid: true},
ID: f.RecordingID,
}); err != nil {
t.Fatalf("set recording mbid: %v", err)
}
}
return tagged, shared
}
// The catalog side of the same finding: an Explore album page knows what
// the user owns only as recording MBIDs, so this is the lookup that
// turns "you own 7 of these 12" into something playable.
func TestGetFilePathsByRecordingMBIDs(t *testing.T) {
t.Parallel()
lib, _ := setupTestLibrary(t)
_, libraryID := seedAlbumsAndGenres(t, lib)
tagged, shared := seedRecordingMBIDs(t, lib)
got, err := lib.GetFilePathsByRecordingMBIDs([]string{tagged, shared}, 0)
if err != nil {
t.Fatalf("GetFilePathsByRecordingMBIDs: %v", err)
}
if len(got[tagged]) != 1 || got[tagged][0] != "/music/a1.mp3" {
t.Errorf("tagged recording = %v, want [/music/a1.mp3]", got[tagged])
}
// One recording, two files: grouping is what keeps that visible.
// A flattened result could not say which was which.
if len(got[shared]) != 2 {
t.Errorf("shared recording = %v, want two paths", got[shared])
}
// Scoping drops the copy in the other library, and nothing else.
scoped, err := lib.GetFilePathsByRecordingMBIDs([]string{tagged, shared}, libraryID)
if err != nil {
t.Fatalf("GetFilePathsByRecordingMBIDs scoped: %v", err)
}
if len(scoped[shared]) != 1 || scoped[shared][0] != "/music/a2.mp3" {
t.Errorf("scoped shared = %v, want [/music/a2.mp3]", scoped[shared])
}
}
// An empty MBID matches every untagged recording in the library, which
// is the opposite of the question being asked — so an unknown track must
// contribute nothing rather than everything.
func TestGetFilePathsByRecordingMBIDs_IgnoresEmpty(t *testing.T) {
t.Parallel()
lib, _ := setupTestLibrary(t)
seedAlbumsAndGenres(t, lib)
seedRecordingMBIDs(t, lib)
got, err := lib.GetFilePathsByRecordingMBIDs([]string{"", ""}, 0)
if err != nil {
t.Fatalf("GetFilePathsByRecordingMBIDs: %v", err)
}
if len(got) != 0 {
t.Errorf("empty MBIDs matched %d recordings, want none", len(got))
}
if _, err := lib.GetFilePathsByRecordingMBIDs(nil, 0); err != nil {
t.Fatalf("nil MBIDs: %v", err)
}
}
+114
View File
@@ -1204,3 +1204,117 @@ func (l *Library) GetFilePathsByGenres(
return paths, nil
}
// GetFilePathsByRecordingMBIDs returns the file paths of every track
// whose recording MBID is in mbids, grouped by MBID.
//
// This is the catalog side of GetFilePathsByAlbums. An Explore album
// page knows what the user owns as a set of recording MBIDs and nothing
// else: that is exactly how the backend decides a track's InLibrary
// flag (markReleasesInLibrary → CheckMBIDs), and MBTrack.LocalID is a
// declared field that nothing writes, so there is no id to ask by.
//
// Grouped rather than flattened for the same two reasons as its
// siblings — the caller owns the order (the tracklist's, not the
// database's), and one recording can have more than one file, which is
// what this app's duplicate detection exists for.
//
// A library id of 0 means "every library".
func (l *Library) GetFilePathsByRecordingMBIDs(
mbids []string, libraryID int64,
) (map[string][]string, error) {
paths := make(map[string][]string, len(mbids))
if len(mbids) == 0 {
return paths, nil
}
// recordings.mbid is nullable, so sqlc asks for NullStrings. An
// empty MBID would match every untagged recording in the library,
// which is the opposite of the question, so those are dropped here
// rather than passed through as NULL.
keys := make([]sql.NullString, 0, len(mbids))
for _, mbid := range mbids {
if mbid == "" {
continue
}
keys = append(keys, sql.NullString{String: mbid, Valid: true})
}
if len(keys) == 0 {
return paths, nil
}
rows, err := l.filePathRowsByMBID(keys, libraryID)
if err != nil {
return nil, err
}
for _, row := range rows {
if !row.mbid.Valid {
continue
}
paths[row.mbid.String] = append(paths[row.mbid.String], row.path)
}
return paths, nil
}
// filePathRowsByMBID runs the scoped or unscoped query behind
// GetFilePathsByRecordingMBIDs and flattens the two row types into one.
func (l *Library) filePathRowsByMBID(
keys []sql.NullString, libraryID int64,
) ([]mbidFilePath, error) {
if libraryID > 0 {
rows, err := l.db.ReadQueries.GetFilePathsByRecordingMBIDsByLibrary(
l.ctx, sqlcgen.GetFilePathsByRecordingMBIDsByLibraryParams{
Mbids: keys,
LibraryID: libraryID,
},
)
if err != nil {
l.logger.Error(
"could not retrieve recording file paths for library",
"recordings", len(keys),
"libraryID", libraryID,
"error", err,
)
return nil, fmt.Errorf("could not get recording file paths: %w", err)
}
out := make([]mbidFilePath, 0, len(rows))
for _, row := range rows {
out = append(out, mbidFilePath{mbid: row.RecordingMbid, path: row.FilePath})
}
return out, nil
}
rows, err := l.db.ReadQueries.GetFilePathsByRecordingMBIDs(l.ctx, keys)
if err != nil {
l.logger.Error(
"could not retrieve recording file paths",
"recordings", len(keys),
"error", err,
)
return nil, fmt.Errorf("could not get recording file paths: %w", err)
}
out := make([]mbidFilePath, 0, len(rows))
for _, row := range rows {
out = append(out, mbidFilePath{mbid: row.RecordingMbid, path: row.FilePath})
}
return out, nil
}
// mbidFilePath is one row of either GetFilePathsByRecordingMBIDs query.
type mbidFilePath struct {
mbid sql.NullString
path string
}
+119
View File
@@ -0,0 +1,119 @@
import { test, expect, callBinding } from '../support/fixtures.js';
import type { Page } from '@playwright/test';
/**
* Plan 007 phase 5: `H-13` — the album page can be played from.
*
* The page is `explore-album-details`; there is no library-side album
* detail page at all, so this catalog page is where a Play button has
* to live, and what it can honestly claim depends on how much of the
* album the user owns.
*
* What this tier adds over the component tests is that the paths
* resolve to something the queue accepts. The first version of this
* feature keyed the lookup on recording MBIDs — which is how the
* backend decides a track is `inLibrary` — and a library-only album has
* none, so Play was wired, labelled correctly, clicked cleanly and
* queued **nothing**. Every component test still passed.
*/
test.describe('playing an album from its page', () => {
test.beforeEach(async ({ app }) => {
await openFirstAlbum(app);
});
test.afterEach(async ({ app }) => {
// The suite shares one backend process in file order, and a queue
// left full is state the next spec did not ask for.
await callBinding(app, 'queue.Queue.Clear', []);
await app.getByTestId('nav-tracks').click();
});
test('Play queues what the user owns of it', async ({ app }) => {
const play = app
.locator('explore-album-details')
.locator('[data-testid="album-play"]');
// The fixture library is untagged, so this album is wholly local
// and the button carries no count.
await expect(play).toContainText('Play');
await play.click();
await expect.poll(() => queueLength(app)).toBeGreaterThan(0);
});
test('Add to queue appends rather than replacing', async ({ app }) => {
const details = app.locator('explore-album-details');
await details.locator('[data-testid="album-play"]').click();
await expect.poll(() => queueLength(app)).toBeGreaterThan(0);
const before = await queueLength(app);
await details.locator('[data-testid="album-queue"]').click();
await expect.poll(() => queueLength(app)).toBe(before * 2);
});
test('the ticks against the tracks have a legend', async ({ app }) => {
// `H-13` calls them unexplained. They were never *unlabelled* — the
// indicator has carried a title and an aria-label reading
// "Track “X” is in your library" all along — but a sighted user
// scanning the page got a column of green circles and no key.
await expect(
app.locator('explore-album-details').locator('.tracklist-legend'),
).toContainText('in your library');
});
});
/** Albums → click the second card, which navigates to the album page. */
async function openFirstAlbum(app: Page): Promise<void> {
await app.getByTestId('nav-albums').click();
await expect(app.getByTestId('main-content')).toHaveAttribute(
'data-active-view',
'albums',
);
// The cards come from a virtualizer, so they are not there when the
// view is: a click dispatched into an empty grid hits nothing and
// silently leaves the app on Albums, which reads as a broken
// navigation rather than a race.
await expect.poll(() => cardCount(app)).toBeGreaterThan(1);
// A plain click on a card navigates here; Enter expands the dropdown
// instead. Dispatched rather than clicked because the card lives in a
// virtualizer inside a shadow root.
await app.evaluate(() => {
document
.querySelector('cover-grid')
?.shadowRoot?.querySelectorAll('.album-card')[1]
?.dispatchEvent(
new MouseEvent('click', { bubbles: true, composed: true }),
);
});
await expect(app.getByTestId('main-content')).toHaveAttribute(
'data-active-view',
'explore-album-details',
);
await expect(
app.locator('explore-album-details').locator('[data-testid="album-play"]'),
).toBeVisible();
}
async function cardCount(app: Page): Promise<number> {
return app.evaluate(
() =>
document
.querySelector('cover-grid')
?.shadowRoot?.querySelectorAll('.album-card').length ?? 0,
);
}
async function queueLength(app: Page): Promise<number> {
const state = (await callBinding(app, 'queue.Queue.GetState', [])) as {
tracks?: unknown[];
};
return state?.tracks?.length ?? 0;
}
@@ -6,7 +6,11 @@ import {
BrowseReleases,
GetThumbnail,
} from '@go/explore/Service';
import { GetAlbumTracks } from '@go/library/Library';
import {
GetAlbumTracks,
GetFilePathsByAlbums,
GetFilePathsByRecordingMBIDs,
} from '@go/library/Library';
import { library } from '@go/models';
import type { download, explore } from '@go/models';
type MBReleaseGroup = explore.MBReleaseGroup;
@@ -25,6 +29,17 @@ import type { CatalogScope } from '../catalog-scope-notice/catalog-scope-notice.
import '@awesome.me/webawesome/dist/components/button/button.js';
import '../download-picker/download-picker';
import { downloadStore } from '../../store/download-store';
import { queueStore } from '../../store/queue-store';
import { notificationStore } from '../../store/notification-store';
import '../notifications/inline-notice';
/**
* The region the album header's own failures are rendered in.
*
* "Inline" says *not global*, not *where* — so the region is named
* once, here, rather than spelled at each call site.
*/
export const ExploreAlbumRegion = 'explore-album';
/* ── Utility functions (duplicated per Knowledge Pattern #9 — no cross-component imports) ── */
@@ -269,6 +284,26 @@ export class ExploreAlbumDetails extends LitElement {
flex-shrink: 0;
}
.album-actions {
display: flex;
flex-wrap: wrap;
align-items: center;
gap: 8px;
margin-top: 8px;
}
/*
* The sentence under a partial Play button. It repeats the
* count on the button on purpose: the button has to be
* short and the claim has to be unambiguous, and "Play 7 of
* 12" alone does not say whether the other five are missing
* or merely unselected.
*/
.album-owned-note {
font-size: var(--yj-text-sm);
color: var(--yj-text-secondary, #aaa);
}
.album-artist {
font-size: var(--yj-text-lg);
color: var(--yj-text-secondary, #b3b3b3);
@@ -301,6 +336,17 @@ export class ExploreAlbumDetails extends LitElement {
}
/* ── Section headers ── */
.tracklist-legend {
display: inline-flex;
align-items: center;
gap: 4px;
margin-left: 10px;
font-weight: 400;
text-transform: none;
letter-spacing: 0;
color: var(--yj-text-tertiary, #888);
}
.section-header {
font-size: 11px;
font-weight: 600;
@@ -1399,6 +1445,13 @@ export class ExploreAlbumDetails extends LitElement {
* a track marked inLibrary where releaseGroup may be null)
* - else → not owned
*
* Four different claims of decreasing confidence, OR'd together and
* reported as one tick — the last of which fires when a *single*
* recording of a forty-track release matches. `ownership()` is the
* honest version of the same question and is what the header's
* actions key off; this stays as it was, because the indicator's
* job is "is any of this yours" and that is what it answers.
*
* No queued state for now — that's reserved for future
* download-client integration.
*/
@@ -1427,6 +1480,56 @@ export class ExploreAlbumDetails extends LitElement {
return 'not-in-library';
}
/**
* How much of the shown release the user actually has.
*
* The tick beside the title is a yes/no answer to "is any of this
* mine", and four of its five branches can be true when one track
* of forty matches. That is fine for a badge and useless for a
* button: "Play" that plays one track of a forty-track release is
* worse than no Play button, so the header asks this instead.
*
* It is counted off the *tracklist being displayed*, which is the
* one thing on this page that is not an inference — each track's
* `inLibrary` is set by the backend from its recording MBID
* (`markReleasesInLibrary`), the same key the file paths are
* fetched by.
*/
private ownership(): { owned: number; total: number } {
const tracks = this.currentVersion()?.tracks ?? [];
return {
owned: tracks.filter((t) => t.inLibrary).length,
total: tracks.length,
};
}
/**
* Say what the green tick against a track means.
*
* `H-13` calls the ticks "unexplained". Half of that has aged: the
* indicator carries a `title` *and* an `aria-label` reading
* "Track \u201cX\u201d is in your library", so a screen reader and a hover
* both get a full sentence. What a sighted user scanning the page
* gets is a column of green circles and no key, which is what this
* is — rendered only when at least one track is actually ticked, so
* it never explains a symbol that is not on screen.
*/
private renderTracklistLegend() {
const { owned } = this.ownership();
if (owned === 0) return nothing;
return html`<span class="tracklist-legend">
<library-status-indicator
status="in-library"
entity-type="track"
size="14"
></library-status-indicator>
in your library
</span>`;
}
/**
* Check whether the selected release's tracklist contains any track
* with discNumber > 1, indicating a multi-disc release.
@@ -1542,6 +1645,7 @@ export class ExploreAlbumDetails extends LitElement {
></library-status-indicator>
</h1>
${this.renderAlbumMeta()}
${this.renderPlayActions()}
${this.renderDownloadAction()}
</div>
</div>
@@ -1549,6 +1653,186 @@ export class ExploreAlbumDetails extends LitElement {
`;
}
/**
* The primary action, and the sentence that says what it will do.
*
* `H-13`: the album page had no Play, no Shuffle and no Add to
* queue. What it could not have is a Play button that means the
* same thing in every case — this is a *catalog* page, and the
* album on it may be entirely yours, partly yours, or not yours at
* all. So the button says which:
*
* - all of it → "Play", and the count is in the meta line
* - some of it → "Play 7 of 12", because playing seven
* tracks under a button that says "Play" is
* the page lying about what you own
* - none of it → no play button at all; the download and
* want actions below are the whole answer
*
* The count is the tracklist's own `inLibrary` flags, which the
* backend sets from each recording's MBID — the same key
* `GetFilePathsByRecordingMBIDs` resolves the files by, so the
* number on the button is the number of tracks that will play.
*/
private renderPlayActions() {
const { owned, total } = this.ownership();
if (owned === 0 || total === 0) return nothing;
const partial = owned < total;
const playLabel = partial ? `Play ${owned} of ${total}` : 'Play';
return html`
<div class="album-actions">
<wa-button
size="small"
appearance="filled"
data-testid="album-play"
@click=${() => void this.playOwned(false)}
>
<wa-icon slot="start" name="play"></wa-icon>
${playLabel}
</wa-button>
<wa-button
size="small"
appearance="outlined"
data-testid="album-shuffle"
@click=${() => void this.playOwned(true)}
>
<wa-icon slot="start" name="shuffle"></wa-icon>
Shuffle album
</wa-button>
<wa-button
size="small"
appearance="outlined"
data-testid="album-queue"
@click=${() => void this.queueOwned()}
>
<wa-icon slot="start" name="list"></wa-icon>
Add to queue
</wa-button>
${partial
? html`<span class="album-owned-note">
You have ${owned} of these ${total} tracks.
</span>`
: nothing}
</div>
<inline-notice
region=${ExploreAlbumRegion}
testid="album-action-message"
></inline-notice>
`;
}
/**
* File paths for the tracks of this release the user actually owns,
* in the tracklist's order.
*
* One call. The obvious alternative — ask for the album's tracks
* and read `FilePath` off them — is the shape `perf.m2` was about,
* and it is not even available here: this page's model is
* `MBTrack`, which carries a recording MBID and a `localId` that
* nothing in the backend ever writes.
*/
private async ownedFilePaths(): Promise<string[]> {
const libraryID = libraryStore.getSelectedLibraryId() ?? 0;
// The local album id is the better key whenever there is one:
// it needs no MBIDs at all, and a library-only album has none —
// its tracks are synthesised from `GetAlbumTracks` with
// `mbid: RecordingMBID || ''`, so an untagged library resolves
// to an empty set and the Play button silently does nothing.
// That is exactly what the first version of this did.
if (this.localAlbumId > 0) {
const byAlbum = await GetFilePathsByAlbums(
[this.localAlbumId],
libraryID,
);
return byAlbum[this.localAlbumId] ?? [];
}
// Catalog-only: the page knows what is owned as recording MBIDs
// and nothing else — which is how the backend decided each
// track's `inLibrary` in the first place.
const tracks = this.currentVersion()?.tracks ?? [];
const mbids = tracks
.filter((t) => t.inLibrary && t.mbid)
.map((t) => t.mbid);
if (mbids.length === 0) return [];
const byMBID = await GetFilePathsByRecordingMBIDs(mbids, libraryID);
// Walked in tracklist order rather than flattened, because the
// grouping is what lets the caller keep its own order. A
// recording with more than one file is a duplicate; play the
// first and leave the rest to the feature that exists for them.
const paths: string[] = [];
for (const mbid of mbids) {
const first = byMBID[mbid]?.[0];
if (first) paths.push(first);
}
return paths;
}
/** Play what the user owns of this release, optionally shuffled. */
private async playOwned(shuffle: boolean): Promise<void> {
try {
const paths = await this.ownedFilePaths();
if (paths.length === 0) {
notificationStore.inline(ExploreAlbumRegion, {
text: 'None of these tracks could be found in your library.',
});
return;
}
// `shuffleStart` only picks a random first track when
// shuffle mode is *already* on — it does not turn it on —
// so the mode has to be set before the queue, not after.
if (shuffle && !queueStore.getState().shuffleMode) {
queueStore.toggleShuffle();
}
queueStore.setQueue(paths, 0, shuffle);
} catch (error) {
console.error('Could not play album:', error);
notificationStore.inline(ExploreAlbumRegion, {
text: describeError(error, 'Could not play this album.'),
});
}
}
/** Append what the user owns of this release to the queue. */
private async queueOwned(): Promise<void> {
try {
const paths = await this.ownedFilePaths();
if (paths.length === 0) {
notificationStore.inline(ExploreAlbumRegion, {
text: 'None of these tracks could be found in your library.',
});
return;
}
queueStore.addTracksToQueue(paths);
} catch (error) {
console.error('Could not queue album:', error);
notificationStore.inline(ExploreAlbumRegion, {
text: describeError(
error,
'Could not add this album to the queue.',
),
});
}
}
/**
* Offers to acquire the album, but only when the user has actually
* connected a download client and does not already own it. Showing
@@ -1959,7 +2243,9 @@ export class ExploreAlbumDetails extends LitElement {
return html`
<section>
<h3 class="section-header">Tracklist</h3>
<h3 class="section-header">
Tracklist ${this.renderTracklistLegend()}
</h3>
<div class="tracklist">
${discNumbers.map((discNum) => {
const discTracks = discMap.get(discNum) ?? [];
@@ -0,0 +1,166 @@
/**
* An album page you can play from.
*
* `H-13`: no Play, no Shuffle, no Add to queue on the album header, and
* green ticks with no legend. The reason it is not simply "add three
* buttons" is that this is a **catalog** page — the album on it may be
* entirely the user's, partly theirs, or not theirs at all — and a Play
* button that plays 7 of a release's 40 tracks under a label saying
* "Play" is the page lying about what is owned.
*
* The partial case is the interesting one and it is **only reachable
* here**: it needs a catalog release whose tracklist is partly matched
* against the library, which the fixture library (untagged, no MBIDs,
* no network) cannot produce. The whole-album case was driven by hand
* in the running app.
*/
import { describe, expect, it, beforeEach } from 'vitest';
import type { LitElement } from 'lit';
import '@components/explore-album-details/explore-album-details';
import { stub, flush, resetHarness, calls } from '@test/support/harness';
import { fixture, shadow, text } from '@test/support/render';
type Version = {
key: string;
label: string;
sublabel: string;
tracks: Array<{
position: number;
discNumber: number;
title: string;
length: number;
mbid: string;
inLibrary: boolean;
}>;
};
function track(n: number, owned: boolean) {
return {
position: n,
discNumber: 1,
title: `Track ${n}`,
length: 200000,
mbid: `mbid-${n}`,
inLibrary: owned,
};
}
/**
* Put a release on the page without the network.
*
* The component builds its versions from fetched releases; this reaches
* past that and sets the state the header actually reads, which is the
* only part under test here.
*/
async function withVersion(
owned: number,
total: number,
): Promise<LitElement> {
const el = await fixture<LitElement>('explore-album-details', {
albumName: 'Glass Harbour',
});
const version: Version = {
key: 'v1',
label: '2019',
sublabel: `${total} tracks`,
tracks: Array.from({ length: total }, (_, i) => track(i + 1, i < owned)),
};
Object.assign(el, {
versionEntries: [version],
selectedVersionKey: 'v1',
loadingReleases: false,
loadingInfo: false,
});
el.requestUpdate();
await flush();
await el.updateComplete;
return el;
}
const playLabel = (el: LitElement) =>
text(el, '[data-testid="album-play"]');
describe('the album header’s primary action', () => {
beforeEach(() => {
resetHarness();
stub('library.Library.GetFilePathsByRecordingMBIDs', {});
stub('library.Library.GetFilePathsByAlbums', {});
stub('library.Library.GetAlbumTracks', []);
// The download actions resolve a target library on mount; without
// this the store awaits an undefined binding result and the whole
// file dies in an unhandled rejection rather than a failed test.
stub('library.Library.GetAllLibrariesWithTrackCounts', []);
});
it('says “Play” when the whole release is owned', async () => {
const el = await withVersion(6, 6);
expect(playLabel(el)).toBe('Play');
expect(shadow(el, '[data-testid="album-shuffle"]')).toBeTruthy();
expect(shadow(el, '[data-testid="album-queue"]')).toBeTruthy();
// No count sentence: there is nothing to qualify.
expect(shadow(el, '.album-owned-note')).toBeNull();
});
it('counts itself when only some of it is owned', async () => {
const el = await withVersion(7, 12);
expect(playLabel(el)).toBe('Play 7 of 12');
expect(text(el, '.album-owned-note')).toBe(
'You have 7 of these 12 tracks.',
);
});
it('offers no play button at all when none of it is owned', async () => {
// A Play button that plays nothing is worse than no Play button;
// the download and want actions are the whole answer here.
const el = await withVersion(0, 12);
expect(shadow(el, '[data-testid="album-play"]')).toBeNull();
expect(shadow(el, '[data-testid="album-shuffle"]')).toBeNull();
expect(shadow(el, '[data-testid="album-queue"]')).toBeNull();
});
it('asks for the owned tracks’ paths once, by the key it owns them by', async () => {
// `perf.m2`'s rule: ask for what the caller uses, once. The caller
// here uses file paths and knows its tracks only as recording
// MBIDs — `MBTrack.localId` is declared and never written by
// anything in the backend.
const el = await withVersion(7, 12);
shadow<HTMLElement>(el, '[data-testid="album-play"]')!.click();
await flush();
const asked = calls('library.Library.GetFilePathsByRecordingMBIDs');
expect(asked).toHaveLength(1);
// Only the owned ones, and no empty MBID — an empty string matches
// every untagged recording in the library.
expect(asked[0]!.args[0]).toHaveLength(7);
expect(asked[0]!.args[0]).not.toContain('');
});
});
describe('the ticks have a legend', () => {
beforeEach(() => {
resetHarness();
stub('library.Library.GetAlbumTracks', []);
stub('library.Library.GetAllLibrariesWithTrackCounts', []);
});
it('names the symbol when at least one track carries it', async () => {
const el = await withVersion(3, 12);
expect(text(el, '.tracklist-legend')).toContain('in your library');
});
it('does not explain a symbol that is not on screen', async () => {
const el = await withVersion(0, 12);
expect(shadow(el, '.tracklist-legend')).toBeNull();
});
});
+2
View File
@@ -47,6 +47,8 @@ export function GetFilePathsByAlbums(arg1:Array<number>,arg2:number):Promise<Rec
export function GetFilePathsByGenres(arg1:Array<string>,arg2:number):Promise<Record<string, Array<string>>>;
export function GetFilePathsByRecordingMBIDs(arg1:Array<string>,arg2:number):Promise<Record<string, Array<string>>>;
export function GetRemovalImpact(arg1:number):Promise<library.RemovalImpact>;
export function GetScanQueueLength():Promise<number>;
+4
View File
@@ -86,6 +86,10 @@ export function GetFilePathsByGenres(arg1, arg2) {
return window['go']['library']['Library']['GetFilePathsByGenres'](arg1, arg2);
}
export function GetFilePathsByRecordingMBIDs(arg1, arg2) {
return window['go']['library']['Library']['GetFilePathsByRecordingMBIDs'](arg1, arg2);
}
export function GetRemovalImpact(arg1) {
return window['go']['library']['Library']['GetRemovalImpact'](arg1);
}