Add local development setup with dev feature flags and improved settings UI #2714
Conversation
WalkthroughAdds a dev-config system with runtime feature flags and conditional client script loading; implements theme modes and a new Display settings UI with multiple setting components; adds dev tooling (dev-stop script, dotenv/port wiring), server feature gating for public games, SAM radius rendering rewrite, AI behavior refactors, localization updates, and assorted styling/component changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant ClientMain as Client Main
participant DevConfig as DevConfig loader
participant Fetch as /config.json
participant UserSettings as UserSettings
Browser->>ClientMain: page load
ClientMain->>DevConfig: seed sync + waitForDevConfig()
DevConfig->>Fetch: GET /config.json
Fetch-->>DevConfig: config or failure
DevConfig-->>ClientMain: resolved config
ClientMain->>ClientMain: isDevFeatureEnabled('cloudflare')?
alt enabled
ClientMain->>Browser: insert Turnstile script
else
ClientMain->>ClientMain: skip Turnstile (token gating)
end
ClientMain->>UserSettings: applyTheme()
UserSettings->>Browser: add/remove "dark" class
sequenceDiagram
participant User
participant SettingsModal as UserSettingModal
participant ThemeComp as SettingThemeMode
participant UserSettings as UserSettings
User->>SettingsModal: open Display tab
SettingsModal->>ThemeComp: mount selector
User->>ThemeComp: choose "Dark"
ThemeComp->>SettingsModal: dispatch change {mode: "dark"}
SettingsModal->>UserSettings: setThemeMode("dark")
UserSettings->>UserSettings: persist & applyTheme()
UserSettings->>SettingsModal: emit dark-mode-changed
SettingsModal->>SettingsModal: re-render with dark UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
nodeGarden
force-pushed
the
feature/local-dev-setup
branch
from
bcddf9f to
e5c817b
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/UserSettingModal.ts (1)
87-112: Remove unusedtoggleDarkModemethod.This method in
UserSettingModal.tsis dead code. It was replaced byhandleThemeModeChange(line 280), which uses the newThemeModetyped union system ("light" | "dark" | "system") instead of the old booleansettings.darkMode. The method is never called from anywhere in the codebase.
🧹 Nitpick comments (12)
webpack.config.js (1)
18-19: Inconsistent parsing:clientPortstays string,serverPortis parsed to int.
clientPortis used directly as a string, whileserverPortis parsed withparseInt. This works becausedevServer.portaccepts both, but the arithmetic onserverPort(e.g.,serverPort + 1) requires an int.For clarity, consider parsing both or keeping both as strings with explicit conversion where needed:
Option: parse both to int
-const clientPort = parseInt(process.env.OPENFRONT_CLIENT_PORT ?? "9000", 10); -const serverPort = parseInt(process.env.OPENFRONT_SERVER_PORT ?? "3000", 10); +const clientPort = parseInt(process.env.OPENFRONT_CLIENT_PORT ?? "9000", 10); +const serverPort = parseInt(process.env.OPENFRONT_SERVER_PORT ?? "3000", 10);(Both already parsed—looks like this is fine as-is,
clientPortjust doesn't need arithmetic. No change needed.)scripts/dev-stop.js (1)
6-7: Same inconsistency as webpack:clientPortis string,serverPortis int.Works fine due to
String(port)on line 23, but matching types would be cleaner.Parse both consistently
-const clientPort = process.env.OPENFRONT_CLIENT_PORT ?? "9000"; -const serverPort = parseInt(process.env.OPENFRONT_SERVER_PORT ?? "3000", 10); +const clientPort = parseInt(process.env.OPENFRONT_CLIENT_PORT ?? "9000", 10); +const serverPort = parseInt(process.env.OPENFRONT_SERVER_PORT ?? "3000", 10);src/client/components/baseComponents/setting/SettingTerritorySkins.ts (2)
11-81: Premium flag has no enforcement.The
premiumflag on skins is decorative only—users can select and save premium skins without restriction (seeselectSkinat line 310). If this is intentional for the placeholder/mockup phase, consider adding a comment. Otherwise, gate premium skins behind a purchase check.
296-308: Remove stale custom URL from localStorage.When switching from a custom skin to a preset, the old
settings.territorySkinCustomvalue remains in storage. On reload, this could cause confusion.🔎 Proposed fix
private saveSkin() { localStorage.setItem("settings.territorySkin", this.selectedSkinId); if (this.customSkinUrl) { localStorage.setItem("settings.territorySkinCustom", this.customSkinUrl); + } else { + localStorage.removeItem("settings.territorySkinCustom"); } this.dispatchEvent( new CustomEvent("change", { detail: { skinId: this.selectedSkinId, customUrl: this.customSkinUrl }, bubbles: true, composed: true, }), ); }src/client/components/baseComponents/setting/SettingColorPalette.ts (1)
432-447: Consider user feedback for invalid input.Invalid hex values are silently ignored. Users might not realize why their input didn't apply. A brief visual cue (e.g., red border flash or tooltip) would improve UX.
src/client/components/baseComponents/setting/SettingThemeMode.ts (1)
140-162: Hardcoded theme labels should use translation keys.The labels "Light", "Dark", and "System" are hardcoded. Other components in this PR use
translateText()for user-facing strings.🔎 Proposed fix to use translations
You would need to import
translateTextand update the labels:+import { translateText } from "../../../Utils"; ... - <span class="theme-label">Light</span> + <span class="theme-label">${translateText("user_setting.theme_light")}</span> ... - <span class="theme-label">Dark</span> + <span class="theme-label">${translateText("user_setting.theme_dark")}</span> ... - <span class="theme-label">System</span> + <span class="theme-label">${translateText("user_setting.theme_system")}</span>src/core/game/UserSettings.ts (1)
87-104: System mode does not react to OS preference changes after initial apply.When
mode === "system", the theme is applied based on current OS preference at call time. If the user changes their OS theme while the app is open, the UI will not update untilapplyTheme()is called again.Consider adding a
matchMedialistener when using system mode to auto-update.🔎 Example approach for reactive system theme
// In applyTheme or a dedicated method: private systemThemeQuery?: MediaQueryList; applyTheme(): void { const mode = this.themeMode(); // Clean up previous listener this.systemThemeQuery?.removeEventListener("change", this.handleSystemThemeChange); if (mode === "system") { this.systemThemeQuery = window.matchMedia("(prefers-color-scheme: dark)"); this.systemThemeQuery.addEventListener("change", this.handleSystemThemeChange); // Apply current value document.documentElement.classList.toggle("dark", this.systemThemeQuery.matches); } else { document.documentElement.classList.toggle("dark", mode === "dark"); } }src/client/styles/components/setting.css (1)
399-411: Sticky tabs use fragile negative margin calculation.The margin
-1.4remand widthcalc(100% + 2.8rem)depend on the parent's exact padding. If modal padding changes, this breaks.Consider using a different approach like
margin-inline: calc(-1 * var(--modal-padding, 1.4rem))if you introduce a padding variable.src/client/components/baseComponents/setting/SettingGroup.ts (2)
96-107: Empty catch blocks hide localStorage errors.While it's fine to ignore parse errors for robustness, logging in dev mode helps debugging.
🔎 Proposed fix to log errors in dev
} catch { - // Ignore parse errors + // Ignore parse errors in production + if (import.meta.env?.DEV) { + console.warn("Failed to parse collapsed state from localStorage"); + } }
72-80: Duplicate!importantrules for collapsed state.Lines 68-70 already define
.setting-group__content.collapsed { display: none !important; }, and lines 78-80 repeat this for.columns.collapsed. The first rule already applies to elements with both classes.🔎 Remove duplicate rule
.setting-group__content.columns { display: grid !important; grid-template-columns: repeat(2, 1fr) !important; gap: 12px; } - - .setting-group__content.columns.collapsed { - display: none !important; - }src/client/DevConfig.ts (2)
106-122: Consider consolidating the helper functions with a generic.The three helper functions follow the same pattern and could be unified to reduce repetition.
🔎 Proposed refactor using a generic helper
- const applyBool = (key: string, value: boolean | undefined) => { - if (value !== undefined && localStorage.getItem(key) === null) { - localStorage.setItem(key, value.toString()); - } - }; - - const applyFloat = (key: string, value: number | undefined) => { - if (value !== undefined && localStorage.getItem(key) === null) { - localStorage.setItem(key, value.toString()); - } - }; - - const applyString = (key: string, value: string | undefined) => { - if (value !== undefined && localStorage.getItem(key) === null) { - localStorage.setItem(key, value); - } - }; + const applySetting = <T>(key: string, value: T | undefined) => { + if (value !== undefined && localStorage.getItem(key) === null) { + localStorage.setItem(key, String(value)); + } + };Then replace all calls to
applyBool,applyFloat, andapplyStringwithapplySetting.
105-183: Consider adding value validation before writing to localStorage.The function writes config values directly to localStorage without validating they are in expected ranges or formats. For example,
backgroundMusicVolumecould be negative or greater than 1, andthemeModecould be an invalid string.🔎 Example validation for constrained values
Add validation helpers before the apply functions:
const validateThemeMode = (value: string | undefined): "light" | "dark" | "system" | undefined => { if (value === "light" || value === "dark" || value === "system") { return value; } return undefined; }; const clampVolume = (value: number | undefined): number | undefined => { if (typeof value === "number") { return Math.max(0, Math.min(1, value)); } return undefined; };Then use them when applying settings:
applyString("settings.themeMode", validateThemeMode(settings.display.themeMode)); applyFloat("settings.backgroundMusicVolume", clampVolume(settings.audio.backgroundMusicVolume));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
docs/user-settings--display-2.jpgis excluded by!**/*.jpgdocs/user-settings--display.jpgis excluded by!**/*.jpgdocs/user-settings--keybind.jpgis excluded by!**/*.jpgdocs/user-settings--settings.jpgis excluded by!**/*.jpgpackage-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
.gitignoreconfig.example.jsoneslint.config.jspackage.jsonresources/lang/en.jsonscripts/dev-stop.jssrc/client/DarkModeButton.tssrc/client/DevConfig.tssrc/client/InputHandler.tssrc/client/Main.tssrc/client/PublicLobby.tssrc/client/UserSettingModal.tssrc/client/components/baseComponents/Modal.tssrc/client/components/baseComponents/setting/SettingColorPalette.tssrc/client/components/baseComponents/setting/SettingGroup.tssrc/client/components/baseComponents/setting/SettingTerritorySkins.tssrc/client/components/baseComponents/setting/SettingThemeMode.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/client/index.htmlsrc/client/styles.csssrc/client/styles/components/modal.csssrc/client/styles/components/setting.csssrc/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/game/UserSettings.tssrc/server/Master.tstests/util/TestServerConfig.tstsconfig.jsonwebpack.config.js
💤 Files with no reviewable changes (2)
- src/client/styles.css
- src/client/styles/components/modal.css
🧰 Additional context used
🧠 Learnings (8)
📚 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/graphics/layers/PlayerInfoOverlay.tssrc/server/Master.tssrc/core/configuration/Config.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/PublicLobby.tssrc/client/Main.tssrc/client/UserSettingModal.ts
📚 Learning: 2025-06-07T19:47:34.514Z
Learnt from: its-sii
Repo: openfrontio/OpenFrontIO PR: 1089
File: src/client/styles/components/setting.css:9-22
Timestamp: 2025-06-07T19:47:34.514Z
Learning: In src/client/styles/components/setting.css, the padding difference between .setting-item and .setting-item-group is intentional - .setting-item-group doesn't need padding for the specific grouped setting styling being implemented.
Applied to files:
src/client/styles/components/setting.csssrc/client/components/baseComponents/setting/SettingGroup.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/core/configuration/Config.ts
📚 Learning: 2025-11-12T23:11:34.445Z
Learnt from: MaxHT0x
Repo: openfrontio/OpenFrontIO PR: 2262
File: src/core/configuration/DefaultConfig.ts:806-832
Timestamp: 2025-11-12T23:11:34.445Z
Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.
Applied to files:
src/client/DevConfig.ts
📚 Learning: 2025-07-26T05:35:03.686Z
Learnt from: aaa4xu
Repo: openfrontio/OpenFrontIO PR: 1581
File: webpack.config.js:139-141
Timestamp: 2025-07-26T05:35:03.686Z
Learning: In OpenFrontIO's webpack configuration, binary-loader processes files from resources/maps at build-time and inlines the binary payload directly into the compiled JS bundle. This means the original .bin assets are no longer needed at runtime and can be safely excluded from the final Docker image using webpack's globOptions.ignore pattern to reduce image size.
Applied to files:
webpack.config.js
📚 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/core/game/UserSettings.tssrc/client/UserSettingModal.ts
📚 Learning: 2025-08-28T15:14:40.664Z
Learnt from: mokizzz
Repo: openfrontio/OpenFrontIO PR: 1956
File: src/client/InputHandler.ts:430-449
Timestamp: 2025-08-28T15:14:40.664Z
Learning: On trackpads and touchscreens, users don't expect purely vertical two-finger swipes to trigger zoom. Pan detection using deltaX !== 0 is appropriate since any horizontal movement indicates pan intent, and zoom should be handled via pinch gestures instead.
Applied to files:
src/client/InputHandler.ts
🧬 Code graph analysis (4)
src/server/Master.ts (3)
src/core/game/GameImpl.ts (1)
config(336-338)src/core/game/GameView.ts (1)
config(665-667)src/client/PublicLobby.ts (1)
fetchLobbies(90-111)
src/client/PublicLobby.ts (1)
src/client/DevConfig.ts (2)
waitForDevConfig(191-193)isDevFeatureEnabled(205-209)
src/client/components/baseComponents/setting/SettingThemeMode.ts (1)
src/core/game/UserSettings.ts (2)
ThemeMode(6-6)darkMode(66-68)
src/client/Main.ts (1)
src/client/DevConfig.ts (2)
loadDevConfig(214-214)isDevFeatureEnabled(205-209)
🔇 Additional comments (52)
src/client/graphics/layers/PlayerInfoOverlay.ts (1)
3-3: Import path is correct for Lit v3.The current import
lit/directives/ref.jsis the correct modern path. The project uses Lit v3.3.1, which supports this structure. No oldlit-htmlimports remain in the codebase. The usage at line 321 is correct and idiomatic.src/client/InputHandler.ts (2)
191-191: LGTM: Keybind change to Backslash.The change from Space to Backslash for the toggleView keybind is reasonable and reduces potential conflicts with text input and browser shortcuts.
555-560: LGTM: Shift+drag pan prevention is well-implemented.The logic correctly prevents panning when Shift is held, reserving Shift+drag for bounding box selection. This implementation is consistent with the existing keyboard panning behavior at lines 243-248, which also skips panning when Shift keys are active.
tsconfig.json (1)
24-34: Good config updates for the dev workflow.
skipLibCheck: truespeeds up builds and avoids type conflicts in third-party packages. Adding"jest"to types keeps global type pollution minimal. The"scripts/**/*"include path matches the newscripts/dev-stop.jsfile added in this PR..gitignore (1)
13-24: Well-organized ignore rules.Good additions:
config.jsonfiles stay local (users copy fromconfig.example.json)package-lock.jsonignored since the project uses pnpm- Clear section comments improve readability
config.example.json (1)
1-36: Clear example config for local development.Good structure with feature flags disabled by default (
analytics,publicLobbies,cloudflare,adsallfalse). Settings are grouped by concern (display, interface, graphics, controls, privacy, audio), making it easy for developers to customize their local setup.webpack.config.js (1)
189-266: Proxy targets correctly use dynamic ports.WebSocket and HTTP proxies for workers use
serverPort + 1,+2,+3, matching theworkerPortByIndexpattern inDefaultConfig. API endpoints route toserverPort. All entries are consistent.package.json (1)
12-12: Handy cleanup script.
dev:stopfits the naming pattern (dev:staging,dev:prod) and provides a quick way to kill dev processes on configured ports.eslint.config.js (1)
31-31: Correct ESLint config for the new script.Adding
scripts/dev-stop.jstoallowDefaultProjectlets ESLint process it without requiring a full tsconfig reference. Placement is alphabetically correct.src/core/configuration/DefaultConfig.ts (1)
234-236: Clean opt-out toggle for public games.The method returns
trueby default, requiring explicitENABLE_PUBLIC_GAMES=falseto disable. This keeps production behavior on while letting developers skip public game scheduling locally. The method is properly declared in theServerConfiginterface and used in game scheduling logic.src/client/components/baseComponents/setting/SettingTerritorySkins.ts (2)
87-93: LGTM!Default skin "cosmos" exists in
AVAILABLE_SKINSand state initialization is clean.
310-315: LGTM!The disabled guard and state updates are correct.
src/client/components/baseComponents/setting/SettingColorPalette.ts (3)
280-302: LGTM!Robust error handling with a safe fallback for corrupted localStorage.
328-356: LGTM!Standard HSL conversion with correct edge case handling.
358-396: LGTM!Clean HSL-to-hex conversion with proper padding.
resources/lang/en.json (1)
367-407: LGTM!Localization updates align with the new Display tab and settings groups. Clean text improvements with emoji cleanup.
tests/util/TestServerConfig.ts (1)
16-18: LGTM!Test stub follows the established pattern for unimplemented methods.
src/client/DarkModeButton.ts (1)
8-8: LGTM!Method rename to
isDarkModeActive()is clearer and consistent.Also applies to: 31-31
src/core/configuration/Config.ts (1)
68-68: LGTM!Clean interface addition for the public games feature toggle.
src/server/Master.ts (2)
96-114: LGTM!Clean feature toggle with informative logging when disabled.
142-149: Restrict port range to 1–65535 to prevent unintended dynamic port assignment.Port validation currently allows 0, which tells the operating system to automatically pick a random available port. This is unusual for a production server and has no documented purpose in the code. Change the validation from
PORT < 0toPORT < 1.src/client/PublicLobby.ts (2)
34-46: LGTM!Clean async feature flag gating that waits for config to load before initializing.
121-124: LGTM!Consistent feature flag gating in render method.
src/client/Main.ts (5)
17-17: LGTM! Clean import for dev feature flag utilities.Imports align with the DevConfig module pattern for conditional feature loading.
112-115: LGTM! Type update correctly reflects nullable token.The union with
nullmatches the newgetTurnstileTokenbehavior when cloudflare feature is disabled.
120-121: LGTM! Config loaded early before dependent logic runs.Awaiting
loadDevConfig()at the start ofinitialize()ensures feature flags are ready for all downstream checks.
348-349: LGTM! Theme application simplified to single call.Replacing manual DOM class manipulation with
applyTheme()centralizes theme logic inUserSettings.
716-720: LGTM! Early exit prevents unnecessary Turnstile wait loop.When cloudflare feature is disabled, returning
nullimmediately avoids the 10-second polling loop forwindow.turnstile.src/client/index.html (5)
133-147: Synchronous XHR blocks page render until config loads.The synchronous request ensures
window.__devConfigis set before any conditional script checks run. This is fine for local dev where/config.jsonexists. In production (missing file), thecatchblock handles the 404 silently.One note: if the server is slow, this blocks the entire page. This is acceptable for dev environments but worth documenting.
149-161: LGTM! Turnstile conditionally loaded based on cloudflare flag.The pattern
!window.__devConfig || ...cloudflare !== falseensures scripts load in production (no config) and only skip in dev when explicitly disabled.
163-176: LGTM! Ads script conditionally loaded.Same safe pattern applied for Publift/Fuse ads.
178-204: LGTM! Analytics scripts conditionally loaded.Google Ads, Analytics, and gtag setup are gated behind the analytics feature flag.
523-535: LGTM! Cloudflare Analytics beacon conditionally loaded.Consistent pattern with other conditional script blocks.
src/client/components/baseComponents/setting/SettingThemeMode.ts (2)
91-100: LGTM! Clean lifecycle management for event listener.The
dark-mode-changedlistener is properly added on connect and removed on disconnect. Arrow function binding is correct.
102-111: LGTM! Legacy migration for darkMode setting.Fallback from
settings.darkModeto"dark"or"system"handles upgrades from the old boolean setting.src/core/game/UserSettings.ts (3)
6-6: LGTM! Clean typed union for theme modes.Using a union type instead of enum or class hierarchy is the right approach.
70-80: LGTM! Theme mode getter with validation and legacy support.Validates stored value before returning and falls back to legacy
darkModemigration.
176-188: LGTM! Toggle now uses theme mode system with event dispatch.The toggle cycles between light and dark (skipping system), which is a reasonable UX for a quick-toggle button. Event dispatch keeps other components in sync.
src/client/styles/components/setting.css (2)
9-27: LGTM! Grouped settings layout with responsive handling.Fixed 720px width on desktop provides consistent layout, with full-width fallback on mobile.
33-50: LGTM! CSS variables enable theme switching.Fallback values ensure dark theme works even outside Modal context.
src/client/components/baseComponents/Modal.ts (3)
13-29: LGTM! MutationObserver properly manages theme detection lifecycle.Observer watches only
classattribute changes ondocumentElementand disconnects on component removal. This is efficient.
118-138: LGTM! Light theme CSS variables provide clean theming.Defining theme tokens in the wrapper class allows child components to inherit correct values without additional logic.
182-189: LGTM! Class list construction is clean and readable.Using array with
filter(Boolean).join(" ")handles conditional classes well without template string clutter.src/client/components/baseComponents/setting/SettingGroup.ts (1)
128-152: LGTM! Clean render with composition via slot.The group component wraps slotted content, enabling flexible use across different setting types.
src/client/UserSettingModal.ts (5)
4-12: LGTM! Clean imports for new setting components.All new components are properly imported with side-effect registrations.
19-19: LGTM! Settings mode extended with display tab.Union type extended cleanly without breaking existing tab logic.
280-298: LGTM! Theme mode change handler with proper event dispatch.Dispatches both
theme-mode-changed(for new consumers) anddark-mode-changed(for DarkModeButton backward compatibility).
300-330: LGTM! Display settings grouped logically.Theme mode, territory skins, and color palette are in separate collapsible groups. The
disabledattribute on placeholders indicates future functionality.
726-729: LGTM! requestAnimationFrame fixes column layout timing.Forcing a reflow after modal opens ensures the grouped/column layout renders correctly on first open.
src/client/DevConfig.ts (3)
4-57: Interface design is clean and well-structured.The nested (preferred) and flat (legacy) format coexistence is well-documented and appropriate for backwards compatibility. Type definitions use clear, simple types that are easy to understand.
186-186: Auto-loading design is appropriate for this use case.The immediate config load on module import is a reasonable trade-off. The API provides both
waitForDevConfig()for async contexts andgetDevConfig()for sync contexts (with defaults fallback). This pattern ensures config is available as early as possible without blocking module imports.
191-214: Public API is well-designed and clearly documented.The mix of async (
waitForDevConfig) and sync (getDevConfig) accessors provides flexibility for different usage contexts. The deprecation notice forloadDevConfigis properly marked. The convenience functionisDevFeatureEnabledsimplifies the common use case.
| private loadSkin() { | ||
| const saved = localStorage.getItem("settings.territorySkin"); | ||
| if (saved) { | ||
| this.selectedSkinId = saved; | ||
| } | ||
| const customUrl = localStorage.getItem("settings.territorySkinCustom"); | ||
| if (customUrl) { | ||
| this.customSkinUrl = customUrl; | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate loaded skin ID and custom URL.
Loading from localStorage without validation risks broken state:
- If
settings.territorySkincontains an invalid ID, the component won't find a matching skin settings.territorySkinCustomcould contain a malicious URL (e.g.,javascript:scheme)
🔎 Suggested validation
private loadSkin() {
const saved = localStorage.getItem("settings.territorySkin");
if (saved) {
- this.selectedSkinId = saved;
+ // Validate the saved skin exists
+ const skinExists = AVAILABLE_SKINS.some(s => s.id === saved) || saved === "custom";
+ if (skinExists) {
+ this.selectedSkinId = saved;
+ }
}
const customUrl = localStorage.getItem("settings.territorySkinCustom");
if (customUrl) {
- this.customSkinUrl = customUrl;
+ // Basic validation: must be http(s) or data URL
+ if (customUrl.startsWith("http://") ||
+ customUrl.startsWith("https://") ||
+ customUrl.startsWith("data:image/")) {
+ this.customSkinUrl = customUrl;
+ }
}
}🤖 Prompt for AI Agents
In src/client/components/baseComponents/setting/SettingTerritorySkins.ts around
lines 285-294, the loadSkin method reads settings.territorySkin and
settings.territorySkinCustom from localStorage without validation; update it to
validate the saved skin ID against the current list of available skins (only
assign this.selectedSkinId if it matches an existing skin, otherwise leave or
set a safe default), and validate/sanitize the custom URL by allowing only safe
schemes (e.g., http and https), rejecting or clearing anything with javascript:
or other disallowed schemes, and handle non-string or malformed values
gracefully; ensure any invalid values fall back to safe defaults and do not get
assigned to component state.
There was a problem hiding this comment.
@nodeGarden, understood! I'll skip this validation since it's placeholder UX. Feel free to revisit when the final implementation is ready. 👍
| private handleUploadClick() { | ||
| if (this.disabled) return; | ||
| // Create hidden file input | ||
| const input = document.createElement("input"); | ||
| input.type = "file"; | ||
| input.accept = "image/*"; | ||
| input.onchange = (e) => { | ||
| const file = (e.target as HTMLInputElement).files?.[0]; | ||
| if (file) { | ||
| const reader = new FileReader(); | ||
| reader.onload = () => { | ||
| this.customSkinUrl = reader.result as string; | ||
| this.selectedSkinId = "custom"; | ||
| this.saveSkin(); | ||
| }; | ||
| reader.readAsDataURL(file); | ||
| } | ||
| }; | ||
| input.click(); | ||
| } |
There was a problem hiding this comment.
Add file size validation and error handling.
Reading large images without limits risks:
- localStorage quota exceeded (typically 5-10MB per origin)
- Browser memory issues
- Poor UX with no feedback on failure
🔎 Recommended safeguards
private handleUploadClick() {
if (this.disabled) return;
- // Create hidden file input
const input = document.createElement("input");
input.type = "file";
input.accept = "image/*";
input.onchange = (e) => {
const file = (e.target as HTMLInputElement).files?.[0];
if (file) {
+ // Limit to 2MB to avoid localStorage quota issues
+ const MAX_FILE_SIZE = 2 * 1024 * 1024;
+ if (file.size > MAX_FILE_SIZE) {
+ console.error("File too large. Maximum size is 2MB.");
+ // TODO: Show user-facing error message
+ return;
+ }
const reader = new FileReader();
reader.onload = () => {
this.customSkinUrl = reader.result as string;
this.selectedSkinId = "custom";
this.saveSkin();
};
+ reader.onerror = () => {
+ console.error("Failed to read file.");
+ // TODO: Show user-facing error message
+ };
reader.readAsDataURL(file);
}
};
input.click();
}📝 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 handleUploadClick() { | |
| if (this.disabled) return; | |
| // Create hidden file input | |
| const input = document.createElement("input"); | |
| input.type = "file"; | |
| input.accept = "image/*"; | |
| input.onchange = (e) => { | |
| const file = (e.target as HTMLInputElement).files?.[0]; | |
| if (file) { | |
| const reader = new FileReader(); | |
| reader.onload = () => { | |
| this.customSkinUrl = reader.result as string; | |
| this.selectedSkinId = "custom"; | |
| this.saveSkin(); | |
| }; | |
| reader.readAsDataURL(file); | |
| } | |
| }; | |
| input.click(); | |
| } | |
| private handleUploadClick() { | |
| if (this.disabled) return; | |
| const input = document.createElement("input"); | |
| input.type = "file"; | |
| input.accept = "image/*"; | |
| input.onchange = (e) => { | |
| const file = (e.target as HTMLInputElement).files?.[0]; | |
| if (file) { | |
| // Limit to 2MB to avoid localStorage quota issues | |
| const MAX_FILE_SIZE = 2 * 1024 * 1024; | |
| if (file.size > MAX_FILE_SIZE) { | |
| console.error("File too large. Maximum size is 2MB."); | |
| // TODO: Show user-facing error message | |
| return; | |
| } | |
| const reader = new FileReader(); | |
| reader.onload = () => { | |
| this.customSkinUrl = reader.result as string; | |
| this.selectedSkinId = "custom"; | |
| this.saveSkin(); | |
| }; | |
| reader.onerror = () => { | |
| console.error("Failed to read file."); | |
| // TODO: Show user-facing error message | |
| }; | |
| reader.readAsDataURL(file); | |
| } | |
| }; | |
| input.click(); | |
| } |
There was a problem hiding this comment.
@nodeGarden, understood. I'll skip this for the placeholder implementation. 👍
…ngs UI Local Development: - Configurable ports via OPENFRONT_CLIENT_PORT and OPENFRONT_SERVER_PORT - Dev feature flags system (DevConfig.ts) for toggling features locally - pnpm dev:stop script to kill processes on dev ports (Windows + Unix) - config.example.json template for local configuration - ENABLE_PUBLIC_GAMES env var to disable public game scheduling Settings UI Refactor: - Tabbed interface (Settings, Keybinds, Display) - Collapsible grouped settings with 2-column masonry layout - Light/dark theme support with synced dark mode button - Territory color palette editor - Territory skins placeholder (cosmetics) Server Improvements: - PORT validation in Master.ts (0-65535 range check) - Removed dead code (modal.css, unused pastel theme files) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
nodeGarden
force-pushed
the
feature/local-dev-setup
branch
from
e5c817b to
379a665
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/client/InputHandler.ts (1)
439-442: Remove debug logging statements.These
console.logcalls appear to be debug artifacts and should be removed before merging to production.🔎 Proposed fix
- // Shift-D to toggle performance overlay - console.log(e.code, e.shiftKey, e.ctrlKey, e.altKey, e.metaKey); if (e.code === "KeyD" && e.shiftKey) { e.preventDefault(); - console.log("TogglePerformanceOverlayEvent"); this.eventBus.emit(new TogglePerformanceOverlayEvent()); }webpack.config.js (1)
136-145: Inconsistent WEBSOCKET_URL still hardcoded to port 3000.The
WEBSOCKET_URLuses a fixedlocalhost:3000while dev server proxies now use dynamicserverPort. This could cause connection failures when using custom ports.🔎 Proposed fix
new webpack.DefinePlugin({ "process.env.WEBSOCKET_URL": JSON.stringify( - isProduction ? "" : "localhost:3000", + isProduction ? "" : `localhost:${serverPort}`, ),
♻️ Duplicate comments (5)
scripts/dev-stop.js (2)
53-59: Unix branch always reports "stopped" even when no process exists.The
|| trueensures the command never fails, sostoppedPorts.push(portNum)runs unconditionally. This was flagged in a past review.
71-72: Logs all ports instead of stopped ports.Line 72 logs
ports.join(", ")rather thanstopped.join(", "). This was flagged in a past review.src/client/components/baseComponents/setting/SettingTerritorySkins.ts (2)
285-294: Validate loaded skin ID and custom URL.This was flagged in a past review. Loading from localStorage without validation risks broken state or malicious URLs.
325-344: Add file size validation and error handling.This was flagged in a past review. Large files could exceed localStorage quota or cause memory issues.
src/client/DevConfig.ts (1)
74-98: Validation for config structure was flagged in a previous review.A past review suggested adding basic structure validation after parsing the JSON. This remains unaddressed.
🧹 Nitpick comments (8)
src/client/components/baseComponents/setting/SettingGroup.ts (1)
96-107: Consider adding a type check for the parsed states object.If localStorage gets corrupted (e.g.,
"settings.collapsedGroups"becomes a non-object), the accessstates[this.groupId]could fail unexpectedly.Optional safeguard
private loadCollapsedState() { if (!this.groupId) return; try { const saved = localStorage.getItem(COLLAPSED_STATE_KEY); if (saved) { const states = JSON.parse(saved) as Record<string, boolean>; - this.collapsed = states[this.groupId] ?? false; + if (states && typeof states === "object" && !Array.isArray(states)) { + this.collapsed = states[this.groupId] ?? false; + } } } catch { // Ignore parse errors } }scripts/dev-stop.js (1)
6-7: Type inconsistency between clientPort and serverPort.
clientPortis a string,serverPortis parsed to number. When building theportsarray, you mix string and number. This works because line 23 coerces to string, but it is confusing.Consistent types
-const clientPort = process.env.OPENFRONT_CLIENT_PORT ?? "9000"; -const serverPort = parseInt(process.env.OPENFRONT_SERVER_PORT ?? "3000", 10); +const clientPort = parseInt(process.env.OPENFRONT_CLIENT_PORT ?? "9000", 10); +const serverPort = parseInt(process.env.OPENFRONT_SERVER_PORT ?? "3000", 10);src/client/index.html (1)
134-147: Synchronous XHR blocks page rendering.The synchronous
XMLHttpRequestblocks the main thread until/config.jsonloads or fails. Consider using a small inline default and loading config asynchronously to avoid blocking initial paint.That said, I understand the need to conditionally load scripts before they execute, so this may be an acceptable trade-off for dev environments.
src/client/components/baseComponents/setting/SettingColorPalette.ts (2)
328-356: hexToHsl assumes valid 7-character hex string.If
hexis malformed (e.g.,"#FFF"shorthand or invalid chars),parseIntmay returnNaNand cause incorrect calculations. Consider adding validation or a fallback.🔎 Proposed defensive check
private hexToHsl(hex: string): { h: number; s: number; l: number } { + // Normalize shorthand hex and validate + if (!/^#[0-9A-Fa-f]{6}$/.test(hex)) { + return { h: 0, s: 0, l: 50 }; // Safe fallback + } const r = parseInt(hex.slice(1, 3), 16) / 255; const g = parseInt(hex.slice(3, 5), 16) / 255; const b = parseInt(hex.slice(5, 7), 16) / 255;
402-424: Click-only color picking may frustrate users.The color preview and hue slider respond only to clicks, not drag. Users often expect to drag to fine-tune colors. Consider adding
mousedown+mousemovelisteners.src/client/DevConfig.ts (2)
92-94: Silent catch swallows all fetch errors.The empty catch block hides network errors, CORS issues, and JSON parse failures. Consider logging in dev mode or distinguishing 404 from other errors.
🔎 Proposed improvement
- } catch { - // config.json not found, use defaults + } catch (error) { + // Log non-404 errors for debugging + if (error instanceof SyntaxError) { + console.warn("DevConfig: Invalid JSON in config.json", error); + } }
105-122: Helper functions are repetitive but clear.The
applyBool,applyFloat, andapplyStringhelpers share the same pattern. Consider a generic helper, though the current approach is readable and type-safe.🔎 Optional consolidation
const applyValue = <T extends string | number | boolean>( key: string, value: T | undefined, ) => { if (value !== undefined && localStorage.getItem(key) === null) { localStorage.setItem(key, String(value)); } };src/client/UserSettingModal.ts (1)
111-112: Debug console.log statements remain in toggle handlers.Multiple toggle handlers still log state changes (e.g., "🌙 Dark Mode: ON"). Consider removing these before merge or gating behind a debug flag.
Also applies to: 120-121, 129-130, 138-139, 147-148, 156-157, 165-166, 173-174, 181-182
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
docs/user-settings--display-2.jpgis excluded by!**/*.jpgdocs/user-settings--display.jpgis excluded by!**/*.jpgdocs/user-settings--keybind.jpgis excluded by!**/*.jpgdocs/user-settings--settings.jpgis excluded by!**/*.jpgpackage-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
.gitignoreconfig.example.jsoneslint.config.jspackage.jsonresources/lang/en.jsonscripts/dev-stop.jssrc/client/DarkModeButton.tssrc/client/DevConfig.tssrc/client/InputHandler.tssrc/client/Main.tssrc/client/PublicLobby.tssrc/client/UserSettingModal.tssrc/client/components/baseComponents/Modal.tssrc/client/components/baseComponents/setting/SettingColorPalette.tssrc/client/components/baseComponents/setting/SettingGroup.tssrc/client/components/baseComponents/setting/SettingTerritorySkins.tssrc/client/components/baseComponents/setting/SettingThemeMode.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/client/index.htmlsrc/client/styles.csssrc/client/styles/components/modal.csssrc/client/styles/components/setting.csssrc/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/game/UserSettings.tssrc/server/Master.tstests/util/TestServerConfig.tstsconfig.jsonwebpack.config.js
💤 Files with no reviewable changes (2)
- src/client/styles.css
- src/client/styles/components/modal.css
🚧 Files skipped from review as they are similar to previous changes (13)
- src/core/game/UserSettings.ts
- package.json
- src/client/styles/components/setting.css
- tsconfig.json
- eslint.config.js
- config.example.json
- tests/util/TestServerConfig.ts
- src/client/graphics/layers/PlayerInfoOverlay.ts
- src/core/configuration/Config.ts
- src/client/components/baseComponents/setting/SettingThemeMode.ts
- src/core/configuration/DefaultConfig.ts
- src/client/PublicLobby.ts
- .gitignore
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-08-28T15:14:40.664Z
Learnt from: mokizzz
Repo: openfrontio/OpenFrontIO PR: 1956
File: src/client/InputHandler.ts:430-449
Timestamp: 2025-08-28T15:14:40.664Z
Learning: On trackpads and touchscreens, users don't expect purely vertical two-finger swipes to trigger zoom. Pan detection using deltaX !== 0 is appropriate since any horizontal movement indicates pan intent, and zoom should be handled via pinch gestures instead.
Applied to files:
src/client/InputHandler.ts
📚 Learning: 2025-06-07T19:47:34.514Z
Learnt from: its-sii
Repo: openfrontio/OpenFrontIO PR: 1089
File: src/client/styles/components/setting.css:9-22
Timestamp: 2025-06-07T19:47:34.514Z
Learning: In src/client/styles/components/setting.css, the padding difference between .setting-item and .setting-item-group is intentional - .setting-item-group doesn't need padding for the specific grouped setting styling being implemented.
Applied to files:
src/client/components/baseComponents/setting/SettingGroup.ts
📚 Learning: 2025-11-12T23:11:34.445Z
Learnt from: MaxHT0x
Repo: openfrontio/OpenFrontIO PR: 2262
File: src/core/configuration/DefaultConfig.ts:806-832
Timestamp: 2025-11-12T23:11:34.445Z
Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.
Applied to files:
src/client/DevConfig.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-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/UserSettingModal.tssrc/client/Main.tssrc/client/components/baseComponents/Modal.ts
📚 Learning: 2025-07-26T05:35:03.686Z
Learnt from: aaa4xu
Repo: openfrontio/OpenFrontIO PR: 1581
File: webpack.config.js:139-141
Timestamp: 2025-07-26T05:35:03.686Z
Learning: In OpenFrontIO's webpack configuration, binary-loader processes files from resources/maps at build-time and inlines the binary payload directly into the compiled JS bundle. This means the original .bin assets are no longer needed at runtime and can be safely excluded from the final Docker image using webpack's globOptions.ignore pattern to reduce image size.
Applied to files:
webpack.config.js
📚 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/server/Master.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
🧬 Code graph analysis (4)
scripts/dev-stop.js (1)
webpack.config.js (2)
clientPort(18-18)serverPort(19-19)
src/client/components/baseComponents/setting/SettingColorPalette.ts (1)
src/client/components/baseComponents/Modal.ts (1)
customElement(5-214)
webpack.config.js (3)
src/core/configuration/DefaultConfig.ts (1)
gitCommit(153-155)src/core/configuration/DevConfig.ts (1)
gitCommit(45-47)scripts/dev-stop.js (1)
clientPort(6-6)
src/server/Master.ts (3)
src/core/game/GameView.ts (1)
config(665-667)src/core/game/GameImpl.ts (1)
config(336-338)src/client/PublicLobby.ts (1)
fetchLobbies(90-111)
🔇 Additional comments (29)
src/client/InputHandler.ts (2)
555-560: Clean implementation of Shift+drag reservation.The logic correctly prevents panning when Shift is held, reserving it for bounding box selection. This is consistent with the existing keyboard pan skip at lines 243-248.
191-191: Note: Default keybind changed from Space to Backslash.The toggleView keybind default changed from "Space" to "Backslash". Existing users with saved preferences won't be affected, but new users will experience the new default. Backslash may be less discoverable than Space but avoids potential conflicts with other Space-key interactions.
src/client/DarkModeButton.ts (1)
8-8: Clean update to the new theme API.The shift to
isDarkModeActive()aligns well with the centralized theme management inUserSettings. Both the initial state and the post-toggle state now use the same accessor, which keeps behavior consistent.Also applies to: 31-31
src/server/Master.ts (2)
96-114: Well-structured feature flag gating.The conditional scheduling based on
config.enablePublicGames()is clean. Logging when disabled helps with debugging.
142-149: Good PORT validation with clear error message.The validation covers all edge cases (NaN, negative, above 65535) and exits gracefully with a helpful error.
src/client/components/baseComponents/Modal.ts (3)
13-33: Solid theme observation pattern.The MutationObserver correctly watches for class changes on
documentElementand cleans up on disconnect. This keeps the modal in sync with global theme state without tight coupling.
118-169: CSS variables for theming are well organized.Defining theme tokens (
--modal-bg,--modal-text, etc.) in both light and dark variants allows child components to inherit styling without duplicating rules. Clean approach.
183-189: Readable class composition.The array-filter-join pattern for building class strings is cleaner than template string concatenation with conditionals.
src/client/components/baseComponents/setting/SettingGroup.ts (1)
1-153: Clean component structure overall.The collapsible group with localStorage persistence is well-implemented. CSS variables integrate nicely with the parent modal theming, and the toggle animation is smooth.
resources/lang/en.json (1)
365-384: Localization keys align with the new settings structure.The new group labels (
group_theme,group_skins, etc.) and display-related strings are well-organized. The keys follow existing naming conventions.src/client/Main.ts (3)
120-121: Good: DevConfig loads before other initialization.Awaiting
loadDevConfig()early ensures feature flags are available before Turnstile prefetch and other feature-gated logic runs.
348-349: Centralized theme application is cleaner.Delegating to
userSettings.applyTheme()instead of manual DOM manipulation keeps the theme logic in one place.
716-720: Clean feature guard for Turnstile.Returning
nullearly when cloudflare is disabled prevents unnecessary script loading and token retrieval in dev mode.src/client/components/baseComponents/setting/SettingTerritorySkins.ts (1)
4-9: Clean component structure with typed Skin interface.Using a simple interface for
Skininstead of a class hierarchy is the right call. The@state()decorators forselectedSkinIdandcustomSkinUrlkeep the component reactive.Also applies to: 87-93
webpack.config.js (3)
3-19: Good setup for configurable dev ports.The dotenv integration and port parsing look correct. Using
parseIntwith radix 10 and sensible defaults (9000/3000) is the right approach.
26-50: Content hashing for assets improves caching.Adding
[contenthash]to asset filenames is a good practice for cache busting. The consistent pattern across binary, text, images, and fonts helps with long-term caching.
188-266: Dynamic proxy targets are correctly calculated.The worker proxies correctly use
serverPort + 1,serverPort + 2, andserverPort + 3for the three workers. Both WebSocket and HTTP proxies are aligned.src/client/index.html (1)
149-204: Feature flag conditional loading pattern is correct.The checks follow a safe pattern: load scripts when no dev config exists (production) OR when the feature is not explicitly set to
false. This preserves production behavior while allowing dev overrides.Also applies to: 523-535
src/client/components/baseComponents/setting/SettingColorPalette.ts (3)
4-60: Clean interface and swatch definitions.The
ColorSwatchinterface is simple and typed. TheDEFAULT_SWATCHESarray provides good semantic defaults for territory colors.
432-447: Hex input validation is robust.The regex
/^#[0-9A-Fa-f]{6}$/correctly validates full hex colors before applying. Good handling of the#prefix normalization.
275-313: localStorage persistence and event dispatch are correct.The component correctly loads from
settings.colorPalette, falls back to defaults on parse error, and dispatches a bubblingchangeevent on save.src/client/DevConfig.ts (2)
124-165: Nested settings applied before flat settings.If both
settings.display.themeModeandsettings.themeModeexist in config.json, the flat value wins because it runs second. This is fine for backwards compatibility, but consider documenting this precedence.
185-214: Clean public API with async and sync accessors.The pattern of auto-loading at module init with both
waitForDevConfig()(async) andgetDevConfig()(sync with fallback) is practical. The deprecation alias forloadDevConfighelps migration.src/client/UserSettingModal.ts (6)
4-13: New imports for display settings components.The imports for
SettingColorPalette,SettingGroup,SettingTerritorySkins, andSettingThemeModeare correctly added. TheThemeModetype import fromUserSettingsis appropriate.
280-298: Theme mode change handler dispatches two events.The handler correctly updates settings and dispatches both
theme-mode-changedanddark-mode-changedevents. Dispatchingdark-mode-changedonwindowkeepsDarkModeButtonin sync.
300-330: Display tab renders theme, skins, and color palette.The three setting groups provide clear organization. The
disabledattribute on skins and color palette indicates these are placeholders for future cosmetics.
723-730: requestAnimationFrame reflow hack indicates timing issue.The double
requestUpdatesuggests a CSS layout race condition with the columns. This works but is fragile. Consider investigating root cause in the modal/group CSS if time permits.
332-491: Basic settings reorganized into logical groups.The grouping into Interface, Graphics, Controls, and Privacy sections improves discoverability. Using
setting-groupwithcolumnsattribute provides the 2-column masonry layout.
493-720: Keybind settings well-organized by function.The four groups (View Options, Build Controls, Attack Controls, Camera Controls) make keybinds easier to find. Each keybind has clear labels and descriptions.
| private getPreviewBackground(): string { | ||
| if (this.customSkinUrl) { | ||
| return `url(${this.customSkinUrl})`; | ||
| } | ||
| const skin = AVAILABLE_SKINS.find((s) => s.id === this.selectedSkinId); | ||
| return skin?.thumbnail ?? DEFAULT_PREVIEW; | ||
| } |
There was a problem hiding this comment.
Potential CSS injection via customSkinUrl.
Directly interpolating customSkinUrl into the url() CSS function without sanitization is risky. A crafted URL containing ) could break out of the url() and inject arbitrary CSS.
Suggested fix: sanitize or validate the URL
private getPreviewBackground(): string {
if (this.customSkinUrl) {
+ // Only allow data URLs or safe http(s) URLs
+ if (
+ this.customSkinUrl.startsWith("data:image/") ||
+ this.customSkinUrl.startsWith("https://") ||
+ this.customSkinUrl.startsWith("http://")
+ ) {
+ // Escape any characters that could break CSS url()
+ const safeUrl = this.customSkinUrl.replace(/[()'"\\]/g, "");
+ return `url(${safeUrl})`;
+ }
+ return DEFAULT_PREVIEW;
- return `url(${this.customSkinUrl})`;
}
const skin = AVAILABLE_SKINS.find((s) => s.id === this.selectedSkinId);
return skin?.thumbnail ?? DEFAULT_PREVIEW;
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/execution/utils/AiAttackBehavior.ts (1)
190-235: Keep these methods private since they are not used outside the class.These methods (
attackBots(),getBotAttackMaxParallelism(),maybeBetrayAndAttack(),isBorderingNukedTerritory(),findNearestIslandEnemy()) are only called internally withinAiAttackBehavior. Making them public without external usage creates an unintended public API surface.src/core/execution/NationExecution.ts (1)
405-436: Document or revert public visibility of sendBoatRandomly.This method is called only internally within NationExecution.ts (lines 387, 391). No external callers exist in the codebase. If this visibility change is intentional for future use, add a comment explaining why. Otherwise, return it to private.
♻️ Duplicate comments (5)
src/client/DevConfig.ts (1)
74-98: Add basic validation for loaded config structure.As noted in a previous review, the loaded JSON is used without validation. If
/config.jsonhas an unexpected shape, runtime errors could occur when accessingconfig.features.🔎 Proposed validation
if (response.ok) { const config = await response.json(); + // Basic structure check + if ( + !config || + typeof config !== "object" || + (config.features && typeof config.features !== "object") + ) { + console.warn("Invalid config.json structure, using defaults"); + cachedConfig = defaultConfig; + return defaultConfig; + } const mergedConfig: DevFeatureConfig = { features: { ...defaultConfig.features, ...config.features }, settings: config.settings, };src/client/index.html (1)
177-180: Trailing space in page_url value persists.Line 179 still has a trailing space in
"http://openfront.io ". A past review comment indicated this was addressed, but it remains.🔎 Proposed fix
googletag.cmd.push(function () { - googletag.pubads().set("page_url", "http://openfront.io "); + googletag.pubads().set("page_url", "http://openfront.io"); });src/client/components/baseComponents/setting/SettingTerritorySkins.ts (3)
285-294: Validate saved skin ID and custom URL from localStorage.Previous review flagged this: loading from localStorage without validation risks broken state if
settings.territorySkincontains an invalid ID, or ifsettings.territorySkinCustomcontains a malicious URL (e.g.,javascript:scheme). The suggested fix was to validate the skin exists inAVAILABLE_SKINSand only allow safe URL schemes.
317-323: Sanitize customSkinUrl before CSS interpolation.Previous review flagged potential CSS injection: directly interpolating
customSkinUrlintourl()without sanitization allows a crafted URL containing)to break out and inject arbitrary CSS.
325-344: Add file size validation for uploads.Previous review flagged missing file size limits: reading large images without bounds risks localStorage quota exceeded (typically 5-10MB per origin) and poor UX with no error feedback. Suggested limiting to 2MB with user-facing error message.
🧹 Nitpick comments (14)
src/core/execution/nation/NationMIRVBehavior.ts (3)
63-79: Potential unthrottled retry whenmaybeSendMIRVfails.If
maybeSendMIRV()cannot fire (e.g.,canBuildreturns false), no cooldown is triggered, butconsiderMIRV()still returnstrue. This means the target selection logic runs again next tick without any throttling. If the failure condition persists, this creates repeated per-tick overhead.Consider triggering cooldown in
maybeSendMIRVeven on failed builds, or returning whether a MIRV was actually sent.🔎 Option: trigger cooldown on failed build
private maybeSendMIRV(enemy: Player): void { if (this.player === null) throw new Error("not initialized"); this.emojiBehavior.maybeSendAttackEmoji(enemy); const centerTile = this.calculateTerritoryCenter(enemy); if (centerTile && this.player.canBuild(UnitType.MIRV, centerTile)) { this.sendMIRV(centerTile); this.emojiBehavior.sendEmoji(AllPlayers, EMOJI_NUKE); return; + } else { + // Trigger cooldown even on failed build to prevent per-tick retry overhead + this.triggerMIRVCooldown(); } }
178-178: Consider extracting cache TTL to a static constant.The local constant
MIRV_TARGETS_CACHE_TICKSis defined inline. For consistency with other static constants in this class, consider moving it to the class level. The comment// 2 secondshelps, but a named constant likeMIRV_TARGETS_CACHE_TICKS = 20at class level would be clearer.
241-243: Avoid creating full array just to get first element.
Array.from(this.player.tiles())[0]builds the entire tiles array just to grab the first tile. If the player has many tiles, this is wasteful. You can use the iterator directly:- const cooldownTile = - tile ?? Array.from(this.player.tiles())[0] ?? this.game.ref(0, 0); + const cooldownTile = + tile ?? this.player.tiles().next().value ?? this.game.ref(0, 0);Note: This assumes
tiles()returns an iterator. If it returns an iterable, usethis.player.tiles()[Symbol.iterator]().next().value.webpack.config.js (2)
28-28: Consider removing inline commit-artifact comments.Comments like
// Added content hashand// Changed from raw-loaderare artifacts from the development process. They don't add value to code readers since the configuration itself is self-documenting.🔎 Proposed cleanup
output: { publicPath: "/", - filename: "js/[name].[contenthash].js", // Added content hash + filename: "js/[name].[contenthash].js", path: path.resolve(__dirname, "static"), clean: isProduction, }, module: { rules: [ { test: /\.bin$/, - type: "asset/resource", // Changed from raw-loader + type: "asset/resource", generator: { - filename: "binary/[name].[contenthash][ext]", // Added content hash + filename: "binary/[name].[contenthash][ext]", }, }, { test: /\.md$/, - type: "asset/resource", // Changed from raw-loader + type: "asset/resource", generator: { - filename: "text/[name].[contenthash][ext]", // Added content hash + filename: "text/[name].[contenthash][ext]", }, }, { test: /\.(webp|png|jpe?g|gif)$/i, type: "asset/resource", generator: { - filename: "images/[name].[contenthash][ext]", // Added content hash + filename: "images/[name].[contenthash][ext]", }, }, { test: /\.svg$/, - type: "asset/resource", // Changed from asset/inline for caching + type: "asset/resource", generator: { - filename: "images/[name].[contenthash][ext]", // Added content hash + filename: "images/[name].[contenthash][ext]", }, }, { test: /\.(woff|woff2|eot|ttf|otf|xml)$/, - type: "asset/resource", // Changed from file-loader + type: "asset/resource", generator: { - filename: "fonts/[name].[contenthash][ext]", // Added content hash and fixed path + filename: "fonts/[name].[contenthash][ext]", }, },Also applies to: 36-39, 47-50, 81-81, 92-92, 99-99
28-28: Good caching improvements with contenthash, but remove commit artifact comments.The contenthash additions are sound for cache busting. The change from
raw-loadertoasset/resourcefor.binfiles is safe—code that requires inlining (likeBinaryLoaderGameMapLoader.ts) uses explicit!!binary-loader!inline loader syntax, which overrides the webpack config rule. The config rule serves as a flexible default for other cases.Remove inline comments like "// Added content hash" and "// Changed from raw-loader"—these are commit artifacts that clutter the code. Keep the config clean by removing these explanatory comments.
src/client/graphics/layers/SAMRadiusLayer.ts (3)
253-298: O(n²) complexity in circle overlap detection.The nested loop checks every circle against every other circle, resulting in O(n²) complexity. With many active SAM launchers, this could impact performance during redraws.
Consider spatial indexing (e.g., quadtree or grid-based bucketing) to reduce the number of overlap checks, especially if games can have 50+ SAM launchers simultaneously.
Since this only runs when
needsRedrawis true (not every frame), the current approach may be acceptable depending on typical SAM counts in production.
186-192: Extract hard-coded styling values.Colors, line widths, and dash patterns are hard-coded. Given the PR adds theme mode support (light/dark/system), these values should integrate with the theme system or at minimum be named constants.
private readonly STYLE_CONFIG = { outlineColor: "rgba(0, 0, 0, 1)", lineColorSelf: "rgba(0, 255, 0, 1)", lineColorEnemy: "rgba(255, 0, 0, 1)", lineColorFriend: "rgba(255, 255, 0, 1)", extraOutlineWidth: 1, lineWidth: 2, lineDash: [12, 6] as const, };Or better, integrate with
this.game.config().theme()if SAM radius colors should respect the theme.
213-251: Complex geometric logic could be extracted for testing.The interval merging (lines 213-251) and arc overlap calculations (lines 253-350) involve intricate geometric reasoning that's difficult to verify by inspection alone.
Consider extracting to a utility module:
// src/client/graphics/utils/CircleGeometry.ts export function mergeAngularIntervals( intervals: Array<[number, number]> ): Array<[number, number]> { // ... current logic } export function computeCircleOverlap( a: Circle, b: Circle ): { fullyCovered: boolean; arcSpan?: [number, number] } { // ... current logic }This would enable:
- Unit tests for edge cases (coincident circles, tangent circles, fully contained, etc.)
- Reuse in other rendering contexts
- Easier verification of correctness
Also applies to: 253-350
map-generator/README.md (1)
7-7: Use markdown link format for the URL.Static analysis flags this as a bare URL. Consider using proper markdown link format for consistency.
🔎 Proposed fix
-1. Install go https://go.dev/doc/install +1. Install go from the [official site](https://go.dev/doc/install)src/core/execution/utils/AiAttackBehavior.ts (1)
106-118: Redundant condition check on line 112.The condition
borderingEnemies.length === 0at line 112 is always true when reached, since lines 106-109 already return whenborderingEnemies.length > 0. You can simplify by removing this check.🔎 Proposed fix
// Attack the weakest player if (borderingEnemies.length > 0) { this.sendAttack(borderingEnemies[0]); return; } // If we don't have bordering enemies, attack someone on an island next to us - if (borderingEnemies.length === 0) { - const nearestIslandEnemy = this.findNearestIslandEnemy(); - if (nearestIslandEnemy) { - this.sendAttack(nearestIslandEnemy); - return; - } + const nearestIslandEnemy = this.findNearestIslandEnemy(); + if (nearestIslandEnemy) { + this.sendAttack(nearestIslandEnemy); } }src/client/components/baseComponents/setting/SettingThemeMode.ts (1)
102-111: Duplicated theme mode loading logic.The
loadThemeMode()method duplicates logic fromUserSettings.themeMode(). Consider using aUserSettingsinstance to read the value instead.🔎 Proposed approach
+import { UserSettings, ThemeMode } from "../../../../core/game/UserSettings"; + export class SettingThemeMode extends LitElement { + private userSettings = new UserSettings(); + private loadThemeMode() { - const stored = localStorage.getItem("settings.themeMode"); - if (stored === "light" || stored === "dark" || stored === "system") { - this.selectedMode = stored; - } else { - // Check legacy darkMode - const darkMode = localStorage.getItem("settings.darkMode"); - this.selectedMode = darkMode === "true" ? "dark" : "system"; - } + this.selectedMode = this.userSettings.themeMode(); }src/client/styles/components/setting.css (1)
399-411: Sticky tabs rely on magic negative margins.The
margin: -1.4rem -1.4rem 0.75rem -1.4remandwidth: calc(100% + 2.8rem)depend on the parent's padding being exactly1.4rem. If the modal content padding changes, this breaks. Consider documenting this coupling or using CSS custom properties for the padding value.src/client/components/baseComponents/setting/SettingTerritorySkins.ts (1)
296-308: Consider clearing custom URL when selecting a preset skin.When
selectSkin(id)is called,customSkinUrlis set tonull, butsaveSkin()only saves the custom URL if it's truthy. This means the old custom URL may persist in localStorage even after switching to a preset skin. Consider explicitly removing the key:Suggested fix
private saveSkin() { localStorage.setItem("settings.territorySkin", this.selectedSkinId); if (this.customSkinUrl) { localStorage.setItem("settings.territorySkinCustom", this.customSkinUrl); + } else { + localStorage.removeItem("settings.territorySkinCustom"); }src/client/UserSettingModal.ts (1)
723-730: Double requestUpdate may be unnecessary.The
open()method callsrequestUpdate(), then schedules anotherrequestUpdate()viarequestAnimationFrame. The second call might be needed for the column layout fix mentioned in the comment, but consider if this is a symptom of a deeper issue with Lit's update lifecycle.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
docs/user-settings--display-2.jpgis excluded by!**/*.jpgdocs/user-settings--display.jpgis excluded by!**/*.jpgdocs/user-settings--keybind.jpgis excluded by!**/*.jpgdocs/user-settings--settings.jpgis excluded by!**/*.jpgpackage-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (53)
.gitignoreconfig.example.jsoneslint.config.jsjest.config.tsmap-generator/README.mdmap-generator/go.modmap-generator/main.gomap-generator/map_generator.gopackage.jsonresources/lang/en.jsonscripts/dev-stop.jssrc/client/DarkModeButton.tssrc/client/DevConfig.tssrc/client/HelpModal.tssrc/client/HostLobbyModal.tssrc/client/InputHandler.tssrc/client/Main.tssrc/client/PublicLobby.tssrc/client/SinglePlayerModal.tssrc/client/UserSettingModal.tssrc/client/components/FluentSlider.tssrc/client/components/baseComponents/Modal.tssrc/client/components/baseComponents/setting/SettingColorPalette.tssrc/client/components/baseComponents/setting/SettingGroup.tssrc/client/components/baseComponents/setting/SettingTerritorySkins.tssrc/client/components/baseComponents/setting/SettingThemeMode.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/layers/SAMRadiusLayer.tssrc/client/index.htmlsrc/client/styles.csssrc/client/styles/components/modal.csssrc/client/styles/components/setting.csssrc/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/DevConfig.tssrc/core/configuration/PastelTheme.tssrc/core/configuration/PastelThemeDark.tssrc/core/execution/NationExecution.tssrc/core/execution/SAMLauncherExecution.tssrc/core/execution/nation/NationAllianceBehavior.tssrc/core/execution/nation/NationMIRVBehavior.tssrc/core/execution/utils/AiAttackBehavior.tssrc/core/game/GameMap.tssrc/core/game/GameView.tssrc/core/game/UserSettings.tssrc/server/MapPlaylist.tssrc/server/Master.tstests/client/components/FluentSlider.test.tstests/util/TestConfig.tstests/util/TestServerConfig.tstsconfig.jsonwebpack.config.js
💤 Files with no reviewable changes (9)
- src/core/game/GameMap.ts
- src/client/styles.css
- src/core/configuration/PastelTheme.ts
- src/server/MapPlaylist.ts
- src/core/configuration/PastelThemeDark.ts
- src/client/components/FluentSlider.ts
- tests/client/components/FluentSlider.test.ts
- src/client/styles/components/modal.css
- map-generator/main.go
🚧 Files skipped from review as they are similar to previous changes (13)
- src/client/DarkModeButton.ts
- src/client/InputHandler.ts
- src/client/graphics/layers/PlayerInfoOverlay.ts
- src/core/configuration/Config.ts
- .gitignore
- tests/util/TestServerConfig.ts
- eslint.config.js
- src/core/configuration/DefaultConfig.ts
- config.example.json
- scripts/dev-stop.js
- src/client/Main.ts
- src/server/Master.ts
- src/client/PublicLobby.ts
🧰 Additional context used
🧠 Learnings (40)
📚 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/core/configuration/DevConfig.tssrc/core/execution/nation/NationAllianceBehavior.tstests/util/TestConfig.tssrc/core/execution/SAMLauncherExecution.tssrc/core/execution/nation/NationMIRVBehavior.tssrc/core/execution/NationExecution.tssrc/core/execution/utils/AiAttackBehavior.ts
📚 Learning: 2025-11-12T23:11:34.445Z
Learnt from: MaxHT0x
Repo: openfrontio/OpenFrontIO PR: 2262
File: src/core/configuration/DefaultConfig.ts:806-832
Timestamp: 2025-11-12T23:11:34.445Z
Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.
Applied to files:
src/core/configuration/DevConfig.ts
📚 Learning: 2025-07-31T06:39:29.144Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1536
File: src/core/execution/utils/BotBehavior.ts:41-54
Timestamp: 2025-07-31T06:39:29.144Z
Learning: In the OpenFrontIO codebase, the `PseudoRandom.chance(odds: number)` method takes inverse odds as a parameter, not a percentage. The probability is calculated as `1.0 / odds`. So `chance(1.5)` gives 66.67% probability, `chance(2)` gives 50% probability, etc. The implementation returns `Math.floor(this._nextFloat() * odds) === 0`.
Applied to files:
src/core/configuration/DevConfig.tstests/util/TestConfig.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/configuration/DevConfig.tssrc/core/execution/nation/NationAllianceBehavior.tssrc/core/execution/SAMLauncherExecution.tssrc/core/execution/nation/NationMIRVBehavior.tssrc/core/execution/NationExecution.tssrc/core/execution/utils/AiAttackBehavior.ts
📚 Learning: 2025-06-07T19:47:34.514Z
Learnt from: its-sii
Repo: openfrontio/OpenFrontIO PR: 1089
File: src/client/styles/components/setting.css:9-22
Timestamp: 2025-06-07T19:47:34.514Z
Learning: In src/client/styles/components/setting.css, the padding difference between .setting-item and .setting-item-group is intentional - .setting-item-group doesn't need padding for the specific grouped setting styling being implemented.
Applied to files:
src/client/styles/components/setting.csssrc/client/components/baseComponents/setting/SettingGroup.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:
map-generator/README.mdsrc/core/execution/nation/NationAllianceBehavior.tssrc/client/HostLobbyModal.tssrc/core/execution/nation/NationMIRVBehavior.tssrc/core/execution/NationExecution.ts
📚 Learning: 2025-08-10T14:37:35.777Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 1770
File: resources/maps/world/manifest.json:279-355
Timestamp: 2025-08-10T14:37:35.777Z
Learning: The OpenFrontIO World Map includes both real-world nations and fictional/creative entities like "Polar Bears" as playable nations. These special nations may use non-two-letter flag codes (e.g., "polar_bears" instead of a two-letter code), and this is intentional design. The corresponding flag assets are stored in resources/flags/ with matching filenames.
Applied to files:
map-generator/README.md
📚 Learning: 2025-05-30T18:11:56.014Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 959
File: resources/maps/GatewayToTheAtlantic.json:124-124
Timestamp: 2025-05-30T18:11:56.014Z
Learning: In OpenFrontIO, flag names in map JSON files can use spaces (like "Amazigh flag") and don't need to follow slug format. Flag assets are stored with matching filenames including spaces (e.g., "Amazigh flag.svg"). Nation names and flag names are not translated/localized in the translation files.
Applied to files:
map-generator/README.md
📚 Learning: 2025-05-30T16:16:27.118Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 957
File: resources/maps/GatewayToTheAtlantic.json:175-179
Timestamp: 2025-05-30T16:16:27.118Z
Learning: In OpenFrontIO project, flag values in map JSON files (like GatewayToTheAtlantic.json) should match the actual flag file names in the resources directory (e.g., "French foreign legion" corresponds to "French foreign legion.svg"). The naming convention follows the file naming system rather than code naming conventions.
Applied to files:
map-generator/README.md
📚 Learning: 2025-08-19T11:00:55.422Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 1864
File: resources/maps/arabianpeninsula/manifest.json:13-170
Timestamp: 2025-08-19T11:00:55.422Z
Learning: In OpenFrontIO, nation names in map manifests are displayed directly in the UI without translation. They do not need to be added to resources/lang/en.json or processed through translateText(). This is the established pattern across all existing maps including Europe, World, Asia, Africa, and others.
Applied to files:
map-generator/README.md
📚 Learning: 2025-08-09T05:14:19.147Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1761
File: src/client/LocalPersistantStats.ts:24-31
Timestamp: 2025-08-09T05:14:19.147Z
Learning: Files in the src/client directory contain browser-only code in the OpenFrontIO project, so browser APIs like localStorage are guaranteed to be available and don't need undefined guards.
Applied to files:
src/client/index.html
📚 Learning: 2025-07-26T05:35:03.686Z
Learnt from: aaa4xu
Repo: openfrontio/OpenFrontIO PR: 1581
File: webpack.config.js:139-141
Timestamp: 2025-07-26T05:35:03.686Z
Learning: In OpenFrontIO's webpack configuration, binary-loader processes files from resources/maps at build-time and inlines the binary payload directly into the compiled JS bundle. This means the original .bin assets are no longer needed at runtime and can be safely excluded from the final Docker image using webpack's globOptions.ignore pattern to reduce image size.
Applied to files:
webpack.config.js
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.
Applied to files:
tests/util/TestConfig.tssrc/client/graphics/layers/SAMRadiusLayer.tssrc/core/execution/SAMLauncherExecution.tssrc/core/execution/nation/NationMIRVBehavior.tssrc/core/execution/NationExecution.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/SAMRadiusLayer.tssrc/client/graphics/GameRenderer.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/SinglePlayerModal.tssrc/client/UserSettingModal.tssrc/client/HostLobbyModal.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/client/SinglePlayerModal.tssrc/core/game/GameView.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/core/execution/SAMLauncherExecution.tssrc/core/execution/nation/NationMIRVBehavior.tssrc/core/execution/NationExecution.tssrc/core/execution/utils/AiAttackBehavior.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/core/execution/SAMLauncherExecution.tssrc/core/execution/nation/NationMIRVBehavior.tssrc/core/execution/NationExecution.tssrc/core/execution/utils/AiAttackBehavior.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from specific UnitType enum values (AtomBomb, HydrogenBomb, MIRV, MIRVWarhead). This means that comparisons in switch statements between a NukeType parameter and UnitType enum values are valid and will work correctly at runtime.
Applied to files:
src/core/execution/SAMLauncherExecution.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/core/execution/SAMLauncherExecution.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/core/game/UserSettings.tssrc/client/UserSettingModal.ts
📚 Learning: 2025-10-17T19:16:57.699Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:711-723
Timestamp: 2025-10-17T19:16:57.699Z
Learning: In the OpenFrontIO game, MIRVs (Multiple Independently targetable Reentry Vehicles) have an area of effect with a radius of 1500 tiles around the aim point. To maximize efficiency, bots should aim at the center of the target player's territory rather than selecting random tiles. This differs from the initial understanding that MIRVs covered an entire player's territory regardless of aim point.
Applied to files:
src/core/execution/nation/NationMIRVBehavior.ts
📚 Learning: 2025-12-26T22:21:15.466Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2689
File: src/client/PublicLobby.ts:245-245
Timestamp: 2025-12-26T22:21:15.466Z
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/core/execution/nation/NationMIRVBehavior.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/core/execution/nation/NationMIRVBehavior.tssrc/core/game/GameView.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/core/execution/nation/NationMIRVBehavior.tssrc/core/execution/NationExecution.tssrc/core/game/GameView.tssrc/core/execution/utils/AiAttackBehavior.ts
📚 Learning: 2025-05-19T06:00:38.007Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.
Applied to files:
src/core/execution/NationExecution.ts
📚 Learning: 2025-08-23T07:48:19.060Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.
Applied to files:
src/core/execution/NationExecution.tssrc/core/execution/utils/AiAttackBehavior.ts
📚 Learning: 2025-06-10T09:56:44.473Z
Learnt from: Ble4Ch
Repo: openfrontio/OpenFrontIO PR: 1063
File: src/core/configuration/PastelThemeDark.ts:53-53
Timestamp: 2025-06-10T09:56:44.473Z
Learning: In ColorAllocator class in src/core/configuration/Colors.ts, the correct method names are assignColor(id: string): Colord for general color assignment and assignTeamColor(team: Team): Colord for team-specific colors. There are no assignPlayerColor() or assignBotColor() methods.
Applied to files:
src/core/game/GameView.ts
📚 Learning: 2025-06-06T15:36:31.739Z
Learnt from: Ble4Ch
Repo: openfrontio/OpenFrontIO PR: 1063
File: src/core/configuration/Colors.ts:114-330
Timestamp: 2025-06-06T15:36:31.739Z
Learning: In the OpenFrontIO project, color improvements are implemented incrementally. The current focus for player colors is ensuring sufficient unique color availability rather than optimizing for visual distinction or accessibility features.
Applied to files:
src/core/game/GameView.ts
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.
Applied to files:
src/core/game/GameView.ts
📚 Learning: 2025-06-06T15:42:21.903Z
Learnt from: Ble4Ch
Repo: openfrontio/OpenFrontIO PR: 1063
File: src/core/configuration/PastelThemeDark.ts:20-21
Timestamp: 2025-06-06T15:42:21.903Z
Learning: In ColorAllocator class, all methods (assignBotColor, assignPlayerColor, assignTeamColor) use the constructor colors from this.availableColors. Only the predefined team cases in assignTeamColor use hardcoded global colors, while the default case uses constructor colors.
Applied to files:
src/core/game/GameView.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/core/execution/utils/AiAttackBehavior.ts
📚 Learning: 2025-11-01T00:24:33.860Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2161
File: src/core/execution/FakeHumanExecution.ts:670-678
Timestamp: 2025-11-01T00:24:33.860Z
Learning: In OpenFrontIO, PlayerType.Bot entities cannot be in teams and do not have friendliness relationships. Only PlayerType.Human and PlayerType.FakeHuman can participate in teams and alliances. Therefore, when targeting bot-owned tiles, friendliness checks like `owner.isFriendly(this.player)` are unnecessary and meaningless for bots.
Applied to files:
src/core/execution/utils/AiAttackBehavior.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/core/execution/utils/AiAttackBehavior.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/core/execution/utils/AiAttackBehavior.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-07-12T08:41:35.101Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1357
File: resources/lang/de.json:523-540
Timestamp: 2025-07-12T08:41:35.101Z
Learning: In OpenFrontIO project localization files, always check the en.json source file before flagging potential spelling errors in other language files, as some keys may intentionally use non-standard spellings that need to be consistent across all translations.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-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.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:42:02.109Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1357
File: resources/lang/zh_cn.json:527-539
Timestamp: 2025-07-12T08:42:02.109Z
Learning: In OpenFrontIO project, the territory pattern key "embelem" is the correct spelling used in all language files, not "emblem". This is the technical identifier that appears in en.json and must be consistent across all localization files.
Applied to files:
resources/lang/en.json
🧬 Code graph analysis (11)
src/core/execution/nation/NationAllianceBehavior.ts (2)
src/core/Util.ts (1)
assertNever(257-259)src/core/game/Game.ts (1)
Player(534-672)
src/client/components/baseComponents/setting/SettingThemeMode.ts (1)
src/core/game/UserSettings.ts (2)
ThemeMode(6-6)darkMode(66-68)
src/client/graphics/GameRenderer.ts (1)
src/client/graphics/layers/SAMRadiusLayer.ts (1)
SAMRadiusLayer(13-351)
src/client/components/baseComponents/setting/SettingColorPalette.ts (2)
src/client/UserSettingModal.ts (1)
customElement(15-735)src/client/components/baseComponents/setting/SettingGroup.ts (1)
customElement(6-153)
src/client/SinglePlayerModal.ts (2)
src/client/LangSelector.ts (1)
translateText(258-278)src/client/Utils.ts (1)
translateText(92-147)
src/client/HelpModal.ts (2)
src/client/LangSelector.ts (1)
translateText(258-278)src/client/Utils.ts (3)
translateText(92-147)getModifierKey(204-211)getAltKey(213-220)
src/core/execution/SAMLauncherExecution.ts (3)
src/core/game/GameView.ts (2)
type(94-96)type(353-355)src/core/game/UnitImpl.ts (1)
type(148-150)src/core/game/PlayerImpl.ts (1)
type(205-207)
src/client/HostLobbyModal.ts (2)
src/client/LangSelector.ts (1)
translateText(258-278)src/client/Utils.ts (1)
translateText(92-147)
src/core/execution/NationExecution.ts (1)
src/core/execution/SpawnExecution.ts (2)
SpawnExecution(10-123)randTile(117-122)
src/core/game/GameView.ts (4)
src/core/game/UnitImpl.ts (1)
tile(178-180)src/core/game/TrainStation.ts (1)
tile(141-143)src/core/game/GameMap.ts (2)
x(127-129)y(131-133)src/core/game/GameImpl.ts (2)
x(842-844)y(845-847)
src/core/execution/utils/AiAttackBehavior.ts (2)
src/core/game/Game.ts (2)
Player(534-672)TerraNullius(521-526)src/core/configuration/DefaultConfig.ts (1)
maxTroops(812-843)
🪛 Gitleaks (8.30.0)
src/client/index.html
[high] 540-540: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 markdownlint-cli2 (0.18.1)
map-generator/README.md
7-7: Bare URL used
(MD034, no-bare-urls)
| private samRanges: SAMRadius[] = []; | ||
| private showStroke: boolean = false; | ||
| private dashOffset = 0; | ||
| private rotationSpeed = 14; // px per second |
There was a problem hiding this comment.
Misleading comment and delayed dashOffset wrapping.
-
Line 23: The comment "px per second" is incorrect. SAM radii are in game units, not pixels. The rotation speed affects dash offset, not physical pixels.
-
Lines 124-126:
dashOffsetaccumulates until it exceeds 1e6 before wrapping. This is arbitrary and could cause precision issues with very large numbers.
🔎 Proposed fixes
- private rotationSpeed = 14; // px per second
+ private rotationSpeed = 14; // units per second
if (this.showStroke) {
this.dashOffset += (this.rotationSpeed * dt) / 1000;
- if (this.dashOffset > 1e6) this.dashOffset = this.dashOffset % 1000000;
+ // Wrap at a smaller value to avoid precision issues
+ if (this.dashOffset > 10000) this.dashOffset = this.dashOffset % 10000;
this.needsRedraw = true;
}Also applies to: 124-126
🤖 Prompt for AI Agents
In src/client/graphics/layers/SAMRadiusLayer.ts around line 23 and lines
124-126, update the misleading comment on rotationSpeed to indicate it is in
"game units per second" (or "SAM units per second") rather than "px per second",
and optionally rename the variable to reflect units if desired; for dashOffset
wrapping replace the arbitrary "if > 1e6 then reset" logic with a proper
modulo-based wrap using the dash pattern cycle (e.g., dashOffset = dashOffset %
dashPatternLength or % 1 if using normalized offsets) so the offset never grows
unbounded and stays within the pattern period to avoid precision issues.
| ) { | ||
| this.canvas = document.createElement("canvas"); | ||
| const ctx = this.canvas.getContext("2d"); | ||
| if (!ctx) { | ||
| throw new Error("2d context not supported"); | ||
| } | ||
| this.context = ctx; | ||
| this.canvas.width = this.game.width(); | ||
| this.canvas.height = this.game.height(); | ||
| } |
There was a problem hiding this comment.
Canvas resize handling is missing.
The off-screen canvas dimensions are set once in the constructor using game.width() and game.height(). If the game resizes (e.g., window resize or canvas size change), this canvas won't update, leading to mismatched rendering.
🔎 Suggested fix: Add resize handling
Consider adding a method to handle canvas resizing:
private resizeCanvas() {
const width = this.game.width();
const height = this.game.height();
if (this.canvas.width !== width || this.canvas.height !== height) {
this.canvas.width = width;
this.canvas.height = height;
this.needsRedraw = true;
}
}Then call it in tick() or add a resize event listener in init().
🤖 Prompt for AI Agents
In src/client/graphics/layers/SAMRadiusLayer.ts around lines 37 to 46, the
off-screen canvas is sized only in the constructor so it will become out-of-sync
when the game resizes; add a private resizeCanvas() method that reads
game.width() and game.height(), compares to this.canvas.width/height, and if
different updates the canvas dimensions and sets this.needsRedraw = true; then
call resizeCanvas() at the start of tick() (or register a resize listener in
init()) so the off-screen canvas always matches the current game size.
| this.samLaunchers.clear(); | ||
| samLaunchers.forEach((sam) => | ||
| this.samLaunchers.set(sam.id(), { | ||
| ownerId: sam.owner().smallID(), | ||
| level: sam.level(), | ||
| }), | ||
| this.samLaunchers.set(sam.id(), sam.owner().smallID()), | ||
| ); |
There was a problem hiding this comment.
Inconsistent data tracking between tick() and redraw().
The tick() method carefully maintains incremental updates to samLaunchers (lines 76-108), but redraw() clears and rebuilds the entire map from scratch. This makes the incremental update logic redundant.
Either:
- Remove the incremental update logic in
tick()and always rebuild inredraw(), or - Don't clear
samLaunchersinredraw()and rely on the incremental updates
The current approach wastes computation tracking changes that get discarded.
🤖 Prompt for AI Agents
In src/client/graphics/layers/SAMRadiusLayer.ts around lines 156-159, redraw()
currently clears and rebuilds this.samLaunchers which discards the incremental
updates performed in tick(); to fix, stop clearing this.samLaunchers in redraw()
and instead iterate over the existing map when painting so redraw uses the
incrementally-maintained state from tick(); ensure tick() continues to remove
entries for destroyed/removed launchers (lines ~76-108) so redraw renders the
up-to-date set and avoid rebuilding the map inside redraw().
| ctx.save(); | ||
| ctx.beginPath(); | ||
| for (const c of circles) { | ||
| ctx.moveTo(c.x + c.r, c.y); | ||
| ctx.arc(c.x, c.y, c.r, 0, Math.PI * 2); | ||
| } | ||
| ctx.restore(); |
There was a problem hiding this comment.
Fill operation missing - path created but never filled.
The code creates a path for the union of circles and the comment says "Fill union," but ctx.fill() is never called. The path is built but not rendered.
🔎 Proposed fix
ctx.save();
ctx.beginPath();
for (const c of circles) {
ctx.moveTo(c.x + c.r, c.y);
ctx.arc(c.x, c.y, c.r, 0, Math.PI * 2);
}
+ ctx.fillStyle = "rgba(0, 0, 0, 0.1)"; // or appropriate fill color
+ ctx.fill();
ctx.restore();If filling was intentionally removed, delete the path-building code and update the comment.
📝 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.
| ctx.save(); | |
| ctx.beginPath(); | |
| for (const c of circles) { | |
| ctx.moveTo(c.x + c.r, c.y); | |
| ctx.arc(c.x, c.y, c.r, 0, Math.PI * 2); | |
| } | |
| ctx.restore(); | |
| ctx.save(); | |
| ctx.beginPath(); | |
| for (const c of circles) { | |
| ctx.moveTo(c.x + c.r, c.y); | |
| ctx.arc(c.x, c.y, c.r, 0, Math.PI * 2); | |
| } | |
| ctx.fillStyle = "rgba(0, 0, 0, 0.1)"; // or appropriate fill color | |
| ctx.fill(); | |
| ctx.restore(); |
🤖 Prompt for AI Agents
In src/client/graphics/layers/SAMRadiusLayer.ts around lines 195 to 201 the code
builds a path for the union of circles but never fills it: add a ctx.fill() (and
optionally set ctx.fillStyle before) after the path construction and before
ctx.restore() so the union is rendered; if the fill was intentionally removed,
instead remove the path-building loop and update the comment to reflect that no
fill is performed.
| for (let i = 1; i < flat.length; i++) { | ||
| const it = flat[i]; | ||
| if (it[0] <= cur[1] + EPS) { | ||
| if (it[0] <= cur[1] + 1e-9) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Numerical tolerance values need review.
Multiple epsilon comparisons use 1e-9 (lines 242, 270, 275, 279, 311, 316) and 1e-3 (line 322) without clear rationale. If game coordinates are large integers (e.g., on 10000×10000 maps), these tolerances might be inappropriate.
- Define named constants for tolerances:
private readonly EPSILON = 1e-6; // or appropriate value based on game scale
private readonly MIN_ARC_LENGTH = 0.01; // radians- Base epsilon on game coordinate scale:
private readonly EPSILON = Math.max(1e-9, this.game.width() * 1e-9);- Document why different tolerances are used in different contexts.
Also applies to: 270-286, 311-322
🤖 Prompt for AI Agents
In src/client/graphics/layers/SAMRadiusLayer.ts around lines 242-322, multiple
raw numeric epsilons (1e-9, 1e-3) are used for floating-point comparisons;
replace these literals with clear, named constants (e.g., EPSILON,
MIN_ARC_LENGTH) declared on the class, initialize EPSILON based on game scale
(for example Math.max(1e-9, this.game.width() * 1e-9) or another
scale-appropriate formula) and set MIN_ARC_LENGTH to an explicit radian
threshold (e.g., 0.01), then use these constants in all comparisons at the
listed lines and add short inline comments explaining the rationale for each
tolerance choice (distance-scale epsilon vs. angular minimum).
| if (this.random.chance(10)) { | ||
| // this.isTraitor = true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove empty conditional block.
This if block with a commented-out assignment is dead code. Either implement the traitor logic or remove the block entirely.
Suggested fix
- if (this.random.chance(10)) {
- // this.isTraitor = true
- }📝 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.
| if (this.random.chance(10)) { | |
| // this.isTraitor = true | |
| } |
🤖 Prompt for AI Agents
In src/core/execution/NationExecution.ts around lines 68-70, there is an empty
conditional with a commented-out assignment (if (this.random.chance(10)) { //
this.isTraitor = true }) which is dead code; either remove the entire if block
or implement the intended traitor behavior by uncommenting and wiring up
this.isTraitor assignment and any necessary side-effects (state initialization,
persistence, and tests) so the condition has functional effect—pick one: delete
the block to clean up or restore and fully implement the traitor logic and its
downstream handling.
| const clientPort = parseInt(process.env.OPENFRONT_CLIENT_PORT ?? "9000", 10); | ||
| const serverPort = parseInt(process.env.OPENFRONT_SERVER_PORT ?? "3000", 10); |
There was a problem hiding this comment.
Validate parsed ports to prevent NaN and invalid values.
If OPENFRONT_CLIENT_PORT or OPENFRONT_SERVER_PORT contain non-numeric values, parseInt returns NaN. This creates invalid URLs like http://localhost:NaN that fail silently or produce cryptic errors. Additionally, ports should be validated for:
- Valid range (0–65535)
- No conflicts (clientPort !== serverPort)
🔎 Proposed validation helper
+function parsePort(envVar: string | undefined, defaultPort: string, name: string): number {
+ const port = parseInt(envVar ?? defaultPort, 10);
+ if (isNaN(port)) {
+ throw new Error(`${name} must be a valid number, got: ${envVar}`);
+ }
+ if (port < 0 || port > 65535) {
+ throw new Error(`${name} must be between 0 and 65535, got: ${port}`);
+ }
+ return port;
+}
+
-const clientPort = parseInt(process.env.OPENFRONT_CLIENT_PORT ?? "9000", 10);
-const serverPort = parseInt(process.env.OPENFRONT_SERVER_PORT ?? "3000", 10);
+const clientPort = parsePort(process.env.OPENFRONT_CLIENT_PORT, "9000", "OPENFRONT_CLIENT_PORT");
+const serverPort = parsePort(process.env.OPENFRONT_SERVER_PORT, "3000", "OPENFRONT_SERVER_PORT");
+
+if (clientPort === serverPort) {
+ throw new Error(`OPENFRONT_CLIENT_PORT and OPENFRONT_SERVER_PORT cannot be the same (both are ${clientPort})`);
+}🤖 Prompt for AI Agents
In webpack.config.js around lines 18-19 the parsed clientPort and serverPort may
be NaN or out of range if OPENFRONT_* env vars are non-numeric; validate the
results of parseInt, ensure they are finite integers in the 0–65535 range, and
ensure clientPort !== serverPort; if validation fails either fall back to safe
defaults (9000/3000) and log a clear error or throw/exit during startup so
invalid ports don't produce URLs like http://localhost:NaN — implement a small
validator that checks Number.isInteger(port) && port >= 0 && port <= 65535,
compares the two ports for conflict, and handles failures deterministically (log
+ default or throw).
- Fix dev-stop.js Unix branch to only record stopped ports when processes actually exist (check lsof output before killing) - Fix dev-stop.js log to show only actually stopped ports, not all configured ports - Add config structure validation in DevConfig.ts loadConfig() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The project CI uses npm, not pnpm. Reverting the lock file changes to maintain CI compatibility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/client/DevConfig.ts (2)
104-106: Silent catch hides JSON parse errors.The empty catch handles "file not found" gracefully, but also silently swallows JSON parse errors if
config.jsonexists but contains invalid JSON. A dev might spend time debugging why their config is ignored.🔎 Proposed fix to log parse errors
- } catch { - // config.json not found, use defaults + } catch (error) { + // Log only if it's likely a parse error (not a 404) + if (error instanceof SyntaxError) { + console.warn("Failed to parse config.json:", error.message); + } }
118-134: Helper functions are nearly identical.
applyBoolandapplyFloatdo the same thing (call.toString()). You could use one function for both, or keep them separate for clarity — your call.🔎 Proposed consolidation
- const applyBool = (key: string, value: boolean | undefined) => { - if (value !== undefined && localStorage.getItem(key) === null) { - localStorage.setItem(key, value.toString()); - } - }; - - const applyFloat = (key: string, value: number | undefined) => { - if (value !== undefined && localStorage.getItem(key) === null) { - localStorage.setItem(key, value.toString()); - } - }; - - const applyString = (key: string, value: string | undefined) => { - if (value !== undefined && localStorage.getItem(key) === null) { - localStorage.setItem(key, value); - } - }; + const applySetting = (key: string, value: string | number | boolean | undefined) => { + if (value !== undefined && localStorage.getItem(key) === null) { + localStorage.setItem(key, String(value)); + } + };src/core/game/UserSettings.ts (1)
87-104: Consider listening for system theme changes.The
applyTheme()method correctly applies the theme class based on mode. However, whenmode === "system", the app checksprefers-color-schemeonce but doesn't listen for changes. If a user switches their OS theme while the app is open, the app theme won't update automatically.💡 Optional: Add listener for system preference changes
You could add a media query listener to react to OS theme changes:
applyTheme(): void { const mode = this.themeMode(); if (mode === "dark") { document.documentElement.classList.add("dark"); } else if (mode === "light") { document.documentElement.classList.remove("dark"); } else { // System mode - check prefers-color-scheme - const prefersDark = window.matchMedia( - "(prefers-color-scheme: dark)", - ).matches; - if (prefersDark) { - document.documentElement.classList.add("dark"); - } else { - document.documentElement.classList.remove("dark"); - } + const mediaQuery = window.matchMedia("(prefers-color-scheme: dark)"); + const applySystemTheme = () => { + if (mediaQuery.matches) { + document.documentElement.classList.add("dark"); + } else { + document.documentElement.classList.remove("dark"); + } + }; + applySystemTheme(); + mediaQuery.addEventListener("change", applySystemTheme); } }Note: You'd need to track and remove the listener when mode changes to avoid memory leaks.
src/client/UserSettingModal.ts (1)
704-711: Layout workaround is acceptable but could be improved.The
requestAnimationFramecall fixes column layout issues after the modal opens, which is a valid workaround. However, this typically indicates either a CSS issue (columns layout calculating before content is rendered) or a Lit lifecycle timing concern.If you encounter flickering or layout jumps, consider investigating:
- Whether the modal content is fully rendered before
open()completes- If CSS grid/flexbox needs explicit dimensions before the modal is shown
- If the column layout calculation needs the modal's actual dimensions
For now, this workaround is fine, but keep an eye on UX for any visible reflow.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignorescripts/dev-stop.jssrc/client/DevConfig.tssrc/client/Main.tssrc/client/UserSettingModal.tssrc/core/game/UserSettings.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/Main.ts
- scripts/dev-stop.js
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-12T23:11:34.445Z
Learnt from: MaxHT0x
Repo: openfrontio/OpenFrontIO PR: 2262
File: src/core/configuration/DefaultConfig.ts:806-832
Timestamp: 2025-11-12T23:11:34.445Z
Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.
Applied to files:
src/client/DevConfig.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.tssrc/core/game/UserSettings.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/UserSettingModal.ts
🧬 Code graph analysis (1)
src/client/UserSettingModal.ts (2)
src/client/DarkModeButton.ts (1)
customElement(5-45)src/core/game/UserSettings.ts (2)
UserSettings(8-260)ThemeMode(6-6)
🔇 Additional comments (11)
src/client/DevConfig.ts (3)
4-57: Clean interface design with typed unions.Good use of typed union for
themeModeinstead of a plain string. The nested structure with optional groups keeps things organized, and the flat legacy fields provide nice backwards compatibility.
136-195: Nested-first, flat-fallback order is correct.The logic correctly applies nested settings first, so they take precedence over flat legacy settings when both are provided. Good backwards compatibility design.
207-221: Callers properly await config before using feature flags in most cases, but PublicLobby.ts render() method may access defaults.Main.ts correctly
await waitForDevConfig()at line 124 before any feature flag checks. However, PublicLobby.ts usesisDevFeatureEnabled()directly in therender()method (line 123) without guaranteed prior loading—it only awaits inconnectedCallback()(line 40). Sincerender()can be called beforeconnectedCallback()completes, the render method may return defaults if config hasn't loaded. IfpublicLobbies: falseis the intended safe default, this is acceptable; otherwise, consider ensuring config loads before render can be called..gitignore (1)
13-21: LGTM! Clean organization for local dev configs.Adding
config.jsonandresources/config.jsonto.gitignorecorrectly prevents local configuration overrides from being committed while allowing theconfig.example.jsontemplate to remain in version control. The reorganization of the Claude Code section is also clean.src/core/game/UserSettings.ts (3)
6-6: LGTM! Clean typed union for theme modes.The
ThemeModetype uses a string literal union (preferred over enums or class hierarchies), and thethemeMode()method includes proper validation and legacy migration logic. Defaulting to"system"when no preference exists is a sensible choice.Also applies to: 70-80
106-111: LGTM! Clear boolean helper.The
isDarkModeActive()method correctly evaluates the current theme state, including system preference resolution. Clean implementation.
176-188: LGTM! Toggle logic correctly bypasses system mode.The updated
toggleDarkMode()correctly switches between"light"and"dark"modes (bypassing"system"), which makes sense for a toggle button UX. Event dispatching synchronizes both the new theme-mode-changed and legacy dark-mode-changed events.src/client/UserSettingModal.ts (4)
4-19: LGTM! Clean imports and state extension.The new
ThemeModeimport and setting components are properly integrated, and thesettingsModestate correctly extends to include"display"as a typed union member.
261-279: LGTM! Dual event dispatch correctly synchronizes theme state.The
handleThemeModeChangemethod correctly updates settings viasetThemeMode()and dispatches events to both the component tree (this.dispatchEvent) and the window (window.dispatchEvent) to synchronize with different listeners likeDarkModeButton.
281-311: LGTM! Display tab structure with placeholders as intended.The
renderDisplaySettingsmethod introduces a clean grouped layout for the Display tab. The theme section is functional, while skins and colors are correctly marked asdisabledplaceholders (matching the PR's description of "territory skins placeholders").
217-259: LGTM! Clean tab integration with widened modal.The Display tab is properly integrated into the tab navigation, and the
user-setting-modal--wideclass supports the expanded content layout. The conditional rendering logic correctly routes to the newrenderDisplaySettings()method.
|
@nodeGarden It seems Claude has made changes to files you weren't attempting to change. Many files that you didn't touch in your original PR now have changes suddenly #2685. Like go.mod and pasteltheme.ts, where all kinds of comments were deleted for no reason. AI acting as Agent tends to do this kind of stuff and needs heavy guarding. It seems the AI did a "those changes shouldn't be here because they weren't here before, i'll convienently delete the changes"... Also Evan closed the previous PR requesting it to be under 300 lines. This one is in the thousands. In your previous PR Description you said "Sorry these aren't separated, but I went too far down the path, and separating became messy. ". But i'm afraid there's some seperation needed after all. Maybe you could combine that work with the sadly needed manual cleanup (putting back deleted comments etc)? |
Ok. I'll make another attempt to break it up |
2 participants
Summary
Fixes: #2685
dev:stopscriptENABLE_PUBLIC_GAMESenv varScreenshots
Changes
Local Development
OPENFRONT_CLIENT_PORTandOPENFRONT_SERVER_PORTenvironment variablesDevConfig.ts) for toggling features during local developmentpnpm dev:stopscript to kill processes on dev ports (Windows + Unix support)config.example.jsontemplate for local configurationSettings UI Refactor
Server
ENABLE_PUBLIC_GAMESenv var to disable public game scheduling in devmodal.css, unused pastel theme files)Test plan
pnpm lintpassespnpm testpasses (47 suites, 252 tests)pnpm dev:stopkills processes correctly