fix: Check alliance breakage once at launch, and is deterministic now #2554

bibizu · 2025-12-03T05:01:45Z

Description:

Removes the check to break alliance after nuke is launched, and alliance breaking is now determined before tiles are randomly chosen for destruction. (It is now slightly stricter, I believe, as a weighted average calculation is a little tricky)

Please complete the following:

I have added screenshots for all UI updates
I process any text displayed to the user through translateText() and I've added it to the en.json file
I have added relevant tests to the test directory
I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced

Please put your Discord username so you can be contacted if a bug or regression is found:

bibizu

And only breaks alliances once, on launch.

coderabbitai · 2025-12-03T05:02:14Z

Walkthrough

Replaces the cached tile-impact logic with a weighted in-range computation (tilesInRange: inner=1.0, outer=0.5) used to decide alliance breaks before detonation; retains tilesToDestroy() for actual destruction. Adds circleSearch APIs on GameMap/GameImpl/GameView.

Changes

Cohort / File(s)	Change Summary
Nuke execution — range & alliance weighting `src/core/execution/NukeExecution.ts`	Adds `tilesInRange(): Map<TileRef, number>` (weights 1.0 inner, 0.5 outer) and throws if uninitialized; keeps `tilesToDestroy()` for destruction; changes `maybeBreakAlliances` signature to accept weighted `Map<TileRef, number>` and aggregate per-owner weights; `tick()` now calls `maybeBreakAlliances(this.tilesInRange())`; removes alliance checks from `detonate()`; minor comment fixes.
Map circle-search API `src/core/game/GameMap.ts`, `src/core/game/GameImpl.ts`, `src/core/game/GameView.ts`	Adds `circleSearch(tile: TileRef, radius: number, filter?: (tile: TileRef, d2: number) => boolean): Set<TileRef>` to `GameMap` interface and `GameMapImpl`; exposes `circleSearch` on `GameImpl` and `GameView` that delegate to the map. Implementation scans bounding square, uses `euclideanDistSquared`, and applies optional filter.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Check correctness of radius math and boundary handling in tilesInRange() and circleSearch.
Verify maybeBreakAlliances() aggregates weighted impact correctly and that MIRVWarhead is excluded.
Ensure tick()/detonate() ordering is correct and no detonation-time alliance side-effects remain.
Review potential perf impact of computing tilesInRange() each tick.

Possibly related PRs

bugfix: when nuking nearby water, the attacker was not marked as a traitor on launch #1574 — Modifies NukeExecution.ts alliance-breaking logic; strong overlap with weighted-alliance changes.
Break alliance on launch #775 — Also touches NukeExecution.ts alliance behavior and tilesToDestroy usage; relates to cache/launch-time logic.
SAM smart targeting #1618 — Changes nuke in-range/trajectory computations in NukeExecution.ts; likely overlaps with range calculations.

Suggested labels

Bug Fix, Feature - Backend

Suggested reviewers

evanpelle
scottanderson

Poem

A circle counts each tile in range,
Some full, some half — a subtle change,
Alliances weighed before the blast,
Destruction kept for moments past,
Code hums soft, the search arranged 🎯

Pre-merge checks

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: moving alliance breakage check to launch time and making it deterministic, which aligns with the primary modifications in NukeExecution.ts.
Description check	✅ Passed	The description clearly explains the key changes: removing post-launch alliance checks, moving to pre-destruction determination, and notes the weighted calculation approach. It is directly related to the changeset.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

each check is only called once now

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/core/execution/NukeExecution.ts (1)

85-100: Alliance-weighting logic matches the new design; consider clearer naming for weights

The move to Map<TileRef, number> and summing weight per attackedPlayer matches the “weighted by chance of destruction” idea and should behave deterministically once tilesInRange() is fixed.

One small clarity tweak: the variable name tilesDestroyed in the loop over attacked now represents a weighted sum (e.g. 1 and 0.5), not a literal tile count. Renaming it to something like attackWeight or effectiveTiles would make the fractional nature obvious and avoid confusion for future readers.

No behavior change needed here, just a naming clean‑up if you agree.

Also applies to: 109-125

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 997cfea and 16c18f3.

📒 Files selected for processing (1)

