Conversation
WalkthroughConsolidates game configuration into a new GameConfigForm component, refactors HostLobbyModal and SinglePlayerModal to use it, adds EventBus-driven lobby updates with debounced config propagation, changes lobby creation to token-based Authorization, introduces ConfigCards helpers, and removes legacy render helpers and inline option state. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HostLobbyModal
participant GameConfigForm
participant EventBus
participant Server
User->>HostLobbyModal: open modal
HostLobbyModal->>GameConfigForm: render variant="host"
HostLobbyModal->>EventBus: startLobbyUpdates()
EventBus-->>HostLobbyModal: LobbyInfoEvent
HostLobbyModal->>HostLobbyModal: handleLobbyInfo (sync clients/lobbyCreator)
User->>GameConfigForm: change config
GameConfigForm-->>HostLobbyModal: config-changed
HostLobbyModal->>HostLobbyModal: debounced handleConfigChanged()
HostLobbyModal->>Server: PUT /game/config (Authorization: Bearer token)
User->>HostLobbyModal: click start
HostLobbyModal->>HostLobbyModal: flush pending config updates
HostLobbyModal->>Server: POST /start_game (Authorization: Bearer token)
User->>HostLobbyModal: close
HostLobbyModal->>GameConfigForm: reset()
HostLobbyModal->>EventBus: stopLobbyUpdates()
sequenceDiagram
participant User
participant SinglePlayerModal
participant GameConfigForm
participant Server
User->>SinglePlayerModal: open modal
SinglePlayerModal->>GameConfigForm: render variant="singleplayer"
User->>GameConfigForm: select options
GameConfigForm-->>SinglePlayerModal: config-changed
User->>SinglePlayerModal: click start
SinglePlayerModal->>GameConfigForm: getConfig()
GameConfigForm-->>SinglePlayerModal: GameConfigSnapshot
SinglePlayerModal->>Server: POST create/join lobby with payload from config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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 |
There was a problem hiding this comment.
Pull request overview
This PR refactors the game configuration UI into shared “card” utilities and a new game-config-form component, while also updating several interactive elements from div/span[role=button] to real <button> elements for improved semantics.
Changes:
- Introduces
src/client/components/GameConfigForm.tsandsrc/client/utilities/ConfigCards.tsto consolidate config UI rendering/styling and reuse it in both Single Player and Host Lobby modals. - Updates map selection UI (
MapPicker/MapDisplay) to use the shared card helpers and<button>-based interactions. - Updates
FluentSlidervalue display and related test to use a<button>instead of aspan[role=button], plus similardiv->buttonchange inUserSettingModal.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/client/components/FluentSlider.test.ts | Updates assertions for the value display now being a real <button>. |
| src/client/utilities/RenderUnitTypeOptions.ts | Removed; functionality moved into shared ConfigCards helpers. |
| src/client/utilities/RenderToggleInputCard.ts | Removed; functionality moved into ConfigCards + GameConfigForm. |
| src/client/utilities/ConfigCards.ts | New shared Tailwind/card rendering utilities + unit option rendering. |
| src/client/components/map/MapPicker.ts | Uses shared card helpers and reduces duplicated markup/classes. |
| src/client/components/map/MapDisplay.ts | Converts the map card wrapper to a real <button> and uses shared card helpers. |
| src/client/components/GameConfigForm.ts | New consolidated config form component emitting config-changed snapshots. |
| src/client/components/FluentSlider.ts | Converts editable value display from span[role=button] to <button>. |
| src/client/UserSettingModal.ts | Converts flag selector container to a <button>. |
| src/client/SinglePlayerModal.ts | Refactors to embed game-config-form and read config snapshots from it. |
| src/client/HostLobbyModal.ts | Refactors to embed game-config-form, debounce config updates, and changes lobby creation/joining flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
|
|
||
| private handleMaxTimerValueKeyDown = (e: KeyboardEvent) => { | ||
| if (["-", "+", "e"].includes(e.key)) { |
There was a problem hiding this comment.
handleMaxTimerValueKeyDown blocks "e" but not "E", even though the sanitizer in handleMaxTimerValueChanges strips both via /[e+-]/gi. For consistency (and to prevent uppercase scientific notation input in some browsers), include "E" in the blocked keys (or reuse handleNumberKeyDown).
| if (["-", "+", "e"].includes(e.key)) { | |
| if (["-", "+", "e", "E"].includes(e.key)) { |
| import { html } from "lit"; | ||
| import { customElement, property, query, state } from "lit/decorators.js"; | ||
| import { translateText } from "../client/Utils"; | ||
| import { getServerConfigFromClient } from "../core/configuration/ConfigLoader"; | ||
| import { EventBus } from "../core/EventBus"; | ||
| import { | ||
| Difficulty, | ||
| Duos, | ||
| GameMapSize, | ||
| GameMapType, | ||
| GameMode, | ||
| HumansVsNations, | ||
| Quads, | ||
| Trios, | ||
| UnitType, | ||
| } from "../core/game/Game"; | ||
| import { | ||
| ClientInfo, | ||
| GameConfig, | ||
| GameInfo, | ||
| TeamCountConfig, | ||
| LobbyInfoEvent, | ||
| isValidGameID, | ||
| } from "../core/Schemas"; | ||
| import { generateID } from "../core/Util"; | ||
| import { getClientIDForGame } from "./Auth"; | ||
| import { getPlayToken } from "./Auth"; | ||
| import "./components/baseComponents/Modal"; | ||
| import { BaseModal } from "./components/BaseModal"; | ||
| import "./components/CopyButton"; | ||
| import "./components/Difficulties"; | ||
| import "./components/FluentSlider"; | ||
| import "./components/GameConfigForm"; | ||
| import { |
There was a problem hiding this comment.
PR description says "only tailwind changes / div->button fixes", but this PR also introduces a new game-config-form component, refactors both SinglePlayer and HostLobby flows, changes lobby creation auth/parameters, and adds an EventBus subscription. Please update the PR description (or split the PR) so reviewers can correctly assess the behavioral/API impact.
src/client/HostLobbyModal.ts
Outdated
| if (!this.lobbyId || lobby.gameID !== this.lobbyId) { | ||
| return; | ||
| } | ||
| this.lobbyCreatorClientID = event.myClientID; |
There was a problem hiding this comment.
LobbyInfoEvent only contains lobby (see src/core/Schemas.ts), so event.myClientID does not exist and will fail type-checking / be undefined at runtime. If you need the current client ID here, store it explicitly (e.g., from the join-lobby flow) or derive host/creator IDs from lobby.clients similarly to JoinLobbyModal.updateFromLobby().
| this.lobbyCreatorClientID = event.myClientID; |
| new CustomEvent("join-lobby", { | ||
| detail: { | ||
| gameID: this.lobbyId, | ||
| clientID: this.lobbyCreatorClientID, | ||
| source: "host", | ||
| } as JoinLobbyEvent, |
There was a problem hiding this comment.
The dispatched join-lobby event is missing the required clientID field of JoinLobbyEvent. This will break joining the lobby (and downstream logic that relies on a stable clientID). Generate a clientID (e.g., via getClientIDForGame(this.lobbyId)) and include it in the event detail.
| this.lobbyId = generateID(); | ||
| this.lobbyCreatorClientID = getClientIDForGame(this.lobbyId); | ||
| // Note: clientID will be assigned by server when we join the lobby | ||
| // lobbyCreatorClientID stays empty until then | ||
|
|
||
| createLobby(this.lobbyCreatorClientID, this.lobbyId) | ||
| // Pass auth token for creator identification (server extracts persistentID from it) | ||
| createLobby(this.lobbyId) | ||
| .then(async (lobby) => { |
There was a problem hiding this comment.
Host lobby creation no longer supplies a creatorClientID (and no initial update-game-config is sent). Since server-side defaults differ (e.g., server default difficulty is Medium, while GameConfigForm defaults to Easy), players joining the lobby can see/configure against the wrong initial settings until the host changes something or presses Start. Consider pushing the initial config to the server immediately after opening (or have game-config-form emit an initial config-changed so the host modal can call putGameConfig once).
ryanbarlow97
force-pushed
the
uichanges
branch
from
566f82d to
4009009
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/HostLobbyModal.ts (1)
209-237:⚠️ Potential issue | 🟡 MinorLobby creation and join event may race with EventBus subscription.
startLobbyUpdates()is called on line 210 withthis.lobbyIdset to a localgenerateID()value (line 211). ThencreateLobbymay return a differentgameID(line 218 overwritesthis.lobbyId). During this window, anyLobbyInfoEventarriving for the server-assigned ID would be silently dropped by the guard on line 60.Consider moving
startLobbyUpdates()after the lobby is confirmed created (i.e., inside the.thenchain after line 218), so the filter uses the server-confirmedlobbyId.Suggested reorder
protected onOpen(): void { - this.startLobbyUpdates(); this.lobbyId = generateID(); createLobby(this.lobbyId) .then(async (lobby) => { this.lobbyId = lobby.gameID; if (!isValidGameID(this.lobbyId)) { throw new Error(`Invalid lobby ID format: ${this.lobbyId}`); } + this.startLobbyUpdates(); crazyGamesSDK.showInviteButton(this.lobbyId); const url = await this.constructUrl(); this.updateHistory(url); })
🤖 Fix all issues with AI agents
In `@src/client/components/GameConfigForm.ts`:
- Around line 788-792: The handleMaxTimerValueKeyDown currently blocks ["-",
"+", "e"] but misses uppercase "E"; update handleMaxTimerValueKeyDown to include
"E" in the blocked keys (matching handleNumberKeyDown) so the max-timer input
prevents both "e" and "E" keystrokes at keydown time and avoids the brief
invalid-input flash; locate the method named handleMaxTimerValueKeyDown and add
"E" to its blocked-keys array.
In `@src/client/HostLobbyModal.ts`:
- Around line 58-67: The handler handleLobbyInfo is reading event.myClientID
which doesn't exist on LobbyInfoEvent; instead read the client ID from the
GameInfo object (event.lobby). Update the assignment to set lobbyCreatorClientID
= event.lobby.<correctClientIdProperty> (replace <correctClientIdProperty> with
the actual field name from the GameInfo schema, e.g., creatorClientID or
hostClientID), and keep the rest of the logic (clients assignment) unchanged.
In `@src/client/SinglePlayerModal.ts`:
- Around line 278-282: SinglePlayerModal currently sets donateGold and
donateTroops based on gameMode rather than the user-configured toggles, so the
GameConfigForm toggles are ignored; update SinglePlayerModal to pass
config.donateGold and config.donateTroops into the modal state (replace the
hardcoded gameMode checks for donateGold/donateTroops) so the single-player form
respects user choices (mirror HostLobbyModal behavior), or alternatively remove
the donateGold/donateTroops controls from the singleplayer GameConfigForm if
those options should not apply in single-player mode.
In `@src/client/utilities/ConfigCards.ts`:
- Around line 67-78: The button returned by renderConfigCard lacks an explicit
type, so when placed inside a form it will default to type="submit" and may
cause unwanted submissions; update the JSX/template in renderConfigCard to add
type="button" to the <button> element (same place where BASE_CARD and
cardStateClasses are used) so it matches renderToggleCard, ensuring opts.onClick
behavior remains unchanged and disabled/selected handling stays the same.
🧹 Nitpick comments (6)
src/client/components/map/MapPicker.ts (2)
105-117: Good use ofrole="tab"andaria-selectedin the tab helper.For full ARIA tab pattern compliance, each tab should have an
aria-controlspointing to its panel'sid, and panels should haverole="tabpanel". Not blocking, but worth considering for screen reader users who navigate tab structures.
144-161: Random map button: consider addingtype="button"for consistency.Same reasoning as the
renderConfigCardcomment inConfigCards.ts— the button defaults totype="submit"without it. Low risk here since there's no ancestor form, but consistent practice prevents surprises.Proposed fix
<button + type="button" class="relative group rounded-xl border transition-all duration-200 overflow-hidden flex flex-col items-stretch active:scale-95 ${cardStateClasses(src/client/components/GameConfigForm.ts (1)
240-245:resolveSelectedMap()mutates internal state as a side effect.Calling this public method changes
this.selectedMapwhenuseRandomMapis true. Callers reading the name "resolve" might not expect it to change component state. Consider either documenting this clearly or returning a value without mutation (let the caller set the map if needed).Alternative: pure resolution without side effect
public resolveSelectedMap(): GameMapType { - if (this.useRandomMap) { - this.selectedMap = this.getRandomMap(); - } - return this.selectedMap; + return this.useRandomMap ? this.getRandomMap() : this.selectedMap; }If the caller needs to persist the resolved map, they can do so explicitly.
src/client/HostLobbyModal.ts (2)
99-112: Redundant polling alongside EventBus subscription.
startLobbyUpdates(line 99) subscribes toLobbyInfoEventviaEventBusto receive client list updates, butpollPlayers(line 365) also runs every second viasetInterval(line 243) to fetch the sameclientsdata over HTTP. Both paths write tothis.clients.If the EventBus subscription is the intended replacement for polling, the interval should be removed (or kept only as a fallback with a much longer period). Right now every lobby pays the cost of both a real-time event stream and a 1-second HTTP poll.
Also applies to: 243-243, 365-377
389-405: Stale-check inloadNationCountis solid but duplicated.The pattern of capturing
currentMapbefore theawait, then re-readinglastConfig?.selectedMapafter, is a good guard against stale updates. However, the fallback?? GameMapType.Worldis repeated three times (lines 390, 394, 400). A small helper or a singleconst fallbackat the top of the method would reduce duplication.Optional cleanup
private async loadNationCount() { - const currentMap = this.lastConfig?.selectedMap ?? GameMapType.World; + const getMap = () => this.lastConfig?.selectedMap ?? GameMapType.World; + const currentMap = getMap(); try { const mapData = this.mapLoader.getMapData(currentMap); const manifest = await mapData.manifest(); - const latestMap = this.lastConfig?.selectedMap ?? GameMapType.World; - if (latestMap === currentMap) { + if (getMap() === currentMap) { this.nationCount = manifest.nations.length; } } catch (error) { console.warn("Failed to load nation count", error); - const latestMap = this.lastConfig?.selectedMap ?? GameMapType.World; - if (latestMap === currentMap) { + if (getMap() === currentMap) { this.nationCount = 0; } } }src/client/SinglePlayerModal.ts (1)
200-201:this.configForm.getConfig()has no null guard.The
@querydecorator with!(line 32) assumes the element always exists whenstartGameruns. IfstartGamefires before the form renders (unlikely but possible during fast interactions), this throws. Consider adding a guard or at least an early return.Small safety net
private async startGame() { - const config = this.configForm.getConfig(); + const config = this.configForm?.getConfig(); + if (!config) return;
| private handleMaxTimerValueKeyDown = (e: KeyboardEvent) => { | ||
| if (["-", "+", "e"].includes(e.key)) { | ||
| e.preventDefault(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
handleMaxTimerValueKeyDown is missing "E" from the blocked keys, unlike handleNumberKeyDown.
handleNumberKeyDown (line 783) blocks ["-", "+", "e", "E"], but handleMaxTimerValueKeyDown only blocks ["-", "+", "e"]. A user typing uppercase "E" in the max timer input bypasses the filter. The handleMaxTimerValueChanges handler does strip it after the fact, but it's inconsistent and allows a brief flash of invalid input.
Proposed fix
private handleMaxTimerValueKeyDown = (e: KeyboardEvent) => {
- if (["-", "+", "e"].includes(e.key)) {
+ if (["-", "+", "e", "E"].includes(e.key)) {
e.preventDefault();
}
};🤖 Prompt for AI Agents
In `@src/client/components/GameConfigForm.ts` around lines 788 - 792, The
handleMaxTimerValueKeyDown currently blocks ["-", "+", "e"] but misses uppercase
"E"; update handleMaxTimerValueKeyDown to include "E" in the blocked keys
(matching handleNumberKeyDown) so the max-timer input prevents both "e" and "E"
keystrokes at keydown time and avoids the brief invalid-input flash; locate the
method named handleMaxTimerValueKeyDown and add "E" to its blocked-keys array.
| return html` | ||
| <button | ||
| ?disabled=${opts.disabled} | ||
| @click=${opts.onClick} | ||
| class="${BASE_CARD} justify-center ${cardStateClasses( | ||
| opts.selected, | ||
| opts, | ||
| )}" | ||
| > | ||
| ${opts.content ?? renderCardLabel(opts.label ?? "", active)} | ||
| </button> | ||
| `; |
There was a problem hiding this comment.
Missing type="button" on the <button> in renderConfigCard.
renderToggleCard (line 122) correctly sets type="button", but renderConfigCard does not. If this card ends up inside a <form>, the button defaults to type="submit" and could trigger unintended form submission.
Proposed fix
<button
+ type="button"
?disabled=${opts.disabled}
`@click`=${opts.onClick}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return html` | |
| <button | |
| ?disabled=${opts.disabled} | |
| @click=${opts.onClick} | |
| class="${BASE_CARD} justify-center ${cardStateClasses( | |
| opts.selected, | |
| opts, | |
| )}" | |
| > | |
| ${opts.content ?? renderCardLabel(opts.label ?? "", active)} | |
| </button> | |
| `; | |
| return html` | |
| <button | |
| type="button" | |
| ?disabled=${opts.disabled} | |
| `@click`=${opts.onClick} | |
| class="${BASE_CARD} justify-center ${cardStateClasses( | |
| opts.selected, | |
| opts, | |
| )}" | |
| > | |
| ${opts.content ?? renderCardLabel(opts.label ?? "", active)} | |
| </button> | |
| `; |
🤖 Prompt for AI Agents
In `@src/client/utilities/ConfigCards.ts` around lines 67 - 78, The button
returned by renderConfigCard lacks an explicit type, so when placed inside a
form it will default to type="submit" and may cause unwanted submissions; update
the JSX/template in renderConfigCard to add type="button" to the <button>
element (same place where BASE_CARD and cardStateClasses are used) so it matches
renderToggleCard, ensuring opts.onClick behavior remains unchanged and
disabled/selected handling stays the same.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/client/HostLobbyModal.ts (3)
209-244:⚠️ Potential issue | 🟠 MajorNo error handling when
createLobbyfails — user stuck in a broken modalIf
createLobbyrejects (network error, server 5xx, etc.), the promise chain on line 216 has no.catch. The modal stays open with an emptylobbyId, the polling interval keeps firing against an invalid endpoint, and the user has no feedback that something went wrong.Suggested fix — add a catch block
createLobby(this.lobbyId) .then(async (lobby) => { this.lobbyId = lobby.gameID; if (!isValidGameID(this.lobbyId)) { throw new Error(`Invalid lobby ID format: ${this.lobbyId}`); } crazyGamesSDK.showInviteButton(this.lobbyId); const url = await this.constructUrl(); this.updateHistory(url); }) .then(() => { this.dispatchEvent( new CustomEvent("join-lobby", { detail: { gameID: this.lobbyId, source: "host", } as JoinLobbyEvent, bubbles: true, composed: true, }), ); - }); + }) + .catch((error) => { + console.error("Failed to create lobby:", error); + this.close(); + });
365-377:⚠️ Potential issue | 🟡 Minor
pollPlayersswallows network errors silentlyThe
.thenchain has no.catch. A transient network failure or non-JSON response will produce an unhandled promise rejection. A simple.catchwith a warning log keeps things tidy.Suggested fix
private async pollPlayers() { const config = await getServerConfigFromClient(); fetch(`/${config.workerPath(this.lobbyId)}/api/game/${this.lobbyId}`, { method: "GET", headers: { "Content-Type": "application/json", }, }) .then((response) => response.json()) .then((data: GameInfo) => { this.clients = data.clients ?? []; - }); + }) + .catch((err) => { + console.warn("pollPlayers failed:", err); + }); }
333-363:⚠️ Potential issue | 🔴 Critical
startGamesends no auth token—any client can start another player's private lobbyThe
startGameendpoint accepts requests from any client with a valid lobby ID. UnlikecreateLobby, which requires aBearertoken, thePOST /api/start_game/{id}handler on the server (src/server/Worker.ts:177) has no auth check and will start the game for anyone.An attacker who knows a lobby ID can force-start someone else's game. The only protection is the
game.isPublic()check, which does nothing for private lobbies.Suggested fix
private async startGame() { // Flush any pending debounced config update if (this.configUpdateTimer !== null) { clearTimeout(this.configUpdateTimer); this.configUpdateTimer = null; } await this.putGameConfig(); const c = this.configForm?.getConfig(); console.log( `Starting private game with map: ${c ? GameMapType[c.selectedMap as keyof typeof GameMapType] : "unknown"}${c?.useRandomMap ? " (Randomly selected)" : ""}`, ); this.leaveLobbyOnClose = false; const serverConfig = await getServerConfigFromClient(); + const token = await getPlayToken(); const response = await fetch( `${window.location.origin}/${serverConfig.workerPath(this.lobbyId)}/api/start_game/${this.lobbyId}`, { method: "POST", headers: { "Content-Type": "application/json", + Authorization: `Bearer ${token}`, }, }, ); if (!response.ok) { this.leaveLobbyOnClose = true; } return response; }Also update the server-side handler to verify the token and check it belongs to the game creator.
🤖 Fix all issues with AI agents
In `@src/client/SinglePlayerModal.ts`:
- Around line 220-222: The log line in SinglePlayerModal uses
GameMapType[selectedMap as keyof typeof GameMapType], which does a reverse enum
lookup and can produce undefined for string enums; instead log the selectedMap
directly or resolve the enum key from the value before formatting. Update the
console.log in SinglePlayerModal (the line referencing GameMapType and
selectedMap) to either use selectedMap as-is (e.g., `${selectedMap}`) or
implement a small lookup that finds the enum key by value (e.g., iterate
Object.keys(GameMapType) to match GameMapType[key] === selectedMap) and then log
that resolved key along with the existing "(Randomly selected)" suffix.
🧹 Nitpick comments (4)
src/client/HostLobbyModal.ts (2)
99-112: Dual update path: EventBus subscription and 1-second polling both updatethis.clients
startLobbyUpdatessubscribes toLobbyInfoEventon the EventBus (line 107), whilepollPlayersfetches the same data via REST every second (line 243). Both write tothis.clients. If the EventBus path is reliable, the polling interval is redundant network traffic. If it is a deliberate fallback, a short comment explaining why would help future readers.Also applies to: 243-243, 365-377
139-142:this.configFormmay benullwhen accessed via@querybefore the form renders
@query("game-config-form")returnsnulluntil the element is in the DOM. Line 276 safely uses optional chaining (this.configForm?.reset()), but line 287 also falls back with??. Just double-check thatgetConfig()is never called before the first render — the non-null assertion on line 38 (configForm!) masks this at compile time.Also applies to: 276-276
src/client/components/map/MapPicker.ts (1)
144-161: Random map card label always shows as active.
renderCardLabel(translateText("map.random"), true)always passesactive = true. Should this reflectthis.useRandomMapso the label dims when the random map is not selected, matching how other map cards behave?- ${renderCardLabel(translateText("map.random"), true)} + ${renderCardLabel(translateText("map.random"), this.useRandomMap)}src/client/utilities/ConfigCards.ts (1)
178-191:renderUnitTypeOptionstoggle logic looks correct but the callback arg name could be clearer.
toggleUnit(type, isEnabled)passes the current enabled state. The consumer (GameConfigForm.toggleUnit) interprets the second arg aschecked(i.e., "is it currently on?") and flips accordingly. This works, but the naming between "isEnabled" here and "checked" in the consumer can confuse readers.Consider renaming for clarity:
- const isEnabled = !disabledUnits.includes(type); + const currentlyEnabled = !disabledUnits.includes(type); return renderConfigCard({ - selected: isEnabled, + selected: currentlyEnabled, dimWhenOff: true, - onClick: () => toggleUnit(type, isEnabled), + onClick: () => toggleUnit(type, currentlyEnabled),
| console.log( | ||
| `Starting single player game with map: ${GameMapType[this.selectedMap as keyof typeof GameMapType]}${this.useRandomMap ? " (Randomly selected)" : ""}`, | ||
| `Starting single player game with map: ${GameMapType[selectedMap as keyof typeof GameMapType]}${config.useRandomMap ? " (Randomly selected)" : ""}`, | ||
| ); |
There was a problem hiding this comment.
Logging may print undefined for the map name.
GameMapType[selectedMap as keyof typeof GameMapType] does a reverse lookup using the enum value as a key. If GameMapType is a string enum where keys differ from values (e.g., World = "world"), this resolves to undefined.
Consider using selectedMap directly, or look up the key from value:
- `Starting single player game with map: ${GameMapType[selectedMap as keyof typeof GameMapType]}${config.useRandomMap ? " (Randomly selected)" : ""}`,
+ `Starting single player game with map: ${selectedMap}${config.useRandomMap ? " (Randomly selected)" : ""}`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log( | |
| `Starting single player game with map: ${GameMapType[this.selectedMap as keyof typeof GameMapType]}${this.useRandomMap ? " (Randomly selected)" : ""}`, | |
| `Starting single player game with map: ${GameMapType[selectedMap as keyof typeof GameMapType]}${config.useRandomMap ? " (Randomly selected)" : ""}`, | |
| ); | |
| console.log( | |
| `Starting single player game with map: ${selectedMap}${config.useRandomMap ? " (Randomly selected)" : ""}`, | |
| ); |
🤖 Prompt for AI Agents
In `@src/client/SinglePlayerModal.ts` around lines 220 - 222, The log line in
SinglePlayerModal uses GameMapType[selectedMap as keyof typeof GameMapType],
which does a reverse enum lookup and can produce undefined for string enums;
instead log the selectedMap directly or resolve the enum key from the value
before formatting. Update the console.log in SinglePlayerModal (the line
referencing GameMapType and selectedMap) to either use selectedMap as-is (e.g.,
`${selectedMap}`) or implement a small lookup that finds the enum key by value
(e.g., iterate Object.keys(GameMapType) to match GameMapType[key] ===
selectedMap) and then log that resolved key along with the existing "(Randomly
selected)" suffix.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/client/components/GameConfigForm.ts`:
- Around line 628-672: The gold multiplier and starting gold cards are using
hardcoded translation keys ("single_modal.gold_multiplier",
"single_modal.starting_gold", etc.); update both
renderToggleInputCardWithHandlers calls to use the modal/variant-aware keys from
ModalTranslationKeys (or the shared key object used elsewhere) instead of the
hardcoded strings so the host variant shows correct labels; specifically replace
the labelKey and any translateText(...) placeholders/ariaLabel usages in the
goldMultiplier block (related symbols: goldMultiplier, goldMultiplierValue,
handleGoldMultiplierValueChanges, renderToggleInputCardWithHandlers) and the
startingGold block (startingGold, startingGoldValue,
handleStartingGoldValueChanges, renderToggleInputCardWithHandlers) to reference
ModalTranslationKeys properties for gold_multiplier and starting_gold.
- Around line 449-473: The renderTeamCountSection currently calls translateText
with hardcoded "host_modal.team_count" and "host_modal.teams_*" keys which
bypass the ModalTranslationKeys mechanism; update renderTeamCountSection to use
the ModalTranslationKeys enum (or add the missing team_count and teams_* keys to
ModalTranslationKeys) and pass those enum values into translateText so the
singleplayer/host translation variant resolves correctly; specifically change
the calls inside renderTeamCountSection (and the label resolution for team
sizes) to reference ModalTranslationKeys entries and ensure translateText is
used with those keys, keeping the existing handleTeamCountSelection and
renderConfigCard usage intact.
- Around line 868-875: The handler handleSpawnImmunityDurationInput currently
returns early on invalid input and leaves spawnImmunityDurationMinutes stale;
change it to mirror handleMaxTimerValueChanges/handleStartingGoldValueChanges by
setting this.spawnImmunityDurationMinutes = undefined when parsed value is NaN
or out of range, then call this.fireConfigChanged() so state and UI remain in
sync (keep the input sanitization and valid-path behavior intact).
- Around line 424-447: renderGameModeSection currently uses hardcoded
"host_modal.mode" and thus breaks singleplayer translations; change its
signature to accept the variant-aware keys object (same shape used by
renderOptionsSection), replace the hardcoded translateText("host_modal.mode")
with translateText(keys.mode), and update any callers to pass the keys object;
inside the function keep using this.gameMode, handleGameModeSelection,
renderConfigCard and GameMode constants as-is so only the translation key wiring
changes.
🧹 Nitpick comments (3)
src/client/components/GameConfigForm.ts (3)
698-711: Unusedhiddenparameter onrenderOptionToggle.The
hiddenparameter defaults tofalseand is never passed astrueanywhere in this file. Consider removing it to keep the API clean. If needed later, add it back then.Proposed fix
private renderOptionToggle( labelKey: string, checked: boolean, onChange: (val: boolean) => void, - hidden: boolean = false, ): TemplateResult { - if (hidden) return html``; return renderConfigCard({
253-332: Consider reducing the field-list repetition acrossgetConfig,reset, andhasOptionsChanged.Every config field is enumerated in at least four places:
DEFAULT_CONFIG,@statedeclarations,getConfig(), andreset(). Adding a new option means touching all of them. A helper that iterates overDEFAULT_CONFIGkeys forreset()(and similarly forgetConfig()) would cut the surface area for mistakes.Not urgent — the TypeScript interface gives compile-time safety — but worth considering as the option count grows.
713-755:checkedandtoggleGetteralways carry the same value — pick one.Every call site passes
checked: this.someFlagalongsidetoggleGetter: () => this.someFlag. If they ever diverge, the rendered state and the click-handler logic will disagree. DroppingtoggleGetterand reading!opts.checkedinsidetoggleLogicremoves the duplication and the risk.Sketch
private renderToggleInputCardWithHandlers(opts: { labelKey: string; checked: boolean; - toggleGetter: () => boolean; toggleSetter: (val: boolean) => void; valueGetter: () => number | undefined; valueSetter: (val: number | undefined) => void; defaultValue: number; inputOptions: { /* ... */ }; }): TemplateResult { const toggleLogic = (e: Event) => { if ((e.target as HTMLElement).tagName.toLowerCase() === "input") return; - const newState = !opts.toggleGetter(); + const newState = !opts.checked; opts.toggleSetter(newState);
| private renderGameModeSection(): TemplateResult { | ||
| return html` | ||
| <div class="space-y-6"> | ||
| ${renderSectionHeader( | ||
| gameModeIcon, | ||
| "bg-purple-500/20 text-purple-400", | ||
| translateText("host_modal.mode"), | ||
| )} | ||
| <div class="grid grid-cols-2 gap-4"> | ||
| ${[GameMode.FFA, GameMode.Team].map((mode) => { | ||
| const isSelected = this.gameMode === mode; | ||
| return renderConfigCard({ | ||
| selected: isSelected, | ||
| onClick: () => this.handleGameModeSelection(mode), | ||
| label: | ||
| mode === GameMode.FFA | ||
| ? translateText("game_mode.ffa") | ||
| : translateText("game_mode.teams"), | ||
| }); | ||
| })} | ||
| </div> | ||
| </div> | ||
| `; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find translation files
fd 'en\.json' --type fRepository: openfrontio/OpenFrontIO
Length of output: 89
🏁 Script executed:
# Check translation keys for host_modal and single_modal
rg '"(?:host_modal|single_modal)\.mode"' --type jsonRepository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Check the GameConfigForm component to see if it accepts a modal type parameter
fd 'GameConfigForm\.ts' --type f --exec head -100 {}Repository: openfrontio/OpenFrontIO
Length of output: 2689
🏁 Script executed:
# Check all uses of renderGameModeSection and how GameConfigForm is instantiated
rg 'GameConfigForm|renderGameModeSection' --type ts --type tsx -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 94
🏁 Script executed:
# Read the translation file to see the actual structure
cat resources/lang/en.json | jq '.host_modal // .single_modal // .' | head -50Repository: openfrontio/OpenFrontIO
Length of output: 1310
🏁 Script executed:
# Get more context from GameConfigForm to see variant parameter and other section renders
sed -n '1,150p' src/client/components/GameConfigForm.ts | tail -60Repository: openfrontio/OpenFrontIO
Length of output: 1811
🏁 Script executed:
# Find how variant is used in the component class
rg 'variant.*GameConfigFormVariant|this\.variant' src/client/components/GameConfigForm.ts -A 2 -B 2Repository: openfrontio/OpenFrontIO
Length of output: 1046
🏁 Script executed:
# Search for other renderSection methods to see if they handle variant
rg 'private render.*Section\(\)' src/client/components/GameConfigForm.ts -A 10Repository: openfrontio/OpenFrontIO
Length of output: 1725
🏁 Script executed:
# Check if single_modal section exists in en.json
jq 'keys | map(select(test("modal")))' resources/lang/en.jsonRepository: openfrontio/OpenFrontIO
Length of output: 329
🏁 Script executed:
# Get the full structure of both modal sections if they exist
jq '{host_modal, single_modal}' resources/lang/en.json 2>/dev/null | head -100Repository: openfrontio/OpenFrontIO
Length of output: 2333
🏁 Script executed:
# Find all hardcoded host_modal keys in GameConfigForm
rg '"host_modal\.' src/client/components/GameConfigForm.tsRepository: openfrontio/OpenFrontIO
Length of output: 983
🏁 Script executed:
# Find the main render method and see which sections are rendered
sed -n '200,400p' src/client/components/GameConfigForm.ts | grep -E 'render\(\)|renderGameModeSection|renderTeamCountSection|renderDifficultySection'Repository: openfrontio/OpenFrontIO
Length of output: 275
🏁 Script executed:
# Get the full render method to see how sections are called
rg 'render\(\):' src/client/components/GameConfigForm.ts -A 50Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Get line numbers and full render method
rg -n 'render\(\):' src/client/components/GameConfigForm.tsRepository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
# Get the full render method with line numbers
sed -n '150,250p' src/client/components/GameConfigForm.ts | cat -nRepository: openfrontio/OpenFrontIO
Length of output: 5892
🏁 Script executed:
# Search for the render method definition and show lines after it
rg -n 'render\(\)' src/client/components/GameConfigForm.ts | head -5Repository: openfrontio/OpenFrontIO
Length of output: 83
🏁 Script executed:
# Get the render method starting from a specific line
sed -n '200,300p' src/client/components/GameConfigForm.tsRepository: openfrontio/OpenFrontIO
Length of output: 4523
🏁 Script executed:
# Get the render method starting from line 348
sed -n '348,380p' src/client/components/GameConfigForm.tsRepository: openfrontio/OpenFrontIO
Length of output: 1197
Game mode and team count sections hardcode host_modal translation keys for both variants.
These sections are rendered unconditionally in both singleplayer and host variants, but they use hardcoded "host_modal.mode" and "host_modal.team_count" keys. The singleplayer variant's translation section (single_modal in en.json) does not contain these keys, only the host variant does. Pass the variant-aware keys object to these render methods, consistent with how renderOptionsSection() handles variant-specific translations.
🤖 Prompt for AI Agents
In `@src/client/components/GameConfigForm.ts` around lines 424 - 447,
renderGameModeSection currently uses hardcoded "host_modal.mode" and thus breaks
singleplayer translations; change its signature to accept the variant-aware keys
object (same shape used by renderOptionsSection), replace the hardcoded
translateText("host_modal.mode") with translateText(keys.mode), and update any
callers to pass the keys object; inside the function keep using this.gameMode,
handleGameModeSelection, renderConfigCard and GameMode constants as-is so only
the translation key wiring changes.
| private renderTeamCountSection(): TemplateResult { | ||
| if (this.gameMode === GameMode.FFA) return html``; | ||
|
|
||
| return html` | ||
| <div class="space-y-6"> | ||
| ${renderCategoryLabel(translateText("host_modal.team_count"))} | ||
| <div class="grid grid-cols-2 md:grid-cols-5 gap-3"> | ||
| ${[2, 3, 4, 5, 6, 7, Quads, Trios, Duos, HumansVsNations].map((o) => { | ||
| const isSelected = this.teamCount === o; | ||
| const label = | ||
| typeof o === "string" | ||
| ? o === HumansVsNations | ||
| ? translateText("public_lobby.teams_hvn") | ||
| : translateText(`host_modal.teams_${o}`) | ||
| : translateText("public_lobby.teams", { num: o }); | ||
| return renderConfigCard({ | ||
| selected: isSelected, | ||
| onClick: () => this.handleTeamCountSelection(o), | ||
| label, | ||
| }); | ||
| })} | ||
| </div> | ||
| </div> | ||
| `; | ||
| } |
There was a problem hiding this comment.
Team count section also hardcodes "host_modal.*" keys.
Lines 454 and 462 use "host_modal.team_count" and "host_modal.teams_*" directly, bypassing the ModalTranslationKeys mechanism. Same concern as the game mode section — singleplayer variant would render host-specific translations.
If both variants show team options, consider adding these keys to ModalTranslationKeys so each variant resolves the right string.
🤖 Prompt for AI Agents
In `@src/client/components/GameConfigForm.ts` around lines 449 - 473, The
renderTeamCountSection currently calls translateText with hardcoded
"host_modal.team_count" and "host_modal.teams_*" keys which bypass the
ModalTranslationKeys mechanism; update renderTeamCountSection to use the
ModalTranslationKeys enum (or add the missing team_count and teams_* keys to
ModalTranslationKeys) and pass those enum values into translateText so the
singleplayer/host translation variant resolves correctly; specifically change
the calls inside renderTeamCountSection (and the label resolution for team
sizes) to reference ModalTranslationKeys entries and ensure translateText is
used with those keys, keeping the existing handleTeamCountSelection and
renderConfigCard usage intact.
| <!-- Gold Multiplier --> | ||
| ${this.renderToggleInputCardWithHandlers({ | ||
| labelKey: "single_modal.gold_multiplier", | ||
| checked: this.goldMultiplier, | ||
| toggleGetter: () => this.goldMultiplier, | ||
| toggleSetter: (val) => (this.goldMultiplier = val), | ||
| valueGetter: () => this.goldMultiplierValue, | ||
| valueSetter: (val) => (this.goldMultiplierValue = val), | ||
| defaultValue: 2, | ||
| inputOptions: { | ||
| id: "gold-multiplier-value", | ||
| min: 0.1, | ||
| max: 1000, | ||
| step: "any", | ||
| ariaLabel: translateText("single_modal.gold_multiplier"), | ||
| placeholder: translateText( | ||
| "single_modal.gold_multiplier_placeholder", | ||
| ), | ||
| onChange: this.handleGoldMultiplierValueChanges, | ||
| onKeyDown: this.handleNumberKeyDown, | ||
| }, | ||
| })} | ||
|
|
||
| <!-- Starting Gold --> | ||
| ${this.renderToggleInputCardWithHandlers({ | ||
| labelKey: "single_modal.starting_gold", | ||
| checked: this.startingGold, | ||
| toggleGetter: () => this.startingGold, | ||
| toggleSetter: (val) => (this.startingGold = val), | ||
| valueGetter: () => this.startingGoldValue, | ||
| valueSetter: (val) => (this.startingGoldValue = val), | ||
| defaultValue: 5000000, | ||
| inputOptions: { | ||
| id: "starting-gold-value", | ||
| min: 0, | ||
| max: 1000000000, | ||
| step: 100000, | ||
| ariaLabel: translateText("single_modal.starting_gold"), | ||
| placeholder: translateText( | ||
| "single_modal.starting_gold_placeholder", | ||
| ), | ||
| onInput: this.handleStartingGoldValueChanges, | ||
| onKeyDown: this.handleNumberKeyDown, | ||
| }, | ||
| })} |
There was a problem hiding this comment.
Gold multiplier and starting gold cards hardcode "single_modal.*" keys.
These sections use "single_modal.gold_multiplier", "single_modal.starting_gold", etc. directly — so the host variant would show singleplayer-labelled text. Same root cause as the game mode / team count sections: these keys should be routed through ModalTranslationKeys (or a shared key) so both variants render the correct translations.
🤖 Prompt for AI Agents
In `@src/client/components/GameConfigForm.ts` around lines 628 - 672, The gold
multiplier and starting gold cards are using hardcoded translation keys
("single_modal.gold_multiplier", "single_modal.starting_gold", etc.); update
both renderToggleInputCardWithHandlers calls to use the modal/variant-aware keys
from ModalTranslationKeys (or the shared key object used elsewhere) instead of
the hardcoded strings so the host variant shows correct labels; specifically
replace the labelKey and any translateText(...) placeholders/ariaLabel usages in
the goldMultiplier block (related symbols: goldMultiplier, goldMultiplierValue,
handleGoldMultiplierValueChanges, renderToggleInputCardWithHandlers) and the
startingGold block (startingGold, startingGoldValue,
handleStartingGoldValueChanges, renderToggleInputCardWithHandlers) to reference
ModalTranslationKeys properties for gold_multiplier and starting_gold.
| private handleSpawnImmunityDurationInput = (e: Event) => { | ||
| const input = e.target as HTMLInputElement; | ||
| input.value = input.value.replace(/[eE+-]/g, ""); | ||
| const value = parseInt(input.value, 10); | ||
| if (Number.isNaN(value) || value < 0 || value > 120) return; | ||
| this.spawnImmunityDurationMinutes = value; | ||
| this.fireConfigChanged(); | ||
| }; |
There was a problem hiding this comment.
Inconsistent invalid-input handling: spawn immunity silently keeps stale state.
Other handlers (handleMaxTimerValueChanges, handleStartingGoldValueChanges) set the backing state to undefined when the input is out of range, keeping state and UI in sync. This handler does an early return instead, so the displayed input can show an invalid value while spawnImmunityDurationMinutes retains its previous (now-stale) value.
Proposed fix — align with the other handlers
private handleSpawnImmunityDurationInput = (e: Event) => {
const input = e.target as HTMLInputElement;
input.value = input.value.replace(/[eE+-]/g, "");
const value = parseInt(input.value, 10);
- if (Number.isNaN(value) || value < 0 || value > 120) return;
- this.spawnImmunityDurationMinutes = value;
+ if (Number.isNaN(value) || value < 0 || value > 120) {
+ this.spawnImmunityDurationMinutes = undefined;
+ } else {
+ this.spawnImmunityDurationMinutes = value;
+ }
this.fireConfigChanged();
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private handleSpawnImmunityDurationInput = (e: Event) => { | |
| const input = e.target as HTMLInputElement; | |
| input.value = input.value.replace(/[eE+-]/g, ""); | |
| const value = parseInt(input.value, 10); | |
| if (Number.isNaN(value) || value < 0 || value > 120) return; | |
| this.spawnImmunityDurationMinutes = value; | |
| this.fireConfigChanged(); | |
| }; | |
| private handleSpawnImmunityDurationInput = (e: Event) => { | |
| const input = e.target as HTMLInputElement; | |
| input.value = input.value.replace(/[eE+-]/g, ""); | |
| const value = parseInt(input.value, 10); | |
| if (Number.isNaN(value) || value < 0 || value > 120) { | |
| this.spawnImmunityDurationMinutes = undefined; | |
| } else { | |
| this.spawnImmunityDurationMinutes = value; | |
| } | |
| this.fireConfigChanged(); | |
| }; |
🤖 Prompt for AI Agents
In `@src/client/components/GameConfigForm.ts` around lines 868 - 875, The handler
handleSpawnImmunityDurationInput currently returns early on invalid input and
leaves spawnImmunityDurationMinutes stale; change it to mirror
handleMaxTimerValueChanges/handleStartingGoldValueChanges by setting
this.spawnImmunityDurationMinutes = undefined when parsed value is NaN or out of
range, then call this.fireConfigChanged() so state and UI remain in sync (keep
the input sanitization and valid-path behavior intact).
1 participant
Description:
Simplifies UI - only tailwind changes / div->button fixes
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
w.o.n