Skip to content

fix(baseball): distinct insight_type for hit/HR milestones (#478) #642

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
njrini99-code merged 1 commit into from
Jul 3, 2026
Merged

fix(baseball): distinct insight_type for hit/HR milestones (#478) #642

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/app/baseball/actions/insights.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,10 @@ function analyzePlayer(
if (totalHits >= 50 && totalHits % 25 === 0) {
insights.push({
player_id: playerId,
insight_type: 'development_milestone',
// Distinct from the HR milestone so both survive reconciliation, which
// keys insights by `${playerId}::${insight_type}`. A player crossing both
// a hit and an HR milestone in one run now persists two independent rows.
insight_type: 'milestone_hits',
Comment on lines +303 to +306

@qodo-code-review qodo-code-review Bot Jul 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. Legacy milestone state stranded 🐞 Bug ≡ Correctness

By changing milestone insights from insight_type 'development_milestone' to
'milestone_hits'/'milestone_hr', any existing persisted milestone rows will no longer match the
reconciliation key ${playerId}::${insight_type} and will never be refreshed or have their
dismissed/resolved status respected for the new types. The next run can therefore re-surface
milestones a coach previously dismissed/resolved (as “new” insight types) while leaving the legacy
rows behind.
Agent Prompt 
## Issue description
Milestone insights used to persist as `insight_type='development_milestone'`. This PR changes new emissions to `milestone_hits` / `milestone_hr`, but reconciliation is keyed by `${playerId}::${insight_type}` and does not delete/rename legacy rows. As a result, legacy milestone rows become orphaned from refresh logic and coach decisions (dismissed/resolved) won’t carry forward to the new milestone types.

## Issue Context
- Reconciliation uses `keyOf(player_id, insight_type)` and only inserts when no matching key exists; it also only respects dismissed/resolved for matching keys.
- After this PR, a team/coach with existing `development_milestone` rows will start inserting `milestone_hits` / `milestone_hr` rows instead of updating or respecting the prior row’s status.

## Fix Focus Areas
- src/app/baseball/actions/insights.ts[136-170]
- src/app/baseball/actions/insights.ts[295-334]

## Suggested fix approach
1. Add a backward-compat mapping step when building `existingByKey`:
   - If `row.insight_type === 'development_milestone'`, inspect `row.metadata` (or title) to infer whether it represents hits vs HR (e.g., `metadata.total_hits` -> `milestone_hits`, `metadata.total_hr` -> `milestone_hr`).
   - Register the row under the inferred new key as well, so updates land on the existing row and dismissed/resolved status remains effective.
2. Optionally, perform an in-action normalization update for active rows:
   - `UPDATE baseball_coach_insights SET insight_type='milestone_hits' ... WHERE insight_type='development_milestone' AND metadata ? 'total_hits'` (and similarly for HR), scoped to `{team_id, coach_id}`.
   - This prevents long-lived legacy rows and ensures future code paths only see the new types.
3. Keep a safe fallback for any `development_milestone` rows that can’t be classified (leave them as-is).

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

priority: 'low',
title: `${playerName} reached ${totalHits} hits!`,
description: `Career milestone: ${totalHits} hits in ${aggregates?.total_sessions || 0} sessions.`,
Expand All @@ -316,7 +319,9 @@ function analyzePlayer(
if (totalHR >= 10 && totalHR % 5 === 0) {
insights.push({
player_id: playerId,
insight_type: 'development_milestone',
// Distinct from the hits milestone (see above) so the HR milestone is
// reconciled and refreshed independently rather than overwriting it.
insight_type: 'milestone_hr',
priority: 'low',
title: `${playerName} hit ${totalHR} home runs!`,
description: `Power milestone achieved.`,
Expand Down
4 changes: 4 additions & 0 deletions src/lib/types/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,10 @@ export type BaseballInsightType =
| 'breakout_candidate'
| 'position_opportunity'
| 'development_milestone'
// Milestone-kind-specific types so a player crossing both a hit and an HR
// milestone in one run persists both (reconcile keys by playerId::insight_type).
| 'milestone_hits'
| 'milestone_hr'
| 'comparison_alert'
// Additional types for AI insights engine
| 'decline'
Expand Down
Loading