src/core/execution/NukeExecution.ts (5 hunks)

🧰 Additional context used

🧠 Learnings (11)

📚 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-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.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.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.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-11-26T20:49:29.110Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.110Z
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/execution/NukeExecution.ts

📚 Learning: 2025-11-26T22:27:31.808Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.808Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>

Applied to files:

src/core/execution/NukeExecution.ts

📚 Learning: 2025-11-29T22:22:37.170Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2539
File: src/core/game/GameImpl.ts:520-542
Timestamp: 2025-11-29T22:22:37.170Z
Learning: In GameImpl.ts, neighborsWithDiag() and forEachNeighborWithDiag() intentionally duplicate coordinate iteration logic. They serve different purposes: forEachNeighborWithDiag() is a zero-allocation hot-path optimization while neighborsWithDiag() is a convenience method that returns an array. Refactoring one to call the other would add callback/closure allocations and indirection overhead, defeating the performance goals.

Applied to files:

src/core/execution/NukeExecution.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 specific UnitType enum values (AtomBomb, HydrogenBomb, MIRV, MIRVWarhead). This means that comparisons in switch statements between a NukeType parameter and UnitType enum values are valid and will work correctly at runtime.

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

🧬 Code graph analysis (1)

src/core/execution/NukeExecution.ts (3)

src/core/game/GameMap.ts (2)

TileRef (3-3)

magnitude (167-169)

src/core/game/GameView.ts (1)

magnitude (717-719)

src/core/game/GameImpl.ts (1)

magnitude (840-842)

🔇 Additional comments (2)

src/core/execution/NukeExecution.ts (2)

147-147: Pre‑launch alliance check via tilesInRange() in tick() looks correct

Calling maybeBreakAlliances(this.tilesInRange()) immediately after spawning the nuke matches the PR goal: alliance status is decided once, at launch time, based on geometry rather than the later random destruction set.

Assuming tilesInRange() is fixed as above, this call site looks good and keeps the behavior deterministic.

23-24: Fix tilesInRange cache typing and verify BFS radius boundary logic

The tilesInRangeCache field is declared as Map<TileRef, number> but should likely be Map<TileRef, number> | undefined to match usage patterns. Additionally, verify that the BFS callback in tilesInRange() correctly:

Includes the center tile (this.dst) with full weight (1)

Checks the outer radius boundary before writing to the cache to prevent including tiles just beyond magnitude.outer

Only weights tiles within the intended radius range

These boundary conditions are critical to ensure alliance-breaking calculations use only tiles genuinely within the nuke's destructive range.

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 16c18f3 and adbf136.

📒 Files selected for processing (1)

src/core/execution/NukeExecution.ts (2 hunks)

🧰 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:

src/core/execution/NukeExecution.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.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.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-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.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-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 specific UnitType enum values (AtomBomb, HydrogenBomb, MIRV, MIRVWarhead). This means that comparisons in switch statements between a NukeType parameter and UnitType enum values are valid and will work correctly at runtime.

Applied to files:

src/core/execution/NukeExecution.ts

📚 Learning: 2025-11-26T20:49:29.110Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.110Z
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/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

🧬 Code graph analysis (1)

src/core/execution/NukeExecution.ts (3)

src/core/game/GameMap.ts (2)

TileRef (3-3)

magnitude (167-169)

src/core/game/GameImpl.ts (1)

magnitude (840-842)

src/core/PseudoRandom.ts (1)

PseudoRandom (3-76)

🔇 Additional comments (3)

src/core/execution/NukeExecution.ts (3)

70-73: Verify: BFS stops when rand.chance(2) fails, possibly skipping tiles behind.

When the random chance fails for an outer-ring tile, BFS stops expanding from that tile. This could create "shadows" where tiles further out are never visited, making destruction patterns depend on BFS traversal order.

This appears to be existing behavior, not introduced by this PR. Just flagging for awareness in case the intent is uniform random destruction across the entire outer ring.

76-117: LGTM! Clean refactor to weighted alliance-breaking logic.

The weight accumulation (line 89) and threshold comparison (line 96) are correct. The typo fix ("missile") is a nice touch.

138-138: Good change - alliance breaking is now deterministic at launch.

