From 481f90605955b882b5318ec14797760ea0334343 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 6 Aug 2026 21:08:58 -0400 Subject: [PATCH 1/4] =?UTF-8?q?feat(metrics):=20publish=20margin=20of=20er?= =?UTF-8?q?ror=20on=20every=20delta=20=E2=80=94=20#2495,=20#2524?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The metrics card had one volume threshold doing two jobs. recMetricsLowVolume = 20 is a DISPLAY floor — below that a skip rate is anecdote — but the card then presented deltas as though it were also a DECISION floor. Those differ by an order of magnitude: detecting the ~13pp differences that matter needs ~133 plays per arm for 80% power at a=0.05. So Discover's taste-matched (59 plays) and random-unheard (70) both rendered as full-confidence rows with a bold delta beside them, and that comparison sits at p ~ 0.06. The card said "signal"; the arithmetic said "maybe". It produced a recommendation the data didn't support, and any reader with the same numbers would have made the same call. Deltas now carry a 95% margin of error and a `distinguishable` flag, computed server-side so both clients read the same arithmetic instead of each re-deriving it. Skip rate is a two-proportion difference; completion is Welch, which needs a variance — hence completion_sqsum in the query. It is the sum of squares rather than stddev_samp on purpose: raw source rows are merged into surface families in Go, and sums of squares combine across groups exactly whereas standard deviations cannot. recMetricsLowVolume is untouched. "Too thin to show" and "too thin to act on" are different questions. Web renders an indistinguishable delta as dimmed and prefixed "≈", with the range on hover and a legend explaining the glyph. Colour is withheld unless the delta clears its margin — colouring noise red is what made the old card misleading. Breakdown rows go through the same path; those are the thinnest samples on screen and where the old card misled most. Also fixes the admin trends view, which had the same problem worse: its "Latest skip"/"Latest completion" columns are one WEEK while the adjacent Plays column is the whole window. I misread exactly that and briefly concluded Deep cuts was the worst surface, from ~17 plays in a single week — over 180 days it is one of the best. Headers now name their period and the skip cell carries that week's play count. #2524: resolveArtist now recognises a duplicate-MBID unique violation as the expected condition it is, matching resolveAlbum. Two rows mapping to one MusicBrainz artist is a merge candidate, not a fault; without the branch it logged a generic warning plus a Postgres ERROR line on every scan, which teaches an operator to ignore database errors. --- internal/api/me_recommendation_metrics.go | 49 +++++- internal/api/metrics_significance.go | 117 ++++++++++++++ internal/api/metrics_significance_test.go | 152 ++++++++++++++++++ internal/db/dbq/recommendation_metrics.sql.go | 24 ++- .../db/queries/recommendation_metrics.sql | 10 +- internal/library/scanner.go | 17 +- web/src/lib/api/metrics.ts | 18 +++ web/src/routes/admin/tuning/+page.svelte | 29 +++- web/src/routes/admin/tuning/tuning.test.ts | 17 +- web/src/routes/settings/+page.svelte | 75 +++++---- web/src/routes/settings/settings.test.ts | 65 ++++++++ 11 files changed, 523 insertions(+), 50 deletions(-) create mode 100644 internal/api/metrics_significance.go create mode 100644 internal/api/metrics_significance_test.go diff --git a/internal/api/me_recommendation_metrics.go b/internal/api/me_recommendation_metrics.go index 7aa02bb1..7716d9f3 100644 --- a/internal/api/me_recommendation_metrics.go +++ b/internal/api/me_recommendation_metrics.go @@ -39,6 +39,14 @@ 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 + // SkipDelta / CompletionDelta are this row's difference from the manual + // baseline WITH its margin of error (#2495). nil on the baseline row + // itself, and whenever the samples are too thin for a margin to mean + // anything. Computed server-side so both clients read the same arithmetic + // instead of each re-deriving it — and so `low_confidence` is no longer + // mistaken for a decision threshold, which it never was. + SkipDelta *metricDelta `json:"skip_delta,omitempty"` + CompletionDelta *metricDelta `json:"completion_delta,omitempty"` // 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 @@ -119,6 +127,10 @@ type familyAccum struct { // completionSum is avg*count re-expanded, so merging N raw rows // reduces to a single weighted division at the end. completionSum float64 + // completionSqSum is the sum of squared completion ratios, which is what + // makes the variance mergeable across raw source rows (#2495). Standard + // deviations cannot be combined; sums of squares add exactly. + completionSqSum float64 } func (a *familyAccum) add(row dbq.RecommendationSourceMetricsForUserRow) { @@ -126,6 +138,7 @@ func (a *familyAccum) add(row dbq.RecommendationSourceMetricsForUserRow) { a.skips += row.Skips a.completionN += row.CompletionN a.completionSum += row.AvgCompletion * float64(row.CompletionN) + a.completionSqSum += row.CompletionSqsum } func (a *familyAccum) metric() surfaceMetric { @@ -145,6 +158,33 @@ func (a *familyAccum) metric() surfaceMetric { return m } +// completionVariance is the sample variance of this family's completion ratios. +func (a *familyAccum) completionVariance() float64 { + return sampleVariance(a.completionSum, a.completionSqSum, a.completionN) +} + +// applyDeltas attaches baseline-relative deltas + margins to a metric. +// Split out so every row — parent surfaces and breakdown rows alike — goes +// through the identical arithmetic; a breakdown arm is exactly where the old +// card was most misleading, because those are the thinnest samples on screen. +func applyDeltas(m *surfaceMetric, acc *familyAccum, baseline *familyAccum) { + if baseline == nil || baseline.plays == 0 { + return + } + m.SkipDelta = proportionDelta( + m.SkipRate, acc.plays, + float64(baseline.skips)/float64(baseline.plays), baseline.plays, + ) + baseMean := 0.0 + if baseline.completionN > 0 { + baseMean = baseline.completionSum / float64(baseline.completionN) + } + m.CompletionDelta = meanDelta( + m.AvgCompletion, acc.completionVariance(), acc.completionN, + baseMean, baseline.completionVariance(), baseline.completionN, + ) +} + // handleGetRecommendationMetrics implements GET /api/me/recommendation-metrics. // Bucketed per-surface-family outcomes for the caller over the last `days` // (default 30, capped at 365), grouped by surface intent and anchored by the @@ -214,7 +254,7 @@ func pickKindFamily(parent recFamily, kind string) recFamily { // 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 { +func pickKindBreakdown(picks map[string]*familyAccum, baseline *familyAccum) []surfaceMetric { attributed := int64(0) for kind, acc := range picks { if kind != "" { @@ -227,7 +267,9 @@ func pickKindBreakdown(picks map[string]*familyAccum) []surfaceMetric { out := make([]surfaceMetric, 0, len(picks)) for _, kind := range pickKindOrder { if acc, ok := picks[kind]; ok && acc.plays > 0 { - out = append(out, acc.metric()) + m := acc.metric() + applyDeltas(&m, acc, baseline) + out = append(out, m) } } return out @@ -287,7 +329,8 @@ func bucketMetricsResponse( for _, acc := range families { if acc.fam.intent == g.intent { m := acc.metric() - m.Breakdown = pickKindBreakdown(picks[acc.fam.key]) + applyDeltas(&m, acc, baseline) + m.Breakdown = pickKindBreakdown(picks[acc.fam.key], baseline) group.Surfaces = append(group.Surfaces, m) } } diff --git a/internal/api/metrics_significance.go b/internal/api/metrics_significance.go new file mode 100644 index 00000000..4bc1d048 --- /dev/null +++ b/internal/api/metrics_significance.go @@ -0,0 +1,117 @@ +package api + +import "math" + +// Uncertainty on the deltas the recommendation-metrics card shows (#2495). +// +// Why this exists: the card had exactly one volume threshold, +// recMetricsLowVolume = 20, and it was doing two jobs. Twenty plays is enough to +// be worth DISPLAYING — below that a skip rate is anecdote — but it is nowhere +// near enough to ACT on. Detecting the ~13pp differences that actually matter +// needs roughly 133 plays per arm for 80% power at α=0.05. +// +// So Discover's taste-matched (59 plays) and random-unheard (70) both rendered as +// full-confidence rows with a bold delta beside them, and that comparison sits at +// p ≈ 0.06. The card said "signal"; the arithmetic said "maybe". It led directly +// to a recommendation the data didn't support, and any reader with the same +// numbers would have made the same call. +// +// The fix is to publish the margin of error next to the delta and flag when the +// delta is smaller than it — i.e. not distinguishable from zero. Computed here, +// server-side, so both clients agree rather than each re-deriving it. +// +// recMetricsLowVolume stays exactly as it was. This is a second, independent +// signal, not a replacement: "too thin to show" and "too thin to act on" are +// different questions and deserve different answers. + +// deltaZ is the two-sided 95% normal critical value. Normal rather than +// Student's t: at the sample sizes where a delta is worth acting on (n in the +// hundreds) the difference is immaterial, and a household dashboard does not +// need a t-table. +const deltaZ = 1.96 + +// metricDelta is a difference from the baseline, with its uncertainty. +// +// Both figures are in PERCENTAGE POINTS, matching how the card reads them out — +// a skip rate of 0.153 against a baseline of 0.270 is "-11.7", not "-0.117". +type metricDelta struct { + // DeltaPP is surface minus baseline. Negative skip is better; negative + // completion is worse. The client owns that colouring. + DeltaPP float64 `json:"delta_pp"` + // MarginPP is the 95% margin of error on DeltaPP. Read the delta as + // DeltaPP ± MarginPP. + MarginPP float64 `json:"margin_pp"` + // Distinguishable reports |DeltaPP| >= MarginPP: the interval excludes + // zero, so the difference is worth reading as a difference. When false the + // number may be pure noise no matter how large it looks. + Distinguishable bool `json:"distinguishable"` +} + +// proportionDelta compares two rates (skips/plays) as a two-proportion +// difference. Returns nil when either sample is empty, or when either rate is +// degenerate (0 or 1) — a rate with no observed variation has an SE of 0 on its +// side, which would report a spuriously narrow margin rather than an honest one. +func proportionDelta(rate1 float64, n1 int64, rate2 float64, n2 int64) *metricDelta { + if n1 <= 0 || n2 <= 0 { + return nil + } + v1 := rate1 * (1 - rate1) / float64(n1) + v2 := rate2 * (1 - rate2) / float64(n2) + se := math.Sqrt(v1 + v2) + if se <= 0 { + // Both rates are 0 or both are 1. The delta is exactly zero and the + // margin is meaningless; reporting nothing is more honest than + // reporting certainty. + return nil + } + return newDelta((rate1-rate2)*100, deltaZ*se*100) +} + +// meanDelta compares two means (average completion ratio) using Welch's +// standard error, which does not assume equal variances between the two groups. +// +// Note the margins here are wider than intuition suggests, and that is correct: +// completion is strongly bimodal — a play is either abandoned early (≈0.05) or +// finished (≈1.0), with little in between — so its standard deviation is large +// (~0.4) even though the mean looks stable. +func meanDelta(mean1 float64, variance1 float64, n1 int64, mean2 float64, variance2 float64, n2 int64) *metricDelta { + // Two observations minimum per side: a sample variance needs n-1 > 0. + if n1 < 2 || n2 < 2 { + return nil + } + se := math.Sqrt(variance1/float64(n1) + variance2/float64(n2)) + if se <= 0 || math.IsNaN(se) || math.IsInf(se, 0) { + return nil + } + return newDelta((mean1-mean2)*100, deltaZ*se*100) +} + +func newDelta(deltaPP, marginPP float64) *metricDelta { + return &metricDelta{ + DeltaPP: deltaPP, + MarginPP: marginPP, + // >= rather than >: a delta exactly equal to its margin sits on the + // boundary, and calling the boundary "distinguishable" is the + // conventional reading of a 95% interval that just excludes zero. + Distinguishable: math.Abs(deltaPP) >= marginPP, + } +} + +// sampleVariance recovers the sample variance from the aggregates the SQL +// returns. sum is mean×n rather than a selected column, which keeps the query to +// one extra expression. +// +// The subtraction can go very slightly negative through floating-point +// cancellation when every observation is identical, so the result is clamped — +// a negative variance would produce NaN downstream. +func sampleVariance(sum, sqSum float64, n int64) float64 { + if n < 2 { + return 0 + } + nf := float64(n) + v := (sqSum - (sum * sum / nf)) / (nf - 1) + if v < 0 { + return 0 + } + return v +} diff --git a/internal/api/metrics_significance_test.go b/internal/api/metrics_significance_test.go new file mode 100644 index 00000000..c8e39ca8 --- /dev/null +++ b/internal/api/metrics_significance_test.go @@ -0,0 +1,152 @@ +package api + +import ( + "math" + "testing" +) + +func TestProportionDelta_ReproducesTheDiscoverCase(t *testing.T) { + // The comparison that motivated #2495: Discover taste-matched (59 plays, + // 15.3% skip) vs random-unheard (70 plays, 28.6%). A 13.3pp gap that the old + // card rendered as a confident coloured number, sitting at p ≈ 0.06. + d := proportionDelta(0.153, 59, 0.286, 70) + if d == nil { + t.Fatal("expected a delta for two real samples") + } + if math.Abs(d.DeltaPP-(-13.3)) > 0.1 { + t.Errorf("DeltaPP = %.2f, want ≈ -13.3", d.DeltaPP) + } + // This is the assertion the whole task exists for: at these sample sizes the + // margin swallows the difference. + if d.Distinguishable { + t.Errorf("13.3pp on n=59/70 reported as distinguishable (margin %.2f) — "+ + "this is exactly the false confidence #2495 set out to remove", d.MarginPP) + } + if d.MarginPP <= 13.3 { + t.Errorf("MarginPP = %.2f, expected it to exceed the 13.3pp delta", d.MarginPP) + } +} + +// Same effect size, ~10x the volume: now it is real. Proves the flag tracks +// sample size rather than just the size of the gap. +func TestProportionDelta_SameGapBecomesDistinguishableWithVolume(t *testing.T) { + d := proportionDelta(0.153, 600, 0.286, 700) + if d == nil { + t.Fatal("expected a delta") + } + if !d.Distinguishable { + t.Errorf("13.3pp on n=600/700 should be distinguishable (margin %.2f)", d.MarginPP) + } +} + +func TestProportionDelta_SignAndDirection(t *testing.T) { + // Surface skips MORE than baseline -> positive delta (worse for skip rate). + worse := proportionDelta(0.40, 500, 0.25, 500) + if worse == nil || worse.DeltaPP <= 0 { + t.Fatalf("expected a positive delta, got %+v", worse) + } + better := proportionDelta(0.10, 500, 0.25, 500) + if better == nil || better.DeltaPP >= 0 { + t.Fatalf("expected a negative delta, got %+v", better) + } +} + +func TestProportionDelta_EmptySamples(t *testing.T) { + if d := proportionDelta(0.2, 0, 0.3, 100); d != nil { + t.Errorf("n1=0 produced a delta: %+v", d) + } + if d := proportionDelta(0.2, 100, 0.3, 0); d != nil { + t.Errorf("n2=0 produced a delta: %+v", d) + } +} + +// Two degenerate rates have zero standard error, which would report a margin of +// 0 and therefore "distinguishable" for a delta of exactly 0. Reporting nothing +// is the honest answer. +func TestProportionDelta_DegenerateRates(t *testing.T) { + if d := proportionDelta(0, 50, 0, 50); d != nil { + t.Errorf("both rates 0 produced a delta: %+v", d) + } + if d := proportionDelta(1, 50, 1, 50); d != nil { + t.Errorf("both rates 1 produced a delta: %+v", d) + } + // One degenerate side is still informative — the other side carries variance. + if d := proportionDelta(0, 200, 0.3, 200); d == nil { + t.Error("one degenerate rate should still yield a delta") + } +} + +func TestMeanDelta(t *testing.T) { + // Completion is bimodal, so ~0.16 variance (sd ≈ 0.4) is realistic. + const v = 0.16 + thin := meanDelta(0.82, v, 59, 0.54, v, 70) + if thin == nil { + t.Fatal("expected a delta") + } + if math.Abs(thin.DeltaPP-28.0) > 0.1 { + t.Errorf("DeltaPP = %.2f, want ≈ 28.0", thin.DeltaPP) + } + // 28pp is large enough to survive even a wide margin at this n. + if !thin.Distinguishable { + t.Errorf("28pp on n=59/70 with sd 0.4 should be distinguishable (margin %.2f)", thin.MarginPP) + } + // A small completion gap at the same volume should not be. + small := meanDelta(0.56, v, 59, 0.54, v, 70) + if small == nil { + t.Fatal("expected a delta") + } + if small.Distinguishable { + t.Errorf("2pp on n=59/70 reported as distinguishable (margin %.2f)", small.MarginPP) + } +} + +// A sample variance needs at least two observations per side. +func TestMeanDelta_NeedsTwoObservations(t *testing.T) { + if d := meanDelta(0.8, 0.1, 1, 0.5, 0.1, 100); d != nil { + t.Errorf("n1=1 produced a delta: %+v", d) + } + if d := meanDelta(0.8, 0.1, 100, 0.5, 0.1, 1); d != nil { + t.Errorf("n2=1 produced a delta: %+v", d) + } +} + +func TestMeanDelta_ZeroVarianceBothSides(t *testing.T) { + if d := meanDelta(0.8, 0, 50, 0.5, 0, 50); d != nil { + t.Errorf("zero variance on both sides produced a delta: %+v", d) + } +} + +func TestSampleVariance(t *testing.T) { + // Observations 0, 1: mean 0.5, sample variance 0.5. + if got := sampleVariance(1.0, 1.0, 2); math.Abs(got-0.5) > 1e-9 { + t.Errorf("sampleVariance = %v, want 0.5", got) + } + // Identical observations -> zero variance, and must not go negative through + // floating-point cancellation. + if got := sampleVariance(4.0, 4.0, 4); got != 0 { + t.Errorf("identical observations gave variance %v, want 0", got) + } + if got := sampleVariance(0, 0, 1); got != 0 { + t.Errorf("n=1 gave variance %v, want 0", got) + } +} + +// Clamping matters: a negative variance would become NaN in the square root and +// propagate into the JSON as a null-ish number. +func TestSampleVariance_NeverNegative(t *testing.T) { + // sqSum slightly below sum²/n, as cancellation can produce. + if got := sampleVariance(10.0, 24.999999999, 4); got < 0 { + t.Errorf("variance went negative: %v", got) + } +} + +func TestNewDelta_BoundaryCountsAsDistinguishable(t *testing.T) { + d := newDelta(5.0, 5.0) + if !d.Distinguishable { + t.Error("a delta exactly equal to its margin should count as distinguishable") + } + d = newDelta(4.999, 5.0) + if d.Distinguishable { + t.Error("a delta just inside its margin should not count as distinguishable") + } +} diff --git a/internal/db/dbq/recommendation_metrics.sql.go b/internal/db/dbq/recommendation_metrics.sql.go index daee78a4..1fe4528b 100644 --- a/internal/db/dbq/recommendation_metrics.sql.go +++ b/internal/db/dbq/recommendation_metrics.sql.go @@ -18,7 +18,8 @@ SELECT count(*)::bigint AS plays, count(*) FILTER (WHERE pe.was_skipped)::bigint AS skips, count(pe.completion_ratio)::bigint AS completion_n, - COALESCE(avg(pe.completion_ratio), 0)::float8 AS avg_completion + COALESCE(avg(pe.completion_ratio), 0)::float8 AS avg_completion, + COALESCE(sum(pe.completion_ratio * pe.completion_ratio), 0)::float8 AS completion_sqsum FROM play_events pe WHERE pe.user_id = $1 AND pe.started_at > now() - ($2::float8 * INTERVAL '1 day') @@ -32,18 +33,26 @@ type RecommendationSourceMetricsForUserParams struct { } type RecommendationSourceMetricsForUserRow struct { - Source *string - PickKind *string - Plays int64 - Skips int64 - CompletionN int64 - AvgCompletion float64 + Source *string + PickKind *string + Plays int64 + Skips int64 + CompletionN int64 + AvgCompletion float64 + CompletionSqsum float64 } // $1 user_id, $2 window_days. plays/skips are counts; avg_completion is the // mean completion ratio over the completion_n plays that recorded one. // pick_kind splits For You plays into taste/fresh/unattributed (#1249); // it is NULL for every other source, so those still group to one row. +// +// completion_sqsum carries the sum of SQUARED completion ratios so the Go +// handler can compute a variance — needed for the margin of error on a +// completion delta (#2495). It is the sum rather than `stddev_samp` on purpose: +// raw source rows get merged into surface families in Go, and sums of squares +// add across groups exactly, whereas standard deviations cannot be combined +// without them. Variance = (sqsum - sum²/n) / (n-1), with sum = avg × n. func (q *Queries) RecommendationSourceMetricsForUser(ctx context.Context, arg RecommendationSourceMetricsForUserParams) ([]RecommendationSourceMetricsForUserRow, error) { rows, err := q.db.Query(ctx, recommendationSourceMetricsForUser, arg.UserID, arg.Column2) if err != nil { @@ -60,6 +69,7 @@ func (q *Queries) RecommendationSourceMetricsForUser(ctx context.Context, arg Re &i.Skips, &i.CompletionN, &i.AvgCompletion, + &i.CompletionSqsum, ); err != nil { return nil, err } diff --git a/internal/db/queries/recommendation_metrics.sql b/internal/db/queries/recommendation_metrics.sql index 48d66565..75c70a5f 100644 --- a/internal/db/queries/recommendation_metrics.sql +++ b/internal/db/queries/recommendation_metrics.sql @@ -44,13 +44,21 @@ ORDER BY 1, 2; -- mean completion ratio over the completion_n plays that recorded one. -- pick_kind splits For You plays into taste/fresh/unattributed (#1249); -- it is NULL for every other source, so those still group to one row. +-- +-- completion_sqsum carries the sum of SQUARED completion ratios so the Go +-- handler can compute a variance — needed for the margin of error on a +-- completion delta (#2495). It is the sum rather than `stddev_samp` on purpose: +-- raw source rows get merged into surface families in Go, and sums of squares +-- add across groups exactly, whereas standard deviations cannot be combined +-- without them. Variance = (sqsum - sum²/n) / (n-1), with sum = avg × n. SELECT pe.source, pe.pick_kind, count(*)::bigint AS plays, count(*) FILTER (WHERE pe.was_skipped)::bigint AS skips, count(pe.completion_ratio)::bigint AS completion_n, - COALESCE(avg(pe.completion_ratio), 0)::float8 AS avg_completion + COALESCE(avg(pe.completion_ratio), 0)::float8 AS avg_completion, + COALESCE(sum(pe.completion_ratio * pe.completion_ratio), 0)::float8 AS completion_sqsum FROM play_events pe WHERE pe.user_id = $1 AND pe.started_at > now() - ($2::float8 * INTERVAL '1 day') diff --git a/internal/library/scanner.go b/internal/library/scanner.go index 985333c6..6d78fc20 100644 --- a/internal/library/scanner.go +++ b/internal/library/scanner.go @@ -414,8 +414,21 @@ func (s *Scanner) resolveArtist(ctx context.Context, q *dbq.Queries, name, mbid ID: existing.ID, Mbid: &m, }); uerr != nil { - s.logger.Warn("library scan: heal artist mbid failed", - "artist_id", existing.ID, "err", uerr) + if isUniqueViolation(uerr) { + // Another artist row already owns this MBID — two rows that + // should be merged (usually two spellings of one name). + // Expected, not a fault: leave NULL and let the operator + // merge. Mirrors resolveAlbum, which has always handled it + // this way — without this branch the identical benign + // condition logged a generic warning plus a Postgres ERROR + // line on every scan, which teaches an operator to ignore + // database errors (#2524). + s.logger.Info("library scan: duplicate artist mbid (canonical row already owns it)", + "artist_id", existing.ID, "artist", name, "mbid", mbid) + } else { + s.logger.Warn("library scan: heal artist mbid failed", + "artist_id", existing.ID, "err", uerr) + } } else { existing.Mbid = &m } diff --git a/web/src/lib/api/metrics.ts b/web/src/lib/api/metrics.ts index 3f467d30..776ee52b 100644 --- a/web/src/lib/api/metrics.ts +++ b/web/src/lib/api/metrics.ts @@ -4,6 +4,20 @@ import { api } from './client'; // Mirrors internal/api/me_recommendation_metrics.go: raw play sources are // bucketed server-side into stable surface families, grouped by intent, and // anchored by the manual-plays baseline (milestone #127). +// A difference from the baseline, with its uncertainty (#2495). Both figures +// are already in percentage points — the server does the arithmetic so both +// clients read the same numbers. +// +// `distinguishable: false` means |delta_pp| < margin_pp: the delta cannot be +// told apart from zero, however large it looks. That distinction is the whole +// point of this type — `low_confidence` answers "is this worth showing?", which +// is a much lower bar than "is this worth acting on?". +export type MetricDelta = { + delta_pp: number; + margin_pp: number; + distinguishable: boolean; +}; + export type SurfaceMetric = { key: string; label: string; @@ -12,6 +26,10 @@ export type SurfaceMetric = { skip_rate: number; avg_completion: number; low_confidence: boolean; + // Absent on the baseline row itself, and whenever the samples are too thin + // for a margin to mean anything. + skip_delta?: MetricDelta; + completion_delta?: MetricDelta; // 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 diff --git a/web/src/routes/admin/tuning/+page.svelte b/web/src/routes/admin/tuning/+page.svelte index 33bb0b19..f1b8419b 100644 --- a/web/src/routes/admin/tuning/+page.svelte +++ b/web/src/routes/admin/tuning/+page.svelte @@ -216,9 +216,17 @@ return plays > 0 ? hits / plays : 0; } - function latest(s: TrendSeries): { skip: number; completion: number } { + // The "latest" columns are ONE WEEK while the Plays column is the whole + // window, which is a trap: a 40% skip rate off 17 plays sat next to a + // four-figure Plays total and read as a solid signal. It isn't — I misread + // exactly this and briefly concluded Deep cuts was the worst surface, when + // over 180 days it's one of the best (#2495). So the week's own play count + // comes back with the rates and is rendered beside them. + function latest(s: TrendSeries): { skip: number; completion: number; plays: number } { const last = s.points[s.points.length - 1]; - return last ? { skip: last.skip_rate, completion: last.avg_completion } : { skip: 0, completion: 0 }; + return last + ? { skip: last.skip_rate, completion: last.avg_completion, plays: last.plays } + : { skip: 0, completion: 0, plays: 0 }; } function pct(v: number): string { @@ -420,6 +428,10 @@

