Fix train: overnight QA issues #943-#950 + #952 (7 lanes, validated as one tree) - #954
Merged
Conversation
…c-strip cost/gain semantics (#943, #944) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg
…ics (#945) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg
…agging chips (#946) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg
… page repairs (#947, #948) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg
…e, calibrating states (#950) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg
…tion (#952) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg
…rs, toolbar overflow (#949) Includes 3-way test-file unions with the #945/#946 lanes (Ribbon.test.tsx, FairwayStatsCockpit.test.ts). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg
…age tripwire Pure presentation helper exported for tests; page exports must reach the gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg
|
Too many files changed for review. ( Bypass the limit by tagging |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
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. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Summary by CodeRabbit
WalkthroughThis PR fixes navigation, loading, scrolling, greeting, trend, chart, insight, chat, course, and responsive-layout behavior across Baseball, Golf, and Fairway surfaces, with focused unit, component, and E2E regression coverage. ChangesBaseball and admin recovery
Time, scrolling, and trends
Charts and controls
Coach insights and shell presentation
Golf interaction states
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (9 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)ast-grep could not parse rule config: /ast-grep-rules/../git/.coderabbit/ast-grep/no-explicit-any.yml 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/golf/`(dashboard)/FairwayDashboardShell.tsx:
- Around line 378-389: Update the window-level onKeyDown handler in the
useEffect to return immediately when event.defaultPrevented is already true,
before checking Home/End targets or scrolling. Preserve the existing modifier,
target, and scroll behavior for unhandled events.
- Around line 345-366: Update the popstate tracking around isPopStateRef and the
[pathname] effect to store the popstate destination URL, including query and
hash state. Only treat a pathname navigation as popstate when its current
destination URL matches the stored popstate URL, then consume and clear that
stored value so same-path history navigation cannot affect the next normal route
navigation.
In `@src/app/golf/actions/team-category-insights.ts`:
- Around line 945-956: Update the playerNamesForVoice construction near
assembleBriefEngineInsights to omit entries whose name is blank or equals the
“Unknown Player” sentinel. Preserve valid player-name mappings so toCoachVoice
falls back to “the player” when no usable name exists.
In `@src/components/fairway/charts/Ribbon.tsx`:
- Around line 232-284: Update the delta formatter in the Readout configuration
within Ribbon’s readoutNode so zero values do not receive a plus sign: use a
strict positive-value check when selecting the sign, while preserving the minus
sign for negative values and no sign for exactly zero.
In `@src/components/fairway/controls/Toolbar.test.tsx`:
- Around line 18-20: Component tests are missing the required framer-motion mock
with useReducedMotion. Add the established mock configuration to
Toolbar.test.tsx (lines 18-20), InsightPanel.test.tsx (lines 18-20),
MetricCard.test.tsx (lines 12-14), and StandingStrip.test.tsx (lines 15-18),
preserving each test’s existing imports and setup.
In `@src/components/fairway/pages/coachhelm/FairwayBrief.footStripLabel.test.ts`:
- Line 16: Re-export PlayerCategoryStat and CategoryInsight from the `@/lib/types`
barrel, then update the imports in
src/components/fairway/pages/coachhelm/FairwayBrief.footStripLabel.test.ts:16-16
and
src/components/fairway/pages/coachhelm/FairwayBrief.heroSupportDetail.test.ts:23-23
to use `@/lib/types` instead of the action module.
In `@src/components/fairway/pages/coachhelm/FairwayBrief.tsx`:
- Line 899: Replace the duplicated 5.5rem grid width in both category-row
templates at src/components/fairway/pages/coachhelm/FairwayBrief.tsx lines
899-899 and 990-990 with the shared category-row grid template/design token.
Ensure both locations reuse the same approved token instead of an arbitrary
inline width.
- Around line 201-230: Update selectHeroSupportDetail and the related
yardage-band chart visibility logic so worstZone is used only when its evidence
includes a categoryId matching the selected categoryId; do not rely on
isYardageBandCategory alone. Extend or reuse the yardage evidence type to carry
that category, and return the existing no-detail result while hiding the chart
whenever the evidence is absent or mismatched, including Putting and Scoring.
- Around line 247-252: Update footStripLabel so an empty players list returns a
label indicating that nobody is flagged, instead of “Dragging” with a zero
count. Preserve the existing improving, dragging, and mixed-category labels for
non-empty lists.
In
`@src/components/fairway/pages/coachhelm/signals/patternToInsightVocabulary.ts`:
- Around line 401-422: The INSIGHT_VOICE_REWRITES list only personalizes
scrambling sentence openers; extend it with targeted replacements for the
remaining “your” and “you” fragments in the sand-shot and finishing clauses,
using the existing name-based replacement style. Update or add assertions
covering the relevant scrambling output to ensure no standalone “you”, “your”,
or “you’re” tokens remain.
In `@src/components/ui/avatar.tsx`:
- Around line 43-50: Update getInitials in src/components/ui/avatar.tsx at lines
43-50 and the corresponding initials-generation logic in
src/components/fairway/controls/avatar.tsx at lines 71-78 to preserve numeric
characters by allowing \p{N} in the name-cleaning regex. Keep existing handling
for letters, whitespace, apostrophes, and hyphens unchanged so names such as
“Player 1” produce numeric initials.
In `@src/lib/coachhelm/v3/brief/assemble.ts`:
- Line 31: Move the pure toCoachVoice transformer from the component-layer
module into a shared src/lib/coachhelm module, then update the Signals adapter
and the assemble logic in assemble.ts to import it from the shared location.
Remove the domain assembly dependency on src/components while preserving the
transformer’s existing behavior and API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d6f7d5e2-fba2-4f69-9763-fbf85fd3423a
📒 Files selected for processing (70)
e2e/baseball-box-score.spec.tssrc/app/admin/golf/__tests__/honest-rounds-delta.test.tssrc/app/admin/golf/honest-rounds-delta.tssrc/app/admin/golf/page.tsxsrc/app/baseball/(dashboard)/dashboard/stats/games/create/NewGameClient.tsxsrc/app/baseball/(dashboard)/dashboard/stats/games/create/__tests__/NewGameClient.test.tsxsrc/app/golf/(auth)/demo/page.tsxsrc/app/golf/(dashboard)/FairwayDashboardShell.tsxsrc/app/golf/(dashboard)/dashboard/alerts/loading.tsxsrc/app/golf/(dashboard)/dashboard/analytics/coachhelm/loading.tsxsrc/app/golf/(dashboard)/dashboard/coachhelm/chat/page.tsxsrc/app/golf/(dashboard)/dashboard/intelligence/loading.tsxsrc/app/golf/(dashboard)/dashboard/stats/team/loading.tsxsrc/app/golf/actions/__tests__/dashboard-data.test.tssrc/app/golf/actions/dashboard-data.tssrc/app/golf/actions/team-category-insights.tssrc/components/auth/baseball-auth-shell.tsxsrc/components/fairway/cards-insight/InsightPanel.test.tsxsrc/components/fairway/cards-insight/InsightPanel.tsxsrc/components/fairway/cards-insight/MetricCard.test.tsxsrc/components/fairway/cards-insight/MetricCard.tsxsrc/components/fairway/charts/Ribbon.test.tsxsrc/components/fairway/charts/Ribbon.tsxsrc/components/fairway/charts/StandingStrip.test.tsxsrc/components/fairway/charts/StandingStrip.tsxsrc/components/fairway/charts/TrendChart.test.tssrc/components/fairway/charts/TrendChart.tsxsrc/components/fairway/charts/TrendChip.tsxsrc/components/fairway/charts/index.tssrc/components/fairway/controls/Toolbar.test.tsxsrc/components/fairway/controls/Toolbar.tsxsrc/components/fairway/controls/avatar.test.tssrc/components/fairway/controls/avatar.tsxsrc/components/fairway/index.tssrc/components/fairway/pages/coachhelm/AskConversationRail.tsxsrc/components/fairway/pages/coachhelm/AskWorkspace.tsxsrc/components/fairway/pages/coachhelm/FairwayBrief.footStripLabel.test.tssrc/components/fairway/pages/coachhelm/FairwayBrief.heroSupportDetail.test.tssrc/components/fairway/pages/coachhelm/FairwayBrief.tsxsrc/components/fairway/pages/coachhelm/FairwayCoachHelmSignals.tsxsrc/components/fairway/pages/coachhelm/FairwayEffectiveness.tsxsrc/components/fairway/pages/coachhelm/FairwayEffectivenessCalibration.test.tsxsrc/components/fairway/pages/coachhelm/FairwayStatsCockpit.test.tssrc/components/fairway/pages/coachhelm/FairwayStatsCockpit.tsxsrc/components/fairway/pages/coachhelm/FairwayTeamStats.test.tssrc/components/fairway/pages/coachhelm/FairwayTeamStats.tsxsrc/components/fairway/pages/coachhelm/PlayersGridSkeleton.tsxsrc/components/fairway/pages/coachhelm/PlayersGridView.tsxsrc/components/fairway/pages/coachhelm/signals/patternToInsightVocabulary.test.tssrc/components/fairway/pages/coachhelm/signals/patternToInsightVocabulary.tssrc/components/fairway/pages/dashboard/FairwayCoachDashboard.tsxsrc/components/fairway/pages/dashboard/FairwayPlayerDashboard.tsxsrc/components/fairway/pages/hub/hub-parts.tsxsrc/components/golf/coachhelm/coach/LeakBoard.test.tsxsrc/components/golf/coachhelm/coach/LeakBoard.tsxsrc/components/golf/coachhelm/v3/Chat/ChatMessageList.test.tsxsrc/components/golf/coachhelm/v3/Chat/ChatMessageList.tsxsrc/components/golf/courses/CourseDetailDrawer.tsxsrc/components/ui/avatar.test.tssrc/components/ui/avatar.tsxsrc/lib/coachhelm/trend.test.tssrc/lib/coachhelm/trend.tssrc/lib/coachhelm/v3/brief/assemble.test.tssrc/lib/coachhelm/v3/brief/assemble.tssrc/lib/entry/greeting.test.tssrc/lib/entry/greeting.tssrc/lib/golf/scroll-behavior.test.tssrc/lib/golf/scroll-behavior.tssrc/lib/utils/time-of-day.test.tssrc/lib/utils/time-of-day.ts
Comment on lines
+345
to
+366
| const isPopStateRef = useRef(false); | ||
| useEffect(() => { | ||
| const onPopState = () => { | ||
| isPopStateRef.current = true; | ||
| }; | ||
| window.addEventListener('popstate', onPopState); | ||
| return () => window.removeEventListener('popstate', onPopState); | ||
| }, []); | ||
|
|
||
| const previousPathnameRef = useRef<string | null>(null); | ||
| useEffect(() => { | ||
| const wasPopState = isPopStateRef.current; | ||
| isPopStateRef.current = false; // consume — good for exactly one pathname change | ||
| const reset = shouldResetScrollOnNavigate({ | ||
| previousPathname: previousPathnameRef.current, | ||
| nextPathname: pathname, | ||
| isPopState: wasPopState, | ||
| hash: typeof window !== 'undefined' ? window.location.hash : '', | ||
| }); | ||
| previousPathnameRef.current = pathname; | ||
| if (reset) window.scrollTo({ top: 0, left: 0, behavior: 'instant' }); | ||
| }, [pathname]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent stale popstate state after same-path history navigation (FairwayDashboardShell.tsx:345-366).
A Back/Forward transition between query or hash states does not rerun the [pathname] effect, leaving isPopStateRef true. The next normal route navigation then incorrectly skips its scroll reset. Store the popstate destination URL and only classify the matching navigation as popstate.
Proposed fix
- const isPopStateRef = useRef(false);
+ const popStateUrlRef = useRef<string | null>(null);
useEffect(() => {
const onPopState = () => {
- isPopStateRef.current = true;
+ popStateUrlRef.current = window.location.href;
};
window.addEventListener('popstate', onPopState);
return () => window.removeEventListener('popstate', onPopState);
}, []);
const previousPathnameRef = useRef<string | null>(null);
useEffect(() => {
- const wasPopState = isPopStateRef.current;
- isPopStateRef.current = false;
+ const currentUrl = window.location.href;
+ const wasPopState = popStateUrlRef.current === currentUrl;
+ popStateUrlRef.current = null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const isPopStateRef = useRef(false); | |
| useEffect(() => { | |
| const onPopState = () => { | |
| isPopStateRef.current = true; | |
| }; | |
| window.addEventListener('popstate', onPopState); | |
| return () => window.removeEventListener('popstate', onPopState); | |
| }, []); | |
| const previousPathnameRef = useRef<string | null>(null); | |
| useEffect(() => { | |
| const wasPopState = isPopStateRef.current; | |
| isPopStateRef.current = false; // consume — good for exactly one pathname change | |
| const reset = shouldResetScrollOnNavigate({ | |
| previousPathname: previousPathnameRef.current, | |
| nextPathname: pathname, | |
| isPopState: wasPopState, | |
| hash: typeof window !== 'undefined' ? window.location.hash : '', | |
| }); | |
| previousPathnameRef.current = pathname; | |
| if (reset) window.scrollTo({ top: 0, left: 0, behavior: 'instant' }); | |
| }, [pathname]); | |
| const popStateUrlRef = useRef<string | null>(null); | |
| useEffect(() => { | |
| const onPopState = () => { | |
| popStateUrlRef.current = window.location.href; | |
| }; | |
| window.addEventListener('popstate', onPopState); | |
| return () => window.removeEventListener('popstate', onPopState); | |
| }, []); | |
| const previousPathnameRef = useRef<string | null>(null); | |
| useEffect(() => { | |
| const currentUrl = window.location.href; | |
| const wasPopState = popStateUrlRef.current === currentUrl; | |
| popStateUrlRef.current = null; // consume only the matching navigation | |
| const reset = shouldResetScrollOnNavigate({ | |
| previousPathname: previousPathnameRef.current, | |
| nextPathname: pathname, | |
| isPopState: wasPopState, | |
| hash: typeof window !== 'undefined' ? window.location.hash : '', | |
| }); | |
| previousPathnameRef.current = pathname; | |
| if (reset) window.scrollTo({ top: 0, left: 0, behavior: 'instant' }); | |
| }, [pathname]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/golf/`(dashboard)/FairwayDashboardShell.tsx around lines 345 - 366,
Update the popstate tracking around isPopStateRef and the [pathname] effect to
store the popstate destination URL, including query and hash state. Only treat a
pathname navigation as popstate when its current destination URL matches the
stored popstate URL, then consume and clear that stored value so same-path
history navigation cannot affect the next normal route navigation.
Comment on lines
+378
to
+389
| useEffect(() => { | ||
| const onKeyDown = (event: KeyboardEvent) => { | ||
| if (event.key !== 'Home' && event.key !== 'End') return; | ||
| if (event.metaKey || event.ctrlKey || event.altKey || event.shiftKey) return; | ||
| if (!isPageScrollHomeEndTarget(event.target instanceof Element ? event.target : null)) return; | ||
| event.preventDefault(); | ||
| window.scrollTo({ | ||
| top: event.key === 'Home' ? 0 : document.documentElement.scrollHeight, | ||
| left: 0, | ||
| behavior: 'instant', | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Respect keyboard events already handled by focused controls (FairwayDashboardShell.tsx:378-389).
This window-level bubbling listener still scrolls the document after a child handler calls preventDefault(), overriding custom widgets and nested scroll regions. Exit when the event is already handled.
Proposed fix
const onKeyDown = (event: KeyboardEvent) => {
+ if (event.defaultPrevented) return;
if (event.key !== 'Home' && event.key !== 'End') return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| const onKeyDown = (event: KeyboardEvent) => { | |
| if (event.key !== 'Home' && event.key !== 'End') return; | |
| if (event.metaKey || event.ctrlKey || event.altKey || event.shiftKey) return; | |
| if (!isPageScrollHomeEndTarget(event.target instanceof Element ? event.target : null)) return; | |
| event.preventDefault(); | |
| window.scrollTo({ | |
| top: event.key === 'Home' ? 0 : document.documentElement.scrollHeight, | |
| left: 0, | |
| behavior: 'instant', | |
| }); | |
| }; | |
| useEffect(() => { | |
| const onKeyDown = (event: KeyboardEvent) => { | |
| if (event.defaultPrevented) return; | |
| if (event.key !== 'Home' && event.key !== 'End') return; | |
| if (event.metaKey || event.ctrlKey || event.altKey || event.shiftKey) return; | |
| if (!isPageScrollHomeEndTarget(event.target instanceof Element ? event.target : null)) return; | |
| event.preventDefault(); | |
| window.scrollTo({ | |
| top: event.key === 'Home' ? 0 : document.documentElement.scrollHeight, | |
| left: 0, | |
| behavior: 'instant', | |
| }); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/golf/`(dashboard)/FairwayDashboardShell.tsx around lines 378 - 389,
Update the window-level onKeyDown handler in the useEffect to return immediately
when event.defaultPrevented is already true, before checking Home/End targets or
scrolling. Preserve the existing modifier, target, and scroll behavior for
unhandled events.
Comment on lines
+945
to
+956
| // Bug #943 — coach-voice the engine row's content using the SAME | ||
| // roster map already built above for `playerStats` (never a second | ||
| // lookup). `player_id -> display name`, mirroring the shape | ||
| // `insightsToSignalRows` takes on the Signals surface. | ||
| const playerNamesForVoice: Record<string, string> = {}; | ||
| for (const [pid, info] of playerInfoMap) { | ||
| playerNamesForVoice[pid] = info.name; | ||
| } | ||
| const engineByCategory = assembleBriefEngineInsights( | ||
| engineResult.data, | ||
| CATEGORIES.map((c) => ({ id: c.id, label: c.label })), | ||
| playerNamesForVoice, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exclude the Unknown Player sentinel from voice substitution.
src/app/golf/actions/team-category-insights.ts:949-952 forwards the sentinel created at Line 790, producing prose such as “Unknown Player is only making…”. Omit blank/sentinel names so toCoachVoice uses its intended “the player” fallback.
for (const [pid, info] of playerInfoMap) {
- playerNamesForVoice[pid] = info.name;
+ if (info.name !== 'Unknown Player') {
+ playerNamesForVoice[pid] = info.name;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Bug #943 — coach-voice the engine row's content using the SAME | |
| // roster map already built above for `playerStats` (never a second | |
| // lookup). `player_id -> display name`, mirroring the shape | |
| // `insightsToSignalRows` takes on the Signals surface. | |
| const playerNamesForVoice: Record<string, string> = {}; | |
| for (const [pid, info] of playerInfoMap) { | |
| playerNamesForVoice[pid] = info.name; | |
| } | |
| const engineByCategory = assembleBriefEngineInsights( | |
| engineResult.data, | |
| CATEGORIES.map((c) => ({ id: c.id, label: c.label })), | |
| playerNamesForVoice, | |
| // Bug `#943` — coach-voice the engine row's content using the SAME | |
| // roster map already built above for `playerStats` (never a second | |
| // lookup). `player_id -> display name`, mirroring the shape | |
| // `insightsToSignalRows` takes on the Signals surface. | |
| const playerNamesForVoice: Record<string, string> = {}; | |
| for (const [pid, info] of playerInfoMap) { | |
| if (info.name !== 'Unknown Player') { | |
| playerNamesForVoice[pid] = info.name; | |
| } | |
| } | |
| const engineByCategory = assembleBriefEngineInsights( | |
| engineResult.data, | |
| CATEGORIES.map((c) => ({ id: c.id, label: c.label })), | |
| playerNamesForVoice, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/golf/actions/team-category-insights.ts` around lines 945 - 956,
Update the playerNamesForVoice construction near assembleBriefEngineInsights to
omit entries whose name is blank or equals the “Unknown Player” sentinel.
Preserve valid player-name mappings so toCoachVoice falls back to “the player”
when no usable name exists.
Comment on lines
+232
to
+284
| // Resolved labels — from the REAL (finite-filtered) first/last points, so a | ||
| // caller can name what the value + delta actually represent (#946). | ||
| const resolvedLabels = first && last ? readoutLabels?.(first, last) : undefined; | ||
|
|
||
| const readoutNode = !isAwaiting ? ( | ||
| <div className="flex items-center gap-2"> | ||
| {last ? ( | ||
| <Readout | ||
| value={last.y} | ||
| display={valueFormatter ? fmt(last.y) : undefined} | ||
| format={valueFormatter ? undefined : { maximumFractionDigits: 2 }} | ||
| size="sm" | ||
| align={readoutPlacement === 'below' ? 'start' : 'end'} | ||
| label={resolvedLabels?.value ?? seriesName} | ||
| delta={ | ||
| typeof trendDelta === 'number' | ||
| ? { | ||
| value: trendDelta, | ||
| direction: trendDirection, | ||
| caption: resolvedLabels?.delta, | ||
| format: (v) => { | ||
| // `fmt` (the caller's valueFormatter, e.g. FairwayBrief's | ||
| // fmtSG) may ALREADY prefix its own +/− sign. Strip any | ||
| // leading sign glyph before prepending ours, or a signed | ||
| // formatter double-signs ("▼ −+0.56" instead of "▼ −0.56"). | ||
| const magnitude = fmt(Math.abs(v)).replace(/^[+\-−]/, ''); | ||
| return `${v >= 0 ? '+' : '−'}${magnitude}`; | ||
| }, | ||
| } | ||
| : undefined | ||
| } | ||
| /> | ||
| ) : null} | ||
| {hasTable ? ( | ||
| <InstrumentTableToggle show={showTable} onToggle={() => setShowTable((v) => !v)} /> | ||
| ) : null} | ||
| </div> | ||
| ) : undefined; | ||
|
|
||
| return ( | ||
| <InstrumentPanel | ||
| depth="base" | ||
| eyebrow={overline} | ||
| header={title} | ||
| readout={ | ||
| !isAwaiting ? ( | ||
| <div className="flex items-center gap-2"> | ||
| {last ? ( | ||
| <Readout | ||
| value={last.y} | ||
| display={valueFormatter ? fmt(last.y) : undefined} | ||
| format={valueFormatter ? undefined : { maximumFractionDigits: 2 }} | ||
| size="sm" | ||
| align="end" | ||
| label={seriesName} | ||
| delta={ | ||
| 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 | ||
| // leading sign glyph before prepending ours, or a signed | ||
| // formatter double-signs ("▼ −+0.56" instead of "▼ −0.56"). | ||
| const magnitude = fmt(Math.abs(v)).replace(/^[+\-−]/, ''); | ||
| return `${v >= 0 ? '+' : '−'}${magnitude}`; | ||
| }, | ||
| } | ||
| : undefined | ||
| } | ||
| /> | ||
| ) : null} | ||
| {hasTable ? ( | ||
| <InstrumentTableToggle show={showTable} onToggle={() => setShowTable((v) => !v)} /> | ||
| ) : null} | ||
| </div> | ||
| ) : undefined | ||
| } | ||
| readout={readoutPlacement === 'corner' ? readoutNode : undefined} | ||
| className={className} | ||
| > | ||
| {/* 'below' placement: the readout stacks under the eyebrow/header as its | ||
| own block, never sharing a flex row with them — a structural | ||
| guarantee against the collision described in #946. */} | ||
| {readoutPlacement === 'below' && readoutNode ? ( | ||
| <div className="mb-4">{readoutNode}</div> | ||
| ) : null} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Omit the plus sign for zero deltas.
For exactly flat trends where v === 0, v >= 0 evaluates to true, resulting in a +0 (or +0.00) display. Using a strict greater-than check ensures that zero deltas are correctly rendered without an artificial plus sign.
💡 Proposed fix for the ternary operator
- return `${v >= 0 ? '+' : '−'}${magnitude}`;
+ return `${v > 0 ? '+' : v < 0 ? '−' : ''}${magnitude}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Resolved labels — from the REAL (finite-filtered) first/last points, so a | |
| // caller can name what the value + delta actually represent (#946). | |
| const resolvedLabels = first && last ? readoutLabels?.(first, last) : undefined; | |
| const readoutNode = !isAwaiting ? ( | |
| <div className="flex items-center gap-2"> | |
| {last ? ( | |
| <Readout | |
| value={last.y} | |
| display={valueFormatter ? fmt(last.y) : undefined} | |
| format={valueFormatter ? undefined : { maximumFractionDigits: 2 }} | |
| size="sm" | |
| align={readoutPlacement === 'below' ? 'start' : 'end'} | |
| label={resolvedLabels?.value ?? seriesName} | |
| delta={ | |
| typeof trendDelta === 'number' | |
| ? { | |
| value: trendDelta, | |
| direction: trendDirection, | |
| caption: resolvedLabels?.delta, | |
| format: (v) => { | |
| // `fmt` (the caller's valueFormatter, e.g. FairwayBrief's | |
| // fmtSG) may ALREADY prefix its own +/− sign. Strip any | |
| // leading sign glyph before prepending ours, or a signed | |
| // formatter double-signs ("▼ −+0.56" instead of "▼ −0.56"). | |
| const magnitude = fmt(Math.abs(v)).replace(/^[+\-−]/, ''); | |
| return `${v >= 0 ? '+' : '−'}${magnitude}`; | |
| }, | |
| } | |
| : undefined | |
| } | |
| /> | |
| ) : null} | |
| {hasTable ? ( | |
| <InstrumentTableToggle show={showTable} onToggle={() => setShowTable((v) => !v)} /> | |
| ) : null} | |
| </div> | |
| ) : undefined; | |
| return ( | |
| <InstrumentPanel | |
| depth="base" | |
| eyebrow={overline} | |
| header={title} | |
| readout={ | |
| !isAwaiting ? ( | |
| <div className="flex items-center gap-2"> | |
| {last ? ( | |
| <Readout | |
| value={last.y} | |
| display={valueFormatter ? fmt(last.y) : undefined} | |
| format={valueFormatter ? undefined : { maximumFractionDigits: 2 }} | |
| size="sm" | |
| align="end" | |
| label={seriesName} | |
| delta={ | |
| 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 | |
| // leading sign glyph before prepending ours, or a signed | |
| // formatter double-signs ("▼ −+0.56" instead of "▼ −0.56"). | |
| const magnitude = fmt(Math.abs(v)).replace(/^[+\-−]/, ''); | |
| return `${v >= 0 ? '+' : '−'}${magnitude}`; | |
| }, | |
| } | |
| : undefined | |
| } | |
| /> | |
| ) : null} | |
| {hasTable ? ( | |
| <InstrumentTableToggle show={showTable} onToggle={() => setShowTable((v) => !v)} /> | |
| ) : null} | |
| </div> | |
| ) : undefined | |
| } | |
| readout={readoutPlacement === 'corner' ? readoutNode : undefined} | |
| className={className} | |
| > | |
| {/* 'below' placement: the readout stacks under the eyebrow/header as its | |
| own block, never sharing a flex row with them — a structural | |
| guarantee against the collision described in #946. */} | |
| {readoutPlacement === 'below' && readoutNode ? ( | |
| <div className="mb-4">{readoutNode}</div> | |
| ) : null} | |
| format: (v) => { | |
| // `fmt` (the caller's valueFormatter, e.g. FairwayBrief's | |
| // fmtSG) may ALREADY prefix its own +/− sign. Strip any | |
| // leading sign glyph before prepending ours, or a signed | |
| // formatter double-signs ("▼ −+0.56" instead of "▼ −0.56"). | |
| const magnitude = fmt(Math.abs(v)).replace(/^[+\-−]/, ''); | |
| return `${v > 0 ? '+' : v < 0 ? '−' : ''}${magnitude}`; | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/fairway/charts/Ribbon.tsx` around lines 232 - 284, Update the
delta formatter in the Readout configuration within Ribbon’s readoutNode so zero
values do not receive a plus sign: use a strict positive-value check when
selecting the sign, while preserving the minus sign for negative values and no
sign for exactly zero.
Comment on lines
+18
to
+20
| import { render } from '@testing-library/react'; | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { Toolbar } from './Toolbar'; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Include the framer-motion mock in component tests.
None of these new component test files include the framer-motion mock. As per path instructions, component tests must use the framer-motion mock with useReducedMotion included to prevent CI state issues.
src/components/fairway/controls/Toolbar.test.tsx#L18-L20: Add the framer-motion mock configuration.src/components/fairway/cards-insight/InsightPanel.test.tsx#L18-L20: Add the framer-motion mock configuration.src/components/fairway/cards-insight/MetricCard.test.tsx#L12-L14: Add the framer-motion mock configuration.src/components/fairway/charts/StandingStrip.test.tsx#L15-L18: Add the framer-motion mock configuration.
📍 Affects 4 files
src/components/fairway/controls/Toolbar.test.tsx#L18-L20(this comment)src/components/fairway/cards-insight/InsightPanel.test.tsx#L18-L20src/components/fairway/cards-insight/MetricCard.test.tsx#L12-L14src/components/fairway/charts/StandingStrip.test.tsx#L15-L18
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/fairway/controls/Toolbar.test.tsx` around lines 18 - 20,
Component tests are missing the required framer-motion mock with
useReducedMotion. Add the established mock configuration to Toolbar.test.tsx
(lines 18-20), InsightPanel.test.tsx (lines 18-20), MetricCard.test.tsx (lines
12-14), and StandingStrip.test.tsx (lines 15-18), preserving each test’s
existing imports and setup.
Source: Path instructions
Comment on lines
+247
to
+252
| export function footStripLabel(categoryLabel: string, players: PlayerCategoryStat[]): string { | ||
| const improving = players.filter((p) => p.trend === 'improving').length; | ||
| const notImproving = players.length - improving; | ||
| if (players.length === 0 || improving === 0) return `Dragging ${categoryLabel}`; | ||
| if (notImproving === 0) return `Improving ${categoryLabel}`; | ||
| return `${notImproving} dragging ${categoryLabel} · ${improving} improving`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return an honest label for an empty player list.
src/components/fairway/pages/coachhelm/FairwayBrief.tsx:250 returns “Dragging …” with zero flagged players, while Lines 862-864 state that nobody is flagged.
Proposed fix
- if (players.length === 0 || improving === 0) return `Dragging ${categoryLabel}`;
+ if (players.length === 0) return `No one flagged in ${categoryLabel}`;
+ if (improving === 0) return `Dragging ${categoryLabel}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function footStripLabel(categoryLabel: string, players: PlayerCategoryStat[]): string { | |
| const improving = players.filter((p) => p.trend === 'improving').length; | |
| const notImproving = players.length - improving; | |
| if (players.length === 0 || improving === 0) return `Dragging ${categoryLabel}`; | |
| if (notImproving === 0) return `Improving ${categoryLabel}`; | |
| return `${notImproving} dragging ${categoryLabel} · ${improving} improving`; | |
| export function footStripLabel(categoryLabel: string, players: PlayerCategoryStat[]): string { | |
| const improving = players.filter((p) => p.trend === 'improving').length; | |
| const notImproving = players.length - improving; | |
| if (players.length === 0) return `No one flagged in ${categoryLabel}`; | |
| if (improving === 0) return `Dragging ${categoryLabel}`; | |
| if (notImproving === 0) return `Improving ${categoryLabel}`; | |
| return `${notImproving} dragging ${categoryLabel} · ${improving} improving`; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/fairway/pages/coachhelm/FairwayBrief.tsx` around lines 247 -
252, Update footStripLabel so an empty players list returns a label indicating
that nobody is flagged, instead of “Dragging” with a zero count. Preserve the
existing improving, dragging, and mixed-category labels for non-empty lists.
| aria-label={`${row.label} ${row.rating} of 100 — open Signals`} | ||
| className={cn( | ||
| 'grid grid-cols-[4.5rem_1fr_auto] items-center gap-3 rounded-fw-md border border-border-subtle px-3 py-2.5', | ||
| 'grid grid-cols-[5.5rem_1fr_auto] items-center gap-3 rounded-fw-md border border-border-subtle px-3 py-2.5', |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace the duplicated 5.5rem grid width with a shared design token.
src/components/fairway/pages/coachhelm/FairwayBrief.tsx#L899-L899: use the shared category-row grid template/token.src/components/fairway/pages/coachhelm/FairwayBrief.tsx#L990-L990: reuse that same template/token instead of duplicating the arbitrary width.
As per path instructions, “Reject inline hex values, ad-hoc spacing, or radius outside the approved tokens.”
📍 Affects 1 file
src/components/fairway/pages/coachhelm/FairwayBrief.tsx#L899-L899(this comment)src/components/fairway/pages/coachhelm/FairwayBrief.tsx#L990-L990
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/fairway/pages/coachhelm/FairwayBrief.tsx` at line 899, Replace
the duplicated 5.5rem grid width in both category-row templates at
src/components/fairway/pages/coachhelm/FairwayBrief.tsx lines 899-899 and
990-990 with the shared category-row grid template/design token. Ensure both
locations reuse the same approved token instead of an arbitrary inline width.
Source: Path instructions
Comment on lines
+401
to
+422
| const INSIGHT_VOICE_REWRITES: ReadonlyArray<{ | ||
| match: RegExp; | ||
| replace: (name: string) => string; | ||
| }> = [ | ||
| // scrambling.ts — "lag, not the escape" branch opener. | ||
| { match: /\bYou ESCAPE the bunker fine\b/g, replace: (n) => `${n} escapes the bunker fine` }, | ||
| // scrambling.ts — "escape is the leak" branch opener. | ||
| { | ||
| match: /\bYou're leaving balls in the bunker\b/g, | ||
| replace: (n) => `${n} is leaving balls in the bunker`, | ||
| }, | ||
| // scrambling.ts — "not your splash" driver clause. | ||
| { match: /\byour splash\b/gi, replace: (n) => `${n}'s splash` }, | ||
| // lag-distance-3putt.ts composite — opening clause. | ||
| { | ||
| match: /\bYour lag putts \(15\+ ft\) aren't finishing\b/g, | ||
| replace: (n) => `${n}'s lag putts (15+ ft) aren't finishing`, | ||
| }, | ||
| // lag-distance-3putt.ts composite — "you're only making N%" clause. | ||
| { match: /\byou're only making\b/gi, replace: (n) => `${n} is only making` }, | ||
| // lag-distance-3putt.ts composite — closing clause. | ||
| { match: /\bcosting you a stroke\b/gi, replace: (n) => `costing ${n} a stroke` }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Complete the scrambling sentence rewrites.
src/components/fairway/pages/coachhelm/signals/patternToInsightVocabulary.ts:405-422 rewrites only the opener. The resulting coach-facing text still says “82% of your 11 sand shots” and “but you finish,” as accepted by the new tests. Add targeted replacements for those remaining fragments and assert that no you, your, or you're tokens remain.
Also applies to: 443-447
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/fairway/pages/coachhelm/signals/patternToInsightVocabulary.ts`
around lines 401 - 422, The INSIGHT_VOICE_REWRITES list only personalizes
scrambling sentence openers; extend it with targeted replacements for the
remaining “your” and “you” fragments in the sand-shot and finishing clauses,
using the existing name-based replacement style. Update or add assertions
covering the relevant scrambling output to ensure no standalone “you”, “your”,
or “you’re” tokens remain.
Comment on lines
+43
to
+50
| export function getInitials(name: string): string { | ||
| if (!name || !name.trim()) return '?'; | ||
| const names = name.trim().split(' ').filter(n => n.length > 0); | ||
| // Strip anything that isn't a letter/space/apostrophe/hyphen BEFORE | ||
| // splitting into words — a display name like "Coach (Demo)" otherwise | ||
| // tokenizes to ["Coach", "(Demo)"], and the last token's first character | ||
| // is "(", producing "C(" instead of two real letters. | ||
| const cleaned = name.replace(/[^\p{L}\s'-]+/gu, ' ').trim(); | ||
| const names = cleaned.split(/\s+/).filter(n => n.length > 0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve numbers in initials generation.
The current regex /[^\p{L}\s'-]+/gu strips numeric characters, which causes names like "Player 1" or "Coach 2" to lose their digits and generate incorrect initials (e.g., "PL" instead of "P1"). Including \p{N} ensures digits are preserved.
src/components/ui/avatar.tsx#L43-L50: add\p{N}to the regex:name.replace(/[^\p{L}\p{N}\s'-]+/gu, ' ').trim().src/components/fairway/controls/avatar.tsx#L71-L78: apply the same regex update to preserve numbers.
📍 Affects 2 files
src/components/ui/avatar.tsx#L43-L50(this comment)src/components/fairway/controls/avatar.tsx#L71-L78
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ui/avatar.tsx` around lines 43 - 50, Update getInitials in
src/components/ui/avatar.tsx at lines 43-50 and the corresponding
initials-generation logic in src/components/fairway/controls/avatar.tsx at lines
71-78 to preserve numeric characters by allowing \p{N} in the name-cleaning
regex. Keep existing handling for letters, whitespace, apostrophes, and hyphens
unchanged so names such as “Player 1” produce numeric initials.
| import type { EvidenceInsight } from '@/app/golf/actions/insight-delivery'; | ||
| import type { AssembledEvidence } from '@/lib/coachhelm/v3/themes/types'; | ||
| import { sanitizeProse } from '@/lib/coachhelm/v3/themes/assemble'; | ||
| import { toCoachVoice } from '@/components/fairway/pages/coachhelm/signals/patternToInsightVocabulary'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move toCoachVoice out of the component layer.
src/lib/coachhelm/v3/brief/assemble.ts:31 makes domain assembly depend on src/components. Put the pure transformer in a shared src/lib/coachhelm module and import it from both the Signals adapter and this assembler.
Also applies to: 103-133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/coachhelm/v3/brief/assemble.ts` at line 31, Move the pure
toCoachVoice transformer from the component-layer module into a shared
src/lib/coachhelm module, then update the Signals adapter and the assemble logic
in assemble.ts to import it from the shared location. Remove the domain assembly
dependency on src/components while preserving the transformer’s existing
behavior and API.
This was referenced Jul 18, 2026
Closed
Closed
This was referenced Jul 18, 2026
This was referenced Jul 19, 2026
Closed
This was referenced Aug 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second merge train — fixes every code-fixable issue from the overnight visual teardown, built from 7 agent lanes and validated as a combined tree.
computeScoringTrendFromRounds(parity test vs roster); trend arrows unified via sharedTrendGlyph— the Team Stats arrows were hardcoded backwardsValidation on this tree: tsc clean · 596 files / 5,772 tests passed · eslint --max-warnings 0 clean. Full e2e dispatched on this branch (see checks).
Deliberately NOT here: cloned engine content across players (engine-side, noted in #944), demo data items (#951 — seed scripts), e2e timeout/drift backlog (#953), CTA contrast design call (#942).
🤖 Generated with Claude Code
https://claude.ai/code/session_01MMdviLDsAg2YYJ8adsM6fg