Conversation
WalkthroughA comprehensive refactoring consolidates game configuration state and UI into a new GameConfigForm component. HostLobbyModal and SinglePlayerModal are refactored to eliminate duplicated per-property state, replacing it with centralized form management. Supporting utilities are reorganized: two modules are removed and replaced with a unified ConfigCards utility. Related components are updated to align with new rendering patterns. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
github-project-automation
bot
moved this from Triage
to Final Review
in OpenFront Release Management
ryanbarlow97
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
ryanbarlow97
force-pushed
the
uichanges
branch
from
877f9db to
1c4ba62
Compare
ryanbarlow97
force-pushed
the
uichanges
branch
from
1c4ba62 to
e692e6f
Compare
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 (3)
src/server/GameServer.ts (2)
710-717:⚠️ Potential issue | 🟠 Major
endTurnsends to all active clients without checking WebSocket readyState.Line 716 calls
c.ws.send(msg)without confirmingc.ws.readyState === WebSocket.OPEN. If a WebSocket is in CLOSING state between theaddListenersrace guard and the nextendTurn, this could throw. Compare withbroadcastLobbyInfo(line 605) which does checkreadyState.Suggested fix
this.activeClients.forEach((c) => { - c.ws.send(msg); + if (c.ws.readyState === WebSocket.OPEN) { + c.ws.send(msg); + } });
926-933:⚠️ Potential issue | 🟡 Minor
markClientDisconnectedcreates aTurnIntentwhereclientIDmeans "target", not "sender".This ties to the
MarkDisconnectedIntentSchemaissue noted inSchemas.ts. TheclientIDhere is the player being marked disconnected, butTurnIntent.clientIDis normally the sender. InExecutionManager.createExec,intent.clientIDis used to find the player — which coincidentally works because the target IS the player to operate on. But this dual meaning could bite if someone refactors the intent handling to distinguish sender from target.src/client/HostLobbyModal.ts (1)
216-237:⚠️ Potential issue | 🟠 MajorMissing
.catch()oncreateLobbypromise chain — failed lobby creation silently swallowed.If
createLobbyrejects (e.g., 401 from invalid token, network error), the unhandled rejection leaves the user stuck on the host modal with no feedback. Add a.catch()to show an error message and close the modal.Proposed fix
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); + window.dispatchEvent( + new CustomEvent("show-message", { + detail: { + message: translateText("host_modal.create_failed"), + color: "red", + duration: 3000, + }, + }), + ); + this.close(); });
🤖 Fix all issues with AI agents
In `@src/client/components/GameConfigForm.ts`:
- Around line 788-792: The max-timer keydown handler handleMaxTimerValueKeyDown
currently prevents "-", "+", and "e" but misses uppercase "E", allowing
scientific notation like "1E2"; update handleMaxTimerValueKeyDown to block ["-",
"+", "e", "E"] (matching handleNumberKeyDown) so uppercase E is prevented and
the input can't be entered in scientific notation.
- Around line 830-837: The handler handleSpawnImmunityDurationInput currently
returns early on invalid input and leaves this.spawnImmunityDurationMinutes
unchanged, causing a UI/state desync; update the function so that when input is
NaN or outside 0–120 you set this.spawnImmunityDurationMinutes = undefined
(matching handleMaxTimerValueChanges behavior) and then call
this.fireConfigChanged(), otherwise parse and assign the valid value as now.
In `@src/core/Schemas.ts`:
- Around line 399-403: Rename the ambiguous clientID in
MarkDisconnectedIntentSchema to targetClientID to distinguish the disconnected
player from the intent sender: update the schema export
(MarkDisconnectedIntentSchema) to use targetClientID: ID, then update all code
that constructs or reads this intent (e.g., GameServer.markClientDisconnected
where the intent is created/queued and any ExecutionManager.createExec or
consumers that currently access intent.clientID) to read/write
intent.targetClientID instead; also adjust any tests/types that rely on
MarkDisconnectedIntentSchema to use the new field name so the semantics no
longer conflict with TurnIntentSchema's clientID.
- Around line 447-449: TurnIntentSchema currently applies clientID via
IntentSchema.and(...), which creates a ZodIntersection and can block object-only
methods and cause TypeScript inference issues; instead, for each intent variant
(e.g., AttackIntentSchema, CancelAttackIntentSchema, MoveIntentSchema, etc.)
apply .and(z.object({ clientID: ID })) to produce per-variant schemas, then
build TurnIntentSchema with z.discriminatedUnion("type", [A, B, ...]) using
those augmented variants; update the TurnIntentSchema declaration to use that
discriminated union pattern and keep the TurnIntent type as Intent & { clientID:
ClientID } if needed.
🧹 Nitpick comments (10)
src/client/components/GameConfigForm.ts (2)
30-54:DEFAULT_CONFIGwithas constplus type casts is a workable pattern.The
undefined as number | undefinedcasts insideas constare needed to keep the types wide enough for mutable state. This works, but the interfaceGameConfigSnapshotalready defines the canonical shape — consider whetheras constadds value here since the object is only used for reset defaults, not as a type source.
239-245:resolveSelectedMap()mutates internal state as a side effect.This public method both writes (
this.selectedMap = this.getRandomMap()) and returns a value. Callers might not expect a "getter-like" method to change component state. Consider either:
- Making it purely return a value without mutating, or
- Renaming to something like
finalizeSelectedMap()to signal the side effect.src/client/utilities/ConfigCards.ts (2)
58-79:renderConfigCardcorrectly uses<button>with?disabledbinding.The Lit
?disableddirective properly toggles the HTMLdisabledattribute. When disabled, thecursor-not-allowedclass fromcardStateClassesprovides visual feedback. Theactive:scale-95inBASE_CARDwill still apply on disabled buttons in some browsers though — consider gating it.
165-191:UNIT_OPTIONSis a hardcoded list — new unit types require a manual update here.This is fine for now, but if
UnitTypegrows frequently, consider deriving the list from the enum to avoid drift. Low priority since the unit roster is likely stable.src/client/components/map/MapDisplay.ts (1)
89-90:renderCardLabelalways called withactive: true— label is always bright white.This means the label text does not visually change between selected and unselected states. If that is intentional for readability on map cards, this is fine. But if you want the label to dim alongside the card when unselected, pass
this.selectedinstead:-${renderCardLabel(this.translation ?? this.mapName ?? "", true)} +${renderCardLabel(this.translation ?? this.mapName ?? "", this.selected)}src/client/components/map/MapPicker.ts (1)
158-160:renderCardLabelalways receivestrue— should it reflect selection state?The second argument to
renderCardLabelcontrols the text style (white vs white/60). Passingtruealways means the "Random" label looks active even when another map is selected. Consider passingthis.useRandomMapfor visual consistency with other cards.Suggested fix
<div class="p-3 border-t border-white/5"> - ${renderCardLabel(translateText("map.random"), true)} + ${renderCardLabel(translateText("map.random"), this.useRandomMap)} </div>src/core/execution/ExecutionManager.ts (1)
36-43: Remove unusedclientIDparameter from the Executor constructor.The
private clientID: ClientIDparameter is never referenced in this class. SincecreateExecusesintent.clientIDfrom theTurnIntentdirectly, storingclientIDon the class is unnecessary and can be safely removed.src/client/HostLobbyModal.ts (1)
365-377: HTTP polling for players still active alongside EventBus updates.
pollPlayersfetches clients via HTTP every second, whilehandleLobbyInfoalso updatesthis.clientsvia EventBus. The dual-path update is not harmful (last-write-wins onthis.clients), but it's unnecessary network traffic once the EventBus is connected. Consider removing the polling interval or keeping it only as a fallback with a longer interval.src/client/ClientGameRunner.ts (2)
195-210: MultipleclientID!non-null assertions across the file rely on implicit ordering guarantees.
clientIDcan beundefined, yet it's used with!at lines 227, 233, 247, 307, 308, 559, 594, 655, 674. Today the flow guaranteesclientIDis set before these paths run (thestartmessage assigns it beforecreateClientGame, and gameplay handlers run after start). But if the call order ever shifts, these become silent bugs.A lightweight guard at the top of
createClientGamewould make the contract explicit:Proposed guard
async function createClientGame( lobbyConfig: LobbyConfig, clientID: ClientID | undefined, ... ): Promise<ClientGameRunner> { if (lobbyConfig.gameStartInfo === undefined) { throw new Error("missing gameStartInfo"); } // For local games only, derive clientID from the first player in GameStartInfo if (!clientID && transport.isLocal) { clientID = lobbyConfig.gameStartInfo.players[0]?.clientID; } + if (!clientID) { + throw new Error("missing clientID: server did not assign one"); + } const config = await getConfig(This replaces many scattered
!assertions with a single, clear fail point.
767-795:showErrorModalnow acceptsclientID: ClientID | undefined— displays "undefined" when unset.At line 790,
client id: ${clientID}will render as"client id: undefined"if the client never received an ID. This is acceptable for error diagnostics (it tells support the ID was never assigned), but consider displaying a more user-friendly fallback like"unknown".Minor readability tweak
- `client id: ${clientID}`, + `client id: ${clientID ?? "unknown"}`,
| private handleMaxTimerValueKeyDown = (e: KeyboardEvent) => { | ||
| if (["-", "+", "e"].includes(e.key)) { | ||
| e.preventDefault(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Missing uppercase "E" in handleMaxTimerValueKeyDown.
handleNumberKeyDown (line 783) blocks ["-", "+", "e", "E"], but handleMaxTimerValueKeyDown only blocks ["-", "+", "e"]. This lets users type 1E2 (scientific notation) into the max timer input, which parseInt would parse as 1 (ignoring E2).
Proposed fix
private handleMaxTimerValueKeyDown = (e: KeyboardEvent) => {
- if (["-", "+", "e"].includes(e.key)) {
+ if (["-", "+", "e", "E"].includes(e.key)) {
e.preventDefault();
}
};📝 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 handleMaxTimerValueKeyDown = (e: KeyboardEvent) => { | |
| if (["-", "+", "e"].includes(e.key)) { | |
| e.preventDefault(); | |
| } | |
| }; | |
| private handleMaxTimerValueKeyDown = (e: KeyboardEvent) => { | |
| if (["-", "+", "e", "E"].includes(e.key)) { | |
| e.preventDefault(); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@src/client/components/GameConfigForm.ts` around lines 788 - 792, The
max-timer keydown handler handleMaxTimerValueKeyDown currently prevents "-",
"+", and "e" but misses uppercase "E", allowing scientific notation like "1E2";
update handleMaxTimerValueKeyDown to block ["-", "+", "e", "E"] (matching
handleNumberKeyDown) so uppercase E is prevented and the input can't be entered
in scientific notation.
| 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 out-of-range handling in spawn immunity input.
Other handlers (e.g., handleMaxTimerValueChanges on line 798) set the value to undefined when input is invalid. This handler silently returns, leaving the old state value in place while the input field shows the invalid number — a visual desync.
Proposed fix — align with 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 830 - 837, The handler
handleSpawnImmunityDurationInput currently returns early on invalid input and
leaves this.spawnImmunityDurationMinutes unchanged, causing a UI/state desync;
update the function so that when input is NaN or outside 0–120 you set
this.spawnImmunityDurationMinutes = undefined (matching
handleMaxTimerValueChanges behavior) and then call this.fireConfigChanged(),
otherwise parse and assign the valid value as now.
src/core/Schemas.ts
Outdated
| export const MarkDisconnectedIntentSchema = z.object({ | ||
| type: z.literal("mark_disconnected"), | ||
| clientID: ID, | ||
| isDisconnected: z.boolean(), | ||
| }); |
There was a problem hiding this comment.
clientID in MarkDisconnectedIntentSchema overlaps with TurnIntentSchema's clientID — different meanings, same field name.
MarkDisconnectedIntentSchema has its own clientID: ID representing the player being disconnected. But TurnIntentSchema (line 448) also adds clientID: ID representing who issued the intent. Because both use the same field name, the intersection merges them silently.
It works today because the server creates mark_disconnected intents directly (in GameServer.markClientDisconnected) and sets clientID to the target player — which is also the player ExecutionManager.createExec looks up. But this conflation of "sender" and "target" in the same field is fragile.
Consider renaming the field in MarkDisconnectedIntentSchema to something like targetClientID to make the semantic difference clear and prevent future bugs if the intent ever needs to track both sender and target.
🤖 Prompt for AI Agents
In `@src/core/Schemas.ts` around lines 399 - 403, Rename the ambiguous clientID in
MarkDisconnectedIntentSchema to targetClientID to distinguish the disconnected
player from the intent sender: update the schema export
(MarkDisconnectedIntentSchema) to use targetClientID: ID, then update all code
that constructs or reads this intent (e.g., GameServer.markClientDisconnected
where the intent is created/queued and any ExecutionManager.createExec or
consumers that currently access intent.clientID) to read/write
intent.targetClientID instead; also adjust any tests/types that rely on
MarkDisconnectedIntentSchema to use the new field name so the semantics no
longer conflict with TurnIntentSchema's clientID.
src/core/Schemas.ts
Outdated
| // TurnIntent = Intent with server-stamped clientID (used in turns and execution) | ||
| export const TurnIntentSchema = IntentSchema.and(z.object({ clientID: ID })); | ||
| export type TurnIntent = Intent & { clientID: ClientID }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the IntentSchema definition
rg "IntentSchema" --type ts -B 2 -A 2Repository: openfrontio/OpenFrontIO
Length of output: 9972
🏁 Script executed:
# Look for the file structure around the review comment
head -n 460 src/core/Schemas.ts | tail -n 40Repository: openfrontio/OpenFrontIO
Length of output: 1178
🏁 Script executed:
# Search for test files related to TurnIntentSchema
fd "test|spec" --type f -e ts | xargs rg "TurnIntentSchema" 2>/dev/null | head -20Repository: openfrontio/OpenFrontIO
Length of output: 49
🌐 Web query:
Zod 4 discriminatedUnion .and() intersection method
💡 Result:
In Zod v4, .and() is just the intersection operator (z.intersection(a, b)), and it can be used with a z.discriminatedUnion(...). The result is a ZodIntersection, not a ZodObject. That means you generally can’t call object-only helpers like .extend() / .merge() on the result. [1][2]
Recommended pattern (add common fields to every variant)
Instead of intersecting the whole union, apply the common object to each option, then rebuild the discriminated union:
import { z } from "zod/v4";
const Common = z.object({ someData: z.string() });
const A = z.object({ type: z.literal("A"), a: z.string() }).and(Common);
const B = z.object({ type: z.literal("B"), b: z.number() }).and(Common);
const U = z.discriminatedUnion("type", [A, B]);
type T = z.infer<typeof U>;Known TypeScript edge case
There’s a known TS inference failure where a discriminatedUnion(...).and(...) combined with recursion can cause the inferred type to become unknown (see issue #4714). [3]
Sources: Zod union/intersection docs summary (DeepWiki) [1], Zod v4 changelog note on intersections [2], Zod issue about .and + discriminated union + recursion inference (#4714) [3].
Consider using the recommended pattern: apply clientID to each intent variant before creating the discriminated union.
In Zod 4, .and() on a z.discriminatedUnion technically works and creates a ZodIntersection. However, this pattern has known limitations: .and() produces a type that can't use object-only methods like .extend() or .merge(), and there's a documented TypeScript inference edge case (issue #4714) where inference can fail to unknown in recursive scenarios.
The recommended pattern is to apply the common field to each variant first, then build the discriminated union:
const A = AttackIntentSchema.and(z.object({ clientID: ID }));
const B = CancelAttackIntentSchema.and(z.object({ clientID: ID }));
// ... apply to all variants ...
const TurnIntentSchema = z.discriminatedUnion("type", [A, B, /* ... */]);The current implementation works for your use case (simple array usage without recursion), but switching to this pattern would be clearer and align with Zod 4 best practices.
🤖 Prompt for AI Agents
In `@src/core/Schemas.ts` around lines 447 - 449, TurnIntentSchema currently
applies clientID via IntentSchema.and(...), which creates a ZodIntersection and
can block object-only methods and cause TypeScript inference issues; instead,
for each intent variant (e.g., AttackIntentSchema, CancelAttackIntentSchema,
MoveIntentSchema, etc.) apply .and(z.object({ clientID: ID })) to produce
per-variant schemas, then build TurnIntentSchema with
z.discriminatedUnion("type", [A, B, ...]) using those augmented variants; update
the TurnIntentSchema declaration to use that discriminated union pattern and
keep the TurnIntent type as Intent & { clientID: ClientID } if needed.
e692e6f to
566f82d
Compare
github-project-automation
bot
moved this from Development
to Complete
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 (2)
src/client/HostLobbyModal.ts (2)
209-245:⚠️ Potential issue | 🟠 MajorNo
awaitoncreateLobby— errors in the second.thensilently lost.The promise chain from
createLobbyis not awaited or returned, so any rejection in the second.then(thejoin-lobbydispatch) is an unhandled promise rejection. Also,this.playersIntervalstarts immediately (line 243) before the lobby is confirmed created — ifcreateLobbyfails, you'll poll a nonexistent lobby.Consider awaiting the chain or adding a
.catchto handle failures and stop the polling interval:Suggested approach
- createLobby(this.lobbyId) + createLobby(this.lobbyId) .then(async (lobby) => { // ...existing logic... }) .then(() => { // ...dispatch join-lobby... }) + .catch((error) => { + console.error("Failed to create lobby:", error); + if (this.playersInterval) { + clearInterval(this.playersInterval); + this.playersInterval = null; + } + });
365-377:⚠️ Potential issue | 🟡 MinorPolling
pollPlayershas no error handling for fetch failures.If the fetch fails (network error, non-JSON response), the
.then(r => r.json())call will throw, and the interval keeps ticking with unhandled rejections each second. Add a.catchto gracefully handle failures.Suggested fix
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("Failed to poll players:", err); + });
🤖 Fix all issues with AI agents
In `@src/client/components/GameConfigForm.ts`:
- Around line 274-290: Update hasOptionsChanged() to include comparisons for
teamCount and useRandomMap against DEFAULT_CONFIG, and also check the
user-configurable numeric/value fields goldMultiplierValue, startingGoldValue,
and maxTimerValue; modify the boolean expression in the hasOptionsChanged method
(the one referencing DEFAULT_CONFIG and properties like disableNations, bots,
etc.) to OR in comparisons like this.teamCount !== DEFAULT_CONFIG.teamCount,
this.useRandomMap !== DEFAULT_CONFIG.useRandomMap, this.goldMultiplierValue !==
DEFAULT_CONFIG.goldMultiplier, this.startingGoldValue !==
DEFAULT_CONFIG.startingGold, and this.maxTimerValue !== DEFAULT_CONFIG.maxTimer
so any change to those settings correctly returns true.
🧹 Nitpick comments (8)
src/client/components/map/MapDisplay.ts (1)
89-89: Label is always rendered as "active" — intentional?
renderCardLabel(..., true)means the label text is always full white (text-white), even when the card is not selected. The selected/unselected distinction comes fromcardStateClasseson the outer button, so this is visually consistent. Just flagging in case you intended the label to dim for unselected cards — if so, passthis.selectedinstead oftrue.src/client/components/map/MapPicker.ts (2)
105-117: Tabs look clean, but consider completing the ARIA tab pattern.The
role="tab"+role="tablist"+aria-selectedis a great start. For full WAI-ARIA compliance, the content panels (featured / all maps) would needrole="tabpanel"with matchingid/aria-labelledby/aria-controlsattributes. Not a blocker, but worth a follow-up if accessibility is a priority.
144-161: Random map card layout differs slightly from<map-display>cards.The random map
<button>usesitems-stretchwith a separate<div class="p-3 border-t ...">wrapper around the label, while<map-display>usesp-3 gap-3 items-center justify-betweenon the outer button with no label wrapper. This could produce visual differences (padding, alignment, separator line) between the random card and the regular map cards.If the visual result looks good after testing, this is fine. Just flagging in case the cards should look identical.
src/client/components/GameConfigForm.ts (4)
30-54:as constdoes not deeply narrow here due to type assertions.The
as conston the object is overridden by the inline type assertions (undefined as number | undefined,[] as UnitType[],2 as TeamCountConfig). These assertions widen the types back, soas constis effectively a no-op on those fields. This is not a bug (the values are still correct defaults), but it is misleading — a reader might think this object is deeply readonly.Consider either removing
as const(since it is not doing what it suggests) or removing the inline type assertions and lettingas constnarrow the types naturally. You can usesatisfiesto validate shape without widening:Option: drop `as const`, add an explicit type
-const DEFAULT_CONFIG = { +const DEFAULT_CONFIG: GameConfigSnapshot = { selectedMap: GameMapType.World, selectedDifficulty: Difficulty.Easy, disableNations: false, bots: 400, ... - disabledUnits: [] as UnitType[], + disabledUnits: [], ... -} as const; +};
239-245:resolveSelectedMap()mutates internal state as a side effect — surprising for a "resolve" method.A method named
resolve...reads like a pure query, but it silently writesthis.selectedMapwhenuseRandomMapis true. Callers inSinglePlayerModalandHostLobbyModalmay not expect this side effect.Consider splitting into a pure getter and a separate "commit" step, or at minimum documenting the mutation clearly in the method name (e.g.,
commitAndGetSelectedMap()).
446-461: Bot slider wrapper<div>is not a focusable/interactive element but has visual states.The outer
<div>usescardStateClasseswhich applies hover styles, but the actual interaction is inside<fluent-slider>. This is fine visually, but for keyboard and screen reader users, the container div provides no semantic meaning. Not a blocker — just something to keep in mind for accessibility.
247-272:reset()manually resets every property — easy to forget a field.If you add a new config property later, you must update
reset(),getConfig(),hasOptionsChanged(), andDEFAULT_CONFIGin sync. Consider deriving reset fromDEFAULT_CONFIGto reduce that surface:Example
public reset(): void { Object.assign(this, { ...DEFAULT_CONFIG, disabledUnits: [...DEFAULT_CONFIG.disabledUnits] }); }This reduces the chance of forgetting a field in a future change.
src/client/SinglePlayerModal.ts (1)
285-287: RedundantUnitTypevalidation —disabledUnitsis alreadyUnitType[].The
.map(u => Object.values(UnitType).find(ut => ut === u)).filter(...)pipeline just validates that each item is a member of theUnitTypeenum, but the source (config.disabledUnits) is already typed asUnitType[]and populated from the form'stoggleUnithandler which only adds validUnitTypevalues. This round-trip is a no-op.You can simplify to just
disabledUnits: [...config.disabledUnits](or pass directly). Not urgent — it's defensive code, but it adds noise.
| /** Check if any options differ from defaults (used by SP for achievement warning). */ | ||
| public hasOptionsChanged(): boolean { | ||
| return ( | ||
| this.disableNations !== DEFAULT_CONFIG.disableNations || | ||
| this.bots !== DEFAULT_CONFIG.bots || | ||
| this.infiniteGold !== DEFAULT_CONFIG.infiniteGold || | ||
| this.infiniteTroops !== DEFAULT_CONFIG.infiniteTroops || | ||
| this.compactMap !== DEFAULT_CONFIG.compactMap || | ||
| this.maxTimer !== DEFAULT_CONFIG.maxTimer || | ||
| this.instantBuild !== DEFAULT_CONFIG.instantBuild || | ||
| this.randomSpawn !== DEFAULT_CONFIG.randomSpawn || | ||
| this.gameMode !== DEFAULT_CONFIG.gameMode || | ||
| this.goldMultiplier !== DEFAULT_CONFIG.goldMultiplier || | ||
| this.startingGold !== DEFAULT_CONFIG.startingGold || | ||
| this.disabledUnits.length > 0 | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate GameConfigForm.ts and examine the full file
fd -e ts -n 'GameConfigForm' | head -20Repository: openfrontio/OpenFrontIO
Length of output: 297
🏁 Script executed:
# Search for the hasOptionsChanged method usage to understand impact
rg -n 'hasOptionsChanged' --type=ts -C 3Repository: openfrontio/OpenFrontIO
Length of output: 1117
🏁 Script executed:
# Find the DEFAULT_CONFIG definition
rg -n 'DEFAULT_CONFIG' --type=ts -B 2 -A 10 | head -100Repository: openfrontio/OpenFrontIO
Length of output: 8867
🏁 Script executed:
# Get the complete DEFAULT_CONFIG definition
sed -n '30,50p' src/client/components/GameConfigForm.tsRepository: openfrontio/OpenFrontIO
Length of output: 683
🏁 Script executed:
# Get the complete hasOptionsChanged method
sed -n '274,295p' src/client/components/GameConfigForm.tsRepository: openfrontio/OpenFrontIO
Length of output: 970
🏁 Script executed:
# Check SinglePlayerModal to understand how optionsChanged affects achievements
rg -n 'optionsChanged' src/client/SinglePlayerModal.ts -B 5 -A 5Repository: openfrontio/OpenFrontIO
Length of output: 1675
Add teamCount and useRandomMap checks to hasOptionsChanged().
The method currently skips these properties, both of which have defaults in DEFAULT_CONFIG. If a player changes team count from 2 or enables random map, hasOptionsChanged() returns false, bypassing the achievement warning. Additionally, goldMultiplierValue, startingGoldValue, and maxTimerValue are also unchecked despite being user-configurable.
🤖 Prompt for AI Agents
In `@src/client/components/GameConfigForm.ts` around lines 274 - 290, Update
hasOptionsChanged() to include comparisons for teamCount and useRandomMap
against DEFAULT_CONFIG, and also check the user-configurable numeric/value
fields goldMultiplierValue, startingGoldValue, and maxTimerValue; modify the
boolean expression in the hasOptionsChanged method (the one referencing
DEFAULT_CONFIG and properties like disableNations, bots, etc.) to OR in
comparisons like this.teamCount !== DEFAULT_CONFIG.teamCount, this.useRandomMap
!== DEFAULT_CONFIG.useRandomMap, this.goldMultiplierValue !==
DEFAULT_CONFIG.goldMultiplier, this.startingGoldValue !==
DEFAULT_CONFIG.startingGold, and this.maxTimerValue !== DEFAULT_CONFIG.maxTimer
so any change to those settings correctly returns true.
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