feat(shared): one expected-empty-state registry for UI copy + telemetry classification#809
Open
njrini99-code wants to merge 2 commits into
Open
feat(shared): one expected-empty-state registry for UI copy + telemetry classification#809njrini99-code wants to merge 2 commits into
njrini99-code wants to merge 2 commits into
Conversation
…ry classification UI-audit item SH-002 (§12 view-state contract): the empty-state codes introduced in #804/#807 were telemetry-only — EXPECTED_EMPTY_STATE_CODES lived in the soft-failure observer while UI surfaces rendered hardcoded copy with no knowledge of WHY data was absent, so user-facing copy and observability classification could silently drift. New leaf module src/lib/view-state/expected-empty-states.ts owns the codes, the global-classification CONTRACT, and user-facing copy (title + next-action description, optional unit/required gap-meter hints) for all four registered codes. Consumers: - observe-action-result.ts imports the code set (behavior unchanged — full 64-test suite green). - The player CoachHelm route preserves the failure codes instead of collapsing to bare null, and FairwayPlayerCoachHelm's InsufficientData surfaces render registry copy for the ACTUAL absence reason (brand-new player vs quiet period vs <3 rounds), falling back to the existing generic lines when no code is present. A registry test locks the invariant: observer classification and registry codes can never disagree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019WJ1Gzjj9MT3UsxhMoMzSV
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Greptile SummaryThis PR shares expected empty-state reasons between UI copy and telemetry classification. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "fix(golf): overview empty banner prefers..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
WalkthroughThe change centralizes expected empty-state definitions, reuses them for observer classification, and passes V3 empty-state codes from the CoachHelm server page to registry-driven client rendering. ChangesCoachHelm Empty-State Handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CoachHelmPage
participant FairwayPlayerCoachHelm
participant expectedEmptyStateCopy
CoachHelmPage->>FairwayPlayerCoachHelm: pass v3EmptyCodes
FairwayPlayerCoachHelm->>expectedEmptyStateCopy: resolve registered code
expectedEmptyStateCopy-->>FairwayPlayerCoachHelm: return empty-state copy
FairwayPlayerCoachHelm-->>CoachHelmPage: render InsufficientData surfaces
🚥 Pre-merge checks | ✅ 4 | ❌ 8❌ Failed checks (1 warning, 7 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…oped Greptile P2: a brand-new player gets no_completed_rounds (all-time) from the profile call but no_rounds_in_period (period-scoped) from shot context; the banner's shots-first priority showed 'widen the date range' to players who should be told to record their first round. Profile's all-time verdict now wins. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019WJ1Gzjj9MT3UsxhMoMzSV
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/fairway/pages/coachhelm/FairwayPlayerCoachHelm.tsx (1)
629-693: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist repeated
expectedEmptyStateCopy(...)lookups per block.Each
InsufficientDatablock re-invokesexpectedEmptyStateCopywith the identical argument 2-3 times (Line 639/643 forprofile ?? shots; Line 666/668 forprofile; Line 684/686/690 fortrend). Functionally fine today, but this is the same shape of bug the PR's own follow-up fix had to correct (one call site getting the priority order right, another not). A single hoisted const per block removes that drift risk and reads cleaner.♻️ Example for the trend block (Lines 682-693)
- ) : ( - <Surface padding="md"> - <InsufficientData - title={expectedEmptyStateCopy(v3EmptyCodes.trend)?.title ?? 'Trends warming up'} - description={ - expectedEmptyStateCopy(v3EmptyCodes.trend)?.description ?? - 'Log a couple more rounds and your performance trend lines fill in.' - } - unit="rounds" - required={expectedEmptyStateCopy(v3EmptyCodes.trend)?.required} - /> - </Surface> - )} + ) : ( + <Surface padding="md"> + {(() => { + const trendCopy = expectedEmptyStateCopy(v3EmptyCodes.trend); + return ( + <InsufficientData + title={trendCopy?.title ?? 'Trends warming up'} + description={ + trendCopy?.description ?? + 'Log a couple more rounds and your performance trend lines fill in.' + } + unit="rounds" + required={trendCopy?.required} + /> + ); + })()} + </Surface> + )}Same pattern applies to the
profile ?? shotsblock (Lines 637-646) and the profile-only block (Lines 665-673).🤖 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/FairwayPlayerCoachHelm.tsx` around lines 629 - 693, Hoist each repeated expectedEmptyStateCopy lookup into a local constant within the three affected InsufficientData blocks: the overview block using v3EmptyCodes.profile ?? v3EmptyCodes.shots, the profile fallback using v3EmptyCodes.profile, and the trend fallback using v3EmptyCodes.trend. Reuse the corresponding constant for title, description, and required values without changing the existing fallback text or priority order.src/app/golf/(dashboard)/dashboard/coachhelm/page.tsx (1)
140-161: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOne throwing V3 action blanks all three empty-state codes, not just its own.
Promise.allat Line 150 means if any ofgetPlayerProfile/getPlayerTrendAnalysis/getPlayerShotContextthrows (vs. returning{success:false, code}), the whole block falls intocatchat Line 161 andv3EmptyCodesstays{}for all three — even for calls that would have resolved successfully. That undercuts the precision this registry is built for: the UI falls back to generic copy for panels that had a perfectly good code available.♻️ Sketch: isolate failures per call
- const [profileResult, trendResult, shotResult] = await Promise.all([ - getPlayerProfile(player.id), - getPlayerTrendAnalysis(player.id), - getPlayerShotContext(player.id), - ]); + const [profileResult, trendResult, shotResult] = await Promise.all([ + getPlayerProfile(player.id).catch(() => ({ success: false as const, code: null })), + getPlayerTrendAnalysis(player.id).catch(() => ({ success: false as const, code: null })), + getPlayerShotContext(player.id).catch(() => ({ success: false as const, code: null })), + ]);🤖 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)/dashboard/coachhelm/page.tsx around lines 140 - 161, Update the V3 fetch flow around getPlayerProfile, getPlayerTrendAnalysis, and getPlayerShotContext so each action’s rejection is handled independently instead of allowing one Promise.all failure to clear all v3EmptyCodes. Preserve successful data and codes for calls that resolve, while assigning only the throwing call’s data and empty code to their existing fallback values.
🤖 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.
Outside diff comments:
In `@src/app/golf/`(dashboard)/dashboard/coachhelm/page.tsx:
- Around line 140-161: Update the V3 fetch flow around getPlayerProfile,
getPlayerTrendAnalysis, and getPlayerShotContext so each action’s rejection is
handled independently instead of allowing one Promise.all failure to clear all
v3EmptyCodes. Preserve successful data and codes for calls that resolve, while
assigning only the throwing call’s data and empty code to their existing
fallback values.
In `@src/components/fairway/pages/coachhelm/FairwayPlayerCoachHelm.tsx`:
- Around line 629-693: Hoist each repeated expectedEmptyStateCopy lookup into a
local constant within the three affected InsufficientData blocks: the overview
block using v3EmptyCodes.profile ?? v3EmptyCodes.shots, the profile fallback
using v3EmptyCodes.profile, and the trend fallback using v3EmptyCodes.trend.
Reuse the corresponding constant for title, description, and required values
without changing the existing fallback text or priority order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9cc187e9-b1af-4a2a-9f8b-d2a0b39c7120
📒 Files selected for processing (5)
src/app/golf/(dashboard)/dashboard/coachhelm/page.tsxsrc/components/fairway/pages/coachhelm/FairwayPlayerCoachHelm.tsxsrc/lib/admin/observe-action-result.tssrc/lib/view-state/__tests__/expected-empty-states.test.tssrc/lib/view-state/expected-empty-states.ts
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
2 participants
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.
Summary
UI-audit item SH-002 (view-state contract) — the natural completion of #804/#807. The empty-state codes those PRs introduced were telemetry-only:
EXPECTED_EMPTY_STATE_CODESlived inside the soft-failure observer while UI empty surfaces rendered hardcoded copy with no knowledge of why data was absent, so user-facing copy and observability classification could silently drift apart.New leaf module
src/lib/view-state/expected-empty-states.ts(zero imports — safe from bothserver-onlymodules and client components) owns:engine_no_recent_rounds,no_rounds_in_period,no_completed_rounds,insufficient_rounds),Consumers:
observe-action-result.tsnow imports the code set — classification behavior unchanged (full 64-test suite green)./golf/dashboard/coachhelm) preserves failure codes instead of collapsing{ success: false, code }to barenull, andFairwayPlayerCoachHelm'sInsufficientDatasurfaces render registry copy for the actual absence reason — "No rounds yet — record or import a round" for a brand-new player vs "Trend analysis unlocks at 3 completed rounds" — falling back to the existing generic lines when no code is present (real failures keep degrading exactly as before).A registry test locks the invariant: observer classification and registry codes can never disagree.
Partner-readable summary
When a player's analytics are empty, the app now explains the actual reason (new player, quiet date range, or not enough rounds yet) using the same classification the error-monitoring system uses — so the copy on screen and the ops dashboards can never tell different stories.
Type of change
Area
golf · coachhelm · shared observability
Risk level
Git Activity Timeline note
Empty-state copy and error-monitoring classification now share one registry, so what players see and what ops dashboards record can't drift apart.
Checklist
npm run typecheckpasses🤖 Generated with Claude Code
https://claude.ai/code/session_019WJ1Gzjj9MT3UsxhMoMzSV
Generated by Claude Code