Perf(UserSettings): caching makes it 10x faster#3481
Draft
VariableVince wants to merge 5 commits intomainfrom
Draft
Perf(UserSettings): caching makes it 10x faster#3481VariableVince wants to merge 5 commits intomainfrom
VariableVince wants to merge 5 commits intomainfrom
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
WalkthroughReplaced direct localStorage access with a typed, cached UserSettings API (getBool/getString/getFloat/set*); removed UserSettings dependency from some layers; cached game config/theme in rendering layers; updated UI bindings (keybinds, booleans, attack ratio) to use the new typed accessors. Changes
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
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
1 participant
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.
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.
BEFORE
AFTER first tests (need to do more)
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
tryout33