Destroy incoming nukes when alliance is created #2716
Conversation
WalkthroughAdds public flight-state APIs to NukeExecution, game-level nukes counting/destruction between two players, alliance-acceptance integration that destroys queued and in‑flight nukes, merges pending tick updates, and adds unit and integration tests covering these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant PlayerA as Player A
participant Game as Game
participant Execs as Executions
participant Alliance as AllianceSystem
participant UI as UI
PlayerA->>Game: acceptAllianceRequest(playerB)
activate Game
Game->>Execs: executions() — iterate executions
note right of Execs `#DDEFFD`: evaluate execution.target() and isInFlight()
alt execution targets playerB
Execs->>Game: isInFlight() true
rect rgb(235,247,235)
note right of Game `#E8F8E8`: call destroyInFlight()
Game->>Execs: destroyInFlight()
Execs->>Execs: deactivate & remove nuke unit
end
else queued for playerB
Execs->>Game: queued
note right of Game `#FFF7DD`: remove queued execution (do not spawn)
else targets other
note right of Game `#F5F5F5`: preserve execution
end
Game-->>Game: return DestroyNukesResult {inFlight, queued, fromRequestorToRecipient, fromRecipientToRequestor}
deactivate Game
Game->>Alliance: continue acceptance (include counts)
Game->>UI: emit display events for canceled/neutralized nukes
UI->>PlayerA: show messages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (5 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 (4)
tests/core/executions/NukeExecution.test.ts (1)
129-149: Good test coverage for destroyInFlight, consider one extra assertion.The test correctly verifies that
destroyInFlight()deactivates the execution. You might also assert thatexec.getNuke()returnsnullafter destruction to confirm the unit reference is cleared.🔎 Optional: verify nuke reference is cleared
exec.destroyInFlight(); expect(exec.isActive()).toBe(false); + expect(exec.getNuke()).toBeNull(); });tests/AllianceAcceptNukes.test.ts (1)
170-184: Remove or document the commented-out assertion.Line 173 has a commented-out assertion. If it's no longer needed, remove it. If it represents an alternative check, add a comment explaining why it's disabled.
🔎 Clean up
const messages = updates[GameUpdateType.DisplayEvent]?.map((e) => e.message) ?? []; - // expect(messages.some((m) => m.includes("planned nuke"))).toBe(true); - // Expect both the queued (planned) message and a directional message expect(src/core/game/GameImpl.ts (2)
286-353: Redundantinstanceofcheck and complex type narrowing.At line 301,
exec instanceof NukeExecutionis checked again, but line 293 already guards this at the start ofdestroy(). Also, the type narrowing at lines 308-314 is a bit hard to follow.🔎 Simplify the target resolution
const destroy = (exec: Execution, isQueued: boolean) => { if (!(exec instanceof NukeExecution)) return; const launcher = exec.owner(); - // queued execution -> target not resolvable yet - const target = - exec.isInFlight() && exec.target() - ? exec.target() - : exec instanceof NukeExecution - ? exec.targetTile() - : null; - - if (!target) return; - - let targetOwner: Player | TerraNullius; - if (typeof target === "object" && "isPlayer" in target) { - // target is already a Player or TerraNullius (in-flight nuke) - targetOwner = target as Player | TerraNullius; - } else { - // target is a TileRef (queued nuke) - targetOwner = this.owner(target as TileRef); - } + // Resolve target owner: in-flight nukes can use target(), queued use tile ownership + const resolvedTarget = exec.target(); // null if queued + const targetOwner: Player | TerraNullius = resolvedTarget + ? resolvedTarget + : this.owner(exec.targetTile());
389-491: Message construction is repetitive — consider a helper.The nuke cancellation messages follow similar patterns but with slight variations. While this works, extracting a small helper function would reduce duplication and make future message changes easier.
This is optional since the current code is clear enough, but worth considering if more message types are added.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/core/execution/NukeExecution.tssrc/core/game/Game.tssrc/core/game/GameImpl.tstests/AllianceAcceptNukes.test.tstests/core/executions/NukeExecution.test.ts
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
tests/AllianceAcceptNukes.test.tssrc/core/game/Game.tssrc/core/execution/NukeExecution.tstests/core/executions/NukeExecution.test.tssrc/core/game/GameImpl.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
tests/AllianceAcceptNukes.test.tssrc/core/execution/NukeExecution.tstests/core/executions/NukeExecution.test.tssrc/core/game/GameImpl.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
tests/AllianceAcceptNukes.test.tssrc/core/execution/NukeExecution.tstests/core/executions/NukeExecution.test.tssrc/core/game/GameImpl.ts
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.
Applied to files:
src/core/execution/NukeExecution.tstests/core/executions/NukeExecution.test.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/core/execution/NukeExecution.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/execution/NukeExecution.tssrc/core/game/GameImpl.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/core/execution/NukeExecution.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/core/execution/NukeExecution.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/core/execution/NukeExecution.ts
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.
Applied to files:
src/core/game/GameImpl.ts
🧬 Code graph analysis (4)
src/core/game/Game.ts (1)
src/core/Schemas.ts (1)
Player(119-119)
src/core/execution/NukeExecution.ts (4)
src/core/game/Game.ts (2)
Player(530-667)TerraNullius(517-522)src/core/Schemas.ts (1)
Player(119-119)src/core/game/GameMap.ts (2)
TileRef(3-3)magnitude(167-169)src/core/configuration/DefaultConfig.ts (1)
maxTroops(800-831)
tests/core/executions/NukeExecution.test.ts (1)
src/core/execution/NukeExecution.ts (1)
NukeExecution(19-360)
src/core/game/GameImpl.ts (5)
src/core/game/Game.ts (3)
Player(530-667)Execution(356-361)TerraNullius(517-522)src/core/Schemas.ts (1)
Player(119-119)src/core/execution/NukeExecution.ts (2)
NukeExecution(19-360)target(43-48)src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/AllianceImpl.ts (2)
requestor(26-28)recipient(30-32)
🔇 Additional comments (5)
src/core/execution/NukeExecution.ts (4)
43-48: Good null-safety for queued executions.Returning
nullwhenthis.mgis not set correctly handles queued nukes that haven't been initialized yet. This enables callers to distinguish between "target unknown" and "targeting TerraNullius".
68-85: Clean lifecycle API for in-flight detection and destruction.The implementation is straightforward:
targetTile()exposes the destination without needing initializationisInFlight()correctly checks if the nuke unit existsdestroyInFlight()properly guards inactive state, deletes the unit, and deactivatesOne small note:
destroyInFlight()works on queued nukes too (wherethis.nukeis null), just marking them inactive. This is intentional based on howdestroyNukesBetweenuses it.
256-262: Verify maxTroops fallback of 1 is intentional.When
targetis null or TerraNullius,maxTroopsdefaults to 1. This affectsnukeDeathFactorcalculations in the loop below. Confirm this fallback produces the expected troop death calculations for nukes landing on unowned territory.
329-333: Good null guard for stats recording.The check prevents
bombLandfrom being called with a null target, which could happen if the destination tile ownership changed between launch and detonation.src/core/game/GameImpl.ts (1)
529-576: The update merging logic is correct. The loop at lines 570-572 executes properly becausecreateGameUpdatesMap()initializes allGameUpdateTypekeys to empty arrays before iteration. Thependingupdates are captured once before resetting, then merged with newly generated updates—no accumulation risk. The clearing at line 574 is necessary preparation for the next tick, not redundant; it resetsthis.updatesso subsequent ticks start fresh.
plazmaezio
force-pushed
the
feat/destroy-incoming-nukes-on-alliance
branch
from
4498601 to
0e8053c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/core/game/Game.ts (1)
770-779: Interface contract still doesn't match implementation—previous fix incomplete.The past review identified that this interface declares a 2-field return type but the implementation returns
DestroyNukesResultwith 4 fields (includingfromRequestorToRecipientandfromRecipientToRequestor). The implementation at GameImpl.ts lines 391-396 destructures all 4 fields to generate direction-specific messages. Without exposing these fields in the interface, the contract is broken.🔎 Fix the interface signature
Either import and use the full type, or inline all 4 fields:
+import type { DestroyNukesResult } from "./GameImpl"; + export interface Game extends GameMap { // ... other methods ... - destroyNukesBetween( - p1: Player, - p2: Player, - ): { - inFlight: number; - queued: number; - }; + destroyNukesBetween(p1: Player, p2: Player): DestroyNukesResult; executions(): Execution[]; }Or inline the full shape without importing:
destroyNukesBetween( p1: Player, p2: Player, ): { inFlight: number; queued: number; + fromRequestorToRecipient: number; + fromRecipientToRequestor: number; };tests/AllianceAcceptNukes.test.ts (1)
138-157: Test still targets terra nullius instead of player2—previous issue unfixed.The past review identified that the nuke targets
game.ref(20, 20)which is not conquered by any player (terra nullius). The setup only conquers tiles at (0,0), (0,1), and (10,10). Since the target is not owned by player2,destroyNukesBetweenwon't cancel this nuke—it only destroys nukes between the two allied players.For this test to validate the race condition properly, the nuke must target a tile owned by player2.
🔎 Fix the test
test("queued nukes never spawn after alliance acceptance (race condition)", () => { + // Ensure target is owned by player2 + player2.conquer(game.ref(20, 20)); + const exec = new NukeExecution( UnitType.AtomBomb, player1, game.ref(20, 20), null, );
🧹 Nitpick comments (2)
tests/AllianceAcceptNukes.test.ts (1)
159-185: Message validation test is reasonable but has commented code.The test validates that appropriate messages are displayed when nukes are destroyed. The commented line 173 appears to be leftover from debugging—consider removing it.
🔎 Remove commented debugging line
const messages = updates[GameUpdateType.DisplayEvent]?.map((e) => e.message) ?? []; - // expect(messages.some((m) => m.includes("planned nuke"))).toBe(true); - // Expect both the queued (planned) message and a directional message expect(src/core/game/GameImpl.ts (1)
294-334: Simplify target resolution—redundant type check.Lines 303-304 check
exec instanceof NukeExecutioninside thedestroyfunction, but line 295 already guards the entire function with the same check. The ternary at lines 300-305 is also complex and hard to read.🔎 Simplify the target resolution
const destroy = (exec: Execution, isQueued: boolean) => { if (!(exec instanceof NukeExecution)) return; const launcher = exec.owner(); - // queued execution -> target not resolvable yet - const target = - exec.isInFlight() && exec.target() - ? exec.target() - : exec instanceof NukeExecution - ? exec.targetTile() - : null; + // Get target: for in-flight nukes use target(), for queued use targetTile() + const target = exec.isInFlight() && exec.target() + ? exec.target() + : exec.targetTile(); if (!target) return; let targetOwner: Player | TerraNullius; if (typeof target === "object" && "isPlayer" in target) { // target is already a Player or TerraNullius (in-flight nuke) targetOwner = target as Player | TerraNullius; } else { // target is a TileRef (queued nuke) targetOwner = this.owner(target as TileRef); }Since we're already inside the
instanceof NukeExecutioncheck, the second check at line 303 is unnecessary.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/core/execution/NukeExecution.tssrc/core/game/Game.tssrc/core/game/GameImpl.tstests/AllianceAcceptNukes.test.tstests/core/executions/NukeExecution.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/core/executions/NukeExecution.test.ts
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.
Applied to files:
src/core/execution/NukeExecution.tstests/AllianceAcceptNukes.test.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/core/execution/NukeExecution.tssrc/core/game/GameImpl.tstests/AllianceAcceptNukes.test.tssrc/core/game/Game.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/core/execution/NukeExecution.tssrc/core/game/GameImpl.tstests/AllianceAcceptNukes.test.tssrc/core/game/Game.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/execution/NukeExecution.tssrc/core/game/GameImpl.tstests/AllianceAcceptNukes.test.tssrc/core/game/Game.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/core/execution/NukeExecution.tstests/AllianceAcceptNukes.test.tssrc/core/game/Game.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/execution/NukeExecution.tssrc/core/game/GameImpl.tstests/AllianceAcceptNukes.test.tssrc/core/game/Game.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/core/execution/NukeExecution.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/core/execution/NukeExecution.tstests/AllianceAcceptNukes.test.ts
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.
Applied to files:
src/core/game/GameImpl.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
tests/AllianceAcceptNukes.test.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/game/Game.ts
📚 Learning: 2025-11-21T22:30:12.246Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2493
File: src/core/game/TrainStation.ts:79-80
Timestamp: 2025-11-21T22:30:12.246Z
Learning: In src/core/game/TrainStation.ts and RailNetworkImpl.ts, the railroad network maintains a system invariant of at most one railroad between any two stations. RailNetworkImpl.connect is private and only called from connectToNearbyStations during initial station connection, which iterates over unique nearby units without duplicates. The Map<TrainStation, Railroad> structure in railroadByNeighbor correctly reflects this 1-to-1 relationship and should not be flagged as potentially allowing silent overwrites.
Applied to files:
src/core/game/Game.ts
🧬 Code graph analysis (1)
tests/AllianceAcceptNukes.test.ts (2)
src/core/game/Game.ts (3)
Game(674-780)Player(534-672)PlayerInfo(413-427)src/core/execution/NukeExecution.ts (1)
NukeExecution(19-383)
🔇 Additional comments (12)
src/core/execution/NukeExecution.ts (5)
43-48: Null guard correctly handles queued execution state.The guard at line 45 returns
nullwhenmgis uninitialized, which correctly signals that the nuke is queued (hasn't calledinit()yet). This integrates well with the newisInFlight()check.
86-88: Clean accessor for target tile.Simple getter that exposes the destination tile for external inspection. Works well with the new destroy logic.
90-92: Clear in-flight status check.Returns
truewhen the nuke unit has spawned. Simple and accurate indicator.
94-103: Proper cleanup for destroyed nukes.The method correctly deletes the nuke unit (if spawned), nulls the reference, and deactivates the execution. The
displayMessage=falseparameter prevents UI spam from alliance-destroyed nukes.
280-280: Defensive null guards in detonation flow.Using a local
targetvariable avoids repeated calls tothis.target(). The null checks at lines 285 and 352 are defensive—detonate()is only called for in-flight nukes, but the guards prevent edge cases if the target is cleared mid-flight.Also applies to: 284-286, 352-356
tests/AllianceAcceptNukes.test.ts (4)
49-63: Test correctly validates queued nuke destruction.The test ensures the target tile is owned by player2 before creating the nuke, then verifies the execution is removed after alliance acceptance. Logic is sound.
65-90: In-flight nuke destruction test is thorough.The test spawns a nuke, verifies it's in flight, then confirms alliance acceptance deactivates it. Good coverage of the in-flight path.
92-124: Counting test validates in-flight vs queued classification.The test spawns one nuke (in-flight) and queues another, then verifies the counts. Good test coverage of the counting logic.
126-136: Third-party nuke test validates scope.Verifies that nukes targeting a third player (player3) are not destroyed by an alliance between player1 and player2. Correct test logic.
src/core/game/GameImpl.ts (3)
391-493: Alliance message generation is thorough and well-structured.The code generates direction-specific messages for requestor and recipient, handles singular/plural grammar correctly, and properly filters inactive executions. The message logic depends on the 4-field return type, which is why the interface must match the implementation.
629-631: Clean accessor for all executions.Combines initialized and uninitialized executions for external inspection. Simple and correct.
540-587: Merge logic is correct. The implementation properly combines pending updates from before the tick with updates generated during tick execution. EachGameUpdateTypeenum key is initialized, so the merge loop executes and concatenates both update sets, ensuring updates like alliance acceptance messages are delivered alongside tick-generated updates.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/core/game/GameImpl.ts (3)
294-299: Simplify redundant type check.Line 297 checks
exec instanceof NukeExecutionagain after line 289 already confirmed this within thedestroyhelper. The nested ternary makes the target resolution harder to follow.🔎 Proposed simplification
- const target = - exec.isInFlight() && exec.target() - ? exec.target() - : exec instanceof NukeExecution - ? exec.targetTile() - : null; + // Already confirmed NukeExecution at line 289 + const target = exec.isInFlight() && exec.target() + ? exec.target() + : exec.targetTile();
303-310: Fragile type narrowing for target owner.The type guard at lines 304-306 checks
typeof target === "object" && "isPlayer" in targetto distinguish Player|TerraNullius from TileRef. Since TileRef is a number, checkingtypeof target === "number"first would be clearer and more robust.🔎 Proposed fix
let targetOwner: Player | TerraNullius; - if (typeof target === "object" && "isPlayer" in target) { - // target is already a Player or TerraNullius (in-flight nuke) - targetOwner = target as Player | TerraNullius; - } else { - // target is a TileRef (queued nuke) + if (typeof target === "number") { + // target is a TileRef (queued nuke) targetOwner = this.owner(target as TileRef); + } else { + // target is already a Player or TerraNullius (in-flight nuke) + targetOwner = target as Player | TerraNullius; }
395-487: Message generation could be simplified and internationalized.The message generation logic is repetitive across four similar blocks (lines 395-443 for directional counts, 445-487 for total counts). The PR objectives mention that UI text is processed via
translateTextand added toen.json, but these messages use hardcoded English strings.Consider extracting a helper function for pluralization and using
translateTextfor internationalization:function generateNukeDestroyedMessage( count: number, launcher: string, target: string, perspective: 'launcher' | 'target', category: 'directional' | 'inFlight' | 'queued' ): string { // Use translateText with interpolation // Reduce duplication across the four message blocks }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/game/Game.tssrc/core/game/GameImpl.tstests/AllianceAcceptNukes.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/AllianceAcceptNukes.test.ts
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/game/Game.tssrc/core/game/GameImpl.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/core/game/Game.tssrc/core/game/GameImpl.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/game/Game.tssrc/core/game/GameImpl.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/core/game/Game.tssrc/core/game/GameImpl.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/core/game/Game.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/game/Game.ts
📚 Learning: 2025-11-21T22:30:12.246Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2493
File: src/core/game/TrainStation.ts:79-80
Timestamp: 2025-11-21T22:30:12.246Z
Learning: In src/core/game/TrainStation.ts and RailNetworkImpl.ts, the railroad network maintains a system invariant of at most one railroad between any two stations. RailNetworkImpl.connect is private and only called from connectToNearbyStations during initial station connection, which iterates over unique nearby units without duplicates. The Map<TrainStation, Railroad> structure in railroadByNeighbor correctly reflects this 1-to-1 relationship and should not be flagged as potentially allowing silent overwrites.
Applied to files:
src/core/game/Game.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/core/game/Game.ts
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.
Applied to files:
src/core/game/GameImpl.ts
🧬 Code graph analysis (2)
src/core/game/Game.ts (1)
src/core/Schemas.ts (1)
Player(122-122)
src/core/game/GameImpl.ts (3)
src/core/game/Game.ts (4)
Player(534-672)DestroyNukesResult(820-825)Execution(358-363)TerraNullius(521-526)src/core/execution/NukeExecution.ts (2)
NukeExecution(19-383)target(43-48)src/core/game/AllianceImpl.ts (2)
requestor(26-28)recipient(30-32)
🔇 Additional comments (4)
src/core/game/Game.ts (2)
770-773: Interface additions look good.The
destroyNukesBetweenmethod signature correctly returns the fullDestroyNukesResulttype, and theexecutions()accessor provides needed visibility into the execution queue.
820-825: Type definition is clear and well-structured.The
DestroyNukesResulttype clearly captures the counts needed for both display messages and tracking nuke destruction directionality.src/core/game/GameImpl.ts (2)
534-581: Verify the necessity of update merging change.The change to merge
pendingupdates with new updates (lines 573-580) appears unrelated to nuke destruction. This modification ensures updates generated during the tick aren't lost, but it's unclear if this is:
- A bug fix for an existing issue
- Required infrastructure for the nuke destruction feature
- An accidental inclusion from another branch
Please confirm this change is intentional and necessary for this PR.
623-625: Clean implementation of executions accessor.The method correctly exposes both initialized (
execs) and uninitialized (unInitExecs) execution queues, which is needed bydestroyNukesBetweento handle both in-flight and queued nukes.
|
One thought - this might be annoying if your ally gets close to a nuke's landing zone but you still want it to destroy an enemy structure near your ally's. e.g. you're trying to destroy a missile silo, and your new ally comes close enough to block you from destroying it. Then you get nuked or h-bombed because you accepted the alliance at a bad time. What if instead of destroying the nuke, you make the nuke not damage your ally (but still splash elsewhere), ONLY if you allied after launching the nuke? To put it in pseudocode: |
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
Executions shouldn't expose any state. Instead, maybe on PlayerExecution, on each tick, look for incoming nukes and check if their owner isFriendly with myPlayer.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/core/game/GameImpl.ts (2)
294-299: Redundant instanceof check can be removed.At line 289, we already verify
exec instanceof NukeExecution. The second check at line 297 is always true and can be simplified.🔎 Suggested simplification
const target = exec.isInFlight() && exec.target() ? exec.target() - : exec instanceof NukeExecution - ? exec.targetTile() - : null; + : exec.targetTile();
304-310: Duck-typing for Player vs TileRef works but is a bit fragile.Checking
"isPlayer" in targetis clever but relies on the Player/TerraNullius interface shape. If TileRef ever gains anisPlayerproperty, this breaks. Consider a more explicit check.🔎 Alternative using typeof for TileRef (which is number)
let targetOwner: Player | TerraNullius; - if (typeof target === "object" && "isPlayer" in target) { + if (typeof target !== "number") { // target is already a Player or TerraNullius (in-flight nuke) targetOwner = target as Player | TerraNullius; } else { // target is a TileRef (queued nuke) - targetOwner = this.owner(target as TileRef); + targetOwner = this.owner(target); }Since
TileRefis a type alias fornumber(persrc/core/game/GameMap.ts), checkingtypeof target !== "number"is more direct and won't break if interface shapes change.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/core/execution/NukeExecution.tssrc/core/game/Game.tssrc/core/game/GameImpl.tstests/core/executions/NukeExecution.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/core/executions/NukeExecution.test.ts
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/game/Game.tssrc/core/execution/NukeExecution.tssrc/core/game/GameImpl.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/core/game/Game.tssrc/core/execution/NukeExecution.tssrc/core/game/GameImpl.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/game/Game.tssrc/core/execution/NukeExecution.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/core/game/Game.tssrc/core/execution/NukeExecution.tssrc/core/game/GameImpl.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/core/game/Game.tssrc/core/execution/NukeExecution.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/game/Game.ts
📚 Learning: 2025-11-21T22:30:12.246Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2493
File: src/core/game/TrainStation.ts:79-80
Timestamp: 2025-11-21T22:30:12.246Z
Learning: In src/core/game/TrainStation.ts and RailNetworkImpl.ts, the railroad network maintains a system invariant of at most one railroad between any two stations. RailNetworkImpl.connect is private and only called from connectToNearbyStations during initial station connection, which iterates over unique nearby units without duplicates. The Map<TrainStation, Railroad> structure in railroadByNeighbor correctly reflects this 1-to-1 relationship and should not be flagged as potentially allowing silent overwrites.
Applied to files:
src/core/game/Game.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/core/game/Game.tssrc/core/execution/NukeExecution.ts
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.
Applied to files:
src/core/execution/NukeExecution.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/core/execution/NukeExecution.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/core/execution/NukeExecution.ts
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.
Applied to files:
src/core/game/GameImpl.ts
🧬 Code graph analysis (2)
src/core/game/Game.ts (1)
src/core/Schemas.ts (1)
Player(122-122)
src/core/game/GameImpl.ts (6)
src/core/Schemas.ts (1)
Player(122-122)src/core/game/Game.ts (4)
Player(534-672)DestroyNukesResult(820-825)Execution(358-363)TerraNullius(521-526)src/core/execution/NukeExecution.ts (2)
NukeExecution(19-385)target(44-49)src/core/game/PlayerImpl.ts (1)
target(540-543)src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/AllianceImpl.ts (2)
requestor(26-28)recipient(30-32)
🔇 Additional comments (10)
src/core/game/Game.ts (2)
770-773: LGTM! Clean interface additions for nuke destruction feature.The new
destroyNukesBetweenandexecutionsmethods provide the needed public API surface for alliance-based nuke cancellation. Return type now matches the implementation.
820-825: Type definition is clear and well-structured.Using a plain object type with four distinct counters (
inFlight,queued,fromRequestorToRecipient,fromRecipientToRequestor) is clean and easy to work with. Good use of typed union-style data over classes.src/core/execution/NukeExecution.ts (4)
44-49: Good addition of null return for queued executions.The null check for
!this.mgcorrectly identifies executions that have not been initialized yet. This allows callers to distinguish between "queued but not launched" and "in-flight" states.
87-93: Clean public accessors for target tile and flight state.
targetTile()gives direct access to the destination, andisInFlight()provides a simple boolean check based on whether the nuke unit exists. Both are straightforward and avoid exposing internal state directly.
95-104: Destruction logic is safe with proper guards.The early return on
!this.activeprevents double-deactivation. Nullifyingthis.nukeafter deletion is good cleanup. Thefalseparameter todelete()correctly suppresses the UI message for alliance-cancelled nukes.
282-287: Good refactor to cache target and guard stats call.Capturing
targetin a local variable avoids repeated method calls and makes the code clearer. The null guard beforebombLandprevents errors if the target tile has no owner at detonation time.Also applies to: 354-358
src/core/game/GameImpl.ts (4)
395-467: Verify: users may see overlapping nuke destruction messages.When nukes are destroyed, users receive up to three separate message types:
- Per-direction messages (lines 395-443): "X nukes launched towards Y were destroyed"
- In-flight summary (lines 445-467): "X nukes in flight were neutralized"
Since in-flight nukes are already counted in the per-direction totals, this may show redundant info. For example, if player A has 2 nukes in flight toward player B, the user could see both "2 nukes launched towards B were destroyed" AND "2 nukes in flight were neutralized".
Is this intentional UX, or should the in-flight message only show when there's additional context not covered by per-direction messages?
573-580: Update merging approach works but adds allocation overhead.Creating a new
mergedmap, iterating all keys, spreading both arrays, then resettingthis.updatesis correct. A minor optimization would be to push pending items intothis.updatesdirectly before returning, but this is fine for now.
623-625: Clean implementation of executions() accessor.Spreading both arrays into a new array prevents external mutation of internal state. Good defensive approach.
282-349: Overall implementation of destroyNukesBetween is solid.The logic correctly:
- Iterates both initialized (
execs) and queued (unInitExecs) executions- Distinguishes in-flight vs queued based on
isInFlight()- Tracks counts per direction for messaging
- Calls
destroyInFlight()to deactivate matching nukesGood use of a nested helper function to avoid duplication.
plazmaezio
force-pushed
the
feat/destroy-incoming-nukes-on-alliance
branch
from
5c7f595 to
0e466b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/core/executions/NukeExecution.test.ts (1)
129-149: Optional: verify isInFlight() also returns false after destruction.The test correctly validates that
isActive()returns false after callingdestroyInFlight(). For more complete coverage, consider also asserting thatisInFlight()returns false, sincedestroyInFlight()clears the nuke reference.🔎 Suggested enhancement
exec.destroyInFlight(); expect(exec.isActive()).toBe(false); + expect(exec.isInFlight()).toBe(false);tests/AllianceAcceptNukes.test.ts (1)
176-188: Remove commented code or document why it's preserved.Line 176 contains a commented-out assertion. If this assertion is no longer needed, remove it for clarity. If it's temporarily disabled pending future work, add a comment explaining why.
🔎 Suggested cleanup
const messages = updates[GameUpdateType.DisplayEvent]?.map((e) => e.message) ?? []; - // expect(messages.some((m) => m.includes("planned nuke"))).toBe(true); - - // Expect both the queued (planned) message and a directional message expect( messages.some( (m) => m.includes("planned nuke") || m.includes("launched towards") || m.includes("launched at") || m.includes("launched by"), ), ).toBe(true);src/core/game/GameImpl.ts (2)
288-328: Simplify target resolution and strengthen type narrowing.The nested ternary on lines 294-299 contains a redundant
instanceof NukeExecutioncheck (line 297) since line 289 already guards for this. Additionally, the type narrowing on lines 304-305 using"isPlayer" in targetis fragile and relies on duck typing rather than proper type guards.🔎 Suggested refactor
Simplify the target resolution:
const launcher = exec.owner(); - // queued execution -> target not resolvable yet - const target = - exec.isInFlight() && exec.target() - ? exec.target() - : exec instanceof NukeExecution - ? exec.targetTile() - : null; + // In-flight nukes have resolved targets; queued nukes use tile refs + const target = exec.isInFlight() && exec.target() + ? exec.target() + : exec.targetTile(); if (!target) return; let targetOwner: Player | TerraNullius; - if (typeof target === "object" && "isPlayer" in target) { - // target is already a Player or TerraNullius (in-flight nuke) - targetOwner = target as Player | TerraNullius; - } else { - // target is a TileRef (queued nuke) - targetOwner = this.owner(target as TileRef); - } + // Type narrow: if target has isPlayer method, it's Player/TerraNullius + if (typeof target === "object" && target !== null && "isPlayer" in target) { + targetOwner = target as Player | TerraNullius; + } else { + targetOwner = this.owner(target as TileRef); + }The redundant check is removed and the type narrowing adds an explicit null check for safety.
395-487: Message generation is correct but could benefit from helper extraction.The directional and aggregate message generation correctly handles pluralization and provides clear messages to both players. The logic is sound, though the string concatenation is repetitive.
🔎 Optional: extract pluralization helper
Consider extracting a helper function to reduce repetition:
function pluralizeNuke(count: number, verb: "was" | "were"): string { return `${count} nuke${count > 1 ? "s" : ""} ${verb === "was" ? (count > 1 ? "were" : "was") : verb}`; }This could simplify messages like:
const requestorMsg = `${pluralizeNuke(fromRequestorToRecipient, "was")} launched towards ${recipient.displayName()} destroyed due to the alliance`;However, this is optional and the current implementation is perfectly functional.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/core/execution/NukeExecution.tssrc/core/game/Game.tssrc/core/game/GameImpl.tstests/AllianceAcceptNukes.test.tstests/core/executions/NukeExecution.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/game/Game.ts
- src/core/execution/NukeExecution.ts
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
tests/core/executions/NukeExecution.test.tssrc/core/game/GameImpl.tstests/AllianceAcceptNukes.test.ts
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.
Applied to files:
tests/core/executions/NukeExecution.test.tstests/AllianceAcceptNukes.test.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
tests/core/executions/NukeExecution.test.tssrc/core/game/GameImpl.tstests/AllianceAcceptNukes.test.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
tests/core/executions/NukeExecution.test.tssrc/core/game/GameImpl.tstests/AllianceAcceptNukes.test.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/game/GameImpl.tstests/AllianceAcceptNukes.test.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/game/GameImpl.ts
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.
Applied to files:
src/core/game/GameImpl.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
tests/AllianceAcceptNukes.test.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
tests/AllianceAcceptNukes.test.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
tests/AllianceAcceptNukes.test.ts
🧬 Code graph analysis (1)
tests/core/executions/NukeExecution.test.ts (1)
src/core/execution/NukeExecution.ts (1)
NukeExecution(19-385)
🔇 Additional comments (1)
src/core/game/GameImpl.ts (1)
534-581: Critical fix: preserves pending updates accumulated between ticks.This change fixes a bug where updates added to
this.updatesoutside ofexecuteNextTick()(such as byacceptAllianceRequest()) would be lost when line 536 cleared the updates map. The new logic correctly saves pending updates before clearing, executes the tick to generate new updates, then merges both sets.The merge ensures that:
- Updates from alliance acceptance (which calls
destroyNukesBetween())- Updates from tick execution
- Player state updates
...are all included in the returned update batch.
plazmaezio
force-pushed
the
feat/destroy-incoming-nukes-on-alliance
branch
from
0e466b9 to
45f83e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/core/game/GameImpl.ts (1)
534-580: Optional: minor redundancy in update reset.Line 579 resets
this.updatesto an empty map, but line 536 already resets it at the start of the nextexecuteNextTick()call. The reset on line 579 is redundant since the function returns immediately afterward.While not incorrect, you could remove line 579 for simplicity.
🔎 Optional simplification
const merged = createGameUpdatesMap(); for (const k in merged) { merged[k] = [...pending[k], ...this.updates[k]]; } - this.updates = createGameUpdatesMap(); return merged;The reset on line 536 at the start of the next call is sufficient.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/core/execution/NukeExecution.tssrc/core/game/Game.tssrc/core/game/GameImpl.tstests/AllianceAcceptNukes.test.tstests/core/executions/NukeExecution.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/AllianceAcceptNukes.test.ts
- src/core/game/Game.ts
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
tests/core/executions/NukeExecution.test.tssrc/core/execution/NukeExecution.tssrc/core/game/GameImpl.ts
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.
Applied to files:
tests/core/executions/NukeExecution.test.tssrc/core/execution/NukeExecution.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
tests/core/executions/NukeExecution.test.tssrc/core/execution/NukeExecution.tssrc/core/game/GameImpl.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
tests/core/executions/NukeExecution.test.tssrc/core/execution/NukeExecution.tssrc/core/game/GameImpl.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/core/execution/NukeExecution.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/execution/NukeExecution.tssrc/core/game/GameImpl.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/core/execution/NukeExecution.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/core/execution/NukeExecution.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/game/GameImpl.ts
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.
Applied to files:
src/core/game/GameImpl.ts
🧬 Code graph analysis (2)
tests/core/executions/NukeExecution.test.ts (1)
src/core/execution/NukeExecution.ts (1)
NukeExecution(20-375)
src/core/game/GameImpl.ts (2)
src/core/execution/NukeExecution.ts (2)
NukeExecution(20-375)target(45-50)src/core/game/GameMap.ts (1)
TileRef(3-3)
🔇 Additional comments (4)
src/core/execution/NukeExecution.ts (3)
45-50: LGTM: Clean null guard for queued executions.The guard correctly handles the uninitialized state when executions are queued but not yet spawned.
70-87: LGTM: Well-designed lifecycle controls.The new public API cleanly exposes nuke state (in-flight vs queued) and provides a safe cleanup mechanism via
destroyInFlight().
272-277: LGTM: Proper null safety in detonate().Caching
targetin a local variable and guarding stats calls prevents potential null reference issues.Also applies to: 344-348
src/core/game/GameImpl.ts (1)
385-393: The filtering ofunInitExecsat line 393 is correct and safe. When alliance acceptance occurs during tick execution (viaBotExecution.acceptAllAllianceRequests()), the filter removes executions for destroyed nukes before they are initialized in the next phase. Since JavaScript is single-threaded and the initialization phase (lines 547-554) runs after the tick loop completes, there is no race condition or concurrent modification issue. The placement of this filter immediately afterdestroyNukesBetween()is intentional for prompt cleanup.
| const destroy = (exec: Execution, isQueued: boolean) => { | ||
| if (!(exec instanceof NukeExecution)) return; | ||
|
|
||
| const launcher = exec.owner(); | ||
|
|
||
| // queued execution -> target not resolvable yet | ||
| const target = | ||
| exec.isInFlight() && exec.target() | ||
| ? exec.target() | ||
| : exec instanceof NukeExecution | ||
| ? exec.targetTile() | ||
| : null; | ||
|
|
||
| if (!target) return; | ||
|
|
||
| let targetOwner: Player | TerraNullius; | ||
| if (typeof target === "object" && "isPlayer" in target) { | ||
| // target is already a Player or TerraNullius (in-flight nuke) | ||
| targetOwner = target as Player | TerraNullius; | ||
| } else { | ||
| // target is a TileRef (queued nuke) | ||
| targetOwner = this.owner(target as TileRef); | ||
| } | ||
|
|
||
| const isRequestorToRecipient = launcher === p1 && targetOwner === p2; | ||
| const isRecipientToRequestor = launcher === p2 && targetOwner === p1; | ||
|
|
||
| const isBetween = isRequestorToRecipient || isRecipientToRequestor; | ||
|
|
||
| if (!isBetween) { | ||
| return; | ||
| } | ||
|
|
||
| if (isQueued) queued++; | ||
| else inFlight++; | ||
|
|
||
| if (isRequestorToRecipient) fromRequestorToRecipient++; | ||
| else fromRecipientToRequestor++; | ||
|
|
||
| exec.destroyInFlight(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor type discrimination logic for clarity and safety.
Lines 294-310 use fragile runtime type checking to distinguish between Player | TerraNullius (in-flight) vs TileRef (queued). The pattern has several issues:
- Line 297: Redundant
instanceof NukeExecutioncheck (already verified on line 289) - Lines 304-306: Unsafe cast to
Player | TerraNulliuswithout verifying the object actually has expected methods - Type discrimination relies on
typeof === "object"which is implicit and unclear
🔎 Proposed refactor for explicit type checking
const destroy = (exec: Execution, isQueued: boolean) => {
if (!(exec instanceof NukeExecution)) return;
const launcher = exec.owner();
- // queued execution -> target not resolvable yet
- const target =
- exec.isInFlight() && exec.target()
- ? exec.target()
- : exec instanceof NukeExecution
- ? exec.targetTile()
- : null;
-
- if (!target) return;
-
- let targetOwner: Player | TerraNullius;
- if (typeof target === "object" && "isPlayer" in target) {
- // target is already a Player or TerraNullius (in-flight nuke)
- targetOwner = target as Player | TerraNullius;
- } else {
- // target is a TileRef (queued nuke)
- targetOwner = this.owner(target as TileRef);
- }
+ let targetOwner: Player | TerraNullius | null;
+
+ if (exec.isInFlight()) {
+ // In-flight: target() returns Player | TerraNullius | null
+ targetOwner = exec.target();
+ } else {
+ // Queued: resolve owner from targetTile()
+ targetOwner = this.owner(exec.targetTile());
+ }
+
+ if (!targetOwner) return;
const isRequestorToRecipient = launcher === p1 && targetOwner === p2;
const isRecipientToRequestor = launcher === p2 && targetOwner === p1;
const isBetween = isRequestorToRecipient || isRecipientToRequestor;
if (!isBetween) {
return;
}
if (isQueued) queued++;
else inFlight++;
if (isRequestorToRecipient) fromRequestorToRecipient++;
else fromRecipientToRequestor++;
exec.destroyInFlight();
};| const { | ||
| inFlight, | ||
| queued, | ||
| fromRequestorToRecipient, | ||
| fromRecipientToRequestor, | ||
| } = this.destroyNukesBetween(requestor, recipient); | ||
|
|
||
| // Destroy counts available for display messages | ||
| this.unInitExecs = this.unInitExecs.filter((e) => e.isActive()); | ||
|
|
||
| if (fromRequestorToRecipient > 0) { | ||
| const requestorMsg = `${fromRequestorToRecipient} nuke${ | ||
| fromRequestorToRecipient > 1 ? "s" : "" | ||
| } launched towards ${recipient.displayName()} ${ | ||
| fromRequestorToRecipient > 1 ? "were" : "was" | ||
| } destroyed due to the alliance`; | ||
|
|
||
| const recipientMsg = `${fromRequestorToRecipient} nuke${ | ||
| fromRequestorToRecipient > 1 ? "s" : "" | ||
| } launched by ${requestor.displayName()} towards you ${ | ||
| fromRequestorToRecipient > 1 ? "were" : "was" | ||
| } destroyed due to the alliance`; | ||
|
|
||
| this.displayMessage( | ||
| requestorMsg, | ||
| MessageType.ALLIANCE_ACCEPTED, | ||
| requestor.id(), | ||
| ); | ||
| this.displayMessage( | ||
| recipientMsg, | ||
| MessageType.ALLIANCE_ACCEPTED, | ||
| recipient.id(), | ||
| ); | ||
| } | ||
|
|
||
| if (fromRecipientToRequestor > 0) { | ||
| const requestorMsg = `${fromRecipientToRequestor} nuke${ | ||
| fromRecipientToRequestor > 1 ? "s" : "" | ||
| } launched by ${recipient.displayName()} towards you ${ | ||
| fromRecipientToRequestor > 1 ? "were" : "was" | ||
| } destroyed due to the alliance`; | ||
|
|
||
| const recipientMsg = `${fromRecipientToRequestor} nuke${ | ||
| fromRecipientToRequestor > 1 ? "s" : "" | ||
| } launched towards ${requestor.displayName()} ${ | ||
| fromRecipientToRequestor > 1 ? "were" : "was" | ||
| } destroyed due to the alliance`; | ||
|
|
||
| this.displayMessage( | ||
| requestorMsg, | ||
| MessageType.ALLIANCE_ACCEPTED, | ||
| requestor.id(), | ||
| ); | ||
| this.displayMessage( | ||
| recipientMsg, | ||
| MessageType.ALLIANCE_ACCEPTED, | ||
| recipient.id(), | ||
| ); | ||
| } | ||
|
|
||
| if (inFlight > 0) { | ||
| const baseMsg = `${inFlight} nuke${inFlight > 1 ? "s" : ""} in flight ${ | ||
| inFlight > 1 ? "were" : "was" | ||
| } neutralized due to alliance formation with`; | ||
|
|
||
| const requestorMsg = `${inFlight} nuke${ | ||
| inFlight > 1 ? "s" : "" | ||
| } in flight ${ | ||
| inFlight > 1 ? "were" : "was" | ||
| } neutralized due to alliance formation with ${recipient.displayName()}`; | ||
| const recipientMsg = baseMsg + ` ${requestor.displayName()}`; | ||
|
|
||
| this.displayMessage( | ||
| requestorMsg, | ||
| MessageType.ALLIANCE_ACCEPTED, | ||
| requestor.id(), | ||
| ); | ||
| this.displayMessage( | ||
| recipientMsg, | ||
| MessageType.ALLIANCE_ACCEPTED, | ||
| recipient.id(), | ||
| ); | ||
| } | ||
|
|
||
| if (queued > 0) { | ||
| const baseMsg = `${queued} planned nuke${queued > 1 ? "s" : ""} ${ | ||
| queued > 1 ? "were" : "was" | ||
| } canceled due to alliance formation with`; | ||
|
|
||
| const requestorMsg = baseMsg + ` ${recipient.displayName()}`; | ||
| const recipientMsg = baseMsg + ` ${requestor.displayName()}`; | ||
|
|
||
| this.displayMessage( | ||
| requestorMsg, | ||
| MessageType.ALLIANCE_ACCEPTED, | ||
| requestor.id(), | ||
| ); | ||
| this.displayMessage( | ||
| recipientMsg, | ||
| MessageType.ALLIANCE_ACCEPTED, | ||
| recipient.id(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Duplicate messages: directional and type-based counters are not mutually exclusive.
The message logic sends multiple notifications for the same nukes:
- Lines 395-418: Messages when
fromRequestorToRecipient > 0 - Lines 420-443: Messages when
fromRecipientToRequestor > 0 - Lines 445-467: Messages when
inFlight > 0 - Lines 469-487: Messages when
queued > 0
Problem: These conditions are not mutually exclusive. A single in-flight nuke from requestor to recipient increments both fromRequestorToRecipient and inFlight, triggering messages in both blocks 1 and 3.
Example: If player A has 1 in-flight nuke targeting player B when they ally:
- Player A receives: "1 nuke launched towards B was destroyed" (line 396) AND "1 nuke in flight was neutralized" (line 450)
- Player B receives: "1 nuke launched by A towards you was destroyed" (line 402) AND "1 nuke in flight was neutralized" (line 455)
🔎 Recommended fix: use mutually exclusive conditions
Consider this approach:
- If nukes are one-directional (only A→B or only B→A): show directional messages
- If nukes are mutual (both A→B and B→A): show non-directional summary messages
- Split in-flight and queued within each category
- if (fromRequestorToRecipient > 0) {
+ const hasMutualNukes = fromRequestorToRecipient > 0 && fromRecipientToRequestor > 0;
+
+ if (!hasMutualNukes && fromRequestorToRecipient > 0) {
// ... directional messages for requestor → recipient
}
- if (fromRecipientToRequestor > 0) {
+ if (!hasMutualNukes && fromRecipientToRequestor > 0) {
// ... directional messages for recipient → requestor
}
- if (inFlight > 0) {
+ if (hasMutualNukes && inFlight > 0) {
// ... non-directional in-flight messages
}
- if (queued > 0) {
+ if (hasMutualNukes && queued > 0) {
// ... non-directional queued messages
}Or alternatively, always use directional messages and remove the non-directional blocks entirely.
Committable suggestion skipped: line range outside the PR's diff.
| test("destroyInFlight deactivates execution and removes nuke unit", async () => { | ||
| player.buildUnit(UnitType.MissileSilo, game.ref(1, 1), {}); | ||
|
|
||
| const exec = new NukeExecution( | ||
| UnitType.AtomBomb, | ||
| player, | ||
| game.ref(10, 10), | ||
| game.ref(1, 1), | ||
| ); | ||
|
|
||
| game.addExecution(exec); | ||
| game.executeNextTick(); // init | ||
| game.executeNextTick(); // spawn nuke | ||
|
|
||
| expect(exec.isInFlight()).toBe(true); | ||
| expect(exec.isActive()).toBe(true); | ||
|
|
||
| exec.destroyInFlight(); | ||
|
|
||
| expect(exec.isActive()).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Verify nuke unit removal in test assertions.
The test title claims it "removes nuke unit", but only isActive() is asserted after destroyInFlight(). Consider also verifying:
isInFlight()returns false- The nuke unit no longer exists in the game
🔎 Suggested additional assertions
exec.destroyInFlight();
expect(exec.isActive()).toBe(false);
+expect(exec.isInFlight()).toBe(false);
+expect(exec.getNuke()).toBeNull();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/core/executions/NukeExecution.test.ts around lines 129-149, the test
title says "removes nuke unit" but only asserts isActive(); update the test to
also assert that exec.isInFlight() is false after destroyInFlight() and that the
nuke unit no longer exists in the game (e.g. check the game/unit registry or
query by position/ref to ensure the spawned nuke unit was removed). Ensure these
assertions run after exec.destroyInFlight() and use existing game APIs to verify
unit absence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/core/game/Game.ts (1)
778-780: Consider more descriptive parameter names and add JSDoc comments.The parameter names
p1andp2are generic. SinceDestroyNukesResultincludes direction-specific fields (fromRequestorToRecipient,fromRecipientToRequestor), it would help callers to use names that indicate the semantic ordering, such asrequestorandrecipientor at leastplayerAandplayerB.Additionally, JSDoc comments would clarify the behavior—especially what "between" means (bidirectional destruction?) and how the result fields map to the parameters.
💡 Suggested improvement
+ /** + * Destroys all queued and in-flight nukes between two players (bidirectional). + * @param requestor - The alliance requestor + * @param recipient - The alliance recipient + * @returns Counts of destroyed nukes by flight state and direction + */ - destroyNukesBetween(p1: Player, p2: Player): DestroyNukesResult; + destroyNukesBetween(requestor: Player, recipient: Player): DestroyNukesResult; + /** + * Returns all active executions in the game. + */ executions(): Execution[];
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/game/Game.ts
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/game/Game.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/core/game/Game.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/game/Game.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/core/game/Game.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/core/game/Game.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/game/Game.ts
📚 Learning: 2025-11-21T22:30:12.246Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2493
File: src/core/game/TrainStation.ts:79-80
Timestamp: 2025-11-21T22:30:12.246Z
Learning: In src/core/game/TrainStation.ts and RailNetworkImpl.ts, the railroad network maintains a system invariant of at most one railroad between any two stations. RailNetworkImpl.connect is private and only called from connectToNearbyStations during initial station connection, which iterates over unique nearby units without duplicates. The Map<TrainStation, Railroad> structure in railroadByNeighbor correctly reflects this 1-to-1 relationship and should not be flagged as potentially allowing silent overwrites.
Applied to files:
src/core/game/Game.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/core/game/Game.ts
🔇 Additional comments (1)
src/core/game/Game.ts (1)
827-832: Clean type definition for nuke destruction results.The
DestroyNukesResulttype is well-defined with descriptive field names. Using a typed object rather than a class is appropriate here, following clean TypeScript practices.
5 participants
Resolves #2484
Description:
This prevents the traitor debuff from being applied on impact, even if the nukes were launched before the alliance was formed.
This is what the players will see:
Edge case:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
assessin.