fix(baseball): compute OBP/SLG/OPS in legacy aggregate recalc (#436) by njrini99-code · Pull Request #646 · njrini99-code/helmv3

njrini99-code · 2026-07-01T16:54:42Z

What & why

recalculatePlayerAggregates (src/app/baseball/actions/stats.ts) only persisted career_avg, but My Stats StatsOverviewCards reads career_obp / career_slg / career_ops — columns that never existed on the live baseball_player_aggregates table. Players with CSV/practice upload data saw AVG populated but OBP/SLG/OPS permanently show ---, despite having the counting stats.

Changes

Migration supabase/migrations/20260701020000_baseball_player_aggregates_slash_line.sql — idempotent ADD COLUMN IF NOT EXISTS career_obp/career_slg/career_ops numeric. Committed only, not applied to any database.
New pure helper src/lib/baseball/aggregates/career-slash-line.ts — computeCareerSlashLine() sums counting stats and applies standard formulas:
- OBP = (H + BB + HBP) / (AB + BB + HBP + SF)
- SLG = total bases / AB, total bases = H + 2B + 2·3B + 3·HR
- OPS = OBP + SLG
- Null on any zero denominator (never a fabricated .000).
stats.ts now derives and upserts the slash line via the helper (extracted so it is testable without a DB; stats.ts is 'use server').
Unit test src/lib/baseball/__tests__/career-slash-line.test.ts — multi-session aggregate with walks/SF/HBP + zero-denominator honesty cases.

Acceptance criteria

recalculatePlayerAggregates derives and stores career_obp, career_slg, career_ops from summed counting stats (standard formulas, null on zero denominators).
My Stats overview cards will show the computed slash line after upload (reads these fields; now written).
Unit test covers a multi-session aggregate with walks/SF/HBP.

Note: the migration must be applied for the new columns to persist; per task directive it is committed only, not applied.

🤖 Generated with Claude Code

recalculatePlayerAggregates only wrote career_avg, but My Stats StatsOverviewCards reads career_obp/career_slg/career_ops — columns that never existed on baseball_player_aggregates, so OBP/SLG/OPS permanently showed "---" despite sufficient counting stats. - Add idempotent migration adding career_obp/career_slg/career_ops (numeric) to baseball_player_aggregates (ADD COLUMN IF NOT EXISTS). - Extract a pure computeCareerSlashLine helper (standard formulas, null on zero denominator) and call it from recalculatePlayerAggregates, upserting the derived slash line. - Unit test covers a multi-session aggregate with walks/SF/HBP plus the zero-denominator honesty cases. Migration is committed only, not applied. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017VUauUXsm9aXegShmpJi5n

chatgpt-codex-connector · 2026-07-01T16:54:52Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

vercel · 2026-07-01T16:54:54Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
helmv3	Ready	Preview, Comment	Jul 1, 2026 5:00pm

qodo-code-review · 2026-07-01T16:55:52Z

PR Summary by Qodo

Fix baseball aggregate recalc to persist career OBP/SLG/OPS

🐞 Bug fix 🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

AI Description

• Add DB columns for career OBP/SLG/OPS so My Stats can display slash line.
• Compute career OBP/SLG/OPS from summed counting stats during aggregate recalculation.
• Add unit tests for multi-session slash line math and zero-denominator null behavior.

Diagram

graph TD
  UI["StatsOverviewCards"] -->|"reads"| DB[("baseball_player_aggregates")]
  A["recalculatePlayerAggregates"] --> B["stats.ts"] --> H["computeCareerSlashLine()"] --> B
  B -->|"upsert obp/slg/ops"| DB
  M["Migration SQL"] -->|"ADD COLUMN"| DB
  subgraph Legend
    direction LR
    _ui["UI"] ~~~ _svc["Server action"] ~~~ _fn["Pure helper"] ~~~ _db[("Database")] ~~~ _mig["Migration"]
  end

High-Level Assessment

The following are alternative approaches to this PR:

1. Compute slash line at read time (UI or API)

➕ Avoids schema change and backfill concerns
➕ Always derived from the latest counting stats
➖ Pushes baseball math into UI/data-fetch layer
➖ Can be inconsistent if multiple consumers reimplement formulas
➖ Harder to index/query or export precomputed rates

2. Database-side computed view/materialized view

➕ Centralizes formulas in SQL for all consumers
➕ Can avoid storing redundant columns in the base table
➖ More complex to deploy/maintain (views, refresh strategy)
➖ Still needs careful null/zero-denominator semantics
➖ May not fit existing upsert-based aggregate pipeline

3. Backfill migration/job to populate existing players immediately

➕ Fixes historical rows without requiring a new recalc run
➕ Better immediate UX for existing users
➖ Operationally heavier (one-off job, timeouts, monitoring)
➖ More risk than incremental “fill on next recalc” approach

Recommendation: Keep the PR’s approach (pure helper + upsert + idempotent migration). It matches the existing aggregate pipeline, keeps the formulas testable in TypeScript, and avoids operational risk. Consider a follow-up backfill if product needs immediate historical correctness without waiting for a recalc trigger.

Files changed (4) +150 / -0

Enhancement (1) +61 / -0

career-slash-line.tsAdd pure computeCareerSlashLine() helper with null-on-zero-denominator contract +61/-0

Add pure computeCareerSlashLine() helper with null-on-zero-denominator contract
• Introduces a testable utility that aggregates counting stats and applies standard OBP/SLG/OPS formulas. Returns null for rates when denominators are zero, and null OPS when either component is null.
src/lib/baseball/aggregates/career-slash-line.ts

Bug fix (1) +8 / -0

stats.tsCompute and upsert career OBP/SLG/OPS during aggregate recalculation +8/-0

Compute and upsert career OBP/SLG/OPS during aggregate recalculation
• Imports a new pure helper to compute OBP/SLG/OPS from summed counting stats. Extends the aggregate upsert payload to persist career_obp, career_slg, and career_ops alongside career_avg.
src/app/baseball/actions/stats.ts

Tests (1) +61 / -0

career-slash-line.test.tsUnit tests for multi-session slash line aggregation and edge cases +61/-0

Unit tests for multi-session slash line aggregation and edge cases
• Adds vitest coverage for multi-session aggregation including BB/HBP/SF effects on OBP. Verifies zero-denominator behavior (no fabricated .000) and the walks-only case (OBP non-null, SLG/OPS null).
src/lib/baseball/tests/career-slash-line.test.ts

Other (1) +20 / -0

20260701020000_baseball_player_aggregates_slash_line.sqlAdd career_obp/career_slg/career_ops columns to baseball_player_aggregates +20/-0

Add career_obp/career_slg/career_ops columns to baseball_player_aggregates
• Creates an idempotent migration that adds numeric career_obp, career_slg, and career_ops columns. Does not attempt backfill; values will populate on the next aggregate recalculation.
supabase/migrations/20260701020000_baseball_player_aggregates_slash_line.sql

qodo-code-review · 2026-07-01T17:03:16Z

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (2) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 93 rules

✅ Skills: modern-saas-ui, pproenca-dot-skills-framer-motion

1. Schema rollout breaks recalc 🐞 Bug ☼ Reliability

Description

recalculatePlayerAggregates now always upserts career_obp/career_slg/career_ops, so on any DB where
those columns don’t exist yet the upsert will error and no aggregates (including career_avg) will be
persisted for that run. uploadStatsCSV awaits recalculatePlayerAggregates but does not check its
returned success flag, so uploads can still report success while aggregates stop updating until the
migration is applied.

Code

src/app/baseball/actions/stats.ts[R592-594]

+    career_obp: careerObp,
+    career_slg: careerSlg,
+    career_ops: careerOps,

Relevance

⭐⭐⭐ High
Team fixes schema-drift/run-failures quickly; recent PRs reconcile missing columns and stop
swallowed errors.
PR-#618
PR-#635

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The baseline schema in-repo shows baseball_player_aggregates currently lacks these slash-line
columns, while the PR’s action now writes them unconditionally; therefore, until the migration is
applied to a given database, the upsert will fail and prevent aggregate updates.
supabase/migrations/20260527000000_prod_public_baseline.sql[7795-7813]
src/app/baseball/actions/stats.ts[555-612]
supabase/migrations/20260701020000_baseball_player_aggregates_slash_line.sql[13-20]
src/app/baseball/actions/stats.ts[290-294]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`recalculatePlayerAggregates` unconditionally writes the new `career_obp/career_slg/career_ops` columns. If code reaches an environment where the migration hasn’t been applied, Postgres will reject the upsert (`column does not exist`), which prevents *any* aggregate fields from being updated for that player.

## Issue Context
This is a common rollout ordering hazard (code deploy before schema migration). Because `uploadStatsCSV` triggers aggregate recalculation and doesn’t act on the returned `{ success: false }`, the user flow can appear successful while aggregates silently fail to update.

## Fix Focus Areas
- src/app/baseball/actions/stats.ts[555-619]

### Implementation guidance
- Keep the new behavior when the columns exist.
- Add a defensive fallback around the aggregates upsert:
 - Try upsert with `career_obp/career_slg/career_ops`.
 - If the error indicates missing columns (e.g., message includes `career_obp` / Postgres undefined_column `42703` if exposed), retry a second upsert with those keys omitted so legacy aggregates (AVG/etc.) still update.
 - Log a clear warning (once per request) when falling back, so ops can detect an out-of-order rollout.
- Do **not** swallow other errors; only fallback on the specific missing-column case.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Slash line change undocumented 📘 Rule violation ⚙ Maintainability

Description

This PR changes business behavior by computing and persisting career_obp, career_slg, and
career_ops for Baseball player aggregates, but no corresponding memory/features/* documentation
update (or explicit in-code rationale for skipping it) is included. This can leave internal feature
docs out of sync with user-visible stats behavior.

Code

src/app/baseball/actions/stats.ts[R592-594]

+    career_obp: careerObp,
+    career_slg: careerSlg,
+    career_ops: careerOps,

Relevance

⭐⭐ Medium
Memory doc/registry enforcement was only partially accepted historically; unclear for this feature.
PR-#296

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The changed code now computes and upserts career_obp, career_slg, and career_ops, and the
migration adds the corresponding columns—this is a user-visible stats behavior change. The registry
entry for baseball_core does not point to a memory/features/* doc, and the changed code does not
include an explicit justification for omitting a memory doc update.
Rule 1519257: Update memory feature docs when changing business behavior
src/app/baseball/actions/stats.ts[555-606]
src/lib/baseball/aggregates/career-slash-line.ts[29-61]
supabase/migrations/20260701020000_baseball_player_aggregates_slash_line.sql[1-20]
memory/registry.yml[110-147]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Business behavior changed (Baseball aggregate recalculation now derives and persists OBP/SLG/OPS), but the change set does not update `memory/features/*` docs or include an explicit explanation in code for why no doc update is needed.

## Issue Context
- The PR introduces a new derivation helper and persists three new aggregate columns.
- The feature registry indicates Baseball is tracked as `baseball_core`, but its docs currently point at `CLAUDE.md` rather than a `memory/features/*` doc.

## Fix Focus Areas
- src/app/baseball/actions/stats.ts[555-606]
- src/lib/baseball/aggregates/career-slash-line.ts[29-61]
- supabase/migrations/20260701020000_baseball_player_aggregates_slash_line.sql[1-20]
- memory/registry.yml[110-147]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. stats.ts outside actions directory 📘 Rule violation ⌂ Architecture

Description

The file src/app/baseball/actions/stats.ts is a server action module ('use server') but it is
not located under src/app/actions/ as required. This increases the risk of inconsistent server
action organization and discoverability across the codebase.

Code

src/app/baseball/actions/stats.ts[22]
+import { computeCareerSlashLine } from '@/lib/baseball/aggregates/career-slash-line';

Relevance

⭐ Low
Repo keeps server actions in feature folders (baseball/actions) as in merged stats.ts guard PR.
PR-#579

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The checklist requires server actions (modules marked with 'use server') to live under
src/app/actions/. The modified module starts with 'use server' but is located under
src/app/baseball/actions/.
Rule 1519234: Place Next.js server actions in src/app/actions directory
src/app/baseball/actions/stats.ts[1-22]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`src/app/baseball/actions/stats.ts` is a server action module (it begins with `'use server'`) but it lives outside `src/app/actions/`, violating the required server action placement convention.

## Issue Context
This PR modifies the server action module, so it’s a good time to align the file location with the required structure.

## Fix Focus Areas
- src/app/baseball/actions/stats.ts[1-22]
- src/app/actions/baseball/stats.ts[1-200]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

4. Repeated stats array scans 🐞 Bug ➹ Performance

Description

computeCareerSlashLine iterates the full stats array once per field (8 reductions), adding avoidable
CPU work when recalculating many sessions/players. A single-pass accumulator would compute all
totals in one traversal and reduce overhead in team-wide recalculation flows.

Code

src/lib/baseball/aggregates/career-slash-line.ts[R37-47]

+  const sum = (pick: (s: SlashLineStatRow) => number | null | undefined): number =>
+    stats.reduce((acc, s) => acc + (pick(s) || 0), 0);
+
+  const atBats = sum((s) => s.at_bats);
+  const hits = sum((s) => s.hits);
+  const doubles = sum((s) => s.doubles);
+  const triples = sum((s) => s.triples);
+  const homeRuns = sum((s) => s.home_runs);
+  const walks = sum((s) => s.walks);
+  const hitByPitch = sum((s) => s.hit_by_pitch);
+  const sacFlies = sum((s) => s.sacrifice_flies);

Relevance

⭐ Low
No evidence team enforces single-pass micro-optimizations; similar perf nits often not acted on.
ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The helper defines a sum() reducer and then calls it once per field, which necessarily scans the
same stats array repeatedly.
src/lib/baseball/aggregates/career-slash-line.ts[34-47]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`computeCareerSlashLine` computes each total via a separate `reduce`, resulting in 8 full passes over the same array. This is avoidable overhead in aggregate recalculation paths.

## Issue Context
This helper is called from `recalculatePlayerAggregates`, which is invoked per affected player (and can also be invoked in team-wide loops).

## Fix Focus Areas
- src/lib/baseball/aggregates/career-slash-line.ts[34-60]

### Implementation guidance
- Replace the `sum(...)` helper + repeated calls with one `for ... of` (or a single `reduce`) that accumulates `atBats, hits, doubles, triples, homeRuns, walks, hitByPitch, sacFlies` in one pass.
- Keep the same null-on-zero-denominator behavior and existing test expectations.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

qodo-code-review · 2026-07-01T17:03:17Z

+    career_obp: careerObp,
+    career_slg: careerSlg,
+    career_ops: careerOps,


2. Slash line change undocumented 📘 Rule violation ⚙ Maintainability

This PR changes business behavior by computing and persisting career_obp, career_slg, and career_ops for Baseball player aggregates, but no corresponding memory/features/* documentation update (or explicit in-code rationale for skipping it) is included. This can leave internal feature docs out of sync with user-visible stats behavior.

Agent Prompt

## Issue description Business behavior changed (Baseball aggregate recalculation now derives and persists OBP/SLG/OPS), but the change set does not update `memory/features/*` docs or include an explicit explanation in code for why no doc update is needed. ## Issue Context - The PR introduces a new derivation helper and persists three new aggregate columns. - The feature registry indicates Baseball is tracked as `baseball_core`, but its docs currently point at `CLAUDE.md` rather than a `memory/features/*` doc. ## Fix Focus Areas - src/app/baseball/actions/stats.ts[555-606] - src/lib/baseball/aggregates/career-slash-line.ts[29-61] - supabase/migrations/20260701020000_baseball_player_aggregates_slash_line.sql[1-20] - memory/registry.yml[110-147]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

qodo-code-review · 2026-07-01T17:03:17Z

+    career_obp: careerObp,
+    career_slg: careerSlg,
+    career_ops: careerOps,


3. Schema rollout breaks recalc 🐞 Bug ☼ Reliability

recalculatePlayerAggregates now always upserts career_obp/career_slg/career_ops, so on any DB where those columns don’t exist yet the upsert will error and no aggregates (including career_avg) will be persisted for that run. uploadStatsCSV awaits recalculatePlayerAggregates but does not check its returned success flag, so uploads can still report success while aggregates stop updating until the migration is applied.

Agent Prompt

## Issue description `recalculatePlayerAggregates` unconditionally writes the new `career_obp/career_slg/career_ops` columns. If code reaches an environment where the migration hasn’t been applied, Postgres will reject the upsert (`column does not exist`), which prevents *any* aggregate fields from being updated for that player. ## Issue Context This is a common rollout ordering hazard (code deploy before schema migration). Because `uploadStatsCSV` triggers aggregate recalculation and doesn’t act on the returned `{ success: false }`, the user flow can appear successful while aggregates silently fail to update. ## Fix Focus Areas - src/app/baseball/actions/stats.ts[555-619] ### Implementation guidance - Keep the new behavior when the columns exist. - Add a defensive fallback around the aggregates upsert: - Try upsert with `career_obp/career_slg/career_ops`. - If the error indicates missing columns (e.g., message includes `career_obp` / Postgres undefined_column `42703` if exposed), retry a second upsert with those keys omitted so legacy aggregates (AVG/etc.) still update. - Log a clear warning (once per request) when falling back, so ops can detect an out-of-order rollout. - Do **not** swallow other errors; only fallback on the specific missing-column case.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

njrini99-code · 2026-07-01T18:20:06Z

🤖 Mission Control — PR summary

Changes: Persists career OBP/SLG/OPS in recalculatePlayerAggregates via a new computeCareerSlashLine helper + an idempotent migration adding career_obp/slg/ops to baseball_player_aggregates. Closes #436.
Areas / risk: DB migration (committed, not yet applied) + aggregate recalc path.
Watch: The migration must be applied in prod or the new columns won’t exist; helper returns null on a zero denominator.
CI: ❌ Supabase lint + RLS tests, Playwright failing — review before merge. (lighthouse-preview = known noise.)
Note: Bundled into batch #650.

vercel Bot deployed to Preview July 1, 2026 17:00 View deployment

qodo-code-review Bot reviewed Jul 1, 2026

View reviewed changes

njrini99-code merged commit 892e087 into main Jul 3, 2026
32 of 38 checks passed

njrini99-code deleted the fix/baseball-436-auto branch July 3, 2026 01:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(baseball): compute OBP/SLG/OPS in legacy aggregate recalc (#436)#646

fix(baseball): compute OBP/SLG/OPS in legacy aggregate recalc (#436)#646
njrini99-code merged 1 commit into
mainfrom
fix/baseball-436-auto

njrini99-code commented Jul 1, 2026

Uh oh!

chatgpt-codex-connector Bot commented Jul 1, 2026

Uh oh!

vercel Bot commented Jul 1, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Jul 1, 2026

Uh oh!

qodo-code-review Bot commented Jul 1, 2026

Uh oh!

qodo-code-review Bot Jul 1, 2026

Uh oh!

qodo-code-review Bot Jul 1, 2026

Uh oh!

njrini99-code commented Jul 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

njrini99-code commented Jul 1, 2026

What & why

Changes

Acceptance criteria

Uh oh!

chatgpt-codex-connector Bot commented Jul 1, 2026

Uh oh!

vercel Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qodo-code-review Bot commented Jul 1, 2026

PR Summary by Qodo

Uh oh!

qodo-code-review Bot commented Jul 1, 2026

Code Review by Qodo

Uh oh!

qodo-code-review Bot Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

qodo-code-review Bot Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

njrini99-code commented Jul 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vercel Bot commented Jul 1, 2026 •

edited

Loading