fix(baseball): distinct insight_type for hit/HR milestones (#478)#642
Merged
Conversation
…ersist (#478) Legacy analyzePlayer emitted both the hit-milestone and HR-milestone insights with insight_type 'development_milestone'. Reconciliation keys insights by `${playerId}::${insight_type}`, so when a player crossed both thresholds in one run only one milestone survived; the other was dropped or blocked the first from refreshing. Give the two branches distinct insight_type values (milestone_hits / milestone_hr) and add them to the BaseballInsightType union. Both now reconcile and refresh independently. No DB migration needed — baseball_coach_insights.insight_type is plain text with no CHECK constraint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017VUauUXsm9aXegShmpJi5n
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR Summary by QodoFix baseball milestone insights overwriting each other during reconciliation
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
93 rules 1. Legacy milestone state stranded
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+303
to
+306
| // 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', |
There was a problem hiding this comment.
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
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Owner
Author
|
🤖 Mission Control — PR summary Changes: Gives the hit-milestone and HR-milestone insights distinct |
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
1 participant
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.
What & Why
Fixes #478.
Legacy
analyzePlayerinsrc/app/baseball/actions/insights.tsemitted separate hit-milestone and HR-milestone insights, but both usedinsight_type: 'development_milestone'. Reconciliation keys insights by`${playerId}::${insight_type}`(lines 142-143), so when a player crossed both thresholds in one run (e.g. 50 hits and 10 HR), only one milestone survived — the other was dropped or blocked the first from refreshing. Coaches could miss an achievement entirely.Fix
insight_type: 'milestone_hits'.insight_type: 'milestone_hr'.BaseballInsightTypeunion insrc/lib/types/index.tsso types stay correct.Distinct types make the reconcile key naturally unique per milestone kind, so each is inserted/refreshed independently.
No DB migration required:
baseball_coach_insights.insight_typeis plaintextwith no CHECK constraint (verified in20260527000000_prod_public_baseline.sql). The substring-toleranttemplateForInsightTypeinpractice-intelligence.tsstill resolves safely (default template).Acceptance criteria
insight_typevalues (milestone_hits,milestone_hr).Scope
Minimal: 2 files, no auth changes, no
GRANT, no destructive writes, no migration.🤖 Generated with Claude Code