docs: record phase 4, and the counts a new guard has to agree with
Plan 008 is complete and moves to completed/. The two findings worth carrying forward are that a new table needs one schema file rather than two (and a datamap entry, which is a gate nobody remembers), and that excluding a path has to reach every place that counts what is in the library — the soft scan's disk-vs-database comparison above all, which would otherwise have rescanned the whole library on every launch with nothing failing anywhere.
This commit is contained in:
@@ -5,7 +5,21 @@ chain got wrong, and when squashing is legitimate — is in `CLAUDE.md`
|
|||||||
under *Backend packages → database*. Read it once. This is the
|
under *Backend packages → database*. Read it once. This is the
|
||||||
checklist.
|
checklist.
|
||||||
|
|
||||||
A schema change needs **two** files, not one:
|
**A brand-new table needs one file, not two.** The rule below is about
|
||||||
|
a *column added to a table that already exists*. `applySchema` runs
|
||||||
|
every file in `sql/schemas/` on every open, so a
|
||||||
|
`CREATE TABLE IF NOT EXISTS` reaches an existing install verbatim and a
|
||||||
|
migration for it would be a second description of the same table — the
|
||||||
|
thing the third rule forbids. Its indexes go in the schema file too,
|
||||||
|
because the column and the index arrive together.
|
||||||
|
|
||||||
|
A new table has a second gate: **`backend/datamap`**. Add an entry
|
||||||
|
stating its Kind and Lifetime, or `TestCatalogCoversSchema` fails — and
|
||||||
|
if it is `Authored` and cascades, `TestAuthoredCascadesAreDeliberate`
|
||||||
|
wants an explicit exemption with a note, because authored data is what
|
||||||
|
a user cannot get back.
|
||||||
|
|
||||||
|
Adding a **column** to an existing table needs **two** files, not one:
|
||||||
|
|
||||||
1. **`backend/database/sql/schemas/*.sql`** — `CREATE TABLE ... IF NOT
|
1. **`backend/database/sql/schemas/*.sql`** — `CREATE TABLE ... IF NOT
|
||||||
EXISTS`, the literal target shape, what sqlc reads and what a fresh
|
EXISTS`, the literal target shape, what sqlc reads and what a fresh
|
||||||
|
|||||||
@@ -2129,3 +2129,70 @@ the flow moves every cell on the playing row and nothing else. A
|
|||||||
`::before` triangle in the 8px left padding costs no layout, and both
|
`::before` triangle in the 8px left padding costs no layout, and both
|
||||||
tiers assert it is *absent* on the other rows — a marker that renders
|
tiers assert it is *absent* on the other rows — a marker that renders
|
||||||
everywhere satisfies "the playing row has one" for free.
|
everywhere satisfies "the playing row has one" for free.
|
||||||
|
|
||||||
|
## A guard is only a feature if everything that counts agrees with it
|
||||||
|
|
||||||
|
Plan 008 phase 4: "remove from library" — the row goes, the path is
|
||||||
|
excluded from future scans, the file is never touched — and with it
|
||||||
|
`tracklist.delete`, which had been advertised in Settings for six
|
||||||
|
phases with nothing on the other end of it.
|
||||||
|
|
||||||
|
The generalisation: **an operation that changes what counts as "in the
|
||||||
|
library" has to be applied everywhere that number is computed, and the
|
||||||
|
places that compute it do not look like the feature.** The scan walk is
|
||||||
|
the obvious one, and skipping an excluded path there is the whole
|
||||||
|
feature as written in the plan. But the *startup soft scan* decides
|
||||||
|
whether to scan at all by comparing files on disk against rows in the
|
||||||
|
database, and an excluded path is on disk and deliberately not a row —
|
||||||
|
so the fix as specified would have left the two counts disagreeing
|
||||||
|
forever and queued a full scan of the entire library on **every
|
||||||
|
launch**. Nothing fails, nothing renders differently, and no tier looks
|
||||||
|
at it; the app is just permanently rescanning. The same shape one step
|
||||||
|
over: deleting an `audio_files` row cascades to `queue_tracks`, so the
|
||||||
|
queue's in-memory copy — and the playing track — goes stale unless the
|
||||||
|
removal calls the reload hook `RemoveLibrary` has had all along.
|
||||||
|
|
||||||
|
Six more things worth keeping:
|
||||||
|
|
||||||
|
- **A new table needs one schema file, and the two-file discipline is
|
||||||
|
not about it.** `applySchema` runs every file in `sql/schemas/` on
|
||||||
|
every open, so `CREATE TABLE IF NOT EXISTS` reaches an existing
|
||||||
|
install verbatim. The migration the plan asked for would have been a
|
||||||
|
*second description of the same table*, which is precisely what tore
|
||||||
|
out the old 48-step chain. Column order and "no index on a migrated
|
||||||
|
column" are rules about `ALTER TABLE ADD COLUMN`, and neither applies
|
||||||
|
when nothing is being altered.
|
||||||
|
- **The repo asked the question the plan did not.** `backend/datamap`
|
||||||
|
failed the build twice for the new table: once for having no entry at
|
||||||
|
all, then again because an *authored* table that cascades needs an
|
||||||
|
argued exemption rather than a default. Two gates, both right, and
|
||||||
|
neither in `references/schema-change.md` until now. A catalogue that
|
||||||
|
fails the build is worth more than a catalogue that is accurate.
|
||||||
|
- **Reversibility is a claim until something implements it.** The
|
||||||
|
decision picked shape A over deleting the file partly because it is
|
||||||
|
reversible — and nothing in the plan made it so. An exclusion with no
|
||||||
|
UI to clear it is a one-way door with the file sitting on disk the
|
||||||
|
whole time. A full rescan clears the table, which is the escape hatch
|
||||||
|
until there is a list to manage, and it is now written down instead
|
||||||
|
of assumed.
|
||||||
|
- **The copy was wrong for the case it will be used in most.** The
|
||||||
|
confirmation's message and impact were written for a multi-select and
|
||||||
|
used for both, so removing one track said "**They** are removed" under
|
||||||
|
a singular title. Nothing failed. Read in the first screenshot of the
|
||||||
|
dialog — sixth regression in four plans that only a PNG has caught,
|
||||||
|
and the one where it mattered most, since the copy is the only thing
|
||||||
|
standing between this feature and a user's music.
|
||||||
|
- **Both halves of a guard need their own test, or one of them is
|
||||||
|
decorative.** The walk's exclusion and the survey's exclusion are two
|
||||||
|
lines in two functions; neutering each in turn failed exactly one
|
||||||
|
test. Had they shared a test, either could have rotted invisibly.
|
||||||
|
Same reason the e2e case asserts a *control* path still returns from
|
||||||
|
the same scan: a guard that excluded everything passes "the removed
|
||||||
|
path did not come back" for free.
|
||||||
|
- **A Playwright hook gets 30 seconds regardless of the test's
|
||||||
|
timeout.** A `db/restore` in `afterAll` passed in isolation and timed
|
||||||
|
out in the full suite, where earlier specs have staged an explore
|
||||||
|
catalog and the copy takes longer. `test.setTimeout()` *inside the
|
||||||
|
hook* is what raises it — and a spec that spends the shared database
|
||||||
|
has to give it back, since the 90 specs share one backend in file
|
||||||
|
order.
|
||||||
|
|||||||
+73
-2
@@ -1,7 +1,7 @@
|
|||||||
# 008 — The last audit, and the one binding that outlived six phases
|
# 008 — The last audit, and the one binding that outlived six phases
|
||||||
|
|
||||||
**Status:** active — Phases 1, 2 and 3 shipped. Phase 4 is all that
|
**Status:** complete — all four phases shipped. `a11y.md` is closed,
|
||||||
remains, and `a11y.md` is closed.
|
and with it all four audits from 2026-08-11.
|
||||||
**Branch:** main
|
**Branch:** main
|
||||||
**Created:** 2026-08-12
|
**Created:** 2026-08-12
|
||||||
**Follows:** 007-ui-reconciliation
|
**Follows:** 007-ui-reconciliation
|
||||||
@@ -561,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
|
row is gone, the dialog said so, and the file still exists. Both halves
|
||||||
matter — the second is the promise the copy makes.
|
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
|
## Deliberately not in this plan
|
||||||
@@ -156,7 +156,7 @@ See `.planning/plans/completed/005-agent-development-harness.md`.
|
|||||||
**Backend packages** (under `backend/`):
|
**Backend packages** (under `backend/`):
|
||||||
- `player` — Audio playback via beep. `BufferedStreamer` provides a ring buffer for smooth seeking.
|
- `player` — Audio playback via beep. `BufferedStreamer` provides a ring buffer for smooth seeking.
|
||||||
- `queue` — Track queue with shuffle (Fisher-Yates), repeat modes, auto-advance, and session persistence.
|
- `queue` — Track queue with shuffle (Fisher-Yates), repeat modes, auto-advance, and session persistence.
|
||||||
- `library` — Concurrent library scanning, metadata extraction, cover art deduplication, incremental rescan.
|
- `library` — Concurrent library scanning, metadata extraction, cover art deduplication, incremental rescan. Also **removal**, below.
|
||||||
- `database` — SQLite via pure-Go driver. Schema in `database/sql/schemas/`, queries in `database/sql/queries/`. **sqlc** generates Go code into `database/sql/sqlcgen/` — never edit that directory by hand.
|
- `database` — SQLite via pure-Go driver. Schema in `database/sql/schemas/`, queries in `database/sql/queries/`. **sqlc** generates Go code into `database/sql/sqlcgen/` — never edit that directory by hand.
|
||||||
|
|
||||||
**Schema changes need two things, not one.** `sql/schemas/*.sql` is
|
**Schema changes need two things, not one.** `sql/schemas/*.sql` is
|
||||||
@@ -212,6 +212,20 @@ See `.planning/plans/completed/005-agent-development-harness.md`.
|
|||||||
`TestNoWritesOnTheReadPool` walks the tree for it, in the same
|
`TestNoWritesOnTheReadPool` walks the tree for it, in the same
|
||||||
spirit as `TestNoDirectRuntimeEmits` and for the same reason — a
|
spirit as `TestNoDirectRuntimeEmits` and for the same reason — a
|
||||||
lint pass only sees one build configuration.
|
lint pass only sees one build configuration.
|
||||||
|
- **A new table needs one file, not two.** The two-file rule is
|
||||||
|
about a column added to a table that already exists.
|
||||||
|
`applySchema` runs every file in `sql/schemas/` on *every* open,
|
||||||
|
so a `CREATE TABLE IF NOT EXISTS` reaches an existing install
|
||||||
|
verbatim and a migration for it would be a second description of
|
||||||
|
the same table — which is exactly what the third rule forbids.
|
||||||
|
`excluded_paths` is the worked example, index included, since the
|
||||||
|
column and its index arrive together.
|
||||||
|
- **A new table has to say what kind of data it holds.**
|
||||||
|
`backend/datamap` is a catalogue of every table's Kind and
|
||||||
|
Lifetime, and `TestCatalogCoversSchema` fails on a table missing
|
||||||
|
from it. `TestAuthoredCascadesAreDeliberate` then makes an
|
||||||
|
*authored* table that cascades an explicit, argued exemption —
|
||||||
|
authored data is what a user cannot get back.
|
||||||
- **Squashing is fine pre-1.0.** While this hasn't shipped to real
|
- **Squashing is fine pre-1.0.** While this hasn't shipped to real
|
||||||
users, periodically folding `sql/migrations/` into `sql/schemas/`
|
users, periodically folding `sql/migrations/` into `sql/schemas/`
|
||||||
and deleting the migration files (then wiping your own dev/sandbox
|
and deleting the migration files (then wiping your own dev/sandbox
|
||||||
@@ -904,6 +918,39 @@ reader and is created lazily, so neither the invalidation nor — more
|
|||||||
expensively — the singleton's own construction warms a cache for a page
|
expensively — the singleton's own construction warms a cache for a page
|
||||||
that may never open.
|
that may never open.
|
||||||
|
|
||||||
|
**"Remove from library" removes the row and excludes the path, and
|
||||||
|
never touches the file.** `RemoveFromLibrary(filePaths)` deletes the
|
||||||
|
`audio_files` rows the way the scan's own orphan cleanup does (tagging
|
||||||
|
group bookkeeping, FTS entry, `pruneOrphanedMetadata` for an album
|
||||||
|
whose last track just went) 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
|
||||||
|
and is worse than no button. It is reached from the track list's
|
||||||
|
context menu behind `confirmAction()`, whose **impact line says the
|
||||||
|
files are not deleted**, and from `tracklist.delete` (Delete), which is
|
||||||
|
bound to *opening that dialog* and nothing else: one keystroke from a
|
||||||
|
focused row, a key that asks is defensible and a key that acts is not.
|
||||||
|
|
||||||
|
Four things about it are load-bearing, and two are invisible from the
|
||||||
|
track list. **The soft scan compares files on disk against rows in the
|
||||||
|
database**, so an excluded path — on disk, deliberately not a row —
|
||||||
|
makes the two disagree forever and queues a full scan of the whole
|
||||||
|
library on *every* launch; `surveyAudioFiles` and `countAudioFiles`
|
||||||
|
therefore both take the exclusion set, because they answer "how many
|
||||||
|
files would a scan import", not "how many files are there". **Deleting
|
||||||
|
an `audio_files` row cascades to `queue_tracks`**, so the removal calls
|
||||||
|
the same `CompactQueue` hook `RemoveLibrary` does, which reloads the
|
||||||
|
queue and unloads the player if the removed track was the one playing.
|
||||||
|
**A full rescan clears the exclusions**, which is the only way back for
|
||||||
|
a path removed by mistake until there is a UI for the list. And
|
||||||
|
`TracksRemovedFromLibrary` carries `{filePaths, count}` so
|
||||||
|
`library-store` splices the tracks array in place and refetches only
|
||||||
|
the album/artist/genre summaries — falling back to a full invalidate
|
||||||
|
only when a tracks fetch is already in flight.
|
||||||
|
|
||||||
|
**Deleting the file from disk is deliberately not this**, and is not
|
||||||
|
foreclosed; it needs its own argument.
|
||||||
|
|
||||||
**An unchanged payload is not an event.** `explore`'s index status used
|
**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
|
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`
|
once the index was ready, and `config-page` assigns it to a `@state`
|
||||||
|
|||||||
Reference in New Issue
Block a user