feat(livestats): per-player killedBy + deathPosition + winner for live standings#4593
Conversation
…e standings The admin-bot live-stats snapshot already carries tilesOwned + publicID, but not who eliminated a player, where they finished, or whether the game is decided, so a semi-live standings board can't score kills or placement off it and must wait for the post-game record. Surface those live. All deterministic sim values (so they survive the per-turn majority consensus), and the full roster is reported (dead players are retained in the client view): - PlayerImpl: killedBy (eliminator clientID) stamped at recordKill; deathPosition (finishing place = non-bot players still standing + 1) stamped when the player hits zero tiles in PlayerExecution. Idempotent, first write wins. - Threaded through the Player interface, the PlayerUpdate diff/apply, PlayerView, and the live-stats wire schema. - GameServer.liveStats() adds the decided winner's clientID (server-side, from the winner vote) alongside the existing publicID/username/connected enrichment. Tests: diff + apply cover the new fields; fixtures updated; full suite green.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughElimination metadata now records killer identity and finishing position, propagates through player updates and renderer state, and appears in live-stat snapshots. The server live-stats response also reports the player winner client ID when available. ChangesElimination live standings
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GameImpl
participant StatsImpl
participant PlayerImpl
participant LiveStatsController
GameImpl->>StatsImpl: record killer and death position
StatsImpl->>PlayerImpl: expose death stats in PlayerUpdate
PlayerImpl->>LiveStatsController: provide player snapshot fields
LiveStatsController->>LiveStatsController: emit enriched live stats
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 2
🧹 Nitpick comments (2)
tests/server/LiveStats.test.ts (2)
58-67: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a non-null elimination metadata case.
This fixture only verifies
killedBy: nullanddeathPosition: null. Add a snapshot with a real killer ID and positive death position, then assert consensus preserves both fields.Based on the PR objective requiring live elimination attribution and placement.
🤖 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 `@tests/server/LiveStats.test.ts` around lines 58 - 67, Extend the snapshot fixture and its consensus assertions in the LiveStats tests to cover a player with a non-null killedBy value and positive deathPosition. Verify the consensus result preserves both elimination metadata fields, while retaining the existing null-field coverage.
94-94: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest a player winner, not only
null.Both new expectations cover the undecided case. Add a winner vote such as
["player", "client01"]and assert thatliveStats().winnerreturns that client ID; otherwise an implementation that always returnsnullwould still pass these tests.Based on the new server-side winner contract.
Also applies to: 157-157
🤖 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 `@tests/server/LiveStats.test.ts` at line 94, Add a decided-winner case to the LiveStats tests near the existing winner expectations: provide a winner vote such as ["player", "client01"] and assert that liveStats().winner returns "client01", while retaining the existing null/undecided coverage.
🤖 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 `@src/core/execution/PlayerExecution.ts`:
- Around line 72-79: Record death position immediately in
GameImpl.conquerPlayer() when a player is conquered, calculating the alive
non-bot player count while excluding the conquered player and assigning that
count plus one. Retain the existing fallback calculation in PlayerExecution’s
elimination branch for deaths not handled through conquest, avoiding duplicate
or conflicting updates.
In `@src/core/game/PlayerImpl.ts`:
- Around line 143-149: Update the _killedBy state and markKilledBy flow so an
explicitly recorded null remains a finalized first result and cannot be
overwritten by later calls. Distinguish the unset state from a stamped null,
using a separate flag or an internal undefined sentinel, while preserving the
existing first-write-wins behavior for both killer and death position.
---
Nitpick comments:
In `@tests/server/LiveStats.test.ts`:
- Around line 58-67: Extend the snapshot fixture and its consensus assertions in
the LiveStats tests to cover a player with a non-null killedBy value and
positive deathPosition. Verify the consensus result preserves both elimination
metadata fields, while retaining the existing null-field coverage.
- Line 94: Add a decided-winner case to the LiveStats tests near the existing
winner expectations: provide a winner vote such as ["player", "client01"] and
assert that liveStats().winner returns "client01", while retaining the existing
null/undecided coverage.
🪄 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: CHILL
Plan: Pro
Run ID: c3c1d60f-f70e-40a1-b78a-8bf29d13201d
📒 Files selected for processing (15)
src/client/controllers/LiveStatsController.tssrc/client/render/types/Renderer.tssrc/client/view/PlayerView.tssrc/core/Schemas.tssrc/core/execution/PlayerExecution.tssrc/core/game/Game.tssrc/core/game/GameImpl.tssrc/core/game/GameUpdateUtils.tssrc/core/game/GameUpdates.tssrc/core/game/PlayerImpl.tssrc/server/GameServer.tstests/GameUpdateUtils.test.tstests/client/render/frame/derive/nuke-telegraphs.test.tstests/client/render/frame/derive/player-status.test.tstests/server/LiveStats.test.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Addresses review: - GameImpl.conquerPlayer stamps deathPosition (excluding the conquered from the alive count) so a game-ending tick still records the finishing place; PlayerExecution keeps the same stamp as a fallback for non-conquest deaths (setDeathPosition is first-write-wins). - markKilledBy uses an explicit stamped flag, so a recorded null killer (a bot/nation elimination) can't be overwritten by a later call.
| // OFM live standings (see PlayerImpl): eliminator's clientID (null while alive | ||
| // or a non-client killer) + finishing position at elimination. Stamped once. | ||
| killedBy(): ClientID | null; | ||
| deathPosition(): number | null; | ||
| markKilledBy(clientID: ClientID | null): void; | ||
| setDeathPosition(position: number): void; |
There was a problem hiding this comment.
can this go in playerstats instead?
There was a problem hiding this comment.
These need to be live, per-tick. PlayerStats/allPlayersStats only gets emitted on the game-end Win update (setWinner → GameUpdateType.Win) and read off winner.allPlayersStats, so it's not there until the game ends.
So instead we enrich LiveStatsController (same as the existing isAlive field that tracks dead players), which snapshots every ~10s off the live PlayerView.
Main reasons, is that the settle sometimes takes too long and delays our games, and we want to see live standings instead of waiting for game end.
There was a problem hiding this comment.
i mean stored in stats here:
export const PlayerStatsSchema = z
.object({
attacks: AtLeastOneNumberSchema.optional(),
betrayals: BigIntStringSchema.optional(),
killedAt: BigIntStringSchema.optional(),
// Tiles owned at game end, for OFM standings (set on setWinner).
finalTiles: BigIntStringSchema.optional(),
// Humans this player eliminated (victim clientID + tick), for OFM kill scoring.
kills: z
.array(z.object({ victim: z.string(), tick: BigIntStringSchema }))
.optional(),
conquests: AtLeastOneNumberSchema.optional(),
boats: z.partialRecord(BoatUnitSchema, AtLeastOneNumberSchema).optional(),
bombs: z.partialRecord(BombUnitSchema, AtLeastOneNumberSchema).optional(),
gold: AtLeastOneNumberSchema.optional(),
units: z.partialRecord(OtherUnitSchema, AtLeastOneNumberSchema).optional(),
})
.optional();
and then this execution does this.mg.stats.player(...).betrayals etc. That way we don't bloat the Player interface.
Move the OFM live-standings elimination fields off the Player interface and into PlayerStats (set via mg.stats().recordKilledBy / recordDeathPosition), per review feedback, so the Player interface isn't bloated with them. They still surface on the live PlayerUpdate: toFullUpdate reads them back from getPlayerStats() every tick, so the standings board stays live (not game-end only). getPlayerStats doesn't create-on-read, so there's no per-tick bloat. - StatsSchemas: add killedBy (nullable, null = non-client killer) + deathPosition - Stats/StatsImpl: recordKilledBy + recordDeathPosition, both first-write-wins - conquerPlayer + PlayerExecution stamp via mg.stats() instead of Player methods - Remove Player.killedBy/deathPosition/markKilledBy/setDeathPosition + fields - Add sim-level test for the stats->live-update path
|
Done. Moved both fields onto One thing I kept: these need to stay live on the |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/StatsSchemas.ts (1)
111-115: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd integer and positive validations to match downstream schema.
Make the schema for
deathPositionstrict to match downstream usage.PlayerLiveStatsSchemaenforces an integer and positive number constraint (z.number().int().positive().nullable()), so aligning this schema ensures data consistency and prevents invalid fractional or negative placements from being persisted.💡 Proposed refactor
// first-write-wins. Surfaced live on the PlayerUpdate (not just at game end). killedBy: z.string().nullable().optional(), - deathPosition: z.number().optional(), + deathPosition: z.number().int().positive().optional(), // Tiles owned at game end, for OFM standings (set on setWinner).🤖 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 `@src/core/StatsSchemas.ts` around lines 111 - 115, Update deathPosition in the relevant stats schema to enforce integer and positive-number validation, matching PlayerLiveStatsSchema; preserve its optional behavior and existing killedBy definition.
🤖 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.
Nitpick comments:
In `@src/core/StatsSchemas.ts`:
- Around line 111-115: Update deathPosition in the relevant stats schema to
enforce integer and positive-number validation, matching PlayerLiveStatsSchema;
preserve its optional behavior and existing killedBy definition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78413c87-aae4-4528-84f7-2e8ae428a52c
📒 Files selected for processing (7)
src/core/StatsSchemas.tssrc/core/execution/PlayerExecution.tssrc/core/game/GameImpl.tssrc/core/game/PlayerImpl.tssrc/core/game/Stats.tssrc/core/game/StatsImpl.tstests/LiveStandingsFields.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/game/GameImpl.ts
- src/core/execution/PlayerExecution.ts
github-project-automation
Bot
moved this from Development
to Final Review
in OpenFront Release Management
github-project-automation
Bot
moved this from Final Review
to Complete
in OpenFront Release Management
2 participants
Description:
Enriches the admin-bot live-stats snapshot (
/api/adminbot/game/:id/stats) with the per-player fields a semi-live standings board needs, so it can score kills + placement live instead of waiting for the post-game record.Per player in
liveStats.players[]:killedBy— the eliminator's clientID (null while alive / non-client killer). Invert → live kill points.deathPosition— finishing place at elimination = non-bot players still standing + 1 (frozen at "last of the then-standing"). Null while alive → live placement.The full roster is reported (dead players are retained in the client view), plus top-level
winner(the decided winner's clientID, else null).Deterministic / consensus-safe:
killedBy(stamped atrecordKill) anddeathPosition(stamped inPlayerExecution) are pure sim values, so in-sync clients agree on the majority vote;winneris added server-side inGameServer.liveStats()from the winner vote (like the existing publicID/username/connected enrichment).Please complete the following: