More Lobby Options (Ceasefire, Starting Gold, Gold Multiplier) #2458
Conversation
WalkthroughAdds spawn-immunity duration, starting-gold presets, and a gold multiplier across UI, server config/schema, simulation logic, rendering, and localization; integrates new fields into start/update payloads and enforces immunity gates at runtime. Changes
Sequence Diagram(s)sequenceDiagram
actor Player
participant Renderer as GameRenderer
participant Ceasefire as CeasefireTimer
participant GameCfg as GameConfig
participant Warship as WarshipExecution
Player->>Renderer: frame tick
Renderer->>Ceasefire: tick()
Ceasefire->>GameCfg: read spawnImmunityDuration & ticks
alt within immunity window
Ceasefire->>Ceasefire: update progressRatio
Ceasefire->>Renderer: render progress bar
else outside immunity
Ceasefire->>Renderer: render nothing
end
Renderer->>Warship: tick()
Warship->>GameCfg: isSpawnImmunityActive?
alt spawn immunity active
Warship->>Warship: clear/skip target & shooting
else inactive
Warship->>Warship: select target and shoot
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
src/core/execution/TransportShipExecution.ts (1)
7-7: Spawn‑immunity guard for transport ships vs humans looks good; consider small cleanupThe new check correctly prevents starting a naval attack on a human player while
numSpawnPhaseTurns + spawnImmunityDurationis still in effect, and it runs early enough that no resources are consumed. That matches the ceasefire idea.Two small follow‑ups you might consider:
- You can rely on
this.target.isPlayer()as a type guard and avoid the explicit cast (as Player) for slightly cleaner TypeScript.- To avoid a “nothing happened” feel for the attacker, consider surfacing a short message when the attack is blocked by ceasefire, similar to the existing “no boats available” feedback.
Also applies to: 92-103
src/server/MapPlaylist.ts (1)
96-98: Playlist defaults for new config fields are reasonableThe public playlist now sets
startingGold: 0,goldMultiplier: 1, and a shortspawnImmunityDuration: 5 * 10, which is a sensible baseline and satisfies the extendedGameConfig.If GameManager/DefaultConfig ever change these defaults, it may be worth centralizing them to avoid drift, but this matches the current pattern in this file.
Also applies to: 105-105
src/client/graphics/GameRenderer.ts (1)
11-11: CeasefireTimer layer wiring matches existing HUD patternImporting
CeasefireTimer, binding it viaquerySelector("ceasefire-timer"), and inserting it intolayersright afterspawnTimershould make the ceasefire UI behave like other HUD elements and render each frame as expected.Just like the other DOM lookups here, this assumes the
<ceasefire-timer>element is always present; if you ever need to runcreateRendererin a context without that element, consider guarding the assignment (ceasefireTimer.game = game) behind a successfulinstanceofcheck.Also applies to: 236-243, 272-272
src/core/execution/WarshipExecution.ts (1)
68-77: Warship ceasefire logic is sound; confirm that global immunity is desiredThe new
spawnImmunityActiveflow keeps warships from acquiring or shooting at targets during the initial window, while still allowing them to patrol. The helperisSpawnImmunityActive()captures the same “spawn phase + extra immunity” condition used elsewhere, which keeps the timing consistent.One design point to double‑check: here the immunity is global (warships won’t attack any target while active), whereas the transport‑ship guard only blocks attacks when the target is a human player. If the ceasefire is meant to cover all early naval aggression, this is fine; if it should only protect humans, you may want to narrow the condition similarly.
If you find yourself tuning this condition more, consider sharing a small helper (or utility function) so the same spawn‑immunity rule is not re‑encoded in multiple execution classes.
Also applies to: 81-82, 292-298
src/core/configuration/DefaultConfig.ts (1)
35-36: Gold multiplier and startingGold wiring look good; a few small edge notes
GOLD_MULTIPLIER_PRECISION = 1000n+ clampinggoldMultiplier()to[0, 10]gives predictable integer math and matches the UI rounding to 3 decimals.applyGoldMultipliercorrectly handles bothbigintandnumberinputs and short‑circuits on0.- Routing
trainGold,tradeShipGold, andgoldAdditionRatethroughapplyGoldMultipliercentralizes the scaling logic and keeps the rest of the code simple.startingGold(playerInfo)only applies toPlayerType.Human, which matches the “does not apply to nations” note.Two minor things to keep in mind (no required change):
- If any caller ever passes a
numberamount larger thanNumber.MAX_SAFE_INTEGERintoapplyGoldMultiplier,Math.floorwill lose precision. Today your callers all generate relatively small amounts, so this is fine.- If you ever want non‑human players to respect
startingGold, that will need an intentional change here; right now bots/FakeHumans always start at0n.Overall, this multiplier / starting gold layer is clean and idiomatic for the existing codebase.
Also applies to: 282-300, 382-397, 409-418, 923-926, 853-858
src/client/SinglePlayerModal.ts (1)
48-49: Numeric input handling is solid; tiny sanitization nitThe new
startingGoldandgoldMultiplierstate plus inputs are wired cleanly:
- Ranges match the core config (
0–100000000and0–10).- Keydown handlers block scientific notation and invalid signs.
- Values are clamped and, for
goldMultiplier, rounded to 3 decimals to match the server.One small edge case:
handleStartingGoldChangestrips[eE+-]but not".", so a pasted value like"1.5"becomesparseInt("1.5", 10) === 1. Probably harmless, but you can align it with the keydown rule by also stripping dots:- input.value = input.value.replace(/[eE+-]/g, ""); + input.value = input.value.replace(/[eE+-.]/g, "");Same change could be mirrored in the host modal for consistency.
Also applies to: 315-351, 503-518, 519-535
src/client/HostLobbyModal.ts (1)
45-56: Host-side ceasefire / gold options are wired cleanlyThe new host options look consistent end‑to‑end:
- State defaults (
spawnImmunityDurationSeconds = 5,startingGold = 0,goldMultiplier = 1) match the server defaults and schema ranges.- Inputs clamp to the same ranges as the backend:
0–300seconds,0–100000000starting gold,0–10multiplier with 3 decimal precision.- Keydown handlers block scientific notation and invalid signs; multipliers are rounded to 3 decimals to line up with
GOLD_MULTIPLIER_PRECISION = 1000n.putGameConfig()sends:spawnImmunityDuration: this.spawnImmunityDurationSeconds * 10, startingGold: this.startingGold, goldMultiplier: this.goldMultiplier,which matches the server
GameConfig(ticks for duration, numbers for gold fields).Same tiny nit as in the single‑player modal:
handleStartingGoldInputstrips[eE+-]but not".", so a pasted"1.5"becomes1. You can make it stricter with:- input.value = input.value.replace(/[eE+-]/g, ""); + input.value = input.value.replace(/[eE+-.]/g, "");Optional, but keeps behavior uniform across both modals.
Also applies to: 530-587, 755-812, 873-900
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
resources/lang/debug.json(2 hunks)resources/lang/en.json(2 hunks)src/client/HostLobbyModal.ts(8 hunks)src/client/SinglePlayerModal.ts(4 hunks)src/client/graphics/GameRenderer.ts(3 hunks)src/client/graphics/layers/CeasefireTimer.ts(1 hunks)src/client/index.html(1 hunks)src/core/Schemas.ts(1 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(7 hunks)src/core/execution/AttackExecution.ts(1 hunks)src/core/execution/TransportShipExecution.ts(2 hunks)src/core/execution/WarshipExecution.ts(2 hunks)src/core/game/PlayerImpl.ts(4 hunks)src/core/game/TransportShipUtils.ts(1 hunks)src/server/GameManager.ts(1 hunks)src/server/GameServer.ts(1 hunks)src/server/MapPlaylist.ts(1 hunks)tests/util/Setup.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (25)
📓 Common learnings
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/configuration/Config.tssrc/core/game/PlayerImpl.tssrc/core/configuration/DefaultConfig.tstests/util/Setup.tssrc/server/GameManager.tssrc/core/Schemas.tssrc/server/GameServer.tssrc/core/game/TransportShipUtils.tssrc/client/HostLobbyModal.tssrc/server/MapPlaylist.tssrc/client/SinglePlayerModal.ts
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.
Applied to files:
src/core/configuration/Config.tssrc/core/game/PlayerImpl.tssrc/client/HostLobbyModal.tssrc/client/SinglePlayerModal.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/configuration/Config.tssrc/core/execution/AttackExecution.tssrc/core/game/PlayerImpl.tssrc/core/configuration/DefaultConfig.tstests/util/Setup.tssrc/server/GameManager.tssrc/core/Schemas.tssrc/client/HostLobbyModal.tssrc/core/execution/WarshipExecution.tssrc/core/execution/TransportShipExecution.tssrc/server/MapPlaylist.tssrc/client/graphics/layers/CeasefireTimer.tssrc/client/SinglePlayerModal.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/AttackExecution.tssrc/core/game/PlayerImpl.tssrc/core/execution/WarshipExecution.tssrc/core/execution/TransportShipExecution.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/AttackExecution.tssrc/core/execution/WarshipExecution.tssrc/core/execution/TransportShipExecution.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/AttackExecution.tssrc/core/execution/TransportShipExecution.ts
📚 Learning: 2025-08-23T08:03:44.914Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:547-592
Timestamp: 2025-08-23T08:03:44.914Z
Learning: In OpenFrontIO, FakeHumanExecution is used for AI-controlled nations that simulate human behavior, which is distinct from PlayerType.Bot. FakeHumanExecution players are not PlayerType.Bot - they represent more sophisticated AI that mimics human nation behavior, while PlayerType.Bot represents basic AI players.
Applied to files:
src/core/execution/AttackExecution.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/AttackExecution.tssrc/core/execution/WarshipExecution.ts
📚 Learning: 2025-11-01T00:24:33.860Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2161
File: src/core/execution/FakeHumanExecution.ts:670-678
Timestamp: 2025-11-01T00:24:33.860Z
Learning: In OpenFrontIO, PlayerType.Bot entities cannot be in teams and do not have friendliness relationships. Only PlayerType.Human and PlayerType.FakeHuman can participate in teams and alliances. Therefore, when targeting bot-owned tiles, friendliness checks like `owner.isFriendly(this.player)` are unnecessary and meaningless for bots.
Applied to files:
src/core/execution/AttackExecution.tssrc/core/game/PlayerImpl.ts
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.jsonresources/lang/debug.json
📚 Learning: 2025-08-16T10:52:08.292Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.
Applied to files:
resources/lang/en.jsonresources/lang/debug.json
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/index.htmlsrc/client/HostLobbyModal.tssrc/client/graphics/layers/CeasefireTimer.tssrc/client/SinglePlayerModal.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.tssrc/core/configuration/DefaultConfig.tssrc/client/SinglePlayerModal.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.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/TransportShipExecution.ts
📚 Learning: 2025-11-08T16:57:19.090Z
Learnt from: mepoohsta
Repo: openfrontio/OpenFrontIO PR: 2413
File: src/core/game/RailNetworkImpl.ts:298-311
Timestamp: 2025-11-08T16:57:19.090Z
Learning: In src/core/game/RailNetworkImpl.ts, the getDestinationWeight method calculates route profitability (not just length) for train destination selection. The heavy relationship bonuses (ally +50, team/other +25, self +10) compared to the base +1 per station are intentional, as profitability considers both route length and player relationships. Longer routes are usually more profitable, but shorter routes to allies can be more valuable.
Applied to files:
src/core/configuration/DefaultConfig.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/configuration/DefaultConfig.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/configuration/DefaultConfig.ts
📚 Learning: 2025-05-30T03:53:52.231Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 949
File: resources/lang/en.json:8-10
Timestamp: 2025-05-30T03:53:52.231Z
Learning: For the OpenFrontIO project, do not suggest updating translation files in resources/lang/*.json except for en.json. The project has a dedicated translation team that handles all other locale files.
Applied to files:
resources/lang/debug.json
📚 Learning: 2025-08-27T08:12:19.610Z
Learnt from: mokizzz
Repo: openfrontio/OpenFrontIO PR: 1940
File: resources/lang/en.json:763-766
Timestamp: 2025-08-27T08:12:19.610Z
Learning: In OpenFrontIO, some country entries in src/client/data/countries.json may have similar names but different codes (e.g., "Empire of Japan" vs "Empire of Japan1"). Each unique code requires its own translation key in resources/lang/en.json after normalization. Always verify against countries.json before suggesting removal of translation keys.
Applied to files:
resources/lang/debug.json
📚 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/GameRenderer.tssrc/client/graphics/layers/CeasefireTimer.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as `z.infer<typeof PlayerStatsSchema>` where PlayerStatsSchema has `.optional()` applied at the object level, making PlayerStats a union type that already includes undefined (PlayerStats | undefined).
Applied to files:
src/core/Schemas.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as a type inferred from a Zod schema that is marked as optional, which means PlayerStats already includes undefined as a possible type (PlayerStats | undefined).
Applied to files:
src/core/Schemas.tssrc/core/execution/TransportShipExecution.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/HostLobbyModal.tssrc/server/MapPlaylist.tssrc/client/SinglePlayerModal.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/core/execution/TransportShipExecution.ts
🧬 Code graph analysis (11)
src/core/configuration/Config.ts (2)
tests/util/Setup.ts (1)
playerInfo(86-88)src/core/game/Game.ts (2)
PlayerInfo(406-420)Gold(24-24)
src/core/execution/AttackExecution.ts (2)
src/core/Schemas.ts (1)
Player(117-117)src/core/game/Game.ts (1)
Player(525-661)
src/core/game/PlayerImpl.ts (2)
tests/util/Setup.ts (1)
playerInfo(86-88)src/core/game/GameImpl.ts (1)
owner(183-185)
src/core/configuration/DefaultConfig.ts (3)
src/core/Util.ts (1)
within(37-39)src/core/game/Game.ts (2)
Gold(24-24)PlayerInfo(406-420)tests/util/Setup.ts (1)
playerInfo(86-88)
src/server/GameServer.ts (1)
src/server/MapPlaylist.ts (1)
gameConfig(79-108)
src/core/game/TransportShipUtils.ts (2)
src/core/game/Game.ts (2)
Game(663-747)Player(525-661)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/client/HostLobbyModal.ts (1)
src/client/Utils.ts (1)
translateText(92-147)
src/core/execution/TransportShipExecution.ts (1)
src/core/game/Game.ts (1)
Player(525-661)
src/server/MapPlaylist.ts (2)
src/core/configuration/DefaultConfig.ts (1)
playerTeams(343-345)src/core/game/Game.ts (1)
HumansVsNations(56-56)
src/client/graphics/layers/CeasefireTimer.ts (2)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/game/GameView.ts (1)
GameView(448-774)
src/client/SinglePlayerModal.ts (1)
src/client/Utils.ts (1)
translateText(92-147)
🔇 Additional comments (13)
resources/lang/debug.json (1)
99-100: LGTM!The debug translation keys are properly added for the new lobby options. The keys follow the existing pattern and are correctly namespaced within
single_modalandhost_modalsections.Also applies to: 150-152
resources/lang/en.json (1)
148-149: LGTM!The English translations are clear and user-friendly. The spawn immunity label helpfully specifies "(seconds)" to clarify the unit of measurement.
Also applies to: 254-256
src/client/index.html (1)
407-407: LGTM!The ceasefire timer element is correctly placed after the spawn timer, which makes logical sense as they're related countdown displays.
src/server/GameServer.ts (1)
121-129: LGTM!The config update handling for the three new fields follows the established pattern consistently. The undefined checks ensure values are only updated when explicitly provided.
src/core/game/TransportShipUtils.ts (1)
11-19: LGTM!The spawn immunity check correctly prevents transport ship attacks on human players during the immunity window. The early return placement is efficient, and targeting only
PlayerType.Humanis appropriate.src/core/configuration/Config.ts (1)
97-97: LGTM!The new
startingGoldmethod signature is well-placed alongside other starting resource methods. TakingPlayerInfoas a parameter allows for flexible per-player starting gold configuration.src/core/execution/AttackExecution.ts (1)
94-99: LGTM!The refactored immunity check now correctly targets only
PlayerType.Humanplayers, which is consistent with the protection logic inTransportShipUtils.ts. ExtractingtargetPlayeras a constant improves readability and avoids duplicate type casts.src/server/GameManager.ts (1)
57-58: LGTM!The default values are sensible:
startingGold: 0maintains standard gameplay (no starting bonus)goldMultiplier: 1keeps income unchanged by defaultspawnImmunityDuration: 5 * 10clearly expresses 5 seconds in ticksAlso applies to: 64-64
src/core/Schemas.ts (1)
173-175: New GameConfig fields and limits look consistent
startingGold,goldMultiplier, andspawnImmunityDurationare wired intoGameConfigSchemawith sensible numeric ranges and keep the config surface in sync with the rest of the PR.tests/util/Setup.ts (1)
67-72: Test GameConfig now covers new fields with safe defaultsSetting
spawnImmunityDuration: 0,startingGold: 0, andgoldMultiplier: 1in the base test config (then spreading_gameConfigafterward) keeps old tests stable and lets new tests override as needed.src/core/game/PlayerImpl.ts (2)
107-119: Config-driven starting gold looks correctUsing
this.mg.config().startingGold(playerInfo)to seed_goldis a clean way to hook into the new lobby option. Types line up (Gold→bigint) and this stays server‑authoritative.
994-1001: Inconsistency confirmed betweennukeSpawnandcanAttackceasefire behaviorThe review correctly identifies a real difference:
nukeSpawn(line 995): Blocks on ANY owner type globallycanAttack(line 1189–1190): Only blocks whenowner.isPlayer() && owner.type() === PlayerType.HumanThis means during ceasefire, ground attacks on bots and TerraNullius tiles are allowed, while nukes are blocked for everyone. The human-only filter in
canAttackappears intentional (similar pattern used incanDonateTroopsandcanDonateGold), but the difference between the two attack methods is not documented.The review provides two reasonable options:
- Global ceasefire: Remove the human-only filter from
canAttackto matchnukeSpawnbehavior- Human-only ceasefire: Keep current code but add a comment explaining why bots/TerraNullius can be attacked during spawn phase
Which behavior matches the PR intent needs clarification from the author.
src/client/SinglePlayerModal.ts (1)
660-663: Config payload matches schema; confirm fixed single‑player ceasefireIncluding
startingGold: this.startingGold, goldMultiplier: this.goldMultiplier, spawnImmunityDuration: 5 * 10,in the single‑player
gameStartInfo.configlooks correct and matches the server schema.One design question: host lobbies can tune
spawnImmunityDuration, but single‑player always sends5 * 10. If you want behavior parity between SP and MP, consider exposing the same ceasefire option here later; otherwise, a short comment noting “SP ceasefire is fixed at 5 seconds” would make the intent clear.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/client/HostLobbyModal.ts (3)
26-26: Consider removing unused import.The Button component is imported but doesn't appear to be used anywhere in the template. If it's not needed, removing it keeps imports clean.
782-798: Make paste handling consistent with keydown prevention.The keydown handler (line 783) prevents "." for integer input, but the paste handler (line 790) only removes
/[eE+-]/g. If someone pastes a value with a decimal point, it won't be stripped.For consistency, update the regex:
- input.value = input.value.replace(/[eE+-]/g, ""); + input.value = input.value.replace(/[eE+-.]/g, "");This keeps the paste and keydown behaviors aligned.
800-816: Make paste handling consistent with keydown prevention.The keydown handler (line 801) prevents "+", "-" but the paste handler (line 808) only removes
/[eE]/g. If someone pastes "+5" or "-5", the signs won't be stripped (thoughparseFloatand clamping will handle them).For consistency:
- input.value = input.value.replace(/[eE]/g, ""); + input.value = input.value.replace(/[eE+-]/g, "");This aligns paste handling with keydown prevention.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/HostLobbyModal.ts(8 hunks)src/client/graphics/layers/CeasefireTimer.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/CeasefireTimer.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-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/client/HostLobbyModal.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.
Applied to files:
src/client/HostLobbyModal.ts
🧬 Code graph analysis (1)
src/client/HostLobbyModal.ts (1)
src/client/Utils.ts (1)
translateText(92-147)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/client/utilities/StartingGoldPresets.ts (1)
14-31: Simplify the closest-preset search.The
forEachloop with manual tracking can be replaced with a simplerreducefor better clarity and functional style.export function startingGoldIndexFromValue(value: number): number { const normalizedValue = Math.max( 0, Math.min(value, STARTING_GOLD_PRESETS[STARTING_GOLD_PRESETS.length - 1]), ); - let closestIndex = 0; - let smallestDelta = Number.POSITIVE_INFINITY; - - STARTING_GOLD_PRESETS.forEach((preset, index) => { - const delta = Math.abs(preset - normalizedValue); - if (delta < smallestDelta) { - smallestDelta = delta; - closestIndex = index; - } - }); - - return closestIndex; + + return STARTING_GOLD_PRESETS.reduce((closestIdx, preset, idx) => { + const currentDelta = Math.abs(preset - normalizedValue); + const closestDelta = Math.abs(STARTING_GOLD_PRESETS[closestIdx] - normalizedValue); + return currentDelta < closestDelta ? idx : closestIdx; + }, 0); }src/client/HostLobbyModal.ts (3)
851-874: Consider debouncing the starting gold slider.Unlike the bot slider which debounces for 300ms, the starting gold slider calls
putGameConfigimmediately on every input event. This could generate many server requests if the user drags the slider. Consider adding the same debouncing pattern used for bots.private startingGoldUpdateTimer: number | null = null; private handleStartingGoldSliderChange(e: Event) { const slider = e.target as HTMLInputElement; this.updateSliderProgressElement(slider); const index = parseInt(slider.value, 10); if (Number.isNaN(index)) { return; } this.startingGold = startingGoldValueFromIndex(index); this.startingGoldEnabled = true; // Debounce like bots if (this.startingGoldUpdateTimer !== null) { clearTimeout(this.startingGoldUpdateTimer); } this.startingGoldUpdateTimer = window.setTimeout(() => { this.putGameConfig(); this.startingGoldUpdateTimer = null; }, 300); }Remember to clear the timer in
close()as well.
876-895: Consider debouncing the gold multiplier slider.Similar to the starting gold slider, this calls
putGameConfigimmediately on every input event. Consider adding debouncing to reduce server requests during slider dragging.
1019-1038: Consolidate duplicate progress calculation logic.
sliderStyleandupdateSliderProgressElementboth compute the slider progress percentage with identical logic. This duplication can be consolidated into a single helper that computes the percentage, which both methods can use.private computeSliderProgress(value: number, min: number, max: number): number { if (max === min) return 0; const percent = ((value - min) / (max - min)) * 100; return Math.max(0, Math.min(100, percent)); } private sliderStyle(value: number, min: number, max: number): string { const percent = this.computeSliderProgress(value, min, max); return `--progress:${percent}%`; } private updateSliderProgressElement(slider: HTMLInputElement): void { const min = Number(slider.min); const max = Number(slider.max); const value = Number(slider.value); if (Number.isNaN(min) || Number.isNaN(max)) { slider.style.setProperty("--progress", "0%"); return; } const percent = this.computeSliderProgress(value, min, max); slider.style.setProperty("--progress", `${percent}%`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
resources/lang/en.json(2 hunks)src/client/HostLobbyModal.ts(12 hunks)src/client/SinglePlayerModal.ts(9 hunks)src/client/styles.css(4 hunks)src/client/utilities/StartingGoldPresets.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/SinglePlayerModal.ts
- resources/lang/en.json
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Learnt from: 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-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-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/client/HostLobbyModal.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/client/HostLobbyModal.ts
🧬 Code graph analysis (1)
src/client/HostLobbyModal.ts (2)
src/client/Utils.ts (2)
translateText(92-147)renderNumber(19-43)src/client/utilities/StartingGoldPresets.ts (3)
STARTING_GOLD_PRESETS(1-4)startingGoldValueFromIndex(6-12)startingGoldIndexFromValue(14-31)
🔇 Additional comments (13)
src/client/utilities/StartingGoldPresets.ts (2)
1-4: LGTM!The preset values provide a good range from 0 to 10M with reasonable increments.
6-12: LGTM!The clamping logic correctly handles out-of-bounds indices.
src/client/styles.css (1)
236-281: LGTM!The refactor from ID selectors to a shared
.option-sliderclass improves reusability across multiple sliders (bots, spawn immunity, starting gold, gold multiplier). All vendor-specific pseudo-elements are updated consistently.src/client/HostLobbyModal.ts (10)
4-4: LGTM!The new imports and state fields are well-structured. Default values are sensible (5 seconds immunity, 0 starting gold, 1.0x multiplier).
Also applies to: 26-26, 32-36, 50-50, 59-62
349-353: LGTM!The bot slider correctly uses the new
.option-sliderclass andsliderStylehelper for consistent styling.
539-564: LGTM!The spawn immunity duration slider is well-implemented with a user-friendly clock display format and appropriate range (0-300 seconds in 5-second increments).
565-603: LGTM!The starting gold UI provides good UX with a toggle and conditional slider. Using preset indices ensures values snap to predefined amounts, and
renderNumberprovides clean display (e.g., "1M" instead of "1000000").
604-638: LGTM!The gold multiplier UI follows the same toggle pattern as starting gold. The 0-10x range with 0.1 precision is reasonable for gameplay balance.
798-818: LGTM!The bot slider handler correctly updates the visual progress and maintains the existing debouncing behavior to reduce server calls.
825-844: LGTM!The spawn immunity handler correctly snaps to 5-second increments and formats the display as a clock. The immediate
putGameConfigcall is acceptable since this control is adjusted less frequently than the bot slider.
728-747: LGTM!The
open()method correctly initializes the new fields from server config, including deriving the enabled flags from their values (startingGold > 0, goldMultiplier !== 1). This matches the addressed past review comment about keeping UI in sync.
981-983: LGTM!The new fields are correctly included in the config payload. The
spawnImmunityDurationis properly converted from seconds to deciseconds (× 10).
1010-1017: LGTM!The helper methods correctly normalize and clamp values. The
normalizeGoldMultiplierrounding to 1 decimal place matches the slider step of 0.1.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/client/SinglePlayerModal.ts (1)
647-673: Consider extracting shared helper methods to a utility module.The helper methods
normalizeGoldMultiplier,sliderStyle, andupdateSliderProgressElementare duplicated identically in HostLobbyModal.ts (lines 1014-1038). Extracting these to a shared utility (e.g.,src/client/utilities/SliderHelpers.ts) would reduce duplication and improve maintainability.src/client/HostLobbyModal.ts (1)
1097-1123: Consider simplifying the enabled flag updates.The conditional logic that only updates
startingGoldEnabledandgoldMultiplierEnabledwhen values change (lines 1104-1106, 1114-1116) adds defensive protection against race conditions. However, it creates inconsistency with the simpler unconditional approach inopen()(lines 734, 740).Since the enabled flags are purely UI-derived state (not server-authoritative), you could simplify by always computing them from the values, matching the
open()pattern:if (typeof data.gameConfig.startingGold === "number") { const snapped = this.snapStartingGoldValue(data.gameConfig.startingGold); - const startingGoldChanged = this.startingGold !== snapped; this.startingGold = snapped; - if (startingGoldChanged) { - this.startingGoldEnabled = this.startingGold !== 0; - } + this.startingGoldEnabled = this.startingGold !== 0; } if (typeof data.gameConfig.goldMultiplier === "number") { const normalized = this.normalizeGoldMultiplier(data.gameConfig.goldMultiplier); - const goldMultiplierChanged = this.goldMultiplier !== normalized; this.goldMultiplier = normalized; - if (goldMultiplierChanged) { - this.goldMultiplierEnabled = this.goldMultiplier !== 1; - } + this.goldMultiplierEnabled = this.goldMultiplier !== 1; }This reduces complexity while maintaining correctness, since the server is the source of truth for the values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
resources/lang/en.json(2 hunks)src/client/HostLobbyModal.ts(12 hunks)src/client/SinglePlayerModal.ts(9 hunks)src/core/Schemas.ts(1 hunks)src/server/GameServer.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/Schemas.ts
- resources/lang/en.json
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
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.
📚 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/server/GameServer.tssrc/client/SinglePlayerModal.tssrc/client/HostLobbyModal.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/server/GameServer.tssrc/client/SinglePlayerModal.tssrc/client/HostLobbyModal.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/SinglePlayerModal.tssrc/client/HostLobbyModal.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/SinglePlayerModal.tssrc/client/HostLobbyModal.ts
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.
Applied to files:
src/client/SinglePlayerModal.tssrc/client/HostLobbyModal.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/client/HostLobbyModal.ts
🧬 Code graph analysis (3)
src/server/GameServer.ts (2)
src/server/MapPlaylist.ts (1)
gameConfig(79-108)src/core/configuration/DefaultConfig.ts (1)
gameConfig(267-269)
src/client/SinglePlayerModal.ts (2)
src/client/utilities/StartingGoldPresets.ts (3)
STARTING_GOLD_PRESETS(1-4)startingGoldValueFromIndex(6-12)startingGoldIndexFromValue(14-31)src/client/Utils.ts (1)
renderNumber(19-43)
src/client/HostLobbyModal.ts (2)
src/client/Utils.ts (1)
renderNumber(19-43)src/client/utilities/StartingGoldPresets.ts (3)
STARTING_GOLD_PRESETS(1-4)startingGoldValueFromIndex(6-12)startingGoldIndexFromValue(14-31)
🔇 Additional comments (7)
src/server/GameServer.ts (1)
121-129: LGTM!The three new config fields follow the existing pattern perfectly. The implementation is consistent with how other fields are handled in this method.
src/client/SinglePlayerModal.ts (3)
30-36: LGTM!The imports and constant are well organized. The default spawn immunity duration of 5 seconds matches the server's expectation (5 * 10 ticks).
325-395: LGTM!The starting gold and gold multiplier UI controls are well structured. The use of toggle + conditional slider is intuitive, and the display formatting is appropriate for each type.
744-751: LGTM!The game config construction correctly uses the enabled flags and provides appropriate defaults. The conversion from seconds to ticks (×10) aligns with the server's expectations.
src/client/HostLobbyModal.ts (3)
728-747: LGTM!The server synchronization in
open()correctly initializes local state from the server's gameConfig. The enabled flags are properly derived from the values, ensuring the UI accurately reflects the server state.
825-836: LGTM!The spawn immunity duration handler correctly quantizes values to 5-second intervals. The slider snapping behavior (line 833) provides a clean user experience with predictable values.
838-844: LGTM!The
formatSecondsAsClockhelper provides clear time formatting for the spawn immunity duration. The implementation is clean and correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/utilities/StartingGoldPresets.ts (1)
16-33: LGTM! Consider early exit for exact matches.The closest-preset search logic is correct and handles edge cases properly. The implementation is clean and readable.
For a minor optimization, you could exit early when an exact match is found (delta = 0), though the performance gain is negligible given the small array size.
If you want to add the early exit:
STARTING_GOLD_PRESETS.forEach((preset, index) => { const delta = Math.abs(preset - normalizedValue); if (delta < smallestDelta) { smallestDelta = delta; closestIndex = index; + if (delta === 0) { + return; // Early exit on exact match + } } });Note:
returninforEachonly exits the current iteration, not the entire loop. For true early exit, you'd need a traditionalforloop orfor...ofwith a break statement. Given the small array size, the current approach is perfectly fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/utilities/StartingGoldPresets.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
🔇 Additional comments (2)
src/client/utilities/StartingGoldPresets.ts (2)
1-6: LGTM! Clean preset definition.The preset values are well-structured, use numeric separators for readability, and the
as constassertion ensures type safety. The progression from 0 to 100M provides good coverage for different game scenarios.
8-14: LGTM! Proper bounds checking.The index clamping ensures safe array access and handles all edge cases correctly. The implementation is simple and easy to understand.
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
I like these additional options, mind spliting them into 3 PRs?
sure, just for easier reviewing or do the PRs need to fully function on their own? There is some shared logic / layout for the sliders. if it's just about reviewing the logic independently then I'll base them on each other if that's okay |
That's fine, just so it's easier for me to review. |
Splitting up the starting gold / gold multiplier was not quite possible - those features are super similar, hope it's okay to review them both together. Let me know if not, then I'll untangle and rewrite the history. |
2 participants
Description:
PR implementing additional lobby options as requested on Discord (Thread)
Includes the following additional options when creating lobbies:
spawnDuration.Defaults are provided for public games / private games with these settings untouched so nothing changes unless the new options are explicitly used.
UI Screenshots
HostLobbyModal:
SinglePlayerModal (same as HostLobbyModal, but without spawn immunity setting):
Visual Ceasefire indicator (only active if duration is > 5 seconds):
Please complete the following:
I don't think tests are required for these features, I'm happy to implement them if a reviewer disagrees.
Please put your Discord username so you can be contacted if a bug or regression is found:
newyearnewphil / [CYN] super mario