From 046abd8d432e2daa076c1e15ef45e3df4536b725 Mon Sep 17 00:00:00 2001 From: Fable Integrator Date: Fri, 17 Jul 2026 17:52:54 -0400 Subject: [PATCH 1/2] =?UTF-8?q?fix(coachhelm):=20Signals=20coherence=20?= =?UTF-8?q?=E2=80=94=20player=20names,=20KPI=20semantics,=20leak=20math,?= =?UTF-8?q?=20accuracy=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four independently-verified bugs on the Signals/Effectiveness surfaces: 1. By-player grouping ("Unknown player"): EvidenceInsight never carried a resolved player name (only golf_coach_insights.player_id), so every insight/alert row hard-coded playerName: undefined and collapsed into one "Unknown player" bucket. Patterns were unaffected (getTeamPatterns already joins golf_players inline). insightToSignalRow/insightsToSignalRows now accept an optional player_id -> name map; /alerts and /insights SSR-fetch the team roster via getTeamPlayers() and pass it through. 2. KPI tile semantics: "Urgent + high" counted every loaded row regardless of status while "Open" filtered by status, so /alerts could show "Urgent + high 13 > Open 10" — a severity breakdown reading as MORE than its own superset. Urgent+high is now a true subset of Open (matches the shell badge's own getAlertCounts().counts.critical semantics). Tile labels now say the sub-tab noun ("Open alerts"/"Open insights"/"Open patterns") instead of a generic "Open signals", and "Loaded" (dev-speak) is renamed "Showing". 3. LeakBoard math/label: the "Where the team is bleeding" total is a SUM across every player and every leak insight in a category, but was labeled "str/rd" (a per-round rate) — reading as if −26.4 meant 26 strokes lost EVERY round, ~8x the real team SG-putting figure (~−3.19/rd). Relabeled to an honest "total", and the rollup now reads from its own full team-wide fetch (no priority filter) instead of the active sub-tab's own scoped `insights` state, so the banner no longer silently re-totals per sub-tab. 4. Effectiveness accuracy headline: gated on GAUGE_MIN_RESOLVED (2) while the calibration side panel's own copy says "needs 5 resolved predictions" — the two thresholds could disagree, letting a near-empty sample render an authoritative "100% ▲+8%". Now gated on the SAME 5-resolved threshold (isAccuracyHeadlineLive), with "Calibrating" replacing "Awaiting predictions". Also fixed: StrokesGainedTornado's y-scale keyed rows by display label, so two rows sharing a label (e.g. one player with two top-N patterns) collapsed onto the same band — bars and value text rendered on top of each other ("+4.10+4.67"). Now keyed by row index. And the "no trend yet" cell rendered a bare "—" with no visible label (only a tooltip); now matches the icon idiom the other three trend states already use, dimmed to read as "no data" rather than a genuine flat trend. Gates: tsc --noEmit clean, eslint clean on changed files, full vitest suite green (272 files / 2636 passed). Part of #914 + Fixes #907 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg --- .../(dashboard)/dashboard/alerts/page.tsx | 15 ++- .../(dashboard)/dashboard/insights/page.tsx | 10 ++ .../charts/StrokesGainedTornado.test.tsx | 61 +++++++++ .../fairway/charts/StrokesGainedTornado.tsx | 26 +++- .../coachhelm/FairwayCoachHelmSignals.tsx | 123 ++++++++++++++---- .../coachhelm/FairwayEffectiveness.test.ts | 38 ++++++ .../pages/coachhelm/FairwayEffectiveness.tsx | 38 +++++- .../patternToInsightVocabulary.test.ts | 104 +++++++++++++++ .../signals/patternToInsightVocabulary.ts | 32 ++++- .../golf/coachhelm/coach/LeakBoard.test.tsx | 70 ++++++++++ .../golf/coachhelm/coach/LeakBoard.tsx | 26 ++-- 11 files changed, 489 insertions(+), 54 deletions(-) create mode 100644 src/components/fairway/charts/StrokesGainedTornado.test.tsx create mode 100644 src/components/fairway/pages/coachhelm/FairwayEffectiveness.test.ts create mode 100644 src/components/fairway/pages/coachhelm/signals/patternToInsightVocabulary.test.ts create mode 100644 src/components/golf/coachhelm/coach/LeakBoard.test.tsx diff --git a/src/app/golf/(dashboard)/dashboard/alerts/page.tsx b/src/app/golf/(dashboard)/dashboard/alerts/page.tsx index 200d27653..93f5aa8d5 100644 --- a/src/app/golf/(dashboard)/dashboard/alerts/page.tsx +++ b/src/app/golf/(dashboard)/dashboard/alerts/page.tsx @@ -16,6 +16,7 @@ import { createClient } from '@/lib/supabase/server'; import { getGolfSessionProfile } from '@/lib/auth/session'; import { getAlertCounts } from '@/app/golf/actions/alerts'; import { getInsightsForCoachWithMeta } from '@/app/golf/actions/insight-delivery'; +import { getTeamPlayers } from '@/app/golf/actions/roster'; import { fairwayScope } from '@/lib/redesign/flag'; import { FairwayCoachHelmSignals, EmptyState, Button } from '@/components/fairway'; import { resolveCoachTeamIdWithCookie } from '@/lib/golf/resolve-team-server'; @@ -98,17 +99,28 @@ export default async function AlertsPage() { // so the Signals workspace paints data on the first frame instead of // mounting at loading=true → an always-on client fetch + skeleton flash on // every /alerts visit. Mirrors how /patterns already SSR-seeds initialPatterns. - const [countsRes, insightsRes] = await Promise.all([ + // Roster names in the SAME round trip: the "By player" grouping resolves + // insight-sourced rows' `playerName` from this map (insights only carry + // `player_id`, never a joined name — unlike patterns, which resolve it + // inline via `getTeamPatterns`). Without it every insight/alert row falls + // back to the grouping's "Unknown player" bucket. + const [countsRes, insightsRes, rosterRes] = await Promise.all([ getAlertCounts(coach.id), getInsightsForCoachWithMeta(coach.id, { limit: 100, priorities: ['urgent', 'high'], }), + getTeamPlayers(), ]); const signalCount = countsRes.success ? (countsRes.counts?.critical ?? null) : null; // Honest fallback: a DB error leaves initialInsights empty so the surface // falls back to its own client fetch + error handling (never a fake feed). const initialInsights = insightsRes.ok ? insightsRes.data : []; + const playerNames: Record = {}; + for (const p of rosterRes.data ?? []) { + const name = [p.first_name, p.last_name].filter(Boolean).join(' ').trim(); + if (name) playerNames[p.id] = name; + } return (
= {}; + for (const p of rosterRes.data ?? []) { + const name = [p.first_name, p.last_name].filter(Boolean).join(' ').trim(); + if (name) playerNames[p.id] = name; + } return (
diff --git a/src/components/fairway/charts/StrokesGainedTornado.test.tsx b/src/components/fairway/charts/StrokesGainedTornado.test.tsx new file mode 100644 index 000000000..30b5726ea --- /dev/null +++ b/src/components/fairway/charts/StrokesGainedTornado.test.tsx @@ -0,0 +1,61 @@ +// @vitest-environment jsdom +/** + * ============================================================================ + * StrokesGainedTornado — duplicate-label row-collision regression guard + * ---------------------------------------------------------------------------- + * Bug: `TornadoInner`'s y-scale (`scaleBand`) used to key its domain by + * `d.label`. Two rows CAN legitimately share a label — e.g. the Effectiveness + * cockpit's "Most impactful patterns" tornado labels each bar with the + * player's NAME, and one player can have two separate top-N patterns — + * `scaleBand`'s domain is a distinct key set, so a duplicate label collapsed + * both rows onto the SAME band. Their bars AND their signed value-annotation + * text then rendered on top of each other — observed live as two adjacent + * labels like "+4.10" and "+4.67" reading as the single garbled string + * "+4.10+4.67". + * + * Fix: the y-scale is now keyed by ROW INDEX (always unique), never by the + * display label. This locks that duplicate labels get distinct rows. + * ========================================================================== */ +import { render } from '@testing-library/react'; +import { describe, it, expect } from 'vitest'; +import { TornadoInner, type SGCategory } from './StrokesGainedTornado'; + +describe('StrokesGainedTornado — duplicate-label collision guard', () => { + it('two rows sharing the SAME label render on DISTINCT y positions (never collide)', () => { + const data: SGCategory[] = [ + { label: 'Jordan Lee', value: 4.1 }, + { label: 'Jordan Lee', value: 4.67 }, // same player, two top-N patterns + { label: 'Sam Park', value: -2.3 }, + ]; + const { container } = render(); + const texts = Array.from(container.querySelectorAll('text')); + const find = (t: string) => texts.find((el) => el.textContent === t); + + const first = find('+4.10'); + const second = find('+4.67'); + const third = find('−2.30'); // formatSigned uses U+2212 minus, not a hyphen + + expect(first).toBeDefined(); + expect(second).toBeDefined(); + expect(third).toBeDefined(); + + const ys = [first, second, third].map((el) => el!.getAttribute('y')); + // All three rows must land on distinct y positions — a regression back to + // label-keying would collapse the two 'Jordan Lee' rows onto the same y. + expect(new Set(ys).size).toBe(3); + }); + + it('unique labels still render one row each at distinct positions (no regression)', () => { + const data: SGCategory[] = [ + { label: 'Off the tee', value: 1.1 }, + { label: 'Approach', value: -0.8 }, + ]; + const { container } = render(); + const texts = Array.from(container.querySelectorAll('text')); + const ys = texts + .filter((el) => el.textContent === '+1.10' || el.textContent === '−0.80') + .map((el) => el.getAttribute('y')); + expect(ys.length).toBe(2); + expect(new Set(ys).size).toBe(2); + }); +}); diff --git a/src/components/fairway/charts/StrokesGainedTornado.tsx b/src/components/fairway/charts/StrokesGainedTornado.tsx index 725706a55..052b0925d 100644 --- a/src/components/fairway/charts/StrokesGainedTornado.tsx +++ b/src/components/fairway/charts/StrokesGainedTornado.tsx @@ -105,7 +105,13 @@ export function StrokesGainedTornado({ ); } -function TornadoInner({ +/** + * Exported for unit testing only — the parent `StrokesGainedTornado` always + * mounts this through ``, which needs real DOM layout. Rendering + * `TornadoInner` directly with explicit `width`/`height` lets a test assert + * on the resulting SVG geometry without fighting ResizeObserver in jsdom. + */ +export function TornadoInner({ width, height, data, @@ -130,10 +136,18 @@ function TornadoInner({ [bound, innerW], ); + // Keyed by ROW INDEX, never by `d.label`. Two rows can legitimately share a + // label (e.g. `PatternImpactDeck`'s "Most impactful patterns" tornado uses + // the player's NAME as the label, and one player can have two separate + // high-impact patterns in the top-N) — `scaleBand`'s domain is a distinct + // key set, so a duplicate label previously collapsed both rows onto the + // SAME band, rendering their bars and value-annotation text on top of each + // other (e.g. two adjacent "+4.10" / "+4.67" labels reading as the single + // garbled string "+4.10+4.67"). Index-based keys are always unique. const yScale = React.useMemo( () => - scaleBand({ - domain: data.map((d) => d.label), + scaleBand({ + domain: data.map((_, i) => i), range: [0, innerH], padding: ROW_PAD, }), @@ -177,15 +191,15 @@ function TornadoInner({ strokeDasharray="5 4" /> - {data.map((d) => { - const y = yScale(d.label) ?? 0; + {data.map((d, i) => { + const y = yScale(i) ?? 0; const valX = xScale(d.value); const positive = d.value >= 0; const barX = positive ? zeroX : valX; const barW = Math.abs(valX - zeroX); const fill = positive ? VIZ_DIVERGING.positive : VIZ_DIVERGING.negative; return ( - + {/* category label in the left gutter */} display name` for the coach's team roster (SSR-resolved via + * `getTeamPlayers`). Insight-sourced rows (alerts/insights) don't carry a + * player name of their own — only patterns do (resolved inline by + * `getTeamPatterns`) — so this is how the "By player" grouping gets a real + * name instead of falling back to "Unknown player" for every row. Optional: + * omitting it just means insight rows group under "Unknown player", the + * prior (degraded) behavior — never a hard failure. + */ + playerNames?: Record; /* shell chrome */ /** Urgent+high open-signal count, computed once server-side (shell badge). */ @@ -287,6 +297,7 @@ export function FairwayCoachHelmSignals({ defaultFilter, initialInsights = [], initialPatterns = [], + playerNames, signalCount, showScanTeam = false, title, @@ -339,7 +350,7 @@ export function FairwayCoachHelmSignals({ const [isBulkPending, startActionTransition] = useTransition(); /* -- P058: the TRUE eligible insight total (post rank+dedupe, pre-100-slice) - from the meta read, so the "Loaded" tile can honestly disclose + from the meta read, so the "Showing" tile can honestly disclose "N of TOTAL" rather than silently truncating at the cap. Insights-only; patterns carry their own `patternCounts`. null until first load. */ const [eligibleTotal, setEligibleTotal] = useState(null); @@ -619,21 +630,46 @@ export function FairwayCoachHelmSignals({ /* ── project both sources into the ONE row vocabulary ──────────────────── */ const allRows: SignalRow[] = useMemo(() => { if (isPatterns) return patternsToSignalRows(patterns); - return insightsToSignalRows(insights); - }, [isPatterns, patterns, insights]); - - // Team "where are we bleeding strokes" rollup (LeakBoard) — groups the live - // insights by skill category by summed stroke-impact. Previously orphaned - // (vizlab demo only); fed here from the SAME live insight rows the list shows. - // Insights only (patterns have their own evidence shape); LeakBoard renders - // its own honest-empty state, so we just pass what we have. + return insightsToSignalRows(insights, playerNames); + }, [isPatterns, patterns, insights, playerNames]); + + // Team "where are we bleeding strokes" rollup (LeakBoard) — groups insights + // by skill category by summed stroke-impact. Fed from its OWN full + // team-wide read (below), NOT the active sub-tab's `insights` state: that + // state is scoped per route (e.g. /alerts fetches urgent+high ONLY, via + // `priorities: defaultFilter.fetchPriorities`; /insights fetches the full + // range), so reusing it made the SAME "Where the team is bleeding" banner + // silently re-total depending on which sub-tab was open. A stable, whole- + // team source means the board reads the same number on /alerts and + // /insights. Insights only (patterns have their own evidence shape); + // LeakBoard renders its own honest-empty state, so we just pass what we have. + const [leakSourceInsights, setLeakSourceInsights] = useState([]); + useEffect(() => { + if (isPatterns) return; + let cancelled = false; + void (async () => { + try { + // Deliberately NO `priorities` filter — the whole team, every + // severity, is the stable scope this rollup promises. + const res = await getInsightsForCoachWithMeta(coachId, { limit: 100 }); + if (!cancelled && res.ok) setLeakSourceInsights(res.data); + } catch { + // Best-effort / failure-silent — the board just stays at its own + // honest empty state, never a stale or wrong total. + } + })(); + return () => { + cancelled = true; + }; + }, [isPatterns, coachId]); + const leakInsights: LeakInsight[] = useMemo(() => { if (isPatterns) return []; const PRIORITY: Record = { critical: 'urgent', urgent: 'urgent', high: 'high', medium: 'medium', low: 'low', info: 'low', }; const out: LeakInsight[] = []; - for (const ins of insights) { + for (const ins of leakSourceInsights) { const impact = ins.evidence?.strokes_impact; if (typeof impact !== 'number' || impact === 0) continue; out.push({ @@ -648,7 +684,7 @@ export function FairwayCoachHelmSignals({ }); } return out; - }, [isPatterns, insights]); + }, [isPatterns, leakSourceInsights]); /* ── client-side filter (ported applyClientFilters) ────────────────────── */ const weight: Record = useMemo( @@ -1124,15 +1160,24 @@ export function FairwayCoachHelmSignals({ return Array.from(seen.entries()).map(([value, label]) => ({ value, label })); }, [allRows]); - /* ── header summary tiles (honest counts, never fabricated) ────────────── */ + /* ── header summary tiles (honest counts, never fabricated) ────────────── + "Urgent + high" is a SUBSET of "Open" — both scoped to open rows, priority + the narrower filter — so the tiles can never contradict each other (the + old version counted urgent+high across EVERY loaded row regardless of + status, which let a sub-tab that pre-filtered its server fetch to + urgent/high-only, like /alerts, show "Urgent + high" HIGHER than "Open" + whenever a few of those loaded rows were already acknowledged/resolved — + e.g. 13 urgent+high vs 10 open, reading as if there were MORE urgent + signals than open ones). This also now matches the shell badge's own + semantics (`getAlertCounts().counts.critical` — open urgent+high only). */ const summary = useMemo(() => { - const open = allRows.filter( + const openRows = allRows.filter( (r) => r.status === 'active' || r.status === 'Detected' || r.status === 'Confirmed', - ).length; - const urgent = allRows.filter( + ); + const urgent = openRows.filter( (r) => r.priority === 'critical' || r.priority === 'high', ).length; - return { total: allRows.length, open, urgent }; + return { total: allRows.length, open: openRows.length, urgent }; }, [allRows]); /* P058: honest "of N" footnote on the Loaded tile when the true eligible set @@ -1142,8 +1187,11 @@ export function FairwayCoachHelmSignals({ IS a known overflow — otherwise the value already IS complete. */ const loadedFootnote = useMemo(() => { if (isPatterns) { + // "capped at N" (not "showing first N") — the tile label itself is + // "Showing", so pairing it with a footnote that repeats the word read + // as a stutter ("SHOWING 5 / showing first 5"). if (patternCounts?.capped) { - return `showing first ${summary.total}`; + return `capped at ${summary.total}`; } return undefined; } @@ -1153,6 +1201,16 @@ export function FairwayCoachHelmSignals({ return undefined; }, [isPatterns, patternCounts, eligibleTotal, allRows.length, summary.total]); + /** The sub-tab noun the KPI tiles are scoped to — each tile already reflects + * ONLY the active segment's own data (its own fetch/state), so the fix is + * labeling that scope honestly instead of a generic "signals" that reads as + * if all three sub-tabs shared one team-wide count. */ + const SIGNAL_NOUN: Record = { + alerts: 'alerts', + insights: 'insights', + patterns: 'patterns', + }; + /* ── grouping — drives BOTH the dense default feed and the grouped view ── */ // While the smart-default shortlist is active, render it FLAT — a curated // cross-player triage list fragments badly grouped one-row-per-player. @@ -1587,20 +1645,31 @@ export function FairwayCoachHelmSignals({ {/* honest summary tiles — never fabricate a 0%; show counts only. Compact 3-up even on mobile so the triage feed isn't pushed below - the fold by three full-width stacked cards (premium-polish pass). */} + the fold by three full-width stacked cards (premium-polish pass). + All three tiles are scoped to the ACTIVE sub-tab's own data (its + own fetch/state) — the label now says so explicitly ("Open + alerts" on /alerts, "Open insights" on /insights, "Open patterns" + on /patterns) instead of a generic "Open signals" that read as if + it were a stable team-wide number, silently re-scoping as the + coach switched tabs. */}
- + {/* P035: no `goodDirection` here — it only colors a `delta` chip, and - there is no delta on this tile, so it was a dead no-op prop. */} + there is no delta on this tile, so it was a dead no-op prop. + Scoped to the SAME open rows as the tile before it (see the + `summary` useMemo above), so this can never read higher than + "Open" — it's a severity breakdown OF the open count, not an + independent count across every loaded row regardless of status. */} - {/* "Loaded" (not "Total"): this counts the rows currently in view — - the read caps at limit:100 and the smart default narrows further — - so labeling it "Total" over-claims completeness when more are - eligible than loaded. P058: when the eligible total exceeds the - loaded set, disclose "of N" honestly rather than silently - truncating; the footnote earns the tile its completeness claim. */} + {/* "Showing" (not "Loaded" — dev-speak a coach shouldn't have to + parse, and not "Total", which over-claims completeness when more + are eligible than loaded: the read caps at limit:100 and the + smart default narrows further). P058: when the eligible total + exceeds the loaded set, disclose "of N" honestly rather than + silently truncating; the footnote earns the tile its + completeness claim. */} diff --git a/src/components/fairway/pages/coachhelm/FairwayEffectiveness.test.ts b/src/components/fairway/pages/coachhelm/FairwayEffectiveness.test.ts new file mode 100644 index 000000000..977f3a972 --- /dev/null +++ b/src/components/fairway/pages/coachhelm/FairwayEffectiveness.test.ts @@ -0,0 +1,38 @@ +/** + * ============================================================================ + * FairwayEffectiveness — accuracy-headline gate regression guard + * ---------------------------------------------------------------------------- + * Bug: the PRIMARY accuracy headline ("ACCURACY 100% ▲+8%") gated on + * `GAUGE_MIN_RESOLVED` (2), a much looser threshold than the calibration side + * panel's own stated requirement ("Still calibrating — needs 5 resolved + * predictions…"). A near-empty sample (e.g. 2-for-2, or genuinely 0/1 + * resolved once any rounding/staleness in the upstream rollup is in play) + * could render an authoritative percentage a coach has no way to distrust. + * + * Fix: the headline now gates on the SAME threshold as the side panel + * (`BUCKET_MIN_RESOLVED` = 5). This locks that the two thresholds cannot + * drift apart again. + * ========================================================================== */ +import { describe, it, expect } from 'vitest'; +import { isAccuracyHeadlineLive } from './FairwayEffectiveness'; + +describe('isAccuracyHeadlineLive', () => { + it('is NOT live at zero resolved predictions', () => { + expect(isAccuracyHeadlineLive(0)).toBe(false); + }); + + it('is NOT live below the 5-resolved threshold (the old 2-resolved gate is gone)', () => { + expect(isAccuracyHeadlineLive(1)).toBe(false); + expect(isAccuracyHeadlineLive(2)).toBe(false); + expect(isAccuracyHeadlineLive(3)).toBe(false); + expect(isAccuracyHeadlineLive(4)).toBe(false); + }); + + it('is live at exactly 5 resolved predictions — matching the calibration panel\'s own stated "needs 5" copy', () => { + expect(isAccuracyHeadlineLive(5)).toBe(true); + }); + + it('stays live well above the threshold', () => { + expect(isAccuracyHeadlineLive(50)).toBe(true); + }); +}); diff --git a/src/components/fairway/pages/coachhelm/FairwayEffectiveness.tsx b/src/components/fairway/pages/coachhelm/FairwayEffectiveness.tsx index 934c059d1..50534ef2b 100644 --- a/src/components/fairway/pages/coachhelm/FairwayEffectiveness.tsx +++ b/src/components/fairway/pages/coachhelm/FairwayEffectiveness.tsx @@ -119,12 +119,27 @@ import { * Honesty thresholds (resolved decisions — deterministic product rule) * • per-bucket InsufficientData when a ConfidenceBucket has < 5 resolved * • a global low-confidence caption while total resolved < 50 - * • the gauge/ribbon need ≥ 2 validated points before they read a value + * • the ribbon needs ≥ 2 validated points before it draws a line * ─────────────────────────────────────────────────────────────────────────── */ const BUCKET_MIN_RESOLVED = 5; const GLOBAL_LOW_CONFIDENCE_RESOLVED = 50; const GAUGE_MIN_RESOLVED = 2; +/** + * Whether the PRIMARY accuracy headline (the huge "ACCURACY 100%" hero + its + * Climbing/Holding delta chip) is trustworthy enough to show a real number. + * Gated on `BUCKET_MIN_RESOLVED` — the SAME threshold the calibration side + * panel already states in its own copy ("Still calibrating — needs + * {BUCKET_MIN_RESOLVED} resolved predictions…"). This used to gate on the + * much looser `GAUGE_MIN_RESOLVED` (2), which let a near-empty sample (e.g. + * 2 for 2) render an authoritative "100% ▲+8%" headline — a coach reading + * the ONE cockpit tile has no way to know that came from two coin flips. + * Exported for unit testing. + */ +export function isAccuracyHeadlineLive(resolved: number): boolean { + return resolved >= BUCKET_MIN_RESOLVED; +} + /* ════════════════════════════════════════════════════════════════════════════ * P1-12 — INSIGHT TRUST LAYER (unified event-ledger rollup) * ---------------------------------------------------------------------------- @@ -277,9 +292,20 @@ function TrustTrendGlyph({ trend }: { trend: TrustSignal['recentTrend'] }) { ); } + // No trend yet (not enough measured outcomes to compute one). Previously a + // bare "—" character with no visible label — an orphan placeholder that + // reads as broken/meaningless at a glance rather than an honest "not enough + // data" state (the title/aria-label were the only context, invisible to a + // sighted user scanning the table without hovering). Now matches the SAME + // icon idiom as the other three states, dimmed to read as "no data" rather + // than a genuine flat trend. return ( - - — + + + No trend data yet ); } @@ -665,7 +691,7 @@ function PrimaryInstrument({ data, days }: { data?: PredictionPerformanceData; d const lowConfidence = resolved > 0 && resolved < GLOBAL_LOW_CONFIDENCE_RESOLVED; - const live = resolved >= GAUGE_MIN_RESOLVED; + const live = isAccuracyHeadlineLive(resolved); return ( @@ -680,8 +706,8 @@ function PrimaryInstrument({ data, days }: { data?: PredictionPerformanceData; d display={formatPercent(accuracy, 0)} size="hero" state={live ? 'live' : 'awaiting'} - samples={live ? undefined : { have: resolved, need: GAUGE_MIN_RESOLVED }} - awaitingLabel="Awaiting predictions" + samples={live ? undefined : { have: resolved, need: BUCKET_MIN_RESOLVED }} + awaitingLabel="Calibrating" /> {live && typeof climbDelta === 'number' ? ( display name` map (the SSR-resolved team roster) and resolve + * `playerName` from it. This locks that contract. + * ========================================================================== */ +import { describe, it, expect } from 'vitest'; +import { + insightToSignalRow, + insightsToSignalRows, +} from './patternToInsightVocabulary'; +import type { EvidenceInsight } from '@/app/golf/actions/insight-delivery'; +import type { InsightEvidence } from '@/lib/coachhelm/v2/insights/types'; + +function makeInsight(overrides: Partial = {}): EvidenceInsight { + return { + id: 'insight-1', + player_id: 'player-1', + category: 'putting', + insight_type: 'putts_per_round', + title: 'Three-putt rate is climbing', + content: 'Three-putts are up over the last 5 rounds.', + signature: 'v3:x', + evidence: { + strokes_impact: 1.2, + confidence: 0.8, + sample_n: 12, + } as unknown as InsightEvidence, + metadata: null, + lifecycle_state: 'detected', + status: 'active', + priority: 'high', + acknowledged_at: null, + resolved_at: null, + created_at: '2026-07-01T00:00:00.000Z', + updated_at: '2026-07-01T00:00:00.000Z', + ...overrides, + }; +} + +describe('insightToSignalRow — player-name resolution', () => { + it('resolves playerName from the roster map when the player_id is present', () => { + const row = insightToSignalRow( + makeInsight({ player_id: 'player-1' }), + { 'player-1': 'Nick Rini', 'player-2': 'Jordan Lee' }, + ); + expect(row.playerName).toBe('Nick Rini'); + // The raw id stays available on the row regardless (used for focus-area + // conversion / deep links) — resolving a name must not erase it. + expect(row.playerId).toBe('player-1'); + }); + + it('falls back to undefined (never a fabricated name) when the map has no entry for the player', () => { + const row = insightToSignalRow( + makeInsight({ player_id: 'player-not-on-roster' }), + { 'player-1': 'Nick Rini' }, + ); + expect(row.playerName).toBeUndefined(); + }); + + it('falls back to undefined when no roster map is supplied at all (back-compat)', () => { + const row = insightToSignalRow(makeInsight()); + expect(row.playerName).toBeUndefined(); + }); + + it('treats an empty resolved name (blank first/last name) the same as "not found"', () => { + const row = insightToSignalRow( + makeInsight({ player_id: 'player-1' }), + { 'player-1': '' }, + ); + expect(row.playerName).toBeUndefined(); + }); +}); + +describe('insightsToSignalRows — batch resolution forwards the SAME map to every row', () => { + it('resolves distinct names for distinct players in one batch', () => { + const rows = insightsToSignalRows( + [ + makeInsight({ id: 'a', player_id: 'player-1' }), + makeInsight({ id: 'b', player_id: 'player-2' }), + makeInsight({ id: 'c', player_id: 'player-3' }), + ], + { 'player-1': 'Nick Rini', 'player-2': 'Jordan Lee' }, + ); + expect(rows.map((r) => r.playerName)).toEqual([ + 'Nick Rini', + 'Jordan Lee', + undefined, // player-3 not on the roster map + ]); + // Distinct real names means the "By player" grouping produces distinct + // buckets instead of collapsing every row into "Unknown player". + const distinctNames = new Set(rows.map((r) => r.playerName).filter(Boolean)); + expect(distinctNames.size).toBe(2); + }); +}); diff --git a/src/components/fairway/pages/coachhelm/signals/patternToInsightVocabulary.ts b/src/components/fairway/pages/coachhelm/signals/patternToInsightVocabulary.ts index 5e0b86ca4..442d2dacd 100644 --- a/src/components/fairway/pages/coachhelm/signals/patternToInsightVocabulary.ts +++ b/src/components/fairway/pages/coachhelm/signals/patternToInsightVocabulary.ts @@ -274,8 +274,24 @@ function formatComparisonValue(value: number): string { return Number.isInteger(value) ? String(value) : value.toFixed(2); } -/** Project a single evidence insight into the shared row shape. */ -export function insightToSignalRow(insight: EvidenceInsight): SignalRow { +/** + * Project a single evidence insight into the shared row shape. + * + * `playerNames` is an OPTIONAL `player_id -> display name` lookup (the coach's + * team roster, SSR-resolved from `golf_players` by the route fork). Insights + * themselves never carry a resolved name — `EvidenceInsight` is joined off + * `golf_coach_insights` only — so without this map every insight-sourced row + * left `playerName` `undefined` and the "By player" grouping's fallback + * (`r.playerName?.trim() || 'Unknown player'`) collapsed EVERY insight/alert + * into one "Unknown player" bucket, unlike patterns (which resolve the name + * inline via `getTeamPatterns`' own `golf_players` join). A missing map entry + * (e.g. a player removed from the roster) still falls back to `undefined` — + * never a fabricated name. + */ +export function insightToSignalRow( + insight: EvidenceInsight, + playerNames?: Record, +): SignalRow { const categoryLabel = titleCaseToken(insight.category) || 'Signal'; const ev = insight.evidence; return { @@ -286,7 +302,7 @@ export function insightToSignalRow(insight: EvidenceInsight): SignalRow { body: insight.content ?? '', overline: `${categoryLabel} · Signal`, playerId: insight.player_id, - playerName: undefined, + playerName: playerNames?.[insight.player_id] || undefined, category: insight.category, status: insight.status, createdAt: insight.created_at, @@ -443,7 +459,11 @@ export function patternsToSignalRows(patterns: ExtendedPattern[]): SignalRow[] { return patterns.map(patternToSignalRow); } -/** Batch helper: project an array of insights. */ -export function insightsToSignalRows(insights: EvidenceInsight[]): SignalRow[] { - return insights.map(insightToSignalRow); +/** Batch helper: project an array of insights. `playerNames` is forwarded to + * every row — see `insightToSignalRow` for why it's needed. */ +export function insightsToSignalRows( + insights: EvidenceInsight[], + playerNames?: Record, +): SignalRow[] { + return insights.map((insight) => insightToSignalRow(insight, playerNames)); } diff --git a/src/components/golf/coachhelm/coach/LeakBoard.test.tsx b/src/components/golf/coachhelm/coach/LeakBoard.test.tsx new file mode 100644 index 000000000..d5898de45 --- /dev/null +++ b/src/components/golf/coachhelm/coach/LeakBoard.test.tsx @@ -0,0 +1,70 @@ +// @vitest-environment jsdom +/** + * ============================================================================ + * LeakBoard — "str/rd" honesty regression guard + * ---------------------------------------------------------------------------- + * The per-category total is a SUM of `strokes_impact` across every leak + * insight for every player in that category — NOT a per-round rate. It used + * to be labeled "str/rd" (a per-round unit), which read as though a −26.4 + * total meant the team lost 26 strokes EVERY round — ~8x the genuine team + * SG-putting figure (~−3.19/rd). This locks the honest "total" label so a + * regression back to "str/rd" (or an equivalent per-round claim) fails. + * ========================================================================== */ +import { render, screen } from '@testing-library/react'; +import { describe, it, expect } from 'vitest'; +import { LeakBoard, type LeakInsight } from './LeakBoard'; + +function makeLeak(overrides: Partial = {}): LeakInsight { + return { + id: 'leak-1', + category: 'putting', + title: 'Three-putt rate is climbing', + strokesImpact: 1.5, + priority: 'high', + playerName: 'player-1', + ...overrides, + }; +} + +describe('LeakBoard — honest units', () => { + it('labels the summed total "total", never "str/rd" (a per-round claim it cannot back)', () => { + render( + , + ); + expect(screen.getAllByText('total').length).toBeGreaterThan(0); + expect(screen.queryByText('str/rd')).toBeNull(); + }); + + it('sums the magnitude across every leak/player in a category (never averages or divides by round count)', () => { + render( + , + ); + // 2 + 3 = 5.0, not an average (2.5) and not a single player's figure. + expect(screen.getByText('−5.0')).toBeInTheDocument(); + }); + + it('never claims a per-round rate in the summary sentence', () => { + render( + , + ); + const summary = screen.getByText(/leak/i, { selector: 'p' }); + expect(summary.textContent).not.toMatch(/per round|str\/rd/i); + }); + + it('renders the honest empty state when there are no leaks', () => { + render(); + expect(screen.getByText(/No live leaks right now/i)).toBeInTheDocument(); + }); +}); diff --git a/src/components/golf/coachhelm/coach/LeakBoard.tsx b/src/components/golf/coachhelm/coach/LeakBoard.tsx index 3a8f940a5..740ca760a 100644 --- a/src/components/golf/coachhelm/coach/LeakBoard.tsx +++ b/src/components/golf/coachhelm/coach/LeakBoard.tsx @@ -9,12 +9,18 @@ * strokes." A coach with 12 players and 90 seconds needs the latter. * * The LeakBoard regroups live insights by skill category and makes the UNIT of - * every row the summed STROKE-IMPACT (−X.X str/rd), biggest bleed first — so the - * flood of insights collapses into a triage board read in one scan. Each row - * carries its leak count, the players affected, the worst severity, and a - * "high bleed" flag at ≥ 0.8 str/rd. + * every row the SUMMED stroke-impact across every leak in that category — + * biggest bleed first — so the flood of insights collapses into a triage + * board read in one scan. Each row carries its leak count, the players + * affected, the worst severity, and a "high bleed" flag at ≥ `flameThreshold`. * - * Honesty: stroke-impact is the engine's own counterfactual magnitude (already + * Honesty: the total is a SUM across every player and every leak insight in + * the category — NOT a per-round rate. It used to be labeled "str/rd" (a + * per-round unit), which read as if −26.4 meant a team losing 26 strokes + * EVERY round — 8x the genuine team SG-putting figure (~−3.19/rd) computed + * from real rounds. The label now says what the number actually is: a + * cross-player, cross-insight total for the window, never a rate. Stroke- + * impact itself is still the engine's own counterfactual magnitude (already * confidence- and sample-gated upstream); the board sums what the engine * surfaced, never invents a number. Decoupled shape — no server import. * ========================================================================== */ @@ -36,7 +42,8 @@ export interface LeakInsight { export interface LeakBoardProps { insights: LeakInsight[]; - /** str/rd at or above which a category is flagged a high bleed. */ + /** Summed total strokes (across every leak in the category) at or above + * which it's flagged a high bleed. NOT a per-round rate. */ flameThreshold?: number; className?: string; } @@ -109,7 +116,7 @@ export function LeakBoard({ insights, flameThreshold = 0.8, className }: LeakBoa

{insights.length} leak{insights.length !== 1 ? 's' : ''} {totalPlayers > 0 ? <> across {totalPlayers} player{totalPlayers !== 1 ? 's' : ''} : null} — - biggest stroke-bleed first. + total strokes lost this window, biggest bleed first.

@@ -119,9 +126,12 @@ export function LeakBoard({ insights, flameThreshold = 0.8, className }: LeakBoa
{r.label} + {/* "total" (never "str/rd") — this is a SUM across every leak + insight for every player in the category, not a per-round + rate. See the file header for why that distinction matters. */} −{r.total.toFixed(1)} - str/rd + total
From 57c7088ef3915f16af629a934f30bfd9c979d90a Mon Sep 17 00:00:00 2001 From: Fable Integrator Date: Fri, 17 Jul 2026 18:04:37 -0400 Subject: [PATCH 2/2] fix(coachhelm): coach-facing surfaces speak coach voice, not player voice (#915) Player-POV components rendered on coach-facing surfaces, and several color/icon valences were inverted: 1. SG cards (StandingStrip, shared by Team Stats and the player's own stats view) always said "YOU -3.34 ... Below team average / Bottom of your team" regardless of who was reading. Threaded a viewer_context/player_name pair through StandingBar's shared StandingBarProps -> StandingStrip -> FairwayTeamStats/ FairwayStatsCockpit/FairwayPlayerStats: a coach reader now sees the player's name/initials and possessive-free cohort text ("Bottom of team"); the player's own view is unchanged ("You" / "your team"). 2. Mined-pattern copy on the coach-only Patterns tab was player-first-person and grammatically malformed: "When After 5+ days off and In tournament, you tend to score 4.7 strokes worse than average. ... discuss with your coach." Fixed the template concatenation in PatternMiner.generateDescription (new joinConditionLabels helper) so condition labels compose into one natural clause instead of double-conjuncting "When X and Y", and added a toCoachVoice rewrite in the coach-only patternToInsightVocabulary.ts adapter that swaps "you tend to" for the player's name and "discuss with your coach" for "Worth a conversation with ." 3. Pattern card icon/accent was derived from `priority`, a severity tier bucketed by |stroke_impact| magnitude alone (blind to sign) - a high-magnitude plays-BETTER pattern could land in the 'high' tier (warning-orange flame) while a low-magnitude plays-WORSE pattern landed in 'medium' (green sparkle). Added a signed `valence` field to SignalRow and an `iconTone` override to InsightCard/InsightPanel that recolors the icon by direction, independent of the severity tint bar. 4. The Score-by-round trend Ribbon (Team Stats / player stats cockpit) rendered an improving (falling) score as an amber "-7.0" decline because Ribbon never told Readout which raw direction was good for the metric. Added a `goodDirection` prop to Ribbon (reusing the shared classifyTrend classifier) and set it to "down" for the score series. Also fixed an inverted `direction` ternary on FairwayRoundDetail's score-to-par delta found during the same audit (an under-par round rendered the same false amber decline). Gates: tsc --noEmit clean; eslint clean on all changed files; 636 vitest tests passing (0 failing) across the new/updated suites plus adjacent regression suites (FairwayTeamStats, surface-registry, CoachInsightCard). Fixes #915 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg --- .../cards-insight/InsightCard.test.tsx | 95 +++++++++++ .../fairway/cards-insight/InsightCard.tsx | 47 +++++- .../fairway/cards-insight/InsightPanel.tsx | 25 ++- src/components/fairway/charts/Ribbon.test.tsx | 59 +++++++ src/components/fairway/charts/Ribbon.tsx | 28 ++++ .../fairway/charts/StandingStrip.tsx | 10 +- .../coachhelm/FairwayCoachHelmSignals.tsx | 7 + .../pages/coachhelm/FairwayPlayerStats.tsx | 8 +- .../pages/coachhelm/FairwayStatsCockpit.tsx | 48 +++++- .../pages/coachhelm/FairwayTeamStats.tsx | 4 + .../patternToInsightVocabulary.test.ts | 157 ++++++++++++++++++ .../signals/patternToInsightVocabulary.ts | 78 ++++++++- .../pages/rounds/FairwayRoundDetail.tsx | 9 +- .../golf/coachhelm/v3/StandingBar/index.tsx | 3 + .../golf/coachhelm/v3/StandingBar/types.ts | 20 +++ .../golf/coachhelm/v3/StandingBar/utils.ts | 71 +++++++- src/lib/coachhelm/v2/mining/pattern-miner.ts | 37 ++++- .../coachhelm/v2/mining/pattern-miner.test.ts | 112 ++++++++++++- src/test/golf/components/StandingBar.test.tsx | 110 ++++++++++++ 19 files changed, 903 insertions(+), 25 deletions(-) create mode 100644 src/components/fairway/cards-insight/InsightCard.test.tsx create mode 100644 src/components/fairway/pages/coachhelm/signals/patternToInsightVocabulary.test.ts diff --git a/src/components/fairway/cards-insight/InsightCard.test.tsx b/src/components/fairway/cards-insight/InsightCard.test.tsx new file mode 100644 index 000000000..ee17c68b2 --- /dev/null +++ b/src/components/fairway/cards-insight/InsightCard.test.tsx @@ -0,0 +1,95 @@ +// @vitest-environment jsdom +/** + * ============================================================================ + * InsightCard.tsx — `iconTone` valence override (bug #915) + * ---------------------------------------------------------------------------- + * A mined pattern's icon/accent was previously always derived from + * `priority` — a severity tier bucketed by |stroke_impact| MAGNITUDE, blind + * to sign. That let a high-magnitude plays-BETTER pattern land in the 'high' + * tier (a warning-orange flame, as if it were bad news) while a + * low-magnitude plays-WORSE pattern landed in 'medium' (a green sparkle, as + * if it were good news). + * + * These tests lock the fix: `iconTone` overrides BOTH the glyph and its + * wrap color independent of `priority`, so a caller with a genuinely signed + * metric can make icon+accent agree with "good or bad for the player" + * instead of "how large is this number". + * ========================================================================== */ +import { describe, it, expect } from 'vitest'; +import { render } from '@testing-library/react'; +import { InsightCard } from './InsightCard'; + +/** The rendered lead icon's wrap — carries the color classes. */ +function iconWrapClassName(container: HTMLElement): string { + const svg = container.querySelector('svg'); + expect(svg).not.toBeNull(); + const wrap = svg!.parentElement; + expect(wrap).not.toBeNull(); + return wrap!.className; +} + +describe('InsightCard — iconTone valence override', () => { + it('with no iconTone, the icon wrap follows priority (unchanged default behavior)', () => { + const { container } = render( + , + ); + const cls = iconWrapClassName(container); + expect(cls).toContain('text-fw-danger'); + expect(cls).toContain('bg-fw-danger-bg'); + }); + + it('iconTone="positive" renders the success wrap even on a "critical" priority card', () => { + // Regression lock: a HIGH-magnitude plays-better pattern (severity + // 'critical') must still read as good news, not an alarm. + const { container } = render( + , + ); + const cls = iconWrapClassName(container); + expect(cls).toContain('text-fw-success'); + expect(cls).toContain('bg-fw-success-bg'); + expect(cls).not.toContain('bg-fw-danger-bg'); + }); + + it('iconTone="negative" renders the warning wrap even on a "medium"/"low" priority card', () => { + // Regression lock: a LOW-magnitude plays-worse pattern (severity + // 'medium'/'low') must still read as a leak, not a strength. + const { container: medium } = render( + , + ); + expect(iconWrapClassName(medium)).toContain('bg-fw-warning-bg'); + expect(iconWrapClassName(medium)).not.toContain('bg-fw-success-bg'); + + const { container: low } = render( + , + ); + expect(iconWrapClassName(low)).toContain('bg-fw-warning-bg'); + }); + + it('iconTone="neutral" renders the neutral wrap regardless of priority', () => { + const { container } = render( + , + ); + const cls = iconWrapClassName(container); + expect(cls).toContain('bg-surface-sunken'); + expect(cls).not.toContain('bg-fw-warning-bg'); + }); + + it('two cards with the SAME priority tier but OPPOSITE iconTone render visibly different wraps', () => { + // The exact reported shape: same magnitude tier, opposite sign. + const { container: better } = render( + , + ); + const { container: worse } = render( + , + ); + expect(iconWrapClassName(better)).not.toBe(iconWrapClassName(worse)); + }); + + it('undefined iconTone is a true no-op — identical to omitting the prop entirely', () => { + const { container: omitted } = render(); + const { container: explicitUndefined } = render( + , + ); + expect(iconWrapClassName(omitted)).toBe(iconWrapClassName(explicitUndefined)); + }); +}); diff --git a/src/components/fairway/cards-insight/InsightCard.tsx b/src/components/fairway/cards-insight/InsightCard.tsx index d3060a5dd..dd17c05e4 100644 --- a/src/components/fairway/cards-insight/InsightCard.tsx +++ b/src/components/fairway/cards-insight/InsightCard.tsx @@ -44,6 +44,8 @@ import { Sparkles, Info, Lightbulb, + TrendingUp, + TrendingDown, type LucideIcon, } from 'lucide-react'; import { cn } from '@/lib/utils'; @@ -93,6 +95,13 @@ export interface InsightCardProps children?: ReactNode; /** Lead icon override. Defaults to a priority-appropriate icon. */ icon?: ReactNode; + /** + * Recolor the icon wrap (and, unless `icon` overrides the glyph too, the + * default icon) by DIRECTION rather than `priority`'s severity tier (bug + * #915 — see `ICON_TONE`'s docstring). The wrap color always follows + * `iconTone` when set, even alongside a custom `icon`. + */ + iconTone?: InsightIconTone; /** Hide the lead icon entirely. */ hideIcon?: boolean; /** Evidence / supporting detail rendered in a tinted Inset (not on compact). */ @@ -189,6 +198,34 @@ export const PRIORITY: Record = { }, }; +/* -- icon tone (bug #915 — icon/accent by DIRECTION, not category) --------- */ + +/** + * `priority` is a severity/urgency tier — appropriate for most signals + * (a critical alert IS alarming regardless of "direction"). But a mined + * PATTERN's priority is bucketed by |stroke_impact| magnitude alone, blind to + * sign — which let a high-magnitude pattern where the player plays BETTER + * (a strength) land in the 'high' tier (warning-orange flame) while a + * low-magnitude one where they play WORSE (a leak) landed in 'medium' (green + * sparkle): the icon read backwards from what the pattern actually means. + * + * `iconTone`, when supplied, overrides BOTH the default icon AND its + * background/text color independent of `priority` — so a caller with a + * genuinely signed metric (a pattern's stroke_impact) can make the glyph + + * accent agree with "is this good or bad for the player" instead of "how + * large is this number". `priority` keeps driving the tint bar (severity/ + * triage ordering is still a legitimate, separate signal) and the a11y word. + * Omitted (the default) → fully unchanged priority-driven rendering for + * every other InsightCard caller. + */ +export type InsightIconTone = 'positive' | 'negative' | 'neutral'; + +export const ICON_TONE: Record = { + positive: { iconWrap: 'text-fw-success bg-fw-success-bg', icon: TrendingUp }, + negative: { iconWrap: 'text-fw-warning bg-fw-warning-bg', icon: TrendingDown }, + neutral: { iconWrap: 'text-text-secondary bg-surface-sunken', icon: Info }, +}; + /* -- component -------------------------------------------------------------- */ const InsightCardImpl = forwardRef( @@ -200,6 +237,7 @@ const InsightCardImpl = forwardRef( title, children, icon, + iconTone, hideIcon = false, evidence, actions, @@ -220,13 +258,18 @@ const InsightCardImpl = forwardRef( ) { const prefersReduced = useReducedMotion(); const tone = PRIORITY[priority]; + // Bug #915: an explicit iconTone overrides the icon glyph AND its + // background/text color independent of priority — see ICON_TONE's + // docstring. The tint bar below stays priority-driven either way. + const toneOverride = iconTone ? ICON_TONE[iconTone] : null; + const iconWrapClass = toneOverride?.iconWrap ?? tone.iconWrap; const titleId = useId(); const isHero = variant === 'hero'; const isCompact = variant === 'compact'; useHeroGlassFallback(isHero); - const LeadIcon = tone.icon; + const LeadIcon = toneOverride?.icon ?? tone.icon; const leadIcon = icon ?? (hideIcon ? null : ); @@ -373,7 +416,7 @@ const InsightCardImpl = forwardRef( diff --git a/src/components/fairway/cards-insight/InsightPanel.tsx b/src/components/fairway/cards-insight/InsightPanel.tsx index 2b04a5633..8478ddfb2 100644 --- a/src/components/fairway/cards-insight/InsightPanel.tsx +++ b/src/components/fairway/cards-insight/InsightPanel.tsx @@ -54,7 +54,10 @@ import { Button, IconButton } from '../controls/button'; import { InsufficientData } from '../feedback/InsufficientData'; // SHARED vocabulary — the single source of truth for priority tone, imported // (never re-declared) so a signal looks identical scanned (card) vs read (panel). -import { PRIORITY, type InsightPriority } from './InsightCard'; +// ICON_TONE/InsightIconTone is the bug #915 direction-by-sign override (see +// InsightCard's docstring) — shared here for the same "must look identical +// scanned vs read" reason. +import { PRIORITY, ICON_TONE, type InsightPriority, type InsightIconTone } from './InsightCard'; export type InsightPanelMode = 'auto' | 'sheet' | 'docked'; @@ -92,6 +95,13 @@ export interface InsightPanelProps { children?: ReactNode; /** Lead icon override. Defaults to the priority-appropriate icon (shared). */ icon?: ReactNode; + /** + * Override the icon glyph + color by DIRECTION rather than `priority`'s + * severity tier — the SAME bug #915 override InsightCard exposes, kept in + * sync so a pattern's icon reads identically scanned vs read. Ignored when + * `icon` is also passed. + */ + iconTone?: InsightIconTone; /** Hide the lead icon entirely. */ hideIcon?: boolean; /** Right-aligned meta in the header (timestamp, source chip, confidence). */ @@ -179,6 +189,7 @@ interface PanelBodyProps | 'title' | 'children' | 'icon' + | 'iconTone' | 'hideIcon' | 'meta' | 'evidence' @@ -201,6 +212,7 @@ function PanelBody({ title, children, icon, + iconTone, hideIcon = false, meta, evidence, @@ -215,7 +227,11 @@ function PanelBody({ titleId, }: PanelBodyProps) { const tone = PRIORITY[priority]; - const LeadIcon = tone.icon; + // Bug #915: the SAME direction-by-sign override InsightCard exposes — see + // its docstring. `priority` still drives the tint bar below. + const toneOverride = iconTone ? ICON_TONE[iconTone] : null; + const iconWrapClass = toneOverride?.iconWrap ?? tone.iconWrap; + const LeadIcon = toneOverride?.icon ?? tone.icon; const leadIcon = icon ?? (hideIcon ? null : ); @@ -233,7 +249,7 @@ function PanelBody({ {leadIcon} @@ -334,6 +350,7 @@ export const InsightPanel = forwardRef( title, children, icon, + iconTone, hideIcon, meta, evidence, @@ -368,6 +385,7 @@ export const InsightPanel = forwardRef( overline={overline} title={title} icon={icon} + iconTone={iconTone} hideIcon={hideIcon} meta={meta} evidence={evidence} @@ -420,6 +438,7 @@ export const InsightPanel = forwardRef( overline={overline} title={title} icon={icon} + iconTone={iconTone} hideIcon={hideIcon} meta={meta} evidence={evidence} diff --git a/src/components/fairway/charts/Ribbon.test.tsx b/src/components/fairway/charts/Ribbon.test.tsx index 7735460f8..dc91561c7 100644 --- a/src/components/fairway/charts/Ribbon.test.tsx +++ b/src/components/fairway/charts/Ribbon.test.tsx @@ -80,3 +80,62 @@ describe('Ribbon — trend delta never double-signs a signed valueFormatter', () expect(text).not.toMatch(/[+\-−]{2}/); }); }); + +/** + * Bug #915 — the "Score by round" trend delta rendered a falling (improving) + * score as an amber "▼" decline, because Ribbon never told Readout which + * raw direction was GOOD for the plotted metric. `goodDirection` fixes this + * by classifying via the shared `classifyTrend` and passing the resulting + * verdict through as Readout's `direction` override. + */ +describe('Ribbon — goodDirection (score/lower-is-better trend coloring)', () => { + function getDeltaDirection(): string | null { + const delta = document.querySelector('[data-slot="readout-delta"]'); + expect(delta).not.toBeNull(); + return delta!.getAttribute('data-direction'); + } + + it('DEFAULT (goodDirection="up", unchanged): a falling series reads "down" — the old, still-correct behavior for higher-is-better metrics', () => { + render(); + expect(getDeltaDirection()).toBe('down'); + }); + + it('goodDirection="down": the SAME falling series now reads "up" (green) — a lower score is an improvement', () => { + render( + v.toFixed(1)} + seriesName="Score" + goodDirection="down" + />, + ); + expect(getDeltaDirection()).toBe('up'); + }); + + it('goodDirection="down": a RISING series (a worsening score) reads "down" (amber)', () => { + render( + 0.66 */} + valueFormatter={(v) => v.toFixed(1)} + seriesName="Score" + goodDirection="down" + />, + ); + expect(getDeltaDirection()).toBe('down'); + }); + + it('goodDirection="down": a flat series (within the deadzone) reads "flat" regardless', () => { + render( + v.toFixed(1)} + seriesName="Score" + goodDirection="down" + />, + ); + expect(getDeltaDirection()).toBe('flat'); + }); +}); diff --git a/src/components/fairway/charts/Ribbon.tsx b/src/components/fairway/charts/Ribbon.tsx index 22c0a6ba5..35a7c6113 100644 --- a/src/components/fairway/charts/Ribbon.tsx +++ b/src/components/fairway/charts/Ribbon.tsx @@ -27,6 +27,7 @@ import { Readout } from '../instrument/Readout'; import { ChartCrosshairLiveRegion, useChartCrosshair } from './ChartCrosshair'; import { InstrumentTable, InstrumentTableToggle } from './InstrumentTable'; import type { ChartTableData } from './ChartFrame'; +import { classifyTrend, type GoodDirection } from './TrendChip'; import { TABULAR_NUMS, VIZ_CHROME, @@ -63,6 +64,16 @@ export interface RibbonProps { minPoints?: number; /** Force the awaiting (dim) state. */ awaiting?: boolean; + /** + * Which direction of the plotted value is GOOD — feeds the top-right trend + * delta's color + glyph via the shared `classifyTrend` (see TrendChip), + * the SAME classifier Sparkline/StatTile/TrendChip use. Defaults to `'up'` + * (unchanged behavior for higher-is-better series like SG / accuracy / GIR%). + * Golf SCORING is lower-is-better — pass `'down'` so a falling score (an + * improvement) renders the green ▲, not a false amber ▼ (bug #915: the + * Score-by-round trend read a −7.0 improvement as a warning-orange decline). + */ + goodDirection?: GoodDirection; className?: string; } @@ -80,6 +91,7 @@ export function Ribbon({ height = 200, minPoints = 2, awaiting = false, + goodDirection = 'up', className, }: RibbonProps) { const reduced = useReducedMotion() ?? false; @@ -165,6 +177,21 @@ export function Ribbon({ const first = points[0]; const last = points[points.length - 1]; const trendDelta = first && last ? last.y - first.y : undefined; + // Bug #915: classify by IMPROVEMENT (goodDirection-aware), not raw sign — + // the same shared classifier Sparkline/StatTile/TrendChip use. Passed + // through explicitly as Readout's `direction` override so its own + // sign-only fallback (up=green/down=amber from the raw sign) never + // second-guesses a lower-is-better series like Score. + const trendVerdict = + typeof trendDelta === 'number' ? classifyTrend(trendDelta, { goodDirection }) : undefined; + const trendDirection: 'up' | 'down' | 'flat' | undefined = + trendVerdict === undefined + ? undefined + : trendVerdict === 'flat' + ? 'flat' + : trendVerdict === 'improving' + ? 'up' + : 'down'; const ariaLabel = chartAriaLabel( typeof title === 'string' ? title : 'Trend', @@ -194,6 +221,7 @@ export function Ribbon({ typeof trendDelta === 'number' ? { value: trendDelta, + direction: trendDirection, format: (v) => { // `fmt` (the caller's valueFormatter, e.g. FairwayBrief's // fmtSG) may ALREADY prefix its own +/− sign. Strip any diff --git a/src/components/fairway/charts/StandingStrip.tsx b/src/components/fairway/charts/StandingStrip.tsx index 7108694a7..5d210bb02 100644 --- a/src/components/fairway/charts/StandingStrip.tsx +++ b/src/components/fairway/charts/StandingStrip.tsx @@ -35,6 +35,8 @@ import { shouldShowTeamMarker, deriveAriaLabel, pgaReferenceLabel, + neutralizeForCoach, + standingSubjectLabel, } from '@/components/golf/coachhelm/v3/StandingBar'; /** StandingStrip shares the legacy StandingBar prop surface verbatim. */ @@ -62,8 +64,12 @@ export function StandingStrip(props: StandingStripProps) { // marker uses (`showTeam`), so we never narrate a percentile we won't draw. const cohortText = props.show_cohort_text !== false && showTeam - ? teamCohortText(props.team_pct, props.team_n) + ? neutralizeForCoach(teamCohortText(props.team_pct, props.team_n), props.viewer_context) : ''; + // Bug #915: the hero marker/readout is labeled "You" for a player's own + // view, but a coach reading a teammate's card sees the player's initials + // instead — the coach is never "you" to the player being read. + const heroLabel = standingSubjectLabel(props.viewer_context, props.player_name); // CF-3: SG metrics anchor to the field average (0), not a PGA Tour score. // Women's teams get "LPGA" instead of "PGA" for non-SG metrics. const refLabel = pgaReferenceLabel(props.metric_id, props.is_womens).short; @@ -117,7 +123,7 @@ export function StandingStrip(props: StandingStripProps) { {/* High-contrast 3-up readouts (You is the green hero figure) */}
- + {showTeam && props.team_avg !== null ? ( ) : ( diff --git a/src/components/fairway/pages/coachhelm/FairwayCoachHelmSignals.tsx b/src/components/fairway/pages/coachhelm/FairwayCoachHelmSignals.tsx index f437ea3bd..cb89ce65a 100644 --- a/src/components/fairway/pages/coachhelm/FairwayCoachHelmSignals.tsx +++ b/src/components/fairway/pages/coachhelm/FairwayCoachHelmSignals.tsx @@ -1355,6 +1355,10 @@ export function FairwayCoachHelmSignals({ key={row.id} variant={opts?.hero ? 'hero' : opts?.compact ? 'compact' : 'default'} priority={row.priority} + // Bug #915: a pattern's icon/accent follows its SIGNED stroke_impact + // (row.valence), not the priority severity tier — undefined for + // insight rows, which fall back to the unchanged priority-driven icon. + iconTone={row.valence} overline={row.overline} title={row.title} evidence={opts?.compact ? undefined : evidenceNode} @@ -1983,6 +1987,9 @@ export function FairwayCoachHelmSignals({ if (!o) setOpenRowId(null); }} priority={openRow.priority} + // Bug #915 — same direction-by-sign override as the card (see + // renderCard above) so the opened panel matches what was scanned. + iconTone={openRow.valence} overline={openRow.overline} title={openRow.title} meta={openRow.confidenceWord ?? undefined} diff --git a/src/components/fairway/pages/coachhelm/FairwayPlayerStats.tsx b/src/components/fairway/pages/coachhelm/FairwayPlayerStats.tsx index fe13d308f..30a63a8f8 100644 --- a/src/components/fairway/pages/coachhelm/FairwayPlayerStats.tsx +++ b/src/components/fairway/pages/coachhelm/FairwayPlayerStats.tsx @@ -121,7 +121,13 @@ export function FairwayPlayerStats({ initialPlayerId = null }: FairwayPlayerStat /> ) : ( - + )}
diff --git a/src/components/fairway/pages/coachhelm/FairwayStatsCockpit.tsx b/src/components/fairway/pages/coachhelm/FairwayStatsCockpit.tsx index eb90f88ff..c9a24671c 100644 --- a/src/components/fairway/pages/coachhelm/FairwayStatsCockpit.tsx +++ b/src/components/fairway/pages/coachhelm/FairwayStatsCockpit.tsx @@ -203,8 +203,20 @@ export interface FairwayStatsCockpitProps { * route). Drives the cold-start CTA — only the player can log a round, so a * coach drill-down (which omits this) gets no player-only action. Defaults * to false so the roster profile path stays CTA-free. + * + * ALSO drives the SG cards' audience voice (bug #915): `false` (the coach + * drill-down default) renders the player's name/initials instead of "You" + * and drops the "your team" possessive from the cohort caption. */ isOwnStats?: boolean; + /** + * The player's display name — only used when `isOwnStats` is false, to + * label the coach-facing SG card hero marker (see `isOwnStats`). The + * cockpit itself has no identity lookup (see the file header note), so the + * caller resolves + passes this. Falls back to a generic "Player" label + * when omitted. + */ + playerName?: string; } /* ─────────────────────────────────────────────────────────────────────────── @@ -443,7 +455,17 @@ function LeakLoadError({ onRetry, retrying }: { onRetry: () => void; retrying: b * Component * ────────────────────────────────────────────────────────────────────────── */ -export function FairwayStatsCockpit({ playerId, className, isOwnStats = false }: FairwayStatsCockpitProps) { +export function FairwayStatsCockpit({ + playerId, + className, + isOwnStats = false, + playerName, +}: FairwayStatsCockpitProps) { + // Bug #915: the SG cards render in the player's own voice ("You" / "your + // team") by default. `isOwnStats` already tells us whether the viewer IS + // that player — reuse it as the StandingStrip audience signal rather than + // introducing a second flag callers have to keep in sync. + const standingViewerContext = isOwnStats ? 'self' : 'coach'; const [detailedStats, setDetailedStats] = useState(null); const [trendData, setTrendData] = useState(null); const [standingRows, setStandingRows] = useState(null); @@ -766,6 +788,8 @@ export function FairwayStatsCockpit({ playerId, className, isOwnStats = false }: sgTotal={sgTotal} detailedStats={detailedStats} gainLeak={gainLeak} + standingViewerContext={standingViewerContext} + playerName={playerName} headerAction={
@@ -944,6 +972,8 @@ export function FairwayStatsCockpit({ playerId, className, isOwnStats = false }: unit={cfg.unit} scale={cfg.default_scale} size="card" + viewer_context={standingViewerContext} + player_name={playerName} /> ))}
@@ -996,6 +1026,8 @@ export function FairwayStatsCockpit({ playerId, className, isOwnStats = false }: matrixByCategory={matrixByCategory} open={showDetailed} onToggle={() => setShowDetailed((v) => !v)} + standingViewerContext={standingViewerContext} + playerName={playerName} /> ) : ( ; matrixByCategory: Map>; open: boolean; onToggle: () => void; + /** Bug #915 — 'self' shows "You" on each SG card, 'coach' shows the player's name/initials. */ + standingViewerContext: 'self' | 'coach'; + playerName?: string; }) { if (detailedGroups.length === 0) return null; return ( @@ -2101,6 +2145,8 @@ function DetailedStandingsSection({ unit={cfg.unit} scale={cfg.default_scale} size="card" + viewer_context={standingViewerContext} + player_name={playerName} /> ))}
diff --git a/src/components/fairway/pages/coachhelm/FairwayTeamStats.tsx b/src/components/fairway/pages/coachhelm/FairwayTeamStats.tsx index c2323ddfa..9b4103c49 100644 --- a/src/components/fairway/pages/coachhelm/FairwayTeamStats.tsx +++ b/src/components/fairway/pages/coachhelm/FairwayTeamStats.tsx @@ -1213,6 +1213,10 @@ function PlayerTile({ unit={renderCfg.unit} scale={renderCfg.default_scale} size="card" + // Bug #915: Team Stats is coach-only — the coach reading this card + // is never the player it describes. + viewer_context="coach" + player_name={fullName} /> ) : ( { + it('rewrites "you tend to" to the player\'s name, third person', () => { + const text = + 'After 5+ days off in tournament rounds, you tend to score 4.7 strokes worse than average.'; + expect(toCoachVoice(text, 'Ethan Rodriguez')).toBe( + 'After 5+ days off in tournament rounds, Ethan Rodriguez tends to score 4.7 strokes worse than average.', + ); + }); + + it('falls back to "the player" when no name is available — never fabricates one', () => { + const text = 'After 7+ days off, you tend to score 2.1 strokes worse than average.'; + expect(toCoachVoice(text, undefined)).toBe( + 'After 7+ days off, the player tends to score 2.1 strokes worse than average.', + ); + expect(toCoachVoice(text, null)).toContain('the player tends to'); + expect(toCoachVoice(text, ' ')).toContain('the player tends to'); + }); + + it('replaces the generic "discuss with your coach" recommendation with a coach-appropriate handoff', () => { + expect(toCoachVoice('Monitor this pattern and discuss with your coach.', 'Ethan')).toBe( + 'Worth a conversation with Ethan.', + ); + }); + + it('leaves the OTHER (already-neutral) recommendation branch untouched', () => { + const text = 'Consider a practice round before important events after extended breaks.'; + expect(toCoachVoice(text, 'Ethan')).toBe(text); + }); + + it('is a no-op for already-third-person text (team-pattern-generator.ts rows)', () => { + const text = "Ethan's short game (2.1 SG:ARG) is 1.4 strokes below team average (0.7)"; + expect(toCoachVoice(text, 'Ethan')).toBe(text); + }); + + it('passes through empty text unchanged', () => { + expect(toCoachVoice('', 'Ethan')).toBe(''); + }); +}); + +// --------------------------------------------------------------------------- +// patternToSignalRow — title/body coach-voiced, valence signed +// --------------------------------------------------------------------------- + +function makePattern(overrides: Partial = {}): ExtendedPattern { + return { + id: 'pattern-1', + playerId: 'player-1', + patternType: 'compound', + conditions: [ + { field: 'days_since_last', operator: 'gte', value: 5, label: 'After 5+ days off' }, + { field: 'round_type', operator: 'eq', value: 'tournament', label: 'In tournament' }, + ], + outcome: { metric: 'score_to_par', direction: 'increase', magnitude: 4.7, comparison: 'vs_baseline' }, + support: 0.2, + confidence: 0.8, + lift: 2.1, + conviction: 3, + strokeImpact: 4.7, + actionability: 0.6, + sampleSize: 8, + firstDetected: '2026-01-01T00:00:00.000Z', + lastOccurrence: '2026-01-10T00:00:00.000Z', + occurrenceCount: 8, + trend: 'stable', + isActive: true, + lifecycleState: 'detected', + severity: 'high', + description: + 'After 5+ days off in tournament rounds, you tend to score 4.7 strokes worse than average.', + recommendation: 'Monitor this pattern and discuss with your coach.', + playerName: 'Ethan Rodriguez', + ...overrides, + }; +} + +describe('patternToSignalRow — coach voice', () => { + it('title is third-person with the player\'s name, no "When X and Y" double-conjunction', () => { + const row = patternToSignalRow(makePattern()); + expect(row.title).toBe( + 'After 5+ days off in tournament rounds, Ethan Rodriguez tends to score 4.7 strokes worse than average.', + ); + expect(row.title).not.toMatch(/\byou\b/i); + expect(row.title).not.toMatch(/^When /); + }); + + it('body drops "discuss with your coach" for the coach-facing handoff', () => { + const row = patternToSignalRow(makePattern()); + expect(row.body).toBe('Worth a conversation with Ethan Rodriguez.'); + expect(row.body).not.toContain('your coach'); + }); +}); + +describe('patternToSignalRow — valence (bug #915 icon/accent inversion)', () => { + it('a plays-BETTER pattern (positive stroke_impact) gets positive valence, even in a "high" priority tier', () => { + // magnitude 2.6 -> priority 'critical' by the |impact| tiers, but the + // SIGN is positive (the player plays better) — valence must say so. + const row = patternToSignalRow(makePattern({ strokeImpact: 2.6 })); + expect(row.valence).toBe('positive'); + expect(row.priority).toBe('critical'); + }); + + it('a plays-WORSE pattern (negative stroke_impact) gets negative valence, even in a "medium" priority tier', () => { + // magnitude 0.9 -> priority 'medium' by the |impact| tiers; sign is + // negative (a leak) — the OLD bug rendered this with the SAME green + // tone as a positive pattern in the same magnitude tier. + const row = patternToSignalRow(makePattern({ strokeImpact: -0.9 })); + expect(row.valence).toBe('negative'); + expect(row.priority).toBe('medium'); + }); + + it('zero / missing stroke_impact is neutral, never fabricated', () => { + expect(patternToSignalRow(makePattern({ strokeImpact: 0 })).valence).toBe('neutral'); + expect( + patternToSignalRow(makePattern({ strokeImpact: null as unknown as number })).valence, + ).toBe('neutral'); + }); + + it('valence and priority disagree exactly in the reported scenario (small positive vs small negative)', () => { + // Regression lock for the reported bug: a small-magnitude positive + // pattern and a small-magnitude negative pattern land in the SAME + // priority tier ('low') but must carry OPPOSITE valence. + const better = patternToSignalRow(makePattern({ strokeImpact: 0.4 })); + const worse = patternToSignalRow(makePattern({ strokeImpact: -0.4 })); + expect(better.priority).toBe(worse.priority); + expect(better.valence).toBe('positive'); + expect(worse.valence).toBe('negative'); + expect(better.valence).not.toBe(worse.valence); + }); +}); diff --git a/src/components/fairway/pages/coachhelm/signals/patternToInsightVocabulary.ts b/src/components/fairway/pages/coachhelm/signals/patternToInsightVocabulary.ts index 5e0b86ca4..4492d0061 100644 --- a/src/components/fairway/pages/coachhelm/signals/patternToInsightVocabulary.ts +++ b/src/components/fairway/pages/coachhelm/signals/patternToInsightVocabulary.ts @@ -91,6 +91,17 @@ export interface SignalRow { * tiebreak (magnitude-desc). Undefined for insights. Additive/optional. */ strokeImpact?: number | null; + /** + * Bug #915 — the pattern icon/accent must derive from the SIGNED + * stroke_impact (a plays-better pattern is 'positive', plays-worse is + * 'negative'), not from the severity/priority tier alone. `priority` + * buckets by |impact| magnitude regardless of sign, which let a + * high-magnitude POSITIVE pattern land in the 'high' tier (warning-orange + * flame) and a low-magnitude NEGATIVE one land in 'medium' (green + * sparkle) — backwards. Set ONLY by `patternToSignalRow`; insights leave + * it undefined (their priority-only tone is unaffected by this bug). + */ + valence?: 'positive' | 'negative' | 'neutral'; /** The original row, kept so action handlers can read whatever they need. */ raw: EvidenceInsight | ExtendedPattern; } @@ -167,6 +178,22 @@ function derivePatternPriority( return impactToPriority(strokeImpact); } +/** + * Bug #915 — the pattern card's icon/accent must derive from the SIGNED + * stroke_impact, not from `priority` (a severity tier keyed off |impact|, + * blind to sign). Positive impact = the player plays BETTER under this + * condition (a strength — green); negative = plays WORSE (a leak — amber). + * Zero/missing stays neutral rather than fabricating a direction. + */ +function derivePatternValence( + strokeImpact: number | null | undefined, +): NonNullable { + if (typeof strokeImpact !== 'number' || !Number.isFinite(strokeImpact) || strokeImpact === 0) { + return 'neutral'; + } + return strokeImpact > 0 ? 'positive' : 'negative'; +} + /* ─────────────────────────────────────────────────────────────────────────── * Confidence translation — the Patterns mustFix "confidence translation". * A coach should read a WORD, never a bare 0.83. @@ -301,18 +328,56 @@ export function insightToSignalRow(insight: EvidenceInsight): SignalRow { }; } +/* ─────────────────────────────────────────────────────────────────────────── + * Coach voice (bug #915) — `/dashboard/patterns` is coach-only (guarded in + * page.tsx: `if (!coach) return `), but + * `PatternMiner.generateDescription`/`generateRecommendation` + * (pattern-miner.ts) write PLAYER-first-person text ("you tend to score…", + * "discuss with your coach") because that engine has no reader-audience + * concept — it just narrates the pattern. Team-authored patterns + * (team-pattern-generator.ts) already write third-person with the player's + * name baked in, so `toCoachVoice` is a no-op for them (no "you tend to" / + * generic-fallback substring to match). This is the seam where both sources + * land in ONE coach-appropriate voice before rendering. + * ─────────────────────────────────────────────────────────────────────────── */ + +/** The exact generic fallback `generateRecommendation` emits (pattern-miner.ts). */ +const GENERIC_COACH_DISCUSSION_RECOMMENDATION = 'Monitor this pattern and discuss with your coach.'; + +/** + * Rewrite a pattern's player-voiced narrative into the coach's third-person + * voice. Falls back to "the player" when no name is available — never + * fabricates one. + */ +export function toCoachVoice(text: string, playerName: string | null | undefined): string { + if (!text) return text; + const name = playerName?.trim() || 'the player'; + // The one player-addressed recommendation the engine emits: "discuss with + // your coach" makes no sense read BY the coach — swap it for the task's + // own suggested phrasing rather than let a generic "your"->possessive + // regex mangle it into "discuss with Ethan's coach". + if (text.trim() === GENERIC_COACH_DISCUSSION_RECOMMENDATION) { + return `Worth a conversation with ${name}.`; + } + // The ONE first-person fragment `generateDescription` produces — every + // conditional/compound/anomaly pattern description ends "…you tend to + // score N strokes worse/better than average." + return text.replace(/\byou tend to\b/gi, `${name} tends to`); +} + /* ─────────────────────────────────────────────────────────────────────────── * PATTERN → SignalRow (the headline rewrite + statistician demotion) * ─────────────────────────────────────────────────────────────────────────── */ /** * Build the plain-language "so what" headline for a pattern. Prefers the - * engine's own description; otherwise composes a coach-readable sentence from - * the (present) condition labels + outcome — never invents numbers. + * engine's own description (coach-voiced via `toCoachVoice`); otherwise + * composes a coach-readable sentence from the (present) condition labels + + * outcome — never invents numbers. */ function patternHeadline(pattern: ExtendedPattern): string { if (pattern.description && pattern.description.trim().length > 0) { - return pattern.description.trim(); + return toCoachVoice(pattern.description.trim(), pattern.playerName); } const player = pattern.playerName ? `${pattern.playerName}: ` : ''; const conditionLabel = @@ -327,15 +392,15 @@ function patternHeadline(pattern: ExtendedPattern): string { ? 'drops' : 'shifts'; if (conditionLabel && outcomeMetric) { - return `${player}When ${conditionLabel.toLowerCase()}, ${outcomeMetric.toLowerCase()} ${dir}`; + return `${player}${conditionLabel}, ${outcomeMetric.toLowerCase()} ${dir}`; } return `${player}${titleCaseToken(pattern.patternType)} pattern detected`; } -/** The body sentence — the recommendation if present, else a quiet fallback. */ +/** The body sentence — the recommendation if present (coach-voiced), else a quiet fallback. */ function patternBody(pattern: ExtendedPattern): string { if (pattern.recommendation && pattern.recommendation.trim().length > 0) { - return pattern.recommendation.trim(); + return toCoachVoice(pattern.recommendation.trim(), pattern.playerName); } // Preserve the SIGN of stroke_impact: a negative value is strokes LOST // (harmful), a positive value is strokes GAINED (helpful). Math.abs would @@ -434,6 +499,7 @@ export function patternToSignalRow(pattern: ExtendedPattern): SignalRow { // > temporal > contextual, then magnitude). Insights never set these. patternTypeRank: patternTypeRank(pattern.patternType), strokeImpact: typeof pattern.strokeImpact === 'number' ? pattern.strokeImpact : null, + valence: derivePatternValence(pattern.strokeImpact), raw: pattern, }; } diff --git a/src/components/fairway/pages/rounds/FairwayRoundDetail.tsx b/src/components/fairway/pages/rounds/FairwayRoundDetail.tsx index a27fede5d..209f060ac 100644 --- a/src/components/fairway/pages/rounds/FairwayRoundDetail.tsx +++ b/src/components/fairway/pages/rounds/FairwayRoundDetail.tsx @@ -335,8 +335,13 @@ export function FairwayRoundDetail({ scoreToPar != null ? { value: scoreToPar, - // golf is lower-is-better: under par is the good direction - direction: scoreToPar < 0 ? 'down' : scoreToPar > 0 ? 'up' : 'flat', + // Bug #915: Readout's `direction` is the VERDICT + // ('up' = green/good, 'down' = amber/bad), not the raw + // numeric sign — golf is lower-is-better, so under par + // (scoreToPar < 0) is the GOOD ('up') direction. The + // previous `scoreToPar < 0 ? 'down' : ...` inverted + // this: a great under-par round rendered amber ▼. + direction: scoreToPar < 0 ? 'up' : scoreToPar > 0 ? 'down' : 'flat', format: () => `${formatToPar(scoreToPar)} vs par`, } : undefined diff --git a/src/components/golf/coachhelm/v3/StandingBar/index.tsx b/src/components/golf/coachhelm/v3/StandingBar/index.tsx index 7459e50cf..0c36b440d 100644 --- a/src/components/golf/coachhelm/v3/StandingBar/index.tsx +++ b/src/components/golf/coachhelm/v3/StandingBar/index.tsx @@ -56,4 +56,7 @@ export { shouldShowTeamMarker, deriveAriaLabel, pgaReferenceLabel, + neutralizeForCoach, + initialsFromName, + standingSubjectLabel, } from './utils'; diff --git a/src/components/golf/coachhelm/v3/StandingBar/types.ts b/src/components/golf/coachhelm/v3/StandingBar/types.ts index a2895303e..cca88206a 100644 --- a/src/components/golf/coachhelm/v3/StandingBar/types.ts +++ b/src/components/golf/coachhelm/v3/StandingBar/types.ts @@ -94,6 +94,26 @@ export interface StandingBarProps { /** Used when state === 'error'. */ errorMessage?: string; + + /** + * Who is reading this card. `'self'` (default) is the player viewing their + * own stats — "You" + "your team" phrasing. `'coach'` is a coach viewing a + * teammate's card — the player is not "you" to this reader, so the hero + * marker/readout labels with `player_name` (or a neutral fallback) instead, + * and cohort captions drop the "your" possessive ("Bottom of team", not + * "Bottom of your team"). Bug #915 — Team Stats showed "YOU −3.34 … Below + * team average" to the coach reading it. + */ + viewer_context?: 'self' | 'coach'; + + /** + * The player's display name, used for the coach-facing hero label when + * `viewer_context === 'coach'`. Callers may pass a full name or first name; + * the card derives short initials from it for the tight 3-up readout row + * and uses the name verbatim in the aria label. Falls back to "Player" when + * omitted. Ignored when `viewer_context !== 'coach'`. + */ + player_name?: string; } /** Threshold below which the team marker is omitted (Part VII.3 cold-start rule). */ diff --git a/src/components/golf/coachhelm/v3/StandingBar/utils.ts b/src/components/golf/coachhelm/v3/StandingBar/utils.ts index d3cd24808..2c218c5fa 100644 --- a/src/components/golf/coachhelm/v3/StandingBar/utils.ts +++ b/src/components/golf/coachhelm/v3/StandingBar/utils.ts @@ -155,6 +155,63 @@ export function teamRelativeText( return better ? 'Above team average' : 'Below team average'; } +/* ─────────────────────────────────────────────────────────────────────────── + * Audience voice — bug #915: a coach reading a player's SG card saw + * player-first-person copy ("YOU −3.34 … Below team average / Bottom of your + * team") even though the coach, not the player, is the reader. `teamCohortText` + * / `teamRelativeText` above are written in the player's own voice ("your + * team") because that's the common case (a player viewing their own stats); + * these two helpers let a `viewer_context: 'coach'` caller neutralize that + * possessive and swap the "You" subject for the player's name. + * ─────────────────────────────────────────────────────────────────────────── */ + +/** + * Strip the player-possessive "your" from a cohort/relative sentence for a + * coach reader. `teamCohortText`/`teamRelativeText` only ever emit "your + * team" (never "my team" or other possessives), so a single case-insensitive + * replacement covers every sentence shape both functions produce: + * "Bottom of your team" -> "Bottom of team" + * "Top 18% on your team" -> "Top 18% on team" + * "About your team average" -> "About team average" + * "Above/Below team average" already carry no possessive and pass through + * unchanged. No-op for `viewer_context !== 'coach'` (or an empty string). + */ +export function neutralizeForCoach(text: string, viewerContext?: 'self' | 'coach'): string { + if (viewerContext !== 'coach' || !text) return text; + return text.replace(/\byour team\b/gi, 'team'); +} + +/** + * Short initials from a display name for the tight coach-facing readout + * label (mirrors the "T"/"P" single/double-letter marker convention already + * used on the bar). "Ethan Rodriguez" -> "ER"; a single-word name takes its + * first two letters ("Ethan" -> "ET"). Falls back to "PL" when no usable name + * is given — never fabricates a real player's initials. + */ +export function initialsFromName(name: string | null | undefined): string { + const trimmed = name?.trim(); + if (!trimmed) return 'PL'; + const parts = trimmed.split(/\s+/).filter(Boolean); + const first = parts[0]; + if (!first) return 'PL'; + if (parts.length === 1) return first.slice(0, 2).toUpperCase(); + const last = parts[parts.length - 1]!; + return `${first[0]}${last[0]}`.toUpperCase(); +} + +/** + * The subject label for the hero marker/readout: "You" for the player's own + * view (default), or the player's initials for a coach reader. Full name is + * reserved for the spoken aria label (`deriveAriaLabel`) — initials keep the + * visual 3-up row ("You"/"Team"/"PGA"-width) from overflowing. + */ +export function standingSubjectLabel( + viewerContext: 'self' | 'coach' | undefined, + playerName: string | null | undefined, +): string { + return viewerContext === 'coach' ? initialsFromName(playerName) : 'You'; +} + /** * Derive the auto state from the props when one isn't passed explicitly. * 'error' and 'empty' must be passed in; this function returns either @@ -174,11 +231,19 @@ export function shouldShowTeamMarker(props: Pick" label ("In tournament" / "In qualifier") as + * "in rounds" so it reads as a continuation of the first clause + * instead of a second "When"-less fragment bolted on with "and": + * "After 5+ days off in tournament rounds, you tend to score…" + * Any other trailing label falls back to a plain lowercase-led "and" join. + * A single condition is returned verbatim (no "When" needed either way). + */ +export function joinConditionLabels(conditions: PatternCondition[]): string { + const labels = conditions + .map((c) => c.label || `${c.field} ${c.operator} ${String(c.value)}`) + .filter((label) => label.length > 0); + if (labels.length === 0) return 'Under these conditions'; + const [first, ...rest] = labels as [string, ...string[]]; + if (rest.length === 0) return first; + const recast = rest.map((label) => { + const roundType = /^In (.+)$/.exec(label); + if (roundType) return `in ${roundType[1]!.toLowerCase()} rounds`; + return `and ${label.charAt(0).toLowerCase()}${label.slice(1)}`; + }); + return [first, ...recast].join(' '); +} + const THRESHOLDS = { minSupport: 0.05, // 5% of rounds — loosened from 0.08 so 11-round players aren't starved minConfidence: 0.55, // 55% confidence — kept as-is to avoid false positives @@ -772,14 +803,12 @@ export class PatternMiner { _outcome: PatternOutcome, strokeImpact: number ): string { - const conditionText = conditions - .map((c) => c.label || `${c.field} ${c.operator} ${c.value}`) - .join(' and '); + const conditionText = joinConditionLabels(conditions); const direction = strokeImpact > 0 ? 'worse' : 'better'; const impact = Math.abs(strokeImpact).toFixed(1); - return `When ${conditionText}, you tend to score ${impact} strokes ${direction} than average.`; + return `${conditionText}, you tend to score ${impact} strokes ${direction} than average.`; } /** diff --git a/src/test/coachhelm/v2/mining/pattern-miner.test.ts b/src/test/coachhelm/v2/mining/pattern-miner.test.ts index fd66eef18..c1d961833 100644 --- a/src/test/coachhelm/v2/mining/pattern-miner.test.ts +++ b/src/test/coachhelm/v2/mining/pattern-miner.test.ts @@ -2,9 +2,75 @@ import { describe, it, expect, vi } from 'vitest'; import { computeConvictionSafe, effectiveMinSampleSize, + joinConditionLabels, PatternMiner, } from '@/lib/coachhelm/v2/mining/pattern-miner'; -import type { MinedPattern } from '@/lib/coachhelm/v2/types'; +import type { MinedPattern, PatternCondition } from '@/lib/coachhelm/v2/types'; + +// --------------------------------------------------------------------------- +// joinConditionLabels — bug #915 template concatenation grammar +// --------------------------------------------------------------------------- + +const AFTER_5_DAYS: PatternCondition = { + field: 'days_since_last', + operator: 'gte', + value: 5, + label: 'After 5+ days off', +}; +const IN_TOURNAMENT: PatternCondition = { + field: 'round_type', + operator: 'eq', + value: 'tournament', + label: 'In tournament', +}; + +describe('joinConditionLabels', () => { + it('the reported compound pair reads as one clause, no "When X and Y" double-conjunction', () => { + // Regression lock for the exact reported bug: "When After 5+ days off + // and In tournament, …" -> "After 5+ days off in tournament rounds, …" + expect(joinConditionLabels([AFTER_5_DAYS, IN_TOURNAMENT])).toBe( + 'After 5+ days off in tournament rounds', + ); + }); + + it('a single condition is returned verbatim — no "When" prefix needed', () => { + expect(joinConditionLabels([AFTER_5_DAYS])).toBe('After 5+ days off'); + expect(joinConditionLabels([IN_TOURNAMENT])).toBe('In tournament'); + }); + + it('recasts a trailing "In " label as "in rounds" for ANY leading condition', () => { + const highPutts: PatternCondition = { + field: 'putts', + operator: 'gte', + value: 36, + label: 'High putts (36+)', + }; + expect(joinConditionLabels([highPutts, IN_TOURNAMENT])).toBe( + 'High putts (36+) in tournament rounds', + ); + }); + + it('falls back to a lowercase-led "and" join for a non-round-type trailing label', () => { + const backToBack: PatternCondition = { + field: 'days_since_last', + operator: 'lte', + value: 1, + label: 'Back-to-back rounds', + }; + expect(joinConditionLabels([AFTER_5_DAYS, backToBack])).toBe( + 'After 5+ days off and back-to-back rounds', + ); + }); + + it('falls back to field/operator/value when a condition carries no label', () => { + const noLabel = { field: 'putts', operator: 'gte', value: 36 } as PatternCondition; + expect(joinConditionLabels([noLabel])).toBe('putts gte 36'); + }); + + it('never crashes on an empty condition list', () => { + expect(joinConditionLabels([])).toBe('Under these conditions'); + }); +}); describe('effectiveMinSampleSize (threshold scaling for low-round players)', () => { // TODO(plan-03): un-skip when Plan 03 (CoachHelm evidence contract) finalizes @@ -155,6 +221,50 @@ describe('PatternMiner.toRow (Task B13 lifecycle metadata)', () => { }); }); +describe('PatternMiner.generateDescription (bug #915 grammar fix, end-to-end)', () => { + type Miner = { + generateDescription: ( + conditions: PatternCondition[], + outcome: unknown, + strokeImpact: number, + ) => string; + generateRecommendation: (conditions: PatternCondition[], outcome: unknown) => string; + }; + + it('the reported compound pattern renders the fixed, single-clause sentence', () => { + const miner = new PatternMiner('player-1') as unknown as Miner; + const description = miner.generateDescription( + [AFTER_5_DAYS, IN_TOURNAMENT], + {}, + 4.7, + ); + expect(description).toBe( + 'After 5+ days off in tournament rounds, you tend to score 4.7 strokes worse than average.', + ); + expect(description).not.toMatch(/^When /); + expect(description).not.toContain(' and In '); + }); + + it('a positive stroke_impact reads "better", never "worse"', () => { + const miner = new PatternMiner('player-1') as unknown as Miner; + const description = miner.generateDescription([AFTER_5_DAYS], {}, -1.8); + expect(description).toBe('After 5+ days off, you tend to score 1.8 strokes better than average.'); + }); + + it('generateRecommendation is unaffected by the grammar fix (still player-voiced; the coach rewrite lives in patternToInsightVocabulary.ts)', () => { + const miner = new PatternMiner('player-1') as unknown as Miner; + expect(miner.generateRecommendation([AFTER_5_DAYS], {})).toBe( + 'Monitor this pattern and discuss with your coach.', + ); + expect( + miner.generateRecommendation( + [{ field: 'days_since_last', operator: 'gte', value: 7, label: 'After 7+ days off' }], + {}, + ), + ).toBe('Consider a practice round before important events after extended breaks.'); + }); +}); + describe('PatternMiner.savePatterns (Task B14 partial success)', () => { it('keeps writing after a row upsert returns an error', async () => { const upsertCalls: number[] = []; diff --git a/src/test/golf/components/StandingBar.test.tsx b/src/test/golf/components/StandingBar.test.tsx index 0dcc657cc..d5dd4490a 100644 --- a/src/test/golf/components/StandingBar.test.tsx +++ b/src/test/golf/components/StandingBar.test.tsx @@ -26,6 +26,9 @@ import { shouldShowTeamMarker, deriveAriaLabel, pgaReferenceLabel, + neutralizeForCoach, + initialsFromName, + standingSubjectLabel, } from '@/components/golf/coachhelm/v3/StandingBar'; // --------------------------------------------------------------------------- @@ -214,6 +217,113 @@ describe('deriveAriaLabel', () => { expect(label).toContain('Field average:'); expect(label).not.toContain('PGA Tour:'); }); + + // Bug #915 — coach reading a teammate's card must never hear "You". + it('coach viewer_context: speaks the player name, not "You", and drops "your team"', () => { + const props: StandingBarProps = { + metric_id: 'sg_total', + metric_label: 'SG: Total', + player_value: -3.34, + team_avg: 0.1, + team_n: 8, + team_pct: 5, + pga_value: 0, + direction: 'higher_better', + unit: 'strokes', + scale: { min: -4, max: 4 }, + size: 'card', + viewer_context: 'coach', + player_name: 'Ethan Rodriguez', + }; + const label = deriveAriaLabel(props); + expect(label).toContain('Ethan Rodriguez: -3.34'); + expect(label).not.toMatch(/\bYou\b/); + expect(label).not.toContain('your team'); + }); + + it('coach viewer_context without a player_name falls back to "Player", never "You"', () => { + const props: StandingBarProps = { + metric_id: 'sg_total', metric_label: 'SG: Total', + player_value: 0.5, team_avg: null, pga_value: 0, + direction: 'higher_better', unit: 'strokes', + scale: { min: -1, max: 1 }, size: 'card', + viewer_context: 'coach', + }; + const label = deriveAriaLabel(props); + expect(label).toContain('Player: 0.50'); + expect(label).not.toMatch(/\bYou\b/); + }); + + it('self viewer_context (default) is unchanged: "You" + "your team" language', () => { + const props: StandingBarProps = { + metric_id: 'sg_total', + metric_label: 'SG: Total', + player_value: -3.34, + team_avg: 0.1, + team_n: 8, + pga_value: 0, + direction: 'higher_better', + unit: 'strokes', + scale: { min: -4, max: 4 }, + size: 'card', + }; + const label = deriveAriaLabel(props); + expect(label).toContain('You: -3.34'); + }); +}); + +// --------------------------------------------------------------------------- +// Audience voice — bug #915 (StandingStrip's "YOU −3.34 … Below team +// average / Bottom of your team" shown to a coach reading a player's card) +// --------------------------------------------------------------------------- + +describe('neutralizeForCoach', () => { + it('strips "your team" for a coach viewer', () => { + expect(neutralizeForCoach('Bottom of your team', 'coach')).toBe('Bottom of team'); + expect(neutralizeForCoach('Top 18% on your team', 'coach')).toBe('Top 18% on team'); + expect(neutralizeForCoach('About your team average', 'coach')).toBe('About team average'); + }); + it('is a no-op for text with no possessive to strip', () => { + expect(neutralizeForCoach('Above team average', 'coach')).toBe('Above team average'); + expect(neutralizeForCoach('Below team average', 'coach')).toBe('Below team average'); + }); + it('is a no-op for the player\'s own view (self, or undefined)', () => { + expect(neutralizeForCoach('Bottom of your team', 'self')).toBe('Bottom of your team'); + expect(neutralizeForCoach('Bottom of your team', undefined)).toBe('Bottom of your team'); + }); + it('passes through an empty string', () => { + expect(neutralizeForCoach('', 'coach')).toBe(''); + }); +}); + +describe('initialsFromName', () => { + it('takes first + last initial for a two-word name', () => { + expect(initialsFromName('Ethan Rodriguez')).toBe('ER'); + }); + it('takes the first two letters of a single-word name', () => { + expect(initialsFromName('Ethan')).toBe('ET'); + }); + it('uses first + last of a multi-word name (ignores middle names)', () => { + expect(initialsFromName('Mary Jane Watson')).toBe('MW'); + }); + it('falls back to "PL" for missing/blank names — never fabricates initials', () => { + expect(initialsFromName(null)).toBe('PL'); + expect(initialsFromName(undefined)).toBe('PL'); + expect(initialsFromName(' ')).toBe('PL'); + }); +}); + +describe('standingSubjectLabel', () => { + it('is "You" for the self viewer (default)', () => { + expect(standingSubjectLabel('self', 'Ethan Rodriguez')).toBe('You'); + expect(standingSubjectLabel(undefined, 'Ethan Rodriguez')).toBe('You'); + }); + it('is the player\'s initials for a coach viewer', () => { + expect(standingSubjectLabel('coach', 'Ethan Rodriguez')).toBe('ER'); + }); + it('falls back to "PL" for a coach viewer with no player name', () => { + expect(standingSubjectLabel('coach', undefined)).toBe('PL'); + }); }); // ---------------------------------------------------------------------------