Weekly trends

+ Skip and completion are the most recent week only — + the number after the skip rate is that week's play count, so a rate off a + handful of plays reads as what it is. Plays and Taste hit cover the whole + window. Skip rate per surface over the last {trends?.weeks ?? 12} weeks (lower is better; all users aggregated, rates only). Dashed ticks mark tuning changes. Taste hit is the share of plays whose artist fits the current taste profile. @@ -439,10 +451,10 @@ Surface Skip rate by week - Plays - Latest skip - Latest completion - Taste hit + Plays (window) + Skip (last wk) + Completion (last wk) + Taste hit (window) @@ -493,7 +505,10 @@ {s.plays} - {pct(latest(s).skip)} + + {pct(latest(s).skip)} + /{latest(s).plays} + {pct(latest(s).completion)} {pct(windowTasteHitRate(s))} diff --git a/web/src/routes/admin/tuning/tuning.test.ts b/web/src/routes/admin/tuning/tuning.test.ts index 3e362bf6..49a91bac 100644 --- a/web/src/routes/admin/tuning/tuning.test.ts +++ b/web/src/routes/admin/tuning/tuning.test.ts @@ -195,9 +195,20 @@ describe('Admin tuning page', () => { await waitFor(() => expect(screen.getByText('Weekly trends')).toBeInTheDocument()); expect(screen.getByTestId('sparkline-radio')).toBeInTheDocument(); expect(screen.getByTestId('sparkline-discover')).toBeInTheDocument(); - // Latest skip rate column for radio = 40% (also discover's latest - // completion, hence getAllBy). - expect(screen.getAllByText('40%').length).toBeGreaterThan(0); + // The skip column is the LAST WEEK's rate, now carrying that week's play + // count so a rate off a handful of plays reads as what it is (#2495). + // Radio's latest week: 40% skip over 15 plays; Discover's: 60% over 5. + expect(screen.getByText('/15')).toBeInTheDocument(); + expect(screen.getByText('/5')).toBeInTheDocument(); + // Completion columns are unchanged and still bare percentages — radio 70%, + // discover 40%. + expect(screen.getByText('70%')).toBeInTheDocument(); + expect(screen.getByText('40%')).toBeInTheDocument(); + // The window/last-week distinction has to be visible in the headers, or the + // Plays total reads as the denominator of the skip rate. That misreading is + // what #2495 was filed over. + expect(screen.getByText(/Plays/)).toHaveTextContent('(window)'); + expect(screen.getByText(/^Skip/)).toHaveTextContent('(last wk)'); // The knob turn is listed under the chart AND tooltipped on each // sparkline's marker tick, hence getAllBy. expect( diff --git a/web/src/routes/settings/+page.svelte b/web/src/routes/settings/+page.svelte index 4d6c98cb..96a950b6 100644 --- a/web/src/routes/settings/+page.svelte +++ b/web/src/routes/settings/+page.svelte @@ -12,7 +12,7 @@ createRecommendationMetricsQuery, type RecommendationMetrics, type SurfaceIntent, - type SurfaceMetric + type MetricDelta } from '$lib/api/metrics'; import { theme, setTheme, type ThemePreference } from '$lib/stores/theme.svelte'; import { player, setCrossfade } from '$lib/player/store.svelte'; @@ -45,20 +45,39 @@ return `${(v * 100).toFixed(0)}%`; } - // Delta in percentage points vs the baseline, signed ("+12" / "−5"). - function deltaPts(value: number, baseline: number): string { - const pts = Math.round((value - baseline) * 100); - return pts > 0 ? `+${pts}` : `${pts}`; + // Deltas come from the server with their margin of error (#2495). The client + // no longer subtracts rates itself: the margin needs the sample sizes and + // variances, and having both clients re-derive it invites them to disagree. + // + // A delta that isn't distinguishable from zero is prefixed "≈" and dimmed. + // That is the point of this whole change — the card used to render a −12 on + // 59 plays exactly as boldly as a −6 on 400, and the first of those is noise. + function deltaText(d: MetricDelta | undefined): string { + if (!d) return ''; + const pts = Math.round(d.delta_pp); + const signed = pts > 0 ? `+${pts}` : `${pts}`; + return d.distinguishable ? signed : `≈${signed}`; } - // A surface's skip delta is "worse" when it skips more than the - // baseline; completion delta is "worse" when it completes less. - function skipDeltaClass(m: SurfaceMetric, baseline: SurfaceMetric): string { - return m.skip_rate > baseline.skip_rate ? 'text-danger' : 'text-text-secondary'; + function deltaTitle(d: MetricDelta | undefined): string | undefined { + if (!d) return undefined; + const range = `${d.delta_pp.toFixed(1)} ± ${d.margin_pp.toFixed(1)} points vs baseline`; + return d.distinguishable + ? `${range} (95% confidence)` + : `${range} — not distinguishable from zero at 95% confidence, so read this as no measured difference.`; } - function completionDeltaClass(m: SurfaceMetric, baseline: SurfaceMetric): string { - return m.avg_completion < baseline.avg_completion ? 'text-danger' : 'text-text-secondary'; + // A skip delta is "worse" above the baseline; a completion delta is "worse" + // below it. Neither gets a colour unless it's distinguishable — colouring + // noise red is what made the old card misleading. + function skipDeltaClass(d: MetricDelta | undefined): string { + if (!d?.distinguishable) return 'text-text-secondary opacity-60'; + return d.delta_pp > 0 ? 'text-danger' : 'text-text-secondary'; + } + + function completionDeltaClass(d: MetricDelta | undefined): string { + if (!d?.distinguishable) return 'text-text-secondary opacity-60'; + return d.delta_pp < 0 ? 'text-danger' : 'text-text-secondary'; } // Pick-kind breakdowns are collapsed by default (#1270): with every @@ -384,18 +403,16 @@ {m.plays} {pct(m.skip_rate)} - {#if baseline} - - {deltaPts(m.skip_rate, baseline.skip_rate)} - + {#if m.skip_delta} + {deltaText(m.skip_delta)} {/if} {pct(m.avg_completion)} - {#if baseline} - - {deltaPts(m.avg_completion, baseline.avg_completion)} - + {#if m.completion_delta} + {deltaText(m.completion_delta)} {/if} @@ -420,18 +437,16 @@ {b.plays} {pct(b.skip_rate)} - {#if baseline} - - {deltaPts(b.skip_rate, baseline.skip_rate)} - + {#if b.skip_delta} + {deltaText(b.skip_delta)} {/if} {pct(b.avg_completion)} - {#if baseline} - - {deltaPts(b.avg_completion, baseline.avg_completion)} - + {#if b.completion_delta} + {deltaText(b.completion_delta)} {/if} @@ -442,6 +457,12 @@

