From 735383fb1de9ea510b5fe8b8a84b42c4d1d1458b Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 7 May 2026 08:14:13 -0400 Subject: [PATCH] feat(server/playlists): generate cover collages for system playlists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit System-generated playlists ("For You", "Songs like X") rendered with empty placeholder boxes in the UI even when their contributing tracks had album art. The build path inserted playlist rows directly without ever invoking the existing playlists.GenerateCollage machinery — that was only wired into user-edited playlist mutations. After this change, BuildSystemPlaylists generates a 4-cell collage for every system playlist it creates, post-commit. Same cover mechanism user playlists use, same on-disk layout (/playlist_covers/.jpg). Cells whose source album lacks art fall back to the FabledSword glyph, so a partially-covered library still produces a sensible visual. Threading: BuildSystemPlaylists, StartSystemPlaylistCron, and the api lazy-build path all gain a dataDir string parameter; main.go passes cfg.Storage.DataDir. Drops the now-redundant PickTopAlbumCoverForArtistByUser lookup — the collage is more visually informative than a single album cover copy, and the seed-artist's top album is one of the contributing tracks in the mix anyway, so it'll appear as one of the four cells. Tests pass t.TempDir() for the dataDir parameter. --- cmd/minstrel/main.go | 2 +- internal/api/playlists.go | 2 +- internal/playlists/system.go | 63 ++++++++++++++++++++++--------- internal/playlists/system_cron.go | 10 ++--- internal/playlists/system_test.go | 18 +++++---- 5 files changed, 62 insertions(+), 33 deletions(-) diff --git a/cmd/minstrel/main.go b/cmd/minstrel/main.go index 9c577762..9bf00598 100644 --- a/cmd/minstrel/main.go +++ b/cmd/minstrel/main.go @@ -178,7 +178,7 @@ func run() error { srv := server.New(logger, pool, scanner, subsonic.Config{ AllowPlaintextPassword: cfg.Subsonic.AllowPlaintextPassword, }, cfg.Events, cfg.Recommendation, cfg.Storage.DataDir, cfg.Branding, coverEnricher, cfg.Library.CoverArtBackfillCap, coverSettings, scanner, scanCfg, scheduler) - playlists.StartSystemPlaylistCron(ctx, pool, logger.With("component", "system_playlist_cron")) + playlists.StartSystemPlaylistCron(ctx, pool, logger.With("component", "system_playlist_cron"), cfg.Storage.DataDir) httpServer := &http.Server{ Addr: cfg.Server.Address, Handler: srv.Router(), diff --git a/internal/api/playlists.go b/internal/api/playlists.go index f7017453..8673f2c3 100644 --- a/internal/api/playlists.go +++ b/internal/api/playlists.go @@ -158,7 +158,7 @@ func (h *handlers) maybeKickBackgroundBuild(ctx context.Context, userID pgtype.U } go func() { bgCtx := context.Background() - if err := playlists.BuildSystemPlaylists(bgCtx, h.pool, h.logger, userID, time.Now()); err != nil { + if err := playlists.BuildSystemPlaylists(bgCtx, h.pool, h.logger, userID, time.Now(), h.dataDir); err != nil { h.logger.Warn("lazy system playlist build failed", "user_id", "redacted", "err", err) } diff --git a/internal/playlists/system.go b/internal/playlists/system.go index 5b5aedb1..5d131586 100644 --- a/internal/playlists/system.go +++ b/internal/playlists/system.go @@ -103,7 +103,7 @@ func noopRNG() float64 { return 0 } // Callable from both the cron loop (system_cron.go) and the lazy fallback. // Returns nil when the user has no library activity to build from. Errors // are captured in system_playlist_runs.last_error before returning. -func BuildSystemPlaylists(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, userID pgtype.UUID, now time.Time) error { +func BuildSystemPlaylists(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, userID pgtype.UUID, now time.Time, dataDir string) error { q := dbq.New(pool) dateStr := now.UTC().Format("2006-01-02") dateOnly := pgtype.Date{Time: now.UTC(), Valid: true} @@ -184,7 +184,6 @@ func BuildSystemPlaylists(ctx context.Context, pool *pgxpool.Pool, logger *slog. type seedMix struct { ArtistID pgtype.UUID ArtistName string - CoverPath *string Tracks []rankedCandidate } seedMixes := make([]seedMix, 0, len(seeds)) @@ -220,13 +219,9 @@ func BuildSystemPlaylists(ctx context.Context, pool *pgxpool.Pool, logger *slog. } } tracks := pickTopN(filtered, dateStr, now, systemMixLength) - coverPath, _ := q.PickTopAlbumCoverForArtistByUser(ctx, dbq.PickTopAlbumCoverForArtistByUserParams{ - UserID: userID, ArtistID: artistID, - }) seedMixes = append(seedMixes, seedMix{ ArtistID: artistID, ArtistName: artistRow.Name, - CoverPath: coverPath, Tracks: tracks, }) } @@ -245,12 +240,20 @@ func BuildSystemPlaylists(ctx context.Context, pool *pgxpool.Pool, logger *slog. return buildErr } + // Track the playlist IDs we create so we can generate collage covers + // for each one after the transaction commits. Collage generation + // reads the playlist's tracks via a separate query that won't see + // uncommitted rows, so the work happens post-commit. + createdIDs := make([]pgtype.UUID, 0, 1+len(seedMixes)) + if len(forYouTracks) > 0 { - if err := insertSystemPlaylist(ctx, qtx, userID, "For You", "for_you", - pgtype.UUID{}, nil, forYouTracks); err != nil { + id, err := insertSystemPlaylist(ctx, qtx, userID, "For You", "for_you", + pgtype.UUID{}, forYouTracks) + if err != nil { buildErr = fmt.Errorf("insert for-you: %w", err) return buildErr } + createdIDs = append(createdIDs, id) } for _, m := range seedMixes { @@ -258,17 +261,35 @@ func BuildSystemPlaylists(ctx context.Context, pool *pgxpool.Pool, logger *slog. continue } name := fmt.Sprintf("Songs like %s", m.ArtistName) - if err := insertSystemPlaylist(ctx, qtx, userID, name, "songs_like_artist", - m.ArtistID, m.CoverPath, m.Tracks); err != nil { + id, err := insertSystemPlaylist(ctx, qtx, userID, name, "songs_like_artist", + m.ArtistID, m.Tracks) + if err != nil { buildErr = fmt.Errorf("insert songs-like %s: %w", uuidStringPL(m.ArtistID), err) return buildErr } + createdIDs = append(createdIDs, id) } if err := tx.Commit(ctx); err != nil { buildErr = fmt.Errorf("commit tx: %w", err) return buildErr } + + // Post-commit: generate a 4-cell collage cover for each system + // playlist from the contributing tracks' album covers. The collage + // gracefully falls back to the FabledSword glyph in cells whose + // album lacks cover art, so a partially-covered library still + // produces a sensible visual rather than an empty placeholder. + // Errors are non-fatal — the playlist is already committed; a + // missing collage just leaves cover_path NULL until next build. + if dataDir != "" { + for _, plID := range createdIDs { + if _, cerr := GenerateCollage(ctx, pool, plID, dataDir); cerr != nil { + logger.Warn("system playlist: collage generation failed", + "playlist_id", uuidStringPL(plID), "err", cerr) + } + } + } return nil } @@ -289,23 +310,29 @@ func pickTopN(cands []recommendation.Candidate, dateStr string, now time.Time, n // insertSystemPlaylist inserts a kind='system' playlists row plus its // playlist_tracks via AppendPlaylistTrack (slice 1's existing query, which -// does the snapshot lookup from tracks/albums/artists). Then recomputes rollups. +// does the snapshot lookup from tracks/albums/artists). Then recomputes +// rollups. Returns the new playlist's ID so the caller can drive +// post-commit collage generation. // // Pass seed_artist_id with Valid=false (zero pgtype.UUID) for the For-You // variant; the CreateSystemPlaylist query persists NULL. +// +// cover_path is always inserted as NULL — the post-commit collage step +// in BuildSystemPlaylists overrides it with a 4-cell composite of the +// contributing tracks' album art. func insertSystemPlaylist(ctx context.Context, qtx *dbq.Queries, userID pgtype.UUID, - name, variant string, seedArtistID pgtype.UUID, coverPath *string, - tracks []rankedCandidate) error { + name, variant string, seedArtistID pgtype.UUID, + tracks []rankedCandidate) (pgtype.UUID, error) { variantStr := variant p, err := qtx.CreateSystemPlaylist(ctx, dbq.CreateSystemPlaylistParams{ UserID: userID, Name: name, SystemVariant: &variantStr, SeedArtistID: seedArtistID, - CoverPath: coverPath, + CoverPath: nil, }) if err != nil { - return fmt.Errorf("insert playlist row: %w", err) + return pgtype.UUID{}, fmt.Errorf("insert playlist row: %w", err) } for _, t := range tracks { @@ -318,14 +345,14 @@ func insertSystemPlaylist(ctx context.Context, qtx *dbq.Queries, userID pgtype.U if errors.Is(err, pgx.ErrNoRows) { continue } - return fmt.Errorf("append track %s: %w", uuidStringPL(t.TrackID), err) + return pgtype.UUID{}, fmt.Errorf("append track %s: %w", uuidStringPL(t.TrackID), err) } } if err := qtx.UpdatePlaylistRollups(ctx, p.ID); err != nil { - return fmt.Errorf("update rollups: %w", err) + return pgtype.UUID{}, fmt.Errorf("update rollups: %w", err) } - return nil + return p.ID, nil } // uuidStringPL renders a pgtype.UUID as the canonical 8-4-4-4-12 form. diff --git a/internal/playlists/system_cron.go b/internal/playlists/system_cron.go index df832130..8ac11426 100644 --- a/internal/playlists/system_cron.go +++ b/internal/playlists/system_cron.go @@ -23,14 +23,14 @@ import ( // // Per-user errors are logged but don't halt the loop. Per-cycle errors (e.g., // "list active users" failed) are logged but the next tick still fires. -func StartSystemPlaylistCron(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger) { +func StartSystemPlaylistCron(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, dataDir string) { go func() { q := dbq.New(pool) if err := q.ClearStaleSystemPlaylistInFlight(ctx); err != nil { logger.Warn("system playlist cron: startup recovery failed", "err", err) } - runOnce(ctx, pool, logger) + runOnce(ctx, pool, logger, dataDir) ticker := time.NewTicker(24 * time.Hour) defer ticker.Stop() @@ -39,13 +39,13 @@ func StartSystemPlaylistCron(ctx context.Context, pool *pgxpool.Pool, logger *sl case <-ctx.Done(): return case <-ticker.C: - runOnce(ctx, pool, logger) + runOnce(ctx, pool, logger, dataDir) } } }() } -func runOnce(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger) { +func runOnce(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, dataDir string) { q := dbq.New(pool) users, err := q.ListActiveUsersForSystemPlaylists(ctx) if err != nil { @@ -53,7 +53,7 @@ func runOnce(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger) { return } for _, userID := range users { - if err := BuildSystemPlaylists(ctx, pool, logger, userID, time.Now()); err != nil { + if err := BuildSystemPlaylists(ctx, pool, logger, userID, time.Now(), dataDir); err != nil { logger.Warn("system playlist build failed in cron", "user_id", uuidStringPL(userID), "err", err) } diff --git a/internal/playlists/system_test.go b/internal/playlists/system_test.go index 0f58ba52..ee67c746 100644 --- a/internal/playlists/system_test.go +++ b/internal/playlists/system_test.go @@ -71,7 +71,7 @@ func TestBuildSystemPlaylists_SufficientActivity(t *testing.T) { pool := newPool(t) logger := discardLogger() u, _ := seedActiveLibrary(t, pool, "act1", 4, 3) - if err := playlists.BuildSystemPlaylists(context.Background(), pool, logger, u.ID, time.Now().UTC()); err != nil { + if err := playlists.BuildSystemPlaylists(context.Background(), pool, logger, u.ID, time.Now().UTC(), t.TempDir()); err != nil { t.Fatalf("build: %v", err) } @@ -131,7 +131,7 @@ func TestBuildSystemPlaylists_QuarantineExcluded(t *testing.T) { quarantined := tracks[0] seedQuarantine(t, pool, u.ID, quarantined.ID) - if err := playlists.BuildSystemPlaylists(context.Background(), pool, logger, u.ID, time.Now().UTC()); err != nil { + if err := playlists.BuildSystemPlaylists(context.Background(), pool, logger, u.ID, time.Now().UTC(), t.TempDir()); err != nil { t.Fatalf("build: %v", err) } @@ -165,7 +165,7 @@ func TestBuildSystemPlaylists_AtomicReplace(t *testing.T) { ctx := context.Background() // First build. - if err := playlists.BuildSystemPlaylists(ctx, pool, logger, u.ID, time.Now().UTC()); err != nil { + if err := playlists.BuildSystemPlaylists(ctx, pool, logger, u.ID, time.Now().UTC(), t.TempDir()); err != nil { t.Fatalf("first build: %v", err) } var ids1 []pgtype.UUID @@ -180,7 +180,7 @@ func TestBuildSystemPlaylists_AtomicReplace(t *testing.T) { } // Second build — old rows must be gone, replaced with fresh ones. - if err := playlists.BuildSystemPlaylists(ctx, pool, logger, u.ID, time.Now().UTC()); err != nil { + if err := playlists.BuildSystemPlaylists(ctx, pool, logger, u.ID, time.Now().UTC(), t.TempDir()); err != nil { t.Fatalf("second build: %v", err) } rows2, _ := dbq.New(pool).ListPlaylistsByUserAndKind(ctx, dbq.ListPlaylistsByUserAndKindParams{ @@ -204,8 +204,9 @@ func TestBuildSystemPlaylists_Concurrency(t *testing.T) { var wg sync.WaitGroup wg.Add(2) - go func() { defer wg.Done(); _ = playlists.BuildSystemPlaylists(ctx, pool, logger, u.ID, now) }() - go func() { defer wg.Done(); _ = playlists.BuildSystemPlaylists(ctx, pool, logger, u.ID, now) }() + dataDir := t.TempDir() + go func() { defer wg.Done(); _ = playlists.BuildSystemPlaylists(ctx, pool, logger, u.ID, now, dataDir) }() + go func() { defer wg.Done(); _ = playlists.BuildSystemPlaylists(ctx, pool, logger, u.ID, now, dataDir) }() wg.Wait() // Exactly one run must have completed; in_flight must be false either way. @@ -228,12 +229,13 @@ func TestBuildSystemPlaylists_DailyNonceDeterminism(t *testing.T) { ctx := context.Background() day1 := time.Date(2026, 5, 4, 12, 0, 0, 0, time.UTC) - if err := playlists.BuildSystemPlaylists(ctx, pool, logger, u.ID, day1); err != nil { + dataDir := t.TempDir() + if err := playlists.BuildSystemPlaylists(ctx, pool, logger, u.ID, day1, dataDir); err != nil { t.Fatalf("build day1 first: %v", err) } snap1 := snapshotSystemTracks(t, pool, u.ID) - if err := playlists.BuildSystemPlaylists(ctx, pool, logger, u.ID, day1); err != nil { + if err := playlists.BuildSystemPlaylists(ctx, pool, logger, u.ID, day1, dataDir); err != nil { t.Fatalf("build day1 second: %v", err) } snap2 := snapshotSystemTracks(t, pool, u.ID)