Moving the alliance check to use tilesInRange() at spawn time makes the decision deterministic and independent of which tiles are randomly selected for destruction later. This matches the PR objective.

src/core/execution/NukeExecution.ts

thx rabbit

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/core/execution/NukeExecution.ts (1)
79-120: Alliance-breaking logic matches intent; consider skipping work for MIRV warheads

The new maybeBreakAlliances(inRange: Map<TileRef, number>) correctly:

Aggregates per-owner weighted impact (1 inner, 0.5 outer), which lines up with the 100% vs ~50% destruction odds.

Uses a single, deterministic snapshot (tilesInRange()) taken at launch.

Keeps MIRV warheads exempt via the this.nuke.type() !== UnitType.MIRVWarhead guard.

If you want a tiny optimization and slightly clearer intent, you can avoid running tilesInRange() and the alliance logic at all for MIRV warheads by guarding at the callsite in tick:
-      this.nuke = this.player.buildUnit(this.nukeType, spawn, {
+      this.nuke = this.player.buildUnit(this.nukeType, spawn, {
         targetTile: this.dst,
         trajectory: this.getTrajectory(this.dst),
       });
-      this.maybeBreakAlliances(this.tilesInRange());
+      if (this.nuke.type() !== UnitType.MIRVWarhead) {
+        this.maybeBreakAlliances(this.tilesInRange());
+      }
This keeps behavior identical while avoiding an extra bfs + map build for warheads that can never break alliances.

Also applies to: 141-141

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between adbf136 and 8be2e79.

📒 Files selected for processing (1)

src/core/execution/NukeExecution.ts (2 hunks)

🧰 Additional context used

🧠 Learnings (15)

📓 Common learnings

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.

📚 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.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.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.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-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.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-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 specific UnitType enum values (AtomBomb, HydrogenBomb, MIRV, MIRVWarhead). This means that comparisons in switch statements between a NukeType parameter and UnitType enum values are valid and will work correctly at runtime.

Applied to files:

src/core/execution/NukeExecution.ts

📚 Learning: 2025-11-29T22:22:37.170Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2539
File: src/core/game/GameImpl.ts:520-542
Timestamp: 2025-11-29T22:22:37.170Z
Learning: In GameImpl.ts, neighborsWithDiag() and forEachNeighborWithDiag() intentionally duplicate coordinate iteration logic. They serve different purposes: forEachNeighborWithDiag() is a zero-allocation hot-path optimization while neighborsWithDiag() is a convenience method that returns an array. Refactoring one to call the other would add callback/closure allocations and indirection overhead, defeating the performance goals.

Applied to files:

src/core/execution/NukeExecution.ts

📚 Learning: 2025-11-26T22:27:31.808Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.808Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>

Applied to files:

src/core/execution/NukeExecution.ts

📚 Learning: 2025-08-23T07:48:19.060Z

Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.

Applied to files:

src/core/execution/NukeExecution.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/execution/NukeExecution.ts

📚 Learning: 2025-11-26T20:49:29.110Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.110Z
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/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

🧬 Code graph analysis (1)

src/core/execution/NukeExecution.ts (3)

src/core/game/GameMap.ts (2)

TileRef (3-3)

magnitude (167-169)

src/core/game/GameImpl.ts (1)

magnitude (840-842)

src/core/PseudoRandom.ts (1)

PseudoRandom (3-76)

🔇 Additional comments (2)

src/core/execution/NukeExecution.ts (2)

46-63: tilesInRange BFS and weighting look correct and deterministic

The bfs-based scan with inner2 / outer2 and weights 1 vs 0.5 is clean, and the outer-radius check in the callback prevents tiles outside the blast radius from entering tilesInRange. This gives a deterministic, range-based view that you can safely reuse for alliance logic without touching the random destruction path.

65-77: Removing the tilesToDestroy cache keeps explosion randomness isolated to detonation

Having tilesToDestroy() compute its set directly (no cache) while seeding PseudoRandom with mg.ticks() keeps the actual destruction pattern tied to the detonation tick only, and fully decouples it from alliance-breaking. The bfs predicate is unchanged, so behavior of the blast itself stays the same.

bibizu · 2025-12-03T13:36:57Z

@coderabbitai review

coderabbitai · 2025-12-03T13:37:18Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

src/core/execution/NukeExecution.ts

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/core/execution/NukeExecution.ts (1)
106-108: Redundant optional chaining.

allianceRequest is already confirmed non-null by the if condition on line 106, so ?. is unnecessary.
         if (allianceRequest) {
-          allianceRequest?.reject();
+          allianceRequest.reject();
         }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 81b9c54 and 6d38bf2.

📒 Files selected for processing (4)

src/core/execution/NukeExecution.ts (2 hunks)
src/core/game/GameImpl.ts (1 hunks)
src/core/game/GameMap.ts (2 hunks)
src/core/game/GameView.ts (1 hunks)

🧰 Additional context used

🧠 Learnings (16)

📓 Common learnings

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.844Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2539
File: src/core/game/GameImpl.ts:520-542
Timestamp: 2025-11-29T22:22:37.178Z
Learning: In GameImpl.ts, neighborsWithDiag() and forEachNeighborWithDiag() intentionally duplicate coordinate iteration logic. They serve different purposes: forEachNeighborWithDiag() is a zero-allocation hot-path optimization while neighborsWithDiag() is a convenience method that returns an array. Refactoring one to call the other would add callback/closure allocations and indirection overhead, defeating the performance goals.

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.

📚 Learning: 2025-11-29T22:22:37.178Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2539
File: src/core/game/GameImpl.ts:520-542
Timestamp: 2025-11-29T22:22:37.178Z
Learning: In GameImpl.ts, neighborsWithDiag() and forEachNeighborWithDiag() intentionally duplicate coordinate iteration logic. They serve different purposes: forEachNeighborWithDiag() is a zero-allocation hot-path optimization while neighborsWithDiag() is a convenience method that returns an array. Refactoring one to call the other would add callback/closure allocations and indirection overhead, defeating the performance goals.

Applied to files:

src/core/game/GameMap.ts
src/core/game/GameImpl.ts
src/core/game/GameView.ts
src/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:

src/core/execution/NukeExecution.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.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.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-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-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.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 specific UnitType enum values (AtomBomb, HydrogenBomb, MIRV, MIRVWarhead). This means that comparisons in switch statements between a NukeType parameter and UnitType enum values are valid and will work correctly at runtime.

Applied to files:

src/core/execution/NukeExecution.ts

📚 Learning: 2025-11-26T22:27:31.844Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.844Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>

Applied to files:

src/core/execution/NukeExecution.ts

📚 Learning: 2025-08-23T07:48:19.060Z

Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.

Applied to files:

src/core/execution/NukeExecution.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/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/execution/NukeExecution.ts

📚 Learning: 2025-10-17T19:16:57.699Z

Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:711-723
Timestamp: 2025-10-17T19:16:57.699Z
Learning: In the OpenFrontIO game, MIRVs (Multiple Independently targetable Reentry Vehicles) have an area of effect with a radius of 1500 tiles around the aim point. To maximize efficiency, bots should aim at the center of the target player's territory rather than selecting random tiles. This differs from the initial understanding that MIRVs covered an entire player's territory regardless of aim point.

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

🧬 Code graph analysis (2)

src/core/game/GameImpl.ts (1)

src/core/game/GameMap.ts (1)

TileRef (3-3)

src/core/execution/NukeExecution.ts (3)

src/core/game/GameMap.ts (2)

TileRef (3-3)

magnitude (172-174)

src/core/game/GameImpl.ts (2)

magnitude (840-842)

owner (183-185)

src/core/execution/PlayerExecution.ts (1)

owner (291-296)

🪛 GitHub Actions: 🧪 CI

src/core/game/GameMap.ts

[error] 118-118: Invalid coordinates: -9,-9

[error] 118-118: Invalid coordinates: 189,200

[error] 118-118: Invalid coordinates: 200,0

[error] 118-118: Invalid coordinates: -1,0

[error] 118-118: Invalid coordinates: -1,14

🔇 Additional comments (7)

src/core/game/GameMap.ts (1)

42-46: Interface declaration looks good.

The method signature is clear and consistent with other search methods in this interface.

src/core/game/GameImpl.ts (1)

885-891: Clean delegation, consistent with existing patterns.

The method correctly delegates to _map.circleSearch following the same pattern as other map methods in this class.

src/core/game/GameView.ts (1)

765-771: Clean delegation, consistent with existing patterns.

The method correctly delegates to _map.circleSearch, matching the approach used for other map methods in this class.

src/core/execution/NukeExecution.ts (4)

46-62: Clean weighted tile computation using circleSearch.

This method correctly uses circleSearch (which already constrains to the outer radius) instead of BFS. The weighting (1.0 inner, 0.5 outer) matches the expected destruction probability from tilesToDestroy(). Good pattern for pre-computing alliance impact at launch time.

64-76: Deterministic destruction via seeded random.

Using PseudoRandom(this.mg.ticks()) ensures reproducible tile destruction. The 50% chance for outer tiles aligns well with the 0.5 weight in tilesInRange().

78-98: Good weighted aggregation for alliance decisions.

The weighted sum approach makes alliance breaking decisions deterministic at launch time. The doc comment clearly explains the weighting purpose. This is a cleaner design than checking after random tile selection.

140-142: Alliance breaking at launch time - key fix.

Moving maybeBreakAlliances() to spawn time with tilesInRange() (instead of at detonation with tilesToDestroy()) makes the decision deterministic before any random tile selection occurs. The MIRVWarhead check here avoids computing tilesInRange() unnecessarily.

src/core/game/GameMap.ts

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/core/execution/NukeExecution.ts (2)

47-63: tilesInRange implementation matches blast model; consider caching only if reused

The circleSearch + inner/outer weighting looks correct and lines up with the destruction odds (inner=1, outer≈0.5). Given it’s only called once at spawn right now, recomputing the map is fine; if this helper ever gets called from multiple places later, you might mirror tilesToDestroyCache with a small tilesInRangeCache to avoid duplicate scans.

83-124: Weighted alliance break logic is consistent; redundant MIRV check could be hoisted

The per-owner weight accumulation over inRange is clear and matches the new “expected destroyed tiles” semantics against nukeAllianceBreakThreshold(). One tiny clean‑up: the this.nuke.type() !== UnitType.MIRVWarhead guard inside the loop is redundant with the caller’s check at spawn; you could early‑return at the top of this method for MIRVWarhead to make intent explicit and avoid repeating the check per player.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4f2740a and 6375627.

📒 Files selected for processing (1)

src/core/execution/NukeExecution.ts (3 hunks)

🧰 Additional context used

🧠 Learnings (15)

📚 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.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.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.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-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-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.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 specific UnitType enum values (AtomBomb, HydrogenBomb, MIRV, MIRVWarhead). This means that comparisons in switch statements between a NukeType parameter and UnitType enum values are valid and will work correctly at runtime.

Applied to files:

src/core/execution/NukeExecution.ts

📚 Learning: 2025-11-29T22:22:37.178Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2539
File: src/core/game/GameImpl.ts:520-542
Timestamp: 2025-11-29T22:22:37.178Z
Learning: In GameImpl.ts, neighborsWithDiag() and forEachNeighborWithDiag() intentionally duplicate coordinate iteration logic. They serve different purposes: forEachNeighborWithDiag() is a zero-allocation hot-path optimization while neighborsWithDiag() is a convenience method that returns an array. Refactoring one to call the other would add callback/closure allocations and indirection overhead, defeating the performance goals.

Applied to files:

src/core/execution/NukeExecution.ts

📚 Learning: 2025-11-26T22:27:31.844Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.844Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>

Applied to files:

src/core/execution/NukeExecution.ts

📚 Learning: 2025-08-23T07:48:19.060Z

Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.

Applied to files:

src/core/execution/NukeExecution.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/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/execution/NukeExecution.ts

📚 Learning: 2025-10-17T19:16:57.699Z

Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:711-723
Timestamp: 2025-10-17T19:16:57.699Z
Learning: In the OpenFrontIO game, MIRVs (Multiple Independently targetable Reentry Vehicles) have an area of effect with a radius of 1500 tiles around the aim point. To maximize efficiency, bots should aim at the center of the target player's territory rather than selecting random tiles. This differs from the initial understanding that MIRVs covered an entire player's territory regardless of aim point.

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

🧬 Code graph analysis (1)

src/core/execution/NukeExecution.ts (1)

src/core/game/GameMap.ts (2)

TileRef (3-3)

magnitude (172-174)

🔇 Additional comments (1)

src/core/execution/NukeExecution.ts (1)

145-147: Alliance break now happens at launch, even if the nuke is later intercepted

Because maybeBreakAlliances(this.tilesInRange()) is now only called here and no longer from detonate(), alliances will be broken (and relations adjusted) at launch time based on expected impact, even if the nuke is intercepted before any tiles are actually destroyed. This aligns with the PR description but is a behavior change from “on detonation” to “on launch”, so just double‑check this is the intended game rule.

bibizu · 2025-12-10T16:23:14Z

@evanpelle Would you be able to re-review this, as it will help for other nuke-related issues?

bibizu added 2 commits December 2, 2025 23:56

fix: Alliance breakage is deterministic now

a579c6e

And only breaks alliances once, on launch.

style: fix misleading doc comment

16c18f3

bibizu requested a review from a team as a code owner December 3, 2025 05:01

github-actions bot assigned bibizu Dec 3, 2025

style: remove tile caches

4129b54

each check is only called once now

coderabbitai bot reviewed Dec 3, 2025

View reviewed changes

coderabbitai bot previously approved these changes Dec 3, 2025

View reviewed changes

style: rename to reflect weight change

adbf136

bibizu dismissed coderabbitai[bot]’s stale review via adbf136 December 3, 2025 05:10

coderabbitai bot requested changes Dec 3, 2025

View reviewed changes

src/core/execution/NukeExecution.ts Outdated Show resolved Hide resolved

fix: add tiles into range after check

8be2e79

thx rabbit

coderabbitai bot reviewed Dec 3, 2025

View reviewed changes

coderabbitai bot previously approved these changes Dec 3, 2025

View reviewed changes

perf: exclude extra bfs search if mirv

81b9c54

bibizu dismissed coderabbitai[bot]’s stale review via 81b9c54 December 3, 2025 05:29

Lavodan approved these changes Dec 3, 2025

View reviewed changes

coderabbitai bot previously approved these changes Dec 3, 2025

View reviewed changes

evanpelle reviewed Dec 3, 2025

View reviewed changes

src/core/execution/NukeExecution.ts Outdated Show resolved Hide resolved

perf: Circle search algorithm to replace bfs

6d38bf2

bibizu dismissed coderabbitai[bot]’s stale review via 6d38bf2 December 4, 2025 05:09

bibizu requested a review from evanpelle December 4, 2025 05:09

coderabbitai bot requested changes Dec 4, 2025

View reviewed changes

src/core/game/GameMap.ts Show resolved Hide resolved

bibizu added 2 commits December 4, 2025 00:17

fix: oops edge of map crash

50a0598

style: rabbit nitpick

4f2740a

coderabbitai bot previously approved these changes Dec 4, 2025

View reviewed changes

fix: re-add toDestroy cache

6375627

bibizu dismissed coderabbitai[bot]’s stale review via 6375627 December 4, 2025 17:58

coderabbitai bot reviewed Dec 4, 2025

View reviewed changes

coderabbitai bot approved these changes Dec 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Check alliance breakage once at launch, and is deterministic now #2554

fix: Check alliance breakage once at launch, and is deterministic now #2554

bibizu commented Dec 3, 2025

Uh oh!

coderabbitai bot commented Dec 3, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

bibizu commented Dec 3, 2025

Uh oh!

coderabbitai bot commented Dec 3, 2025

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

bibizu commented Dec 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix: Check alliance breakage once at launch, and is deterministic now #2554

Are you sure you want to change the base?

fix: Check alliance breakage once at launch, and is deterministic now #2554

Conversation

bibizu commented Dec 3, 2025

Description:

Please complete the following:

Please put your Discord username so you can be contacted if a bug or regression is found:

Uh oh!

coderabbitai bot commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Poem

Pre-merge checks

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

bibizu commented Dec 3, 2025

Uh oh!

coderabbitai bot commented Dec 3, 2025

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

bibizu commented Dec 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai bot commented Dec 3, 2025 •

edited

Loading