Add player limit feature for private lobbies #2664
Conversation
WalkthroughAdds host-configurable per-lobby player limits (2–1000) with persistence and server enforcement, localized "lobby full" errors (with translationKey+args), client UI updates to display limits/warnings, a new CheckboxWithInput component, and schema updates for maxPlayers and server error payloads. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Host
participant HostUI as HostLobbyModal (Client)
participant Server as GameServer
participant Joiner as ClientGameRunner
participant JoinUI as JoinPrivateLobbyModal (Client)
Host->>HostUI: enable player limit & set value (e.g. 6)
HostUI->>Server: updateGameConfig { maxPlayers: 6 }
Server-->>HostUI: ack
Joiner->>Server: joinLobby request
alt slots available (current < max)
Server->>Joiner: join success
JoinUI->>JoinUI: show players count (X/6)
else full (current >= max)
Server-->>Joiner: error { translationKey: "private_lobby.lobby_full", args: { limit: 6 }, message }
Note right of Joiner #: `#f3f7ff`: Client prefers translationKey/args\nfalls back to raw message
Joiner->>Joiner: show localized "lobby full (limit: 6)" modal
Joiner->>Server: dispatch leave-lobby
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧠 Learnings (11)📓 Common learnings📚 Learning: 2025-12-26T22:21:21.904ZApplied to files:
📚 Learning: 2025-10-21T20:06:04.823ZApplied to files:
📚 Learning: 2025-10-26T15:37:07.732ZApplied to files:
📚 Learning: 2025-10-08T17:14:49.369ZApplied to files:
📚 Learning: 2025-06-09T02:20:43.637ZApplied to files:
📚 Learning: 2026-01-02T18:11:06.832ZApplied to files:
📚 Learning: 2025-10-08T17:14:49.369ZApplied to files:
📚 Learning: 2025-08-12T00:31:50.144ZApplied to files:
📚 Learning: 2025-08-17T20:48:49.411ZApplied to files:
📚 Learning: 2025-10-20T20:15:28.858ZApplied to files:
🧬 Code graph analysis (2)src/server/GameServer.ts (2)
src/client/HostLobbyModal.ts (2)
🔇 Additional comments (11)
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: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
resources/lang/debug.json(2 hunks)resources/lang/en.json(2 hunks)src/client/ClientGameRunner.ts(1 hunks)src/client/HostLobbyModal.ts(6 hunks)src/client/JoinPrivateLobbyModal.ts(4 hunks)src/core/Schemas.ts(1 hunks)src/server/GameServer.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (12)
📚 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/Schemas.tssrc/server/GameServer.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-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/Schemas.tssrc/client/ClientGameRunner.tssrc/server/GameServer.tssrc/client/HostLobbyModal.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as a type inferred from a Zod schema that is marked as optional, which means PlayerStats already includes undefined as a possible type (PlayerStats | undefined).
Applied to files:
src/core/Schemas.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/JoinPrivateLobbyModal.tssrc/client/ClientGameRunner.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/JoinPrivateLobbyModal.tssrc/client/ClientGameRunner.tssrc/server/GameServer.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: 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/client/ClientGameRunner.tssrc/server/GameServer.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/server/GameServer.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-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.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.json
📚 Learning: 2025-07-12T08:41:35.101Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1357
File: resources/lang/de.json:523-540
Timestamp: 2025-07-12T08:41:35.101Z
Learning: In OpenFrontIO project localization files, always check the en.json source file before flagging potential spelling errors in other language files, as some keys may intentionally use non-standard spellings that need to be consistent across all translations.
Applied to files:
resources/lang/en.json
🧬 Code graph analysis (2)
src/client/ClientGameRunner.ts (2)
src/client/LangSelector.ts (1)
translateText(258-278)src/client/Utils.ts (1)
translateText(92-147)
src/client/HostLobbyModal.ts (2)
src/client/LangSelector.ts (1)
translateText(258-278)src/client/Utils.ts (1)
translateText(92-147)
🔇 Additional comments (14)
src/server/GameServer.ts (2)
132-134: LGTM! Consistent pattern for config updates.The maxPlayers update follows the same conditional pattern used for all other config properties in this method. Clean and straightforward.
153-174: Enhanced lobby-full diagnostics look good.The updated error handling includes structured player count data (currentPlayers, maxPlayers) which the client parses to show user-friendly error messages. The JSON-stringified message field establishes a clear client-server contract that the client properly handles with try/catch blocks.
src/core/Schemas.ts (1)
173-173: LGTM! Proper validation bounds for player limits.The schema now enforces integer values in the range [2, 1000], which aligns perfectly with the client-side constants (MIN_PLAYER_LIMIT and MAX_PLAYER_LIMIT in HostLobbyModal). The minimum of 2 ensures viable multiplayer games, while the maximum of 1000 is generous yet prevents abuse.
resources/lang/en.json (1)
267-267: LGTM! Clear and user-friendly localization strings.The new localization entries support the player limit feature with clear, actionable messages. The
{limit}placeholder in lobby_full will be properly replaced using the ICU message format, and the warning message clearly explains the consequence of setting a limit below the current player count.Also applies to: 321-322
src/client/JoinPrivateLobbyModal.ts (4)
21-21: LGTM! Proper state management for optional player limit.The
maxPlayers: number | nulltype correctly represents an optional limit, wherenullmeans unlimited. Proper use of@state()decorator for Lit reactivity.
79-81: LGTM! Clean conditional rendering of player limit.The ternary expression cleanly handles the optional limit display, showing "X/Y" format when maxPlayers is set. This matches the pattern used in HostLobbyModal for consistency.
133-134: LGTM! Proper state cleanup on lobby exit.Resetting maxPlayers to null and clearing the players array ensures no stale data persists when leaving the lobby.
323-323: LGTM! Proper polling synchronization for maxPlayers.The polling logic correctly updates maxPlayers from the server's gameConfig, using optional chaining and nullish coalescing to handle undefined values gracefully.
resources/lang/debug.json (1)
131-131: LGTM! Proper debug localization entries.The debug locale entries correctly mirror the en.json additions with self-referential values, following the standard debug pattern used throughout the file.
Also applies to: 159-160
src/client/HostLobbyModal.ts (5)
62-66: LGTM! Proper state management and constants.The state properties correctly model the player limit feature (number | null for optional limit, boolean for warning state). The static readonly constants (MIN=2, MAX=1000) match the schema validation exactly.
535-582: LGTM! Clean UI implementation matching existing patterns.The player limit UI follows the same checkbox + conditional input pattern as the max timer feature, ensuring consistency. The warning display is appropriately styled and conditionally rendered. The player count display ("X/Y") mirrors the pattern in JoinPrivateLobbyModal.
Also applies to: 608-608
706-736: LGTM! Robust input validation and state management.The event handlers properly prevent invalid input (blocking e/+/- keys and sanitizing values), enforce the [MIN, MAX] range, and update both the state and warning appropriately. The warning correctly triggers when the limit is set below the current player count.
877-877: LGTM! Clean config persistence.Using
this.playerLimit ?? undefinedcorrectly converts null (unlimited) to undefined for the optional schema field. Clean and idiomatic TypeScript.
946-946: LGTM! Proper warning synchronization during polling.Calling
updatePlayerLimitWarning()after updating the clients array ensures the warning state stays current as players join or leave the lobby.
|
Very nice work, great feature. I have tagged this up and will assign for peer review |
github-project-automation
bot
moved this to Development
in OpenFront Release Management
Lavodan
left a comment
Lavodan
left a comment
There was a problem hiding this comment.
Looks quite good, the new error message is a lot nicer, I left some minor comments.
github-project-automation
bot
moved this from Final Review
to Development
in OpenFront Release Management
Thanks i will take a look when i have time |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
resources/lang/en.json (1)
269-270: LGTM! Clear error message for full lobbies.The new
lobby_fullkey provides users with clear feedback when they attempt to join a full lobby. The placeholder format{limit}is consistent with other localization keys in the file, and the trailing comma on line 269 is correct JSON syntax.Optional: Consider making the limit reference even more explicit
If you want to make it crystal clear that
{limit}refers to the number of players, you could use:- "lobby_full": "Lobby is full (limit: {limit})." + "lobby_full": "Lobby is full ({limit} players max)."However, the current wording is clear and concise, so this is purely optional.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
resources/lang/en.jsonsrc/client/JoinPrivateLobbyModal.tssrc/core/Schemas.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/JoinPrivateLobbyModal.ts
- src/core/Schemas.ts
🧰 Additional context used
🧠 Learnings (2)
📚 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.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.json
🔇 Additional comments (1)
resources/lang/en.json (1)
327-328: LGTM! Clear labels and helpful warning for hosts.The new localization keys for the player limit feature are well-crafted:
player_limit: Simple, clear label for the UI controlplayer_limit_warning: Effectively warns the host about the consequence of setting a limit below the current player count without causing alarm (existing players remain, only new joins are blocked)The semicolon usage is grammatically correct and keeps the message concise.
|
looks like it's out of date |
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 (1)
src/client/HostLobbyModal.ts (1)
774-831: Eliminate code duplication: extract config object.The configuration object is built twice—once for
detail.config(lines 775–801) and again for the JSON body (lines 804–830). Extract it into a variable and reuse it.🔎 Proposed refactor
private async putGameConfig() { + const config = { + gameMap: this.selectedMap, + gameMapSize: this.compactMap + ? GameMapSize.Compact + : GameMapSize.Normal, + difficulty: this.selectedDifficulty, + bots: this.bots, + infiniteGold: this.infiniteGold, + donateGold: this.donateGold, + infiniteTroops: this.infiniteTroops, + donateTroops: this.donateTroops, + instantBuild: this.instantBuild, + randomSpawn: this.randomSpawn, + gameMode: this.gameMode, + disabledUnits: this.disabledUnits, + playerTeams: this.teamCount, + ...(this.gameMode === GameMode.Team && + this.teamCount === HumansVsNations + ? { disableNations: false } + : { disableNations: this.disableNations }), + maxTimerValue: + this.maxTimer === true ? this.maxTimerValue : undefined, + maxPlayers: this.playerLimit ?? undefined, + } satisfies Partial<GameConfig>; + this.dispatchEvent( new CustomEvent("update-game-config", { - detail: { - config: { - gameMap: this.selectedMap, - // ... (all the repeated properties) - } satisfies Partial<GameConfig>, - }, - body: JSON.stringify({ - gameMap: this.selectedMap, - // ... (all the repeated properties) - } satisfies Partial<GameConfig>), + detail: { config }, + body: JSON.stringify(config), bubbles: true, composed: true, }), ); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
resources/lang/en.jsonsrc/client/HostLobbyModal.tssrc/core/Schemas.tssrc/server/GameServer.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/Schemas.ts
- src/server/GameServer.ts
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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.
📚 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.json
📚 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:
resources/lang/en.jsonsrc/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/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-12-26T22:21:21.904Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2689
File: src/client/PublicLobby.ts:245-245
Timestamp: 2025-12-26T22:21:21.904Z
Learning: In public lobbies with HumansVsNations mode in src/client/PublicLobby.ts, maxPlayers represents only human player slots (already halved in DefaultConfig.ts). The nation NPCs are added automatically server-side and don't count toward maxPlayers. Therefore, getTeamSize correctly returns maxPlayers directly for HumansVsNations to display the proper team size (e.g., maxPlayers=5 yields "5 Humans vs 5 Nations").
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2026-01-02T18:11:06.832Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:06.832Z
Learning: In src/client/HostLobbyModal.ts, the ?s=xxxxx URL suffix used in lobby URLs is purely for cache-busting platform previews (e.g., Discord, WhatsApp, x.com) and is not used by the join logic. The join flow ignores the suffix value, so regenerating it via updateUrlWithSuffix() on configuration changes will not break existing shared URLs; it only prompts platforms to refresh preview metadata. Treat the suffix as non-functional for join behavior and ensure any related changes preserve that invariant.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-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
🧬 Code graph analysis (1)
src/client/HostLobbyModal.ts (4)
src/client/Utils.ts (1)
translateText(92-147)src/client/LangSelector.ts (1)
translateText(258-278)src/core/game/Game.ts (1)
HumansVsNations(56-56)src/core/Schemas.ts (1)
GameConfig(90-90)
🪛 GitHub Actions: 🧪 CI
src/client/HostLobbyModal.ts
[error] 832-834: Transform failed: Expected ')' but found ':' during esbuild transformation at line 832. Likely a syntax error in an object literal (misplaced/missing braces or property).
🔇 Additional comments (9)
resources/lang/en.json (2)
272-273: LGTM! Clear error message with helpful context.The new
lobby_fullentry is well-structured: the comma on line 272 properly extends the JSON, and the message clearly communicates why the user can't join while showing the limit value.
330-331: LGTM! Clear and user-friendly text.Both entries follow existing localization patterns:
- The label is concise
- The warning clearly explains the consequence (new players can't join)
The text is grammatical and informative for hosts managing their lobby settings.
src/client/HostLobbyModal.ts (7)
26-26: LGTM! Component import added correctly.The CheckboxWithInput component import is properly added and used consistently for both the max-timer and player-limit features.
66-70: LGTM! Clean state and constants.The
playerLimitusesnullfor unlimited (clear and idiomatic), and the constants are properly typed asprivate static readonly. The range 2–1000 is sensible.
497-516: LGTM! Consistent refactor to Lit component.Replacing the inline timer UI with
CheckboxWithInputimproves consistency and addresses previous feedback about componentizing checkboxes. The logic correctly preserves the enable/disable behavior and updates the config.
519-551: LGTM! Player limit UI implemented correctly.The checkbox/input logic correctly initializes the limit on enable and clears it on disable. The warning display is conditional and appropriately styled. Both handlers update the warning state and persist changes.
577-577: LGTM! Lobby header shows limit correctly.The conditional formatting displays "X/Y Players" when a limit is set and "X Players" when unlimited, matching the expected behavior.
680-683: LGTM! Warning logic is sound.The method correctly triggers the warning only when a limit exists and current players exceed it. The call sites (limit changes and player polling) ensure the warning stays synchronized.
897-897: LGTM! Warning refresh on player poll.Calling
updatePlayerLimitWarning()after updating the clients list ensures the warning state reflects the latest player count.
i resolved the conflicts |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/HostLobbyModal.ts
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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.
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2026-01-02T18:11:06.832Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:06.832Z
Learning: In src/client/HostLobbyModal.ts, the ?s=xxxxx URL suffix used in lobby URLs is purely for cache-busting platform previews (e.g., Discord, WhatsApp, x.com) and is not used by the join logic. The join flow ignores the suffix value, so regenerating it via updateUrlWithSuffix() on configuration changes will not break existing shared URLs; it only prompts platforms to refresh preview metadata. Treat the suffix as non-functional for join behavior and ensure any related changes preserve that invariant.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-12-26T22:21:21.904Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2689
File: src/client/PublicLobby.ts:245-245
Timestamp: 2025-12-26T22:21:21.904Z
Learning: In public lobbies with HumansVsNations mode in src/client/PublicLobby.ts, maxPlayers represents only human player slots (already halved in DefaultConfig.ts). The nation NPCs are added automatically server-side and don't count toward maxPlayers. Therefore, getTeamSize correctly returns maxPlayers directly for HumansVsNations to display the proper team size (e.g., maxPlayers=5 yields "5 Humans vs 5 Nations").
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-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-08-17T20:48:49.411Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-10-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/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
🧬 Code graph analysis (1)
src/client/HostLobbyModal.ts (2)
src/client/Utils.ts (1)
translateText(92-147)src/client/LangSelector.ts (1)
translateText(258-278)
🔇 Additional comments (6)
src/client/HostLobbyModal.ts (6)
66-70: Clean state design for the player limit feature.Using
nullto represent "unlimited" is clear and idiomatic. The min/max constants prevent magic numbers throughout the component.
518-551: Player limit implementation is solid.The checkbox toggle correctly assigns the default value when enabled and resets to
null(unlimited) when disabled. The warning updates synchronously after state changes. Good work.
577-577: Simple and clear display logic.Shows "X/Y Players" when a limit is set, and just "X Players" when unlimited. Easy to understand.
680-683: Warning logic is correct.Only triggers when current players strictly exceed the limit. This matches the expected behavior where existing players are not kicked but the host is warned.
801-801: Config payload correctly converts null to undefined.The nullish coalescing operator keeps the config object clean by using
undefinedfor optional fields.
869-869: Good placement for warning update.Calling
updatePlayerLimitWarning()after fetching the latest client list ensures the warning stays accurate when players join or leave.
looks like there is more 😢 |
no i fixed it, but every time another PR changes the same file/lines, the conflict comes back |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/HostLobbyModal.ts (1)
557-592: Remove duplicate max-timer code.This entire block is leftover code from before the
CheckboxWithInputrefactor. The new component (lines 498–519) already handles max timer, so this creates duplicate UI controls and ID collisions. Delete lines 557–592 completely.🗑️ Proposed fix
Remove the entire old max-timer label block:
- <label - for="max-timer" - class="option-card ${this.maxTimer ? "selected" : ""}" - > - <div class="checkbox-icon"></div> - <input - type="checkbox" - id="max-timer" - @change=${(e: Event) => { - const checked = (e.target as HTMLInputElement).checked; - if (!checked) { - this.maxTimerValue = undefined; - } - this.maxTimer = checked; - this.putGameConfig(); - }} - .checked=${this.maxTimer} - /> - ${ - this.maxTimer === false - ? "" - : html`<input - type="number" - id="end-timer-value" - min="0" - max="120" - .value=${String(this.maxTimerValue ?? "")} - class="w-15 text-black text-right rounded-lg" - @input=${this.handleMaxTimerValueChanges} - @keydown=${this.handleMaxTimerValueKeyDown} - />` - } - <div class="option-card-title"> - ${translateText("host_modal.max_timer")} - </div> - </label>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
resources/lang/debug.jsonresources/lang/en.jsonsrc/client/HostLobbyModal.tssrc/core/Schemas.tssrc/server/GameServer.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- resources/lang/en.json
- resources/lang/debug.json
- src/core/Schemas.ts
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
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.
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2689
File: src/client/PublicLobby.ts:245-245
Timestamp: 2025-12-26T22:21:21.904Z
Learning: In public lobbies with HumansVsNations mode in src/client/PublicLobby.ts, maxPlayers represents only human player slots (already halved in DefaultConfig.ts). The nation NPCs are added automatically server-side and don't count toward maxPlayers. Therefore, getTeamSize correctly returns maxPlayers directly for HumansVsNations to display the proper team size (e.g., maxPlayers=5 yields "5 Humans vs 5 Nations").
📚 Learning: 2025-12-26T22:21:21.904Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2689
File: src/client/PublicLobby.ts:245-245
Timestamp: 2025-12-26T22:21:21.904Z
Learning: In public lobbies with HumansVsNations mode in src/client/PublicLobby.ts, maxPlayers represents only human player slots (already halved in DefaultConfig.ts). The nation NPCs are added automatically server-side and don't count toward maxPlayers. Therefore, getTeamSize correctly returns maxPlayers directly for HumansVsNations to display the proper team size (e.g., maxPlayers=5 yields "5 Humans vs 5 Nations").
Applied to files:
src/server/GameServer.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/server/GameServer.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/server/GameServer.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/server/GameServer.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/HostLobbyModal.ts
📚 Learning: 2026-01-02T18:11:06.832Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:06.832Z
Learning: In src/client/HostLobbyModal.ts, the ?s=xxxxx URL suffix used in lobby URLs is purely for cache-busting platform previews (e.g., Discord, WhatsApp, x.com) and is not used by the join logic. The join flow ignores the suffix value, so regenerating it via updateUrlWithSuffix() on configuration changes will not break existing shared URLs; it only prompts platforms to refresh preview metadata. Treat the suffix as non-functional for join behavior and ensure any related changes preserve that invariant.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-10-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-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-08-17T20:48:49.411Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1836
File: src/client/Main.ts:482-482
Timestamp: 2025-08-17T20:48:49.411Z
Learning: In PR #1836, user woodydrn prefers to keep changes minimal and focused on the specific issue (clientID persistence) rather than refactoring redundant code in JoinLobbyEvent dispatchers. They want to avoid scope creep in focused bug fix PRs.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-10-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
🧬 Code graph analysis (2)
src/server/GameServer.ts (2)
src/core/configuration/DefaultConfig.ts (1)
gameConfig(284-286)src/server/MapPlaylist.ts (1)
gameConfig(90-136)
src/client/HostLobbyModal.ts (2)
src/client/LangSelector.ts (1)
translateText(224-244)src/client/Utils.ts (1)
translateText(92-147)
🔇 Additional comments (11)
src/server/GameServer.ts (3)
138-141: LGTM!The
maxPlayersupdate follows the same conditional pattern as other config fields in this method. Clean and consistent.
166-167: Good logging addition.Including
currentPlayersandmaxPlayersin the log makes debugging lobby capacity issues easier.
174-175: Translation support looks good.The error payload correctly includes
translationKeyandargsfor client-side localization. The?? "?"fallback on Line 175 is technically unreachable (since Line 161 ensuresmaxPlayersis truthy), but it's harmless defensive code.src/client/HostLobbyModal.ts (8)
27-27: LGTM!Import for the new
CheckboxWithInputcomponent is correctly added.
69-73: Well-structured state and constants.The
playerLimittype (number | null) cleanly represents unlimited (null) vs. limited lobbies. Constants align with the server-side validation range (2–1000).
498-519: LGTM! Previous issue resolved.The
CheckboxWithInputintegration for max timer is correct. The checkbox-change handler properly assignsmaxTimerValuetodefaultValuewhen enabled (Line 509), which resolves the issue flagged in previous reviews.
521-554: Clean player limit implementation.The player limit
CheckboxWithInputfollows the same pattern as max timer and correctly:
- Sets limit to default (20) or null based on checkbox state
- Updates the warning state after changes
- Displays the warning message conditionally
657-657: LGTM!The player count display correctly shows "X" when unlimited or "X/Y" when a limit is set.
760-763: Correct warning logic.The warning state is properly computed: show warning only when a limit exists and current player count exceeds it.
904-904: LGTM!The
maxPlayersfield is correctly derived fromplayerLimit(null becomes undefined for the wire format).
972-972: LGTM!Calling
updatePlayerLimitWarning()after polling ensures the UI warning reflects the current player count.
5 participants
Description:
Added a "Player limit" setting for private lobbies that allows the host to set a maximum number of players (2-1000) who can join.
Features:
Screenshots
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
abodcraft1