Destroy incoming nukes when alliance is created #2716
Conversation
WalkthroughAdds APIs and game logic to track, count, and cancel queued and in-flight nukes between two players (including destroying in-flight nukes), integrates this into alliance acceptance flow, and adds unit and integration tests covering the behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant PlayerA as Player A
participant Game as Game
participant NukeExec as NukeExecution
participant Alliance as AllianceSystem
participant UI as UI
PlayerA->>Game: acceptAllianceRequest(playerB)
activate Game
Game->>Game: destroyNukesBetween(playerA, playerB)
activate Game
note right of Game `#DDEFFD`: iterate executions() (queued + active)
alt execution targets playerB
Game->>NukeExec: isInFlight()?
alt in-flight
NukeExec-->>Game: true
rect rgb(235,247,235)
note right of Game `#E8F8E8`: call destroyInFlight()
Game->>NukeExec: destroyInFlight()
NukeExec->>NukeExec: deactivate, remove unit
end
else queued
NukeExec-->>Game: false
note right of Game `#FFF7DD`: remove queued execution
end
else targets other
note right of Game `#F5F5F5`: skip (preserve)
end
Game-->>Game: return DestroyNukesResult {inFlight, queued, fromRequestorToRecipient, fromRecipientToRequestor}
deactivate Game
Game->>Alliance: continue acceptance (include counts)
Game->>UI: emit display events for canceled nukes
UI->>PlayerA: show messages
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: |
3 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.