{/each} +

+ Deltas compare each surface with your manual plays. A delta marked + ≈ is smaller than its own margin of error at this + sample size — it can't be told apart from no difference, however big it looks. + Hover any delta for its range. +

{:else}

No plays recorded yet. Play something from For You, Discover, or a mix. diff --git a/web/src/routes/settings/settings.test.ts b/web/src/routes/settings/settings.test.ts index fcd21fe5..53179a6a 100644 --- a/web/src/routes/settings/settings.test.ts +++ b/web/src/routes/settings/settings.test.ts @@ -241,6 +241,71 @@ describe('Settings page — Recommendation metrics card', () => { expect(screen.queryByText(/Taste picks/)).not.toBeInTheDocument(); }); + // #2495: the card used to render a delta computed client-side with no notion + // of uncertainty, so a -12 on 59 plays looked exactly as solid as a -6 on 400. + // Deltas now arrive from the server with a margin, and an indistinguishable + // one is marked with "≈" and dimmed rather than coloured. + test('a delta smaller than its margin is marked as indistinguishable', async () => { + setupPage(); + metricsMock.data = { + window_days: 30, + baseline: metric('manual', 'Manual library plays', { plays: 400, skip_rate: 0.27 }), + groups: [ + { + intent: 'discovery', + label: 'Discovery mixes', + surfaces: [ + metric('discover', 'Discover', { + plays: 59, + skip_rate: 0.153, + // 13.3pp gap, but the margin at n=59 is wider than the gap. + skip_delta: { delta_pp: -13.3, margin_pp: 14.5, distinguishable: false }, + completion_delta: { delta_pp: 28.0, margin_pp: 12.1, distinguishable: true } + }) + ] + } + ] + }; + render(SettingsPage); + await waitFor(() => expect(screen.getByText('Discover')).toBeInTheDocument()); + + // The indistinguishable skip delta is prefixed and explained on hover. + const skip = screen.getByText('≈-13'); + expect(skip).toBeInTheDocument(); + expect(skip).toHaveAttribute('title', expect.stringContaining('not distinguishable from zero')); + // It must NOT be coloured as a real regression/improvement. + expect(skip.className).toContain('opacity-60'); + + // The completion delta clears its margin, so it renders plainly. + const completion = screen.getByText('+28'); + expect(completion).toBeInTheDocument(); + expect(completion.className).not.toContain('opacity-60'); + + // And the legend explains the glyph rather than leaving it a mystery. + expect(screen.getByText(/smaller than its own margin of error/i)).toBeInTheDocument(); + }); + + // A delta is omitted entirely when the samples are too thin for a margin to + // mean anything — the server decides that, and the cell must simply show the + // rate rather than a bare "0". + test('a surface with no delta shows its rate and nothing else', async () => { + setupPage(); + metricsMock.data = { + window_days: 30, + baseline: metric('manual', 'Manual library plays', { plays: 400 }), + groups: [ + { + intent: 'go_to', + label: 'Go-to surfaces', + surfaces: [metric('radio', 'Radio', { plays: 1, skip_rate: 0 })] + } + ] + }; + render(SettingsPage); + await waitFor(() => expect(screen.getByText('Radio')).toBeInTheDocument()); + expect(screen.queryByText(/≈/)).not.toBeInTheDocument(); + }); + test('surfaces without a breakdown render no toggle and no sub-rows', async () => { setupPage(); metricsMock.data = { From a094d5f8b0b4a808f37ed2234de7f1a42b4645ce Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 6 Aug 2026 21:14:53 -0400 Subject: [PATCH 2/4] =?UTF-8?q?test(metrics):=20target=20deltas=20by=20tes?= =?UTF-8?q?t=20id,=20not=20by=20glyph=20=E2=80=94=20#2495?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two CI failures, both in my own new tests, both informative. settings: queryByText(/≈/) matched the LEGEND explaining the glyph rather than a delta, so the "no delta" case failed on the explanation being present. Delta spans now carry data-testid so a test can name what it means instead of pattern-matching prose that sits next to it. tuning: getByText("40%") found two elements. testing-library matches an element and its OWN direct text nodes, so the skip cell still matches "40%" despite the trailing play-count span — and discover late-week completion is also 40%. Genuinely ambiguous now; assert the count. --- web/src/routes/admin/tuning/tuning.test.ts | 9 ++++++--- web/src/routes/settings/+page.svelte | 4 ++++ web/src/routes/settings/settings.test.ts | 15 +++++++++------ 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/web/src/routes/admin/tuning/tuning.test.ts b/web/src/routes/admin/tuning/tuning.test.ts index 49a91bac..d0e2f63c 100644 --- a/web/src/routes/admin/tuning/tuning.test.ts +++ b/web/src/routes/admin/tuning/tuning.test.ts @@ -200,10 +200,13 @@ describe('Admin tuning page', () => { // Radio's latest week: 40% skip over 15 plays; Discover's: 60% over 5. expect(screen.getByText('/15')).toBeInTheDocument(); expect(screen.getByText('/5')).toBeInTheDocument(); - // Completion columns are unchanged and still bare percentages — radio 70%, - // discover 40%. + // Completion columns are unchanged and still bare percentages — radio 70%. expect(screen.getByText('70%')).toBeInTheDocument(); - expect(screen.getByText('40%')).toBeInTheDocument(); + // '40%' is now genuinely ambiguous: radio's latest SKIP rate and discover's + // latest COMPLETION are both 40%. testing-library matches an element's own + // direct text nodes, so the skip cell still matches despite its trailing + // play-count span. Assert the count rather than pretending it's unique. + expect(screen.getAllByText('40%')).toHaveLength(2); // The window/last-week distinction has to be visible in the headers, or the // Plays total reads as the denominator of the skip rate. That misreading is // what #2495 was filed over. diff --git a/web/src/routes/settings/+page.svelte b/web/src/routes/settings/+page.svelte index 96a950b6..17fbabc5 100644 --- a/web/src/routes/settings/+page.svelte +++ b/web/src/routes/settings/+page.svelte @@ -405,6 +405,7 @@ {pct(m.skip_rate)} {#if m.skip_delta} {deltaText(m.skip_delta)} {/if} @@ -412,6 +413,7 @@ {pct(m.avg_completion)} {#if m.completion_delta} {deltaText(m.completion_delta)} {/if} @@ -439,6 +441,7 @@ {pct(b.skip_rate)} {#if b.skip_delta} {deltaText(b.skip_delta)} {/if} @@ -446,6 +449,7 @@ {pct(b.avg_completion)} {#if b.completion_delta} {deltaText(b.completion_delta)} {/if} diff --git a/web/src/routes/settings/settings.test.ts b/web/src/routes/settings/settings.test.ts index 53179a6a..7923241b 100644 --- a/web/src/routes/settings/settings.test.ts +++ b/web/src/routes/settings/settings.test.ts @@ -269,16 +269,18 @@ describe('Settings page — Recommendation metrics card', () => { render(SettingsPage); await waitFor(() => expect(screen.getByText('Discover')).toBeInTheDocument()); - // The indistinguishable skip delta is prefixed and explained on hover. - const skip = screen.getByText('≈-13'); - expect(skip).toBeInTheDocument(); + // Targeted by test id rather than text: the legend below the table also + // contains a "≈", so matching on the glyph finds the explanation instead of + // the delta. (It did, on the first attempt at this test.) + const skip = screen.getByTestId('skip-delta-discover'); + expect(skip).toHaveTextContent('≈-13'); expect(skip).toHaveAttribute('title', expect.stringContaining('not distinguishable from zero')); // It must NOT be coloured as a real regression/improvement. expect(skip.className).toContain('opacity-60'); // The completion delta clears its margin, so it renders plainly. - const completion = screen.getByText('+28'); - expect(completion).toBeInTheDocument(); + const completion = screen.getByTestId('completion-delta-discover'); + expect(completion).toHaveTextContent('+28'); expect(completion.className).not.toContain('opacity-60'); // And the legend explains the glyph rather than leaving it a mystery. @@ -303,7 +305,8 @@ describe('Settings page — Recommendation metrics card', () => { }; render(SettingsPage); await waitFor(() => expect(screen.getByText('Radio')).toBeInTheDocument()); - expect(screen.queryByText(/≈/)).not.toBeInTheDocument(); + expect(screen.queryByTestId('skip-delta-radio')).not.toBeInTheDocument(); + expect(screen.queryByTestId('completion-delta-radio')).not.toBeInTheDocument(); }); test('surfaces without a breakdown render no toggle and no sub-rows', async () => { From 96abb4808622f770b69e712e47670327fc5bcf20 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 6 Aug 2026 21:20:18 -0400 Subject: [PATCH 3/4] =?UTF-8?q?test(tuning):=20query=20the=20window/last-w?= =?UTF-8?q?eek=20headers=20as=20column=20headers=20=E2=80=94=20#2495?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getByText(/Plays/) matched my own new caption as well as the header, since the caption explains which columns cover the window. Query by columnheader role instead, which is what the assertion actually means. Also reordered the caption: prepending the clarification turned it into a run-on that opened mid-explanation before saying what the chart was. --- web/src/routes/admin/tuning/+page.svelte | 7 +++---- web/src/routes/admin/tuning/tuning.test.ts | 7 +++++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/web/src/routes/admin/tuning/+page.svelte b/web/src/routes/admin/tuning/+page.svelte index f1b8419b..bececa1f 100644 --- a/web/src/routes/admin/tuning/+page.svelte +++ b/web/src/routes/admin/tuning/+page.svelte @@ -428,13 +428,12 @@

