Skip to content

Conversation

@RjManhas
Copy link
Contributor

@RjManhas RjManhas commented Nov 9, 2025

Description:

Describe the PR.

To help with game balancing and to help with ensuring their isn't a "stalement" and to help prevent games to go for hour + sometimes when theirs a situlation when its a 1v1v1 and everyone has mrivs, this PR suggestions to block new alliances after 40 mins to help encourge activity.

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:

DISCORD_USERNAME

notifxy (1379678982676676639)

@RjManhas RjManhas requested a review from a team as a code owner November 9, 2025 01:05
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 9, 2025

Walkthrough

Adds a 40-minute (24,000 ticks) time gate that blocks new alliance requests and renewals by short-circuiting AllianceExtensionExecution.init and PlayerImpl.canSendAllianceRequest; corresponding constant and tests added.

Changes

Cohort / File(s) Summary
Alliance time gates
src/core/execution/alliance/AllianceExtensionExecution.ts, src/core/game/PlayerImpl.ts
Add an early tick-based guard using ALLIANCE_BLOCK_TICKS to prevent processing alliance renewals and sending alliance requests once game ticks reach the threshold.
Timing constant
src/core/game/constants.ts
Add export const ALLIANCE_BLOCK_TICKS = 40 * 60 * 10 (24,000 ticks) with comments explaining conversion.
Tests for time gate
tests/AllianceExtensionExecution.test.ts, tests/AllianceRequestExecution.test.ts, tests/PlayerImpl.test.ts
Add tests that advance game ticks to the threshold and assert that alliance renewals and new alliance requests are blocked after the cutoff.

Sequence Diagram(s)

sequenceDiagram
  participant Player as Player
  participant MG as Game (mg)
  participant Ext as AllianceExtensionExecution
  rect `#E8F5E9`
    note left of Ext: New/changed path
    Player->>Ext: requestAlliance() / renewAlliance()
    Ext->>MG: mg.ticks()
    alt ticks < ALLIANCE_BLOCK_TICKS
      Ext->>Ext: continue processing (existing validations/workflow)
      Ext->>Player: proceed (request created / renewal displayed)
    else ticks >= ALLIANCE_BLOCK_TICKS
      Ext-->>Player: early return (no request / no renewal)
    end
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Review focus: correctness of tick comparison, import placement, and ensuring tests properly advance the simulated clock.
  • Files to pay special attention to:
    • AllianceExtensionExecution.ts — confirm early return doesn't skip required cleanup or side-effects.
    • PlayerImpl.ts — ensure canSendAllianceRequest still enforces existing checks after the new guard.
    • Tests — confirm they reliably advance ticks and assert negative paths.

Possibly related PRs

Suggested labels

Balance Tweak, Feature - Test, Feature - New

Suggested reviewers

  • evanpelle
  • scottanderson

Poem

⏱️ Forty minutes pass the gate,
Requests and renewals meet their fate.
Ticks march on, the bell is rung,
New pacts paused, the rule is sprung.
Quiet now — alliances wait 🛡️

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and accurately summarizes the main change: disabling alliance creation after 40 minutes of game time.
Description check ✅ Passed The description explains the feature's purpose for game balancing and includes all required checklist items, directly relating to the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d10c6ad and b354687.

📒 Files selected for processing (6)
  • src/core/execution/alliance/AllianceExtensionExecution.ts (2 hunks)
  • src/core/game/PlayerImpl.ts (2 hunks)
  • src/core/game/constants.ts (1 hunks)
  • tests/AllianceExtensionExecution.test.ts (2 hunks)
  • tests/AllianceRequestExecution.test.ts (2 hunks)
  • tests/PlayerImpl.test.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/core/game/PlayerImpl.ts
🧰 Additional context used
🧠 Learnings (12)
📓 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/game/constants.ts
  • tests/AllianceRequestExecution.test.ts
  • src/core/execution/alliance/AllianceExtensionExecution.ts
  • tests/PlayerImpl.test.ts
  • tests/AllianceExtensionExecution.test.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/AllianceRequestExecution.test.ts
  • tests/PlayerImpl.test.ts
  • tests/AllianceExtensionExecution.test.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.

Applied to files:

  • tests/AllianceRequestExecution.test.ts
  • tests/PlayerImpl.test.ts
  • tests/AllianceExtensionExecution.test.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.

Applied to files:

  • tests/PlayerImpl.test.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.

Applied to files:

  • tests/PlayerImpl.test.ts
  • tests/AllianceExtensionExecution.test.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.

Applied to files:

  • tests/AllianceExtensionExecution.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:

  • tests/AllianceExtensionExecution.test.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:

  • tests/AllianceExtensionExecution.test.ts
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.

Applied to files:

  • tests/AllianceExtensionExecution.test.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.

Applied to files:

  • tests/AllianceExtensionExecution.test.ts
📚 Learning: 2025-06-22T05:48:19.241Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.

Applied to files:

  • tests/AllianceExtensionExecution.test.ts
🧬 Code graph analysis (4)
tests/AllianceRequestExecution.test.ts (2)
src/core/game/constants.ts (1)
  • ALLIANCE_BLOCK_TICKS (4-4)
src/core/execution/alliance/AllianceRequestExecution.ts (1)
  • AllianceRequestExecution (9-72)
