From 723eee9773e185bd017b5ecb23c18bc369382a32 Mon Sep 17 00:00:00 2001
From: Bryan Van Deusen <bryan.vandeusen@fabledsword.com>
Date: Sat, 2 May 2026 22:28:49 -0400
Subject: [PATCH] =?UTF-8?q?docs(m7):=20revise=20#372=20spec=20=E2=80=94=20?=
 =?UTF-8?q?Remove=20from=20library=20no=20longer=20goes=20via=20Lidarr?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Operator decision during Task 2 implementation: routing track removal
through lidarrquarantine.DeleteViaLidarr is wrong because Lidarr is
album-granular (no per-track delete). The original spec would have
silently deleted sibling tracks.

New shape:
- Always delete file + DB row + cascade through Minstrel (os.Remove).
- Confirm dialog asks "find a replacement?": Yes (default — no Lidarr
  call; monitoring re-imports on next scan) or No (call new
  Lidarr.UnmonitorTrack(mbid) primitive).
- Lidarr unmonitor failure is non-fatal — sets lidarr_unmonitor_failed
  on the success envelope so the UI toasts a follow-up message.

Adds a new internal/lidarr.UnmonitorTrack method (LookupTrack →
PUT /api/v1/track/monitor with monitored:false) to Task 2's scope.
Wire codes lidarr_unreachable / lidarr_unauthorized / lidarr_server_error
no longer come back from this endpoint.
---
 ...2026-05-03-m7-track-actions-menu-design.md | 80 +++++++++++--------
 1 file changed, 45 insertions(+), 35 deletions(-)

diff --git a/docs/superpowers/specs/2026-05-03-m7-track-actions-menu-design.md b/docs/superpowers/specs/2026-05-03-m7-track-actions-menu-design.md
index 4141baff..ba2d5d56 100644
--- a/docs/superpowers/specs/2026-05-03-m7-track-actions-menu-design.md
+++ b/docs/superpowers/specs/2026-05-03-m7-track-actions-menu-design.md
@@ -20,7 +20,7 @@ Per-track kebab menu wherever `<TrackRow>` or the `<PlayerBar>` track display is
 - "Like / Unlike" and "Hide / Unhide" entries duplicate functionality already on the row (heart and eye icon buttons). Reason: keyboard discoverability — once the kebab is focused, arrow-keys reach every action; mouse users still get one-tap heart/eye on the row.
 - "Add to playlist…" reserves the menu slot but the submenu of playlists is wired in #352, not here. The slot renders as disabled with a tooltip "Coming with playlists" until #352 ships.
 - "Go to artist" is single-target (`track.artistId`); compilation/feature artists aren't modeled in the schema today and that's a schema problem to solve before this entry needs a submenu.
