diff --git a/internal/api/me_recommendation_metrics.go b/internal/api/me_recommendation_metrics.go index 20b8604d..75761b05 100644 --- a/internal/api/me_recommendation_metrics.go +++ b/internal/api/me_recommendation_metrics.go @@ -39,10 +39,12 @@ type surfaceMetric struct { SkipRate float64 `json:"skip_rate"` // skips / plays, [0,1] AvgCompletion float64 `json:"avg_completion"` // mean completion ratio, [0,1] LowConfidence bool `json:"low_confidence"` // plays < recMetricsLowVolume - // Breakdown splits the family into sub-populations. Only For You - // carries one (#1249): taste picks vs fresh picks (the deliberate - // freshness injection), plus earlier plays that predate attribution. - // The parent row remains the sum of its breakdown. + // Breakdown splits the family into the pick-kind populations its + // builder stamped (#1249, generalized #1270): For You's taste/fresh, + // Discover's buckets, tier1-3 for tiered mixes — plus earlier plays + // that predate attribution. Present only when the family has at + // least one attributed play; the parent row remains the sum of its + // breakdown. Breakdown []surfaceMetric `json:"breakdown,omitempty"` } @@ -168,22 +170,50 @@ func (h *handlers) handleGetRecommendationMetrics(w http.ResponseWriter, r *http writeJSON(w, http.StatusOK, bucketMetricsResponse(days, rows)) } -// forYouPickFamilies defines the For You breakdown rows (#1249), keyed -// by play_events.pick_kind. "" (NULL pick_kind) = plays recorded before -// attribution shipped, or whose track had already rotated out of the -// snapshot at ingestion — kept visible so the parent row's sums stay -// transparent instead of silently shrinking. -var forYouPickFamilies = map[string]recFamily{ - "taste": {"for_you_taste", "Taste picks", intentGoTo}, - "fresh": {"for_you_fresh", "Fresh picks", intentGoTo}, - "": {"for_you_unattributed", "Earlier plays", intentGoTo}, +// pickKindLabels is the display vocabulary for play_events.pick_kind +// values (mirrors the CHECK in migration 0039). Every system mix that +// stamps provenance gets its breakdown from this one map — adding a +// stamping mix needs no metrics change. +var pickKindLabels = map[string]string{ + "taste": "Taste picks", + "fresh": "Fresh picks", + "dormant": "Dormant artists", + "cross_user": "Liked by others", + "random": "Random unheard", + "tier1": "Tier 1 (exact)", + "tier2": "Tier 2 (relaxed)", + "tier3": "Tier 3 (stretched)", } -// forYouBreakdown folds pick-kind accums into the parent metric's -// Breakdown. Attached only when at least one attributed (taste/fresh) -// play exists — an all-unattributed breakdown would just repeat the -// parent row. -func forYouBreakdown(picks map[string]*familyAccum) []surfaceMetric { +// pickKindOrder fixes breakdown row order; unattributed ("", i.e. NULL +// pick_kind — plays recorded before the mix stamped provenance, or +// whose track had rotated out of the snapshot at ingestion) renders +// last, kept visible so the parent row's sums stay transparent instead +// of silently shrinking. The DB CHECK gates pick_kind to exactly this +// vocabulary, so iterating the list is exhaustive. +var pickKindOrder = []string{ + "taste", "fresh", "dormant", "cross_user", "random", + "tier1", "tier2", "tier3", "", +} + +// pickKindFamily derives the sub-family for one (family, pick_kind) +// population, e.g. ("for_you", "taste") → for_you_taste "Taste picks". +func pickKindFamily(parent recFamily, kind string) recFamily { + if kind == "" { + return recFamily{parent.key + "_unattributed", "Earlier plays", parent.intent} + } + label, ok := pickKindLabels[kind] + if !ok { + label = kind + } + return recFamily{parent.key + "_" + kind, label, parent.intent} +} + +// pickKindBreakdown folds a family's per-pick-kind accums into its +// Breakdown rows. Attached only when at least one attributed play +// exists — an all-unattributed breakdown would just repeat the parent +// row, and families that never stamp (radio, direct plays) stay flat. +func pickKindBreakdown(picks map[string]*familyAccum) []surfaceMetric { attributed := int64(0) for kind, acc := range picks { if kind != "" { @@ -194,7 +224,7 @@ func forYouBreakdown(picks map[string]*familyAccum) []surfaceMetric { return nil } out := make([]surfaceMetric, 0, len(picks)) - for _, kind := range []string{"taste", "fresh", ""} { + for _, kind := range pickKindOrder { if acc, ok := picks[kind]; ok && acc.plays > 0 { out = append(out, acc.metric()) } @@ -210,7 +240,7 @@ func bucketMetricsResponse( ) recommendationMetricsResp { baseline := &familyAccum{fam: recFamily{"manual", "Manual library plays", ""}} families := map[string]*familyAccum{} - forYouPicks := map[string]*familyAccum{} + picks := map[string]map[string]*familyAccum{} for _, row := range rows { if row.Source == nil || *row.Source == "" { baseline.add(row) @@ -223,18 +253,23 @@ func bucketMetricsResponse( families[fam.key] = acc } acc.add(row) - if fam.key == "for_you" { - kind := "" - if row.PickKind != nil { - kind = *row.PickKind - } - pick, ok := forYouPicks[kind] - if !ok { - pick = &familyAccum{fam: forYouPickFamilies[kind]} - forYouPicks[kind] = pick - } - pick.add(row) + // Accumulate the pick-kind population unconditionally; families + // that never stamp end up all-unattributed and get no breakdown. + kind := "" + if row.PickKind != nil { + kind = *row.PickKind } + byKind, ok := picks[fam.key] + if !ok { + byKind = map[string]*familyAccum{} + picks[fam.key] = byKind + } + pick, ok := byKind[kind] + if !ok { + pick = &familyAccum{fam: pickKindFamily(fam, kind)} + byKind[kind] = pick + } + pick.add(row) } resp := recommendationMetricsResp{WindowDays: days, Groups: []surfaceGroup{}} @@ -251,9 +286,7 @@ func bucketMetricsResponse( for _, acc := range families { if acc.fam.intent == g.intent { m := acc.metric() - if acc.fam.key == "for_you" { - m.Breakdown = forYouBreakdown(forYouPicks) - } + m.Breakdown = pickKindBreakdown(picks[acc.fam.key]) group.Surfaces = append(group.Surfaces, m) } } diff --git a/internal/api/me_recommendation_metrics_test.go b/internal/api/me_recommendation_metrics_test.go index c17b8761..de717b79 100644 --- a/internal/api/me_recommendation_metrics_test.go +++ b/internal/api/me_recommendation_metrics_test.go @@ -168,6 +168,48 @@ func TestBucketMetricsResponse_ForYouBreakdown(t *testing.T) { } } +func TestBucketMetricsResponse_DiscoverBucketBreakdown(t *testing.T) { + // Provenance is standard (#1270): Discover's bucket stamps surface as + // a breakdown exactly like For You's taste/fresh — this is what makes + // the 40/30/30 allocation judgeable instead of a guess. + src := func(s string) *string { return &s } + kind := func(s string) *string { return &s } + rows := []dbq.RecommendationSourceMetricsForUserRow{ + {Source: src("discover"), PickKind: kind("dormant"), Plays: 8, Skips: 2, + CompletionN: 8, AvgCompletion: 0.8}, + {Source: src("discover"), PickKind: kind("cross_user"), Plays: 6, Skips: 3, + CompletionN: 6, AvgCompletion: 0.6}, + {Source: src("discover"), PickKind: kind("random"), Plays: 4, Skips: 3, + CompletionN: 4, AvgCompletion: 0.4}, + // NULL pick_kind = plays that predate bucket stamping. + {Source: src("discover"), Plays: 2, Skips: 0, CompletionN: 2, AvgCompletion: 0.9}, + } + resp := bucketMetricsResponse(recMetricsDefaultDays, rows) + + d := findSurface(resp, "discover") + if d == nil { + t.Fatal("discover family missing") + } + if d.Plays != 20 || d.Skips != 8 { + t.Errorf("discover plays/skips = %d/%d, want 20/8", d.Plays, d.Skips) + } + wantKeys := []string{ + "discover_dormant", "discover_cross_user", "discover_random", + "discover_unattributed", + } + if len(d.Breakdown) != len(wantKeys) { + t.Fatalf("breakdown rows = %d, want %d", len(d.Breakdown), len(wantKeys)) + } + for i, k := range wantKeys { + if d.Breakdown[i].Key != k { + t.Errorf("breakdown[%d].key = %s, want %s", i, d.Breakdown[i].Key, k) + } + } + if b := d.Breakdown[0]; b.Label != "Dormant artists" || b.Plays != 8 { + t.Errorf("dormant row = %+v, want label=Dormant artists plays=8", b) + } +} + func TestRecommendationMetrics_BucketsWithBaseline(t *testing.T) { if os.Getenv("MINSTREL_TEST_DATABASE_URL") == "" { t.Skip("MINSTREL_TEST_DATABASE_URL not set") diff --git a/internal/db/dbq/events.sql.go b/internal/db/dbq/events.sql.go index 27b94317..6db1800c 100644 --- a/internal/db/dbq/events.sql.go +++ b/internal/db/dbq/events.sql.go @@ -31,33 +31,6 @@ func (q *Queries) GetCurrentSessionVectorForUser(ctx context.Context, userID pgt return session_vector_at_play, err } -const getForYouPickKindForTrack = `-- name: GetForYouPickKindForTrack :one -SELECT pt.pick_kind -FROM playlist_tracks pt -JOIN playlists p ON p.id = pt.playlist_id -WHERE p.user_id = $1 - AND p.system_variant = 'for_you' - AND pt.track_id = $2 -LIMIT 1 -` - -type GetForYouPickKindForTrackParams struct { - UserID pgtype.UUID - TrackID pgtype.UUID -} - -// Looks a track up in the user's CURRENT For You snapshot and returns -// its pick_kind ('taste'/'fresh'). Used at play-ingestion time to -// freeze exploration attribution onto the play_event — the snapshot is -// rebuilt daily, so attribution can't be reconstructed at read time -// (#1249). No row = track not in today's snapshot (caller stores NULL). -func (q *Queries) GetForYouPickKindForTrack(ctx context.Context, arg GetForYouPickKindForTrackParams) (*string, error) { - row := q.db.QueryRow(ctx, getForYouPickKindForTrack, arg.UserID, arg.TrackID) - var pick_kind *string - err := row.Scan(&pick_kind) - return pick_kind, err -} - const getMostRecentPlaySessionForUser = `-- name: GetMostRecentPlaySessionForUser :one SELECT id, user_id, started_at, ended_at, last_event_at, track_count, client_id FROM play_sessions WHERE user_id = $1 @@ -137,6 +110,38 @@ func (q *Queries) GetPlayEventByID(ctx context.Context, id pgtype.UUID) (PlayEve return i, err } +const getSystemPickKindForTrack = `-- name: GetSystemPickKindForTrack :one +SELECT pt.pick_kind +FROM playlist_tracks pt +JOIN playlists p ON p.id = pt.playlist_id +WHERE p.user_id = $1 + AND p.system_variant = $2 + AND pt.track_id = $3 +LIMIT 1 +` + +type GetSystemPickKindForTrackParams struct { + UserID pgtype.UUID + SystemVariant *string + TrackID pgtype.UUID +} + +// Looks a track up in the user's CURRENT snapshot of the given system +// variant and returns its pick_kind. Used at play-ingestion time to +// freeze provenance onto the play_event — snapshots rebuild daily, so +// attribution can't be reconstructed at read time (#1249/#1270). No +// row = track not in today's snapshot (caller stores NULL); a row with +// NULL pick_kind = the variant doesn't stamp (yet). songs_like_artist +// is non-singleton (up to 3 mixes/user); a track in two of them takes +// whichever LIMIT 1 hits — acceptable, tier stamps there describe the +// same eligibility ladder. +func (q *Queries) GetSystemPickKindForTrack(ctx context.Context, arg GetSystemPickKindForTrackParams) (*string, error) { + row := q.db.QueryRow(ctx, getSystemPickKindForTrack, arg.UserID, arg.SystemVariant, arg.TrackID) + var pick_kind *string + err := row.Scan(&pick_kind) + return pick_kind, err +} + const insertPlayEvent = `-- name: InsertPlayEvent :one INSERT INTO play_events ( user_id, track_id, session_id, started_at, client_id, source, pick_kind @@ -154,8 +159,9 @@ type InsertPlayEventParams struct { PickKind *string } -// pick_kind is non-NULL only for source='for_you' plays whose track was -// found in the user's live For You snapshot at ingestion time (#1249). +// pick_kind is non-NULL only for system-playlist plays whose track was +// found (with a stamped kind) in the user's live snapshot for that +// variant at ingestion time (#1249, generalized in #1270). func (q *Queries) InsertPlayEvent(ctx context.Context, arg InsertPlayEventParams) (PlayEvent, error) { row := q.db.QueryRow(ctx, insertPlayEvent, arg.UserID, diff --git a/internal/db/migrations/0039_pick_kind_vocabulary.down.sql b/internal/db/migrations/0039_pick_kind_vocabulary.down.sql new file mode 100644 index 00000000..eb447d38 --- /dev/null +++ b/internal/db/migrations/0039_pick_kind_vocabulary.down.sql @@ -0,0 +1,19 @@ +-- Rows stamped with the widened vocabulary must be cleared before the +-- narrow CHECK can be re-added; NULL reads as "unattributed", which is +-- the honest downgrade. +UPDATE play_events SET pick_kind = NULL + WHERE pick_kind NOT IN ('taste', 'fresh'); +UPDATE playlist_tracks SET pick_kind = NULL + WHERE pick_kind NOT IN ('taste', 'fresh'); + +ALTER TABLE play_events + DROP CONSTRAINT play_events_pick_kind_check; +ALTER TABLE play_events + ADD CONSTRAINT play_events_pick_kind_check + CHECK (pick_kind IN ('taste', 'fresh')); + +ALTER TABLE playlist_tracks + DROP CONSTRAINT playlist_tracks_pick_kind_check; +ALTER TABLE playlist_tracks + ADD CONSTRAINT playlist_tracks_pick_kind_check + CHECK (pick_kind IN ('taste', 'fresh')); diff --git a/internal/db/migrations/0039_pick_kind_vocabulary.up.sql b/internal/db/migrations/0039_pick_kind_vocabulary.up.sql new file mode 100644 index 00000000..36f619d2 --- /dev/null +++ b/internal/db/migrations/0039_pick_kind_vocabulary.up.sql @@ -0,0 +1,41 @@ +-- Provenance as standard (milestone #127, Scribe #1270). +-- +-- #1249 introduced pick_kind as a For You one-off ('taste'/'fresh'). +-- The mechanism — stamp WHY a track is in the snapshot at build time, +-- freeze it onto the play at ingestion, break it down in metrics — is +-- now the standard for every system mix, so the CHECK vocabulary +-- widens to cover: +-- +-- taste / fresh For You head vs freshness-injection tail +-- dormant / cross_user / random +-- Discover's three candidate buckets — makes +-- the 40/30/30 allocation measurable instead +-- of a guess +-- tier1 / tier2 / tier3 the tiered-eligibility ladder (project rule +-- #131): tier 1 pins the mix's exact desire, +-- each higher tier steps back a little. Tier +-- provenance is how we measure what the +-- step-back trades away. +-- +-- Postgres CHECK whitelists can't be altered in place: DROP + re-ADD +-- with the expanded list, in the same migration. + +ALTER TABLE playlist_tracks + DROP CONSTRAINT playlist_tracks_pick_kind_check; +ALTER TABLE playlist_tracks + ADD CONSTRAINT playlist_tracks_pick_kind_check + CHECK (pick_kind IN ( + 'taste', 'fresh', + 'dormant', 'cross_user', 'random', + 'tier1', 'tier2', 'tier3' + )); + +ALTER TABLE play_events + DROP CONSTRAINT play_events_pick_kind_check; +ALTER TABLE play_events + ADD CONSTRAINT play_events_pick_kind_check + CHECK (pick_kind IN ( + 'taste', 'fresh', + 'dormant', 'cross_user', 'random', + 'tier1', 'tier2', 'tier3' + )); diff --git a/internal/db/queries/events.sql b/internal/db/queries/events.sql index c0c0a059..565b510c 100644 --- a/internal/db/queries/events.sql +++ b/internal/db/queries/events.sql @@ -24,25 +24,30 @@ ORDER BY started_at DESC LIMIT 1; -- name: InsertPlayEvent :one --- pick_kind is non-NULL only for source='for_you' plays whose track was --- found in the user's live For You snapshot at ingestion time (#1249). +-- pick_kind is non-NULL only for system-playlist plays whose track was +-- found (with a stamped kind) in the user's live snapshot for that +-- variant at ingestion time (#1249, generalized in #1270). INSERT INTO play_events ( user_id, track_id, session_id, started_at, client_id, source, pick_kind ) VALUES ($1, $2, $3, $4, $5, $6, sqlc.narg(pick_kind)::text) RETURNING *; --- name: GetForYouPickKindForTrack :one --- Looks a track up in the user's CURRENT For You snapshot and returns --- its pick_kind ('taste'/'fresh'). Used at play-ingestion time to --- freeze exploration attribution onto the play_event — the snapshot is --- rebuilt daily, so attribution can't be reconstructed at read time --- (#1249). No row = track not in today's snapshot (caller stores NULL). +-- name: GetSystemPickKindForTrack :one +-- Looks a track up in the user's CURRENT snapshot of the given system +-- variant and returns its pick_kind. Used at play-ingestion time to +-- freeze provenance onto the play_event — snapshots rebuild daily, so +-- attribution can't be reconstructed at read time (#1249/#1270). No +-- row = track not in today's snapshot (caller stores NULL); a row with +-- NULL pick_kind = the variant doesn't stamp (yet). songs_like_artist +-- is non-singleton (up to 3 mixes/user); a track in two of them takes +-- whichever LIMIT 1 hits — acceptable, tier stamps there describe the +-- same eligibility ladder. SELECT pt.pick_kind FROM playlist_tracks pt JOIN playlists p ON p.id = pt.playlist_id WHERE p.user_id = $1 - AND p.system_variant = 'for_you' - AND pt.track_id = $2 + AND p.system_variant = $2 + AND pt.track_id = $3 LIMIT 1; -- name: UpdatePlayEventEnded :one diff --git a/internal/playevents/writer.go b/internal/playevents/writer.go index 48a2d82d..22999945 100644 --- a/internal/playevents/writer.go +++ b/internal/playevents/writer.go @@ -97,25 +97,26 @@ var systemPlaylistSources = map[string]bool{ "songs_like_artist": true, } -// sourceForYou is the one source whose plays carry exploration -// attribution (taste vs fresh pick, #1249). -const sourceForYou = "for_you" - -// lookupForYouPickKind resolves a For-You play's pick_kind ('taste' or -// 'fresh') from the user's live snapshot at ingestion time. Attribution -// must be frozen now — the snapshot rebuilds daily, so it can't be -// reconstructed at metrics-read time. Returns nil (unattributed) when -// the track isn't in the current snapshot: plays predating the feature, -// or an offline replay arriving after the track rotated out. Real DB +// lookupSystemPickKind resolves a system-playlist play's pick_kind +// (taste/fresh, Discover's bucket, or a rule-#131 tier) from the user's +// live snapshot of that variant at ingestion time. Attribution must be +// frozen now — snapshots rebuild daily, so it can't be reconstructed at +// metrics-read time (#1249, generalized in #1270). Source strings +// double as system_variant values (see systemPlaylistSources). Returns +// nil (unattributed) when the track isn't in the current snapshot — +// plays predating the feature, or an offline replay arriving after the +// track rotated out — and when the variant doesn't stamp yet. Real DB // errors propagate and fail the caller's transaction. -func lookupForYouPickKind( +func lookupSystemPickKind( ctx context.Context, q *dbq.Queries, userID, trackID pgtype.UUID, + variant string, ) (*string, error) { - kind, err := q.GetForYouPickKindForTrack(ctx, dbq.GetForYouPickKindForTrackParams{ - UserID: userID, - TrackID: trackID, + kind, err := q.GetSystemPickKindForTrack(ctx, dbq.GetSystemPickKindForTrackParams{ + UserID: userID, + SystemVariant: &variant, + TrackID: trackID, }) if errors.Is(err, pgx.ErrNoRows) { return nil, nil @@ -157,8 +158,8 @@ func (w *Writer) RecordPlayStartedWithSource( sourcePtr = &source } var pickKind *string - if source == sourceForYou { - pickKind, err = lookupForYouPickKind(ctx, q, userID, trackID) + if systemPlaylistSources[source] { + pickKind, err = lookupSystemPickKind(ctx, q, userID, trackID, source) if err != nil { return err } @@ -433,8 +434,8 @@ func (w *Writer) RecordOfflinePlay( sourcePtr = &source } var pickKind *string - if source == sourceForYou { - pickKind, err = lookupForYouPickKind(ctx, q, userID, trackID) + if systemPlaylistSources[source] { + pickKind, err = lookupSystemPickKind(ctx, q, userID, trackID, source) if err != nil { return err } diff --git a/internal/playevents/writer_test.go b/internal/playevents/writer_test.go index d707f7dc..b6dc1bd6 100644 --- a/internal/playevents/writer_test.go +++ b/internal/playevents/writer_test.go @@ -489,14 +489,13 @@ func TestRecordPlayStarted_NoRotationWithoutSource(t *testing.T) { } } -// seedForYouSnapshot creates a system For You playlist containing the -// fixture track with the given pick_kind, mimicking what the builder -// persists (#1249). -func seedForYouSnapshot(t *testing.T, f fixture, pickKind string) { +// seedSystemSnapshot creates a system playlist of the given variant +// containing the fixture track with the given pick_kind, mimicking +// what the builder persists (#1249, generalized #1270). +func seedSystemSnapshot(t *testing.T, f fixture, variant, pickKind string) { t.Helper() - variant := "for_you" pl, err := f.q.CreateSystemPlaylist(context.Background(), dbq.CreateSystemPlaylistParams{ - UserID: f.user, Name: "For You", SystemVariant: &variant, + UserID: f.user, Name: variant, SystemVariant: &variant, }) if err != nil { t.Fatalf("CreateSystemPlaylist: %v", err) @@ -510,7 +509,7 @@ func seedForYouSnapshot(t *testing.T, f fixture, pickKind string) { func TestRecordPlayStartedWithSource_StampsForYouPickKind(t *testing.T) { f := newFixture(t, 200_000) - seedForYouSnapshot(t, f, "fresh") + seedSystemSnapshot(t, f, "for_you", "fresh") res, err := f.w.RecordPlayStartedWithSource( context.Background(), f.user, f.track, "c", "for_you", time.Now().UTC(), ) @@ -545,11 +544,13 @@ func TestRecordPlayStartedWithSource_NoSnapshotMatch_PickKindNull(t *testing.T) } } -func TestRecordPlayStartedWithSource_NonForYouSource_PickKindNull(t *testing.T) { - // Even with a For You snapshot containing the track, a play from a - // different surface must not pick up For You attribution. +func TestRecordPlayStartedWithSource_CrossVariant_PickKindNull(t *testing.T) { + // Attribution is variant-scoped: even with a For You snapshot + // containing the track, a play from a different surface must not + // pick up For You's stamp — it reads its OWN variant's snapshot, + // which here doesn't exist. f := newFixture(t, 200_000) - seedForYouSnapshot(t, f, "taste") + seedSystemSnapshot(t, f, "for_you", "taste") res, err := f.w.RecordPlayStartedWithSource( context.Background(), f.user, f.track, "c", "discover", time.Now().UTC(), ) @@ -561,7 +562,27 @@ func TestRecordPlayStartedWithSource_NonForYouSource_PickKindNull(t *testing.T) t.Fatalf("get: %v", err) } if got.PickKind != nil { - t.Errorf("pick_kind = %q, want NULL for non-for_you source", *got.PickKind) + t.Errorf("pick_kind = %q, want NULL for a cross-variant play", *got.PickKind) + } +} + +func TestRecordPlayStartedWithSource_StampsDiscoverBucket(t *testing.T) { + // Discover stamps its candidate bucket (#1270); a discover play whose + // track is in the live snapshot freezes that bucket onto the play. + f := newFixture(t, 200_000) + seedSystemSnapshot(t, f, "discover", "dormant") + res, err := f.w.RecordPlayStartedWithSource( + context.Background(), f.user, f.track, "c", "discover", time.Now().UTC(), + ) + if err != nil { + t.Fatalf("RecordPlayStartedWithSource: %v", err) + } + got, err := f.q.GetPlayEventByID(context.Background(), res.PlayEventID) + if err != nil { + t.Fatalf("get: %v", err) + } + if got.PickKind == nil || *got.PickKind != "dormant" { + t.Errorf("pick_kind = %v, want dormant (frozen from live snapshot)", got.PickKind) } } @@ -569,7 +590,7 @@ func TestRecordOfflinePlay_StampsForYouPickKind(t *testing.T) { // The offline replay path threads source the same way; a replayed // for_you play whose track is still in the snapshot gets attributed. f := newFixture(t, 200_000) - seedForYouSnapshot(t, f, "taste") + seedSystemSnapshot(t, f, "for_you", "taste") at := time.Now().UTC().Add(-time.Hour) if err := f.w.RecordOfflinePlay( context.Background(), f.user, f.track, "c", "for_you", at, 180_000, diff --git a/internal/playlists/discover.go b/internal/playlists/discover.go index 65f41d48..809e33fb 100644 --- a/internal/playlists/discover.go +++ b/internal/playlists/discover.go @@ -20,11 +20,15 @@ const ( // discoverTrack is the common shape used by the bucket allocator. The // three sqlc-generated row types collapse into this internal struct so -// downstream functions don't need to be generic over them. +// downstream functions don't need to be generic over them. PickKind is +// the originating bucket (dormant/cross_user/random, #1270), stamped in +// the row adapters so it survives the interleave — bucket identity is +// what makes the 40/30/30 allocation measurable in the metrics. type discoverTrack struct { ID pgtype.UUID AlbumID pgtype.UUID ArtistID pgtype.UUID + PickKind string } // buildDiscoverCandidates assembles the Discover playlist track list. @@ -91,7 +95,7 @@ func buildDiscoverCandidates(ctx context.Context, q *dbq.Queries, logger *slog.L // already gave us the ranking. ranked := make([]rankedCandidate, 0, len(out)) for _, t := range out { - ranked = append(ranked, rankedCandidate{TrackID: t.ID}) + ranked = append(ranked, rankedCandidate{TrackID: t.ID, PickKind: t.PickKind}) } return ranked, nil } @@ -99,7 +103,10 @@ func buildDiscoverCandidates(ctx context.Context, q *dbq.Queries, logger *slog.L func dormantRowsToTracks(rows []dbq.ListDormantArtistTracksForDiscoverRow) []discoverTrack { out := make([]discoverTrack, len(rows)) for i, r := range rows { - out[i] = discoverTrack{ID: r.ID, AlbumID: r.AlbumID, ArtistID: r.ArtistID} + out[i] = discoverTrack{ + ID: r.ID, AlbumID: r.AlbumID, ArtistID: r.ArtistID, + PickKind: pickKindDormant, + } } return out } @@ -107,7 +114,10 @@ func dormantRowsToTracks(rows []dbq.ListDormantArtistTracksForDiscoverRow) []dis func crossUserRowsToTracks(rows []dbq.ListCrossUserLikedTracksForDiscoverRow) []discoverTrack { out := make([]discoverTrack, len(rows)) for i, r := range rows { - out[i] = discoverTrack{ID: r.ID, AlbumID: r.AlbumID, ArtistID: r.ArtistID} + out[i] = discoverTrack{ + ID: r.ID, AlbumID: r.AlbumID, ArtistID: r.ArtistID, + PickKind: pickKindCrossUser, + } } return out } @@ -115,7 +125,10 @@ func crossUserRowsToTracks(rows []dbq.ListCrossUserLikedTracksForDiscoverRow) [] func randomRowsToTracks(rows []dbq.ListRandomUnheardTracksForDiscoverRow) []discoverTrack { out := make([]discoverTrack, len(rows)) for i, r := range rows { - out[i] = discoverTrack{ID: r.ID, AlbumID: r.AlbumID, ArtistID: r.ArtistID} + out[i] = discoverTrack{ + ID: r.ID, AlbumID: r.AlbumID, ArtistID: r.ArtistID, + PickKind: pickKindRandom, + } } return out } diff --git a/internal/playlists/discover_test.go b/internal/playlists/discover_test.go index e299cb11..fc1d54a8 100644 --- a/internal/playlists/discover_test.go +++ b/internal/playlists/discover_test.go @@ -161,3 +161,27 @@ func TestInterleaveBuckets_DedupsAcrossBuckets(t *testing.T) { } } } + +func TestInterleaveBuckets_PreservesBucketPickKind(t *testing.T) { + // Bucket provenance (#1270) is stamped on discoverTrack before the + // interleave and must survive it — including dedup, where a track in + // two buckets keeps the stamp of the bucket it was taken from. + dormant := []discoverTrack{ + {ID: uuidN(1), PickKind: pickKindDormant}, + {ID: uuidN(2), PickKind: pickKindDormant}, + } + random := []discoverTrack{ + {ID: uuidN(1), PickKind: pickKindRandom}, // shared with dormant + {ID: uuidN(3), PickKind: pickKindRandom}, + } + got := interleaveBuckets(dormant, nil, random) + if len(got) != 3 { + t.Fatalf("len = %d, want 3", len(got)) + } + want := map[byte]string{1: pickKindDormant, 2: pickKindDormant, 3: pickKindRandom} + for _, tr := range got { + if w := want[tr.ID.Bytes[15]]; tr.PickKind != w { + t.Errorf("track %d pick_kind = %q, want %q", tr.ID.Bytes[15], tr.PickKind, w) + } + } +} diff --git a/internal/playlists/system.go b/internal/playlists/system.go index e1527873..969f0584 100644 --- a/internal/playlists/system.go +++ b/internal/playlists/system.go @@ -127,20 +127,26 @@ func pickSeedArtistsForDay(pool []pgtype.UUID, userID pgtype.UUID, dateStr strin // rankedCandidate is a (track_id, score) pair used during in-memory // sorting before insert into playlist_tracks. T5 fills these from -// recommendation.Candidate scores. PickKind is set only by For-You's -// head/tail composition (#1249); empty persists as NULL. +// recommendation.Candidate scores. PickKind is the track's provenance +// within its mix — For-You's head/tail split (#1249) and Discover's +// buckets (#1270); empty persists as NULL (variant doesn't stamp yet). type rankedCandidate struct { TrackID pgtype.UUID Score float64 PickKind string } -// For-You pick kinds, persisted on playlist_tracks.pick_kind so plays -// can attribute skips to "the taste engine missing" vs "the freshness -// tax" (#1249). Values mirror the CHECK in migration 0038. +// Pick kinds, persisted on playlist_tracks.pick_kind so plays can +// attribute skips to the population that sourced the track — For You's +// "taste engine missing" vs "freshness tax", Discover's three buckets +// (#1270). Values mirror the CHECK in migration 0039; the tier1-3 +// values there land with the tiered-mix rebuilds. const ( - pickKindTaste = "taste" - pickKindFresh = "fresh" + pickKindTaste = "taste" + pickKindFresh = "fresh" + pickKindDormant = "dormant" + pickKindCrossUser = "cross_user" + pickKindRandom = "random" ) const systemMixLength = 25 diff --git a/web/src/lib/api/metrics.ts b/web/src/lib/api/metrics.ts index 01175ffd..3f467d30 100644 --- a/web/src/lib/api/metrics.ts +++ b/web/src/lib/api/metrics.ts @@ -12,8 +12,10 @@ export type SurfaceMetric = { skip_rate: number; avg_completion: number; low_confidence: boolean; - // Only For You carries a breakdown (#1249): taste picks vs fresh picks - // (the deliberate freshness injection), plus pre-attribution plays. + // Present when the surface's builder stamps pick-kind provenance and + // the window holds attributed plays (#1249, generalized #1270): For + // You's taste/fresh split, Discover's candidate buckets, the tiered + // mixes' tier1-3 — plus pre-attribution plays. breakdown?: SurfaceMetric[]; }; diff --git a/web/src/routes/settings/+page.svelte b/web/src/routes/settings/+page.svelte index 711ed1f9..3c6c0d23 100644 --- a/web/src/routes/settings/+page.svelte +++ b/web/src/routes/settings/+page.svelte @@ -59,6 +59,18 @@ function completionDeltaClass(m: SurfaceMetric, baseline: SurfaceMetric): string { return m.avg_completion < baseline.avg_completion ? 'text-danger' : 'text-text-secondary'; } + + // Pick-kind breakdowns are collapsed by default (#1270): with every + // system mix stamping provenance, always-open sub-rows would triple + // the table height and bury the surface-level comparison. + let expandedBreakdowns = $state(new Set()); + + function toggleBreakdown(key: string) { + const next = new Set(expandedBreakdowns); + if (next.has(key)) next.delete(key); + else next.add(key); + expandedBreakdowns = next; + } const tokenMutation = createTokenMutation(queryClient); const enabledMutation = createEnabledMutation(queryClient); @@ -352,7 +364,21 @@ : undefined} > - {m.label}{#if m.low_confidence}· low data{/if} + {#if (m.breakdown ?? []).length > 0} + + {:else} + {m.label} + {/if}{#if m.low_confidence}· low data{/if} {m.plays} @@ -372,40 +398,44 @@ {/if} - - {#each m.breakdown ?? [] as b (b.key)} - - - ↳ {b.label}{#if b.low_confidence}· low data{/if} - - {b.plays} - - {pct(b.skip_rate)} - {#if baseline} - - {deltaPts(b.skip_rate, baseline.skip_rate)} - - {/if} - - - {pct(b.avg_completion)} - {#if baseline} - - {deltaPts(b.avg_completion, baseline.avg_completion)} - - {/if} - - - {/each} + + {#if expandedBreakdowns.has(m.key)} + {#each m.breakdown ?? [] as b (b.key)} + + + ↳ {b.label}{#if b.low_confidence}· low data{/if} + + {b.plays} + + {pct(b.skip_rate)} + {#if baseline} + + {deltaPts(b.skip_rate, baseline.skip_rate)} + + {/if} + + + {pct(b.avg_completion)} + {#if baseline} + + {deltaPts(b.avg_completion, baseline.avg_completion)} + + {/if} + + + {/each} + {/if} {/each} diff --git a/web/src/routes/settings/settings.test.ts b/web/src/routes/settings/settings.test.ts index 0ea50d1d..fcd21fe5 100644 --- a/web/src/routes/settings/settings.test.ts +++ b/web/src/routes/settings/settings.test.ts @@ -198,7 +198,7 @@ describe('Settings page — Recommendation metrics card', () => { ...over }); - test('renders the For You taste/fresh breakdown rows (#1249)', async () => { + test('breakdown rows are collapsed by default and expand on toggle (#1249/#1270)', async () => { setupPage(); metricsMock.data = { window_days: 30, @@ -225,13 +225,23 @@ describe('Settings page — Recommendation metrics card', () => { ] }; render(SettingsPage); - await waitFor(() => expect(screen.getByText('For You')).toBeInTheDocument()); + const toggle = await screen.findByRole('button', { name: /for you/i }); + // Collapsed by default: every stamping mix now carries a breakdown, + // so always-open sub-rows would swamp the surface-level view. + expect(toggle).toHaveAttribute('aria-expanded', 'false'); + expect(screen.queryByText(/Taste picks/)).not.toBeInTheDocument(); + + await fireEvent.click(toggle); + expect(toggle).toHaveAttribute('aria-expanded', 'true'); expect(screen.getByText(/Taste picks/)).toBeInTheDocument(); expect(screen.getByText(/Fresh picks/)).toBeInTheDocument(); expect(screen.getByText(/Earlier plays/)).toBeInTheDocument(); + + await fireEvent.click(toggle); + expect(screen.queryByText(/Taste picks/)).not.toBeInTheDocument(); }); - test('surfaces without a breakdown render no sub-rows', async () => { + test('surfaces without a breakdown render no toggle and no sub-rows', async () => { setupPage(); metricsMock.data = { window_days: 30, @@ -246,6 +256,7 @@ describe('Settings page — Recommendation metrics card', () => { }; render(SettingsPage); await waitFor(() => expect(screen.getByText('Radio')).toBeInTheDocument()); + expect(screen.queryByRole('button', { name: /radio/i })).not.toBeInTheDocument(); expect(screen.queryByText(/Taste picks/)).not.toBeInTheDocument(); }); });