Conversation
Contributor
WalkthroughRefactors spawn-phase tracking to use an explicit startTick and SpawnPhaseEnd update; adds elapsedGameSeconds() and endSpawnPhase(); introduces SpawnTimerExecution and updates SpawnExecution to end spawn for singleplayer humans; client and many tests updated to use the new flow and timing APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GameRunner
participant SpawnTimerExec as SpawnTimerExecution
participant Game
Note over GameRunner, SpawnTimerExec: Server-side spawn control flow
Client->>GameRunner: connect / start game
GameRunner->>Game: create game instance
alt multiplayer
GameRunner->>SpawnTimerExec: register execution
SpawnTimerExec->>Game: ticks -> check numSpawnPhaseTurns
SpawnTimerExec->>Game: endSpawnPhase() (when threshold exceeded)
Game->>GameRunner: emit GameUpdate SpawnPhaseEnd(startTick)
GameRunner->>Client: forward SpawnPhaseEnd
else singleplayer
Game->>GameRunner: SpawnExecution assigns spawn tile for human
Game->>Game: endSpawnPhase() (immediately after human spawn)
Game->>GameRunner: emit SpawnPhaseEnd update
GameRunner->>Client: forward SpawnPhaseEnd
end
Client->>Client: use elapsedGameSeconds() for timers/UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/core/executions/MIRVExecution.test.ts (1)
19-31:⚠️ Potential issue | 🟠 MajorRemove redundant
endSpawnPhase()call on line 31 —setup()already ends spawn phase by default.
setup()with defaultautoEndSpawnPhase: truecallsgame.endSpawnPhase()before returning (seetests/util/Setup.tsline 83). The explicit call on line 31 is redundant.Additionally,
GameImpl.endSpawnPhase()lacks a guard check: it unconditionally emits aSpawnPhaseEndupdate without checkinginSpawnPhase()first. Calling it twice produces two duplicate updates, risking corruption of clientGameViewstate. SinceSpawnExecutionandSpawnTimerExecutionboth callendSpawnPhase()in production, this design could cause the same issue outside tests.Fix: Either remove line 31, or add an
inSpawnPhase()guard inGameImpl.endSpawnPhase()so it only emits the update when currently in spawn phase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/executions/MIRVExecution.test.ts` around lines 19 - 31, The test calls game.endSpawnPhase() redundantly because setup(..., autoEndSpawnPhase: true) already ends spawn; remove the explicit call in MIRVExecution.test.ts, and harden GameImpl.endSpawnPhase() to be idempotent by adding an in-spawn-phase guard: in GameImpl.endSpawnPhase() check inSpawnPhase() (or this.inSpawnPhase()) and return immediately if false so it only emits the SpawnPhaseEnd update once; this prevents duplicate updates when SpawnExecution or SpawnTimerExecution also call endSpawnPhase().src/core/execution/SpawnExecution.ts (1)
40-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
SpawnExecutionfenced to the spawn phase.
tick()now reassigns tiles even after spawn has ended. If a lateSpawnExecutionis ever queued for a manual-spawn player, this will wipe their territory and relocate them instead of being ignored. Please keep the new singleplayer branch, but bail once the player has already spawned and the game is no longer in spawn phase.Suggested guard
tick(ticks: number) { this.active = false; let player: Player | null = null; if (this.mg.hasPlayer(this.playerInfo.id)) { player = this.mg.player(this.playerInfo.id); } else { player = this.mg.addPlayer(this.playerInfo); } + + if (!this.mg.inSpawnPhase() && player.hasSpawned()) { + return; + } // Security: If random spawn is enabled, prevent players from re-rolling their spawn location if (this.mg.config().isRandomSpawn() && player.hasSpawned()) { return; }Run this to verify whether
SpawnExecutionis reachable outside initial setup. Expected result: only startup/spawn bootstrap paths should create it; any turn-processing or post-start path makes this a live respawn bug.#!/bin/bash set -euo pipefail echo "SpawnExecution call sites:" rg -n --type=ts '\bnew\s+SpawnExecution\s*\(' echo echo "Nearby executor / turn-processing references:" rg -n --type=ts -C3 '\bSpawnExecution\b|spawnPlayers\s*\(|createExecs\s*\(' src tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/SpawnExecution.ts` around lines 40 - 83, SpawnExecution.tick can run after the spawn phase and wipe a manual player's territory; add an early guard to bail out if the game is no longer in the spawn phase and the player has already spawned. In SpawnExecution.tick (after obtaining/creating the Player via this.mg.player/addPlayer and before reassigning tiles), check the manager's spawn-phase state (e.g. this.mg.isSpawnPhase() or equivalent API) and if it returns false and player.hasSpawned() is true, return immediately; keep the existing singleplayer branch that calls this.mg.endSpawnPhase().src/core/game/GameView.ts (1)
668-710:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHydrate spawn state from a snapshot, not only from the end event.
This now starts every
GameViewin spawn phase and only flips it when aSpawnPhaseEndupdate arrives. A late joiner or reconnect after that transition will never see the event, soinSpawnPhase(), immunity, andelapsedGameSeconds()can stay wrong for the rest of the session. The initial payload needs authoritative spawn state /startTick, withSpawnPhaseEndtreated as an incremental update only.Also applies to: 807-813
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/GameView.ts` around lines 668 - 710, The constructor currently forces inSpawnPhaseValue = true and startTick = _config.numSpawnPhaseTurns(), causing the view to rely solely on SpawnPhaseEnd updates; instead, initialize spawn state from the initial snapshot payload: read the authoritative spawn flag and startTick from the snapshot (or initial game state object passed into GameView) and set inSpawnPhaseValue and startTick accordingly in the constructor (or in the hydrate/init method), while continuing to treat SpawnPhaseEnd updates as incremental overrides; also ensure the same hydration logic is applied where inSpawnPhaseValue and startTick are referenced later (see methods around lines where inSpawnPhaseValue, startTick, elapsedGameSeconds(), and immunity logic are used, and the block noted at 807-813) so late joiners/reconnects get correct spawn state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/execution/SpawnTimerExecution.ts`:
- Around line 10-13: In SpawnTimerExecution.tick, the spawn phase is being ended
one tick late because executeNextTick runs before _ticks increments; change the
comparison in tick(ticks: number) to end the spawn phase when this.mg.ticks() is
greater than or equal to the configured limit (use >= with
this.mg.config().numSpawnPhaseTurns()) so endSpawnPhase() is called exactly on
the configured tick.
In `@src/core/game/GameImpl.ts`:
- Around line 472-480: This block can emit SpawnPhaseEnd twice because
endSpawnPhase() (called from SpawnTimerExecution.tick()) can set startTick to
_ticks just before this check; change the guard in GameImpl to prevent duplicate
emissions by tracking the last tick when a SpawnPhaseEnd was emitted: add a
private field like _lastSpawnPhaseEndTick, only call addUpdate({ type:
GameUpdateType.SpawnPhaseEnd, startTick: this.startTick }) when this._ticks ===
this.startTick && this._lastSpawnPhaseEndTick !== this.startTick, and update
_lastSpawnPhaseEndTick = this.startTick when emitting; reference symbols:
GameImpl, this._ticks, this.startTick, addUpdate, GameUpdateType.SpawnPhaseEnd,
SpawnTimerExecution.tick(), endSpawnPhase().
In `@tests/core/game/GameImpl.test.ts`:
- Around line 135-137: The test currently calls setup("plains", { gameType:
GameType.Singleplayer }) which auto-ends the spawn phase; change the call to
pass autoEndSpawnPhase: false so the test remains in the spawn phase (i.e., call
setup("plains", { gameType: GameType.Singleplayer, autoEndSpawnPhase: false }))
to ensure it covers the late SpawnExecution call to endSpawnPhase(); locate the
setup invocation in GameImpl.test and add the autoEndSpawnPhase flag.
In `@tests/Disconnected.test.ts`:
- Around line 293-298: The assertion currently uses player1.troops() >=
troopsBeforeAttack which can hide net troop loss offset by passive growth;
change the test to assert the exact no-loss behavior by either (a) capturing and
asserting on the returned startTroops value from the attack routine (e.g.,
verify startTroops equals the expected attacking troops) or (b) compute expected
passive growth and assert player1.troops() >= troopsBeforeAttack +
expectedPassiveGrowth; update the lines around executeNextTick(), retreat(), and
the post-attack assertion to use one of these explicit checks so the test name's
“no troop loss” behavior is directly verifiable.
---
Outside diff comments:
In `@src/core/execution/SpawnExecution.ts`:
- Around line 40-83: SpawnExecution.tick can run after the spawn phase and wipe
a manual player's territory; add an early guard to bail out if the game is no
longer in the spawn phase and the player has already spawned. In
SpawnExecution.tick (after obtaining/creating the Player via
this.mg.player/addPlayer and before reassigning tiles), check the manager's
spawn-phase state (e.g. this.mg.isSpawnPhase() or equivalent API) and if it
returns false and player.hasSpawned() is true, return immediately; keep the
existing singleplayer branch that calls this.mg.endSpawnPhase().
In `@src/core/game/GameView.ts`:
- Around line 668-710: The constructor currently forces inSpawnPhaseValue = true
and startTick = _config.numSpawnPhaseTurns(), causing the view to rely solely on
SpawnPhaseEnd updates; instead, initialize spawn state from the initial snapshot
payload: read the authoritative spawn flag and startTick from the snapshot (or
initial game state object passed into GameView) and set inSpawnPhaseValue and
startTick accordingly in the constructor (or in the hydrate/init method), while
continuing to treat SpawnPhaseEnd updates as incremental overrides; also ensure
the same hydration logic is applied where inSpawnPhaseValue and startTick are
referenced later (see methods around lines where inSpawnPhaseValue, startTick,
elapsedGameSeconds(), and immunity logic are used, and the block noted at
807-813) so late joiners/reconnects get correct spawn state.
In `@tests/core/executions/MIRVExecution.test.ts`:
- Around line 19-31: The test calls game.endSpawnPhase() redundantly because
setup(..., autoEndSpawnPhase: true) already ends spawn; remove the explicit call
in MIRVExecution.test.ts, and harden GameImpl.endSpawnPhase() to be idempotent
by adding an in-spawn-phase guard: in GameImpl.endSpawnPhase() check
inSpawnPhase() (or this.inSpawnPhase()) and return immediately if false so it
only emits the SpawnPhaseEnd update once; this prevents duplicate updates when
SpawnExecution or SpawnTimerExecution also call endSpawnPhase().
🪄 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: 4ca53d36-ca20-4a6b-9bf0-80ae73395014
📒 Files selected for processing (46)
src/client/ClientGameRunner.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/SpawnTimer.tssrc/core/GameRunner.tssrc/core/execution/SpawnExecution.tssrc/core/execution/SpawnTimerExecution.tssrc/core/execution/WinCheckExecution.tssrc/core/game/Game.tssrc/core/game/GameImpl.tssrc/core/game/GameUpdates.tssrc/core/game/GameView.tstests/AiAttackBehavior.test.tstests/AllianceAcceptNukes.test.tstests/AllianceDonation.test.tstests/AllianceExtensionExecution.test.tstests/AllianceRequestExecution.test.tstests/Attack.test.tstests/AttackStats.test.tstests/DeleteUnitExecution.test.tstests/Disconnected.test.tstests/Donate.test.tstests/GameInfoRanking.test.tstests/MissileSilo.test.tstests/NationAllianceBehavior.test.tstests/NationCounterWarshipInfestation.test.tstests/NationMIRV.test.tstests/NationNukeSamOverwhelm.test.tstests/PlayerImpl.test.tstests/PortExecution.test.tstests/ShellRandom.test.tstests/Stats.test.tstests/Warship.test.tstests/WarshipMultiSelection.test.tstests/core/execution/SpawnExecution.test.tstests/core/executions/MIRVExecution.test.tstests/core/executions/NoInverseAnnexation.test.tstests/core/executions/NukeExecution.test.tstests/core/executions/PlayerExecution.test.tstests/core/executions/SAMLauncherExecution.test.tstests/core/executions/WinCheckExecution.test.tstests/core/game/GameImpl.test.tstests/core/pathfinding/SpatialQuery.test.tstests/economy/ConstructionGold.test.tstests/nukes/HydrogenAndMirv.test.tstests/nukes/WaterNukes.test.tstests/util/Setup.ts
💤 Files with no reviewable changes (23)
- tests/core/executions/NoInverseAnnexation.test.ts
- tests/ShellRandom.test.ts
- tests/AllianceRequestExecution.test.ts
- tests/core/executions/NukeExecution.test.ts
- tests/WarshipMultiSelection.test.ts
- tests/NationNukeSamOverwhelm.test.ts
- tests/DeleteUnitExecution.test.ts
- tests/GameInfoRanking.test.ts
- tests/AiAttackBehavior.test.ts
- tests/NationCounterWarshipInfestation.test.ts
- tests/core/executions/PlayerExecution.test.ts
- tests/PlayerImpl.test.ts
- tests/AllianceExtensionExecution.test.ts
- tests/AllianceAcceptNukes.test.ts
- tests/PortExecution.test.ts
- tests/MissileSilo.test.ts
- tests/Stats.test.ts
- tests/Donate.test.ts
- tests/nukes/WaterNukes.test.ts
- tests/core/executions/SAMLauncherExecution.test.ts
- tests/core/executions/WinCheckExecution.test.ts
- tests/AllianceDonation.test.ts
- tests/NationMIRV.test.ts
Comment on lines
+10
to
+13
| tick(ticks: number): void { | ||
| if (this.mg.ticks() > this.mg.config().numSpawnPhaseTurns()) { | ||
| this.mg.endSpawnPhase(); | ||
| } |
Contributor
There was a problem hiding this comment.
End spawn phase on the configured tick, not one tick later.
executeNextTick() runs executions before _ticks is incremented, so > delays the transition to numSpawnPhaseTurns() + 1. That shifts the countdown, startTick, and spawn-immunity window by one tick.
Suggested change
tick(ticks: number): void {
- if (this.mg.ticks() > this.mg.config().numSpawnPhaseTurns()) {
+ if (this.mg.ticks() >= this.mg.config().numSpawnPhaseTurns()) {
this.mg.endSpawnPhase();
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| tick(ticks: number): void { | |
| if (this.mg.ticks() > this.mg.config().numSpawnPhaseTurns()) { | |
| this.mg.endSpawnPhase(); | |
| } | |
| tick(ticks: number): void { | |
| if (this.mg.ticks() >= this.mg.config().numSpawnPhaseTurns()) { | |
| this.mg.endSpawnPhase(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/execution/SpawnTimerExecution.ts` around lines 10 - 13, In
SpawnTimerExecution.tick, the spawn phase is being ended one tick late because
executeNextTick runs before _ticks increments; change the comparison in
tick(ticks: number) to end the spawn phase when this.mg.ticks() is greater than
or equal to the configured limit (use >= with
this.mg.config().numSpawnPhaseTurns()) so endSpawnPhase() is called exactly on
the configured tick.
Comment on lines
+472
to
+480
| if ( | ||
| this.config().gameConfig().gameType !== GameType.Singleplayer && | ||
| this._ticks === this.startTick | ||
| ) { | ||
| this.addUpdate({ | ||
| type: GameUpdateType.SpawnPhaseEnd, | ||
| startTick: this.startTick, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid emitting SpawnPhaseEnd twice in multiplayer.
On the tick where SpawnTimerExecution.tick() calls endSpawnPhase(), startTick has just been set to _ticks, so this block appends a second SpawnPhaseEnd update for the same transition. Clients and tests that expect one end event per transition will see duplicates here.
Suggested change
- if (
- this.config().gameConfig().gameType !== GameType.Singleplayer &&
- this._ticks === this.startTick
- ) {
- this.addUpdate({
- type: GameUpdateType.SpawnPhaseEnd,
- startTick: this.startTick,
- });
- }
-
this._ticks++;
return this.updates;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ( | |
| this.config().gameConfig().gameType !== GameType.Singleplayer && | |
| this._ticks === this.startTick | |
| ) { | |
| this.addUpdate({ | |
| type: GameUpdateType.SpawnPhaseEnd, | |
| startTick: this.startTick, | |
| }); | |
| } | |
| this._ticks++; | |
| return this.updates; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/game/GameImpl.ts` around lines 472 - 480, This block can emit
SpawnPhaseEnd twice because endSpawnPhase() (called from
SpawnTimerExecution.tick()) can set startTick to _ticks just before this check;
change the guard in GameImpl to prevent duplicate emissions by tracking the last
tick when a SpawnPhaseEnd was emitted: add a private field like
_lastSpawnPhaseEndTick, only call addUpdate({ type:
GameUpdateType.SpawnPhaseEnd, startTick: this.startTick }) when this._ticks ===
this.startTick && this._lastSpawnPhaseEndTick !== this.startTick, and update
_lastSpawnPhaseEndTick = this.startTick when emitting; reference symbols:
GameImpl, this._ticks, this.startTick, addUpdate, GameUpdateType.SpawnPhaseEnd,
SpawnTimerExecution.tick(), endSpawnPhase().
Comment on lines
+135
to
+137
| const singleplayerGame = await setup("plains", { | ||
| gameType: GameType.Singleplayer, | ||
| }); |
Contributor
There was a problem hiding this comment.
Keep this test in spawn phase until the late spawn.
setup() now ends spawn phase by default, so this test starts with startTick = 0 and only proves that a later SpawnExecution can call endSpawnPhase() again. Pass false for autoEndSpawnPhase here so the test actually covers the intended singleplayer transition.
Suggested change
- const singleplayerGame = await setup("plains", {
- gameType: GameType.Singleplayer,
- });
+ const singleplayerGame = await setup(
+ "plains",
+ {
+ gameType: GameType.Singleplayer,
+ },
+ [],
+ undefined,
+ undefined,
+ false,
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const singleplayerGame = await setup("plains", { | |
| gameType: GameType.Singleplayer, | |
| }); | |
| const singleplayerGame = await setup( | |
| "plains", | |
| { | |
| gameType: GameType.Singleplayer, | |
| }, | |
| [], | |
| undefined, | |
| undefined, | |
| false, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/game/GameImpl.test.ts` around lines 135 - 137, The test currently
calls setup("plains", { gameType: GameType.Singleplayer }) which auto-ends the
spawn phase; change the call to pass autoEndSpawnPhase: false so the test
remains in the spawn phase (i.e., call setup("plains", { gameType:
GameType.Singleplayer, autoEndSpawnPhase: false })) to ensure it covers the late
SpawnExecution call to endSpawnPhase(); locate the setup invocation in
GameImpl.test and add the autoEndSpawnPhase flag.
Comment on lines
+293
to
+298
| // retreat() fires in the tick after player2's last tile is conquered | ||
| // (toConquer empties, refreshToConquer() finds nothing, then retreat). | ||
| game.executeNextTick(); | ||
|
|
||
| const expectedFinalTroops = Number( | ||
| toInt(troopsBeforeAttack) + expectedTotalGrowth, | ||
| ); | ||
|
|
||
| // Verify no troop loss | ||
| expect(player1.troops()).toBe(expectedFinalTroops); | ||
| // startTroops returned with no malus → no net troop loss, only passive growth | ||
| expect(player1.troops()).toBeGreaterThanOrEqual(troopsBeforeAttack); |
Contributor
There was a problem hiding this comment.
Keep the assertion tied to “no troop loss.”
toBeGreaterThanOrEqual(troopsBeforeAttack) can still pass if the attack loses troops but passive growth during the conquest loop covers that loss. Please compare against the expected passive growth separately, or assert on the returned attack troops, so the behavior in the test name stays observable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Disconnected.test.ts` around lines 293 - 298, The assertion currently
uses player1.troops() >= troopsBeforeAttack which can hide net troop loss offset
by passive growth; change the test to assert the exact no-loss behavior by
either (a) capturing and asserting on the returned startTroops value from the
attack routine (e.g., verify startTroops equals the expected attacking troops)
or (b) compute expected passive growth and assert player1.troops() >=
troopsBeforeAttack + expectedPassiveGrowth; update the lines around
executeNextTick(), retreat(), and the post-attack assertion to use one of these
explicit checks so the test name's “no troop loss” behavior is directly
verifiable.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
tests/Disconnected.test.ts (1)
293-298:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe
toBeGreaterThanOrEqualassertion does not isolate the no-malus-retreat property.Passive growth earned during the
while (player2.isAlive())conquest loop can silently compensate for any troop loss caused by the retreat, so the assertion can pass even when troops were incorrectly penalised. Counting ticks in the loop and subtracting expected growth makes the guard meaningful.🐛 Proposed fix
+ let conquestTicks = 0; while (player2.isAlive()) { game.executeNextTick(); + conquestTicks++; } // retreat() fires in the tick after player2's last tile is conquered game.executeNextTick(); + conquestTicks++; - // startTroops returned with no malus → no net troop loss, only passive growth - expect(player1.troops()).toBeGreaterThanOrEqual(troopsBeforeAttack); + // startTroops returned with no malus: net change is passive growth only + const troopIncPerTick = game.config().troopIncreaseRate(player1); + const expectedPassiveGrowth = toInt(troopIncPerTick * conquestTicks); + expect(player1.troops()).toBe(toInt(troopsBeforeAttack) + expectedPassiveGrowth);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Disconnected.test.ts` around lines 293 - 298, The current assertion using expect(player1.troops()).toBeGreaterThanOrEqual(troopsBeforeAttack) can be satisfied by passive growth and doesn't verify that retreat() applied no malus; update the test to track the number of ticks advanced inside the conquest loop (where while (player2.isAlive()) runs and game.executeNextTick() is called), compute the expected passive growth = ticksDuringConquest * passiveGrowthPerTick (use the same growth logic used by the game), then after the final game.executeNextTick() that triggers refreshToConquer()/retreat(), assert that player1.troops() === troopsBeforeAttack + expectedPassiveGrowth (or use toBeCloseTo/strict equality depending on integer/float handling) so the assertion ensures no retreat penalty was applied; use identifiers player1, player2, game.executeNextTick(), refreshToConquer(), and retreat() to locate and change the test.src/core/game/GameImpl.ts (1)
472-480:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis emits
SpawnPhaseEndtwice on the transition tick.
endSpawnPhase()already pushes theSpawnPhaseEndupdate at Lines 417-422. IfSpawnTimerExecution.tick()flipsstartTickearlier in the same tick, this block appends a second identical event, so clients/tests see two spawn-end transitions for one state change.tests/core/game/GameImpl.test.ts (1)
135-163:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis test still starts after spawn phase has already ended.
setup("plains", { gameType: GameType.Singleplayer })auto-ends spawn here, so the lateSpawnExecutiononly proves thatendSpawnPhase()can run again and reset immunity timing. Please keep the test in spawn phase at setup time, otherwise it misses the singleplayer transition this PR is trying to cover.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/game/GameImpl.test.ts` around lines 135 - 163, The test advances the game past spawn phase before adding the late SpawnExecution, so it never exercises the singleplayer spawn-phase transition; remove the loop that advances ticks (the for loop using pastSpawnCountdown and calls to singleplayerGame.executeNextTick()) so the game remains in spawn phase after setup("plains", { gameType: GameType.Singleplayer }), then add the SpawnExecution and run the two executeNextTick() calls as written to initialize and apply the spawn and assert isSpawnImmunityActive(); keep the other calls (setSpawnImmunityDuration, new PlayerInfo, SpawnExecution, and the expect checks) intact.
🧹 Nitpick comments (1)
tests/nukes/HydrogenAndMirv.test.ts (1)
23-24: ⚡ Quick winAdd a guard assertion to lock in the spawn-phase exit assumption.
The two-tick sequence is a hardcoded assumption that
SpawnTimerExecution/endSpawnPhasefires within exactly 2 ticks. If that timing ever changes,playerwill be retrieved and used in a state where the spawn phase has not ended — tests may still pass (becauseplayer.conquer()is called manually afterward) but will silently test the wrong game state, hiding regressions.Adding
expect(game.inSpawnPhase()).toBe(false)after tick 2 makes the contract explicit and turns timing drift into a loud, immediate test failure.✅ Proposed fix
game.executeNextTick(); // init spawns game.executeNextTick(); // tick spawns → players get territory + expect(game.inSpawnPhase()).toBe(false); player = game.player(info.id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/nukes/HydrogenAndMirv.test.ts` around lines 23 - 24, Add an explicit assertion that the spawn phase has ended after the two executeNextTick() calls: after calling game.executeNextTick() twice (the lines containing game.executeNextTick() // init spawns and game.executeNextTick() // tick spawns → players get territory), insert expect(game.inSpawnPhase()).toBe(false) to lock the assumption that SpawnTimerExecution/endSpawnPhase ran; this uses the game.inSpawnPhase() API to fail fast if spawn timing changes and prevents using player/conquer() against an incorrect game state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/game/GameImpl.ts`:
- Around line 848-865: elapsedGameSeconds() introduces floating-point seconds
into src/core; change core to expose integer time (deciseconds) instead: replace
or rename elapsedGameSeconds() with elapsedGameDeciseconds() that returns
this.ticksSinceStart() (an integer), keep ticksSinceStart() and
isNationSpawnImmunityActive() as-is, and update all core-side callers to use
elapsedGameDeciseconds() (convert to seconds only in UI/adapter code by dividing
by 10 there).
In `@tests/Attack.test.ts`:
- Around line 72-73: The test advances only two ticks but SpawnTimerExecution
now requires numSpawnPhaseTurns() ticks to finish, so the match remains in spawn
phase and later assertions run under spawn immunity; modify the tests that call
game.executeNextTick() (e.g., at Attack.test.ts lines around the shown diff and
also at 185-186, 356-357, 404-405) to either loop/advance until
game.inSpawnPhase() returns false or explicitly call the routine that ends spawn
(or simulate the spawn timer finishing) after the initial territory-init ticks;
ensure you reference game.executeNextTick(), SpawnTimerExecution,
numSpawnPhaseTurns(), and game.inSpawnPhase() when locating and updating the
test setup so post-spawn assertions run in the correct state.
---
Duplicate comments:
In `@tests/core/game/GameImpl.test.ts`:
- Around line 135-163: The test advances the game past spawn phase before adding
the late SpawnExecution, so it never exercises the singleplayer spawn-phase
transition; remove the loop that advances ticks (the for loop using
pastSpawnCountdown and calls to singleplayerGame.executeNextTick()) so the game
remains in spawn phase after setup("plains", { gameType: GameType.Singleplayer
}), then add the SpawnExecution and run the two executeNextTick() calls as
written to initialize and apply the spawn and assert isSpawnImmunityActive();
keep the other calls (setSpawnImmunityDuration, new PlayerInfo, SpawnExecution,
and the expect checks) intact.
In `@tests/Disconnected.test.ts`:
- Around line 293-298: The current assertion using
expect(player1.troops()).toBeGreaterThanOrEqual(troopsBeforeAttack) can be
satisfied by passive growth and doesn't verify that retreat() applied no malus;
update the test to track the number of ticks advanced inside the conquest loop
(where while (player2.isAlive()) runs and game.executeNextTick() is called),
compute the expected passive growth = ticksDuringConquest * passiveGrowthPerTick
(use the same growth logic used by the game), then after the final
game.executeNextTick() that triggers refreshToConquer()/retreat(), assert that
player1.troops() === troopsBeforeAttack + expectedPassiveGrowth (or use
toBeCloseTo/strict equality depending on integer/float handling) so the
assertion ensures no retreat penalty was applied; use identifiers player1,
player2, game.executeNextTick(), refreshToConquer(), and retreat() to locate and
change the test.
---
Nitpick comments:
In `@tests/nukes/HydrogenAndMirv.test.ts`:
- Around line 23-24: Add an explicit assertion that the spawn phase has ended
after the two executeNextTick() calls: after calling game.executeNextTick()
twice (the lines containing game.executeNextTick() // init spawns and
game.executeNextTick() // tick spawns → players get territory), insert
expect(game.inSpawnPhase()).toBe(false) to lock the assumption that
SpawnTimerExecution/endSpawnPhase ran; this uses the game.inSpawnPhase() API to
fail fast if spawn timing changes and prevents using player/conquer() against an
incorrect game state.
🪄 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: 3aac8489-ea39-47c8-967e-6a9a4a34aba1
📒 Files selected for processing (46)
src/client/ClientGameRunner.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/SpawnTimer.tssrc/core/GameRunner.tssrc/core/execution/SpawnExecution.tssrc/core/execution/SpawnTimerExecution.tssrc/core/execution/WinCheckExecution.tssrc/core/game/Game.tssrc/core/game/GameImpl.tssrc/core/game/GameUpdates.tssrc/core/game/GameView.tstests/AiAttackBehavior.test.tstests/AllianceAcceptNukes.test.tstests/AllianceDonation.test.tstests/AllianceExtensionExecution.test.tstests/AllianceRequestExecution.test.tstests/Attack.test.tstests/AttackStats.test.tstests/DeleteUnitExecution.test.tstests/Disconnected.test.tstests/Donate.test.tstests/GameInfoRanking.test.tstests/MissileSilo.test.tstests/NationAllianceBehavior.test.tstests/NationCounterWarshipInfestation.test.tstests/NationMIRV.test.tstests/NationNukeSamOverwhelm.test.tstests/PlayerImpl.test.tstests/PortExecution.test.tstests/ShellRandom.test.tstests/Stats.test.tstests/Warship.test.tstests/WarshipMultiSelection.test.tstests/core/execution/SpawnExecution.test.tstests/core/executions/MIRVExecution.test.tstests/core/executions/NoInverseAnnexation.test.tstests/core/executions/NukeExecution.test.tstests/core/executions/PlayerExecution.test.tstests/core/executions/SAMLauncherExecution.test.tstests/core/executions/WinCheckExecution.test.tstests/core/game/GameImpl.test.tstests/core/pathfinding/SpatialQuery.test.tstests/economy/ConstructionGold.test.tstests/nukes/HydrogenAndMirv.test.tstests/nukes/WaterNukes.test.tstests/util/Setup.ts
💤 Files with no reviewable changes (23)
- tests/AllianceExtensionExecution.test.ts
- tests/AllianceAcceptNukes.test.ts
- tests/MissileSilo.test.ts
- tests/Stats.test.ts
- tests/AllianceDonation.test.ts
- tests/DeleteUnitExecution.test.ts
- tests/core/executions/NoInverseAnnexation.test.ts
- tests/core/executions/PlayerExecution.test.ts
- tests/NationCounterWarshipInfestation.test.ts
- tests/AiAttackBehavior.test.ts
- tests/PlayerImpl.test.ts
- tests/core/executions/NukeExecution.test.ts
- tests/NationNukeSamOverwhelm.test.ts
- tests/WarshipMultiSelection.test.ts
- tests/core/executions/SAMLauncherExecution.test.ts
- tests/GameInfoRanking.test.ts
- tests/ShellRandom.test.ts
- tests/PortExecution.test.ts
- tests/NationMIRV.test.ts
- tests/Donate.test.ts
- tests/nukes/WaterNukes.test.ts
- tests/AllianceRequestExecution.test.ts
- tests/core/executions/WinCheckExecution.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/core/game/GameUpdates.ts
- src/core/execution/SpawnTimerExecution.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- tests/core/executions/MIRVExecution.test.ts
- tests/economy/ConstructionGold.test.ts
- tests/core/pathfinding/SpatialQuery.test.ts
- tests/util/Setup.ts
- src/client/graphics/layers/SpawnTimer.ts
- src/core/execution/WinCheckExecution.ts
- tests/core/execution/SpawnExecution.test.ts
- tests/NationAllianceBehavior.test.ts
- src/core/GameRunner.ts
- tests/AttackStats.test.ts
- src/core/game/Game.ts
Comment on lines
+848
to
+865
| public elapsedGameSeconds(): number { | ||
| return this.ticksSinceStart() / 10; | ||
| } | ||
|
|
||
| public isNationSpawnImmunityActive(): boolean { | ||
| return ( | ||
| this.config().numSpawnPhaseTurns() + | ||
| this.config().nationSpawnImmunityDuration() > | ||
| this.ticks() | ||
| this.inSpawnPhase() || | ||
| this.ticksSinceStart() < this.config().nationSpawnImmunityDuration() | ||
| ); | ||
| } | ||
|
|
||
| private ticksSinceStart(): number { | ||
| if (this.inSpawnPhase()) { | ||
| return 0; | ||
| } | ||
|
|
||
| return Math.max(0, this.ticks() - this.startTick!); | ||
| } |
Contributor
There was a problem hiding this comment.
Keep time as integers inside src/core.
elapsedGameSeconds() now depends on ticksSinceStart() / 10, which adds floating-point math to the core API. That is a poor fit for deterministic simulation code, especially now that core-side consumers use this helper too. Prefer keeping ticks or deciseconds as integers in src/core, and only convert to display seconds in UI code.
As per coding guidelines src/core/**/*.ts: Ensure deterministic behavior in src/core/ by using seeded PRNG and avoiding floating-point math.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/game/GameImpl.ts` around lines 848 - 865, elapsedGameSeconds()
introduces floating-point seconds into src/core; change core to expose integer
time (deciseconds) instead: replace or rename elapsedGameSeconds() with
elapsedGameDeciseconds() that returns this.ticksSinceStart() (an integer), keep
ticksSinceStart() and isNationSpawnImmunityActive() as-is, and update all
core-side callers to use elapsedGameDeciseconds() (convert to seconds only in
UI/adapter code by dividing by 10 there).
Comment on lines
+72
to
+73
| game.executeNextTick(); // init spawns | ||
| game.executeNextTick(); // tick spawns → players get territory |
Contributor
There was a problem hiding this comment.
Two ticks do not end multiplayer spawn phase.
SpawnTimerExecution now ends spawn only after numSpawnPhaseTurns(), so these setups still leave the match in spawn phase. The attack and transport assertions below then run under spawn immunity instead of the post-spawn state they are meant to cover. Please either advance until !game.inSpawnPhase() or explicitly end spawn after the territory-init ticks for these tests.
Also applies to: 185-186, 356-357, 404-405
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Attack.test.ts` around lines 72 - 73, The test advances only two ticks
but SpawnTimerExecution now requires numSpawnPhaseTurns() ticks to finish, so
the match remains in spawn phase and later assertions run under spawn immunity;
modify the tests that call game.executeNextTick() (e.g., at Attack.test.ts lines
around the shown diff and also at 185-186, 356-357, 404-405) to either
loop/advance until game.inSpawnPhase() returns false or explicitly call the
routine that ends spawn (or simulate the spawn timer finishing) after the
initial territory-init ticks; ensure you reference game.executeNextTick(),
SpawnTimerExecution, numSpawnPhaseTurns(), and game.inSpawnPhase() when locating
and updating the test setup so post-spawn assertions run in the correct state.
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.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME