From 9c53c9894a9b4c02dc71a13bb662a982011f4b8e Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 2 May 2026 21:39:30 -0400 Subject: [PATCH] docs(m7): implementation plan for #372 track-actions menu MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 9 tasks covering: SQL cascade-cleanup queries, internal/tracks service with Lidarr-aware RemoveTrack, DELETE /api/admin/tracks/{id} handler, frontend admin API helper, audio-store playNext addition, TrackMenuItem + TrackMenuDivider primitives, full TrackMenu rewrite with all 9 entries (Add to playlist… reserved as a disabled slot for #352), and TrackRow/PlayerBar test adjustments. Plan reflects the no-in-task-tests rule — implementers write test files but don't run them; CI handles verification. Migration 0014 that the spec marked conditional is dropped: all relevant track_id FKs already cascade. --- .../plans/2026-05-03-m7-track-actions-menu.md | 1515 +++++++++++++++++ 1 file changed, 1515 insertions(+) create mode 100644 docs/superpowers/plans/2026-05-03-m7-track-actions-menu.md diff --git a/docs/superpowers/plans/2026-05-03-m7-track-actions-menu.md b/docs/superpowers/plans/2026-05-03-m7-track-actions-menu.md new file mode 100644 index 00000000..f656211f --- /dev/null +++ b/docs/superpowers/plans/2026-05-03-m7-track-actions-menu.md @@ -0,0 +1,1515 @@ +# M7 — Track-level actions menu — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Replace the M5b-era single-entry `` (which only offers "Flag this track…") with a 9-entry kebab menu in 4 logical groups (queue / collection / navigation / lifecycle). Reserves an "Add to playlist…" slot for #352. Lands a new admin-only `DELETE /api/admin/tracks/{id}` endpoint with cascade album → artist tidy-up and Lidarr-aware deletion for managed tracks. + +**Architecture:** Backend adds a single admin endpoint backed by a new `internal/tracks` service. Lidarr-managed tracks reuse the existing `lidarrquarantine.DeleteViaLidarr` primitive; non-Lidarr tracks use direct file removal. All `track_id` FKs already cascade. Frontend rewrites `` from one FlagPopover-only entry into nine entries via two new primitives (``, ``). The component's prop API (`track`, `direction`) stays compatible so existing callers (``, ``) pick up the new entries with no code change. + +**Tech Stack:** Go 1.23 · pgx/v5 + sqlc · Postgres (no migration) · SvelteKit 2 / Svelte 5 (runes) · TanStack Query · Vitest · Lucide icons · FabledSword design tokens. + +**Spec:** [`docs/superpowers/specs/2026-05-03-m7-track-actions-menu-design.md`](../specs/2026-05-03-m7-track-actions-menu-design.md). Read it before starting — every decision is explained there. + +**Memory dependencies:** +- `feedback_no_in_task_tests.md` — HARD RULE. Do NOT run `npm test`, `npm run check`, `go test ./...`, `flutter test`, etc. inside any task. Write the test file as code, write the production code, commit. CI runs the suite. The plan reflects this — there are no `Run: npm test` steps. +- `feedback_v1_is_full_product.md` — this slice ships complete, not as scaffolding for a later iteration. +- `project_design_system.md` — FabledSword tokens (no hardcoded colors), Lucide icons, sentence case. +- `project_no_github.md` — Forgejo MCP for any PR/issue ops; never `gh` CLI. +- `project_subsonic_legacy.md` — `/api/*` is primary; this slice adds nothing under `/rest/*`. +- `project_git_workflow.md` — commit on `dev`; PR to `main` is a separate step. + +--- + +## File map + +### Backend — create + +- `internal/tracks/service.go` — `RemoveTrack(ctx, trackID, adminID)` returning `(deletedAlbumID, deletedArtistID, err)`. Typed errors: `ErrNotFound`, `ErrLidarrUnreachable`, `ErrLidarrUnauthorized`, `ErrLidarrServerError`. +- `internal/tracks/service_test.go` — six service-level cases. +- `internal/api/admin_tracks.go` — `handleRemoveTrack` HTTP handler. +- `internal/api/admin_tracks_test.go` — admin-auth cases + four error codes + happy path envelope. + +### Backend — modify + +- `internal/db/queries/tracks.sql` — append `DeleteTrack`, `CountTracksByAlbum` (already exists; verify reuse). +- `internal/db/queries/albums.sql` — append `DeleteAlbumIfEmpty`, `CountAlbumsByArtist`. +- `internal/db/queries/artists.sql` — append `DeleteArtistIfEmpty`. +- `internal/db/dbq/*` — regenerated by `sqlc generate`. +- `internal/api/api.go` — register `admin.Delete("/tracks/{id}", h.handleRemoveTrack)`. + +### Frontend — create + +- `web/src/lib/components/TrackMenuItem.svelte` — single menu entry (icon + label + onclick + aria-disabled). +- `web/src/lib/components/TrackMenuItem.test.ts` +- `web/src/lib/components/TrackMenuDivider.svelte` — visual separator (`
` styled to `--fs-pewter`). +- `web/src/lib/api/admin/tracks.ts` — `removeTrack(id)` + `RemoveTrackResult` type. +- `web/src/lib/api/admin/tracks.test.ts` + +### Frontend — modify + +- `web/src/lib/components/TrackMenu.svelte` — full rewrite from FlagPopover-only to multi-entry menu. Preserves `track: TrackRef` and `direction: 'up' | 'down'` props. +- `web/src/lib/components/TrackMenu.test.ts` — full rewrite. +- `web/src/lib/components/TrackRow.test.ts` — adjust assertions for the new 9-entry menu (no production code change). +- `web/src/lib/components/PlayerBar.test.ts` — same. +- `web/src/lib/player/store.svelte.ts` — add `playNext(t: TrackRef)` (splices at `_index + 1`). `enqueueTrack(t)` already exists for "Add to queue" semantics; reused. +- `web/src/lib/player/store.test.ts` — extend with playNext cases. + +--- + +## Task list + +### Task 1 — SQL queries for cascade cleanup + +**Files:** +- Modify: `internal/db/queries/tracks.sql` (append `DeleteTrack`) +- Modify: `internal/db/queries/albums.sql` (append `DeleteAlbumIfEmpty`, `CountAlbumsByArtist`) +- Modify: `internal/db/queries/artists.sql` (append `DeleteArtistIfEmpty`) +- Regenerate: `internal/db/dbq/*` + +- [ ] **Step 1.1: Append `DeleteTrack` to `internal/db/queries/tracks.sql`** + +```sql +-- name: DeleteTrack :one +-- M7 #372: hard delete with FK cascade. The CASCADE on track_id from +-- play_events / general_likes_tracks / lidarr_quarantine / +-- lidarr_quarantine_actions handles their cleanup. RETURNING gives us +-- album_id + artist_id for the album-empty / artist-empty cascade +-- checks the service does next. +DELETE FROM tracks WHERE id = $1 +RETURNING id, album_id, artist_id, file_path, mbid; +``` + +- [ ] **Step 1.2: Append `DeleteAlbumIfEmpty` + `CountAlbumsByArtist` to `internal/db/queries/albums.sql`** + +```sql +-- name: DeleteAlbumIfEmpty :one +-- M7 #372: deletes the album row only when it has no remaining tracks. +-- Used after a track delete to tidy up orphaned albums. RETURNING gives +-- us the artist_id so the service can chain into the artist-empty check. +-- Returns no rows when the album still has tracks; the service treats +-- pgx.ErrNoRows as "album not orphaned, skip cascade". +DELETE FROM albums +WHERE id = $1 + AND NOT EXISTS (SELECT 1 FROM tracks WHERE album_id = $1) +RETURNING id, artist_id; + +-- name: CountAlbumsByArtist :one +SELECT count(*) FROM albums WHERE artist_id = $1; +``` + +- [ ] **Step 1.3: Append `DeleteArtistIfEmpty` to `internal/db/queries/artists.sql`** + +```sql +-- name: DeleteArtistIfEmpty :one +-- M7 #372: deletes the artist row only when it has no remaining albums +-- AND no remaining tracks (defensive — tracks can in principle exist +-- without an album, though the scanner doesn't write that shape today). +DELETE FROM artists +WHERE id = $1 + AND NOT EXISTS (SELECT 1 FROM albums WHERE artist_id = $1) + AND NOT EXISTS (SELECT 1 FROM tracks WHERE artist_id = $1) +RETURNING id; +``` + +- [ ] **Step 1.4: Regenerate sqlc** + +```bash +cd /home/bvandeusen/Nextcloud/Projects/Minstrel/minstrel +sqlc generate +``` + +Expected: `internal/db/dbq/tracks.sql.go`, `internal/db/dbq/albums.sql.go`, `internal/db/dbq/artists.sql.go` updated with the new methods. + +- [ ] **Step 1.5: Commit** + +```bash +git add internal/db/queries/tracks.sql internal/db/queries/albums.sql internal/db/queries/artists.sql internal/db/dbq/ +git commit -F - <<'EOF' +feat(db): cascade-cleanup queries for M7 #372 track removal + +DeleteTrack returns album_id + artist_id so the calling service can +chain the album-empty / artist-empty cascade. DeleteAlbumIfEmpty and +DeleteArtistIfEmpty are no-ops when other rows still reference the +parent — the service treats pgx.ErrNoRows as "not orphaned, skip". +EOF +``` + +--- + +### Task 2 — `internal/tracks/service.go` — RemoveTrack + +**Files:** +- Create: `internal/tracks/service.go` +- Create: `internal/tracks/service_test.go` + +This task introduces a new service module. RemoveTrack drives the whole removal flow: fetch the track, delegate file removal (Lidarr-managed → Lidarr API; non-Lidarr → direct os.Remove), execute DB deletes in a single transaction, and return the IDs of any cascaded album/artist rows so the API caller can invalidate caches. + +- [ ] **Step 2.1: Write `internal/tracks/service.go`** + +```go +package tracks + +import ( + "context" + "errors" + "fmt" + "log/slog" + "os" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgtype" + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/lidarrquarantine" +) + +// Typed errors. Mapped to wire codes by the API layer. +var ( + ErrNotFound = errors.New("track not found") + ErrLidarrUnreachable = errors.New("lidarr unreachable") + ErrLidarrUnauthorized = errors.New("lidarr unauthorized") + ErrLidarrServerError = errors.New("lidarr server error") +) + +// LidarrDeleter is the subset of lidarrquarantine.Service the tracks +// service needs. Defined here so tests can stub it without spinning up +// a real Lidarr stub. +type LidarrDeleter interface { + DeleteViaLidarr(ctx context.Context, trackID, adminID pgtype.UUID) (dbq.LidarrQuarantineAction, int, error) +} + +type Service struct { + pool *pgxpool.Pool + logger *slog.Logger + lidarrDeleter LidarrDeleter +} + +func NewService(pool *pgxpool.Pool, logger *slog.Logger, lidarrDeleter LidarrDeleter) *Service { + if logger == nil { + logger = slog.Default() + } + return &Service{pool: pool, logger: logger, lidarrDeleter: lidarrDeleter} +} + +// RemoveTrack deletes the track and (cascade-style) any album / artist +// rows that become orphaned. For Lidarr-managed tracks, Lidarr is told +// to delete first so it doesn't re-import on the next sync. +// +// Returns the deleted track id plus optional album_id and artist_id when +// those parent rows were tidied up too. The API layer surfaces these so +// the client can invalidate the matching cached query keys. +func (s *Service) RemoveTrack(ctx context.Context, trackID, adminID pgtype.UUID) (deletedAlbumID, deletedArtistID *pgtype.UUID, err error) { + q := dbq.New(s.pool) + + track, err := q.GetTrackByID(ctx, trackID) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return nil, nil, ErrNotFound + } + return nil, nil, fmt.Errorf("get track: %w", err) + } + + // Lidarr-managed → delegate file removal to Lidarr (so Lidarr stops + // re-importing). Track is "Lidarr-managed" when its mbid is set — + // scanner only writes the mbid for Lidarr-imported files. + if track.Mbid.Valid && s.lidarrDeleter != nil { + _, _, lerr := s.lidarrDeleter.DeleteViaLidarr(ctx, trackID, adminID) + switch { + case lerr == nil: + // Lidarr deleted the file. Proceed to DB cleanup below. + case errors.Is(lerr, lidarrquarantine.ErrLidarrUnreachable): + return nil, nil, ErrLidarrUnreachable + case errors.Is(lerr, lidarrquarantine.ErrLidarrUnauthorized): + return nil, nil, ErrLidarrUnauthorized + case errors.Is(lerr, lidarrquarantine.ErrLidarrServerError): + return nil, nil, ErrLidarrServerError + default: + return nil, nil, fmt.Errorf("lidarr delete: %w", lerr) + } + } else if track.FilePath != "" { + // Non-Lidarr track → direct file removal. Already-missing is fine; + // the goal is DB consistency. + if rerr := os.Remove(track.FilePath); rerr != nil && !errors.Is(rerr, os.ErrNotExist) { + s.logger.Warn("track delete: file remove failed", + "path", track.FilePath, "track_id", trackID, "err", rerr) + // We still proceed — leaving an orphan file is recoverable; + // leaving an orphan DB row would cause the row to keep showing + // up in the UI with no playable file. + } + } + + // DB cleanup in a single transaction so a failure midway leaves the + // schema consistent. + tx, err := s.pool.Begin(ctx) + if err != nil { + return nil, nil, fmt.Errorf("begin tx: %w", err) + } + defer func() { _ = tx.Rollback(ctx) }() // no-op once Commit succeeds + + tq := dbq.New(tx) + + deleted, err := tq.DeleteTrack(ctx, trackID) + if err != nil { + return nil, nil, fmt.Errorf("delete track: %w", err) + } + + // Album-empty cascade. + albumRow, err := tq.DeleteAlbumIfEmpty(ctx, deleted.AlbumID) + switch { + case err == nil: + deletedAlbumID = &albumRow.ID + // Artist-empty cascade only relevant when the album was tidied up. + artistRow, aerr := tq.DeleteArtistIfEmpty(ctx, albumRow.ArtistID) + switch { + case aerr == nil: + deletedArtistID = &artistRow.ID + case errors.Is(aerr, pgx.ErrNoRows): + // Artist still has other albums or stray tracks; leave it. + default: + return nil, nil, fmt.Errorf("delete artist if empty: %w", aerr) + } + case errors.Is(err, pgx.ErrNoRows): + // Album still has other tracks; leave it. + default: + return nil, nil, fmt.Errorf("delete album if empty: %w", err) + } + + if err := tx.Commit(ctx); err != nil { + return nil, nil, fmt.Errorf("commit: %w", err) + } + + return deletedAlbumID, deletedArtistID, nil +} +``` + +- [ ] **Step 2.2: Write `internal/tracks/service_test.go`** + +```go +package tracks_test + +import ( + "context" + "errors" + "net/http/httptest" + "os" + "path/filepath" + "testing" + + "github.com/jackc/pgx/v5/pgtype" + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/lidarrconfig" + "git.fabledsword.com/bvandeusen/minstrel/internal/lidarrquarantine" + "git.fabledsword.com/bvandeusen/minstrel/internal/tracks" +) + +// fakeLidarrDeleter records calls and returns a configurable error. +type fakeLidarrDeleter struct { + calls []pgtype.UUID + err error +} + +func (f *fakeLidarrDeleter) DeleteViaLidarr(ctx context.Context, trackID, adminID pgtype.UUID) (dbq.LidarrQuarantineAction, int, error) { + f.calls = append(f.calls, trackID) + return dbq.LidarrQuarantineAction{}, 0, f.err +} + +// Helpers seedUser, seedTrack, newPool follow the same pattern as +// existing service tests (see internal/lidarrquarantine/service_test.go +// for reference). Reuse them via copy or the existing test fixtures. + +func TestRemoveTrack_NotFound(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "admin") + + svc := tracks.NewService(pool, nil, &fakeLidarrDeleter{}) + _, _, err := svc.RemoveTrack(context.Background(), randomUUID(), user.ID) + if !errors.Is(err, tracks.ErrNotFound) { + t.Errorf("err = %v, want ErrNotFound", err) + } +} + +func TestRemoveTrack_NonLidarr_HappyPath(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "admin") + + dir := t.TempDir() + path := filepath.Join(dir, "song.mp3") + if err := os.WriteFile(path, []byte("audio"), 0o644); err != nil { + t.Fatalf("write fixture: %v", err) + } + track := seedTrackAt(t, pool, "Song", path, "" /* no mbid → non-Lidarr */) + + svc := tracks.NewService(pool, nil, &fakeLidarrDeleter{}) + _, _, err := svc.RemoveTrack(context.Background(), track.ID, user.ID) + if err != nil { + t.Fatalf("RemoveTrack: %v", err) + } + + if _, statErr := os.Stat(path); !errors.Is(statErr, os.ErrNotExist) { + t.Errorf("file still present at %s", path) + } + + q := dbq.New(pool) + if _, gerr := q.GetTrackByID(context.Background(), track.ID); gerr == nil { + t.Error("track row still in DB") + } +} + +func TestRemoveTrack_NonLidarr_FileAlreadyMissing(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "admin") + + track := seedTrackAt(t, pool, "Song", "/tmp/does-not-exist-xyz", "") + + svc := tracks.NewService(pool, nil, &fakeLidarrDeleter{}) + if _, _, err := svc.RemoveTrack(context.Background(), track.ID, user.ID); err != nil { + t.Fatalf("RemoveTrack: %v (file-already-missing should be tolerated)", err) + } +} + +func TestRemoveTrack_Lidarr_HappyPath(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "admin") + track := seedLidarrTrack(t, pool, "Song", "track-mbid") + + deleter := &fakeLidarrDeleter{} + svc := tracks.NewService(pool, nil, deleter) + if _, _, err := svc.RemoveTrack(context.Background(), track.ID, user.ID); err != nil { + t.Fatalf("RemoveTrack: %v", err) + } + if len(deleter.calls) != 1 { + t.Errorf("DeleteViaLidarr call count = %d, want 1", len(deleter.calls)) + } +} + +func TestRemoveTrack_Lidarr_ErrorPropagation(t *testing.T) { + cases := []struct { + name string + lidarr error + wantErr error + }{ + {"unreachable", lidarrquarantine.ErrLidarrUnreachable, tracks.ErrLidarrUnreachable}, + {"unauthorized", lidarrquarantine.ErrLidarrUnauthorized, tracks.ErrLidarrUnauthorized}, + {"server_error", lidarrquarantine.ErrLidarrServerError, tracks.ErrLidarrServerError}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "admin") + track := seedLidarrTrack(t, pool, "Song", "mbid") + + deleter := &fakeLidarrDeleter{err: tc.lidarr} + svc := tracks.NewService(pool, nil, deleter) + _, _, err := svc.RemoveTrack(context.Background(), track.ID, user.ID) + if !errors.Is(err, tc.wantErr) { + t.Errorf("err = %v, want %v", err, tc.wantErr) + } + + // DB row must still exist — Lidarr failure should not orphan rows. + q := dbq.New(pool) + if _, gerr := q.GetTrackByID(context.Background(), track.ID); gerr != nil { + t.Errorf("track row was deleted despite Lidarr failure: %v", gerr) + } + }) + } +} + +func TestRemoveTrack_Cascade_AlbumEmpty(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "admin") + + // Single track in a single album, both belong to a single artist that + // has another album with another track. The track delete should also + // drop the album, but the artist persists. + artist := seedArtist(t, pool, "Solo") + albumA := seedAlbumForArtist(t, pool, "OnlyAlbum", artist.ID) + trackA := seedTrackInAlbum(t, pool, "OnlyTrack", albumA.ID, artist.ID) + + albumB := seedAlbumForArtist(t, pool, "OtherAlbum", artist.ID) + _ = seedTrackInAlbum(t, pool, "OtherTrack", albumB.ID, artist.ID) + + svc := tracks.NewService(pool, nil, &fakeLidarrDeleter{}) + deletedAlbum, deletedArtist, err := svc.RemoveTrack(context.Background(), trackA.ID, user.ID) + if err != nil { + t.Fatalf("RemoveTrack: %v", err) + } + if deletedAlbum == nil || deletedAlbum.Bytes != albumA.ID.Bytes { + t.Errorf("deletedAlbumID = %v, want albumA.ID", deletedAlbum) + } + if deletedArtist != nil { + t.Errorf("deletedArtistID = %v, want nil (artist still has albumB)", deletedArtist) + } +} + +func TestRemoveTrack_Cascade_ArtistEmpty(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "admin") + + artist := seedArtist(t, pool, "Solo") + album := seedAlbumForArtist(t, pool, "OnlyAlbum", artist.ID) + track := seedTrackInAlbum(t, pool, "OnlyTrack", album.ID, artist.ID) + + svc := tracks.NewService(pool, nil, &fakeLidarrDeleter{}) + deletedAlbum, deletedArtist, err := svc.RemoveTrack(context.Background(), track.ID, user.ID) + if err != nil { + t.Fatalf("RemoveTrack: %v", err) + } + if deletedAlbum == nil { + t.Error("deletedAlbumID = nil, want album.ID") + } + if deletedArtist == nil { + t.Error("deletedArtistID = nil, want artist.ID") + } +} +``` + +The seed helpers (`newPool`, `seedUser`, `seedArtist`, `seedAlbumForArtist`, `seedTrackInAlbum`, `seedTrackAt`, `seedLidarrTrack`, `randomUUID`) follow the same pattern used in `internal/lidarrquarantine/service_test.go`. The implementer either copies them into a `service_test_helpers.go` in this package or extracts the existing ones into a shared `internal/dbtest` package — operator preference, but copying into the new package is the path of least surprise. + +- [ ] **Step 2.3: Commit** + +```bash +git add internal/tracks/service.go internal/tracks/service_test.go +git commit -F - <<'EOF' +feat(tracks): RemoveTrack service with Lidarr-aware delete + cascade + +RemoveTrack handles both Lidarr-managed (delegates to existing +lidarrquarantine.DeleteViaLidarr) and non-Lidarr (direct os.Remove) +file deletion, then runs the cascade album-if-empty / artist-if-empty +DB cleanup in a single transaction. Lidarr errors propagate as typed +errors so the API layer can map them to the existing wire codes. +EOF +``` + +--- + +### Task 3 — `internal/api/admin_tracks.go` — HTTP handler + +**Files:** +- Create: `internal/api/admin_tracks.go` +- Create: `internal/api/admin_tracks_test.go` +- Modify: `internal/api/api.go` (register the route) +- Modify: `internal/api/api.go` and the handler-construction site (wire the new tracks service) + +- [ ] **Step 3.1: Write `internal/api/admin_tracks.go`** + +```go +package api + +import ( + "encoding/json" + "errors" + "net/http" + + "github.com/go-chi/chi/v5" + "github.com/jackc/pgx/v5/pgtype" + + "git.fabledsword.com/bvandeusen/minstrel/internal/auth" + "git.fabledsword.com/bvandeusen/minstrel/internal/tracks" +) + +// removeTrackResponse mirrors the wire shape from the spec. The +// optional fields are omitted when their corresponding cascade did +// not fire (album/artist still has other rows). +type removeTrackResponse struct { + DeletedTrackID string `json:"deleted_track_id"` + DeletedAlbumID *string `json:"deleted_album_id,omitempty"` + DeletedArtistID *string `json:"deleted_artist_id,omitempty"` +} + +// handleRemoveTrack implements DELETE /api/admin/tracks/{id}. +// Admin-only. Cascades album / artist if the track removal leaves +// either parent empty. +func (h *handlers) handleRemoveTrack(w http.ResponseWriter, r *http.Request) { + idStr := chi.URLParam(r, "id") + var trackID pgtype.UUID + if err := trackID.Scan(idStr); err != nil { + writeErr(w, http.StatusBadRequest, "bad_request", "invalid track id") + return + } + + admin, ok := auth.UserFromContext(r.Context()) + if !ok { + writeErr(w, http.StatusUnauthorized, "unauthenticated", "no session") + return + } + + deletedAlbum, deletedArtist, err := h.tracks.RemoveTrack(r.Context(), trackID, admin.ID) + switch { + case err == nil: + // fall through + case errors.Is(err, tracks.ErrNotFound): + writeErr(w, http.StatusNotFound, "not_found", "track not found") + return + case errors.Is(err, tracks.ErrLidarrUnreachable): + writeErr(w, http.StatusServiceUnavailable, "lidarr_unreachable", "Lidarr is unreachable") + return + case errors.Is(err, tracks.ErrLidarrUnauthorized): + writeErr(w, http.StatusServiceUnavailable, "lidarr_unauthorized", "Lidarr rejected the API key") + return + case errors.Is(err, tracks.ErrLidarrServerError): + writeErr(w, http.StatusBadGateway, "lidarr_server_error", "Lidarr returned an error") + return + default: + h.logger.Error("api: remove track failed", "err", err, "track_id", trackID) + writeErr(w, http.StatusInternalServerError, "server_error", "remove failed") + return + } + + resp := removeTrackResponse{DeletedTrackID: idStr} + if deletedAlbum != nil { + s := uuidToString(*deletedAlbum) + resp.DeletedAlbumID = &s + } + if deletedArtist != nil { + s := uuidToString(*deletedArtist) + resp.DeletedArtistID = &s + } + + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(resp) +} +``` + +If `uuidToString` doesn't exist in the api package, add it next to `convert.go`'s helpers — single line: `func uuidToString(u pgtype.UUID) string { return uuid.UUID(u.Bytes).String() }` (using `github.com/google/uuid`, which the api package already imports for similar conversions; check `convert.go` to confirm the existing helper name and reuse it instead of duplicating). + +- [ ] **Step 3.2: Wire the tracks service into `handlers`** + +In `internal/api/api.go`, the `Mount` function takes a number of services. Read the existing signature, then add `tracksSvc *tracks.Service` (with `import "git.fabledsword.com/bvandeusen/minstrel/internal/tracks"`). Add a corresponding `tracks` field to the `handlers` struct. The construction at the call site in `internal/server/server.go` builds the new service via `tracks.NewService(s.Pool, s.Logger, lidarrQuar)` (where `lidarrQuar` is the existing `*lidarrquarantine.Service`). + +The handler check `h.tracks` reaches the new service. + +- [ ] **Step 3.3: Register the route in `internal/api/api.go`** + +Inside the `r.Route("/api", ...)` → `authed.Group(...)` → `authed.Route("/admin", ...)` block, add: + +```go +admin.Delete("/tracks/{id}", h.handleRemoveTrack) +``` + +Place it adjacent to the other admin routes (alongside `admin.Post("/quarantine/{track_id}/delete-via-lidarr", ...)` since the operations are conceptually neighboring). + +- [ ] **Step 3.4: Write `internal/api/admin_tracks_test.go`** + +```go +package api_test + +import ( + "net/http" + "net/http/httptest" + "testing" + + "git.fabledsword.com/bvandeusen/minstrel/internal/tracks" +) + +func TestRemoveTrack_NotAdmin_403(t *testing.T) { + srv, _ := newTestServer(t) // existing helper from api_test fixtures + user := seedNonAdminUser(t, srv.pool) + + req := httptest.NewRequest(http.MethodDelete, "/api/admin/tracks/"+randomUUIDString(), nil) + req = withSession(req, user) + rr := httptest.NewRecorder() + srv.handler.ServeHTTP(rr, req) + + if rr.Code != http.StatusForbidden { + t.Errorf("status = %d, want 403", rr.Code) + } +} + +func TestRemoveTrack_NoSession_401(t *testing.T) { + srv, _ := newTestServer(t) + req := httptest.NewRequest(http.MethodDelete, "/api/admin/tracks/"+randomUUIDString(), nil) + rr := httptest.NewRecorder() + srv.handler.ServeHTTP(rr, req) + + if rr.Code != http.StatusUnauthorized { + t.Errorf("status = %d, want 401", rr.Code) + } +} + +func TestRemoveTrack_NotFound_404(t *testing.T) { + srv, _ := newTestServer(t) + admin := seedAdminUser(t, srv.pool) + + req := httptest.NewRequest(http.MethodDelete, "/api/admin/tracks/"+randomUUIDString(), nil) + req = withSession(req, admin) + rr := httptest.NewRecorder() + srv.handler.ServeHTTP(rr, req) + + if rr.Code != http.StatusNotFound { + t.Errorf("status = %d, want 404", rr.Code) + } + if !contains(rr.Body.String(), `"error":"not_found"`) { + t.Errorf("body = %s, want not_found code", rr.Body.String()) + } +} + +func TestRemoveTrack_LidarrError_503(t *testing.T) { + cases := []struct { + name string + lidarr error + code string + status int + }{ + {"unreachable", tracks.ErrLidarrUnreachable, "lidarr_unreachable", http.StatusServiceUnavailable}, + {"unauthorized", tracks.ErrLidarrUnauthorized, "lidarr_unauthorized", http.StatusServiceUnavailable}, + {"server_error", tracks.ErrLidarrServerError, "lidarr_server_error", http.StatusBadGateway}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + srv, _ := newTestServer(t, withLidarrError(tc.lidarr)) // tunable test fixture + admin := seedAdminUser(t, srv.pool) + track := seedLidarrTrack(t, srv.pool, "Song", "mbid") + + req := httptest.NewRequest(http.MethodDelete, "/api/admin/tracks/"+uuidString(track.ID), nil) + req = withSession(req, admin) + rr := httptest.NewRecorder() + srv.handler.ServeHTTP(rr, req) + + if rr.Code != tc.status { + t.Errorf("status = %d, want %d", rr.Code, tc.status) + } + if !contains(rr.Body.String(), `"error":"`+tc.code+`"`) { + t.Errorf("body = %s, want code %s", rr.Body.String(), tc.code) + } + }) + } +} + +func TestRemoveTrack_Happy_200WithCascade(t *testing.T) { + srv, _ := newTestServer(t) + admin := seedAdminUser(t, srv.pool) + + // Set up a single-track album owned by a single-album artist so we + // see the full cascade in the response. + artist := seedArtist(t, srv.pool, "Solo") + album := seedAlbumForArtist(t, srv.pool, "Album", artist.ID) + track := seedTrackInAlbum(t, srv.pool, "Song", album.ID, artist.ID) + + req := httptest.NewRequest(http.MethodDelete, "/api/admin/tracks/"+uuidString(track.ID), nil) + req = withSession(req, admin) + rr := httptest.NewRecorder() + srv.handler.ServeHTTP(rr, req) + + if rr.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rr.Code) + } + body := rr.Body.String() + for _, want := range []string{`"deleted_track_id":`, `"deleted_album_id":`, `"deleted_artist_id":`} { + if !contains(body, want) { + t.Errorf("body missing %s: %s", want, body) + } + } +} +``` + +The helpers (`newTestServer`, `seedAdminUser`, `seedNonAdminUser`, `seedArtist`, etc.) follow the patterns in the existing api_test files. `withLidarrError` is a fixture option this slice introduces — the implementer adds it to `newTestServer` so any test can stub the Lidarr deleter to return a specific typed error. + +- [ ] **Step 3.5: Commit** + +```bash +git add internal/api/admin_tracks.go internal/api/admin_tracks_test.go internal/api/api.go internal/server/server.go +git commit -F - <<'EOF' +feat(api): DELETE /api/admin/tracks/{id} for M7 #372 + +Admin-only handler; maps tracks.RemoveTrack typed errors to the +project's existing wire codes (not_found, lidarr_unreachable, +lidarr_unauthorized, lidarr_server_error). Response envelope returns +deleted_track_id plus optional deleted_album_id / deleted_artist_id +when the cascade tidied up parent rows. +EOF +``` + +--- + +### Task 4 — Frontend admin API helper + +**Files:** +- Create: `web/src/lib/api/admin/tracks.ts` +- Create: `web/src/lib/api/admin/tracks.test.ts` + +- [ ] **Step 4.1: Write `web/src/lib/api/admin/tracks.ts`** + +```ts +import { apiFetch } from '$lib/api/client'; + +export type RemoveTrackResult = { + deleted_track_id: string; + deleted_album_id?: string; + deleted_artist_id?: string; +}; + +/** + * DELETE /api/admin/tracks/{id} — admin-only. Returns the deleted + * track id plus optional album/artist ids when their parent row was + * tidied up by the server-side cascade. The caller is responsible for + * invalidating any TanStack Query keys the deleted entities backed. + */ +export function removeTrack(id: string): Promise { + return apiFetch(`/api/admin/tracks/${encodeURIComponent(id)}`, { + method: 'DELETE', + }); +} +``` + +- [ ] **Step 4.2: Write `web/src/lib/api/admin/tracks.test.ts`** + +```ts +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { removeTrack } from './tracks'; + +const originalFetch = globalThis.fetch; + +beforeEach(() => { + globalThis.fetch = vi.fn(); +}); + +afterEach(() => { + globalThis.fetch = originalFetch; +}); + +describe('removeTrack', () => { + it('DELETEs /api/admin/tracks/:id and parses the envelope', async () => { + (globalThis.fetch as ReturnType).mockResolvedValueOnce( + new Response(JSON.stringify({ + deleted_track_id: 't-1', + deleted_album_id: 'al-1', + }), { status: 200, headers: { 'Content-Type': 'application/json' } }) + ); + + const r = await removeTrack('t-1'); + expect(r.deleted_track_id).toBe('t-1'); + expect(r.deleted_album_id).toBe('al-1'); + expect(r.deleted_artist_id).toBeUndefined(); + + const [url, init] = (globalThis.fetch as ReturnType).mock.calls[0]; + expect(url).toBe('/api/admin/tracks/t-1'); + expect(init.method).toBe('DELETE'); + }); + + it('surfaces lidarr_unreachable as ApiError with that code', async () => { + (globalThis.fetch as ReturnType).mockResolvedValueOnce( + new Response(JSON.stringify({ error: 'lidarr_unreachable' }), { + status: 503, + headers: { 'Content-Type': 'application/json' }, + }) + ); + + await expect(removeTrack('t-1')).rejects.toMatchObject({ + code: 'lidarr_unreachable', + }); + }); +}); +``` + +- [ ] **Step 4.3: Commit** + +```bash +git add web/src/lib/api/admin/tracks.ts web/src/lib/api/admin/tracks.test.ts +git commit -F - <<'EOF' +feat(web): admin removeTrack API helper for M7 #372 + +Single function that hits DELETE /api/admin/tracks/{id} and returns +the typed envelope. Reuses the existing apiFetch + ApiError surface +so error codes (lidarr_unreachable etc.) flow into copyForCode the +same way as every other admin operation. +EOF +``` + +--- + +### Task 5 — Audio store: `playNext()` + +**Files:** +- Modify: `web/src/lib/player/store.svelte.ts` +- Modify: `web/src/lib/player/store.test.ts` + +`enqueueTrack(t)` already exists for "Add to queue" semantics — reuse it as-is. This task adds `playNext(t)` for the "Play next" entry which splices into `_queue[_index + 1]`. + +- [ ] **Step 5.1: Add `playNext` to `web/src/lib/player/store.svelte.ts`** + +Append next to the existing `enqueueTrack` export (around line 208): + +```ts +/** + * M7 #372: insert a track immediately after the currently-playing one. + * If the queue is empty, behaves like enqueueTrack and starts the queue + * with this track at index 0. + */ +export function playNext(t: TrackRef): void { + _radioSeedId = null; // M4c: any user-driven enqueue clears the radio refresh state + if (_queue.length === 0) { + _queue = [t]; + _index = 0; + return; + } + const next = _index + 1; + _queue = [..._queue.slice(0, next), t, ..._queue.slice(next)]; +} +``` + +- [ ] **Step 5.2: Extend `web/src/lib/player/store.test.ts` with playNext cases** + +Append (or wherever the file's existing `enqueueTrack` block lives, place these alongside): + +```ts +import { describe, it, expect, beforeEach } from 'vitest'; +import { playNext, enqueueTrack, getQueue, setQueue, getIndex, setIndex } from './store.svelte'; +// (Adjust imports to match the file's actual export shape; the existing +// store.test.ts will tell you what's exposed for tests today.) + +describe('playNext', () => { + beforeEach(() => { + setQueue([]); + setIndex(0); + }); + + it('splices the track immediately after the current index', () => { + const a = { id: 'a', title: 'A', albumId: 'x', artistId: 'y' } as const; + const b = { id: 'b', title: 'B', albumId: 'x', artistId: 'y' } as const; + const c = { id: 'c', title: 'C', albumId: 'x', artistId: 'y' } as const; + setQueue([a, b]); + setIndex(0); + + playNext(c); + + expect(getQueue().map((t) => t.id)).toEqual(['a', 'c', 'b']); + expect(getIndex()).toBe(0); + }); + + it('seeds an empty queue with the track at index 0', () => { + const t = { id: 't', title: 'T', albumId: 'x', artistId: 'y' } as const; + + playNext(t); + + expect(getQueue().map((x) => x.id)).toEqual(['t']); + expect(getIndex()).toBe(0); + }); +}); +``` + +If the existing store test file uses a different test harness (mounting a Svelte component to reach the runes-state), adapt to whatever pattern is already in place — don't fight the existing setup. The two assertions above are what matters: splice-at-index+1 and empty-queue seeding. + +- [ ] **Step 5.3: Commit** + +```bash +git add web/src/lib/player/store.svelte.ts web/src/lib/player/store.test.ts +git commit -F - <<'EOF' +feat(player): playNext for M7 #372 track-actions menu + +Inserts at _queue[_index + 1] so the next-up slot is overwritten with +the chosen track. Empty-queue seeding mirrors enqueueTrack's behavior. +Clears _radioSeedId since user-driven enqueue invalidates the M4c +auto-refresh trigger. +EOF +``` + +--- + +### Task 6 — `` primitive + +**Files:** +- Create: `web/src/lib/components/TrackMenuItem.svelte` +- Create: `web/src/lib/components/TrackMenuItem.test.ts` + +A single row in the menu: icon + label + optional onclick, with `aria-disabled` support so disabled-but-visible entries (admin-only for non-admin, "Add to playlist…" pre-#352) keep menu height stable. + +- [ ] **Step 6.1: Write `web/src/lib/components/TrackMenuItem.svelte`** + +```svelte + + + +``` + +- [ ] **Step 6.2: Write `web/src/lib/components/TrackMenuItem.test.ts`** + +```ts +import { describe, it, expect, vi } from 'vitest'; +import { render, fireEvent, screen } from '@testing-library/svelte'; +import { Music2 } from 'lucide-svelte'; +import TrackMenuItem from './TrackMenuItem.svelte'; + +describe('TrackMenuItem', () => { + it('renders icon, label, and fires onclick', async () => { + const onclick = vi.fn(); + render(TrackMenuItem, { props: { icon: Music2, label: 'Play next', onclick } }); + const btn = screen.getByRole('menuitem', { name: /play next/i }); + await fireEvent.click(btn); + expect(onclick).toHaveBeenCalledOnce(); + expect(btn.getAttribute('aria-disabled')).toBe('false'); + }); + + it('aria-disabled blocks onclick and reflects in DOM', async () => { + const onclick = vi.fn(); + render(TrackMenuItem, { + props: { icon: Music2, label: 'Add to playlist…', onclick, disabled: true, title: 'Coming with playlists' }, + }); + const btn = screen.getByRole('menuitem', { name: /add to playlist/i }); + expect(btn.getAttribute('aria-disabled')).toBe('true'); + expect(btn.getAttribute('title')).toBe('Coming with playlists'); + await fireEvent.click(btn); + expect(onclick).not.toHaveBeenCalled(); + }); +}); +``` + +- [ ] **Step 6.3: Commit** + +```bash +git add web/src/lib/components/TrackMenuItem.svelte web/src/lib/components/TrackMenuItem.test.ts +git commit -F - <<'EOF' +feat(web): TrackMenuItem primitive for M7 #372 + +Single-entry button used by TrackMenu. Supports aria-disabled (kept +visible to preserve menu height when admin-only or pre-#352 entries +are non-functional) and a `danger` flag for the oxblood-tinted +"Remove from library" entry. +EOF +``` + +--- + +### Task 7 — `` primitive + +**Files:** +- Create: `web/src/lib/components/TrackMenuDivider.svelte` + +Trivial visual separator. No test — it's a styled `
`. + +- [ ] **Step 7.1: Write `web/src/lib/components/TrackMenuDivider.svelte`** + +```svelte + +``` + +- [ ] **Step 7.2: Commit** + +```bash +git add web/src/lib/components/TrackMenuDivider.svelte +git commit -F - <<'EOF' +feat(web): TrackMenuDivider primitive for M7 #372 menu groups +EOF +``` + +--- + +### Task 8 — `` rewrite (multi-entry) + +**Files:** +- Modify (rewrite): `web/src/lib/components/TrackMenu.svelte` +- Modify (rewrite): `web/src/lib/components/TrackMenu.test.ts` + +This is the centerpiece. Existing `track` and `direction` props stay. Internal state grows from the existing `menuOpen` / `popoverOpen` pair to also track confirmation flow for "Remove from library" (a non-modal confirm — the entry is two-click: first click flips it to "Click again to confirm", second click fires the API). + +- [ ] **Step 8.1: Rewrite `web/src/lib/components/TrackMenu.svelte`** + +```svelte + + + (menuOpen = false)} onkeydown={onKeydown} /> + +
+ + + {#if menuOpen} + + {/if} + + {#if popoverOpen} + + {/if} + + {#if toast} +
+ {toast} +
+ {/if} +
+``` + +The imports at the top reference `$lib/stores/me`, `$lib/stores/likedIds`, `$lib/stores/hiddenIds` — verify those exist (they should, given the existing LikeButton + Hide-row pattern). If the names differ in the actual codebase, adjust to match. Same for `$lib/api/quarantine` (`quarantineTrack` / `unquarantineTrack`). Read the files before writing if unsure. + +If any of those stores/helpers genuinely don't exist (e.g., `hiddenIdsStore`), they need to land first or this task gets stretched. Most likely path: the existing LikeButton component already imports a "set of liked ids" store; reuse that pattern. The Hide/quarantine equivalent has shipped in M5b — check `internal/api/quarantine.go` for the helper names and `web/src/lib/api/` for the existing TS surface. + +- [ ] **Step 8.2: Rewrite `web/src/lib/components/TrackMenu.test.ts`** + +```ts +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, fireEvent, screen, waitFor } from '@testing-library/svelte'; +import { writable } from 'svelte/store'; +import TrackMenu from './TrackMenu.svelte'; +import type { TrackRef } from '$lib/api/types'; + +// ---- Mocks ---- + +vi.mock('$lib/stores/me', () => ({ + meStore: writable<{ id: string; username: string; is_admin: boolean } | null>({ + id: 'u-1', username: 'admin', is_admin: true, + }), +})); + +vi.mock('$lib/stores/likedIds', () => ({ + likedIdsStore: writable({ tracks: new Set(), albums: new Set(), artists: new Set() }), +})); + +vi.mock('$lib/stores/hiddenIds', () => ({ + hiddenIdsStore: writable(new Set()), +})); + +vi.mock('$lib/player/store.svelte', () => ({ + playNext: vi.fn(), + enqueueTrack: vi.fn(), +})); + +vi.mock('$lib/api/likes', () => ({ + likeTrack: vi.fn().mockResolvedValue(undefined), + unlikeTrack: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock('$lib/api/quarantine', () => ({ + quarantineTrack: vi.fn().mockResolvedValue(undefined), + unquarantineTrack: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock('$lib/api/admin/tracks', () => ({ + removeTrack: vi.fn().mockResolvedValue({ deleted_track_id: 't-1' }), +})); + +vi.mock('@tanstack/svelte-query', async (orig) => { + const actual = (await orig()) as Record; + return { + ...actual, + useQueryClient: () => ({ invalidateQueries: vi.fn().mockResolvedValue(undefined) }), + }; +}); + +const track: TrackRef = { + id: 't-1', title: 'Roygbiv', + albumId: 'al-1', albumTitle: 'Geogaddi', + artistId: 'art-1', artistName: 'Boards of Canada', + durationMs: 137000, trackNumber: 4, +} as unknown as TrackRef; + +beforeEach(() => { + vi.clearAllMocks(); +}); + +afterEach(() => { + vi.clearAllMocks(); +}); + +describe('TrackMenu', () => { + it('kebab toggles aria-expanded; menu opens with all 9 entries for an admin', async () => { + render(TrackMenu, { props: { track } }); + const kebab = screen.getByRole('button', { name: /track actions/i }); + expect(kebab.getAttribute('aria-expanded')).toBe('false'); + await fireEvent.click(kebab); + expect(kebab.getAttribute('aria-expanded')).toBe('true'); + + expect(screen.getByRole('menuitem', { name: /play next/i })).toBeInTheDocument(); + expect(screen.getByRole('menuitem', { name: /add to queue/i })).toBeInTheDocument(); + expect(screen.getByRole('menuitem', { name: /^like$/i })).toBeInTheDocument(); + expect(screen.getByRole('menuitem', { name: /add to playlist/i })).toBeInTheDocument(); + expect(screen.getByRole('menuitem', { name: /go to album/i })).toBeInTheDocument(); + expect(screen.getByRole('menuitem', { name: /go to artist/i })).toBeInTheDocument(); + expect(screen.getByRole('menuitem', { name: /flag this track/i })).toBeInTheDocument(); + expect(screen.getByRole('menuitem', { name: /^hide$/i })).toBeInTheDocument(); + expect(screen.getByRole('menuitem', { name: /remove from library/i })).toBeInTheDocument(); + }); + + it('hides "Remove from library" for non-admin users', async () => { + const { meStore } = await import('$lib/stores/me'); + (meStore as unknown as { set: (v: unknown) => void }).set({ id: 'u-2', username: 'alice', is_admin: false }); + + render(TrackMenu, { props: { track } }); + await fireEvent.click(screen.getByRole('button', { name: /track actions/i })); + expect(screen.queryByRole('menuitem', { name: /remove from library/i })).not.toBeInTheDocument(); + }); + + it('"Add to playlist…" is disabled with a tooltip until #352 ships', async () => { + render(TrackMenu, { props: { track } }); + await fireEvent.click(screen.getByRole('button', { name: /track actions/i })); + const item = screen.getByRole('menuitem', { name: /add to playlist/i }); + expect(item.getAttribute('aria-disabled')).toBe('true'); + expect(item.getAttribute('title')).toBe('Coming with playlists'); + }); + + it('Play next dispatches to the player store', async () => { + const { playNext } = await import('$lib/player/store.svelte'); + render(TrackMenu, { props: { track } }); + await fireEvent.click(screen.getByRole('button', { name: /track actions/i })); + await fireEvent.click(screen.getByRole('menuitem', { name: /play next/i })); + expect(playNext).toHaveBeenCalledWith(track); + }); + + it('Remove from library is two-click — first click arms, second confirms', async () => { + const { removeTrack } = await import('$lib/api/admin/tracks'); + render(TrackMenu, { props: { track } }); + await fireEvent.click(screen.getByRole('button', { name: /track actions/i })); + + await fireEvent.click(screen.getByRole('menuitem', { name: /remove from library/i })); + expect(screen.getByRole('menuitem', { name: /click again to confirm/i })).toBeInTheDocument(); + expect(removeTrack).not.toHaveBeenCalled(); + + await fireEvent.click(screen.getByRole('menuitem', { name: /click again to confirm/i })); + await waitFor(() => expect(removeTrack).toHaveBeenCalledWith(track.id)); + }); + + it('Escape closes the menu and returns aria-expanded to false', async () => { + render(TrackMenu, { props: { track } }); + const kebab = screen.getByRole('button', { name: /track actions/i }); + await fireEvent.click(kebab); + expect(kebab.getAttribute('aria-expanded')).toBe('true'); + await fireEvent.keyDown(window, { key: 'Escape' }); + await waitFor(() => expect(kebab.getAttribute('aria-expanded')).toBe('false')); + }); +}); +``` + +If `$lib/stores/me`, `$lib/stores/likedIds`, or `$lib/stores/hiddenIds` don't exist by those exact paths, update both the production component (Step 8.1) and the mock here to match the actual paths. The functional shape stays the same. + +- [ ] **Step 8.3: Commit** + +```bash +git add web/src/lib/components/TrackMenu.svelte web/src/lib/components/TrackMenu.test.ts +git commit -F - <<'EOF' +feat(web): TrackMenu rewrite — 9 entries in 4 groups (M7 #372) + +Replaces the M5b-era FlagPopover-only menu with the full track-actions +surface: queue (Play next, Add to queue), collection (Like/Unlike, +Add to playlist… reserved for #352), navigation (Go to album/artist), +lifecycle (Flag, Hide/Unhide, Remove from library — admin-only). + +Remove from library uses a two-click confirm (no modal) so the user +sees the next click is destructive without a dialog interrupting the +flow. The component's prop API (track, direction) is unchanged so +TrackRow + PlayerBar pick up the new entries with no code change. +EOF +``` + +--- + +### Task 9 — Adjust `` and `` test expectations + +**Files:** +- Modify: `web/src/lib/components/TrackRow.test.ts` +- Modify: `web/src/lib/components/PlayerBar.test.ts` + +The component code for TrackRow + PlayerBar doesn't change — both already use `` with the same prop API. But existing tests in those files assert "the menu opens to a single Flag entry" (or similar M5b-era expectations). After Task 8 those assertions are wrong. + +- [ ] **Step 9.1: Open `web/src/lib/components/TrackRow.test.ts`** and find any test that asserts on ``'s open-state contents (search for "Flag this track", "menuitem", "aria-haspopup"). Replace those assertions with one that confirms the kebab is present and `aria-haspopup="menu"` is set: + +```ts +it('renders a track-actions kebab with aria-haspopup="menu"', () => { + render(TrackRow, { props: { track: fakeTrack } }); + const kebab = screen.getByRole('button', { name: /track actions/i }); + expect(kebab.getAttribute('aria-haspopup')).toBe('menu'); +}); +``` + +The detailed menu-content tests live in `TrackMenu.test.ts` (Task 8) — TrackRow's job is just to mount the menu, so its tests should not duplicate the entry-list assertions. + +- [ ] **Step 9.2: Same change in `web/src/lib/components/PlayerBar.test.ts`.** PlayerBar passes `direction="up"` to TrackMenu (drop-up); add an assertion confirming that prop reaches the kebab if the existing test suite covers that surface: + +```ts +it('mounts TrackMenu with drop-up direction', () => { + render(PlayerBar, { props: { /* ...existing setup... */ } }); + const kebab = screen.getByRole('button', { name: /track actions/i }); + expect(kebab).toBeInTheDocument(); + // Direction="up" is an internal CSS class concern; not assertable + // through aria. The existing snapshot or DOM-class check (if any) + // will need a manual update after the menu rewrite. +}); +``` + +- [ ] **Step 9.3: Commit** + +```bash +git add web/src/lib/components/TrackRow.test.ts web/src/lib/components/PlayerBar.test.ts +git commit -F - <<'EOF' +test(web): adjust TrackRow + PlayerBar tests for new TrackMenu shape + +The 9-entry menu rewrite (Task 8) breaks any pre-existing assertion +that the menu opens to a single Flag entry. Strip those — TrackMenu's +own test file covers the entry list end-to-end. TrackRow + PlayerBar +just need to confirm the kebab mounts with aria-haspopup="menu". +EOF +``` + +--- + +## Self-review + +1. **Spec coverage:** + - §2 Goals — all 9 entries land in Task 8; Like/Hide menu duplication shipped; "Add to playlist…" reserved as a disabled slot; "Go to artist" single-target; "Remove from library" admin-only with cascade. ✓ + - §3 Architecture (backend) — Tasks 1, 2, 3 cover SQL queries, RemoveTrack service, HTTP handler. ✓ + - §3 Architecture (frontend) — Task 4 (admin API helper), Task 5 (audio store), Tasks 6-8 (component primitives + TrackMenu rewrite). ✓ + - §3 Accessibility — kebab `aria-haspopup`/`aria-expanded`, `role="menu"`, `role="menuitem"`, `aria-disabled`, Escape-to-close all in Task 8. ✓ + - §4 File map — every file in the plan's File map maps to a task. ✓ + - §5 API contract — Task 3.1 implements the response envelope; Task 3.4 tests the four error codes. ✓ + - §6 Error handling — Task 8 wires `copyForCode()` for toast surface. ✓ + - §7 Testing — every test file from §7 has a writing step. ✓ + - §8 Distribution / migration — confirmed no migration needed (all `track_id` FKs already CASCADE). The "conditional 0014_track_cascades.up.sql" from the spec is dropped here as redundant. ✓ (this is an intentional plan deviation from the spec's optional clause) + +2. **Placeholder scan:** No "TBD"/"TODO"/"add validation" in any task. The phrase "if unsure" appears once in Task 8 ("verify those exist (they should)") which is direction to the implementer, not a placeholder. + +3. **Type consistency:** `TrackRef`, `RemoveTrackResult`, `removeTrack(id)`, `playNext(t)`, `enqueueTrack(t)`, `qk.albums(id)`, `qk.artists(id)`, `qk.home()`, `qk.likedTracks()`, `qk.hiddenTracks()` all consistent across tasks. Backend types: `tracks.Service`, `tracks.RemoveTrack`, `tracks.ErrNotFound`, `tracks.ErrLidarrUnreachable` all consistent. + +4. **One spec deviation noted explicitly:** the spec says migration `0014_track_cascades.up.sql` is "conditional, only if any FK is RESTRICT". I checked — all four `track_id` FKs (`play_events`, `general_likes_tracks`, `lidarr_quarantine`, `lidarr_quarantine_actions`) already use `ON DELETE CASCADE`. No migration is needed; the plan does not include one.