fix(baseball): migrate player-today/passport/snapshot-cards off legacy stat layer (#379) - #845
Conversation
|
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. 1 Skipped Deployment
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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 |
Greptile SummaryMigrates three BaseballHelm read models (
Confidence Score: 4/5Safe to merge. The migration is mechanically correct and the honesty invariants (trust/provenance always null for box-score rows) are re-proved by updated contract tests. The core data-source swap is clean: new queries are properly tenant-scoped, error paths degrade gracefully, and the stat-layer-contract CI guard passes. Two observations hold the score below 5: (1)
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[getPlayerToday / getPlayerPassport / getPlayerSnapshotCards] --> B{Read source}
B -- recentStats / recentActivity --> C[fetchRecentBoxScoreActivity]
C --> D[baseball_box_score_batting\n.select game_id]
C --> E[baseball_box_score_pitching\n.select game_id]
D & E --> F[Set dedup game IDs]
F --> G[baseball_games\n.select id game_date game_type opponent_name\n.order game_date desc .limit N]
G --> H[PlayerTodayStat / recentActivity\ntrust: null provenance: null]
B -- exit velocity --> I[baseball_batted_ball_events\ndata_context in official_game scrimmage\nsuperseded_by_run_id IS NULL\nbatter_id = playerId]
I --> J[buildHitterMetrics\navg_exit_velocity metric\nevLaPoints for max]
J --> K[avgExitVelocity / maxExitVelocity]
B -- pitch velocity --> L[adaptLegacyPlayerStats]
L --> M{event input?}
M -- yes --> N[event.avgPitchVelocity wins]
M -- no --> O[legacy.avg_pitch_velocity fallback]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[getPlayerToday / getPlayerPassport / getPlayerSnapshotCards] --> B{Read source}
B -- recentStats / recentActivity --> C[fetchRecentBoxScoreActivity]
C --> D[baseball_box_score_batting\n.select game_id]
C --> E[baseball_box_score_pitching\n.select game_id]
D & E --> F[Set dedup game IDs]
F --> G[baseball_games\n.select id game_date game_type opponent_name\n.order game_date desc .limit N]
G --> H[PlayerTodayStat / recentActivity\ntrust: null provenance: null]
B -- exit velocity --> I[baseball_batted_ball_events\ndata_context in official_game scrimmage\nsuperseded_by_run_id IS NULL\nbatter_id = playerId]
I --> J[buildHitterMetrics\navg_exit_velocity metric\nevLaPoints for max]
J --> K[avgExitVelocity / maxExitVelocity]
B -- pitch velocity --> L[adaptLegacyPlayerStats]
L --> M{event input?}
M -- yes --> N[event.avgPitchVelocity wins]
M -- no --> O[legacy.avg_pitch_velocity fallback]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (battingRes.error || pitchingRes.error) { | ||
| return { data: [], error: 'Recent activity could not be loaded.' }; | ||
| } | ||
|
|
||
| const gameIds = [ | ||
| ...new Set([ | ||
| ...((battingRes.data ?? []) as Array<{ game_id: string }>).map((r) => r.game_id), | ||
| ...((pitchingRes.data ?? []) as Array<{ game_id: string }>).map((r) => r.game_id), | ||
| ]), | ||
| ]; | ||
| if (gameIds.length === 0) return { data: [], error: null }; | ||
|
|
||
| const { data: games, error: gamesErr } = await db | ||
| .from('baseball_games') | ||
| .select('id, game_date, opponent_name') |
There was a problem hiding this comment.
capturedSessions count semantics change for two-way players
The old code counted rows in baseball_player_stats (which could have separate batting and pitching rows per game for a two-way player). The new code counts distinct games via Set dedup in fetchRecentBoxScoreActivity. A pitcher who also bats now counts each game once instead of twice, so capturedSessions will be lower than before for those players. The change is semantically correct (a game is one activity, not two), but the passport's "captured sessions" label may have been calibrated to the old per-row count. Worth verifying that coaches viewing a two-way player's passport don't interpret the lower count as missing data.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/baseball/read-models/player-passport.ts
Line: 471-485
Comment:
**`capturedSessions` count semantics change for two-way players**
The old code counted rows in `baseball_player_stats` (which could have separate batting and pitching rows per game for a two-way player). The new code counts distinct games via `Set` dedup in `fetchRecentBoxScoreActivity`. A pitcher who also bats now counts each game once instead of twice, so `capturedSessions` will be lower than before for those players. The change is semantically correct (a game is one activity, not two), but the passport's "captured sessions" label may have been calibrated to the old per-row count. Worth verifying that coaches viewing a two-way player's passport don't interpret the lower count as missing data.
How can I resolve this? If you propose a fix, please make it concise.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!
…y stat layer (#379) Migrates the three remaining Phase-1 read-model consumers of the deprecated flat/aggregate stat tables onto the canonical layers, per the #379 reconciliation design: - player-today.ts: "recent activity" now reads baseball_box_score_batting/ _pitching (joined to baseball_games) instead of baseball_player_stats. Box-score rows carry no CSV-import provenance, so trust/provenance are honestly null rather than a fabricated stamp. Manifest entry deleted. - player-passport.ts: the "recent activity" counts card (capturedSessions/ lastSessionDate/lastSessionSource) migrates the same way. The #434 thirds-aware IP summation (summarizePitchingSeason, merged as #815) is untouched — it already read box-score data and needed no change. Manifest entry deleted. - player-snapshot-cards.ts: exit-velocity fields now derive from baseball_batted_ball_events via elite-stat-events.ts's own buildHitterMetrics aggregator (closes the file's former "typed but un-migrated" comment) instead of baseball_player_stats.exit_velocity. Still reads baseball_player_aggregates for the Hitting/Pitching legacy-fallback tier and the game/scrimmage/practice "Performance" card — no canonical replacement exists yet for a standalone scrimmage split or a practice-session shape (see stats-migration-plan.md's open question). Manifest note updated to reflect this partial, deliberate scope; entry not deleted since the file still references the deprecated table. Also closes the #828 residual: adaptLegacyPlayerStats never read the legacy row's avg_pitch_velocity/max_pitch_velocity columns. Wired through as a per-field fallback under an explicit event-grain reading (pitch velocity has a legitimate legacy scalar; exit velocity has no legacy equivalent and stays event-only/null-safe). Adapter tests extended to cover the fallback, precedence, and no-data cases. Updated src/contracts/baseball/product-trust/player-today-honest-loop.test.ts and src/contracts/baseball/access/player-today-self-scope.test.ts fixtures to match the box-score source (not in the original chunk file list, but directly affected — these tests exercise getPlayerToday's DB path). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa
…ce in #379 read paths (#845 review fix) The #379 migration of player-today.ts/player-passport.ts/player-snapshot- cards.ts onto the canonical box-score/event layer regressed legacy-only players — real history captured before the box-score/event-grain pipelines existed — from "shows real data" to a silent, honest-LOOKING empty state. None of the three read paths ever consulted the deprecated flat table as a fallback, so a genuine data-migration artifact was indistinguishable from a player who truly has no activity. - player-today.ts: fetchRecentBoxScoreActivity now falls back to baseball_player_stats ONLY when this player has zero box-score rows, restoring the pre-#379 stamped-provenance trust/provenance build. Every PlayerTodayStat entry now carries `sourceLayer` ('box-score' | 'legacy-fallback') so the UI can label an old number honestly. - player-passport.ts: fetchRecentActivity (renamed from fetchRecentBoxScoreActivity) applies the same fallback to recentActivity.capturedSessions/lastSessionDate/lastSessionSource/ lastSessionTrust/lastSessionProvenance and the compact-mode 'Captured stats' completeness signal. recentActivity now carries `sourceLayer` too. - player-snapshot-cards.ts: avgExitVelocity/maxExitVelocity fall back to the deprecated baseball_player_stats.exit_velocity column ONLY when this player has zero baseball_batted_ball_events rows. Extracted the precedence logic into a new pure, exported resolveExitVelocityFields() so it's directly unit-testable (the DB-bound getPlayerSnapshotCards itself stays integration-only, per this file's existing test strategy). All three mirror the box-score > legacy-fallback > no-data precedence legacy-stat-adapters.ts already enforces for aggregate rows; the two sources are never blended for the same player. Manifest: re-added grandfathered entries for player-today.ts and player-passport.ts (both now reference baseball_player_stats again, as an intentional fallback-only read), updated player-snapshot-cards.ts's note, and added entries for the two test files whose fixtures now reference the deprecated table. Tests: extended player-today-honest-loop.test.ts with a legacy-fallback describe block (shows real data, real stamped provenance, box-score precedence over legacy), added player-passport-recent-activity.test.ts (no prior DB-path coverage existed for getPlayerPassport), and added resolveExitVelocityFields unit tests to player-snapshot-cards.test.ts. 574 test files / 5206 tests green across src/lib/baseball + src/contracts/baseball + src/app/baseball. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa
1926c84 to
7ac6198
Compare
Review fix applied (rebased onto
|
1 participant
Summary
Implements the #379 reconciliation design's chunk "Migrate player-today.ts, player-snapshot-cards.ts, player-passport.ts", building on the already-merged Phase 0 seed fix (#827) and shared adapter module (#828).
Per-file changes
src/lib/baseball/read-models/player-today.tsrecentStats("last few captured stat sessions") now readsbaseball_box_score_batting/_pitching(game ids for this player) joined tobaseball_games, instead of the deprecatedbaseball_player_statstable.trust/provenanceare now honestlynullinstead of a fabricated import stamp.src/lib/baseball/read-models/player-passport.tscapturedSessions/lastSessionDate/lastSessionSource) migrates the same way (box-score games, not the flat table).summarizePitchingSeason, merged tonight as fix(baseball): finish #434 IP thirds-aware propagation (player-passport + scout-packet) #815) is untouched — it already reads box-score/season data viaassemblePerformanceand needed no change. Verified viaplayer-passport-innings.test.ts(all green, unmodified).src/lib/baseball/read-models/player-snapshot-cards.tsavgExitVelocity/maxExitVelocity) now derive frombaseball_batted_ball_eventsvia elite-stat-events.ts's own exportedbuildHitterMetricsaggregator (same official/scrimmage context default, same current-rowsuperseded_by_run_idfilter) — closes the file's former "typed but un-migrated" comment.baseball_player_aggregatesfor (a) the Hitting/Pitching season-average legacy-fallback tier and (b) the game/scrimmage/practice "Performance" card. Neither has a canonical replacement today —stats-center.tsexposes official-vs-all splits, not a standalone scrimmage split, and neither canonical layer has a practice-session shape yet (the design doc's own open question). Forcing a full cutover here would either regress real teams or require new scope outside this chunk. Manifest note updated to document this precisely; entry not deleted.src/lib/baseball/read-models/legacy-stat-adapters.ts+ its test (closes the #828 residual the verifier flagged)adaptLegacyPlayerStatsnever readlegacy.avg_pitch_velocity/max_pitch_velocity. Wired through as a per-field fallback: an explicit event-grain reading still wins outright when supplied, otherwise falls back to the legacy scalar (a real, previously-captured measurement — not a fabrication). Exit velocity has no legacy column at all and stays event-only/null-safe, unchanged.src/lib/baseball/stat-layer-manifest.ts— entries updated per above.Also touched (not in the original chunk file list, but directly affected):
src/contracts/baseball/product-trust/player-today-honest-loop.test.ts— its "recent stats never fabricate provenance" tests asserted againstbaseball_player_statsfixture rows; replaced with box-score/baseball_gamesfixtures pinning the same honesty invariant (trust/provenance always null).src/contracts/baseball/access/player-today-self-scope.test.ts— fixture table list swapped to the box-score tables so the self-scope contract keeps exercising the real (non-mocked)getPlayerTodayDB path.src/app/baseball/(player-dashboard)/player/today/__tests__/page.test.tsx— reviewed, no change needed (getPlayerToday/getPlayerPassportare fully mocked at the module boundary there).Gate evidence
npm run typecheck— clean.npx eslint --max-warnings 0 <8 touched files>— clean, no output.npx vitest runacross all affected/new test files (legacy-stat-adapters, roster-aggregates-merge, player-snapshot-cards, player-passport-innings, player-today-honest-loop, player-today-self-scope, stat-layer-contract, player/today page.test.tsx) — 31 test files / 249 tests, all passing.stat-layer-contract.test.ts(the BaseballHelm: consolidate legacy flat stats, box-score stats, and elite stat-event imports #381 CI guard) passes cleanly against the updated manifest — verified both directions (no new offenders, no stale entries for the two deleted files).Base-branch residuals fixed in passing (manifest only)
The #381 stat-layer contract test was already red on
batch/bbh-finish-0714before this PR, in both scan directions. Since this PR edits the manifest anyway, both bookkeeping residuals are corrected here (no production code touched for either):src/app/baseball/actions/insights.ts— fix(baseball): migrate discover.ts/insights.ts actions onto withBaseballAction (#394) #819 (merged tonight) migrated it off the deprecated tables but did not delete itsGRANDFATHERED_CONSUMERSentry in the same commit, tripping the contract test's stale-entry direction.src/app/baseball/actions/__tests__/practice-effectiveness.test.ts— test(baseball): add practice + practice-effectiveness action coverage #825 (merged tonight) landed this test with a fake-supabase fixture seedingbaseball_player_stats, without a manifest entry, tripping the offender direction. Entry added mirroring the grandfathered production file it tests (practice-effectiveness.ts, which stays excluded from this chunk per the design's open practice-shape question).Two other base-branch offenders (
player-today-self-scope.test.ts,player-today-honest-loop.test.ts, both from #826) are resolved naturally by this PR's fixture migration — they no longer reference a deprecated table at all.🤖 Generated with Claude Code
https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa