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..d308b4a 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 @@ -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 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..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/.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/.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/progress-line.test.ts b/frontend/test/components/progress-line.test.ts index 5409ce3..e63a033 100644 --- a/frontend/test/components/progress-line.test.ts +++ b/frontend/test/components/progress-line.test.ts @@ -172,8 +172,7 @@ describe('', () => { * The reason this component asks `matchMedia` instead of letting a * stylesheet hide it: a media query cannot stop a 1 Hz interval * running for the life of every desktop session. That claim is - * load-bearing in CLAUDE.md, so it is asserted rather than - * described — the timer count, because a desktop render is empty + * load-bearing, so it is asserted rather than described — the timer count, because a desktop render is empty * either way and so cannot tell the two apart. */ it('runs no interpolation timer above the breakpoint', async () => { 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', { 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/main.go b/main.go index e853b3e..9ead817 100644 --- a/main.go +++ b/main.go @@ -64,8 +64,7 @@ var frontendDistAssets embed.FS // Returning early is not a degraded mode: `nativeInit` has already // re-attached the bridge, so the recreated activity's WebView talks to // the app that is still running, with its queue and its playback -// position intact. See CLAUDE.md, "An activity is a view onto the -// process". +// position intact. // // It is inert off Android, where a process has exactly one main(). var mainStarted atomic.Bool 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: 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