Fix/refactor: centralized removals on death + remove alliances too

[VariableVince]

Description:

• Centralize cleanup on death: Do the same for Bots as was already done for Nations and human Players on death, that is deleting remaining non-nuke units and gold. (by the way, seen from NationExecution it wasn't all too clear that a PlayerExecution is also running for Nations and when that becomes inActive it is what deletes eg. Nation warships on death. Added comment in NationExecution clarifying that it happens from PlayerExecution, if that's ok). Bots can conquer units even when they can't build them. There may also be edge cases in which Bots have gold left at death. So it seems better to run the same centralized cleanup for them too.

• Remove alliances on death too from the centralized function: after death, alliances would stay active including countdown timers and (when dead player kept spectating) icons. Now remove alliances from the central function.

• Remove renewal request from Events Display when Alliance doesn't exist anymore (after death or otherwise).

• Also cleanup this.alliancesCheckedAt when alliance doesn't exist anymore. Before, old/broken alliance id's would accumulate in it during a game.

• Removed now-redundant isAlive check in EventsDisplay. Both the alliances array as the isAlive are updated in the same tick from PlayerUpdates so now alliance is removed from alliances array on player death, the other.isAlive() check is no longer needed. Of course we could keep it in just to be very safe, so just let me know when you're doubtful about this.

• Attack.test.ts: fix failing test. Player B dies because of the attack, meaning the alliance now gets removed. Prevent this by gving both a different, adjecent, starting tile. And to be more clear about what is needed for the test to pass, add isAlive check for both of them after the attacks.

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:

tryout33

[coderabbitai]

Walkthrough

Centralized death cleanup was added via a new Game.removeOnDeath(deadPlayer) method. Execution modules (PlayerExecution, BotExecution) now call this method instead of inlining removal logic. EventsDisplay now tracks current alliance IDs per tick and removes stale alliance entries; alliance-alive guard was removed.

Changes

Cohort / File(s)	Summary
Game Core Interface & Implementation src/core/game/Game.ts, src/core/game/GameImpl.ts	Added removeOnDeath(deadPlayer: Player): void to the Game API and implemented it: removes player's gold, deletes non-bomb units, and prunes alliances involving the dead player.
Execution Cleanup Refactoring src/core/execution/PlayerExecution.ts, src/core/execution/BotExecution.ts, src/core/execution/NationExecution.ts	Replaced inline death cleanup with calls to mg.removeOnDeath(...) in PlayerExecution and BotExecution; NationExecution received a comment (no behavioral change).
Alliance Event Tracking src/client/graphics/layers/EventsDisplay.ts	Introduced currentAllianceIds set per tick and post-processing loop to remove stale alliancesCheckedAt entries; removed the !other.isAlive() guard and added cleanup on broken-alliance events.
Tests tests/Attack.test.ts	Adjusted Player B spawn tile from (0,10) to (0,11) and updated assertions to check both players remain alive and alliance state after the attack sequence.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

⚔️ A player falls, the game tidies the scene,
One call sweeps gold and units clean,
Alliances fade where ghosts once stood,
Ticks march on, the board renewed.
Small code, calm end — tidy and keen.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check	✅ Passed	The PR description clearly outlines centralized cleanup on death, alliance removal, and related fixes with specific rationale for each change.
Title check	✅ Passed	The title clearly identifies the main changes: centralizing player death cleanup and removing alliances, which matches the primary objectives of the pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/client/graphics/layers/EventsDisplay.ts`:
- Line 645: The handler for alliance expiration currently returns early only
when other is missing or myPlayer is dead; add the missing alive-check for the
other player to avoid showing "alliance expired" for players who died in the
same tick — i.e., update the condition in EventsDisplay (the
AllianceExpired/removeOnDeath handler) from "if (!other || !myPlayer.isAlive())
return;" to also check other.isAlive() so it reads effectively as "if (!other ||
!myPlayer.isAlive() || !other.isAlive()) return;". Ensure the change is applied
where the AllianceExpired/PlayerExecution expiry handling logic references other
and myPlayer.

🧹 Nitpick comments (1)

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

1166-1175: Replace repeated nuke type checks with the nukeTypes constant.

The same four nuke types are already defined as nukeTypes in Game.ts (line 343-348) and are exported. Hardcoding them here creates duplicate logic that must be maintained in multiple places—if a new nuke type is added, you'll need to update both locations.

Import nukeTypes from Game.ts and use it instead:

Suggested change

import {
  Alliance,
  AllianceRequest,
  Cell,
  ColoredTeams,
  Duos,
  EmojiMessage,
  Execution,
  Game,
  GameMode,
  GameUpdates,
  HumansVsNations,
  MessageType,
  MutableAlliance,
  Nation,
  Player,
  PlayerID,
  PlayerInfo,
  PlayerType,
  Quads,
  Team,
  TerrainType,
  TerraNullius,
  Trios,
  Unit,
  UnitInfo,
  UnitType,
+ nukeTypes,
} from "./Game";

     deadPlayer.units().forEach((u) => {
-      if (
-        u.type() !== UnitType.AtomBomb &&
-        u.type() !== UnitType.HydrogenBomb &&
-        u.type() !== UnitType.MIRVWarhead &&
-        u.type() !== UnitType.MIRV
-      ) {
+      if (!nukeTypes.includes(u.type())) {
         u.delete();
       }
     });

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

tests/Attack.test.ts (1)

296-303: ⚠️ Potential issue | 🟡 Minor

playerC should use unique name and ID, not duplicate playerB's.

In addPlayer() (GameImpl.ts:1442-1454), players are stored in a Map keyed by playerInfo.id. When playerC is created with ID "playerB_id", it overwrites the playerB entry in that Map. The test currently works only because it holds direct object references to playerB, but any future code calling game.player("playerB_id") would incorrectly retrieve playerC instead of playerB.

Proposed fix

     const playerCInfo = new PlayerInfo(
-      "playerB",
+      "playerC",
       PlayerType.Human,
       null,
-      "playerB_id",
+      "playerC_id",
     );