-- "Remove from library" is admin-only — gated client-side by `currentUser.is_admin`, server-side by the existing `auth.RequireAdmin()` middleware on the new `/api/admin/tracks/{id}` route. New backend endpoint deletes the track row, removes the file from disk (or via Lidarr for Lidarr-managed tracks), and cascades album → artist tidy-up if either becomes empty.
+- "Remove from library" is admin-only — gated client-side by `currentUser.is_admin`, server-side by `auth.RequireAdmin()` on the new `/api/admin/tracks/{id}` route. The endpoint always deletes the file from disk via `os.Remove` (not via Lidarr, which is album-granular and has no per-track delete) plus the DB row, then cascades album → artist tidy-up. For Lidarr-managed tracks the operator chooses whether Lidarr should look for a replacement (default — no extra action; Lidarr's monitoring re-imports on next scan) or the track should be unmonitored (server calls `Lidarr.UnmonitorTrack(mbid)` to flip `monitored: false`). The `?unmonitor=true` query param drives this — unset / `false` = replace, `true` = unmonitor.
 - Keyboard accessibility: kebab is `<button aria-haspopup="menu" aria-expanded>`; opened menu has `role="menu"` with each entry `role="menuitem"`. Up/Down arrows cycle, Enter activates, Escape closes and returns focus to the kebab.
 - Error envelope: existing `copyForCode()` helper resolves all surface errors (`lidarr_unreachable`, `lidarr_unauthorized`, `lidarr_server_error`, `not_found`, `unauthorized`, `connection_refused`).
 
@@ -37,27 +37,23 @@ Per-track kebab menu wherever `<TrackRow>` or the `<PlayerBar>` track display is
 
 ### Backend
 
-**One new admin endpoint:** `DELETE /api/admin/tracks/{id}`. Handler in `internal/api/admin_tracks.go` (new file). Calls a new `internal/tracks` package that owns the removal logic.
+**One new admin endpoint:** `DELETE /api/admin/tracks/{id}?unmonitor=true|false`. Handler in `internal/api/admin_tracks.go` (new file). Calls a new `internal/tracks` package that owns the removal logic. Also adds one new method to `internal/lidarr/client.go`: `UnmonitorTrack(ctx, trackMbid)` — looks up the track in Lidarr by mbid via `LookupTrack`, then PUTs `/api/v1/track/monitor` with `{trackIds: [lidarrTrackID], monitored: false}`. (Requires a small `put` helper alongside the existing `get` / `post`.)
 
-`internal/tracks/service.go` exposes `RemoveTrack(ctx, trackID, adminID)` returning `(deletedAlbumID *string, deletedArtistID *string, err)`. Steps:
+`internal/tracks/service.go` exposes `RemoveTrack(ctx, trackID, adminID, unmonitor bool)` returning `(deletedAlbumID *pgtype.UUID, deletedArtistID *pgtype.UUID, err)`. Steps:
 
-1. Look up the track. Need: `file_path`, `lidarr_managed`, `album_id`, `artist_id`. Returns `ErrNotFound` if missing.
-2. **For Lidarr-managed tracks:** call the existing Lidarr client to delete the track via Lidarr first. The current `lidarrquarantine.DeleteViaLidarr` handles this for quarantined tracks; the same primitive applies. If Lidarr is unreachable / unauthorized / errors, return the same typed errors (`ErrLidarrUnreachable`, `ErrLidarrUnauthorized`, `ErrLidarrServerError`) that the quarantine flow already maps. The DB row is NOT deleted on Lidarr failure — Lidarr would re-import on next sync and the row would come back inconsistent.
-3. **For non-Lidarr tracks:** `os.Remove(file_path)`. If the file is already missing, log `slog.Warn("track delete: file already missing", "path", file_path, "track_id", id)` and proceed — the goal is DB consistency.
-4. Begin a single DB transaction:
-   - `DELETE FROM tracks WHERE id=$1`. Existing FK `ON DELETE CASCADE` handles `play_events`, `general_likes_tracks`, `lidarr_quarantine`. (Future `playlist_tracks` will also need `ON DELETE CASCADE` — locked when #352's migration lands.)
-   - Verify all relevant FKs are CASCADE: read `internal/db/migrations/*.up.sql`. If any are RESTRICT or have no FK rule, fix as part of this slice via migration `0014_track_cascades.up.sql` — only if needed.
-   - Count remaining tracks for the album. If 0, `DELETE FROM albums WHERE id=$2`. Set `deletedAlbumID`.
-   - If album was deleted: count remaining albums for the artist. If 0, `DELETE FROM artists WHERE id=$3`. Set `deletedArtistID`.
+1. Look up the track. Need: `file_path`, `mbid`, `album_id`, `artist_id`. Returns `ErrNotFound` if missing.
+2. **Always:** `os.Remove(file_path)`. Tolerate `os.ErrNotExist` (file already gone — proceed with DB cleanup; the goal is consistency). Other `os.Remove` errors log a warning and proceed; orphan files are recoverable, orphan DB rows are not.
+3. Begin a single DB transaction:
+   - `DeleteTrack(id)` — returns `album_id`, `artist_id` from RETURNING. Existing FK CASCADE on `track_id` handles `play_events`, `general_likes_tracks`, `lidarr_quarantine`, `lidarr_quarantine_actions`. (Verified live — all four already cascade. Future `playlist_tracks` from #352 will also need `ON DELETE CASCADE`.)
+   - `DeleteAlbumIfEmpty(album_id)`. On `pgx.ErrNoRows` (album still has other tracks) skip. Otherwise set `deletedAlbumID`.
+   - If album was deleted: `DeleteArtistIfEmpty(artist_id)`. On `pgx.ErrNoRows` skip. Otherwise set `deletedArtistID`.
    - Commit.
-5. Return `(deletedAlbumID, deletedArtistID, nil)`.
+4. **If `unmonitor` AND track had a non-empty mbid (Lidarr-managed):** call `Lidarr.UnmonitorTrack(ctx, mbid)`. Lidarr errors here are **non-fatal** — log a warning and set a `lidarrUnmonitorFailed` flag, but don't fail the request. The file is already gone and the DB is consistent; failing now would leave the operator with no recovery path. The flag flows to the wire response so the UI can toast a follow-up message.
+5. Return `(deletedAlbumID, deletedArtistID, lidarrUnmonitorFailed bool, nil)`.
 
 **API handler** maps the typed errors:
 - `ErrNotFound` → 404 `{"error": "not_found"}`
-- `ErrLidarrUnreachable` → 503 `{"error": "lidarr_unreachable"}`
-- `ErrLidarrUnauthorized` → 503 `{"error": "lidarr_unauthorized"}`
-- `ErrLidarrServerError` → 502 `{"error": "lidarr_server_error"}`
-- success → 200 `{"deleted_track_id": "...", "deleted_album_id": "...?", "deleted_artist_id": "...?"}`
+- success → 200 `{"deleted_track_id": "...", "deleted_album_id": "...?", "deleted_artist_id": "...?", "lidarr_unmonitor_failed": true|false}` — `lidarr_unmonitor_failed` is only set to `true` when the operator requested unmonitor AND the Lidarr call failed; omitted in all other cases.
 
 ### Frontend
 
@@ -138,10 +134,14 @@ Navigation entries call `goto('/albums/' + track.albumId)` and `goto('/artists/'
 
 ## 5. API contract
 
-### `DELETE /api/admin/tracks/{id}`
+### `DELETE /api/admin/tracks/{id}?unmonitor=true|false`
 
 **Auth:** admin session.
 
+**Query params:**
+
+- `unmonitor` (optional, default `false`) — when `true` AND the track is Lidarr-managed (has an mbid), the server calls `Lidarr.UnmonitorTrack` after the file/DB delete so Lidarr won't search for a replacement. When `false` or omitted, no Lidarr call — Lidarr's monitoring re-imports on next scan, which is the operator's "find a replacement" path.
+
 **Request:** no body.
 
 **Response (200 OK):**
@@ -150,11 +150,13 @@ Navigation entries call `goto('/albums/' + track.albumId)` and `goto('/artists/'
 {
   "deleted_track_id": "uuid",
   "deleted_album_id": "uuid",
-  "deleted_artist_id": "uuid"
+  "deleted_artist_id": "uuid",
+  "lidarr_unmonitor_failed": true
 }
 ```
 
-`deleted_album_id` and `deleted_artist_id` are optional. Present only when removing the track left the parent empty and the row was deleted too.
+- `deleted_album_id` and `deleted_artist_id` are optional. Present only when removing the track left the parent empty and the row was deleted too.
+- `lidarr_unmonitor_failed` is only `true` when the operator requested `unmonitor=true` AND the Lidarr call failed (network / auth / 5xx). The file deletion + DB cleanup still succeeded — the field is informational so the UI can toast "File removed, but Lidarr couldn't be reached to stop the replacement search." Omitted in all other cases.
 
 **Errors:**
 
@@ -163,21 +165,20 @@ Navigation entries call `goto('/albums/' + track.albumId)` and `goto('/artists/'
 | 401 | `unauthenticated` | no session |
 | 403 | `unauthorized` | non-admin session |
 | 404 | `not_found` | track id doesn't exist |
-| 502 | `lidarr_server_error` | Lidarr returned 5xx |
-| 503 | `lidarr_unreachable` | network error reaching Lidarr |
-| 503 | `lidarr_unauthorized` | Lidarr rejected the API key |
-| 500 | `server_error` | unexpected error (DB, filesystem) |
+| 500 | `server_error` | unexpected error (DB, filesystem) before delete completed |
+
+Note: Lidarr errors during the unmonitor step are NOT mapped to wire errors — the response is 200 with `lidarr_unmonitor_failed: true`, since the destructive part (file + DB) already succeeded and the operator needs to know to retry the unmonitor manually.
 
 ## 6. Error handling (frontend)
 
 `removeTrack(id)` reuses `apiFetch` with the existing dual-envelope handling. Errors surface as a toast via `copyForCode(code)`:
 
-- `lidarr_unreachable` → "Lidarr is unreachable right now. Try again, or check Admin → Integrations." (existing copy)
-- `lidarr_unauthorized` → existing copy
-- `lidarr_server_error` → existing copy
 - `not_found` → "Track no longer exists." Trigger refetch of parent list query.
 - `unauthorized` → "Admins only." (Defensive — UI should hide the entry for non-admins.)
 - `connection_refused` → existing copy.
+- `server_error` → existing copy.
+
+Lidarr-related error codes do NOT come back from this endpoint anymore (they're handled inline as a non-fatal `lidarr_unmonitor_failed: true` flag in the success envelope). When the response sets that flag, surface a follow-up toast: "File removed, but Lidarr couldn't be reached to stop the replacement search. Unmonitor manually in Admin → Integrations if you don't want it to come back."
 
 Audio queue mutations have no error path — they are local state writes. Defensive guard: if `_queue` is `null` (audio store uninitialized), the action is a no-op.
 
@@ -186,18 +187,25 @@ Audio queue mutations have no error path — they are local state writes. Defens
 ### Backend
 
 - `internal/tracks/service_test.go` — unit tests, table-driven where it helps:
-  - Non-Lidarr track happy path. File present, deletes cleanly. DB row gone. Album / artist persist.
-  - Lidarr-managed track happy path. Lidarr returns 200; file removed by Lidarr; DB row gone.
+  - Local-file happy path. `unmonitor=false`. File present, deletes cleanly. DB row gone. No Lidarr call.
   - File-already-missing path. `os.Remove` returns `ErrNotExist`. Service logs warning, proceeds. DB row gone.
-  - Lidarr error propagation. `lidarr_unreachable` / `lidarr_unauthorized` / `lidarr_server_error` each surface as the matching `Err…`. DB row stays.
-  - Album-empty cascade. Track was the album's only track. Album row deleted, artist persists if has other albums. `deletedAlbumID` returned.
-  - Artist-empty cascade. Track was the artist's only track. Album AND artist deleted. Both IDs returned.
+  - Album-empty cascade. Track was the album's only track. Album deleted; artist persists if it has other albums. `deletedAlbumID` returned, `deletedArtistID` nil.
+  - Artist-empty cascade. Track was the artist's only track. Album AND artist deleted; both IDs returned.
+  - `unmonitor=true` AND track has mbid → `Lidarr.UnmonitorTrack` is called once with the mbid. `lidarrUnmonitorFailed=false` returned.
+  - `unmonitor=true` AND Lidarr fails (unreachable / 5xx / auth) → file + DB still deleted, `lidarrUnmonitorFailed=true` returned, no error from RemoveTrack.
+  - `unmonitor=true` AND track has empty mbid (non-Lidarr) → no Lidarr call, `lidarrUnmonitorFailed=false`.
+  - `unmonitor=false` AND track has mbid → no Lidarr call.
+- `internal/lidarr/client_test.go` — extend with `UnmonitorTrack` tests:
+  - Track found in lookup → PUT to `/api/v1/track/monitor` with `{trackIds: [...], monitored: false}`.
+  - Track not found in lookup → typed error.
+  - Lidarr 5xx → `ErrServerError`.
+  - Lidarr unreachable → `ErrUnreachable`.
 - `internal/api/admin_tracks_test.go` — HTTP tests:
   - Non-admin → 403 `unauthorized`.
   - No session → 401 `unauthenticated`.
   - Unknown id → 404 `not_found`.
-  - Each typed Lidarr error → matching response code + envelope.
-  - Happy path → 200 with `deleted_track_id` (and optionally `deleted_album_id` / `deleted_artist_id`).
+  - Happy path no unmonitor → 200 with `deleted_track_id` (and optionally `deleted_album_id` / `deleted_artist_id`); no `lidarr_unmonitor_failed` in response.
+  - Happy path with `?unmonitor=true` and Lidarr failure → 200 with `lidarr_unmonitor_failed: true`.
 
 ### Frontend
 
@@ -210,13 +218,13 @@ Audio queue mutations have no error path — they are local state writes. Defens
   - Outside click closes.
   - Submenu plumbing (`role="menu"` on the playlist hook) renders even though disabled.
 - `TrackMenuItem.test.ts` — primitive renders icon, label, click handler fires, `aria-disabled` honored.
-- `tracks.test.ts` (new admin API helper) — `removeTrack` posts the correct method+URL, parses success envelope, surfaces errors.
+- `tracks.test.ts` (new admin API helper) — `removeTrack(id, {unmonitor})` sends `?unmonitor=true|false`, parses success envelope (incl. optional `lidarr_unmonitor_failed`), surfaces typed errors via `apiFetch`.
 - `audio.test.ts` (extend) — `playNext(track)` splices at `_index+1`; `addToQueue(track)` appends; both no-op on empty queue.
 - `TrackRow.test.ts` and `PlayerBar.test.ts` (adjust) — existing assertions about the menu's sole "Flag" entry need updating to match the new 9-entry structure. The compatible prop API means no other test changes needed.
 
 ## 8. Distribution / migration notes
 
-- **Migration `0014`:** conditional. Read the existing FKs from migrations 0002 (tracks), 0005 (play_events), 0006 (general_likes), 0011 (lidarr_quarantine). If any FK on `track_id` is `RESTRICT` or `NO ACTION`, write a migration to `ALTER TABLE … DROP CONSTRAINT … ADD CONSTRAINT … ON DELETE CASCADE`. If all four are already CASCADE, skip the migration entirely.
+- **No migration needed.** All four `track_id` FKs (`play_events`, `general_likes_tracks`, `lidarr_quarantine`, `lidarr_quarantine_actions`) already use `ON DELETE CASCADE`; verified live before plan-write.
 - No client breaking changes. The `<TrackMenu>` prop API is unchanged.
 - `min_client_version` bump in `internal/server/version.go`: not required — the new endpoint only matters for admins, and the menu degrades gracefully on old clients (they don't show entries they don't know about).
 
@@ -230,6 +238,8 @@ M7 #372. Brainstorm 2026-05-03. Decisions locked:
 4. `Remove from library` admin-only, with cascade album → artist tidy-up.
 5. Submenu plumbing only matters for #352's `Add to playlist…` and is reserved as a slot here.
 
+**Revision 2026-05-03 (during Task 2 implementation):** original spec routed Lidarr-managed track removal through `lidarrquarantine.DeleteViaLidarr`. Implementer surfaced that primitive deletes the entire **Lidarr album** (Lidarr is album-granular), not the single track — the spec's behavior would silently delete sibling tracks. Operator decision: drop the routing-via-Lidarr approach entirely. Always delete file + DB directly through Minstrel; ask the operator at confirm time whether Lidarr should look for a replacement (default — no extra action) or be told to unmonitor (new `Lidarr.UnmonitorTrack(mbid)` primitive). Lidarr unmonitor failure is non-fatal — the destructive part already succeeded; surface a follow-up toast so the operator can manually unmonitor.
+
 ## 10. Related
 
 - M7 #352 — Playlists CRUD. Wires `Add to playlist…` submenu into the menu slot reserved here. Brainstorm parked at task-log id 44.