fix(coachhelm): Signals coherence — player names, KPI semantics, leak math, accuracy gate - #930
fix(coachhelm): Signals coherence — player names, KPI semantics, leak math, accuracy gate#930njrini99-code wants to merge 1 commit into
Conversation
… math, accuracy gate
Four independently-verified bugs on the Signals/Effectiveness surfaces:
1. By-player grouping ("Unknown player"): EvidenceInsight never carried a
resolved player name (only golf_coach_insights.player_id), so every
insight/alert row hard-coded playerName: undefined and collapsed into one
"Unknown player" bucket. Patterns were unaffected (getTeamPatterns already
joins golf_players inline). insightToSignalRow/insightsToSignalRows now
accept an optional player_id -> name map; /alerts and /insights SSR-fetch
the team roster via getTeamPlayers() and pass it through.
2. KPI tile semantics: "Urgent + high" counted every loaded row regardless of
status while "Open" filtered by status, so /alerts could show
"Urgent + high 13 > Open 10" — a severity breakdown reading as MORE than
its own superset. Urgent+high is now a true subset of Open (matches the
shell badge's own getAlertCounts().counts.critical semantics). Tile labels
now say the sub-tab noun ("Open alerts"/"Open insights"/"Open patterns")
instead of a generic "Open signals", and "Loaded" (dev-speak) is renamed
"Showing".
3. LeakBoard math/label: the "Where the team is bleeding" total is a SUM
across every player and every leak insight in a category, but was labeled
"str/rd" (a per-round rate) — reading as if −26.4 meant 26 strokes lost
EVERY round, ~8x the real team SG-putting figure (~−3.19/rd). Relabeled to
an honest "total", and the rollup now reads from its own full team-wide
fetch (no priority filter) instead of the active sub-tab's own scoped
`insights` state, so the banner no longer silently re-totals per sub-tab.
4. Effectiveness accuracy headline: gated on GAUGE_MIN_RESOLVED (2) while the
calibration side panel's own copy says "needs 5 resolved predictions" —
the two thresholds could disagree, letting a near-empty sample render an
authoritative "100% ▲+8%". Now gated on the SAME 5-resolved threshold
(isAccuracyHeadlineLive), with "Calibrating" replacing "Awaiting
predictions". Also fixed: StrokesGainedTornado's y-scale keyed rows by
display label, so two rows sharing a label (e.g. one player with two
top-N patterns) collapsed onto the same band — bars and value text
rendered on top of each other ("+4.10+4.67"). Now keyed by row index.
And the "no trend yet" cell rendered a bare "—" with no visible label
(only a tooltip); now matches the icon idiom the other three trend states
already use, dimmed to read as "no data" rather than a genuine flat trend.
Gates: tsc --noEmit clean, eslint clean on changed files, full vitest suite
green (272 files / 2636 passed).
Part of #914 + Fixes #907
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🤖 Mission Control — PR summary What it changes: Four independently-verified bugs on the Signals / Effectiveness surfaces:
Risk / areas: golf CoachHelm Watch: the added roster fetch on the SSR path (latency); the subset invariant (urgent+high ≤ open) holds across filters; player-name fallback when the map misses. CI: ✅ green so far — 34 checks passing, 4 pending, 0 failing; mergeable state BLOCKED on required review (no CI failure). Awaiting review. |
…s in patternToInsightVocabulary.test.ts Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg
1 participant
Problem
Verified live on
/golf/dashboard/alerts(Signals) and/golf/dashboard/analytics/coachhelm(Effectiveness):By-player grouping broken — ALL signals grouped under "Unknown player".
EvidenceInsight(the insight/alert row shape) never carries a resolved player name — onlyplayer_idoffgolf_coach_insights— soinsightToSignalRowhard-codedplayerName: undefinedfor every row. The "By player" grouping's fallback (r.playerName?.trim() || 'Unknown player') then swallowed everything. Patterns were unaffected —getTeamPatternsalready joinsgolf_playersinline.KPI tile semantics inconsistent — the three tiles (OPEN SIGNALS / URGENT + HIGH / LOADED) silently re-scoped per sub-tab with identical labels, and on
/alerts"Urgent + high" (13, counted regardless of status) read HIGHER than "Open" (10, status-filtered) — a severity breakdown reading as more than its own superset. "LOADED" was dev-speak."Where the team is bleeding" banner mislabels a SUM as a per-round rate —
LeakBoardsumsstrokes_impactacross every player and every leak insight in a category and labeled it "str/rd", reading as if −26.4 meant the team lost 26 strokes EVERY round (~8x the real team SG-putting figure, ~−3.19/rd). It also recomputed from the active sub-tab's own scoped insight fetch, so the SAME banner totaled differently on/alertsvs/insights.Effectiveness "ACCURACY 100% ▲+8%" renders from a near-empty sample — the headline gated on
GAUGE_MIN_RESOLVED(2) while the calibration side panel's own copy says "needs 5 resolved predictions" — the two thresholds could disagree. Also: the impact-chart tornado's y-scale keyed rows by display label, so two rows sharing a label (one player with two top-N patterns) collapsed onto the same row — bars and value text overlapped ("+4.10+4.67"). And the "no trend yet" table cell rendered a bare "—" with zero visible context (only a hover tooltip).Fix
insightToSignalRow/insightsToSignalRows(patternToInsightVocabulary.ts) now accept an optionalplayer_id -> namemap./alertsand/insightsSSR-fetch the team roster viagetTeamPlayers()(existing roster action, unchanged) and pass it through as a newplayerNamesprop onFairwayCoachHelmSignals. Missing entries still fall back toundefined— never a fabricated name.summary.urgentis now computed as a subset ofsummary.open(same status filter, narrower priority filter) — it can never read higher than "Open" again, and now matches the shell badge's owngetAlertCounts().counts.criticalsemantics. Tile 1 label is now sub-tab-scoped (Open alerts/Open insights/Open patterns); tile 3 renamedLoaded→Showing.LeakBoard's unit/copy now honestly says "total" (not "str/rd"), and its data source is a dedicated full team-wide fetch (no priority filter) independent of the active sub-tab's owninsightsstate — the banner reads the same number regardless of which sub-tab is open.Extracted
isAccuracyHeadlineLive(resolved)gated on the SAMEBUCKET_MIN_RESOLVED(5) threshold the calibration panel already states, withawaitingLabel="Calibrating".StrokesGainedTornado's y-scale is now keyed by row index (always unique) instead of display label. The "no trend yet" glyph now uses the same dimmed-icon idiom as its sibling states instead of a bare em-dash.Gates
npx tsc --noEmit -p tsconfig.json— cleannpx eslint <changed files>— cleannpx vitest run <full suite>— 272 files / 2636 passed, 4 skipped (pre-existing skips, unrelated)patternToInsightVocabulary.test.ts(player-name resolution),LeakBoard.test.tsx(label honesty),StrokesGainedTornado.test.tsx(duplicate-label row collision),FairwayEffectiveness.test.ts(accuracy gate threshold)Caveats
flameThreshold(default 0.8, "high bleed" flag) was calibrated for the old per-metric magnitude; now that the value is an honest multi-player/multi-insight sum, it may light up more readily. Left untouched — out of scope for this fix, flagged for a follow-up if it reads as noisy in practice.Part of #914
Fixes #907
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg