docs: record the fourth pass, and the bugs hiding behind three findings

Each of perf.p2, H-13 and the dialog naming had a second defect behind the one named, reachable only once the first fix made the code path run. Also records two probe failures worth more than the fixes: the a11y snapshot cannot see a dialog's accessible name at all, and a scroll assertion that could not fail was hiding both a bug and a false claim.
2026-08-12 15:29:59 -04:00
parent f854076d95
commit f5621bf7c5
3 changed files with 252 additions and 0 deletions
@@ -1583,3 +1583,103 @@ Five things worth keeping:
  written and had been carried forward through every subsequent read of
  that file, including two this session. A measurement in a comment
  needs the same expiry as one in a plan.
 ## A finding creates the conditions for the next one
 Plan 007 phase 5, fourth pass: naming every `wa-dialog`, drawing
 `cover-grid`'s album dropdown, and giving the album page a primary
 action.
 The generalisation this pass adds: **a finding is a door, and what is
 behind it has never been looked at.** Three of the four items here had
 a second bug hiding *behind* the one the audit named, and none of them
 could have been found by reading — each was only reachable once the
 first fix made the code path run for the first time.
 - `perf.p2` files `renderSplitGrid` as "dead code carried in the
  bundle", housekeeping, delete it. It was a **missing feature** whose
  data path already worked, and behind it sat 1 463 lines that had
  never executed: `scroll-manager.ts` (916) and `album-dropdown.ts`
  (461). Drawing it revealed that **the albums grid could not scroll
  at all** — `.grid-scroll-container` is the markup `artists-view`
  uses and `cover-grid` had the class with no rule for it, so 186 984
  px of albums sat in a 772 px box at 5 000 albums, unreachable by
  wheel, keyboard or scrollbar — which in turn revealed that the
  scroll manager had spent its whole life saving and restoring a
  `scrollTop` that was permanently 0. And *that* revealed the shared
  context menu labelled "Album actions" on a track row, which nothing
  could observe while the only menu that could open on a track was
  unreachable.
 - `H-13` asks for a Play button. The button needs file paths; the
  obvious key is `MBTrack.LocalID`, which is declared, is in the
  generated bindings, and **is never written by anything in the
  backend**. Ownership is decided by recording MBID. Keying on that
  produced a button that was wired, labelled correctly, clicked
  cleanly and **queued nothing** — a library-only album has no
  recording MBIDs, because its tracks are synthesised with
  `mbid: RecordingMBID || ''`. Every component test passed. It was
  caught by clicking the button in the running app and reading the
  queue.
 Seven more things worth keeping:
 - **A count of call sites in an audit is a count as of its date.**
  `a11y.md` lists five `wa-dialog`s; last pass estimated eight; there
  are eleven. Three of the six added since were added by *this plan*.
  The fix was written as a helper called from each host rather than a
  list, so the number stopped mattering — which is the right response
  to a number that drifts.
 - **Reaching into another library's shadow root is acceptable when the
  failure is bounded.** `name-dialog.ts` queries Web Awesome's open
  shadow root, which is not API. If the structure moves, the query
  misses, nothing is written, and the dialog is exactly as unnamed as
  it was — no state to get wrong, nothing to throw. The alternative,
  patching `WaDialog.prototype`, fixes every call site for free and
  fails loudly and strangely instead. The bound is the argument, not
  the tidiness.
 - **`aria-labelledby` beats `aria-label` when the label can change.**
  Three call sites compute their label at render time. An IDREF to the
  heading the component re-renders anyway stays correct with nothing
  resyncing it, so the helper can be a one-shot call.
 - **A web component's shadow root is populated in its own update, not
  its host's.** Naming a `wa-dialog` from the host's `firstUpdated`
  finds an element with an empty shadow root and names nothing. Same
  lifecycle trap as `wa-dropdown-item`'s role two passes ago, which
  suggests it is not a trap so much as a rule: **never query inside a
  child custom element without awaiting its `updateComplete`.**
 - **The a11y snapshot cannot see a dialog's accessible name.**
  `playwright-cli snapshot` prints a bare `- dialog [ref=…]` whether
  the dialog is named by `aria-labelledby`, by `aria-label`, or not at
  all — checked all three ways against the running app. Twenty minutes
  went into "the fix did not work" before the probe was suspected.
  `getByRole('dialog', {name})` and CDP's
  `Accessibility.getFullAXTree` both answer, and CDP additionally
  reports *where* the name came from. The e2e spec was watched failing
  on a probe-disabled build before it was believed, which is the only
  reason the twenty minutes did not become an hour.
 - **An assertion that cannot fail will pass a bug, and the fix is to
  make the probe move.** "The scroll position is preserved when the
  dropdown opens" passed against a `scrollTop` of 0 both times on an
  eight-album fixture. Shrinking the viewport until the grid genuinely
  scrolled turned it red — and the red was **correct**:
  `scrollToShowDropdown` deliberately moves the scroll to reveal the
  dropdown (80 → 4, with the content *taller* after, so not clamping).
  The premise was wrong, not the app. Two lessons in one: a vacuous
  assertion hides a bug *and* a false claim, and the way to tell them
  apart is to make the number move before deciding what it means.
 - **A parameter name is not a specification.** `Queue.SetQueue`'s
  `shuffleStart` does not start a shuffle: it picks a random first
  track *if shuffle mode is already on*. A Shuffle button written from
  the name plays track 1 and looks broken. Reading the Go was thirty
  seconds.
 And one on the audit's own accounting: **`H-13`'s "unexplained ✓
 badges" was half-aged before it was read.** The indicator has carried a
 `title` and an `aria-label` reading "Album “X” is in your library" all
 along, so a hover and a screen reader were both already answered; what
 was missing was a key for a sighted user scanning a column of green
 circles. The same element turns out to be a `<button>` whose click
 handler is a comment saying "wire this up later" and a
 `stopPropagation` — 30-odd keyboard stops per page that promise an
 action and perform none. Not fixed here, and recorded rather than
 implied.
