From 14aa22198f53c32bebcf01d8af64a84c098b1fe8 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 1 Aug 2026 12:45:34 -0400 Subject: [PATCH 01/14] =?UTF-8?q?feat(discover):=20seed=20request=20sugges?= =?UTF-8?q?tions=20from=20the=20taste=20profile=20=E2=80=94=20#2372?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Discover request surface was the one recommendation surface still on its M5c implementation from early May. #796's taste profile, #1488's taste_unheard bucket and #1490's folksonomy enrichment all modernized in-library surfaces; this one was never in scope for any of them, so it still projected raw likes + plays through artist_similarity_unmatched. Two defects fall out of that signal, `5*liked + Σexp(-age/halflife)` summed over every play of the artist. It is unbounded, and contribution is signal × similarity — so a handful of heavily-played artists monopolize all twelve slots, and their share GROWS the more the user listens. The surface entrenched harder the better it knew you, which is exactly backwards and matches the reported "goes stale once it has a strong signal of your taste". It also counted every play_event with no was_skipped filter, so skipping an artist repeatedly INCREASED its signal and pushed more of its neighbours at the user. ListMostPlayedTracksForUser and the taste engine both filter skips; this query was the odd one out. Seeds now come from taste_profile_artists.weight, which the taste engine has already engagement-graded, time-decayed and signed — an artist the user drifted away from stops contributing instead of accumulating forever, and can even contribute negatively. Tiered per rule #131 rather than hard-switched: tier 1 is the profile, tier 2 is likes + completed plays for a user who has no profile rows yet (new account, or before the first daily recompute), so the surface never empties. The old unfiltered-play signal is gone, not kept behind a toggle. The signal is also log-damped, so one artist cannot take every slot even when its weight dwarfs the rest. $2 stays wired to the tier-2 decay: it is genuinely still used there, and dropping the parameter would have changed the generated signature. sqlc's image is not on this workstation and the change preserves the query signature exactly — same three params, same seven columns — so only the embedded SQL const moves. Both copies are edited and verified byte-identical rather than pulling a container onto the operator's machine; a malformed query fails the integration lane loudly, which is the real check either way. Four integration tests cover what changed: a taste weight alone seeds with no like or play; tier 2 does not run alongside tier 1; a non-positive weight never seeds (guarded by a second positive row, so an empty tier 1 can't make it pass for the wrong reason); and skip-only history seeds nothing. Co-Authored-By: Claude Opus 5 (1M context) --- internal/db/dbq/recommendation.sql.go | 58 ++++++- internal/db/queries/recommendation.sql | 65 ++++++-- internal/recommendation/suggestions.go | 23 ++- .../suggestions_integration_test.go | 142 ++++++++++++++++++ 4 files changed, 263 insertions(+), 25 deletions(-) diff --git a/internal/db/dbq/recommendation.sql.go b/internal/db/dbq/recommendation.sql.go index 13167583..cf1bd09b 100644 --- a/internal/db/dbq/recommendation.sql.go +++ b/internal/db/dbq/recommendation.sql.go @@ -1022,20 +1022,66 @@ func (q *Queries) LoadRadioCandidatesV2(ctx context.Context, arg LoadRadioCandid } const suggestArtistsForUser = `-- name: SuggestArtistsForUser :many -WITH seeds AS ( +-- Per-user artist suggestions ranked by taste signal x similarity, projected +-- through artist_similarity_unmatched (out-of-library candidates only). +-- +-- Seeds are TIERED (rule #131) so the surface never empties: +-- tier 1 - taste_profile_artists.weight: engagement-graded, time-decayed and +-- SIGNED by internal/taste, so an artist the user has drifted away +-- from stops contributing instead of accumulating forever. +-- tier 2 - likes + completed plays, used ONLY when the profile has no rows +-- (new account, or before the first daily recompute). +-- +-- The signal is log-damped: contribution is signal x similarity, and the old +-- undamped sum let one heavily-played artist's neighbours take every slot -- +-- entrenching harder the MORE the user listened (issue #2367 mechanism 2). +-- +-- Candidates already in the library, or already requested and not terminal, +-- are excluded. $1=user_id, $2=half_life_days (tier 2 decay), $3=limit. +WITH artist_plays AS ( + -- Completed plays only. The previous seed query counted every play_event, + -- so skipping an artist repeatedly INCREASED its signal and pushed more of + -- its neighbours at the user (issue #2367 mechanism 3). + SELECT t.artist_id, count(*)::bigint AS play_count + FROM play_events pe + JOIN tracks t ON t.id = pe.track_id + WHERE pe.user_id = $1 AND pe.was_skipped = false + GROUP BY t.artist_id +), +profile_seeds AS ( + SELECT tpa.artist_id, tpa.weight AS raw_signal + FROM taste_profile_artists tpa + WHERE tpa.user_id = $1 AND tpa.weight > 0 +), +fallback_seeds AS ( SELECT a.id AS artist_id, 5.0 * (CASE WHEN gla.artist_id IS NOT NULL THEN 1 ELSE 0 END) + COALESCE(SUM(EXP(- EXTRACT(epoch FROM now() - pe.started_at) / ($2::float8 * 86400.0))), 0) - AS signal, - (gla.artist_id IS NOT NULL) AS is_liked, - COUNT(pe.id)::bigint AS play_count + AS raw_signal FROM artists a LEFT JOIN general_likes_artists gla ON gla.artist_id = a.id AND gla.user_id = $1 LEFT JOIN tracks t ON t.artist_id = a.id - LEFT JOIN play_events pe ON pe.track_id = t.id AND pe.user_id = $1 - WHERE gla.artist_id IS NOT NULL OR pe.id IS NOT NULL + LEFT JOIN play_events pe + ON pe.track_id = t.id AND pe.user_id = $1 AND pe.was_skipped = false + WHERE (gla.artist_id IS NOT NULL OR pe.id IS NOT NULL) + AND NOT EXISTS (SELECT 1 FROM profile_seeds) GROUP BY a.id, gla.artist_id ), +seeds AS ( + SELECT s.artist_id, + ln(1.0 + s.raw_signal) AS signal, + (gla.artist_id IS NOT NULL) AS is_liked, + COALESCE(ap.play_count, 0)::bigint AS play_count + FROM ( + SELECT artist_id, raw_signal FROM profile_seeds + UNION ALL + SELECT artist_id, raw_signal FROM fallback_seeds + ) s + LEFT JOIN general_likes_artists gla + ON gla.artist_id = s.artist_id AND gla.user_id = $1 + LEFT JOIN artist_plays ap ON ap.artist_id = s.artist_id + WHERE s.raw_signal > 0 +), contributions AS ( SELECT u.candidate_mbid, u.candidate_name, diff --git a/internal/db/queries/recommendation.sql b/internal/db/queries/recommendation.sql index bf9e10f7..d4c8de68 100644 --- a/internal/db/queries/recommendation.sql +++ b/internal/db/queries/recommendation.sql @@ -258,27 +258,66 @@ ORDER BY started_at DESC LIMIT 1; -- name: SuggestArtistsForUser :many --- M5c: per-user artist suggestions ranked by signal x similarity. The --- seeds CTE collects the user's likes (x5) plus recency-decayed plays --- (exp(-age_days / $2)). The contributions CTE joins those seeds against --- artist_similarity_unmatched and filters out candidates already in --- library or already in a non-terminal lidarr_request. The outer SELECT --- aggregates per candidate, returning the top-3 contributing seeds for --- attribution. $1=user_id, $2=half_life_days, $3=limit. -WITH seeds AS ( +-- Per-user artist suggestions ranked by taste signal x similarity, projected +-- through artist_similarity_unmatched (out-of-library candidates only). +-- +-- Seeds are TIERED (rule #131) so the surface never empties: +-- tier 1 - taste_profile_artists.weight: engagement-graded, time-decayed and +-- SIGNED by internal/taste, so an artist the user has drifted away +-- from stops contributing instead of accumulating forever. +-- tier 2 - likes + completed plays, used ONLY when the profile has no rows +-- (new account, or before the first daily recompute). +-- +-- The signal is log-damped: contribution is signal x similarity, and the old +-- undamped sum let one heavily-played artist's neighbours take every slot -- +-- entrenching harder the MORE the user listened (issue #2367 mechanism 2). +-- +-- Candidates already in the library, or already requested and not terminal, +-- are excluded. $1=user_id, $2=half_life_days (tier 2 decay), $3=limit. +WITH artist_plays AS ( + -- Completed plays only. The previous seed query counted every play_event, + -- so skipping an artist repeatedly INCREASED its signal and pushed more of + -- its neighbours at the user (issue #2367 mechanism 3). + SELECT t.artist_id, count(*)::bigint AS play_count + FROM play_events pe + JOIN tracks t ON t.id = pe.track_id + WHERE pe.user_id = $1 AND pe.was_skipped = false + GROUP BY t.artist_id +), +profile_seeds AS ( + SELECT tpa.artist_id, tpa.weight AS raw_signal + FROM taste_profile_artists tpa + WHERE tpa.user_id = $1 AND tpa.weight > 0 +), +fallback_seeds AS ( SELECT a.id AS artist_id, 5.0 * (CASE WHEN gla.artist_id IS NOT NULL THEN 1 ELSE 0 END) + COALESCE(SUM(EXP(- EXTRACT(epoch FROM now() - pe.started_at) / ($2::float8 * 86400.0))), 0) - AS signal, - (gla.artist_id IS NOT NULL) AS is_liked, - COUNT(pe.id)::bigint AS play_count + AS raw_signal FROM artists a LEFT JOIN general_likes_artists gla ON gla.artist_id = a.id AND gla.user_id = $1 LEFT JOIN tracks t ON t.artist_id = a.id - LEFT JOIN play_events pe ON pe.track_id = t.id AND pe.user_id = $1 - WHERE gla.artist_id IS NOT NULL OR pe.id IS NOT NULL + LEFT JOIN play_events pe + ON pe.track_id = t.id AND pe.user_id = $1 AND pe.was_skipped = false + WHERE (gla.artist_id IS NOT NULL OR pe.id IS NOT NULL) + AND NOT EXISTS (SELECT 1 FROM profile_seeds) GROUP BY a.id, gla.artist_id ), +seeds AS ( + SELECT s.artist_id, + ln(1.0 + s.raw_signal) AS signal, + (gla.artist_id IS NOT NULL) AS is_liked, + COALESCE(ap.play_count, 0)::bigint AS play_count + FROM ( + SELECT artist_id, raw_signal FROM profile_seeds + UNION ALL + SELECT artist_id, raw_signal FROM fallback_seeds + ) s + LEFT JOIN general_likes_artists gla + ON gla.artist_id = s.artist_id AND gla.user_id = $1 + LEFT JOIN artist_plays ap ON ap.artist_id = s.artist_id + WHERE s.raw_signal > 0 +), contributions AS ( SELECT u.candidate_mbid, u.candidate_name, diff --git a/internal/recommendation/suggestions.go b/internal/recommendation/suggestions.go index dfe970e3..fbef853f 100644 --- a/internal/recommendation/suggestions.go +++ b/internal/recommendation/suggestions.go @@ -1,7 +1,14 @@ -// suggestions.go is the M5c per-user artist-suggestion service. Reads -// the user's likes + plays, projects them through artist_similarity_unmatched -// via a single CTE, returns top-N candidates with top-3 attribution seeds -// resolved to artist names. +// suggestions.go is the per-user artist-suggestion service behind the +// Discover request surface. Seeds from the taste profile (falling back to +// likes + completed plays for a user who has none yet), projects those seeds +// through artist_similarity_unmatched via a single CTE, and returns top-N +// out-of-library candidates with top-3 attribution seeds resolved to names. +// +// Originally M5c, which seeded from raw likes + plays. That signal grew +// without bound and counted skips as engagement, so a few heavily-played +// artists monopolized every slot and the surface entrenched harder the more +// the user listened. Reworked to seed from the taste profile in issue #2367; +// see internal/db/queries/recommendation.sql for the tiering. package recommendation import ( @@ -32,8 +39,12 @@ type SeedContribution struct { } // SuggestArtists returns top-N artist suggestions for the user. limit is -// capped at 50 (default 12 when out of range); halfLifeDays is the -// recency-decay half-life for plays (default 30, operator-tunable). +// capped at 50 (default 12 when out of range). +// +// halfLifeDays (default 30) is the recency-decay half-life for the TIER-2 +// seed path only — the likes + completed-plays fallback used while the user +// has no taste-profile rows yet. Once the profile is populated it seeds +// instead, carrying its own decay, so this knob stops applying. func SuggestArtists(ctx context.Context, pool *pgxpool.Pool, userID pgtype.UUID, halfLifeDays float64, limit int) ([]ArtistSuggestion, error) { if limit <= 0 || limit > 50 { limit = 12 diff --git a/internal/recommendation/suggestions_integration_test.go b/internal/recommendation/suggestions_integration_test.go index 9b5cf0c4..67a4cefd 100644 --- a/internal/recommendation/suggestions_integration_test.go +++ b/internal/recommendation/suggestions_integration_test.go @@ -330,3 +330,145 @@ func TestSuggestArtists_EmptyForNewUser(t *testing.T) { t.Errorf("len = %d, want 0 (new user has no signal)", len(out)) } } + +// --- Slice 1 (#2372): taste-profile seeding, tiering, and the skip fix --- + +func setTasteWeight(t *testing.T, pool *pgxpool.Pool, userID, artistID pgtype.UUID, weight float64) { + t.Helper() + if _, err := pool.Exec(context.Background(), + `INSERT INTO taste_profile_artists (user_id, artist_id, weight) VALUES ($1, $2, $3) + ON CONFLICT (user_id, artist_id) DO UPDATE SET weight = EXCLUDED.weight`, + userID, artistID, weight, + ); err != nil { + t.Fatalf("set taste weight: %v", err) + } +} + +func insertSkippedPlayEvent(t *testing.T, pool *pgxpool.Pool, userID, trackID pgtype.UUID, startedAt time.Time) { + t.Helper() + ctx := context.Background() + var sessionID pgtype.UUID + if err := pool.QueryRow(ctx, + `INSERT INTO play_sessions (user_id, started_at, last_event_at, client_id) + VALUES ($1, $2, $2, 'skip-test') RETURNING id`, + userID, startedAt, + ).Scan(&sessionID); err != nil { + t.Fatalf("insert play_session: %v", err) + } + if _, err := pool.Exec(ctx, + `INSERT INTO play_events (user_id, track_id, session_id, started_at, was_skipped) + VALUES ($1, $2, $3, $4, true)`, + userID, trackID, sessionID, startedAt, + ); err != nil { + t.Fatalf("insert skipped play_event: %v", err) + } +} + +// Tier 1: a taste-profile weight alone seeds a suggestion, with no like and no +// play on the seed artist. Before #2367 the profile was ignored entirely here. +func TestSuggestArtists_TasteProfileWeightSeedsTier1(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + seed := seedArtist(t, pool, "Profile Seed", "") + + setTasteWeight(t, pool, user.ID, seed.ID, 4.0) + seedUnmatched(t, pool, seed.ID, "out-mbid", "Outsider", 0.9) + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 1 { + t.Fatalf("len = %d, want 1 (taste weight alone should seed)", len(out)) + } + if out[0].MBID != "out-mbid" { + t.Errorf("mbid = %q, want out-mbid", out[0].MBID) + } + if out[0].Score <= 0 { + t.Errorf("score = %v, want > 0", out[0].Score) + } +} + +// Once the profile has any positive row, tier 2 is not consulted — so an artist +// the user played but that the taste engine did not keep does NOT seed. That is +// the point of the rewrite: the taste engine decides what counts as affinity. +func TestSuggestArtists_TasteProfileSupersedesRawPlays(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + kept := seedArtist(t, pool, "Kept By Taste", "") + dropped := seedArtist(t, pool, "Dropped By Taste", "") + + setTasteWeight(t, pool, user.ID, kept.ID, 3.0) + + // `dropped` has real completed plays but no profile row. + album := seedAlbumForArtist(t, pool, dropped.ID, "Album") + track := seedTrackOnAlbum(t, pool, album.ID, dropped.ID, "Track") + insertPlayEvent(t, pool, user.ID, track.ID, time.Now().Add(-1*time.Hour)) + + seedUnmatched(t, pool, kept.ID, "kept-cand", "Kept Candidate", 0.9) + seedUnmatched(t, pool, dropped.ID, "dropped-cand", "Dropped Candidate", 0.9) + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 1 { + t.Fatalf("len = %d, want 1 (tier 2 must not run alongside tier 1)", len(out)) + } + if out[0].MBID != "kept-cand" { + t.Errorf("mbid = %q, want kept-cand", out[0].MBID) + } +} + +// A non-positive taste weight is not affinity. Guarded by a second, positive +// row so tier 1 stays active — otherwise an empty tier 1 would fall through to +// tier 2 and the assertion would pass for the wrong reason. +func TestSuggestArtists_NonPositiveTasteWeightDoesNotSeed(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + positive := seedArtist(t, pool, "Still Liked", "") + abandoned := seedArtist(t, pool, "Abandoned", "") + + setTasteWeight(t, pool, user.ID, positive.ID, 2.0) + setTasteWeight(t, pool, user.ID, abandoned.ID, -1.5) + + seedUnmatched(t, pool, positive.ID, "pos-cand", "Positive Candidate", 0.9) + seedUnmatched(t, pool, abandoned.ID, "neg-cand", "Negative Candidate", 0.9) + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + for _, s := range out { + if s.MBID == "neg-cand" { + t.Fatalf("negative-weight artist seeded a suggestion: %+v", s) + } + } + if len(out) != 1 || out[0].MBID != "pos-cand" { + t.Errorf("out = %+v, want only pos-cand", out) + } +} + +// Tier 2 counts COMPLETED plays only. Previously every play_event counted, so +// skipping an artist repeatedly increased its signal and pushed more of its +// neighbours at the user (#2367 mechanism 3). +func TestSuggestArtists_SkippedPlaysDoNotSeedTier2(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + skipped := seedArtist(t, pool, "Only Skipped", "") + + album := seedAlbumForArtist(t, pool, skipped.ID, "Album") + track := seedTrackOnAlbum(t, pool, album.ID, skipped.ID, "Track") + for i := 0; i < 5; i++ { + insertSkippedPlayEvent(t, pool, user.ID, track.ID, time.Now().Add(-time.Duration(i+1)*time.Hour)) + } + seedUnmatched(t, pool, skipped.ID, "skip-cand", "Skip Candidate", 0.9) + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 0 { + t.Errorf("len = %d, want 0 (skips are not affinity): %+v", len(out), out) + } +} From b27029f67482f7e48823bd4b88847c7d98ff94cc Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 1 Aug 2026 12:56:42 -0400 Subject: [PATCH 02/14] =?UTF-8?q?feat(discover):=20rotate=20the=20suggesti?= =?UTF-8?q?on=20deck=20daily=20+=20cap=20one=20seed's=20share=20=E2=80=94?= =?UTF-8?q?=20#2373?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second half of the reported symptom: suggestions "show the same artists until you request one". The ranking was `ORDER BY total_score DESC` with no randomization and no seen-state, so the only things that could ever change the deck were a candidate entering the library or the user filing a request. The tail of the ranking was unreachable — requesting was literally the only lever. No SQL change was needed. The query already takes a limit, so it over-fetches a pool (4x the slots, capped at 60) and the selection moves to Go, where it is a pure function of (pool, limit, day) — no DB, no clock — and therefore unit testable in the fast lane instead of behind the integration gate. Three rules. The best few by score always lead, so the strongest matches never rotate out of sight (For You's head/tail shape). The remaining slots are drawn by md5(mbid + day), the same daily-stable idiom the Home rows already use: stable within a day so pull-to-refresh doesn't reshuffle, different tomorrow, and no stored state. And a per-seed cap keeps roughly a quarter of the deck attributable to any one seed artist, so twelve neighbours of a single artist can't be the whole surface. The cap is a preference, not a quota. A user whose pool hangs off one or two seeds would otherwise get a three-card surface — worse than the monoculture being avoided, and exactly the vanish-or-nothing shape rule #131 exists to prevent — so a short deck tops up in score order from what the cap set aside. This is also what keeps the existing Top12Cap integration test honest: its 30 candidates share one seed, and without the top-up it would return 3. Eight unit tests, including one that had to be rewritten mid-change: the first version asserted the cap against an evenly-spread pool, where the top-N is already diverse and the assertion could not fail. It now uses a skewed pool where one seed owns the entire top of the ranking, which is the only shape that actually exercises a cap. Dropped two //nolint:gosec directives added in passing — gosec isn't in .golangci.yml, so they suppressed nothing and only implied a check that runs. Co-Authored-By: Claude Opus 5 (1M context) --- internal/recommendation/suggestions.go | 140 ++++++++++++++- .../recommendation/suggestions_select_test.go | 170 ++++++++++++++++++ 2 files changed, 308 insertions(+), 2 deletions(-) create mode 100644 internal/recommendation/suggestions_select_test.go diff --git a/internal/recommendation/suggestions.go b/internal/recommendation/suggestions.go index fbef853f..1898d35b 100644 --- a/internal/recommendation/suggestions.go +++ b/internal/recommendation/suggestions.go @@ -13,7 +13,11 @@ package recommendation import ( "context" + "crypto/md5" + "encoding/hex" "fmt" + "sort" + "time" "github.com/jackc/pgx/v5/pgtype" "github.com/jackc/pgx/v5/pgxpool" @@ -53,10 +57,14 @@ func SuggestArtists(ctx context.Context, pool *pgxpool.Pool, userID pgtype.UUID, halfLifeDays = 30 } q := dbq.New(pool) + // Over-fetch so there is something to rotate through. A deterministic + // top-N over a score ordering shows the same faces every day until one is + // requested away — the tail of the ranking was unreachable (#2367 + // mechanism 1). Selection from this pool happens in selectSuggestions. rows, err := q.SuggestArtistsForUser(ctx, dbq.SuggestArtistsForUserParams{ UserID: userID, Column2: halfLifeDays, - Limit: int32(limit), + Limit: int32(poolSizeFor(limit)), }) if err != nil { return nil, fmt.Errorf("suggest: query: %w", err) @@ -109,5 +117,133 @@ func SuggestArtists(ctx context.Context, pool *pgxpool.Pool, userID pgtype.UUID, Attribution: attribution, }) } - return out, nil + return selectSuggestions(out, limit, rotationDay(time.Now())), nil +} + +// Pool multiplier: how many scored candidates to fetch per slot shown, so the +// rotation has somewhere to rotate. 4x keeps a day's deck genuinely different +// from yesterday's without pulling the whole long tail (whose scores are noise) +// into every request. +const suggestionPoolFactor = 4 + +// Hard ceiling on the fetched pool. The scored tail past this is low-signal, so +// paying for it would buy churn rather than better suggestions. +const suggestionPoolMax = 60 + +func poolSizeFor(limit int) int { + n := limit * suggestionPoolFactor + if n > suggestionPoolMax { + return suggestionPoolMax + } + return n +} + +// rotationDay is the bucket the daily rotation hashes against. Server-local +// date, matching the `current_date` the Home rows already rotate on, so the +// whole product turns over at the same moment. +func rotationDay(now time.Time) string { + return now.Format("2006-01-02") +} + +// selectSuggestions turns the scored pool into the response. +// +// Pure by design — no DB, no clock — so the rotation and diversity rules are +// unit-testable without Postgres. Callers pass the day bucket in. +// +// Three rules, in order: +// +// 1. Diversity cap. Walking in score order, a candidate is dropped once its +// dominant seed already owns maxPerSeed slots. Nothing capped this before, +// so all twelve slots could be neighbours of one artist and the surface +// read as a single narrow cluster (#2367 mechanism 4). +// 2. Head. The best few by score always lead, so the strongest matches are +// never rotated out of sight. Mirrors For You's head/tail shape. +// 3. Tail. The rest of the slots are drawn from the remaining pool ordered by +// md5(mbid + day) — the same daily-stable idiom the Home rows use. Stable +// within a day, different tomorrow, and it needs no stored state. +func selectSuggestions(pool []ArtistSuggestion, limit int, day string) []ArtistSuggestion { + if len(pool) <= limit { + return pool + } + preferred, overflow := partitionPerSeed(pool, maxPerSeedFor(limit)) + + headCount := limit / 3 + if headCount > len(preferred) { + headCount = len(preferred) + } + out := make([]ArtistSuggestion, 0, limit) + out = append(out, preferred[:headCount]...) + + // Hash once per candidate, not once per comparison. + rest := preferred[headCount:] + tail := make([]rotatable, 0, len(rest)) + for _, s := range rest { + tail = append(tail, rotatable{key: rotationKey(s.MBID, day), suggestion: s}) + } + sort.SliceStable(tail, func(i, j int) bool { return tail[i].key < tail[j].key }) + for _, r := range tail { + if len(out) >= limit { + break + } + out = append(out, r.suggestion) + } + // Diversity is a PREFERENCE, not a quota that may starve the deck. A user + // whose whole pool hangs off one or two seeds would otherwise get a + // three-card surface, which is worse than the monoculture we were avoiding + // (and is the vanish-or-nothing shape rule #131 exists to prevent). Top up + // in score order from what the cap set aside. + for _, s := range overflow { + if len(out) >= limit { + break + } + out = append(out, s) + } + return out +} + +// rotatable pairs a candidate with its precomputed daily rotation key. +type rotatable struct { + key string + suggestion ArtistSuggestion +} + +// maxPerSeedFor keeps roughly a quarter of the deck attributable to any single +// seed artist — enough for a strong affinity to be well represented, not enough +// for it to BE the deck. Floored at 1 so a small limit still returns something. +func maxPerSeedFor(limit int) int { + n := limit / 4 + if n < 1 { + return 1 + } + return n +} + +// partitionPerSeed splits the pool by whether each candidate's dominant +// (highest-contributing) seed still has allowance left. Input must be in score +// order; both outputs preserve it. +// +// Candidates with no attribution are always preferred — there is no seed to +// attribute them to, so they cannot be the cause of a monoculture. +func partitionPerSeed(pool []ArtistSuggestion, maxPerSeed int) (preferred, overflow []ArtistSuggestion) { + perSeed := make(map[pgtype.UUID]int, len(pool)) + preferred = make([]ArtistSuggestion, 0, len(pool)) + for _, s := range pool { + if len(s.Attribution) == 0 { + preferred = append(preferred, s) + continue + } + dominant := s.Attribution[0].ArtistID + if perSeed[dominant] >= maxPerSeed { + overflow = append(overflow, s) + continue + } + perSeed[dominant]++ + preferred = append(preferred, s) + } + return preferred, overflow +} + +func rotationKey(mbid, day string) string { + sum := md5.Sum([]byte(mbid + day)) + return hex.EncodeToString(sum[:]) } diff --git a/internal/recommendation/suggestions_select_test.go b/internal/recommendation/suggestions_select_test.go new file mode 100644 index 00000000..c73ad91c --- /dev/null +++ b/internal/recommendation/suggestions_select_test.go @@ -0,0 +1,170 @@ +package recommendation + +import ( + "fmt" + "testing" + + "github.com/jackc/pgx/v5/pgtype" +) + +// selectSuggestions is deliberately pure (no DB, no clock), so the rotation and +// diversity rules from slice #2373 are covered here in the fast lane rather +// than behind the integration gate. + +func seedID(n byte) pgtype.UUID { + var u pgtype.UUID + u.Bytes[0] = n + u.Valid = true + return u +} + +// candidate builds a pool entry attributed to the given dominant seed. +func candidate(mbid string, score float64, dominant byte) ArtistSuggestion { + return ArtistSuggestion{ + MBID: mbid, + Name: mbid, + Score: score, + Attribution: []SeedContribution{ + {ArtistID: seedID(dominant), Contribution: score}, + }, + } +} + +// poolOf returns n candidates in descending score order, spread across +// `seeds` distinct dominant seeds. +func poolOf(n int, seeds int) []ArtistSuggestion { + out := make([]ArtistSuggestion, 0, n) + for i := 0; i < n; i++ { + out = append(out, candidate(fmt.Sprintf("mbid-%02d", i), 1.0-float64(i)*0.01, byte(i%seeds))) + } + return out +} + +func mbids(in []ArtistSuggestion) []string { + out := make([]string, 0, len(in)) + for _, s := range in { + out = append(out, s.MBID) + } + return out +} + +func TestSelectSuggestions_ReturnsPoolUnchangedWhenNotOverfetched(t *testing.T) { + pool := poolOf(5, 5) + got := selectSuggestions(pool, 12, "2026-08-01") + if len(got) != 5 { + t.Fatalf("len = %d, want 5 (nothing to rotate)", len(got)) + } + if got[0].MBID != "mbid-00" { + t.Errorf("first = %q, want mbid-00", got[0].MBID) + } +} + +func TestSelectSuggestions_FillsTheRequestedLimit(t *testing.T) { + got := selectSuggestions(poolOf(48, 8), 12, "2026-08-01") + if len(got) != 12 { + t.Fatalf("len = %d, want 12", len(got)) + } +} + +// The reported symptom: the deck must change day to day without the user +// requesting anything. +func TestSelectSuggestions_RotatesAcrossDays(t *testing.T) { + pool := poolOf(48, 8) + day1 := mbids(selectSuggestions(pool, 12, "2026-08-01")) + day2 := mbids(selectSuggestions(pool, 12, "2026-08-02")) + + if fmt.Sprint(day1) == fmt.Sprint(day2) { + t.Fatalf("deck identical across days: %v", day1) + } + // ...but stable WITHIN a day, or the surface would reshuffle on every + // pull-to-refresh, which reads as broken rather than fresh. + again := mbids(selectSuggestions(pool, 12, "2026-08-01")) + if fmt.Sprint(day1) != fmt.Sprint(again) { + t.Errorf("same day differed:\n %v\n %v", day1, again) + } +} + +// The strongest matches should never rotate out of sight. +func TestSelectSuggestions_HeadIsStableTopScorers(t *testing.T) { + pool := poolOf(48, 8) + for _, day := range []string{"2026-08-01", "2026-08-02", "2026-09-15"} { + got := selectSuggestions(pool, 12, day) + if got[0].MBID != "mbid-00" { + t.Errorf("day %s: first = %q, want mbid-00 (top score leads)", day, got[0].MBID) + } + } +} + +// #2367 mechanism 4: all twelve slots could be neighbours of one artist. +// +// The pool is deliberately SKEWED — one seed owns the entire top of the score +// ranking — because that is the only shape where a cap can bite. With scores +// spread evenly across seeds the top-N is already diverse and the cap is +// untestable (an earlier version of this test asserted against an even pool and +// could not fail). +func TestSelectSuggestions_CapsOneDominantSeed(t *testing.T) { + const dominantRun = 20 + pool := make([]ArtistSuggestion, 0, 48) + for i := 0; i < dominantRun; i++ { // seed 0 owns the 20 best scores + pool = append(pool, candidate(fmt.Sprintf("dom-%02d", i), 1.0-float64(i)*0.01, 0)) + } + for i := 0; i < 28; i++ { // seeds 1..9 hold everything below + pool = append(pool, candidate(fmt.Sprintf("oth-%02d", i), 0.80-float64(i)*0.01, byte(1+i%9))) + } + + got := selectSuggestions(pool, 12, "2026-08-01") + if len(got) != 12 { + t.Fatalf("len = %d, want 12", len(got)) + } + + perSeed := map[pgtype.UUID]int{} + for _, s := range got { + perSeed[s.Attribution[0].ArtistID]++ + } + // Uncapped, the top 12 by score would be 12 of seed 0's neighbours. + if n := perSeed[seedID(0)]; n > maxPerSeedFor(12) { + t.Errorf("dominant seed owns %d of 12 slots, want <= %d: %v", + n, maxPerSeedFor(12), mbids(got)) + } + if len(perSeed) < 4 { + t.Errorf("deck spans only %d seeds, want >= 4: %v", len(perSeed), mbids(got)) + } +} + +// Diversity must not starve the deck (rule #131): a pool hanging off a single +// seed should still fill, not collapse to maxPerSeed entries. +func TestSelectSuggestions_SingleSeedPoolStillFills(t *testing.T) { + got := selectSuggestions(poolOf(30, 1), 12, "2026-08-01") + if len(got) != 12 { + t.Fatalf("len = %d, want 12 (cap must not starve the deck)", len(got)) + } + if got[0].MBID != "mbid-00" { + t.Errorf("first = %q, want mbid-00", got[0].MBID) + } +} + +// A candidate with no attribution has no seed to blame, so it must never be +// held back by the diversity cap. +func TestPartitionPerSeed_UnattributedNeverCapped(t *testing.T) { + pool := []ArtistSuggestion{ + candidate("a", 0.9, 1), + candidate("b", 0.8, 1), + {MBID: "orphan", Score: 0.1}, + } + preferred, overflow := partitionPerSeed(pool, 1) + if len(preferred) != 2 || preferred[1].MBID != "orphan" { + t.Errorf("preferred = %v, want [a orphan]", mbids(preferred)) + } + if len(overflow) != 1 || overflow[0].MBID != "b" { + t.Errorf("overflow = %v, want [b]", mbids(overflow)) + } +} + +func TestPoolSizeFor_OverfetchesButIsBounded(t *testing.T) { + if got := poolSizeFor(12); got != 48 { + t.Errorf("poolSizeFor(12) = %d, want 48", got) + } + if got := poolSizeFor(50); got != suggestionPoolMax { + t.Errorf("poolSizeFor(50) = %d, want the %d ceiling", got, suggestionPoolMax) + } +} From 94e2cac03b1ece1a52fdf3634ecdd2096de88952 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 1 Aug 2026 22:16:45 -0400 Subject: [PATCH 03/14] =?UTF-8?q?ci(go):=20verify=20committed=20sqlc=20out?= =?UTF-8?q?put=20matches=20its=20.sql=20sources=20=E2=80=94=20#2380?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit internal/db/dbq is 39 files and ~12k lines of generated Go covering 307 queries, and nothing checked that it still matched internal/db/queries. test-go.yml referenced sqlc.yaml only as a path trigger; sqlc never ran. So a hand-edit, a half-applied regen, or a migration changed without a regen would all pass CI while the typed layer quietly lied about the SQL underneath it — which is the single thing adopting sqlc is supposed to buy. This session's slice-1 change is an instance: its SQL const was verified byte-identical against its own .sql source by script, but never against what sqlc would actually emit. Nothing in the repo could have told the difference. make verify-generate runs ahead of vet/lint/test, because if the typed layer disagrees with its sources then everything downstream is testing a lie. generate-go runs sqlc as a Go tool rather than a container: the ci-go image already has Go, so this avoids docker-in-docker on the runner. It's pinned to the same SQLC_VERSION as the existing containerised `generate`, so both routes emit identical output and there is one version to bump — now annotated for Renovate per rule #44. The diff prints BEFORE the exit-code check on purpose. On failure the log then holds sqlc's exact expected output, so correcting it is a copy rather than a guess. That is also what makes new queries workable without installing anything: this workstation has neither Go nor sqlc. Makefile joins the workflow's paths:. Without it a Makefile-only change — including this one — would not trigger the workflow that now depends on it. Same class as #2204, where CI never ran on plugin/** changes. Landing this on its own, ahead of slice 3, so that if it fails it is unambiguous whether the drift came from slice 1 or from new code. Co-Authored-By: Claude Opus 5 (1M context) --- .gitea/workflows/test-go.yml | 4 ++++ Makefile | 22 +++++++++++++++++++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/.gitea/workflows/test-go.yml b/.gitea/workflows/test-go.yml index 0eae539f..b3eb990f 100644 --- a/.gitea/workflows/test-go.yml +++ b/.gitea/workflows/test-go.yml @@ -27,6 +27,7 @@ on: - 'go.mod' - 'go.sum' - 'sqlc.yaml' + - 'Makefile' - 'internal/**' - 'cmd/**' - '.golangci.yml' @@ -53,6 +54,9 @@ jobs: go version golangci-lint --version + - name: Generated code matches queries (sqlc) + run: make verify-generate + - name: go vet run: go vet ./... diff --git a/Makefile b/Makefile index fa1e2e04..69fca217 100644 --- a/Makefile +++ b/Makefile @@ -1,10 +1,30 @@ -.PHONY: generate test test-short test-integration lint build +.PHONY: generate generate-go verify-generate test test-short test-integration lint build +# renovate: datasource=docker depName=sqlc/sqlc SQLC_VERSION := 1.31.1 +# Local codegen. Containerised so a dev needs no sqlc install. generate: docker run --rm -v "$(CURDIR):/src" -w /src sqlc/sqlc:$(SQLC_VERSION) generate +# Same codegen, run as a Go tool instead of a container. This is the CI path: +# the ci-go image already has Go, so it avoids docker-in-docker. Pinned to the +# SAME version as `generate` above so both routes emit identical output. +generate-go: + go run github.com/sqlc-dev/sqlc/cmd/sqlc@v$(SQLC_VERSION) generate + +# Fail if the committed generated code no longer matches the .sql sources. +# +# Nothing verified this before, so internal/db/dbq could silently drift from +# internal/db/queries — a hand-edit, a half-applied regen, or a schema change +# without a regen would all pass CI while the typed layer lied about the SQL. +# +# The diff is printed BEFORE the exit-code check on purpose: when this fails, +# the log then contains sqlc's exact expected output, which is what you commit. +verify-generate: generate-go + git --no-pager diff -- internal/db/dbq + git diff --quiet -- internal/db/dbq + test: go test -race ./... From e006de5d4bcc73f7116af6f0b2f4b63b7c56861d Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 1 Aug 2026 22:23:35 -0400 Subject: [PATCH 04/14] =?UTF-8?q?fix(db):=20apply=20sqlc's=20actual=20outp?= =?UTF-8?q?ut=20for=20SuggestArtistsForUser=20=E2=80=94=20#2380?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new codegen check failed on its first run, against the slice-1 hand-edit, which is precisely why it landed on its own commit. What I got wrong: sqlc does not embed the leading `--` header block in the SQL const. It strips those lines and promotes them to the generated method's Go doc comment, gofmt-formatted — blank `//` separators around the indented list, tabs for the indent. My hand-edit left the header inside the string AND left the stale M5c doc comment sitting on the function, so the generated file described behaviour the query no longer had. Comments *inside* the statement body are kept as-is; only the header block moves. Worth knowing before slices 5 and 6 add more queries. Taken verbatim from the diff the check printed, which is the reason it prints before asserting. Round-trip cost: one CI run, no guessing. Note the integration lane passed on the previous push even with the wrong generated file — the SQL text was valid and the signature was unchanged, so executing it against real Postgres proved nothing about whether the committed Go matched its source. That gap is exactly what #2380 closes. Co-Authored-By: Claude Opus 5 (1M context) --- internal/db/dbq/recommendation.sql.go | 40 ++++++++++++--------------- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/internal/db/dbq/recommendation.sql.go b/internal/db/dbq/recommendation.sql.go index cf1bd09b..443fc375 100644 --- a/internal/db/dbq/recommendation.sql.go +++ b/internal/db/dbq/recommendation.sql.go @@ -1022,22 +1022,6 @@ func (q *Queries) LoadRadioCandidatesV2(ctx context.Context, arg LoadRadioCandid } const suggestArtistsForUser = `-- name: SuggestArtistsForUser :many --- Per-user artist suggestions ranked by taste signal x similarity, projected --- through artist_similarity_unmatched (out-of-library candidates only). --- --- Seeds are TIERED (rule #131) so the surface never empties: --- tier 1 - taste_profile_artists.weight: engagement-graded, time-decayed and --- SIGNED by internal/taste, so an artist the user has drifted away --- from stops contributing instead of accumulating forever. --- tier 2 - likes + completed plays, used ONLY when the profile has no rows --- (new account, or before the first daily recompute). --- --- The signal is log-damped: contribution is signal x similarity, and the old --- undamped sum let one heavily-played artist's neighbours take every slot -- --- entrenching harder the MORE the user listened (issue #2367 mechanism 2). --- --- Candidates already in the library, or already requested and not terminal, --- are excluded. $1=user_id, $2=half_life_days (tier 2 decay), $3=limit. WITH artist_plays AS ( -- Completed plays only. The previous seed query counted every play_event, -- so skipping an artist repeatedly INCREASED its signal and pushed more of @@ -1128,13 +1112,23 @@ type SuggestArtistsForUserRow struct { TopPlayCounts []int64 } -// M5c: per-user artist suggestions ranked by signal x similarity. The -// seeds CTE collects the user's likes (x5) plus recency-decayed plays -// (exp(-age_days / $2)). The contributions CTE joins those seeds against -// artist_similarity_unmatched and filters out candidates already in -// library or already in a non-terminal lidarr_request. The outer SELECT -// aggregates per candidate, returning the top-3 contributing seeds for -// attribution. $1=user_id, $2=half_life_days, $3=limit. +// Per-user artist suggestions ranked by taste signal x similarity, projected +// through artist_similarity_unmatched (out-of-library candidates only). +// +// Seeds are TIERED (rule #131) so the surface never empties: +// +// tier 1 - taste_profile_artists.weight: engagement-graded, time-decayed and +// SIGNED by internal/taste, so an artist the user has drifted away +// from stops contributing instead of accumulating forever. +// tier 2 - likes + completed plays, used ONLY when the profile has no rows +// (new account, or before the first daily recompute). +// +// The signal is log-damped: contribution is signal x similarity, and the old +// undamped sum let one heavily-played artist's neighbours take every slot -- +// entrenching harder the MORE the user listened (issue #2367 mechanism 2). +// +// Candidates already in the library, or already requested and not terminal, +// are excluded. $1=user_id, $2=half_life_days (tier 2 decay), $3=limit. func (q *Queries) SuggestArtistsForUser(ctx context.Context, arg SuggestArtistsForUserParams) ([]SuggestArtistsForUserRow, error) { rows, err := q.db.Query(ctx, suggestArtistsForUser, arg.UserID, arg.Column2, arg.Limit) if err != nil { From 86af79bd2f7244b81c8eeb04a96b0d83f364d64c Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 1 Aug 2026 22:51:51 -0400 Subject: [PATCH 05/14] =?UTF-8?q?feat(discover):=20time-boxed=20suggestion?= =?UTF-8?q?=20snooze,=20server=20side=20=E2=80=94=20#2374?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Migration 0049 adds suggestion_snoozes(user_id, candidate_mbid, candidate_name, snoozed_until), and SuggestArtistsForUser excludes rows whose snooze hasn't expired. This is NOT a dislike. Rule #101 forbids a "Not for me" / thumbs-down UI; a snooze is the approved shape instead because it records no verdict on the music, expires on its own (~90d), and never reaches the taste profile. It's acquisition triage — "not right now" — so the filter sits at the candidate stage rather than in the score, where it would become a ranking signal by the back door. Per-user throughout (rule #47): one household member parking a candidate leaves everyone else's deck untouched. candidate_name is denormalized because suggestions are out-of-library by definition — there is no artists row to resolve a display name from, and the un-snooze list has to show something. That list is why GET /discover/snoozes exists at all: a parked candidate is by definition absent from the deck, so without it the DELETE would be unreachable. Also fixes a hole in the codegen check from #2380: `git diff` ignores untracked paths, so a brand-new generated file would have passed it silently. `git add -N` first. This commit is the first to add one. Endpoints: POST /api/discover/suggestions/{mbid}/snooze (body: name, days) DELETE /api/discover/suggestions/{mbid}/snooze GET /api/discover/snoozes UI lands in slice 4 (#2375) before any of this merges — rule #27. Co-Authored-By: Claude Opus 5 (1M context) --- Makefile | 4 + internal/api/api.go | 5 + internal/api/suggestions.go | 150 ++++++++++++ internal/db/dbq/models.go | 8 + internal/db/dbq/recommendation.sql.go | 17 +- internal/db/dbq/suggestion_snoozes.sql.go | 126 +++++++++++ .../0049_suggestion_snoozes.down.sql | 2 + .../migrations/0049_suggestion_snoozes.up.sql | 33 +++ internal/db/queries/recommendation.sql | 17 +- internal/db/queries/suggestion_snoozes.sql | 39 ++++ internal/dbtest/reset.go | 6 + internal/gc/worker.go | 2 + .../suggestions_integration_test.go | 214 ++++++++++++++++++ 13 files changed, 619 insertions(+), 4 deletions(-) create mode 100644 internal/db/dbq/suggestion_snoozes.sql.go create mode 100644 internal/db/migrations/0049_suggestion_snoozes.down.sql create mode 100644 internal/db/migrations/0049_suggestion_snoozes.up.sql create mode 100644 internal/db/queries/suggestion_snoozes.sql diff --git a/Makefile b/Makefile index 69fca217..7b5f63c0 100644 --- a/Makefile +++ b/Makefile @@ -22,6 +22,10 @@ generate-go: # The diff is printed BEFORE the exit-code check on purpose: when this fails, # the log then contains sqlc's exact expected output, which is what you commit. verify-generate: generate-go + # -N (intent-to-add) so a BRAND-NEW generated file is visible to `git + # diff`, which otherwise ignores untracked paths entirely — a whole + # missing *.sql.go would sail through the check below. + git add -N -- internal/db/dbq git --no-pager diff -- internal/db/dbq git diff --quiet -- internal/db/dbq diff --git a/internal/api/api.go b/internal/api/api.go index a16f8f23..7e04e4c8 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -104,6 +104,11 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev authed.Get("/search", h.handleSearch) authed.Get("/radio", h.handleRadio) authed.Get("/discover/suggestions", h.handleListSuggestions) + // Snooze = "not right now", time-boxed and self-expiring + // (#2374). Not a dislike — see the migration for why. + authed.Post("/discover/suggestions/{mbid}/snooze", h.handleSnoozeSuggestion) + authed.Delete("/discover/suggestions/{mbid}/snooze", h.handleUnsnoozeSuggestion) + authed.Get("/discover/snoozes", h.handleListSuggestionSnoozes) authed.Get("/home", h.handleGetHome) authed.Get("/home/index", h.handleGetHomeIndex) authed.Post("/events", h.handleEvents) diff --git a/internal/api/suggestions.go b/internal/api/suggestions.go index 1ad5bf7f..da3bd18c 100644 --- a/internal/api/suggestions.go +++ b/internal/api/suggestions.go @@ -2,13 +2,19 @@ package api import ( "context" + "encoding/json" + "errors" + "io" "net/http" "strconv" + "strings" "sync" + "github.com/go-chi/chi/v5" "github.com/jackc/pgx/v5/pgtype" "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/lidarr" "git.fabledsword.com/bvandeusen/minstrel/internal/recommendation" ) @@ -92,6 +98,150 @@ func (h *handlers) handleListSuggestions(w http.ResponseWriter, r *http.Request) writeJSON(w, http.StatusOK, out) } +// Snooze duration bounds. 90 days is long enough that a parked suggestion +// stops feeling like it's nagging, short enough that a taste shift brings it +// back on its own — the whole point of a snooze over a dismissal (#2374). +const ( + defaultSnoozeDays = 90.0 + maxSnoozeDays = 365.0 +) + +// snoozeRequest is the POST body. Both fields are optional in the JSON sense +// (an absent body snoozes for the default), but Name is required in practice: +// candidates are out-of-library, so the server has no artists row to resolve a +// display name from and the un-snooze list would have nothing to show. The +// client always has it — it just rendered the card. +type snoozeRequest struct { + Name string `json:"name"` + Days float64 `json:"days"` +} + +// snoozeView is one row of GET /api/discover/snoozes. +type snoozeView struct { + MBID string `json:"mbid"` + Name string `json:"name"` + SnoozedUntil pgtype.Timestamptz `json:"snoozed_until"` + CreatedAt pgtype.Timestamptz `json:"created_at"` +} + +// handleSnoozeSuggestion implements +// POST /api/discover/suggestions/{mbid}/snooze. +// +// Parks a candidate for `days` (default 90, capped at 365). Idempotent: +// snoozing an already-snoozed candidate extends it rather than conflicting. +// +// This is NOT negative feedback. It records no verdict on the artist and is +// never read by internal/taste — see 0049_suggestion_snoozes.up.sql for the +// rule #101 reasoning. Returns 204. +func (h *handlers) handleSnoozeSuggestion(w http.ResponseWriter, r *http.Request) { + user, ok := requireUser(w, r) + if !ok { + return + } + mbid := strings.TrimSpace(chi.URLParam(r, "mbid")) + if mbid == "" { + writeErr(w, apierror.BadRequest("invalid_id", "missing mbid")) + return + } + + // An empty body is a valid "snooze this for the default period", so EOF + // is not an error here — decodeBody would reject it as a malformed body. + var body snoozeRequest + if err := json.NewDecoder(r.Body).Decode(&body); err != nil && !errors.Is(err, io.EOF) { + writeErr(w, apierror.BadRequest("invalid_body", "")) + return + } + name := strings.TrimSpace(body.Name) + if name == "" { + writeErr(w, apierror.BadRequest("invalid_body", "name is required")) + return + } + days := body.Days + if days <= 0 { + days = defaultSnoozeDays + } + if days > maxSnoozeDays { + // Clamp rather than reject: a client asking for longer than we allow + // still means "park this", and failing the write would leave the card + // sitting there as if the tap did nothing. + days = maxSnoozeDays + } + + q := dbq.New(h.pool) + if err := q.SnoozeSuggestion(r.Context(), dbq.SnoozeSuggestionParams{ + UserID: user.ID, + CandidateMbid: mbid, + CandidateName: name, + Column4: days, + }); err != nil { + h.logger.Error("api: snooze suggestion", "err", err) + writeErr(w, apierror.InternalMsg("failed to snooze suggestion", err)) + return + } + w.WriteHeader(http.StatusNoContent) +} + +// handleUnsnoozeSuggestion implements +// DELETE /api/discover/suggestions/{mbid}/snooze. +// +// Brings a parked candidate back immediately. 404s an MBID this user never +// snoozed, so the client can tell "undone" from "there was nothing there". +func (h *handlers) handleUnsnoozeSuggestion(w http.ResponseWriter, r *http.Request) { + user, ok := requireUser(w, r) + if !ok { + return + } + mbid := strings.TrimSpace(chi.URLParam(r, "mbid")) + if mbid == "" { + writeErr(w, apierror.BadRequest("invalid_id", "missing mbid")) + return + } + q := dbq.New(h.pool) + rows, err := q.UnsnoozeSuggestion(r.Context(), dbq.UnsnoozeSuggestionParams{ + UserID: user.ID, + CandidateMbid: mbid, + }) + if err != nil { + h.logger.Error("api: unsnooze suggestion", "err", err) + writeErr(w, apierror.InternalMsg("failed to unsnooze suggestion", err)) + return + } + if rows == 0 { + writeErr(w, apierror.NotFound("snooze")) + return + } + w.WriteHeader(http.StatusNoContent) +} + +// handleListSuggestionSnoozes implements GET /api/discover/snoozes. +// +// The un-snooze surface needs this: a parked candidate is by definition +// absent from the suggestion deck, so without a list there is no way to +// reach the DELETE above. Scoped to the caller (rule #47). Expired rows are +// already filtered by the query — the hourly gc sweep only reclaims space. +func (h *handlers) handleListSuggestionSnoozes(w http.ResponseWriter, r *http.Request) { + user, ok := requireUser(w, r) + if !ok { + return + } + rows, err := dbq.New(h.pool).ListActiveSuggestionSnoozes(r.Context(), user.ID) + if err != nil { + h.logger.Error("api: list suggestion snoozes", "err", err) + writeErr(w, apierror.InternalMsg("failed to load snoozes", err)) + return + } + out := make([]snoozeView, 0, len(rows)) + for _, row := range rows { + out = append(out, snoozeView{ + MBID: row.CandidateMbid, + Name: row.CandidateName, + SnoozedUntil: row.SnoozedUntil, + CreatedAt: row.CreatedAt, + }) + } + writeJSON(w, http.StatusOK, out) +} + // resolveSuggestionArt fills ImageURL on-demand from Lidarr's artist // lookup, matched by MBID (foreignArtistId). Best-effort and cache-free: // Lidarr is the only source — when it's disabled, unreachable, or has diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index 10e990ef..cc61f970 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -518,6 +518,14 @@ type SmtpConfig struct { UpdatedAt pgtype.Timestamptz } +type SuggestionSnooze struct { + UserID pgtype.UUID + CandidateMbid string + CandidateName string + SnoozedUntil pgtype.Timestamptz + CreatedAt pgtype.Timestamptz +} + type SystemPlaylistRotationState struct { UserID pgtype.UUID PlaylistKind string diff --git a/internal/db/dbq/recommendation.sql.go b/internal/db/dbq/recommendation.sql.go index 443fc375..d6ed888c 100644 --- a/internal/db/dbq/recommendation.sql.go +++ b/internal/db/dbq/recommendation.sql.go @@ -1082,6 +1082,18 @@ contributions AS ( AND r.lidarr_artist_mbid = u.candidate_mbid AND r.status NOT IN ('rejected', 'failed') ) + -- Snoozed by this user and not yet expired (#2374). Time-boxed and + -- per-user: the candidate returns on its own once snoozed_until + -- passes, and stays visible to everyone else meanwhile. Deliberately + -- filtered at the candidate stage, NOT folded into the score — a + -- snooze carries no opinion about the music, so it must not become a + -- ranking signal. + AND NOT EXISTS ( + SELECT 1 FROM suggestion_snoozes s + WHERE s.user_id = $1 + AND s.candidate_mbid = u.candidate_mbid + AND s.snoozed_until > now() + ) ) SELECT candidate_mbid, candidate_name, @@ -1127,8 +1139,9 @@ type SuggestArtistsForUserRow struct { // undamped sum let one heavily-played artist's neighbours take every slot -- // entrenching harder the MORE the user listened (issue #2367 mechanism 2). // -// Candidates already in the library, or already requested and not terminal, -// are excluded. $1=user_id, $2=half_life_days (tier 2 decay), $3=limit. +// Candidates already in the library, already requested and not terminal, or +// snoozed by this user, are excluded. $1=user_id, $2=half_life_days (tier 2 +// decay), $3=limit. func (q *Queries) SuggestArtistsForUser(ctx context.Context, arg SuggestArtistsForUserParams) ([]SuggestArtistsForUserRow, error) { rows, err := q.db.Query(ctx, suggestArtistsForUser, arg.UserID, arg.Column2, arg.Limit) if err != nil { diff --git a/internal/db/dbq/suggestion_snoozes.sql.go b/internal/db/dbq/suggestion_snoozes.sql.go new file mode 100644 index 00000000..f1642a34 --- /dev/null +++ b/internal/db/dbq/suggestion_snoozes.sql.go @@ -0,0 +1,126 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: suggestion_snoozes.sql + +package dbq + +import ( + "context" + + "github.com/jackc/pgx/v5/pgtype" +) + +const gcDeleteExpiredSuggestionSnoozes = `-- name: GcDeleteExpiredSuggestionSnoozes :execrows +DELETE FROM suggestion_snoozes WHERE snoozed_until < now() +` + +// Keeps the table from growing without bound. Every read already filters on +// snoozed_until > now(), so deleting an expired row changes no behaviour — +// this is purely reclamation. +func (q *Queries) GcDeleteExpiredSuggestionSnoozes(ctx context.Context) (int64, error) { + result, err := q.db.Exec(ctx, gcDeleteExpiredSuggestionSnoozes) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + +const listActiveSuggestionSnoozes = `-- name: ListActiveSuggestionSnoozes :many +SELECT candidate_mbid, candidate_name, snoozed_until, created_at + FROM suggestion_snoozes + WHERE user_id = $1 AND snoozed_until > now() + ORDER BY snoozed_until, candidate_mbid +` + +type ListActiveSuggestionSnoozesRow struct { + CandidateMbid string + CandidateName string + SnoozedUntil pgtype.Timestamptz + CreatedAt pgtype.Timestamptz +} + +// Backs the manage / un-snooze surface. Expired rows are filtered HERE +// rather than left to the sweeper: gc runs on an hourly tick, so a row can +// outlive its expiry by up to a tick and must not read as still-snoozed in +// the meantime. +func (q *Queries) ListActiveSuggestionSnoozes(ctx context.Context, userID pgtype.UUID) ([]ListActiveSuggestionSnoozesRow, error) { + rows, err := q.db.Query(ctx, listActiveSuggestionSnoozes, userID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListActiveSuggestionSnoozesRow + for rows.Next() { + var i ListActiveSuggestionSnoozesRow + if err := rows.Scan( + &i.CandidateMbid, + &i.CandidateName, + &i.SnoozedUntil, + &i.CreatedAt, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const snoozeSuggestion = `-- name: SnoozeSuggestion :exec + +INSERT INTO suggestion_snoozes (user_id, candidate_mbid, candidate_name, snoozed_until) +VALUES ($1, $2, $3, now() + ($4::float8 * INTERVAL '1 day')) +ON CONFLICT (user_id, candidate_mbid) DO UPDATE + SET snoozed_until = EXCLUDED.snoozed_until, + candidate_name = EXCLUDED.candidate_name +` + +type SnoozeSuggestionParams struct { + UserID pgtype.UUID + CandidateMbid string + CandidateName string + Column4 float64 +} + +// Time-boxed "not right now" on a Discover artist suggestion (#2374). +// +// See 0049_suggestion_snoozes.up.sql for why this is a snooze and not a +// dismissal: it records no verdict on the music, expires on its own, and +// must never reach the taste profile. Nothing in internal/taste may read +// this table. +// Upsert, so re-snoozing an already-snoozed candidate EXTENDS it instead of +// erroring on the PK. The name is refreshed too — a later suggestion may +// carry a corrected spelling from the similarity feed. +// $1=user_id, $2=candidate_mbid, $3=candidate_name, $4=duration in days. +func (q *Queries) SnoozeSuggestion(ctx context.Context, arg SnoozeSuggestionParams) error { + _, err := q.db.Exec(ctx, snoozeSuggestion, + arg.UserID, + arg.CandidateMbid, + arg.CandidateName, + arg.Column4, + ) + return err +} + +const unsnoozeSuggestion = `-- name: UnsnoozeSuggestion :execrows +DELETE FROM suggestion_snoozes + WHERE user_id = $1 AND candidate_mbid = $2 +` + +type UnsnoozeSuggestionParams struct { + UserID pgtype.UUID + CandidateMbid string +} + +// Row count is returned so the handler can 404 an MBID that was never +// snoozed rather than reporting success for a no-op. +func (q *Queries) UnsnoozeSuggestion(ctx context.Context, arg UnsnoozeSuggestionParams) (int64, error) { + result, err := q.db.Exec(ctx, unsnoozeSuggestion, arg.UserID, arg.CandidateMbid) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} diff --git a/internal/db/migrations/0049_suggestion_snoozes.down.sql b/internal/db/migrations/0049_suggestion_snoozes.down.sql new file mode 100644 index 00000000..05f6770d --- /dev/null +++ b/internal/db/migrations/0049_suggestion_snoozes.down.sql @@ -0,0 +1,2 @@ +DROP INDEX IF EXISTS suggestion_snoozes_expiry_idx; +DROP TABLE IF EXISTS suggestion_snoozes; diff --git a/internal/db/migrations/0049_suggestion_snoozes.up.sql b/internal/db/migrations/0049_suggestion_snoozes.up.sql new file mode 100644 index 00000000..02b0907f --- /dev/null +++ b/internal/db/migrations/0049_suggestion_snoozes.up.sql @@ -0,0 +1,33 @@ +-- 0049_suggestion_snoozes.up.sql — time-boxed "not right now" on a Discover +-- artist suggestion (#2374, milestone #268 slice 3). +-- +-- This is NOT a dislike. Rule #101 forbids a "Not for me" / thumbs-down / +-- exclusion UI, and a snooze deliberately isn't one: it records no verdict on +-- the music, expires on its own, and MUST NEVER feed the taste profile. It is +-- acquisition triage — "I don't want to request this right now" — so the same +-- candidate is free to return once snoozed_until passes. Anything that reads +-- this table as negative preference signal is a bug. +-- +-- Per-user (rule #47), never global: one household member parking a +-- suggestion must not remove it from anyone else's deck. +-- +-- candidate_mbid is text with NO foreign key, on purpose. Suggestions come +-- from artist_similarity_unmatched and are out-of-library BY DEFINITION, so +-- there is no artists row to reference — the MBID is the only stable identity +-- available. candidate_name is denormalized for the same reason: the manage / +-- un-snooze list has nowhere else to resolve a display name from. + +CREATE TABLE suggestion_snoozes ( + user_id uuid NOT NULL REFERENCES users(id) ON DELETE CASCADE, + candidate_mbid text NOT NULL, + candidate_name text NOT NULL, + snoozed_until timestamptz NOT NULL, + created_at timestamptz NOT NULL DEFAULT now(), + PRIMARY KEY (user_id, candidate_mbid) +); + +-- Supports the gc sweep's unqualified `WHERE snoozed_until < now()` scan. The +-- composite PK already covers every per-user read, so this is the only extra +-- index worth its write cost at household-scale row counts (same reasoning as +-- lidarr_quarantine in 0011). +CREATE INDEX suggestion_snoozes_expiry_idx ON suggestion_snoozes (snoozed_until); diff --git a/internal/db/queries/recommendation.sql b/internal/db/queries/recommendation.sql index d4c8de68..fcb35e8b 100644 --- a/internal/db/queries/recommendation.sql +++ b/internal/db/queries/recommendation.sql @@ -272,8 +272,9 @@ LIMIT 1; -- undamped sum let one heavily-played artist's neighbours take every slot -- -- entrenching harder the MORE the user listened (issue #2367 mechanism 2). -- --- Candidates already in the library, or already requested and not terminal, --- are excluded. $1=user_id, $2=half_life_days (tier 2 decay), $3=limit. +-- Candidates already in the library, already requested and not terminal, or +-- snoozed by this user, are excluded. $1=user_id, $2=half_life_days (tier 2 +-- decay), $3=limit. WITH artist_plays AS ( -- Completed plays only. The previous seed query counted every play_event, -- so skipping an artist repeatedly INCREASED its signal and pushed more of @@ -334,6 +335,18 @@ contributions AS ( AND r.lidarr_artist_mbid = u.candidate_mbid AND r.status NOT IN ('rejected', 'failed') ) + -- Snoozed by this user and not yet expired (#2374). Time-boxed and + -- per-user: the candidate returns on its own once snoozed_until + -- passes, and stays visible to everyone else meanwhile. Deliberately + -- filtered at the candidate stage, NOT folded into the score — a + -- snooze carries no opinion about the music, so it must not become a + -- ranking signal. + AND NOT EXISTS ( + SELECT 1 FROM suggestion_snoozes s + WHERE s.user_id = $1 + AND s.candidate_mbid = u.candidate_mbid + AND s.snoozed_until > now() + ) ) SELECT candidate_mbid, candidate_name, diff --git a/internal/db/queries/suggestion_snoozes.sql b/internal/db/queries/suggestion_snoozes.sql new file mode 100644 index 00000000..952b1ef7 --- /dev/null +++ b/internal/db/queries/suggestion_snoozes.sql @@ -0,0 +1,39 @@ +-- Time-boxed "not right now" on a Discover artist suggestion (#2374). +-- +-- See 0049_suggestion_snoozes.up.sql for why this is a snooze and not a +-- dismissal: it records no verdict on the music, expires on its own, and +-- must never reach the taste profile. Nothing in internal/taste may read +-- this table. + +-- name: SnoozeSuggestion :exec +-- Upsert, so re-snoozing an already-snoozed candidate EXTENDS it instead of +-- erroring on the PK. The name is refreshed too — a later suggestion may +-- carry a corrected spelling from the similarity feed. +-- $1=user_id, $2=candidate_mbid, $3=candidate_name, $4=duration in days. +INSERT INTO suggestion_snoozes (user_id, candidate_mbid, candidate_name, snoozed_until) +VALUES ($1, $2, $3, now() + ($4::float8 * INTERVAL '1 day')) +ON CONFLICT (user_id, candidate_mbid) DO UPDATE + SET snoozed_until = EXCLUDED.snoozed_until, + candidate_name = EXCLUDED.candidate_name; + +-- name: UnsnoozeSuggestion :execrows +-- Row count is returned so the handler can 404 an MBID that was never +-- snoozed rather than reporting success for a no-op. +DELETE FROM suggestion_snoozes + WHERE user_id = $1 AND candidate_mbid = $2; + +-- name: ListActiveSuggestionSnoozes :many +-- Backs the manage / un-snooze surface. Expired rows are filtered HERE +-- rather than left to the sweeper: gc runs on an hourly tick, so a row can +-- outlive its expiry by up to a tick and must not read as still-snoozed in +-- the meantime. +SELECT candidate_mbid, candidate_name, snoozed_until, created_at + FROM suggestion_snoozes + WHERE user_id = $1 AND snoozed_until > now() + ORDER BY snoozed_until, candidate_mbid; + +-- name: GcDeleteExpiredSuggestionSnoozes :execrows +-- Keeps the table from growing without bound. Every read already filters on +-- snoozed_until > now(), so deleting an expired row changes no behaviour — +-- this is purely reclamation. +DELETE FROM suggestion_snoozes WHERE snoozed_until < now(); diff --git a/internal/dbtest/reset.go b/internal/dbtest/reset.go index b499dfeb..180690ce 100644 --- a/internal/dbtest/reset.go +++ b/internal/dbtest/reset.go @@ -52,6 +52,12 @@ var dataTables = []string{ "sessions", "lidarr_quarantine_actions", "lidarr_quarantine", + // #2374. Keyed by (user_id, candidate_mbid) with no FK to artists — + // candidates are out-of-library — so the CASCADE from artists/users + // does NOT reach it for a leftover row whose user survived. Truncate + // explicitly or a stale snooze silently hides a candidate from the + // next test's suggestion assertions. + "suggestion_snoozes", "playlist_tracks", "playlists", "library_changes", // M7 #357 — must reset to keep cursor isolated per test diff --git a/internal/gc/worker.go b/internal/gc/worker.go index b133e6c4..b38dc8f6 100644 --- a/internal/gc/worker.go +++ b/internal/gc/worker.go @@ -17,6 +17,7 @@ // - GcResetStuckSystemPlaylistRuns (#574) // - GcDeleteExpiredPasswordResets (#575) // - GcPruneDiagnostics (M9 — diagnostics 30d retention) +// - GcDeleteExpiredSuggestionSnoozes (#2374 — snoozes expire, then go) package gc import ( @@ -84,6 +85,7 @@ func (w *Worker) tickOnce(ctx context.Context) { w.runSweep(ctx, "reset_stuck_system_runs", q.GcResetStuckSystemPlaylistRuns) w.runSweep(ctx, "delete_expired_password_resets", q.GcDeleteExpiredPasswordResets) w.runSweep(ctx, "prune_diagnostics", q.GcPruneDiagnostics) + w.runSweep(ctx, "delete_expired_suggestion_snoozes", q.GcDeleteExpiredSuggestionSnoozes) } // runSweep is a small adapter so each sweep call site is a one-liner diff --git a/internal/recommendation/suggestions_integration_test.go b/internal/recommendation/suggestions_integration_test.go index 67a4cefd..1f82da8c 100644 --- a/internal/recommendation/suggestions_integration_test.go +++ b/internal/recommendation/suggestions_integration_test.go @@ -472,3 +472,217 @@ func TestSuggestArtists_SkippedPlaysDoNotSeedTier2(t *testing.T) { t.Errorf("len = %d, want 0 (skips are not affinity): %+v", len(out), out) } } + +// --- Slice 3 (#2374): time-boxed suggestion snooze --- +// +// The defining property under test is that a snooze EXPIRES. A permanent +// dismissal would pass most of these; only TestSuggestArtists_ExpiredSnooze +// distinguishes the two, and it is the reason this shape was approved over an +// exclusion UI (rule #101). + +// snoozeUntil inserts a snooze row with an explicit absolute expiry, so a +// test can place it in the past without depending on the duration arithmetic +// in SnoozeSuggestion. +func snoozeUntil(t *testing.T, pool *pgxpool.Pool, userID pgtype.UUID, mbid, name string, until time.Time) { + t.Helper() + if _, err := pool.Exec(context.Background(), + `INSERT INTO suggestion_snoozes (user_id, candidate_mbid, candidate_name, snoozed_until) + VALUES ($1, $2, $3, $4) + ON CONFLICT (user_id, candidate_mbid) DO UPDATE SET snoozed_until = EXCLUDED.snoozed_until`, + userID, mbid, name, until, + ); err != nil { + t.Fatalf("snoozeUntil: %v", err) + } +} + +// seedOneCandidate wires the minimum that puts exactly one candidate in the +// deck: a liked seed artist with one unmatched neighbour. +func seedOneCandidate(t *testing.T, pool *pgxpool.Pool, userID pgtype.UUID, mbid, name string) { + t.Helper() + seed := seedArtist(t, pool, "Seed", "") + likeArtist(t, pool, userID, seed.ID) + seedUnmatched(t, pool, seed.ID, mbid, name, 0.9) +} + +func TestSuggestArtists_ActiveSnoozeHidesCandidate(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + seedOneCandidate(t, pool, user.ID, "snoozed-mbid", "Parked Artist") + + if err := dbq.New(pool).SnoozeSuggestion(context.Background(), dbq.SnoozeSuggestionParams{ + UserID: user.ID, + CandidateMbid: "snoozed-mbid", + CandidateName: "Parked Artist", + Column4: 90, + }); err != nil { + t.Fatalf("SnoozeSuggestion: %v", err) + } + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 0 { + t.Errorf("len = %d, want 0 (active snooze should hide the candidate): %+v", len(out), out) + } +} + +// The whole point of a snooze over a dismissal: it comes back on its own. +func TestSuggestArtists_ExpiredSnoozeShowsCandidateAgain(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + seedOneCandidate(t, pool, user.ID, "expired-mbid", "Returning Artist") + snoozeUntil(t, pool, user.ID, "expired-mbid", "Returning Artist", time.Now().Add(-time.Hour)) + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 1 { + t.Fatalf("len = %d, want 1 (an expired snooze must not hide anything): %+v", len(out), out) + } + if out[0].MBID != "expired-mbid" { + t.Errorf("mbid = %q, want expired-mbid", out[0].MBID) + } +} + +// Rule #47: one household member parking a suggestion must not remove it +// from anyone else's deck. +func TestSuggestArtists_SnoozeIsPerUser(t *testing.T) { + pool := newPool(t) + alice := seedUser(t, pool, "alice") + bob := seedUser(t, pool, "bob") + + seed := seedArtist(t, pool, "Seed", "") + likeArtist(t, pool, alice.ID, seed.ID) + likeArtist(t, pool, bob.ID, seed.ID) + seedUnmatched(t, pool, seed.ID, "shared-mbid", "Shared Candidate", 0.9) + + snoozeUntil(t, pool, alice.ID, "shared-mbid", "Shared Candidate", time.Now().Add(24*time.Hour)) + + aliceOut, err := SuggestArtists(context.Background(), pool, alice.ID, 30, 12) + if err != nil { + t.Fatalf("SuggestArtists(alice): %v", err) + } + if len(aliceOut) != 0 { + t.Errorf("alice len = %d, want 0 (she snoozed it)", len(aliceOut)) + } + bobOut, err := SuggestArtists(context.Background(), pool, bob.ID, 30, 12) + if err != nil { + t.Fatalf("SuggestArtists(bob): %v", err) + } + if len(bobOut) != 1 { + t.Errorf("bob len = %d, want 1 (alice's snooze is not his)", len(bobOut)) + } +} + +func TestUnsnoozeSuggestion_RestoresImmediately(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + seedOneCandidate(t, pool, user.ID, "undo-mbid", "Undo Artist") + snoozeUntil(t, pool, user.ID, "undo-mbid", "Undo Artist", time.Now().Add(90*24*time.Hour)) + + q := dbq.New(pool) + rows, err := q.UnsnoozeSuggestion(context.Background(), dbq.UnsnoozeSuggestionParams{ + UserID: user.ID, CandidateMbid: "undo-mbid", + }) + if err != nil { + t.Fatalf("UnsnoozeSuggestion: %v", err) + } + if rows != 1 { + t.Errorf("rows = %d, want 1", rows) + } + // A second delete affects nothing — the handler turns this into a 404 + // rather than reporting success for a no-op. + rows, err = q.UnsnoozeSuggestion(context.Background(), dbq.UnsnoozeSuggestionParams{ + UserID: user.ID, CandidateMbid: "undo-mbid", + }) + if err != nil { + t.Fatalf("UnsnoozeSuggestion (repeat): %v", err) + } + if rows != 0 { + t.Errorf("repeat rows = %d, want 0", rows) + } + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 1 { + t.Errorf("len = %d, want 1 (unsnooze restores the candidate)", len(out)) + } +} + +// Re-snoozing must extend, not conflict on the PK. +func TestSnoozeSuggestion_UpsertExtends(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + q := dbq.New(pool) + + snoozeUntil(t, pool, user.ID, "extend-mbid", "Old Name", time.Now().Add(time.Hour)) + if err := q.SnoozeSuggestion(context.Background(), dbq.SnoozeSuggestionParams{ + UserID: user.ID, + CandidateMbid: "extend-mbid", + CandidateName: "New Name", + Column4: 90, + }); err != nil { + t.Fatalf("SnoozeSuggestion (re-snooze): %v", err) + } + + rows, err := q.ListActiveSuggestionSnoozes(context.Background(), user.ID) + if err != nil { + t.Fatalf("ListActiveSuggestionSnoozes: %v", err) + } + if len(rows) != 1 { + t.Fatalf("len = %d, want 1 (upsert, not a second row)", len(rows)) + } + if rows[0].CandidateName != "New Name" { + t.Errorf("name = %q, want New Name (upsert refreshes it)", rows[0].CandidateName) + } + if got := time.Until(rows[0].SnoozedUntil.Time); got < 80*24*time.Hour { + t.Errorf("snoozed_until is %v away, want ~90d (re-snooze should extend)", got) + } +} + +// ListActiveSuggestionSnoozes filters expired rows itself rather than +// trusting the hourly gc sweep to have run. +func TestListActiveSuggestionSnoozes_ExcludesExpired(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + snoozeUntil(t, pool, user.ID, "live-mbid", "Live", time.Now().Add(24*time.Hour)) + snoozeUntil(t, pool, user.ID, "dead-mbid", "Dead", time.Now().Add(-24*time.Hour)) + + rows, err := dbq.New(pool).ListActiveSuggestionSnoozes(context.Background(), user.ID) + if err != nil { + t.Fatalf("ListActiveSuggestionSnoozes: %v", err) + } + if len(rows) != 1 { + t.Fatalf("len = %d, want 1 (expired row must not be listed)", len(rows)) + } + if rows[0].CandidateMbid != "live-mbid" { + t.Errorf("mbid = %q, want live-mbid", rows[0].CandidateMbid) + } +} + +func TestGcDeleteExpiredSuggestionSnoozes_KeepsActive(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + snoozeUntil(t, pool, user.ID, "live-mbid", "Live", time.Now().Add(24*time.Hour)) + snoozeUntil(t, pool, user.ID, "dead-mbid", "Dead", time.Now().Add(-24*time.Hour)) + + q := dbq.New(pool) + deleted, err := q.GcDeleteExpiredSuggestionSnoozes(context.Background()) + if err != nil { + t.Fatalf("GcDeleteExpiredSuggestionSnoozes: %v", err) + } + if deleted != 1 { + t.Errorf("deleted = %d, want 1 (only the expired row)", deleted) + } + rows, err := q.ListActiveSuggestionSnoozes(context.Background(), user.ID) + if err != nil { + t.Fatalf("ListActiveSuggestionSnoozes: %v", err) + } + if len(rows) != 1 { + t.Errorf("len = %d, want 1 (active snooze survives the sweep)", len(rows)) + } +} From 6e39471a70ad3a3133d4999a1fc7402db7861024 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 2 Aug 2026 19:03:10 -0400 Subject: [PATCH 06/14] =?UTF-8?q?feat(discover):=20snooze=20affordance=20o?= =?UTF-8?q?n=20Android=20+=20web=20suggestion=20cards=20=E2=80=94=20#2375?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the snooze from slice 3 (#2374), so it's now touchable on both clients (rule #27 — the server side alone was never shippable). Copy is "Not right now" everywhere, never a dislike (rule #101). The parked list even says so out loud: "Nothing here counts against your taste profile." Both clients flip the card in place to a "Not right now" state with an Undo, rather than yanking it out of the grid under the cursor. The row leaves on the next refetch; the persistent way back is a parked-list section below the deck. That list isn't optional garnish — a snoozed candidate is by definition absent from the deck, so without it the DELETE endpoint is unreachable. Android routes the write through the offline MutationQueue per rule #100, as ONE toggle kind (SUGGESTION_SNOOZE_TOGGLE) carrying the desired state rather than two action kinds. That reuses the LIKE_TOGGLE collapse: a queued snooze the user has since undone is dropped unsent instead of replaying after the undo and re-hiding an artist they asked to see. The collapse helper is now a pure top-level function so that rule is unit tested rather than inferred. The repository does NOT enqueue on a 4xx — a permanent rejection would replay to the same failure and would raise a misleading "will sync when online" hint. The common case is a 404 from un-snoozing a row that already lapsed, which is the user's intended end state anyway. Also: an empty deck used to have one meaning (no listening signal yet). It can now also mean "you parked them all", so the empty copy branches — telling that user to go listen to something would be wrong advice. Co-Authored-By: Claude Opus 5 (1M context) --- .../minstrel/api/endpoints/DiscoverApi.kt | 30 ++++ .../minstrel/cache/mutations/MutationQueue.kt | 40 +++++ .../cache/mutations/MutationReplayer.kt | 93 +++++++--- .../discover/data/DiscoverRepository.kt | 80 +++++++++ .../minstrel/discover/ui/DiscoverScreen.kt | 80 ++++++++- .../minstrel/discover/ui/DiscoverTiles.kt | 67 +++++++- .../minstrel/discover/ui/DiscoverViewModel.kt | 53 ++++++ .../fabledsword/minstrel/models/Discover.kt | 56 ++++++ .../minstrel/models/wire/DiscoverWire.kt | 30 ++++ .../mutations/SupersededToggleIdsTest.kt | 132 ++++++++++++++ .../models/SuggestionSnoozeRefTest.kt | 67 ++++++++ web/src/lib/api/queries.ts | 1 + web/src/lib/api/suggestions.test.ts | 64 ++++++- web/src/lib/api/suggestions.ts | 35 +++- web/src/lib/api/types.ts | 10 ++ .../lib/components/DiscoverResultCard.svelte | 70 +++++++- .../lib/components/DiscoverResultCard.test.ts | 43 +++++ web/src/lib/components/SuggestionFeed.svelte | 131 +++++++++++++- web/src/lib/components/SuggestionFeed.test.ts | 162 +++++++++++++++--- 19 files changed, 1176 insertions(+), 68 deletions(-) create mode 100644 android/app/src/test/java/com/fabledsword/minstrel/cache/mutations/SupersededToggleIdsTest.kt create mode 100644 android/app/src/test/java/com/fabledsword/minstrel/models/SuggestionSnoozeRefTest.kt diff --git a/android/app/src/main/java/com/fabledsword/minstrel/api/endpoints/DiscoverApi.kt b/android/app/src/main/java/com/fabledsword/minstrel/api/endpoints/DiscoverApi.kt index 9614eacf..cd0215ea 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/api/endpoints/DiscoverApi.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/api/endpoints/DiscoverApi.kt @@ -3,9 +3,13 @@ package com.fabledsword.minstrel.api.endpoints import com.fabledsword.minstrel.models.wire.ArtistSuggestionWire import com.fabledsword.minstrel.models.wire.CreateRequestBody import com.fabledsword.minstrel.models.wire.LidarrSearchResultWire +import com.fabledsword.minstrel.models.wire.SnoozeSuggestionBody +import com.fabledsword.minstrel.models.wire.SuggestionSnoozeWire import retrofit2.http.Body +import retrofit2.http.DELETE import retrofit2.http.GET import retrofit2.http.POST +import retrofit2.http.Path import retrofit2.http.Query /** @@ -30,4 +34,30 @@ interface DiscoverApi { @POST("api/requests") suspend fun createRequest(@Body body: CreateRequestBody) + + /** + * Parks a suggestion — "not right now", NOT a dislike. Time-boxed + * server-side (90 days) and never fed into the taste profile. + * + * [body] must carry the artist's name: candidates are out-of-library, so + * the server has no local row to resolve a display name from and returns + * 400 without it. + */ + @POST("api/discover/suggestions/{mbid}/snooze") + suspend fun snoozeSuggestion( + @Path("mbid") mbid: String, + @Body body: SnoozeSuggestionBody, + ) + + /** Brings a parked suggestion back. 404 when it wasn't snoozed. */ + @DELETE("api/discover/suggestions/{mbid}/snooze") + suspend fun unsnoozeSuggestion(@Path("mbid") mbid: String) + + /** + * Currently-parked suggestions. Server filters expired rows, so every + * row returned is still snoozed. This is the only route back to an + * un-snooze once the card has left the deck. + */ + @GET("api/discover/snoozes") + suspend fun listSnoozes(): List } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/cache/mutations/MutationQueue.kt b/android/app/src/main/java/com/fabledsword/minstrel/cache/mutations/MutationQueue.kt index 79dda6ce..84a0bdf0 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/cache/mutations/MutationQueue.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/cache/mutations/MutationQueue.kt @@ -35,6 +35,12 @@ object MutationKind { // background avoids the duplicate + orphan row the old offline-on-stop // path produced (see 2026-06-11 contract audit). const val PLAY_ENDED: String = "play_ended" + + // #2374 suggestion snooze. ONE toggle kind rather than separate + // snooze/unsnooze kinds, mirroring LIKE_TOGGLE, so a snooze followed by + // an undo collapses to the latest intent instead of replaying as two + // opposed calls whose order decides the outcome. + const val SUGGESTION_SNOOZE_TOGGLE: String = "suggestion_snooze_toggle" } /** @@ -152,6 +158,25 @@ class MutationQueue @Inject constructor( ), ) + /** + * Queues a suggestion snooze (or its undo) for replay. [desiredSnoozed] + * is the TARGET state, so repeated taps collapse to one replay. + * + * [name] is carried even for an un-snooze, where the server ignores it, + * so a single payload shape serves both directions. + */ + suspend fun enqueueSuggestionSnoozeToggle( + mbid: String, + name: String, + desiredSnoozed: Boolean, + ): Long = insertUserDriven( + MutationKind.SUGGESTION_SNOOZE_TOGGLE, + json.encodeToString( + SuggestionSnoozeTogglePayload.serializer(), + SuggestionSnoozeTogglePayload(mbid, name, desiredSnoozed), + ), + ) + suspend fun enqueueRequestCancel(requestId: String): Long = insertUserDriven( MutationKind.REQUEST_CANCEL, json.encodeToString( @@ -192,6 +217,21 @@ class MutationQueue @Inject constructor( } } +/** + * Persisted payload for `MutationKind.SUGGESTION_SNOOZE_TOGGLE` (#2374). + * `desiredSnoozed` is the *target* state, matching [LikeTogglePayload], so + * the replayer can collapse repeated toggles for one candidate down to the + * last intent. Both directions are idempotent server-side: re-snoozing + * extends the window, and un-snoozing something already back is a 404 the + * replayer treats as permanent (nothing left to do). + */ +@Serializable +data class SuggestionSnoozeTogglePayload( + val mbid: String, + val name: String, + val desiredSnoozed: Boolean, +) + /** * Persisted payload for `MutationKind.QUARANTINE_UNFLAG` — the * `DELETE /api/quarantine/{trackId}` call lost during a connectivity diff --git a/android/app/src/main/java/com/fabledsword/minstrel/cache/mutations/MutationReplayer.kt b/android/app/src/main/java/com/fabledsword/minstrel/cache/mutations/MutationReplayer.kt index c3ddfb2d..ef54a76a 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/cache/mutations/MutationReplayer.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/cache/mutations/MutationReplayer.kt @@ -16,6 +16,7 @@ import com.fabledsword.minstrel.connectivity.NetworkStatusController import com.fabledsword.minstrel.connectivity.ServerHealth import com.fabledsword.minstrel.models.wire.PlayEndedRequest import com.fabledsword.minstrel.models.wire.PlayOfflineRequest +import com.fabledsword.minstrel.models.wire.SnoozeSuggestionBody import com.fabledsword.minstrel.auth.AuthStore import com.fabledsword.minstrel.cache.db.dao.CachedMutationDao import com.fabledsword.minstrel.cache.db.entities.CachedMutationEntity @@ -114,11 +115,12 @@ class MutationReplayer @Inject constructor( private suspend fun drain() { val rows = dao.getAll() - // Collapse superseded like-toggles: only the latest desired state per - // (entity) is replayed; older toggles for the same entity are dropped - // unsent. Without this, partial-failure + differential retry could - // replay an older toggle last and invert the final like state. - val superseded = supersededLikeToggleIds(rows) + // Collapse superseded toggles (likes, suggestion snoozes): only the + // latest desired state per entity is replayed; older toggles for the + // same entity are dropped unsent. Without this, partial-failure + + // differential retry could replay an older toggle last and invert the + // final state — a snooze the user already undid would come back. + val superseded = supersededToggleIds(rows, json) for (row in rows) { if (row.id in superseded) { dao.delete(row.id) @@ -131,25 +133,6 @@ class MutationReplayer @Inject constructor( } } - /** Row ids of like-toggles superseded by a later toggle for the same entity. */ - private fun supersededLikeToggleIds(rows: List): Set { - val latestByEntity = HashMap() - val superseded = HashSet() - rows.asSequence() - .filter { it.kind == MutationKind.LIKE_TOGGLE } - .forEach { row -> - val decoded = runCatching { - json.decodeFromString(LikeTogglePayload.serializer(), row.payload) - }.getOrNull() - if (decoded != null) { - val key = "${decoded.entityType}:${decoded.entityId}" - // `rows` is ascending by id, so a prior entry is always older. - latestByEntity.put(key, row.id)?.let(superseded::add) - } - } - return superseded - } - private suspend fun outcomeFor(row: CachedMutationEntity): Outcome = try { dispatch(row) } catch (e: HttpException) { @@ -182,6 +165,7 @@ class MutationReplayer @Inject constructor( MutationKind.PLAY_ENDED -> dispatchPlayEnded(row.payload) MutationKind.REQUEST_CANCEL -> dispatchRequestCancel(row.payload) MutationKind.PLAYBACK_ERROR_REPORT -> dispatchPlaybackErrorReport(row.payload) + MutationKind.SUGGESTION_SNOOZE_TOGGLE -> dispatchSuggestionSnoozeToggle(row.payload) // Unknown kind — drop so a stale schema entry can't wedge the queue. else -> Outcome.DROP } @@ -277,6 +261,24 @@ class MutationReplayer @Inject constructor( return Outcome.SENT } + /** + * Replays a suggestion snooze in whichever direction the payload asks for. + * + * The un-snooze branch can legitimately 404 (the row already lapsed, or a + * previous attempt landed and the response was lost). [outcomeFor] classes + * 404 as permanent → DROP, which is right: the user's intended end state + * already holds, so there is nothing left to send. + */ + private suspend fun dispatchSuggestionSnoozeToggle(payload: String): Outcome { + val decoded = json.decodeFromString(SuggestionSnoozeTogglePayload.serializer(), payload) + if (decoded.desiredSnoozed) { + discoverApi.snoozeSuggestion(decoded.mbid, SnoozeSuggestionBody(name = decoded.name)) + } else { + discoverApi.unsnoozeSuggestion(decoded.mbid) + } + return Outcome.SENT + } + private suspend fun dispatchPlaybackErrorReport(payload: String): Outcome { val decoded = json.decodeFromString(PlaybackErrorReportPayload.serializer(), payload) playbackErrorsApi.report( @@ -297,3 +299,46 @@ class MutationReplayer @Inject constructor( const val HTTP_TOO_MANY = 429 } } + +/** + * Row ids of desired-state toggles superseded by a later toggle for the same + * entity. Applies to every kind whose payload encodes a TARGET state rather + * than an action — like-toggles and suggestion snoozes (#2374) — because + * replaying a stale one last would invert the final state. + * + * Top-level and pure so it can be unit-tested without standing up a Retrofit + * instance. [rows] must be ascending by id (FIFO), which is what + * `CachedMutationDao.getAll()` returns. + */ +internal fun supersededToggleIds(rows: List, json: Json): Set { + val latestByEntity = HashMap() + val superseded = HashSet() + rows.asSequence() + .mapNotNull { row -> toggleKeyOf(row, json)?.let { key -> key to row.id } } + .forEach { (key, id) -> + // Ascending ids mean a prior entry for this key is always older. + latestByEntity.put(key, id)?.let(superseded::add) + } + return superseded +} + +/** + * Collapse key for a toggle row, or null when the row isn't a toggle — or its + * payload won't decode. Undecodable rows are deliberately left alone rather + * than grouped under a shared "corrupt" key, so one bad row can't suppress a + * good one behind it; the dispatcher DROPs it on its own. + * + * The kind is part of the key so two toggle kinds can never collide on the + * same entity id. + */ +private fun toggleKeyOf(row: CachedMutationEntity, json: Json): String? = when (row.kind) { + MutationKind.LIKE_TOGGLE -> runCatching { + json.decodeFromString(LikeTogglePayload.serializer(), row.payload) + }.getOrNull()?.let { "${row.kind}:${it.entityType}:${it.entityId}" } + + MutationKind.SUGGESTION_SNOOZE_TOGGLE -> runCatching { + json.decodeFromString(SuggestionSnoozeTogglePayload.serializer(), row.payload) + }.getOrNull()?.let { "${row.kind}:${it.mbid}" } + + else -> null +} diff --git a/android/app/src/main/java/com/fabledsword/minstrel/discover/data/DiscoverRepository.kt b/android/app/src/main/java/com/fabledsword/minstrel/discover/data/DiscoverRepository.kt index 2d561fd4..c89178e4 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/discover/data/DiscoverRepository.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/discover/data/DiscoverRepository.kt @@ -7,10 +7,14 @@ import com.fabledsword.minstrel.models.ArtistSuggestionRef import com.fabledsword.minstrel.models.LidarrRequestKind import com.fabledsword.minstrel.models.LidarrSearchResultRef import com.fabledsword.minstrel.models.SeedContributionRef +import com.fabledsword.minstrel.models.SuggestionSnoozeRef import com.fabledsword.minstrel.models.wire.ArtistSuggestionWire import com.fabledsword.minstrel.models.wire.CreateRequestBody import com.fabledsword.minstrel.models.wire.LidarrSearchResultWire import com.fabledsword.minstrel.models.wire.SeedContributionWire +import com.fabledsword.minstrel.models.wire.SnoozeSuggestionBody +import com.fabledsword.minstrel.models.wire.SuggestionSnoozeWire +import retrofit2.HttpException import retrofit2.Retrofit import retrofit2.create import javax.inject.Inject @@ -46,6 +50,69 @@ class DiscoverRepository @Inject constructor( suspend fun listSuggestions(): List = api.listSuggestions().map { it.toDomain() } + suspend fun listSnoozes(): List = + api.listSnoozes().map { it.toDomain() } + + /** + * Parks a suggestion ("not right now"). Offline-first per rule #100: on + * transport failure the target state is queued for the replayer rather + * than dropped. + * + * Always reports success to the caller. Unlike a request, a snooze has no + * meaningful failed state to show — the user asked for a card to go away, + * and it will, either now or when the queue drains. + */ + suspend fun snoozeSuggestion(mbid: String, name: String): Unit = toggleSnooze( + mbid = mbid, + name = name, + desiredSnoozed = true, + ) { api.snoozeSuggestion(mbid, SnoozeSuggestionBody(name = name)) } + + /** Brings a parked suggestion back. Same offline-first contract. */ + suspend fun unsnoozeSuggestion(mbid: String, name: String): Unit = toggleSnooze( + mbid = mbid, + name = name, + desiredSnoozed = false, + ) { api.unsnoozeSuggestion(mbid) } + + private suspend fun toggleSnooze( + mbid: String, + name: String, + desiredSnoozed: Boolean, + call: suspend () -> Unit, + ) { + try { + call() + } catch (e: HttpException) { + // A 4xx is the server's considered answer, not a lost call, so + // queueing it would be wrong twice over: the replay is guaranteed + // to fail again, and the enqueue would raise a "will sync when + // online" snackbar for something already settled. The common case + // is a 404 from un-snoozing a row that already lapsed — which is + // the end state the user wanted anyway. + if (!isPermanent(e.code())) { + mutationQueue.enqueueSuggestionSnoozeToggle(mbid, name, desiredSnoozed) + } + } catch ( + @Suppress("TooGenericExceptionCaught", "SwallowedException") e: Throwable, + ) { + // Transport failure — intentional swallow, same offline-first + // rationale as createRequest above. The queue carries the desired + // STATE, so a later undo supersedes this rather than fighting it + // on replay. + mutationQueue.enqueueSuggestionSnoozeToggle(mbid, name, desiredSnoozed) + } + } + + /** + * Mirrors MutationReplayer's classification so the enqueue decision here + * and the drop decision there can't disagree: 4xx is permanent except the + * two "retry me" statuses. + */ + private fun isPermanent(code: Int): Boolean = + code in HTTP_CLIENT_ERR_MIN..HTTP_CLIENT_ERR_MAX && + code != HTTP_TIMEOUT && code != HTTP_TOO_MANY + suspend fun search(query: String, kind: LidarrRequestKind): List = api.search(query = query, kind = kind.wire).map { it.toDomain() } @@ -85,6 +152,13 @@ class DiscoverRepository @Inject constructor( RequestOutcome.QUEUED } } + + private companion object { + const val HTTP_CLIENT_ERR_MIN = 400 + const val HTTP_CLIENT_ERR_MAX = 499 + const val HTTP_TIMEOUT = 408 + const val HTTP_TOO_MANY = 429 + } } // ── Mappers (internal — wire types stay out of UI) ── @@ -112,6 +186,12 @@ private fun SeedContributionWire.toDomain(): SeedContributionRef = SeedContribut isLiked = isLiked, ) +private fun SuggestionSnoozeWire.toDomain(): SuggestionSnoozeRef = SuggestionSnoozeRef( + mbid = mbid, + name = name, + snoozedUntil = snoozedUntil, +) + private fun RequestCreatePayload.toBody(): CreateRequestBody = CreateRequestBody( kind = kind, artistMbid = artistMbid, diff --git a/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverScreen.kt b/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverScreen.kt index 56630ac7..02a21224 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverScreen.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverScreen.kt @@ -40,6 +40,7 @@ import com.fabledsword.minstrel.discover.data.RequestOutcome import com.fabledsword.minstrel.models.ArtistSuggestionRef import com.fabledsword.minstrel.models.LidarrRequestKind import com.fabledsword.minstrel.models.LidarrSearchResultRef +import com.fabledsword.minstrel.models.SuggestionSnoozeRef import com.fabledsword.minstrel.nav.Discover import com.fabledsword.minstrel.shared.widgets.ErrorRetry import com.fabledsword.minstrel.shared.widgets.LoadingCentered @@ -104,6 +105,18 @@ private fun DiscoverBody( ResultsState.Idle -> SuggestionsPane( state = state.suggestions, locallyRequestedMbids = state.locallyRequestedMbids, + snoozeUi = SnoozeUi( + locallySnoozedMbids = state.locallySnoozedMbids, + snoozes = state.snoozes, + // No snackbar on snooze: the row itself flips to "Not + // right now" with an Undo, so a snackbar would only + // repeat what the user can already see — and cover the + // next row while doing it. + onSnooze = { s -> scope.launch { viewModel.snoozeSuggestion(s) } }, + onUnsnooze = { mbid, name -> + scope.launch { viewModel.unsnoozeSuggestion(mbid, name) } + }, + ), onRequest = { s -> scope.launch { val outcome = viewModel.requestSuggestion(s) @@ -178,10 +191,23 @@ private fun KindChips(kind: LidarrRequestKind, onChange: (LidarrRequestKind) -> } } +/** + * The snooze surface's data and callbacks, bundled rather than threaded + * through as four more parameters — the pane grew from one action to three + * with slice 4 and the signatures stopped being readable. + */ +private data class SnoozeUi( + val locallySnoozedMbids: Set, + val snoozes: List, + val onSnooze: (ArtistSuggestionRef) -> Unit, + val onUnsnooze: (String, String) -> Unit, +) + @Composable private fun SuggestionsPane( state: SuggestionState, locallyRequestedMbids: Set, + snoozeUi: SnoozeUi, onRequest: (ArtistSuggestionRef) -> Unit, onRetry: () -> Unit, ) { @@ -194,6 +220,7 @@ private fun SuggestionsPane( ) is SuggestionState.Loaded -> SuggestionsList( items = state.items.filter { it.mbid !in locallyRequestedMbids }, + snoozeUi = snoozeUi, onRequest = onRequest, ) } @@ -202,6 +229,7 @@ private fun SuggestionsPane( @Composable private fun SuggestionsList( items: List, + snoozeUi: SnoozeUi, onRequest: (ArtistSuggestionRef) -> Unit, ) { LazyColumn( @@ -210,13 +238,61 @@ private fun SuggestionsList( ) { item { SuggestionsHeader() } if (items.isEmpty()) { - item { CenteredMessage("Listen to or like an artist to fill this in.") } + // An empty deck used to mean one thing — no listening signal yet. + // With snoozing it can also mean "you parked them all", and telling + // that user to go listen to something would be wrong advice. + item { + CenteredMessage( + if (snoozeUi.snoozes.isEmpty()) { + "Listen to or like an artist to fill this in." + } else { + "Nothing new right now — the artists you've parked are below." + }, + ) + } } else { items(items = items, key = { it.mbid }) { s -> - SuggestionTile(s = s, onRequest = { onRequest(s) }) + SuggestionTile( + s = s, + snoozed = s.mbid in snoozeUi.locallySnoozedMbids, + onRequest = { onRequest(s) }, + onSnooze = { snoozeUi.onSnooze(s) }, + onUnsnooze = { snoozeUi.onUnsnooze(s.mbid, s.name) }, + ) HorizontalDivider() } } + // Parked candidates live at the bottom of the same scroll, not behind a + // separate screen: it's a short list the user rarely needs, but it must + // be reachable — a snoozed candidate is gone from the deck above, so + // this is the only way back to it. + if (snoozeUi.snoozes.isNotEmpty()) { + item { SnoozedHeader() } + items(items = snoozeUi.snoozes, key = { "snoozed-${it.mbid}" }) { row -> + SnoozedTile( + row = row, + onUnsnooze = { snoozeUi.onUnsnooze(row.mbid, row.name) }, + ) + HorizontalDivider() + } + } + } +} + +@Composable +private fun SnoozedHeader() { + Column(modifier = Modifier.padding(horizontal = 16.dp, vertical = 12.dp)) { + HorizontalDivider(modifier = Modifier.padding(bottom = 12.dp)) + Text( + text = "Not right now", + style = MaterialTheme.typography.titleMedium, + color = MaterialTheme.colorScheme.onBackground, + ) + Text( + text = "These come back on their own. Nothing here counts against your taste profile.", + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + ) } } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverTiles.kt b/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverTiles.kt index 613c7bae..1d18d392 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverTiles.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverTiles.kt @@ -14,8 +14,10 @@ import androidx.compose.foundation.shape.RoundedCornerShape import androidx.compose.material3.AssistChip import androidx.compose.material3.Button import androidx.compose.material3.Icon +import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Text +import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier @@ -24,14 +26,22 @@ import androidx.compose.ui.graphics.vector.ImageVector import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp import coil3.compose.AsyncImage +import com.composables.icons.lucide.Clock import com.composables.icons.lucide.Disc3 import com.composables.icons.lucide.Lucide import com.composables.icons.lucide.User import com.fabledsword.minstrel.models.ArtistSuggestionRef import com.fabledsword.minstrel.models.LidarrSearchResultRef +import com.fabledsword.minstrel.models.SuggestionSnoozeRef @Composable -internal fun SuggestionTile(s: ArtistSuggestionRef, onRequest: () -> Unit) { +internal fun SuggestionTile( + s: ArtistSuggestionRef, + snoozed: Boolean, + onRequest: () -> Unit, + onSnooze: () -> Unit, + onUnsnooze: () -> Unit, +) { Row( modifier = Modifier .fillMaxWidth() @@ -48,9 +58,13 @@ internal fun SuggestionTile(s: ArtistSuggestionRef, onRequest: () -> Unit) { maxLines = 1, overflow = TextOverflow.Ellipsis, ) - if (s.attributionText.isNotEmpty()) { + // Once parked, the "because you liked X" line is no longer the + // useful thing to say — confirming what just happened is. + val secondary = + if (snoozed) "Not right now — hidden for a while" else s.attributionText + if (secondary.isNotEmpty()) { Text( - text = s.attributionText, + text = secondary, style = MaterialTheme.typography.bodySmall, color = MaterialTheme.colorScheme.onSurfaceVariant, maxLines = 2, @@ -58,7 +72,52 @@ internal fun SuggestionTile(s: ArtistSuggestionRef, onRequest: () -> Unit) { ) } } - Button(onClick = onRequest) { Text("Request") } + if (snoozed) { + TextButton(onClick = onUnsnooze) { Text("Undo") } + } else { + Button(onClick = onRequest) { Text("Request") } + IconButton(onClick = onSnooze) { + Icon( + imageVector = Lucide.Clock, + // Rule #101: the label states what happens, and passes no + // judgement on the music. Never "not for me". + contentDescription = "Not right now — hide ${s.name} for a while", + tint = MaterialTheme.colorScheme.onSurfaceVariant, + ) + } + } + } +} + +/** + * One row of the parked list. This exists because a snoozed candidate is by + * definition absent from the deck above, so without it there is no route back + * to an un-snooze once the card has gone. + */ +@Composable +internal fun SnoozedTile(row: SuggestionSnoozeRef, onUnsnooze: () -> Unit) { + Row( + modifier = Modifier + .fillMaxWidth() + .padding(horizontal = 16.dp, vertical = 8.dp), + verticalAlignment = Alignment.CenterVertically, + horizontalArrangement = Arrangement.spacedBy(12.dp), + ) { + Column(modifier = Modifier.weight(1f)) { + Text( + text = row.name, + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.onSurface, + maxLines = 1, + overflow = TextOverflow.Ellipsis, + ) + Text( + text = "Back ${row.returnsIn()}", + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + ) + } + TextButton(onClick = onUnsnooze) { Text("Bring back") } } } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverViewModel.kt b/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverViewModel.kt index ab0c7997..f26011c3 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverViewModel.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverViewModel.kt @@ -10,6 +10,7 @@ import com.fabledsword.minstrel.discover.data.RequestOutcome import com.fabledsword.minstrel.models.ArtistSuggestionRef import com.fabledsword.minstrel.models.LidarrRequestKind import com.fabledsword.minstrel.models.LidarrSearchResultRef +import com.fabledsword.minstrel.models.SuggestionSnoozeRef import dagger.hilt.android.lifecycle.HiltViewModel import kotlinx.coroutines.Job import kotlinx.coroutines.flow.MutableStateFlow @@ -27,6 +28,17 @@ data class DiscoverState( val suggestions: SuggestionState = SuggestionState.Loading, val results: ResultsState = ResultsState.Idle, val locallyRequestedMbids: Set = emptySet(), + /** + * Parked candidates, for the manage list under the feed. Empty is the + * normal case and hides the section entirely. + */ + val snoozes: List = emptyList(), + /** + * Just-snoozed MBIDs. These keep their row visible showing an Undo rather + * than yanking it out from under the user's finger; the row is gone on the + * next load, and [snoozes] is the way back after that. + */ + val locallySnoozedMbids: Set = emptySet(), ) sealed interface SuggestionState { @@ -96,6 +108,47 @@ class DiscoverViewModel @Inject constructor( ) } } + // Refresh the parked list alongside the deck: a snooze made on another + // client should show up here, and one whose window lapsed should drop + // off. Sequenced after the deck load rather than raced with it so the + // two panes can't disagree about a candidate mid-refresh. + loadSnoozes() + } + + /** + * Loads the parked list. Failure is deliberately silent: this is a + * secondary pane, and an error banner for it would sit above the suggestion + * feed the user actually came for. The list stays as-is and the next + * refresh retries. + */ + private suspend fun loadSnoozes() { + try { + val rows = repository.listSnoozes() + internal.update { it.copy(snoozes = rows) } + } catch ( + @Suppress("TooGenericExceptionCaught", "SwallowedException") e: Throwable, + ) { + // Keep whatever we last showed rather than blanking the section. + } + } + + /** + * Parks a suggestion. Flips the row locally first so the tap registers + * immediately; the repository handles the offline case, so there is no + * failure branch to revert here — unlike the web client, where the fetch + * either lands or doesn't. + */ + suspend fun snoozeSuggestion(s: ArtistSuggestionRef) { + internal.update { it.copy(locallySnoozedMbids = it.locallySnoozedMbids + s.mbid) } + repository.snoozeSuggestion(s.mbid, s.name) + loadSnoozes() + } + + /** Brings a parked suggestion back, from either the card or the list. */ + suspend fun unsnoozeSuggestion(mbid: String, name: String) { + internal.update { it.copy(locallySnoozedMbids = it.locallySnoozedMbids - mbid) } + repository.unsnoozeSuggestion(mbid, name) + loadSnoozes() } fun runSearch() { diff --git a/android/app/src/main/java/com/fabledsword/minstrel/models/Discover.kt b/android/app/src/main/java/com/fabledsword/minstrel/models/Discover.kt index b04e9fe8..4c1b39e4 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/models/Discover.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/models/Discover.kt @@ -1,5 +1,8 @@ package com.fabledsword.minstrel.models +import kotlinx.datetime.Instant +import kotlin.math.roundToInt + /** * Kind of Lidarr request being created. Wire form is the lowercase * enum name; the helper [wire] keeps that mapping in one place. @@ -67,3 +70,56 @@ data class ArtistSuggestionRef( private const val MAX_ATTRIBUTION_PHRASES = 3 } } + +/** + * A suggestion the user parked with "not right now" (#2374). + * + * Deliberately NOT a dislike: it carries no verdict on the artist, expires on + * its own, and never reaches the taste profile. Anything that treats this as + * negative preference signal is a bug. + * + * [snoozedUntil] is the raw RFC3339 string from the wire. Only the server + * decides whether a snooze is still in effect — every row the client receives + * already is — so this is read purely to phrase "back in about 3 months". + */ +data class SuggestionSnoozeRef( + val mbid: String, + val name: String, + val snoozedUntil: String, +) { + /** + * Relative return phrase for the manage list. Relative rather than a + * calendar date because the exact day a 90-day snooze lapses is noise the + * user never asked for. + * + * [nowMs] is injectable so this is testable without freezing the clock. + * Returns "shortly" for an unparseable or already-past timestamp: the row + * is on screen, so the server still considers it snoozed, and guessing is + * better than rendering an empty line. + */ + fun returnsIn(nowMs: Long = System.currentTimeMillis()): String { + val untilMs = runCatching { Instant.parse(snoozedUntil).toEpochMilliseconds() } + .getOrNull() ?: return "shortly" + // Already lapsed by our clock, yet the server still returned it — the + // two disagree. Say something plausible rather than "today", which + // would read as a real prediction. + if (untilMs <= nowMs) return "shortly" + val days = ((untilMs - nowMs).toDouble() / MILLIS_PER_DAY).roundToInt() + return when { + days < 1 -> "today" + days == 1 -> "tomorrow" + days < DAYS_BEFORE_MONTHS -> "in $days days" + else -> { + val months = (days.toDouble() / DAYS_PER_MONTH).roundToInt() + if (months == 1) "in about a month" else "in about $months months" + } + } + } + + private companion object { + const val MILLIS_PER_DAY = 86_400_000.0 + // Below this, days read more naturally than a rounded month count. + const val DAYS_BEFORE_MONTHS = 45 + const val DAYS_PER_MONTH = 30.0 + } +} diff --git a/android/app/src/main/java/com/fabledsword/minstrel/models/wire/DiscoverWire.kt b/android/app/src/main/java/com/fabledsword/minstrel/models/wire/DiscoverWire.kt index bf09d37d..5ad6ac60 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/models/wire/DiscoverWire.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/models/wire/DiscoverWire.kt @@ -48,6 +48,36 @@ data class SeedContributionWire( @SerialName("is_liked") val isLiked: Boolean = false, ) +/** + * One row of `GET /api/discover/snoozes` — a suggestion the user parked + * with "not right now". The server only returns rows that are still in + * effect, so the client never compares [snoozedUntil] against the clock to + * decide whether to show it; it reads it only to say when the artist comes + * back. + */ +@Serializable +data class SuggestionSnoozeWire( + val mbid: String = "", + val name: String = "", + @SerialName("snoozed_until") val snoozedUntil: String = "", + @SerialName("created_at") val createdAt: String = "", +) + +/** + * Body for `POST /api/discover/suggestions/{mbid}/snooze`. + * + * [name] is required by the server, not decorative: suggestions are + * out-of-library, so there is no artists row to resolve a display name from + * and the snooze list would have nothing to render. Omitting it is a 400. + * + * No `days` field. The duration is the server's to own (90 days); pinning it + * client-side would freeze the default at whatever this build shipped. + */ +@Serializable +data class SnoozeSuggestionBody( + val name: String, +) + /** * Body posted to `POST /api/requests`. Mirrors the Flutter `createRequest` * payload shape. Optional fields are emitted only when non-null diff --git a/android/app/src/test/java/com/fabledsword/minstrel/cache/mutations/SupersededToggleIdsTest.kt b/android/app/src/test/java/com/fabledsword/minstrel/cache/mutations/SupersededToggleIdsTest.kt new file mode 100644 index 00000000..b280ab80 --- /dev/null +++ b/android/app/src/test/java/com/fabledsword/minstrel/cache/mutations/SupersededToggleIdsTest.kt @@ -0,0 +1,132 @@ +package com.fabledsword.minstrel.cache.mutations + +import com.fabledsword.minstrel.cache.db.entities.CachedMutationEntity +import kotlinx.serialization.json.Json +import org.junit.jupiter.api.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * Collapse rules for desired-state toggles in the offline queue. + * + * The hazard this guards against is real and silent: without collapsing, a + * queued snooze that replays AFTER the user's undo re-hides an artist they + * asked to see again, and nothing surfaces the contradiction. + */ +class SupersededToggleIdsTest { + + private val json = Json { ignoreUnknownKeys = true } + + private fun snoozeRow(id: Long, mbid: String, desiredSnoozed: Boolean) = CachedMutationEntity( + id = id, + kind = MutationKind.SUGGESTION_SNOOZE_TOGGLE, + payload = json.encodeToString( + SuggestionSnoozeTogglePayload.serializer(), + SuggestionSnoozeTogglePayload(mbid, "Name", desiredSnoozed), + ), + ) + + private fun likeRow(id: Long, entityId: String, desired: Boolean) = CachedMutationEntity( + id = id, + kind = MutationKind.LIKE_TOGGLE, + payload = json.encodeToString( + LikeTogglePayload.serializer(), + LikeTogglePayload("artist", entityId, desired), + ), + ) + + @Test + fun `a snooze followed by its undo drops the snooze`() { + val rows = listOf( + snoozeRow(1, "mb-a", desiredSnoozed = true), + snoozeRow(2, "mb-a", desiredSnoozed = false), + ) + // Only the later intent (the undo) survives to be replayed. + assertEquals(setOf(1L), supersededToggleIds(rows, json)) + } + + @Test + fun `toggles for different candidates never collapse into each other`() { + val rows = listOf( + snoozeRow(1, "mb-a", desiredSnoozed = true), + snoozeRow(2, "mb-b", desiredSnoozed = true), + ) + assertTrue(supersededToggleIds(rows, json).isEmpty()) + } + + @Test + fun `only the newest of several toggles for one candidate survives`() { + val rows = listOf( + snoozeRow(1, "mb-a", desiredSnoozed = true), + snoozeRow(2, "mb-a", desiredSnoozed = false), + snoozeRow(3, "mb-a", desiredSnoozed = true), + ) + assertEquals(setOf(1L, 2L), supersededToggleIds(rows, json)) + } + + // The kind is part of the collapse key, so a snooze and a like that happen + // to share an id string must not shadow one another. + @Test + fun `a like and a snooze on the same id string do not collide`() { + val rows = listOf( + likeRow(1, "same-id", desired = true), + snoozeRow(2, "same-id", desiredSnoozed = true), + ) + assertTrue(supersededToggleIds(rows, json).isEmpty()) + } + + @Test + fun `like toggles still collapse — the pre-existing behaviour is intact`() { + val rows = listOf( + likeRow(1, "artist-1", desired = true), + likeRow(2, "artist-1", desired = false), + ) + assertEquals(setOf(1L), supersededToggleIds(rows, json)) + } + + @Test + fun `non-toggle kinds are never collapsed, even repeated for one entity`() { + // Two appends to the same playlist are two real actions, not one + // desired state — collapsing them would lose a write. + val rows = listOf( + CachedMutationEntity( + id = 1, + kind = MutationKind.PLAYLIST_APPEND, + payload = json.encodeToString( + PlaylistAppendPayload.serializer(), + PlaylistAppendPayload("pl-1", listOf("t1")), + ), + ), + CachedMutationEntity( + id = 2, + kind = MutationKind.PLAYLIST_APPEND, + payload = json.encodeToString( + PlaylistAppendPayload.serializer(), + PlaylistAppendPayload("pl-1", listOf("t2")), + ), + ), + ) + assertTrue(supersededToggleIds(rows, json).isEmpty()) + } + + // A row whose payload won't decode gets no key at all, rather than sharing + // a "corrupt" bucket — otherwise one bad row could suppress a good one + // behind it. The dispatcher DROPs the bad row on its own. + @Test + fun `an undecodable payload does not suppress a valid later row`() { + val rows = listOf( + CachedMutationEntity( + id = 1, + kind = MutationKind.SUGGESTION_SNOOZE_TOGGLE, + payload = "{ not json", + ), + snoozeRow(2, "mb-a", desiredSnoozed = true), + ) + assertTrue(supersededToggleIds(rows, json).isEmpty()) + } + + @Test + fun `an empty queue collapses nothing`() { + assertTrue(supersededToggleIds(emptyList(), json).isEmpty()) + } +} diff --git a/android/app/src/test/java/com/fabledsword/minstrel/models/SuggestionSnoozeRefTest.kt b/android/app/src/test/java/com/fabledsword/minstrel/models/SuggestionSnoozeRefTest.kt new file mode 100644 index 00000000..f1123e46 --- /dev/null +++ b/android/app/src/test/java/com/fabledsword/minstrel/models/SuggestionSnoozeRefTest.kt @@ -0,0 +1,67 @@ +package com.fabledsword.minstrel.models + +import kotlinx.datetime.Instant +import org.junit.jupiter.api.Test +import kotlin.test.assertEquals + +/** + * `returnsIn` phrasing for the parked-suggestions list (#2375). The clock is + * injected rather than frozen, so these assertions are stable. + */ +class SuggestionSnoozeRefTest { + + private val now = 1_800_000_000_000L // fixed epoch ms; any value works + + private fun snoozeIn(days: Double) = SuggestionSnoozeRef( + mbid = "mb", + name = "Parked", + snoozedUntil = Instant + .fromEpochMilliseconds(now + (days * 86_400_000L).toLong()) + .toString(), + ) + + @Test + fun `the default 90-day snooze reads as about 3 months`() { + assertEquals("in about 3 months", snoozeIn(90.0).returnsIn(now)) + } + + @Test + fun `a month reads in the singular`() { + assertEquals("in about a month", snoozeIn(30.0).returnsIn(now)) + } + + @Test + fun `under the month threshold it counts days`() { + assertEquals("in 14 days", snoozeIn(14.0).returnsIn(now)) + } + + @Test + fun `tomorrow is named, not rendered as 1 days`() { + assertEquals("tomorrow", snoozeIn(1.0).returnsIn(now)) + } + + @Test + fun `later today rounds down to today rather than going negative`() { + assertEquals("today", snoozeIn(0.1).returnsIn(now)) + } + + // The server only ever returns unexpired rows, so a past timestamp means + // our clock and the server's disagree. The row is on screen either way, so + // say something plausible rather than leaving the line blank. + @Test + fun `an already-past expiry degrades to shortly`() { + assertEquals("shortly", snoozeIn(-5.0).returnsIn(now)) + } + + @Test + fun `an unparseable timestamp degrades to shortly`() { + val row = SuggestionSnoozeRef(mbid = "mb", name = "Parked", snoozedUntil = "not-a-date") + assertEquals("shortly", row.returnsIn(now)) + } + + @Test + fun `an empty timestamp degrades to shortly`() { + val row = SuggestionSnoozeRef(mbid = "mb", name = "Parked", snoozedUntil = "") + assertEquals("shortly", row.returnsIn(now)) + } +} diff --git a/web/src/lib/api/queries.ts b/web/src/lib/api/queries.ts index f59d7260..73db1e60 100644 --- a/web/src/lib/api/queries.ts +++ b/web/src/lib/api/queries.ts @@ -58,6 +58,7 @@ export const qk = { smtpConfig: () => ['smtpConfig'] as const, suggestions: (limit?: number) => ['suggestions', { limit: limit ?? 12 }] as const, + suggestionSnoozes: () => ['suggestionSnoozes'] as const, home: () => ['home'] as const, albumsAlpha: () => ['albumsAlpha'] as const, artistTracks: (artistId: string) => ['artistTracks', artistId] as const, diff --git a/web/src/lib/api/suggestions.test.ts b/web/src/lib/api/suggestions.test.ts index 094bd4ba..86b2c861 100644 --- a/web/src/lib/api/suggestions.test.ts +++ b/web/src/lib/api/suggestions.test.ts @@ -1,13 +1,18 @@ import { afterEach, describe, expect, test, vi } from 'vitest'; vi.mock('./client', () => ({ - api: { get: vi.fn() } + api: { get: vi.fn(), post: vi.fn(), del: vi.fn() } })); -import { listSuggestions } from './suggestions'; +import { + listSuggestions, + listSnoozes, + snoozeSuggestion, + unsnoozeSuggestion +} from './suggestions'; import { qk } from './queries'; import { api } from './client'; -import type { ArtistSuggestion } from './types'; +import type { ArtistSuggestion, SuggestionSnooze } from './types'; afterEach(() => vi.clearAllMocks()); @@ -40,3 +45,56 @@ describe('suggestions client', () => { expect(qk.suggestions(20)).toEqual(['suggestions', { limit: 20 }]); }); }); + +describe('suggestion snoozes (#2375)', () => { + test('snoozeSuggestion sends the name — the server 400s without it', async () => { + (api.post as ReturnType).mockResolvedValueOnce(null); + await snoozeSuggestion('mb-1', 'Parked Artist'); + expect(api.post).toHaveBeenCalledWith('/api/discover/suggestions/mb-1/snooze', { + name: 'Parked Artist' + }); + }); + + test('snoozeSuggestion sends no days, leaving the default to the server', async () => { + (api.post as ReturnType).mockResolvedValueOnce(null); + await snoozeSuggestion('mb-1', 'Parked Artist'); + const body = (api.post as ReturnType).mock.calls[0][1] as Record; + expect(body).not.toHaveProperty('days'); + }); + + // MBIDs are UUIDs today, but the column is free-text and the value comes + // from an external similarity feed, so it goes through encodeURIComponent. + test('the mbid is URL-encoded into the path', async () => { + (api.post as ReturnType).mockResolvedValueOnce(null); + await snoozeSuggestion('weird/id?x', 'Odd'); + expect(api.post).toHaveBeenCalledWith( + '/api/discover/suggestions/weird%2Fid%3Fx/snooze', + { name: 'Odd' } + ); + }); + + test('unsnoozeSuggestion DELETEs the same path', async () => { + (api.del as ReturnType).mockResolvedValueOnce(null); + await unsnoozeSuggestion('mb-1'); + expect(api.del).toHaveBeenCalledWith('/api/discover/suggestions/mb-1/snooze'); + }); + + test('listSnoozes hits the snoozes collection', async () => { + const fixture: SuggestionSnooze[] = [ + { + mbid: 'mb-1', + name: 'Parked Artist', + snoozed_until: '2026-11-01T00:00:00Z', + created_at: '2026-08-03T00:00:00Z' + } + ]; + (api.get as ReturnType).mockResolvedValueOnce(fixture); + const got = await listSnoozes(); + expect(api.get).toHaveBeenCalledWith('/api/discover/snoozes'); + expect(got).toEqual(fixture); + }); + + test('qk.suggestionSnoozes key shape', () => { + expect(qk.suggestionSnoozes()).toEqual(['suggestionSnoozes']); + }); +}); diff --git a/web/src/lib/api/suggestions.ts b/web/src/lib/api/suggestions.ts index 2d998898..2abcd30a 100644 --- a/web/src/lib/api/suggestions.ts +++ b/web/src/lib/api/suggestions.ts @@ -1,7 +1,7 @@ import { createQuery } from '@tanstack/svelte-query'; import { api } from './client'; import { qk } from './queries'; -import type { ArtistSuggestion } from './types'; +import type { ArtistSuggestion, SuggestionSnooze } from './types'; export async function listSuggestions(limit = 12): Promise { return api.get(`/api/discover/suggestions?limit=${limit}`); @@ -14,3 +14,36 @@ export function createSuggestionsQuery(limit = 12) { staleTime: 5 * 60_000 // 5 minutes — see M5c spec §5 }); } + +// Parks a suggestion for the server's default period (90 days). `name` is +// REQUIRED by the server and is not optional bookkeeping: candidates are +// out-of-library, so there is no artists row to resolve a display name from +// and the snooze list would have nothing to render. Omitting it is a 400. +// +// No `days` is sent. There is deliberately no duration UI yet — that knob is +// slice 6 (#2377) — and hardcoding a value here would pin the default to the +// client instead of the server that owns it. +export async function snoozeSuggestion(mbid: string, name: string): Promise { + await api.post(`/api/discover/suggestions/${encodeURIComponent(mbid)}/snooze`, { name }); +} + +// Brings a parked suggestion back immediately. The server 404s an MBID that +// was never snoozed; callers treat that as already-unsnoozed rather than as a +// failure, since the end state the user asked for is the one they get. +export async function unsnoozeSuggestion(mbid: string): Promise { + await api.del(`/api/discover/suggestions/${encodeURIComponent(mbid)}/snooze`); +} + +export async function listSnoozes(): Promise { + return api.get('/api/discover/snoozes'); +} + +export function createSnoozesQuery() { + return createQuery({ + queryKey: qk.suggestionSnoozes(), + queryFn: listSnoozes + // No staleTime, unlike the suggestions query: this list is the only route + // back to an un-snooze, so it must reflect a snooze made seconds ago + // rather than a cached view of the world. + }); +} diff --git a/web/src/lib/api/types.ts b/web/src/lib/api/types.ts index c8cdf030..5772c4d1 100644 --- a/web/src/lib/api/types.ts +++ b/web/src/lib/api/types.ts @@ -334,6 +334,16 @@ export type ArtistSuggestion = { image_url?: string; // resolved on-demand from Lidarr; absent → card placeholder }; +// One parked suggestion — "not right now", not a dislike. The server only +// ever returns rows whose snoozed_until is still in the future, so the client +// never has to compare against the clock to decide what to show. +export type SuggestionSnooze = { + mbid: string; + name: string; + snoozed_until: string; // RFC3339 + created_at: string; +}; + // Mirrors internal/api/types.go HomePayload. All slices are non-null // per the server contract — empty sections render as []. export type HomePayload = { diff --git a/web/src/lib/components/DiscoverResultCard.svelte b/web/src/lib/components/DiscoverResultCard.svelte index 5738092b..ea3ae967 100644 --- a/web/src/lib/components/DiscoverResultCard.svelte +++ b/web/src/lib/components/DiscoverResultCard.svelte @@ -1,10 +1,14 @@
@@ -58,7 +128,16 @@ {#if !query.isPending && suggestions.length === 0} -

Listen to something or like an artist to start getting suggestions.

+ +

+ {snoozes.length === 0 + ? 'Listen to something or like an artist to start getting suggestions.' + : "Nothing new right now — the artists you've parked are below."} +

{:else if suggestions.length > 0}
{#each suggestions.filter(visible) as s (s.mbid)} @@ -66,11 +145,49 @@ kind="artist" title={s.name} imageUrl={s.image_url} - state="requestable" + state={cardState(s)} attribution={attributionText(s.attribution)} onRequest={() => onRequest(s)} + onSnooze={() => onSnooze(s)} + onUnsnooze={() => onUnsnooze(s.mbid, s.name)} /> {/each}
{/if} + + + {#if snoozes.length > 0} +
+

+ Not right now +

+

+ These come back on their own. Nothing here counts against your taste profile. +

+
    + {#each snoozes as snoozed (snoozed.mbid)} +
  • +
    +
    {snoozed.name}
    +
    + Back {returnsIn(snoozed.snoozed_until)} +
    +
    + +
  • + {/each} +
+
+ {/if}
diff --git a/web/src/lib/components/SuggestionFeed.test.ts b/web/src/lib/components/SuggestionFeed.test.ts index 0e6fdb43..984dedbb 100644 --- a/web/src/lib/components/SuggestionFeed.test.ts +++ b/web/src/lib/components/SuggestionFeed.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, test, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'; import { render, screen, fireEvent } from '@testing-library/svelte'; import { mockQuery } from '../../test-utils/query'; @@ -9,17 +9,30 @@ vi.mock('@tanstack/svelte-query', async (orig) => { }); vi.mock('$lib/api/suggestions', () => ({ - createSuggestionsQuery: vi.fn() + createSuggestionsQuery: vi.fn(), + createSnoozesQuery: vi.fn(), + snoozeSuggestion: vi.fn().mockResolvedValue(undefined), + unsnoozeSuggestion: vi.fn().mockResolvedValue(undefined) })); vi.mock('$lib/api/requests', () => ({ createRequest: vi.fn().mockResolvedValue({}) })); +const pushToastMock = vi.fn(); +vi.mock('$lib/stores/toast.svelte', () => ({ + pushToast: (...args: unknown[]) => pushToastMock(...args) +})); + import SuggestionFeed from './SuggestionFeed.svelte'; -import { createSuggestionsQuery } from '$lib/api/suggestions'; +import { + createSuggestionsQuery, + createSnoozesQuery, + snoozeSuggestion, + unsnoozeSuggestion +} from '$lib/api/suggestions'; import { createRequest } from '$lib/api/requests'; -import type { ArtistSuggestion } from '$lib/api/types'; +import type { ArtistSuggestion, SuggestionSnooze } from '$lib/api/types'; const oneSeed: ArtistSuggestion = { mbid: 'mb1', @@ -51,46 +64,50 @@ const threeSeeds: ArtistSuggestion = { ] }; +/** Days from now as an RFC3339 string, for snooze fixtures. */ +function inDays(n: number): string { + return new Date(Date.now() + n * 86_400_000).toISOString(); +} + +function setSuggestions(data: ArtistSuggestion[]) { + (createSuggestionsQuery as ReturnType).mockReturnValue(mockQuery({ data })); +} + +function setSnoozes(data: SuggestionSnooze[]) { + (createSnoozesQuery as ReturnType).mockReturnValue(mockQuery({ data })); +} + +beforeEach(() => setSnoozes([])); afterEach(() => vi.clearAllMocks()); describe('SuggestionFeed', () => { test('renders one card per suggestion', () => { - (createSuggestionsQuery as ReturnType).mockReturnValue( - mockQuery({ data: [oneSeed, twoSeeds] }) - ); + setSuggestions([oneSeed, twoSeeds]); render(SuggestionFeed); expect(screen.getByText('Outsider')).toBeInTheDocument(); expect(screen.getByText('Outsider Two')).toBeInTheDocument(); }); test('attribution copy: 1 seed → "Because you liked X."', () => { - (createSuggestionsQuery as ReturnType).mockReturnValue( - mockQuery({ data: [oneSeed] }) - ); + setSuggestions([oneSeed]); render(SuggestionFeed); expect(screen.getByText(/because you liked seed\./i)).toBeInTheDocument(); }); test('attribution copy: 2 seeds → "Because you liked A and played B."', () => { - (createSuggestionsQuery as ReturnType).mockReturnValue( - mockQuery({ data: [twoSeeds] }) - ); + setSuggestions([twoSeeds]); render(SuggestionFeed); expect(screen.getByText(/because you liked a and played b\./i)).toBeInTheDocument(); }); test('attribution copy: 3 seeds → Oxford comma', () => { - (createSuggestionsQuery as ReturnType).mockReturnValue( - mockQuery({ data: [threeSeeds] }) - ); + setSuggestions([threeSeeds]); render(SuggestionFeed); expect(screen.getByText(/because you liked x, played y, and played z\./i)).toBeInTheDocument(); }); test('Request button calls createRequest with artist-kind body', async () => { - (createSuggestionsQuery as ReturnType).mockReturnValue( - mockQuery({ data: [oneSeed] }) - ); + setSuggestions([oneSeed]); render(SuggestionFeed); await fireEvent.click(screen.getByRole('button', { name: /request outsider/i })); expect(createRequest).toHaveBeenCalledWith({ @@ -102,8 +119,113 @@ describe('SuggestionFeed', () => { }); test('empty state when data is []', () => { - (createSuggestionsQuery as ReturnType).mockReturnValue(mockQuery({ data: [] })); + setSuggestions([]); render(SuggestionFeed); expect(screen.getByText(/listen to something or like an artist/i)).toBeInTheDocument(); }); }); + +describe('SuggestionFeed snooze (#2375)', () => { + test('snooze sends BOTH mbid and name — the server 400s without the name', async () => { + setSuggestions([oneSeed]); + render(SuggestionFeed); + await fireEvent.click(screen.getByRole('button', { name: /not right now/i })); + expect(snoozeSuggestion).toHaveBeenCalledWith('mb1', 'Outsider'); + }); + + test('the card stays in place showing Undo, rather than vanishing', async () => { + setSuggestions([oneSeed]); + render(SuggestionFeed); + await fireEvent.click(screen.getByRole('button', { name: /not right now/i })); + // Still on screen — the disappearance happens on refetch, not under the + // cursor (rule #24). + expect(screen.getByText('Outsider')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /bring outsider back/i })).toBeInTheDocument(); + expect(screen.getByRole('status')).toHaveTextContent('Not right now'); + }); + + test('a failed snooze reverts the card and says so', async () => { + (snoozeSuggestion as ReturnType).mockRejectedValueOnce(new Error('offline')); + setSuggestions([oneSeed]); + render(SuggestionFeed); + await fireEvent.click(screen.getByRole('button', { name: /not right now/i })); + // Back to requestable — a snooze that silently did nothing would leave + // the user tapping it again. + expect(screen.getByRole('button', { name: /request outsider/i })).toBeInTheDocument(); + expect(pushToastMock).toHaveBeenCalledWith("Couldn't hide Outsider", 'error'); + }); + + test('undo on the card calls unsnoozeSuggestion', async () => { + setSuggestions([oneSeed]); + render(SuggestionFeed); + await fireEvent.click(screen.getByRole('button', { name: /not right now/i })); + await fireEvent.click(screen.getByRole('button', { name: /bring outsider back/i })); + expect(unsnoozeSuggestion).toHaveBeenCalledWith('mb1'); + }); + + test('the snoozed list is the way back once the card is gone', async () => { + // Deck empty, one parked artist: exactly the state after a refetch. + setSuggestions([]); + setSnoozes([ + { mbid: 'mbX', name: 'Parked', snoozed_until: inDays(90), created_at: inDays(0) } + ]); + render(SuggestionFeed); + expect(screen.getByRole('heading', { name: /not right now/i })).toBeInTheDocument(); + expect(screen.getByText('Parked')).toBeInTheDocument(); + await fireEvent.click(screen.getByRole('button', { name: /bring parked back now/i })); + expect(unsnoozeSuggestion).toHaveBeenCalledWith('mbX'); + }); + + test('a 404 from unsnooze is not surfaced as an error', async () => { + (unsnoozeSuggestion as ReturnType).mockRejectedValueOnce({ status: 404 }); + setSuggestions([]); + setSnoozes([ + { mbid: 'mbX', name: 'Parked', snoozed_until: inDays(90), created_at: inDays(0) } + ]); + render(SuggestionFeed); + await fireEvent.click(screen.getByRole('button', { name: /bring parked back now/i })); + // Already-unsnoozed IS the end state the user asked for. + expect(pushToastMock).not.toHaveBeenCalled(); + }); + + test('a non-404 unsnooze failure does surface', async () => { + (unsnoozeSuggestion as ReturnType).mockRejectedValueOnce({ status: 500 }); + setSuggestions([]); + setSnoozes([ + { mbid: 'mbX', name: 'Parked', snoozed_until: inDays(90), created_at: inDays(0) } + ]); + render(SuggestionFeed); + await fireEvent.click(screen.getByRole('button', { name: /bring parked back now/i })); + expect(pushToastMock).toHaveBeenCalledWith("Couldn't bring Parked back", 'error'); + }); + + test('return time reads as a relative phrase, not a calendar date', () => { + setSuggestions([]); + setSnoozes([ + { mbid: 'a', name: 'Quarter', snoozed_until: inDays(90), created_at: inDays(0) }, + { mbid: 'b', name: 'Fortnight', snoozed_until: inDays(14), created_at: inDays(0) } + ]); + render(SuggestionFeed); + expect(screen.getByText(/back in about 3 months/i)).toBeInTheDocument(); + expect(screen.getByText(/back in 14 days/i)).toBeInTheDocument(); + }); + + test('no snoozed section when nothing is parked', () => { + setSuggestions([oneSeed]); + setSnoozes([]); + render(SuggestionFeed); + expect(screen.queryByRole('heading', { name: /not right now/i })).not.toBeInTheDocument(); + }); + + // An empty deck has two causes now, and the advice differs. Telling someone + // who parked everything to go listen to music would be wrong. + test('empty-deck copy distinguishes "no signal" from "you parked them all"', () => { + setSuggestions([]); + setSnoozes([ + { mbid: 'a', name: 'Parked', snoozed_until: inDays(90), created_at: inDays(0) } + ]); + render(SuggestionFeed); + expect(screen.getByText(/nothing new right now/i)).toBeInTheDocument(); + expect(screen.queryByText(/listen to something or like an artist/i)).not.toBeInTheDocument(); + }); +}); From 18a61f106511f5280971c66f91cb29d02c15b8d4 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 2 Aug 2026 19:09:24 -0400 Subject: [PATCH 07/14] =?UTF-8?q?fix(discover):=20complete=20the=20page-te?= =?UTF-8?q?st=20mock=20+=20drop=20a=20return=20from=20returnsIn=20?= =?UTF-8?q?=E2=80=94=20#2375?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two CI failures from 6e39471a, both mechanical. web: src/routes/discover/discover.test.ts mocks $lib/api/suggestions with a factory, and SuggestionFeed now imports createSnoozesQuery from it. A factory-shaped module mock must export everything the component tree imports or rendering throws before any assertion runs — so all 12 of that suite's tests failed on a surface they don't even exercise. Stubbed the three new exports and defaulted the snooze query to empty, which keeps the feed's empty-state copy on the "no signal yet" branch those tests assert. (Same shape as Scribe #2109: when a shared component grows a dependency, the break is in unrelated fixtures, not assertions.) android: detekt ReturnCount — returnsIn had 3 returns against a limit of 2. Folded the two "nothing to state" guards into one by computing the remaining duration as a nullable up front. The Android compile and unit tests never ran on the last push: detekt gates them, so Lucide.Clock is still unproven. Co-Authored-By: Claude Opus 5 (1M context) --- .../com/fabledsword/minstrel/models/Discover.kt | 15 ++++++++------- web/src/routes/discover/discover.test.ts | 16 ++++++++++++++-- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/models/Discover.kt b/android/app/src/main/java/com/fabledsword/minstrel/models/Discover.kt index 4c1b39e4..817eb0aa 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/models/Discover.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/models/Discover.kt @@ -98,13 +98,14 @@ data class SuggestionSnoozeRef( * better than rendering an empty line. */ fun returnsIn(nowMs: Long = System.currentTimeMillis()): String { - val untilMs = runCatching { Instant.parse(snoozedUntil).toEpochMilliseconds() } - .getOrNull() ?: return "shortly" - // Already lapsed by our clock, yet the server still returned it — the - // two disagree. Say something plausible rather than "today", which - // would read as a real prediction. - if (untilMs <= nowMs) return "shortly" - val days = ((untilMs - nowMs).toDouble() / MILLIS_PER_DAY).roundToInt() + val remainingMs = runCatching { Instant.parse(snoozedUntil).toEpochMilliseconds() } + .getOrNull()?.minus(nowMs) + // Two ways to have nothing to state: an unparseable timestamp, or one + // already lapsed by our clock though the server still returned the row + // (the two disagree). Neither is "today", which would read as a real + // prediction. + if (remainingMs == null || remainingMs <= 0) return "shortly" + val days = (remainingMs.toDouble() / MILLIS_PER_DAY).roundToInt() return when { days < 1 -> "today" days == 1 -> "tomorrow" diff --git a/web/src/routes/discover/discover.test.ts b/web/src/routes/discover/discover.test.ts index c8cf2351..45b77c53 100644 --- a/web/src/routes/discover/discover.test.ts +++ b/web/src/routes/discover/discover.test.ts @@ -21,8 +21,16 @@ vi.mock('$lib/api/lidarr', () => ({ createLidarrSearchQuery: vi.fn() })); +// SuggestionFeed reaches for the snooze surface too (#2375). These are +// stubbed here even though this page-level suite asserts nothing about +// snoozing: a factory-shaped module mock must export everything the +// component tree imports, or rendering the feed throws before any +// assertion runs. vi.mock('$lib/api/suggestions', () => ({ - createSuggestionsQuery: vi.fn() + createSuggestionsQuery: vi.fn(), + createSnoozesQuery: vi.fn(), + snoozeSuggestion: vi.fn().mockResolvedValue(undefined), + unsnoozeSuggestion: vi.fn().mockResolvedValue(undefined) })); vi.mock('$lib/api/requests', () => ({ @@ -42,11 +50,12 @@ vi.mock('@tanstack/svelte-query', async (importOriginal) => { import DiscoverPage from './+page.svelte'; import { createLidarrSearchQuery } from '$lib/api/lidarr'; -import { createSuggestionsQuery } from '$lib/api/suggestions'; +import { createSuggestionsQuery, createSnoozesQuery } from '$lib/api/suggestions'; import { createRequest } from '$lib/api/requests'; const mockedCreateQuery = createLidarrSearchQuery as ReturnType; const mockedCreateSuggestionsQuery = createSuggestionsQuery as ReturnType; +const mockedCreateSnoozesQuery = createSnoozesQuery as ReturnType; const mockedCreateRequest = createRequest as ReturnType; function result(over: Partial = {}): LidarrSearchResult { @@ -69,6 +78,9 @@ beforeEach(() => { // Default: empty suggestion feed so its empty-state copy renders without // interfering with search-mode tests. mockedCreateSuggestionsQuery.mockReturnValue(mockQuery({ data: [] })); + // Nothing parked, which keeps the feed's empty-state copy on the + // "no signal yet" branch that this suite's assertions expect. + mockedCreateSnoozesQuery.mockReturnValue(mockQuery({ data: [] })); }); afterEach(() => { From f17356560d0bb7a5ff2562395a5519b8756f9c43 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 2 Aug 2026 19:19:05 -0400 Subject: [PATCH 08/14] =?UTF-8?q?fix(discover):=20"in=20about=20a=20month"?= =?UTF-8?q?=20was=20unreachable=20in=20both=20clients=20=E2=80=94=20#2375?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The days→months threshold (45) sat above the divisor (30), so a rounded month count of 1 — which needs 15..44 days — could never be reached: every one of those day counts hit the `in N days` branch first. The singular branch was dead code on Android AND web. Lowered the threshold to 30 in both clients, which makes 30..44 days read "in about a month" instead of "in 44 days", and documented the invariant (threshold must not exceed the divisor) next to each constant so the two can't drift apart again. Found by the unit test written for that branch, which is the whole reason to assert on copy that looks obviously correct. Both suites now pin the seam from both sides — 29 days and 30 days — so the branch can't go dead again silently. Co-Authored-By: Claude Opus 5 (1M context) --- .../com/fabledsword/minstrel/models/Discover.kt | 9 ++++++++- .../minstrel/models/SuggestionSnoozeRefTest.kt | 16 ++++++++++++++++ web/src/lib/components/SuggestionFeed.svelte | 7 ++++++- web/src/lib/components/SuggestionFeed.test.ts | 15 +++++++++++++++ 4 files changed, 45 insertions(+), 2 deletions(-) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/models/Discover.kt b/android/app/src/main/java/com/fabledsword/minstrel/models/Discover.kt index 817eb0aa..00173f1a 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/models/Discover.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/models/Discover.kt @@ -119,8 +119,15 @@ data class SuggestionSnoozeRef( private companion object { const val MILLIS_PER_DAY = 86_400_000.0 + // Below this, days read more naturally than a rounded month count. - const val DAYS_BEFORE_MONTHS = 45 + // + // Must be <= DAYS_PER_MONTH, or the singular "in about a month" is + // unreachable: a rounded month count of 1 needs 15..44 days, and any + // threshold above 30 sends all of those down the days branch instead. + // This was 45 and the singular branch was dead code — the unit test + // for it is what surfaced that. + const val DAYS_BEFORE_MONTHS = 30 const val DAYS_PER_MONTH = 30.0 } } diff --git a/android/app/src/test/java/com/fabledsword/minstrel/models/SuggestionSnoozeRefTest.kt b/android/app/src/test/java/com/fabledsword/minstrel/models/SuggestionSnoozeRefTest.kt index f1123e46..2dc598ec 100644 --- a/android/app/src/test/java/com/fabledsword/minstrel/models/SuggestionSnoozeRefTest.kt +++ b/android/app/src/test/java/com/fabledsword/minstrel/models/SuggestionSnoozeRefTest.kt @@ -35,6 +35,22 @@ class SuggestionSnoozeRefTest { assertEquals("in 14 days", snoozeIn(14.0).returnsIn(now)) } + // Pins the days→months boundary. The singular branch was originally dead + // code because the threshold (45) sat above the divisor (30), so no day + // count could ever round to one month without being caught by the days + // branch first. Asserting both sides of the seam keeps that from + // regressing silently. + @Test + fun `the days-to-months boundary is exactly at 30 days`() { + assertEquals("in 29 days", snoozeIn(29.0).returnsIn(now)) + assertEquals("in about a month", snoozeIn(30.0).returnsIn(now)) + } + + @Test + fun `well past a month still reads in the singular rather than jumping to two`() { + assertEquals("in about a month", snoozeIn(40.0).returnsIn(now)) + } + @Test fun `tomorrow is named, not rendered as 1 days`() { assertEquals("tomorrow", snoozeIn(1.0).returnsIn(now)) diff --git a/web/src/lib/components/SuggestionFeed.svelte b/web/src/lib/components/SuggestionFeed.svelte index 26df6ad3..abd91b7e 100644 --- a/web/src/lib/components/SuggestionFeed.svelte +++ b/web/src/lib/components/SuggestionFeed.svelte @@ -66,7 +66,12 @@ const days = Math.round(ms / 86_400_000); if (days < 1) return 'today'; if (days === 1) return 'tomorrow'; - if (days < 45) return `in ${days} days`; + // The 30 must not exceed the divisor below, or the singular "in about a + // month" is unreachable — a rounded month count of 1 needs 15..44 days, + // and any higher threshold sends all of those down the days branch. This + // read 45 and the singular case was dead code (caught by the Android + // unit test for the same logic). + if (days < 30) return `in ${days} days`; const months = Math.round(days / 30); return months === 1 ? 'in about a month' : `in about ${months} months`; } diff --git a/web/src/lib/components/SuggestionFeed.test.ts b/web/src/lib/components/SuggestionFeed.test.ts index 984dedbb..cebe14c5 100644 --- a/web/src/lib/components/SuggestionFeed.test.ts +++ b/web/src/lib/components/SuggestionFeed.test.ts @@ -210,6 +210,21 @@ describe('SuggestionFeed snooze (#2375)', () => { expect(screen.getByText(/back in 14 days/i)).toBeInTheDocument(); }); + // Pins the days→months seam. The singular branch was originally dead code + // here too: the threshold (45) sat above the divisor (30), so no day count + // could round to one month without hitting the days branch first. Kept in + // lockstep with SuggestionSnoozeRefTest on Android. + test('the days-to-months boundary sits at 30 days, so "a month" is reachable', () => { + setSuggestions([]); + setSnoozes([ + { mbid: 'a', name: 'JustUnder', snoozed_until: inDays(29), created_at: inDays(0) }, + { mbid: 'b', name: 'JustOver', snoozed_until: inDays(30), created_at: inDays(0) } + ]); + render(SuggestionFeed); + expect(screen.getByText(/back in 29 days/i)).toBeInTheDocument(); + expect(screen.getByText(/back in about a month/i)).toBeInTheDocument(); + }); + test('no snoozed section when nothing is parked', () => { setSuggestions([oneSeed]); setSnoozes([]); From 4f9b083eeca6c9f85d53801dd3b76bac8f367a93 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 2 Aug 2026 20:04:15 -0400 Subject: [PATCH 09/14] =?UTF-8?q?feat(discover):=20artist-tag=20cache=20fo?= =?UTF-8?q?r=20out-of-library=20candidates=20=E2=80=94=20#2376?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Migration 0050 adds candidate_artist_tags + candidate_artist_tag_state: folksonomy tags for artists NOT in the library, which track_tags cannot hold because it's FK'd to tracks(id) and a Discover candidate has no local row. Slice 6 ranks against these; this slice only fills the cache. The reuse the task claimed is real and verified: MusicBrainz's fetchEntityTags(ctx, "artist", mbid, scale) already existed for the #1519 recording→artist fallback, so FetchArtistTags is a thin wrapper. Two subtleties it does NOT inherit: - Weight scale is 1.0, not artistTagWeightFactor (0.6). That discount exists because FetchTrackTags uses artist tags as a *proxy* for a track's; here the artist IS the subject. Applying it would make these weights incomparable with track_tags — exactly the comparison slice 6 depends on. Pinned by a test. - fetchEntityTags reports existing-but-untagged as (empty, nil) so the track path can fall through. There's no next level here, so empty becomes the terminal ErrNotFound; otherwise the enricher would settle a candidate as "enriched" with zero tags. ArtistTagProvider is the split TrackTagProvider's own doc comment anticipated ("e.g. artist-level tags"). Last.fm gains artist.getTopTags, which returns the same toptags envelope, so the response type and normalizer are reused unchanged. Rather than write the merge-and-classify loop twice, extracted it from EnrichTrack into runChain(). The ErrNotFound-vs-transient split is the load-bearing part — those lead to opposite persistence decisions — so it now has direct unit tests it never had while inlined. Bookkeeping is a separate table, not columns, because the "providers had nothing" outcome must be recordable for a candidate with zero tag rows, and there is no per-candidate row to hang columns off ( artist_similarity_unmatched holds many rows per candidate). Absence of a state row means "never processed", so a transient failure writes nothing and stays eligible. Two capacity realities are designed for, not papered over: - The pool is O(library artists x neighbours) and MusicBrainz allows ~1 req/s, so it can never drain in one pass. The eligibility query returns candidates in descending summed-similarity order, so the ones that can actually reach a deck are enriched first. - candidateBatch (50) is smaller than the track batch (200): tracks are finite and drain to completion, candidates are effectively unbounded and would otherwise starve the track arm forever. GC sweeps both tables — the similarity feed churns, and a candidate that joins the library has its tags in track_tags now. Tags swept before state so a mid-sweep crash leaves a valid state, not a re-fetch loop. Co-Authored-By: Claude Opus 5 (1M context) --- internal/db/dbq/candidate_artist_tags.sql.go | 233 +++++++++++++++ internal/db/dbq/models.go | 13 + .../0050_candidate_artist_tags.down.sql | 3 + .../0050_candidate_artist_tags.up.sql | 54 ++++ internal/db/queries/candidate_artist_tags.sql | 102 +++++++ internal/dbtest/reset.go | 6 + internal/gc/worker.go | 9 + .../candidate_tags_integration_test.go | 282 ++++++++++++++++++ internal/tags/enricher.go | 238 +++++++++++++-- internal/tags/enricher_chain_test.go | 142 +++++++++ internal/tags/provider.go | 26 ++ internal/tags/provider_artist_tags_test.go | 173 +++++++++++ internal/tags/provider_lastfm.go | 49 ++- internal/tags/provider_musicbrainz.go | 32 +- internal/tags/settings.go | 21 ++ internal/tags/worker.go | 45 ++- 16 files changed, 1387 insertions(+), 41 deletions(-) create mode 100644 internal/db/dbq/candidate_artist_tags.sql.go create mode 100644 internal/db/migrations/0050_candidate_artist_tags.down.sql create mode 100644 internal/db/migrations/0050_candidate_artist_tags.up.sql create mode 100644 internal/db/queries/candidate_artist_tags.sql create mode 100644 internal/recommendation/candidate_tags_integration_test.go create mode 100644 internal/tags/enricher_chain_test.go create mode 100644 internal/tags/provider_artist_tags_test.go diff --git a/internal/db/dbq/candidate_artist_tags.sql.go b/internal/db/dbq/candidate_artist_tags.sql.go new file mode 100644 index 00000000..29ec71a9 --- /dev/null +++ b/internal/db/dbq/candidate_artist_tags.sql.go @@ -0,0 +1,233 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: candidate_artist_tags.sql + +package dbq + +import ( + "context" +) + +const countCandidateArtistTagCoverage = `-- name: CountCandidateArtistTagCoverage :one +SELECT count(*)::bigint AS processed, + count(*) FILTER (WHERE tag_source <> 'none')::bigint AS with_tags + FROM candidate_artist_tag_state +` + +type CountCandidateArtistTagCoverageRow struct { + Processed int64 + WithTags int64 +} + +// Operator-facing coverage: how many distinct candidates have been processed, +// and how many of those actually yielded tags. The gap is the honest ceiling +// from the task — obscure artists with no MBID presence or no upstream tags +// stay thin no matter how long the worker runs, and that is worth being able +// to see rather than inferring from a silent surface. +func (q *Queries) CountCandidateArtistTagCoverage(ctx context.Context) (CountCandidateArtistTagCoverageRow, error) { + row := q.db.QueryRow(ctx, countCandidateArtistTagCoverage) + var i CountCandidateArtistTagCoverageRow + err := row.Scan(&i.Processed, &i.WithTags) + return i, err +} + +const deleteCandidateArtistTags = `-- name: DeleteCandidateArtistTags :exec +DELETE FROM candidate_artist_tags WHERE candidate_mbid = $1 +` + +// Clear a candidate's cached tags before rewriting (atomic replace by the +// caller, same shape as DeleteTrackTags). +func (q *Queries) DeleteCandidateArtistTags(ctx context.Context, candidateMbid string) error { + _, err := q.db.Exec(ctx, deleteCandidateArtistTags, candidateMbid) + return err +} + +const gcDeleteOrphanedCandidateArtistTagState = `-- name: GcDeleteOrphanedCandidateArtistTagState :execrows +DELETE FROM candidate_artist_tag_state s + WHERE NOT EXISTS ( + SELECT 1 FROM artist_similarity_unmatched u + WHERE u.candidate_mbid = s.candidate_mbid + ) + OR EXISTS (SELECT 1 FROM artists a WHERE a.mbid = s.candidate_mbid) +` + +// Same sweep for the bookkeeping rows. Kept as a separate statement rather +// than a cascade: the two tables are independent by design (a 'none' outcome +// has state but no tags), so neither can be the parent of the other. +func (q *Queries) GcDeleteOrphanedCandidateArtistTagState(ctx context.Context) (int64, error) { + result, err := q.db.Exec(ctx, gcDeleteOrphanedCandidateArtistTagState) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + +const gcDeleteOrphanedCandidateArtistTags = `-- name: GcDeleteOrphanedCandidateArtistTags :execrows +DELETE FROM candidate_artist_tags t + WHERE NOT EXISTS ( + SELECT 1 FROM artist_similarity_unmatched u + WHERE u.candidate_mbid = t.candidate_mbid + ) + OR EXISTS (SELECT 1 FROM artists a WHERE a.mbid = t.candidate_mbid) +` + +// Drops cached tags for candidates that no longer appear in the similarity +// feed, or that have since been added to the library (their tags now live in +// track_tags). The feed is refetched periodically and churns, so without this +// the cache only ever grows. +func (q *Queries) GcDeleteOrphanedCandidateArtistTags(ctx context.Context) (int64, error) { + result, err := q.db.Exec(ctx, gcDeleteOrphanedCandidateArtistTags) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + +const insertCandidateArtistTag = `-- name: InsertCandidateArtistTag :exec +INSERT INTO candidate_artist_tags (candidate_mbid, tag, weight) +VALUES ($1, $2, $3) +ON CONFLICT (candidate_mbid, tag) + DO UPDATE SET weight = GREATEST(candidate_artist_tags.weight, EXCLUDED.weight) +` + +type InsertCandidateArtistTagParams struct { + CandidateMbid string + Tag string + Weight float64 +} + +// Upsert one (candidate, tag); keep the stronger weight when two providers +// agree on a tag with different folksonomy strengths. +func (q *Queries) InsertCandidateArtistTag(ctx context.Context, arg InsertCandidateArtistTagParams) error { + _, err := q.db.Exec(ctx, insertCandidateArtistTag, arg.CandidateMbid, arg.Tag, arg.Weight) + return err +} + +const listCandidateArtistTagsForMbids = `-- name: ListCandidateArtistTagsForMbids :many +SELECT candidate_mbid, tag, weight + FROM candidate_artist_tags + WHERE candidate_mbid = ANY($1::text[]) +` + +type ListCandidateArtistTagsForMbidsRow struct { + CandidateMbid string + Tag string + Weight float64 +} + +// Cached tags for a set of candidates, for slice 6's taste-overlap ranking. +// One row per (candidate, tag). +func (q *Queries) ListCandidateArtistTagsForMbids(ctx context.Context, dollar_1 []string) ([]ListCandidateArtistTagsForMbidsRow, error) { + rows, err := q.db.Query(ctx, listCandidateArtistTagsForMbids, dollar_1) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListCandidateArtistTagsForMbidsRow + for rows.Next() { + var i ListCandidateArtistTagsForMbidsRow + if err := rows.Scan(&i.CandidateMbid, &i.Tag, &i.Weight); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listCandidateArtistsMissingTags = `-- name: ListCandidateArtistsMissingTags :many + +SELECT u.candidate_mbid, + coalesce(max(u.candidate_name), '')::text AS candidate_name, + sum(u.score)::float8 AS total_score + FROM artist_similarity_unmatched u + LEFT JOIN candidate_artist_tag_state s ON s.candidate_mbid = u.candidate_mbid + WHERE NOT EXISTS (SELECT 1 FROM artists a WHERE a.mbid = u.candidate_mbid) + AND ( + s.candidate_mbid IS NULL + OR (s.tag_source = 'none' AND s.tag_sources_version < $1) + ) + GROUP BY u.candidate_mbid + ORDER BY total_score DESC, u.candidate_mbid + LIMIT $2 +` + +type ListCandidateArtistsMissingTagsParams struct { + TagSourcesVersion int32 + Limit int32 +} + +type ListCandidateArtistsMissingTagsRow struct { + CandidateMbid string + CandidateName string + TotalScore float64 +} + +// Folksonomy tags for out-of-library Discover candidates (#2376). Parallel to +// track_tags.sql, but keyed by MBID because the artist has no local row. See +// 0050_candidate_artist_tags.up.sql for why the bookkeeping is its own table. +// Candidates eligible for tag enrichment: never processed (no state row) or +// settled 'none' under an older provider version. +// +// artist_similarity_unmatched holds one row per (seed, candidate, source), so +// this GROUPs to one row per candidate — enriching the same MBID once per seed +// that pointed at it would multiply the API calls for no gain. +// +// ORDER BY summed similarity DESC is the load-bearing part. The candidate pool +// is O(library artists x neighbours per artist) — thousands — and MusicBrainz +// allows ~1 req/s, so it can NEVER be fully enriched in one pass. Draining in +// strength order means the candidates most likely to actually reach a user's +// deck get tags first, and the long tail fills in over subsequent ticks +// instead of starving behind it. +// +// Already-in-library candidates are skipped: they have an artists row, so +// their tags belong in track_tags, and the suggestion query filters them out +// anyway. $1 = current tag_sources_version, $2 = limit. +// candidate_name is coalesced to '' so it lands non-nullable in Go: the name is +// only a Last.fm lookup key, and empty simply means "MBID-keyed providers only", +// which the provider chain already handles. max() is an arbitrary-but- +// deterministic pick when several seeds spell the same MBID differently. +func (q *Queries) ListCandidateArtistsMissingTags(ctx context.Context, arg ListCandidateArtistsMissingTagsParams) ([]ListCandidateArtistsMissingTagsRow, error) { + rows, err := q.db.Query(ctx, listCandidateArtistsMissingTags, arg.TagSourcesVersion, arg.Limit) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListCandidateArtistsMissingTagsRow + for rows.Next() { + var i ListCandidateArtistsMissingTagsRow + if err := rows.Scan(&i.CandidateMbid, &i.CandidateName, &i.TotalScore); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const setCandidateArtistTagState = `-- name: SetCandidateArtistTagState :exec +INSERT INTO candidate_artist_tag_state (candidate_mbid, tag_source, tag_sources_version) +VALUES ($1, $2, $3) +ON CONFLICT (candidate_mbid) DO UPDATE + SET tag_source = EXCLUDED.tag_source, + tag_sources_version = EXCLUDED.tag_sources_version, + updated_at = now() +` + +type SetCandidateArtistTagStateParams struct { + CandidateMbid string + TagSource string + TagSourcesVersion int32 +} + +// Stamp the enrichment outcome so the drainer skips settled candidates. +// $2 = 'musicbrainz' | 'lastfm' | 'mixed' | 'none', $3 = current version. +func (q *Queries) SetCandidateArtistTagState(ctx context.Context, arg SetCandidateArtistTagStateParams) error { + _, err := q.db.Exec(ctx, setCandidateArtistTagState, arg.CandidateMbid, arg.TagSource, arg.TagSourcesVersion) + return err +} diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index cc61f970..d4ec39f7 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -240,6 +240,19 @@ type AuditLog struct { CreatedAt pgtype.Timestamptz } +type CandidateArtistTagState struct { + CandidateMbid string + TagSource string + TagSourcesVersion int32 + UpdatedAt pgtype.Timestamptz +} + +type CandidateArtistTag struct { + CandidateMbid string + Tag string + Weight float64 +} + type ContextualLike struct { ID pgtype.UUID UserID pgtype.UUID diff --git a/internal/db/migrations/0050_candidate_artist_tags.down.sql b/internal/db/migrations/0050_candidate_artist_tags.down.sql new file mode 100644 index 00000000..cd7cd862 --- /dev/null +++ b/internal/db/migrations/0050_candidate_artist_tags.down.sql @@ -0,0 +1,3 @@ +DROP INDEX IF EXISTS candidate_artist_tag_state_source_idx; +DROP TABLE IF EXISTS candidate_artist_tag_state; +DROP TABLE IF EXISTS candidate_artist_tags; diff --git a/internal/db/migrations/0050_candidate_artist_tags.up.sql b/internal/db/migrations/0050_candidate_artist_tags.up.sql new file mode 100644 index 00000000..660a3da7 --- /dev/null +++ b/internal/db/migrations/0050_candidate_artist_tags.up.sql @@ -0,0 +1,54 @@ +-- 0050_candidate_artist_tags.up.sql — folksonomy tags for OUT-OF-LIBRARY +-- artists (#2376, milestone #268 slice 5). +-- +-- track_tags (0042) cannot hold these: it is FK'd to tracks(id), and a +-- Discover candidate has no local row by definition. So this is a parallel +-- cache keyed by the candidate's MusicBrainz MBID — the only stable identity +-- available for an artist we don't have. +-- +-- Purpose is slice 6: rank suggestions by overlap between these tags and the +-- user's taste_profile_tags, turning "neighbour of an artist you play" into +-- "matches the sound you like". +-- +-- GLOBAL, not per-user: a candidate's tags are a property of the artist, not +-- of anyone's taste. Nothing here is user-scoped, so rule #47 has nothing to +-- scope — the per-user part lives entirely in slice 6's ranking. +-- +-- weight is a normalized folksonomy strength in [0,1], same scale as +-- track_tags, so the two can be compared without a conversion step. +CREATE TABLE candidate_artist_tags ( + candidate_mbid text NOT NULL, + tag text NOT NULL, + weight double precision NOT NULL DEFAULT 1, + PRIMARY KEY (candidate_mbid, tag) +); + +-- Enrichment bookkeeping. This is a SEPARATE table rather than columns on the +-- tags table, because the "providers had nothing" outcome must be recordable +-- for a candidate with zero tag rows — otherwise every empty candidate stays +-- eligible forever and the worker re-fetches it on every tick. +-- +-- tracks solved the same problem with columns on `tracks` (0042), but there is +-- no per-candidate row anywhere to hang them off: artist_similarity_unmatched +-- is keyed (seed_artist_id, candidate_mbid, source) and holds MANY rows per +-- candidate. +-- +-- Absence of a row here means "never processed", so unlike tracks.tag_source +-- this column can be NOT NULL — there is no null-means-pending state to model. +-- 'musicbrainz' | 'lastfm' | 'mixed' → found, cached +-- 'none' → providers confirmed nothing +-- tag_sources_version → bump to re-process settled 'none' +-- +-- A transient failure writes NO row at all (rather than a row it would then +-- have to distinguish), which leaves the candidate eligible for the next pass. +CREATE TABLE candidate_artist_tag_state ( + candidate_mbid text PRIMARY KEY, + tag_source text NOT NULL, + tag_sources_version integer NOT NULL DEFAULT 0, + updated_at timestamptz NOT NULL DEFAULT now() +); + +-- Serves the eligibility scan's "settled 'none' under an older version" +-- branch. The PK already covers the per-candidate lookups. +CREATE INDEX candidate_artist_tag_state_source_idx + ON candidate_artist_tag_state (tag_source, tag_sources_version); diff --git a/internal/db/queries/candidate_artist_tags.sql b/internal/db/queries/candidate_artist_tags.sql new file mode 100644 index 00000000..150b0637 --- /dev/null +++ b/internal/db/queries/candidate_artist_tags.sql @@ -0,0 +1,102 @@ +-- Folksonomy tags for out-of-library Discover candidates (#2376). Parallel to +-- track_tags.sql, but keyed by MBID because the artist has no local row. See +-- 0050_candidate_artist_tags.up.sql for why the bookkeeping is its own table. + +-- name: ListCandidateArtistsMissingTags :many +-- Candidates eligible for tag enrichment: never processed (no state row) or +-- settled 'none' under an older provider version. +-- +-- artist_similarity_unmatched holds one row per (seed, candidate, source), so +-- this GROUPs to one row per candidate — enriching the same MBID once per seed +-- that pointed at it would multiply the API calls for no gain. +-- +-- ORDER BY summed similarity DESC is the load-bearing part. The candidate pool +-- is O(library artists x neighbours per artist) — thousands — and MusicBrainz +-- allows ~1 req/s, so it can NEVER be fully enriched in one pass. Draining in +-- strength order means the candidates most likely to actually reach a user's +-- deck get tags first, and the long tail fills in over subsequent ticks +-- instead of starving behind it. +-- +-- Already-in-library candidates are skipped: they have an artists row, so +-- their tags belong in track_tags, and the suggestion query filters them out +-- anyway. $1 = current tag_sources_version, $2 = limit. +-- candidate_name is coalesced to '' so it lands non-nullable in Go: the name is +-- only a Last.fm lookup key, and empty simply means "MBID-keyed providers only", +-- which the provider chain already handles. max() is an arbitrary-but- +-- deterministic pick when several seeds spell the same MBID differently. +SELECT u.candidate_mbid, + coalesce(max(u.candidate_name), '')::text AS candidate_name, + sum(u.score)::float8 AS total_score + FROM artist_similarity_unmatched u + LEFT JOIN candidate_artist_tag_state s ON s.candidate_mbid = u.candidate_mbid + WHERE NOT EXISTS (SELECT 1 FROM artists a WHERE a.mbid = u.candidate_mbid) + AND ( + s.candidate_mbid IS NULL + OR (s.tag_source = 'none' AND s.tag_sources_version < $1) + ) + GROUP BY u.candidate_mbid + ORDER BY total_score DESC, u.candidate_mbid + LIMIT $2; + +-- name: DeleteCandidateArtistTags :exec +-- Clear a candidate's cached tags before rewriting (atomic replace by the +-- caller, same shape as DeleteTrackTags). +DELETE FROM candidate_artist_tags WHERE candidate_mbid = $1; + +-- name: InsertCandidateArtistTag :exec +-- Upsert one (candidate, tag); keep the stronger weight when two providers +-- agree on a tag with different folksonomy strengths. +INSERT INTO candidate_artist_tags (candidate_mbid, tag, weight) +VALUES ($1, $2, $3) +ON CONFLICT (candidate_mbid, tag) + DO UPDATE SET weight = GREATEST(candidate_artist_tags.weight, EXCLUDED.weight); + +-- name: SetCandidateArtistTagState :exec +-- Stamp the enrichment outcome so the drainer skips settled candidates. +-- $2 = 'musicbrainz' | 'lastfm' | 'mixed' | 'none', $3 = current version. +INSERT INTO candidate_artist_tag_state (candidate_mbid, tag_source, tag_sources_version) +VALUES ($1, $2, $3) +ON CONFLICT (candidate_mbid) DO UPDATE + SET tag_source = EXCLUDED.tag_source, + tag_sources_version = EXCLUDED.tag_sources_version, + updated_at = now(); + +-- name: ListCandidateArtistTagsForMbids :many +-- Cached tags for a set of candidates, for slice 6's taste-overlap ranking. +-- One row per (candidate, tag). +SELECT candidate_mbid, tag, weight + FROM candidate_artist_tags + WHERE candidate_mbid = ANY($1::text[]); + +-- name: CountCandidateArtistTagCoverage :one +-- Operator-facing coverage: how many distinct candidates have been processed, +-- and how many of those actually yielded tags. The gap is the honest ceiling +-- from the task — obscure artists with no MBID presence or no upstream tags +-- stay thin no matter how long the worker runs, and that is worth being able +-- to see rather than inferring from a silent surface. +SELECT count(*)::bigint AS processed, + count(*) FILTER (WHERE tag_source <> 'none')::bigint AS with_tags + FROM candidate_artist_tag_state; + +-- name: GcDeleteOrphanedCandidateArtistTags :execrows +-- Drops cached tags for candidates that no longer appear in the similarity +-- feed, or that have since been added to the library (their tags now live in +-- track_tags). The feed is refetched periodically and churns, so without this +-- the cache only ever grows. +DELETE FROM candidate_artist_tags t + WHERE NOT EXISTS ( + SELECT 1 FROM artist_similarity_unmatched u + WHERE u.candidate_mbid = t.candidate_mbid + ) + OR EXISTS (SELECT 1 FROM artists a WHERE a.mbid = t.candidate_mbid); + +-- name: GcDeleteOrphanedCandidateArtistTagState :execrows +-- Same sweep for the bookkeeping rows. Kept as a separate statement rather +-- than a cascade: the two tables are independent by design (a 'none' outcome +-- has state but no tags), so neither can be the parent of the other. +DELETE FROM candidate_artist_tag_state s + WHERE NOT EXISTS ( + SELECT 1 FROM artist_similarity_unmatched u + WHERE u.candidate_mbid = s.candidate_mbid + ) + OR EXISTS (SELECT 1 FROM artists a WHERE a.mbid = s.candidate_mbid); diff --git a/internal/dbtest/reset.go b/internal/dbtest/reset.go index 180690ce..b39669cc 100644 --- a/internal/dbtest/reset.go +++ b/internal/dbtest/reset.go @@ -58,6 +58,12 @@ var dataTables = []string{ // explicitly or a stale snooze silently hides a candidate from the // next test's suggestion assertions. "suggestion_snoozes", + // #2376. Same reasoning: keyed by candidate MBID with no FK anywhere, + // so nothing cascades to them. A leftover tag row would make a + // candidate look enriched to the next test, and a leftover state row + // would make it look already-settled and thus ineligible. + "candidate_artist_tags", + "candidate_artist_tag_state", "playlist_tracks", "playlists", "library_changes", // M7 #357 — must reset to keep cursor isolated per test diff --git a/internal/gc/worker.go b/internal/gc/worker.go index b38dc8f6..39ca9a3c 100644 --- a/internal/gc/worker.go +++ b/internal/gc/worker.go @@ -18,6 +18,8 @@ // - GcDeleteExpiredPasswordResets (#575) // - GcPruneDiagnostics (M9 — diagnostics 30d retention) // - GcDeleteExpiredSuggestionSnoozes (#2374 — snoozes expire, then go) +// - GcDeleteOrphanedCandidateArtistTags(+State) (#2376 — the similarity +// feed churns, so cached candidate tags outlive their candidates) package gc import ( @@ -86,6 +88,13 @@ func (w *Worker) tickOnce(ctx context.Context) { w.runSweep(ctx, "delete_expired_password_resets", q.GcDeleteExpiredPasswordResets) w.runSweep(ctx, "prune_diagnostics", q.GcPruneDiagnostics) w.runSweep(ctx, "delete_expired_suggestion_snoozes", q.GcDeleteExpiredSuggestionSnoozes) + // Tags before state: if the process dies between the two, a candidate left + // with a state row and no tags simply reads as "settled, nothing found", + // which is already a valid state. The reverse order could leave tags with + // no state row, which the drainer would treat as never-processed and + // re-fetch on top of rows that are already there. + w.runSweep(ctx, "orphaned_candidate_artist_tags", q.GcDeleteOrphanedCandidateArtistTags) + w.runSweep(ctx, "orphaned_candidate_artist_tag_state", q.GcDeleteOrphanedCandidateArtistTagState) } // runSweep is a small adapter so each sweep call site is a one-liner diff --git a/internal/recommendation/candidate_tags_integration_test.go b/internal/recommendation/candidate_tags_integration_test.go new file mode 100644 index 00000000..eda08a71 --- /dev/null +++ b/internal/recommendation/candidate_tags_integration_test.go @@ -0,0 +1,282 @@ +package recommendation + +import ( + "context" + "testing" + + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// Slice 5 (#2376): the candidate-artist tag cache for out-of-library Discover +// candidates. These cover the SQL rather than the provider chain — the +// eligibility query is the piece with real risk in it (a GROUP BY over a +// many-rows-per-candidate table, a LEFT JOIN to bookkeeping, and two exclusion +// branches), and it's consumed by this package's slice-6 ranking. +// +// Fixtures live here because the harness and seedUnmatched do. + +const ( + tagVersionCurrent = 2 + tagVersionOld = 1 +) + +// listEligible is the query under test, at the current provider version. +func listEligible(t *testing.T, pool *pgxpool.Pool, limit int32) []dbq.ListCandidateArtistsMissingTagsRow { + t.Helper() + rows, err := dbq.New(pool).ListCandidateArtistsMissingTags(context.Background(), + dbq.ListCandidateArtistsMissingTagsParams{ + TagSourcesVersion: tagVersionCurrent, + Limit: limit, + }) + if err != nil { + t.Fatalf("ListCandidateArtistsMissingTags: %v", err) + } + return rows +} + +func setState(t *testing.T, pool *pgxpool.Pool, mbid, source string, version int32) { + t.Helper() + if err := dbq.New(pool).SetCandidateArtistTagState(context.Background(), + dbq.SetCandidateArtistTagStateParams{ + CandidateMbid: mbid, TagSource: source, TagSourcesVersion: version, + }); err != nil { + t.Fatalf("SetCandidateArtistTagState: %v", err) + } +} + +func mbidsOf(rows []dbq.ListCandidateArtistsMissingTagsRow) []string { + out := make([]string, 0, len(rows)) + for _, r := range rows { + out = append(out, r.CandidateMbid) + } + return out +} + +func TestCandidateTags_NeverProcessedIsEligible(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + seed := seedArtist(t, pool, "Seed", "") + likeArtist(t, pool, user.ID, seed.ID) + seedUnmatched(t, pool, seed.ID, "cand-1", "Candidate One", 0.9) + + rows := listEligible(t, pool, 10) + if len(rows) != 1 { + t.Fatalf("len = %d, want 1: %v", len(rows), mbidsOf(rows)) + } + if rows[0].CandidateName != "Candidate One" { + t.Errorf("name = %q, want Candidate One", rows[0].CandidateName) + } + if rows[0].TotalScore != 0.9 { + t.Errorf("score = %v, want 0.9", rows[0].TotalScore) + } +} + +// An in-library candidate's tags belong in track_tags, and the suggestion query +// filters it out anyway — enriching it would be wasted API budget. +func TestCandidateTags_InLibraryCandidateIsExcluded(t *testing.T) { + pool := newPool(t) + seed := seedArtist(t, pool, "Seed", "") + seedArtist(t, pool, "Already Here", "cand-in-lib") + seedUnmatched(t, pool, seed.ID, "cand-in-lib", "Already Here", 0.9) + + if rows := listEligible(t, pool, 10); len(rows) != 0 { + t.Errorf("len = %d, want 0: %v", len(rows), mbidsOf(rows)) + } +} + +func TestCandidateTags_SettledWithTagsIsExcluded(t *testing.T) { + pool := newPool(t) + seed := seedArtist(t, pool, "Seed", "") + seedUnmatched(t, pool, seed.ID, "cand-1", "Candidate One", 0.9) + setState(t, pool, "cand-1", "musicbrainz", tagVersionCurrent) + + if rows := listEligible(t, pool, 10); len(rows) != 0 { + t.Errorf("len = %d, want 0 (already enriched): %v", len(rows), mbidsOf(rows)) + } +} + +// A candidate that settled 'none' becomes eligible again when the provider set +// widens (version bump) — that's the whole point of the version column. It must +// NOT be eligible at the current version, or the worker re-fetches it forever. +func TestCandidateTags_SettledNoneReopensOnlyOnVersionBump(t *testing.T) { + pool := newPool(t) + seed := seedArtist(t, pool, "Seed", "") + seedUnmatched(t, pool, seed.ID, "cand-1", "Candidate One", 0.9) + + setState(t, pool, "cand-1", "none", tagVersionCurrent) + if rows := listEligible(t, pool, 10); len(rows) != 0 { + t.Errorf("current version: len = %d, want 0 (settled)", len(rows)) + } + + setState(t, pool, "cand-1", "none", tagVersionOld) + if rows := listEligible(t, pool, 10); len(rows) != 1 { + t.Errorf("older version: len = %d, want 1 (eligible again)", len(rows)) + } +} + +// artist_similarity_unmatched holds one row per (seed, candidate, source). +// Without the GROUP BY, a candidate that five seeds point at would be fetched +// five times — five times the MusicBrainz budget for identical data. +func TestCandidateTags_ManySeedsCollapseToOneRowAndSumScores(t *testing.T) { + pool := newPool(t) + seedA := seedArtist(t, pool, "Seed A", "") + seedB := seedArtist(t, pool, "Seed B", "") + seedUnmatched(t, pool, seedA.ID, "cand-1", "Candidate One", 0.4) + seedUnmatched(t, pool, seedB.ID, "cand-1", "Candidate One", 0.3) + + rows := listEligible(t, pool, 10) + if len(rows) != 1 { + t.Fatalf("len = %d, want 1 (grouped): %v", len(rows), mbidsOf(rows)) + } + if got := rows[0].TotalScore; got < 0.69 || got > 0.71 { + t.Errorf("total_score = %v, want ~0.7 (summed across seeds)", got) + } +} + +// The pool is far larger than one pass can drain at ~1 req/s, so the ordering +// IS the feature: the strongest candidates must be enriched first, or the ones +// that actually reach a user's deck starve behind the long tail. +func TestCandidateTags_StrongestCandidatesComeFirst(t *testing.T) { + pool := newPool(t) + seed := seedArtist(t, pool, "Seed", "") + seedUnmatched(t, pool, seed.ID, "weak", "Weak", 0.1) + seedUnmatched(t, pool, seed.ID, "strong", "Strong", 0.95) + seedUnmatched(t, pool, seed.ID, "middle", "Middle", 0.5) + + rows := listEligible(t, pool, 10) + want := []string{"strong", "middle", "weak"} + got := mbidsOf(rows) + if len(got) != 3 { + t.Fatalf("len = %d, want 3: %v", len(got), got) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("order = %v, want %v", got, want) + } + } + + // And the limit takes the strongest, not an arbitrary slice. + if top := mbidsOf(listEligible(t, pool, 1)); len(top) != 1 || top[0] != "strong" { + t.Errorf("limit 1 returned %v, want [strong]", top) + } +} + +func TestCandidateTags_InsertKeepsTheStrongerWeight(t *testing.T) { + pool := newPool(t) + q := dbq.New(pool) + ctx := context.Background() + ins := func(w float64) { + if err := q.InsertCandidateArtistTag(ctx, dbq.InsertCandidateArtistTagParams{ + CandidateMbid: "cand-1", Tag: "shoegaze", Weight: w, + }); err != nil { + t.Fatalf("InsertCandidateArtistTag: %v", err) + } + } + ins(0.8) + ins(0.3) // weaker second write must not clobber + + rows, err := q.ListCandidateArtistTagsForMbids(ctx, []string{"cand-1"}) + if err != nil { + t.Fatalf("ListCandidateArtistTagsForMbids: %v", err) + } + if len(rows) != 1 { + t.Fatalf("len = %d, want 1", len(rows)) + } + if rows[0].Weight != 0.8 { + t.Errorf("weight = %v, want 0.8 (GREATEST)", rows[0].Weight) + } +} + +func TestCandidateTags_ListForMbidsIgnoresUnaskedCandidates(t *testing.T) { + pool := newPool(t) + q := dbq.New(pool) + ctx := context.Background() + for _, mbid := range []string{"want-1", "want-2", "other"} { + if err := q.InsertCandidateArtistTag(ctx, dbq.InsertCandidateArtistTagParams{ + CandidateMbid: mbid, Tag: "rock", Weight: 1, + }); err != nil { + t.Fatalf("insert %s: %v", mbid, err) + } + } + rows, err := q.ListCandidateArtistTagsForMbids(ctx, []string{"want-1", "want-2"}) + if err != nil { + t.Fatalf("ListCandidateArtistTagsForMbids: %v", err) + } + if len(rows) != 2 { + t.Errorf("len = %d, want 2", len(rows)) + } + for _, r := range rows { + if r.CandidateMbid == "other" { + t.Error("returned a candidate that wasn't asked for") + } + } +} + +// The similarity feed is refetched and churns, so without the sweep the cache +// only grows. Both halves must survive/die together for the right candidates. +func TestCandidateTags_GcDropsOrphansAndKeepsLiveOnes(t *testing.T) { + pool := newPool(t) + q := dbq.New(pool) + ctx := context.Background() + + seed := seedArtist(t, pool, "Seed", "") + seedUnmatched(t, pool, seed.ID, "live", "Live", 0.9) + // "gone" is cached but no longer in the feed; "adopted" got added to the + // library since, so its tags belong in track_tags now. + seedArtist(t, pool, "Adopted", "adopted") + seedUnmatched(t, pool, seed.ID, "adopted", "Adopted", 0.8) + + for _, mbid := range []string{"live", "gone", "adopted"} { + if err := q.InsertCandidateArtistTag(ctx, dbq.InsertCandidateArtistTagParams{ + CandidateMbid: mbid, Tag: "rock", Weight: 1, + }); err != nil { + t.Fatalf("insert %s: %v", mbid, err) + } + setState(t, pool, mbid, "musicbrainz", tagVersionCurrent) + } + + deleted, err := q.GcDeleteOrphanedCandidateArtistTags(ctx) + if err != nil { + t.Fatalf("GcDeleteOrphanedCandidateArtistTags: %v", err) + } + if deleted != 2 { + t.Errorf("deleted %d tag rows, want 2 (gone + adopted)", deleted) + } + deletedState, err := q.GcDeleteOrphanedCandidateArtistTagState(ctx) + if err != nil { + t.Fatalf("GcDeleteOrphanedCandidateArtistTagState: %v", err) + } + if deletedState != 2 { + t.Errorf("deleted %d state rows, want 2", deletedState) + } + + rows, err := q.ListCandidateArtistTagsForMbids(ctx, []string{"live", "gone", "adopted"}) + if err != nil { + t.Fatalf("ListCandidateArtistTagsForMbids: %v", err) + } + if len(rows) != 1 || rows[0].CandidateMbid != "live" { + t.Errorf("survivors = %v, want [live] only", rows) + } +} + +// Coverage is the operator's window onto the honest ceiling: processed vs +// actually-tagged. A big gap means thin upstream data, not a broken worker. +func TestCandidateTags_CoverageCountsProcessedAndTagged(t *testing.T) { + pool := newPool(t) + setState(t, pool, "has-tags", "musicbrainz", tagVersionCurrent) + setState(t, pool, "mixed-tags", "mixed", tagVersionCurrent) + setState(t, pool, "no-tags", "none", tagVersionCurrent) + + got, err := dbq.New(pool).CountCandidateArtistTagCoverage(context.Background()) + if err != nil { + t.Fatalf("CountCandidateArtistTagCoverage: %v", err) + } + if got.Processed != 3 { + t.Errorf("processed = %d, want 3", got.Processed) + } + if got.WithTags != 2 { + t.Errorf("with_tags = %d, want 2 ('none' excluded)", got.WithTags) + } +} diff --git a/internal/tags/enricher.go b/internal/tags/enricher.go index 60b9909f..b41ac4cb 100644 --- a/internal/tags/enricher.go +++ b/internal/tags/enricher.go @@ -61,40 +61,25 @@ func NewEnricher(pool *pgxpool.Pool, logger *slog.Logger, settings *SettingsServ // enabled) the row settles to 'none'. If a provider fails transiently and // no tags surfaced, the row is left NULL for a next-pass retry. func (e *Enricher) EnrichTrack(ctx context.Context, trackID pgtype.UUID, ref TrackRef) (outcome, error) { - merged := map[string]float64{} - contributors := map[string]bool{} - anyTransient := false - - for _, provider := range e.settings.EnabledTrackTagProviders() { - tags, perr := provider.FetchTrackTags(ctx, ref) - switch { - case perr == nil: - for _, t := range tags { - if t.Weight > merged[t.Name] { - merged[t.Name] = t.Weight - } - } - if len(tags) > 0 { - contributors[provider.ID()] = true - } - case errors.Is(perr, ErrNotFound): - // Clean "no data from this source" — try the next provider. - default: - anyTransient = true - e.logger.Warn("tags: provider fetch failed; continuing", - "track_id", uuidString(trackID), "provider", provider.ID(), "err", perr) - } + providers := e.settings.EnabledTrackTagProviders() + calls := make([]tagFetch, 0, len(providers)) + for _, provider := range providers { + calls = append(calls, tagFetch{ + providerID: provider.ID(), + fetch: func(c context.Context) ([]Tag, error) { return provider.FetchTrackTags(c, ref) }, + }) } + res := e.runChain(ctx, calls, "track_id", uuidString(trackID)) - if len(merged) > 0 { - top := topKByWeight(merged, e.topK) - source := sourceLabel(contributorIDs(contributors)) + if len(res.merged) > 0 { + top := topKByWeight(res.merged, e.topK) + source := sourceLabel(res.contributors) if err := e.writeTags(ctx, trackID, top, source, e.settings.CurrentVersion()); err != nil { return outcomeLeftNull, err } return outcomeEnriched, nil } - if anyTransient { + if res.anyTransient { // Nothing landed but a source may recover — leave NULL for retry. return outcomeLeftNull, nil } @@ -106,6 +91,65 @@ func (e *Enricher) EnrichTrack(ctx context.Context, trackID pgtype.UUID, ref Tra return outcomeNone, nil } +// tagFetch pairs a provider ID with a bound fetch call, so the merge-and- +// classify loop below is shared between the track chain and the candidate- +// artist chain (#2376) instead of being written twice with one word changed. +type tagFetch struct { + providerID string + fetch func(context.Context) ([]Tag, error) +} + +// chainResult is what running a provider chain produced. contributors is the +// sorted set of provider IDs that actually returned tags — the input to +// sourceLabel. +type chainResult struct { + merged map[string]float64 + contributors []string + anyTransient bool +} + +// runChain queries every provider in order and UNIONS the results (max weight +// wins on overlap), unlike coverart's first-success-wins. +// +// A clean ErrNotFound means "this source has nothing" and moves to the next. +// Anything else is transient and recorded, so the caller can leave the row +// eligible for a retry rather than wrongly settling it as "nothing exists" — +// the distinction between those two is the whole point of the return value. +// +// logKey/logVal identify the subject in warnings (a track id or a candidate +// MBID), since this is shared across entity types. +func (e *Enricher) runChain(ctx context.Context, calls []tagFetch, logKey, logVal string) chainResult { + merged := map[string]float64{} + contributors := map[string]bool{} + anyTransient := false + + for _, c := range calls { + tags, perr := c.fetch(ctx) + switch { + case perr == nil: + for _, t := range tags { + if t.Weight > merged[t.Name] { + merged[t.Name] = t.Weight + } + } + if len(tags) > 0 { + contributors[c.providerID] = true + } + case errors.Is(perr, ErrNotFound): + // Clean "no data from this source" — try the next provider. + default: + anyTransient = true + e.logger.Warn("tags: provider fetch failed; continuing", + logKey, logVal, "provider", c.providerID, "err", perr) + } + } + return chainResult{ + merged: merged, + contributors: contributorIDs(contributors), + anyTransient: anyTransient, + } +} + // writeTags atomically replaces a track's cached tags and stamps the source // + version. tags may be empty (the 'none' settle path), which just clears // any prior tags and records the outcome. @@ -196,6 +240,146 @@ func (e *Enricher) EnrichTrackBatch(ctx context.Context, limit int, return processed, enriched, settledNone + leftNull + errored, nil } +// EnrichCandidateArtist runs the artist-tag chain for one out-of-library +// Discover candidate and caches the merged result (#2376). +// +// Mirrors EnrichTrack, with one deliberate difference in the transient case: +// there is no row to "leave NULL", because eligibility is the ABSENCE of a +// candidate_artist_tag_state row. So a transient failure writes nothing at all, +// which leaves the candidate eligible for the next tick. Writing a state row +// here would settle a candidate whose tags we simply failed to fetch. +func (e *Enricher) EnrichCandidateArtist(ctx context.Context, mbid, name string) (outcome, error) { + providers := e.settings.EnabledArtistTagProviders() + ref := ArtistRef{MBID: mbid, Name: name} + calls := make([]tagFetch, 0, len(providers)) + for _, provider := range providers { + calls = append(calls, tagFetch{ + providerID: provider.ID(), + fetch: func(c context.Context) ([]Tag, error) { return provider.FetchArtistTags(c, ref) }, + }) + } + res := e.runChain(ctx, calls, "candidate_mbid", mbid) + version := e.settings.CurrentVersion() + + if len(res.merged) > 0 { + top := topKByWeight(res.merged, e.topK) + if err := e.writeCandidateTags(ctx, mbid, top, sourceLabel(res.contributors), version); err != nil { + return outcomeLeftNull, err + } + return outcomeEnriched, nil + } + if res.anyTransient { + return outcomeLeftNull, nil + } + if err := e.writeCandidateTags(ctx, mbid, nil, sourceNone, version); err != nil { + return outcomeNone, err + } + return outcomeNone, nil +} + +// writeCandidateTags atomically replaces a candidate's cached tags and stamps +// its state. tags may be empty (the 'none' settle path), which clears any prior +// tags and records that the providers had nothing. +func (e *Enricher) writeCandidateTags( + ctx context.Context, mbid string, tags []Tag, source string, version int32, +) error { + tx, err := e.pool.Begin(ctx) + if err != nil { + return fmt.Errorf("begin tx: %w", err) + } + defer func() { _ = tx.Rollback(ctx) }() + q := dbq.New(tx) + if err := q.DeleteCandidateArtistTags(ctx, mbid); err != nil { + return fmt.Errorf("delete candidate artist tags: %w", err) + } + for _, t := range tags { + if err := q.InsertCandidateArtistTag(ctx, dbq.InsertCandidateArtistTagParams{ + CandidateMbid: mbid, Tag: t.Name, Weight: t.Weight, + }); err != nil { + return fmt.Errorf("insert candidate artist tag: %w", err) + } + } + if err := q.SetCandidateArtistTagState(ctx, dbq.SetCandidateArtistTagStateParams{ + CandidateMbid: mbid, + TagSource: source, + TagSourcesVersion: version, + }); err != nil { + return fmt.Errorf("set candidate artist tag state: %w", err) + } + if err := tx.Commit(ctx); err != nil { + return fmt.Errorf("commit: %w", err) + } + return nil +} + +// EnrichCandidateArtistBatch drains up to limit out-of-library candidates and +// enriches each serially. Same limit semantics as EnrichTrackBatch: 0 = +// disabled, >0 = bounded, <0 = unbounded. +// +// The candidate pool is far larger than the track pool (every library artist's +// neighbours) and can never be drained in one pass at MusicBrainz's ~1 req/s, +// so the query hands them back in descending similarity order — see +// ListCandidateArtistsMissingTags. A bounded batch here is therefore normal +// operation, not a degraded mode. +func (e *Enricher) EnrichCandidateArtistBatch(ctx context.Context, limit int) ( + processed, succeeded, failed int, err error, +) { + if limit == 0 { + return 0, 0, 0, nil + } + queryLimit := int32(limit) + if limit < 0 { + queryLimit = 1<<31 - 1 + } + q := dbq.New(e.pool) + rows, qerr := q.ListCandidateArtistsMissingTags(ctx, dbq.ListCandidateArtistsMissingTagsParams{ + TagSourcesVersion: e.settings.CurrentVersion(), + Limit: queryLimit, + }) + if qerr != nil { + return 0, 0, 0, fmt.Errorf("list candidate artists missing tags: %w", qerr) + } + + var enriched, settledNone, leftNull, errored int + for _, r := range rows { + if ctx.Err() != nil { + e.logCandidateBatchSummary(len(rows), processed, enriched, settledNone, leftNull, errored) + return processed, enriched, settledNone + leftNull + errored, ctx.Err() + } + processed++ + oc, eerr := e.EnrichCandidateArtist(ctx, r.CandidateMbid, r.CandidateName) + if eerr != nil { + e.logger.Warn("tags: candidate batch entry failed", + "candidate_mbid", r.CandidateMbid, "err", eerr) + errored++ + continue + } + switch oc { + case outcomeEnriched: + enriched++ + case outcomeNone: + settledNone++ + case outcomeLeftNull: + leftNull++ + } + } + e.logCandidateBatchSummary(len(rows), processed, enriched, settledNone, leftNull, errored) + return processed, enriched, settledNone + leftNull + errored, nil +} + +// logCandidateBatchSummary mirrors logBatchSummary. `settled_none` is the +// honest-ceiling counter for this surface: candidates whose MBID has no +// upstream tags at all, which no amount of retrying will fix. +func (e *Enricher) logCandidateBatchSummary(eligible, processed, enriched, settledNone, leftNull, errored int) { + e.logger.Info("tags: candidate-artist enrichment batch complete", + "eligible", eligible, + "processed", processed, + "enriched", enriched, + "settled_none", settledNone, + "left_null", leftNull, + "errored", errored) +} + // logBatchSummary emits one Info line with the category breakdown — the // enriched/settled/left-null split is the operator's diagnostic for a // "0 enriched" symptom the collapsed tally can't explain. diff --git a/internal/tags/enricher_chain_test.go b/internal/tags/enricher_chain_test.go new file mode 100644 index 00000000..23b73fb4 --- /dev/null +++ b/internal/tags/enricher_chain_test.go @@ -0,0 +1,142 @@ +package tags + +import ( + "context" + "errors" + "io" + "log/slog" + "testing" +) + +// runChain is the merge-and-classify loop shared by the track and +// candidate-artist drains (#2376). It touches no DB, so it is testable with a +// bare Enricher. +// +// The classification is the part that matters: "every source cleanly had +// nothing" and "a source failed" lead to opposite persistence decisions +// (settle vs. leave eligible for retry), and conflating them either writes off +// an artist over a transient blip or re-fetches a genuinely untagged one +// forever. + +func chainEnricher() *Enricher { + return &Enricher{logger: slog.New(slog.NewTextHandler(io.Discard, nil))} +} + +func fixedFetch(tags []Tag, err error) func(context.Context) ([]Tag, error) { + return func(context.Context) ([]Tag, error) { return tags, err } +} + +func TestRunChain_UnionsAcrossProvidersMaxWeightWins(t *testing.T) { + res := chainEnricher().runChain(context.Background(), []tagFetch{ + {providerID: "musicbrainz", fetch: fixedFetch([]Tag{ + {Name: "shoegaze", Weight: 0.4}, + {Name: "noise", Weight: 0.9}, + }, nil)}, + {providerID: "lastfm", fetch: fixedFetch([]Tag{ + {Name: "shoegaze", Weight: 0.8}, // higher — should win + {Name: "dream pop", Weight: 0.3}, + }, nil)}, + }, "subject", "x") + + if got := res.merged["shoegaze"]; got != 0.8 { + t.Errorf("shoegaze = %v, want 0.8 (max across providers)", got) + } + if got := res.merged["noise"]; got != 0.9 { + t.Errorf("noise = %v, want 0.9", got) + } + if got := res.merged["dream pop"]; got != 0.3 { + t.Errorf("dream pop = %v, want 0.3", got) + } + if len(res.merged) != 3 { + t.Errorf("merged has %d tags, want 3: %v", len(res.merged), res.merged) + } + if res.anyTransient { + t.Error("anyTransient set with no failures") + } + if len(res.contributors) != 2 { + t.Errorf("contributors = %v, want both providers", res.contributors) + } +} + +// A lower weight arriving second must not overwrite a higher one — the +// ordering of the chain must not change the result. +func TestRunChain_LowerWeightSecondDoesNotClobber(t *testing.T) { + res := chainEnricher().runChain(context.Background(), []tagFetch{ + {providerID: "a", fetch: fixedFetch([]Tag{{Name: "rock", Weight: 0.9}}, nil)}, + {providerID: "b", fetch: fixedFetch([]Tag{{Name: "rock", Weight: 0.2}}, nil)}, + }, "subject", "x") + if got := res.merged["rock"]; got != 0.9 { + t.Errorf("rock = %v, want 0.9", got) + } +} + +func TestRunChain_NotFoundIsSkippedNotTransient(t *testing.T) { + res := chainEnricher().runChain(context.Background(), []tagFetch{ + {providerID: "a", fetch: fixedFetch(nil, ErrNotFound)}, + {providerID: "b", fetch: fixedFetch([]Tag{{Name: "folk", Weight: 1}}, nil)}, + }, "subject", "x") + + if res.anyTransient { + t.Error("ErrNotFound must not be treated as transient — it would keep a settled subject eligible forever") + } + if len(res.contributors) != 1 || res.contributors[0] != "b" { + t.Errorf("contributors = %v, want [b] only (a returned nothing)", res.contributors) + } +} + +func TestRunChain_TransientIsRecordedEvenWhenAnotherProviderSucceeds(t *testing.T) { + res := chainEnricher().runChain(context.Background(), []tagFetch{ + {providerID: "a", fetch: fixedFetch(nil, ErrTransient)}, + {providerID: "b", fetch: fixedFetch([]Tag{{Name: "folk", Weight: 1}}, nil)}, + }, "subject", "x") + + if !res.anyTransient { + t.Error("anyTransient should be set — 'a' may have had tags we never saw") + } + // Tags DID land, so the caller writes them; anyTransient only decides the + // no-tags case. Asserting both here pins that they're independent. + if len(res.merged) != 1 { + t.Errorf("merged = %v, want folk", res.merged) + } +} + +// An unexpected error type is transient, not terminal. Defaulting the other way +// would settle a subject on any bug in a provider. +func TestRunChain_UnknownErrorIsTransient(t *testing.T) { + res := chainEnricher().runChain(context.Background(), []tagFetch{ + {providerID: "a", fetch: fixedFetch(nil, errors.New("boom"))}, + }, "subject", "x") + if !res.anyTransient { + t.Error("unknown error should count as transient") + } + if len(res.merged) != 0 { + t.Errorf("merged = %v, want empty", res.merged) + } +} + +// A provider returning (empty, nil) is not a contributor: sourceLabel would +// otherwise stamp its ID onto a subject it gave nothing to. +func TestRunChain_EmptySuccessIsNotAContributor(t *testing.T) { + res := chainEnricher().runChain(context.Background(), []tagFetch{ + {providerID: "a", fetch: fixedFetch(nil, nil)}, + {providerID: "b", fetch: fixedFetch([]Tag{{Name: "folk", Weight: 1}}, nil)}, + }, "subject", "x") + if len(res.contributors) != 1 || res.contributors[0] != "b" { + t.Errorf("contributors = %v, want [b]", res.contributors) + } +} + +// No enabled providers is a clean "nothing found", NOT a failure — the caller +// settles the subject rather than retrying an empty chain on every tick. +func TestRunChain_EmptyChainSettlesRatherThanRetries(t *testing.T) { + res := chainEnricher().runChain(context.Background(), nil, "subject", "x") + if res.anyTransient { + t.Error("an empty chain must not look transient") + } + if len(res.merged) != 0 || len(res.contributors) != 0 { + t.Errorf("empty chain produced %v / %v", res.merged, res.contributors) + } + if sourceLabel(res.contributors) != sourceNone { + t.Errorf("sourceLabel = %q, want %q", sourceLabel(res.contributors), sourceNone) + } +} diff --git a/internal/tags/provider.go b/internal/tags/provider.go index d1d8c8ce..900825f3 100644 --- a/internal/tags/provider.go +++ b/internal/tags/provider.go @@ -87,6 +87,32 @@ type TrackTagProvider interface { FetchTrackTags(ctx context.Context, ref TrackRef) ([]Tag, error) } +// ArtistRef is the lookup key for artist-level tags. MBID is the artist's +// MusicBrainz ID (required by MBID-keyed providers); Name is the fallback for +// name-based providers (Last.fm). At least one must be set or every provider +// returns ErrNotFound. +type ArtistRef struct { + MBID string + Name string +} + +// ArtistTagProvider is the artist-level tag capability, added for tag-space +// Discover (#2376). Separate from TrackTagProvider — exactly the split that +// interface's doc comment anticipated — so a source can implement either +// without the other, and so the enricher can ask for the capability it needs +// rather than checking at the call site. +// +// The subject here is an artist Minstrel does NOT have locally, so there is no +// track to fall back to and no recording-level tag to prefer. +type ArtistTagProvider interface { + Provider + // FetchArtistTags returns the artist's folksonomy tags, or ErrNotFound + // (terminal — nothing upstream) / ErrTransient (retry). A disabled + // provider, or one missing a required key, returns ErrNotFound so the + // chain simply skips it. + FetchArtistTags(ctx context.Context, ref ArtistRef) ([]Tag, error) +} + // TestableProvider is an opt-in capability for the admin Test-Connection // button: answer "is my config working?" without a full enrichment cycle. type TestableProvider interface { diff --git a/internal/tags/provider_artist_tags_test.go b/internal/tags/provider_artist_tags_test.go new file mode 100644 index 00000000..489be798 --- /dev/null +++ b/internal/tags/provider_artist_tags_test.go @@ -0,0 +1,173 @@ +package tags + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "testing" +) + +// Artist-level tag fetching for out-of-library Discover candidates (#2376). +// Reuses the mbEntityServer / newMBProvider / newLastfmProvider helpers from +// the per-provider test files. + +func TestMusicBrainzFetchArtistTags_UsesFullWeightNotTheFallbackDiscount(t *testing.T) { + // THE distinction worth a test. artistTagWeightFactor (0.6) exists because + // FetchTrackTags uses artist tags as a *proxy* for a track's tags. Here the + // artist IS the subject, so weights must land unscaled — otherwise these + // are not comparable with track_tags, which is the exact comparison slice 6 + // is built on. + srv := mbEntityServer(``, `{"tags":[{"count":4,"name":"shoegaze"},{"count":2,"name":"dream pop"}]}`) + defer srv.Close() + old := mbBaseURL + mbBaseURL = srv.URL + defer func() { mbBaseURL = old }() + + tags, err := newMBProvider(true).FetchArtistTags(context.Background(), + ArtistRef{MBID: "art-1", Name: "Some Band"}) + if err != nil { + t.Fatalf("fetch: %v", err) + } + m := tagsByName(tags) + if got := m["shoegaze"]; got != 1.0 { + t.Errorf("shoegaze = %v, want 1.0 — not discounted by artistTagWeightFactor", got) + } + if got := m["dream pop"]; got != 0.5 { + t.Errorf("dream pop = %v, want 0.5", got) + } +} + +func TestMusicBrainzFetchArtistTags_GatedOff(t *testing.T) { + if _, err := newMBProvider(false).FetchArtistTags(context.Background(), + ArtistRef{MBID: "art-1"}); !errors.Is(err, ErrNotFound) { + t.Errorf("disabled: err = %v, want ErrNotFound", err) + } + // No MBID → nothing MusicBrainz can look up. A name-based guess could + // silently attach the wrong artist's tags, so it deliberately doesn't try. + if _, err := newMBProvider(true).FetchArtistTags(context.Background(), + ArtistRef{Name: "Some Band"}); !errors.Is(err, ErrNotFound) { + t.Errorf("no MBID: err = %v, want ErrNotFound", err) + } +} + +// fetchEntityTags reports an existing-but-untagged entity as (empty, nil) so +// FetchTrackTags can fall through to the artist level. FetchArtistTags has no +// next level, so it must convert that to the terminal ErrNotFound — otherwise +// the enricher would read "no error" as success and settle the candidate as +// enriched with zero tags. +func TestMusicBrainzFetchArtistTags_UntaggedArtistIsNotFound(t *testing.T) { + srv := mbEntityServer(``, `{"tags":[]}`) + defer srv.Close() + old := mbBaseURL + mbBaseURL = srv.URL + defer func() { mbBaseURL = old }() + + _, err := newMBProvider(true).FetchArtistTags(context.Background(), ArtistRef{MBID: "art-1"}) + if !errors.Is(err, ErrNotFound) { + t.Errorf("err = %v, want ErrNotFound", err) + } +} + +func TestLastfmFetchArtistTags_CallsArtistGetTopTags(t *testing.T) { + var gotMethod, gotArtist, gotMBID string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotMethod = r.URL.Query().Get("method") + gotArtist = r.URL.Query().Get("artist") + gotMBID = r.URL.Query().Get("mbid") + _, _ = w.Write([]byte(`{"toptags":{"tag":[{"name":"post-punk","count":100},{"name":"moody","count":40}]}}`)) + })) + defer srv.Close() + old := lastfmBaseURL + lastfmBaseURL = srv.URL + "/" + defer func() { lastfmBaseURL = old }() + + p := &lastfmProvider{client: newHTTPClient(httpClientOptions{Name: "lastfm", MaxRetries: 1})} + _ = p.Configure(ProviderSettings{Enabled: true, APIKey: "k"}) + + tags, err := p.FetchArtistTags(context.Background(), + ArtistRef{MBID: "art-1", Name: "Some Band"}) + if err != nil { + t.Fatalf("fetch: %v", err) + } + if gotMethod != "artist.gettoptags" { + t.Errorf("method = %q, want artist.gettoptags", gotMethod) + } + if gotArtist != "Some Band" { + t.Errorf("artist = %q, want Some Band", gotArtist) + } + // MBID is sent as a disambiguating hint when we have one. + if gotMBID != "art-1" { + t.Errorf("mbid = %q, want art-1", gotMBID) + } + m := tagsByName(tags) + if m["post-punk"] != 1.0 { + t.Errorf("post-punk = %v, want 1.0", m["post-punk"]) + } + if m["moody"] != 0.4 { + t.Errorf("moody = %v, want 0.4", m["moody"]) + } +} + +func TestLastfmFetchArtistTags_GatedOff(t *testing.T) { + unkeyed := &lastfmProvider{client: newHTTPClient(httpClientOptions{Name: "lastfm", MaxRetries: 1})} + _ = unkeyed.Configure(ProviderSettings{Enabled: true}) + if _, err := unkeyed.FetchArtistTags(context.Background(), + ArtistRef{Name: "A"}); !errors.Is(err, ErrNotFound) { + t.Errorf("unkeyed: err = %v, want ErrNotFound", err) + } + + keyed := &lastfmProvider{client: newHTTPClient(httpClientOptions{Name: "lastfm", MaxRetries: 1})} + _ = keyed.Configure(ProviderSettings{Enabled: false, APIKey: "k"}) + if _, err := keyed.FetchArtistTags(context.Background(), + ArtistRef{Name: "A"}); !errors.Is(err, ErrNotFound) { + t.Errorf("disabled: err = %v, want ErrNotFound", err) + } + + // Name-based provider with no name → nothing to query. This is the case + // that matters in practice: a candidate whose name coalesced to '' still + // has an MBID, so MusicBrainz can serve it while Last.fm cannot. + enabled := &lastfmProvider{client: newHTTPClient(httpClientOptions{Name: "lastfm", MaxRetries: 1})} + _ = enabled.Configure(ProviderSettings{Enabled: true, APIKey: "k"}) + if _, err := enabled.FetchArtistTags(context.Background(), + ArtistRef{MBID: "art-1"}); !errors.Is(err, ErrNotFound) { + t.Errorf("no name: err = %v, want ErrNotFound", err) + } +} + +func TestLastfmFetchArtistTags_TransientErrorCodeRetries(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"error":29}`)) // rate limited + })) + defer srv.Close() + old := lastfmBaseURL + lastfmBaseURL = srv.URL + "/" + defer func() { lastfmBaseURL = old }() + + p := &lastfmProvider{client: newHTTPClient(httpClientOptions{Name: "lastfm", MaxRetries: 1})} + _ = p.Configure(ProviderSettings{Enabled: true, APIKey: "k"}) + _, err := p.FetchArtistTags(context.Background(), ArtistRef{Name: "A"}) + // Must be ErrTransient, not ErrNotFound: the enricher settles a candidate + // to 'none' on ErrNotFound, which would permanently write off an artist we + // were merely throttled on. + if !errors.Is(err, ErrTransient) { + t.Errorf("err = %v, want ErrTransient", err) + } +} + +func TestLastfmFetchArtistTags_UnknownArtistIsNotFound(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"error":6}`)) // "not found" — terminal + })) + defer srv.Close() + old := lastfmBaseURL + lastfmBaseURL = srv.URL + "/" + defer func() { lastfmBaseURL = old }() + + p := &lastfmProvider{client: newHTTPClient(httpClientOptions{Name: "lastfm", MaxRetries: 1})} + _ = p.Configure(ProviderSettings{Enabled: true, APIKey: "k"}) + if _, err := p.FetchArtistTags(context.Background(), + ArtistRef{Name: "Nobody"}); !errors.Is(err, ErrNotFound) { + t.Errorf("err = %v, want ErrNotFound", err) + } +} diff --git a/internal/tags/provider_lastfm.go b/internal/tags/provider_lastfm.go index e37a3eb8..12917333 100644 --- a/internal/tags/provider_lastfm.go +++ b/internal/tags/provider_lastfm.go @@ -121,6 +121,50 @@ func (p *lastfmProvider) FetchTrackTags(ctx context.Context, ref TrackRef) ([]Ta return out, nil } +// FetchArtistTags looks up an artist's top tags by name, with the MBID as an +// extra hint when present (#2376). Name-first is deliberate and the opposite +// emphasis from MusicBrainz: Last.fm's tag data is keyed on its own artist +// pages, and autocorrect resolves most spelling drift from the similarity feed. +// +// `artist.getTopTags` returns the same `toptags` envelope as +// `track.getTopTags`, so the response type and normalizer are reused as-is — +// the 0-100 popularity scale is identical. +func (p *lastfmProvider) FetchArtistTags(ctx context.Context, ref ArtistRef) ([]Tag, error) { + if !p.enabled.Load() || p.currentKey() == "" { + return nil, ErrNotFound + } + if ref.Name == "" { + return nil, ErrNotFound + } + + q := url.Values{ + "method": {"artist.gettoptags"}, + "api_key": {p.currentKey()}, + "format": {"json"}, + "artist": {ref.Name}, + "autocorrect": {"1"}, + } + if ref.MBID != "" { + q.Set("mbid", ref.MBID) + } + + var resp lastfmTopTags + if err := p.client.getJSON(ctx, lastfmBaseURL+"?"+q.Encode(), &resp); err != nil { + return nil, err + } + if resp.Error != 0 { + if lastfmTransientErrors[resp.Error] { + return nil, ErrTransient + } + return nil, ErrNotFound + } + out := normalizeLastfmTags(resp.TopTags.Tag) + if len(out) == 0 { + return nil, ErrNotFound + } + return out, nil +} + // TestConnection verifies the key against a well-known track. func (p *lastfmProvider) TestConnection(ctx context.Context) error { if p.currentKey() == "" { @@ -163,6 +207,7 @@ func normalizeLastfmTags(raw []lastfmTag) []Tag { // Compile-time capability checks. var ( - _ TrackTagProvider = (*lastfmProvider)(nil) - _ TestableProvider = (*lastfmProvider)(nil) + _ TrackTagProvider = (*lastfmProvider)(nil) + _ ArtistTagProvider = (*lastfmProvider)(nil) + _ TestableProvider = (*lastfmProvider)(nil) ) diff --git a/internal/tags/provider_musicbrainz.go b/internal/tags/provider_musicbrainz.go index 5c89e7d9..5a0362c5 100644 --- a/internal/tags/provider_musicbrainz.go +++ b/internal/tags/provider_musicbrainz.go @@ -96,6 +96,33 @@ func (p *musicbrainzProvider) FetchTrackTags(ctx context.Context, ref TrackRef) return nil, ErrNotFound } +// FetchArtistTags looks up an artist's own tags by MBID (#2376). MBID-only: +// MusicBrainz has no name-based tag lookup worth trusting for this, and a +// wrong-artist match would poison the tag cache silently. +// +// Note the scale is 1.0, NOT artistTagWeightFactor. That discount exists +// because FetchTrackTags uses artist tags as a *proxy* for a track's tags, and +// the artist's overall character is the coarser signal of the two. Here the +// artist IS the subject, so there is nothing to discount relative to — and +// applying it would make these weights incomparable with track_tags, which is +// exactly the comparison slice 6 depends on. +func (p *musicbrainzProvider) FetchArtistTags(ctx context.Context, ref ArtistRef) ([]Tag, error) { + if !p.enabled.Load() || ref.MBID == "" { + return nil, ErrNotFound + } + tags, err := p.fetchEntityTags(ctx, "artist", ref.MBID, 1.0) + if err != nil { + return nil, err + } + // fetchEntityTags reports an existing-but-untagged entity as (empty, nil) + // so FetchTrackTags can fall through to the next level. There is no next + // level here, so empty is the terminal "nothing upstream". + if len(tags) == 0 { + return nil, ErrNotFound + } + return tags, nil +} + // fetchEntityTags loads folksonomy tags for a MusicBrainz entity ("recording" // or "artist") by MBID and scales the normalized weights by `scale`. Returns // an empty slice (not ErrNotFound) when the entity exists but is untagged, so @@ -153,6 +180,7 @@ func normalizeMBTags(raw []mbTag) []Tag { // Compile-time capability checks. var ( - _ TrackTagProvider = (*musicbrainzProvider)(nil) - _ TestableProvider = (*musicbrainzProvider)(nil) + _ TrackTagProvider = (*musicbrainzProvider)(nil) + _ ArtistTagProvider = (*musicbrainzProvider)(nil) + _ TestableProvider = (*musicbrainzProvider)(nil) ) diff --git a/internal/tags/settings.go b/internal/tags/settings.go index b16a3117..3914bb3d 100644 --- a/internal/tags/settings.go +++ b/internal/tags/settings.go @@ -135,6 +135,27 @@ func (s *SettingsService) EnabledTrackTagProviders() []TrackTagProvider { return out } +// EnabledArtistTagProviders returns the enabled providers implementing +// ArtistTagProvider, in registration order. Snapshot — do not mutate. +// +// Separate from EnabledTrackTagProviders rather than one call with a capability +// argument: the two chains are consumed by different drains, and a provider may +// implement one capability without the other. +func (s *SettingsService) EnabledArtistTagProviders() []ArtistTagProvider { + s.mu.RLock() + defer s.mu.RUnlock() + var out []ArtistTagProvider + for _, p := range AllProviders() { + if !s.enabledIDs[p.ID()] { + continue + } + if ap, ok := p.(ArtistTagProvider); ok { + out = append(out, ap) + } + } + return out +} + // CurrentVersion returns the version the enricher stamps onto rows. func (s *SettingsService) CurrentVersion() int32 { s.mu.RLock() diff --git a/internal/tags/worker.go b/internal/tags/worker.go index 701a08f3..35ab883c 100644 --- a/internal/tags/worker.go +++ b/internal/tags/worker.go @@ -14,22 +14,32 @@ import ( // providers' httpClients, so a tick just drains a bounded batch and the // external APIs pace themselves. type Worker struct { - enricher *Enricher - logger *slog.Logger - tick time.Duration - batch int + enricher *Enricher + logger *slog.Logger + tick time.Duration + batch int + candidateBatch int } // NewWorker constructs a worker with production defaults: an initial drain // shortly after boot, then every 30 minutes, up to 200 tracks per tick. // MusicBrainz's 1 req/s ceiling is the real throttle, so the batch size // mainly bounds how long one tick runs, not the request rate. +// +// candidateBatch is smaller than the track batch on purpose. Library tracks are +// a finite set that drains to completion and then costs nothing; out-of-library +// candidates (#2376) are effectively unbounded — every library artist's +// neighbours — so this arm would otherwise monopolise every tick forever and +// starve the track arm. 50/tick at ~1 req/s is roughly a minute of work, and +// the query hands back the highest-similarity candidates first so the ones that +// can actually reach a user's deck are enriched first. func NewWorker(enricher *Enricher, logger *slog.Logger) *Worker { return &Worker{ - enricher: enricher, - logger: logger, - tick: 30 * time.Minute, - batch: 200, + enricher: enricher, + logger: logger, + tick: 30 * time.Minute, + batch: 200, + candidateBatch: 50, } } @@ -48,12 +58,27 @@ func (w *Worker) Run(ctx context.Context) { } } -// tickOnce drains one bounded batch. EnrichTrackBatch already logs a -// category breakdown, so this only surfaces a fatal batch error. +// tickOnce drains one bounded batch of each kind. Both Enrich*Batch methods +// already log a category breakdown, so this only surfaces a fatal batch error. +// +// Tracks first: they back the taste profile the whole app reads from, whereas +// candidate tags only affect the Discover request surface. On a fresh install +// both are cold, and getting the profile warm matters more. func (w *Worker) tickOnce(ctx context.Context) { if _, _, _, err := w.enricher.EnrichTrackBatch(ctx, w.batch, nil); err != nil { if ctx.Err() == nil { w.logger.Error("tags: enrichment tick failed", "err", err) } } + // Not gated on the track arm's success: the two drains share nothing but a + // provider chain, and a track-side failure says nothing about whether + // candidate lookups will work. + if ctx.Err() != nil { + return + } + if _, _, _, err := w.enricher.EnrichCandidateArtistBatch(ctx, w.candidateBatch); err != nil { + if ctx.Err() == nil { + w.logger.Error("tags: candidate-artist enrichment tick failed", "err", err) + } + } } From 7315e37c15955992343201c5e39d9bc87a52d656 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 2 Aug 2026 20:11:03 -0400 Subject: [PATCH 10/14] =?UTF-8?q?fix(db):=20apply=20sqlc's=20actual=20outp?= =?UTF-8?q?ut=20for=20candidate=5Fartist=5Ftags=20=E2=80=94=20#2376?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three divergences in the hand-written generated file, all caught by verify-generate on the first run. Two are sqlc rules I had wrong: 1. When a query's SELECT list exactly matches a table's columns in order, sqlc REUSES the model struct rather than emitting a bespoke Row type. So ListCandidateArtistTagsForMbids returns []CandidateArtistTag, and ListCandidateArtistTagsForMbidsRow should never have existed. 2. models.go is ordered by GO STRUCT NAME, not table name. Table order would put candidate_artist_tag_state before candidate_artist_tags; sqlc emits CandidateArtistTag before CandidateArtistTagState. The earlier slice-3 observation ("ordered by table name") was consistent with both orderings and so never discriminated — this case does. 3. sqlc smart-quotes a doubled '' inside a promoted comment into a typographic ”. Reworded the prose to say "the empty string" instead of encoding a mangling into the source. Note the integration lane PASSED on the broken push while this failed. That is #2380's lesson landing again, and the reason the check exists: valid SQL executing against real Postgres proves nothing about whether the committed Go matches its source. Co-Authored-By: Claude Opus 5 (1M context) --- internal/db/dbq/candidate_artist_tags.sql.go | 21 +++++++------------ internal/db/dbq/models.go | 12 +++++------ internal/db/queries/candidate_artist_tags.sql | 9 ++++---- 3 files changed, 19 insertions(+), 23 deletions(-) diff --git a/internal/db/dbq/candidate_artist_tags.sql.go b/internal/db/dbq/candidate_artist_tags.sql.go index 29ec71a9..a733e4a2 100644 --- a/internal/db/dbq/candidate_artist_tags.sql.go +++ b/internal/db/dbq/candidate_artist_tags.sql.go @@ -110,23 +110,17 @@ SELECT candidate_mbid, tag, weight WHERE candidate_mbid = ANY($1::text[]) ` -type ListCandidateArtistTagsForMbidsRow struct { - CandidateMbid string - Tag string - Weight float64 -} - // Cached tags for a set of candidates, for slice 6's taste-overlap ranking. // One row per (candidate, tag). -func (q *Queries) ListCandidateArtistTagsForMbids(ctx context.Context, dollar_1 []string) ([]ListCandidateArtistTagsForMbidsRow, error) { +func (q *Queries) ListCandidateArtistTagsForMbids(ctx context.Context, dollar_1 []string) ([]CandidateArtistTag, error) { rows, err := q.db.Query(ctx, listCandidateArtistTagsForMbids, dollar_1) if err != nil { return nil, err } defer rows.Close() - var items []ListCandidateArtistTagsForMbidsRow + var items []CandidateArtistTag for rows.Next() { - var i ListCandidateArtistTagsForMbidsRow + var i CandidateArtistTag if err := rows.Scan(&i.CandidateMbid, &i.Tag, &i.Weight); err != nil { return nil, err } @@ -186,10 +180,11 @@ type ListCandidateArtistsMissingTagsRow struct { // Already-in-library candidates are skipped: they have an artists row, so // their tags belong in track_tags, and the suggestion query filters them out // anyway. $1 = current tag_sources_version, $2 = limit. -// candidate_name is coalesced to '' so it lands non-nullable in Go: the name is -// only a Last.fm lookup key, and empty simply means "MBID-keyed providers only", -// which the provider chain already handles. max() is an arbitrary-but- -// deterministic pick when several seeds spell the same MBID differently. +// candidate_name is coalesced to the empty string so it lands non-nullable in +// Go: the name is only a Last.fm lookup key, and empty simply means "MBID-keyed +// providers only", which the provider chain already handles. max() is an +// arbitrary-but-deterministic pick when several seeds spell one MBID +// differently. func (q *Queries) ListCandidateArtistsMissingTags(ctx context.Context, arg ListCandidateArtistsMissingTagsParams) ([]ListCandidateArtistsMissingTagsRow, error) { rows, err := q.db.Query(ctx, listCandidateArtistsMissingTags, arg.TagSourcesVersion, arg.Limit) if err != nil { diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index d4ec39f7..524bb6a9 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -240,6 +240,12 @@ type AuditLog struct { CreatedAt pgtype.Timestamptz } +type CandidateArtistTag struct { + CandidateMbid string + Tag string + Weight float64 +} + type CandidateArtistTagState struct { CandidateMbid string TagSource string @@ -247,12 +253,6 @@ type CandidateArtistTagState struct { UpdatedAt pgtype.Timestamptz } -type CandidateArtistTag struct { - CandidateMbid string - Tag string - Weight float64 -} - type ContextualLike struct { ID pgtype.UUID UserID pgtype.UUID diff --git a/internal/db/queries/candidate_artist_tags.sql b/internal/db/queries/candidate_artist_tags.sql index 150b0637..4403689f 100644 --- a/internal/db/queries/candidate_artist_tags.sql +++ b/internal/db/queries/candidate_artist_tags.sql @@ -20,10 +20,11 @@ -- Already-in-library candidates are skipped: they have an artists row, so -- their tags belong in track_tags, and the suggestion query filters them out -- anyway. $1 = current tag_sources_version, $2 = limit. --- candidate_name is coalesced to '' so it lands non-nullable in Go: the name is --- only a Last.fm lookup key, and empty simply means "MBID-keyed providers only", --- which the provider chain already handles. max() is an arbitrary-but- --- deterministic pick when several seeds spell the same MBID differently. +-- candidate_name is coalesced to the empty string so it lands non-nullable in +-- Go: the name is only a Last.fm lookup key, and empty simply means "MBID-keyed +-- providers only", which the provider chain already handles. max() is an +-- arbitrary-but-deterministic pick when several seeds spell one MBID +-- differently. SELECT u.candidate_mbid, coalesce(max(u.candidate_name), '')::text AS candidate_name, sum(u.score)::float8 AS total_score From 799dab029a0f164c92dd3947384ee570c0efaf95 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 2 Aug 2026 20:31:12 -0400 Subject: [PATCH 11/14] =?UTF-8?q?feat(discover):=20rank=20suggestions=20by?= =?UTF-8?q?=20taste-tag=20overlap=20=E2=80=94=20#2377=20(server)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The payoff slice. Until now a candidate's only claim on a slot was "some artist you play is adjacent to it in a similarity graph" — a fact that says nothing about whether the music sounds like anything you like. Now the candidate's own folksonomy tags (cached by slice 5) are compared against the user's taste-profile tags, so the deck ranks on taste and can say WHY. The blend is MULTIPLICATIVE — score × (1 + weight × overlap) — and that choice carries the whole safety argument: - An untagged candidate has overlap 0, so its score is EXACTLY unchanged. Tag coverage is permanently partial (#2376); it must cost a candidate nothing, not sink it (rule #131). - Nothing can leapfrog on tags alone. An additive term with a large weight would let a near-zero-similarity artist outrank a strong match for sharing one popular tag, which reads as noise. - Weight 0 restores pure similarity order bit-for-bit, so the operator's knob has a real off position. overlap = Σ(shared) candWeight × normalizedTasteWeight ÷ Σ(all) candWeight. Normalizing the taste side by the user's strongest tag makes the score comparable across users (taste weights accumulate with listening, so a heavy listener's raw numbers dwarf a new user's while meaning the same thing). Dividing by the candidate's own mass makes it comparable across candidates, so a densely-tagged artist can't win on tag count alone. Applied to the whole over-fetched pool BEFORE selectSuggestions, so the rotation and diversity rules operate on blended scores — boosting only the twelve already chosen by similarity would leave the re-ranking undone. A query failure is returned, NOT degraded past. Graceful degradation is for expected absence (no taste profile, no cached tags) and both are handled explicitly as empty inputs; swallowing a real error would hide a broken DB behind a subtly worse ranking that nothing reports. Migration 0051 adds a FOURTH tuning scope rather than columns on taste_tuning, because snooze_days lives here too and a snooze must never be read as taste signal (#2374) — filing it under 'taste' would put it one careless join from the leak that design forbids. Expanding recommendation_tuning_audit's CHECK is in the same migration per rule #36, and a test asserts the audit row lands, which is what would catch its absence. snooze_days moves out of a Go constant onto the tuning card (rule #25), closing the deferral from #2374. Tag-overlap tests use deliberately SKEWED fixtures: an evenly-matching pool cannot exercise a re-ranking, since every candidate gets the same multiplier and the order is unchanged whether the blend works or not. Admin UI + client attribution follow in this batch — rule #27. Co-Authored-By: Claude Opus 5 (1M context) --- internal/api/admin_recommendation_tuning.go | 24 +- internal/api/suggestions.go | 26 +- internal/db/dbq/models.go | 7 + internal/db/dbq/recommendation_tuning.sql.go | 60 +++++ .../migrations/0051_discover_tuning.down.sql | 10 + .../db/migrations/0051_discover_tuning.up.sql | 38 +++ internal/db/queries/recommendation_tuning.sql | 18 ++ internal/dbtest/reset.go | 4 + .../candidate_tags_integration_test.go | 139 ++++++++++ internal/recommendation/suggestions.go | 76 +++++- .../suggestions_integration_test.go | 34 +-- internal/recommendation/tagoverlap.go | 156 +++++++++++ internal/recommendation/tagoverlap_test.go | 244 ++++++++++++++++++ internal/recsettings/patch.go | 64 +++++ internal/recsettings/service.go | 99 +++++++ internal/recsettings/service_test.go | 115 +++++++++ 16 files changed, 1085 insertions(+), 29 deletions(-) create mode 100644 internal/db/migrations/0051_discover_tuning.down.sql create mode 100644 internal/db/migrations/0051_discover_tuning.up.sql create mode 100644 internal/recommendation/tagoverlap.go create mode 100644 internal/recommendation/tagoverlap_test.go diff --git a/internal/api/admin_recommendation_tuning.go b/internal/api/admin_recommendation_tuning.go index a97276c5..918a3b55 100644 --- a/internal/api/admin_recommendation_tuning.go +++ b/internal/api/admin_recommendation_tuning.go @@ -67,15 +67,30 @@ func tasteRespFrom(t recsettings.TasteTuning) tasteTuningResp { } } +// discoverTuningResp is the Discover scope on the wire (#2377). +type discoverTuningResp struct { + TagOverlapWeight float64 `json:"tag_overlap_weight"` + SnoozeDays float64 `json:"snooze_days"` +} + +func discoverRespFrom(d recsettings.DiscoverTuning) discoverTuningResp { + return discoverTuningResp{ + TagOverlapWeight: d.TagOverlapWeight, + SnoozeDays: d.SnoozeDays, + } +} + // tuningSnapshot is both the GET response and the post-mutation echo: // current values alongside shipped defaults so the card can mark // which knobs deviate. type tuningSnapshot struct { Profiles map[string]weightsResp `json:"profiles"` Taste tasteTuningResp `json:"taste"` + Discover discoverTuningResp `json:"discover"` Shipped struct { Profiles map[string]weightsResp `json:"profiles"` Taste tasteTuningResp `json:"taste"` + Discover discoverTuningResp `json:"discover"` } `json:"shipped"` } @@ -86,11 +101,13 @@ func (h *handlers) tuningSnapshot() tuningSnapshot { recsettings.ScopeDailyMix: weightsRespFrom(h.recSettings.Weights(recsettings.ScopeDailyMix)), } out.Taste = tasteRespFrom(h.recSettings.Taste()) + out.Discover = discoverRespFrom(h.recSettings.Discover()) out.Shipped.Profiles = map[string]weightsResp{ recsettings.ScopeRadio: weightsRespFrom(recsettings.ShippedRadioWeights()), recsettings.ScopeDailyMix: weightsRespFrom(recsettings.ShippedDailyMixWeights()), } out.Shipped.Taste = tasteRespFrom(recsettings.ShippedTasteTuning()) + out.Shipped.Discover = discoverRespFrom(recsettings.ShippedDiscoverTuning()) return out } @@ -120,9 +137,12 @@ func (h *handlers) handlePatchRecommendationTuning(w http.ResponseWriter, r *htt } var err error - if scope == recsettings.ScopeTaste { + switch scope { + case recsettings.ScopeTaste: err = h.recSettings.UpdateTaste(r.Context(), body.Values) - } else { + case recsettings.ScopeDiscover: + err = h.recSettings.UpdateDiscover(r.Context(), body.Values) + default: err = h.recSettings.UpdateProfile(r.Context(), scope, body.Values) } if err != nil { diff --git a/internal/api/suggestions.go b/internal/api/suggestions.go index da3bd18c..977d571f 100644 --- a/internal/api/suggestions.go +++ b/internal/api/suggestions.go @@ -25,6 +25,12 @@ type suggestionView struct { Name string `json:"name"` Score float64 `json:"score"` Attribution []seedContributionView `json:"attribution"` + // MatchedTags are the candidate's tags that overlap the user's taste + // profile, strongest first (#2377) — the "matches: shoegaze, melancholic" + // line. Omitted when empty, which is common: tag coverage for + // out-of-library artists is permanently partial (#2376), and the card + // falls back to the seed attribution it has always shown. + MatchedTags []string `json:"matched_tags,omitempty"` // ImageURL is resolved on-demand from Lidarr (out-of-library // artists have no local art row). Omitted when Lidarr is disabled // or has no match — the client falls back to a placeholder. Not @@ -71,7 +77,11 @@ func (h *handlers) handleListSuggestions(w http.ResponseWriter, r *http.Request) halfLife = f } - suggestions, err := recommendation.SuggestArtists(r.Context(), h.pool, user.ID, halfLife, limit) + // Read the tuned weight per request so an admin change takes effect on the + // next refresh, no restart (rule #25). + tagWeight := h.recSettings.Discover().TagOverlapWeight + suggestions, err := recommendation.SuggestArtists( + r.Context(), h.pool, user.ID, halfLife, limit, tagWeight) if err != nil { h.logger.Error("api: list suggestions", "err", err) writeErr(w, apierror.InternalMsg("failed to load suggestions", err)) @@ -92,19 +102,17 @@ func (h *handlers) handleListSuggestions(w http.ResponseWriter, r *http.Request) } out = append(out, suggestionView{ MBID: s.MBID, Name: s.Name, Score: s.Score, Attribution: attr, + MatchedTags: s.MatchedTags, }) } h.resolveSuggestionArt(r.Context(), out) writeJSON(w, http.StatusOK, out) } -// Snooze duration bounds. 90 days is long enough that a parked suggestion -// stops feeling like it's nagging, short enough that a taste shift brings it -// back on its own — the whole point of a snooze over a dismissal (#2374). -const ( - defaultSnoozeDays = 90.0 - maxSnoozeDays = 365.0 -) +// maxSnoozeDays caps a client-supplied duration. The DEFAULT is not here: it's +// a DB-backed knob on the admin tuning card (rule #25), read per request via +// recSettings.Discover().SnoozeDays. See #2377. +const maxSnoozeDays = 365.0 // snoozeRequest is the POST body. Both fields are optional in the JSON sense // (an absent body snoozes for the default), but Name is required in practice: @@ -158,7 +166,7 @@ func (h *handlers) handleSnoozeSuggestion(w http.ResponseWriter, r *http.Request } days := body.Days if days <= 0 { - days = defaultSnoozeDays + days = h.recSettings.Discover().SnoozeDays } if days > maxSnoozeDays { // Clamp rather than reject: a client asking for longer than we allow diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index 524bb6a9..bfbb4ef8 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -290,6 +290,13 @@ type DiagnosticEvent struct { ReceivedAt pgtype.Timestamptz } +type DiscoverTuning struct { + Singleton bool + TagOverlapWeight float64 + SnoozeDays float64 + UpdatedAt pgtype.Timestamptz +} + type GeneralLike struct { UserID pgtype.UUID TrackID pgtype.UUID diff --git a/internal/db/dbq/recommendation_tuning.sql.go b/internal/db/dbq/recommendation_tuning.sql.go index f90aaa97..251eeed5 100644 --- a/internal/db/dbq/recommendation_tuning.sql.go +++ b/internal/db/dbq/recommendation_tuning.sql.go @@ -9,6 +9,22 @@ import ( "context" ) +const getDiscoverTuning = `-- name: GetDiscoverTuning :one +SELECT singleton, tag_overlap_weight, snooze_days, updated_at FROM discover_tuning WHERE singleton = true +` + +func (q *Queries) GetDiscoverTuning(ctx context.Context) (DiscoverTuning, error) { + row := q.db.QueryRow(ctx, getDiscoverTuning) + var i DiscoverTuning + err := row.Scan( + &i.Singleton, + &i.TagOverlapWeight, + &i.SnoozeDays, + &i.UpdatedAt, + ) + return i, err +} + const getTasteTuning = `-- name: GetTasteTuning :one SELECT singleton, half_life_days, engagement_hard_skip, engagement_neutral, engagement_full, updated_at, enriched_tag_scale, era_scale, mood_scale FROM taste_tuning WHERE singleton = true ` @@ -118,6 +134,32 @@ func (q *Queries) ListWeightProfiles(ctx context.Context) ([]RecommendationWeigh return items, nil } +const updateDiscoverTuning = `-- name: UpdateDiscoverTuning :one +UPDATE discover_tuning + SET tag_overlap_weight = $1, + snooze_days = $2, + updated_at = now() + WHERE singleton = true +RETURNING singleton, tag_overlap_weight, snooze_days, updated_at +` + +type UpdateDiscoverTuningParams struct { + TagOverlapWeight float64 + SnoozeDays float64 +} + +func (q *Queries) UpdateDiscoverTuning(ctx context.Context, arg UpdateDiscoverTuningParams) (DiscoverTuning, error) { + row := q.db.QueryRow(ctx, updateDiscoverTuning, arg.TagOverlapWeight, arg.SnoozeDays) + var i DiscoverTuning + err := row.Scan( + &i.Singleton, + &i.TagOverlapWeight, + &i.SnoozeDays, + &i.UpdatedAt, + ) + return i, err +} + const updateTasteTuning = `-- name: UpdateTasteTuning :one UPDATE taste_tuning SET half_life_days = $1, @@ -226,6 +268,24 @@ func (q *Queries) UpdateWeightProfile(ctx context.Context, arg UpdateWeightProfi return i, err } +const upsertDiscoverTuningDefaults = `-- name: UpsertDiscoverTuningDefaults :exec +INSERT INTO discover_tuning (singleton, tag_overlap_weight, snooze_days) +VALUES (true, $1, $2) +ON CONFLICT (singleton) DO NOTHING +` + +type UpsertDiscoverTuningDefaultsParams struct { + TagOverlapWeight float64 + SnoozeDays float64 +} + +// Boot reconcile for the Discover scope (#2377). Never overwrites +// operator-tuned values, same contract as the other two. +func (q *Queries) UpsertDiscoverTuningDefaults(ctx context.Context, arg UpsertDiscoverTuningDefaultsParams) error { + _, err := q.db.Exec(ctx, upsertDiscoverTuningDefaults, arg.TagOverlapWeight, arg.SnoozeDays) + return err +} + const upsertTasteTuningDefaults = `-- name: UpsertTasteTuningDefaults :exec INSERT INTO taste_tuning ( singleton, half_life_days, engagement_hard_skip, diff --git a/internal/db/migrations/0051_discover_tuning.down.sql b/internal/db/migrations/0051_discover_tuning.down.sql new file mode 100644 index 00000000..1da70de4 --- /dev/null +++ b/internal/db/migrations/0051_discover_tuning.down.sql @@ -0,0 +1,10 @@ +-- Drop any audit rows under the scope the constraint is about to forbid, +-- otherwise re-adding the narrower CHECK fails against existing data. +DELETE FROM recommendation_tuning_audit WHERE scope = 'discover'; +ALTER TABLE recommendation_tuning_audit + DROP CONSTRAINT recommendation_tuning_audit_scope_check; +ALTER TABLE recommendation_tuning_audit + ADD CONSTRAINT recommendation_tuning_audit_scope_check + CHECK (scope IN ('radio', 'daily_mix', 'taste')); + +DROP TABLE IF EXISTS discover_tuning; diff --git a/internal/db/migrations/0051_discover_tuning.up.sql b/internal/db/migrations/0051_discover_tuning.up.sql new file mode 100644 index 00000000..54bc2be0 --- /dev/null +++ b/internal/db/migrations/0051_discover_tuning.up.sql @@ -0,0 +1,38 @@ +-- 0051_discover_tuning.up.sql — tunable knobs for the Discover request +-- surface (#2377, milestone #268 slice 6). +-- +-- A FOURTH tuning scope alongside radio / daily_mix / taste. Its own scope +-- rather than extra columns on taste_tuning, for a reason that matters: +-- snooze_days lives here, and a snooze must never be read as taste signal +-- (#2374). Filing it under 'taste' would put it one careless join away from +-- exactly the leak that design forbids. +-- +-- Per rule #25 these are DB-backed and editable in the admin UI with no +-- restart — the shipped values below are defaults, not settings. +CREATE TABLE discover_tuning ( + singleton boolean PRIMARY KEY DEFAULT true + CONSTRAINT discover_tuning_singleton_check CHECK (singleton), + -- How strongly taste-tag overlap boosts a candidate's similarity score. + -- The blend is MULTIPLICATIVE: score * (1 + w * overlap), overlap in + -- [0,1]. So 0 disables the feature outright and leaves pure similarity + -- ranking, 1.0 lets a perfectly-matching candidate double its score, and + -- a candidate with no cached tags is unchanged rather than penalised + -- (rule #131 — tag coverage is permanently partial, see #2376). + tag_overlap_weight double precision NOT NULL, + -- Default snooze duration in days. Was a Go constant in + -- internal/api/suggestions.go; moved here per rule #25. + snooze_days double precision NOT NULL, + updated_at timestamptz NOT NULL DEFAULT now() +); + +INSERT INTO discover_tuning (singleton, tag_overlap_weight, snooze_days) +VALUES (true, 1.0, 90); + +-- Rule #36: a new value for a CHECK-gated column needs the constraint +-- rewritten in the SAME change, or the first audit row written under the new +-- scope fails at runtime rather than at migrate time. +ALTER TABLE recommendation_tuning_audit + DROP CONSTRAINT recommendation_tuning_audit_scope_check; +ALTER TABLE recommendation_tuning_audit + ADD CONSTRAINT recommendation_tuning_audit_scope_check + CHECK (scope IN ('radio', 'daily_mix', 'taste', 'discover')); diff --git a/internal/db/queries/recommendation_tuning.sql b/internal/db/queries/recommendation_tuning.sql index b017e542..92ae3bce 100644 --- a/internal/db/queries/recommendation_tuning.sql +++ b/internal/db/queries/recommendation_tuning.sql @@ -53,6 +53,24 @@ UPDATE taste_tuning WHERE singleton = true RETURNING *; +-- name: UpsertDiscoverTuningDefaults :exec +-- Boot reconcile for the Discover scope (#2377). Never overwrites +-- operator-tuned values, same contract as the other two. +INSERT INTO discover_tuning (singleton, tag_overlap_weight, snooze_days) +VALUES (true, $1, $2) +ON CONFLICT (singleton) DO NOTHING; + +-- name: GetDiscoverTuning :one +SELECT * FROM discover_tuning WHERE singleton = true; + +-- name: UpdateDiscoverTuning :one +UPDATE discover_tuning + SET tag_overlap_weight = $1, + snooze_days = $2, + updated_at = now() + WHERE singleton = true +RETURNING *; + -- name: InsertTuningAudit :exec -- changes is a jsonb array of {field, old, new} objects. INSERT INTO recommendation_tuning_audit (scope, action, changes) diff --git a/internal/dbtest/reset.go b/internal/dbtest/reset.go index b39669cc..c71b9aae 100644 --- a/internal/dbtest/reset.go +++ b/internal/dbtest/reset.go @@ -82,6 +82,10 @@ var dataTables = []string{ // (#1250), so truncating gives each test pristine tuning values. "recommendation_weight_profiles", "taste_tuning", + // #2377. Same reasoning as taste_tuning above: recsettings.New re-seeds + // shipped defaults on every construction, so truncating gives each test + // pristine Discover knobs rather than whatever a previous test tuned. + "discover_tuning", "recommendation_tuning_audit", "tracks", "albums", diff --git a/internal/recommendation/candidate_tags_integration_test.go b/internal/recommendation/candidate_tags_integration_test.go index eda08a71..373b4134 100644 --- a/internal/recommendation/candidate_tags_integration_test.go +++ b/internal/recommendation/candidate_tags_integration_test.go @@ -4,6 +4,7 @@ import ( "context" "testing" + "github.com/jackc/pgx/v5/pgtype" "github.com/jackc/pgx/v5/pgxpool" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" @@ -280,3 +281,141 @@ func TestCandidateTags_CoverageCountsProcessedAndTagged(t *testing.T) { t.Errorf("with_tags = %d, want 2 ('none' excluded)", got.WithTags) } } + +// --- Slice 6 (#2377): the taste-tag blend, end to end --- +// +// The pure tests in tagoverlap_test.go cover the scoring maths. These cover the +// wiring the pure tests cannot reach: that loadTagInputs actually reads both +// sides from the DB and that the blend reaches the returned deck. + +func seedTasteTag(t *testing.T, pool *pgxpool.Pool, userID pgtype.UUID, tag string, weight float64) { + t.Helper() + if _, err := pool.Exec(context.Background(), + `INSERT INTO taste_profile_tags (user_id, tag, weight) VALUES ($1, $2, $3) + ON CONFLICT (user_id, tag) DO UPDATE SET weight = EXCLUDED.weight`, + userID, tag, weight, + ); err != nil { + t.Fatalf("seedTasteTag: %v", err) + } +} + +func seedCandidateTag(t *testing.T, pool *pgxpool.Pool, mbid, tag string, weight float64) { + t.Helper() + if err := dbq.New(pool).InsertCandidateArtistTag(context.Background(), + dbq.InsertCandidateArtistTagParams{CandidateMbid: mbid, Tag: tag, Weight: weight}, + ); err != nil { + t.Fatalf("seedCandidateTag: %v", err) + } +} + +// twoCandidatePool wires a liked seed with two unmatched neighbours: "loud" +// scores higher on similarity, "match" lower. Skewed on purpose — with equal +// similarity the reorder assertion below could not fail. +func twoCandidatePool(t *testing.T, pool *pgxpool.Pool, userID pgtype.UUID) { + t.Helper() + seed := seedArtist(t, pool, "Seed", "") + likeArtist(t, pool, userID, seed.ID) + seedUnmatched(t, pool, seed.ID, "loud", "Loud Neighbour", 0.9) + seedUnmatched(t, pool, seed.ID, "match", "Taste Match", 0.6) +} + +func TestSuggestArtists_TasteTagMatchOvertakesStrongerSimilarity(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + twoCandidatePool(t, pool, user.ID) + + seedTasteTag(t, pool, user.ID, "shoegaze", 10) + seedCandidateTag(t, pool, "match", "shoegaze", 1.0) + seedCandidateTag(t, pool, "loud", "death metal", 1.0) + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 1.0) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 2 { + t.Fatalf("len = %d, want 2", len(out)) + } + if out[0].MBID != "match" { + t.Errorf("first = %q, want match (0.6×2 beats 0.9×1)", out[0].MBID) + } + // And it explains itself. + if len(out[0].MatchedTags) != 1 || out[0].MatchedTags[0] != "shoegaze" { + t.Errorf("matched tags = %v, want [shoegaze]", out[0].MatchedTags) + } + if out[1].MatchedTags != nil { + t.Errorf("non-matching candidate got matched tags: %v", out[1].MatchedTags) + } +} + +// The same fixture with the knob at 0 must return pure similarity order — the +// operator's off switch, verified against a real DB rather than assumed. +func TestSuggestArtists_ZeroTagWeightKeepsSimilarityOrder(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + twoCandidatePool(t, pool, user.ID) + + seedTasteTag(t, pool, user.ID, "shoegaze", 10) + seedCandidateTag(t, pool, "match", "shoegaze", 1.0) + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if out[0].MBID != "loud" { + t.Errorf("first = %q, want loud (tag term disabled)", out[0].MBID) + } +} + +// A user with no taste profile must still get a full deck in similarity order: +// the cold-start path, which is every user's first days (rule #131). +func TestSuggestArtists_NoTasteTagsStillReturnsSimilarityOrder(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + twoCandidatePool(t, pool, user.ID) + // Candidate tags exist, but the user has no taste tags to match them. + seedCandidateTag(t, pool, "match", "shoegaze", 1.0) + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 1.0) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 2 { + t.Fatalf("len = %d, want 2 (nothing dropped)", len(out)) + } + if out[0].MBID != "loud" { + t.Errorf("first = %q, want loud", out[0].MBID) + } +} + +// An untagged candidate must never be dropped or sunk just because another +// candidate has tags — permanently-partial coverage (#2376) must not become a +// permanent ranking penalty. +func TestSuggestArtists_UntaggedCandidateSurvivesAlongsideTagged(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + twoCandidatePool(t, pool, user.ID) + + seedTasteTag(t, pool, user.ID, "shoegaze", 10) + seedCandidateTag(t, pool, "match", "shoegaze", 1.0) + // "loud" is deliberately left with NO cached tags at all. + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 1.0) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 2 { + t.Fatalf("len = %d, want 2 — the untagged candidate must still appear", len(out)) + } + var loud *ArtistSuggestion + for i := range out { + if out[i].MBID == "loud" { + loud = &out[i] + } + } + if loud == nil { + t.Fatal("untagged candidate vanished from the deck") + } + if loud.Score != 0.9 { + t.Errorf("untagged score = %v, want 0.9 unchanged", loud.Score) + } +} diff --git a/internal/recommendation/suggestions.go b/internal/recommendation/suggestions.go index 1898d35b..348ebf6d 100644 --- a/internal/recommendation/suggestions.go +++ b/internal/recommendation/suggestions.go @@ -31,6 +31,15 @@ type ArtistSuggestion struct { Name string Score float64 Attribution []SeedContribution + // MatchedTags are the candidate's own tags that overlap the user's taste + // profile, strongest first (max 3) — the "matches: shoegaze, melancholic" + // explanation (#2377). Empty when the candidate has no cached tags, which + // is common and not an error: coverage is permanently partial (#2376). + MatchedTags []string + // TagOverlap is the [0,1] share of the candidate's tag mass the user likes. + // Exposed for the admin tuning lab — seeing the term's actual distribution + // is how the operator picks a weight rather than guessing at one. + TagOverlap float64 } // SeedContribution is one of the top-3 contributing seeds for a candidate. @@ -49,7 +58,10 @@ type SeedContribution struct { // seed path only — the likes + completed-plays fallback used while the user // has no taste-profile rows yet. Once the profile is populated it seeds // instead, carrying its own decay, so this knob stops applying. -func SuggestArtists(ctx context.Context, pool *pgxpool.Pool, userID pgtype.UUID, halfLifeDays float64, limit int) ([]ArtistSuggestion, error) { +func SuggestArtists( + ctx context.Context, pool *pgxpool.Pool, userID pgtype.UUID, + halfLifeDays float64, limit int, tagOverlapWeight float64, +) ([]ArtistSuggestion, error) { if limit <= 0 || limit > 50 { limit = 12 } @@ -117,9 +129,71 @@ func SuggestArtists(ctx context.Context, pool *pgxpool.Pool, userID pgtype.UUID, Attribution: attribution, }) } + + // Taste-tag term (#2377). Applied to the whole over-fetched pool BEFORE + // selection, so the rotation and diversity rules in selectSuggestions + // operate on taste-blended scores — boosting only the twelve already + // chosen by similarity would leave the actual re-ranking undone. + // + // A query error here is returned, NOT degraded past. Graceful degradation + // is for expected absence — no taste profile yet, no cached tags for a + // candidate — and both of those are handled explicitly as empty inputs + // below. A failing query is neither: swallowing it would hide a broken DB + // behind a subtly worse ranking that nothing reports. + tasteTags, candTags, err := loadTagInputs(ctx, q, userID, out) + if err != nil { + return nil, err + } + out = applyTagOverlap(out, candTags, tasteTags, tagOverlapWeight) + return selectSuggestions(out, limit, rotationDay(time.Now())), nil } +// tasteTagLimit caps how many of the user's taste tags participate. The +// profile's long tail is near-zero weight and contributes nothing after +// normalization, so this bounds the query rather than the meaning. +const tasteTagLimit = 50 + +// loadTagInputs fetches both sides of the overlap comparison: the user's taste +// tags and the cached tags for exactly the candidates in this pool. +func loadTagInputs( + ctx context.Context, q *dbq.Queries, userID pgtype.UUID, pool []ArtistSuggestion, +) (TagWeights, map[string]TagWeights, error) { + tasteRows, err := q.ListTasteProfileTagsForUser(ctx, dbq.ListTasteProfileTagsForUserParams{ + UserID: userID, + Limit: tasteTagLimit, + }) + if err != nil { + return nil, nil, fmt.Errorf("suggest: taste tags: %w", err) + } + // No taste tags is a cold start, not a failure — return early and skip the + // candidate-tag fetch entirely, since nothing could match. + if len(tasteRows) == 0 { + return nil, nil, nil + } + taste := make(TagWeights, len(tasteRows)) + for _, r := range tasteRows { + taste[r.Tag] = r.Weight + } + + mbids := make([]string, 0, len(pool)) + for _, s := range pool { + mbids = append(mbids, s.MBID) + } + tagRows, err := q.ListCandidateArtistTagsForMbids(ctx, mbids) + if err != nil { + return nil, nil, fmt.Errorf("suggest: candidate tags: %w", err) + } + byCandidate := make(map[string]TagWeights, len(pool)) + for _, r := range tagRows { + if byCandidate[r.CandidateMbid] == nil { + byCandidate[r.CandidateMbid] = TagWeights{} + } + byCandidate[r.CandidateMbid][r.Tag] = r.Weight + } + return taste, byCandidate, nil +} + // Pool multiplier: how many scored candidates to fetch per slot shown, so the // rotation has somewhere to rotate. 4x keeps a day's deck genuinely different // from yesterday's without pulling the whole long tail (whose scores are noise) diff --git a/internal/recommendation/suggestions_integration_test.go b/internal/recommendation/suggestions_integration_test.go index 1f82da8c..7a8cd40c 100644 --- a/internal/recommendation/suggestions_integration_test.go +++ b/internal/recommendation/suggestions_integration_test.go @@ -147,7 +147,7 @@ func TestSuggestArtists_LikesAndPlaysContributeToScore(t *testing.T) { seedUnmatched(t, pool, seedA.ID, "out-mbid", "Outsider", 0.9) seedUnmatched(t, pool, seedB.ID, "out-mbid", "Outsider", 0.5) - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -174,7 +174,7 @@ func TestSuggestArtists_Top12Cap(t *testing.T) { for i := 0; i < 30; i++ { seedUnmatched(t, pool, seed.ID, fmt.Sprintf("mbid-%02d", i), fmt.Sprintf("Artist %02d", i), 0.99-float64(i)*0.01) } - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -195,7 +195,7 @@ func TestSuggestArtists_AttributionTopThree(t *testing.T) { likeArtist(t, pool, user.ID, seeds[i].ID) seedUnmatched(t, pool, seeds[i].ID, "shared-mbid", "Shared", 0.9-float64(i)*0.1) } - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -227,7 +227,7 @@ func TestSuggestArtists_RecencyDecayDownweightsOldPlays(t *testing.T) { seedUnmatched(t, pool, recentSeed.ID, "cand", "Cand", 0.5) seedUnmatched(t, pool, oldSeed.ID, "cand", "Cand", 0.5) - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -255,7 +255,7 @@ func TestSuggestArtists_FiltersInLibraryCandidates(t *testing.T) { seedArtist(t, pool, "InLib", inLibMBID) seedUnmatched(t, pool, seed.ID, inLibMBID, "InLib", 0.9) - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -279,7 +279,7 @@ func TestSuggestArtists_FiltersAlreadyRequested(t *testing.T) { t.Fatalf("CreateLidarrRequest: %v", err) } - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -310,7 +310,7 @@ func TestSuggestArtists_RejectedRequestStillShown(t *testing.T) { t.Fatalf("RejectLidarrRequest: %v", err) } - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -322,7 +322,7 @@ func TestSuggestArtists_RejectedRequestStillShown(t *testing.T) { func TestSuggestArtists_EmptyForNewUser(t *testing.T) { pool := newPool(t) user := seedUser(t, pool, "newbie") - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -374,7 +374,7 @@ func TestSuggestArtists_TasteProfileWeightSeedsTier1(t *testing.T) { setTasteWeight(t, pool, user.ID, seed.ID, 4.0) seedUnmatched(t, pool, seed.ID, "out-mbid", "Outsider", 0.9) - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -408,7 +408,7 @@ func TestSuggestArtists_TasteProfileSupersedesRawPlays(t *testing.T) { seedUnmatched(t, pool, kept.ID, "kept-cand", "Kept Candidate", 0.9) seedUnmatched(t, pool, dropped.ID, "dropped-cand", "Dropped Candidate", 0.9) - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -435,7 +435,7 @@ func TestSuggestArtists_NonPositiveTasteWeightDoesNotSeed(t *testing.T) { seedUnmatched(t, pool, positive.ID, "pos-cand", "Positive Candidate", 0.9) seedUnmatched(t, pool, abandoned.ID, "neg-cand", "Negative Candidate", 0.9) - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -464,7 +464,7 @@ func TestSuggestArtists_SkippedPlaysDoNotSeedTier2(t *testing.T) { } seedUnmatched(t, pool, skipped.ID, "skip-cand", "Skip Candidate", 0.9) - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -518,7 +518,7 @@ func TestSuggestArtists_ActiveSnoozeHidesCandidate(t *testing.T) { t.Fatalf("SnoozeSuggestion: %v", err) } - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -534,7 +534,7 @@ func TestSuggestArtists_ExpiredSnoozeShowsCandidateAgain(t *testing.T) { seedOneCandidate(t, pool, user.ID, "expired-mbid", "Returning Artist") snoozeUntil(t, pool, user.ID, "expired-mbid", "Returning Artist", time.Now().Add(-time.Hour)) - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -560,14 +560,14 @@ func TestSuggestArtists_SnoozeIsPerUser(t *testing.T) { snoozeUntil(t, pool, alice.ID, "shared-mbid", "Shared Candidate", time.Now().Add(24*time.Hour)) - aliceOut, err := SuggestArtists(context.Background(), pool, alice.ID, 30, 12) + aliceOut, err := SuggestArtists(context.Background(), pool, alice.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists(alice): %v", err) } if len(aliceOut) != 0 { t.Errorf("alice len = %d, want 0 (she snoozed it)", len(aliceOut)) } - bobOut, err := SuggestArtists(context.Background(), pool, bob.ID, 30, 12) + bobOut, err := SuggestArtists(context.Background(), pool, bob.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists(bob): %v", err) } @@ -604,7 +604,7 @@ func TestUnsnoozeSuggestion_RestoresImmediately(t *testing.T) { t.Errorf("repeat rows = %d, want 0", rows) } - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } diff --git a/internal/recommendation/tagoverlap.go b/internal/recommendation/tagoverlap.go new file mode 100644 index 00000000..8e2d3bbd --- /dev/null +++ b/internal/recommendation/tagoverlap.go @@ -0,0 +1,156 @@ +// tagoverlap.go — the taste-tag term for the Discover request surface +// (#2377, milestone #268 slice 6). +// +// Slices 1-2 made the deck stop repeating; this is what makes it *relevant*. +// Before this, a candidate's only claim on a slot was "some artist you play is +// similar to it" — a graph-adjacency fact that says nothing about whether the +// music sounds like anything you actually like. Here the candidate's own +// folksonomy tags (cached by slice 5) are compared against the user's +// taste-profile tags, so the surface can rank on "matches the sound you like" +// and say WHY. +// +// Pure by design — no DB, no clock — so the scoring rules are unit-testable in +// the fast lane rather than behind the integration gate. +package recommendation + +import "sort" + +// maxMatchedTags caps the "matches: …" explanation. Three is what the existing +// seed attribution shows, and a longer list stops being a reason and becomes a +// tag dump. +const maxMatchedTags = 3 + +// TagWeights is a tag → weight map. Both sides of the comparison use it: +// candidate tags (normalized [0,1] by the enrichment providers) and the user's +// taste-profile tags (accumulated, unbounded — normalized here). +type TagWeights map[string]float64 + +// tagOverlap scores how much of a candidate's tag identity the user actually +// likes, in [0,1], and returns the matched tags ordered by contribution. +// +// The measure is: of this candidate's total tag mass, what share sits on tags +// the user likes — each weighted by how strongly they like it? +// +// overlap = Σ(shared) candWeight × normalizedTasteWeight ÷ Σ(all) candWeight +// +// Normalizing the taste side by the user's STRONGEST tag is what makes this +// comparable across users: taste weights accumulate with listening, so a +// heavy listener's raw numbers dwarf a new user's while meaning the same +// thing — "this is my favourite tag". Dividing by the candidate's own total +// mass makes it comparable across candidates, so a densely-tagged artist +// can't out-score a sparsely-tagged one just by having more tags. +// +// Returns (0, nil) when either side is empty. That is the load-bearing +// degradation path: tag coverage for out-of-library candidates is permanently +// partial (#2376), and a cold-start user has no taste tags at all. Both must +// leave the candidate's similarity score untouched rather than sink it — +// rule #131, tiered degradation, never vanish-or-nothing. +func tagOverlap(candidate, taste TagWeights) (float64, []string) { + if len(candidate) == 0 || len(taste) == 0 { + return 0, nil + } + + maxTaste := 0.0 + for _, w := range taste { + if w > maxTaste { + maxTaste = w + } + } + // Every taste weight <= 0 carries no preference to match against. Guarding + // here also avoids dividing by zero below. + if maxTaste <= 0 { + return 0, nil + } + + totalMass := 0.0 + for _, w := range candidate { + // Negative or zero candidate weights would let a tag subtract from the + // denominator and inflate the ratio past 1. + if w > 0 { + totalMass += w + } + } + if totalMass <= 0 { + return 0, nil + } + + type contribution struct { + tag string + score float64 + } + var matched []contribution + sum := 0.0 + for tag, candWeight := range candidate { + if candWeight <= 0 { + continue + } + tasteWeight, ok := taste[tag] + if !ok || tasteWeight <= 0 { + continue + } + c := candWeight * (tasteWeight / maxTaste) + sum += c + matched = append(matched, contribution{tag: tag, score: c}) + } + if len(matched) == 0 { + return 0, nil + } + + // Strongest contribution first; tag name breaks ties so the explanation is + // deterministic for a given input rather than map-iteration order. + sort.Slice(matched, func(i, j int) bool { + if matched[i].score != matched[j].score { + return matched[i].score > matched[j].score + } + return matched[i].tag < matched[j].tag + }) + names := make([]string, 0, min(len(matched), maxMatchedTags)) + for i := 0; i < len(matched) && i < maxMatchedTags; i++ { + names = append(names, matched[i].tag) + } + return sum / totalMass, names +} + +// applyTagOverlap re-scores and re-orders a candidate pool by taste-tag +// overlap, stamping the matched tags onto each suggestion for the UI. +// +// The blend is MULTIPLICATIVE — score × (1 + weight × overlap) — not additive, +// and the difference is the whole safety argument: +// +// - A candidate with no tags has overlap 0, so its score is EXACTLY +// unchanged. Partial tag coverage costs a candidate nothing. +// - Nothing can leapfrog on tags alone. An additive term with a large +// weight would let a near-zero-similarity artist outrank a strong match +// just for sharing a popular tag, which reads as noise to the user. +// - weight 0 disables the feature completely and restores pure similarity +// order, so the operator's knob has a real off position. +// +// Callers must pass the pool in similarity order; it is returned in blended +// order. Mutates the elements in place (they're the caller's own slice built +// per request), and re-sorts, because selectSuggestions downstream relies on +// score order for its head/tail split. +func applyTagOverlap( + pool []ArtistSuggestion, candidateTags map[string]TagWeights, taste TagWeights, weight float64, +) []ArtistSuggestion { + // A zero weight is the operator turning the feature off. Skip the work + // AND the re-sort so the ordering is bit-for-bit the pre-slice-6 result. + if weight == 0 || len(taste) == 0 { + return pool + } + for i := range pool { + overlap, matched := tagOverlap(candidateTags[pool[i].MBID], taste) + pool[i].MatchedTags = matched + pool[i].TagOverlap = overlap + pool[i].Score *= 1 + weight*overlap + } + sort.SliceStable(pool, func(i, j int) bool { + if pool[i].Score != pool[j].Score { + return pool[i].Score > pool[j].Score + } + // Stable tiebreak by MBID. Without it, two candidates on equal scores + // could swap between requests within the same day, which the daily + // rotation (#2373) exists to prevent. + return pool[i].MBID < pool[j].MBID + }) + return pool +} diff --git a/internal/recommendation/tagoverlap_test.go b/internal/recommendation/tagoverlap_test.go new file mode 100644 index 00000000..e66269e4 --- /dev/null +++ b/internal/recommendation/tagoverlap_test.go @@ -0,0 +1,244 @@ +package recommendation + +import ( + "fmt" + "testing" +) + +// tagOverlap / applyTagOverlap are pure, so slice 6's ranking rules are covered +// in the fast lane rather than behind the integration gate. +// +// Fixtures here are deliberately SKEWED — candidates that match the taste +// profile to clearly different degrees. An evenly-matching pool cannot exercise +// a re-ranking at all: every candidate gets the same multiplier and the order is +// unchanged whether the blend works or not. Slice 2's first diversity test had +// exactly that defect, so it's called out explicitly here. + +func TestTagOverlap_FullMatchScoresOne(t *testing.T) { + // Every unit of the candidate's tag mass sits on the user's single + // strongest tag → the whole mass matches at full strength. + got, matched := tagOverlap( + TagWeights{"shoegaze": 1.0}, + TagWeights{"shoegaze": 5.0}, + ) + if got != 1.0 { + t.Errorf("overlap = %v, want 1.0", got) + } + if len(matched) != 1 || matched[0] != "shoegaze" { + t.Errorf("matched = %v, want [shoegaze]", matched) + } +} + +func TestTagOverlap_NoSharedTagsScoresZero(t *testing.T) { + got, matched := tagOverlap( + TagWeights{"death metal": 1.0}, + TagWeights{"shoegaze": 5.0}, + ) + if got != 0 { + t.Errorf("overlap = %v, want 0", got) + } + if matched != nil { + t.Errorf("matched = %v, want nil", matched) + } +} + +// Half the candidate's mass is on a matching tag, and that tag is the user's +// strongest → 0.5. +func TestTagOverlap_PartialMassMatchIsProportional(t *testing.T) { + got, _ := tagOverlap( + TagWeights{"shoegaze": 1.0, "death metal": 1.0}, + TagWeights{"shoegaze": 5.0}, + ) + if got != 0.5 { + t.Errorf("overlap = %v, want 0.5", got) + } +} + +// Matching a tag the user barely likes must score below matching one they love. +func TestTagOverlap_WeakTasteTagScoresLowerThanStrong(t *testing.T) { + taste := TagWeights{"shoegaze": 10.0, "polka": 1.0} + strong, _ := tagOverlap(TagWeights{"shoegaze": 1.0}, taste) + weak, _ := tagOverlap(TagWeights{"polka": 1.0}, taste) + if !(strong > weak) { + t.Errorf("strong=%v weak=%v — a favourite tag must outscore a marginal one", strong, weak) + } + if weak != 0.1 { // 1.0 * (1/10) / 1.0 + t.Errorf("weak = %v, want 0.1", weak) + } +} + +// THE degradation path. Tag coverage for out-of-library candidates is +// permanently partial (#2376) and cold-start users have no taste tags, so both +// must be scored 0 — never negative, never dropped (rule #131). +func TestTagOverlap_EmptyEitherSideIsZeroNotNegative(t *testing.T) { + cases := []struct { + name string + candidate, taste TagWeights + }{ + {"no candidate tags", nil, TagWeights{"shoegaze": 5}}, + {"no taste tags", TagWeights{"shoegaze": 1}, nil}, + {"both empty", nil, nil}, + {"taste weights all zero", TagWeights{"shoegaze": 1}, TagWeights{"shoegaze": 0}}, + {"candidate weights all zero", TagWeights{"shoegaze": 0}, TagWeights{"shoegaze": 5}}, + } + for _, c := range cases { + got, matched := tagOverlap(c.candidate, c.taste) + if got != 0 { + t.Errorf("%s: overlap = %v, want 0", c.name, got) + } + if matched != nil { + t.Errorf("%s: matched = %v, want nil", c.name, matched) + } + } +} + +// A densely-tagged artist must not out-score a focused one merely by having +// more tags — that's what dividing by the candidate's own mass buys. +func TestTagOverlap_IsNotAPopularityContest(t *testing.T) { + taste := TagWeights{"shoegaze": 5.0} + focused, _ := tagOverlap(TagWeights{"shoegaze": 1.0}, taste) + sprawling, _ := tagOverlap(TagWeights{ + "shoegaze": 1.0, "rock": 1.0, "alternative": 1.0, "90s": 1.0, + }, taste) + if !(focused > sprawling) { + t.Errorf("focused=%v sprawling=%v — extra unmatched tags must dilute, not add", + focused, sprawling) + } +} + +// Taste weights accumulate with listening, so raw magnitudes differ wildly +// between a new user and a heavy one while meaning the same thing. Normalizing +// by the user's own strongest tag is what makes the score comparable. +func TestTagOverlap_IsInvariantToTasteMagnitude(t *testing.T) { + candidate := TagWeights{"shoegaze": 1.0, "dream pop": 1.0} + newUser, _ := tagOverlap(candidate, TagWeights{"shoegaze": 2.0, "polka": 1.0}) + heavyUser, _ := tagOverlap(candidate, TagWeights{"shoegaze": 2000.0, "polka": 1000.0}) + if newUser != heavyUser { + t.Errorf("newUser=%v heavyUser=%v — scaling all taste weights must not change the result", + newUser, heavyUser) + } +} + +func TestTagOverlap_MatchedTagsAreOrderedAndCapped(t *testing.T) { + got, matched := tagOverlap( + TagWeights{"a": 0.2, "b": 0.9, "c": 0.5, "d": 0.7}, + TagWeights{"a": 10, "b": 10, "c": 10, "d": 10}, + ) + if got <= 0 { + t.Fatalf("overlap = %v, want > 0", got) + } + // All taste weights equal, so candidate weight decides: b(.9) d(.7) c(.5). + want := []string{"b", "d", "c"} + if fmt.Sprint(matched) != fmt.Sprint(want) { + t.Errorf("matched = %v, want %v (strongest first, capped at %d)", + matched, want, maxMatchedTags) + } +} + +// --- applyTagOverlap --- + +func poolFor(specs ...struct { + mbid string + score float64 +}) []ArtistSuggestion { + out := make([]ArtistSuggestion, 0, len(specs)) + for _, s := range specs { + out = append(out, ArtistSuggestion{MBID: s.mbid, Name: s.mbid, Score: s.score}) + } + return out +} + +type spec = struct { + mbid string + score float64 +} + +// The payoff, and the reason the fixture is skewed: a taste-matching candidate +// that started BELOW another must be able to overtake it. With an evenly +// matching pool this assertion could not fail. +func TestApplyTagOverlap_TasteMatchOvertakesAStrongerNonMatch(t *testing.T) { + pool := poolFor(spec{"loud", 1.0}, spec{"match", 0.7}) + candTags := map[string]TagWeights{ + "loud": {"death metal": 1.0}, + "match": {"shoegaze": 1.0}, + } + taste := TagWeights{"shoegaze": 5.0} + + got := applyTagOverlap(pool, candTags, taste, 1.0) + if got[0].MBID != "match" { + t.Errorf("first = %q, want match (0.7×2 = 1.4 beats 1.0×1)", got[0].MBID) + } + if got[0].MatchedTags == nil { + t.Error("matched tags not stamped onto the winner") + } +} + +// An untagged candidate keeps its score EXACTLY. This is the multiplicative +// blend's whole safety argument: partial coverage costs a candidate nothing. +func TestApplyTagOverlap_UntaggedCandidateScoreIsUnchanged(t *testing.T) { + pool := poolFor(spec{"untagged", 0.9}) + got := applyTagOverlap(pool, map[string]TagWeights{}, TagWeights{"shoegaze": 5}, 1.0) + if got[0].Score != 0.9 { + t.Errorf("score = %v, want 0.9 exactly (no tags must not penalise)", got[0].Score) + } + if got[0].MatchedTags != nil { + t.Errorf("matched = %v, want nil", got[0].MatchedTags) + } +} + +// Weight 0 is the operator's off switch: the ordering must be bit-for-bit the +// pre-slice-6 result, not merely similar. +func TestApplyTagOverlap_ZeroWeightIsAnExactNoOp(t *testing.T) { + pool := poolFor(spec{"a", 1.0}, spec{"b", 0.7}) + candTags := map[string]TagWeights{"b": {"shoegaze": 1.0}} + got := applyTagOverlap(pool, candTags, TagWeights{"shoegaze": 5.0}, 0) + if got[0].MBID != "a" || got[0].Score != 1.0 || got[1].Score != 0.7 { + t.Errorf("weight 0 changed the pool: %+v", got) + } + // And it must not stamp matched tags either — the UI would otherwise + // explain a boost that never happened. + if got[1].MatchedTags != nil { + t.Errorf("matched tags stamped while the feature is off: %v", got[1].MatchedTags) + } +} + +// A user with no taste profile is the cold-start case: every candidate scores +// the same multiplier of 1, so similarity order must survive intact. +func TestApplyTagOverlap_NoTasteProfileLeavesOrderIntact(t *testing.T) { + pool := poolFor(spec{"a", 1.0}, spec{"b", 0.7}, spec{"c", 0.4}) + candTags := map[string]TagWeights{"c": {"shoegaze": 1.0}} + got := applyTagOverlap(pool, candTags, nil, 1.0) + for i, want := range []string{"a", "b", "c"} { + if got[i].MBID != want { + t.Fatalf("order = %v..., want a b c", got[i].MBID) + } + } +} + +// Bounded boost: even a perfect match at the maximum weight can only multiply +// by (1 + w), so a candidate cannot be catapulted arbitrarily far. +func TestApplyTagOverlap_BoostIsBoundedByOnePlusWeight(t *testing.T) { + pool := poolFor(spec{"perfect", 1.0}) + got := applyTagOverlap(pool, + map[string]TagWeights{"perfect": {"shoegaze": 1.0}}, + TagWeights{"shoegaze": 5.0}, 2.0) + if got[0].Score != 3.0 { + t.Errorf("score = %v, want 3.0 (1.0 × (1 + 2×1))", got[0].Score) + } + if got[0].TagOverlap != 1.0 { + t.Errorf("TagOverlap = %v, want 1.0", got[0].TagOverlap) + } +} + +// Equal blended scores must resolve deterministically, or two candidates could +// swap between requests inside one day — the exact churn the daily rotation +// (#2373) exists to prevent. +func TestApplyTagOverlap_TiesBreakDeterministically(t *testing.T) { + first := applyTagOverlap(poolFor(spec{"zzz", 1.0}, spec{"aaa", 1.0}), + map[string]TagWeights{}, TagWeights{"x": 1}, 1.0) + second := applyTagOverlap(poolFor(spec{"aaa", 1.0}, spec{"zzz", 1.0}), + map[string]TagWeights{}, TagWeights{"x": 1}, 1.0) + if first[0].MBID != "aaa" || second[0].MBID != "aaa" { + t.Errorf("tie order not deterministic: %q then %q", first[0].MBID, second[0].MBID) + } +} diff --git a/internal/recsettings/patch.go b/internal/recsettings/patch.go index 6ae86833..f7139cdf 100644 --- a/internal/recsettings/patch.go +++ b/internal/recsettings/patch.go @@ -161,6 +161,70 @@ func applyTastePatch(current TasteTuning, patch map[string]float64) (TasteTuning return next, changes, nil } +// Discover tuning bounds. +const ( + // A tag-overlap weight above this stops being a boost and becomes the + // ranking — at 10, a perfect match multiplies similarity by 11, which lets + // tag agreement swamp the similarity signal entirely. The bound is for + // typos, not to constrain exploration; the multiplicative blend keeps even + // the maximum from reordering an untagged candidate. + tagOverlapWeightMax = 10.0 + // Snooze duration: at least a day (anything less isn't a snooze, it's a + // flicker), at most a year — past that it's a permanent dismissal wearing a + // snooze's clothes, which is exactly the shape rule #101 rules out. + snoozeDaysMin = 1.0 + snoozeDaysMax = 365.0 +) + +// applyDiscoverPatch validates and applies a partial Discover update. +func applyDiscoverPatch( + current DiscoverTuning, patch map[string]float64, +) (DiscoverTuning, []fieldChange, error) { + next := current + var changes []fieldChange + for field, v := range patch { + var target *float64 + switch field { + case "tag_overlap_weight": + if v < 0 || v > tagOverlapWeightMax { + return current, nil, fmt.Errorf("%w: %s = %v (must be in [0, %v])", + ErrOutOfRange, field, v, tagOverlapWeightMax) + } + target = &next.TagOverlapWeight + case "snooze_days": + if v < snoozeDaysMin || v > snoozeDaysMax { + return current, nil, fmt.Errorf("%w: %s = %v (must be in [%v, %v])", + ErrOutOfRange, field, v, snoozeDaysMin, snoozeDaysMax) + } + target = &next.SnoozeDays + default: + return current, nil, fmt.Errorf("%w: %q", ErrUnknownField, field) + } + if *target == v { + continue + } + changes = append(changes, fieldChange{Field: field, Old: *target, New: v}) + *target = v + } + return next, changes, nil +} + +// diffDiscover returns per-field changes from a to b (empty when equal). +func diffDiscover(a, b DiscoverTuning) []fieldChange { + var out []fieldChange + if a.TagOverlapWeight != b.TagOverlapWeight { + out = append(out, fieldChange{ + Field: "tag_overlap_weight", Old: a.TagOverlapWeight, New: b.TagOverlapWeight, + }) + } + if a.SnoozeDays != b.SnoozeDays { + out = append(out, fieldChange{ + Field: "snooze_days", Old: a.SnoozeDays, New: b.SnoozeDays, + }) + } + return out +} + // diffWeights returns per-field changes from a to b (empty when equal). func diffWeights(a, b recommendation.ScoringWeights) []fieldChange { var out []fieldChange diff --git a/internal/recsettings/service.go b/internal/recsettings/service.go index 22640847..29b41d14 100644 --- a/internal/recsettings/service.go +++ b/internal/recsettings/service.go @@ -35,6 +35,11 @@ const ( ScopeRadio = "radio" ScopeDailyMix = "daily_mix" ScopeTaste = "taste" + // ScopeDiscover is the Discover request surface (#2377). Its own scope + // rather than columns on taste: SnoozeDays lives here, and a snooze must + // never be read as taste signal (#2374) — filing it under taste would put + // it one careless join from the leak that design forbids. + ScopeDiscover = "discover" ) // TasteTuning is the tunable subset of taste.Config: the engagement @@ -87,6 +92,33 @@ func ShippedDailyMixWeights() recommendation.ScoringWeights { } } +// DiscoverTuning is the tunable set for the Discover request surface (#2377). +type DiscoverTuning struct { + // TagOverlapWeight scales the taste-tag term: score × (1 + w × overlap). + // 0 disables it and restores pure similarity ranking. + TagOverlapWeight float64 + // SnoozeDays is the default "not right now" duration (#2374). + SnoozeDays float64 +} + +// ShippedDiscoverTuning are the shipped Discover defaults. +// +// TagOverlapWeight 1.0 lets a perfect tag match at most double a candidate's +// similarity score — enough to reorder the deck meaningfully, not enough for a +// popular-tag coincidence to beat a genuinely strong similarity match. It is a +// starting point for the tuning lab, not a tuned value: the honest way to pick +// it is the metrics trend view after some real use. +// +// SnoozeDays 90 matches the operator's approved shape: long enough that a +// parked suggestion stops nagging, short enough that a taste shift brings it +// back on its own. +func ShippedDiscoverTuning() DiscoverTuning { + return DiscoverTuning{ + TagOverlapWeight: 1.0, + SnoozeDays: 90, + } +} + // ShippedTasteTuning mirrors taste.DefaultConfig's tunable subset. func ShippedTasteTuning() TasteTuning { d := taste.DefaultConfig() @@ -110,6 +142,7 @@ type Service struct { mu sync.RWMutex profiles map[string]recommendation.ScoringWeights taste TasteTuning + discover DiscoverTuning } // New boots the service: seeds shipped defaults for missing rows, @@ -151,6 +184,13 @@ func (s *Service) reconcile(ctx context.Context) error { }); err != nil { return fmt.Errorf("seed taste tuning: %w", err) } + sd := ShippedDiscoverTuning() + if err := q.UpsertDiscoverTuningDefaults(ctx, dbq.UpsertDiscoverTuningDefaultsParams{ + TagOverlapWeight: sd.TagOverlapWeight, + SnoozeDays: sd.SnoozeDays, + }); err != nil { + return fmt.Errorf("seed discover tuning: %w", err) + } rows, err := q.ListWeightProfiles(ctx) if err != nil { @@ -160,6 +200,10 @@ func (s *Service) reconcile(ctx context.Context) error { if err != nil { return fmt.Errorf("get taste tuning: %w", err) } + dt, err := q.GetDiscoverTuning(ctx) + if err != nil { + return fmt.Errorf("get discover tuning: %w", err) + } s.mu.Lock() s.profiles = map[string]recommendation.ScoringWeights{} @@ -175,6 +219,10 @@ func (s *Service) reconcile(ctx context.Context) error { EraScale: tt.EraScale, MoodScale: tt.MoodScale, } + s.discover = DiscoverTuning{ + TagOverlapWeight: dt.TagOverlapWeight, + SnoozeDays: dt.SnoozeDays, + } s.mu.Unlock() s.push() @@ -208,6 +256,15 @@ func (s *Service) Taste() TasteTuning { return s.taste } +// Discover returns the cached Discover-tuning values. Read per request by the +// suggestions handler, so an admin change takes effect on the next refresh +// with no restart (rule #25). +func (s *Service) Discover() DiscoverTuning { + s.mu.RLock() + defer s.mu.RUnlock() + return s.discover +} + // TasteConfig assembles the full taste.Config the profile builder // consumes: shipped non-tunable knobs (like bonuses, floors, caps) // plus the tuned half-life and curve. WindowDays scales with the @@ -267,6 +324,19 @@ func (s *Service) UpdateTaste(ctx context.Context, patch map[string]float64) err return s.persistTaste(ctx, next, "update", changes) } +// UpdateDiscover applies a partial update to the Discover tuning singleton. +func (s *Service) UpdateDiscover(ctx context.Context, patch map[string]float64) error { + current := s.Discover() + next, changes, err := applyDiscoverPatch(current, patch) + if err != nil { + return err + } + if len(changes) == 0 { + return nil + } + return s.persistDiscover(ctx, next, "update", changes) +} + // Reset restores a scope to its shipped defaults, with one audit row // carrying the full diff. A scope already at defaults is a no-op. func (s *Service) Reset(ctx context.Context, scope string) error { @@ -288,6 +358,13 @@ func (s *Service) Reset(ctx context.Context, scope string) error { return nil } return s.persistTaste(ctx, shipped, "reset", changes) + case ScopeDiscover: + shipped := ShippedDiscoverTuning() + changes := diffDiscover(s.Discover(), shipped) + if len(changes) == 0 { + return nil + } + return s.persistDiscover(ctx, shipped, "reset", changes) default: return fmt.Errorf("%w: %q", ErrUnknownScope, scope) } @@ -338,6 +415,28 @@ func (s *Service) persistTaste( return nil } +// persistDiscover writes the discover row + audit entry and refreshes the +// cache. No push(): unlike taste and daily_mix, nothing precomputes from these +// — the suggestions handler reads Discover() per request. +func (s *Service) persistDiscover( + ctx context.Context, d DiscoverTuning, action string, changes []fieldChange, +) error { + q := dbq.New(s.pool) + if _, err := q.UpdateDiscoverTuning(ctx, dbq.UpdateDiscoverTuningParams{ + TagOverlapWeight: d.TagOverlapWeight, + SnoozeDays: d.SnoozeDays, + }); err != nil { + return fmt.Errorf("update discover tuning: %w", err) + } + if err := s.audit(ctx, q, ScopeDiscover, action, changes); err != nil { + return err + } + s.mu.Lock() + s.discover = d + s.mu.Unlock() + return nil +} + // audit writes one recommendation_tuning_audit row. Changes are // sorted by field so rows are deterministic and diff-friendly. func (s *Service) audit( diff --git a/internal/recsettings/service_test.go b/internal/recsettings/service_test.go index cf96f8a3..083a38bd 100644 --- a/internal/recsettings/service_test.go +++ b/internal/recsettings/service_test.go @@ -281,3 +281,118 @@ func TestUpdate_NoOpWritesNoAudit(t *testing.T) { t.Errorf("no-op update wrote %d audit rows, want 0", len(rows)) } } + +// --- Discover scope (#2377, milestone #268 slice 6) --- + +func TestNew_SeedsDiscoverDefaults(t *testing.T) { + pool := newPool(t) + s := newService(t, pool) + got := s.Discover() + want := ShippedDiscoverTuning() + if got != want { + t.Errorf("Discover() = %+v, want shipped %+v", got, want) + } +} + +func TestUpdateDiscover_PersistsAndAudits(t *testing.T) { + pool := newPool(t) + s := newService(t, pool) + if err := s.UpdateDiscover(context.Background(), map[string]float64{ + "tag_overlap_weight": 2.5, + "snooze_days": 30, + }); err != nil { + t.Fatalf("UpdateDiscover: %v", err) + } + if got := s.Discover().TagOverlapWeight; got != 2.5 { + t.Errorf("TagOverlapWeight = %v, want 2.5", got) + } + if got := s.Discover().SnoozeDays; got != 30 { + t.Errorf("SnoozeDays = %v, want 30", got) + } + + // The audit row must land under the new scope. This is the assertion that + // would have caught a missing rule-#36 CHECK migration: without expanding + // recommendation_tuning_audit's whitelist, this INSERT fails at runtime. + rows := auditRows(t, pool) + if len(rows) != 1 { + t.Fatalf("audit rows = %d, want 1", len(rows)) + } + if rows[0].Scope != ScopeDiscover { + t.Errorf("audit scope = %q, want %q", rows[0].Scope, ScopeDiscover) + } + if len(rows[0].Changes) != 2 { + t.Errorf("audit changes = %+v, want both fields", rows[0].Changes) + } + + // Reload from the DB to prove it persisted rather than only caching. + s2 := newService(t, pool) + if got := s2.Discover().TagOverlapWeight; got != 2.5 { + t.Errorf("after reload TagOverlapWeight = %v, want 2.5 (not re-seeded to shipped)", got) + } +} + +func TestUpdateDiscover_Validation(t *testing.T) { + pool := newPool(t) + s := newService(t, pool) + cases := []struct { + name string + patch map[string]float64 + }{ + {"unknown field", map[string]float64{"nope": 1}}, + {"negative weight", map[string]float64{"tag_overlap_weight": -1}}, + {"weight past the typo bound", map[string]float64{"tag_overlap_weight": 100}}, + // A sub-day snooze isn't a snooze, it's a flicker. + {"snooze under a day", map[string]float64{"snooze_days": 0.5}}, + // Past a year it's a permanent dismissal wearing a snooze's clothes — + // the shape rule #101 rules out. + {"snooze past a year", map[string]float64{"snooze_days": 400}}, + } + for _, c := range cases { + if err := s.UpdateDiscover(context.Background(), c.patch); err == nil { + t.Errorf("%s: expected rejection, got nil", c.name) + } + } + if got := s.Discover(); got != ShippedDiscoverTuning() { + t.Errorf("a rejected patch mutated state: %+v", got) + } + if rows := auditRows(t, pool); len(rows) != 0 { + t.Errorf("rejected patches wrote %d audit rows, want 0", len(rows)) + } +} + +// Weight 0 must be accepted — it's the operator's off switch for the whole +// tag term, so a "must be positive" bound would remove their ability to +// disable the feature. +func TestUpdateDiscover_ZeroWeightIsAllowed(t *testing.T) { + pool := newPool(t) + s := newService(t, pool) + if err := s.UpdateDiscover(context.Background(), + map[string]float64{"tag_overlap_weight": 0}); err != nil { + t.Fatalf("UpdateDiscover(0): %v", err) + } + if got := s.Discover().TagOverlapWeight; got != 0 { + t.Errorf("TagOverlapWeight = %v, want 0", got) + } +} + +func TestResetDiscover_RestoresShippedDefaults(t *testing.T) { + pool := newPool(t) + s := newService(t, pool) + if err := s.UpdateDiscover(context.Background(), + map[string]float64{"tag_overlap_weight": 4}); err != nil { + t.Fatalf("UpdateDiscover: %v", err) + } + if err := s.Reset(context.Background(), ScopeDiscover); err != nil { + t.Fatalf("Reset: %v", err) + } + if got := s.Discover(); got != ShippedDiscoverTuning() { + t.Errorf("after reset = %+v, want shipped %+v", got, ShippedDiscoverTuning()) + } + // Already-at-defaults is a no-op: update + reset = 2 rows, not 3. + if err := s.Reset(context.Background(), ScopeDiscover); err != nil { + t.Fatalf("second Reset: %v", err) + } + if rows := auditRows(t, pool); len(rows) != 2 { + t.Errorf("audit rows = %d, want 2 (the no-op reset must not audit)", len(rows)) + } +} From cf0d37bf8ec5291317b3ca7f5563098367ec3306 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 2 Aug 2026 23:45:48 -0400 Subject: [PATCH 12/14] =?UTF-8?q?fix(discover):=20compare=20against=20a=20?= =?UTF-8?q?baseline=20run,=20not=20a=20hardcoded=20score=20=E2=80=94=20#23?= =?UTF-8?q?77?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestSuggestArtists_UntaggedCandidateSurvivesAlongsideTagged asserted the untagged candidate's score was 0.9 — the raw similarity value I'd seeded. It's actually 1.61, because the pool score is signal-weighted by the seed query: ln(1+signal) x similarity, and a liked seed carries signal 5, so ln(6) x 0.9. The assertion was testing the seeding arithmetic, which is a different layer and not what the test is about. Rewritten to run the same request twice — once with the tag term disabled, once enabled — and assert the untagged candidate's score is IDENTICAL across both. That states the real property (the blend leaves untagged candidates alone) without depending on how the pool score is derived, so it survives future changes to seeding. Added a sanity assertion that the TAGGED candidate's score did move, so the comparison can't pass by both runs being trivially identical — the same "a test that cannot fail" trap recorded for this milestone. Exact-preservation at the arithmetic level is already covered where it belongs, by TestApplyTagOverlap_UntaggedCandidateScoreIsUnchanged. Co-Authored-By: Claude Opus 5 (1M context) --- .../candidate_tags_integration_test.go | 58 +++++++++++++++---- 1 file changed, 46 insertions(+), 12 deletions(-) diff --git a/internal/recommendation/candidate_tags_integration_test.go b/internal/recommendation/candidate_tags_integration_test.go index 373b4134..19cefbc8 100644 --- a/internal/recommendation/candidate_tags_integration_test.go +++ b/internal/recommendation/candidate_tags_integration_test.go @@ -399,23 +399,57 @@ func TestSuggestArtists_UntaggedCandidateSurvivesAlongsideTagged(t *testing.T) { seedCandidateTag(t, pool, "match", "shoegaze", 1.0) // "loud" is deliberately left with NO cached tags at all. - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 1.0) + // Compared against the same request with the term disabled, rather than + // against a literal. The pool score is signal-weighted by the seed query + // (ln(1+signal) x similarity), so hardcoding a number here would assert + // against the seeding maths — a different layer — and break whenever that + // changes. The property under test is only that the blend leaves an + // untagged candidate alone. + blended, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 1.0) if err != nil { - t.Fatalf("SuggestArtists: %v", err) + t.Fatalf("SuggestArtists(blended): %v", err) } - if len(out) != 2 { - t.Fatalf("len = %d, want 2 — the untagged candidate must still appear", len(out)) + baseline, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) + if err != nil { + t.Fatalf("SuggestArtists(baseline): %v", err) } - var loud *ArtistSuggestion - for i := range out { - if out[i].MBID == "loud" { - loud = &out[i] + if len(blended) != 2 { + t.Fatalf("len = %d, want 2 — the untagged candidate must still appear", len(blended)) + } + + scoreOf := func(out []ArtistSuggestion, mbid string) (float64, bool) { + for _, s := range out { + if s.MBID == mbid { + return s.Score, true + } } + return 0, false } - if loud == nil { - t.Fatal("untagged candidate vanished from the deck") + want, ok := scoreOf(baseline, "loud") + if !ok { + t.Fatal("untagged candidate missing from the baseline deck") } - if loud.Score != 0.9 { - t.Errorf("untagged score = %v, want 0.9 unchanged", loud.Score) + got, ok := scoreOf(blended, "loud") + if !ok { + t.Fatal("untagged candidate vanished once the tag term was enabled") + } + if got != want { + t.Errorf("untagged score = %v, want %v (identical to the term-disabled run)", got, want) + } + // Sanity: the tagged candidate DID move, so the comparison above is + // meaningful rather than both runs being trivially identical. + if tagged, _ := scoreOf(blended, "match"); tagged == mustScore(t, baseline, "match") { + t.Error("the tagged candidate's score did not change — the term did nothing") } } + +func mustScore(t *testing.T, out []ArtistSuggestion, mbid string) float64 { + t.Helper() + for _, s := range out { + if s.MBID == mbid { + return s.Score + } + } + t.Fatalf("no candidate %q in %v", mbid, out) + return 0 +} From ca4832e62069085e6edd9f68172fe9db851a5b07 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 2 Aug 2026 23:49:17 -0400 Subject: [PATCH 13/14] =?UTF-8?q?feat(discover):=20Discover=20tuning=20car?= =?UTF-8?q?d=20on=20the=20admin=20lab=20=E2=80=94=20#2377=20(web=20admin)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rule #25/#27: the two knobs slice 6 added server-side are now touchable — taste-tag weight and snooze length, with deviation dots, save, and reset, matching the existing profile/taste cards. Copy states what each knob does AND what it doesn't: the tag-weight hint says 0 turns the term off and that an untagged candidate is never penalised, and the snooze hint says it records no opinion about the artist and never feeds the taste profile. Those are the two properties most likely to be assumed backwards by whoever turns these next. Also fixed a latent fragility the new card exposed rather than caused: all three reset buttons had the accessible name "Reset to defaults", so the existing test picked the LAST one and assumed that meant taste. Adding a card below it would have silently retargeted that assertion at the wrong scope. Each reset button now names its scope — better for screen readers too, since three identical buttons on one page is a real a11y defect — and the test selects by name instead of position. The page's test fixture needed the new `discover` key in both `snapshot` and `shipped`: the `as TuningSnapshot` cast means a missing field is not a compile error, it's every test on the page throwing inside fillForm. Noted that in the fixture so the next scope doesn't rediscover it. Includes a test that a weight of 0 is actually SENT rather than dropped as falsy — the off switch is the one value a truthiness bug would eat. Co-Authored-By: Claude Opus 5 (1M context) --- web/src/lib/api/tuning.ts | 12 +++- web/src/routes/admin/tuning/+page.svelte | 81 +++++++++++++++++++++- web/src/routes/admin/tuning/tuning.test.ts | 80 ++++++++++++++++++++- 3 files changed, 167 insertions(+), 6 deletions(-) diff --git a/web/src/lib/api/tuning.ts b/web/src/lib/api/tuning.ts index fae0f40f..10d1979d 100644 --- a/web/src/lib/api/tuning.ts +++ b/web/src/lib/api/tuning.ts @@ -26,14 +26,24 @@ export type TasteTuning = { mood_scale: number; }; -export type TuningScope = 'radio' | 'daily_mix' | 'taste'; +// Discover request-surface knobs (#2377). Its own scope rather than part of +// taste: snooze_days belongs here, and a snooze must never be read as taste +// signal (#2374). +export type DiscoverTuning = { + tag_overlap_weight: number; + snooze_days: number; +}; + +export type TuningScope = 'radio' | 'daily_mix' | 'taste' | 'discover'; export type TuningSnapshot = { profiles: Record<'radio' | 'daily_mix', WeightProfile>; taste: TasteTuning; + discover: DiscoverTuning; shipped: { profiles: Record<'radio' | 'daily_mix', WeightProfile>; taste: TasteTuning; + discover: DiscoverTuning; }; }; diff --git a/web/src/routes/admin/tuning/+page.svelte b/web/src/routes/admin/tuning/+page.svelte index 79e96668..33bb0b19 100644 --- a/web/src/routes/admin/tuning/+page.svelte +++ b/web/src/routes/admin/tuning/+page.svelte @@ -9,6 +9,7 @@ type TuningSnapshot, type WeightProfile, type TasteTuning, + type DiscoverTuning, type TrendsResponse, type TrendSeries, type TrendMarker @@ -43,6 +44,11 @@ { key: 'mood_scale', label: 'Mood weight', hint: 'How strongly a mood-tagged play imprints on the mood facet (from folksonomy tags), in [0, 1]. 0 = mood ignored.' } ]; + const discoverFields: { key: keyof DiscoverTuning; label: string; hint: string }[] = [ + { key: 'tag_overlap_weight', label: 'Taste-tag weight', hint: "How strongly a candidate's tags matching your taste boosts it. score x (1 + w x overlap), so 0 turns the tag term off and ranks on similarity alone. An artist with no cached tags is never penalised." }, + { key: 'snooze_days', label: 'Snooze length (days)', hint: 'How long "not right now" parks a suggestion before it returns on its own. Records no opinion about the artist and never feeds the taste profile.' } + ]; + const profileScopes: { scope: 'radio' | 'daily_mix'; label: string; blurb: string }[] = [ { scope: 'radio', label: 'Radio', blurb: 'Seed-directed listening — the user picked a direction.' }, { scope: 'daily_mix', label: 'Daily mixes', blurb: 'For You, Songs like…, and the discovery mixes.' } @@ -55,11 +61,17 @@ let saving = $state(null); function fillForm(snap: TuningSnapshot) { - const f: Record> = { radio: {}, daily_mix: {}, taste: {} }; + const f: Record> = { + radio: {}, + daily_mix: {}, + taste: {}, + discover: {} + }; for (const p of ['radio', 'daily_mix'] as const) { for (const { key } of weightFields) f[p][key] = String(snap.profiles[p][key]); } for (const { key } of tasteFields) f.taste[key] = String(snap.taste[key]); + for (const { key } of discoverFields) f.discover[key] = String(snap.discover[key]); form = f; } @@ -76,11 +88,17 @@ // A knob deviates when its CURRENT SAVED value differs from shipped; // the dot marks where this install has drifted from defaults. - function deviates(scope: 'radio' | 'daily_mix' | 'taste', key: string): boolean { + function deviates(scope: TuningScope, key: string): boolean { if (!snapshot) return false; if (scope === 'taste') { return snapshot.taste[key as keyof TasteTuning] !== snapshot.shipped.taste[key as keyof TasteTuning]; } + if (scope === 'discover') { + return ( + snapshot.discover[key as keyof DiscoverTuning] !== + snapshot.shipped.discover[key as keyof DiscoverTuning] + ); + } return ( snapshot.profiles[scope][key as keyof WeightProfile] !== snapshot.shipped.profiles[scope][key as keyof WeightProfile] @@ -90,6 +108,7 @@ function currentValue(scope: TuningScope, key: string): number { if (!snapshot) return 0; if (scope === 'taste') return snapshot.taste[key as keyof TasteTuning]; + if (scope === 'discover') return snapshot.discover[key as keyof DiscoverTuning]; return snapshot.profiles[scope][key as keyof WeightProfile]; } @@ -276,6 +295,7 @@ type="button" disabled={saving !== null} onclick={() => reset(p.scope)} + aria-label="Reset {p.label} to defaults" class="rounded border border-border px-3 py-1.5 text-sm text-text-secondary hover:text-text-primary disabled:opacity-50" > Reset to defaults @@ -328,6 +348,63 @@ type="button" disabled={saving !== null} onclick={() => reset('taste')} + aria-label="Reset taste profile build to defaults" + class="rounded border border-border px-3 py-1.5 text-sm text-text-secondary hover:text-text-primary disabled:opacity-50" + > + Reset to defaults + + + + + +
+
+

Discover requests

+

+ How the Discover suggestion deck ranks out-of-library artists. Tag coverage for + artists you don't own is partial by nature, so an untagged candidate keeps its + similarity score rather than being pushed down. +

+
+
+ {#each discoverFields as f (f.key)} +
+ + +
+ {/each} +
+
+ +