Main Menu UI Overhaul [DRAFT] #2818
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/client/HostLobbyModal.ts:
- Around line 289-293: The "Special" header in HostLobbyModal.ts is hardcoded
and breaks i18n; replace the literal string with a call to
translateText("map_categories.special") (matching how other headers use
translateText(`map_categories.${categoryKey}`)) and add the
"map_categories.special" entry to en.json with the appropriate English value so
translations work consistently.
- Around line 692-705: The placeholder "Mins" on the numeric input should be
localized like the other numeric input; replace the hardcoded placeholder string
with the same i18n call used elsewhere (e.g., use the component's translation
method such as this.t(...) or i18n.t(...) consistent with other inputs) on the
input that renders spawnImmunityDurationMinutes and ensure the matching
translation key is present in your locale files; update both numeric inputs to
use the same translation approach and keep the event handlers
handleSpawnImmunityDurationInput and handleSpawnImmunityDurationKeyDown
unchanged.
- Around line 621-631: The input's hardcoded placeholder "Mins" in
HostLobbyModal should be replaced with a translation call; change the
placeholder to use translateText("common.mins") (or the project's equivalent)
instead of the literal string, ensuring the translateText function is
imported/available in the component and referenced as
.placeholder=${translateText("common.mins")} (or placeholder=${...} per your
templating conventions) so the placeholder is localized; verify HostLobbyModal
continues to compile and update any tests or snapshots that assert the
placeholder.
🧹 Nitpick comments (1)
src/client/HostLobbyModal.ts (1)
1090-1107: Remove commented-out code if not needed.The
// this.close();on line 1095 should be removed if it is no longer required. If there is a reason to keep it for reference, add a comment explaining why. Dead code can confuse future readers.Suggested fix
private async startGame() { await this.putGameConfig(); console.log( `Starting private game with map: ${GameMapType[this.selectedMap as keyof typeof GameMapType]} ${this.useRandomMap ? " (Randomly selected)" : ""}`, ); - // this.close(); const config = await getServerConfigFromClient();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/HostLobbyModal.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.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2026-01-02T18:11:06.832Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:06.832Z
Learning: In src/client/HostLobbyModal.ts, the ?s=xxxxx URL suffix used in lobby URLs is purely for cache-busting platform previews (e.g., Discord, WhatsApp, x.com) and is not used by the join logic. The join flow ignores the suffix value, so regenerating it via updateUrlWithSuffix() on configuration changes will not break existing shared URLs; it only prompts platforms to refresh preview metadata. Treat the suffix as non-functional for join behavior and ensure any related changes preserve that invariant.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-07-12T06:35:10.457Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1357
File: resources/lang/ja.json:0-0
Timestamp: 2025-07-12T06:35:10.457Z
Learning: In OpenFrontIO project, "giantworldmap" is the correct localization key name for the giant world map, used consistently across all language files and TypeScript code. Do not suggest renaming this key.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-12-26T22:21:21.904Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2689
File: src/client/PublicLobby.ts:245-245
Timestamp: 2025-12-26T22:21:21.904Z
Learning: In public lobbies with HumansVsNations mode in src/client/PublicLobby.ts, maxPlayers represents only human player slots (already halved in DefaultConfig.ts). The nation NPCs are added automatically server-side and don't count toward maxPlayers. Therefore, getTeamSize correctly returns maxPlayers directly for HumansVsNations to display the proper team size (e.g., maxPlayers=5 yields "5 Humans vs 5 Nations").
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-10-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.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (4)
src/client/HostLobbyModal.ts (4)
866-897: Good cleanup pattern in close().The separation of
leaveLobby()for event dispatch and proper cleanup ofplayersIntervalandbotsUpdateTimeris clean. No issues here.
1018-1025: Auto-enabling donateGold and donateTroops for Team mode.When switching to Team mode, these options are forced on. This makes sense for team games, but the UI does not reset them when switching back to FFA. If the user switches modes, they might end up with unexpected settings. Consider whether this is the intended behavior.
941-991: Good use of arrow functions for callbacks.Converting handlers to arrow functions ensures proper
thisbinding when passed torenderOptionToggle. Clean and consistent pattern.
1109-1115: Clean integration with shared copyToClipboard utility.Using the centralized utility from
Utils.tsreduces duplication and keeps clipboard behavior consistent across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/client/ClientGameRunner.ts:
- Around line 124-135: Replace the catch handler so it hides the starting modal
via the consistent classList pattern instead of calling hide(), and show errors
using the existing showErrorModal(...) helper with a safe string extracted from
the thrown value; specifically, in the promise chain that calls .then(r =>
r.start()).catch(...), locate the startingModal variable and replace
startingModal?.hide() with startingModal?.classList?.add("hidden"), compute a
safe message like (e instanceof Error ? e.message : String(e)) and pass that to
showErrorModal(...) instead of alert(), and keep the console.error(e) for
logging.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/ClientGameRunner.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/ClientGameRunner.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/ClientGameRunner.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
| ) | ||
| .then((r) => r.start()) | ||
| .catch((e) => { | ||
| console.error("error creating client game", e); | ||
| const startingModal = document.querySelector( | ||
| "game-starting-modal", | ||
| ) as HTMLElement & { hide?: () => void }; | ||
| if (startingModal?.hide) { | ||
| startingModal.hide(); | ||
| } | ||
| alert("Error starting game: " + e.message); | ||
| }); |
There was a problem hiding this comment.
Use consistent error handling and modal hiding patterns.
Three concerns with this error handling:
-
Inconsistent modal hiding: The code calls
hide()on the modal, but per the codebase pattern, UI elements should be hidden usingclassList.add("hidden"). Thehide()method may not exist on all modal implementations. -
Alert vs existing error modal: Using
alert()provides poor UX (blocks the thread). The existingshowErrorModal()function would provide a better, consistent experience. -
Type safety: Accessing
e.messageassumeseis anErrortype. If a non-Error is thrown, this could beundefined.
Based on learnings, UI hiding should use classList pattern for consistency.
🔧 Proposed fix using codebase patterns
- )
- .then((r) => r.start())
- .catch((e) => {
- console.error("error creating client game", e);
- const startingModal = document.querySelector(
- "game-starting-modal",
- ) as HTMLElement & { hide?: () => void };
- if (startingModal?.hide) {
- startingModal.hide();
- }
- alert("Error starting game: " + e.message);
- });
+ )
+ .then((r) => r.start())
+ .catch((e: unknown) => {
+ console.error("error creating client game", e);
+ const startingModal = document.querySelector("game-starting-modal");
+ if (startingModal) {
+ startingModal.classList.add("hidden");
+ }
+ const errorMessage = e instanceof Error ? e.message : String(e);
+ showErrorModal(
+ "game_start_failed",
+ errorMessage,
+ lobbyConfig.gameID,
+ lobbyConfig.clientID,
+ true,
+ false,
+ "error_modal.game_start_error",
+ );
+ });Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/client/ClientGameRunner.ts around lines 124 - 135, Replace the catch
handler so it hides the starting modal via the consistent classList pattern
instead of calling hide(), and show errors using the existing
showErrorModal(...) helper with a safe string extracted from the thrown value;
specifically, in the promise chain that calls .then(r => r.start()).catch(...),
locate the startingModal variable and replace startingModal?.hide() with
startingModal?.classList?.add("hidden"), compute a safe message like (e
instanceof Error ? e.message : String(e)) and pass that to showErrorModal(...)
instead of alert(), and keep the console.error(e) for logging.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/JoinPrivateLobbyModal.ts (1)
384-395: Critical bug: Leave-lobby event sent with empty lobby ID.The
closeAndLeave()method callsclose()first (line 385), which clearscurrentLobbyIdat line 368. Then it attempts to usethis.currentLobbyIdin the leave-lobby event detail at line 390, which will always be an empty string.🐛 Proposed fix
public closeAndLeave() { + const lobbyId = this.currentLobbyId; this.close(); this.hasJoined = false; this.message = ""; this.dispatchEvent( new CustomEvent("leave-lobby", { - detail: { lobby: this.currentLobbyId }, + detail: { lobby: lobbyId }, bubbles: true, composed: true, }), ); }
🤖 Fix all issues with AI agents
In @src/client/JoinPrivateLobbyModal.ts:
- Around line 329-334: The "Disabled Units" label in the JoinPrivateLobbyModal
component is hardcoded; replace it with a translation lookup (e.g., use the i18n
hook/translator function already used in this component) and ensure the
translation key (suggested: "joinPrivateLobby.disabledUnits") is used instead of
the literal string; add the new key to the locale files and import/use the
existing translator (t) in JoinPrivateLobbyModal so the div with class "text-xs
font-bold text-red-400..." renders t('joinPrivateLobby.disabledUnits') rather
than the hardcoded text.
In @src/client/LanguageModal.ts:
- Around line 12-14: handleClose currently only sets this.style.pointerEvents =
"none" which prevents the modal's base closing logic from running when wired to
o-modal's onClose; update handleClose to call this.close() after (or before)
adjusting pointer-events so BaseModal cleanup runs, or move the pointer-events
change into the inherited onClose lifecycle method on BaseModal and let
handleClose simply call this.close(); ensure you reference the handleClose
method, the close() method, the onClose lifecycle from BaseModal, and
this.style.pointerEvents when making the change.
🧹 Nitpick comments (15)
src/types/global.d.ts (1)
1-5: Use a string literal union type forpageIdinstead of loosestring.The
pageIdparameter accepts any string, allowing typos and invalid page IDs to slip through at compile time. Define a typed union of valid page IDs to catch errors early and provide autocomplete. Current usage shows these pages:'page-play','page-single-player','page-item-store','page-host-lobby','page-join-private-lobby','page-language','page-options'.♻️ Proposed refactor with typed union
+type PageId = 'page-play' | 'page-single-player' | 'page-item-store' | 'page-host-lobby' | 'page-join-private-lobby' | 'page-language' | 'page-options'; + declare global { interface Window { - showPage?: (pageId: string) => void; + showPage?: (pageId: PageId) => void; } }src/client/styles.css (2)
55-61: Scope scrollbar styles to scrollable elements instead of*selector.Applying scrollbar styles to all elements via the universal selector (
*) has performance implications, as browsers must apply these properties to every element in the DOM regardless of whether they overflow.Consider scoping these styles to scrollable containers or creating a utility class:
/* Option 1: Target likely scrollable elements */ body, [data-scrollable], .scrollable, div[style*="overflow"] { scrollbar-width: auto; scrollbar-color: rgba(255, 255, 255, 0.2) transparent; } /* Option 2: Create a utility class */ .custom-scrollbar { scrollbar-width: auto; scrollbar-color: rgba(255, 255, 255, 0.2) transparent; }
41-53: Duplicate property is intentional; consider focus management enhancements.The duplicate
heightproperty (lines 43–44) is a valid progressive enhancement pattern (fallback first, modern value second). Biome flags it correctly—this is expected behavior.The
!importantonoverflow: hiddenis acceptable here since the JavaScript scroll lock (inOModal.close()) is the primary mechanism; the CSS is supplementary.Modal accessibility is well-implemented: focus is properly set when opening (SendResourceModal), and ARIA attributes (
role="dialog",aria-modal="true",aria-labelledby) are present. However, consider these optional improvements:
- Focus trap: Currently nothing prevents keyboard users from tabbing outside the modal. Consider a focus-trap implementation for modals that overlay content.
- Focus restoration: After closing a modal, focus should return to the element that opened it. Currently, focus location is not managed on close.
These are optional refinements for enhanced keyboard navigation accessibility, not blocking issues.
src/client/components/baseComponents/setting/SettingKeybind.ts (3)
21-21: Simplify the currentValue default logic.The ternary
this.value === "" ? "" : this.value || this.defaultKeyis redundant—ifthis.value === "", the result is"", but ifthis.valueis falsy, you fallback tothis.defaultKey. Consider simplifying to:const currentValue = this.value || this.defaultKey || "";This achieves the same result with clearer intent.
71-74: Hardcoded "Press a key" string should be translatable.For consistency with the rest of the UI and to support internationalization, replace the hardcoded string with a translation key.
🌐 Proposed fix
private displayKey(key: string): string { - if (!key) return "Press a key"; + if (!key) return translateText("user_setting.press_a_key"); return formatKeyForDisplay(key); }Don't forget to add the key to
resources/lang/en.json:"user_setting": { ... "press_a_key": "Press a key", ... }
119-133: Consider usingnullor empty string instead of "Null" string.Using the literal string
"Null"to represent an unbound key is unconventional and could lead to confusion. Consider using:
null(requires allowing null in the type)""(empty string, which already means "no key")If
"Null"is intentional (e.g., to match a specific backend contract), add a comment explaining why.src/client/StatsModal.ts (1)
185-201: Simplify nested ternaries with a helper function or lookup.The nested ternaries for
rankColorandrankIconreduce readability. Consider extracting them into small helper functions:♻️ Proposed refactor
+ private getRankColor(index: number): string { + if (index === 0) return "text-yellow-400 bg-yellow-400/10 ring-1 ring-yellow-400/20"; + if (index === 1) return "text-slate-300 bg-slate-400/10 ring-1 ring-slate-400/20"; + if (index === 2) return "text-amber-600 bg-amber-600/10 ring-1 ring-amber-600/20"; + return "text-white/40 bg-white/5"; + } + + private getRankIcon(index: number): string { + if (index === 0) return "👑"; + if (index === 1) return "🥈"; + if (index === 2) return "🥉"; + return "#" + (index + 1); + } ${clans.map((clan, index) => { - const rankColor = index === 0 ? "..." : index === 1 ? "..." : index === 2 ? "..." : "..."; - const rankIcon = index === 0 ? "👑" : index === 1 ? "🥈" : index === 2 ? "🥉" : "#" + (index + 1); + const rankColor = this.getRankColor(index); + const rankIcon = this.getRankIcon(index);src/client/components/BaseModal.ts (1)
105-109: Consider extracting "page-play" to a constant.The hardcoded
"page-play"string on line 108 could become inconsistent if the page ID changes elsewhere. Consider extracting it to a constant or configuration:private static readonly DEFAULT_PAGE = "page-play"; // Then use: window.showPage(BaseModal.DEFAULT_PAGE);src/client/PublicLobby.ts (1)
352-368: Consider adding a typed interface for username-input.The
as anycast and runtime type check forisValidsuggest the username-input component lacks a proper TypeScript interface. Consider creating an interface:interface UsernameInputElement extends HTMLElement { isValid(): boolean; validationError: string; }Then use:
const usernameInput = document.querySelector("username-input") as UsernameInputElement | null; if (usernameInput && !usernameInput.isValid()) { // ... }This provides compile-time type safety and better IDE support.
src/client/FlagInputModal.ts (1)
18-31: Potential code duplication with onClose hook.The
firstUpdatedmethod setsmodalEl.onClosedirectly (lines 20-29), but theonClosehook method (lines 166-174) contains identical logic. SinceBaseModalshould automatically call theonClosehook, thefirstUpdatedsetup might be redundant.♻️ Consider simplifying by removing firstUpdated
The
onClosehook should be sufficient if BaseModal is calling it properly. If thefirstUpdatedsetup is necessary for some reason, add a comment explaining why.- firstUpdated() { - if (this.modalEl) { - this.modalEl.onClose = () => { - this.unregisterEscapeHandler(); - if (this.returnTo) { - const returnEl = document.querySelector(this.returnTo) as any; - if (returnEl?.open) { - returnEl.open(); - } - this.returnTo = ""; - } - }; - } - }Or if needed for some BaseModal quirk, document it:
firstUpdated() { + // BaseModal requires direct assignment of onClose in addition to hook if (this.modalEl) { this.modalEl.onClose = () => {src/client/HelpModal.ts (1)
1136-1147: Minor redundancy in modal configuration.The
title="Instructions"prop is unnecessary sincehideHeaderis set totrue, which prevents the title from being displayed. ThetranslationKeyis also redundant in this context.♻️ Suggested simplification
return html` <o-modal id="helpModal" - title="Instructions" - translationKey="main.instructions" ?inline=${this.inline} ?hideHeader=${true} ?hideCloseButton=${true} >src/client/HostLobbyModal.ts (3)
72-96: Consider extracting this helper to a shared utility.The
renderOptionTogglehelper follows a similar pattern torenderUnitTypeOptionsinsrc/client/utilities/RenderUnitTypeOptions.ts. Since this pattern is reused across multiple option rendering contexts, consider extracting it to a shared utility module to avoid duplication.
98-836: Consider breaking down the render method.The render method spans 738 lines and handles all UI rendering inline. While functional, breaking it into smaller rendering methods (e.g.,
renderHeader(),renderMapSelection(),renderGameMode(),renderOptions(),renderPlayerList(),renderFooter()) would improve readability and maintainability.
1033-1040: Consider making donation toggles explicit when switching to Team mode.Lines 1036-1037 automatically enable
donateGoldanddonateTroopswhen switching to Team mode. While donations are common in team games, this automatic behavior might surprise users who want team mode without donations. Consider making this more explicit, such as showing a tooltip or keeping these as separate toggles that users can adjust.src/client/TerritoryPatternsModal.ts (1)
157-162: Confusing logic for showOnlyOwned toggle.The
showOnlyOwnedflag has inverted behavior depending on context:
- When
true: shows only owned items (lines 157-158)- When
false: shows only store items by hiding owned items (lines 160-161)This creates a confusing mental model. Consider renaming to something like
filterModewith enum values"owned"and"store", or using separate flags likeshowOwnedandshowStoreto make the intent clearer.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
index.htmlresources/lang/en.jsonsrc/client/AccountModal.tssrc/client/FlagInputModal.tssrc/client/HelpModal.tssrc/client/HostLobbyModal.tssrc/client/JoinPrivateLobbyModal.tssrc/client/KeybindsModal.tssrc/client/LanguageModal.tssrc/client/NewsModal.tssrc/client/PublicLobby.tssrc/client/SinglePlayerModal.tssrc/client/StatsModal.tssrc/client/TerritoryPatternsModal.tssrc/client/UserSettingModal.tssrc/client/Utils.tssrc/client/components/BaseModal.tssrc/client/components/Maps.tssrc/client/components/baseComponents/setting/SettingKeybind.tssrc/client/styles.csssrc/types/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/Utils.ts
🧰 Additional context used
🧠 Learnings (22)
📚 Learning: 2025-07-12T06:35:10.457Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1357
File: resources/lang/ja.json:0-0
Timestamp: 2025-07-12T06:35:10.457Z
Learning: In OpenFrontIO project, "giantworldmap" is the correct localization key name for the giant world map, used consistently across all language files and TypeScript code. Do not suggest renaming this key.
Applied to files:
src/client/components/Maps.tssrc/client/SinglePlayerModal.tssrc/client/HostLobbyModal.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/AccountModal.tssrc/client/UserSettingModal.tssrc/client/TerritoryPatternsModal.tssrc/client/LanguageModal.tssrc/client/components/BaseModal.tssrc/client/HelpModal.tsindex.htmlsrc/client/SinglePlayerModal.tssrc/client/JoinPrivateLobbyModal.tssrc/client/PublicLobby.tssrc/client/FlagInputModal.tssrc/client/HostLobbyModal.tssrc/client/StatsModal.tssrc/client/KeybindsModal.ts
📚 Learning: 2025-06-20T20:11:00.965Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with `new UserSettings()` in each component that needs them. This pattern is used consistently across at least 12+ files including OptionsMenu, EventsDisplay, DarkModeButton, Main, UserSettingModal, UsernameInput, NameLayer, AlertFrame, UILayer, InputHandler, ClientGameRunner, and GameView. This is the established architectural pattern and should be followed for consistency.
Applied to files:
src/client/UserSettingModal.ts
📚 Learning: 2025-06-22T05:48:19.241Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.
Applied to files:
src/client/TerritoryPatternsModal.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/client/TerritoryPatternsModal.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/client/TerritoryPatternsModal.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/client/TerritoryPatternsModal.ts
📚 Learning: 2026-01-08T13:52:00.939Z
Learnt from: deshack
Repo: openfrontio/OpenFrontIO PR: 2777
File: src/client/Main.ts:374-378
Timestamp: 2026-01-08T13:52:00.939Z
Learning: In src/client/Main.ts, when the browser back button is pressed, the `popstate` event fires before the `hashchange` event. The `preventHashUpdate` flag is used to prevent the `hashchange` listener (`onHashUpdate`) from executing after a navigation rollback in the `popstate` listener (`onPopState`), specifically when the user cancels leaving an active game.
Applied to files:
src/client/TerritoryPatternsModal.ts
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-08-16T10:52:08.292Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-07-12T08:41:35.101Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1357
File: resources/lang/de.json:523-540
Timestamp: 2025-07-12T08:41:35.101Z
Learning: In OpenFrontIO project localization files, always check the en.json source file before flagging potential spelling errors in other language files, as some keys may intentionally use non-standard spellings that need to be consistent across all translations.
Applied to files:
resources/lang/en.jsonsrc/client/HostLobbyModal.ts
📚 Learning: 2025-08-27T08:12:19.610Z
Learnt from: mokizzz
Repo: openfrontio/OpenFrontIO PR: 1940
File: resources/lang/en.json:763-766
Timestamp: 2025-08-27T08:12:19.610Z
Learning: In OpenFrontIO, some country entries in src/client/data/countries.json may have similar names but different codes (e.g., "Empire of Japan" vs "Empire of Japan1"). Each unique code requires its own translation key in resources/lang/en.json after normalization. Always verify against countries.json before suggesting removal of translation keys.
Applied to files:
resources/lang/en.jsonsrc/client/HostLobbyModal.ts
📚 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-19T18:12:24.553Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1417
File: src/client/index.html:335-350
Timestamp: 2025-07-19T18:12:24.553Z
Learning: For the OpenFrontIO project: rel="noopener" is not required for internal links (like /privacy-policy.html and /terms-of-service.html) since the project controls the destination pages. The security concern only applies to external, untrusted links.
Applied to files:
index.html
📚 Learning: 2025-12-29T23:33:17.920Z
Learnt from: wraith4081
Repo: openfrontio/OpenFrontIO PR: 2735
File: index.html:390-391
Timestamp: 2025-12-29T23:33:17.920Z
Learning: In Tailwind CSS v4, update blur-related utilities in HTML templates. The mappings are: backdrop-blur-sm (v3) -> backdrop-blur-xs (v4); backdrop-blur (bare) -> backdrop-blur-sm; blur-sm -> blur-xs. Apply these changes to all HTML files (e.g., index.html and other templates) to reflect the v4 naming. Consider updating a project-wide search/replace or using a codemod to ensure consistency across the codebase.
Applied to files:
index.html
📚 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/JoinPrivateLobbyModal.tssrc/client/PublicLobby.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/JoinPrivateLobbyModal.tssrc/client/PublicLobby.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.tssrc/client/HostLobbyModal.ts
📚 Learning: 2026-01-02T18:11:15.132Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:15.132Z
Learning: In src/client/HostLobbyModal.ts, the `?s=xxxxx` URL suffix in lobby URLs is purely for cache-busting embed previews on platforms like Discord, WhatsApp, and x.com. The suffix value is ignored by the join logic (any value works), so regenerating it on config changes via `updateUrlWithSuffix()` doesn't break existing shared URLs - it only forces platforms to re-fetch updated preview metadata.
Applied to files:
src/client/JoinPrivateLobbyModal.tssrc/client/PublicLobby.ts
📚 Learning: 2025-12-26T22:21:21.904Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2689
File: src/client/PublicLobby.ts:245-245
Timestamp: 2025-12-26T22:21:21.904Z
Learning: In public lobbies with HumansVsNations mode in src/client/PublicLobby.ts, maxPlayers represents only human player slots (already halved in DefaultConfig.ts). The nation NPCs are added automatically server-side and don't count toward maxPlayers. Therefore, getTeamSize correctly returns maxPlayers directly for HumansVsNations to display the proper team size (e.g., maxPlayers=5 yields "5 Humans vs 5 Nations").
Applied to files:
src/client/JoinPrivateLobbyModal.tssrc/client/HostLobbyModal.ts
📚 Learning: 2026-01-02T18:11:06.832Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2740
File: src/client/HostLobbyModal.ts:821-821
Timestamp: 2026-01-02T18:11:06.832Z
Learning: In src/client/HostLobbyModal.ts, the ?s=xxxxx URL suffix used in lobby URLs is purely for cache-busting platform previews (e.g., Discord, WhatsApp, x.com) and is not used by the join logic. The join flow ignores the suffix value, so regenerating it via updateUrlWithSuffix() on configuration changes will not break existing shared URLs; it only prompts platforms to refresh preview metadata. Treat the suffix as non-functional for join behavior and ensure any related changes preserve that invariant.
Applied to files:
src/client/HostLobbyModal.ts
📚 Learning: 2025-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.ts
🧬 Code graph analysis (13)
src/client/components/Maps.ts (2)
src/client/Utils.ts (1)
translateText(147-202)src/client/LangSelector.ts (1)
translateText(248-268)
src/client/AccountModal.ts (3)
src/client/Utils.ts (2)
copyToClipboard(19-36)translateText(147-202)src/client/LangSelector.ts (1)
translateText(248-268)src/client/Auth.ts (1)
isLoggedIn(67-70)
src/client/UserSettingModal.ts (1)
src/client/FlagInputModal.ts (1)
customElement(7-175)
src/client/TerritoryPatternsModal.ts (4)
src/client/Utils.ts (1)
translateText(147-202)src/client/Api.ts (1)
hasLinkedAccount(137-145)src/core/CosmeticSchemas.ts (1)
Pattern(7-7)src/client/components/PatternButton.ts (2)
renderPatternPreview(161-174)render(70-158)
src/client/LanguageModal.ts (2)
src/client/TerritoryPatternsModal.ts (1)
customElement(20-433)src/client/components/baseComponents/Modal.ts (1)
customElement(5-108)
src/client/SinglePlayerModal.ts (4)
src/client/Utils.ts (1)
translateText(147-202)src/client/LangSelector.ts (1)
translateText(248-268)src/core/game/Game.ts (5)
mapCategories(125-176)Quads(55-55)Trios(54-54)Duos(53-53)HumansVsNations(56-56)src/client/utilities/RenderUnitTypeOptions.ts (1)
renderUnitTypeOptions(23-46)
src/client/JoinPrivateLobbyModal.ts (2)
src/core/Schemas.ts (2)
ClientInfo(141-144)GameConfig(90-90)src/client/Utils.ts (2)
translateText(147-202)copyToClipboard(19-36)
src/client/PublicLobby.ts (2)
src/client/Utils.ts (1)
translateText(147-202)src/client/LangSelector.ts (1)
translateText(248-268)
src/client/FlagInputModal.ts (2)
src/client/FlagInput.ts (1)
customElement(9-100)src/client/components/baseComponents/Modal.ts (1)
customElement(5-108)
src/client/HostLobbyModal.ts (8)
src/client/Utils.ts (2)
translateText(147-202)copyToClipboard(19-36)src/client/LangSelector.ts (1)
translateText(248-268)src/core/game/Game.ts (1)
mapCategories(125-176)src/client/utilities/RenderUnitTypeOptions.ts (1)
renderUnitTypeOptions(23-46)src/core/game/PlayerImpl.ts (1)
clientID(197-199)src/core/game/GameView.ts (1)
clientID(458-460)src/core/game/TerraNulliusImpl.ts (1)
clientID(9-11)src/client/JoinPrivateLobbyModal.ts (1)
copyToClipboard(397-403)
src/client/NewsModal.ts (3)
src/client/components/baseComponents/Modal.ts (1)
customElement(5-108)src/client/Utils.ts (1)
translateText(147-202)src/client/LangSelector.ts (1)
translateText(248-268)
src/client/KeybindsModal.ts (4)
src/client/components/baseComponents/setting/SettingKeybind.ts (2)
customElement(5-134)displayKey(71-74)src/client/graphics/layers/HeadsUpMessage.ts (1)
customElement(9-142)src/client/Utils.ts (2)
formatKeyForDisplay(79-98)translateText(147-202)src/client/LangSelector.ts (1)
translateText(248-268)
src/client/components/baseComponents/setting/SettingKeybind.ts (2)
src/client/Utils.ts (2)
translateText(147-202)formatKeyForDisplay(79-98)src/client/LangSelector.ts (1)
translateText(248-268)
🪛 Biome (2.1.2)
src/client/styles.css
[error] 44-44: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
height is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
🔇 Additional comments (44)
src/types/global.d.ts (1)
7-7: LGTM!The
export {}statement correctly ensures this file is treated as a module, which is required for ambient declarations.src/client/styles.css (1)
64-76: LGTM!The webkit scrollbar customization looks clean. The 12px width, transparent track, and semi-transparent thumb provide good visual consistency with the Firefox scrollbar styling defined earlier.
src/client/components/baseComponents/setting/SettingKeybind.ts (2)
81-102: LGTM—good validation pattern with prevValue.The approach of temporarily setting the value for parent validation, then allowing the parent to restore it if rejected, is clean and flexible. Including
prevValuein the event detail enables proper rollback logic.
104-117: LGTM—reset logic is clear.Setting both
valueandkeytodefaultKeyon reset is consistent with the component's intended behavior.src/client/StatsModal.ts (2)
11-23: LGTM—clean BaseModal migration with lazy loading.The
onOpenlifecycle hook ensures leaderboard data is fetched only when needed, avoiding unnecessary network calls. ThehasLoadedflag prevents duplicate requests.
275-361: LGTM—flexible inline/modal rendering.The render method cleanly supports both inline page mode and modal dialog mode, with appropriate back button and header logic for each context.
resources/lang/en.json (2)
1-906: LGTM—translation additions are clear and consistent.All new translation keys follow proper naming conventions, have clear English values, and support the expanded UI features. JSON structure is valid.
Based on learnings: Correctly updating only
en.jsonin this PR; other language files will be updated by dedicated translation PRs.
368-368: Remove unused translation key with space in name.The key
"teams_Humans Vs Nations"(line 368) is not used anywhere in the codebase and violates naming conventions. All other team type keys use snake_case (teams_Duos,teams_Trios,teams_Quads,teams_hvn). The same team type already has a properly named key at line 313:"teams_hvn": "Humans vs Nations". Remove the unused key on line 368 or rename it toteams_hvnfor consistency.⛔ Skipped due to learnings
Learnt from: mokizzz Repo: openfrontio/OpenFrontIO PR: 1940 File: resources/lang/en.json:763-766 Timestamp: 2025-08-27T08:12:19.610Z Learning: In OpenFrontIO, some country entries in src/client/data/countries.json may have similar names but different codes (e.g., "Empire of Japan" vs "Empire of Japan1"). Each unique code requires its own translation key in resources/lang/en.json after normalization. Always verify against countries.json before suggesting removal of translation keys.Learnt from: Saphereye Repo: openfrontio/OpenFrontIO PR: 2233 File: src/client/HostLobbyModal.ts:891-891 Timestamp: 2025-10-21T20:06:04.823Z Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.Learnt from: FloPinguin Repo: openfrontio/OpenFrontIO PR: 2689 File: src/client/PublicLobby.ts:245-245 Timestamp: 2025-12-26T22:21:21.904Z Learning: In public lobbies with HumansVsNations mode in src/client/PublicLobby.ts, maxPlayers represents only human player slots (already halved in DefaultConfig.ts). The nation NPCs are added automatically server-side and don't count toward maxPlayers. Therefore, getTeamSize correctly returns maxPlayers directly for HumansVsNations to display the proper team size (e.g., maxPlayers=5 yields "5 Humans vs 5 Nations").src/client/components/BaseModal.ts (3)
14-26: Inheritance from LitElement is acceptable here.While composition is generally preferred over inheritance, extending
LitElementis the standard pattern for Lit components. The abstract base class provides shared lifecycle and event handling, which fits this use case well.
60-74: LGTM—clean lifecycle hooks for subclasses.The protected
onOpen()andonClose()hooks provide a clear extension point for subclasses without requiring them to override the full open/close methods.
80-95: LGTM—open method handles both inline and modal modes.The method correctly branches between inline (page-based) and modal element rendering. Defensive checks for
window.showPageexistence prevent runtime errors.src/client/PublicLobby.ts (2)
119-219: LGTM—clean responsive layout for public lobby.The new flex-based layout clearly separates the map image, status, mode details, and player/time information. Responsive classes ensure it works across screen sizes.
358-367: LGTM—proper integration with global messaging system.The validation error correctly dispatches a
show-messageevent with appropriate styling (red color, 3s duration) for user feedback. This integrates well with the existing HeadsUpMessage flow.src/client/KeybindsModal.ts (7)
1-31: LGTM - Clean default keybindings setup.The imports and default keybindings are well-structured. Using
KeyboardEvent.codeformat (e.g.,KeyA,Digit1) is the correct approach for keybindings as it's layout-independent.
38-49: LGTM - Safe localStorage loading with error handling.The try-catch around JSON parsing is appropriate. If parsing fails,
keybindsremains empty and the component will fall back toDefaultKeybindsvalues.
51-123: LGTM - Well-implemented conflict detection.The conflict detection logic is solid:
- Correctly builds active keybinds by merging defaults with user overrides
- Allows multiple "Null" (unbound) values
- Provides user-friendly error messages using
formatKeyForDisplay- Safely restores previous value on conflict
- Proper localStorage persistence
125-130: LGTM - Clean value accessor.The method correctly translates "Null" to empty string for unbound keys and returns undefined for defaults.
132-193: LGTM - Clean dual-mode rendering.The render implementation correctly supports both inline and modal modes, matching the pattern established in other BaseModal-extending components. The conditional styling and o-modal wrapping are appropriate.
195-431: LGTM - Comprehensive keybind settings.All keybind settings are consistently configured with proper translation keys, default values, and event handling. The categorization into logical sections improves UX.
433-448: LGTM - Proper lifecycle management.The open/close methods correctly handle escape key registration and navigation for both inline and modal modes.
src/client/UserSettingModal.ts (6)
1-14: LGTM - Clean imports and interface definition.The
FlagInputModalElementinterface properly types the flag input modal interaction, ensuring type safety when callingopen()and settingreturnTo.
28-45: LGTM - Good defensive programming.The containment check at lines 31-35 ensures the easter egg only triggers when typing inside this component, preventing unintended activation from other inputs.
186-194: LGTM - Clean modal navigation pattern.The method properly closes the current modal before opening the flag selector, and correctly sets
returnTofor navigation back.
196-270: LGTM - Consistent dual-mode pattern.The render and close methods follow the established BaseModal pattern for supporting both inline and modal rendering modes.
274-293: LGTM - Well-integrated flag selector.The flag selector UI is cleanly integrated into the settings. The
pointer-events-noneon the flag-input component is correct since clicks are handled by the parent container.
452-467: LGTM - Proper modal lifecycle.The open method correctly handles both inline and modal modes, with appropriate visibility checks for inline mode.
src/client/components/Maps.ts (4)
66-68: LGTM - Correct approach for Tailwind styling.Returning
thisfromcreateRenderRootenables light DOM rendering, which is necessary for Tailwind CSS classes to work properly.
91-134: LGTM - Clean responsive map display.The render method properly handles all states (loading, loaded, error) with appropriate Tailwind v4 classes and smooth transitions.
136-162: LGTM - Clean medal rendering with CSS masks.The medal rendering uses CSS masks effectively to colorize the SVG icons based on earned status. The opacity variation (opacity-100 vs opacity-25) clearly indicates earned vs unearned medals.
8-53: MapDescription mapping is complete and correct.All 44 keys from the
GameMapTypeenum are present in theMapDescriptionconstant with no missing or extra entries.index.html (6)
12-12: Verify that disabling user scaling is intentional.The viewport meta tag includes
user-scalable=noandmaximum-scale=1.0, which prevents users from zooming. This can be an accessibility concern for users who need zoom to read content.For a game application, this might be intentional to prevent accidental zoom gestures during gameplay. However, ensure this aligns with your accessibility requirements and consider whether zoom should be enabled in menus/settings screens.
98-103: Note: Verify blur value for Tailwind v4.Line 99 uses
blur-[6px]as an arbitrary value. In Tailwind v4, blur utilities were updated (e.g.,blur-sm→blur-xs). Arbitrary values likeblur-[6px]should still work, but consider using a named utility if one matches your needs for better consistency.
183-220: LGTM - Sensible mobile navigation.Removing the keybinds menu item for mobile users (line 204) is appropriate since keybinds are keyboard-specific. The desktop navigation correctly includes it at lines 309-313.
360-388: LGTM - Good responsive design.The hamburger button for mobile and flag input for desktop are properly hidden/shown based on screen size. The hamburger has appropriate ARIA attributes for accessibility.
678-762: LGTM - Robust fallback navigation.The fallback sidebar toggle ensures the hamburger menu works even if the main bundle fails to load. The implementation properly handles:
- Open/close state management
- ARIA attributes for accessibility
- Click outside to close
- Event capture to ensure it runs
This is good defensive programming.
541-618: LGTM - Proper link security attributes.External links correctly include
rel="noopener noreferrer"for security. Internal links (terms of service, privacy policy) correctly omit these attributes since the project controls those destinations.src/client/FlagInputModal.ts (3)
7-12: LGTM - Clean BaseModal integration.The
returnToproperty provides a clean way to chain modal navigation, allowing the flag selector to return to the calling modal.
33-138: LGTM - Clean dual-mode flag selector.The render implementation properly handles both inline and modal modes. The flag grid is responsive, has good hover states, and includes error handling for missing flag images.
140-174: LGTM - Clean helper methods.The search filtering and flag selection logic is straightforward and correct. The
setFlagmethod properly updates localStorage and dispatches the event for other components to react.src/client/HostLobbyModal.ts (2)
1105-1122: Verify the commented-out close() call.Line 1110 has a commented-out
this.close()call. Verify whether this is intentional (e.g., the modal should remain open during game start) or if it's leftover debugging code that should be removed or uncommented.
872-887: No action needed —window.showPageis properly typed.
window.showPageis correctly declared insrc/types/global.d.tsas an optional property:showPage?: (pageId: string) => void;. Your code at line 880 properly checksif (needsShow && window.showPage)before calling it, which is the correct defensive programming pattern for an optional property.src/client/TerritoryPatternsModal.ts (2)
402-432: LGTM! Good defensive DOM handling.The refresh method includes proper guards to prevent rendering errors when DOM nodes are disconnected (lines 409-416 and 421-426). The warning message at line 422 is helpful for debugging. This is a solid pattern for working with external DOM references.
358-378: No action needed — the event listener is properly registered.The
show-messageevent is handled by theHeadsUpMessageWeb Component, which registers the listener inconnectedCallback()and cleans it up indisconnectedCallback(). The event handler safely extracts and validates the detail object, supporting both string messages and Lit template results. The implementation is type-safe and follows idiomatic TypeScript patterns.Likely an incorrect or invalid review comment.
src/client/LanguageModal.ts (1)
75-136: LGTM! Clean language list implementation.The language list rendering is well-structured with:
- Proper active state indicators (lines 116-133)
- Responsive grid layout (line 73)
- Good use of truncation for long language names (lines 105, 111)
- Accessible button elements with hover states
| private handleKeyDown = (e: KeyboardEvent) => { | ||
| if (e.key === "Escape" && this.isModalOpen) { | ||
| e.preventDefault(); | ||
| this.close(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd src/client/components && head -n 60 BaseModal.ts | tail -n 35Repository: openfrontio/OpenFrontIO
Length of output: 904
🏁 Script executed:
#!/bin/bash
echo "=== Searching for modal.open() calls ==="
rg -n "\.open\(\)" --type=ts -A2 -B2
echo "=== Searching for showPage calls with modal pages ==="
rg -n "showPage\(" --type=ts -A1 -B1
echo "=== Searching for multiple BaseModal instantiations ==="
rg -n "new BaseModal\|<BaseModal" --type=ts --type=tsx -A1 -B1
echo "=== Checking if there's a modal manager or stack ==="
rg -n "modal" --type=ts --type=tsx -i | grep -i "stack\|manager\|queue" -A1 -B1Repository: openfrontio/OpenFrontIO
Length of output: 11126
Remove misleading comment about event origin check that does not exist.
Line 35 claims the handler "only closes if the modal is open and the event originated within this component," but the code only checks e.key === "Escape" && this.isModalOpen. There is no event.target validation.
If multiple modals can open simultaneously (example: UserSettingModal → FlagInputModal chain), each will register a window keydown listener. When Escape is pressed, all handlers fire and check their own isModalOpen state. This works only if each modal's isModalOpen is mutually exclusive—a fragile assumption not enforced by the architecture.
Either:
- Verify modals are designed to never overlap and document this constraint
- Implement a modal stack to ensure only the top modal responds to Escape
- Add event.target validation to confirm the modal is actually focused
| <div class="mt-4 p-3 bg-red-500/10 border border-red-500/20 rounded-lg"> | ||
| <div | ||
| class="text-xs font-bold text-red-400 uppercase tracking-widest mb-2" | ||
| > | ||
| Disabled Units | ||
| </div> |
There was a problem hiding this comment.
Hardcoded text should use translation.
The "Disabled Units" label is hardcoded instead of using a translation key, which breaks localization.
🌍 Proposed fix
<div
class="text-xs font-bold text-red-400 uppercase tracking-widest mb-2"
>
- Disabled Units
+ ${translateText("private_lobby.disabled_units")}
</div>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/client/JoinPrivateLobbyModal.ts around lines 329 - 334, The "Disabled
Units" label in the JoinPrivateLobbyModal component is hardcoded; replace it
with a translation lookup (e.g., use the i18n hook/translator function already
used in this component) and ensure the translation key (suggested:
"joinPrivateLobby.disabledUnits") is used instead of the literal string; add the
new key to the locale files and import/use the existing translator (t) in
JoinPrivateLobbyModal so the div with class "text-xs font-bold text-red-400..."
renders t('joinPrivateLobby.disabledUnits') rather than the hardcoded text.
| private handleClose = () => { | ||
| this.style.pointerEvents = "none"; | ||
| }; |
There was a problem hiding this comment.
Incomplete handleClose implementation.
The handleClose method only sets pointer-events to "none" but doesn't perform any other cleanup or call the base close logic. This is wired to o-modal's onClose callback (line 151), which might lead to incomplete cleanup when the modal's close button is clicked. Consider calling this.close() or adding any necessary cleanup logic.
Suggested fix
- private handleClose = () => {
- this.style.pointerEvents = "none";
- };
+ private handleClose = () => {
+ this.close();
+ };Note: If the pointer-events manipulation is necessary, it should be moved to the onClose lifecycle method inherited from BaseModal.
📝 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 handleClose = () => { | |
| this.style.pointerEvents = "none"; | |
| }; | |
| private handleClose = () => { | |
| this.close(); | |
| }; |
🤖 Prompt for AI Agents
In @src/client/LanguageModal.ts around lines 12 - 14, handleClose currently only
sets this.style.pointerEvents = "none" which prevents the modal's base closing
logic from running when wired to o-modal's onClose; update handleClose to call
this.close() after (or before) adjusting pointer-events so BaseModal cleanup
runs, or move the pointer-events change into the inherited onClose lifecycle
method on BaseModal and let handleClose simply call this.close(); ensure you
reference the handleClose method, the close() method, the onClose lifecycle from
BaseModal, and this.style.pointerEvents when making the change.
Description:
Overhauls the Main Menu UI, visit https://menu.openfront.dev to see everything.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
w.o.n