@@ -1945,6 +1945,129 @@ is**: a step plays three seconds and fails if they take under two,
 because otherwise the failure surfaces three steps later as "the
 elapsed clock is 19 s adrift" and reads as an app bug.
 ---
 ### Phase 5 — the fourth pass: the dialogs get names, and the album page gets an action
 The last of item 4, in three independently landable pieces. Each was
 reproduced in the running app before it was fixed, and two of the three
 changed shape when it was.
 - **Every `wa-dialog` has an accessible name.** Eleven call sites (not
  the eight the last pass estimated, and not the five `a11y.md` lists —
  six have been added since it was written), one helper,
  `utils/name-dialog.ts`. It points the native `<dialog>` at the `<h2
  id="title">` Web Awesome renders `label` into and never links,
  falling back to `aria-label` under `without-header`. Verified through
  CDP's `Accessibility.getFullAXTree`, which reports the name and that
  it came from `relatedElement`.
 - **`perf.p2` — the album dropdown is drawn.** `renderSplitGrid` was
  not dead code but a missing feature whose data path already worked:
  Enter on an album card fetched the tracks and ran the whole split
  state machine, and `render()` ignored `splitMode`. It is the only
  route from the albums grid to `track-details`, since a plain click
  navigates to the catalog page.
 - **`H-13` — the album page has a primary action.** Play / Shuffle
  album / Add to queue on `explore-album-details`, labelled by how much
  of the release the user owns, plus the legend the ✓ badges never had.
  Backed by a new `GetFilePathsByRecordingMBIDs` binding, the third
  member of the `GetFilePathsBy…` family.
 #### Where the plan was wrong — the fourth pass
 Nine things, and three of them are findings that had a second bug
 hiding behind them:
 - **`perf.p2` is filed as housekeeping and is a two-bug feature.**
  "Dead code carried in the bundle" is `renderSplitGrid` plus
  `scroll-manager.ts` (916 lines) and `album-dropdown.ts` (461) — 1 463
  lines that had never executed. Enabling the render exposed both of
  the others.
 - **The albums grid could not scroll at all.**
  `.grid-scroll-container` is the same markup `artists-view` and
  `genres-view` use, and `cover-grid` had the class with **no rule for
  it**, so the container grew to its full content height inside an
  `overflow: hidden` host: 186 984 px of albums in a 772 px box at
  5 000 albums, unreachable by wheel, keyboard or scrollbar. Invisible
  on the eight-album fixture, which is why nothing had ever caught it —
  and it is the element the scroll manager saves and restores, so that
  machinery had been aiming at a `scrollTop` that was permanently 0.
  With a real scroller it works as designed (2891 preserved exactly
  across an expand at 5 000 albums).
 - **The shared context menu was labelled "Album actions"
  unconditionally** — including on a track row, which nothing could
  observe while the only menu that could open on one was unreachable. A
  finding creates the conditions for the next one.
 - **`H-13`'s "unexplained ✓ badges" has half aged.**
  `library-status-indicator` carries a `title` *and* an `aria-label`
  reading "Album “X” is in your library", so a hover and a screen
  reader both get a full sentence. What was missing was a key for a
  sighted user scanning a column of green circles. Also worth knowing:
  it is a `<button>` whose click handler is a comment saying "wire this
  up later" and a `stopPropagation` — a badge in a button's clothes.
 - **`albumLibraryStatus()` is four claims OR'd into one tick**, the
  weakest of which fires when *one* recording of a forty-track release
  matches. Right for a badge, useless for a button — which is why the
  header counts the tracklist instead of reading the status.
 - **The obvious key for "play what I own" does not exist.**
  `MBTrack.LocalID` is declared, is in the generated bindings, and
  **nothing in the backend ever writes it**. Ownership is decided by
  recording MBID (`markReleasesInLibrary` → `CheckMBIDs`), so that is
  what the new binding is keyed on.
 - **…and keying on it alone shipped a Play button that queued
  nothing.** A library-only album has no recording MBIDs at all — its
  tracks are synthesised with `mbid: RecordingMBID || ''` — so on the
  fixture library the button was wired, labelled correctly, clicked
  cleanly and queued 0 tracks. Every component test passed. Caught by
  clicking it in the running app and reading the queue.
 - **`shuffleStart` does not start a shuffle.** `Queue.SetQueue`'s third
  argument picks a random first track *when shuffle mode is already
  on*, so a Shuffle button has to set the mode first. Reading the Go
  rather than the parameter name was the difference between a working
  button and one that plays track 1.
 - **"Shuffle" is still two controls with one name.** The first pass hit
  this on Home; the album header would have hit it again, since the
  transport's shuffle mode is on screen whenever this page is. It is
  "Shuffle album".
 And two about the probes rather than the findings:
 - **The a11y snapshot cannot see a dialog's name.**
  `playwright-cli snapshot` prints `- dialog [ref=…]` whether the
  dialog is named by `aria-labelledby`, by `aria-label`, or not at all
  — checked all three ways. A snapshot read as the oracle here reports
  failure on a working build. `getByRole('dialog', {name})` and CDP
  both answer correctly, and the e2e spec was watched failing on a
  probe-disabled build before it was believed.
 - **An e2e assertion about scroll position was vacuous and said so
  under pressure.** "Wherever the scroll was, it stays" passed against
  a `scrollTop` of 0 both times on an eight-album fixture. Shrinking
  the viewport until the grid actually scrolled turned it red — and
  the red was *correct*: `scrollToShowDropdown` deliberately moves the
  scroll to reveal the dropdown (80 → 4, with the content taller
  after, so not clamping). The premise was wrong, not the app; the
  assertion is now "the dropdown is on screen".
 #### Not done, and still worth doing (after the fourth pass)
 Item 4 is complete. What remains from the inherited list is
 `tracklist.delete`, which still needs a "remove from library" that does
 not exist and a decision about what it removes.
 Two things this pass found and did not fix:
 - **`library-status-indicator` is a button that does nothing.** Every
  tick and every "add to library" affordance in Explore is a `<button>`
  whose handler stops propagation and returns. It should be a
  non-interactive `role="img"` with its existing label until the
  download-client integration it is waiting for exists — as written it
  is a keyboard stop that promises an action on 30-odd elements per
  page.
 - **The split grid's roving tab stop was not re-examined.** The
  dropdown path renders two virtualizers where there was one;
  `roving-grid` is attached to the scroll container and keeps working,
  but nobody has checked what Home/End mean across a split.
 ## Phase 6 — Explore starts the conversation
 The only phase that adds rather than repairs.
