docs: record plan 007, its four audits, and what measuring corrected
`.planning/audits/2026-08-11-ui/` is the pass this work came from: the app driven by hand headless plus three static reviews, ~118 findings that are really five problems, each spread by being copied rather than fixed. `.planning/plans/active/007-ui-reconciliation.md` sequences them by blast radius and records what each of the six passes actually shipped — including twenty-five entries under "where the plan was wrong", which is the point of writing it down. The discipline those entries add up to, now in NOTES.md: a finding is three hypotheses — how big it is, why it is that big, and what to do about it — and they can be independently right and wrong. Three of the audit's recommended fixes would have shipped a bug (`m1` stops the card grids repainting, `m6`'s index-ordered selection goes stale on any re-sort, `m5`'s guard leaves the marquee short), all three because they reasoned from the shape of the code and not from what the rest of the file already knew about it. Five findings evaporated or inverted on contact. CLAUDE.md gains the invariants that came out of it, and the skill gains the fourteen measurement traps, each of which produced a wrong number first — the newest being that a longtask entry arrives after the task that produced it, so two numbers that must agree are worth more than one you have to be sceptical about.
This commit is contained in:
@@ -26,7 +26,11 @@ make dev-headless # Start headless in the background and return (SEED=<name>
|
||||
make dev-stop # Stop it (SIGTERM, so shutdown hooks run)
|
||||
make dev-logs # Tail .dev/app.log
|
||||
make testdata # Generate the deterministic fixture music library
|
||||
make bulkdata # Generate the ~50k-track measurement library (BULK_TRACKS=)
|
||||
make sandbox-seed NAME=<n> # Build a seeded YJ_HOME by *running* the app
|
||||
make sandbox-seed-bulk # Same, from the bulk library (minutes; it is a real scan)
|
||||
make perf LABEL=<n> # Measure a running app; writes .dev/perf/<n>.json
|
||||
make perf-compare BEFORE=<a> AFTER=<b> # Print the before/after table
|
||||
make build-dev # Debug build with symbols
|
||||
make build-prod # Production build (stripped, UPX-compressed)
|
||||
make generate # Run code generators (sqlc + templ via go generate)
|
||||
@@ -111,7 +115,21 @@ 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.
|
||||
|
||||
**The cheapest tier needs none of that.** `make ui-test` runs 313
|
||||
**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 480
|
||||
Vitest tests in a real Chromium in ~2 s with no Wails, no backend, no
|
||||
seeded library and no virtual display, because `frontend/wailsjs/` is a
|
||||
pure passthrough to `window.go` / `window.runtime` and
|
||||
@@ -179,6 +197,20 @@ See `.planning/plans/active/005-agent-development-harness.md`.
|
||||
same drift is possible again if a schema change ships without
|
||||
updating both files. Don't reintroduce a *second* description of
|
||||
the schema anywhere else.
|
||||
- **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 and the unit
|
||||
tests exercised a handle the app does not have.
|
||||
`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.
|
||||
- **Squashing is fine pre-1.0.** While this hasn't shipped to real
|
||||
users, periodically folding `sql/migrations/` into `sql/schemas/`
|
||||
and deleting the migration files (then wiping your own dev/sandbox
|
||||
@@ -187,6 +219,12 @@ See `.planning/plans/active/005-agent-development-harness.md`.
|
||||
it" workflow, opt-in instead of mandatory. Stop doing that once
|
||||
real user databases exist in the wild.
|
||||
- `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.
|
||||
- `config` — TOML-based settings. Settings page uses HTMX + templ for server-rendered HTML fragments.
|
||||
- `playlist` / `smartplaylist` — Playlist CRUD and rule-based smart playlists.
|
||||
- `mediacontrols` — MPRIS integration on Linux via D-Bus.
|
||||
@@ -224,6 +262,101 @@ work happens **once, centrally**, and users download the result:
|
||||
|
||||
**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 in `frontend/wailsjs/` — don't edit by hand.
|
||||
|
||||
**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 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 `<inline-notice region="…">` 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.
|
||||
|
||||
**One keyboard authority.** No component owns a document keydown
|
||||
listener for its own shortcuts; it registers *panel-scoped* bindings
|
||||
(`autotag.*`, `tracklist.*` in `backend/shortcuts/config.go`) and
|
||||
claims a scope by setting `shortcutScope` on the mixin, which publishes
|
||||
`data-shortcut-scope` and claims it as the ambient scope
|
||||
(`services/shortcut-scope.ts`) while it is on screen. The shortcut
|
||||
service resolves focus → panel → global, and yields a key to a focused
|
||||
control that owns it (button/select/slider/checkbox, or anything inside
|
||||
an open dialog) so the unmodified single-key global bindings do not
|
||||
steal Space and the arrows.
|
||||
|
||||
Two cross-cutting pieces of that UI are worth knowing before touching
|
||||
a list or a detail view:
|
||||
|
||||
@@ -244,8 +377,192 @@ a list or a detail view:
|
||||
since "something is renderable" and "this is the catalog's answer"
|
||||
are different questions.
|
||||
|
||||
**Icons are bundled, and the app works offline.** `src/icons/`
|
||||
overrides Web Awesome's `default` icon library, whose resolver fetches
|
||||
every `<wa-icon>` from `ka-f.fontawesome.com` at runtime — so the app
|
||||
had no icons at all offline, and `setBasePath()` does not affect it
|
||||
(only the component autoloader reads that). Overriding the library
|
||||
fixes all 165 call sites without changing one of them. Three things
|
||||
about it are load-bearing. The set is **Font Awesome Free** (CC BY 4.0,
|
||||
vendored with its licence by `frontend/scripts/fetch-icons.mjs`)
|
||||
because the kit CDN serves **Pro**, which cannot be redistributed. The
|
||||
names are a committed list (`src/icons/names.txt`) rather than anything
|
||||
derived, because twenty call sites compute their icon name from state
|
||||
and no static pass can enumerate them. And a name that is not bundled
|
||||
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.
|
||||
|
||||
**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
|
||||
`FilePath` off them — 5 genres cost 6 MB over the IPC (`perf.m2`).
|
||||
`GetFilePathsByAlbums(ids, libraryID)` and `GetFilePathsByGenres(names,
|
||||
libraryID)` answer in one query and carry only the paths. They return
|
||||
the paths **grouped by album id / genre name**, because the caller owns
|
||||
the order — an album list is sorted by name, not by id, and a flattened
|
||||
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.
|
||||
|
||||
**Event-driven communication**: Backend emits events via Wails runtime; frontend stores subscribe to them. Event names are constants in `backend/events/`.
|
||||
|
||||
`frontend/src/events.ts` is **generated** from `backend/events/events.go`
|
||||
by `backend/events/cmd/genevents` — never edit it. It renders a const
|
||||
block's doc comment as TypeScript line comments, every line of it: it
|
||||
used to prefix only the first, so a comment that ran to a second
|
||||
paragraph emitted bare prose into the object literal and `make generate`
|
||||
— a pre-commit hook — produced a file that does not parse.
|
||||
|
||||
**An event's cost is part of its meaning.** `TrackMetadataChanged`
|
||||
means *the tags on disk were rewritten*, which can change an album, an
|
||||
artist or a genre — so `library-store` answers it by discarding every
|
||||
cached collection and refetching. That makes it the most expensive
|
||||
event in the app, and it must not be reused for something cheap:
|
||||
finishing a track used to emit it, which cost ~37 MB across the IPC and
|
||||
~0.8 s of blocked main thread *per song* at 50 000 tracks, and cleared
|
||||
the user's track selection while it did. `TrackPlayCountChanged`
|
||||
carries `{audioFileId, filePath, playCount, lastPlayed}` — deliberately
|
||||
everything needed to patch one track in place, so no consumer has any
|
||||
reason to invalidate anything. The store replaces the tracks array
|
||||
(consumers key memoized filter/sort caches on its identity) while
|
||||
sharing every unchanged Track, and `track-list` **retains** its
|
||||
selection across a refetch rather than clearing it, since the keys are
|
||||
file paths and those survive one.
|
||||
|
||||
The same rule reaches the other way: **an event carries what a consumer
|
||||
needs so it never has to invalidate.** `PlaylistTracksChanged` carries
|
||||
the playlist id, and `playlist-store` refetches *that* playlist
|
||||
(`GetPlaylistTracks`, plus `GetAllPlaylists` for the summaries, since
|
||||
`UpdatedAt` is a sort key) rather than `GetAllPlaylistsWithTracks`,
|
||||
which is every row of every playlist — 2.6 MB and 172 ms for one heart
|
||||
before the fix. It falls back to a full invalidate only where a patch
|
||||
cannot be shown to be equivalent: no id (the bulk paths emit one), a
|
||||
cold cache, an unknown id, or a fetch already in flight. And **a store
|
||||
with no subscriber fetches nothing**: `playlist-view` is the only
|
||||
reader and is created lazily, so neither the invalidation nor — more
|
||||
expensively — the singleton's own construction warms a cache for a page
|
||||
that may never open.
|
||||
|
||||
**An unchanged payload is not an event.** `explore`'s index status used
|
||||
to be pushed on a 3 s ticker for the life of the process, byte-identical
|
||||
once the index was ready, and `config-page` assigns it to a `@state`
|
||||
field — so a user who had once opened Settings paid a full re-render of
|
||||
a 2 000-line template every 3 s, forever, for no news. `emitStatus` now
|
||||
drops a status equal to the last one it sent, which is the rule stated
|
||||
once instead of at twenty call sites. The corollary is load-bearing:
|
||||
**every mutation of something the status derives must call `emitStatus`
|
||||
itself**, because there is no longer a poll to notice it. `si.ready`
|
||||
and `si.cancel` are both derived, and both were relying on the ticker.
|
||||
|
||||
**A cache on a cached view needs a ceiling, and so does everything
|
||||
else holding what it holds.** `explore-view` never unmounts, and its
|
||||
two art caches were plain `Map`s: twenty-four searches retained
|
||||
20.58 MB and were still accelerating, because a cover thumbnail is a
|
||||
~27 kB base64 data URL and an artist photo is a ~128 kB one. They are
|
||||
`LRUMap`s now (`utils/lru-map.ts` — a `Map` re-inserted on read and
|
||||
trimmed from the front), capped from the measured size of an entry and
|
||||
kept several times larger than a screenful, since a cap below the
|
||||
visible count evicts art that is still rendered and the re-render
|
||||
fetches it straight back.
|
||||
|
||||
Two things about it are load-bearing. **A cap is only a bound if it
|
||||
covers every reference**: the artist photo's data URL is held by both
|
||||
`artistImageCache` and `exploreCache.artists`, so capping either alone
|
||||
frees nothing at all and reads as a fix that did not work —
|
||||
`ARTIST_IMAGE_CACHE_LIMIT` is exported and shared for that reason. And
|
||||
**a bound has to stay checkable**: caches register with
|
||||
`utils/cache-stats.ts`, so `window.__yjCacheStats()` reports entries,
|
||||
retained chars and cap in one eval, rather than the next session having
|
||||
to rebuild the twenty-four-search reproduction before it can tell
|
||||
whether the ceiling still holds.
|
||||
|
||||
**A list pays per row, and only while scrolling.** The track list's Art
|
||||
column rendered `CoverArtPath` — the original artwork — into a 24 px
|
||||
box while `CoverArtSmall` sat unused on the same model, and
|
||||
`artists-view` linear-scanned every cached album per card per frame to
|
||||
find an avatar fallback. Both are invisible to every test tier: nothing
|
||||
renders differently and nothing fails, the app is just slower to
|
||||
scroll. Pick the tier for the box you are drawing (`cover-grid`'s
|
||||
`getCoverUrl()` is the model), always `loading="lazy" decoding="async"`
|
||||
on a row image, and build a lookup keyed on the store array's identity
|
||||
rather than searching it — the store replaces that array when its
|
||||
contents change and shares the unchanged members, which is the same
|
||||
signal `track-list`'s memoized caches key on.
|
||||
|
||||
**The same rule, on the selection path, was the worst stall in the
|
||||
app.** Five components turned selected file paths back into tracks with
|
||||
`filePaths.map(fp => tracks.find(…))`, so "Select all → Edit tags" at
|
||||
50 000 tracks blocked the main thread for **three to six seconds**
|
||||
(`perf.m6`). `utils/track-index.ts` is that lookup written once: a
|
||||
`WeakMap` from the array's identity to a `Map<FilePath, Track>`, safe
|
||||
for exactly the reason above and collected for free when the store
|
||||
drops the array. **68 ms after.**
|
||||
|
||||
Its neighbour is a deliberate non-fix. `getSelectedKeysOrdered()` walks
|
||||
the *list* rather than the selection, which the same finding calls out —
|
||||
measured at **3 ms** for 50 000 items, so it stays a walk, with an early
|
||||
exit once everything is found (which helps a selection near the top of
|
||||
the list and, honestly, almost nothing at the bottom). The obvious fix —
|
||||
keep each key's index beside it — is the one thing that cannot be done
|
||||
here: an index goes stale on any re-sort, re-filter or refetch while a
|
||||
file path survives all three, which is precisely why `retain()` drops
|
||||
`lastSelectedIndex` and keeps the keys. Three milliseconds does not buy
|
||||
a silently mis-ordered queue insert.
|
||||
|
||||
**Work in `updated()` runs on every pass, so it has to say what it
|
||||
depends on.** `now-playing` measured and rewrote its text geometry
|
||||
every update — six `querySelector`s and a read/write interleave — and
|
||||
`player-store` notifies while playing, so it did that several times a
|
||||
second about a component whose DOM had not changed (`perf.m5`). It now
|
||||
runs only when its geometry key changes: the rendered title, the
|
||||
rendered artist, **the two scroll flags**, or the ResizeObserver
|
||||
reporting the panel resized. The scroll flags are in that list because
|
||||
`.will-scroll .scroll-content` carries `padding-right: 2em`, so
|
||||
applying the class changes the distance the marquee travels (−128 px
|
||||
before it, −158 px after) — a guard on the text alone leaves every
|
||||
first hover scrolling short, and nothing would fail. Reads before
|
||||
writes in one place, so the interleave cannot come back.
|
||||
|
||||
Two corollaries. **Cost is measured where the work runs, not where it
|
||||
is written**: those same reads cost 3 µs when the layout is clean and
|
||||
0.103 ms when it is dirty, so the finding's magnitude only appears if
|
||||
the DOM actually changed. And **a drag's document listeners belong to
|
||||
the drag** — `track-list` and `now-playing` attach `mousemove`/`mouseup`
|
||||
on `mousedown` and drop them on `mouseup` (and on disconnect, for a
|
||||
drag interrupted by the component going away).
|
||||
|
||||
**A virtualized list repaints only when you tell it to, and the
|
||||
accidental way you were telling it may be the thing you are about to
|
||||
delete.** `<lit-virtualizer>`'s rows are produced by the `virtualize`
|
||||
directive, which runs when one of the *virtualizer's own* properties
|
||||
changes — not when its parent re-renders. So a list with memoized
|
||||
`items` and a stable `renderItem` never repaints on host state, and
|
||||
selection silently stops highlighting while the controller holds
|
||||
exactly the right keys. Both playlist detail views virtualize now
|
||||
(`perf.M5`: 22 090 elements and 2 000 eager cover requests for a
|
||||
2 000-track playlist, against 487 and 0), and both therefore push
|
||||
`virtualizer.requestUpdate()` on a selection change and on a
|
||||
playing-track change, which is what `track-list` has always done.
|
||||
|
||||
The corollary is that **`artists-view` and `genres-view`'s per-render
|
||||
arrow functions are load-bearing.** `perf.m1` asks for them to be
|
||||
hoisted to stable fields the way `cover-grid` does; hoisting them is
|
||||
what *stops* the virtualizer seeing a changed property, so the cards
|
||||
keep whatever classes they had — measured at 1 highlighted card before
|
||||
and 0 after, with no compensating win to pay for it.
|
||||
`frontend/test/components/card-grid-repaint.test.ts` fails on that
|
||||
change and exists for no other reason.
|
||||
|
||||
Two smaller rules from the same pass. A row inside a virtualizer needs
|
||||
`width: 100%`, because the virtualizer positions its children
|
||||
absolutely and a grid row otherwise shrinks to fit its content and
|
||||
stops lining up with the header above it. And a panel that is closed
|
||||
renders no list at all (`perf.m7`): `width: 0` and `contain` bound the
|
||||
damage but do not stop a virtualizer inside from measuring its window
|
||||
on every change, or `scrollToIndex` from calling `scrollIntoView()` on
|
||||
something invisible.
|
||||
|
||||
Emit through **`events.Emit(ctx, name, data...)`**, never
|
||||
`runtime.EventsEmit` — wails `log.Fatalf`s (unrecoverably) on any
|
||||
context that does not carry its runtime, which includes every
|
||||
|
||||
Reference in New Issue
Block a user