fix(stats): scope the approach lie filter so the pills actually filter - #302
fix(stats): scope the approach lie filter so the pills actually filter#302njrini99-code wants to merge 1 commit into
Conversation
…mething
The Fairway / Rough / Sand lie pills on the approach breakdown read as broken:
tapping them appeared to change nothing. Root cause was placement + data, not a
dead control —
- The pills sat directly above the prominent "GIR by approach distance" board,
and the card copy claimed everything was "filtered by the lie you played
from." But that board (and "GIR by hole type") is LIE-AGNOSTIC: GolfStats has
per-band GIR (girPct50_75…) and per-lie GIR (girPctFromRough), but NO per-lie
*by band* GIR — only efficiency is keyed by lie. So those boards can't be
lie-filtered, and never changed on tap.
- The only lie-specific cards ("{Lie} lie", "Efficiency from {lie}") were below
the board, and in the cockpit they were hidden when a lie had no data — so
switching to Rough/Sand could change nothing visible at all.
Fix (mirrors the proven scrambling section's pill→grid+empty pattern):
- FairwayStatsCockpit `ApproachLegacyDetail`: move the lie-agnostic boards ABOVE
the pills; put the pills in a clearly-scoped "By lie" sub-section that governs
only the lie cards; add an honest empty state so a lie with no shots still
shows a response; drop the false "filtered by the lie" claim from the top copy.
- FairwayStatsTabs `ApproachBody`: same move — "GIR by approach distance" lifted
above the pills; pills now scope only the "{Lie} lie" + "Efficiency from {lie}"
cards (which retitle + revalue on every tap).
Verified live (demo coach, dev redesign-on, Cole Bennett profile → Approach tab):
tapping Fairway/Rough/Sand switches the pill AND retitles+revalues the by-lie
cards ("Fairway lie"→"Rough lie"→"Sand lie", "Efficiency from …"), while the
lie-agnostic GIR-by-distance board correctly stays put. typecheck clean · lint 0
errors · 2510 unit tests pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary by CodeRabbit
WalkthroughTwo components — ChangesApproach stats lie-filter scope and render order
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.43.0)src/components/fairway/pages/coachhelm/FairwayStatsCockpit.tsxError: Cannot parse rule /ast-grep-rules/../git/.coderabbit/ast-grep/no-explicit-any.yml ✖ Caused by src/components/fairway/pages/roster/FairwayStatsTabs.tsxError: Cannot parse rule /ast-grep-rules/../git/.coderabbit/ast-grep/no-explicit-any.yml ✖ Caused by 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f63c997fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The lie pills govern ONLY the by-lie block below. There is no per-lie GIR | ||
| // by distance in GolfStats (girPct{band} is overall; only approachEff{band} is | ||
| // keyed by lie), so the GIR-by-distance + GIR-by-hole-type boards are overall | ||
| // across every lie and must NOT sit under the lie filter. |
There was a problem hiding this comment.
npm run knowledge:map reports no impacted features for either changed Fairway stats component, even though memory/registry.yml defines the high-criticality stats_analytics feature and its current-state doc explicitly includes roster profile stats. This change therefore bypasses that feature's required context and checks, contrary to the /workspace/helmv3/AGENTS.md instruction to add a mapping or explicitly mark the feature-awareness gap before reviewing or changing mapped feature behavior. Add these component paths to stats_analytics and update or explicitly account for the current-state documentation.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| {/* Lie-AGNOSTIC — overall GIR across every lie (there is no per-lie GIR by | ||
| distance in GolfStats), so it sits ABOVE the lie filter. The pills | ||
| below do not change this board. */} | ||
| <BreakdownGrid title="GIR by approach distance" rows={girByDistance} columns={4} /> | ||
|
|
||
| {/* BY LIE — the pills govern ONLY the cards below (GIR/proximity from the | ||
| selected lie + efficiency by distance from it); both retitle and | ||
| re-value on every pill tap. */} | ||
| <div className="flex flex-wrap gap-2 px-1" aria-label="Approach lie filter"> | ||
| <ToggleChip active={selectedLie === 'fairway'} value="fairway" label="Fairway" onSelect={setSelectedLie} /> | ||
| <ToggleChip active={selectedLie === 'rough'} value="rough" label="Rough" onSelect={setSelectedLie} /> | ||
| <ToggleChip active={selectedLie === 'sand'} value="sand" label="Sand" onSelect={setSelectedLie} /> | ||
| </div> |
There was a problem hiding this comment.
The cockpit fix introduced a visible <h4>By lie</h4> heading between the lie-agnostic GIR board and the lie pills to make it clear the pills only govern the block below. In FairwayStatsTabs the comment describes the same intent, but no visible heading was added. With GIR by approach distance sitting directly above the pills and no separator, a user encountering this view still has no visual signal that the GIR board is unaffected by the selection — the same ambiguity the PR set out to eliminate.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/fairway/pages/roster/FairwayStatsTabs.tsx (1)
267-275:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing empty-state fallback for selected-lie no-data case in tabs view.
In
src/components/fairway/pages/roster/FairwayStatsTabs.tsxLine 267–Line 275, both by-lie cards can disappear when the selected lie has no data (BreakdownGridself-hides on all-dash rows). That makes the filter look unresponsive for that lie.Suggested fix
+ const lieHasData = !allDash(lieRows[selectedLie]) || !allDash(efficiencyRows); ... - <div className="grid grid-cols-1 gap-4 lg:grid-cols-2"> - <BreakdownGrid title={`${lieLabel} lie`} rows={lieRows[selectedLie]} /> - <BreakdownGrid - title={`Efficiency from ${selectedLie}`} - hint="Average strokes to hole out" - rows={efficiencyRows} - columns={4} - /> - </div> + {lieHasData ? ( + <div className="grid grid-cols-1 gap-4 lg:grid-cols-2"> + <BreakdownGrid title={`${lieLabel} lie`} rows={lieRows[selectedLie]} /> + <BreakdownGrid + title={`Efficiency from ${selectedLie}`} + hint="Average strokes to hole out" + rows={efficiencyRows} + columns={4} + /> + </div> + ) : ( + <Surface elevation="border" padding="md"> + <p className="font-fw-sans text-caption text-text-tertiary"> + No approach data from the {selectedLie} yet. + </p> + </Surface> + )}🤖 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/roster/FairwayStatsTabs.tsx` around lines 267 - 275, The BreakdownGrid components in the grid container (lines 267-275) self-hide when their data is all dashes, causing both the lie breakdown and efficiency cards to disappear when the selectedLie has no data. This makes the filter appear unresponsive. Add an empty-state fallback that displays when both lieRows[selectedLie] and efficiencyRows have no valid data by checking if these row arrays contain only empty or dash values, and if so, render a helpful message (e.g., "No data available for this lie") instead of allowing the grid to collapse entirely. This ensures the filter selection remains visibly responsive even when data is unavailable.
🤖 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/components/fairway/pages/coachhelm/FairwayStatsCockpit.tsx`:
- Around line 1525-1527: The lieHasData check in the guard condition incorrectly
depends only on the currently selected lie (selectedLie), which means if the
default selected lie (fairway) has no data, the entire section returns null even
if other lies like rough or sand have data. Fix this by modifying the lieHasData
calculation to check if ANY of the lie rows contain data, not just
lieRows[selectedLie]. This will allow the section to render and enable users to
switch between lies that actually have data.
---
Outside diff comments:
In `@src/components/fairway/pages/roster/FairwayStatsTabs.tsx`:
- Around line 267-275: The BreakdownGrid components in the grid container (lines
267-275) self-hide when their data is all dashes, causing both the lie breakdown
and efficiency cards to disappear when the selectedLie has no data. This makes
the filter appear unresponsive. Add an empty-state fallback that displays when
both lieRows[selectedLie] and efficiencyRows have no valid data by checking if
these row arrays contain only empty or dash values, and if so, render a helpful
message (e.g., "No data available for this lie") instead of allowing the grid to
collapse entirely. This ensures the filter selection remains visibly responsive
even when data is unavailable.
🪄 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
Run ID: bd2d03d8-085d-4ce3-9476-0745cfb97964
📒 Files selected for processing (2)
src/components/fairway/pages/coachhelm/FairwayStatsCockpit.tsxsrc/components/fairway/pages/roster/FairwayStatsTabs.tsx
| const lieHasData = !allDash(lieRows[selectedLie]) || !allDash(approachEfficiencyRows); | ||
| const hasAny = hasGirBands || !allDash(girByPar) || lieHasData; | ||
| if (!hasAny) return null; |
There was a problem hiding this comment.
Section-level guard incorrectly depends on the currently selected lie (can hide valid data).
In src/components/fairway/pages/coachhelm/FairwayStatsCockpit.tsx Line 1525–Line 1527, hasAny uses lieHasData for only selectedLie (default fairway). If fairway has no data but rough/sand do, the whole section returns null, so users can’t switch lies.
Suggested fix
- const lieHasData = !allDash(lieRows[selectedLie]) || !allDash(approachEfficiencyRows);
- const hasAny = hasGirBands || !allDash(girByPar) || lieHasData;
+ const lieHasData = !allDash(lieRows[selectedLie]) || !allDash(approachEfficiencyRows);
+ const anyLieHasData = (['fairway', 'rough', 'sand'] as const).some((lie) => {
+ const effRows: DetailRow[] = [
+ { label: '30-75 yds', value: fmtNum(s.approachEff30_75[lie], 2) },
+ { label: '75-100 yds', value: fmtNum(s.approachEff75_100[lie], 2) },
+ { label: '100-125 yds', value: fmtNum(s.approachEff100_125[lie], 2) },
+ { label: '125-150 yds', value: fmtNum(s.approachEff125_150[lie], 2) },
+ { label: '150-175 yds', value: fmtNum(s.approachEff150_175[lie], 2) },
+ { label: '175-200 yds', value: fmtNum(s.approachEff175_200[lie], 2) },
+ { label: '200-225 yds', value: fmtNum(s.approachEff200_225[lie], 2) },
+ { label: '225+ yds', value: fmtNum(s.approachEff225Plus[lie], 2) },
+ ];
+ return !allDash(lieRows[lie]) || !allDash(effRows);
+ });
+ const hasAny = hasGirBands || !allDash(girByPar) || anyLieHasData;🤖 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/FairwayStatsCockpit.tsx` around lines
1525 - 1527, The lieHasData check in the guard condition incorrectly depends
only on the currently selected lie (selectedLie), which means if the default
selected lie (fairway) has no data, the entire section returns null even if
other lies like rough or sand have data. Fix this by modifying the lieHasData
calculation to check if ANY of the lie rows contain data, not just
lieRows[selectedLie]. This will allow the section to render and enable users to
switch between lies that actually have data.
|
Superseded by #304, which rolls this onto a single branch (feat/coachhelm-stats-roundup) with all four PRs merged clean + a merge-interaction fix. Branch |
1 participant
The bug
On the approach breakdown, the Fairway / Rough / Sand lie pills appeared to do nothing when tapped.
Root cause — placement + data, not a dead control
The
ToggleChipwas wired fine (state +aria-pressedupdated). The problem:GolfStatshas per-band GIR (girPct50_75…) and per-lie GIR (girPctFromRough), but no per-lie by-band GIR — only efficiency is keyed by lie. So those boards genuinely can't be lie-filtered, and never changed on tap."{Lie} lie","Efficiency from {lie}") were below the board — and in the cockpit they were hidden when a lie had no data, so switching to Rough/Sand could change nothing visible at all.Net: the pills looked like they filtered the big board (they don't / can't), and their real target was easy to miss or empty.
Fix — mirror the proven scrambling section (pills → grid + honest empty)
FairwayStatsCockpitApproachLegacyDetail: lie-agnostic boards moved above the pills; pills now live in a clearly-labeled "By lie" sub-section that governs only the lie cards; added an honest empty state ("No approach data from the {lie} yet") so a lie with no shots still shows a response; removed the false "filtered by the lie" claim from the top copy.FairwayStatsTabsApproachBody: same move — "GIR by approach distance" lifted above the pills; pills now scope only the "{Lie} lie" + "Efficiency from {lie}" cards (which retitle + revalue on every tap).Verified live
Demo coach · dev redesign-on · Cole Bennett profile → Approach tab. Tapping the pills:
Fairway lie→Rough lie→Sand lie,Efficiency from fairway→… rough→… sand).tscclean ·npm run lint0 errors · 2510 unit tests pass.🤖 Generated with Claude Code
Greptile Summary
This PR fixes the approach lie pills by repositioning lie-agnostic boards (
GIR by approach distance,GIR by hole type) above the filter so they no longer appear to respond to pill taps, and scoping the pills to govern only the per-lie cards below them.FairwayStatsCockpit(ApproachLegacyDetail): full "By lie" sub-section added with a visible<h4>label, correctlieHasDataguard, and an honest empty state so switching to a lie with no shots always produces visible feedback.FairwayStatsTabs(ApproachBody):GIR by approach distancelifted above the pills, but the "By lie" sub-label and no-data empty state present in the cockpit were not carried over, leaving a UX gap in this view.Confidence Score: 4/5
The filter mechanics and data scoping are correct; the cockpit view is fully fixed. The tabs view moves the board correctly but omits the sub-label and empty state, leaving a visible inconsistency.
The core layout fix works in both views and the lie pills now correctly scope only the per-lie cards. The cockpit implementation is thorough. The tabs view leaves a noticeable UX gap — no "By lie" heading and no empty state — that the PR description implies was part of the fix but was not implemented there.
src/components/fairway/pages/roster/FairwayStatsTabs.tsx — missing the "By lie" sub-heading and empty state that the cockpit counterpart received.
Important Files Changed
lieHasData/hasAnyguards — logic is sound and consistent.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Approach section renders] --> B{hasGirBands OR girByPar has data OR lieHasData?} B -- No --> Z[return null] B -- Yes --> C[Render GirByDistanceBoard\nlie-AGNOSTIC, always fixed] C --> D[Render GIR by hole type grid\nlie-AGNOSTIC, always fixed] D --> E[By lie sub-section] E --> F[Lie pills: Fairway / Rough / Sand\nsetSelectedLie on tap] F --> G{lieHasData?} G -- Yes --> H[Render lieRows grid\nCockpit only: conditional on allDash] H --> I[Render approachEfficiencyRows grid\nCockpit only: conditional on allDash] G -- No --> J[Cockpit: empty state\n'No approach data from selectedLie yet.'\nTabs: dashes rendered unconditionally]Comments Outside Diff (1)
src/components/fairway/pages/roster/FairwayStatsTabs.tsx, line 267-275 (link)The cockpit counterpart (
ApproachLegacyDetail) wraps the lie cards in alieHasDataguard and shows"No approach data from the {selectedLie} yet."when a lie has no shots. The tabsApproachBodyrenders bothBreakdownGridcomponents unconditionally, so switching to Rough or Sand when a player has never hit from those lies renders two grids of dashes with no contextual message. The pill still moves (so it isn’t a dead control), but the response is less informative than the cockpit’s explicit empty state.Reviews (1): Last reviewed commit: "fix(stats): scope the approach lie filte..." | Re-trigger Greptile