BUG FIX: Gold double deduction + Rmoval of UnitType.Construction #2378
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
Contributor
WalkthroughReplaces UnitType.Construction / constructionType with a boolean isUnderConstruction across core game types, Unit/UnitView/updates, UI rendering, UnitGrid queries, and execution logic; many execution constructors now accept Player | Unit (with optional tile) and construction flows treat structures as timed under-construction entities. Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant Game
participant Exec as Execution
participant Unit
rect rgba(223,239,255,0.5)
Note right of Exec: Executors accept Player | Unit via overloads
Player->>Game: request startExecution(type, tile?)
Game->>Exec: new(playerOrUnit, tile?)
end
alt actor is Player
Exec->>Game: canBuild(type, tile)
Game-->>Exec: spawnTile / false
alt spawnTile
Exec->>Game: buildUnit(type, spawnTile)
Game-->>Exec: Unit (new)
else no spawn
Exec-->>Game: warn + deactivate
end
else actor is Unit
Exec->>Unit: use provided unit directly
end
alt unit is structure
Exec->>Unit: setUnderConstruction(true) + set duration
loop ticks
Exec->>Unit: tick -> decrement remaining
end
Exec->>Unit: setUnderConstruction(false)
Exec->>Game: trigger post-construction executors (pass structure)
else not structure
Exec->>Unit: complete immediately
Exec->>Game: trigger immediate post-construction executors
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (8)📓 Common learnings📚 Learning: 2025-08-12T00:31:50.144ZApplied to files:
📚 Learning: 2025-10-20T20:15:28.858ZApplied to files:
📚 Learning: 2025-08-29T16:16:11.309ZApplied to files:
📚 Learning: 2025-10-27T09:47:26.395ZApplied to files:
📚 Learning: 2025-05-31T18:15:03.445ZApplied to files:
📚 Learning: 2025-10-08T17:14:49.369ZApplied to files:
📚 Learning: 2025-10-18T17:54:01.311ZApplied to files:
🧬 Code graph analysis (1)src/core/game/PlayerImpl.ts (1)
🔇 Additional comments (2)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/game/PlayerImpl.ts (1)
251-263: Critical logic error in unitsOwned method.Lines 258-260 contain unreachable code. If
unit.type() === typeis true at line 254, the method continues at line 256, skipping lines 258-260. But line 258 also checksunit.type() === type, which means this condition can never be true at that point.It appears the intended logic might be:
- Count completed units by their level
- Also count units under construction as +1 each
Apply this diff to fix the logic:
unitsOwned(type: UnitType): number { let total = 0; for (const unit of this._units) { if (unit.type() === type) { - total += unit.level(); - continue; - } - if (unit.type() === type && unit.isUnderConstruction()) { - total++; + if (unit.isUnderConstruction()) { + total++; + } else { + total += unit.level(); + } } } return total; }src/core/execution/ConstructionExecution.ts (1)
93-100: Fix the countdown off-by-oneRight now structures finish one whole tick later than the configured
constructionDuration. We start at the full duration, only decrement after the zero check, and complete on the following tick. With a duration of 1 this means waiting through 2 ticks. Please flip the order so we finish on time.- if (this.ticksUntilComplete === 0) { - this.player = this.structure.owner(); - this.completeConstruction(); - this.active = false; - return; - } - this.ticksUntilComplete--; + if (this.ticksUntilComplete !== undefined) { + if (this.ticksUntilComplete <= 1) { + this.player = this.structure.owner(); + this.completeConstruction(); + this.active = false; + return; + } + this.ticksUntilComplete--; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
src/client/graphics/layers/RadialMenuElements.ts(1 hunks)src/client/graphics/layers/StructureDrawingUtils.ts(1 hunks)src/client/graphics/layers/StructureIconsLayer.ts(5 hunks)src/client/graphics/layers/StructureLayer.ts(4 hunks)src/client/graphics/layers/UILayer.ts(2 hunks)src/core/configuration/DefaultConfig.ts(0 hunks)src/core/execution/CityExecution.ts(3 hunks)src/core/execution/ConstructionExecution.ts(5 hunks)src/core/execution/DefensePostExecution.ts(3 hunks)src/core/execution/FactoryExecution.ts(1 hunks)src/core/execution/MissileSiloExecution.ts(3 hunks)src/core/execution/NukeExecution.ts(1 hunks)src/core/execution/PortExecution.ts(5 hunks)src/core/execution/SAMLauncherExecution.ts(1 hunks)src/core/game/Game.ts(1 hunks)src/core/game/GameUpdates.ts(1 hunks)src/core/game/GameView.ts(1 hunks)src/core/game/PlayerImpl.ts(3 hunks)src/core/game/UnitImpl.ts(3 hunks)tests/client/graphics/UILayer.test.ts(2 hunks)tests/economy/ConstructionGold.test.ts(1 hunks)tests/nukes/HydrogenAndMirv.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/core/configuration/DefaultConfig.ts
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
Learnt from: Egraveline
Repo: openfrontio/OpenFrontIO PR: 1012
File: src/core/execution/UpgradeStructureExecution.ts:49-54
Timestamp: 2025-06-05T02:34:45.899Z
Learning: In the upgrade system, gold deduction for structure upgrades is handled internally by the `upgradeUnit` method in PlayerImpl, not in the UpgradeStructureExecution class. The UpgradeStructureExecution only needs to check if the player has sufficient gold before calling `upgradeUnit`.
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.
📚 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/GameUpdates.tssrc/core/game/GameView.tssrc/core/execution/MissileSiloExecution.tstests/economy/ConstructionGold.test.tssrc/core/game/UnitImpl.tssrc/core/execution/CityExecution.tssrc/core/game/PlayerImpl.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/core/game/Game.tstests/nukes/HydrogenAndMirv.test.tssrc/core/execution/ConstructionExecution.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/game/Game.tstests/nukes/HydrogenAndMirv.test.ts
📚 Learning: 2025-08-14T14:05:00.867Z
Learnt from: Dimitrije-V
Repo: openfrontio/OpenFrontIO PR: 1814
File: src/client/graphics/layers/PlayerActionHandler.ts:0-0
Timestamp: 2025-08-14T14:05:00.867Z
Learning: In TypeScript, when dealing with union types like NukeType (derived from UnitType), it's better to accept the broader type (UnitType) as a parameter and use runtime type checking rather than trying to narrow the parameter type at compile time. The approach of keeping flexible input parameters with runtime validation followed by properly typed state/storage is more practical than over-constraining function signatures.
Applied to files:
src/core/game/Game.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
tests/nukes/HydrogenAndMirv.test.tssrc/core/execution/SAMLauncherExecution.tssrc/core/execution/PortExecution.tssrc/core/execution/MissileSiloExecution.tssrc/core/execution/ConstructionExecution.tssrc/core/execution/CityExecution.tssrc/core/execution/NukeExecution.tssrc/core/execution/FactoryExecution.tssrc/core/execution/DefensePostExecution.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:
tests/nukes/HydrogenAndMirv.test.tssrc/core/execution/SAMLauncherExecution.tssrc/core/execution/PortExecution.tssrc/core/execution/MissileSiloExecution.tstests/economy/ConstructionGold.test.tssrc/core/execution/CityExecution.tssrc/core/game/PlayerImpl.tssrc/core/execution/NukeExecution.tssrc/core/execution/FactoryExecution.tssrc/core/execution/DefensePostExecution.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/nukes/HydrogenAndMirv.test.tssrc/core/execution/SAMLauncherExecution.tssrc/core/execution/MissileSiloExecution.tssrc/core/execution/ConstructionExecution.tssrc/core/execution/CityExecution.tssrc/core/game/PlayerImpl.tssrc/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:
tests/nukes/HydrogenAndMirv.test.tssrc/core/execution/PortExecution.tssrc/core/execution/MissileSiloExecution.ts
📚 Learning: 2025-08-28T22:47:31.406Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: tests/core/executions/PlayerExecution.test.ts:16-39
Timestamp: 2025-08-28T22:47:31.406Z
Learning: In test files, PlayerExecution instances must be manually registered with game.addExecution() because the setup utility doesn't automatically register SpawnExecution, which would normally handle this during the spawn phase in a real game.
Applied to files:
tests/nukes/HydrogenAndMirv.test.tstests/economy/ConstructionGold.test.tssrc/core/execution/ConstructionExecution.tssrc/core/execution/FactoryExecution.ts
📚 Learning: 2025-06-16T03:03:59.778Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1192
File: src/client/graphics/layers/RadialMenuElements.ts:312-314
Timestamp: 2025-06-16T03:03:59.778Z
Learning: In RadialMenuElements.ts, when fixing undefined params errors in subMenu functions, use explicit checks like `if (params === undefined || params.selected === null)` rather than optional chaining, as it makes the intent clearer and matches the specific error scenarios where params can be undefined during spawn phase operations.
Applied to files:
src/client/graphics/layers/RadialMenuElements.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/SAMLauncherExecution.tssrc/core/execution/NukeExecution.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/core/execution/PortExecution.tssrc/core/execution/MissileSiloExecution.tssrc/core/execution/ConstructionExecution.tssrc/core/execution/CityExecution.tssrc/core/game/PlayerImpl.tssrc/core/execution/FactoryExecution.tssrc/core/execution/DefensePostExecution.ts
📚 Learning: 2025-06-05T02:34:45.899Z
Learnt from: Egraveline
Repo: openfrontio/OpenFrontIO PR: 1012
File: src/core/execution/UpgradeStructureExecution.ts:49-54
Timestamp: 2025-06-05T02:34:45.899Z
Learning: In the upgrade system, gold deduction for structure upgrades is handled internally by the `upgradeUnit` method in PlayerImpl, not in the UpgradeStructureExecution class. The UpgradeStructureExecution only needs to check if the player has sufficient gold before calling `upgradeUnit`.
Applied to files:
src/core/execution/ConstructionExecution.ts
📚 Learning: 2025-08-18T03:11:18.617Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1851
File: src/core/execution/FakeHumanExecution.ts:433-441
Timestamp: 2025-08-18T03:11:18.617Z
Learning: In FakeHumanExecution.maybeSpawnStructure, the perceived cost multiplier (owned + 1, capped at 5) applied to structure costs is intentional. This makes AI nations build fewer structures than players could, saving gold for bombs/nukes rather than maxing out structures.
Applied to files:
src/core/execution/ConstructionExecution.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/ConstructionExecution.tssrc/core/execution/DefensePostExecution.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/StructureIconsLayer.tssrc/client/graphics/layers/StructureLayer.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/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/core/game/PlayerImpl.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/StructureLayer.ts
🧬 Code graph analysis (11)
tests/nukes/HydrogenAndMirv.test.ts (5)
src/core/game/Game.ts (3)
Game(660-744)Player(521-658)PlayerInfo(402-416)tests/util/Setup.ts (1)
setup(23-81)src/core/game/PlayerImpl.ts (2)
info(335-337)target(527-530)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(7-61)src/core/execution/ConstructionExecution.ts (1)
ConstructionExecution(21-169)
src/core/execution/PortExecution.ts (2)
src/core/game/Game.ts (3)
Player(521-658)Unit(428-506)isUnit(418-426)src/core/execution/TradeShipExecution.ts (1)
TradeShipExecution(15-181)
src/core/execution/MissileSiloExecution.ts (1)
src/core/game/Game.ts (3)
Player(521-658)Unit(428-506)isUnit(418-426)
tests/economy/ConstructionGold.test.ts (4)
src/core/game/Game.ts (3)
Game(660-744)Player(521-658)PlayerInfo(402-416)tests/util/Setup.ts (1)
setup(23-81)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(7-61)src/core/execution/ConstructionExecution.ts (1)
ConstructionExecution(21-169)
src/core/execution/ConstructionExecution.ts (7)
src/core/game/Game.ts (1)
Unit(428-506)src/core/execution/PortExecution.ts (1)
PortExecution(7-111)src/core/execution/MissileSiloExecution.ts (1)
MissileSiloExecution(4-67)src/core/execution/DefensePostExecution.ts (1)
DefensePostExecution(5-137)src/core/execution/SAMLauncherExecution.ts (1)
SAMLauncherExecution(163-337)src/core/execution/CityExecution.ts (1)
CityExecution(5-67)src/core/execution/FactoryExecution.ts (1)
FactoryExecution(5-67)
src/client/graphics/layers/StructureIconsLayer.ts (2)
src/core/game/GameView.ts (3)
unit(658-660)type(94-96)type(340-342)src/core/game/UnitImpl.ts (1)
type(145-147)
src/core/execution/CityExecution.ts (2)
src/core/game/Game.ts (3)
Player(521-658)Unit(428-506)isUnit(418-426)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/core/game/PlayerImpl.ts (2)
src/core/game/GameView.ts (3)
unit(658-660)type(94-96)type(340-342)src/core/game/UnitImpl.ts (1)
type(145-147)
tests/client/graphics/UILayer.test.ts (1)
src/core/game/GameView.ts (2)
UnitView(44-179)unit(658-660)
src/core/execution/FactoryExecution.ts (2)
src/core/game/Game.ts (5)
Execution(347-352)Unit(428-506)Game(660-744)Player(521-658)isUnit(418-426)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/core/execution/DefensePostExecution.ts (2)
src/core/game/Game.ts (3)
Player(521-658)Unit(428-506)isUnit(418-426)src/core/game/GameMap.ts (1)
TileRef(3-3)
🪛 GitHub Actions: 🧪 CI
tests/nukes/HydrogenAndMirv.test.ts
[warning] 1-1: Prettier formatting issues detected. Run 'npx prettier --write' to fix.
src/core/execution/PortExecution.ts
[warning] 1-1: Prettier formatting issues detected. Run 'npx prettier --write' to fix.
src/client/graphics/layers/UILayer.ts
[warning] 1-1: Prettier formatting issues detected. Run 'npx prettier --write' to fix.
src/core/execution/MissileSiloExecution.ts
[warning] 1-1: Prettier formatting issues detected. Run 'npx prettier --write' to fix.
tests/economy/ConstructionGold.test.ts
[warning] 1-1: Prettier formatting issues detected. Run 'npx prettier --write' to fix.
src/core/execution/ConstructionExecution.ts
[warning] 1-1: Prettier formatting issues detected. Run 'npx prettier --write' to fix.
src/core/execution/CityExecution.ts
[warning] 1-1: Prettier formatting issues detected. Run 'npx prettier --write' to fix.
src/core/execution/NukeExecution.ts
[error] 1-1: Nuke execution did not destroy out-of-range buildings as expected. Expected City units to be 0 after nuke, but 1 remained.
src/core/execution/FactoryExecution.ts
[warning] 1-1: Prettier formatting issues detected. Run 'npx prettier --write' to fix.
src/core/execution/DefensePostExecution.ts
[warning] 1-1: Prettier formatting issues detected. Run 'npx prettier --write' to fix.
🔇 Additional comments (11)
src/core/game/GameUpdates.ts (1)
130-130: LGTM!The change from
constructionType?: UnitTypetounderConstruction?: booleanis a clear improvement. A boolean flag is the right choice for representing construction state rather than storing a type.src/core/game/GameView.ts (1)
124-126: LGTM!The new
isUnderConstruction()API is cleaner and more expressive than the previousconstructionType()approach. The explicit check for=== trueensures proper handling of undefined values.src/core/game/PlayerImpl.ts (2)
227-237: LGTM!The updated
unitsConstructedmethod correctly counts units under construction using the newisUnderConstruction()API. The logic is clear and consistent with the refactor.
1000-1010: Good addition of under-construction constraint.Adding the
!silo.isUnderConstruction()filter at line 1003 correctly prevents nukes from spawning at missile silos that are still being built. This is logical and aligns with the expected game behavior.src/client/graphics/layers/RadialMenuElements.ts (1)
450-462: LGTM!The updated delete eligibility filter using
!unit.isUnderConstruction()is clearer and more expressive than the previousconstructionType() === undefinedcheck. The logic correctly prevents deletion of units still under construction.src/core/execution/SAMLauncherExecution.ts (1)
219-221: LGTM!The early return guard for SAMs under construction is well-placed and prevents cooldown handling and targeting logic from executing on incomplete units. This is the right approach.
src/core/game/Game.ts (2)
494-496: LGTM!The new construction state API using
isUnderConstruction()andsetUnderConstruction()is cleaner and more expressive than the previous type-based approach. The comment clearly indicates this is for structures during their construction phase.
181-212: LGTM!The removal of
UnitType.Constructionfrom the enum and related data structures is complete and consistent with the PR objectives. The refactor to use boolean construction state instead of a dedicated type is cleaner.tests/client/graphics/UILayer.test.ts (1)
119-160: LGTM!The test updates correctly use the new
isUnderConstruction()API and provide good coverage of construction state transitions. The tests verify that loading bars are properly added for units under construction and removed when construction completes or units become inactive.src/core/execution/NukeExecution.ts (1)
104-112: Review comment is incorrect - src change does not affect building destruction.The review comment claims the spawn calculation change impacts building destruction through
redrawBuildings(). This is false.The
redrawBuildings()method (line 317) calculates distance only fromthis.dst(the target location) to each unit:this.mg.euclideanDistSquared(this.dst, unit.tile()) < rangeSquaredIt never uses
this.src. The src change only affects trajectory targetability logic (lines 211-213 inisTargetable()), not building destruction.The pipeline failure about buildings not being destroyed is unrelated to when
this.srcis set. If the test is failing, the cause lies elsewhere—possibly in the test setup, magnitude configuration, or a different code path. The spawn calculation change itself is sound and does not introduce the reported issue.Likely an incorrect or invalid review comment.
src/client/graphics/layers/StructureDrawingUtils.ts (1)
146-148: Switch to the construction flag looks good.Using
unit.isUnderConstruction()keeps the draw code aligned with the new typed-flag flow.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/core/executions/NukeExecution.test.ts (1)
79-99: Fix silo location for this launch setup.NukeExecution now refuses to spawn unless a friendly silo covers the launch tile. We add a silo at (1, 1) but launch from (199, 199), so the missile never spawns and
getNuke()stays null (matching the CI failure). Move the test silo next to the launch origin so the execution can validate and produce the nuke.- player.buildUnit(UnitType.MissileSilo, game.ref(1, 1), {}); + player.buildUnit(UnitType.MissileSilo, game.ref(199, 199), {});
♻️ Duplicate comments (1)
tests/nukes/HydrogenAndMirv.test.ts (1)
41-41: Tighten the Hydrogen bomb assertion.
length >= 0is always true, so the test never fails even if the launch gives no bomb. Assert that the count is greater than zero to prove the construction completed.- expect(player.units(UnitType.HydrogenBomb).length).toBeGreaterThanOrEqual(0); + expect(player.units(UnitType.HydrogenBomb).length).toBeGreaterThan(0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/client/graphics/layers/UILayer.ts(2 hunks)src/core/execution/CityExecution.ts(3 hunks)src/core/execution/ConstructionExecution.ts(6 hunks)src/core/execution/DefensePostExecution.ts(3 hunks)src/core/execution/FactoryExecution.ts(3 hunks)src/core/execution/MissileSiloExecution.ts(3 hunks)src/core/execution/PortExecution.ts(5 hunks)tests/core/executions/NukeExecution.test.ts(1 hunks)tests/economy/ConstructionGold.test.ts(1 hunks)tests/nukes/HydrogenAndMirv.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/graphics/layers/UILayer.ts
- tests/economy/ConstructionGold.test.ts
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: Egraveline
Repo: openfrontio/OpenFrontIO PR: 1012
File: src/core/execution/UpgradeStructureExecution.ts:49-54
Timestamp: 2025-06-05T02:34:45.899Z
Learning: In the upgrade system, gold deduction for structure upgrades is handled internally by the `upgradeUnit` method in PlayerImpl, not in the UpgradeStructureExecution class. The UpgradeStructureExecution only needs to check if the player has sufficient gold before calling `upgradeUnit`.
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.
📚 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/core/executions/NukeExecution.test.tssrc/core/execution/MissileSiloExecution.tstests/nukes/HydrogenAndMirv.test.tssrc/core/execution/CityExecution.tssrc/core/execution/ConstructionExecution.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/core/executions/NukeExecution.test.tssrc/core/execution/PortExecution.tssrc/core/execution/MissileSiloExecution.tstests/nukes/HydrogenAndMirv.test.tssrc/core/execution/CityExecution.tssrc/core/execution/DefensePostExecution.tssrc/core/execution/ConstructionExecution.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:
tests/core/executions/NukeExecution.test.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:
tests/core/executions/NukeExecution.test.tstests/nukes/HydrogenAndMirv.test.tssrc/core/execution/ConstructionExecution.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:
tests/core/executions/NukeExecution.test.tssrc/core/execution/PortExecution.tssrc/core/execution/MissileSiloExecution.tssrc/core/execution/FactoryExecution.tstests/nukes/HydrogenAndMirv.test.tssrc/core/execution/CityExecution.tssrc/core/execution/DefensePostExecution.tssrc/core/execution/ConstructionExecution.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:
tests/core/executions/NukeExecution.test.tstests/nukes/HydrogenAndMirv.test.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/core/execution/PortExecution.tssrc/core/execution/MissileSiloExecution.tssrc/core/execution/FactoryExecution.tssrc/core/execution/CityExecution.tssrc/core/execution/DefensePostExecution.tssrc/core/execution/ConstructionExecution.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/execution/PortExecution.tssrc/core/execution/MissileSiloExecution.tssrc/core/execution/CityExecution.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/PortExecution.tssrc/core/execution/MissileSiloExecution.tstests/nukes/HydrogenAndMirv.test.ts
📚 Learning: 2025-08-28T22:47:31.406Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: tests/core/executions/PlayerExecution.test.ts:16-39
Timestamp: 2025-08-28T22:47:31.406Z
Learning: In test files, PlayerExecution instances must be manually registered with game.addExecution() because the setup utility doesn't automatically register SpawnExecution, which would normally handle this during the spawn phase in a real game.
Applied to files:
tests/nukes/HydrogenAndMirv.test.tssrc/core/execution/ConstructionExecution.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/DefensePostExecution.tssrc/core/execution/ConstructionExecution.ts
📚 Learning: 2025-06-05T02:34:45.899Z
Learnt from: Egraveline
Repo: openfrontio/OpenFrontIO PR: 1012
File: src/core/execution/UpgradeStructureExecution.ts:49-54
Timestamp: 2025-06-05T02:34:45.899Z
Learning: In the upgrade system, gold deduction for structure upgrades is handled internally by the `upgradeUnit` method in PlayerImpl, not in the UpgradeStructureExecution class. The UpgradeStructureExecution only needs to check if the player has sufficient gold before calling `upgradeUnit`.
Applied to files:
src/core/execution/ConstructionExecution.ts
📚 Learning: 2025-08-18T03:11:18.617Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1851
File: src/core/execution/FakeHumanExecution.ts:433-441
Timestamp: 2025-08-18T03:11:18.617Z
Learning: In FakeHumanExecution.maybeSpawnStructure, the perceived cost multiplier (owned + 1, capped at 5) applied to structure costs is intentional. This makes AI nations build fewer structures than players could, saving gold for bombs/nukes rather than maxing out structures.
Applied to files:
src/core/execution/ConstructionExecution.ts
🧬 Code graph analysis (7)
src/core/execution/PortExecution.ts (2)
src/core/game/Game.ts (3)
Player(521-658)Unit(428-506)isUnit(418-426)src/core/execution/TradeShipExecution.ts (1)
TradeShipExecution(15-181)
src/core/execution/MissileSiloExecution.ts (2)
src/core/game/Game.ts (3)
Player(521-658)Unit(428-506)isUnit(418-426)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/core/execution/FactoryExecution.ts (2)
src/core/game/Game.ts (3)
Player(521-658)Unit(428-506)isUnit(418-426)src/core/game/GameMap.ts (1)
TileRef(3-3)
tests/nukes/HydrogenAndMirv.test.ts (4)
src/core/game/Game.ts (3)
Game(660-744)Player(521-658)PlayerInfo(402-416)tests/util/Setup.ts (1)
setup(23-81)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(7-61)src/core/execution/ConstructionExecution.ts (1)
ConstructionExecution(13-161)
src/core/execution/CityExecution.ts (3)
src/core/game/Game.ts (3)
Player(521-658)Unit(428-506)isUnit(418-426)src/core/game/UnitImpl.ts (2)
tile(175-177)isUnit(100-102)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/core/execution/DefensePostExecution.ts (2)
src/core/game/Game.ts (3)
Player(521-658)Unit(428-506)isUnit(418-426)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/core/execution/ConstructionExecution.ts (7)
src/core/game/Game.ts (1)
Unit(428-506)src/core/execution/PortExecution.ts (1)
PortExecution(7-115)src/core/execution/MissileSiloExecution.ts (1)
MissileSiloExecution(4-71)src/core/execution/DefensePostExecution.ts (1)
DefensePostExecution(5-140)src/core/execution/SAMLauncherExecution.ts (1)
SAMLauncherExecution(163-337)src/core/execution/CityExecution.ts (1)
CityExecution(5-67)src/core/execution/FactoryExecution.ts (1)
FactoryExecution(5-77)
🪛 GitHub Actions: 🧪 CI
tests/core/executions/NukeExecution.test.ts
[error] 71-71: Nuke should destroy buildings and redraw out of range buildings. Expected city count 0, but found 1.
[error] 91-91: Cannot read properties of null (reading 'isTargetable') in NukeExecution test near-launch scenario.
tests/nukes/HydrogenAndMirv.test.ts
[warning] 1-1: Code style issues found by Prettier. Run 'npx prettier --write' to fix.
src/core/execution/ConstructionExecution.ts
[warning] 1-1: Code style issues found by Prettier. Run 'npx prettier --write' to fix.
🔇 Additional comments (4)
src/core/execution/PortExecution.ts (4)
29-46: Port initialization logic handles both paths correctly.The branching clearly separates the Unit-based path (direct assignment) from the Player-based path (build via canBuild/buildUnit). Both paths create the station, and error handling for failed builds is appropriate.
53-55: Early return during construction prevents premature trade ship spawning.The
isUnderConstruction()check correctly blocks trade ship spawning until the port is ready, aligning with the PR's removal of the Construction unit type.
62-76: Port ownership correctly derives from port.owner() for trade ship spawning.The change from
this.playertothis.port.owner()ensures trade ships spawn for the current port owner, handling scenarios where port ownership changes (e.g., capture). The TradeShipExecution receives the correct owner reference.
86-101: Spawn rate calculation uses port owner's units correctly.The method correctly queries
port.owner().unitCount()for the current owner's ports and trade ships. Non-null assertions onthis.port!are safe because this method is only called after initialization and active checks.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/execution/CityExecution.ts (1)
1-74: Run prettier to fix formatting.The pipeline detected code style issues.
Run this command to fix:
#!/bin/bash npx prettier --write src/core/execution/CityExecution.tssrc/core/execution/PortExecution.ts (1)
1-120: Run prettier to fix formatting.The pipeline detected code style issues.
Run this command to fix:
#!/bin/bash npx prettier --write src/core/execution/PortExecution.ts
♻️ Duplicate comments (1)
src/core/execution/PortExecution.ts (1)
14-22: Type safety concern from past review is now addressed.The constructor overload and runtime validation ensure tile is provided when passing a Player. This is the correct pattern.
🧹 Nitpick comments (1)
src/core/execution/ConstructionExecution.ts (1)
146-158: isStructure helper could use a Set for clarity.A switch statement works, but a Set would be more idiomatic TypeScript and easier to maintain.
Consider this alternative:
- private isStructure(type: UnitType): boolean { - switch (type) { - case UnitType.Port: - case UnitType.MissileSilo: - case UnitType.DefensePost: - case UnitType.SAMLauncher: - case UnitType.City: - case UnitType.Factory: - return true; - default: - return false; - } - } + private static readonly STRUCTURE_TYPES = new Set([ + UnitType.Port, + UnitType.MissileSilo, + UnitType.DefensePost, + UnitType.SAMLauncher, + UnitType.City, + UnitType.Factory, + ]); + + private isStructure(type: UnitType): boolean { + return ConstructionExecution.STRUCTURE_TYPES.has(type); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/core/execution/CityExecution.ts(2 hunks)src/core/execution/ConstructionExecution.ts(6 hunks)src/core/execution/PortExecution.ts(5 hunks)tests/core/executions/NukeExecution.test.ts(2 hunks)tests/nukes/HydrogenAndMirv.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/nukes/HydrogenAndMirv.test.ts
- tests/core/executions/NukeExecution.test.ts
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: Egraveline
Repo: openfrontio/OpenFrontIO PR: 1012
File: src/core/execution/UpgradeStructureExecution.ts:49-54
Timestamp: 2025-06-05T02:34:45.899Z
Learning: In the upgrade system, gold deduction for structure upgrades is handled internally by the `upgradeUnit` method in PlayerImpl, not in the UpgradeStructureExecution class. The UpgradeStructureExecution only needs to check if the player has sufficient gold before calling `upgradeUnit`.
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.
📚 Learning: 2025-06-05T02:34:45.899Z
Learnt from: Egraveline
Repo: openfrontio/OpenFrontIO PR: 1012
File: src/core/execution/UpgradeStructureExecution.ts:49-54
Timestamp: 2025-06-05T02:34:45.899Z
Learning: In the upgrade system, gold deduction for structure upgrades is handled internally by the `upgradeUnit` method in PlayerImpl, not in the UpgradeStructureExecution class. The UpgradeStructureExecution only needs to check if the player has sufficient gold before calling `upgradeUnit`.
Applied to files:
src/core/execution/ConstructionExecution.ts
📚 Learning: 2025-08-18T03:11:18.617Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1851
File: src/core/execution/FakeHumanExecution.ts:433-441
Timestamp: 2025-08-18T03:11:18.617Z
Learning: In FakeHumanExecution.maybeSpawnStructure, the perceived cost multiplier (owned + 1, capped at 5) applied to structure costs is intentional. This makes AI nations build fewer structures than players could, saving gold for bombs/nukes rather than maxing out structures.
Applied to files:
src/core/execution/ConstructionExecution.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/ConstructionExecution.tssrc/core/execution/PortExecution.tssrc/core/execution/CityExecution.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/core/execution/ConstructionExecution.tssrc/core/execution/PortExecution.tssrc/core/execution/CityExecution.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/ConstructionExecution.tssrc/core/execution/CityExecution.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/ConstructionExecution.tssrc/core/execution/PortExecution.tssrc/core/execution/CityExecution.ts
📚 Learning: 2025-08-28T22:47:31.406Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: tests/core/executions/PlayerExecution.test.ts:16-39
Timestamp: 2025-08-28T22:47:31.406Z
Learning: In test files, PlayerExecution instances must be manually registered with game.addExecution() because the setup utility doesn't automatically register SpawnExecution, which would normally handle this during the spawn phase in a real game.
Applied to files:
src/core/execution/ConstructionExecution.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/ConstructionExecution.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/ConstructionExecution.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/execution/PortExecution.tssrc/core/execution/CityExecution.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/PortExecution.ts
🧬 Code graph analysis (3)
src/core/execution/ConstructionExecution.ts (7)
src/core/game/Game.ts (1)
Unit(428-506)src/core/execution/PortExecution.ts (1)
PortExecution(7-120)src/core/execution/MissileSiloExecution.ts (1)
MissileSiloExecution(4-71)src/core/execution/DefensePostExecution.ts (1)
DefensePostExecution(5-140)src/core/execution/SAMLauncherExecution.ts (1)
SAMLauncherExecution(163-337)src/core/execution/CityExecution.ts (1)
CityExecution(5-74)src/core/execution/FactoryExecution.ts (1)
FactoryExecution(5-77)
src/core/execution/PortExecution.ts (3)
src/core/game/Game.ts (3)
Unit(428-506)Player(521-658)isUnit(418-426)src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/execution/TradeShipExecution.ts (1)
TradeShipExecution(15-181)
src/core/execution/CityExecution.ts (2)
src/core/game/Game.ts (4)
Player(521-658)Unit(428-506)Game(660-744)isUnit(418-426)src/core/game/GameMap.ts (1)
TileRef(3-3)
🪛 GitHub Actions: 🧪 CI
src/core/execution/PortExecution.ts
[warning] Code style issues found in file. Run 'npx prettier --write' to fix.
src/core/execution/CityExecution.ts
[warning] Code style issues found in file. Run 'npx prettier --write' to fix.
🔇 Additional comments (6)
src/core/execution/CityExecution.ts (1)
19-41: Dual-path construction logic is clean.The isUnit guard clearly separates the Unit-assignment path from the Player-build path. The structure is easy to follow.
src/core/execution/PortExecution.ts (2)
58-60: Good practice: early exit during construction.Preventing trade ship spawning while the port is under construction avoids logic errors. This pattern is consistent with other structure executors.
71-80: Owner references are correctly maintained.Using
this.port.owner()for TradeShipExecution ensures the execution uses the current owner, which is important if the port changes hands during construction.src/core/execution/ConstructionExecution.ts (3)
45-78: Structure vs non-structure split is clear and fixes the gold issue.The two-path approach makes sense:
- Non-structures: delegate validation and gold deduction to specialized executions (NukeExecution, MirvExecution, WarshipExecution)
- Structures: build immediately via
buildUnit(which charges gold once), then wait for construction completionThis centralizes the gold charge and eliminates double deduction.
98-101: Safe null check before clearing construction state.Checking
if (this.structure)before callingsetUnderConstruction(false)handles the case where non-structures never create a structure. Good defensive coding.
118-137: Specialized executions validate before spawning units—no issues found.All three executions properly validate before calling
buildUnit():
- NukeExecution (tick, lines ~85-91): Calls
canBuild(), returns early if validation fails- MirvExecution (tick, lines ~47-53): Calls
canBuild(), returns early if validation fails- WarshipExecution (init, lines ~34-42): Calls
canBuild(), returns early if validation failsGold deduction happens internally within
buildUnit(), following the same pattern as the upgrade system.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/nukes/HydrogenAndMirv.test.ts (1)
68-70: Prefer idiomatic assertion style.The boolean expression
expect(mirvs > 0 || warheads > 0).toBe(true)works but is less clear than alternatives.Consider one of these more idiomatic options:
- const mirvs = player.units(UnitType.MIRV).length; - const warheads = player.units(UnitType.MIRVWarhead).length; - expect(mirvs > 0 || warheads > 0).toBe(true); + const totalUnits = + player.units(UnitType.MIRV).length + + player.units(UnitType.MIRVWarhead).length; + expect(totalUnits).toBeGreaterThan(0);Or if you want to keep separate counts:
const mirvs = player.units(UnitType.MIRV).length; const warheads = player.units(UnitType.MIRVWarhead).length; - expect(mirvs > 0 || warheads > 0).toBe(true); + expect(mirvs + warheads).toBeGreaterThan(0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/core/execution/CityExecution.ts(2 hunks)src/core/execution/DefensePostExecution.ts(3 hunks)src/core/execution/FactoryExecution.ts(1 hunks)src/core/execution/MissileSiloExecution.ts(1 hunks)src/core/execution/PortExecution.ts(5 hunks)tests/nukes/HydrogenAndMirv.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: Egraveline
Repo: openfrontio/OpenFrontIO PR: 1012
File: src/core/execution/UpgradeStructureExecution.ts:49-54
Timestamp: 2025-06-05T02:34:45.899Z
Learning: In the upgrade system, gold deduction for structure upgrades is handled internally by the `upgradeUnit` method in PlayerImpl, not in the UpgradeStructureExecution class. The UpgradeStructureExecution only needs to check if the player has sufficient gold before calling `upgradeUnit`.
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.
📚 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/FactoryExecution.tssrc/core/execution/DefensePostExecution.tstests/nukes/HydrogenAndMirv.test.tssrc/core/execution/CityExecution.tssrc/core/execution/PortExecution.tssrc/core/execution/MissileSiloExecution.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/core/execution/FactoryExecution.tssrc/core/execution/DefensePostExecution.tstests/nukes/HydrogenAndMirv.test.tssrc/core/execution/CityExecution.tssrc/core/execution/PortExecution.tssrc/core/execution/MissileSiloExecution.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/FactoryExecution.tssrc/core/execution/DefensePostExecution.tstests/nukes/HydrogenAndMirv.test.tssrc/core/execution/PortExecution.tssrc/core/execution/MissileSiloExecution.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/FactoryExecution.tstests/nukes/HydrogenAndMirv.test.tssrc/core/execution/PortExecution.tssrc/core/execution/MissileSiloExecution.ts
📚 Learning: 2025-08-28T22:47:31.406Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: tests/core/executions/PlayerExecution.test.ts:16-39
Timestamp: 2025-08-28T22:47:31.406Z
Learning: In test files, PlayerExecution instances must be manually registered with game.addExecution() because the setup utility doesn't automatically register SpawnExecution, which would normally handle this during the spawn phase in a real game.
Applied to files:
src/core/execution/FactoryExecution.tstests/nukes/HydrogenAndMirv.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:
src/core/execution/DefensePostExecution.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/nukes/HydrogenAndMirv.test.tssrc/core/execution/CityExecution.tssrc/core/execution/MissileSiloExecution.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:
tests/nukes/HydrogenAndMirv.test.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:
tests/nukes/HydrogenAndMirv.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/nukes/HydrogenAndMirv.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/nukes/HydrogenAndMirv.test.tssrc/core/execution/CityExecution.tssrc/core/execution/MissileSiloExecution.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:
tests/nukes/HydrogenAndMirv.test.ts
🧬 Code graph analysis (6)
src/core/execution/FactoryExecution.ts (1)
src/core/game/Game.ts (5)
Execution(347-352)Unit(428-506)Game(660-744)Player(521-658)isUnit(418-426)
src/core/execution/DefensePostExecution.ts (2)
src/core/game/Game.ts (3)
Unit(428-506)Player(521-658)isUnit(418-426)src/core/game/GameMap.ts (1)
TileRef(3-3)
tests/nukes/HydrogenAndMirv.test.ts (4)
src/core/game/Game.ts (3)
Game(660-744)Player(521-658)PlayerInfo(402-416)tests/util/Setup.ts (1)
setup(23-81)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(7-61)src/core/execution/ConstructionExecution.ts (1)
ConstructionExecution(13-167)
src/core/execution/CityExecution.ts (2)
src/core/game/Game.ts (4)
Unit(428-506)Player(521-658)isUnit(418-426)Game(660-744)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/core/execution/PortExecution.ts (2)
src/core/game/Game.ts (3)
Unit(428-506)Player(521-658)isUnit(418-426)src/core/execution/TradeShipExecution.ts (1)
TradeShipExecution(15-181)
src/core/execution/MissileSiloExecution.ts (2)
src/core/game/Game.ts (4)
Game(660-744)Unit(428-506)Player(521-658)isUnit(418-426)src/core/game/GameMap.ts (1)
TileRef(3-3)
🔇 Additional comments (3)
src/core/execution/FactoryExecution.ts (2)
1-1: Clean constructor overload pattern.The overload design correctly handles both Unit and Player+TileRef inputs. Runtime validation (lines 17-19) ensures type safety, and the unified
playerOrUnitparameter keeps the implementation simple.Also applies to: 10-20
27-47: Factory creation logic is correct.The branching properly handles both cases:
- When given a Unit (lines 28-30): directly assigns and creates station
- When given a Player (lines 32-46): validates with
canBuildthen callsbuildUnitonceThe single validation→creation flow aligns with the PR objective to prevent double gold deduction.
tests/nukes/HydrogenAndMirv.test.ts (1)
1-72: Gold deduction is thoroughly tested intests/economy/ConstructionGold.test.ts. The test "City charges gold once and no refund thereafter (allow passive income)" verifies all three scenarios: gold is deducted, no double-charging occurs, and no refunds happen after construction. The HydrogenAndMirv tests are correctly focused on unit creation flow only.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/nukes/HydrogenAndMirv.test.ts (4)
46-49: Remove obsolete comments.These comments describe a test scenario that's now fully implemented in test 2 (lines 51-174). Remove them to avoid confusion.
- - // Now build another silo with construction time and ensure it won't be used - // Use non-instant config by simulating an under-construction flag on a new silo - // (Use normal construction with default duration in a fresh game instance)
127-129: Use idiomatic assertion.The assertion works but is verbose. Use
.toBeGreaterThanOrEqual()for clearer intent.- // Assert no refunds during construction - const goldDuringConstruction = playerWithConstruction.gold(); - expect(goldDuringConstruction >= goldAfterConstruction).toBe(true); + // Assert no refunds during construction (tolerates passive income) + const goldDuringConstruction = playerWithConstruction.gold(); + expect(goldDuringConstruction).toBeGreaterThanOrEqual(goldAfterConstruction);
150-154: Use idiomatic assertion.Checking
.not.toBe(false)works but.toBeTruthy()is more conventional for checking thatcanBuildreturned a TileRef.const canBuildAfterCompletion = playerWithConstruction.canBuild( UnitType.HydrogenBomb, targetTile, ); - expect(canBuildAfterCompletion).not.toBe(false); + expect(canBuildAfterCompletion).toBeTruthy();
192-195: Simplify assertion for clarity.Asserting on the sum is clearer than checking a boolean expression equals true.
- // MIRV should appear briefly before separation, otherwise warheads should be queued - const mirvs = player.units(UnitType.MIRV).length; - const warheads = player.units(UnitType.MIRVWarhead).length; - expect(mirvs > 0 || warheads > 0).toBe(true); + // MIRV should appear briefly before separation, otherwise warheads should be queued + const mirvs = player.units(UnitType.MIRV).length; + const warheads = player.units(UnitType.MIRVWarhead).length; + expect(mirvs + warheads).toBeGreaterThan(0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/nukes/HydrogenAndMirv.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: Egraveline
Repo: openfrontio/OpenFrontIO PR: 1012
File: src/core/execution/UpgradeStructureExecution.ts:49-54
Timestamp: 2025-06-05T02:34:45.899Z
Learning: In the upgrade system, gold deduction for structure upgrades is handled internally by the `upgradeUnit` method in PlayerImpl, not in the UpgradeStructureExecution class. The UpgradeStructureExecution only needs to check if the player has sufficient gold before calling `upgradeUnit`.
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.
📚 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/nukes/HydrogenAndMirv.test.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:
tests/nukes/HydrogenAndMirv.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/nukes/HydrogenAndMirv.test.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:
tests/nukes/HydrogenAndMirv.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/nukes/HydrogenAndMirv.test.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:
tests/nukes/HydrogenAndMirv.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/nukes/HydrogenAndMirv.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/nukes/HydrogenAndMirv.test.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
tests/nukes/HydrogenAndMirv.test.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:
tests/nukes/HydrogenAndMirv.test.ts
🧬 Code graph analysis (1)
tests/nukes/HydrogenAndMirv.test.ts (6)
src/core/game/Game.ts (3)
Game(660-744)Player(521-658)PlayerInfo(402-416)tests/util/Setup.ts (1)
setup(23-81)src/core/game/UnitImpl.ts (2)
info(182-184)targetTile(350-352)src/core/game/PlayerImpl.ts (2)
info(335-337)target(527-530)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(7-61)src/core/execution/ConstructionExecution.ts (1)
ConstructionExecution(13-167)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/client/graphics/layers/StructureDrawingUtils.ts(1 hunks)src/client/graphics/layers/StructureIconsLayer.ts(5 hunks)src/core/configuration/DefaultConfig.ts(0 hunks)src/core/execution/PortExecution.ts(5 hunks)src/core/execution/SAMLauncherExecution.ts(1 hunks)src/core/game/Game.ts(1 hunks)src/core/game/GameUpdates.ts(1 hunks)src/core/game/PlayerImpl.ts(3 hunks)
💤 Files with no reviewable changes (1)
- src/core/configuration/DefaultConfig.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/graphics/layers/StructureDrawingUtils.ts
- src/core/game/GameUpdates.ts
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: Egraveline
Repo: openfrontio/OpenFrontIO PR: 1012
File: src/core/execution/UpgradeStructureExecution.ts:49-54
Timestamp: 2025-06-05T02:34:45.899Z
Learning: In the upgrade system, gold deduction for structure upgrades is handled internally by the `upgradeUnit` method in PlayerImpl, not in the UpgradeStructureExecution class. The UpgradeStructureExecution only needs to check if the player has sufficient gold before calling `upgradeUnit`.
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.
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/core/execution/PortExecution.tssrc/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/execution/PortExecution.tssrc/core/execution/SAMLauncherExecution.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/PortExecution.tssrc/core/game/PlayerImpl.tssrc/core/execution/SAMLauncherExecution.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/PortExecution.tssrc/core/game/PlayerImpl.tssrc/core/execution/SAMLauncherExecution.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/PortExecution.tssrc/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.tssrc/core/game/Game.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
📚 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/game/PlayerImpl.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/SAMLauncherExecution.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/core/game/Game.ts
📚 Learning: 2025-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/game/Game.ts
📚 Learning: 2025-08-14T14:05:00.867Z
Learnt from: Dimitrije-V
Repo: openfrontio/OpenFrontIO PR: 1814
File: src/client/graphics/layers/PlayerActionHandler.ts:0-0
Timestamp: 2025-08-14T14:05:00.867Z
Learning: In TypeScript, when dealing with union types like NukeType (derived from UnitType), it's better to accept the broader type (UnitType) as a parameter and use runtime type checking rather than trying to narrow the parameter type at compile time. The approach of keeping flexible input parameters with runtime validation followed by properly typed state/storage is more practical than over-constraining function signatures.
Applied to files:
src/core/game/Game.ts
🧬 Code graph analysis (2)
src/core/execution/PortExecution.ts (2)
src/core/game/Game.ts (3)
Unit(428-506)Player(521-657)isUnit(418-426)src/core/execution/TradeShipExecution.ts (1)
TradeShipExecution(15-181)
src/core/game/PlayerImpl.ts (1)
src/core/game/UnitImpl.ts (1)
type(145-147)
🪛 GitHub Actions: 🧪 CI
src/core/execution/PortExecution.ts
[error] 73-73: TS2339: Property 'tradingPorts' does not exist on type 'Player'.
[error] 81-81: TS2345: Argument of type 'unknown' is not assignable to parameter of type 'Unit'.
🔇 Additional comments (9)
src/client/graphics/layers/StructureIconsLayer.ts (1)
472-472: Construction state refactor looks solid.The shift from
UnitType.ConstructiontoisUnderConstruction()is clean and consistent across all usages in this layer. Structure type resolution now uses the actual unit type directly, and construction state is tracked via the boolean flag as intended.Also applies to: 485-485, 526-526, 574-574, 628-628
src/core/execution/SAMLauncherExecution.ts (1)
219-221: Good placement for construction guard.The early return correctly prevents the SAM from targeting or intercepting while under construction. Placing it after
targetingSysteminitialization but before cooldown checks makes sense.src/core/game/Game.ts (1)
494-496: Cleaner construction state API.Replacing
constructionType(): UnitType | nullwith a simple boolean flagisUnderConstruction()is more straightforward and easier to reason about.src/core/game/PlayerImpl.ts (2)
231-232: Correct construction counting.The logic correctly identifies units of the specified type that are under construction.
1000-1003: Correct filtering for nuke spawns.The filter correctly ensures only active, completed missile silos can launch nukes by excluding those on cooldown or under construction.
src/core/execution/PortExecution.ts (4)
14-24: Type-safe constructor overloads.The overloads correctly enforce that a
TileRefis required when passing aPlayer, while making it optional for aUnit. The runtime validation at lines 21-23 provides a clear error message if the constraint is violated.
37-52: Clean dual initialization paths.The branching logic correctly handles both unit-based and player-based initialization, with appropriate validation and station creation in each path.
60-62: Consistent construction gate.The early return for under-construction ports aligns with the pattern used in other execution classes.
95-98: Correct ownership references.Using
this.port!.owner()for all port owner queries is consistent with the new ownership model.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/execution/PortExecution.ts (1)
125-157: Filter out ports under construction from trade destinations.The method returns all ports from other players without checking
isUnderConstruction(). If an under-construction port is selected as a trade destination, the trade ship may fail or behave incorrectly.Apply this filter to exclude ports under construction:
tradingPorts(): Unit[] { const ports = this.mg .players() .filter((p) => p !== this.port!.owner() && p.canTrade(this.port!.owner())) .flatMap((p) => p.units(UnitType.Port)) + .filter((port) => !port.isUnderConstruction()) .sort((p1, p2) => { return ( this.mg.manhattanDist(this.port!.tile(), p1.tile()) - this.mg.manhattanDist(this.port!.tile(), p2.tile()) ); });Additionally, in
createStation()at line 112-116, verify whetherhasUnitNearby()should also filter out factories under construction, as a factory under construction logically shouldn't enable a train station.
🧹 Nitpick comments (1)
src/client/graphics/layers/TerritoryLayer.ts (1)
92-92: Consider clarifying the comment.The comment mentions "barrier creation," but the code skips border update logic (BFS traversal and tile enqueueing for repaint). A more precise comment would improve readability.
Consider this wording:
- // Only update borders if the defense post is not under construction + // Skip border updates for DefensePosts under construction
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/client/graphics/layers/TerritoryLayer.ts(1 hunks)src/core/execution/PortExecution.ts(5 hunks)src/core/game/Game.ts(2 hunks)src/core/game/GameImpl.ts(1 hunks)src/core/game/PlayerImpl.ts(3 hunks)src/core/game/UnitGrid.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/game/Game.ts
- src/core/game/PlayerImpl.ts
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: Egraveline
Repo: openfrontio/OpenFrontIO PR: 1012
File: src/core/execution/UpgradeStructureExecution.ts:49-54
Timestamp: 2025-06-05T02:34:45.899Z
Learning: In the upgrade system, gold deduction for structure upgrades is handled internally by the `upgradeUnit` method in PlayerImpl, not in the UpgradeStructureExecution class. The UpgradeStructureExecution only needs to check if the player has sufficient gold before calling `upgradeUnit`.
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.
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/client/graphics/layers/TerritoryLayer.tssrc/core/execution/PortExecution.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/client/graphics/layers/TerritoryLayer.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/TerritoryLayer.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:
src/client/graphics/layers/TerritoryLayer.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/GameImpl.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/core/execution/PortExecution.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/PortExecution.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/PortExecution.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/PortExecution.ts
🧬 Code graph analysis (4)
src/client/graphics/layers/TerritoryLayer.ts (1)
src/core/game/GameView.ts (2)
update(76-80)update(495-556)
src/core/game/GameImpl.ts (3)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/game/UnitGrid.ts (1)
UnitPredicate(5-8)src/core/game/Game.ts (1)
Unit(428-506)
src/core/game/UnitGrid.ts (2)
src/core/game/GameView.ts (2)
unit(658-660)tile(106-108)src/core/game/UnitImpl.ts (1)
tile(175-177)
src/core/execution/PortExecution.ts (2)
src/core/game/Game.ts (3)
Unit(428-506)Player(521-657)isUnit(418-426)src/core/execution/TradeShipExecution.ts (1)
TradeShipExecution(15-181)
🔇 Additional comments (9)
src/core/game/UnitGrid.ts (3)
135-170: LGTM! Clean filtering logic for construction units.The default behavior of excluding under-construction units makes sense for most queries (finding targets, checking threats, etc.). The parameter allows callers to include them when needed (e.g., spacing validation).
172-192: Filtering logic matches nearbyUnits behavior.The early-return pattern keeps the logic clean and consistent with
nearbyUnits.
195-227: LGTM! Correct delegation to unitIsInRange.The parameter is properly propagated to
unitIsInRange, which handles the actual filtering. Clean separation of concerns.src/core/game/GameImpl.ts (2)
766-780: LGTM! Correct propagation to UnitGrid.The optional parameter is properly passed through to
unitGrid.hasUnitNearby, whereundefinedwill default tofalse.
782-799: LGTM! Parameter properly threaded through to UnitGrid.The optional
includeUnderConstructionparameter is correctly passed to the underlyingunitGrid.nearbyUnitscall.src/client/graphics/layers/TerritoryLayer.ts (1)
92-95: Early return logic is correct.The property name
update.underConstructionis verified on theUnitUpdateinterface as an optional boolean field. The early return properly skips border updates for under-construction DefensePosts, which aligns with the PR's goal of centralizing construction state handling and preventing gameplay effects during construction.src/core/execution/PortExecution.ts (3)
14-24: LGTM! Constructor overloads properly enforce type safety.The overloads ensure that a
TileRefmust be provided when constructing with aPlayer, while making it optional when constructing with aUnit. The runtime check at lines 21-23 provides an additional safety net.
60-62: LGTM! Construction check correctly pauses execution.The check properly prevents trade ship spawning while the port is under construction.
95-98: The suggested fix is incorrect based on method implementations.The review assumes
unitsConstructed()excludes units under construction, but inspection shows it actually includes them. TheunitsConstructed()method returns both pre-constructed units fromnumUnitsConstructed[type]and units currently under construction (whereisUnderConstruction()is true). Switching to this method would not fix the spawn rate concern—it would produce the same or potentially higher counts.Additionally,
unitsOwned()has a logic issue: it lacks a type filter, making it unsuitable for this use case.The current code using
unitCount()appears intentional for spawn rate calculations. If the concern is valid, clarify game design intent with the team before suggesting specific method changes.Likely an incorrect or invalid review comment.
evanpelle
reviewed
Nov 8, 2025
Collaborator
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
looks good! just a few comments.
| onUnitEvent(unit: UnitView) { | ||
| const underConst = | ||
| ( | ||
| unit as unknown as { isUnderConstruction?: () => boolean } |
Collaborator
There was a problem hiding this comment.
why do we need to do this? why not just unit.isUnderConstruction?
| ); | ||
| const underConst = | ||
| ( | ||
| unit as unknown as { isUnderConstruction?: () => boolean } |
| private player: Player, | ||
| private tile: TileRef, | ||
| ) {} | ||
| private playerOrUnit: Player | Unit, |
Collaborator
There was a problem hiding this comment.
is it possible to remove the playerOrUnit and just require unit to passed in?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
Tests updated
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
CrackeRR1