diff --git a/.pi/skills/yellowjacket-dev/references/schema-change.md b/.pi/skills/yellowjacket-dev/references/schema-change.md index b74f1c3..fab717e 100644 --- a/.pi/skills/yellowjacket-dev/references/schema-change.md +++ b/.pi/skills/yellowjacket-dev/references/schema-change.md @@ -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 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 EXISTS`, the literal target shape, what sqlc reads and what a fresh diff --git a/.planning/NOTES.md b/.planning/NOTES.md index 8d9ccb5..5fa8dcf 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -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 tiers assert it is *absent* on the other rows — a marker that renders 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. diff --git a/.planning/plans/active/008-the-last-audit.md b/.planning/plans/completed/008-the-last-audit.md similarity index 88% rename from .planning/plans/active/008-the-last-audit.md rename to .planning/plans/completed/008-the-last-audit.md index aedd79c..0e252a1 100644 --- a/.planning/plans/active/008-the-last-audit.md +++ b/.planning/plans/completed/008-the-last-audit.md @@ -1,7 +1,7 @@ # 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 -remains, and `a11y.md` is closed. +**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 @@ -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 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 diff --git a/CLAUDE.md b/CLAUDE.md index c961e2c..cc38929 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -156,7 +156,7 @@ See `.planning/plans/completed/005-agent-development-harness.md`. **Backend packages** (under `backend/`): - `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. -- `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. **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 spirit as `TestNoDirectRuntimeEmits` and for the same reason — a 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 users, periodically folding `sql/migrations/` into `sql/schemas/` 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 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 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`