Configurable immunity timer #2763
Conversation
WalkthroughAdds a configurable PVP spawn-immunity feature: lobby UI toggle and numeric duration, game-config/schema/default, server handling, game-state checks (isSpawnImmunityActive, player isImmune/canAttackPlayer), execution changes to block actions during immunity, renderer layer and DOM element for an immunity progress bar, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/UI
participant GameRenderer as Renderer Layer
participant Game as GameImpl
participant AttExec as AttackExecution
participant Player as PlayerImpl
participant Server as GameServer
rect rgb(255,245,235)
note over Client,Game: During spawn-immunity period
Client->>GameRenderer: render immunty bar (reads config & ticks)
Client->>AttExec: request attack init(attacker, target)
AttExec->>Player: canAttackPlayer(attacker)
Player->>Game: isImmune()
Game-->>Player: true (isSpawnImmunityActive)
Player-->>AttExec: false (cannot attack)
AttExec->>AttExec: deactivate / block attack
end
rect rgb(235,255,245)
note over Client,Game: After immunity expires
Client->>GameRenderer: hide immunity bar
Client->>AttExec: request attack init(attacker, target)
AttExec->>Player: canAttackPlayer(attacker)
Player->>Game: isImmune()
Game-->>Player: false (immunity ended)
Player-->>AttExec: true (can attack)
AttExec->>AttExec: proceed with attack
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 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)
src/core/execution/TransportShipExecution.ts (1)
96-99: Consider adding user feedback when transport ships are blocked.The immunity check correctly prevents transport ships from targeting immune players. However, unlike the boat limit check (lines 79-85), this fails silently without user feedback. Consider displaying a message to inform the user why their transport ship couldn't be sent.
🔎 Suggested improvement
if (this.target.isPlayer() && !this.attacker.canAttackPlayer(this.target)) { + mg.displayMessage( + `Cannot send naval invasion, ${this.target.displayName()} is immune`, + MessageType.ATTACK_FAILED, + this.attacker.id(), + ); this.active = false; return; }tests/Attack.test.ts (1)
464-472: Variable name is misleading.The variable is named
nationbut it holds aPlayerType.Bot. This could confuse future readers.Proposed fix
test("Should be able to attack bots during immunity phase", async () => { - const nation = new PlayerInfo("bot dude", PlayerType.Bot, null, "bot_id"); - game.addPlayer(nation); - // Player A attacks the nation + const bot = new PlayerInfo("bot dude", PlayerType.Bot, null, "bot_id"); + game.addPlayer(bot); + // Player A attacks the bot const attackExecution = new AttackExecution(null, playerA, "bot_id", null); game.addExecution(attackExecution); game.executeNextTick(); expect(playerA.outgoingAttacks()).toHaveLength(1); });src/client/graphics/layers/ImmunityTimer.ts (2)
7-9: Public property lacks initialization.The
gameproperty is declared without initialization. Iftick()is called beforegameis assigned, the guardif (!this.game)prevents crashes, but TypeScript's strict mode would flag this. Consider using a definite assignment assertion or making it optional.Proposed fix
@customElement("immunity-timer") export class ImmunityTimer extends LitElement implements Layer { - public game: GameView; + public game!: GameView;
44-47: Magic number duplicates default config value.The value
5 * 10(50 ticks = 5 seconds) appears to match the default immunity duration fromDefaultConfig.ts. Consider extracting this to a shared constant or adding a comment explaining the value.+ // Hide timer for default/disabled immunity (5 seconds = 50 ticks) + const DEFAULT_IMMUNITY_TICKS = 5 * 10; - if (immunityDuration <= 5 * 10 || this.game.inSpawnPhase()) { + if (immunityDuration <= DEFAULT_IMMUNITY_TICKS || this.game.inSpawnPhase()) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
index.htmlresources/lang/debug.jsonresources/lang/en.jsonsrc/client/HostLobbyModal.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/ImmunityTimer.tssrc/core/Schemas.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/AttackExecution.tssrc/core/execution/TransportShipExecution.tssrc/core/execution/WarshipExecution.tssrc/core/game/Game.tssrc/core/game/GameImpl.tssrc/core/game/PlayerImpl.tssrc/core/game/TransportShipUtils.tssrc/server/GameServer.tssrc/server/MapPlaylist.tstests/Attack.test.tstests/util/TestConfig.ts
🧰 Additional context used
🧠 Learnings (16)
📚 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.tssrc/server/GameServer.tssrc/core/Schemas.tssrc/core/game/PlayerImpl.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/core/game/Game.tssrc/core/game/TransportShipUtils.tssrc/core/execution/TransportShipExecution.tssrc/core/execution/WarshipExecution.tssrc/core/game/PlayerImpl.tssrc/core/execution/AttackExecution.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/TransportShipUtils.tstests/Attack.test.tssrc/core/execution/TransportShipExecution.tssrc/core/execution/WarshipExecution.tssrc/core/execution/AttackExecution.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/TransportShipUtils.tssrc/core/execution/TransportShipExecution.tssrc/core/execution/WarshipExecution.ts
📚 Learning: 2025-11-01T00:24:33.860Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2161
File: src/core/execution/FakeHumanExecution.ts:670-678
Timestamp: 2025-11-01T00:24:33.860Z
Learning: In OpenFrontIO, PlayerType.Bot entities cannot be in teams and do not have friendliness relationships. Only PlayerType.Human and PlayerType.FakeHuman can participate in teams and alliances. Therefore, when targeting bot-owned tiles, friendliness checks like `owner.isFriendly(this.player)` are unnecessary and meaningless for bots.
Applied to files:
src/core/game/TransportShipUtils.tssrc/core/execution/WarshipExecution.tssrc/core/game/PlayerImpl.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/configuration/DefaultConfig.tssrc/core/execution/TransportShipExecution.tstests/util/TestConfig.tssrc/client/HostLobbyModal.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/TransportShipExecution.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/TransportShipExecution.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/TransportShipExecution.tssrc/core/execution/AttackExecution.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as `z.infer<typeof PlayerStatsSchema>` where PlayerStatsSchema has `.optional()` applied at the object level, making PlayerStats a union type that already includes undefined (PlayerStats | undefined).
Applied to files:
src/core/Schemas.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as a type inferred from a Zod schema that is marked as optional, which means PlayerStats already includes undefined as a possible type (PlayerStats | undefined).
Applied to files:
src/core/Schemas.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/layers/ImmunityTimer.tssrc/client/graphics/GameRenderer.tsindex.htmlsrc/client/HostLobbyModal.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/ImmunityTimer.tssrc/client/graphics/GameRenderer.ts
📚 Learning: 2025-12-29T23:33:17.920Z
Learnt from: wraith4081
Repo: openfrontio/OpenFrontIO PR: 2735
File: index.html:390-391
Timestamp: 2025-12-29T23:33:17.920Z
Learning: In Tailwind CSS v4, update blur-related utilities in HTML templates. The mappings are: backdrop-blur-sm (v3) -> backdrop-blur-xs (v4); backdrop-blur (bare) -> backdrop-blur-sm; blur-sm -> blur-xs. Apply these changes to all HTML files (e.g., index.html and other templates) to reflect the v4 naming. Consider updating a project-wide search/replace or using a codemod to ensure consistency across the codebase.
Applied to files:
index.html
📚 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:
src/client/HostLobbyModal.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:
src/client/HostLobbyModal.ts
🧬 Code graph analysis (7)
src/core/game/Game.ts (3)
src/core/game/GameImpl.ts (1)
player(483-489)src/core/game/GameView.ts (1)
player(732-738)src/core/Schemas.ts (1)
Player(122-122)
tests/Attack.test.ts (3)
tests/util/TestConfig.ts (1)
TestConfig(12-88)src/core/game/Game.ts (1)
PlayerInfo(417-431)src/core/execution/AttackExecution.ts (1)
AttackExecution(20-389)
src/server/GameServer.ts (2)
src/core/configuration/DefaultConfig.ts (1)
gameConfig(251-253)src/server/MapPlaylist.ts (1)
gameConfig(87-115)
src/client/graphics/layers/ImmunityTimer.ts (2)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/game/GameView.ts (1)
GameView(584-917)
src/core/game/PlayerImpl.ts (2)
src/core/game/Game.ts (1)
Player(538-678)src/core/game/GameMap.ts (1)
TileRef(3-3)
tests/util/TestConfig.ts (1)
src/core/game/Game.ts (1)
Tick(23-23)
src/client/HostLobbyModal.ts (2)
src/client/Utils.ts (1)
translateText(92-147)src/client/LangSelector.ts (1)
translateText(258-278)
🔇 Additional comments (25)
resources/lang/debug.json (1)
148-148: LGTM!The debug translation key follows the established pattern and is correctly placed within the
host_modalsection.src/core/configuration/DefaultConfig.ts (1)
247-249: LGTM!The implementation follows the established pattern for config getters with sensible defaults. The nullish coalescing provides a clean fallback when the config value is undefined.
src/core/game/GameImpl.ts (1)
680-686: LGTM!The logic correctly determines if immunity is active by checking whether the current tick falls within the immunity window (spawn phase + immunity duration). The implementation is straightforward and follows the existing code patterns.
resources/lang/en.json (1)
307-307: LGTM!The translation text is clear and helpfully includes the unit (minutes) to guide users when configuring the immunity duration.
index.html (1)
454-454: LGTM!The immunity timer element is appropriately placed after the spawn timer, matching the intended rendering order. The web component will handle its own presentation and behavior.
src/server/GameServer.ts (1)
124-126: LGTM!The
spawnImmunityDurationhandling follows the same pattern as other config fields in this method. The implementation is consistent and correct.src/core/game/Game.ts (2)
659-660: LGTM!The new Player interface methods
canAttackPlayerandisImmuneare well-defined. The optionaltreatAFKFriendlyparameter is consistent with the existingisFriendlymethod signature.
715-717: LGTM!The
isSpawnImmunityActivemethod is clearly defined with an appropriate section comment. The placement in the interface is logical.src/core/game/TransportShipUtils.ts (1)
26-28: LGTM!The change from
isFriendlyto!canAttackPlayercorrectly generalizes the check. The logic now blocks transport ship creation for any player that can't be attacked, including immune players and friendly players. The negation is correct: if you can't attack them, return false to prevent the transport ship.src/server/MapPlaylist.ts (1)
112-112: Extract the spawn immunity default to a shared constant.The value
5 * 10is duplicated in multiple files (DefaultConfig.ts,MapPlaylist.ts,ImmunityTimer.ts). Define a constant likeDEFAULT_SPAWN_IMMUNITY_TICKS = 5 * 10in a shared location (e.g.,src/core/configuration/defaults.tsor similar) and use it consistently across the codebase. This prevents inconsistency if the default needs to change later.⛔ 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.Learnt from: MaxHT0x Repo: openfrontio/OpenFrontIO PR: 2262 File: src/core/configuration/DefaultConfig.ts:806-832 Timestamp: 2025-11-12T23:11:34.445Z Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.src/client/graphics/GameRenderer.ts (1)
21-21: LGTM! Clean integration of ImmunityTimer layer.The implementation follows the same pattern as SpawnTimer and other UI layers: DOM query, validation, property assignment, and layer registration. The ordering (after SpawnTimer) makes sense for visual stacking.
Also applies to: 238-244, 274-274
src/core/Schemas.ts (1)
178-178: LGTM! Schema addition is backward compatible.The optional
spawnImmunityDurationfield with non-negative integer validation is appropriate. The comment clarifies that the unit is ticks, which helps developers.tests/util/TestConfig.ts (1)
15-15: LGTM! Test configuration follows established patterns.The addition of configurable spawn immunity duration for tests follows the same pattern as other test config properties (e.g.,
_defaultNukeSpeed). The default value of 0 (no immunity) makes sense for tests that don't explicitly need immunity behavior.Also applies to: 58-64
src/core/execution/WarshipExecution.ts (1)
92-92: Warships intentionally have different AFK targeting rules—consider documenting why.The inconsistency is real:
WarshipExecutionpassestreatAFKFriendly=true, treating AFK players as friendly and blocking attacks. All other attacks (AttackExecution,TransportShipExecution,PlayerImpl) use the defaultfalse, allowing attacks on AFK players. ThecanAttackPlayer()method's second parameter controls this AFK-friendliness behavior.If this difference is intentional game design, add a code comment explaining why warships have special AFK targeting rules. If unintentional, align all calls to use the same parameter value.
tests/Attack.test.ts (3)
30-35: Clear helper for advancing game state past immunity.The helper is straightforward. One minor note: the
+ 1in the loop condition could use a brief comment explaining it advances one tick beyond the immunity period to ensure immunity has ended.
335-365: Well-structured immunity test suite.The beforeEach setup properly configures immunity duration and spawns players. Good use of the shared helper
addPlayerToGamefor consistency.
394-411: Good boundary test for immunity timing.This test verifies precise tick-based immunity boundaries - attacking fails at
immunityPhaseTicksbut succeeds atimmunityPhaseTicks + 1. This is valuable for catching off-by-one bugs.src/client/HostLobbyModal.ts (3)
48-49: State properties for spawn immunity feature.Clean addition of the two state properties needed for the toggle and duration input.
733-748: Input handlers follow existing patterns.The keydown and input handlers mirror the existing
handleMaxTimerValueKeyDownandhandleMaxTimerValueChangespatterns. The sanitization logic (replace(/[eE+-]/g, "")) properly prevents scientific notation and signs.
817-819: Spawn immunity tick calculation is correct.The formula
minutes * 60 * 10properly converts minutes to ticks (10 ticks per second). When disabled, the code sendsundefined, which matches the optional field in the schema.src/client/graphics/layers/ImmunityTimer.ts (2)
30-64: Tick logic correctly handles immunity phases.The progress calculation properly:
- Skips rendering during spawn phase
- Computes elapsed ticks relative to spawn phase end
- Clamps progress ratio between 0 and 1
- Adjusts vertical position based on team ownership bar visibility
77-92: Clean render implementation.The progress bar uses inline styles for dynamic width and Tailwind for layout. The early return for inactive state avoids unnecessary DOM updates.
src/core/game/PlayerImpl.ts (3)
1013-1015: Nukes blocked during immunity.This check prevents missile launches while spawn immunity is active. This aligns with the PR objective that missiles should be disabled during immunity.
1206-1215: Clean immunity and attack permission methods.The
isImmune()method correctly restricts immunity toPlayerType.Humanonly, matching the PR objective that nations and bots remain attackable. ThecanAttackPlayer()composes the immunity check with existing friendliness logic.
1217-1248: Good refactoring of canAttack method.The owner variable improves readability, and canAttackPlayer() centralizes attack permission logic. The 200-tile distance limit for terra nullius attacks is confirmed and intentional—it's specific to searching unowned land tiles.
One note: the 200 value is hardcoded rather than defined as a named constant. Consider extracting it as a constant if this distance limit is used elsewhere or needs to be tuned in the future.
|
@DevelopingTom Maybe call it "Spawn immunity vs humans (minutes)" or "No attack timer against humans (minutes") or something? It's a mouthful but currently it doesn't say anywhere that Nations and Bots still fight and can be fought against. It should speak for itself without having to read the manual. |
Because this is on a small button it is impractical to use large text. I changed it to "PVP spawn immunity", would that be clear enough? The window design may need to change to have clearer info on each setting. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/Attack.test.ts (2)
413-442: Verify inconsistent source coordinates in boat tests.The boat tests use different source coordinates:
- Line 421 (during immunity):
game.ref(10, 5)- Line 437 (after immunity):
game.ref(7, 0)(playerA's spawn location)Is this intentional to test different scenarios, or should both tests use the same source coordinate for consistency?
If both should use playerA's spawn location, apply this fix:
🔎 Proposed fix to use consistent source coordinates
game.addExecution( new TransportShipExecution( playerA, playerB.id(), game.ref(15, 8), 10, - game.ref(10, 5), + game.ref(7, 0), ), );
466-477: Use consistent test naming.The test name uses "Can't" while other tests in this suite use "Should not be able to" for consistency.
🔎 Proposed fix for consistent naming
- test("Can't send nuke during immunity phase", async () => { + test("Should not be able to send nuke during immunity phase", async () => {Beyond the naming, the test logic correctly verifies that nukes are blocked during immunity and allowed afterward.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
resources/lang/en.jsonsrc/client/HostLobbyModal.tssrc/core/Schemas.tssrc/server/GameServer.tstests/Attack.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- resources/lang/en.json
- src/server/GameServer.ts
- src/core/Schemas.ts
🧰 Additional context used
🧠 Learnings (5)
📚 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/Attack.test.tssrc/client/HostLobbyModal.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/Attack.test.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/HostLobbyModal.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:
src/client/HostLobbyModal.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:
src/client/HostLobbyModal.ts
🧬 Code graph analysis (2)
tests/Attack.test.ts (1)
tests/util/TestConfig.ts (1)
TestConfig(12-88)
src/client/HostLobbyModal.ts (2)
src/client/Utils.ts (1)
translateText(92-147)src/client/LangSelector.ts (1)
translateText(258-278)
🔇 Additional comments (11)
src/client/HostLobbyModal.ts (5)
48-49: State properties look good.The new spawn immunity properties follow the established pattern of the
maxTimer/maxTimerValuepair. Type choices are appropriate.
515-515: Good fix: numeric default now consistent.Changing from
?? ""to?? 0properly aligns the default value with the number input type.
816-818: Tick calculation is correct.The conversion from minutes to ticks (
minutes * 60 * 10) properly accounts for 60 seconds per minute and 10 ticks per second. Clean logic.
837-839: Config dispatch follows the right pattern.The conditional inclusion of
spawnImmunityDurationmatches the approach used for other toggle-based settings likemaxTimerValue. Clean implementation.
562-562: Translation adequately clarifies human-only immunity scope.The translation for
host_modal.spawn_immunity_durationis "PVP Spawn immunity (minutes)". The "PVP" label (Player vs Player, a standard gaming term) makes clear that this timer applies only to human players, not nations or bots. This addresses the PR feedback effectively—users will understand that spawn immunity protects against other humans only.tests/Attack.test.ts (6)
30-35: LGTM! Clean helper utilities.The
immunityPhaseTicksconstant andwaitForImmunityToEnd()helper are clear and straightforward. The loop correctly runs 11 ticks to advance past the 10-tick immunity window.
195-195: LGTM! Good consistency improvement.Capitalizing test descriptions improves readability and maintains consistent style across the test suite.
Also applies to: 239-239, 271-271, 295-295
367-392: LGTM! Core immunity tests are solid.These tests correctly verify the basic immunity behavior: attacks blocked during immunity, allowed after immunity ends. The use of
waitForImmunityToEnd()is clear and appropriate.
394-411: Edge-case boundary test is thorough.This test carefully verifies that immunity is active through the entire window and deactivates at exactly the right tick. The manual tick counting provides good precision coverage.
Note: This test depends on the assumption verified in the earlier comment about when immunity counting starts.
444-464: LGTM! Excellent coverage of nation and bot exemptions.These tests properly verify that immunity only affects human-vs-human combat. Nations and bots remain attackable during the immunity phase, which matches the PR objectives.
335-365: The setup and tests are correct—no changes needed.The immunity timer uses absolute tick counting:
numSpawnPhaseTurns() + spawnImmunityDuration() >= ticks(). After spawn phase ends at tick 101, immunity remains active through tick 110. The spawn phase does not consume immunity ticks because the immunity window is anchored to the spawn phase end, not the game start. The test correctly validates that attacks are blocked while immune and allowed once immunity expires.
|
how does this look on mobile? |
Maybe just "PVP Immunity" ? I don't think "spawn" adds much information |
I simply reused the timer from "game length": 
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/core/game/PlayerImpl.ts (1)
1210-1219: Logic correctly handles immunity restrictions.The implementation properly differentiates between Human and Bot/Nation attackers:
- Human attackers cannot attack immune players
- Bot/Nation attackers ignore immunity and can attack immune humans
This matches the PR objectives and addresses the previous review feedback about bots being unable to retaliate.
Optional: Clarify the comment for future maintainers
The comment on line 1217 could be more explicit:
- // Only humans are affected by immunity, bots and nations should be able to attack freely + // Bots and nations ignore immunity and can attack anyone (including immune humans) if not friendlyThis makes it clearer that bots/nations bypass immunity checks entirely.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
resources/lang/debug.jsonresources/lang/en.jsonsrc/client/HostLobbyModal.tssrc/core/configuration/DefaultConfig.tssrc/core/game/Game.tssrc/core/game/PlayerImpl.tssrc/server/MapPlaylist.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/server/MapPlaylist.ts
- src/core/game/Game.ts
- src/core/configuration/DefaultConfig.ts
- resources/lang/debug.json
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/HostLobbyModal.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:
src/client/HostLobbyModal.ts
📚 Learning: 2026-01-02T18:11:06.832Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:06.832Z
Learning: In src/client/HostLobbyModal.ts, the ?s=xxxxx URL suffix used in lobby URLs is purely for cache-busting platform previews (e.g., Discord, WhatsApp, x.com) and is not used by the join logic. The join flow ignores the suffix value, so regenerating it via updateUrlWithSuffix() on configuration changes will not break existing shared URLs; it only prompts platforms to refresh preview metadata. Treat the suffix as non-functional for join behavior and ensure any related changes preserve that invariant.
Applied to files:
src/client/HostLobbyModal.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/client/HostLobbyModal.tssrc/core/game/PlayerImpl.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/client/HostLobbyModal.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/client/HostLobbyModal.ts
📚 Learning: 2025-08-17T20:48:49.411Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Applied to files:
src/client/HostLobbyModal.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:
src/client/HostLobbyModal.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/PlayerImpl.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/PlayerImpl.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/game/PlayerImpl.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/PlayerImpl.ts
📚 Learning: 2025-08-23T08:03:44.914Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:547-592
Timestamp: 2025-08-23T08:03:44.914Z
Learning: In OpenFrontIO, FakeHumanExecution is used for AI-controlled nations that simulate human behavior, which is distinct from PlayerType.Bot. FakeHumanExecution players are not PlayerType.Bot - they represent more sophisticated AI that mimics human nation behavior, while PlayerType.Bot represents basic AI players.
Applied to files:
src/core/game/PlayerImpl.ts
📚 Learning: 2025-11-01T00:24:33.860Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2161
File: src/core/execution/FakeHumanExecution.ts:670-678
Timestamp: 2025-11-01T00:24:33.860Z
Learning: In OpenFrontIO, PlayerType.Bot entities cannot be in teams and do not have friendliness relationships. Only PlayerType.Human and PlayerType.FakeHuman can participate in teams and alliances. Therefore, when targeting bot-owned tiles, friendliness checks like `owner.isFriendly(this.player)` are unnecessary and meaningless for bots.
Applied to files:
src/core/game/PlayerImpl.ts
🧬 Code graph analysis (2)
src/client/HostLobbyModal.ts (2)
src/client/Utils.ts (1)
translateText(92-147)src/client/LangSelector.ts (1)
translateText(258-278)
src/core/game/PlayerImpl.ts (2)
src/core/game/Game.ts (1)
Player(540-680)src/core/game/GameMap.ts (1)
TileRef(3-3)
🔇 Additional comments (9)
resources/lang/en.json (1)
310-310: LGTM! Implements previous review feedback.This translation key directly implements the suggestion from the previous review comment. The key name
player_immunity_durationis descriptive and follows the file's naming convention, and the label "PVP immunity duration (minutes)" is clear and concise while matching the format of other time-based settings likemax_timer.src/core/game/PlayerImpl.ts (3)
1013-1015: Verify that blocking all nukes during immunity is intentional.This check prevents any player (Human, Bot, or Nation) from spawning nukes during the immunity phase. If the intent is to only prevent Humans from nuking other Humans during immunity, this would be overly restrictive.
However, the PR objectives state "Nukes cannot be thrown during the truce/immunity phase," which suggests this broad block is intentional.
Can you confirm this is the intended behavior? If Bots/Nations should still be able to nuke during immunity, the check should be more granular (e.g., checking attacker type or target immunity).
1206-1208: Clean implementation of Human-only immunity.The logic correctly restricts immunity to Human players during the spawn immunity phase. Bots and Nations remain unaffected, matching the PR objectives.
1221-1252: Good integration of immunity logic and bug fix.The immunity check is correctly integrated at line 1227 using
canAttackPlayer(owner), which ensures Humans cannot attack immune players while allowing Bots/Nations to attack freely.The change from
sharesBorderWith(other)tosharesBorderWith(owner)on line 1235 fixes an undefined variable reference —otherwas never declared in this method scope.src/client/HostLobbyModal.ts (5)
48-49: LGTM: State properties are correctly typed and initialized.The boolean toggle and optional duration follow the same pattern as other configurable options in the file.
519-519: Verify this change is intentional.This line modifies the max-timer feature (not spawn immunity). Changing the default from
?? ""to?? 0is correct for a numeric input, but this appears to be an unrelated change that might belong in a separate commit or should be explicitly mentioned in the PR description.If this is an intentional bug fix, consider noting it in the PR description for clarity.
530-569: LGTM: UI structure follows established patterns.The spawn immunity section correctly mirrors the max-timer implementation above it. The checkbox/input pairing, conditional rendering, and state binding are all implemented properly. The HTML
max="120"matches the validation logic in the handler.Verify that the translation key
"host_modal.player_immunity_duration"at line 566 matches the final agreed label from the PR discussion. The PR objectives mention the label was changed to "PVP spawn immunity" with discussion about whether "spawn" is necessary. Ensure the key inen.jsonprovides the intended label text.
737-752: LGTM: Input handlers correctly validate and prevent invalid characters.The keydown prevention and input sanitization follow the same pattern as the max-timer handlers. The validation range (0-120) properly matches the HTML input's max attribute, and immediate
putGameConfig()calls are appropriate for manual number entry.
820-843: LGTM: Spawn immunity tick calculation is correct.The conversion from minutes to ticks (
spawnImmunityDurationMinutes * 60 * 10) is correct. The game tick rate is 10 ticks per second, as confirmed by multiple usages throughout the codebase (e.g., ally expiration timers in PlayerInfoOverlay.ts, immunityTimer logic). The conditional logic properly includes the duration only when the feature is enabled.
6 participants
Description:
Resolve discussions about stalled PR #2460
Changes:
Player::canAttackPlayer(other)function to determine whether a player can be attacked.UI:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
IngloriousTom