src/core/execution/alliance/AllianceExtensionExecution.ts (1)
src/core/game/constants.ts (1)
  • ALLIANCE_BLOCK_TICKS (4-4)
tests/PlayerImpl.test.ts (1)
src/core/game/constants.ts (1)
  • ALLIANCE_BLOCK_TICKS (4-4)
tests/AllianceExtensionExecution.test.ts (4)
src/core/execution/alliance/AllianceRequestExecution.ts (1)
  • AllianceRequestExecution (9-72)
src/core/execution/alliance/AllianceRequestReplyExecution.ts (1)
  • AllianceRequestReplyExecution (3-56)
src/core/game/constants.ts (1)
  • ALLIANCE_BLOCK_TICKS (4-4)
src/core/execution/alliance/AllianceExtensionExecution.ts (1)
  • AllianceExtensionExecution (10-91)
🔇 Additional comments (5)
src/core/execution/alliance/AllianceExtensionExecution.ts (1)

8-19: LGTM! Clean implementation of the 40-minute alliance block.

The early guard correctly prevents alliance renewal processing once the time threshold is reached. Using the centralized ALLIANCE_BLOCK_TICKS constant ensures consistency across the codebase.

tests/PlayerImpl.test.ts (1)

8-8: LGTM! Test correctly validates the 40-minute alliance block.

The test properly imports the constant and verifies that alliance requests are blocked after the threshold is reached.

Also applies to: 96-108

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

1-4: LGTM! Well-documented constant with clear calculation.

The comments explain the conversion from minutes to ticks, making the magic number transparent. This single source of truth resolves the constant duplication issue flagged in earlier reviews.

tests/AllianceRequestExecution.test.ts (1)

4-4: LGTM! Comprehensive test for blocking new alliance requests.

The test correctly advances to the 40-minute threshold and verifies that no alliance request is created, no alliance is formed, and neither player becomes allied. Good coverage of the blocking behavior.

Also applies to: 105-127

tests/AllianceExtensionExecution.test.ts (1)

5-5: LGTM! Test correctly validates that renewal messages are blocked.

The test establishes an alliance, advances to the 40-minute threshold, and uses a spy to verify that no renewal message is sent when attempting an extension. Clean test design.

Also applies to: 170-211


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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6a78494 and d10c6ad.

📒 Files selected for processing (5)
  • src/core/execution/alliance/AllianceExtensionExecution.ts (1 hunks)
  • src/core/game/PlayerImpl.ts (1 hunks)
  • tests/AllianceExtensionExecution.test.ts (1 hunks)
  • tests/AllianceRequestExecution.test.ts (1 hunks)
  • tests/PlayerImpl.test.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/AllianceRequestExecution.test.ts
  • tests/AllianceExtensionExecution.test.ts
  • src/core/execution/alliance/AllianceExtensionExecution.ts
  • src/core/game/PlayerImpl.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/alliance/AllianceExtensionExecution.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/game/PlayerImpl.ts
🧬 Code graph analysis (2)
tests/AllianceRequestExecution.test.ts (1)
src/core/execution/alliance/AllianceRequestExecution.ts (1)
  • AllianceRequestExecution (9-72)
tests/AllianceExtensionExecution.test.ts (3)
src/core/execution/alliance/AllianceRequestExecution.ts (1)
  • AllianceRequestExecution (9-72)
src/core/execution/alliance/AllianceRequestReplyExecution.ts (1)
  • AllianceRequestReplyExecution (3-56)
src/core/execution/alliance/AllianceExtensionExecution.ts (1)
  • AllianceExtensionExecution (9-93)
🔇 Additional comments (1)
src/core/execution/alliance/AllianceExtensionExecution.ts (1)

16-21: Extract the duplicated constant to a shared configuration.

The ALLIANCE_BLOCK_TICKS constant is defined in multiple files (this file, src/core/game/PlayerImpl.ts, and three test files). This code duplication violates the DRY principle and creates maintenance risk, especially since game balance constants may change frequently during testing.

Consider extracting this to a shared location such as Game.ts or a dedicated constants/config file:

// In a shared config file or Game.ts
export const ALLIANCE_BLOCK_TICKS = 40 * 60 * 10; // 24,000 ticks (40 minutes)

Then import and use it across all files:

+import { ALLIANCE_BLOCK_TICKS } from "../../game/Game";
+
 export class AllianceExtensionExecution implements Execution {
   constructor(
     private readonly from: Player,
     private readonly toID: PlayerID,
   ) {}

   init(mg: Game, ticks: number): void {
     // Block alliance renewals after 40 minutes of game time
-    // 40 minutes = 2400 seconds = 24,000 ticks (10 ticks per second)
-    const ALLIANCE_BLOCK_TICKS = 40 * 60 * 10; // 24,000 ticks
     if (mg.ticks() >= ALLIANCE_BLOCK_TICKS) {
       return;
     }
⛔ Skipped due to learnings
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.

@RjManhas
Copy link
Contributor Author

RjManhas commented Nov 9, 2025

I dont mind if this is denied due to it being a game meta change, but i talked to people, some hate it, some like it, so it took 5 mins to make, so i thought ill offer it as a idea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant