feat(scoring): record held / thrown-out runners on doubles and triples#184
Open
chanced58 wants to merge 6 commits into
Open
feat(scoring): record held / thrown-out runners on doubles and triples#184chanced58 wants to merge 6 commits into
chanced58 wants to merge 6 commits into
Conversation
…EventId Plays where the batter's hit credit is unaffected but an existing runner diverges from the default auto-advance (held short, thrown out advancing) can now be recorded via a linked BASERUNNER_OUT or BASERUNNER_ADVANCE event with relatedEventId pointing back to the parent HIT. deriveGameState builds a runner-overrides map in a single pre-pass and consults it in the HIT branch — overridden runners are skipped from default scoring and base placement so the linked event can remove them (BASERUNNER_OUT, increments outs) or place them at the specified base (BASERUNNER_ADVANCE). Same override map is built per-game inside deriveBattingStats so RBI auto-derivation no longer credits batters for runners who were held short or thrown out. Also hardens BASERUNNER_ADVANCE replay: the handler now only clears the fromBase slot if the runner there matches runnerId, preventing a linked override from wiping the batter that the parent HIT just placed. https://claude.ai/code/session_01MTbjN7Th2bm388VPs3pwC7
When a HIT carries linked BASERUNNER_OUT or BASERUNNER_ADVANCE events via relatedEventId, the at-bat result label now appends a parenthetical like "Double (Alice thrown out advancing)" or "Double (Bob held at 3B)" so the event feed clearly shows what happened on the basepaths. The running scoreline carried on each at-bat node also discounts runs that would have scored on a default auto-advance but were suppressed by a linked thrown-out or held outcome, keeping the per-at-bat home/away score consistent with deriveGameState. https://claude.ai/code/session_01MTbjN7Th2bm388VPs3pwC7
When the scorer taps 2B or 3B with runners on base, a follow-up modal prompts for each runner with three choices: "Advanced as expected" (default, one-tap pass-through), "Held at base X" (runner stops short of the default), or "Thrown out" (runner retired advancing). Confirming emits the HIT plus a linked BASERUNNER_OUT or BASERUNNER_ADVANCE event per non-default runner, attached via relatedEventId so the engine, RBI derivation, and play-feed label all reflect the actual outcome. Singles and home runs keep their one-tap path (no modal). Singles because atypical runner outcomes are rare; HRs because everyone scores by default. Cascade-undo: tapping Undo on a parent play now voids any linked runner-outcome events first, so a single Undo retires the full multi-event play. useRecordEvent now returns the persisted event id so callers can chain follow-up events that reference the parent via relatedEventId. https://claude.ai/code/session_01MTbjN7Th2bm388VPs3pwC7
Mirrors the mobile flow: when the scorer resolves an in-play double or triple with runners on base, a follow-up panel asks for each runner's outcome — "Advanced as expected" (default), "Held at base X", or "Thrown out advancing". Confirming emits the HIT plus any linked BASERUNNER_OUT / BASERUNNER_ADVANCE events with relatedEventId pointing to the parent so the engine, RBI derivation, and play-feed label all reflect the actual outcome. Singles and HRs keep their one-tap path. When no runners are on base the panel is skipped entirely. recordEvent now returns the persisted row id so the runner-outcome flow can attach linked events to the parent HIT. https://claude.ai/code/session_01MTbjN7Th2bm388VPs3pwC7
Adds a Domain Glossary entry for RunnerOutcome in CLAUDE.md and a matching subsection under OBR 5.06 in docs/baseball-rules.md explaining the linked-event pattern (relatedEventId on BASERUNNER_OUT / BASERUNNER_ADVANCE) that lets the scorer record held or thrown-out runners on a hit while keeping the batter's hit credit intact. https://claude.ai/code/session_01MTbjN7Th2bm388VPs3pwC7
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces a linked runner outcomes feature that allows scorekeepers to capture how runners actually advance on double/triple hits. When a hit occurs and runners are on base, a modal prompts for each runner's fate (auto-advance, held at a base, or thrown out), then records a parent HIT event and linked BASERUNNER_OUT/BASERUNNER_ADVANCE child events to override default scoring and base placement across the app. ChangesLinked Runner Outcomes
Sequence DiagramsequenceDiagram
participant Scorer
participant Recorder
participant HIT
participant Child
participant Derive
participant Stats
participant History
Scorer->>Recorder: recordEvent(HIT)
Recorder->>Scorer: returns eventId
Scorer->>HIT: emit HIT using eventId
alt some runners overridden
Scorer->>Child: emit BASERUNNER_* with relatedEventId
end
Derive->>Derive: build runnerOverridesByParentId
Derive->>Derive: skip overridden runners on HIT
Stats->>Stats: build override index
Stats->>Stats: exclude overridden runners in scoring
History->>History: format linked-runner parentheticals
History->>Scorer: display HIT label with parenthetical
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@apps/web/src/app/`(app)/games/[gameId]/score/ScoringBoard.tsx:
- Around line 1268-1297: confirmHitWithRunnerOutcomes is dropping pitch metadata
from pendingHitRunnerOutcomes; destructure pitchExtra from
pendingHitRunnerOutcomes alongside hitType/trajectory/sprayExtra and (1) call
recordEvent('pitch_thrown', { ...pitchExtra, pitcherId, relatedEventId: <some
id?> }) before recording the hit to mirror the normal path, and (2) include
pitchExtra in the recordEvent('hit', { batterId, pitcherId, hitType, trajectory,
...sprayExtra, ...pitchExtra }) payload so pitch type/zone are preserved; update
references to pendingHitRunnerOutcomes, confirmHitWithRunnerOutcomes, and
recordEvent to implement this.
In `@packages/shared/src/utils/__tests__/game-history.test.ts`:
- Around line 260-299: The test declares constants named in camelCase (pitch,
hit, out, held, tree, atBat) which violate the SCREAMING_SNAKE_CASE rule; rename
those bindings to PITCH, HIT, OUT, HELD, TREE, AT_BAT across the same test block
and update all uses (including HIT.id passed to relatedEventId and the call to
buildGameHistoryTree and subsequent at-bat assertions) so mkEvent and
buildGameHistoryTree usages still reference the renamed symbols.
- Around line 287-301: The test uses a conditional guard around assertions on
atBat which lets the test pass vacuously if the node is missing or the shape
regresses; update the test that calls buildGameHistoryTree and references atBat
so it first asserts the node exists and has the expected shape (e.g., add
expect(atBat).toBeDefined() and expect(atBat?.type).toBe('at-bat') or similar)
before asserting atBat.result.label === 'Single' (or other expectations), so
failures are explicit; change the conditional-if to a required precondition
check on atBat and then perform the label assertion.
In `@packages/shared/src/utils/__tests__/game-state.test.ts`:
- Around line 455-566: Rename the newly introduced test-local constants to
SCREAMING_SNAKE_CASE and update their usages: e.g., pa1 -> PA1, pitch2 ->
PITCH2, hit2 -> HIT2, out1 -> OUT1, held -> HELD, thrownOut -> THROWN_OUT, hr ->
HR (and EVENTS -> EVENTS if you rename that array), keeping the same values and
references inside the test cases (look for variables created via batterHit,
e(EventType.*), and the events arrays passed into deriveGameState); ensure all
occurrences in this test suite (game-state.test.ts) are updated so the tests
still reference the renamed constants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b696aee-0bb3-4cce-838a-9169c66c8132
📒 Files selected for processing (11)
CLAUDE.mdapps/mobile/app/(tabs)/games/[gameId]/score.tsxapps/mobile/src/features/scoring/PitchInput.tsxapps/mobile/src/features/scoring/use-record-event.tsapps/web/src/app/(app)/games/[gameId]/score/ScoringBoard.tsxdocs/baseball-rules.mdpackages/shared/src/utils/__tests__/game-history.test.tspackages/shared/src/utils/__tests__/game-state.test.tspackages/shared/src/utils/batting-stats.tspackages/shared/src/utils/game-history.tspackages/shared/src/utils/game-state.ts
Comment on lines
+260
to
+299
| const pitch = mkEvent(EventType.PITCH_THROWN, { batterId: 'p2', pitcherId: 'pit1', outcome: PitchOutcome.IN_PLAY }); | ||
| const hit = mkEvent(EventType.HIT, { batterId: 'p2', pitcherId: 'pit1', hitType: HitType.DOUBLE }); | ||
| const out = mkEvent(EventType.BASERUNNER_OUT, { | ||
| runnerId: 'p1', | ||
| fromBase: 1, | ||
| relatedEventId: hit.id, | ||
| }); | ||
| const tree = buildGameHistoryTree([pitch, hit, out], players); | ||
| const atBat = tree.innings[0]?.top?.items[0]; | ||
| expect(atBat?.type).toBe('at-bat'); | ||
| if (atBat?.type === 'at-bat') { | ||
| expect(atBat.result?.label).toContain('Double'); | ||
| expect(atBat.result?.label).toContain('Alice Atbat thrown out advancing'); | ||
| } | ||
| }); | ||
|
|
||
| it('appends "held at 3B" to the HIT label when a BASERUNNER_ADVANCE is linked with toBase < default', () => { | ||
| const pitch = mkEvent(EventType.PITCH_THROWN, { batterId: 'p2', pitcherId: 'pit1', outcome: PitchOutcome.IN_PLAY }); | ||
| const hit = mkEvent(EventType.HIT, { batterId: 'p2', pitcherId: 'pit1', hitType: HitType.DOUBLE }); | ||
| const held = mkEvent(EventType.BASERUNNER_ADVANCE, { | ||
| runnerId: 'p1', | ||
| fromBase: 2, | ||
| toBase: 3, | ||
| reason: 'on_play', | ||
| relatedEventId: hit.id, | ||
| }); | ||
| const tree = buildGameHistoryTree([pitch, hit, held], players); | ||
| const atBat = tree.innings[0]?.top?.items[0]; | ||
| if (atBat?.type === 'at-bat') { | ||
| expect(atBat.result?.label).toContain('Double'); | ||
| expect(atBat.result?.label).toContain('Alice Atbat held at 3B'); | ||
| } | ||
| }); | ||
|
|
||
| it('does not append parenthetical when no linked outcome events exist', () => { | ||
| const pitch = mkEvent(EventType.PITCH_THROWN, { batterId: 'p2', pitcherId: 'pit1', outcome: PitchOutcome.IN_PLAY }); | ||
| const hit = mkEvent(EventType.HIT, { batterId: 'p2', pitcherId: 'pit1', hitType: HitType.SINGLE }); | ||
| const tree = buildGameHistoryTree([pitch, hit], players); | ||
| const atBat = tree.innings[0]?.top?.items[0]; | ||
| if (atBat?.type === 'at-bat') { |
There was a problem hiding this comment.
Rename newly added constants to SCREAMING_SNAKE_CASE.
New constants in this block (pitch, hit, out, held, tree, atBat) violate the repo naming rule for constants in TS test files.
As per coding guidelines: "**/*.{ts,tsx,js,jsx}: Constants must use SCREAMING_SNAKE_CASE naming (e.g., MAX_ROSTER_SIZE)."
🤖 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 `@packages/shared/src/utils/__tests__/game-history.test.ts` around lines 260 -
299, The test declares constants named in camelCase (pitch, hit, out, held,
tree, atBat) which violate the SCREAMING_SNAKE_CASE rule; rename those bindings
to PITCH, HIT, OUT, HELD, TREE, AT_BAT across the same test block and update all
uses (including HIT.id passed to relatedEventId and the call to
buildGameHistoryTree and subsequent at-bat assertions) so mkEvent and
buildGameHistoryTree usages still reference the renamed symbols.
Owner
Author
There was a problem hiding this comment.
Skipping. The MAX_ROSTER_SIZE rule in CLAUDE.md applies to module-level configuration constants, not local-scope bindings inside test cases. Every existing test in this file (and across packages/shared/src/utils/__tests__/) uses camelCase for const events, const tree, const atBat, const thrownOut, etc. — see lines 39–129 of this same file. Renaming only the new block would diverge from the established style.
Generated by Claude Code
There was a problem hiding this comment.
@chanced58, that's a valid distinction — the existing test file clearly uses camelCase for all local test-scope bindings (events, tree, atBat, thrownOut, etc.), and applying SCREAMING_SNAKE_CASE to those local variables would diverge from the established style. Acknowledged, and I'll discard this comment.
✏️ Learnings added
Learnt from: chanced58
Repo: chanced58/diamondos PR: 184
File: packages/shared/src/utils/__tests__/game-history.test.ts:0-0
Timestamp: 2026-05-28T23:03:36.979Z
Learning: In the `diamondos` repository, the SCREAMING_SNAKE_CASE constant naming rule (e.g., `MAX_ROSTER_SIZE`) in CLAUDE.md applies only to **module-level configuration constants**, not to local-scope bindings inside test cases. Test files under `packages/shared/src/utils/__tests__/` (and throughout the codebase) use camelCase for local test variables such as `const events`, `const tree`, `const atBat`, `const thrownOut`, etc. Do not flag camelCase local test bindings as a naming violation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
- ScoringBoard: drop dead `pitchExtra` field from pendingHitRunnerOutcomes state. The `pitch_thrown` event was already recorded earlier in handleInPlay (line 1196) before the early return that opens the runner-outcomes panel, so capturing pitchExtra here was unused state. HitPayload has no pitchType field, so there is nothing to add to the HIT event either. - game-history.test.ts: replace conditional `if (atBat?.type === 'at-bat')` guards with hard preconditions that throw on mismatch, so the at-bat assertions cannot pass vacuously if the tree shape regresses. https://claude.ai/code/session_01MTbjN7Th2bm388VPs3pwC7
|
Actionable comments posted: 0 |
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
Adds a way for the scorer to record plays where the batter's hit credit stands but an existing runner diverges from the default auto-advance — held short of the expected base, or thrown out advancing. Covers the two scenarios you raised:
The same pattern generalizes to any hit where the auto-advance can be overridden per runner.
How it works
BASERUNNER_OUTorBASERUNNER_ADVANCEevent whoserelatedEventIdpoints back to the parentHIT. No schema migration — the existing JSONB payload +relatedEventIdfield already supports it.deriveGameStateand the stats reducers build a pre-pass map of overrides keyed by parent event id and skip default scoring + base placement for any runner referenced by a linked outcome event. RBI auto-derivation honors the same map so a batter never gets RBI credit for a runner who was held or thrown out.Commits
feat(shared/scoring)— engine + stats override logic, plus a defensive fix to BASERUNNER_ADVANCE replay so a linked override doesn't wipe the batter the parent HIT just placed.feat(shared/game-history)— parenthetical on hit rows + at-bat running scoreline correction.feat(mobile/scoring)— runner-outcomes modal inPitchInput+ cascade-undo inhandleUndo.useRecordEventnow returns the persisted event id so callers can chain follow-ups.feat(web/scoring)— equivalent panel inScoringBoard.docs— Domain Glossary + OBR 5.06 cross-reference.Test plan
pnpm --filter @baseball/shared test— 319 tests pass (6 new for game-state + 3 new for game-history covering Scenarios 1, 2a, 2b, the regression fast path, a triple with held runner, and an HR with a trailing thrown-out runner).pnpm --filter web exec tsc --noEmit— clean.https://claude.ai/code/session_01MTbjN7Th2bm388VPs3pwC7
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests