fix(coachhelm): coach-facing surfaces speak coach voice, not player voice

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 <span> — 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(
+      <InsightCard priority="critical" title="Critical signal" />,
+    );
+    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(
+      <InsightCard priority="critical" iconTone="positive" title="Plays better under fatigue" />,
+    );
+    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(
+      <InsightCard priority="medium" iconTone="negative" title="Small leak" />,
+    );
+    expect(iconWrapClassName(medium)).toContain('bg-fw-warning-bg');
+    expect(iconWrapClassName(medium)).not.toContain('bg-fw-success-bg');
+
+    const { container: low } = render(
+      <InsightCard priority="low" iconTone="negative" title="Small leak" />,
+    );
+    expect(iconWrapClassName(low)).toContain('bg-fw-warning-bg');
+  });
+
+  it('iconTone="neutral" renders the neutral wrap regardless of priority', () => {
+    const { container } = render(
+      <InsightCard priority="high" iconTone="neutral" title="No clear direction" />,
+    );
+    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(
+      <InsightCard priority="medium" iconTone="positive" title="Better" />,
+    );
+    const { container: worse } = render(
+      <InsightCard priority="medium" iconTone="negative" title="Worse" />,
+    );
+    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(<InsightCard priority="low" title="A" />);
+    const { container: explicitUndefined } = render(
+      <InsightCard priority="low" iconTone={undefined} title="A" />,
+    );
+    expect(iconWrapClassName(omitted)).toBe(iconWrapClassName(explicitUndefined));
+  });
+});

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<InsightPriority, PriorityTone> = {
   },
 };
 
+/* -- 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<InsightIconTone, { iconWrap: string; icon: LucideIcon }> = {
+  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<HTMLDivElement, InsightCardProps>(
@@ -200,6 +237,7 @@ const InsightCardImpl = forwardRef<HTMLDivElement, InsightCardProps>(
       title,
       children,
       icon,
+      iconTone,
       hideIcon = false,
       evidence,
       actions,
@@ -220,13 +258,18 @@ const InsightCardImpl = forwardRef<HTMLDivElement, InsightCardProps>(
   ) {
     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 : <LeadIcon aria-hidden className="h-full w-full" strokeWidth={1.5} />);
 
@@ -373,7 +416,7 @@ const InsightCardImpl = forwardRef<HTMLDivElement, InsightCardProps>(
           <span
             className={cn(
               'pointer-events-none relative z-10 flex shrink-0 items-center justify-center rounded-fw-md',
-              tone.iconWrap,
+              iconWrapClass,
               isCompact ? 'h-8 w-8 p-1.5' : 'h-10 w-10 p-2',
             )}
           >

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 : <LeadIcon aria-hidden className="h-full w-full" strokeWidth={2} />);
 
@@ -233,7 +249,7 @@ function PanelBody({
             <span
               className={cn(
                 'flex h-11 w-11 shrink-0 items-center justify-center rounded-fw-md p-2.5',
-                tone.iconWrap,
+                iconWrapClass,
               )}
             >
               {leadIcon}
@@ -334,6 +350,7 @@ export const InsightPanel = forwardRef<HTMLDivElement, InsightPanelProps>(
       title,
       children,
       icon,
+      iconTone,
       hideIcon,
       meta,
       evidence,
@@ -368,6 +385,7 @@ export const InsightPanel = forwardRef<HTMLDivElement, InsightPanelProps>(
         overline={overline}
         title={title}
         icon={icon}
+        iconTone={iconTone}
         hideIcon={hideIcon}
         meta={meta}
         evidence={evidence}
@@ -420,6 +438,7 @@ export const InsightPanel = forwardRef<HTMLDivElement, InsightPanelProps>(
                 overline={overline}
                 title={title}
                 icon={icon}
+                iconTone={iconTone}
                 hideIcon={hideIcon}
                 meta={meta}
                 evidence={evidence}

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(<Ribbon title="SG total" data={DECLINING} seriesName="SG" />);
+    expect(getDeltaDirection()).toBe('down');
+  });
+
+  it('goodDirection="down": the SAME falling series now reads "up" (green) — a lower score is an improvement', () => {
+    render(
+      <Ribbon
+        title="Score by round"
+        data={DECLINING}
+        valueFormatter={(v) => v.toFixed(1)}
+        seriesName="Score"
+        goodDirection="down"
+      />,
+    );
+    expect(getDeltaDirection()).toBe('up');
+  });
+
+  it('goodDirection="down": a RISING series (a worsening score) reads "down" (amber)', () => {
+    render(
+      <Ribbon
+        title="Score by round"
+        data={IMPROVING /* raw values rise 0.1 -> 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(
+      <Ribbon
+        title="Score by round"
+        data={[{ x: 'R1', y: 72 }, { x: 'R2', y: 72 }]}
+        valueFormatter={(v) => v.toFixed(1)}
+        seriesName="Score"
+        goodDirection="down"
+      />,
+    );
+    expect(getDeltaDirection()).toBe('flat');
+  });
+});