From e5dc54d0ec91e31b4800ce7276078d746c1c06d9 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Wed, 23 Sep 2026 07:50:51 -0400 Subject: [PATCH 01/16] ci(skill-check): find a make target inside a hard-wrapped span `scripts/skill-check.sh` matched one regex against one line, so a mention the file hard-wraps -- `` `make `` at the end of one line and the target at the start of the next -- was invisible to it. These docs are mostly hard-wrapped prose, so the wrap is what the author does not think about, and `CONTRIBUTING.md:80` is already that shape. Lines are now joined while the inline span is still open, which an odd number of backticks means. The fence and line-start halves are untouched: a fenced command is already whole, and joining inside one would break the rule that made this awk rather than a grep. Joining is bounded three ways -- a fence, a blank line (CommonMark allows no blank line inside a code span) and a file boundary -- so a stray backtick costs one paragraph of over-matching rather than the rest of the file. The reporting loop needed the other half of the same fix: it named the offending file with `grep -ln "make $t"`, which cannot see a wrapped mention either, so a target the new parser found reported no file at all and `set -o pipefail` turned the empty grep into exit 123 before the line telling the author what to do. It falls back to the bare name. Verified by planting the report's own wrapped `make no-such-wrapped-target` into `CONTRIBUTING.md`: the old script reports 48 targets and exits 0, the new one names the target and the file and exits 1. Plant removed afterwards. Closes #228 --- scripts/skill-check.sh | 69 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 62 insertions(+), 7 deletions(-) diff --git a/scripts/skill-check.sh b/scripts/skill-check.sh index 08181bb..af4dbfb 100755 --- a/scripts/skill-check.sh +++ b/scripts/skill-check.sh @@ -82,23 +82,69 @@ targets="$({ make -pqRr 2>/dev/null || true; } | # happened to break there, and a check that fails on reflow gets # disabled rather than fixed. # +# **An inline span may be hard-wrapped, and then the mention is split +# across two lines.** `make` at the end of one line and its target at +# the start of the next is one code span to Markdown and two strings to +# a per-line regex, so the target was invisible — and these docs are +# mostly hard-wrapped prose, so the wrap is what the author does not +# think about. Lines are therefore joined while the span is still open, +# which is what an odd number of backticks means. +# +# Joining re-opens the reflow trap above unless it is bounded, so it is +# bounded three ways: a fence flushes first (a fenced command is already +# whole, and joining inside one would break the line-start rule), a +# blank line flushes (CommonMark does not allow a blank line inside a +# code span, so nothing legitimate is split by one), and so does a file +# boundary. A stray odd backtick in prose therefore costs one paragraph +# of over-matching rather than the rest of the file. +# # AGENTS.md is deliberately not in this list: it is a symlink to # CLAUDE.md, asserted above, so scanning it would report every failure # twice under two names. mentioned="$(printf '%s\n' "$docs" | xargs awk ' - FNR == 1 { fence = 0 } - /^```/ { fence = !fence; next } - { - rest = $0 + function scan(text, rest) { + rest = text while (match(rest, /`make [a-z][a-z0-9-]*/)) { print substr(rest, RSTART + 6, RLENGTH - 6) rest = substr(rest, RSTART + RLENGTH) } - if (fence && match($0, /^make [a-z][a-z0-9-]*/)) { - print substr($0, 6, RLENGTH - 5) + } + + function lineStart(text) { + if (match(text, /^make [a-z][a-z0-9-]*/)) { + print substr(text, 6, RLENGTH - 5) } } + + function ticks(s, n, i) { + n = 0 + for (i = 1; i <= length(s); i++) { + if (substr(s, i, 1) == "`") n++ + } + return n + } + + function flush() { + if (buf == "") return + scan(buf) + if (fence) lineStart(buf) + buf = "" + } + + FNR == 1 { flush(); fence = 0 } + + /^```/ { flush(); fence = !fence; next } + + /^[[:space:]]*$/ { flush(); next } + + { + if (fence) { scan($0); lineStart($0); next } + buf = (buf == "" ? $0 : buf " " $0) + if (ticks(buf) % 2 == 0) flush() + } + + END { flush() } ' | sort -u)" missing="" @@ -113,7 +159,16 @@ if [ -n "$missing" ]; then echo "skill-check: the docs name make targets that do not exist:" >&2 for t in $missing; do echo " make $t" >&2 - printf '%s\n' "$docs" | xargs grep -ln "make $t" | sed 's/^/ /' >&2 + # `make ` on one line first, because that is where a target is + # normally named and it is the precise answer. The bare name is the + # fallback, and it exists because the parser above can now find a + # mention that *this* grep cannot: a wrapped span has `make` and its + # target on different lines. Without it a missing target reported no + # file at all, and `set -o pipefail` turned the empty grep into exit + # 123, before the line telling the author what to do. + hits="$(printf '%s\n' "$docs" | xargs grep -ln "make $t" 2>/dev/null || true)" + [ -n "$hits" ] || hits="$(printf '%s\n' "$docs" | xargs grep -ln -- "$t" 2>/dev/null || true)" + [ -n "$hits" ] && printf '%s\n' "$hits" | sed 's/^/ /' >&2 done echo "Fix the docs, or restore the target." >&2 exit 1 -- 2.54.0 From e67462ab53e1c2e90d6625377d7a1952f6928ea7 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Wed, 23 Sep 2026 07:50:59 -0400 Subject: [PATCH 02/16] ci: lint every commit a PR would merge, not just its tip Gitea leaves `github.event.before` empty on a `pull_request`, so the Commit messages step fell through to bare `make commit-check`, which lints `git log -1` -- the tip alone. Every other commit the branch would bring was first examined by *main's* post-merge run, so a green PR stopped being true after the merge, and it happened twice: PR #245 merged a 75-char subject its own CI never saw. The PR's base is the stand-in. `base.sha..head` lints the PR's own commits because base advances on main, so the commits the branch shares with it stay reachable from it and drop out of the range. Both payload fields are handed to the shell rather than chosen in an expression: `github.event.issue.number` in unclaim.yml is this repo's proof that payload fields resolve, and `github.event` is the webhook body unmarshalled into a map, so `pull_request.base.sha` comes from Gitea's own `PRBranchInfo.Sha`. The shell then falls back to today's behaviour for a dispatch run, an all-zeros push, or a base commit the clone does not have -- so the worst case is the fix not taking effect rather than a broken job. Verified locally against the report's own evidence: at 68e7edb8 the old invocation passes ("HEAD is well-formed") while the range catches a3b5b437 at 75 chars, which is what main's post-merge run did. All four event shapes were exercised against the new snippet. The end-to-end proof is the next PR with an over-length commit that is not its tip. Closes #254 --- .gitea/workflows/ci.yml | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 78a2763..f9e750b 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -107,15 +107,32 @@ jobs: # Conventional Commits. `.releaserc.yml` has always derived the # version from the commit type; until now nothing checked that the # type was one it recognises, so a malformed subject silently meant - # "no release". BEFORE is the push's previous tip and is absent or - # all-zeros for a new branch, in which case only the tip is linted. + # "no release". + # + # **On a `pull_request` there is no `before`.** Gitea leaves + # `github.event.before` empty for one, so this step fell through to + # bare `make commit-check`, which lints `git log -1` — the tip + # alone. Every other commit the branch would bring was first + # examined by *main's* post-merge run, which is a green PR that + # stops being true after the merge, and which happened twice (#254). + # The PR's base is the stand-in: the range below already excludes + # what the base shares with the branch, because base advances on + # main and those commits stay reachable from it. + # + # Both are handed to the shell rather than chosen in an expression: + # `github.event.issue.number` in unclaim.yml is this repo's proof + # that payload fields resolve, and the shell then falls back to + # today's behaviour for a dispatch run or a missing field instead of + # depending on how `&&`/`||` treat an absent context. - name: Commit messages working-directory: /src env: - BEFORE: ${{ github.event.before }} + PR_BASE: ${{ github.event.pull_request.base.sha }} + PUSH_BEFORE: ${{ github.event.before }} run: | set -eu - if [ -n "${BEFORE:-}" ] && [ "${BEFORE#0000000}" = "$BEFORE" ] \ + BEFORE="${PR_BASE:-${PUSH_BEFORE:-}}" + if [ -n "$BEFORE" ] && [ "${BEFORE#0000000}" = "$BEFORE" ] \ && git cat-file -e "$BEFORE^{commit}" 2>/dev/null; then make commit-check RANGE="$BEFORE..$SHA" else -- 2.54.0 From 62c1a95eade4177e26b9805b941306f6b3fdafe1 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Wed, 23 Sep 2026 07:51:07 -0400 Subject: [PATCH 03/16] test(e2e): give the job specs their own state back Both specs stage a job through `/__test/emit` and neither cleared it. Nothing resets those stores, so the spec that staged it is the one that should put it back, and `JobsChanged` with `[]` is the whole cleanup -- `JobStore` replaces its list from every snapshot, so `testctl` needs no special case. **The leak as reported did not reproduce, and that is worth recording rather than quietly fixing.** Measured with a temporary probe: a positive control confirmed a staged job really does move the shell at a phone width (`job-band` renders a row, `.main-panel`'s top goes 0 to 55), and the very next page had no job at all. The reason is that every test gets a fresh page and `JobStore.init()` refetches `GetJobs()` from a backend registry `/__test/emit` never writes to -- it calls `events.Deliver`, which touches frontends and no state. So the state cannot cross a spec boundary as described, and the 55px offset the draft assertion saw in that suite run has another cause that is not in evidence. The cleanup stays, because it costs a line and the leak would need only one spec that keeps a page alive, and the comments say what was measured rather than asserting the mechanism. The durable half is the rule, now in the harness reference: measure against the element next to you, not an absolute coordinate. An absolute number in a shell measurement is also a claim about everything above it -- `contentTop === 0` asserts "and no background job is running", which that spec could not arrange. Closes #168 --- .../yellowjacket-dev/references/harness.md | 15 +++++++++++++++ e2e/specs/jobs-on-a-phone.spec.ts | 17 +++++++++++++++++ e2e/specs/top-bar-fit.spec.ts | 16 ++++++++++++++++ 3 files changed, 48 insertions(+) diff --git a/.pi/skills/yellowjacket-dev/references/harness.md b/.pi/skills/yellowjacket-dev/references/harness.md index e9d6d8c..4873ef0 100644 --- a/.pi/skills/yellowjacket-dev/references/harness.md +++ b/.pi/skills/yellowjacket-dev/references/harness.md @@ -57,6 +57,21 @@ behind `YJ_TESTCTL=1`, which `scripts/dev-headless.sh` sets and staging the work that would produce it — job progress, download progress, scan progress. It calls `events.Deliver`, which *errors* when the event reaches nobody, so a `200` means it really arrived. +- **State you stage, you own** (#168). Nothing resets those stores, so + clear yours in `test.afterEach` with the same event that staged it + (`emit('JobsChanged', [])`) — the store replaces its list from every + snapshot, so `testctl` needs no special case. **Measured: this does + not currently cross a spec boundary**, because every test gets a fresh + page and `JobStore.init()` refetches `GetJobs()` from a backend + registry that `/__test/emit` never writes to. Stated anyway, because + it costs one line and the leak needs only one spec that keeps a page + alive — but do not cite #168 for a symptom you have not reproduced. +- **Measure against the thing next to you, not an absolute + coordinate.** An absolute number in a shell measurement is also a + claim about everything above it — `contentTop === 0` quietly asserts + "and no background job is running", which is not what that spec was + about or could arrange, while `contentTop === jobBandBottom` is true + either way. This is the half of #168 that stands on its own. - **`restore` is slow** (~40 s in the suite) because it copies every table. Prefer snapshotting once and restoring only when a spec genuinely mutates state. diff --git a/e2e/specs/jobs-on-a-phone.spec.ts b/e2e/specs/jobs-on-a-phone.spec.ts index f76b639..1b8ae6c 100644 --- a/e2e/specs/jobs-on-a-phone.spec.ts +++ b/e2e/specs/jobs-on-a-phone.spec.ts @@ -60,6 +60,23 @@ const PHONE = { width: 424, height: 439 }; const DESKTOP = { width: 1100, height: 800 }; test.describe('background jobs on a phone', () => { + /** + * **State a spec stages is the spec's to clear.** `/__test/emit` writes + * to a store nothing resets, so the event that staged a job is the + * event that clears it — `JobStore` replaces its whole list from every + * snapshot, so `testctl` needs no special case. + * + * **Measured on #168: this does not currently outlive the page.** Every + * test gets a fresh page, and `JobStore.init()` refetches `GetJobs()` + * from a backend registry that `/__test/emit` never writes to, so the + * staged job is gone before the next spec starts. Ownership is stated + * rather than a live leak repaired — the leak needs a page that + * survives its own spec, and there is none today. + */ + test.afterEach(async ({ testctl }) => { + await testctl.emit('JobsChanged', []); + }); + test('are shown in the band, without opening anything', async ({ app, testctl, diff --git a/e2e/specs/top-bar-fit.spec.ts b/e2e/specs/top-bar-fit.spec.ts index badd8d0..0955942 100644 --- a/e2e/specs/top-bar-fit.spec.ts +++ b/e2e/specs/top-bar-fit.spec.ts @@ -102,6 +102,22 @@ const collapsed = (page: Page) => })); test.describe('the top bar fits the window', () => { + /** + * **State a spec stages is the spec's to clear** (#168). `/__test/emit` + * writes to a store nothing resets, and this file stages the widest job + * in the app, so it puts it back — with the same event, since the store + * replaces its whole list from every snapshot. + * + * **Measured: it does not currently outlive the page.** Every test gets + * a fresh page and `JobStore.init()` refetches `GetJobs()` from a + * backend registry `/__test/emit` never writes to, so nothing is being + * repaired here; the rule is stated because it costs one line and the + * leak would need only one spec that keeps a page alive. + */ + test.afterEach(async ({ testctl }) => { + await testctl.emit('JobsChanged', []); + }); + /** * The phone's answer, which is not "it fits" (#57). * -- 2.54.0 From d4ea14ca5c91c65421b28d62f106d606b78591e1 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Fri, 25 Sep 2026 10:18:06 -0400 Subject: [PATCH 04/16] fix(explore): merge the catalog artifact in its own mbid encoding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prebuilt catalog never merged. `mergeArtifactRows` positions itself with `WHERE mbid > ? ORDER BY mbid LIMIT 1 OFFSET ?` against the attached artifact, and it bound that cursor as a Go `string` while `cmd/indexexport` publishes `explore_index.mbid` as 16 raw bytes — the storage change that took the table from 677 MB to 389 MB. SQLite does not coerce between TEXT and BLOB and orders every blob after every text value, so against a byte column the predicate was not wrong but unconditional: `mbid > ` matched the whole artifact, so the bound the walk looked up was the same row every time and the cursor never advanced, and `mbid <= ` matched nothing, so no batch merged. No error, no rows, no state change — a fresh install sat at "0 of 1,077,893 rows" burning a core indefinitely, which is what it did here for a day, while Explore showed only the rows the library scan and the lazy artist enrichment had produced and popularity for none of the catalog. The cursor is now an `artifactKey`, typed to the encoding `artifactStoresText` reports for the file it is attached to, so the comparison is made in the same type as the column it is made against. Two things guard the class rather than the instance: a nil key binds as an empty value instead of SQL NULL, because `mbid > NULL` agrees with nothing and would import nothing just as silently; and the walk returns an error when its bound does not strictly advance, because the failure here is silence and the next one should be a failed job with a reason. It was never caught because the fixture that guards the walk writes the old text encoding, and the only compact fixture is a single row — below `artifactMergeBatch`, so the bound query never ran at all. The walk is now covered on both encodings, across several batch boundaries. Closes #258 --- CLAUDE.md | 16 ++ backend/explore/artifactimport.go | 80 ++++++- backend/explore/artifactimport_test.go | 277 +++++++++++++++++++------ 3 files changed, 300 insertions(+), 73 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 2512365..605b8a8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -845,6 +845,22 @@ that is quietly empty. top-N, exact match, FTS search, popularity batch, the CAA map — and asserts each returns something with a dashed id. A missed conversion site shows up there and essentially nowhere else. +- **A comparison is typed on *both* sides, and a parameter is the half + that gets forgotten.** The paragraph above is about a literal; the + artifact merge positioned its batch walk with a Go `string` cursor + against the artifact's byte column, and SQLite answered rather than + complained: `mbid > ?` with a text key is true of every row, so the + bound the walk looked up was the same every time and the cursor + never advanced, while `mbid <= ?` is false of every row, so no batch + merged at all. The import looped indefinitely at 100% CPU behind a + progress bar reading "0 of 1,077,893 rows", merged nothing and + raised nothing (#258). Nothing caught it because the fixture that + guards the walk writes the old text form and the only compact one is + a single row — below `artifactMergeBatch`, so the bound query never + ran. `artifactKey` types the cursor to the artifact's own encoding + now, and the walk fails loudly when its bound does not strictly + advance, because the failure mode here is silence rather than a + wrong answer. **The artifact is read in either encoding.** A published artifact carries whichever form the exporter that built it used, and there is one diff --git a/backend/explore/artifactimport.go b/backend/explore/artifactimport.go index 49f92ca..c174eb0 100644 --- a/backend/explore/artifactimport.go +++ b/backend/explore/artifactimport.go @@ -1,8 +1,10 @@ package explore import ( + "bytes" "context" "database/sql" + "database/sql/driver" "errors" "fmt" "os" @@ -348,8 +350,12 @@ func (si *SearchIndex) analyzeIndex() { // is an index range scan and a cancelled import leaves committed work // behind rather than rolling it all back. func (si *SearchIndex) mergeArtifactRows(ctx context.Context, total int) (int, error) { + // Asked once, because it is a property of the file and it decides + // how the walk's own comparisons are typed. See artifactKey. + storesText := si.artifactStoresText() + selectColumns := artifactSelectColumns( - si.artifactStoresText(), si.artifactHasTotals(), + storesText, si.artifactHasTotals(), ) insertSQL := ` @@ -367,7 +373,7 @@ func (si *SearchIndex) mergeArtifactRows(ctx context.Context, total int) (int, e WHERE mbid > ? AND mbid <= ?` + upsertIndexConflictSQL var ( - cursor string + cursor artifactKey merged int ) @@ -376,17 +382,30 @@ func (si *SearchIndex) mergeArtifactRows(ctx context.Context, total int) (int, e return merged, err } - upper, hasUpper, err := si.artifactBatchBound(cursor) + upper, hasUpper, err := si.artifactBatchBound(storesText, cursor) if err != nil { return merged, err } + if hasUpper && bytes.Compare(upper, cursor) <= 0 { + // The predicate matched the cursor itself, so the walk can + // never advance. SQLite says nothing when a comparison is + // made between types it will not coerce - the query simply + // answers wrongly - so a mismatch here would otherwise spin + // forever behind an unmoving progress bar. Fail instead. + return merged, fmt.Errorf( + "%w: artifact walk did not advance past %x", + ErrArtifactUnusable, []byte(cursor), + ) + } + var res sql.Result if hasUpper { - res, err = si.db.ExecContext(insertRangeSQL, cursor, upper) + res, err = si.db.ExecContext(insertRangeSQL, + cursor.bind(storesText), upper.bind(storesText)) } else { - res, err = si.db.ExecContext(insertSQL, cursor) + res, err = si.db.ExecContext(insertSQL, cursor.bind(storesText)) } if err != nil { @@ -413,26 +432,65 @@ func (si *SearchIndex) mergeArtifactRows(ctx context.Context, total int) (int, e } } +// artifactKey is one MBID as the attached artifact stores it: 16 raw +// bytes in a compact artifact, the dashed 36-character form in one +// published before that storage change. +// +// It is a type with a bind method rather than a string because the +// comparison it feeds is typed, and the wrong type is silent. SQLite +// does not coerce between TEXT and BLOB and orders every blob after +// every text value, so a cursor bound as text against a byte column +// makes `mbid > ?` true of the whole table - the walk rediscovers the +// same batch bound forever, and `mbid <= ?` false of the whole table, +// so no batch merges at all. Nothing errors; the import simply never +// finishes. bind is the one place that knows which form the column is +// in, decided by artifactStoresText, which asks the artifact rather than +// trusting a version number. +type artifactKey []byte + +// bind renders the key as a statement argument in the artifact's own +// encoding. +func (k artifactKey) bind(storesText bool) driver.Value { + if storesText { + return string(k) + } + + // Never nil. database/sql converts a nil []byte to SQL NULL, and + // `mbid > NULL` is NULL for every row - so an unset cursor would + // agree with nothing and import nothing, which is the same silently + // empty merge this type exists to prevent, one type over. + if k == nil { + return []byte{} + } + + return []byte(k) +} + // artifactBatchBound returns the MBID that ends the next batch, and // whether one exists — no bound means the remainder is the last batch. -func (si *SearchIndex) artifactBatchBound(cursor string) (string, bool, error) { - var bound string +// +// The bound is read out of the artifact and handed back as an +// artifactKey, because it becomes the next comparison the walk makes. +func (si *SearchIndex) artifactBatchBound( + storesText bool, cursor artifactKey, +) (artifactKey, bool, error) { + var bound []byte err := si.db.QueryRowWriter( `SELECT mbid FROM core.explore_index WHERE mbid > ? ORDER BY mbid LIMIT 1 OFFSET ?`, - cursor, artifactMergeBatch-1, + cursor.bind(storesText), artifactMergeBatch-1, ).Scan(&bound) if errors.Is(err, sql.ErrNoRows) { - return "", false, nil + return nil, false, nil } if err != nil { - return "", false, fmt.Errorf("%w: batch bound: %w", ErrArtifactUnusable, err) + return nil, false, fmt.Errorf("%w: batch bound: %w", ErrArtifactUnusable, err) } - return bound, true, nil + return artifactKey(bound), true, nil } // stampArtifactMeta records what the merge established: the catalog half diff --git a/backend/explore/artifactimport_test.go b/backend/explore/artifactimport_test.go index 6a8184e..0379775 100644 --- a/backend/explore/artifactimport_test.go +++ b/backend/explore/artifactimport_test.go @@ -1,6 +1,7 @@ package explore import ( + "bytes" "context" "database/sql" "encoding/hex" @@ -71,13 +72,7 @@ func writeTestArtifact( } } - for k, v := range meta { - if _, err := db.Exec( - `INSERT INTO artifact_meta (key, value) VALUES (?, ?)`, k, v, - ); err != nil { - t.Fatalf("stamp artifact meta: %v", err) - } - } + stampArtifactMeta(t, db, meta) for _, r := range rows { if _, err := db.Exec(` @@ -93,6 +88,101 @@ func writeTestArtifact( return path } +// compactArtifactSchema is the artifact cmd/indexexport publishes: the +// catalog's ids as 16 raw bytes, its entity types as codes, and the +// per-release-group total_tracks the exporter added after the first +// artifact was shipped. +// +// It matters that a fixture carries this encoding and not the older +// text one, because SQLite does not coerce between TEXT and BLOB and +// every comparison the importer makes against an mbid is therefore +// encoding-sensitive. writeTestArtifact above is the *other* fixture: +// it still writes the text form, which is what the first published +// artifact carries and what the importer must keep reading. +var compactArtifactSchema = []string{ + `CREATE TABLE explore_index ( + entity_type INTEGER NOT NULL, + mbid BLOB NOT NULL, + title TEXT NOT NULL, + artist_name TEXT NOT NULL, + artist_mbid BLOB NOT NULL, + aliases TEXT NOT NULL DEFAULT '', + popularity INTEGER NOT NULL DEFAULT 0, + listener_count INTEGER NOT NULL DEFAULT 0, + duration INTEGER NOT NULL DEFAULT 0, + caa_release_mbid BLOB NOT NULL DEFAULT x'', + release_name TEXT NOT NULL DEFAULT '', + primary_type TEXT NOT NULL DEFAULT '', + secondary_types TEXT NOT NULL DEFAULT '', + release_date TEXT NOT NULL DEFAULT '', + total_tracks INTEGER NOT NULL DEFAULT 0, + artist_type TEXT NOT NULL DEFAULT '', + country TEXT NOT NULL DEFAULT '', + disambiguation TEXT NOT NULL DEFAULT '', + sort_name TEXT NOT NULL DEFAULT '', + discog_fetched INTEGER NOT NULL DEFAULT 0, + PRIMARY KEY (mbid) + ) WITHOUT ROWID`, + `CREATE TABLE artifact_meta ( + key TEXT PRIMARY KEY, + value TEXT NOT NULL + )`, +} + +// writeCompactTestArtifact builds the artifact the exporter publishes +// today, in its own encoding, so the importer is exercised against what +// a client actually downloads rather than against what it was written +// for. +func writeCompactTestArtifact( + t *testing.T, meta map[string]string, rows []artifactRow, +) string { + t.Helper() + + path := filepath.Join(t.TempDir(), "core-index.db") + + db, err := sql.Open("sqlite", "file:"+path) + if err != nil { + t.Fatalf("open artifact: %v", err) + } + + defer func() { _ = db.Close() }() + + for _, stmt := range compactArtifactSchema { + if _, err := db.Exec(stmt); err != nil { + t.Fatalf("create artifact schema: %v", err) + } + } + + stampArtifactMeta(t, db, meta) + + for _, r := range rows { + if _, err := db.Exec(` + INSERT INTO explore_index + (entity_type, mbid, title, artist_name, artist_mbid, popularity) + VALUES (?, ?, ?, ?, ?, ?)`, + entityCode(r.entityType), mbidBytes(r.mbid), r.title, + r.artistName, mbidBytes(r.artistMBID), r.popularity, + ); err != nil { + t.Fatalf("insert artifact row: %v", err) + } + } + + return path +} + +// stampArtifactMeta writes the artifact_meta rows a fixture declares. +func stampArtifactMeta(t *testing.T, db *sql.DB, meta map[string]string) { + t.Helper() + + for k, v := range meta { + if _, err := db.Exec( + `INSERT INTO artifact_meta (key, value) VALUES (?, ?)`, k, v, + ); err != nil { + t.Fatalf("stamp artifact meta: %v", err) + } + } +} + // validMeta is the artifact_meta a well-formed artifact carries. func validMeta() map[string]string { return map[string]string{ @@ -270,6 +360,71 @@ func TestImportCoreArtifactBatchWalkCoversAllRows(t *testing.T) { } } +// TestImportCoreArtifactBatchWalkCoversAllRowsCompact is the batch walk +// on the encoding the exporter actually publishes. +// +// The walk positions itself by comparing the artifact's own mbid column +// against the last id it reached, and that column holds 16 raw bytes. +// SQLite does not coerce between TEXT and BLOB, and a blob sorts after +// every text value, so a cursor bound as text is a predicate that either +// matches every row or none: `mbid > ?` with an empty text key is true +// of the whole table, so +// the 100th row is always the 100th row and the bound never advances, +// while `mbid <= ` is false of the whole table, so no batch ever +// merges. The result is not a wrong import but an unbounded loop that +// merges nothing and never fails. +// +// Both encodings are covered on purpose. The walk was only ever tested +// against the text fixture above, which is why it shipped broken on the +// one the clients download. +func TestImportCoreArtifactBatchWalkCoversAllRowsCompact(t *testing.T) { + db := database.NewTestDB(t) + si := NewSearchIndex(db, nil, nil, testLogger()) + + original := artifactMergeBatch + artifactMergeBatch = 100 + + t.Cleanup(func() { artifactMergeBatch = original }) + + const total = 337 + + rows := make([]artifactRow, 0, total) + for i := range total { + rows = append(rows, artifactRow{ + entityType: EntityRecording, + mbid: syntheticMBID(i), + title: "Song", + artistName: "Artist", + artistMBID: artA, + popularity: i, + }) + } + + path := writeCompactTestArtifact(t, validMeta(), rows) + + if err := si.importCoreArtifact(context.Background(), path); err != nil { + t.Fatalf("importCoreArtifact: %v", err) + } + + var got, top int + + if err := db.QueryRowWriter( + "SELECT COUNT(*), MAX(popularity) FROM explore_index", + ).Scan(&got, &top); err != nil { + t.Fatalf("count rows: %v", err) + } + + if got != total { + t.Errorf("merged %d rows, want %d", got, total) + } + + // A count alone would pass if the walk re-merged the same first + // batch forever, so the far end of the artifact is checked too. + if top != total-1 { + t.Errorf("highest popularity = %d, want %d", top, total-1) + } +} + func TestImportCoreArtifactRejectsBadArtifacts(t *testing.T) { tests := []struct { name string @@ -454,61 +609,9 @@ func TestArtifactColumnsMatchExporter(t *testing.T) { // the importer decides by asking the artifact, not by trusting a // version number, and both must land identically. func TestImportCoreArtifactAcceptsBothEncodings(t *testing.T) { - compact := filepath.Join(t.TempDir(), "core-index.db") - - db, err := sql.Open("sqlite", "file:"+compact) - if err != nil { - t.Fatalf("open artifact: %v", err) - } - - if _, err := db.Exec(`CREATE TABLE explore_index ( - entity_type INTEGER NOT NULL, - mbid BLOB NOT NULL, - title TEXT NOT NULL, - artist_name TEXT NOT NULL, - artist_mbid BLOB NOT NULL, - aliases TEXT NOT NULL DEFAULT '', - popularity INTEGER NOT NULL DEFAULT 0, - listener_count INTEGER NOT NULL DEFAULT 0, - duration INTEGER NOT NULL DEFAULT 0, - caa_release_mbid BLOB NOT NULL DEFAULT x'', - release_name TEXT NOT NULL DEFAULT '', - primary_type TEXT NOT NULL DEFAULT '', - secondary_types TEXT NOT NULL DEFAULT '', - release_date TEXT NOT NULL DEFAULT '', - artist_type TEXT NOT NULL DEFAULT '', - country TEXT NOT NULL DEFAULT '', - disambiguation TEXT NOT NULL DEFAULT '', - sort_name TEXT NOT NULL DEFAULT '', - discog_fetched INTEGER NOT NULL DEFAULT 0, - PRIMARY KEY (mbid) - )`); err != nil { - t.Fatalf("create artifact table: %v", err) - } - - if _, err := db.Exec( - `CREATE TABLE artifact_meta (key TEXT PRIMARY KEY, value TEXT NOT NULL)`, - ); err != nil { - t.Fatalf("create artifact meta: %v", err) - } - - for k, v := range validMeta() { - if _, err := db.Exec( - "INSERT INTO artifact_meta (key, value) VALUES (?, ?)", k, v, - ); err != nil { - t.Fatalf("write artifact meta: %v", err) - } - } - - if _, err := db.Exec(` - INSERT INTO explore_index (entity_type, mbid, title, artist_name, artist_mbid, popularity) - VALUES (1, ?, 'Artist A', 'Artist A', ?, 5000)`, - mbidBytes(artA), mbidBytes(artA), - ); err != nil { - t.Fatalf("write artifact row: %v", err) - } - - _ = db.Close() + compact := writeCompactTestArtifact(t, validMeta(), []artifactRow{ + {EntityArtist, artA, "Artist A", "Artist A", artA, 5000}, + }) live := database.NewTestDB(t) si := NewSearchIndex(live, nil, nil, testLogger()) @@ -748,3 +851,53 @@ func TestImportCoreArtifactWithoutCredits(t *testing.T) { t.Errorf("credit refs = %d, want 0", refs) } } + +// TestArtifactKeyBindsInTheArtifactsOwnEncoding pins the one place the +// batch walk's comparison type is decided. +// +// Every wrong answer is silent, which is why it is worth pinning all +// four. SQLite does not coerce TEXT to BLOB and orders every blob after +// every text value, so a text key against a byte column makes +// `mbid > ?` true of the whole artifact - the cursor never advances and +// the walk spins forever without merging a row - while a byte key +// against a text column makes it false of the whole artifact, so every +// batch merges nothing and the import "succeeds" empty. An unset cursor +// is the same fault once more: database/sql converts a nil []byte to +// SQL NULL, and `mbid > NULL` matches no row at all. +func TestArtifactKeyBindsInTheArtifactsOwnEncoding(t *testing.T) { + raw := mbidBytes(artA) + + for _, tt := range []struct { + name string + key artifactKey + want []byte + }{ + {"unset", nil, []byte{}}, + {"set", artifactKey(raw), raw}, + } { + t.Run("bytes/"+tt.name, func(t *testing.T) { + got, ok := tt.key.bind(false).([]byte) + if !ok { + t.Fatalf("bind(false) = %T, want []byte", tt.key.bind(false)) + } + + if got == nil { + t.Fatal("bound to SQL NULL, which matches no row") + } + + if !bytes.Equal(got, tt.want) { + t.Errorf("bind(false) = %x, want %x", got, tt.want) + } + }) + } + + // The dashed form is what an artifact published before the storage + // change carries, and it has to compare as text against text. + if got := artifactKey(nil).bind(true); got != "" { + t.Errorf("bind(true) on an unset cursor = %#v, want an empty string", got) + } + + if got := artifactKey(artA).bind(true); got != artA { + t.Errorf("bind(true) = %#v, want %q", got, artA) + } +} -- 2.54.0 From 1e3a490c12dc06d6e4874077d7c42c5c67b4c7c2 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Fri, 25 Sep 2026 11:03:33 -0400 Subject: [PATCH 05/16] fix(explore): refuse a catalog merge that does not land every row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The walk's predicates partition the artifact's key space, so a merge that ends with fewer rows than the artifact declares does not mean the artifact was smaller than it said — it means a predicate filtered rows out, and the catalog is quietly partial while reporting complete. Equality rather than a lower bound: RowsAffected counts an upsert that changes nothing, and a row already merged locally is counted again here. One reachable case, so this is not merely a tripwire. A row whose mbid is empty is excluded by `mbid > ?` in both encodings, so an artifact carrying one imports as a success with a row missing — which is the shape #258 had, one cause over. The test covers exactly that artifact. Refs #258 --- backend/explore/artifactimport.go | 19 ++++++++++++++ backend/explore/artifactimport_test.go | 35 ++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/backend/explore/artifactimport.go b/backend/explore/artifactimport.go index c174eb0..c6cfaaa 100644 --- a/backend/explore/artifactimport.go +++ b/backend/explore/artifactimport.go @@ -285,6 +285,25 @@ func (si *SearchIndex) importCoreArtifact(ctx context.Context, path string) erro } merged, mergeErr := si.mergeArtifactRows(ctx, info.rows) + + // Every row the artifact declares has to land. The walk partitions + // the artifact's key space, so a total short of info.rows does not + // mean the artifact was smaller than it said -- it means a predicate + // filtered rows out, and the catalog is quietly partial. Equality + // rather than a lower bound because RowsAffected counts an upsert + // that changes nothing, and a row already merged locally is counted + // again here. + // + // One reachable case, so this is not merely a tripwire: a row whose + // mbid is empty is excluded by `mbid > ?` in both encodings, and an + // artifact carrying one would otherwise import as complete. + if mergeErr == nil && merged != info.rows { + mergeErr = fmt.Errorf( + "%w: merged %d of %d rows — a row the artifact holds was not selected", + ErrArtifactUnusable, merged, info.rows, + ) + } + if mergeErr == nil { si.mergeArtifactCredits(ctx) } diff --git a/backend/explore/artifactimport_test.go b/backend/explore/artifactimport_test.go index 0379775..2b4a552 100644 --- a/backend/explore/artifactimport_test.go +++ b/backend/explore/artifactimport_test.go @@ -852,6 +852,41 @@ func TestImportCoreArtifactWithoutCredits(t *testing.T) { } } +// TestImportCoreArtifactRefusesAMergeThatLosesRows is the count guard's +// positive case. +// +// The walk's predicates partition the artifact's key space, so a merge +// that lands fewer rows than the artifact declares means a predicate +// dropped some — and the failure is a catalog that looks populated and +// is missing things nobody can name. An empty mbid is the reachable +// way to get there: `mbid > ?` is false of it in both encodings, so it +// is never selected, and nothing else in the import would notice. +func TestImportCoreArtifactRefusesAMergeThatLosesRows(t *testing.T) { + db := database.NewTestDB(t) + si := NewSearchIndex(db, nil, nil, testLogger()) + + path := writeCompactTestArtifact(t, validMeta(), []artifactRow{ + {EntityArtist, artA, "Artist A", "Artist A", artA, 5000}, + {EntityArtist, "", "Nameless", "Artist A", artA, 4000}, + }) + + err := si.importCoreArtifact(context.Background(), path) + if err == nil { + t.Fatal("a merge that lost a row was reported as a complete import") + } + + if !strings.Contains(err.Error(), "merged 1 of 2 rows") { + t.Errorf("error = %v, want it to name the shortfall", err) + } + + // And the same rule as every other rejection: a failed merge must not + // leave the index claiming it has a catalog, or the real build would + // never run again. + if si.hasMeta(dumpImportDoneKey) { + t.Error("a failed import still stamped dump_import_done") + } +} + // TestArtifactKeyBindsInTheArtifactsOwnEncoding pins the one place the // batch walk's comparison type is decided. // -- 2.54.0 From 5d9c677cf7745615d7cf11072c9052b9ad3b1d6a Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Fri, 25 Sep 2026 11:03:52 -0400 Subject: [PATCH 06/16] ci(index-artifact): import the exported artifact before publishing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing should be published until the code that imports it on a user's machine has imported it here. The exporter and the importer are two descriptions of one storage format, and every other tier tests the importer against a *fixture* rather than against the file being shipped — a second description free to be wrong in the same direction as the code reading it. That is how #258 reached everyone: the importer positioned its batch walk with a Go `string` cursor against this file's 16-byte `mbid` column, and SQLite neither coerces between TEXT and BLOB nor complains about the comparison, so the walk merged no rows and never advanced. The fixture guarding that walk writes the old text encoding, and the only compact fixture is one row, below the batch size, so the bound query never ran. Both were green throughout, and no install could finish a first index build. `TestImportPublishedArtifact` takes the published file and runs the client's own path over it — checksum, decompress, merge — and asserts that what the artifact holds is what the client ends up with: the row count, the rows carrying a listen count, an FTS index in step with the table, and one row read back through the app's own MBID conversions. It skips without `YJ_CORE_INDEX_ARTIFACT`, so an ordinary run pays nothing. It needs no Wails, which is why the step runs it under the indexbuild tag: that container has no GTK. Measured on the current artifact, 64.8 MB compressed: 39 seconds including the decompress. Verified against the pre-fix comparison behaviour, the step goes red in about 3 seconds — the strictly-advancing guard fails the import with a named reason rather than the day-long spin it used to produce. Refs #258 --- .gitea/workflows/index-artifact.yml | 45 +++- backend/explore/publishedartifact_test.go | 266 ++++++++++++++++++++++ 2 files changed, 310 insertions(+), 1 deletion(-) create mode 100644 backend/explore/publishedartifact_test.go diff --git a/.gitea/workflows/index-artifact.yml b/.gitea/workflows/index-artifact.yml index 4ae894c..b28ccd2 100644 --- a/.gitea/workflows/index-artifact.yml +++ b/.gitea/workflows/index-artifact.yml @@ -68,7 +68,7 @@ jobs: # claim with a test behind it now (cmd/indexbuild/deps_test.go), # because the v3 migration quietly broke it and this job was where # that surfaced. - image: golang:1.25 + image: golang:1.26 # This host path must exist on the runner and be listed verbatim in # act_runner's container.valid_volumes. It holds explore-staging/ # (counts.bin + state.json) and yj.db — the checkpoint that makes @@ -148,6 +148,49 @@ jobs: sha256sum /tmp/core-index.db.zst | tee /tmp/core-index.db.zst.sha256 ls -lh /tmp/core-index.db.zst + # Nothing is published until it has been imported by the code that + # imports it on a user's machine. The exporter and the importer are + # two descriptions of one storage format, and every other tier tests + # the importer against a *fixture* rather than against the file being + # shipped — a second description free to be wrong in the same + # direction as the code reading it. + # + # That is how #258 reached everyone: the importer positioned its batch + # walk with a Go `string` cursor against this file's 16-byte `mbid` + # column, and SQLite neither coerces between TEXT and BLOB nor + # complains about the comparison — so the walk merged no rows and + # never advanced, and no install could finish its first index build. + # The fixture guarding that walk writes the old text encoding, and the + # only compact fixture is one row, below the batch size, so the bound + # query never ran. Both were green throughout. + # + # Running it here is also what keeps the failure cheap: the previous + # artifact stays published while this runs, so a failure costs one + # stale catalog rather than an empty one for every install. + # + # `-tags indexbuild` because this container has no GTK and the default + # tag set links the app through Wails. The `--- PASS` grep is not + # decoration — the test skips without the path, and a skip is + # indistinguishable from a pass in a summary line. + - name: Import the exported artifact as a client does + if: steps.maintain.outputs.complete == 'true' && steps.maintain.outputs.changed == 'true' + working-directory: /src + env: + YJ_CORE_INDEX_ARTIFACT: /tmp/core-index.db.zst + run: | + set -eu + log=/tmp/import-check.log + if ! go test -tags indexbuild -count=1 -timeout 30m -v \ + -run TestImportPublishedArtifact ./backend/explore/ > "$log" 2>&1; + then + tail -60 "$log" + echo "::error::The artifact does not import; not publishing it." + exit 1 + fi + cat "$log" + grep -qF -- 'PASS: TestImportPublishedArtifact' "$log" + echo "::notice::The artifact imports as a client would merge it." + - name: Publish to the Gitea package registry if: steps.maintain.outputs.complete == 'true' && steps.maintain.outputs.changed == 'true' run: | diff --git a/backend/explore/publishedartifact_test.go b/backend/explore/publishedartifact_test.go new file mode 100644 index 0000000..3501e75 --- /dev/null +++ b/backend/explore/publishedartifact_test.go @@ -0,0 +1,266 @@ +package explore + +import ( + "context" + "database/sql" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "yellowjacket/backend/database" +) + +// Import of the artifact we actually publish, as a client imports it. +// +// Every other test here builds a fixture, and a fixture is a second +// description of the storage format that can be wrong in the same +// direction as the code reading it. That is how #258 shipped: the +// importer positioned its batch walk with a Go `string` cursor against +// the artifact's 16-byte `mbid` column, and SQLite neither coerces +// between TEXT and BLOB nor complains about the comparison — so the walk +// merged nothing and never advanced, and no install could finish its +// first index build. The fixture that guards the walk writes the old +// text encoding; the only compact fixture is one row, below the batch +// size, so the bound query never ran. Both passed throughout. +// +// So this one takes the published file and runs the client's own path +// over it — checksum, decompress, merge — and asserts that what the +// artifact holds is what the client ends up with. +// +// It skips without the path, so an ordinary test run pays nothing for +// it, and the publish job is where it is meant to run: +// +// YJ_CORE_INDEX_ARTIFACT=/tmp/core-index.db.zst \ +// go test -tags indexbuild -run TestImportPublishedArtifact \ +// ./backend/explore/ +// +// The indexbuild tag is not incidental: that job's container has no GTK, +// and the default tag set links the app through Wails. + +// publishedArtifactEnv points at the published artifact: the compressed +// core-index.db.zst, or the unpacked core-index.db. +const publishedArtifactEnv = "YJ_CORE_INDEX_ARTIFACT" + +// artifactTotals is the pair this test compares across the boundary. +// +// Rows is the whole point — a merge that lands fewer of them than the +// artifact declares is a catalog that looks populated and is missing +// things nobody can name — and popularity is the half whose absence was +// reported when it happened, because it arrives only through the merge. +type artifactTotals struct { + rows int + withListen int +} + +func TestImportPublishedArtifact(t *testing.T) { + published := strings.TrimSpace(os.Getenv(publishedArtifactEnv)) + if published == "" { + t.Skipf("set %s= to import the published artifact", + publishedArtifactEnv) + } + + if _, err := os.Stat(published); err != nil { + t.Fatalf("%s: %v", publishedArtifactEnv, err) + } + + // A file-backed database rather than NewTestDB's in-memory one: the + // artifact is ~135MB and a million rows, which is not a thing to hold + // in RAM inside a test. YJ_HOME is how NewDB is pointed somewhere + // disposable, and going through NewDB means this is the constructor, + // the schema and the read pool the app itself opens. + // + // Nothing closes it, because nothing can: `DB` has no Close and the + // app's handles are process-lifetime by design. The directory is + // unlinked at cleanup and the file goes with it. + t.Setenv("YJ_HOME", t.TempDir()) + + db, err := database.NewDB(testLogger()) + if err != nil { + t.Fatalf("open database: %v", err) + } + + si := NewSearchIndex(db, nil, nil, testLogger()) + + // The checksum the publisher shipped, if it shipped one. Every + // client verifies it and refuses the artifact when it does not + // match, so a wrong one breaks Explore for everyone who has not + // already imported — and nothing else would see it, because the + // comparison is between two files only the publisher has. + if want, ok := publishedChecksum(published); ok { + got, err := fileSHA256(published) + if err != nil { + t.Fatalf("checksum the artifact: %v", err) + } + + if got != want { + t.Errorf("published artifact hashes to %s, but its .sha256 says %s", + got, want) + } + } + + unpacked := unpackPublishedArtifact(t, si, published) + + want, err := artifactTotalsOf(unpacked) + if err != nil { + t.Fatalf("count the artifact's rows: %v", err) + } + + if err := si.importCoreArtifact(context.Background(), unpacked); err != nil { + t.Fatalf("importCoreArtifact: %v", err) + } + + got, err := indexTotalsOf(db) + if err != nil { + t.Fatalf("count the index's rows: %v", err) + } + + if got.rows != want.rows { + t.Errorf("merged %d rows, but the artifact holds %d", + got.rows, want.rows) + } + + if got.withListen != want.withListen { + t.Errorf("%d rows carry a listen count, but the artifact holds %d of them", + got.withListen, want.withListen) + } + + // The FTS index is rebuilt from the table once the merge is done, and + // it is what search actually reads: a merge that lands without it + // leaves Explore silently matching nothing, which is the state #258 + // produced by a different route. + var indexed int + if err := db.QueryRowWriter( + "SELECT COUNT(*) FROM explore_index_fts", + ).Scan(&indexed); err != nil { + t.Fatalf("count the FTS index: %v", err) + } + + if indexed != got.rows { + t.Errorf("FTS index holds %d rows against the table's %d", + indexed, got.rows) + } + + // And one row read back through the app's own path, which is the + // other direction of every conversion the merge makes: a byte MBID + // out of the table, the app's dashed form, and back in as a lookup. + var raw []byte + if err := db.QueryRowWriter(` + SELECT mbid FROM explore_index + WHERE entity_type = 1 /* artist */ AND popularity > 0 + ORDER BY popularity DESC LIMIT 1`).Scan(&raw); err != nil { + t.Fatalf("read a stored mbid: %v", err) + } + + dashed, err := mbidFromBytes(raw) + if err != nil { + t.Fatalf("the stored mbid is not one: %v", err) + } + + artist := si.LookupArtistByMBID(dashed) + if artist == nil { + t.Fatalf("the artifact's most popular artist %s does not look up", dashed) + } + + if artist.Popularity == 0 { + t.Errorf("artist %s came back with no popularity", dashed) + } +} + +// publishedChecksum reads the sha256 the publisher wrote beside the +// artifact, in `sha256sum` output form. A missing file is not a +// failure: it is only there when the artifact came from the publish job. +func publishedChecksum(path string) (string, bool) { + body, err := os.ReadFile(path + ".sha256") + if err != nil { + return "", false + } + + sum := strings.TrimSpace(string(body)) + if i := strings.IndexAny(sum, " \t"); i > 0 { + sum = sum[:i] + } + + if len(sum) != 64 { + return "", false + } + + return strings.ToLower(sum), true +} + +// unpackPublishedArtifact returns a path to the unpacked database, +// going through the client's own decompression when it is handed the +// compressed file that is actually published. +func unpackPublishedArtifact(t *testing.T, si *SearchIndex, path string) string { + t.Helper() + + if strings.HasSuffix(path, ".db") { + return path + } + + // Copied into the test's own directory first: decompress writes + // beside the compressed file, and the publisher's directory is not + // this test's to write in. + staging := t.TempDir() + dst := filepath.Join(staging, coreArtifactFile) + + src, err := os.Open(path) + if err != nil { + t.Fatalf("open the published artifact: %v", err) + } + + defer func() { _ = src.Close() }() + + out, err := os.Create(dst) + if err != nil { + t.Fatalf("create a staging copy: %v", err) + } + + if _, err := io.Copy(out, src); err != nil { + t.Fatalf("copy the published artifact: %v", err) + } + + if err := out.Close(); err != nil { + t.Fatalf("close the staging copy: %v", err) + } + + fetcher := &artifactFetcher{si: si, stagingDir: staging} + + if err := fetcher.decompress(context.Background()); err != nil { + t.Fatalf("decompress the published artifact: %v", err) + } + + return fetcher.unpackedPath() +} + +// artifactTotalsOf counts what an artifact file holds, read directly so +// the numbers do not depend on anything the client does. +func artifactTotalsOf(path string) (artifactTotals, error) { + db, err := sql.Open("sqlite", "file:"+path+"?mode=ro") + if err != nil { + return artifactTotals{}, err + } + + defer func() { _ = db.Close() }() + + var totals artifactTotals + + err = db.QueryRow(`SELECT COUNT(*), COALESCE(SUM(popularity > 0), 0) + FROM explore_index`).Scan(&totals.rows, &totals.withListen) + if err != nil { + return artifactTotals{}, err + } + + return totals, nil +} + +// indexTotalsOf counts what the client ended up with. +func indexTotalsOf(db *database.DB) (artifactTotals, error) { + var totals artifactTotals + + err := db.QueryRowWriter(`SELECT COUNT(*), COALESCE(SUM(popularity > 0), 0) + FROM explore_index`).Scan(&totals.rows, &totals.withListen) + + return totals, err +} -- 2.54.0 From 1997276def890c13d46fbf95ffefecb08d07b51b Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Fri, 25 Sep 2026 09:27:42 -0400 Subject: [PATCH 07/16] docs: cut CLAUDE.md to the rules it is for CLAUDE.md had grown to 4,046 lines, ~3,300 of them per-component write-ups: why a breakpoint is 500px, why a cap is a quarter, what a spec once missed. Every session loaded all of it, and the rules that apply to every change were buried among decisions that apply to one. Those write-ups were already duplicated as comments beside the code they describe -- every issue number they cite also appears in a code comment -- so they are deleted rather than moved. What is left is the tracker workflow, the commands, the verification tiers and a set of broad engineering rules distilled from them, each pointing at where its example lives. Three declined decisions recorded nowhere else go to NOTES.md, and three code comments that cited CLAUDE.md sections by name no longer do. Closes #256 Co-Authored-By: Claude Opus 5.5 --- .planning/NOTES.md | 19 + CLAUDE.md | 4332 ++--------------- e2e/specs/phone-search.spec.ts | 4 +- .../test/components/progress-line.test.ts | 3 +- main.go | 3 +- 5 files changed, 354 insertions(+), 4007 deletions(-) diff --git a/.planning/NOTES.md b/.planning/NOTES.md index 8e4ba30..b429d49 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -5078,3 +5078,22 @@ last rendered card. ("ask the virtualizer for a larger overscan") is therefore not available without patching a private, which is why the request is issued ahead of the element instead. + +## Declined, and recorded nowhere else (2026-09-25, #256) + +When CLAUDE.md was cut down to rules, most of its "considered and +declined" paragraphs already had a home in a code or config comment +beside what they explain. These three did not: + +- **`touch-action: manipulation` was declined** (#54). The 300ms tap + delay it is offered for is already absent on a `width=device-width` + viewport; what it would actually change is the gesture stack #63 tuned + by measurement on the reference device. +- **There is no "Go to Genre"** in the phone row menu (#67). That menu + replaces name links a phone cannot use, and there has never been a + genre link to replace — it would be new navigation, which wants its + own issue. +- **The overlaid queue has no tap-outside gutter on a phone** (#171). + The drawer-style gutter would buy the affordance by taking width off a + full-screen surface on a 424px viewport; back and a 44px close button + answer it instead. diff --git a/CLAUDE.md b/CLAUDE.md index 2512365..3ee96cc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,153 +1,57 @@ # CLAUDE.md -This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. +Guidance for agents working in this repository. `AGENTS.md` is a symlink +to this file (`make skill-check` asserts it). -## Project +YellowJacket is a music player for desktop Linux/macOS and Android: Go +backend, TypeScript/Lit frontend, bridged by Wails v3. Plays MP3, FLAC, +OGG Vorbis and WAV. -YellowJacket is a cross-platform desktop music player built with Go (backend) and TypeScript/Lit (frontend), using the Wails framework to bridge them. It supports MP3, FLAC, OGG Vorbis, and WAV playback. +**Where things are written down.** `README.md` is for users; +`CONTRIBUTING.md` is setup, build and the contributor workflow. This +file is the rules. The *reasons* for a specific shape live in a comment +beside the code that has it — the code here is heavily annotated, and +the comment usually cites the issue — so read the comments of what you +are changing before changing it. `.planning/NOTES.md` holds dated +measurements, gotchas and rejected ideas; the `yellowjacket-dev` skill +(`.pi/skills/yellowjacket-dev/`) holds the operational how-to for each +test tier. Packaging channels keep their own documents +(`packaging/*/README.md`, `docs/android-release.md`). -**The three prose documents are split by reader, not by topic** (#50). -`README.md` is the landing page and answers *a user's* questions only — -what it does, which channel installs it on which platform, where its -data lives — with three screenshots in `docs/images/`, captured from the -fixture library (`make sandbox-seed NAME=default` → `make dev-headless -SEED=default`) so they can be retaken by anyone. `CONTRIBUTING.md` holds -what used to be the second half of that README — prerequisites, the -system libraries, the build and codegen commands, which verification -tier a change demands, the tracker workflow and the commit grammar. This -file stays the deep reference both of them point at, and is the only one -of the three that explains *why* a shape is what it is. A fact that -belongs to a user goes in one place; the packaging channels keep their -own documents (`packaging/*/README.md`, `docs/android-release.md`) and -are linked rather than summarised, because a version-restart note copied -into the README is a second copy to keep true. +## Issues and workflow -## Issues +The Gitea tracker is the source of truth, shared with a collaborator +who cannot see this session. `scripts/issue.sh` is the whole interface +(`list`, `mine`, `search`, `show`, `new`, `claim`, `unclaim`, +`comment`, `close`, `label`, `depends`, `labels`; needs `GITEA_TOKEN` +with `write:issue`). -**The tracker is the source of truth for what is wanted and what is -already being worked on**, and it is shared with a collaborator who -cannot see this session. `scripts/issue.sh` is the whole interface to -it (`list`, `mine`, `search`, `show`, `new`, `claim`, `unclaim`, -`comment`, `close`, `label`, `depends`, `labels`); it needs a -`GITEA_TOKEN` with `write:issue`. +- **Search before starting** (`issue.sh search `, which covers + closed issues too). **If nothing covers the work, open an issue + first.** +- **Claim before the first edit.** `claim` sets the assignee, + `Status/In Progress` and a comment naming the branch and approach. If + someone else holds it, talk to them rather than working around it. +- **File findings.** A bug found along the way, or work deliberately not + done, is an issue with a reproduction — not a sentence in chat. +- **Labels are a taxonomy**: `Kind/*`, `Area/*`, `Priority/*`, + `Platform/*`, `Reviewed/Confirmed`, `Status/*` (`Status/*` and + `Reviewed/*` are exclusive). **#73 is the roadmap**; work in its + order. Hard blockers are Gitea dependencies plus `Status/Blocked`. +- **`Closes #n` goes in a commit body, one issue per line.** Gitea does + not parse PR bodies, and a comma list half-works. After a merge, run + `issue.sh list --state open` and close whatever did not take. Unclaim + happens automatically on close (`.gitea/workflows/unclaim.yml`). +- **`main` is protected**: feature branch and PR only. The issue number + goes in the branch name and PR body, never the commit subject. A PR + body carries a commit-to-issue table, the verification actually run, + and a `Closes` list (PR #83 is the shape). -**Search the tracker before starting any work, and claim what you -find.** Fifty-odd issues make that a real lookup rather than a -formality. `./scripts/issue.sh search ` covers open and closed — -closed matters, because "that was fixed three weeks ago" is the -cheapest possible answer. - -**Claiming happens before the first edit, not before the commit.** The -whole point is that the collaborator can see the work is taken *while -it is being done*, so `claim` sets the assignee, applies -`Status/In Progress` and posts a comment naming the branch and the -approach — all three, or none. It refuses outright if somebody else -already holds it, and that refusal is the feature: talk to them rather -than working around it. - -**If no issue covers the work, open one first.** The issue exists -before the branch does. That is what makes the tracker a description -of the project rather than a description of the past. - -**Findings get filed.** A bug tripped over while doing something else -is an issue with a reproduction, not a sentence in a chat message -nobody can search. So is a piece of work deliberately not done — the -issue is where "we decided not to, and here is why" survives. - -Four conventions are already established and are not up for -reinvention: - -- **The labels are a taxonomy**, not tags: `Kind/*`, `Area/*`, - `Priority/*`, `Platform/*`, plus `Reviewed/Confirmed` (the code was - read and the defect confirmed) and the `Status/*` family. `Status/*` - and `Reviewed/*` are **exclusive scopes** — one of each at most, so - applying a second replaces the first. -- **#73 is the roadmap.** It states the order the backlog should be - worked in and the soft relations that are not expressible as - blockers. Picking work off the open list by eye when a meta issue - states the sequence is how the sequence stops meaning anything. -- **Hard blockers are real Gitea dependencies**, which render on the - issue itself, and the blocked issue carries `Status/Blocked`. -- **A PR body carries a commit-to-issue table, the verification - actually run, and a `Closes` list** — PR #83 is the shape. That list - is for whoever reads the PR; what actually closes an issue is the - footer below. - -**The closing keyword goes in the commit body, one issue per line.** - -``` -docs: delete four documents that contradict the code - - - -Closes #98 -``` - -**Gitea parses commit messages that reach `main`; it does not parse the -PR body**, which only closes anything if the merge happens to copy it -into the merge commit. Both halves of that were measured. #83's merge -commit carried `Closes #9, #13, #14, …` and closed **five of ten** — a -comma list is partially matched. #93's merge commit body was one -`Reviewed-on:` trailer, so #92 stayed open behind a perfectly correct -`Closes` line in the PR description. - -A footer costs nothing elsewhere: Conventional Commits allows one, -`scripts/commit-check.sh` only regexes the subject, and -semantic-release reads the type from the subject — so this changes no -release decision. The rule that the issue number stays out of the -**subject** is unaffected, and was never about the body. - -**Check it anyway.** A squash, or a merge message edited by hand, -still drops the footer. `./scripts/issue.sh list --state open` after a -merge, looking for what you just shipped; `./scripts/issue.sh close -` for whatever did not take, with a comment naming the commit. - -**Unclaiming is automatic, and it is hooked to the close rather than -to the merge.** Gitea's auto-close changes state and nothing else, so a -footer left `Status/In Progress` on a closed issue — #100 was closed -and marked as being actively worked on at the same time. -`.gitea/workflows/unclaim.yml` runs on `issues: [closed]`, which covers -the footer, `issue.sh close` and a click in the web UI alike; stripping -the label in the PR instead would have been a per-PR habit, and habits -are what the footer removed. It is not instant — the runner has -capacity 1 — and reopening deliberately does not restore the label. -`./scripts/issue.sh list --state closed --label "Status/In Progress"` -is how you find out it has stopped firing. - -## Planning - -`.planning/` is **design documents and measured history**, not a queue -— the queue is the tracker, and a plan file that describes work nobody -has started is a second, staler answer to "what are we doing next". - -- `.planning/NOTES.md` — gotchas, measured facts, open architecture - questions, and the "we already considered and rejected" list. Dated, - because several are properties of someone else's server. **This is - where a decision reached on an issue gets written down** when it - outlives the issue. -- `.planning/plans/completed/` — one recap per shipped milestone, kept - for the arguments in it. Where a plan shipped incompletely, its - header says which issue carries the remainder. -- `.planning/audits/` — the read-only audits that produced the - reconciliation plans. Historical evidence; not a backlog. -- `.planning/plans/active/` — a multi-phase design document for work - **in flight**, linked from the issue that tracks it. Empty is the - normal state. There is no `pending/`: a plan nobody is executing is - an issue. - -Numbering is sequential and stable across status moves (a plan keeps -its `NNN-` prefix). Abandoned plans are deleted. - -**The autonomous loop** (plan 020, `.pi/skills/yj-loop/`) is the pi -configuration that works the tracker one issue at a time — a cron tick -in a dedicated worktree and session, with the tracker labels as its -state machine. It claims with `issue.sh` like anyone, merges only PRs -it opened once the protection contexts are green, and files what it -finds. Its switch is `.pi/schedule-prompts.json` (gitignored): it runs -only while that pi session is open, and that limitation is the whole -on/off design. Where a loop discovery contradicts this file, this file -is wrong and should be fixed by the diary leg — the loop never quietly -decides otherwise. +`.planning/` is design documents and measured history, not a queue: +`NOTES.md`, `plans/completed/` (one recap per milestone), `audits/`, +and `plans/active/` (in-flight work only; usually empty). A decision +reached on an issue that outlives it goes in `NOTES.md`. The autonomous +loop (`.pi/skills/yj-loop/`, plan 020) follows the same rules. ## Commands @@ -166,7 +70,8 @@ make perf-compare BEFORE= AFTER= # Print the before/after table make build-dev # Debug build with symbols make build-prod # Production build (stripped and trimmed; no UPX) make generate # Run code generators (sqlc + templ via go generate) -make e2e # Playwright smoke suite against a running dev-headless app +make bindings # Regenerate frontend/bindings (wails3) +make e2e # Playwright suite against a running dev-headless app make e2e-setup # Install the e2e runner + its browser (once) make ui-test # Vitest component/store suite in a real browser (no app) make ui-visual # Same, including toMatchScreenshot comparisons @@ -174,3873 +79,298 @@ make ui-setup # Install the Vitest provider's own Chromium (once) make bindings-check # Fail if frontend/bindings is stale vs the Go bindings make skill-check # Fail if a doc names a make target that doesn't exist make commit-check # Fail if a commit subject is not a Conventional Commit +make css-check # Fail on a nested CSS rule Chrome 113 would drop make lint # golangci-lint v2 (strict), all three build configurations make test # All tests with race detector, all three build configurations make vulncheck # govulncheck for CVEs +make release-dry # What a release from here would ship make setup # Install go tools, frontend deps, git hooks (lefthook) ``` -### Running tests - -Go test commands need no build tag for the app configuration. The -`webkit2_41` tag every command here used to carry is gone with wails -v2: v3 builds against GTK4 + WebKitGTK 6.0 by default, which both Arch -and ubuntu:24.04 ship. - -```bash -go test ./... # All tests -go test ./backend/player/ # Single package -go test -run TestName ./backend/player/ # Single test -``` - -The central index builder is behind a second tag and is **not** covered -by the command above — `make test` runs both passes, but a manual run -needs it spelled out: +Go tests: `go test ./...`, or `go test -run TestName ./backend/player/`. +Two tagged passes are **not** covered by `./...` (`make test` runs all +three): ```bash go test -tags indexbuild ./backend/explore/... ./cmd/... -``` - -`backend/testctl` is behind a third tag and needs its own pass too -(`make test` runs all three): - -```bash go test -tags dev ./backend/testctl/... ``` -Audio playback integration tests require `YELLOWJACKET_INTEGRATION=1`. - -### Fixtures and the headless harness - -`test_data/music_library_test/` is **generated, not committed**: run -`make testdata` (~1 s) before anything that needs audio. Tests reach it -through `internal/testfixtures`, selecting files by *case* -(`CaseCoverDedup`, `CaseUnicode`, `CaseDuplicates`, …) rather than by -path, and skip themselves when it has not been generated. - -The app itself can be run without a blocking window — `make -dev-headless` — and driven with `playwright-cli` against it on -`:34115`. That is wails v3's first-class `-tags server` mode: the real -app, the real bindings, the same Go backend a desktop window would use, -served over HTTP with **no display at all**. The Xvfb this used to -require is gone, from the script and from CI; `dbus-run-session` stays, -for MPRIS. - -**The operational half of all this lives in the -`yellowjacket-dev` skill** (`.pi/skills/yellowjacket-dev/`): which tier -to reach for, the exact command sequences, seed lifecycle, and the -failure modes worth knowing before you meet them. It is deliberately -not repeated here — this section describes what exists, the skill says -what to run. - -Two things ride on top of the headless launch, both from plan 005 -phase 3: - -- **The event bridge.** `.playwright/cli.config.json` loads - `.playwright/init-events.js` as an `initScript`, which records every - backend event on `window.__yjEvents`. Half this app is push-driven, - so assertions **await an event, not a timeout**: - `await window.__yjEvents.wait('LibraryScanComplete', {timeoutMs: 60000})`. - It also provides `ready()` and `call('queue.Queue.GetState', [])`. - Both hook v3's own seams rather than an internal: inbound is - `window._wails.dispatchWailsEvent`, the entry point the backend's push - uses, and outbound is **`fetch`** — v3 routes every runtime call - through one POST to `/wails/runtime`, so one hook sees calls from any - module and cannot miss one made before the harness looked. `call()` - posts by method name, so it depends on nothing in the app's bundle and - works on a page with no init script — which is why `seed-sandbox.sh` - is `curl` now and needs no browser. It no longer races a timeout - either: v3 rejects bad arguments and unknown methods cleanly. -- **The dev-only control surface**, `backend/testctl`, mounted at - `/__test/` on the same port: `health`, `db/snapshot`, `db/restore`, - `emit` (force any backend event, which renders push-driven views - without staging the work that would produce them) and `sql`. It is - compiled out of non-dev builds and additionally requires - `YJ_TESTCTL=1`, which `dev-headless.sh` sets and `make dev` does not. - -Frozen regression specs live in `e2e/` (its own npm package, so the -Vitest browser mode does not share a package with the Playwright -runner): `make e2e` against an already-running app. - -**A fifth tier exists for questions whose answer is a number**, not a -pass: `make bulkdata` generates a ~50 000-track library (11 s, 466 MB, -gitignored), `make sandbox-seed-bulk` seeds from it by running the app -like any other seed, and `make perf LABEL=x` measures startup, the -bundle's shape and each view's first open, keystroke cost, what a -finished track provokes, what one favourite toggle costs, what sitting -idle on Settings costs, and heap after a scripted browse. A measurement -that needs state the seed does not have stages it itself, idempotently, -so a before and an after see the same shape — the favourite number is -meaningless against the seed's one empty playlist, so it builds ten -500-track ones first. -It wraps every bound Go method, so "did that refetch the library" is a -fact rather than an inference. It is not a spec and does not run in CI. - -**The cheapest tier needs none of that.** `make ui-test` runs 776 -Vitest tests in a real Chromium with no Wails, no backend, no seeded -library and no virtual display, because **v3 routes every runtime call -— bindings, event emits, window, dialogs, clipboard — through one IPC -transport**, and `frontend/test/support/wails-fake.ts` replaces it via -`setTransport()`, which is public documented API. So the fake covers -strictly more than v2's two globals did while being shorter, and the -tests exercise the real generated bindings, the real runtime and the -real store code. A binding carries an **ID**, not a name -(`$Call.ByID(2822423495)` is FNV-1a over -`yellowjacket/backend/home.Service.GetShelves`), so the fake derives -that map from the generated tree rather than writing it down. - -**A test file does not get its own origin, so `setup.ts` clears -`localStorage` between tests.** `@vitest/browser-playwright` opens one -BrowserContext per session and runs several files in it one after -another, so everything a component persists — the track list's sort and -column widths, the cover size, `now-playing`'s scroll mode — is still -there when the next file mounts the same component. Which files share a -tab, and in what order, changes run to run, so the symptom is a spec -that fails about one test in three and passes every time it is run on -its own: #138 cost three scheduled runs, one of them a PR whose diff -held no frontend code at all. Measured on the build before the fix, a -single full run started **24** tests with storage already set. Two -things follow. The clear is safe precisely because the leak is -sequential — files in a session do not overlap, so it cannot wipe -storage a concurrently-running file is in the middle of using — and it -belongs in `setup.ts` rather than in the specs that write, because the -spec that *reads* is never the one that knows. And a spec whose -assertion depends on an order still **states that order** rather than -inheriting a default, or the next change to a default is the same -mystery again. - -**`frontend/bindings/` is generated by `wails3`, not `go generate`**, so -the pre-commit codegen check does not cover it. `make bindings-check` -(~3.5 s warm, ~20 s on a cold build cache, also a pre-commit hook) -regenerates it and fails on a dirty tree; `make bindings` regenerates it -for real. It is slower than v2's because v3's generator is a **static -analyser** over the whole package graph rather than runtime reflection -— which is also why the tag set it runs under matters, and why it is -the *default* one: that is the configuration users run, and neither -`indexbuild` nor `dev` adds a bound service. See -`scripts/bindings-check.sh`. - -**Seeds are produced by running the app**, never by hand-writing a -`config.toml` and DB rows — the same discipline `sql/schemas/` gets, -for the same reason. - -See `.planning/plans/completed/005-agent-development-harness.md`. - -## Architecture - -**Wails app lifecycle** (`main.go` → `backend/app.go`): `main.go` is -`application.New(opts)` → `app.Window.NewWithOptions(…)` → `app.Run()`. -Each bound service takes its context from `ServiceStartup(ctx, -application.ServiceOptions{})` — v3 calls it on every service, in -registration order, on the main goroutine — and gives it back in -`ServiceShutdown()`. That context is **cancelled on app shutdown**, -which `SetContext` never was. - -Three things about it are load-bearing. - -**`ServiceShutdown()` takes no context.** A method with a -`context.Context` parameter does not satisfy the interface and is -**silently never called** — no error, no warning. - -**The cross-service wiring is a service, not an event.** v3 has no -`OnStartup`/`OnDomReady` option, and the obvious replacement — -`app.Event.OnApplicationEvent(events.Common.ApplicationStarted, …)` — -is the right *moment* and the wrong *mechanism*: **server mode emits no -application events at all** (`setupCommonEvents` is an explicit no-op -under `-tags server`), so the desktop build wired itself and the -headless harness did not. `backend/startup.go` is registered last -instead, which takes the ordering from the mechanism rather than from -an event and therefore holds in every mode. Anything else keyed on -`Common.*` is suspect for the same reason. - -**The quit veto is asynchronous now.** v2's `MessageDialog` blocked and -returned the button; v3's `Show()` returns immediately and the answer -arrives on a `Button.OnClick` callback, so `ShouldQuit` cannot ask and -answer in one call — it vetoes, shows the dialog, and calls -`app.Quit()` from the callback. `quitConfirmed` is what stops that -second `Quit()` asking again; `quitAsking` stops a second close attempt -stacking dialogs. Window state moved off that path entirely, onto a -`Common.WindowClosing` hook, because the size has to be read while the -window still exists and `OnShutdown` has neither a context nor a -window. - -**An activity is a view onto the process, and `main()` runs once per -process.** On Android the Wails entry point is `nativeInit`, which -`MainActivity.onCreate` calls — and it does two things: it re-points the -native library's global JNI reference at the calling `WailsBridge`, and -it runs `go mainFunc()`. Android destroys and recreates an activity -**without restarting the process** (a configuration change the manifest -does not declare, memory pressure, or every background under "Don't keep -activities"), so `main()` ran again on a live app. `application.New` -returns the *existing* app rather than building a second one, -`app.Run()` then refuses — `a.starting` is still true, because Android's -`platformRun` is `select{}` and never returns — and the `os.Exit(1)` -under that error took the **first**, healthy app down with it: its -database, its queue, and the audio a `mediaPlayback` foreground service -was holding the process alive to play. `mainStarted` latches it, first -statement in `main()`. - -Four things about it are load-bearing. - -**The answer to "restore the session or cold-start" is settled by -playback, not by preference.** The audio lives in the Go process, so a -cold start on every activity recreation would stop the music mid-song — -which is the exact thing the foreground service exists to prevent. The -activity is a view; the app is the process. The frontend already -cooperates, because a recreated WebView loads the page fresh and fetches -its state from a backend that never went away. - -**Returning early is not a degraded mode, and that is why the latch is -in Go rather than in Java.** The obvious fix — making -`WailsBridge.initialized` `static`, so the second `nativeInit` is -skipped — keeps the process alive and silently breaks the app, because -skipping `nativeInit` skips the reference re-point too: Go would keep -executing JavaScript against the *destroyed* activity's WebView, and the -app would open, render, and never receive another backend event. The -latch lets `nativeInit` do its first job and declines only its second. - -**`ServiceShutdown` has never run on Android**, and nothing should be -built on the assumption that it will. `App.Quit()` reaches an -`androidApp.destroy()` that is an empty method, and `Run()`'s deferred -`shutdownServices()` cannot fire behind `select{}`. Durability on this -platform is the persist writers, which submit on every mutation rather -than at exit — which is also why `MainActivity.onDestroy` no longer -calls `bridge.shutdown()`: the activity going away is not the app -shutting down, and there is no callback for the process going away -because Android simply kills it. - -**No tier here can see any of this**, so the guard is split. A source -sweep (`TestMainClaimsBeforeItDoesAnything`) asserts the latch is the -*first* statement of `main()` — the failure it exists for is not -deletion, which is loud, but a line creeping in above it, since a second -`NewYellowJacketApp` opens the SQLite database again on every -recreation. The rest is a documented device check in -`.pi/skills/yellowjacket-dev/references/android-tier.md`, with the -logcat signature and a one-line way to force a recreation. - -`internalServiceMethods` auto-excludes `ServiceStartup`, -`ServiceShutdown`, `ServiceName` and `ServeHTTP` from bindings, so this -shape **removed** 12 spurious bindings and the bogus `context` model -rather than renaming them. - -**Backend packages** (under `backend/`): -- `player` — Audio playback via beep. `BufferedStreamer` provides a ring buffer for smooth seeking. -- `queue` — Track queue with shuffle (Fisher-Yates), repeat modes, auto-advance, and session persistence. -- `library` — Concurrent library scanning, metadata extraction, cover art deduplication, incremental rescan. Also **removal**, below. -- `database` — SQLite via pure-Go driver. Schema in `database/sql/schemas/`, queries in `database/sql/queries/`. **sqlc** generates Go code into `database/sql/sqlcgen/` — never edit that directory by hand. - - **The schema is one description, and there is no migration chain.** - `sql/schemas/*.sql` is `CREATE ... IF NOT EXISTS`, declares the - current shape of every table, and is what sqlc reads and what an - install gets verbatim. That is the whole mechanism: `sql/migrations/`, - `applyMigrations` and `schema_migrations` were squashed away with the - file-shaped rewrite (plan 013). A schema change is one edit to one - file plus `make generate`. Reintroducing a chain means reintroducing - the drift it caused before — `sql/schemas/` and the migrations - disagreed, and sqlc generated against the stale one. - - **What that costs an existing database is repaired once, at open.** - `CREATE ... IF NOT EXISTS` reaches an existing table only if its shape - already matches and otherwise silently no-ops, so a *changed* table - never migrates. Plan 014 added `total_tracks` to `explore_index` and - to `indexRowFields` — the projection every explore read uses — and no - database that already existed grew the column: **every** Explore - search, browse, artist and album page on such an install failed with - `no such column: total_tracks`, while a fresh install was perfectly - healthy, which is exactly why no test saw it. Plan 013 was worse on - the same install: `applySchema` could not be applied at all over a - pre-013 `audio_files`, so the app did not open. - - `backend/database/staleshape.go` runs before `applySchema` and - retires what is stale, so the create is a create. Five things about - it are load-bearing: - - **It parses `sql/schemas/` for the expectation** rather than - writing the column list down a second time, because a second list - is a second thing to forget — the fault it exists to repair. - - **It notices a changed *type*, not just a missing column.** 013 - moved `mbid` from TEXT to BLOB, and SQLite does not coerce between - them: a comparison against 16 raw bytes returns no rows rather than - an error. `ALTER TABLE ADD COLUMN` would have handled - `total_tracks` alone and cannot express this at all, which is why - the repair drops rather than migrates. - - **`Authored` is never retired**, and that boundary is a test - (`TestAuthoredTablesAreNeverRetired`), not a comment. Everything - else is rebuildable: `Cache` by definition, `Owned` by a rescan — - plan 013's stated "delete and rescan" — and `Derived` from Owned. - A table the schema no longer describes at all goes too; 013 left - seven behind plus `schema_migrations`. - - **Whether a stale `Cache` table may be rebuilt is a build tag**, and - it is the most expensive thing in this file to get wrong. In the app - the catalog is *downloaded*, so a wrong shape costs a minute of - re-fetching the artifact and keeping it costs every Explore read. In - `cmd/indexbuild` the catalog is *derived*, and the only way back is - the ~205 GB dump stream the `/cache` volume exists to avoid — so - `retireStaleCache` is false there (`staleshape_policy_indexbuild.go`) - and `TestTheCatalogSurvivesAStaleShape` fails the moment it is not. - `TestNoCacheTableIsRetiredHere` is the same assertion made of *every* - `datamap` Cache table rather than one, because the risk is not that - shape recurring — it is the next destructive repair added to - `database.NewDB`, the chokepoint every binary here shares, without - asking which binary it is in. - This is written down because it already happened: the repair shipped - without the distinction and dropped the real CI catalog on its first - run, with `reason="column entity_type is TEXT, schema declares - INTEGER"`. The mismatch was genuine — that database is deliberately - kept in the older encoding, which `fix(indexexport): read an index - older than the binary` exists to tolerate — so it would have been - dropped on *every* run. The consequence is that a future - `explore_index` column fails the index job loudly on `applySchema` - rather than silently costing it a rebuild, which is the trade a - human should get to make. - - **The drops are one transaction with `defer_foreign_keys`.** Those - legacy tables reference each other, so dropping them in any order - fails on whichever goes first, and turning foreign keys *off* - instead would silently take `playlist_tracks.audio_file_id`'s - ON DELETE SET NULL with it — leaving playlist entries pointing at - ids a rescan reissues to *different songs*. Nulled entries are - empty; stale ones are wrong, and wrong quietly. - - **The order is sorted, so a failure reproduces.** Map order is - random, and the foreign-key bug above passed its own regression - test on two runs in three until the order was fixed. - - Retiring `explore_index` takes its FTS and its meta with it, because - the `dump_import_done` marker is what would otherwise stop the - artifact ever being fetched again. - - **What that costs an existing database is that it does not open**, and - "delete and rescan" is the answer (plan 013, open question 1) — free - for everyone except one machine. The index job's `/cache` volume is a - real `YJ_HOME` that survives between runs, and half of it is the - catalog: deleting it means re-downloading ~205 GB. So `cmd/indexbuild` - repairs it instead (`staleschema.go`), dropping every table `datamap` - does not classify as `Cache` **before** the schema is applied. Nothing - scans, plays or authors in that database, so its non-catalog half is - empty by construction and a shape left over from an older schema is - pure liability. 013's reshaped `audio_files` failed every run of that - job on `CREATE INDEX ... album_id` against the old table until this; - `TestRetireLibraryTables` reproduces exactly that, symptom first. - - **The local library is shaped like files, not like MusicBrainz.** - `audio_files` carries its own tags — title, artist credit, track and - disc numbers, year, composer, the recording MBID — and points at two - shared rows: `albums` (many files to one) and `artists` (many albums - to one). `file_genres` is the one genuine many-to-many. That is the - entire local model. - - It used to be MusicBrainz's: `recordings`, `release_group_recordings`, - `artist_credit` and `artist_credit_artist` sat between a file and its - own tags. Measured on a real 25,966-file library, **every** - many-to-many that model expressed was 1:1 in the data — no recording - had two files, none belonged to two release groups, and 3 credits of - 2,823 listed more than one artist. What it cost was a six-way join in - every read, a `MIN(release_group_id)` subquery in eleven queries and a - first-credited-artist subquery in nine to undo fan-outs that never - happened, and a class of bugs where a metadata row **outlived the file - that created it**: retagging a file created a new recording and - abandoned the old one, so that library carried 812 orphaned - recordings, 216 release groups and 260 artists — and 129 catalog rows - that confidently claimed to be owned by files that no longer existed. - - A few things that follow, and bite if forgotten: - - **Ownership is a file.** "Do I own this" is asked of `audio_files` - and nothing else — `GetFilePathsByRecordingMBIDs`, - `LibraryMBIDIndex.CheckMBIDs`, `collectLibraryEntities` and - `pruneStaleLocalCrossReferences` all join it. A metadata row is not - ownership; that was the bug, and it is now structurally impossible - for a row to exist without its file. - - **The projection is defined once, in the `track_metadata` view.** - Every query that returns a track selects from it, which is why - there is one row type (`sqlcgen.TrackMetadatum`) and one mapper - (`trackFromRow`). It existed before and only the raw-SQL search - paths used it, so nine hand-rolled copies had already drifted: the - view preferred the album's original year and one copy used the - track's, and the same library reported different years on different - screens. The FTS searches cannot be sqlc queries (MATCH is not in - its grammar) and spell the column list out in `search.go` — that is - the one exception and it is one constant. - - **`library_id = 0` means every library.** Each list query used to - exist twice, scoped and unscoped, with a branch at every call site - and a separate binding for each. One query answers both, and the - scoped form costs nothing measurable (23 ms against 21 ms over 26k - rows). - - **A slice and a named parameter do not compose in sqlc.** - `sqlc.slice` expands to N placeholders but a named argument is - numbered independently, so `GetFilePathsByAlbums([1,2], 0)` read - album id 2 as the library id. Where a query needs both, return - `library_id` and filter in Go (`inLibrary`). - - **A cache without a ceiling is a leak with a schedule.** Every - store that grows with use declares a budget beside its retention - (`browsedArtBudget`, `httpCacheBudget`), because an age bound does - not bound anything a user can outrun in an afternoon. - - **A query file must be ASCII.** sqlc's parameter rewriter works on - byte offsets, so one non-ASCII character in a *query* comment - corrupts the generated Go into garbage like `SELECid`. Schema files - are not rewritten and may contain anything. - - **A view is dropped and recreated, not migrated.** - `CREATE VIEW IF NOT EXISTS` no-ops against a database holding the - old definition, so `track_metadata.sql` opens with - `DROP VIEW IF EXISTS`; a view holds no data, so rebuilding it on - every open costs nothing. - - **A write wearing a query's shape still needs the writer.** - `DB.QueryContext`/`QueryContextWith`/`QueryRow` route to a - *query-only* read pool (a second `sql.DB` over the same file), so - an `INSERT ... RETURNING` issued through one fails at runtime with - "attempt to write a readonly database (8)" — which is exactly what - `CreateSmartPlaylist` did, meaning no smart playlist could be - created at all. Use `ExecContext`, or `QueryRowWriter` when the - statement really does return a row. Nothing caught this because - `NewTestDB` shares one in-memory connection and leaves `readDB` - nil, so `reader()` returns the *writer* under test. - `TestNoWritesOnTheReadPool` walks the tree for it, in the same - spirit as `TestNoDirectRuntimeEmits` and for the same reason — a - lint pass only sees one build configuration. - - **A new table has to say what kind of data it holds.** - `backend/datamap` is a catalogue of every table's Kind and - Lifetime, and `TestCatalogCoversSchema` fails on a table missing - from it. `TestAuthoredCascadesAreDeliberate` then makes an - *authored* table that cascades an explicit, argued exemption — - authored data is what a user cannot get back. Two entries say - **MIXED KIND** and mean it: `audio_files` is an owned projection - except for `play_count`, `last_played` and `tag_status`, which are - authored; `lyrics` carries a `source` column because a lyric read - from a tag is free to rebuild and one fetched from LRCLIB is not. - - **Test data has one seeder.** `database.InsertTestTrack` inserts a - file with its artist, album and genres. Twenty test files used to - carry their own, each assembling the old FK chain in a slightly - different order. -- `metadata` — Tag extraction (ID3v2, Vorbis Comments, FLAC). -- `jobs` — The registry every long-running operation reports through: - progress, pause/cancel, a global indicator and (for scans) a pause - that survives a restart. Library scans, index builds, downloads and - the autotag apply are registered; anything that is not registered has - none of that, which is exactly how the three gaps the audit found - came about. - - **Its rows are shown where the work is started, not on a page of - their own.** #27 folded the Jobs destination away, and the shape it - folded into is `` embedded four times — scans in - Settings → Libraries, index and enrichment in Settings → Search - Index, downloads under the download clients, the autotag apply in - `autotag-view`. One "Background jobs" section in Settings was the - obvious reading of the report and is the tab again under another - name. - - Four things about it are load-bearing. **Four of the five kinds - already had a home** that showed their work — the tier list, the - download list, the apply ring — and what none of them had is the - *generic* affordances, so the panel carries pause, cancel, Details - and the log to each rather than replacing what is there. **The - controls are `applyJobControl`**, not a reimplementation, which is - what keeps the "you will discard hours of downloading" confirmation - alive: it is keyed on `KindIndexBuild` inside the shared handler, and - a host drawing its own buttons would drop it silently. **A panel with - nothing to say is `hidden`**, host margin included, because an idle - panel in four places is four pieces of furniture describing an - absence. And **there is no "Clear finished"** in it, because - `ClearFinishedJobs` is global — a Clear under Libraries would discard - the index build's history too; a finished row dismisses itself. - - The header `job-indicator` is still the one view of everything at - once, from every page — **on a desktop.** One consequence worth - knowing before writing a spec: a section holding a `job-panel` also - holds a `job-details-drawer`, whose own header carries `.header` — so - `config-section .header` is ambiguous the moment a job exists. - - **Below 600px that indicator stands down and `` takes - over** (#62), because a popover is a *disclosure* and background work - is the one thing a phone should not make you open something to see — - and because #57 deletes the bar it is anchored to and is blocked on - it having somewhere else to live. The band is the same `job-panel`, - so `applyJobControl` and its index-build confirmation come along - rather than being reimplemented; `kinds="*"` is how it says "every - kind", which is what the indicator was for. - - Three things about it are load-bearing. **It is in the layout, not - over it**, as its own grid row above the main panel: the first - version put it in `notification-host`'s fixed band, which reads fine - in a screenshot and is unusable — at 424×439 a compact panel is - ~200px of a 439px screen and it *covers* what is under it, which four - e2e specs caught by failing on taps it was intercepting. **It shows - active work only** (`active-only`), because in flow a finished row is - furniture that keeps the content pushed down after the work is done; - finished rows stay where the work was started, which is #27's rule. - And **it renders nothing above 600px**, from `matchMedia` rather than - a media query, because that decides whether the element *exists* — - Settings already holds four `job-panel`s and a fifth answering for - every kind is `bottom-nav`'s "resolved to 2 elements" trap again. - `index.css` keeps it `display: none` outside the phone for a second - reason: an in-flow grid child with no named area is auto-placed into - one of the shell's rows, which is what the skip link is absolutely - positioned to avoid. -- `config` — TOML-based settings. Settings page uses HTMX + templ for server-rendered HTML fragments. - - **A setter that can reject its argument puts the old value back**, and - that is a correctness rule rather than hygiene (#231). `Save()` - validates the *whole* config, so a value left behind by a failed write - does not merely fail its own call: it fails every later save, of every - unrelated setting — theme, launch page, shortcuts, libraries — for the - rest of the session. Nothing reaches disk, so a restart clears it, - which is exactly what makes the fault invisible and unreportable. One - rejected track-list column list was enough to stop the app saving - anything at all. - - Two shapes are safe and a third is the trap. A setter that assigns and - *then* validates snapshots the field first and restores it on the - error path — seven do. `SetLibraryDirectory` is the better shape where - the value can be built on its own: it validates a candidate *before* - assigning, so there is nothing to undo. And a setter whose argument no - validation inspects needs neither — the bools, the favourites playlist - id and the shortcut bindings, plus `SetViewVisible`, which refuses an - unknown, non-hideable or launch-page view up front so - `GeneralConfig.Validate` never sees one it would fail on. Which set a - new setter joins is decided by whether its own `Validate` can reject - it, not by preference. -- `playlist` / `smartplaylist` — Playlist CRUD and rule-based smart playlists. -- `mediacontrols` — OS media controls behind one `Handler`: MPRIS over - D-Bus on desktop Linux, a MediaSession on Android, a no-op stub - elsewhere. The split is by build tag and `android` implies `linux`, - so the three files read `linux && !android`, `android` and `!linux`. - Its Android half needs no JNI beyond what Wails exports — a JSON - payload out through `application.Android.StartForegroundService`, a - command event back through `WailsBridge.emitEvent` — and the Java it - talks to is `build/android/.../WailsForegroundService.java`. That - contract (payload keys, state words, command names) is in - `androidpayload.go` **without** the build tag, because a tagged file - is compiled by nothing `make lint` or `make test` runs and is - untestable off a phone. - - `OnDuck` is the one callback MPRIS does not use: Android asks for - attenuation rather than a pause when something short needs the - output. `Player.SetDuck` keeps it as an offset on top of the user's - level rather than writing through to the volume, so it cannot - accumulate and nothing persists or emits a level the user did not - choose — and it only ever fires below API 26, where the framework - does not already duck the app itself. On that platform "the user's - level" is a constant, since #64 pins it at maximum and refuses every - way to move it; the duck is the one thing that still may, and it - works unchanged because it was always an offset applied *to* that - level rather than a write of it. -- `system` — OS-specific paths (XDG on Linux, `%LOCALAPPDATA%` on Windows). -- `explore` — Catalog search and browse over `explore_index`. See below. - Its **shelves** (`shelves.go`) are the page Explore shows before - anyone types, on `home`'s terms — a shelf is a reason, it carries the - sentence that says so, and an empty one is omitted. Queries return - `explore_index` row ids and are joined back by `rowsByIDs`, so a card - has one definition (`artistFromIndex` / `releaseGroupFromIndex` / - `recordingFromIndex`, shared with the search path, which is where - they were inlined). - - Three things about it are load-bearing. **"No shelves" is three - different statements here and the page says which** — Home can omit - an empty shelf honestly, because a library with no history really has - less to say, but Explore's data is a *downloaded artifact* that can - be absent or still arriving, so `ShelfPage.State` is `ready`, - `building` or `no-index` and the empty page names the missing catalog - and points at Settings. **Whether there is a catalog is asked of the - database, not of a flag**: `GetIndexStatus().TotalRows` is refreshed - only between build tiers (0 beside a full catalog on an ordinary - launch) and `IsReady()` is set once at startup (so rows staged by a - spec afterwards are invisible) — both are the shape `emitStatus` - warns about, and one `SELECT 1 … LIMIT 1` cannot be stale. And **two - shelves with disjoint ids still repeat each other**: ordered by raw - listen count the top albums are one act and its members and the - artists row underneath was the same people, which `home`'s - duplicate guard cannot see because the rows hold different entity - types. Shelves are one album per artist and skip whoever a row above - already showed. Found by reading a screenshot. - - Two of the four shelves the plan named **cannot be built**, and the - schema decides that rather than the design: `explore_index` has no - genre column to join a genre shelf to, and `similar_artist_map` is - not in the shipped artifact and is filled lazily from the network, so - a "similar artists" shelf is empty exactly when the page most needs - content. The library-joining shelf reads `in_library`, which is set - by MBID, so it is correctly absent on an untagged library — the - fixture one included. -- `home` — The home page's "start listening" shelves. Each shelf is a - *reason* (what you played last, what you never played, a genre you - have depth in) rather than a filter, and carries the sentence that - says so. Its queries (`sql/queries/home.sql`) return album ids only - and are joined back to `GetAllAlbumsWithDetails` in Go, so the album - projection has one definition. A shelf with nothing behind it is - omitted, never rendered empty — **and so is a shelf that repeats the - one above it**, which is the same rule one step further: "On repeat" - was "Pick up where you left off" reordered, because a small library - has one signal and answers several questions with the same albums. - Two guards make that safe, and both were arrived at by breaking the - existing tests: only shelves of three or more albums are judged (two - rows of one overlap by 100% whenever they agree at all), and only - when the shelf is **not showing the whole library** — a repeat is a - fault only if a different row was possible. Measured against a fixed - shelf size instead, an 11-album library kept three identical shelves - while a 13-album one lost them. - - **The app lands here**, from `index.ts` after the stores are wired. - `index.html` still renders the track list eagerly and it is still what - paints first — it is the cached `tracks` view, so the navigation is a - class toggle plus one chunk rather than a second render of the shell. - `app-sidebar`'s default `activeView` is `home` to match, because the - sidebar does not hear a `navigate` it did not send. -- `profiling` — pprof server on `:6060`, compiled out in non-dev builds via build tags (`internal/dev/`). - -**Explore catalog** (`backend/explore/`): the searchable MusicBrainz/ -ListenBrainz catalog in `explore_index`. Deriving it from the MetaBrainz -dumps means streaming ~89 GB from a server that caps a client near -2 MB/s — half a day, for a catalog identical for every user. So that -work happens **once, centrally**, and users download the result: - -- `cmd/indexbuild` builds the catalog from the dumps; `cmd/indexexport` - cuts it down to a shippable core and stamps its provenance. - `.gitea/workflows/index-artifact.yml` runs both and publishes the - compressed artifact under a fixed `latest` version. -- The app fetches and merges that artifact (`artifactfetch.go`, - `artifactimport.go`) — about a minute, versus a day. -- Everything the app does **not** need is behind the `indexbuild` build - tag (`dumpimport.go`, `dumpcounts.go`, `dumpcatalog.go`, - `dumpproject.go`, `dumpparallel.go`, `indexpatch.go`) so it is not - linked into the binary. `dumpbuild_stub.go` is the app-side entry - point; `dumpshared.go` holds what both sides use. -- The app keeps popularity current with the daily incremental dumps - (`dumpincremental.go`), and resolves artists outside the artifact's - coverage lazily on first view. - -**The catalog stores ids as bytes, and that is a size decision.** -`explore_index` is 2,052,200 rows, and its MBIDs and entity types were -half of it: three 36-character text columns and one storing the words -"artist", "release_group", "recording" two million times. They are 16 -raw bytes and a small integer now. Measured on a real catalog, the -table and its six indexes went **780 MB to 405 MB** — the largest -single saving available in this app, and the reason a fresh install is -~0.6 GB rather than ~1.0 GB. - -`backend/explore/mbid.go` is the only place that encoding is known. -Everything above it speaks dashed strings and entity names — -`SearchIndexResult`, the bindings, the frontend — and `dbMBID` / -`dbEntityType` convert at the SQL boundary. That confinement is the -point: the alternative is blobs reaching code that has no use for them. - -Four things about it are load-bearing, and they exist because of *how* -this fails when it fails: **SQLite does not coerce between TEXT and -BLOB**, so a query comparing the column against a 36-character string -returns no rows rather than an error, and a scan into a plain string -yields sixteen bytes of mojibake. Neither is visible except as a result -that is quietly empty. - -- **The column checks itself.** `CHECK(length(mbid) = 16)` means a - stringly *write* fails at the insert that made it. It also caught - every fixture that had been using `"rh"` as an MBID; `testMBID()` - hashes a label into a real one so they stay readable. -- **The projection is one constant and one scanner.** - `indexRowColumns` / `scanIndexRow` replaced four copies of a 22-column - list and four matching `Scan` calls — four chances to decode wrongly. - `indexRowColumnsFor("i")` is the same list qualified, for the FTS join - where both sides have a `title`. -- **A query that names an entity type inline writes the code with the - name beside it** — `entity_type = 1 /* artist */`. Splicing a Go - constant in would keep them in step automatically but makes every such - query a concatenation; `TestEntityCodesAreStable` pins the mapping - instead, because it is a storage format and changing one is not a - refactor. -- **`TestStoredEncodingRoundTrips` sweeps every read path** — lookup, - top-N, exact match, FTS search, popularity batch, the CAA map — and - asserts each returns something with a dashed id. A missed conversion - site shows up there and essentially nowhere else. - -**The artifact is read in either encoding.** A published artifact -carries whichever form the exporter that built it used, and there is one -already out there in the old text form. `artifactStoresText` asks the -artifact (`typeof(mbid)`) rather than trusting a version number, and -`artifactSelectColumns` converts on the way in — one `unhex` per row on -a once-a-month import, against requiring a rebuilt artifact before a new -build can read anything. That probe **must** run on the writer: -`core` is attached to that one connection, so `QueryContext` asks a -pool where the artifact does not exist, and the error would silently -select the conversion path for an artifact that needs none. - -**Its shape is the pattern for every column added after the fact.** -`artifactHasTotals` is the same question about `total_tracks`, on the -same handle: an artifact built before a column existed is still a -perfectly good catalog, so it is *asked* and the missing column is -selected as a literal `0`. Adding the column to the importer's SELECT -list without that is how a published artifact — which nobody can re-cut -retroactively — starts failing with `no such column`. - -**A credit is ordered parts, and the string is derived from them.** A -track credited to several artists had exactly one navigable artist and -the rest were punctuation: `primaryArtist()` string-parses the credit, -strips a " feat. " clause and discards the guest, and deliberately does -not split on `&`, `with` or `,` because those live inside real artist -names ("Simon & Garfunkel"). Measured on a real 26,069-file library, -**13%** of recordings are multi-artist upstream while only **0.86%** of -files carry a structured multi-artist tag — mp3 carries *zero* files -with multiple `MUSICBRAINZ_ARTISTID` across 19,840 — so this cannot be -a tag-parsing feature. (The "3 credits of 2,823" figure that justified -plan 013's removal of the credit tables measured our own *writer*: -`cachedLinkArtist` ran once per credit, so a collaboration could never -have been recorded. Dropping the join table was still right on cost.) - -`artist_credit_part` / `artist_credit_ref` carry the decomposition for -multi-artist credits only — a single-artist credit is already -`explore_index`'s own `artist_name`, and storing those would triple the -table to say nothing. Five things about it are load-bearing: - -- **Join phrases are assembly instructions, not disassembly ones.** - `creditLink` concatenates parts, so link boundaries are known by - construction. Locating a `credited_name` *inside* the stored credit - string would reintroduce the fault this exists to fix: that string may - come from the file's tags while the parts come from the catalog, and - the two disagree for ~1 in 3 multi-artist credits (`'Skrillex feat. - Swae Lee'` tagged against `'Skrillex & Swae Lee'` upstream). -- **`credited_name` is stored per row**, never joined from `artists`: - MusicBrainz credits "Snoop Dogg" on a track by the artist called - "Snoop Doggy Dogg". Display follows the credit, navigation the MBID. -- **The lookup is keyed on the recording MBID**, which the catalog and a - local file both carry (`library.Track.RecordingMBID`), so one binding - serves Explore and the library's own lists — which is why this needed - no local table. `file_artists` remains the offline-resilience step and - is deliberately *not* declared until something writes it. -- **Absence is cached as an answer.** `credit-store.ts` stores `[]` for - a single-artist credit — *asked*, not *answered* — or the ~87% that - have nothing to decompose are re-requested on every render forever. - `request()` is per-row and coalesces into one call per frame, because - a virtualized list cannot hand over "the whole list": 50,000 rows is - 100 queries for the ~30 on screen. -- **The dump is a third source, and it had to be.** The canonical dump - CI already streams has no join phrases and no as-credited names, and - the JSON dumps cover 153,691 recordings of ~35M with *zero* overlap - against a real library. So `mbdump.tar.bz2` — 7.1 GB, ~13.7 min in - pure-Go bzip2, whose members are alphabetical, which is what lets one - pass resolve an entity's credit without buffering 35M recordings. The - pass runs on **every** mode, because a complete import means - `refresh`, which never enters the importer at all, and it reports - whether it populated anything so `changed` republishes the artifact. - -**A 0.6 GB download asks about the connection first.** `explore`'s -catalog artifact had no network awareness at all, which on a phone is a -month's data allowance spent without being asked (plan 016 B4). -`netpolicy.go` is the gate, and its shape is dictated by one constraint: -`explore` is imported by `cmd/indexbuild`, which is built with -`CGO_ENABLED=0` and must not link Wails — so the *policy* and the -*parsing* live here and are tested on every platform, while the platform -call is a closure injected from `app.go`. It is -`application.Mobile.NetworkJSON()`, not `application.Android`'s: the -latter exists only under the `android` build tag, and `Mobile`'s desktop -implementation is a stub returning `""`. - -Three rules in it are load-bearing. **An unknown answer is not a metered -one** — only mobile answers at all, so treating silence as metered would -refuse the download on every desktop. **Cellular is the only signal -available**: the runtime reports `wifi|cellular|ethernet|none` and no -metered flag, so a metered *Wi-Fi* (a hotspot, a hotel) cannot be -detected and is not refused, which is a documented gap rather than an -oversight. And **the gate runs before anything is staged**, so declining -is a no-op rather than a job in the indicator and a status the user has -to dismiss. The permission (`AllowMeteredCatalogDownload`, default -false, so an existing config is careful without a migration) is read at -the moment a download would start, so turning it on takes effect on the -next attempt rather than the next launch. - -**Background work yields, and says so in the context.** The post-scan -backfills share MusicBrainz's rate limiters with every page the user -can open, and both were FIFO — so a thousand-artist enrichment put an -album page behind an hour of queued work. -`RateLimiter.WithBackgroundLane(perSecond)` adds a second, slower lane -and `WithBackgroundPriority(ctx)` marks a caller as belonging to it: a -marked wait takes no token at all while any interactive wait is -outstanding, and is then paced at MB's own 1/s rather than the -interactive burst rate. It is a **context marker rather than a -parameter** because a backfill calls the same `MusicBrainzClient` -methods a detail page does — `GetArtistImage` takes a `ctx` for no -other reason than to carry it. One request of slippage is accepted and -documented at `waitBackground`: cancelling an already-granted -reservation is not something a token bucket can express, and the cost -is one request-time. - -The other half is that a long backfill has to be **visible and -stoppable**: `jobs.KindCatalogEnrich` and `startBackfillJob` -(`backfilljob.go`) register both backfills with progress and cancel. -Two rules in it are load-bearing. The job is registered *after* the -work is counted, because these passes are a no-op on every launch once -the library is covered and an empty job in the indicator is noise. And -the kind is distinct from `index-build` rather than reused, because -`job-controls.ts` keys its "you will discard hours of downloading" -confirmation on that kind — wrong prompt for a pass that is resumable -per artist and free to stop. - -**An owned artist's discography is fetched, not sampled.** -`BackfillLibraryDiscographies` is two fetches per owned artist, each -skipped by its own persistent mark, because they fail independently and -one boolean covering both either over-claims or forces repeats: -the ListenBrainz top release groups and recordings -(`explore_index.discog_fetched`) and the **full** MusicBrainz browse -(`artist_enrichment.browsed_at`). - -**What it does not fetch is the point.** It ran for hours against a -900-artist library and marked nothing, because three of the four things -it did per artist were work nobody had asked for. Similar artists were -fetched for every owned artist, when the artist page already resolves -them on view through `SimilarArtists` → `ensureSimilarArtistsAsync` — -which is what stamps `similar_at` now. And `indexOneArtist` reached the -MB artist lookup it wants (`GetArtistDetails` reads that cache) by -calling `GetArtistImage`, which additionally queried fanart.tv, -TheAudioDB, Wikidata and Wikipedia and downloaded up to ten full-size -portraits; `EnsureArtistRels` is the lookup on its own. The corollary -is written into the query: **`similar_at` must not be one of the -conditions** in `unenrichedLibraryArtistMBIDs`, because testing a mark -this pass no longer sets makes every owned artist a candidate on every -run, forever. - -The rest is that the pass was **serial across artists** while every -limiter that keeps us polite is per-host and idle — so one artist's -slowest upstream set the pace for the whole run. It runs -`discogBackfillWorkers` artists at once (concurrency here raises no -origin's request rate), each under `discogBackfillArtistTimeout`, -because the MB client retries a 503 five times honouring Retry-After -and one throttled artist could otherwise outlast a hundred healthy -ones. A timed-out artist goes unmarked and is retried next run, which -is what every other failure here already does. SQLite's writer pool is -`MaxOpenConns(1)`, so the workers queue at the Go level rather than -racing for the file. - -Five things about the marks are load-bearing. **A mark records that the -upstream was asked, not that it answered with something.** ListenBrainz -returns 200 and `[]` for an artist it has no popularity data for — which -is most of a long-tail library, and the same is true of an artist whose -every row falls under `indexMinPopularity` — and keying -`discog_fetched` on "did rows come back" made those artists permanent -candidates: "Filling in artist details" re-ran for up to -`discogBackfillMaxPerRun` of them on **every launch**, forever, doing -the same two fetches to the same empty answer. So `indexOneArtist` sets -the mark when both fetches *succeeded* (`fetchTopReleaseGroups` and -`fetchTopRecordings` return an error for that reason), and only a real -failure — transport, non-2xx, unreadable body — leaves the artist for -the next run. `browseFullDiscography` already had this right: an artist -with genuinely no release groups is still marked browsed. The marks are **a table, not -more `explore_index` columns**, because `artifactimport.go` merges the -downloaded catalog by column list — a flag added there is a second -place to remember, and forgetting it silently wipes every mark on the -next catalog update. `discog_fetched` stays in `explore_index` for the -opposite reason: the artifact legitimately answers it for artists it -covers. **The artifact answering it is not "we have their -discography"** — its per-artist coverage is graded, so an artist can -arrive `discog_fetched = 1` and never have been browsed, which is why -the unenriched query ORs its conditions instead of testing the -first. **`BrowseReleaseGroupsAll` pages to exhaustion** where -`BrowseReleaseGroups` asks for `MaxLimit` once and takes what comes -back — a prolific artist was silently cut at 100 release groups, and a -hundred albums looks like a complete answer unless you count. And the -per-artist mark **replaced a heuristic that could never be satisfied**: -`BrowseReleaseGroups` used to re-browse whenever no indexed row carried -a secondary type, which is permanently true for an artist whose -releases are all plain albums. - -**One artist portrait is downloaded; the rest are remembered as URLs.** -`resolveAllSources` asked five upstreams what images they had for an -artist and then downloaded **every** candidate, up to ten, full size, -serially — while nothing in the app has ever read anything but -`primary.jpg` and its three tiers. Measured on a real cache: 5.3 GB, -of which 4.1 GB was candidates no code path can reach, ~940 kB per -artist against the ~217 kB that is actually used. - -It is split now. `resolveCandidates` does the metadata lookups and -returns an ordered list; `fetchPrimary` walks that list downloading -until one **succeeds**, makes that the primary, and records the rest -with an empty `file_path` — known, not fetched — so replacing a -portrait later is one download rather than five lookups again. Taking -the first that succeeds rather than the first outright is also a fix: -the old loop keyed `is_primary` on the index, so a failed candidate 0 -left the artist with a stored image, no `primary.jpg`, and a `.miss` -marker claiming there was no artwork at all. The winning candidate is -no longer also written under its own name, since `setPrimary` writes -the same bytes to `primary.jpg`. - -Two janitor jobs go with it, and the first is why the waste survived. -`OrphanedArtistImagesJob` joined the bare MBID onto the images -directory — but artist directories are **sharded** under a two-character -prefix, so it named a path that has never existed, `RemoveAll` -succeeded on it, and the job deleted the rows that were the only record -of the files it left behind. `explore.ArtistImageDir` is that layout's -one definition, passed in the way `OrphanedCoverFilesJob` takes -`expandVariants`, and the test lays its fixtures out with it — a test -that invents its own flat layout agrees with the bug. -`StrayArtistImageFilesJob` reclaims what earlier versions downloaded, -keeping only `explore.ArtistImageKeepNames()` and refusing an empty -keep set for the reason the covers sweep refuses an empty live set. - -**An age is not a ceiling, and a cache needs one.** Art for an artist -the user owns is kept indefinitely; everything else aged out after 90 -days and nothing counted it, so the same install held portraits for -**5,770 artists in a 1,301-artist library** — every artist page opened -in Explore fetches one, and a browsing afternoon is entirely inside the -retention window. `browsedArtBudget` (256 MB) is the second pass: -oldest browsed artist first, until what is left fits, with owned -artists outside the budget entirely. `httpCacheBudget` is the same rule -one cache over, and it is what makes the year-long entity TTL below -safe — once answers stop expiring, expiry stops being a bound. - -**"Owned" is a file here too.** The sweep's live set used to be "there -is an `artists` row", which the file-shaped schema made meaningless; -it joins `audio_files` now, like every other ownership question. Its -test had seeded an artists row with no file and called it owned — the -exact phantom, in the fixture of the test that guards it. - -**Only the tiers of a cover are stored.** `saveCoverArt` writes -`_sm`/`_md`/`_lg` and records the largest as `cover_art.file_path`; -`coverart.ResolveURLs` reports that one as `Original` too, because it -is the largest kept. The full-resolution image used to be written -beside them and was **1,134 MB of a 1.4 GB covers directory** against -110 MB for all three tiers — with nothing rendering it, since the grid -caps at 350 px and the largest tier is 400. The bytes are still in the -audio file, which is where they came from, so the repair pass that -regenerated tiers *from the stored original* went with it. - -**Frontend** (`frontend/`): Lit 3.2 web components + Web Awesome UI library + HTMX. State management via singleton reactive stores in `src/store/`. Wails bindings auto-generated as TypeScript in `frontend/bindings/`, nested by Go import path — don't edit by hand. The `@go` alias absorbs the constant prefix, so a call site imports `@go/library/library.js`. - -**One seam states what the generated types get wrong, rather than 78 patches.** v3's generator is honest where v2's lied: a Go `nil` slice marshals to JSON `null` and always has (v2 typed it `T[]`), and a Go named string type is a closed set (v2 typed it `string`). There is no flag to turn either off, correctly. So `utils/binding.ts` states the app's actual contract at the only place it is true — `list` yields `[]` for a nil slice, `dict`/`dictByName` yield `{}` for a nil map and drop null-valued keys (which loses nothing: `noUncheckedIndexedAccess` already makes every read `V | undefined`), and `compact` is the same for a map arriving as a *field*. Where a nullable slice is a model field there is no boundary to put it at, and those are `?? []` at the point of use. - -All three also return a **plain `Promise`**: v3 bindings return a `CancellablePromise` and nothing in this app cancels one, so letting it inward would put a Wails type in every store signature for a capability none of them use. - -**A view is a chunk, and three components are not.** `index.ts` holds a -loader table (`VIEW_LOADERS`, `DETAIL_LOADERS`) and `await`s a view's -module before creating its element — `document.createElement` on an -undefined tag yields an inert `HTMLElement` rather than throwing, so a -missing entry is a blank page, not an error. Navigations are numbered -and anything after the `await` re-checks it is still the newest, or a -slow chunk lands on top of a faster navigation. Every chunk is then -warmed on idle, so the split is paid once at startup rather than on -every first visit. **`notification-host`, `inline-notice` and -`confirm-dialog` stay eager on purpose**: a failure surface that has to -fetch a chunk before it can speak is not a failure surface, and the -moment it is most needed is the likeliest moment loading one fails. -`first-run-wizard` and the startup chrome are eager for the ordinary -reason — they are the first paint. - -**A navigation is a history entry, and that is the whole back stack.** -`index.ts` records each navigation with `pushState` (same URL — the app -has no routes, and a path a reload cannot resolve is worse than none) -and replays `popstate` with `_isBack`. It exists for Android, whose back -button is not a key the page can bind: the scaffold's -`MainActivity.onBackPressed` asks `webView.canGoBack()` and finishes the -activity otherwise, so an app that never touched `history` quit from any -depth — which is what a device reported. Hooking the platform's own -mechanism rather than adding a JNI callback is also what makes it -testable in a browser (`page.goBack()`), and the Java half needed no -change at all. - -Two rules hold it up. The **first** navigation *replaces* the launch -entry rather than pushing one, or every launch costs a back press before -the app will close. **There are two launch navigations**, which is what -defeated that rule for five phases: the eager `navigate → home` at the -foot of `index.ts` and the configured page `GetDefaultPage()` resolves -to later. Only the first replaced, so a fresh session was already one -entry deep, the first back press replayed home over home, and on Android -`canGoBack()` was true so the press that should have exited the app did -nothing (#142). The landing-page navigation carries `_replace`, honoured -only while still at index 0 — past that the user has navigated during -the backend call, and a slow answer must not overwrite an entry they -made. And the in-app back buttons (`navigate-back`, fired -by the detail views and `now-playing-view`) go through `history.back()` -rather than a stack of their own: the old `navStack` is **deleted**, not -kept beside it, because two stacks is precisely how a view's own back -button and the phone's gesture come to disagree about what one press -means. - -**And there is one statement of which view is active**, for the same -reason: `popstate` calls `handleNavigate()` directly and dispatches no -`navigate`, so the two nav components — which learned the active view -from that event — kept highlighting the view the user had just *left*. -`store/active-view-store.ts` is the shell saying where the user is, and -both navs read it through `ActiveViewController` rather than holding an -`activeView` of their own. - -Four things about it are load-bearing. - -**"Please go to X" and "the active view is now X" are different -statements**, and only the first existed — dispatched from 28 call -sites across 18 files. A re-dispatch from inside `handleNavigate` is -not the fix and cannot be: that function is the `document` listener for -`navigate`, so it is an infinite loop. - -**It is a store rather than an event, because a component that mounts -after a navigation still has to know.** `bottom-nav`'s "More" sheet -creates its `` on open, and that copy had heard no -`navigate` at all — standing on Albums, it opened highlighting -Home. An event has no answer for a listener that was not there. - -**A detail view is not a view here**, so the destination it was opened -from stays lit. `app-sidebar` did that by accident (it guarded on -`navItems.some(...)`, so an unmatched name left its highlight alone) -and `bottom-nav` had no such guard and so lit *nothing* — which is why -one looked right and the other looked broken on the same screen. -Whether a view is primary is the shell's fact: `view in VIEW_TAGS` is -passed to `setView`, never re-derived, because a second copy of that -list is a second thing to forget. - -**Nothing is lit until the shell has navigated.** The store starts -empty rather than defaulting to `home`, which is what `app-sidebar`'s -field used to do to match the landing view — a default that is correct -only while `GetDefaultPage()` agrees with it. - -**Back and forward are chrome, and the depth is the shell's own -count.** `` in the top bar is #6: the stack was always -global — every navigation is an entry and `popstate` restores any of -them in either direction — so what was missing was an affordance, since -the only way back was a detail view's own button, which leaves the -screen with the view it belongs to. The buttons dispatch -`navigate-back` / `navigate-forward` and the shell owns both guards, -for the reason the old `navStack` was deleted: a second caller reaching -for `history` is how two stacks come to disagree. - -Three things about it are load-bearing. **Forward is not back -negated**, so the single `pushedEntries` counter could not express it — -`popstate` carries no direction and fires identically both ways, so a -counter decremented on every pop reads a forward as a second back. Each -entry carries its index (`yjIdx`) and the shell keeps the current one -and a high-water mark; that also survives a jump of more than one, -which `history.go(-n)` and a long-press on a browser's back button both -produce. **A control that cannot act is `disabled` here**, which is the -documented exception to `library-status-indicator`'s rule: the two are -a pair whose positions the user learns, and hiding one moves the other -under the cursor. And **it stands down below 900px** — the top bar is -what runs out of room first below that (it already overflows 600px by -11px, #143), and nothing becomes unreachable: `nav.back` / `nav.forward` -(`Alt+Left` / `Alt+Right`, the browser's own combination, and clear of -the bare arrows that seek) are global at every width, and the phone has -the platform's gesture. - -The assertion is `aria-current="page"`, in -`e2e/specs/back-navigation.spec.ts`. That file existed throughout the -bug, covered exactly these journeys, and asserted only -`data-active-view` — the shell's own bookkeeping, which was right the -whole way through — so it was green on the broken build. Same trap as -`layout-overflow.spec.ts` and `page-header`: a spec named for the -behaviour, measuring the plumbing. - -**Which destinations exist is configuration, and hiding one takes away -the nav item and nothing else.** Eleven sidebar entries is more than -most libraries need (#25), so each is toggleable from Settings → -Navigation, Autotag is off until asked for, and Downloads is absent -until there is a client to download with — a destination for a feature -that cannot work is worse than none. `navigate` still resolves a hidden -view, which is not a nicety: detail views navigate into these and the -launch page is one of them. Nothing needed a special case for the -highlight either, because the paragraph above moved that onto -`active-view-store`: the sidebar asks `isActive(id)` per *rendered* -item, so a hidden view lights nothing exactly as a detail view does. - -Five things about it are load-bearing. - -**The stored shape is a map keyed by view id, and an absent key means -that view's own default** (`backend/config.Views`). That is what makes -this need no migration in either direction, and it is the polarity rule -`AllowMeteredCatalogDownload` states: the zero value is the intended -answer. A `HiddenViews []string` cannot express "Autotag off by -default" at all — its zero value is *hide nothing* — and a struct with -a boolean per view turns a view that later stops existing into stored -garbage. Here an unknown key is dropped on load and a view added later -gets its own default rather than being invisible or forcibly visible. -It is also what makes #73's `#25 → #27` order safe rather than -backwards: when Jobs folds into Settings, `jobs = true` in somebody's -config is a key nothing asks about. - -**Two states the user could not get out of are refused, in the config -and not in the checkbox.** Settings is never hideable and the launch -page is not hideable while it is the launch page. `config.toml` is -hand-editable, so a disabled checkbox is the affordance and -`SetViewVisible` is the rule — an app that can be locked out of its own -Settings by a typo in TOML is a support problem nobody can debug -remotely. On *load* the launch page is instead un-hidden rather than -refused: there is nobody to tell, and the honest reading of "my launch -page is Autotag" is that this user wants Autotag, not that their launch -page should be silently reset to something they did not choose. - -**Downloads is gated at the nav and not in the config**, on -`downloadStore.available`, so switching it on in Settings still means -what it says once a client exists and the tab appears without a restart -(#37's rule). `available` is false until the providers have loaded, -which makes the item *appear* on a fresh launch rather than appearing -and then vanishing. - -**The tab bar honours the toggles too, and the reason is local rather -than a general rule about phones.** `PHONE_COLUMN_IDS` is the precedent -for "what a phone shows is a different question", and it would apply — -except that `bottom-nav`'s "More" opens the *same* ``, -which filters, so an unfiltered bar would contradict its own sheet one -tap away. Which four tabs is still plan 016's committed subset; this -only removes from it, and "More" is never filtered because it is how -everything else stays reachable. - -**A retired destination is the one shape this does not make free.** An -absent visibility key takes its default and an unknown one is dropped, -but `DefaultPage` is a *value*: a launch page naming a view that no -longer exists fails validation, and on the load path that means the app -refuses to start for whoever had it selected. `RetiredViews` is that -list, and `ApplyDefaults` treats a retired name as a zero value while -an unknown-but-not-retired one still errors — a typo is worth being -told about. #27 retiring `jobs` is its first entry. - -**The list of destinations is `services/view-meta.ts`**, on -`shortcut-meta.ts`'s pattern, because #25 gave it a second reader: -Settings renders a toggle per view and needs the same labels in the -same order. Which views exist and what an unconfigured install shows is -Go's (`backend/config.Views`, which `DefaultPage`'s validation reads -too, so the launchable set is not a second list); how they are *drawn* -is the frontend's, beside the rest of the icon vocabulary. The binding -returns the **resolved** map for every view, so the frontend holds no -copy of the defaults — which would be the copy that shipped in the -binary rather than the one being edited. - -**A primary view is cached, not unmounted.** `index.ts` keeps every -primary view in the DOM and toggles a `.view-hidden` class, because that -is what preserves `scrollTop` across navigation — so -`disconnectedCallback` never fires for one, and anything registered -there runs for the life of the session from pages it is not on. The -missing half is `utils/view-lifecycle.ts`: navigation calls -`viewDeactivated()` on the outgoing view and `viewActivated()` on the -incoming one, and a view registers its document listeners, timers and -backend subscriptions through `listenWhileActive` / -`intervalWhileActive` / `whileActive`, which are torn down on the way -out. An off-screen view also does not render. A shared reactive -controller gets the same treatment via `registerViewAware`. - -**The player's position comes from the player.** `seek-bar` renders -`PlaybackPositionChanged` (payload `player.PositionInfo`), emitted at -1 Hz while playing and immediately on load, play, pause, seek and -natural finish. Its local `setInterval` is interpolation *between* -reports only, stopped and restarted by every one of them — it used to -be the clock, and counted itself 30 s away from the backend across four -keyboard seeks. A report carries `trackChangeId` (the store is a -singleton, so a bar mounting later must not adopt a report about the -previous track) and a `seq` (the same second reported twice still has -to reset the interpolation). - -What the player cannot do, it says: `PlaybackFailed` is emitted from -both the load and the play path, auto-advance **skips** the failed -track (bounded by the queue length, so a disconnected drive stops after -one pass), and the bottom bar shows one coalescing line — "Skipped 12 -tracks that could not be played." That line is the Inline level of the -app's one notification surface, below. - -**Failure has one voice, and the caller picks how loud.** -`store/notification-store.ts` is the only notification surface; before -it, 84 `catch` blocks ended at `console.error` and two components had -grown private toasts. Four levels, chosen by the call site from one -rule — *a failure is only worth interrupting for if the user can do -something about it that they are not already doing*: - -- **Blocking** (`wa-dialog`, must be acknowledged) for data at risk: a - folder left holding a mix of old and new tags. Two callers are - anticipated; a third should be argued for. -- **Persistent** (stays, with an action) for something the user asked - for that did not happen and retrying is meaningful. -- **Transient** (a toast) for a small action whose state visibly - reverted anyway — a favourite that came back. -- **Inline**, rendered by `` in the panel - that failed, never as a toast. - -Three things about it are load-bearing. **Coalescing lives in the -store**, keyed by `(level, region, key)` within a window, so 200 -unplayable files are one message with a count and no future caller has -to remember that. **An inline notification carries a region**, because -"inline" says *not global*, not *where*. And **the bottom band belongs -to the player** — the app-level stack sits under the header, since the -player's own floating notice grows upward by however many lines it -needs and a bottom-anchored stack collides with it on a small window. - -What reaches a person is a sentence: `utils/describe-error.ts` maps the -causes a user can act on (offline, timeout, not found, permission, -database busy) to copy, `explainError` repeats a backend message when it -is one of *our* sentinels rather than a Go wrapping chain, and the raw -text stays in `console.error`. The one documented exception is a -download client's connection test, whose verbatim error is the user's -debugging tool. - -Destructive actions ask once, through `confirmAction()` -(`components/confirm-dialog/`), which is a `wa-dialog` and so brings the -focus trap and Escape the hand-rolled overlays do not have. - -**Every dialog in the app is a `wa-dialog`, and there is no sixth -pattern.** The four hand-rolled autotag overlays and the remove-library -confirmation had no `role`, no `aria-modal`, no focus trap and no focus -restore — including the two gating an irreversible on-disk metadata -rewrite. The split is by *shape*, not by owner: a dialog that only asks -a question is a `confirmAction()` call (title, message, impact, -confirm/cancel), and a dialog carrying **input** is a `` in -the host's own template. Both remaining autotag dialogs render -unconditionally with `?open` deciding which is up — mounting one on -demand puts the element and its `showModal()` in the same update. -`autotag-view`'s last document keydown listener died with them; it -existed only because its dialogs could not close themselves. - -**None of them had an accessible name, and one helper gives all of them -one.** Every call site passes `label`; Web Awesome renders it into an -`

` in the same shadow root as the native `` and -never points `aria-labelledby` at it — so for eleven dialogs -`getByRole('dialog', {name})` matched nothing and a screen reader -announced an unnamed dialog. `utils/name-dialog.ts` sets that IDREF -(and falls back to `aria-label` under `without-header`, which renders -no heading to point at), called from each host's `updated()`. - -Three things about it are load-bearing. It **reaches into another -library's shadow root**, which is open but is not API — acceptable -here only because the failure is bounded: if Web Awesome moves the -structure the query misses, nothing is written, and the dialog is as -unnamed as it was. It uses **`aria-labelledby`, not `aria-label`**, -because three call sites compute their label at render time and an -IDREF to the heading Web Awesome re-renders stays correct with nothing -resyncing it. And it **waits for the dialog's own first update**, not -its host's: `wa-dialog` is a Lit element whose shadow root is populated -in *its* update, so a query at the host's `firstUpdated` finds an empty -root and names nothing — the same lifecycle trap that hid -`wa-dropdown-item`'s role from the menu keyboard model. - -Two awkwardnesses remain, and they are about *locating* one rather than -naming it. The host is `display: contents`, so the element carrying the -testid always reports hidden — what is visible is the `` inside -it — and what holds the slotted content is the *host's* shadow root, -not the dialog's subtree. A third is worth knowing before checking any -of this: the Playwright **a11y snapshot never prints a dialog's name**, -named or not, so it cannot tell you whether this works. `getByRole` -can, and CDP's `Accessibility.getFullAXTree` gives the browser's own -answer. - -**A disclosure is a button, and it says what it controls.** -`config-section`'s header was a bare `
` with no `tabindex`, -no `role` and no `aria-expanded`, and every section defaults to -collapsed — so every setting in the app sat behind a control that could -not be tabbed to (the audit's last Critical). It is a -`
@@ -2572,25 +2560,53 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost if (this.ownedLocalAlbumIds().length === 0) return nothing; return html` -
+
void this.playLibraryTracks(false)} > - Play library tracks + Play - void this.playLibraryTracks(true)} + - - Shuffle - + + +
`; } @@ -2786,25 +2802,27 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost const request = downloadStore.requestFor(this.artistMBID); return html` -
- void this.toggleFollow(request?.id)} - > - - - ${request ? 'Following' : 'Follow for new releases'} - -
+ void this.toggleFollow(request?.id)} + > + + + ${request ? 'Following' : 'Follow'} + `; } @@ -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``; })()} + +
+ ${owned + ? html`` + : html``} +
${trackLink(t.trackName, t.releaseName, t.releaseGroupMbid ?? '', t.recordingMbid)}
@@ -2979,15 +3012,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost ${formatListenCount(t.totalListenCount)} plays - ${owned - ? nothing - : html``}
`; })} @@ -3093,6 +3117,18 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
+
+ +
@@ -3102,18 +3138,6 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
${rg.date ? html`${extractYear(rg.date)}` : nothing}
- ${badge.status === 'in-library' - ? nothing - : html``}
@@ -3164,37 +3188,16 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost

Discography

${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.type === 'Other' ? 'Other Releases' : g.type.endsWith('s') ? g.type : `${g.type}s`} -

-
- ${visibleItems.map((rg) => this.renderAlbumCard(rg))} -
- ${showToggle - ? html` - - ` - : nothing} -
- `; - }, + (g) => html` +
+

+ ${g.type === 'Other' ? 'Other Releases' : g.type.endsWith('s') ? g.type : `${g.type}s`} +

+ + ${g.items.map((rg) => this.renderAlbumCard(rg))} + +
+ `, )}
`; @@ -3234,23 +3237,25 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost
+
+ +
${rg.title}
+
${rg.artistCredit ?? ''}
${year ? html`${year}` : nothing}
- ${badge.status === 'in-library' - ? nothing - : html``}
`; @@ -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`

Similar Artists

-
- ${visible.map((a) => { + + ${artists.map((a) => { const imgURL = this.similarImageURLs.get(a.artistMbid); return html`
`; })} -
- ${showToggle - ? html` - - ` - : nothing} +
`; } diff --git a/frontend/src/components/explore-view/explore-view.ts b/frontend/src/components/explore-view/explore-view.ts index 4c73434..5c26994 100644 --- a/frontend/src/components/explore-view/explore-view.ts +++ b/frontend/src/components/explore-view/explore-view.ts @@ -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`

${subtitle}

` : nothing} -
+ ${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
`; })} - + `; } @@ -2187,7 +2066,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte ${subtitle ? html`

${subtitle}

` : nothing} -
+ ${releaseGroups.map((rg) => { const artURL = this.thumbnailCache.get(rg.mbid) || ''; const year = extractYear(rg.firstReleaseDate); @@ -2249,6 +2128,18 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte >
+
+ +
${rg.title} @@ -2256,29 +2147,18 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte
${creditLink(creditStore.credits(rg.mbid), rg.artistCredit, rg.artistMbid ?? '')}
+ ${year ? html`${year}` : nothing} ${rg.primaryType ? html`${rg.primaryType}` : nothing} - ${year ? html`${year}` : nothing}
- ${badge.status === 'in-library' - ? nothing - : html``}
`; })} - + `; } diff --git a/frontend/src/components/home-view/home-view.ts b/frontend/src/components/home-view/home-view.ts index aa31d96..a9289c5 100644 --- a/frontend/src/components/home-view/home-view.ts +++ b/frontend/src/components/home-view/home-view.ts @@ -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) { ${shelf.title}

${shelf.subtitle}

-
+ ${(shelf.albums ?? []).map((album) => this.renderCard(album))} -
+ `; } diff --git a/frontend/src/components/scroll-row/scroll-row.ts b/frontend/src/components/scroll-row/scroll-row.ts new file mode 100644 index 0000000..bcd6a64 --- /dev/null +++ b/frontend/src/components/scroll-row/scroll-row.ts @@ -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` + +
+
+
+ + `; + } +} + +declare global { + interface HTMLElementTagNameMap { + 'scroll-row': ScrollRow; + } +} diff --git a/frontend/src/components/top-results-row/top-results-row.ts b/frontend/src/components/top-results-row/top-results-row.ts index 8f4cdd8..34fc376 100644 --- a/frontend/src/components/top-results-row/top-results-row.ts +++ b/frontend/src/components/top-results-row/top-results-row.ts @@ -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`
Top Results
-
+ ${this.results.map((r) => this.renderCard(r))} -
+ `; } diff --git a/frontend/src/icons/names.txt b/frontend/src/icons/names.txt index 6154848..d94ae1a 100644 --- a/frontend/src/icons/names.txt +++ b/frontend/src/icons/names.txt @@ -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 diff --git a/frontend/src/styles/album-card.css.ts b/frontend/src/styles/album-card.css.ts new file mode 100644 index 0000000..a619dbd --- /dev/null +++ b/frontend/src/styles/album-card.css.ts @@ -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; + } +`; diff --git a/frontend/src/utils/external-link.ts b/frontend/src/utils/external-link.ts new file mode 100644 index 0000000..0bb6b42 --- /dev/null +++ b/frontend/src/utils/external-link.ts @@ -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(); +} diff --git a/frontend/src/utils/ownership.ts b/frontend/src/utils/ownership.ts index 04b4ff9..791c695 100644 --- a/frontend/src/utils/ownership.ts +++ b/frontend/src/utils/ownership.ts @@ -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. * diff --git a/frontend/test/components/album-card-size.test.ts b/frontend/test/components/album-card-size.test.ts new file mode 100644 index 0000000..0ff9355 --- /dev/null +++ b/frontend/test/components/album-card-size.test.ts @@ -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 `` 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 { + 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('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 + // ``: 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, + ); + }); +}); diff --git a/frontend/test/components/artist-header.test.ts b/frontend/test/components/artist-header.test.ts new file mode 100644 index 0000000..c6a642b --- /dev/null +++ b/frontend/test/components/artist-header.test.ts @@ -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 { + const el = await fixture('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(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(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(el, '.artist-title')!; + const listens = shadow(el, '.artist-listens')!; + const meta = shadow(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(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(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(); + }); +}); diff --git a/frontend/test/components/artist-release-menu.test.ts b/frontend/test/components/artist-release-menu.test.ts index cb5ed00..1d62e7e 100644 --- a/frontend/test/components/artist-release-menu.test.ts +++ b/frontend/test/components/artist-release-menu.test.ts @@ -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 diff --git a/frontend/test/components/scroll-row.test.ts b/frontend/test/components/scroll-row.test.ts new file mode 100644 index 0000000..05ac8bb --- /dev/null +++ b/frontend/test/components/scroll-row.test.ts @@ -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 { + const el = await fixture('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('', () => { + 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('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); + }); +}); diff --git a/frontend/test/components/unowned-everywhere.test.ts b/frontend/test/components/unowned-everywhere.test.ts index 5bee2de..b560d1e 100644 --- a/frontend/test/components/unowned-everywhere.test.ts +++ b/frontend/test/components/unowned-everywhere.test.ts @@ -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(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', { -- 2.54.0 From 792c2d9fbc5e4ea0d10cefd2133a5b6b9285bc0d Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Sat, 26 Sep 2026 17:00:47 -0400 Subject: [PATCH 12/16] build: require Go 1.26 everywhere at once The newest go-json-experiment/json, which wails/v3's application package imports, declares go 1.26, so taking it raises our go directive with it. Every other place that names a Go version moves in the same commit: GO_VERSION in ci, desktop-assets and android-apk, the Arch makedepends, and CONTRIBUTING's table. index-artifact's container image moves too, and it is the one that matters most. The official golang images set GOTOOLCHAIN=local, so a golang:1.25 container refuses a go 1.26 module outright rather than fetching a newer toolchain, and that job owns the ~205 GB checkpoint. Closes #265 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT --- .gitea/workflows/android-apk.yml | 2 +- .gitea/workflows/ci.yml | 4 ++-- .gitea/workflows/desktop-assets.yml | 2 +- .gitea/workflows/index-artifact.yml | 2 +- CONTRIBUTING.md | 2 +- go.mod | 4 ++-- go.sum | 4 ++-- packaging/arch/PKGBUILD | 2 +- 8 files changed, 11 insertions(+), 11 deletions(-) diff --git a/.gitea/workflows/android-apk.yml b/.gitea/workflows/android-apk.yml index 94e6c06..491706a 100644 --- a/.gitea/workflows/android-apk.yml +++ b/.gitea/workflows/android-apk.yml @@ -68,7 +68,7 @@ jobs: SHA: ${{ github.sha }} REF_NAME: ${{ github.ref_name }} DEBIAN_FRONTEND: noninteractive - GO_VERSION: '1.25.0' + GO_VERSION: '1.26.0' npm_config_store_dir: /cache/pnpm-store # The Go half wants the NDK; the Gradle half wants a platform. ANDROID_HOME: /cache/android-sdk diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 78a2763..9212bc9 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -36,7 +36,7 @@ concurrency: cancel-in-progress: true env: - GO_VERSION: '1.25.0' + GO_VERSION: '1.26.0' # Shared by all three Playwright consumers (@playwright/cli, e2e/'s # @playwright/test, frontend/'s Vitest provider). See the browsers # step in job 2 for why that is not the whole story. @@ -53,7 +53,7 @@ jobs: check: runs-on: ubuntu-latest container: - # Not golang:1.25 — this job runs `make ui-test`, which is Vitest + # Not golang:1.26 — this job runs `make ui-test`, which is Vitest # *browser* mode and needs a Chromium and its system libraries # anyway, so the "fast job needs no browser" split does not hold. # Not the Playwright image either: e2e/ pins @playwright/test diff --git a/.gitea/workflows/desktop-assets.yml b/.gitea/workflows/desktop-assets.yml index a993830..994a166 100644 --- a/.gitea/workflows/desktop-assets.yml +++ b/.gitea/workflows/desktop-assets.yml @@ -48,7 +48,7 @@ jobs: SHA: ${{ github.sha }} REF_NAME: ${{ github.ref_name }} DEBIAN_FRONTEND: noninteractive - GO_VERSION: '1.25.0' + GO_VERSION: '1.26.0' npm_config_store_dir: /cache/pnpm-store steps: # The same set ci.yml's check job installs: the app is cgo, and diff --git a/.gitea/workflows/index-artifact.yml b/.gitea/workflows/index-artifact.yml index 4ae894c..4a28903 100644 --- a/.gitea/workflows/index-artifact.yml +++ b/.gitea/workflows/index-artifact.yml @@ -68,7 +68,7 @@ jobs: # claim with a test behind it now (cmd/indexbuild/deps_test.go), # because the v3 migration quietly broke it and this job was where # that surfaced. - image: golang:1.25 + image: golang:1.26 # This host path must exist on the runner and be listed verbatim in # act_runner's container.valid_volumes. It holds explore-staging/ # (counts.bin + state.json) and yj.db — the checkpoint that makes diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5311019..e48d075 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -13,7 +13,7 @@ frontend, bridged by [Wails v3](https://wails.io/). | Tool | Version | |------|---------| -| Go | 1.25+ | +| Go | 1.26+ | | Node.js | 22+ | | pnpm | 10+ | | Wails CLI | v3 — vendored, no install needed (`go tool wails3`) | diff --git a/go.mod b/go.mod index ebc414e..440fe82 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module yellowjacket -go 1.25.0 +go 1.26 require ( github.com/BurntSushi/toml v1.6.0 @@ -144,7 +144,7 @@ require ( github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 // indirect github.com/go-git/go-billy/v5 v5.9.0 // indirect github.com/go-git/go-git/v5 v5.19.2 // indirect - github.com/go-json-experiment/json v0.0.0-20251027170946-4849db3c2f7e // indirect + github.com/go-json-experiment/json v0.0.0-20260820222146-c27c302e5fc3 // indirect github.com/go-ole/go-ole v1.3.0 // indirect github.com/go-resty/resty/v2 v2.17.1 // indirect github.com/go-sql-driver/mysql v1.9.3 // indirect diff --git a/go.sum b/go.sum index 1858f35..1ca5a86 100644 --- a/go.sum +++ b/go.sum @@ -362,8 +362,8 @@ github.com/go-git/go-git/v5 v5.19.2/go.mod h1:QqCBE1EFN5ddFmrliLQ3/ntRCUjZU3EJuw github.com/go-gl/glfw v0.0.0-20190409004039-e6da0acd62b1/go.mod h1:vR7hzQXu2zJy9AVAgeJqvqgH9Q5CA+iKCZ2gyEVpxRU= github.com/go-gl/glfw/v3.3/glfw v0.0.0-20191125211704-12ad95a8df72/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8= github.com/go-gl/glfw/v3.3/glfw v0.0.0-20200222043503-6f7a984d4dc4/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8= -github.com/go-json-experiment/json v0.0.0-20251027170946-4849db3c2f7e h1:Lf/gRkoycfOBPa42vU2bbgPurFong6zXeFtPoxholzU= -github.com/go-json-experiment/json v0.0.0-20251027170946-4849db3c2f7e/go.mod h1:uNVvRXArCGbZ508SxYYTC5v1JWoz2voff5pm25jU1Ok= +github.com/go-json-experiment/json v0.0.0-20260820222146-c27c302e5fc3 h1:UADEEmDKgfXbtnGJZ97beY5XLo9ZechG1nlU4KnRrkE= +github.com/go-json-experiment/json v0.0.0-20260820222146-c27c302e5fc3/go.mod h1:tphK2c80bpPhMOI4v6bIc2xWywPfbqi1Z06+RcrMkDg= github.com/go-kit/kit v0.8.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as= github.com/go-kit/kit v0.9.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as= github.com/go-kit/log v0.1.0/go.mod h1:zbhenjAZHb184qTLMA9ZjW7ThYL0H2mk7Q6pNt4vbaY= diff --git a/packaging/arch/PKGBUILD b/packaging/arch/PKGBUILD index d6d2949..6a8a2d6 100644 --- a/packaging/arch/PKGBUILD +++ b/packaging/arch/PKGBUILD @@ -18,7 +18,7 @@ arch=('x86_64') url="https://git.ljones.me/yonlu/yellowjacket" license=('custom') depends=('webkitgtk-6.0' 'gtk4' 'alsa-lib' 'hicolor-icon-theme') -makedepends=('go>=1.25' 'nodejs>=22' 'pnpm' 'git') +makedepends=('go>=1.26' 'nodejs>=22' 'pnpm' 'git') options=('!lto') # Source is overridable so the same PKGBUILD works two ways: -- 2.54.0 From 7fbfd9c10577830aea620c324a4e48aeba25e481 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Sat, 26 Sep 2026 17:02:56 -0400 Subject: [PATCH 13/16] test(library): skip the cover-tier scan test when fixtures are absent TestScan_StoresOnlyCoverTiers built the fixture path by hand, so in a tree where make testdata had not run it failed on a missing covers directory, where every other fixture test skips via testfixtures.Load. In a fresh worktree that failure blocked the pre-push hook for every branch. Closes #266 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT --- backend/library/coverart_storage_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/backend/library/coverart_storage_test.go b/backend/library/coverart_storage_test.go index 2c2b60d..1f0e6b7 100644 --- a/backend/library/coverart_storage_test.go +++ b/backend/library/coverart_storage_test.go @@ -8,6 +8,7 @@ import ( "yellowjacket/backend/coverart" "yellowjacket/backend/database/sql/sqlcgen" + "yellowjacket/internal/testfixtures" ) // TestScan_StoresOnlyCoverTiers pins the size decision: a scan writes @@ -26,10 +27,9 @@ func TestScan_StoresOnlyCoverTiers(t *testing.T) { lib, db := setupTestLibrary(t) - root, err := filepath.Abs("../../test_data/music_library_test") - if err != nil { - t.Fatalf("resolve fixture path: %v", err) - } + // Load skips when the fixture library has not been generated, as + // every other fixture test does. + root := testfixtures.Load(t).Root() library, err := db.Queries.CreateLibrary(lib.ctx, sqlcgen.CreateLibraryParams{ Name: "Fixtures", -- 2.54.0 From f81a95091659e89e49503fb99cf251ffb14fb7a8 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Sat, 26 Sep 2026 17:07:28 -0400 Subject: [PATCH 14/16] fix(download): score a multi-disc rip as one album, and count tracks Three faults in how candidates are shaped and scored, one commit because they meet in the same completeness number. Multi-disc albums were split in two. Soulseek shares them as Album/CD1 and Album/CD2, and candidates were grouped by the immediate parent, so each disc became its own candidate titled "CD1": about half complete, with an album title that could not match. Such a release essentially never cleared auto-pick. AlbumDir groups a disc folder under its parent, ParsePath takes the disc number from the folder (a disc in the filename still wins), and collect keeps the disc folders in staging, where flattened, disc 2's "01 Intro.flac" overwrote disc 1's. A single-track request could never be served from Soulseek. A track search matches one file per folder, and the two-file floor that screens out noise for an album screened out every result. A recording request takes one. Completeness counted files. Ten files against a ten-track album scored full marks whether or not they were its tracks, and title fit is the mean over the files that did align, so a folder where three titles matched read as near-perfect on both. Coverage is now counted in aligned tracks, with the file count still setting the penalty for extras. Closes #270 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT --- backend/download/candidate_shape_test.go | 248 +++++++++++++++++++++++ backend/download/pathmatch.go | 59 +++++- backend/download/provider_slskd.go | 45 +++- backend/download/rank.go | 46 ++++- backend/download/rank_test.go | 24 ++- 5 files changed, 391 insertions(+), 31 deletions(-) create mode 100644 backend/download/candidate_shape_test.go diff --git a/backend/download/candidate_shape_test.go b/backend/download/candidate_shape_test.go new file mode 100644 index 0000000..6b979a0 --- /dev/null +++ b/backend/download/candidate_shape_test.go @@ -0,0 +1,248 @@ +package download + +import ( + "context" + "os" + "path/filepath" + "testing" +) + +// Multi-disc rips, single-track results and coverage counted in tracks +// rather than files (#270). + +func TestParsePathReadsTheDiscFromItsFolder(t *testing.T) { + t.Parallel() + + cases := []struct { + path string + disc int + track int + folder string + }{ + {`\share\Pink Floyd - The Wall (1979)\CD2\03 Hey You.flac`, 2, 3, "The Wall"}, + {`\share\The Wall\Disc 1\01 In The Flesh.flac`, 1, 1, "The Wall"}, + {`\share\The Wall\[Disk-2]\01 Hey You.flac`, 2, 1, "The Wall"}, + {`\share\The Wall\CD1 - Live\04 Mother.flac`, 1, 4, "The Wall"}, + // The filename's own disc number is more specific than the folder. + {`\share\The Wall\CD1\2-05 Comfortably Numb.flac`, 2, 5, "The Wall"}, + // Not a disc folder: a number is required. + {`\share\CDs\The Wall\01 In The Flesh.flac`, 0, 1, "The Wall"}, + } + + for _, tc := range cases { + t.Run(tc.path, func(t *testing.T) { + t.Parallel() + + got := ParsePath(tc.path) + if got.Disc != tc.disc || got.Track != tc.track || got.Folder != tc.folder { + t.Errorf( + "ParsePath = disc %d track %d folder %q, want %d %d %q", + got.Disc, got.Track, got.Folder, tc.disc, tc.track, tc.folder, + ) + } + }) + } +} + +func TestAlbumDir(t *testing.T) { + t.Parallel() + + cases := map[string]string{ + `\share\Album\CD1\01 A.flac`: "/share/Album", + `\share\Album\01 A.flac`: "/share/Album", + `CD1\01 A.flac`: "CD1", + `\share\CD Collection\01.mp3`: "/share/CD Collection", + } + + for in, want := range cases { + if got := AlbumDir(in); got != want { + t.Errorf("AlbumDir(%q) = %q, want %q", in, got, want) + } + } +} + +// One album shared as CD1/CD2 is one candidate, named after the album. +func TestSlskdGroupsDiscFoldersIntoOneCandidate(t *testing.T) { + t.Parallel() + + stub := newSlskdStub(t) + stub.responses = []slskdResponse{{ + Username: "peer", + Files: []slskdFile{ + {Filename: `\m\The Wall\CD1\01 In The Flesh.flac`, Size: 1}, + {Filename: `\m\The Wall\CD1\02 The Thin Ice.flac`, Size: 1}, + {Filename: `\m\The Wall\CD2\01 Hey You.flac`, Size: 1}, + {Filename: `\m\The Wall\CD2\02 Is There Anybody Out There.flac`, Size: 1}, + }, + }} + + s, _ := newStubSlskd(t, stub) + + got, err := s.Search(context.Background(), Download{Query: "the wall"}) + if err != nil { + t.Fatalf("Search: %v", err) + } + + if len(got) != 1 { + t.Fatalf("got %d candidates, want the two discs as one", len(got)) + } + + if got[0].Title != "The Wall" || len(got[0].Files) != 4 { + t.Errorf( + "candidate = %q with %d files, want \"The Wall\" with 4", + got[0].Title, len(got[0].Files), + ) + } +} + +// A track search matches one file per folder, so a single-track request +// must accept a one-file folder that an album request rightly drops. +func TestSlskdKeepsASingleFileForATrackRequest(t *testing.T) { + t.Parallel() + + stub := newSlskdStub(t) + stub.responses = []slskdResponse{{ + Username: "peer", + Files: []slskdFile{ + {Filename: `\m\OK Computer\02 Paranoid Android.flac`, Size: 1}, + }, + }} + + s, _ := newStubSlskd(t, stub) + + track, err := s.Search(context.Background(), Download{ + RecordingMBID: "rec-1", Artist: "Radiohead", Album: "Paranoid Android", + }) + if err != nil { + t.Fatalf("Search: %v", err) + } + + if len(track) != 1 { + t.Errorf("track request: got %d candidates, want 1", len(track)) + } + + album, err := s.Search(context.Background(), Download{ + ReleaseMBID: "rel-1", Artist: "Radiohead", Album: "OK Computer", + }) + if err != nil { + t.Fatalf("Search: %v", err) + } + + if len(album) != 0 { + t.Errorf("album request: got %d candidates, want the one-file folder dropped", len(album)) + } +} + +// Two discs with a file of the same name both reach staging, each under +// its disc folder, where the importer reads the disc number from. +func TestSlskdCollectKeepsDiscFolders(t *testing.T) { + t.Parallel() + + stub := newSlskdStub(t) + s, downloads := newStubSlskd(t, stub) + + for _, disc := range []string{"CD1", "CD2"} { + dir := filepath.Join(downloads, disc) + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + + if err := os.WriteFile( + filepath.Join(dir, "01 Intro.flac"), []byte(disc), 0o600, + ); err != nil { + t.Fatalf("write: %v", err) + } + } + + dst := t.TempDir() + + got, err := s.collect(Candidate{Files: []CandidateFile{ + {Path: `\m\Album\CD1\01 Intro.flac`, IsAudio: true}, + {Path: `\m\Album\CD2\01 Intro.flac`, IsAudio: true}, + }}, dst) + if err != nil { + t.Fatalf("collect: %v", err) + } + + if len(got.Files) != 2 { + t.Fatalf("collected %d files, want 2", len(got.Files)) + } + + for _, disc := range []string{"CD1", "CD2"} { + data, err := os.ReadFile(filepath.Join(dst, disc, "01 Intro.flac")) + if err != nil || string(data) != disc { + t.Errorf("%s's file missing or overwritten: %q, %v", disc, data, err) + } + + if hint := ParsePath(filepath.Join(dst, disc, "01 Intro.flac")); hint.Disc == 0 { + t.Errorf("staged %s file lost its disc number", disc) + } + } +} + +// A two-disc release whose discs both number from 01 aligns completely +// once the disc comes from the folder; before, disc 2's 01 collided with +// disc 1's. +func TestMultiDiscCandidateAlignsEveryTrack(t *testing.T) { + t.Parallel() + + dl := Download{ + ReleaseMBID: "the-wall", + Artist: "Pink Floyd", + Album: "The Wall", + Expected: []ExpectedTrack{ + {DiscNumber: 1, Position: 1, Title: "In the Flesh?"}, + {DiscNumber: 1, Position: 2, Title: "The Thin Ice"}, + {DiscNumber: 2, Position: 1, Title: "Hey You"}, + {DiscNumber: 2, Position: 2, Title: "Is There Anybody Out There?"}, + }, + } + + c := Candidate{ + Title: "The Wall", + Files: []CandidateFile{ + {Path: `\m\Pink Floyd - The Wall\CD1\01 In the Flesh.flac`, Size: 1}, + {Path: `\m\Pink Floyd - The Wall\CD1\02 The Thin Ice.flac`, Size: 1}, + {Path: `\m\Pink Floyd - The Wall\CD2\01 Hey You.flac`, Size: 1}, + {Path: `\m\Pink Floyd - The Wall\CD2\02 Is There Anybody Out There.flac`, Size: 1}, + }, + } + + got := Score(dl, c, 50, AutoDownloadPrefs{}) + + if got.Match.Completeness != 1 { + t.Errorf("completeness = %f, want 1", got.Match.Completeness) + } + + if got.Match.AlbumFit < 0.99 { + t.Errorf("album fit = %f, want the album's own name to match", got.Match.AlbumFit) + } + + if got.Match.Overall < minMatch { + t.Errorf("match = %f, want it to clear the auto-pick bar %f", got.Match.Overall, minMatch) + } +} + +// Ten files against a ten-track album is not a complete album when only +// three of them are its tracks. +func TestCompletenessCountsTracksNotFiles(t *testing.T) { + t.Parallel() + + dl := okComputer() + + c := Candidate{Title: "OK Computer", Files: []CandidateFile{ + {Path: `\m\Radiohead - OK Computer\Airbag.flac`, Size: 1}, + {Path: `\m\Radiohead - OK Computer\Paranoid Android.flac`, Size: 1}, + {Path: `\m\Radiohead - OK Computer\Exit Music (For a Film).flac`, Size: 1}, + {Path: `\m\Radiohead - OK Computer\Creep.flac`, Size: 1}, + }} + + got := Score(dl, c, 50, AutoDownloadPrefs{}) + + if got.Match.Completeness > 0.76 { + t.Errorf( + "completeness = %f with 3 of 4 tracks present, want at most 0.75", + got.Match.Completeness, + ) + } +} diff --git a/backend/download/pathmatch.go b/backend/download/pathmatch.go index 78a2249..a9149e6 100644 --- a/backend/download/pathmatch.go +++ b/backend/download/pathmatch.go @@ -66,6 +66,14 @@ var ( // separatorPattern splits "Artist - Album" style folder names. separatorPattern = regexp.MustCompile(`\s+[-–—]\s+`) + + // discFolderPattern matches a directory that holds one disc of an + // album rather than the album: "CD1", "CD 2", "Disc 3", "Disk-1", + // "[Disc 2]", "CD1 - The Early Years". A number is required, so a + // folder merely called "CDs" is not one. + discFolderPattern = regexp.MustCompile( + `(?i)^\s*[\[(]?\s*(?:cd|disc|disk)\s*[-_.#]?\s*(\d{1,2})\b`, + ) ) // FormatForPath returns the audio format implied by a path's extension, @@ -94,16 +102,63 @@ type TrackHint struct { Folder string } +// discFolder reports whether a directory name is one disc of an album, +// and which. +func discFolder(name string) (int, bool) { + m := discFolderPattern.FindStringSubmatch(name) + if m == nil { + return 0, false + } + + n, err := strconv.Atoi(m[1]) + if err != nil || n == 0 { + return 0, false + } + + return n, true +} + +// AlbumDir is the directory that holds a file's *album*: its parent, +// or its grandparent when the parent is a disc folder. +// +// Multi-disc rips are shared as `Album/CD1/…` and `Album/CD2/…`, and +// grouping candidates by the immediate parent split one album into two +// half-albums, each titled "CD1". Neither could clear the completeness +// or album-title bars, so a multi-disc release could not be auto-picked +// at all. A disc folder at the root has no album above it and is +// returned as it is. +func AlbumDir(p string) string { + dir := path.Dir(strings.ReplaceAll(p, `\`, "/")) + + if _, ok := discFolder(path.Base(dir)); !ok { + return dir + } + + parent := path.Dir(dir) + if parent == "." || parent == "/" || parent == "" { + return dir + } + + return parent +} + // ParsePath extracts what it can from one candidate file path. func ParsePath(p string) TrackHint { // Soulseek paths are Windows-style; normalize before splitting. norm := strings.ReplaceAll(p, `\`, "/") base := path.Base(norm) - folder := path.Base(path.Dir(norm)) name := strings.TrimSuffix(base, path.Ext(base)) - hint := TrackHint{Folder: cleanAlbumName(folder)} + // The album's name is the album directory's, not a disc folder's, + // and the disc folder is where a multi-disc rip says which disc a + // file is on. A disc number in the filename ("2-01 …") is more + // specific and overrides it below. + hint := TrackHint{Folder: cleanAlbumName(path.Base(AlbumDir(norm)))} + + if disc, ok := discFolder(path.Base(path.Dir(norm))); ok { + hint.Disc = disc + } if m := trackNumPattern.FindStringSubmatch(name); m != nil { if m[1] != "" { diff --git a/backend/download/provider_slskd.go b/backend/download/provider_slskd.go index ffe0542..ae7f5f4 100644 --- a/backend/download/provider_slskd.go +++ b/backend/download/provider_slskd.go @@ -70,8 +70,9 @@ const ( slskdTransferPoll = 3 * time.Second // slskdMinFiles is the fewest audio files a folder needs before it - // is offered as a candidate. Soulseek returns a lot of one-file - // noise for common queries. + // is offered as a candidate for an album. Soulseek returns a lot of + // one-file noise for common queries. A single-track request takes + // one (see minFilesFor). slskdMinFiles = 2 // slskdHTTPTimeout bounds one API call. @@ -330,7 +331,24 @@ func (s *slskd) Search(ctx context.Context, dl Download) ([]Candidate, error) { ) }() - return s.candidatesFrom(search), nil + return s.candidatesFrom(search, minFilesFor(dl)), nil +} + +// minFilesFor is the fewest audio files a folder must offer to be a +// candidate for this request. +// +// Soulseek answers a search with the files that match it, not with the +// folders they sit in. An album query matches every file in the album's +// folder, because the folder name carries the terms; a *track* query +// usually matches one file per folder. The two-file floor that filters +// out one-file noise for an album therefore filtered out every result +// for a track, and a single-track request could never be served here. +func minFilesFor(dl Download) int { + if dl.RecordingMBID != "" { + return 1 + } + + return slskdMinFiles } // awaitSearch polls until the search completes or the budget runs out. @@ -371,8 +389,9 @@ func (s *slskd) awaitSearch( return last, nil } -// candidatesFrom groups a search's responses into candidates. -func (s *slskd) candidatesFrom(search slskdSearch) []Candidate { +// candidatesFrom groups a search's responses into candidates, dropping +// folders with fewer than minFiles audio files. +func (s *slskd) candidatesFrom(search slskdSearch, minFiles int) []Candidate { out := make([]Candidate, 0, len(search.Responses)) for _, resp := range search.Responses { @@ -400,7 +419,7 @@ func (s *slskd) candidatesFrom(search slskdSearch) []Candidate { total += f.Size } - if audio < slskdMinFiles { + if audio < minFiles { continue } @@ -421,13 +440,15 @@ func (s *slskd) candidatesFrom(search slskdSearch) []Candidate { return out } -// groupByFolder buckets a peer's files by their containing directory. +// groupByFolder buckets a peer's files by the album directory they sit +// in — the containing directory, or the one above it for a disc folder +// (see AlbumDir), so a multi-disc rip is one candidate and not two. func groupByFolder(files []slskdFile) map[string][]slskdFile { out := map[string][]slskdFile{} for _, f := range files { - norm := strings.ReplaceAll(f.Filename, `\`, "/") - out[path.Dir(norm)] = append(out[path.Dir(norm)], f) + dir := AlbumDir(f.Filename) + out[dir] = append(out[dir], f) } return out @@ -817,7 +838,13 @@ func (s *slskd) collect(c Candidate, dst string) (Result, error) { continue } + // A multi-disc candidate keeps its disc folders in staging. + // Flattened, disc 2's "01 Intro.flac" overwrites disc 1's, and + // the importer loses the folder it reads the disc number from. target := filepath.Join(dst, base) + if _, ok := discFolder(folder); ok { + target = filepath.Join(dst, folder, base) + } if err := movePath(src, target); err != nil { return Result{}, fmt.Errorf("collect %s: %w", base, err) diff --git a/backend/download/rank.go b/backend/download/rank.go index 731f9a2..ff90f46 100644 --- a/backend/download/rank.go +++ b/backend/download/rank.go @@ -347,7 +347,9 @@ func scoreMatch( TitleFit: titleFit, } - m.Completeness = completeness(len(audio), len(dl.Expected)) + m.Completeness = completeness( + alignedCount(c.Files), len(audio), len(dl.Expected), + ) // The candidate's own title, and the folder its files sit in, are // two independent guesses at the album name. Take the better one: @@ -415,30 +417,54 @@ func artistFit(want string, c Candidate) float64 { return best } -// completeness scores audio file count against the expected track -// count. Extra files are penalized far more gently than missing ones: +// completeness scores how much of the expected tracklist a candidate +// covers. Extra files are penalized far more gently than missing ones: // a folder with bonus tracks or a stray intro is still the album, while // a folder missing half the tracks is not. -func completeness(got, want int) float64 { +// +// **Coverage is counted in aligned tracks, not in files.** It used to +// be the audio file count, so any ten files scored full marks against +// a ten-track album whether or not they were its tracks — and since +// title fit is the mean over the files that *did* align, a folder where +// three titles matched read as a near-perfect candidate on both counts. +// `aligned` is how many files matchFiles assigned to an expected track; +// `audio` still sets the penalty for extras, because a folder of thirty +// files holding the ten wanted is a worse copy than one holding ten. +func completeness(aligned, audio, want int) float64 { if want == 0 { - if got > 0 { + if audio > 0 { return 0.5 } return 0 } - if got == 0 { + if aligned == 0 { return 0 } - if got >= want { - extra := float64(got-want) / float64(want) + cover := float64(min(aligned, want)) / float64(want) - return math.Max(0.75, 1.0-0.25*extra) + if audio > want { + extra := float64(audio-want) / float64(want) + cover *= math.Max(0.75, 1.0-0.25*extra) } - return float64(got) / float64(want) + return cover +} + +// alignedCount is how many audio files were assigned to an expected +// track. +func alignedCount(files []CandidateFile) int { + n := 0 + + for _, f := range files { + if f.IsAudio && f.MatchedTo != 0 { + n++ + } + } + + return n } // scoreQuality answers whether this is a good copy. diff --git a/backend/download/rank_test.go b/backend/download/rank_test.go index 9c0a2cf..4aeb93f 100644 --- a/backend/download/rank_test.go +++ b/backend/download/rank_test.go @@ -282,28 +282,32 @@ func TestCompleteness(t *testing.T) { tests := []struct { name string - got int + aligned int + audio int want int minScore float64 maxScore float64 }{ - {"exact", 10, 10, 1.0, 1.0}, - {"half missing", 5, 10, 0.49, 0.51}, - {"one bonus track", 11, 10, 0.95, 1.0}, - {"double", 20, 10, 0.74, 0.76}, - {"nothing", 0, 10, 0, 0}, - {"no expectation", 5, 0, 0.5, 0.5}, + {"exact", 10, 10, 10, 1.0, 1.0}, + {"half missing", 5, 5, 10, 0.49, 0.51}, + {"one bonus track", 10, 11, 10, 0.95, 1.0}, + {"double", 10, 20, 10, 0.74, 0.76}, + {"nothing", 0, 0, 10, 0, 0}, + {"no expectation", 0, 5, 0, 0.5, 0.5}, + // Ten files are not ten tracks: three that align are three. + {"right count, wrong tracks", 3, 10, 10, 0.29, 0.31}, + {"files that align to nothing", 0, 10, 10, 0, 0}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := completeness(tt.got, tt.want) + got := completeness(tt.aligned, tt.audio, tt.want) if got < tt.minScore || got > tt.maxScore { t.Errorf( - "completeness(%d, %d) = %f, want in [%f, %f]", - tt.got, tt.want, got, tt.minScore, tt.maxScore, + "completeness(%d, %d, %d) = %f, want in [%f, %f]", + tt.aligned, tt.audio, tt.want, got, tt.minScore, tt.maxScore, ) } }) -- 2.54.0 From 5e3ac8fb1b7723d631b49508bd00d94234bfd853 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Sat, 26 Sep 2026 17:11:23 -0400 Subject: [PATCH 15/16] fix(download): search Soulseek more than once, and read file lengths The slskd search asked one question and ignored part of the answer. Two queries. Soulseek matches every term against a file's full path, so every extra word is a filter, and several filter wrongly: an edition qualifier from the catalog title that no one puts in a folder name, a term with a leading "-", which Soulseek reads as an exclusion, and "Various Artists", which is in no one's path. When a normalised form of the request differs, it runs alongside the original and the candidates are merged by peer and folder. Concurrently, not as a fallback: the manager gives a provider one search budget, and a Soulseek search spends most of it waiting. A query the user typed is searched as written. Stated options. The search carried only its id and text, so slskd's own defaults for its timeout and response limits applied. Its timeout is now set inside our wait, the limits are well above a popular album, and slskd drops folders below the file floor and peers with a queue we would not reach today. A state-only poll. Every one-second poll re-sent every response; the responses are now fetched once at the end, falling back to the old includeResponses form for a daemon without that endpoint. Durations. slskd reports each file's length and it was discarded. It is now carried as CandidateFile.LengthMillis and scored against the expected tracks as DurationFit, which takes 0.15 of title fit's weight when at least half the aligned pairs are timed: a title says which song a file claims to be, a length says whether it is that recording. Without lengths the score is exactly the previous formula. freeUploadSlots is removed from the response type; slskd sends hasFreeUploadSlot and nothing by that name. Closes #271 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT --- backend/download/pathmatch.go | 89 ++++- backend/download/provider_slskd.go | 304 +++++++++++++++--- backend/download/provider_slskd_test.go | 37 +++ backend/download/rank.go | 37 ++- backend/download/search_recall_test.go | 238 ++++++++++++++ backend/download/types.go | 26 +- .../yellowjacket/backend/download/models.ts | 17 +- 7 files changed, 686 insertions(+), 62 deletions(-) create mode 100644 backend/download/search_recall_test.go diff --git a/backend/download/pathmatch.go b/backend/download/pathmatch.go index a9149e6..5348c4d 100644 --- a/backend/download/pathmatch.go +++ b/backend/download/pathmatch.go @@ -258,21 +258,72 @@ func AnnotateFiles(files []CandidateFile) []CandidateFile { // matchFiles aligns a candidate's audio files to the expected tracklist // and returns the per-file assignment plus the mean title similarity of -// the aligned pairs. +// the aligned pairs. alignFiles is the same alignment with the +// duration evidence as well. +func matchFiles( + files []CandidateFile, + expected []ExpectedTrack, +) ([]CandidateFile, float64) { + a := alignFiles(files, expected) + + return a.files, a.titleFit +} + +// alignment is what aligning a candidate to a tracklist found. +type alignment struct { + files []CandidateFile + + // titleFit is the mean title similarity over aligned pairs. + titleFit float64 + + // durationFit is the mean duration agreement over aligned pairs + // where both sides state a length, and timedPairs is how many such + // pairs there were. + durationFit float64 + timedPairs int + aligned int +} + +// durationAgreement scores how well a file's length matches the +// expected track's, in 0..1. Rips of the same master differ by a +// second or two of silence; a different edit, a live take or a +// truncated file differs by tens of seconds. +func durationAgreement(got, want int64) float64 { + const ( + exactMillis = 3_000 + wrongMillis = 30_000 + ) + + d := got - want + if d < 0 { + d = -d + } + + switch { + case d <= exactMillis: + return 1 + case d >= wrongMillis: + return 0 + default: + return 1 - float64(d-exactMillis)/float64(wrongMillis-exactMillis) + } +} + +// alignFiles aligns a candidate's audio files to the expected tracklist. // // Alignment is greedy by score rather than optimal: candidate folders // are small (a few dozen files at most) and the common cases — correct // track numbers, or clean "NN Title" names — are unambiguous, so the // extra machinery of Hungarian assignment buys nothing here. -func matchFiles( +func alignFiles( files []CandidateFile, expected []ExpectedTrack, -) ([]CandidateFile, float64) { +) alignment { annotated := make([]CandidateFile, len(files)) copy(annotated, files) if len(expected) == 0 { - return annotated, 0 + return alignment{files: annotated} } hints := make([]TrackHint, len(annotated)) @@ -285,8 +336,19 @@ func matchFiles( var ( total float64 matched int + + durTotal float64 + timed int ) + // timing adds a pair's duration evidence when both sides state one. + timing := func(f CandidateFile, e ExpectedTrack) { + if f.LengthMillis > 0 && e.LengthMillis > 0 { + durTotal += durationAgreement(f.LengthMillis, e.LengthMillis) + timed++ + } + } + // Pass 1: trust explicit track numbers when they are unique and in // range. A folder that numbers its files correctly is the strong // case, and title comparison only adds noise there. @@ -305,6 +367,8 @@ func matchFiles( total += autotag.TitleSimilarity(hints[i].Title, expected[idx].Title) matched++ + + timing(annotated[i], expected[idx]) } // Pass 2: title similarity for whatever is left. @@ -339,13 +403,26 @@ func matchFiles( total += bestSim matched++ + + timing(annotated[i], expected[bestIdx]) } if matched == 0 { - return annotated, 0 + return alignment{files: annotated} } - return annotated, total / float64(matched) + a := alignment{ + files: annotated, + titleFit: total / float64(matched), + timedPairs: timed, + aligned: matched, + } + + if timed > 0 { + a.durationFit = durTotal / float64(timed) + } + + return a } // indexForPosition finds the expected track at a disc/track position. diff --git a/backend/download/provider_slskd.go b/backend/download/provider_slskd.go index ae7f5f4..1286c72 100644 --- a/backend/download/provider_slskd.go +++ b/backend/download/provider_slskd.go @@ -9,6 +9,7 @@ import ( "os" "path" "path/filepath" + "regexp" "strings" "time" @@ -78,6 +79,9 @@ const ( // slskdHTTPTimeout bounds one API call. slskdHTTPTimeout = 20 * time.Second + // millisPerSecond converts slskd's whole-second file lengths. + millisPerSecond = 1000 + // slskdStallAfter is how long a grab may go without a byte arriving // before the peer is given up on. It is measured from enqueue, so // it covers a peer that queues us and never starts as well as one @@ -255,14 +259,13 @@ type slskdSearch struct { // slskdResponse is one peer's answer to a search. type slskdResponse struct { - Username string `json:"username"` - HasFreeUploadSlot bool `json:"hasFreeUploadSlot"` - QueueLength int `json:"queueLength"` - UploadSpeed int64 `json:"uploadSpeed"` - Files []slskdFile `json:"files"` - LockedFileCount int `json:"lockedFileCount"` - FileCount int `json:"fileCount"` - FreeUploadSlotFlag bool `json:"freeUploadSlots"` + Username string `json:"username"` + HasFreeUploadSlot bool `json:"hasFreeUploadSlot"` + QueueLength int `json:"queueLength"` + UploadSpeed int64 `json:"uploadSpeed"` + Files []slskdFile `json:"files"` + LockedFileCount int `json:"lockedFileCount"` + FileCount int `json:"fileCount"` } // slskdFile is one file a peer is offering. @@ -270,7 +273,9 @@ type slskdFile struct { Filename string `json:"filename"` Size int64 `json:"size"` BitRate int `json:"bitRate"` - Length int `json:"length"` + + // Length is the duration in whole seconds. + Length int `json:"length"` } // slskdTransfer is one download's state. @@ -302,24 +307,89 @@ func (t slskdTransfer) done() (finished, ok bool) { // per-folder candidates. A folder from one peer is the unit a user // actually wants: Soulseek has no album concept, but people organise // their shares by album directory. +// +// Up to two queries run at once — the request as written and a +// normalised form of it (see slskdQueries) — and their candidates are +// merged. They run concurrently rather than as a fallback because the +// manager gives a provider one search budget, and a Soulseek search +// spends most of it waiting for peers to answer; a second query after +// the first would not fit. func (s *slskd) Search(ctx context.Context, dl Download) ([]Candidate, error) { + queries := slskdQueries(dl) + if len(queries) == 0 { + return nil, nil + } + + type found struct { + candidates []Candidate + err error + } + + results := make(chan found, len(queries)) + + for _, q := range queries { + go func(q string) { + c, err := s.searchOnce(ctx, q, minFilesFor(dl)) + results <- found{candidates: c, err: err} + }(q) + } + + var ( + out []Candidate + seen = map[string]bool{} + firstErr error + answered int + ) + + for range queries { + r := <-results + if r.err != nil { + s.logger.Debug("slskd search failed", "error", r.err) + + if firstErr == nil { + firstErr = r.err + } + + continue + } + + answered++ + + // The same peer's folder turns up under both queries; the ID is + // peer and folder, so it is the same candidate. + for _, c := range r.candidates { + if seen[c.ID] { + continue + } + + seen[c.ID] = true + + out = append(out, c) + } + } + + if answered == 0 { + return nil, firstErr + } + + return out, nil +} + +// searchOnce runs one query to completion and returns its candidates. +func (s *slskd) searchOnce( + ctx context.Context, + text string, + minFiles int, +) ([]Candidate, error) { // slskd's search endpoint deserializes id as a .NET Guid server-side, // so it must be a dashed UUID — the app's own newID() (a plain hex // string, used for request/item IDs elsewhere) is rejected with an // HTTP 400 before any search happens. searchID := uuid.NewString() - body := map[string]any{ - "id": searchID, - "searchText": dl.SearchText(), - } - - if err := s.client.post(ctx, "/api/v0/searches", body, nil); err != nil { - return nil, err - } - - search, err := s.awaitSearch(ctx, searchID) - if err != nil { + if err := s.client.post( + ctx, "/api/v0/searches", s.searchRequest(searchID, text, minFiles), nil, + ); err != nil { return nil, err } @@ -331,7 +401,123 @@ func (s *slskd) Search(ctx context.Context, dl Download) ([]Candidate, error) { ) }() - return s.candidatesFrom(search, minFilesFor(dl)), nil + if err := s.awaitSearch(ctx, searchID); err != nil { + return nil, err + } + + responses, err := s.searchResponses(ctx, searchID) + if err != nil { + return nil, err + } + + return s.candidatesFrom(responses, minFiles), nil +} + +// searchRequest is the body that starts a search. +// +// Every option is stated rather than left to the daemon, because +// slskd's defaults are its own and not ours. Its search timeout in +// particular has to finish inside our wait: a search that slskd is still +// running when we stop polling is results we asked for and discarded. +// The response and file limits are raised well above what a popular +// album produces, and the peer filters let slskd drop answers this +// provider would only score down to nothing — a folder too small to be +// a candidate, a peer with a queue it will not reach today. +func (s *slskd) searchRequest(id, text string, minFiles int) map[string]any { + const ( + responseLimit = 500 + fileLimit = 20_000 + maximumPeerQueueLength = 100 + ) + + // A tenth of the wait is left for the last poll and the responses + // fetch. + timeout := s.searchWait - s.searchWait/10 + + return map[string]any{ + "id": id, + "searchText": text, + "searchTimeout": timeout.Milliseconds(), + "responseLimit": responseLimit, + "fileLimit": fileLimit, + "filterResponses": true, + "minimumResponseFileCount": minFiles, + "maximumPeerQueueLength": maximumPeerQueueLength, + } +} + +// slskdQueries is what is searched for a request: the request's own +// search text, and a normalised form of it when that differs. +// +// Soulseek matches every term against the file's full path, so each +// extra word is a filter, and some words filter wrongly: +// +// - edition qualifiers — "(Deluxe Edition)", "[2011 Remaster]" — are +// in the catalog's title and rarely in anyone's folder name; +// - punctuation splits a term oddly, and a term that starts with "-" +// is an *exclusion*, so an album called "-ism" searches for +// everything without it; +// - "Various Artists" is in no one's path for a compilation. +// +// A query the user typed is theirs and is searched exactly as written. +func slskdQueries(dl Download) []string { + primary := strings.TrimSpace(dl.SearchText()) + if primary == "" { + return nil + } + + out := []string{primary} + + if dl.Query != "" { + return out + } + + artist := dl.Artist + if isVariousArtists(artist) { + artist = "" + } + + normal := Download{ + Artist: normalizeSearchTerms(artist), + Album: normalizeSearchTerms(editionPattern.ReplaceAllString(dl.Album, " ")), + } + + if alt := strings.TrimSpace(normal.SearchText()); alt != "" && + !strings.EqualFold(alt, primary) { + out = append(out, alt) + } + + return out +} + +var ( + // editionPattern finds an edition qualifier: a bracketed group that + // names an edition, or a trailing " - 2011 Remaster". + editionPattern = regexp.MustCompile( + `(?i)\s*[(\[][^)\]]*\b(?:deluxe|edition|remaster(?:ed)?|expanded|` + + `anniversary|bonus|explicit|reissue|special|collector'?s?|` + + `version|mono|stereo)\b[^)\]]*[)\]]` + + `|\s+-\s+(?:\d{4}\s+)?remaster(?:ed)?\b.*$`, + ) + + // nonWordPattern is everything that is not a letter or a digit. + nonWordPattern = regexp.MustCompile(`[^\p{L}\p{N}]+`) +) + +// normalizeSearchTerms reduces text to plain words. +func normalizeSearchTerms(s string) string { + return strings.Join(strings.Fields(nonWordPattern.ReplaceAllString(s, " ")), " ") +} + +// isVariousArtists reports whether an artist credit is a compilation's +// placeholder rather than an artist. +func isVariousArtists(artist string) bool { + switch strings.ToLower(strings.TrimSpace(artist)) { + case "various artists", "various", "va": + return true + default: + return false + } } // minFilesFor is the fewest audio files a folder must offer to be a @@ -354,47 +540,83 @@ func minFilesFor(dl Download) int { // awaitSearch polls until the search completes or the budget runs out. // A timeout is not an error: partial Soulseek results are normal and // often good enough. -func (s *slskd) awaitSearch( - ctx context.Context, - searchID string, -) (slskdSearch, error) { +// +// The poll asks for the search's state only. It used to ask for every +// response on every one-second tick, which for a popular album is the +// same few thousand file entries serialised twenty times to be read +// once; searchResponses fetches them once at the end. +func (s *slskd) awaitSearch(ctx context.Context, searchID string) error { deadline := time.Now().Add(s.searchWait) - var last slskdSearch - for time.Now().Before(deadline) { select { case <-ctx.Done(): - return last, fmt.Errorf("%w: search cancelled", ErrSlskdTimeout) + return fmt.Errorf("%w: search cancelled", ErrSlskdTimeout) case <-time.After(s.searchPoll): } var search slskdSearch if err := s.client.get( - ctx, - "/api/v0/searches/"+searchID+"?includeResponses=true", - &search, + ctx, "/api/v0/searches/"+searchID, &search, ); err != nil { - return last, err + return err } - last = search - if search.IsComplete { - return search, nil + return nil } } - return last, nil + return nil +} + +// searchResponses fetches a search's responses once. +// +// `/searches/{id}/responses` is the endpoint for that; a daemon that +// does not answer it is asked the older way, with the search itself +// carrying its responses, so an older slskd degrades to the previous +// behaviour rather than to no results at all. +func (s *slskd) searchResponses( + ctx context.Context, + searchID string, +) ([]slskdResponse, error) { + var responses []slskdResponse + + err := s.client.get( + ctx, "/api/v0/searches/"+searchID+"/responses", &responses, + ) + if err == nil { + return responses, nil + } + + s.logger.Debug( + "slskd responses endpoint failed; asking with the search", + "error", err, + ) + + var search slskdSearch + + if err := s.client.get( + ctx, + "/api/v0/searches/"+searchID+"?includeResponses=true", + &search, + ); err != nil { + return nil, err + } + + return search.Responses, nil } // candidatesFrom groups a search's responses into candidates, dropping // folders with fewer than minFiles audio files. -func (s *slskd) candidatesFrom(search slskdSearch, minFiles int) []Candidate { - out := make([]Candidate, 0, len(search.Responses)) +func (s *slskd) candidatesFrom( + responses []slskdResponse, + minFiles int, +) []Candidate { + out := make([]Candidate, 0, len(responses)) - for _, resp := range search.Responses { + for _, resp := range responses { for folder, files := range groupByFolder(resp.Files) { audio := 0 @@ -414,6 +636,8 @@ func (s *slskd) candidatesFrom(search slskdSearch, minFiles int) []Candidate { Format: format, Bitrate: f.BitRate, IsAudio: isAudio, + + LengthMillis: int64(f.Length) * millisPerSecond, }) total += f.Size @@ -462,7 +686,7 @@ func groupByFolder(files []slskdFile) map[string][]slskdFile { func peerHealth(r slskdResponse) float64 { score := 0.35 - if r.HasFreeUploadSlot || r.FreeUploadSlotFlag { + if r.HasFreeUploadSlot { score += 0.4 } diff --git a/backend/download/provider_slskd_test.go b/backend/download/provider_slskd_test.go index de855a4..d06cec5 100644 --- a/backend/download/provider_slskd_test.go +++ b/backend/download/provider_slskd_test.go @@ -46,6 +46,15 @@ type slskdStub struct { // unauthorized makes every call return 401. unauthorized bool + + // searches records every search request body, and searchGets the + // request URI of every search GET. + searches []map[string]any + searchGets []string + + // noResponsesEndpoint makes /searches/{id}/responses 404, as an + // older daemon would. + noResponsesEndpoint bool } func newSlskdStub(t *testing.T) *slskdStub { @@ -67,6 +76,16 @@ func newSlskdStub(t *testing.T) *slskdStub { return } + var body map[string]any + + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Errorf("decode search body: %v", err) + } + + s.mu.Lock() + s.searches = append(s.searches, body) + s.mu.Unlock() + w.WriteHeader(http.StatusCreated) }) @@ -83,8 +102,26 @@ func newSlskdStub(t *testing.T) *slskdStub { s.mu.Lock() responses := s.responses + noEndpoint := s.noResponsesEndpoint + s.searchGets = append(s.searchGets, r.URL.RequestURI()) s.mu.Unlock() + if strings.HasSuffix(r.URL.Path, "/responses") { + if noEndpoint { + w.WriteHeader(http.StatusNotFound) + + return + } + + writeJSON(t, w, responses) + + return + } + + if r.URL.Query().Get("includeResponses") != "true" { + responses = nil + } + writeJSON(t, w, slskdSearch{ ID: "search-1", IsComplete: true, diff --git a/backend/download/rank.go b/backend/download/rank.go index ff90f46..b50abb1 100644 --- a/backend/download/rank.go +++ b/backend/download/rank.go @@ -35,6 +35,15 @@ const ( weightArtistFit = 0.12 ) +// Match sub-weights when the candidate's durations are known. Duration +// takes its weight from title fit, the signal it corroborates: a title +// says which song a file claims to be, a length says whether it is that +// recording — the right edit, the whole file, not the live take. +const ( + timedWeightTitleFit = 0.25 + timedWeightDurationFit = 0.15 +) + // Quality sub-weights. Each set sums to 1.0. // // There are two of them because a stated preference changes what the @@ -319,13 +328,13 @@ func Score(dl Download, c Candidate, priority int, prefs AutoDownloadPrefs) Cand audio := c.AudioFiles() - matched, titleFit := matchFiles(audio, dl.Expected) + a := alignFiles(audio, dl.Expected) // Write the alignment back so the picker can show which file maps // to which track. - c.Files = mergeMatched(c.Files, matched) + c.Files = mergeMatched(c.Files, a.files) - c.Match = scoreMatch(dl, c, audio, titleFit) + c.Match = scoreMatch(dl, c, audio, a) c.Quality = scoreQuality( c, audio, priority, prefs, dl.runtimeMillis(), ) @@ -340,11 +349,16 @@ func scoreMatch( dl Download, c Candidate, audio []CandidateFile, - titleFit float64, + a alignment, ) MatchScore { m := MatchScore{ - Anchored: dl.Anchored(), - TitleFit: titleFit, + Anchored: dl.Anchored(), + TitleFit: a.titleFit, + DurationFit: a.durationFit, + + // Durations count once at least half the aligned pairs state + // one; a single timed pair would be a coin toss carrying 15%. + DurationKnown: a.timedPairs > 0 && a.timedPairs*2 >= a.aligned, } m.Completeness = completeness( @@ -369,9 +383,16 @@ func scoreMatch( // With no expected tracklist there is no title signal at all, so // redistribute its weight onto the album/artist evidence rather // than scoring every free-text result as half-wrong. - if len(dl.Expected) == 0 { + switch { + case len(dl.Expected) == 0: m.Overall = 0.55*m.AlbumFit + 0.45*m.ArtistFit - } else { + case m.DurationKnown: + m.Overall = timedWeightTitleFit*m.TitleFit + + timedWeightDurationFit*m.DurationFit + + weightCompleteness*m.Completeness + + weightAlbumFit*m.AlbumFit + + weightArtistFit*m.ArtistFit + default: m.Overall = weightTitleFit*m.TitleFit + weightCompleteness*m.Completeness + weightAlbumFit*m.AlbumFit + diff --git a/backend/download/search_recall_test.go b/backend/download/search_recall_test.go new file mode 100644 index 0000000..4973733 --- /dev/null +++ b/backend/download/search_recall_test.go @@ -0,0 +1,238 @@ +package download + +import ( + "context" + "strings" + "testing" +) + +// What Soulseek is asked, how, and what is kept from the answer (#271). + +func TestSlskdQueries(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + dl Download + want []string + }{ + { + name: "a plain request is searched once", + dl: Download{Artist: "Radiohead", Album: "OK Computer"}, + want: []string{"Radiohead OK Computer"}, + }, + { + name: "an edition qualifier gets a second query without it", + dl: Download{Artist: "Radiohead", Album: "OK Computer (Collector's Edition)"}, + want: []string{ + "Radiohead OK Computer (Collector's Edition)", + "Radiohead OK Computer", + }, + }, + { + name: "a trailing remaster note", + dl: Download{Artist: "Pink Floyd", Album: "Animals - 2018 Remaster"}, + want: []string{ + "Pink Floyd Animals - 2018 Remaster", + "Pink Floyd Animals", + }, + }, + { + name: "a leading dash would be an exclusion", + dl: Download{Artist: "Mocky", Album: "-ism"}, + want: []string{"Mocky -ism", "Mocky ism"}, + }, + { + name: "a compilation is not searched by its placeholder artist", + dl: Download{Artist: "Various Artists", Album: "Pulp Fiction"}, + want: []string{"Various Artists Pulp Fiction", "Pulp Fiction"}, + }, + { + name: "what the user typed is searched as written", + dl: Download{Query: "ok computer (deluxe)", Album: "OK Computer (Deluxe)"}, + want: []string{"ok computer (deluxe)"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + got := slskdQueries(tc.dl) + if strings.Join(got, "|") != strings.Join(tc.want, "|") { + t.Errorf("slskdQueries = %q, want %q", got, tc.want) + } + }) + } +} + +// Both queries run, the options are stated rather than left to the +// daemon's defaults, and a folder both queries found is one candidate. +func TestSlskdSearchRunsBothQueriesAndMerges(t *testing.T) { + t.Parallel() + + stub := newSlskdStub(t) + stub.responses = []slskdResponse{{ + Username: "peer", + Files: []slskdFile{ + {Filename: `\m\Radiohead - OK Computer\01 Airbag.flac`, Size: 1, Length: 284}, + {Filename: `\m\Radiohead - OK Computer\02 Paranoid Android.flac`, Size: 1, Length: 383}, + }, + }} + + s, _ := newStubSlskd(t, stub) + + got, err := s.Search(context.Background(), Download{ + ReleaseMBID: "rel", Artist: "Radiohead", Album: "OK Computer (Deluxe Edition)", + }) + if err != nil { + t.Fatalf("Search: %v", err) + } + + if len(got) != 1 { + t.Fatalf("got %d candidates, want the one folder once", len(got)) + } + + if got[0].Files[0].LengthMillis != 284_000 { + t.Errorf("length = %d ms, want 284000 from slskd's seconds", got[0].Files[0].LengthMillis) + } + + stub.mu.Lock() + searches := append([]map[string]any(nil), stub.searches...) + gets := append([]string(nil), stub.searchGets...) + stub.mu.Unlock() + + if len(searches) != 2 { + t.Fatalf("ran %d searches, want 2", len(searches)) + } + + for _, body := range searches { + for _, key := range []string{ + "searchTimeout", "responseLimit", "fileLimit", + "minimumResponseFileCount", "maximumPeerQueueLength", + } { + if _, ok := body[key]; !ok { + t.Errorf("search %q does not state %s", body["searchText"], key) + } + } + } + + // The responses are fetched once at the end, not with every poll. + for _, uri := range gets { + if strings.Contains(uri, "includeResponses") { + t.Errorf("poll %s asked for every response", uri) + } + } +} + +// A daemon without the responses endpoint still returns results. +func TestSlskdSearchFallsBackForAnOlderDaemon(t *testing.T) { + t.Parallel() + + stub := newSlskdStub(t) + stub.noResponsesEndpoint = true + stub.responses = []slskdResponse{{ + Username: "peer", + Files: []slskdFile{ + {Filename: `\m\Album\01 A.flac`, Size: 1}, + {Filename: `\m\Album\02 B.flac`, Size: 1}, + }, + }} + + s, _ := newStubSlskd(t, stub) + + got, err := s.Search(context.Background(), Download{Query: "album"}) + if err != nil { + t.Fatalf("Search: %v", err) + } + + if len(got) != 1 { + t.Errorf("got %d candidates, want 1 through the fallback", len(got)) + } +} + +func TestDurationAgreement(t *testing.T) { + t.Parallel() + + cases := []struct { + got, want int64 + score float64 + }{ + {300_000, 300_000, 1}, + {301_500, 300_000, 1}, // a second of silence + {300_000, 316_500, 0.5}, + {300_000, 345_000, 0}, // a different edit + } + + for _, tc := range cases { + if got := durationAgreement(tc.got, tc.want); got < tc.score-0.01 || got > tc.score+0.01 { + t.Errorf("durationAgreement(%d, %d) = %f, want %f", tc.got, tc.want, got, tc.score) + } + } +} + +// Two folders with the same track names are told apart by their +// lengths: one is the album, the other a live record of the same songs. +func TestDurationsSeparateTheRightRecording(t *testing.T) { + t.Parallel() + + dl := okComputer() + + timed := func(id string, lengths ...int64) Candidate { + c := candidateFor(id, allTitles(), ".flac", 30_000_000) + for i := range c.Files { + c.Files[i].LengthMillis = lengths[i] + } + + return c + } + + studio := timed("studio", trackMillis, trackMillis+1_000, trackMillis, trackMillis-500) + live := timed( + "live", + trackMillis+60_000, + trackMillis+75_000, + trackMillis+50_000, + trackMillis+90_000, + ) + + ranked := Rank(dl, []Candidate{live, studio}, nil, AutoDownloadPrefs{}) + + if ranked[0].ID != "studio" { + t.Fatalf("winner = %s, want the recording whose lengths match", ranked[0].ID) + } + + if !ranked[0].Match.DurationKnown || ranked[0].Match.DurationFit < 0.99 { + t.Errorf( + "studio duration fit = %f known=%v", + ranked[0].Match.DurationFit, + ranked[0].Match.DurationKnown, + ) + } + + if ranked[1].Match.DurationFit != 0 { + t.Errorf("live duration fit = %f, want 0", ranked[1].Match.DurationFit) + } +} + +// Without lengths the score is exactly what it was before durations +// were read, so a provider that reports none is not penalised. +func TestUnknownDurationsLeaveTheScoreAlone(t *testing.T) { + t.Parallel() + + dl := okComputer() + c := Score(dl, candidateFor("c", allTitles(), ".flac", 30_000_000), 50, AutoDownloadPrefs{}) + + if c.Match.DurationKnown { + t.Fatal("no file states a length, yet durations are known") + } + + want := weightTitleFit*c.Match.TitleFit + + weightCompleteness*c.Match.Completeness + + weightAlbumFit*c.Match.AlbumFit + + weightArtistFit*c.Match.ArtistFit + + if c.Match.Overall != want { + t.Errorf("match = %f, want the untimed formula's %f", c.Match.Overall, want) + } +} diff --git a/backend/download/types.go b/backend/download/types.go index d76c2ec..b753267 100644 --- a/backend/download/types.go +++ b/backend/download/types.go @@ -232,12 +232,17 @@ type Candidate struct { // results give paths and sizes but no tags, so Format and duration are // inferred from the path and size where possible. type CandidateFile struct { - Path string `json:"path"` - Size int64 `json:"size"` - Format Format `json:"format"` - Bitrate int `json:"bitrate,omitempty"` // kbps, 0 when unknown - IsAudio bool `json:"isAudio"` - MatchedTo int `json:"matchedTo,omitempty"` // expected track position + Path string `json:"path"` + Size int64 `json:"size"` + Format Format `json:"format"` + Bitrate int `json:"bitrate,omitempty"` // kbps, 0 when unknown + IsAudio bool `json:"isAudio"` + + // LengthMillis is the file's duration as the source reports it, or + // 0 when it does not. Soulseek reports it for most audio files. + LengthMillis int64 `json:"lengthMillis,omitempty"` + + MatchedTo int `json:"matchedTo,omitempty"` // expected track position } // Format is a normalized audio container/codec name. @@ -286,7 +291,14 @@ type MatchScore struct { TitleFit float64 `json:"titleFit"` // filenames vs expected titles ArtistFit float64 `json:"artistFit"` // path/origin vs expected artist AlbumFit float64 `json:"albumFit"` // folder name vs album title - Completeness float64 `json:"completeness"` // audio files vs expected count + Completeness float64 `json:"completeness"` // aligned tracks vs expected count + + // DurationFit is how well the aligned files' lengths agree with the + // expected tracks', and DurationKnown whether enough of them stated + // a length for that to count. When it does not, the score is the + // four text signals alone, exactly as before durations were read. + DurationFit float64 `json:"durationFit"` + DurationKnown bool `json:"durationKnown"` // Anchored records whether an MBID drove this score. Unanchored // matches are capped, because there is nothing to be right about. diff --git a/frontend/bindings/yellowjacket/backend/download/models.ts b/frontend/bindings/yellowjacket/backend/download/models.ts index fbe539a..0db4f82 100644 --- a/frontend/bindings/yellowjacket/backend/download/models.ts +++ b/frontend/bindings/yellowjacket/backend/download/models.ts @@ -121,6 +121,12 @@ export interface CandidateFile { "bitrate"?: number; "isAudio": boolean; + /** + * LengthMillis is the file's duration as the source reports it, or + * 0 when it does not. Soulseek reports it for most audio files. + */ + "lengthMillis"?: number; + /** * expected track position */ @@ -453,10 +459,19 @@ export interface MatchScore { "albumFit": number; /** - * audio files vs expected count + * aligned tracks vs expected count */ "completeness": number; + /** + * DurationFit is how well the aligned files' lengths agree with the + * expected tracks', and DurationKnown whether enough of them stated + * a length for that to count. When it does not, the score is the + * four text signals alone, exactly as before durations were read. + */ + "durationFit": number; + "durationKnown": boolean; + /** * Anchored records whether an MBID drove this score. Unanchored * matches are capped, because there is nothing to be right about. -- 2.54.0 From 9710c1147689c239cba8c37b803d32d5ea7f5922 Mon Sep 17 00:00:00 2001 From: Caleb Allen Date: Sat, 26 Sep 2026 17:31:35 -0400 Subject: [PATCH 16/16] feat(download): one grab per Soulseek peer, several peers at once slskd was capped at one transfer per daemon, on the grounds that Soulseek peers punish clients that ask for too much. That politeness is per peer: two different users do not compete for anyone's upload slot. So one slow peer serialised every other Soulseek download behind it. The manager now takes a per-(provider, peer) lock before any slot, so a grab waiting on a busy peer does not hold a provider slot another peer could use, and the slskd default rises to 3, which now counts peers. The help text says so. Running grabs at once exposed the folder collision: slskd names a download's directory after the remote leaf folder, so two peers' "Greatest Hits" (or any two rips' "CD1") share one directory, and collect finds files by name there. Grabs whose local folders overlap now take a package-level lock per folder, in sorted order, keyed on the full path because two clients can share one daemon. Closes #272 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT --- backend/download/concurrency_test.go | 23 +-- backend/download/keyedlock.go | 77 ++++++++ backend/download/manager.go | 45 ++++- backend/download/peer_concurrency_test.go | 224 ++++++++++++++++++++++ backend/download/provider.go | 15 +- backend/download/provider_slskd.go | 60 ++++++ 6 files changed, 420 insertions(+), 24 deletions(-) create mode 100644 backend/download/keyedlock.go create mode 100644 backend/download/peer_concurrency_test.go diff --git a/backend/download/concurrency_test.go b/backend/download/concurrency_test.go index 729c07f..2181582 100644 --- a/backend/download/concurrency_test.go +++ b/backend/download/concurrency_test.go @@ -79,9 +79,9 @@ func TestConcurrencyForPrefersOverrideThenKind(t *testing.T) { want int }{ { - name: "slskd defaults to one", + name: "slskd defaults to a few peers", cfg: Config{Kind: KindSlskd}, - want: 1, + want: 3, }, { name: "usenet defaults higher", @@ -92,9 +92,9 @@ func TestConcurrencyForPrefersOverrideThenKind(t *testing.T) { name: "explicit override wins", cfg: Config{ Kind: KindSlskd, - Settings: map[string]string{concurrencyKey: "3"}, + Settings: map[string]string{concurrencyKey: "1"}, }, - want: 3, + want: 1, }, { name: "nonsense override falls back", @@ -102,7 +102,7 @@ func TestConcurrencyForPrefersOverrideThenKind(t *testing.T) { Kind: KindSlskd, Settings: map[string]string{concurrencyKey: "not a number"}, }, - want: 1, + want: 3, }, { name: "zero override falls back", @@ -110,7 +110,7 @@ func TestConcurrencyForPrefersOverrideThenKind(t *testing.T) { Kind: KindSlskd, Settings: map[string]string{concurrencyKey: "0"}, }, - want: 1, + want: 3, }, { name: "unknown kind falls back to the global default", @@ -126,9 +126,9 @@ func TestConcurrencyForPrefersOverrideThenKind(t *testing.T) { } } -// The reason the per-provider cap exists: a Soulseek daemon capped at -// one transfer must serialize, even when the global cap would allow -// more and the user has queued several albums at once. +// The reason the per-provider cap exists: a daemon capped at one +// transfer must serialize, even when the global cap would allow more and +// the user has queued several albums at once. func TestPerProviderCapSerializesTransfers(t *testing.T) { t.Parallel() @@ -142,6 +142,7 @@ func TestPerProviderCapSerializesTransfers(t *testing.T) { ID: 1, Kind: KindSlskd, Priority: 50, + Settings: map[string]string{concurrencyKey: "1"}, }, slow) // Three requests against the same one-at-a-time provider. @@ -210,8 +211,8 @@ func TestSyncSemaphoresReplacesChangedLimits(t *testing.T) { f.manager.installProvider(Config{ID: 1, Kind: KindSlskd}, nil) first := f.manager.semaphoreFor(1) - if cap(first) != 1 { - t.Fatalf("slskd semaphore cap = %d, want 1", cap(first)) + if want := kindConcurrency[KindSlskd]; cap(first) != want { + t.Fatalf("slskd semaphore cap = %d, want %d", cap(first), want) } // Same limit: the semaphore is kept, so in-flight accounting is not diff --git a/backend/download/keyedlock.go b/backend/download/keyedlock.go new file mode 100644 index 0000000..95f45f8 --- /dev/null +++ b/backend/download/keyedlock.go @@ -0,0 +1,77 @@ +package download + +import ( + "context" + "sync" +) + +// keyedLock is a set of mutexes created on demand, one per key, that +// honour a context while waiting. An entry lives only while someone +// holds or waits on it, so a key per Soulseek peer or per folder name +// does not accumulate for the life of the process. +type keyedLock[K comparable] struct { + mu sync.Mutex + held map[K]*keyedEntry +} + +type keyedEntry struct { + ch chan struct{} + + // refs counts holders and waiters; the entry is dropped at zero. + refs int +} + +// acquire blocks until k is free or ctx ends, and returns the function +// that frees it. +func (l *keyedLock[K]) acquire(ctx context.Context, k K) (func(), error) { + l.mu.Lock() + + if l.held == nil { + l.held = map[K]*keyedEntry{} + } + + e, ok := l.held[k] + if !ok { + e = &keyedEntry{ch: make(chan struct{}, 1)} + l.held[k] = e + } + + e.refs++ + + l.mu.Unlock() + + select { + case e.ch <- struct{}{}: + case <-ctx.Done(): + l.drop(k, e) + + return nil, ctx.Err() + } + + var once sync.Once + + return func() { + once.Do(func() { + <-e.ch + l.drop(k, e) + }) + }, nil +} + +func (l *keyedLock[K]) drop(k K, e *keyedEntry) { + l.mu.Lock() + defer l.mu.Unlock() + + e.refs-- + if e.refs == 0 { + delete(l.held, k) + } +} + +// size reports how many keys are held or awaited, for tests. +func (l *keyedLock[K]) size() int { + l.mu.Lock() + defer l.mu.Unlock() + + return len(l.held) +} diff --git a/backend/download/manager.go b/backend/download/manager.go index 2af26c4..3b429de 100644 --- a/backend/download/manager.go +++ b/backend/download/manager.go @@ -59,13 +59,15 @@ const concurrencyKey = "maxConcurrent" // A single global cap is the wrong shape here: usenet and torrent // clients are built to run many transfers at once and are throttled by // bandwidth, while Soulseek transfers come from one person's home -// upload slot. Hitting the same peer with parallel requests gets you -// queued behind everyone else at best and banned at worst, so slskd is -// capped at one — the polite number, and the one that actually -// completes fastest, because a Soulseek peer serves one file at a time -// regardless of how many you ask for. +// upload slot. Politeness there is per *peer* — asking one user for two +// folders at once gets you queued behind everyone else at best and +// banned at worst — and the manager holds that line separately, one +// grab per peer (peerLocks). Two different users do not compete for +// anyone's slot, so the daemon-wide number only bounds how many peers +// are asked at once, and one slow peer no longer serialises every other +// Soulseek download behind it. var kindConcurrency = map[Kind]int{ - KindSlskd: 1, + KindSlskd: 3, KindYtDlp: 2, KindQBittorrent: 4, KindSABnzbd: 4, @@ -155,6 +157,11 @@ type Manager struct { semMu sync.Mutex provSem map[int64]chan struct{} + // peerLocks holds one grab per Soulseek peer, taken before any + // slot: a grab waiting for a busy peer must not sit on a provider + // slot another peer could be using. + peerLocks keyedLock[peerKey] + // delegatePoll is how often delegating managers are asked for // status. A field rather than the constant so tests can drive the // full delegate flow without sleeping through it. @@ -790,6 +797,23 @@ func (m *Manager) grab( // whole list for six hours. const maxGrabAttempts = 3 +// peerKey names one Soulseek user on one daemon. The same username on +// two daemons is two logins and two queues. +type peerKey struct { + provider int64 + peer string +} + +// peerKeyFor returns the peer a candidate is fetched from, when the +// source is one where asking a peer for two things at once is rude. +func peerKeyFor(c Candidate) (peerKey, bool) { + if c.Kind != KindSlskd || c.Origin == "" { + return peerKey{}, false + } + + return peerKey{provider: c.ProviderID, peer: c.Origin}, true +} + // grabOutcome is how one candidate's attempt ended. type grabOutcome struct { item DownloadItem @@ -825,6 +849,15 @@ func (m *Manager) attemptGrab( } if !plan.delegated() { + if key, ok := peerKeyFor(c); ok { + release, err := m.peerLocks.acquire(ctx, key) + if err != nil { + return grabOutcome{err: err} + } + + defer release() + } + provSem := m.semaphoreFor(plan.transportID) select { diff --git a/backend/download/peer_concurrency_test.go b/backend/download/peer_concurrency_test.go new file mode 100644 index 0000000..113604f --- /dev/null +++ b/backend/download/peer_concurrency_test.go @@ -0,0 +1,224 @@ +package download + +import ( + "context" + "errors" + "path/filepath" + "sync" + "testing" + "time" +) + +// Soulseek politeness is per peer, not per daemon (#272). + +func TestKeyedLockSerialisesOneKeyOnly(t *testing.T) { + t.Parallel() + + var l keyedLock[string] + + ctx := context.Background() + + releaseA, err := l.acquire(ctx, "a") + if err != nil { + t.Fatalf("acquire a: %v", err) + } + + // Another key is free while "a" is held. + releaseB, err := l.acquire(ctx, "b") + if err != nil { + t.Fatalf("acquire b: %v", err) + } + + releaseB() + + // The same key waits, and gives up with its context. + short, cancel := context.WithTimeout(ctx, 20*time.Millisecond) + defer cancel() + + if _, err := l.acquire(short, "a"); !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("second acquire of a held key = %v, want the deadline", err) + } + + releaseA() + releaseA() // Idempotent: a second call must not free someone else's hold. + + if n := l.size(); n != 0 { + t.Errorf("%d keys left behind, want none once nobody holds or waits", n) + } +} + +// grabEach runs one grab per candidate and returns a function that waits +// for all of them; grabAll's reasons for waiting apply. +func grabEach(t *testing.T, f managerFixture, cands []Candidate) func() { + t.Helper() + + ctx := context.Background() + + var wg sync.WaitGroup + + for i, c := range cands { + dl := fourTrackDownload() + dl.ID = "dl-" + string(rune('a'+i)) + + if err := f.store.CreateDownload(ctx, dl); err != nil { + t.Fatalf("CreateDownload: %v", err) + } + + wg.Add(1) + + go func() { + defer wg.Done() + + f.manager.grab(ctx, dl, c, nil, false) + }() + } + + return func() { + done := make(chan struct{}) + + go func() { + wg.Wait() + close(done) + }() + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Error("transfers did not finish") + } + } +} + +func slskdCandidates(p *FakeProvider, peers ...string) []Candidate { + out := make([]Candidate, 0, len(peers)) + + for i, peer := range peers { + c := p.Candidates[0] + c.ID = c.ID + "-" + itoa(i) + c.Kind = KindSlskd + c.ProviderID = 1 + c.Origin = peer + out = append(out, c) + } + + return out +} + +// Three albums from one user are asked for one at a time, even though +// the daemon would allow three transfers. +func TestOnePeerIsAskedForOneThingAtATime(t *testing.T) { + t.Parallel() + + f := newManagerFixture(t) + f.manager.SetMaxConcurrent(4) + + p := fakeWithAlbum(1, "slskd", ".flac") + p.GrabGate = make(chan struct{}) + + f.manager.installProvider(Config{ID: 1, Kind: KindSlskd, Priority: 50}, p) + + wait := grabEach(t, f, slskdCandidates(p, "alice", "alice", "alice")) + + waitFor(t, func() bool { return p.GrabCallCount() >= 1 }, "no grab started") + time.Sleep(150 * time.Millisecond) + + if got := p.MaxParallelGrabs(); got != 1 { + t.Errorf("%d simultaneous grabs from one peer, want 1", got) + } + + close(p.GrabGate) + + waitFor(t, func() bool { return p.GrabCallCount() == 3 }, "queued grabs never ran") + wait() + + if n := f.manager.peerLocks.size(); n != 0 { + t.Errorf("%d peer locks left behind", n) + } +} + +// Different users run at once, up to the daemon's cap — the point of +// the change: one slow peer no longer holds up every other. +func TestDifferentPeersRunTogether(t *testing.T) { + t.Parallel() + + f := newManagerFixture(t) + f.manager.SetMaxConcurrent(8) + + p := fakeWithAlbum(1, "slskd", ".flac") + p.GrabGate = make(chan struct{}) + + f.manager.installProvider(Config{ID: 1, Kind: KindSlskd, Priority: 50}, p) + + wait := grabEach(t, f, slskdCandidates(p, "alice", "bob", "carol", "dave")) + + waitFor( + t, + func() bool { return p.MaxParallelGrabs() >= kindConcurrency[KindSlskd] }, + "different peers were serialised", + ) + time.Sleep(100 * time.Millisecond) + + if got := p.MaxParallelGrabs(); got != kindConcurrency[KindSlskd] { + t.Errorf("%d simultaneous grabs, want the daemon cap %d", got, kindConcurrency[KindSlskd]) + } + + close(p.GrabGate) + wait() +} + +func TestSlskdLocalFolders(t *testing.T) { + t.Parallel() + + s := &slskd{downloadsPath: "/dl"} + + got := s.localFolders(Candidate{Files: []CandidateFile{ + {Path: `\m\The Wall\CD2\01 Hey You.flac`}, + {Path: `\m\The Wall\CD1\01 In The Flesh.flac`}, + {Path: `\m\The Wall\CD1\02 The Thin Ice.flac`}, + }}) + + want := []string{filepath.Join("/dl", "CD1"), filepath.Join("/dl", "CD2")} + if len(got) != len(want) || got[0] != want[0] || got[1] != want[1] { + t.Errorf("localFolders = %q, want %q", got, want) + } +} + +// Two peers' "Greatest Hits" land in one slskd directory, so the second +// grab does not enqueue until the first has collected its files. +func TestSlskdSameFolderNameWaits(t *testing.T) { + t.Parallel() + + stub := newSlskdStub(t) + s, downloads := newStubSlskd(t, stub) + + c := Candidate{ + Payload: map[string]string{"username": "bob"}, + Files: []CandidateFile{ + {Path: `\music\Greatest Hits\01 Intro.flac`, Size: 1, IsAudio: true}, + }, + } + + release, err := lockSlskdFolders( + context.Background(), []string{filepath.Join(downloads, "Greatest Hits")}, + ) + if err != nil { + t.Fatalf("lock: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + if _, err := s.Grab(ctx, c, t.TempDir(), nil); !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("Grab = %v, want it to wait on the held folder", err) + } + + release() + + stub.mu.Lock() + posted := stub.posted + stub.mu.Unlock() + + if posted { + t.Error("enqueued transfers into a folder another grab held") + } +} diff --git a/backend/download/provider.go b/backend/download/provider.go index f748978..b2d9965 100644 --- a/backend/download/provider.go +++ b/backend/download/provider.go @@ -236,17 +236,18 @@ func Register(d Descriptor, c Constructor) { } // concurrencyField describes the per-provider transfer limit, with help -// text explaining why the default is what it is — a user who raises -// slskd from 1 to 8 and gets themselves queued behind every other -// Soulseek user deserves to have been warned. +// text explaining what the number means where it means something +// unusual: on slskd it counts peers, since each peer is only ever asked +// for one folder at a time whatever it is set to. func concurrencyField(k Kind) Field { help := "Maximum simultaneous transfers from this client." if k == KindSlskd { - help = "Maximum simultaneous transfers. Soulseek peers serve " + - "one file at a time and queue or ban clients that ask for " + - "more, so 1 is both the polite setting and usually the " + - "fastest." + help = "How many Soulseek users to download from at once. " + + "Each user is only ever asked for one album at a time, " + + "since peers queue or ban clients that ask for more; " + + "this bounds how many different users are asked in " + + "parallel." } return Field{ diff --git a/backend/download/provider_slskd.go b/backend/download/provider_slskd.go index 1286c72..51a6064 100644 --- a/backend/download/provider_slskd.go +++ b/backend/download/provider_slskd.go @@ -10,6 +10,7 @@ import ( "path" "path/filepath" "regexp" + "slices" "strings" "time" @@ -742,6 +743,12 @@ func (s *slskd) Grab( // first poll came back — an old failure failing a transfer that has // not started. So what is already terminal is noted before enqueueing // and ignored after. + release, err := lockSlskdFolders(ctx, s.localFolders(c)) + if err != nil { + return Result{}, err + } + defer release() + stale := s.terminalTransferIDs(ctx, username) wanted := make([]map[string]any, 0, len(c.Files)) @@ -767,6 +774,59 @@ func (s *slskd) Grab( return s.collect(c, dst) } +// slskdFolders serialises grabs that land in the same local folder. +// +// slskd names a download's directory after the remote *leaf* folder, so +// two different albums both shared as "Greatest Hits" — or any two +// multi-disc rips, whose leaves are "CD1" and "CD2" — are written into +// one directory, and collect finds files by name there. Run at once, +// a file one peer never sent is filled by the other peer's file of the +// same name. One grab per peer made that impossible; several peers at +// once makes it likely. It is package-level and keyed on the full +// path because two configured clients can share one daemon. +var slskdFolders keyedLock[string] + +// localFolders returns the directories under downloadsPath a candidate's +// files will be written to, sorted so every grab takes them in the same +// order and two cannot each hold what the other waits for. +func (s *slskd) localFolders(c Candidate) []string { + var out []string + + for _, f := range c.Files { + norm := strings.ReplaceAll(f.Path, `\`, "/") + out = append(out, filepath.Join(s.downloadsPath, path.Base(path.Dir(norm)))) + } + + slices.Sort(out) + + return slices.Compact(out) +} + +// lockSlskdFolders takes every folder in order, releasing what it holds +// if the context ends part way. +func lockSlskdFolders(ctx context.Context, folders []string) (func(), error) { + releases := make([]func(), 0, len(folders)) + + releaseAll := func() { + for _, r := range slices.Backward(releases) { + r() + } + } + + for _, f := range folders { + r, err := slskdFolders.acquire(ctx, f) + if err != nil { + releaseAll() + + return nil, err + } + + releases = append(releases, r) + } + + return releaseAll, nil +} + // slskdDownloadsPath is the transfers endpoint for one peer. Soulseek // usernames may contain spaces and punctuation, so the name is escaped // rather than spliced into the path. -- 2.54.0