Weekly trends

- Skip and completion are the most recent week only — - the number after the skip rate is that week's play count, so a rate off a - handful of plays reads as what it is. Plays and Taste hit cover the whole - window. Skip rate per surface over the last {trends?.weeks ?? 12} weeks (lower is better; all users aggregated, rates only). Dashed ticks mark tuning changes. Taste hit is the share of plays whose artist fits the current taste profile. + The skip and completion columns show the most recent week + alone, not the whole window — the figure after the skip rate is that week's + play count, so a rate drawn from a handful of listens reads as what it is.

{#if trendsFailed} diff --git a/web/src/routes/admin/tuning/tuning.test.ts b/web/src/routes/admin/tuning/tuning.test.ts index d0e2f63c..00abfa72 100644 --- a/web/src/routes/admin/tuning/tuning.test.ts +++ b/web/src/routes/admin/tuning/tuning.test.ts @@ -210,8 +210,11 @@ describe('Admin tuning page', () => { // The window/last-week distinction has to be visible in the headers, or the // Plays total reads as the denominator of the skip rate. That misreading is // what #2495 was filed over. - expect(screen.getByText(/Plays/)).toHaveTextContent('(window)'); - expect(screen.getByText(/^Skip/)).toHaveTextContent('(last wk)'); + // + // Queried as column headers rather than by text: the caption below also + // mentions "Plays", and matching on the word finds the prose too. + expect(screen.getByRole('columnheader', { name: /Plays/ })).toHaveTextContent('(window)'); + expect(screen.getByRole('columnheader', { name: /^Skip/ })).toHaveTextContent('(last wk)'); // The knob turn is listed under the chart AND tooltipped on each // sparkline's marker tick, hence getAllBy. expect( From 304de88c50c81119e9f2f8b2e7b85c6b8dd7ed05 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 6 Aug 2026 21:27:22 -0400 Subject: [PATCH 4/4] =?UTF-8?q?test(tuning):=20assert=20headers=20by=20exa?= =?UTF-8?q?ct=20accessible=20name=20=E2=80=94=20#2495?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third attempt at the same assertion, so I stopped guessing and got vitest running locally instead: the web lane uses the same ci-go image, so `docker run ... -w /src/web ci-go:1.26 npx vitest run` works and turns a 5-minute CI round trip into 7 seconds. /^Skip/ matched the "Skip rate by week" sparkline column as well as "Skip (last wk)", just as /Plays/ had matched the caption. Exact names say what the assertion means and cannot drift onto a neighbour. Verified locally before pushing: svelte-check 0 errors, 110 files / 786 tests pass. --- web/src/routes/admin/tuning/tuning.test.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/web/src/routes/admin/tuning/tuning.test.ts b/web/src/routes/admin/tuning/tuning.test.ts index 00abfa72..85283657 100644 --- a/web/src/routes/admin/tuning/tuning.test.ts +++ b/web/src/routes/admin/tuning/tuning.test.ts @@ -213,8 +213,11 @@ describe('Admin tuning page', () => { // // Queried as column headers rather than by text: the caption below also // mentions "Plays", and matching on the word finds the prose too. - expect(screen.getByRole('columnheader', { name: /Plays/ })).toHaveTextContent('(window)'); - expect(screen.getByRole('columnheader', { name: /^Skip/ })).toHaveTextContent('(last wk)'); + // Exact accessible names: /^Skip/ also matches the "Skip rate by week" + // sparkline column, and /Plays/ matched the caption prose before that. + expect(screen.getByRole('columnheader', { name: 'Plays (window)' })).toBeInTheDocument(); + expect(screen.getByRole('columnheader', { name: 'Skip (last wk)' })).toBeInTheDocument(); + expect(screen.getByRole('columnheader', { name: 'Completion (last wk)' })).toBeInTheDocument(); // The knob turn is listed under the chart AND tooltipped on each // sparkline's marker tick, hence getAllBy. expect(