docs: record phase 6, and the shelf that repeated with no ids in common
Twelve corrections to a plan written before any of phases 1-5 existed, of which the load-bearing one is that a rule written against a mechanism does not cover what the rule is for: `home` suppresses a repeated shelf by comparing album ids, Explore's first two shelves hold different entity types and share none, and the page repeated itself anyway because a person reads artists.
This commit is contained in:
@@ -1683,3 +1683,102 @@ 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.
|
||||
|
||||
## A shelf that shares no ids can still be the shelf above it
|
||||
|
||||
Plan 007 phase 6: the two inherited one-liners from the fourth pass,
|
||||
and then the only part of this plan that *adds* rather than repairs —
|
||||
Explore's shelves.
|
||||
|
||||
The generalisation: **a rule is written against a mechanism, and the
|
||||
thing it exists to prevent is not the mechanism.** `backend/home`
|
||||
suppresses a shelf that repeats the one above it, by comparing album
|
||||
ids. Explore's first two shelves hold *different entity types*, so
|
||||
their ids are disjoint by construction and no overlap is possible — I
|
||||
wrote that in a comment as the reason the guard was unnecessary, and it
|
||||
is true, and the page repeated itself anyway. Ordered by raw
|
||||
ListenBrainz listen count, the catalog's top twelve albums are seven
|
||||
records by one act and its members, and the artists row underneath is
|
||||
then the same seven people. One fandom, twice, with nothing in common
|
||||
by the only measure the rule knew how to take.
|
||||
|
||||
It was found by **reading the screenshot** — the shelves rendered, the
|
||||
counts were right, four component tests and six Go tests were green,
|
||||
and the page was obviously wrong to anyone looking at it. The fix is
|
||||
one album per artist (a shelf is a selection, not a leaderboard) and
|
||||
skipping whoever a row above already showed. The existing Go test
|
||||
caught the semantic change immediately: an artist seeded as the maker
|
||||
of the album shelf's only album correctly stopped appearing in the
|
||||
artists shelf, which read as a regression and was the rule working.
|
||||
|
||||
Nine more things worth keeping, and the first four are all one theme —
|
||||
**a plan's shelf list is a design; the schema is the constraint**:
|
||||
|
||||
- **Two of the four planned shelves cannot be built at all.**
|
||||
`explore_index` has no genre or tag column, so "big in a genre you
|
||||
already have depth in" has nothing to join to — genre exists only in
|
||||
the library's own `recording_genres`. And "artists next to ones you
|
||||
own" needs `similar_artist_map`, which `cmd/indexexport` does not
|
||||
ship and which is filled lazily by network calls from artist pages:
|
||||
empty on a fresh install, empty offline, which is exactly when this
|
||||
page most needs content. Both were answerable in ten minutes by
|
||||
reading two schema files, before writing anything.
|
||||
- **The third is empty on every library the repo can look at.** It
|
||||
reads `in_library`, set from MusicBrainz IDs, and the fixture library
|
||||
has **0 artists with an MBID** — so a shelf about the user's own
|
||||
music is correctly absent on the seed, in CI, and on any untagged
|
||||
library however large. A feature you cannot see locally has to be
|
||||
designed so that the state you *can* see is a legitimate one.
|
||||
- **"The shipped artifact already contains the answer" is false in the
|
||||
place the tests run.** `ci.yml` points `YJ_CORE_INDEX_URL` at a dead
|
||||
address, so CI's app has **0 catalog rows** — as does every user's
|
||||
first run. A developer machine silently downloads the real 1.1 M-row
|
||||
artifact at launch, which is why the local page looked finished. The
|
||||
empty world was reproduced locally by setting the same variable, and
|
||||
it is now the world the e2e spec stages a catalog into.
|
||||
- **A spec that skips is not a spec that passes.** The first version
|
||||
branched on the row count and skipped three of its four cases where
|
||||
there was no catalog — which is CI, i.e. the only place both browser
|
||||
engines run. Staging six rows through `/__test/sql` costs nothing
|
||||
and turns "skipped" into signal.
|
||||
- **…and it only works because the readiness gate is a question rather
|
||||
than a flag.** Two cached answers were tried first and were wrong in
|
||||
the same way. `GetIndexStatus().TotalRows` is refreshed between build
|
||||
tiers, so on an ordinary launch it reads 0 next to a full catalog and
|
||||
hid every shelf. `IsReady()` is set once at startup by counting, so
|
||||
rows staged afterwards are invisible to it. Both are the shape the
|
||||
`emitStatus` note warns about — a derived value with nothing polling
|
||||
behind it. `SELECT 1 FROM explore_index LIMIT 1` cannot be stale and
|
||||
costs nothing.
|
||||
- **A setup step whose failure is not checked is not setup.** The
|
||||
staging fetch passed six values to seven placeholders and never read
|
||||
the response: every insert failed, the table stayed empty, and the
|
||||
helper looked exactly like a helper that had worked. Same family as
|
||||
every "probe that cannot move" in this plan, on the *arrange* side
|
||||
rather than the assert side.
|
||||
- **A reproduction of a fix at one scale is not one at another.** End
|
||||
in a split grid worked on the eight-album fixture, because everything
|
||||
is rendered and the scroll is a no-op. At 5 000 albums the fix moved
|
||||
the index and focused nothing: the card arrives a few hundred ms
|
||||
after the host's `updateComplete`, and a ten-*frame* retry budget
|
||||
expired first. The retry is a time budget now. (And a probe that
|
||||
scrolled the grid to demonstrate the *old* behaviour left it
|
||||
somewhere that broke the next measurement in the same eval —
|
||||
measuring the before can spoil the after.)
|
||||
- **The audit did not contain the biggest bug this pass fixed.** "Check
|
||||
what Home/End mean across a split grid" was a one-line hunch from the
|
||||
previous session. What it found is that `offsetTop` inside a
|
||||
`lit-virtualizer` is always 0 — the children are positioned by
|
||||
transform — so ArrowDown and ArrowUp have been End and Home in the
|
||||
albums, artists *and* genres grids since the roving controller was
|
||||
written. Reproduced at 700×700 with three real rows: ArrowDown from
|
||||
card 0 landed on card 7. One `getBoundingClientRect` fixed all three.
|
||||
- **A label can promise what the control cannot do.**
|
||||
`library-status-indicator` was recorded last pass as a button that
|
||||
does nothing. Its *label* was also an offer — "Add artist “Eno” to
|
||||
library" — from an element that cannot accept it. Making it a badge
|
||||
meant changing the copy too, which is the part a mechanical fix would
|
||||
have left saying the wrong thing. Also worth knowing: a `<span>` does
|
||||
not inherit `box-sizing: border-box` from the UA stylesheet the way a
|
||||
`<button>` does, so swapping the tag grew the badge 36→38px. Nothing
|
||||
but the stored screenshot would have noticed.
|
||||
|
||||
@@ -2135,6 +2135,129 @@ a search returns to the shelves.
|
||||
Anything requiring a network call on page load. The point is that the
|
||||
shipped artifact already contains the answer.
|
||||
|
||||
### Phase 6 — what actually shipped
|
||||
|
||||
The two inherited one-liners from Phase 5's fourth pass, and then the
|
||||
phase itself. Three landings.
|
||||
|
||||
- **`library-status-indicator` is a badge.** It was a `<button>` whose
|
||||
click handler was a `stopPropagation()` and a comment. Measured in
|
||||
the running app on an Explore results page: **66 tab stops, 20 of
|
||||
them inert** → **46 and 0**. It is `role="img"` with its existing
|
||||
label, and the unowned label says "… is not in your library" rather
|
||||
than "Add … to library", which was the button's promise written out.
|
||||
- **The card grids move by a row.** Reproduced first, then fixed: at
|
||||
700×700 with three real rows of 3/3/2, ArrowDown from card 0 landed
|
||||
on card **7**.
|
||||
- **`H-23` — Explore opens with shelves.** Three of them, on `home`'s
|
||||
terms, over `explore_index`; two of the plan's four could not be
|
||||
built at all. Plus an honest page for the no-catalog case, which is
|
||||
what CI and every first run actually have.
|
||||
|
||||
Pinned by `roving-grid.test.ts` (6), `explore-shelves.test.ts` (7),
|
||||
`shelves_test.go` (8), and `e2e/specs/explore-shelves.spec.ts` (4) plus
|
||||
one case added to `album-actions.spec.ts`. `make ui-test` 545 → **558**;
|
||||
`make e2e` 62 → **68**.
|
||||
|
||||
#### Where the plan was wrong — Phase 6
|
||||
|
||||
Twelve things. This phase's plan text was the least tested material in
|
||||
the repo — written before any of Phases 1–5 existed — and it shows
|
||||
most in the shelf list, where **half the named shelves are not
|
||||
buildable against the schema they were specified over**:
|
||||
|
||||
- **"Big in a genre you already have depth in" cannot be built.**
|
||||
`explore_index` has no genre or tag column; genre exists only in the
|
||||
library's own `recording_genres`. There is nothing to join. Dropped,
|
||||
not deferred — building it means changing the dump pipeline.
|
||||
- **"Artists next to ones you own" cannot be built offline.** It needs
|
||||
`similar_artist_map`, which `cmd/indexexport` does not ship (the
|
||||
artifact carries `explore_index` and its metadata, nothing else) and
|
||||
which is filled lazily by ListenBrainz calls from artist pages. It is
|
||||
empty on a fresh install and empty offline — exactly when this page
|
||||
most needs something to show. The plan's own "not in this phase"
|
||||
rules it out in the same breath as naming it.
|
||||
- **"You own one album by this artist" is empty on every untagged
|
||||
library**, including the fixture one. Ownership is `in_library`, set
|
||||
by MusicBrainz ID; the seed has **0 artists with an MBID**, so the
|
||||
shelf is correctly absent everywhere it could be looked at locally.
|
||||
- **The plan says the queries return MBIDs. They return row ids.**
|
||||
`rowsByIDs` is keyed on the primary key and preserves the order it is
|
||||
given, which is what lets the ordering stay in SQL. MBIDs would mean
|
||||
a second lookup for nothing.
|
||||
- **The card projection existed but not as a function.** "One
|
||||
definition of an Explore card" was three inline struct literals
|
||||
inside `mergeIndexHits`, tangled with search scoring. Extracting them
|
||||
is what made the claim true rather than aspirational — and `Score` is
|
||||
deliberately *not* part of them: it is a property of a search, and a
|
||||
shelf has no query to be relevant to.
|
||||
- **"A shelf with nothing behind it is omitted" is the wrong rule for
|
||||
this page.** On Home an omitted shelf means a library with no
|
||||
history, which is honest. Explore's data is a *downloaded artifact*,
|
||||
so an empty page can mean it has not arrived — and rendering nothing
|
||||
is the blank panel the phase exists to remove. The page carries a
|
||||
`state` (`ready` / `building` / `no-index`) and says which.
|
||||
- **The premise "the shipped artifact already contains the answer" is
|
||||
false in CI and on every first run.** `ci.yml` points
|
||||
`YJ_CORE_INDEX_URL` at a dead address, so the e2e job has **0**
|
||||
catalog rows. The first version of the spec skipped three of its four
|
||||
cases there, which is no signal at all; it stages its own small
|
||||
catalog through `/__test/sql` instead, verified by reproducing the
|
||||
empty world locally with the same environment variable.
|
||||
- **…which only works because the readiness gate is a question, not a
|
||||
flag.** Two cached answers were tried and both were wrong in the same
|
||||
way. `GetIndexStatus().TotalRows` is refreshed only between build
|
||||
tiers, so on an ordinary launch it reads 0 beside a full catalog and
|
||||
hid every shelf. `IsReady()` is set once at startup, so rows staged
|
||||
afterwards are invisible. One `SELECT 1 … LIMIT 1` cannot be stale.
|
||||
Both are the shape `emitStatus` warns about — a derived value with
|
||||
nothing polling behind it.
|
||||
- **Two shelves with disjoint ids still repeated each other.** Ordered
|
||||
by raw listen count, the catalog's top albums are seven records by
|
||||
one act and its members, and the artists row underneath was the same
|
||||
seven people. `home`'s adjacent-duplicate guard cannot see it — the
|
||||
rows hold different entity types, so no two share an id, which the
|
||||
code comment cited as proof the guard was unnecessary. **Found in a
|
||||
screenshot, by reading it.** The fix is one album per artist, and
|
||||
skipping artists a row above already showed.
|
||||
- **The e2e staging step staged nothing, and looked like it worked.**
|
||||
Six values against seven placeholders, and the response was never
|
||||
read. A setup whose failure is not checked is not setup.
|
||||
- **`library-status-indicator`'s label was only right for one of its
|
||||
three states.** "Add artist “Eno” to library" is an offer, from an
|
||||
element that cannot accept it.
|
||||
- **A `<button>` and a `<span>` are not the same box.** Dropping the
|
||||
button grew the badge 36px → 38px, because the UA stylesheet gives a
|
||||
button `box-sizing: border-box` and a span nothing. Caught by a
|
||||
stored screenshot, which is the only thing that would have.
|
||||
|
||||
And one about a probe, in this plan's longest-running family: **an e2e
|
||||
spec that reads the DOM immediately after a navigation reads it before
|
||||
the fetch it triggered.** `shelfHeadings()` returned `[]`, which is
|
||||
also what a broken page returns; it passed on the second run of the
|
||||
same build because the caches were warm. The wait belongs in
|
||||
`beforeEach`, so no test can start from a page that has not answered.
|
||||
|
||||
#### Not done, and still worth doing (after Phase 6)
|
||||
|
||||
- **The albums row still leads with one act.** One-per-artist fixed the
|
||||
literal repetition; it cannot know that eight artists are one group
|
||||
and its solo members, and nothing in `explore_index` expresses that.
|
||||
A "related act" notion would need dump-side data.
|
||||
- **The unowned badge still draws a `+`.** It is no longer a control
|
||||
and no longer says "Add", but a plus glyph is an affordance. Left
|
||||
alone deliberately: it becomes correct again the day the badge
|
||||
becomes a button, and changing it touches four components' visual
|
||||
baselines for a judgement call that is better made then.
|
||||
- **No `make perf` before/after.** Both seeds' catalogs come from the
|
||||
artifact rather than from the seed tarball, so a before and an after
|
||||
are not measuring the same corpus unless the staging fixture is
|
||||
extended to bulk scale. The shelves are three indexed queries behind
|
||||
a view activation, not a startup-path cost, so this is a want rather
|
||||
than a gap — but it is not measured, and is recorded as such.
|
||||
- **`tracklist.delete`**, still inherited, still needs a "remove from
|
||||
library" that does not exist.
|
||||
|
||||
---
|
||||
|
||||
## Decisions
|
||||
|
||||
@@ -231,6 +231,41 @@ See `.planning/plans/active/005-agent-development-harness.md`.
|
||||
- `mediacontrols` — MPRIS integration on Linux via D-Bus.
|
||||
- `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
|
||||
@@ -585,6 +620,39 @@ result would silently reorder a queue — and because `cover-grid`'s drag
|
||||
cache stores them per album. A `libraryID` of 0 means "every library",
|
||||
matching an unset library filter.
|
||||
|
||||
**A badge is not a button, and a control that cannot act is worse than
|
||||
none.** `library-status-indicator` — the tick/plus on every Explore
|
||||
card and track row — was a `<button>` whose click handler was a
|
||||
`stopPropagation()` and a comment saying to wire up the download client
|
||||
later: 20 of the 66 tab stops on a results page announced themselves as
|
||||
buttons and did nothing (46 and 0 after). It is `role="img"` with a
|
||||
label until there is something to click, and its unowned label says
|
||||
"… is not in your library" rather than "Add … to library", which was
|
||||
the button's promise written into the copy. When the download client
|
||||
lands, the change is a `<button>` *with* a handler — not a handler
|
||||
bolted onto something already shaped like one. Two smaller things came
|
||||
with it: a `<span>` does not get `box-sizing: border-box` from the UA
|
||||
stylesheet the way a `<button>` does (the badge grew 36→38px, caught by
|
||||
a stored screenshot), and with no click of its own the badge is part of
|
||||
its card, so a click on it means what the card means.
|
||||
|
||||
**A grid moves by a row, and `offsetTop` cannot tell you how wide a row
|
||||
is.** `utils/roving-grid.ts` measured columns by counting cards sharing
|
||||
an `offsetTop`, and every card in these grids is positioned by
|
||||
`lit-virtualizer` with a **transform**, which `offsetTop` does not see —
|
||||
so all of them reported 0, every rendered card counted as one row, and
|
||||
ArrowDown was `min(i + everything, last)` while ArrowUp was
|
||||
`max(i - everything, 0)`. The vertical arrows were End and Home in the
|
||||
albums, artists and genres grids alike, from the day it was written.
|
||||
`getBoundingClientRect().top`, rounded, is the measurement. Two things
|
||||
behind it are only visible once `cover-grid` splits: `scrollToIndex`
|
||||
must pick the half that holds the index and rebase it (it was
|
||||
`querySelector('lit-virtualizer')`, always the first), and the focus is
|
||||
retried on a **time** budget rather than taken once at `updateComplete`
|
||||
— a scroll of 5 000 rows produces the card a few hundred ms later, so
|
||||
the tab stop moved and nothing took focus, which is indistinguishable
|
||||
from the key not being handled.
|
||||
|
||||
**A view says what it is, in one component.** `<page-header>`
|
||||
(`components/page-header/`) is title, count, sort and actions, and the
|
||||
nine primary views use it rather than each writing its own — which is
|
||||
|
||||
Reference in New Issue
Block a user