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/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/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. @@ -1355,6 +1413,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} @@ -1587,20 +1649,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. */} @@ -1983,6 +2056,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/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' ? ( ) : ( - + )}
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} /> ) : ( display name` map (the SSR-resolved team roster) and resolve + * `playerName` from it. This locks that contract. + */ +import { describe, it, expect } from 'vitest'; +import { + toCoachVoice, + patternToSignalRow, + insightToSignalRow, + insightsToSignalRows, +} from './patternToInsightVocabulary'; +import type { ExtendedPattern } from '@/app/golf/actions/pattern-management'; +import type { EvidenceInsight } from '@/app/golf/actions/insight-delivery'; +import type { InsightEvidence } from '@/lib/coachhelm/v2/insights/types'; + +// --------------------------------------------------------------------------- +// toCoachVoice — the voice switch +// --------------------------------------------------------------------------- + +describe('toCoachVoice', () => { + 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, + }; +} + +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('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); + }); +}); + +// --------------------------------------------------------------------------- +// insightToSignalRow / insightsToSignalRows — player-name resolution +// --------------------------------------------------------------------------- + +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..62b4618b6 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. @@ -274,8 +301,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 +329,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, @@ -301,18 +344,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 +408,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 +515,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, }; } @@ -443,7 +525,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/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/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
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'); + }); }); // ---------------------------------------------------------------------------