Add lobby preset #2566
Add lobby preset #2566
Conversation
|
|
WalkthroughAdds lobby preset support: new translation keys, preset schemas and defaults, persistence in UserSettings, a new LobbyPresetControls component, and integration into HostLobbyModal and SinglePlayerModal for listing, saving, applying, and deleting presets. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Modal as Host/SinglePlayer Modal
participant PresetUI as LobbyPresetControls
participant Settings as UserSettings
Note over User,Modal: Open modal
User->>Modal: open()
Modal->>Settings: getLobbyPresets()
Settings-->>Modal: presets[]
Modal->>PresetUI: render(presets)
Note over User,PresetUI: Select or load a preset
User->>PresetUI: select preset
PresetUI->>Modal: preset-select(name)
Modal->>Modal: applyPreset(name)
Note over User,PresetUI: Save a new preset
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)
Note over User,PresetUI: Delete a preset
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✅ Passed checks (5 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
…t management in HostLobbyModal and SinglePlayerModal
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/client/SinglePlayerModal.ts (1)
485-501: Save logic is correct but duplicated.This preset save logic works correctly. However, as noted in the past review comment by Tidwell, there's significant duplication between SinglePlayerModal and HostLobbyModal. The PresetControls component currently only handles UI, but the state management logic is repeated in both modals.
src/client/HostLobbyModal.ts (1)
665-676: Logic is correct but duplicated with SinglePlayerModal.These handlers work correctly but are identical to SinglePlayerModal. As noted in Tidwell's past review comment, this duplication could be avoided with better component extraction.
🧹 Nitpick comments (1)
src/client/components/lobbyConfig/PresetControls.ts (1)
26-43: Save preset logic is correct but lacks name validation.The logic correctly replaces presets with the same name. However, there's no validation on the preset name (e.g., length limits, forbidden characters). If you want to prevent issues with storage or display, consider adding name validation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/HostLobbyModal.ts(6 hunks)src/client/SinglePlayerModal.ts(7 hunks)src/client/components/lobbyConfig/PresetControls.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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-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/components/lobbyConfig/PresetControls.tssrc/client/HostLobbyModal.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/SinglePlayerModal.tssrc/client/HostLobbyModal.ts
📚 Learning: 2025-10-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/SinglePlayerModal.ts (1)
src/core/Schemas.ts (2)
LobbyPreset(187-187)LobbyPresetConfig(181-181)
src/client/HostLobbyModal.ts (1)
src/core/Schemas.ts (2)
LobbyPreset(187-187)LobbyPresetConfig(181-181)
🔇 Additional comments (27)
src/client/SinglePlayerModal.ts (11)
19-28: LGTM: Clean imports for preset functionality.The imports are well-organized and bring in the necessary types and components for lobby presets.
48-49: Verify donation defaults differ from multiplayer intentionally.Single player defaults
donateGoldanddonateTroopstotrue, while HostLobbyModal defaults them tofalse. If this is intentional (giving single players more permissive defaults), that's fine. Otherwise, consider aligning the defaults.
60-62: LGTM: Preset state properly declared.
76-78: LGTM: Presets loaded on first update.
91-101: LGTM: Preset controls properly integrated.The component binding and event wiring look correct.
451-461: LGTM: Preset loading and event handlers are correct.
463-483: LGTM: Preset config building is comprehensive.All lobby settings are correctly serialized for saving.
503-530: LGTM: Apply preset logic is correct for single player.The synchronous implementation is appropriate since single player doesn't need server coordination.
532-544: LGTM: Delete preset logic is sound.
546-550: LGTM: Presets refreshed on modal open.Loading presets both on first update and when opening ensures the UI reflects the latest saved presets.
704-705: LGTM: Donation toggles properly integrated.The donation settings are correctly included in the game configuration payload.
src/client/HostLobbyModal.ts (9)
23-30: LGTM: Preset imports properly added.
65-67: LGTM: Preset state declared correctly.
85-87: LGTM: Presets loaded on initialization.
184-194: LGTM: Preset controls properly integrated.
621-649: LGTM: Presets refreshed when opening host lobby.
677-697: LGTM: Config building is comprehensive.All lobby settings are properly serialized, matching the pattern in SinglePlayerModal.
699-715: LGTM: Save preset logic is correct.
717-747: LGTM: Async apply preset correctly syncs with server.This async implementation properly calls
loadNationCount()andputGameConfig()to update the server-side lobby state. This is the correct approach for multiplayer, unlike SinglePlayerModal which doesn't need server synchronization.
749-761: LGTM: Delete preset logic is sound.src/client/components/lobbyConfig/PresetControls.ts (7)
1-12: LGTM: Imports are clean and well-organized.
14-18: LGTM: Component properties properly declared.
20-24: LGTM: List presets correctly normalizes loaded data.
45-51: LGTM: Delete preset logic is straightforward.
85-87: LGTM: Light DOM rendering for styling.
89-151: LGTM: Event handlers are well-structured.All handlers correctly validate inputs and dispatch events that bubble up to parent components.
153-206: LGTM: Preset controls UI is well-designed.The UI properly handles empty states, includes dark mode support, and all text is internationalized. Good work on accessibility and UX.
|
please review |
| private static normalizePresetConfig( | ||
| config: Partial<LobbyPresetConfig>, | ||
| ): LobbyPresetConfig { | ||
| const parsed = LobbyPresetConfigSchema.parse(config); |
There was a problem hiding this comment.
You can do what coderabbit is saying, or maybe even better, use safeParse, which doesn't throw on invalid schema, but instead returns an object with info on what failed.
…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? |
6 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