Perf/Refactor(UserSettings): caching makes it 10-20x faster#3481
Merged
Perf/Refactor(UserSettings): caching makes it 10-20x faster#3481
Conversation
…ache Theme/Config better in two files (Theme in DefaultConfig in turn calls dark mode setting, so it's related to the changes)
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughReplaced direct localStorage access with a cached, typed UserSettings API, added typed getters/setters (attack ratio, keybinds, performance overlay), renamed settings-change events to parameterized keys, removed UserSettings dependency from some layers, and updated UI components and event wiring to use the new API and event keys. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/game/UserSettings.ts`:
- Around line 23-28: The getCached method currently uses the || operator which
treats empty strings as falsy and converts them to null; update getCached in
UserSettings to use the nullish coalescing operator (??) when returning the
cached value so that empty strings are preserved (i.e., return
UserSettings.cache.get(key) ?? null) and ensure the cache set remains
localStorage.getItem(key) so null is only used for missing keys; reference the
getCached method and UserSettings.cache when making this change.
- Line 7: Replace all generic userSettings.get(...) and userSettings.set(...)
call sites with the typed API provided by UserSettings: use
userSettings.getBool(key, defaultValue) and userSettings.setBool(key, value) for
boolean settings; use userSettings.getString/setString for string values and
userSettings.getFloat/setFloat for numeric values. Specifically update the call
in CopyButton (was userSettings.get("settings.lobbyIdVisibility", true)) to
userSettings.getBool("settings.lobbyIdVisibility", true) and the calls in
PerformanceOverlay (was userSettings.set("settings.performanceOverlay",
nextVisible)) to userSettings.setBool("settings.performanceOverlay",
nextVisible); audit other usages and migrate any remaining generic get/set to
the appropriate typed method (getBool/setBool, getString/setString,
getFloat/setFloat).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d12040b-0ed7-4571-bdea-48f6f1e3430a
📒 Files selected for processing (7)
src/client/UserSettingModal.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/TerrainLayer.tssrc/client/graphics/layers/TerritoryLayer.tssrc/client/graphics/layers/UILayer.tssrc/core/game/GameView.tssrc/core/game/UserSettings.ts
💤 Files with no reviewable changes (2)
- src/client/graphics/layers/TerritoryLayer.ts
- src/client/graphics/layers/UILayer.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/core/game/UserSettings.ts (1)
40-50:⚠️ Potential issue | 🔴 CriticalFinish migrating the attacking troops overlay setting.
Line 119 and Line 123 still call
this.get()/this.set(), but this class now exposes only the typed API. That breakssrc/client/graphics/layers/SettingsModal.tsandsrc/client/graphics/layers/AttackingTroopsOverlay.ts, and CI is already failing onTS2339.🔧 Minimal fix
attackingTroopsOverlay() { - return this.get("settings.attackingTroopsOverlay", true); + return this.getBool("settings.attackingTroopsOverlay", true); } toggleAttackingTroopsOverlay() { - this.set("settings.attackingTroopsOverlay", !this.attackingTroopsOverlay()); + this.setBool( + "settings.attackingTroopsOverlay", + !this.attackingTroopsOverlay(), + ); }Also applies to: 118-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/UserSettings.ts` around lines 40 - 50, The code still uses the removed untyped methods this.get() / this.set() for the attacking troops overlay setting; update callers to use the new typed API such as getBool(key, defaultValue) and setBool(key, value). Locate the places that call this.get("...") or this.set("...", ...) (notably in SettingsModal.ts and AttackingTroopsOverlay.ts) and replace them with calls to UserSettings.getBool(key, defaultValue) and UserSettings.setBool(key, booleanValue) (or the appropriate getX/setX typed helpers) so they use getBool/setBool and preserve default values and emitChange behavior via setBool.
🧹 Nitpick comments (1)
src/core/game/UserSettings.ts (1)
7-38: Verify cache sync outside the current tab.After the first read,
UserSettings.cachekeeps the value for the rest of the tab. If another tab or window changeslocalStorage, reads likedarkMode()orperformanceOverlay()will stay stale until reload. Please verify this with a simple two-tab test before merging; if multi-tab use matters, this needs onestoragelistener to update or clear the static cache.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/UserSettings.ts` around lines 7 - 38, UserSettings.cache is never updated when other tabs change localStorage, so getCached()/darkMode()/performanceOverlay() can become stale; add a window "storage" event listener (registered once, e.g., in a static initializer or class bootstrap) that checks event.key and updates UserSettings.cache: if event.newValue is null set cache entry to null (matching removeCached behavior), otherwise set the cache to event.newValue; ensure the listener only touches keys managed by UserSettings and does not throw, and keep existing setCached/removeCached to continue writing to localStorage and emitting events via emitChange; verify with a two-tab test that changing a setting in one tab updates the other tab’s cached value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/core/game/UserSettings.ts`:
- Around line 40-50: The code still uses the removed untyped methods this.get()
/ this.set() for the attacking troops overlay setting; update callers to use the
new typed API such as getBool(key, defaultValue) and setBool(key, value). Locate
the places that call this.get("...") or this.set("...", ...) (notably in
SettingsModal.ts and AttackingTroopsOverlay.ts) and replace them with calls to
UserSettings.getBool(key, defaultValue) and UserSettings.setBool(key,
booleanValue) (or the appropriate getX/setX typed helpers) so they use
getBool/setBool and preserve default values and emitChange behavior via setBool.
---
Nitpick comments:
In `@src/core/game/UserSettings.ts`:
- Around line 7-38: UserSettings.cache is never updated when other tabs change
localStorage, so getCached()/darkMode()/performanceOverlay() can become stale;
add a window "storage" event listener (registered once, e.g., in a static
initializer or class bootstrap) that checks event.key and updates
UserSettings.cache: if event.newValue is null set cache entry to null (matching
removeCached behavior), otherwise set the cache to event.newValue; ensure the
listener only touches keys managed by UserSettings and does not throw, and keep
existing setCached/removeCached to continue writing to localStorage and emitting
events via emitChange; verify with a two-tab test that changing a setting in one
tab updates the other tab’s cached value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6aefa97a-c104-4c68-98f8-c27eed41f7b1
📒 Files selected for processing (4)
src/client/UserSettingModal.tssrc/client/graphics/GameRenderer.tssrc/core/game/GameView.tssrc/core/game/UserSettings.ts
✅ Files skipped from review due to trivial changes (1)
- src/core/game/GameView.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/UserSettingModal.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/game/UserSettings.ts (1)
7-37:⚠️ Potential issue | 🟠 MajorKeep the shared cache in sync with external
localStorageupdates.After the first read,
getCached()returns the memoized value forever. If a setting changes in another tab/window, through DevTools, or through any remaining directlocalStoragewrite, every newUserSettingsinstance in this tab will keep seeing stale data until reload. Please update or clear the cache from the browserstorageevent at minimum.Possible fix
export class UserSettings { private static cache = new Map<string, string | null>(); + private static storageSyncAttached = false; + + constructor() { + UserSettings.attachStorageSync(); + } + + private static attachStorageSync(): void { + if (UserSettings.storageSyncAttached) return; + if (typeof globalThis.addEventListener !== "function") return; + + globalThis.addEventListener("storage", (event: StorageEvent) => { + if (event.storageArea !== localStorage) return; + + if (event.key === null) { + UserSettings.cache.clear(); + return; + } + + UserSettings.cache.set(event.key, event.newValue); + }); + + UserSettings.storageSyncAttached = true; + }Based on learnings,
new UserSettings()is the established pattern in this repo, so this static cache is shared across many component-local instances.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/UserSettings.ts` around lines 7 - 37, The static cache (UserSettings.cache) isn't updated when other tabs or scripts change localStorage; add a window "storage" event handler in the UserSettings class (e.g., in a static initializer or constructor) that listens for storage events, and when event.key matches a cached key update UserSettings.cache to event.newValue (or set to null if newValue is null) so getCached() returns fresh values; ensure existing setters (setCached/removeCached) continue to write localStorage and update the cache and that the storage handler ignores events with key === null or from the same window if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/core/game/UserSettings.ts`:
- Around line 7-37: The static cache (UserSettings.cache) isn't updated when
other tabs or scripts change localStorage; add a window "storage" event handler
in the UserSettings class (e.g., in a static initializer or constructor) that
listens for storage events, and when event.key matches a cached key update
UserSettings.cache to event.newValue (or set to null if newValue is null) so
getCached() returns fresh values; ensure existing setters
(setCached/removeCached) continue to write localStorage and update the cache and
that the storage handler ignores events with key === null or from the same
window if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2391ec50-b44c-4046-a051-022855ff4e55
📒 Files selected for processing (1)
src/core/game/UserSettings.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/game/UserSettings.ts (2)
248-260:⚠️ Potential issue | 🟠 MajorCache inconsistency:
setFlagandclearFlagbypass the cache.
getFlag()reads fromgetCached("flag"), butsetFlag()andclearFlag()write directly tolocalStoragewithout updating the cache. This causes stale reads after a flag change.🐛 Proposed fix to use cached methods
setFlag(flag: string): void { if (flag === "country:xx") { this.clearFlag(); } else { - localStorage.setItem("flag", flag); + this.setCached("flag", flag); } console.log("emitting change!"); this.emitChange("flag", flag); } clearFlag(): void { - localStorage.removeItem("flag"); + this.removeCached("flag"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/UserSettings.ts` around lines 248 - 260, setFlag and clearFlag currently write directly to localStorage, causing getFlag (which reads via getCached("flag")) to return stale data; change setFlag to update the cache (use the existing cache setter method, e.g., setCached("flag", flag) or equivalent) except when flag === "country:xx" when you should call the cache-removal method, and change clearFlag to remove the cached "flag" (use removeCached or the cache API used elsewhere) instead of touching localStorage directly; keep emitting the change via emitChange("flag", flag) as before.
254-254:⚠️ Potential issue | 🟡 MinorRemove debug log.
console.log("emitting change!")looks like a leftover debug statement.🧹 Proposed fix
- console.log("emitting change!"); this.emitChange("flag", flag);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/UserSettings.ts` at line 254, Remove the leftover debug statement console.log("emitting change!") from the UserSettings class (the method that emits/dispatches setting changes — e.g., emitChange/update or similar) so production code has no stray console output; if structured logging is required instead, replace it with the appropriate logger call (e.g., processLogger.debug or this.logger.debug) rather than console.log.
🧹 Nitpick comments (1)
src/core/game/UserSettings.ts (1)
200-200: Nitpick: Redundant?? null.
getCached()already returnsstring | null, so?? nullhas no effect.♻️ Optional cleanup
- let data = this.getCached(PATTERN_KEY) ?? null; + let data = this.getCached(PATTERN_KEY);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/UserSettings.ts` at line 200, The assignment uses a redundant nullish coalescing on a call that already returns string | null; update the declaration by removing "?? null" so that "data" is directly assigned from this.getCached(PATTERN_KEY); this touches the getCached call and the PATTERN_KEY usage in UserSettings (remove the unnecessary coalescing only).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/core/game/UserSettings.ts`:
- Around line 248-260: setFlag and clearFlag currently write directly to
localStorage, causing getFlag (which reads via getCached("flag")) to return
stale data; change setFlag to update the cache (use the existing cache setter
method, e.g., setCached("flag", flag) or equivalent) except when flag ===
"country:xx" when you should call the cache-removal method, and change clearFlag
to remove the cached "flag" (use removeCached or the cache API used elsewhere)
instead of touching localStorage directly; keep emitting the change via
emitChange("flag", flag) as before.
- Line 254: Remove the leftover debug statement console.log("emitting change!")
from the UserSettings class (the method that emits/dispatches setting changes —
e.g., emitChange/update or similar) so production code has no stray console
output; if structured logging is required instead, replace it with the
appropriate logger call (e.g., processLogger.debug or this.logger.debug) rather
than console.log.
---
Nitpick comments:
In `@src/core/game/UserSettings.ts`:
- Line 200: The assignment uses a redundant nullish coalescing on a call that
already returns string | null; update the declaration by removing "?? null" so
that "data" is directly assigned from this.getCached(PATTERN_KEY); this touches
the getCached call and the PATTERN_KEY usage in UserSettings (remove the
unnecessary coalescing only).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0506da30-c0da-4beb-9532-8f749e6445ee
📒 Files selected for processing (5)
src/client/UserSettingModal.tssrc/client/components/CopyButton.tssrc/client/graphics/layers/PerformanceOverlay.tssrc/core/game/GameView.tssrc/core/game/UserSettings.ts
✅ Files skipped from review due to trivial changes (2)
- src/client/components/CopyButton.ts
- src/core/game/GameView.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/client/graphics/layers/PerformanceOverlay.ts
- src/client/UserSettingModal.ts
github-project-automation
Bot
moved this from Development
to Final Review
in OpenFront Release Management
VariableVince
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
github-project-automation
Bot
moved this from Development
to Final Review
in OpenFront Release Management
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
VariableVince
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/client/graphics/layers/PerformanceOverlay.ts (1)
483-487: String comparison for boolean value works but is subtle.The handler compares
event.detail === "true"becausesetCachedstores and emits string values. This is correct but relies on the implementation detail thatsetBoolconverts to"true"/"false"strings.Consider documenting this behavior or using a helper:
Optional: Add clarity to the comparison
private onUserSettingsChanged = (event: CustomEvent<string>) => { - const nextVisible = event.detail === "true"; + // setCached emits string values; setBool stores "true"/"false" + const nextVisible = event.detail === "true"; if (this.isVisible === nextVisible) return; this.setVisible(nextVisible); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/PerformanceOverlay.ts` around lines 483 - 487, The comparison in onUserSettingsChanged currently relies on setCached/setBool emitting "true"/"false" strings (event.detail === "true"), which is subtle; update the handler to use a clear conversion helper or explicit parse (e.g., a booleanFromString helper or BooleanString.equals) and reference the same conversion where setBool/setCached emit values so the contract is explicit; ensure you update onUserSettingsChanged to call that helper (instead of === "true") and/or add a short comment documenting that setBool emits "true"/"false" string values so future readers see the intent.src/core/game/UserSettings.ts (1)
51-57:getBooltreats empty string""as falsy, returning the default.The check
if (!value)returnsdefaultValuewhenvalueis"". This is probably fine for booleans (an empty string is not a valid boolean), but worth noting for consistency with the??fix ingetCached.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/UserSettings.ts` around lines 51 - 57, getBool currently uses a falsy check (`if (!value)`) which treats empty string as missing; change this to a nullish check to match the getCached/?? behavior. In the getBool method of UserSettings.ts replace `if (!value) return defaultValue;` with a null/undefined guard such as `if (value == null) return defaultValue;` so only null/undefined are treated as missing while keeping the rest of the boolean parsing logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client/graphics/layers/PerformanceOverlay.ts`:
- Around line 483-487: The comparison in onUserSettingsChanged currently relies
on setCached/setBool emitting "true"/"false" strings (event.detail === "true"),
which is subtle; update the handler to use a clear conversion helper or explicit
parse (e.g., a booleanFromString helper or BooleanString.equals) and reference
the same conversion where setBool/setCached emit values so the contract is
explicit; ensure you update onUserSettingsChanged to call that helper (instead
of === "true") and/or add a short comment documenting that setBool emits
"true"/"false" string values so future readers see the intent.
In `@src/core/game/UserSettings.ts`:
- Around line 51-57: getBool currently uses a falsy check (`if (!value)`) which
treats empty string as missing; change this to a nullish check to match the
getCached/?? behavior. In the getBool method of UserSettings.ts replace `if
(!value) return defaultValue;` with a null/undefined guard such as `if (value ==
null) return defaultValue;` so only null/undefined are treated as missing while
keeping the rest of the boolean parsing logic intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea7616bb-cde5-4c79-8c00-cf72ed3b69a1
📒 Files selected for processing (8)
src/client/FlagInput.tssrc/client/Main.tssrc/client/PatternInput.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/components/baseComponents/setting/SettingToggle.tssrc/client/graphics/layers/PerformanceOverlay.tssrc/core/game/UserSettings.ts
💤 Files with no reviewable changes (1)
- src/client/components/baseComponents/setting/SettingToggle.ts
evanpelle
pushed a commit
that referenced
this pull request
Apr 7, 2026
## Description: NameLayer perf part 2 after #3475 with thanks to @scamiv. Shaves off another 10% or thereabouts, even doing something extra for a fix (see below). Also refactor/fixes around NameLayer and PlayerIcons, which is used by both NameLayer and PlayerInfoOverlay, and underlying function in GameView. This would go well with other PR #3481, since this layer reads multiple settings. Reasoning to not use events and instead rely on fast caching is explained in that PR. ### Contents - Fixes: -- Fixes bug on .dev introduced by wrong assumption by me in previous PR #3475. displayName CAN change during game, when Hidden Names is toggled from settings, so needs to be put back in renderPlayerInfo. -- Fixes longer existing bug: it was assumed Dark Mode didn't change after creation of icon element. Now it also sets Dark Mode attribute when updating icons elements. -- Fixes target mark icons not being shown to team members, while the icons were shown to normal allies. And EventsDisplay displayed message "XX requests you attack XX" to both team members and allies already. So why is the icon not shown to both if the message already is. While we improve performance of GameView > PlayerView > transitiveTargets (which is only used by NameLayer/PlayerIcons so only in this context). We can add team members' targets to it in one go. So previously transitiveTargets returned: your own targets and allies' targets. Now transitiveTargets is faster and returns: your own targets and allies' targets and team members' targets. - NameLayer: -- renderLayer: for target icons, getPlayerIcons used to fetch myPlayer.transitiveTargets each time. While that doesn't change per player we're rendering for. So now, we fetch myPlayer.transitiveTargets once per call to renderPlayerInfo, which passes it on to getPlayerIcons. So now we check it 1x each 100ms (renderCheckRate) inside of renderLayer. Instead of up to 100s of times each 500ms (renderRefreshRate) inside of getPlayerIcons inside of renderPlayerInfo loop. -- createBasePlayerElement and renderPlayerInfo: use cloneNode where possible with templates -- createPlayerElement: only find the elements and set font and flag. Leave the rest to renderLayer > renderPlayerInfo which fills displayName and troops and font color very soon after anyway. I haven't noticed a difference in testing. -- cache game.config() and others -- renderPlayerInfo: remove check if render.flagDiv exists, we know it exists. Check if fontColor changed before assigning it (it never changes, currently, be it dark mode or light mode). Don't check if troops or size changes, that happens so often that the overhead for checking would be smaller than the win, probably. -- We don't require nameLocation to be changed to change scale (see previous Namelayer perf PR for the reason). But it seems good to check if the transform changed before 'overwriting' it, so do that now instead. -- Remove Alliance icon DOM traversals. Only do it once, for each time an alliance icon is displayed. To this end, also made NameLayer more agnostic on Alliance icon stuff. By moving more code to PlayerIcons. See below. -- Use cached allianceDuration instead of fetching this static value every time -- Re-use from PlayerIcons: ALLIANCE_ICON_ID, TRAITOR_ICON_ID etc -- create more sub-functions to make the icons loop in renderLayer more readable: handleEmojiIcon, handleAllianceIcons, createOrUpdateIconElement (createIconElement already existed, now combined), handleTraitorIcon. For Alliance icons, this was already done in PlayerIcons.ts through createAllianceProgressIcon (now createAllianceProgressIconRefs), and more now to skip some DOM traversals. But most of this belongs in NameLayer itself when it comes to seperation of concern. - cache dark mode (as boolean and as string) - use dark mode to update (alliance) icons too, not only on create, since the setting can change after icon element creation and before it is removed -- for getPlayerIcons, add this.alliancesDisabled. If disabled, getPlayerIcons won't fetch Alliance icon and Alliance Request icon. - PlayerIcons: -- use cloneNode where possible -- added check for alliances disabled: then skip alliance (request) icon checks -- See point under NameLayer about the move of Alliance icon code to PlayerIcons. To make NameLayer even more agnostic on it and keep it in one place. -- getPlayerIcons: skip creating a new Set from myPlayer.transitiveTargets() each time getPlayerIcons is called. One allocation less. Just do .includes on the returned array. Probably just as fast in this case, also because not many Targets are present many times anyway. -- getPlayerIcons: on outgoingEmojis(), use .find() instead of .filter() since we only use the first result anyway and it saves us another allocation. -- getPlayerIcons: for nukes, only fetch the ones from the player we're rendering for, not all game nukes. Also don't use .filter() and just a normal loop to skip an allocation. Logic outcome is the same. -- getPlayerIcons: for target icons, it used to fetch myPlayer.transitiveTargets each time. While that doesn't change per player we're rendering for. So now, NameLayer fetches myPlayer.transitiveTargets once per call to renderPlayerInfo, which passes it on to getPlayerIcons. -- Remove the need for querySelector and getElementsByTagName("img") alltogether. Since this would be done for every time an alliance was (re-)created, here in createAllianceProgressIconRefs in PlayerIcons it makes more sense to not do DOM traversal than in createPlayerElement in NameLayer where we only do it once per player per game anyway. We assume updateAllianceProgressIconRefs just knows of all image names in createAllianceProgressIconRefs. This is a bit less dynamic and maintainable maybe, but i think worth the win. And the functions are all one-purpose and not meant to be used dynamically by another caller anyway. -- So instead of updateAllianceProgressIconRefs looping through refs.images, now just update the different images each. See point above. - PlayerInfoOverlay: also re-use the new exported consts from PlayerIcons. Since we put those in PlayerIcons anyway, need to be consistent. Even though PlayerInfoOverlay is outside of the scope of this PR otherwise. -- for getPlayerIcons, add this.alliancesDisabled here too. If disabled, getPlayerIcons won't fetch Alliance icon and Alliance Request icon. We also send includeAllianceIcon = false, which means Alliance icon will already be excluded but Alliance Request icon is normally still fetched and shown. - GameView > PlayerView: for transitiveTargets (only used in NameLayer/PlayerIcons so only in this context), improve performance. It did several allocations. Now it loops directly over the arrays we need. Also (as mentioned under Fixes) previously transitiveTargets returned: your own targets and allies' targets. Now transitiveTargets is faster and returns: your own targets and allies' targets and team members' targets. **BEFORE**  **AFTER** (including now getting team members' targets for myPlayer.transitiveTargets)  ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: tryout33
github-project-automation
Bot
moved this from Final Review
to Complete
in OpenFront Release Management
evanpelle
pushed a commit
that referenced
this pull request
Apr 10, 2026
…uld not update flag on clearFlag (#3626) ## Description: #3479 put setFlag and clearFlag in UserSettings. #3481 did some updates to incorporate them into UserSettings cache. But made two misjudgments. Which led to: when default/none flag was selected, FlagInput would still display the previously selected flag. And on clearFlag call, unnecessary updates where send to FlagInput > updateFlag. This PR fixes it. - Don't send update event on clearFlag by default, just like PR 3479 didn't. Although i could imagine potential cases where we would want to update the displayed flag in FlagInput. Not when clearFlag is called from Auth > logOut. But maybe in some, but not all, cases when it is called from Cosmetics > getPlayerCosmeticsRefs. That is for future investigations by another contributor. - Do sent update event when clearFlag is called from setFlag, if the "country:xx" (default/none) flag is set. Previously, it would have sent event this.emitChange(FLAG_KEY, **"country:xx"**). Now, via clearFlag, it sends this.emitChange(FLAG_KEY, **null**) - Have FlagInput > updateFlag handle null. Previously it expected to always recieve a string, even for default/none flag "country:xx". Now it will also set this.flag to null if it recieves an event with null. It being able to handle event value null actually hardens the code so seems better to me either way. FlagInput > isDefaultFlagValue() already did handle null values to determine the default flag, so no changes needed there. ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: tryout33
evanpelle
pushed a commit
that referenced
this pull request
Apr 13, 2026
…ngs, and Keybinds fixes (#3619) ## Description: 1) Have last localstorage calls for keybinds and attack ratio also use UserSettings cache instead, after #3481. Remaining calls to localstorage are for different things than user settings, so they are left as is. 2) Consolidate and centralize keybinds logic. And three fixes for it. - **UnitDisplay** and **UserSettingsModal**: _parsedUserKeybinds_ is introduced in **UserSettings** to centralize their logic. It is also used by _normalizedUserKeybinds_, see point below. - **UserSettingsModal** -- replaced unwanted cast `as SettingKeybind` by a typed QuerySelector. -- renamed this.keybinds to this.userKeybinds for more clarity, and distinction from defaultKeybinds. -- state private _userKeybinds_: remove type string[] since loadKeybindsFromStorage replaces a value array by its first string element, so it can not contain string[] anymore. -- _handleKeybindChange_ and _getKeyValue_: no need to check for Array.isArray anymore, see above reason. -- **Fix**: checks after calling _parsedUserKeybinds_ are improved a bit: don't delete all keybinds and print a console warning when finding just one invalid keybind and (i think i have seen people complaining about things being removed). Instead it now migrates or throws out the invalid ones but keeps the valid ones. Also works with the "Null" value expected and removed within **UserSettingsModal**._handleyKeybindChange_() and in **HelpModal**. When legacy value is an array and key is empty, don't put value as key but get first array element or empty string as key name. So that check on line 68 is true. - **HelpModal** and **InputHandler**: Also centralize/consolidate their logic more, by having __keybinds()_ from **UserSettings** perform fetching _getDefaultKeybinds_ and _normalizedUserKeybinds_. -- Functionality in _normalizedUserKeybinds_ is the same: Where HelpModal did return [k, v.value] if typeof (v as any).value === "string", this is now handled by lines 309-310 of normalizedKeybinds still the same but with less lines. Same for old HelplModal if (typeof v === "string") return [k, v], this is stil returned by line 112 of normalizedKeybinds. And return [k, undefined] when (typeof val !== "string") as was done in InputHandler, isn't needed as values that weren't strings were already filtered out right after which we still do on line 314 of normalizedKeybinds. -- **Fix** in _normalizedUserKeybinds_: added one extra thing that was a discrepancy between **HelpModal**/**InputHandler** and **UserSettingsModal** before: **UserSettingsModal** would handle array values, and normalize them by picking only the first value if it is a string. Now have _normalizedKeybinds_ do the same. Otherwise it would have thrown those values out while **UserSettingsModal** would have kept the first value. This may still help a returning player who hasn't played in the last version (i think i have seen people complaining about things being removed, but that may not have been about this). And makes the logic more consistent between **UserSettingsModal** and **HelpModal**/**InputHandler**. - **UserSettings**: -- _getDefaultKeybinds_: centralized/consolidated logic, accepts Platform.isMac parameter. In **HelpModal**, **InputHandler** and **UserSettingsModal** the same list with default keybinds was hardcoded. Now they all read from _getDefaultKeybinds_. The list of default keybinds in **HelpModal** was a little shorter, but that doesn't matter since its _render_() function has hardcoded which of the hotkeys **HelpModal** shows. Have thought about putting default keybinds in **DefaultConfig** but with all the logic handled through **UserSettings**, this seemed the better place in the current refactor. -- _removeCached_: make public, now that **InputHandler.test.ts** needs to be able to call it. We could instead make a public function like removeKeybinds() and keep removeCached() private, but went with this for now. -- _parsedUserKeybinds_: centralized/consolidated logic for **UserSettingsModal**/**UserDisplay**. Always returns an object, even an empty one if the JSON wasn't parsable. -- _normalizedKeybinds_: centralized/consolidated logic. Used by _keybinds_() which is now called by **HelpModal**/**InputHandler**. -- _keybinds_: now uses getDefaultKeybinds() and normalizedKeybinds() to get the default and user changed keybinds. -- **Fix** in _keybinds_: it now removes a key if it is Unbound by the user in **UserSettingsModal**. Instead of first loading the parsedUserKeybinds, removing "Null" keys from it, and then merging that with defaultKeybinds (so default key would overwrite an unbound key), we now merge parsedUserKeybinds with defaultKeybinds and after that remove "Null" keys from it (so that unbound key stays removed). For example if Boat Attack Up is set to "None" ("Null") by clicking Unbind, there is now no hotkey working for it anymore. Even when the default is "B". Why? This prevents the user from being confused, they have deliberately Unbound it, they don't understand why it still works (have seen bug reports and game feedback about this)? Also more importantly: they used to now be able to bind "B" to another action. Effectively making key "B" bound to two actions: the user choosen one and Boat Attack. This also makes the logic more consistent. Because building hotkeys in **UnitDisplay** already didn't work when unbound, eg. when Build Missile Silo was Unbound, the "5" key did not do anything anymore (there is a fallback in **UnitDisplay** in case the key is actually null, but it does respect "Null" as it should). -- _setKeybinds_: have it accept an object, it stringifies it itself. Callers UserSettingsModal and InputHandler.test.ts now just send either a string or an object. - **InputHandler.test.ts**: -- use **UserSettings** functions instead of localStorage for more real-world testing. -- change test "ignores non-string values and preserves defaults, removes 'Null' for unbound keys". As explained above, as a fix we no longer preserve unbound ("Null") keys within InputHandler. UserSettings.keybinds() now removes "Null" keys as explained above. - ControlPanel: use UserSettings to fetch initial attack ratio. ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: tryout33
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
Skip slow and blocking LocalStorage reads, replace by a Map. Also some refactoring.
Contains
No out-of-sync issue between main and worker thread: Earlier PRs got a comment from evan about main & worker.worker thread having their own version of usersettings and possibly getting out-of-sync (see Cache local storage reads to improve performance #760 (review), Cache User Settings #896 (review) and Add dark mode change event #1266.
But userSettings is not used in files ran by worker.worker, not even 10 months after evan's first comment about it. In GameRunner, createGameRunner sends NULL to getConfig as argument for userSettings. And DefaultConfig guards against userSettings being null by throwing an error, but it has never been thrown which points to worker.worker thread not using userSettings. So we do not need to worry about syncing between the threads currently.
(If needed in the future after all, we could quite easily sync it, by loading the userSettings cache on worker.worker and listening to the "user-settings-changed" event @scamiv to keep it synced (changes in WorkerMessages and WorkerClient etc would be needed to handle this).
Went with cache in UserSettings, not with listening to "user-settings-changed" event: "user-settings-changed" was added by @scamiv and is used in PerformanceOverlay. Which is great for single files that need the very best performance. But having to add that same system to any file reading settings, scales poorly and would lead to messy code. Also, a developer could make the mistake of not listening to the event and it would end up just reading LocalStorage again just like now. Also a developer might forget removing the listener or so etc. The cache is a central solution and fast, without changes to other files needed and future-proof.
Make sure each setting is cached: UserSettingsModal was using LocalStorage directly by itself for some things. Made it use the central UserSettings methods instead so we avoid LocalStorage reads as much as possible. For this, changed get() and set() in UserSettings to getBool() and setBool(), to introduce a getString() and setString() for use in UserSettingsModal while keeping getCached() and setCached() private within UserSettings.
Remove unused 'focusLocked' and 'toggleFocusLocked' from UserSettings: was last changed 11 months ago to just return false. Since then we've moved to different ways of highlighting and this setting isn't used anymore. No existing references or callers are left.
Other files:
-- Have callers call the renamed functions (see point above)
-- Remove userSettings from UILayer and Territorylayer: the variable is unused in those files. Also remove from GameRenderer when it calls TerritoryLayer.
-- Cache calls to defaultconfig Theme (which in turn calls dark mode setting)/Config better in: GameView and Terrainlayer.
Update on Contents later on
It wasn't really in scope of this PR but further consolidation was called for. These changes could also pave the way for UserSettingsModal (main menu) perhaps being partly mergable with SettingsModal (in-game) one day as it begins to look more like it. Even though UserSettingsModal still does things its own way, and does console.log where SettingsModal doesn't, etc. They both have partially different content and settings but also have a large overlap.
UserSettings: Removed localStorage call from clearFlag() and setFlag() which were added after creation of this PR, and were neatly merged in silence without merge conflicts so i wasn't aware of them yet until now.
UserSettings: added key constants, exported to use both inside UserSettings and in files that listen to its events.
UserSettings 'emitChange': now done from setCached, removed from setBool, setFlag etc. Also removed from the new setFlag. And from setPattern even though it emitted "pattern" instead of key name "territoryPattern"; now it emits the default "territoryPattern" from PATTERN_KEY which is re-used in Store, TerritoryPatternsModal and PatternInput.
UserSettingsModal: made UserSettingsModal call existing toggle functions in UserSettings, or new or existing getter or setter. We do not need CustomEvent: checked anymore. In UserSettingsModal, its toggle functions did not all actually toggle, some like toggleLeftClickOpensMenu actually just set a value. Based on the 'checked' value of the CustomEvent. But we don't need that 'checked' value anymore and none of the checks for it inside the toggle functions in UserSettingsModal, now that we just directly call toggleLeftClickOpensMenu and others in UserSettings.
SettingToggle: continuing about not needing CustomEvent anymore: the old way actually fired two events. The native change event from and our own CustomEvent from handleChange in SettingToggle. It prevented handling both events by checking e.detail?.checked === undefined. But now, the native event is all we need to show the visual toggle change and trigger @Changed in UserSettingsModal which calls the toggle function.
Use the toggle functions too from CopyButton and PerformanceOverlay.ts. In PerformanceOverlay, change in onUserSettingsChanged was needed because of how setBool works.
UserSettingsModal 'toggleDarkMode': in UserSettingsModal, removed the event from toggleDarkMode in UserSettingsModal; nothing is listening to this event anymore after DarkModeButton.ts was removed some time ago. Also both UserSettingsModal an UserSettings added/removed "dark" from the document element. Now that UserSettingsModal calls toggleDarkMode in UserSettings, we could centralize that. But UserSettings is in core, not in client like UserSettingsModal. But now that we emit "user-settings-changed", we could handle it even more centralized and not have UserSettingsModal or UserSettings touch the element directly. Instead have Main.ts listen to the event and change it dark mode from there.
UserSettings: added claryfing comment to attackRatioIncrement and the new attackRatio setters/getters, to explain their difference. Noticed a small omitment in its description and fixed that right away in en.json: you can change attack ratio increment by shift+mouse wheel scroll or by hotkey. So made "How much the attack ratio keybinds change per press" also mention "/scroll."
BEFORE (with getDisplayName added back to NameLayer as a fix i will do soon)
get > getItem in UserSettings
renderLayer in NameLayer (with getDisplayName added back to NameLayer as a fix i will do soon)
AFTER (with getDisplayName added back to NameLayer as a fix i will do soon)
getCached in UserSettings
renderLayer in NameLayer (with getDisplayName added back to NameLayer as a fix i will do soon)
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
tryout33