From a90ff11f0f60014362548840c4785b60dbb494f7 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 4 May 2026 09:24:27 -0400 Subject: [PATCH] feat(server/m7-352): kind filter on GET /api/playlists + lazy fallback Adds ?kind= filter (user|system|all, default user) to GET /api/playlists so system mixes don't leak into slice-1 SPA views. Lazy background build fires when kind=system|all and the caller's run row is stale (>24h). Propagates Kind/SystemVariant/SeedArtistID through PlaylistRow and playlistRowView wire types. Co-Authored-By: Claude Sonnet 4.6 --- internal/api/playlists.go | 99 ++++++++++++++++++++++++---- internal/api/playlists_test.go | 115 +++++++++++++++++++++++++++++++++ internal/playlists/service.go | 9 +++ 3 files changed, 212 insertions(+), 11 deletions(-) diff --git a/internal/api/playlists.go b/internal/api/playlists.go index 223f3884..f9b0dd8b 100644 --- a/internal/api/playlists.go +++ b/internal/api/playlists.go @@ -1,17 +1,22 @@ package api import ( + "context" "encoding/json" "errors" "fmt" "net/http" "path/filepath" "strconv" + "strings" + "time" "github.com/go-chi/chi/v5" + "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgtype" "git.fabledsword.com/bvandeusen/minstrel/internal/auth" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/playlists" ) @@ -20,17 +25,20 @@ import ( // derives the cover_url from cover_path so clients don't have to know // the on-disk layout. type playlistRowView struct { - ID string `json:"id"` - UserID string `json:"user_id"` - OwnerUsername string `json:"owner_username"` - Name string `json:"name"` - Description string `json:"description"` - IsPublic bool `json:"is_public"` - CoverURL string `json:"cover_url,omitempty"` - TrackCount int32 `json:"track_count"` - DurationSec int32 `json:"duration_sec"` - CreatedAt string `json:"created_at"` - UpdatedAt string `json:"updated_at"` + ID string `json:"id"` + UserID string `json:"user_id"` + OwnerUsername string `json:"owner_username"` + Name string `json:"name"` + Description string `json:"description"` + IsPublic bool `json:"is_public"` + Kind string `json:"kind"` + SystemVariant *string `json:"system_variant,omitempty"` + SeedArtistID *string `json:"seed_artist_id,omitempty"` + CoverURL string `json:"cover_url,omitempty"` + TrackCount int32 `json:"track_count"` + DurationSec int32 `json:"duration_sec"` + CreatedAt string `json:"created_at"` + UpdatedAt string `json:"updated_at"` } // playlistTrackView is one row of a playlist's track list on the wire. @@ -70,12 +78,39 @@ type listPlaylistsResponse struct { // every playlist visible to the caller (own + others' public); we split // the result here so the wire shape carries the distinction the UI cares // about without a second DB query. +// +// An optional ?kind= query parameter filters the Owned bucket: +// - "user" (default): only user-created playlists +// - "system": only system-generated mixes +// - "all": no filter +// +// The Public bucket (other users' public playlists) is always unfiltered +// since system mixes are always private. func (h *handlers) handleListPlaylists(w http.ResponseWriter, r *http.Request) { caller, ok := auth.UserFromContext(r.Context()) if !ok { writeErr(w, http.StatusUnauthorized, "unauthenticated", "no session") return } + + kind := strings.TrimSpace(r.URL.Query().Get("kind")) + if kind == "" { + kind = "user" + } + switch kind { + case "user", "system", "all": + // valid + default: + writeErr(w, http.StatusBadRequest, "bad_kind", "kind must be one of: user, system, all") + return + } + + // Lazy fallback: when the caller wants system mixes and they're stale, + // kick a background build but serve current state. + if kind == "system" || kind == "all" { + h.maybeKickBackgroundBuild(r.Context(), caller.ID) + } + rows, err := h.playlists.List(r.Context(), caller.ID) if err != nil { h.logger.Error("api: list playlists failed", "err", err) @@ -86,14 +121,50 @@ func (h *handlers) handleListPlaylists(w http.ResponseWriter, r *http.Request) { for i := range rows { v := playlistRowToView(&rows[i]) if uuidEqual(rows[i].UserID, caller.ID) { + // Filter own playlists by the kind selector. + if kind != "all" && rows[i].Kind != kind { + continue + } resp.Owned = append(resp.Owned, v) } else { + // Public-from-others: kind filter doesn't apply (system mixes + // are always private; this slot is for public-share feature). resp.Public = append(resp.Public, v) } } writeJSON(w, http.StatusOK, resp) } +// maybeKickBackgroundBuild fires a background BuildSystemPlaylists when the +// run is stale (>24h) AND not currently in_flight. Best-effort: any error +// fetching the run row is silently swallowed. A real cron tick will catch +// up the user within 24h regardless. +func (h *handlers) maybeKickBackgroundBuild(ctx context.Context, userID pgtype.UUID) { + run, err := dbq.New(h.pool).GetSystemPlaylistRun(ctx, userID) + var stale bool + switch { + case errors.Is(err, pgx.ErrNoRows): + stale = true // no run yet — first request — definitely stale + case err != nil: + h.logger.Warn("lazy system build: get run row failed", "err", err) + return + case !run.LastRunAt.Valid: + stale = true + case run.LastRunAt.Time.Before(time.Now().Add(-24 * time.Hour)): + stale = true + } + if !stale || run.InFlight { + return + } + go func() { + bgCtx := context.Background() + if err := playlists.BuildSystemPlaylists(bgCtx, h.pool, h.logger, userID, time.Now()); err != nil { + h.logger.Warn("lazy system playlist build failed", + "user_id", "redacted", "err", err) + } + }() +} + type createPlaylistBody struct { Name string `json:"name"` Description string `json:"description"` @@ -370,11 +441,17 @@ func playlistRowToView(r *playlists.PlaylistRow) playlistRowView { Name: r.Name, Description: r.Description, IsPublic: r.IsPublic, + Kind: r.Kind, + SystemVariant: r.SystemVariant, TrackCount: r.TrackCount, DurationSec: r.DurationSec, CreatedAt: formatTimestamp(r.CreatedAt), UpdatedAt: formatTimestamp(r.UpdatedAt), } + if r.SeedArtistID.Valid { + s := uuidToString(r.SeedArtistID) + v.SeedArtistID = &s + } if r.CoverPath != nil && *r.CoverPath != "" { v.CoverURL = "/api/playlists/" + v.ID + "/cover" } diff --git a/internal/api/playlists_test.go b/internal/api/playlists_test.go index 1c31237e..3369c244 100644 --- a/internal/api/playlists_test.go +++ b/internal/api/playlists_test.go @@ -454,3 +454,118 @@ func TestPlaylists_CoverNoneReturns404(t *testing.T) { t.Errorf("status = %d, want 404; body = %s", w.Code, w.Body.String()) } } + +// TestListPlaylists_KindFilterDefaultsToUser verifies that an absent ?kind= +// parameter defaults to "user" and excludes system-kind playlists. +func TestListPlaylists_KindFilterDefaultsToUser(t *testing.T) { + h, pool := testHandlers(t) + truncateLibrary(t, pool) + u := seedUser(t, pool, "klistdefault", "pw", false) + + // Create one user-kind playlist via the service. + userPL, err := h.playlists.Create(context.Background(), u.ID, "My PL", "", false) + if err != nil { + t.Fatalf("create user playlist: %v", err) + } + // Insert a system-kind playlist directly via SQL. + if _, err := pool.Exec(context.Background(), ` + INSERT INTO playlists (user_id, name, description, is_public, kind, system_variant) + VALUES ($1, 'For You', '', false, 'system', 'for_you') + `, u.ID); err != nil { + t.Fatalf("seed system playlist: %v", err) + } + + w := doPlaylistsReq(h, u, "GET", "/api/playlists", nil) + if w.Code != http.StatusOK { + t.Fatalf("status: got %d, want 200; body=%s", w.Code, w.Body.String()) + } + var resp listPlaylistsResponse + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v", err) + } + if len(resp.Owned) != 1 { + t.Fatalf("default should return 1 user-kind playlist; got %d", len(resp.Owned)) + } + if resp.Owned[0].Kind != "user" { + t.Errorf("expected kind=user; got %q", resp.Owned[0].Kind) + } + if resp.Owned[0].ID != uuidToString(userPL.ID) { + t.Errorf("returned wrong playlist id") + } +} + +// TestListPlaylists_KindSystem verifies ?kind=system returns only +// system-generated playlists and excludes user-created ones. +func TestListPlaylists_KindSystem(t *testing.T) { + h, pool := testHandlers(t) + truncateLibrary(t, pool) + u := seedUser(t, pool, "klistsys", "pw", false) + + if _, err := h.playlists.Create(context.Background(), u.ID, "User PL", "", false); err != nil { + t.Fatalf("create user playlist: %v", err) + } + if _, err := pool.Exec(context.Background(), ` + INSERT INTO playlists (user_id, name, description, is_public, kind, system_variant) + VALUES ($1, 'For You', '', false, 'system', 'for_you') + `, u.ID); err != nil { + t.Fatalf("seed system playlist: %v", err) + } + + w := doPlaylistsReq(h, u, "GET", "/api/playlists?kind=system", nil) + if w.Code != http.StatusOK { + t.Fatalf("status: got %d, want 200; body=%s", w.Code, w.Body.String()) + } + var resp listPlaylistsResponse + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v", err) + } + if len(resp.Owned) != 1 { + t.Fatalf("kind=system should return 1 system playlist; got %d", len(resp.Owned)) + } + if resp.Owned[0].Kind != "system" { + t.Errorf("expected kind=system; got %q", resp.Owned[0].Kind) + } +} + +// TestListPlaylists_KindAll verifies ?kind=all returns all playlists +// regardless of kind. +func TestListPlaylists_KindAll(t *testing.T) { + h, pool := testHandlers(t) + truncateLibrary(t, pool) + u := seedUser(t, pool, "klistall", "pw", false) + + if _, err := h.playlists.Create(context.Background(), u.ID, "User PL", "", false); err != nil { + t.Fatalf("create user playlist: %v", err) + } + if _, err := pool.Exec(context.Background(), ` + INSERT INTO playlists (user_id, name, description, is_public, kind, system_variant) + VALUES ($1, 'For You', '', false, 'system', 'for_you') + `, u.ID); err != nil { + t.Fatalf("seed system playlist: %v", err) + } + + w := doPlaylistsReq(h, u, "GET", "/api/playlists?kind=all", nil) + if w.Code != http.StatusOK { + t.Fatalf("status: got %d, want 200; body=%s", w.Code, w.Body.String()) + } + var resp listPlaylistsResponse + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v", err) + } + if len(resp.Owned) != 2 { + t.Fatalf("kind=all should return both playlists; got %d", len(resp.Owned)) + } +} + +// TestListPlaylists_BadKindReturns400 verifies an unrecognised kind value +// is rejected with 400 bad_kind. +func TestListPlaylists_BadKindReturns400(t *testing.T) { + h, pool := testHandlers(t) + u := seedUser(t, pool, "klistbad", "pw", false) + _ = pool + + w := doPlaylistsReq(h, u, "GET", "/api/playlists?kind=garbage", nil) + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400; got %d, body=%s", w.Code, w.Body.String()) + } +} diff --git a/internal/playlists/service.go b/internal/playlists/service.go index 32c61676..3c2587f8 100644 --- a/internal/playlists/service.go +++ b/internal/playlists/service.go @@ -65,6 +65,9 @@ type PlaylistRow struct { Name string Description string IsPublic bool + Kind string // 'user' | 'system' + SystemVariant *string // 'for_you' | 'songs_like_artist' | nil + SeedArtistID pgtype.UUID // Valid=true only for 'songs_like_artist' CoverPath *string TrackCount int32 DurationSec int32 @@ -498,6 +501,9 @@ func playlistFromGetRow(r dbq.GetPlaylistRow) *PlaylistRow { Name: r.Name, Description: r.Description, IsPublic: r.IsPublic, + Kind: r.Kind, + SystemVariant: r.SystemVariant, + SeedArtistID: r.SeedArtistID, CoverPath: r.CoverPath, TrackCount: r.TrackCount, DurationSec: r.DurationSec, @@ -514,6 +520,9 @@ func playlistFromListRow(r dbq.ListPlaylistsForUserRow) *PlaylistRow { Name: r.Name, Description: r.Description, IsPublic: r.IsPublic, + Kind: r.Kind, + SystemVariant: r.SystemVariant, + SeedArtistID: r.SeedArtistID, CoverPath: r.CoverPath, TrackCount: r.TrackCount, DurationSec: r.DurationSec,