@@ -544,6 +544,35 @@ is therefore **reported at runtime** to `window.__yjIconMisses` and
 drawn as a fallback — an e2e sweep asserts there are none — since a
 missing icon used to be impossible, the CDN having had everything.
 **An album page says how much of the album is yours.**
 `explore-album-details` is a *catalog* page and there is no
 library-side album detail page at all, so the album on it may be
 wholly the user's, partly theirs, or not theirs — and its primary
 action has to mean the same thing in each case. It says which: **Play**
 when the whole release is owned, **Play 7 of 12** when some of it is,
 and **no play button at all** when none is, because a Play button that
 plays nothing (or seven tracks of forty) is worse than none.
 `albumLibraryStatus()` is deliberately *not* what decides that. It is
 four claims of decreasing confidence OR'd into one tick — a local album
 id, the backend's cross-reference, a cached MBID match, and finally
 *any single track* marked `inLibrary` — which is a fine answer to "is
 any of this mine" and a useless basis for a button. `ownership()`
 counts the displayed tracklist instead, whose `inLibrary` flags the
 backend sets per recording MBID.
 **And the key it plays by is not the key it looks owned by.** The local
 album id is used wherever there is one, because a library-only album
 has *no* recording MBIDs — its tracks are synthesised from
 `GetAlbumTracks` with `mbid: RecordingMBID || ''` — so an MBID-keyed
 lookup on an untagged library resolves to nothing and Play queues
 nothing while looking entirely correct.
 `GetFilePathsByRecordingMBIDs` is the catalog-only fallback and the
 third member of the `GetFilePathsBy…` family: one query, paths only,
 grouped so the caller keeps the tracklist's order. It exists rather
 than a lookup by track id because **`MBTrack.LocalID` is declared and
 nothing in the backend ever writes it**.
 **Ask for what the caller uses, once.** "Play this artist" resolved
 file paths with one `GetAlbumTracks` per album, sequentially, and every
 one of the four sites doing that asked for whole track rows to read