feat(library): remove a track from the library without deleting the file
RemoveFromLibrary deletes the audio_files rows the way the scan's own orphan cleanup does and records each path in excluded_paths. The exclusion is not an enhancement: without it the next scan finds the file, sees no row and imports it again, so the button undoes itself. The soft scan compares files on disk against rows in the database, so surveyAudioFiles and countAudioFiles both take the exclusion set — otherwise an excluded path makes the two disagree forever and queues a full scan on every launch. Deleting a row cascades to queue_tracks, so the removal calls the same CompactQueue hook RemoveLibrary does. Also lands the requested badge: library-status-indicator is a button again where it can act, utils/library-status.ts states once what owning and wanting mean, and the long-declared queued state finally has a producer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UDCbcCZQepnpSQYJ6SxxZm
This commit is contained in:
+186
-11
@@ -1,6 +1,7 @@
|
||||
# 008 — The last audit, and the one binding that outlived six phases
|
||||
|
||||
**Status:** active — Phases 1 and 2 shipped.
|
||||
**Status:** complete — all four phases shipped. `a11y.md` is closed,
|
||||
and with it all four audits from 2026-08-11.
|
||||
**Branch:** main
|
||||
**Created:** 2026-08-12
|
||||
**Follows:** 007-ui-reconciliation
|
||||
@@ -64,17 +65,20 @@ fixed until it has been reproduced in the running app.
|
||||
| `15` | Major | `now-playing` is not among the four files carrying `prefers-reduced-motion`. WCAG 2.2.2: moving content over 5 s with no pause mechanism. |
|
||||
| `14` | Major | `combobox.ts` has no `aria-controls`, no `aria-activedescendant`, no option ids. |
|
||||
| `11` | Major | No `altKey` handler in `queue-panel`. The *other* half of this finding — "no keyboard path to add a track to the queue or a playlist" — was closed by Phase 5's `MenuKeyboard`. |
|
||||
| `21` | Minor | `body { height: 100vh; overflow: hidden }` unchanged. WCAG 1.4.10. |
|
||||
| `22` | Minor | `queue-panel` gained `aria-current`; `track-list` did not, and neither has a non-colour marker. |
|
||||
| `24` | Minor | No `title` on the truncating element in `track-info`, `playlist-view`, `queue-panel` or `track-list`. |
|
||||
| `25` | Minor | `<wa-progress-bar value=…>` with no label, verbatim as filed. |
|
||||
| — | new | **Two unnamed native `<select>`s**, one of them `page-header`'s sort control on nine views. Not in the audit: `a11y.6` scanned `<button>`. Found in the AX tree while reproducing `14`. Belongs with `26`. |
|
||||
| `21` | Minor | `body { height: 100vh; overflow: hidden }` unchanged. WCAG 1.4.10. **Shipped — and the stated mechanism was wrong; the failure is horizontal.** |
|
||||
| `22` | Minor | `queue-panel` gained `aria-current`; `track-list` did not, and neither has a non-colour marker. **Shipped.** |
|
||||
| `24` | Minor | No `title` on the truncating element in `track-info`, `playlist-view`, `queue-panel` or `track-list`. **Shipped.** |
|
||||
| `25` | Minor | `<wa-progress-bar value=…>` with no label, verbatim as filed. **Shipped — it was named "Progress", not unnamed.** |
|
||||
| — | ~~new~~ | ~~**Two unnamed native `<select>`s**, one of them `page-header`'s sort control on nine views.~~ **False.** The sort control is named "Sort: " by its wrapping `<label>` on all nine. The two unnamed roles were **one** `config-field` select and the **seek bar**. See Phase 3's list. |
|
||||
| — | new | **24 of 93 controls on Settings unnamed** — every `config-field` select and toggle, all eighteen column checkboxes. **Shipped, 0 of 93.** |
|
||||
| — | new | **Both `wa-slider`s have no accessible name**, which `a11y.md` files under *what is already correct*. **Shipped.** |
|
||||
| `26` | Minor | Explore's search box is named by its placeholder only — which *is* an accname fallback, so an AX sweep reports it clean. `search-bar` was already fixed. **Shipped.** |
|
||||
| `28` | ~~dropped~~ | **Measured, stays dropped.** One header *label* clips at 800×600; zero data cells do. |
|
||||
| `29` | Polish | `<h3 class="subtitle">` for type size. |
|
||||
| `30` | Polish | No skip link anywhere. |
|
||||
| `32` | Polish | `title="Remove from queue"`, not identifying the track. |
|
||||
| `34` | Polish | The 10 px sort arrow, unchanged. |
|
||||
| — | — | Colour contrast, never measured. |
|
||||
| `29` | Polish | `<h3 class="subtitle">` for type size. **Shipped.** |
|
||||
| `30` | Polish | No skip link anywhere. **Shipped.** |
|
||||
| `32` | Polish | `title="Remove from queue"`, not identifying the track. **Shipped.** |
|
||||
| `34` | Polish | The 10 px sort arrow, unchanged. **Shipped — half of it was closed by Phase 1's `aria-sort`.** |
|
||||
| — | — | Colour contrast, never measured. **Measured and fixed in Phase 2.** |
|
||||
|
||||
## Ordering principle
|
||||
|
||||
@@ -400,6 +404,106 @@ Two of them are not one-liners and should be treated as such:
|
||||
- **`22`** asks for a non-colour marker on the playing row, which is a
|
||||
visual change to the densest list in the app and moves a baseline.
|
||||
|
||||
### Phase 3 — what actually shipped
|
||||
|
||||
Six landings rather than one, ordered by risk, each reproduced in the
|
||||
running app before anything was written and each watched failing on the
|
||||
pre-fix build.
|
||||
|
||||
- **Web Awesome's two hidden roles.** `label` on both `wa-slider`s and
|
||||
on `wa-progress-bar` (`25`), plus `styles/wa-slider-label.css.ts`,
|
||||
which hides the slider's visible label by part and puts back the 8px
|
||||
margin `#slider` takes as soon as one exists.
|
||||
- **Settings' form controls.** `for`/`id` in `config-field`,
|
||||
`aria-label` on the eighteen column toggles and thirty-six column
|
||||
arrows, and the action's name on every `shortcut-capture`.
|
||||
**24 unnamed of 93 → 0.**
|
||||
- **`24` and `32`.** `title` on the four clipping surfaces, on the
|
||||
track-list *cell* rather than on what is inside it; and a queue row's
|
||||
remove button named after its own track.
|
||||
- **`29`, `30`, `34`.** A skip link, `<h3>` → `<p>`, and the sort arrow
|
||||
at the type scale's floor. Plus the state that landed in: the hgroup
|
||||
measured 67px in a 64px bar and the subtitle's descenders were
|
||||
clipped once the h3's bottom margin went with it.
|
||||
- **`22`.** A triangle in each row's own left padding, in both lists,
|
||||
and `aria-current` on the track-list row.
|
||||
- **`21`.** `overflow-x: auto` — measured, and the finding's stated
|
||||
mechanism is not the one that exists.
|
||||
|
||||
And `26`'s remaining half, found last: Explore's search box.
|
||||
|
||||
Pinned by `wa-control-names.test.ts` (4), `settings-names.test.ts`
|
||||
(11), `aria-tail.test.ts` (+5), `queue-reorder.test.ts` (+3),
|
||||
`e2e/specs/control-names.spec.ts` (3), `e2e/specs/skip-link.spec.ts`
|
||||
(4), `e2e/specs/layout-overflow.spec.ts` (+6) and
|
||||
`e2e/specs/playback.spec.ts` (+1). `make ui-test` 649 → **672**;
|
||||
`make e2e` 74 → **88**.
|
||||
|
||||
#### Where the plan was wrong — Phase 3
|
||||
|
||||
Ten things. The first four are the audit or the plan being wrong about
|
||||
where a control's name lives.
|
||||
|
||||
- **The two unnamed `<select>`s from Phase 1 were one `<select>` and a
|
||||
slider, and neither was the page header's.** `page-header`'s sort
|
||||
control computes "Sort: " from its wrapping `<label>`, on every one
|
||||
of the nine views — checked with `getFullAXTree`, `from:
|
||||
relatedElement`. The other unnamed role was the **seek bar**, which
|
||||
`a11y.md` lists under *what is already correct*. Fourth probe error
|
||||
in two passes, and the same shape as the rest: read at the wrong
|
||||
level.
|
||||
- **`aria-label` on a Web Awesome host does not name the control.**
|
||||
`wa-slider` puts `role="slider"` on a div in its own shadow root
|
||||
pointing `aria-labelledby` at an empty internal `<label>`, and that
|
||||
IDREF outranks the host's `aria-label`. Both sliders computed `""`.
|
||||
Exactly `wa-dialog`'s trap one component over, and the audit made
|
||||
exactly the same mistake in the opposite direction — it read the
|
||||
source and credited a name that was never computed.
|
||||
`volume-control` did not even have the `aria-label` it is credited
|
||||
with.
|
||||
- **`a11y.25` is not "unnamed".** `wa-progress-bar` falls back to the
|
||||
localised word *progress*, so it announced "Progress, 45%" — named
|
||||
after the widget rather than after the work. Same fix, smaller claim.
|
||||
- **Settings was full of unnamed controls and no finding says so.** 24
|
||||
of 93. `a11y.6` is not wrong: it says in its own line that it scanned
|
||||
every `<button>`. Third time this pass that a count in the audit was
|
||||
answering a narrower question than it reads as.
|
||||
- **A placeholder is an accessible name.** Explore's search box
|
||||
therefore reported *clean* in an AX sweep of all eleven views, which
|
||||
is why `a11y.26` outlived four phases of people looking for exactly
|
||||
this. A sweep for empty names cannot see a weak one.
|
||||
- **`a11y.21`'s mechanism does not exist.** "The 4em bars grow while
|
||||
the viewport does not, and anything that no longer fits is clipped
|
||||
with no scrollbar" — the middle row is `1fr` and absorbs them
|
||||
exactly. At 200% text on 800×600 the bars go 64 → 128 and the panel
|
||||
472 → 344, footer still on 600. The real failure is horizontal, which
|
||||
the finding does not mention: 784px of app in a 320px viewport, 464px
|
||||
of it unreachable.
|
||||
- **…and the obvious probe for it passes on the broken build.**
|
||||
`overflow: hidden` still permits *programmatic* scrolling, so
|
||||
`scrollLeft = 9999` returns a healthy number on the build with the
|
||||
bug. It did. The spec is a wheel gesture now.
|
||||
- **A fix's own test was pinning the bug.** `transport.test.ts`
|
||||
asserted `aria-label` on the `wa-slider` host and called it "carries
|
||||
an accessible name". Running the existing suite is what found it,
|
||||
for the third plan running.
|
||||
- **`a11y.34` was half closed by Phase 1 and nobody had noticed.** "The
|
||||
sort direction is a 10px glyph *or nothing*" — it is announced now,
|
||||
via the `aria-sort` Phase 1 added. What was left is one declaration.
|
||||
- **The queue's `aria-current` is dead in the common path.** A track
|
||||
started from the *track list* leaves the queue's `currentIndex` at
|
||||
−1, so the panel has no current row at all — which is why `22`'s
|
||||
marker looked broken the first time it was checked in the running
|
||||
app. Pre-existing, not fixed here, and the reason the e2e case plays
|
||||
from the queue.
|
||||
|
||||
And one that is about the harness rather than the audit: **a synthetic
|
||||
`MouseEvent` does not reach a delegated handler the way a real gesture
|
||||
does.** Three probes in a row reported the queue row as never becoming
|
||||
active; `page.getByTestId('queue-row').dblclick()` made it active
|
||||
immediately. Same family as everything above — the probe was wrong, not
|
||||
the code.
|
||||
|
||||
---
|
||||
|
||||
## Phase 4 — `tracklist.delete`, and the operation behind it
|
||||
@@ -457,6 +561,77 @@ A Go test that a removed path survives a rescan; an e2e case that the
|
||||
row is gone, the dialog said so, and the file still exists. Both halves
|
||||
matter — the second is the promise the copy makes.
|
||||
|
||||
### Phase 4 — what actually shipped
|
||||
|
||||
Three landings, in the order the plan proposed, each watched failing on
|
||||
the pre-fix build by neutering one line rather than stashing.
|
||||
|
||||
- **The schema, `RemoveFromLibrary`, and the scanner honouring the
|
||||
list.** `excluded_paths` (one file — see below), rows deleted the way
|
||||
the scan's own orphan cleanup deletes them, `TracksRemovedFromLibrary`
|
||||
carrying `{filePaths, count}`, and both of the scanner's walks taking
|
||||
the exclusion set.
|
||||
- **The context-menu command**, behind `confirmAction()` with an impact
|
||||
line that says the files are not deleted, plus `library-store`
|
||||
splicing rather than invalidating.
|
||||
- **`tracklist.delete`**, bound to opening that dialog, and the e2e
|
||||
case.
|
||||
|
||||
Pinned by `remove_tracks_test.go` (6), `library-store.test.ts` (+4),
|
||||
`keyboard-shortcuts.test.ts` (+1) and
|
||||
`e2e/specs/remove-from-library.spec.ts` (2). `make ui-test` 672 →
|
||||
**677**; `make e2e` 88 → **90**.
|
||||
|
||||
#### Where the plan was wrong — Phase 4
|
||||
|
||||
Six things, and the first two are the plan asking for work that does
|
||||
not exist and skipping work that does.
|
||||
|
||||
- **"Following the two-file schema discipline" is wrong for a new
|
||||
table.** `applySchema` runs every file in `sql/schemas/` on every
|
||||
open, so a `CREATE TABLE IF NOT EXISTS` reaches an existing install
|
||||
verbatim; the migration file the plan asked for would have been a
|
||||
*second description of the same table*, which is the one thing the
|
||||
checklist's third rule forbids. Column order and "no index on a
|
||||
migrated column" do not apply either — nothing is being added to an
|
||||
existing table, so the index lives beside its own `CREATE TABLE`.
|
||||
- **The half that would have undone the feature is not in the plan.**
|
||||
The startup soft scan decides "library unchanged" by comparing files
|
||||
on disk against rows in the database. An excluded path is on disk and
|
||||
deliberately not a row, so the two counts disagree *forever* and
|
||||
every launch queues a full scan of the whole library. Both walks take
|
||||
the exclusion set now. Nothing in any tier would have caught it: it
|
||||
is not a wrong answer, it is a permanent, invisible re-scan.
|
||||
- **…and neither is the queue.** Deleting an `audio_files` row cascades
|
||||
to `queue_tracks`, so the queue's in-memory copy — and possibly the
|
||||
playing track — goes stale. `RemoveLibrary` has had the
|
||||
`CompactQueue` hook for exactly this since it was written; the
|
||||
removal reuses it.
|
||||
- **The plan says nothing about undo, and the operation needs one.** An
|
||||
exclusion with no UI to clear it is a one-way door: the file is on
|
||||
disk and the user cannot get it back. A full rescan clears the table,
|
||||
which is the escape hatch until there is a list to manage. Recorded
|
||||
rather than implied, because it is the difference between
|
||||
"reversible" (shape A's stated advantage) and a claim.
|
||||
- **A new table has a second gate nobody remembers.**
|
||||
`backend/datamap` catalogues every table's Kind and Lifetime, and two
|
||||
of its tests fail on a new one: `TestCatalogCoversSchema` for the
|
||||
missing entry, then `TestAuthoredCascadesAreDeliberate` because an
|
||||
*authored* table that cascades needs an argued exemption. Both are
|
||||
right to ask; neither is mentioned in `references/schema-change.md`.
|
||||
- **The copy was wrong in the first screenshot, and only there.** The
|
||||
title was singular and the body said "**They** are removed" — the
|
||||
message and impact strings were written for the multi-select case and
|
||||
used for both. Nothing failed. Found by reading the PNG, which is now
|
||||
the sixth regression in four plans that only a PNG has caught.
|
||||
|
||||
And one about the harness rather than the work: **a hook gets 30
|
||||
seconds, not the test's timeout.** The e2e case's `afterAll` restore
|
||||
passed in isolation and timed out in the full suite, where earlier
|
||||
specs have staged an explore catalog and the restore takes longer than
|
||||
the hook's default budget. `test.setTimeout()` inside the hook is what
|
||||
raises it.
|
||||
|
||||
---
|
||||
|
||||
## Deliberately not in this plan
|
||||
@@ -0,0 +1,300 @@
|
||||
# 009 — The badge that cannot act, and the state it already had
|
||||
|
||||
**Status:** complete — all three phases shipped.
|
||||
**Branch:** main
|
||||
**Created:** 2026-08-13
|
||||
**Follows:** 008-the-last-audit
|
||||
|
||||
## Problem
|
||||
|
||||
007 phase 6 turned `library-status-indicator` from a `<button>` that did
|
||||
nothing into a `role="img"` badge, on the rule that **a control which
|
||||
cannot act is worse than none**, and wrote down what would change the
|
||||
answer: *"when the download-client integration lands, the right change
|
||||
is to make it a `<button>` again with a handler."*
|
||||
|
||||
Two things about that are wrong, and both were found by reading the code
|
||||
and then the running app rather than the note.
|
||||
|
||||
**The download client has largely already landed.** `backend/download`
|
||||
is 16 541 lines: a durable request model with four entity types
|
||||
(`artist` / `release-group` / `release` / `recording`, `request.go`), a
|
||||
reconciler, a staging importer, six provider adapters, 20 bound methods,
|
||||
`downloads-view`, the `download-picker` dialog, and a working **"Want
|
||||
this"** toggle on `explore-album-details`. What has not landed is the
|
||||
badge.
|
||||
|
||||
**And the badge is not merely inert — it is wrong.** `LibraryStatus`
|
||||
declares, styles and labels a third state, `queued` ("… is queued for
|
||||
download"). **Zero of the eight call sites ever produce it**
|
||||
(`explore-view:1839,1877`, `explore-artist-details:2116,2228,2323`,
|
||||
`explore-album-details:1641,2283`, `top-results-row:258` — every one is
|
||||
a two-way ternary). So an album the user has *already requested*
|
||||
displays a plus and says it is not in their library.
|
||||
|
||||
### Reproduced, 2026-08-13, before anything was written
|
||||
|
||||
Against `SEED=default` with the real 900 000-row catalog:
|
||||
`AddRequest({mbid: e51c54ea…, entity: 'release-group'})` for *GOLDEN* by
|
||||
Jung Kook, then Explore → search "GOLDEN":
|
||||
|
||||
```
|
||||
status not-in-library
|
||||
icon plus
|
||||
aria Album "GOLDEN" is not in your library
|
||||
```
|
||||
|
||||
and on the album's **own detail page**, forty pixels apart in the same
|
||||
screenshot: the button reads **"Wanted"** (filled) and the badge beside
|
||||
the title reads **plus / "is not in your library"**. One component,
|
||||
two surfaces, opposite answers. This is the header-badge-contradicting-
|
||||
Settings failure again, and again only a PNG showed it.
|
||||
|
||||
The same PNG showed a second one, which is why it is in this plan:
|
||||
**`bookmark-check` is not a bundled icon.** `window.__yjIconMisses`
|
||||
reports exactly `["bookmark-check"]`, so the "Wanted" button renders the
|
||||
fallback question-mark glyph. `e2e/specs/offline-icons.spec.ts` asserts
|
||||
that array is empty and passes, because no spec has ever put the app in
|
||||
a state where an album is requested — precisely the "twenty call sites
|
||||
compute their icon name from state" case `names.txt` exists for.
|
||||
|
||||
## Ordering principle
|
||||
|
||||
By **what is a fact and what is a decision**.
|
||||
|
||||
Phase 1 is a bug: the badge contradicts the app's own state, and fixing
|
||||
it needs no interaction design at all. It also produces the evidence
|
||||
Phase 2 needs — once the badge can say "requested", whether it must also
|
||||
*become* requestable is a question that can be looked at rather than
|
||||
assumed.
|
||||
|
||||
Phase 2 is a decision made before any code, in the shape 008 phase 4
|
||||
used, because one 20 px circle would otherwise mean three different
|
||||
commitments: on an artist card a **discography subscription**
|
||||
(`scope: 'future'`, `Expands()`, never satisfied), on an album a
|
||||
release-group request, on a track row a recording request.
|
||||
|
||||
Phase 3 is whatever Phase 2 leaves. **"Album only" is a legitimate
|
||||
outcome** and shrinks this plan rather than inventing work for it.
|
||||
|
||||
---
|
||||
|
||||
## Phase 1 — the badge tells the truth
|
||||
|
||||
**Ships:**
|
||||
|
||||
- `utils/library-status.ts` — one definition of the rule, since the
|
||||
reason all eight sites are two-state is that the rule is written at
|
||||
all eight. Owning something outranks wanting it, so `in-library` wins
|
||||
over `queued`.
|
||||
- The eight call sites using it.
|
||||
- `explore-view` gaining the `downloadStore` subscription both detail
|
||||
views already have (`init()` + `subscribe()`), through
|
||||
`view-lifecycle` — it is a **cached primary view**, so a raw
|
||||
`connectedCallback` subscription would live for the session.
|
||||
- `bookmark-check` in `src/icons/names.txt`, and an e2e case that
|
||||
reaches the state that exposes it.
|
||||
|
||||
**The badge stays `role="img"`.** Telling the truth is not acting.
|
||||
|
||||
**Watch for:** `downloadStore.init()` fetches providers, descriptors,
|
||||
downloads *and* requests, so this warms a singleton on a page that
|
||||
previously did not construct it — "a store with no subscriber fetches
|
||||
nothing" cuts the other way here, and the cost belongs in the note.
|
||||
|
||||
### Phase 1 — what actually shipped
|
||||
|
||||
Three landings. `make ui-test` 677 → **685**; `make e2e` 90 → **92**.
|
||||
|
||||
- **The rule, written once.** `utils/library-status.ts`, the eight call
|
||||
sites, and `explore-view`'s subscription.
|
||||
- **The Pro icon.** `regular/bookmark` / `solid/bookmark`, vendored.
|
||||
- **`e2e/specs/requested-badge.spec.ts`**, which is also the first spec
|
||||
that reaches the state the icon sweep needed.
|
||||
|
||||
Pinned by `library-status.test.ts` (8) and `requested-badge.spec.ts`
|
||||
(2). Both e2e cases were watched failing on the pre-fix build by
|
||||
neutering one line each — the badge reported `not-in-library` where
|
||||
`queued` was expected, and the sweep returned `["bookmark-check"]`.
|
||||
|
||||
#### Where the plan was wrong — Phase 1
|
||||
|
||||
Six things, and the first is the plan's own framing.
|
||||
|
||||
- **"When the download client lands" had already half happened, and
|
||||
the note that said otherwise was written before it.** 007 phase 6
|
||||
left a condition ("make it a button *with* a handler") that reads as
|
||||
future work; `backend/download` was 16 541 lines and 20 bound methods
|
||||
at the time it was written. The badge was not waiting on the download
|
||||
client. It was waiting on somebody looking.
|
||||
- **The bug was one layer below the one in the plan.** The plan says
|
||||
the badge cannot act. What the reproduction says is that it could not
|
||||
even *report* — three states declared, two produced, at eight sites
|
||||
none of which knew about the third. "A control that cannot act" and
|
||||
"a control that is wrong" are different faults and only the second
|
||||
one is a lie.
|
||||
- **The second bug was in the screenshot of the first.** The "Wanted"
|
||||
button rendered a question mark, which is the missing-icon fallback:
|
||||
`bookmark-check` is a **Pro** name. It has been that way for as long
|
||||
as anything could be requested, and `offline-icons.spec.ts` — which
|
||||
exists to assert exactly this — passed throughout, because it never
|
||||
reached a state where an album was requested. Seventh regression in
|
||||
five plans that only a PNG has caught, and the first one caught in a
|
||||
PNG taken of a *different* bug.
|
||||
- **A sibling component does not hear its host re-render.**
|
||||
`top-results-row` takes `results` as a property; `explore-view`
|
||||
re-rendering hands back the same array, so Lit stops at the property
|
||||
and the row keeps its old badges. Same shape as the virtualizer rule
|
||||
one level milder, and the fix is the same: subscribe where the state
|
||||
is read.
|
||||
- **The cleanup ran on a page that could not run it.** `afterAll` used
|
||||
`callBinding`, which goes through `window.__yjEvents` — installed by
|
||||
the `app` fixture and not by `browser.newPage()`. It threw where
|
||||
nothing was watching, left the request behind, and failed the *next*
|
||||
run of the same spec with a stale `queued`. A spec that gives state
|
||||
back has to be checked by running it twice, which is what found this.
|
||||
- **A freshly launched app cannot search its own catalog for ~40 s.**
|
||||
The core artifact merge (`core artifact: merge complete` in
|
||||
`.dev/app.log`) has to land first, and until it does Explore's search
|
||||
returns nothing — *including for rows staged directly into
|
||||
`explore_index` a moment earlier*, which is what makes it look like a
|
||||
staging bug. It cost a cycle here reading as a failure of the neuter
|
||||
it was run under.
|
||||
|
||||
---
|
||||
|
||||
## Phase 2 — what a badge click means, per entity
|
||||
|
||||
*(Decided 2026-08-13, before any code.)*
|
||||
|
||||
**A badge is a button where it is the only way to act, and what it
|
||||
toggles is a request — never a download.**
|
||||
|
||||
Two of the three questions were answered by the code rather than by a
|
||||
judgement, which is the point of asking them before writing anything.
|
||||
|
||||
**There is no artist badge, and there never was.** The worry that one
|
||||
20 px circle would commit a user to a whole discography does not apply:
|
||||
`top-results-row` renders `nothing` for an artist, and no other site
|
||||
passes `entity-type="artist"` to this component at all. Artist
|
||||
subscription already has a home — `explore-artist-details`'s
|
||||
`renderFollowAction()`, a labelled button with the scope beside it,
|
||||
which is where a commitment that never completes belongs.
|
||||
|
||||
**A track badge is honoured end to end.** `EntityRecording` is not a
|
||||
placeholder in the request model: `Reconciler.tracklistFor` has a
|
||||
deliberate branch for it ("A track request is its own tracklist") whose
|
||||
comment explains that the single expected title is what lets filename
|
||||
matching score a one-song download at all. So a track badge promises
|
||||
something the backend can keep, and it is a button too.
|
||||
|
||||
That also disposes of the second observation. An hourglass on an album
|
||||
over a row of plusses read as noise while a plus meant nothing; once a
|
||||
plus on a track means *want just this one*, the mixed row is the
|
||||
interface working. No special case, and none of the four surfaces needs
|
||||
to know what contains what.
|
||||
|
||||
**The album detail header keeps its badge read-only.** "Want this" sits
|
||||
directly below it saying the same thing in words. The rule is not "a
|
||||
badge is decorative on detail pages" — it is that a call site **opts in
|
||||
by supplying the MBID to act on**, so a redundancy is visible in the
|
||||
template rather than hidden in the component.
|
||||
|
||||
**And it is a request, not an acquisition.** The old copy said "Add …
|
||||
to library", which 007 called the button's promise written into the
|
||||
copy — and it would still be a lie, because clicking adds a row to the
|
||||
request list and nothing to the library. The name is the action, in the
|
||||
words the rest of the app already uses: **"Want …"**, and **"Cancel the
|
||||
request for …"** when it is already wanted. No confirmation: the action
|
||||
is one click to undo, which is the whole test for whether a dialog is
|
||||
owed.
|
||||
|
||||
---
|
||||
|
||||
## Phase 3 — the button
|
||||
|
||||
Ships what Phase 2 decided: `request-mbid` as the opt-in, a `<button>`
|
||||
where a call site passes one and the entity is not already owned, and
|
||||
`toggleRequest()` beside `libraryStatusFor()` because
|
||||
`explore-album-details`'s "Want this" asks the same question and two
|
||||
implementations of *what wanting something means* is what Phase 1 was
|
||||
about.
|
||||
|
||||
### Phase 3 — what actually shipped
|
||||
|
||||
Seven of the eight call sites opt in; the album header does not.
|
||||
`make ui-test` 685 → **695**; `make e2e` 92 → **93**.
|
||||
|
||||
Verified in the running app with a **real mouse gesture and a real
|
||||
keyboard path**, not a synthetic event: click the badge → the request
|
||||
is filed, the badge becomes an hourglass, the album page does not
|
||||
open. Tab → the badge takes focus with its own ring inside the card's;
|
||||
Enter → same, and the card's own Enter handler does not fire.
|
||||
|
||||
Pinned by `library-status.test.ts` (+10, watched failing on the
|
||||
pre-fix build — 8 of 18) and `requested-badge.spec.ts` (+1).
|
||||
|
||||
#### Where the plan was wrong — Phase 3
|
||||
|
||||
Five things, and the first two are the plan asking questions the code
|
||||
had already answered.
|
||||
|
||||
- **Two thirds of the Phase 2 decision was not a decision.** "An artist
|
||||
badge would mean a discography subscription" describes a badge that
|
||||
does not exist — `top-results-row` renders `nothing` for an artist
|
||||
and no other site passes `entity-type="artist"` at all. And "should a
|
||||
track inside a requested album show something different" evaporated
|
||||
the moment a plus on a track meant *want just this one*. A decision
|
||||
phase is worth having; two of its three items were answered by
|
||||
reading rather than by choosing, which is the cheaper half of it
|
||||
working.
|
||||
- **`EntityRecording` is load-bearing and reads like a placeholder.**
|
||||
It would have been easy to rule tracks out as unsupported; the
|
||||
reconciler has an explicit branch for them whose comment explains
|
||||
that a one-entry expected tracklist is what lets filename matching
|
||||
score a single-track download at all. Ruling it out would have been a
|
||||
feature removed by assumption.
|
||||
- **A test that passes on the neutered build is not a test.** "Keeps
|
||||
its click off the card it sits on" asserted that nothing bubbled —
|
||||
which is free when there is no button to click, since `?.click()` on
|
||||
null is a silent no-op. It passed on the neutered build. It asserts
|
||||
the click *did the thing it was swallowed for* as well now, and fails
|
||||
there like the other seven.
|
||||
- **A measured coordinate is stale before it is used.** The e2e gesture
|
||||
read a bounding box the moment the search settled; cover art is still
|
||||
arriving then, and a card that grows moves the badge, so the click
|
||||
landed on the card and opened the album — reported as a failure to
|
||||
file a request, which is a different bug entirely. A locator
|
||||
re-resolves and waits for the element to stop moving.
|
||||
- **A fix moves its own assertions.** Phase 1's spec asserted the
|
||||
badge's name was "… is queued for download"; a control is named after
|
||||
what activating it does, so it is "Cancel the request for …" now. The
|
||||
spec was right when it was written and wrong two commits later, which
|
||||
is the ordinary cost of naming a thing after its state.
|
||||
|
||||
---
|
||||
|
||||
## Deliberately not in this plan
|
||||
|
||||
- **Deleting the file from disk** (008 phase 4's explicit sequel). Not
|
||||
refused — mis-ordered. 008's own notes record that the *reversible*
|
||||
option shipped with **nothing implementing its reversibility**:
|
||||
`excluded_paths` has no management surface, and "a full rescan clears
|
||||
it" is the escape hatch. Shipping an irreversible delete beside a
|
||||
reversible one that cannot yet be undone is backwards, and the
|
||||
platform trash is a new cross-platform dependency besides.
|
||||
- **`a11y.20`, deriving `_itemSize` from a measured row.** Real and
|
||||
confirmed in code — `.track-row` is `height: 33px; contain: strict`
|
||||
with a `rem` font size, so text scales and the box does not, across
|
||||
four lists (33 / 49 / 45 / 45 px). It waits because its only honest
|
||||
verification does not exist yet: both surviving comments
|
||||
(`track-list.ts:349`, `queue-panel.ts:179`) say a wrong `_itemSize`
|
||||
desynchronises the **native scrollbar at 20k+ rows**, and `make perf`
|
||||
has no scroll-fidelity row. That measurement is its own first phase
|
||||
and belongs to a plan that is about it.
|
||||
|
||||
## First step
|
||||
|
||||
Phase 1, and within it the helper rather than the call sites — the
|
||||
reproduction above is already the failing case, and the point of the
|
||||
helper is that there is one place for the next state to be added.
|
||||
Reference in New Issue
Block a user