Add lobby preset #2566
Add lobby preset #2566
Conversation
|
|
WalkthroughAdds lobby preset support: new translation keys, preset schemas and defaults, local persistence in UserSettings, a new LobbyPresetControls web component, and integration into HostLobbyModal and SinglePlayerModal to list, save, apply, and delete lobby presets. (32 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Modal as Host/SinglePlayer Modal
participant PresetUI as LobbyPresetControls
participant Settings as UserSettings
User->>Modal: open()
Modal->>Settings: getLobbyPresets()
Settings-->>Modal: presets[]
Modal->>PresetUI: render(presets)
User->>PresetUI: select preset
PresetUI->>Modal: preset-select(name)
Modal->>Modal: applyPreset(name)
User->>PresetUI: enter name
PresetUI->>Modal: preset-name-input(text)
User->>PresetUI: click Save
PresetUI->>Modal: preset-save(name)
Modal->>Modal: buildPresetConfig()
Modal->>Settings: setLobbyPresets(updatedList)
Settings-->>Modal: updated presets
Modal->>PresetUI: update(presets)
User->>PresetUI: click Delete
PresetUI->>Modal: preset-delete(name)
Modal->>Settings: setLobbyPresets(updatedList)
Settings-->>Modal: updated presets
Modal->>PresetUI: update(presets)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 0
🧹 Nitpick comments (4)
src/core/game/UserSettings.ts (2)
1-1: Import from client in core module creates wrong dependency direction.The
coremodule should not import fromclient. This creates a coupling where the core layer depends on the client layer. Consider movingLobbyPresettype to a shared location (e.g.,src/core/Schemas.ts) or keeping the type inline here.-import { LobbyPreset } from "../../client/LobbyPresets"; +// Define type locally or import from a shared location +export type LobbyPreset = { + name: string; + config: Record<string, unknown>; +};Alternatively, move
LobbyPresetandLobbyPresetConfigtypes tosrc/core/Schemas.tsand import from there in both files.
53-64: UsegetJsonto reduce duplication.
getLobbyPresetsduplicates JSON parsing logic. Consider usinggetJsoninternally while keeping the array validation.getLobbyPresets(defaultValue: LobbyPreset[] = []): LobbyPreset[] { - const value = localStorage.getItem("settings.lobbyPresets"); - if (!value) return defaultValue; - - try { - const parsed = JSON.parse(value) as LobbyPreset[]; - return Array.isArray(parsed) ? parsed : defaultValue; - } catch (error) { - console.warn("Failed to parse lobby presets", error); - return defaultValue; - } + const parsed = this.getJson<LobbyPreset[]>("settings.lobbyPresets", defaultValue); + return Array.isArray(parsed) ? parsed : defaultValue; }src/client/SinglePlayerModal.ts (1)
498-577: Consider extracting shared preset logic.The preset methods (
loadPresets,savePreset,applyPreset,deletePreset,buildPresetConfig) are nearly identical betweenSinglePlayerModalandHostLobbyModal. Consider extracting shared logic to reduce duplication.One approach: create a utility module with shared preset operations, or extract the presets UI section to a reusable component that emits events for load/save/delete actions.
src/client/LobbyPresets.ts (1)
60-82: Defaultbotsvalue differs from modal defaults.
normalizePresetConfigdefaultsbotsto0, but both modals initializebotsto400. This inconsistency means loading an old preset missingbotsfield will unexpectedly change the bot count.private normalizePresetConfig( config: Partial<LobbyPresetConfig>, ): LobbyPresetConfig { return { gameMap: config.gameMap ?? GameMapType.World, useRandomMap: config.useRandomMap ?? false, difficulty: config.difficulty ?? Difficulty.Medium, disableNPCs: config.disableNPCs ?? false, - bots: config.bots ?? 0, + bots: config.bots ?? 400, infiniteGold: config.infiniteGold ?? false, donateGold: config.donateGold ?? false,
📜 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(6 hunks)src/client/LobbyPresets.ts(1 hunks)src/client/SinglePlayerModal.ts(7 hunks)src/core/game/UserSettings.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 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-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/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
📚 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/client/LobbyPresets.tssrc/client/SinglePlayerModal.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.tssrc/client/SinglePlayerModal.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/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/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/client/SinglePlayerModal.ts
🧬 Code graph analysis (2)
src/core/game/UserSettings.ts (1)
src/client/LobbyPresets.ts (1)
LobbyPreset(25-28)
src/client/SinglePlayerModal.ts (3)
src/client/LobbyPresets.ts (3)
LobbyPreset(25-28)LobbyPresetStore(30-83)LobbyPresetConfig(5-23)src/core/game/UserSettings.ts (1)
UserSettings(7-235)src/client/LangSelector.ts (1)
translateText(258-278)
🔇 Additional comments (13)
resources/lang/en.json (1)
139-144: LGTM!Translation keys for presets are consistent between
single_modalandhost_modalsections, follow existing naming patterns, and onlyen.jsonis updated. Based on learnings, other locale files are handled by the dedicated translation team.Also applies to: 269-274
src/core/game/UserSettings.ts (1)
37-51: LGTM!Clean generic helpers for JSON storage. Error handling with
console.warnis appropriate for localStorage operations.src/client/HostLobbyModal.ts (5)
67-75: LGTM!State management for presets is well-structured. Initializing
presetStorewith the existinguserSettingsinstance ensures consistency.
187-237: LGTM!Presets UI is clean with proper dark mode support. Load/Delete buttons correctly disable when no preset is selected.
720-740: LGTM!
buildPresetConfigcaptures all settings correctly. ConditionalmaxTimerValuehandling aligns with the UI behavior.
742-750: LGTM!Save logic correctly handles both new presets and updates to existing ones by name matching.
752-782: LGTM!Preset application correctly updates all state fields with sensible fallbacks and syncs with the server via
putGameConfig().src/client/SinglePlayerModal.ts (4)
48-49: LGTM!Changing
donateGoldanddonateTroopsfrom hardcoded values to state variables allows presets to control these settings properly.
92-148: LGTM!Presets UI is consistent with
HostLobbyModal. The identical structure provides a unified experience.
542-569: LGTM!
applyPresetcorrectly omits server sync for single-player mode. The synchronous implementation is appropriate here.
737-738: LGTM!Game config now correctly uses state values for
donateGoldanddonateTroops, enabling preset control of these settings.src/client/LobbyPresets.ts (2)
5-23: LGTM!Type definition is complete and well-structured. Using typed unions with explicit fields is clean and idiomatic TypeScript.
30-53: LGTM!Store implementation is clean. Using composition with
UserSettingsis the right approach. Thesavemethod correctly handles both new presets and updates by filtering then appending.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/LobbyPresets.ts (1)
39-47: Consider validating the preset name.The
nameparameter is not validated. Empty strings, excessive whitespace, or very long names could cause issues in the UI or storage.Consider adding validation:
save(name: string, config: LobbyPresetConfig): LobbyPreset[] { + const trimmedName = name.trim(); + if (!trimmedName) { + throw new Error("Preset name cannot be empty"); + } + if (trimmedName.length > 50) { + throw new Error("Preset name is too long (max 50 characters)"); + } - const presets = this.list().filter((preset) => preset.name !== name); + const presets = this.list().filter((preset) => preset.name !== trimmedName); const updated = [ ...presets, - { name, config: this.normalizePresetConfig(config) }, + { name: trimmedName, config: this.normalizePresetConfig(config) }, ]; this.userSettings.setLobbyPresets(updated); return updated; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/LobbyPresets.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/LobbyPresets.ts
🧬 Code graph analysis (1)
src/client/LobbyPresets.ts (3)
src/core/Schemas.ts (1)
TeamCountConfig(155-155)src/core/configuration/DefaultConfig.ts (1)
userSettings(276-281)src/core/game/UserSettings.ts (1)
UserSettings(7-235)
🔇 Additional comments (5)
src/client/LobbyPresets.ts (5)
1-28: Clean type definitions.The imports and type definitions are well-structured. Using plain typed objects instead of class hierarchies aligns with best practices for this codebase.
30-31: Good dependency injection pattern.The constructor uses optional dependency injection with a sensible default, making the code testable while keeping production usage simple.
33-37: LGTM.The list() method correctly retrieves and normalizes presets from storage.
49-53: LGTM.The delete() method correctly removes the preset and updates storage.
60-82: Verify the default bots value.Line 68 sets the default
botsvalue to 400, which seems unusually high. Please confirm this is the intended default for lobby capacity or bot count, or check related constants and configuration files in the codebase to ensure this value is correct.
There was a problem hiding this comment.
Take all comments with a grain of salt, I'm very new to this codebase, and may have absolutely no idea what im talking about - however this PR excited me! I branched off and made a quick POC to define "scenarios" in the config, that are presets that auto-set then launch and appear in another list on the single player screen. This enables a whole lot of things with very minimal work:
- multiplayer lobby presets (the use case already described)
- in-game tutorials ex:
- tiny map w/ bots + terrain types + islands, only cities / factories
- tiny map w/ bots + nations + ports + bombs
- single-player puzzles / scenarios or group challenge modes
- tiny map w/ 4 or 5 nations for Humans vs Nations & specific bot/difficulty settings
- potentially more variety in configs for public lobbies
The things that I noticed while messing with it:
- I really wanted the types to be somewhere in core so that I didn't have to import types from the client if I was making a scenario on the server
- There might be a bug w/ setting bots to 0 - i think its gets reset to the default
- I noticed having to update stuff in multiple places a lot because of duplication between the two modals
- it probably wants to be near the bottom of the modals instead of the top
- If all fields are required, as time goes on and new properties are added to the config, old presets will no longer be valid. You could version the configs, but I also think a minimal config would be a useful thing to be able to use, ex:
{
"bots": 0
}or a "no nukes" setting that is just the disabledUnits list
src/client/LobbyPresets.ts
Outdated
| export type LobbyPresetConfig = { | ||
| gameMap: GameMapType; | ||
| useRandomMap: boolean; | ||
| difficulty: Difficulty; | ||
| disableNPCs: boolean; | ||
| bots: number; | ||
| infiniteGold: boolean; | ||
| donateGold: boolean; | ||
| infiniteTroops: boolean; | ||
| donateTroops: boolean; | ||
| instantBuild: boolean; | ||
| randomSpawn: boolean; | ||
| compactMap: boolean; | ||
| maxTimer: boolean; | ||
| maxTimerValue?: number; | ||
| gameMode: GameMode; | ||
| playerTeams: TeamCountConfig; | ||
| disabledUnits: UnitType[]; | ||
| }; |
There was a problem hiding this comment.
Is this the same as GameConfigSchema in src/core/Schemas.ts - could that be z.infer for this type?
I could see a point to redeclare these to be optional if you want to support presets with just a few properties to make them short.
src/client/HostLobbyModal.ts
Outdated
| <div class="options-section"> | ||
| <div class="option-title"> | ||
| ${translateText("host_modal.presets_title")} | ||
| </div> | ||
| <div class="option-cards" style="gap: 10px;"> | ||
| <div style="display: flex; gap: 8px; flex-wrap: wrap; width: 100%;"> | ||
| <select | ||
| @change=${this.handlePresetSelect} | ||
| .value=${this.selectedPresetName} | ||
| class="preset-select px-2 py-1 rounded-lg border border-gray-300 text-black dark:bg-gray-700 dark:text-white dark:border-gray-300/60" | ||
| style="flex: 1; min-width: 160px;" | ||
| > | ||
| <option value=""> | ||
| ${translateText("host_modal.presets_select")} | ||
| </option> | ||
| ${this.lobbyPresets.map( | ||
| (preset) => html` | ||
| <option value=${preset.name}>${preset.name}</option> | ||
| `, | ||
| )} | ||
| </select> | ||
| <o-button | ||
| title=${translateText("host_modal.presets_load")} | ||
| @click=${() => this.applyPreset()} | ||
| ?disabled=${!this.selectedPresetName} | ||
| secondary | ||
| ></o-button> | ||
| <o-button | ||
| title=${translateText("host_modal.presets_delete")} | ||
| @click=${this.deletePreset} | ||
| ?disabled=${!this.selectedPresetName} | ||
| secondary | ||
| ></o-button> | ||
| </div> | ||
| <div style="display: flex; gap: 8px; flex-wrap: wrap; width: 100%;"> | ||
| <input | ||
| type="text" | ||
| placeholder=${translateText("host_modal.presets_placeholder")} | ||
| .value=${this.presetNameInput} | ||
| @input=${this.handlePresetNameInput} | ||
| class="px-2 py-2 rounded-lg border border-gray-300 text-black dark:bg-gray-700 dark:text-white dark:border-gray-300/60" | ||
| style="flex: 1; min-width: 160px;" | ||
| /> | ||
| <o-button | ||
| title=${translateText("host_modal.presets_save")} | ||
| @click=${this.savePreset} | ||
| secondary | ||
| ></o-button> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
since this is effectively the same in here and in the single player modal, would a component like the difficulties make sense? with that structure, you could have the component emit an event w/ the already good config and all the markup and methods aren't repeated.
Would prolly still require keeping the config building logic in the modals for saving, but the component could emit a "save your config" event w/ the cleaned name.
There was a problem hiding this comment.
Could you review the separated portion once more?
src/core/game/UserSettings.ts
Outdated
| getJson<T>(key: string, defaultValue: T): T { | ||
| const value = localStorage.getItem(key); | ||
| if (!value) return defaultValue; | ||
|
|
||
| try { | ||
| return JSON.parse(value) as T; | ||
| } catch (error) { | ||
| console.warn(`Failed to parse JSON for key ${key}`, error); | ||
| return defaultValue; | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this used somewhere or required for the class? I'm not seeing it used.
There was a problem hiding this comment.
I forgot to remove this part.
Thank you for pointing it out!
| try { | ||
| const parsed = JSON.parse(value) as LobbyPreset[]; | ||
| return Array.isArray(parsed) ? parsed : defaultValue; | ||
| } catch (error) { | ||
| console.warn("Failed to parse lobby presets", error); | ||
| return defaultValue; | ||
| } |
There was a problem hiding this comment.
if you use the existing GameConfigSchema I think you can call GameConfigSchema.safeParse
…glePlayerModal, and add PresetControls component
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/client/HostLobbyModal.ts (2)
669-716: Consider extracting shared preset logic.The methods
loadPresets,handlePresetSelect,handlePresetNameInput,buildPresetConfig, andsavePresetare nearly identical to those inSinglePlayerModal. While the current extraction oflobby-preset-controlsas a UI component addresses the previous review feedback, the business logic (building config, saving, etc.) could be shared via a mixin or utility module to reduce duplication.This is optional since both modals work correctly as-is.
717-747: Verify the async is necessary for applyPreset.The method is marked
asyncbutawaitis only used at the end forloadNationCount(). This is fine, though consider whether error handling is needed ifloadNationCount()fails (currently it logs a warning internally, which may be acceptable).The redundant fallbacks using
??(e.g.,config.difficulty ?? this.selectedDifficulty) are safe but unnecessary sinceLobbyPresetStore.normalizePresetConfigalready provides defaults for all fields.private async applyPreset(name?: string) { const presetName = name ?? this.selectedPresetName; const preset = this.lobbyPresets.find((p) => p.name === presetName); if (!preset) { return; } const config = preset.config; this.useRandomMap = config.useRandomMap; this.selectedMap = config.useRandomMap ? this.getRandomMap() - : (config.gameMap ?? this.selectedMap); - this.selectedDifficulty = config.difficulty ?? this.selectedDifficulty; - this.disableNPCs = config.disableNPCs ?? this.disableNPCs; - this.bots = config.bots ?? this.bots; + : config.gameMap; + this.selectedDifficulty = config.difficulty; + this.disableNPCs = config.disableNPCs; + this.bots = config.bots; // ... similar for other fieldssrc/client/SinglePlayerModal.ts (1)
453-539: Preset methods duplicate HostLobbyModal.As noted for
HostLobbyModal, these methods are nearly identical. The only difference isapplyPresetis synchronous here (noloadNationCount()call) vs async inHostLobbyModal.This works correctly but is worth noting for future refactoring.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
resources/lang/en.json(1 hunks)src/client/HostLobbyModal.ts(6 hunks)src/client/SinglePlayerModal.ts(7 hunks)src/client/components/lobbyConfig/PresetControls.ts(1 hunks)src/core/game/UserSettings.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- resources/lang/en.json
- src/core/game/UserSettings.ts
🧰 Additional context used
🧠 Learnings (5)
📚 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-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.tssrc/client/components/lobbyConfig/PresetControls.tssrc/client/SinglePlayerModal.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/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/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/client/SinglePlayerModal.ts
🧬 Code graph analysis (2)
src/client/HostLobbyModal.ts (2)
src/client/LobbyPresets.ts (3)
LobbyPreset(25-28)LobbyPresetStore(30-83)LobbyPresetConfig(5-23)src/core/game/UserSettings.ts (1)
UserSettings(7-223)
src/client/components/lobbyConfig/PresetControls.ts (2)
src/client/HostLobbyModal.ts (1)
customElement(39-998)src/client/SinglePlayerModal.ts (1)
customElement(37-725)
🔇 Additional comments (17)
src/client/HostLobbyModal.ts (5)
28-35: Good addition of preset imports.Clean import structure for the new preset functionality.
68-76: State and store setup looks correct.The preset-related state variables and store instance are well-organized. Sharing the same
userSettingsinstance withpresetStoreis a sensible design choice.
89-91: Good use of lifecycle hook for initial load.Using
firstUpdatedensures presets are ready before any user interaction.
188-198: Preset controls integration looks solid.The event wiring between
lobby-preset-controlsand the modal handlers is clear and follows the same pattern used inSinglePlayerModal.
749-758: Delete preset logic is correct.The method properly clears
selectedPresetNamewhen the deleted preset was the selected one.src/client/SinglePlayerModal.ts (6)
61-66: Preset state and store setup matches HostLobbyModal.Consistent pattern across both modals.
78-80: firstUpdated loads presets correctly.Same pattern as HostLobbyModal.
93-103: Preset controls integration is clean.Event bindings match the HostLobbyModal implementation.
541-545: Good: loadPresets called on open.Ensures fresh preset list when modal opens.
699-700: Good change: using state values instead of hardcoded true.This allows presets to control donation settings in single-player games.
49-50: Verify whether donateGold/donateTroops state variables in SinglePlayerModal should have UI controls.The added
donateGoldanddonateTroopsstate variables are initialized totrueand used in preset configuration and game start logic. Clarify whether these are configuration flags meant to be set once (thus not requiring UI controls), or if users should be able to toggle them in the modal like they can in HostLobbyModal. If UI controls are needed, add corresponding checkboxes and handler methods for consistency.src/client/components/lobbyConfig/PresetControls.ts (6)
1-14: Clean component setup.Good use of
@propertydecorators for controlled inputs. Using light DOM (createRenderRootreturningthis) is consistent with the codebase pattern.
16-53: Event handlers are well-structured.Each handler validates input (trims and checks for empty strings) before dispatching events. The event options (
bubbles: true, composed: true) are correct for cross-shadow-DOM communication.
66-78: Save logic allows overwriting existing presets.Using
this.nameInput || this.selectedNamemeans users can select a preset and click Save to update it. This is a reasonable UX choice, but ensure it's the intended behavior.
102-128: Button disabled states are correct.Load and Delete buttons are properly disabled when no preset is selected. The Save button is always enabled, which allows saving new presets.
80-133: Good dark mode support.The CSS classes include both light and dark mode variants (
dark:bg-gray-700 dark:text-white dark:border-gray-300/60), matching the PR objectives.
88-101: Select element value binding may not reflect external changes.The
.value=${this.selectedName}binding on the<select>element sets the value property, but native<select>elements don't always respond well to property updates when the value doesn't match any option. IfselectedNameis set to a value not inpresets, the select will show the first option butselectedNamewon't be cleared.Consider adding a check or handling this edge case in the parent component when a preset is deleted.
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
This change makes adding/removing new config params more difficult since we'll need to add the same config param to many different places in the code.
Is there a way would could refactor this so we can use the GameConfigSchema?
src/client/LobbyPresets.ts
Outdated
There was a problem hiding this comment.
could this be merged into PresetControls.ts?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/client/HostLobbyModal.ts (1)
668-757: Code duplication with SinglePlayerModal.The preset methods (
loadPresets,handlePresetSelect,handlePresetNameInput,buildPresetConfig,savePreset,applyPreset,deletePreset) are nearly identical to SinglePlayerModal. Per the past review discussion, consider extracting shared logic into a helper or having the component emit a "save your config" event with the cleaned name.For now, this works and the duplication is manageable. The key difference is that
applyPresethere correctly callsloadNationCount()andputGameConfig()to sync with the server.
🧹 Nitpick comments (2)
src/client/SinglePlayerModal.ts (1)
500-527: Minor:disabledUnitsfallback differs from other fields.On line 523,
disabledUnitsfalls back to[]rather thanthis.disabledUnits. This means loading a preset withoutdisabledUnitswill clear any existing disabled units. This may be intentional (reset to defaults), but it differs from the other field fallbacks.src/client/components/lobbyConfig/PresetControls.ts (1)
50-73: Potential runtime error if stored data is invalid.
LobbyPresetConfigSchema.parse(config)on line 53 will throw aZodErrorif the config contains invalid data (e.g., wrong types). This could happen if stored presets become corrupted or schema changes in a future update.Consider using
safeParsewith fallback:private normalizePresetConfig( config: Partial<LobbyPresetConfig>, ): LobbyPresetConfig { - const parsed = LobbyPresetConfigSchema.parse(config); + const result = LobbyPresetConfigSchema.safeParse(config); + const parsed = result.success ? result.data : {}; return { gameMap: parsed.gameMap ?? GameMapType.World, // ... rest unchanged }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/client/HostLobbyModal.ts(6 hunks)src/client/SinglePlayerModal.ts(7 hunks)src/client/components/lobbyConfig/PresetControls.ts(1 hunks)src/core/Schemas.ts(1 hunks)src/core/game/UserSettings.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/game/UserSettings.ts
🧰 Additional context used
🧠 Learnings (6)
📚 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/SinglePlayerModal.tssrc/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/SinglePlayerModal.tssrc/client/HostLobbyModal.tssrc/client/components/lobbyConfig/PresetControls.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/core/Schemas.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/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/client/SinglePlayerModal.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
🧬 Code graph analysis (4)
src/client/SinglePlayerModal.ts (3)
src/client/components/lobbyConfig/PresetControls.ts (3)
LobbyPreset(15-18)LobbyPresetStore(20-74)LobbyPresetConfig(13-13)src/core/game/UserSettings.ts (1)
UserSettings(7-223)src/core/Schemas.ts (1)
LobbyPresetConfig(181-181)
src/core/Schemas.ts (1)
src/client/components/lobbyConfig/PresetControls.ts (2)
LobbyPresetConfigSchema(12-12)LobbyPresetConfig(13-13)
src/client/HostLobbyModal.ts (3)
src/client/components/lobbyConfig/PresetControls.ts (3)
LobbyPreset(15-18)LobbyPresetStore(20-74)LobbyPresetConfig(13-13)src/core/game/UserSettings.ts (1)
UserSettings(7-223)src/core/Schemas.ts (1)
LobbyPresetConfig(181-181)
src/client/components/lobbyConfig/PresetControls.ts (3)
src/core/Schemas.ts (3)
LobbyPresetConfig(181-181)LobbyPresetConfigSchema(176-180)TeamCountConfig(155-155)src/core/configuration/DefaultConfig.ts (1)
userSettings(276-281)src/core/game/UserSettings.ts (1)
UserSettings(7-223)
🔇 Additional comments (17)
src/core/Schemas.ts (1)
176-181: LGTM! Clean schema extension for partial preset configs.The schema correctly extends
GameConfigSchemawith UI-specific flags (useRandomMap,compactMap,maxTimer) and applies.partial()to allow storing only the fields users actually changed. This is a good pattern for preset storage.src/client/SinglePlayerModal.ts (7)
48-49: Defaults differ from HostLobbyModal.
donateGoldanddonateTroopsdefault totruehere butfalseinHostLobbyModal.ts(lines 51-53). This may be intentional (single-player vs multiplayer semantics), but verify this is the desired behavior.
77-79: LGTM!Using
firstUpdatedto load presets on component initialization is the correct Lit lifecycle hook for this purpose.
92-102: LGTM!The preset controls integration follows Lit patterns correctly with property bindings and event handlers.
464-484: LGTM!The config builder correctly captures all relevant lobby settings and handles
maxTimerValueconditionally.
486-498: LGTM!The save flow handles name resolution with sensible fallbacks and clears the input after saving.
529-538: LGTM!Delete handling correctly clears the selection when removing the currently selected preset.
698-699: Good fix: respects user-configured donation settings.Using
this.donateGoldandthis.donateTroopsinstead of hardcoded values ensures presets and UI toggles affect the game configuration.src/client/HostLobbyModal.ts (4)
28-32: LGTM!Import structure matches the SinglePlayerModal implementation for consistency.
187-197: LGTM!Preset controls integration follows the same pattern as SinglePlayerModal, maintaining consistency across modals.
716-746: Good: Server sync after preset application.The
asyncpattern withawait this.loadNationCount()andthis.putGameConfig()ensures the server receives the updated configuration after applying a preset. This is the key difference from SinglePlayerModal and is correctly implemented.
624-626: LGTM!Presets are correctly reloaded when the modal opens.
src/client/components/lobbyConfig/PresetControls.ts (5)
1-13: LGTM!Clean imports and useful re-exports so consumers can import everything from one location.
15-18: LGTM!Simple, clean type definition using composition.
76-84: LGTM!Light DOM usage is appropriate here since the component needs to inherit styles from parent modals.
86-148: LGTM!Event handlers follow a consistent pattern with proper guards and bubbling configuration.
150-203: LGTM!The template is well-structured with:
- Proper i18n via
translateText- Disabled states for buttons when no preset is selected
- Dark mode support via Tailwind classes
- Accessible form controls
…obbySettings for state initialization
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/client/SinglePlayerModal.ts (1)
476-491: Avoid side effects in buildPresetConfig.Lines 477 and 478-480 mutate instance state (
this.gameTypeandthis.gameMapSize) when building a preset config. Same issue as in HostLobbyModal—saving a preset shouldn't change current settings. Remove these assignments and only read from state, or move the mutation to where it's intentional.🔎 Proposed fix to remove side effects
private buildPresetConfig(): LobbyPresetConfig { - this.gameType = GameType.Singleplayer; - this.gameMapSize = this.compactMap - ? GameMapSize.Compact - : GameMapSize.Normal; const ret = {} as Record<LobbyPresetKey, LobbyPresetConfig[LobbyPresetKey]>; const state = this as unknown as Record< LobbyPresetKey, LobbyPresetConfig[LobbyPresetKey] >; lobbyPresetKeys.forEach((key) => { ret[key] = state[key]; }); + ret.gameType = GameType.Singleplayer; + ret.gameMapSize = this.compactMap ? GameMapSize.Compact : GameMapSize.Normal; ret.maxTimerValue = this.maxTimer ? this.maxTimerValue : undefined; return ret as LobbyPresetConfig; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/client/HostLobbyModal.tssrc/client/SinglePlayerModal.tssrc/client/components/lobbyConfig/PresetControls.tssrc/core/Schemas.ts
🧰 Additional context used
🧠 Learnings (8)
📚 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.tssrc/client/components/lobbyConfig/PresetControls.tssrc/client/SinglePlayerModal.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/core/Schemas.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/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.tssrc/client/SinglePlayerModal.ts
📚 Learning: 2025-06-01T04:56:16.870Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 992
File: src/core/configuration/DefaultConfig.ts:0-0
Timestamp: 2025-06-01T04:56:16.870Z
Learning: In the OpenFrontIO codebase, the number of player teams (`numPlayerTeams`) will never be higher than the minimum team size, so the modulo operation in `lobbyMaxPlayers()` will not result in zero players.
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/core/Schemas.tssrc/client/SinglePlayerModal.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-08-14T10:07:44.588Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1811
File: src/client/FlagInput.ts:0-0
Timestamp: 2025-08-14T10:07:44.588Z
Learning: The codebase uses FlagSchema from "../core/Schemas" for validating flag values. The validation is done using FlagSchema.safeParse(value).success pattern, which is preferred over custom regex validation for flag input validation.
Applied to files:
src/client/components/lobbyConfig/PresetControls.ts
🧬 Code graph analysis (2)
src/client/HostLobbyModal.ts (3)
src/core/Schemas.ts (3)
defaultLobbySettings(183-203)LobbyPreset(209-209)LobbyPresetConfig(181-181)src/core/game/Game.ts (1)
HumansVsNations(56-56)src/client/components/lobbyConfig/PresetControls.ts (1)
lobbyPresetKeys(13-33)
src/client/components/lobbyConfig/PresetControls.ts (2)
src/core/Schemas.ts (4)
LobbyPresetConfig(181-181)LobbyPreset(209-209)defaultLobbySettings(183-203)LobbyPresetConfigSchema(176-180)src/core/game/UserSettings.ts (1)
UserSettings(7-223)
github-project-automation
bot
moved this from Peer Review
to Development
in OpenFront Release Management
…r HostLobbyModal and SinglePlayerModal
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 (2)
src/client/HostLobbyModal.tssrc/client/SinglePlayerModal.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/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-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/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-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-01T04:56:16.870Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 992
File: src/core/configuration/DefaultConfig.ts:0-0
Timestamp: 2025-06-01T04:56:16.870Z
Learning: In the OpenFrontIO codebase, the number of player teams (`numPlayerTeams`) will never be higher than the minimum team size, so the modulo operation in `lobbyMaxPlayers()` will not result in zero players.
Applied to files:
src/client/HostLobbyModal.ts
🧬 Code graph analysis (1)
src/client/HostLobbyModal.ts (3)
src/core/Schemas.ts (4)
defaultLobbySettings(183-203)TeamCountConfig(155-155)LobbyPreset(209-209)LobbyPresetConfig(181-181)src/core/game/Game.ts (1)
HumansVsNations(56-56)src/client/components/lobbyConfig/PresetControls.ts (2)
LobbyPresetKey(35-35)lobbyPresetKeys(13-33)
🔇 Additional comments (19)
src/client/SinglePlayerModal.ts (11)
19-33: LGTM! Clean preset imports.The imports for lobby preset functionality are well-organized and all are used within the component.
47-75: LGTM! Consistent state initialization.State fields are properly typed and initialized from
defaultLobbySettings, ensuring consistency across the codebase. The new preset-related state and donation flags are correctly added.
89-91: LGTM! Proper lifecycle integration.Loading presets in
firstUpdatedensures they're available for the initial render.
104-114: LGTM! Complete preset controls integration.The preset controls component is properly wired with all necessary events and properties.
493-509: LGTM! Robust preset saving.The method handles name fallbacks gracefully and clears input after saving for good UX.
511-535: LGTM! Careful preset application logic.The method correctly handles random maps, normalizes configs, and appropriately overrides
gameTypetoSingleplayerfor this modal context.
537-549: LGTM! Safe preset deletion.Clearing
selectedPresetNamewhen deleting the selected preset prevents dangling references and maintains UI consistency.
551-555: LGTM! Refresh presets on open.Reloading presets when opening the modal ensures the UI stays in sync with potentially updated user settings.
565-632: LGTM! Consistent state updates.Handlers correctly update the refactored state field names. Updating
gameMapSizealongsidecompactMap(lines 600-602) keeps state consistent.
648-737: LGTM! Donation flags integrated correctly.The
donateGoldanddonateTroopsflags (lines 712-713) are now properly included in the game start configuration, aligning with the preset feature. All state references use the refactored field names consistently.
468-474: LGTM! Simple event handlers.Clean state updates for preset selection and name input.
src/client/HostLobbyModal.ts (8)
12-12: Clean preset integration with centralized defaults.The imports bring in the preset framework and the state initialization now uses
defaultLobbySettingsfor consistent defaults. The new preset state fields (lobbyPresets,selectedPresetName,presetNameInput) are well-typed and purpose-clear.Also applies to: 24-28, 32-36, 48-81
99-101: Good: Preset loading at both lifecycle points.Loading presets in
firstUpdated(once) andopen()(each modal open) ensures presets are available initially and refresh if modified elsewhere. This handles the case where presets change between modal opens.Also applies to: 635-636
198-208: Well-integrated preset controls component.The
lobby-preset-controlscomponent integration follows the same pattern as existing components (likedifficulty-display). Event-driven communication keeps concerns separated and addresses the previous review feedback about extracting shared UI.
679-689: LGTM: Clean delegation and simple handlers.These methods correctly delegate preset loading to
LobbyPresetControlsand handle events with straightforward state updates.
691-706: Side effects removed as requested.The method now correctly reads state without mutation—
gameTypeandgameMapSizeare set on the returned config object (lines 700-703), not onthis. This addresses the earlier review feedback. The logic cleanly derives values from current state.
708-724: Solid preset save implementation.The name resolution fallback chain and empty-name guard are sensible. Delegating persistence to
LobbyPresetControlsand updating UI state afterward keeps the logic clean.
726-753: Preset application is thorough and handles special cases.The method correctly:
- Normalizes the preset config before applying
- Handles
useRandomMap(line 735): when true, generates a fresh random map rather than reusing the storedgameMap—this makes sense because "use random map" should randomize each time- Filters
gameMapanduseRandomMapfrom the copy loop (lines 740-744) since they need custom logic- Forces
gameTypeto Private and derivesgameMapSizefromcompactMap(lines 746-749) to maintain consistency with the modal's constraints- Refreshes nation count and syncs to server
755-767: Consistent refactoring to new state properties throughout.The
deletePresetmethod correctly clears the selection when deleting the active preset. All handlers (handleMapSelection,handleDifficultySelection,handleTeamCountSelection, etc.) and methods (putGameConfig,loadNationCount,startGame) have been updated to reference the new state properties (gameMap,difficulty,playerTeams). The refactoring is consistent and complete.Also applies to: 769-1009
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
This is a lot of code for this feature. is there a way to simplify this?
Just to make sure I understand your intent correctly, could you let me know whether you’re mainly suggesting simplifying the logic, or if you would prefer this to be split into smaller PRs? |
I think he meant simplifying the code. Also I think this is a great feature, would love to see it in the game! |
|
If possible, could you point out which parts you feel are overly complex and where you think the logic could be simplified? |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/client/HostLobbyModal.ts:
- Around line 714-733: The applyPreset method is using an unsafe cast this as
unknown as PresetConfigState when calling LobbyPresetControls.applyPresetConfig,
which causes the static method to mutate the modal instance; instead create an
explicit PresetConfigState-shaped object (copy only the fields applyPresetConfig
needs, similar to the mapping used in buildPresetConfig) and pass that to
LobbyPresetControls.applyPresetConfig, then apply any mutated fields back to the
modal (or refactor applyPresetConfig to return a new config object and use that
result). Update applyPreset to use the explicit state object (or the returned
config) rather than casting this.
- Around line 690-694: The current buildPresetConfig method unsafely casts the
entire HostLobbyModal instance to PresetConfigState; replace this by explicitly
constructing an object that implements PresetConfigState (or use a small mapper
function) that pulls only the required fields from HostLobbyModal and pass that
to LobbyPresetControls.buildPresetConfig; update buildPresetConfig to reference
the new mapper (or inline object) so you remove the "this as unknown as
PresetConfigState" cast and ensure the required properties match
PresetConfigState explicitly.
In @src/client/SinglePlayerModal.ts:
- Around line 475-479: The buildPresetConfig method unsafely casts this to
PresetConfigState; replace the cast with an explicit object mapping that
constructs a real PresetConfigState instance from SinglePlayerModal's relevant
properties before calling LobbyPresetControls.buildPresetConfig. Locate
buildPresetConfig in SinglePlayerModal and identify the fields
LobbyPresetControls.buildPresetConfig expects (PresetConfigState); create a new
const presetState: PresetConfigState = { ... } by reading the modal's properties
(e.g., selectedPreset, rules, playerLimits, timerConfig, etc.) and pass
presetState to LobbyPresetControls.buildPresetConfig instead of using this as
unknown as PresetConfigState so type safety is preserved and runtime errors
avoided.
- Around line 499-515: The applyPreset method currently uses an unsafe cast
("this as unknown as PresetConfigState") so
LobbyPresetControls.applyPresetConfig can mutate the modal instance; instead,
create an explicit PresetConfigState object mapped from the modal's public/state
fields and pass that object to LobbyPresetControls.applyPresetConfig, then apply
any returned changes back to the modal (or, if you prefer, change
applyPresetConfig to be pure and return a new PresetConfig result which you then
assign to the modal). Specifically, stop casting this, gather the needed
properties from SinglePlayerModal into a new PresetConfigState-shaped object,
call LobbyPresetControls.applyPresetConfig with that object (or refactor
applyPresetConfig to return the modified config), and then update
SinglePlayerModal fields from the resulting config so no static method mutates
the modal instance directly.
🧹 Nitpick comments (5)
src/client/components/lobbyConfig/PresetControls.ts (3)
60-71: Clarify the gameType and gameMapSize override logic.The method copies all keys from state, then unconditionally overwrites
gameTypetoGameType.Privateand derivesgameMapSizefromcompactMap. This means if the caller passes a state with a different gameType, it will be silently replaced. Consider documenting this behavior or accepting gameType as a parameter to make the override explicit.💡 Suggested improvement
- static buildPresetConfig(state: PresetConfigState): LobbyPresetConfig { + static buildPresetConfig( + state: PresetConfigState, + overrideGameType?: GameType + ): LobbyPresetConfig { const ret = {} as Record<LobbyPresetKey, LobbyPresetConfig[LobbyPresetKey]>; lobbyPresetKeys.forEach((key) => { ret[key] = state[key]; }); - ret.gameType = GameType.Private; + ret.gameType = overrideGameType ?? GameType.Private; ret.gameMapSize = state.compactMap ? GameMapSize.Compact : GameMapSize.Normal; ret.maxTimerValue = state.maxTimer ? state.maxTimerValue : undefined; return ret as LobbyPresetConfig; }Then update callers to pass the appropriate gameType explicitly.
73-96: Direct mutation of state parameter may surprise callers.The
applyPresetConfigmethod directly mutates thestateparameter by assigning properties. While this appears intentional (returning void signals mutation), it's less common in modern TypeScript patterns and can lead to reactivity issues in some frameworks. Consider documenting this mutation pattern clearly or returning a new state object instead.
131-136: Fallback preset name "Preset" may cause confusion.If
preset?.nameis missing or falsy, the fallback value"Preset"could collide with user-created presets of the same name or be confusing in the UI. Consider using a more descriptive fallback like"Untitled Preset"or handling missing names as an error condition.src/client/HostLobbyModal.ts (1)
678-747: Preset management logic duplicated with SinglePlayerModal.The methods
loadPresets,handlePresetSelect,handlePresetNameInput,buildPresetConfig,savePreset,applyPreset, anddeletePresetare nearly identical across HostLobbyModal and SinglePlayerModal. This duplication means any bug fix or enhancement must be applied twice, increasing maintenance burden.Consider extracting this logic into a shared mixin, base class, or helper module that both modals can use.
Based on learnings, the user prefers minimal scope in focused PRs, so this refactor could be deferred to a follow-up.
src/client/SinglePlayerModal.ts (1)
463-529: Preset logic duplicated from HostLobbyModal.The preset management methods in SinglePlayerModal are nearly identical to those in HostLobbyModal. Consider extracting this shared logic to reduce duplication and maintenance burden.
Based on learnings, scope creep should be avoided in focused PRs. This refactor can be addressed in a follow-up.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/HostLobbyModal.tssrc/client/SinglePlayerModal.tssrc/client/components/lobbyConfig/PresetControls.ts
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
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.
📚 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/client/SinglePlayerModal.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.tssrc/client/SinglePlayerModal.tssrc/client/components/lobbyConfig/PresetControls.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.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/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.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/client/HostLobbyModal.tssrc/client/SinglePlayerModal.ts
📚 Learning: 2025-06-01T04:56:16.870Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 992
File: src/core/configuration/DefaultConfig.ts:0-0
Timestamp: 2025-06-01T04:56:16.870Z
Learning: In the OpenFrontIO codebase, the number of player teams (`numPlayerTeams`) will never be higher than the minimum team size, so the modulo operation in `lobbyMaxPlayers()` will not result in zero players.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/client/SinglePlayerModal.ts
📚 Learning: 2025-08-14T10:07:44.588Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1811
File: src/client/FlagInput.ts:0-0
Timestamp: 2025-08-14T10:07:44.588Z
Learning: The codebase uses FlagSchema from "../core/Schemas" for validating flag values. The validation is done using FlagSchema.safeParse(value).success pattern, which is preferred over custom regex validation for flag input validation.
Applied to files:
src/client/components/lobbyConfig/PresetControls.ts
🧬 Code graph analysis (2)
src/client/HostLobbyModal.ts (3)
src/core/Schemas.ts (3)
defaultLobbySettings(183-203)LobbyPreset(209-209)LobbyPresetConfig(181-181)src/core/game/Game.ts (1)
HumansVsNations(56-56)src/client/components/lobbyConfig/PresetControls.ts (1)
PresetConfigState(41-52)
src/client/SinglePlayerModal.ts (3)
src/core/Schemas.ts (4)
defaultLobbySettings(183-203)TeamCountConfig(155-155)LobbyPreset(209-209)LobbyPresetConfig(181-181)src/core/game/Game.ts (1)
HumansVsNations(56-56)src/client/components/lobbyConfig/PresetControls.ts (1)
PresetConfigState(41-52)
🔇 Additional comments (8)
src/client/components/lobbyConfig/PresetControls.ts (1)
220-273: LGTM—Clean preset UI with proper translations.The render method correctly uses
translateTextfor all user-facing strings, includes appropriate event bindings, and applies dark mode styling. The button disable logic properly guards against empty selections.src/client/HostLobbyModal.ts (3)
98-100: Good addition of firstUpdated to load presets.Loading presets in
firstUpdatedensures they're available when the component is first rendered, which is the right lifecycle hook for this initialization.
197-207: Preset controls properly integrated with event wiring.The
lobby-preset-controlselement is correctly bound with all necessary properties and event handlers. The inline event handler forpreset-loadis clean and concise.
634-662: Preset loading integrated into open flow.Calling
loadPresets()at the start ofopen()ensures the preset list reflects any changes made in other modals or sessions. This is the correct placement for refresh logic.src/client/SinglePlayerModal.ts (4)
88-90: Consistent lifecycle usage for preset loading.Using
firstUpdatedto load presets matches the pattern in HostLobbyModal and ensures presets are available on first render.
103-113: PresetControls cleanly integrated.The preset controls component is properly bound with all required properties and event handlers, mirroring the HostLobbyModal integration.
531-535: Preset reload on open ensures fresh data.Calling
loadPresets()when opening the modal refreshes the preset list, which is the correct behavior for consistency with HostLobbyModal.
628-717: Start game flow correctly uses new state names.The
startGamemethod properly referencesgameMap,difficulty,playerTeams,donateGold, anddonateTroops, consistent with the renamed state properties. The HumansVsNations conditional logic correctly usesplayerTeams.
Aotumuri
force-pushed
the
lobby-config-preset
branch
from
7a45040 to
2dd10e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @src/client/components/lobbyConfig/PresetControls.ts:
- Around line 86-108: Replace the unsafe casts and key-copying in getPresetState
and applyPresetState with explicit typed parameters and property-by-property
assignments: define a PresetConfigSource (read-only) interface and a
PresetConfigTarget (writable) interface matching all LobbyPresetConfig fields,
change getPresetState(target: object) to getPresetState(source:
PresetConfigSource): PresetConfigState and return an object literal that maps
each named property (gameMap, difficulty, disableNPCs, bots, ... gameMapSize)
from source, and change applyPresetState(target: object, state:
PresetConfigState) to applyPresetState(target: PresetConfigTarget, state:
PresetConfigState) and assign each property explicitly from state to target;
remove reliance on copyPresetKeys for these two methods so the compiler can
verify types.
- Around line 60-70: buildPresetConfig currently takes an untyped object and
casts it, which loses compile-time safety; define a PresetConfigSource interface
describing all required properties (e.g., gameMap, difficulty, compactMap,
maxTimer, maxTimerValue, disabledUnits, etc.), change the signature of
buildPresetConfig to accept PresetConfigSource instead of object, replace the
unchecked property casts with direct property access from the typed parameter,
and update callers (e.g., HostLobbyModal, SinglePlayerModal) to implement/return
PresetConfigSource (or map their state to it) so the compiler enforces
correctness and eliminates unsafe casts.
In @src/client/HostLobbyModal.ts:
- Around line 709-724: The applyPreset flow applies a preset synchronously then
calls async operations without error handling; wrap the async section in a
try/catch inside applyPreset so failures during loadNationCount() or
putGameConfig() are caught, log the error (use existing logger or
console.error), show a user-facing error/notification, and restore UI state if
needed (e.g., revert changes or re-load current config) after
LobbyPresetControls.applyPresetConfig(this, config, ...) fails; refer to
applyPreset, loadNationCount, putGameConfig, and
LobbyPresetControls.applyPresetConfig when making the change.
In @src/client/SinglePlayerModal.ts:
- Around line 472-474: buildPresetConfig currently delegates to
LobbyPresetControls.buildPresetConfig(this) causing the same unsafe type-casting
issue flagged in PresetControls.ts; fix it by either implementing the config
assembly directly inside SinglePlayerModal.buildPresetConfig using this's known
properties (avoiding passing untyped this), or define a strict interface (e.g.,
SinglePlayerPresetSource) that exposes only the properties
LobbyPresetControls.buildPresetConfig needs and change the call to
LobbyPresetControls.buildPresetConfig(thisAsTyped) where thisAsTyped is typed to
that interface; update the method signature/locals accordingly so no unchecked
casts are used.
🧹 Nitpick comments (7)
src/client/HostLobbyModal.ts (3)
95-97: Consider using connectedCallback instead of firstUpdated.The
firstUpdatedlifecycle hook runs after the first render. For loading presets that don't depend on rendered DOM,connectedCallbackwould be more appropriate and align with the existing pattern in this codebase (lines 85-88 already use it for event listeners).♻️ Suggested refactor
connectedCallback() { super.connectedCallback(); window.addEventListener("keydown", this.handleKeyDown); + this.loadPresets(); }And remove the separate
firstUpdatedhook:- firstUpdated() { - this.loadPresets(); - }
687-689: Simplify buildPresetConfig to avoid indirect delegation.This method simply delegates to a static method on
LobbyPresetControls, passingthisas an untyped object. Consider building the config directly here for clarity and type safety.♻️ Proposed refactor
private buildPresetConfig(): LobbyPresetConfig { - return LobbyPresetControls.buildPresetConfig(this); + return { + gameMap: this.gameMap, + difficulty: this.difficulty, + disableNPCs: this.disableNPCs, + bots: this.bots, + infiniteGold: this.infiniteGold, + donateGold: this.donateGold, + infiniteTroops: this.infiniteTroops, + donateTroops: this.donateTroops, + gameType: GameType.Private, + gameMode: this.gameMode, + gameMapSize: this.compactMap ? GameMapSize.Compact : GameMapSize.Normal, + instantBuild: this.instantBuild, + randomSpawn: this.randomSpawn, + useRandomMap: this.useRandomMap, + compactMap: this.compactMap, + maxTimer: this.maxTimer, + maxTimerValue: this.maxTimer ? this.maxTimerValue : undefined, + playerTeams: this.playerTeams, + disabledUnits: this.disabledUnits, + }; }
631-632: Duplicate preset loading in open() method.Presets are loaded in both
firstUpdated()(line 96) andopen()(line 632). The duplicate call inopen()is unnecessary iffirstUpdatedalready ran.Remove the duplicate call:
public open() { - this.loadPresets(); this.lobbyCreatorClientID = generateID();If you need to refresh presets when opening the modal (e.g., if they might change externally), keep only the
open()call and remove thefirstUpdatedhook.src/client/components/lobbyConfig/PresetControls.ts (4)
72-84: copyPresetKeys indirection adds complexity without clear benefit.The
copyPresetKeyshelper iterates overlobbyPresetKeysand copies properties between records. This indirection makes the code harder to follow compared to explicit property assignment.Prefer explicit property copying in the methods that use this helper. Explicit code is easier to understand, refactor, and maintain, especially for non-native English speakers as noted in the guidelines.
// Instead of: LobbyPresetControls.copyPresetKeys(result, stateRecord); // Write: result.gameMap = stateRecord.gameMap; result.difficulty = stateRecord.difficulty; // ... etc for each propertyWhile more verbose, this approach is clearer, easier to search, and benefits from IDE autocomplete and type checking.
145-149: Unnecessary default UserSettings instantiation.
listPresetscreates a newUserSettings()instance when no argument is provided, but every caller in the codebase passes an explicit instance. The default is unused and misleading.Remove the default parameter:
- static listPresets(userSettings = new UserSettings()): LobbyPreset[] { + static listPresets(userSettings: UserSettings): LobbyPreset[] {This makes the API clearer and prevents accidental misuse.
178-183: Preset normalization provides weak guarantees.
normalizePresetuses fallback values (preset?.name ?? "Preset") but still returns aLobbyPresettype, giving a false sense of safety. Ifpresetis deeply malformed (e.g., from corrupted localStorage), the fallback may not be sufficient.Consider validating the entire preset object and returning a safe default if validation fails:
private static normalizePreset(preset: unknown): LobbyPreset { if (!preset || typeof preset !== 'object' || !('name' in preset) || !('config' in preset)) { console.warn('Invalid preset detected, using default'); return { name: 'Default', config: defaultLobbySettings, }; } const config = LobbyPresetControls.normalizePresetConfig( (preset as { config: unknown }).config as Partial<LobbyPresetConfig>, ); return { name: typeof (preset as { name: unknown }).name === 'string' ? (preset as { name: string }).name : 'Preset', config }; }
254-265: Consider disabling Save button when name is empty.The
handlePresetSavemethod silently returns if the name is empty. Users might expect feedback or a disabled button state instead.In the render method, disable the Save button when both
nameInputandselectedNameare empty or whitespace-only:<o-button title=${translateText("lobby_config.preset.save")} @click=${this.handlePresetSave} + ?disabled=${!this.nameInput.trim() && !this.selectedName.trim()} secondary ></o-button>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/HostLobbyModal.tssrc/client/SinglePlayerModal.tssrc/client/components/lobbyConfig/PresetControls.ts
🧰 Additional context used
🧠 Learnings (10)
📚 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/client/SinglePlayerModal.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.tssrc/client/components/lobbyConfig/PresetControls.tssrc/client/SinglePlayerModal.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.tssrc/client/SinglePlayerModal.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/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/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.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/client/HostLobbyModal.tssrc/client/SinglePlayerModal.ts
📚 Learning: 2025-06-01T04:56:16.870Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 992
File: src/core/configuration/DefaultConfig.ts:0-0
Timestamp: 2025-06-01T04:56:16.870Z
Learning: In the OpenFrontIO codebase, the number of player teams (`numPlayerTeams`) will never be higher than the minimum team size, so the modulo operation in `lobbyMaxPlayers()` will not result in zero players.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-08-14T10:07:44.588Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1811
File: src/client/FlagInput.ts:0-0
Timestamp: 2025-08-14T10:07:44.588Z
Learning: The codebase uses FlagSchema from "../core/Schemas" for validating flag values. The validation is done using FlagSchema.safeParse(value).success pattern, which is preferred over custom regex validation for flag input validation.
Applied to files:
src/client/components/lobbyConfig/PresetControls.ts
🧬 Code graph analysis (3)
src/client/HostLobbyModal.ts (2)
src/core/Schemas.ts (3)
defaultLobbySettings(183-203)LobbyPreset(209-209)LobbyPresetConfig(181-181)src/core/game/Game.ts (1)
HumansVsNations(56-56)
src/client/components/lobbyConfig/PresetControls.ts (2)
src/core/Schemas.ts (4)
LobbyPresetConfig(181-181)LobbyPreset(209-209)defaultLobbySettings(183-203)LobbyPresetConfigSchema(176-180)src/core/configuration/DefaultConfig.ts (1)
userSettings(276-281)
src/client/SinglePlayerModal.ts (1)
src/core/Schemas.ts (3)
defaultLobbySettings(183-203)LobbyPreset(209-209)LobbyPresetConfig(181-181)
🔇 Additional comments (1)
src/client/SinglePlayerModal.ts (1)
494-506: Inconsistent async behavior compared to HostLobbyModal.
applyPresetinSinglePlayerModalis synchronous, while the equivalent method inHostLobbyModal(lines 709-724) is async and callsloadNationCount()andputGameConfig().Is this intentional? If singleplayer mode doesn't need to sync with a server or load nation counts, this is fine. Otherwise, the methods should have consistent behavior.
Please verify whether
SinglePlayerModal.applyPresetshould also:
- Call any async initialization (e.g., load nation count)
- Be marked as
async
Aotumuri
force-pushed
the
lobby-config-preset
branch
from
2dd10e7 to
4882fe3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/client/HostLobbyModal.ts:
- Around line 674-689: The buildPresetConfig() method uses unsafe type
assertions (as unknown as Record<...>) and duplicates logic from
SinglePlayerModal.ts; replace this by creating a single, explicit, type-safe
factory that returns LobbyPresetConfig (no casts) and accept GameType as a
parameter, then have HostLobbyModal.buildPresetConfig call that helper with
GameType.Private and SinglePlayerModal call it with GameType.Singleplayer;
ensure the helper enumerates all LobbyPresetConfig fields (gameMap,
useRandomMap, difficulty, disableNPCs, bots, infiniteGold, donateGold,
infiniteTroops, donateTroops, gameMapSize via compactMap, instantBuild,
randomSpawn, compactMap, maxTimer, maxTimerValue conditional, gameMode,
playerTeams, disabledUnits, etc.) so TypeScript will catch missing/misnamed
properties and remove lobbyPresetKeys and the cast usage.
- Around line 691-718: applyPreset uses unsafe as-unknown type assertions and a
fragile lobbyPresetKeys loop that can create undefined/mismatched properties and
duplicates logic found in SinglePlayerModal; replace the looped dynamic
assignment with explicit, type-checked assignments for each preset field (e.g.,
difficulty, disableNPCs, bots, infiniteGold, donateGold, infiniteTroops,
donateTroops, instantBuild, randomSpawn, compactMap, maxTimer, maxTimerValue,
gameMode, playerTeams, disabledUnits) while keeping existing handling for
useRandomMap/getRandomMap and gameMapSize, remove the as unknown as Record<...>
cast and rely on LobbyPresetControls.normalizePresetConfig to type the config,
and factor the shared assignment logic into a new typed helper (e.g.,
applyPresetConfig) in PresetControls.ts that both HostLobbyModal.applyPreset and
SinglePlayerModal call; ensure loadNationCount and putGameConfig calls remain
after applying the typed fields.
In @src/client/SinglePlayerModal.ts:
- Around line 475-499: applyPreset uses an unsafe Record cast and immediately
randomizes the map; remove the fragile cast and instead copy the known
LobbyPresetKey properties explicitly from the normalized config into this (avoid
dynamic indexing like state[key]), and stop calling getRandomMap() inside
applyPreset—set this.gameMap = config.gameMap and this.useRandomMap =
config.useRandomMap ?? false (so map randomization is deferred to startGame
which already handles useRandomMap); also keep setting this.maxTimerValue,
this.gameType, and this.gameMapSize from the config as before and mirror the
safer pattern used in buildPresetConfig.
- Around line 458-473: The buildPresetConfig function uses an unsafe cast of
this and dynamic key access via lobbyPresetKeys; replace the loop and cast with
an explicit construction of the LobbyPresetConfig object by directly reading
each required property from this (e.g., assign each LobbyPresetKey property by
name), keep the existing logic for gameType, gameMapSize (use compactMap), and
maxTimerValue (use maxTimer ? maxTimerValue : undefined), and remove the Record
cast so TypeScript can statically verify property names and types.
🧹 Nitpick comments (4)
src/client/HostLobbyModal.ts (2)
42-42: Consider composition over inheritance for preset management.The class extends
PresetManagement(which extendsLitElement), creating a deep inheritance chain. This couples preset functionality tightly to the modal component. Consider using composition instead: extract preset management into a separate helper/service that the modal can instantiate and delegate to. This would improve testability, reusability, and follow the architectural principle of favoring composition over inheritance.♻️ Alternative composition-based approach
Create a separate preset manager class:
// PresetManager.ts class PresetManager { constructor(private userSettings: UserSettings) {} loadPresets(): LobbyPreset[] { /* ... */ } savePreset(name: string, config: LobbyPresetConfig): LobbyPreset[] { /* ... */ } deletePreset(name: string): LobbyPreset[] { /* ... */ } applyPreset(name: string): LobbyPresetConfig | null { /* ... */ } }Then use it in the modal:
export class HostLobbyModal extends LitElement { private presetManager = new PresetManager(new UserSettings()); // ... }This eliminates the inheritance chain and makes the preset functionality portable and testable in isolation.
93-95: Verify preset loading doesn't cause performance issues.
loadPresets()is called in bothfirstUpdated()(line 94) andopen()(line 631). This means presets are loaded twice when the modal opens for the first time. While the operation is likely inexpensive (reading from UserSettings), the duplicate call is unnecessary.Consider removing the call from
firstUpdated()sinceopen()already loads presets. Alternatively, add a guard to prevent double-loading:private presetsLoaded = false; protected loadPresets() { if (this.presetsLoaded) return; this.lobbyPresets = this.listPresets(); this.presetsLoaded = true; }src/client/components/lobbyConfig/PresetControls.ts (2)
40-117: Consider composition over inheritance for PresetManagement.The
PresetManagementbase class uses inheritance, requiring subclasses to extend it. A composition-based approach (e.g., a preset manager service or a custom hook-like pattern) would be more flexible and align better with modern TypeScript patterns. This would allow modals to "use" preset management without being locked into an inheritance hierarchy.For example, you could create a
PresetManagerclass that both modals instantiate and delegate to, similar to howUserSettingsis used.♻️ Composition alternative sketch
// Instead of a base class: export class PresetManager { private userSettings: UserSettings; public presets: LobbyPreset[] = []; public selectedName = ""; public nameInput = ""; constructor() { this.userSettings = new UserSettings(); } loadPresets() { /* ... */ } savePreset(name: string, config: LobbyPresetConfig) { /* ... */ } // ... other methods } // Then in modals: export class SinglePlayerModal extends LitElement { private presetManager = new PresetManager(); // Use presetManager.loadPresets(), etc. }This avoids inheritance and makes the preset logic reusable without extending LitElement.
132-144: Type assertions in normalizePresetConfig bypass safety checks.Lines 136 and 138 cast the merged config to
LobbyPresetDefaultsandLobbyPresetConfigwithout verification. While the fallback todefaultLobbySettingson parse failure (line 143) provides safety, the intermediate casts reduce type safety during merging.Consider removing the intermediate casts and relying on the schema parse result:
♻️ Safer typing alternative
static normalizePresetConfig( config: Partial<LobbyPresetConfig>, ): LobbyPresetConfig { - const merged = { - ...(defaultLobbySettings as LobbyPresetDefaults), - ...config, - } as LobbyPresetConfig; + const merged = { + ...defaultLobbySettings, + ...config, + }; const parsedResult = LobbyPresetConfigSchema.safeParse(merged); if (parsedResult.success) { return parsedResult.data; } return defaultLobbySettings as LobbyPresetConfig; }The schema validation provides the type guarantee; the casts add no value.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/HostLobbyModal.tssrc/client/SinglePlayerModal.tssrc/client/components/lobbyConfig/PresetControls.ts
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
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.
📚 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.tssrc/client/components/lobbyConfig/PresetControls.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/SinglePlayerModal.tssrc/client/components/lobbyConfig/PresetControls.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.tssrc/client/components/lobbyConfig/PresetControls.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/client/SinglePlayerModal.tssrc/client/HostLobbyModal.tssrc/client/components/lobbyConfig/PresetControls.ts
📚 Learning: 2026-01-02T18:11:15.132Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:15.132Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
Applied to files:
src/client/SinglePlayerModal.ts
📚 Learning: 2025-06-20T20:11:00.965Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with `new UserSettings()` in each component that needs them. This pattern is used consistently across at least 12+ files including OptionsMenu, EventsDisplay, DarkModeButton, Main, UserSettingModal, UsernameInput, NameLayer, AlertFrame, UILayer, InputHandler, ClientGameRunner, and GameView. This is the established architectural pattern and should be followed for consistency.
Applied to files:
src/client/SinglePlayerModal.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/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: 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-06-01T04:56:16.870Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 992
File: src/core/configuration/DefaultConfig.ts:0-0
Timestamp: 2025-06-01T04:56:16.870Z
Learning: In the OpenFrontIO codebase, the number of player teams (`numPlayerTeams`) will never be higher than the minimum team size, so the modulo operation in `lobbyMaxPlayers()` will not result in zero players.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-08-14T10:07:44.588Z
Learnt from: woodydrn
Repo: openfrontio/OpenFrontIO PR: 1811
File: src/client/FlagInput.ts:0-0
Timestamp: 2025-08-14T10:07:44.588Z
Learning: The codebase uses FlagSchema from "../core/Schemas" for validating flag values. The validation is done using FlagSchema.safeParse(value).success pattern, which is preferred over custom regex validation for flag input validation.
Applied to files:
src/client/components/lobbyConfig/PresetControls.ts
🧬 Code graph analysis (1)
src/client/SinglePlayerModal.ts (3)
src/client/components/lobbyConfig/PresetControls.ts (3)
PresetManagement(40-117)LobbyPresetKey(35-35)lobbyPresetKeys(13-33)src/core/Schemas.ts (2)
defaultLobbySettings(183-203)LobbyPresetConfig(181-181)src/core/game/Game.ts (1)
HumansVsNations(56-56)
🔇 Additional comments (3)
src/client/components/lobbyConfig/PresetControls.ts (1)
214-267: Clean event-driven UI pattern.The component properly delegates all logic to parent handlers via custom events. The disabled states and input handling are well implemented. The use of light DOM (
createRenderRoot) keeps styling consistent with the parent.src/client/SinglePlayerModal.ts (2)
82-84: Good use of lifecycle hook for preset loading.Using
firstUpdated()to load presets ensures they're available when the component renders. This complements theopen()call toloadPresets()(line 502), ensuring presets are fresh whether the modal is opened immediately or after DOM initialization.
97-108: Clean preset controls integration.The lobby preset controls are properly wired with reactive properties and event handlers. The inline callbacks for
preset-loadandpreset-savecorrectly invokeapplyPresetandsavePresetwith the appropriate config builder.
| private applyPreset(name?: string) { | ||
| const presetName = name ?? this.selectedPresetName; | ||
| const preset = this.lobbyPresets.find((p) => p.name === presetName); | ||
| if (!preset) { | ||
| return; | ||
| } | ||
|
|
||
| const config = LobbyPresetControls.normalizePresetConfig(preset.config); | ||
| this.useRandomMap = config.useRandomMap ?? false; | ||
| this.gameMap = this.useRandomMap ? this.getRandomMap() : config.gameMap; | ||
| const state = this as unknown as Record< | ||
| LobbyPresetKey, | ||
| LobbyPresetConfig[LobbyPresetKey] | ||
| >; | ||
| lobbyPresetKeys | ||
| .filter((key) => key !== "gameMap" && key !== "useRandomMap") | ||
| .forEach((key) => { | ||
| state[key] = config[key]; | ||
| }); | ||
| this.maxTimerValue = this.maxTimer ? config.maxTimerValue : undefined; | ||
| this.gameType = GameType.Singleplayer; | ||
| this.gameMapSize = this.compactMap | ||
| ? GameMapSize.Compact | ||
| : GameMapSize.Normal; | ||
| } |
There was a problem hiding this comment.
Same unsafe type cast pattern in applyPreset.
Lines 485-493 use the same fragile Record cast and dynamic property access as buildPresetConfig. This shares the same compile-time safety issues.
Additionally, line 484 immediately randomizes the map if useRandomMap is true when applying a preset. This might surprise users who expect to see the preset's configured map selection before starting the game. Consider deferring randomization to startGame() (line 598-607 already handles this).
🔒 Safer implementation
private applyPreset(name?: string) {
const presetName = name ?? this.selectedPresetName;
const preset = this.lobbyPresets.find((p) => p.name === presetName);
if (!preset) {
return;
}
const config = LobbyPresetControls.normalizePresetConfig(preset.config);
- this.useRandomMap = config.useRandomMap ?? false;
- this.gameMap = this.useRandomMap ? this.getRandomMap() : config.gameMap;
- const state = this as unknown as Record<
- LobbyPresetKey,
- LobbyPresetConfig[LobbyPresetKey]
- >;
- lobbyPresetKeys
- .filter((key) => key !== "gameMap" && key !== "useRandomMap")
- .forEach((key) => {
- state[key] = config[key];
- });
- this.maxTimerValue = this.maxTimer ? config.maxTimerValue : undefined;
+ this.gameMap = config.gameMap;
+ this.useRandomMap = config.useRandomMap ?? false;
+ this.difficulty = config.difficulty;
+ this.disableNPCs = config.disableNPCs;
+ this.bots = config.bots;
+ this.infiniteGold = config.infiniteGold;
+ this.donateGold = config.donateGold;
+ this.infiniteTroops = config.infiniteTroops;
+ this.donateTroops = config.donateTroops;
+ this.instantBuild = config.instantBuild;
+ this.randomSpawn = config.randomSpawn;
+ this.compactMap = config.compactMap;
+ this.maxTimer = config.maxTimer;
+ this.maxTimerValue = config.maxTimer ? config.maxTimerValue : undefined;
+ this.gameMode = config.gameMode;
+ this.playerTeams = config.playerTeams;
+ this.disabledUnits = config.disabledUnits ?? [];
this.gameType = GameType.Singleplayer;
this.gameMapSize = this.compactMap
? GameMapSize.Compact
: GameMapSize.Normal;
}This removes the cast and lets startGame() handle map randomization if needed.
🤖 Prompt for AI Agents
In @src/client/SinglePlayerModal.ts around lines 475 - 499, applyPreset uses an
unsafe Record cast and immediately randomizes the map; remove the fragile cast
and instead copy the known LobbyPresetKey properties explicitly from the
normalized config into this (avoid dynamic indexing like state[key]), and stop
calling getRandomMap() inside applyPreset—set this.gameMap = config.gameMap and
this.useRandomMap = config.useRandomMap ?? false (so map randomization is
deferred to startGame which already handles useRandomMap); also keep setting
this.maxTimerValue, this.gameType, and this.gameMapSize from the config as
before and mirror the safer pattern used in buildPresetConfig.
|
This has been partly fixed with the new UI rework. I will close this as there will be some incredible merge conflicts, and we can readdress this after v29 and that new system |
github-project-automation
bot
moved this from Development
to Complete
in OpenFront Release Management
7 participants
Resolves #2489
Description:
Add Host/Single lobby config presets with UI support for dark mode.
dark mode
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME