Cache User Settings #896
Cache User Settings #896
Conversation
WalkthroughThe codebase was refactored to use a shared singleton instance of Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant userSettings
participant localStorage
Component->>userSettings: get or set setting
userSettings->>userSettings: read/write from cache
userSettings->>localStorage: (on first load or update) read/write value
userSettings-->>Component: return setting value
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/client/ClientGameRunner.ts(2 hunks)src/client/DarkModeButton.ts(1 hunks)src/client/InputHandler.ts(2 hunks)src/client/Main.ts(2 hunks)src/client/UserSettingModal.ts(11 hunks)src/client/graphics/layers/OptionsMenu.ts(3 hunks)src/core/game/GameView.ts(1 hunks)src/core/game/UserSettings.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/client/Main.ts (1)
src/core/game/UserSettings.ts (1)
userSettings(115-115)
src/client/graphics/layers/OptionsMenu.ts (2)
src/core/game/UserSettings.ts (1)
userSettings(115-115)src/client/InputHandler.ts (1)
RefreshGraphicsEvent(64-64)
src/client/UserSettingModal.ts (1)
src/core/game/UserSettings.ts (1)
userSettings(115-115)
src/client/InputHandler.ts (1)
src/core/game/UserSettings.ts (1)
userSettings(115-115)
src/client/ClientGameRunner.ts (1)
src/core/game/UserSettings.ts (1)
userSettings(115-115)
src/core/game/UserSettings.ts (1)
src/core/configuration/DefaultConfig.ts (1)
userSettings(205-210)
🪛 GitHub Actions: 🧪 CI
src/core/game/UserSettings.ts
[error] 19-19: ReferenceError: localStorage is not defined. The test suite failed to run due to usage of localStorage in a Node environment where it is not available.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (14)
src/core/game/UserSettings.ts (2)
29-46: Cache implementation looks good for performance.The getter methods correctly read from the cache instead of repeatedly accessing
localStorage. This should significantly improve performance as intended by the PR.
115-115: Singleton pattern correctly implemented.The singleton instance export provides a clean way to share state across the application without creating multiple instances.
src/core/game/GameView.ts (2)
35-35: Import change correctly implements singleton pattern.The change from importing the
UserSettingsclass to importing theuserSettingssingleton instance is correct and aligns with the caching refactor.
183-191: Singleton usage in player name methods looks good.The methods correctly use the singleton instance to check the
anonymousNamessetting. This maintains the same functionality while using the cached values.src/client/Main.ts (2)
7-7: Import change correctly adopts singleton pattern.The change from importing the
UserSettingsclass to importing theuserSettingssingleton instance is correct and consistent with the refactor.
234-238: Good separation of concerns for DOM manipulation.The dark mode DOM manipulation is appropriately handled in the client code rather than inside the settings class. This is better architecture than having DOM manipulation in the core settings class.
src/client/graphics/layers/OptionsMenu.ts (3)
7-7: Import change correctly adopts singleton pattern.The change from importing the
UserSettingsclass to importing theuserSettingssingleton instance is correct and maintains consistency across the codebase.
98-124: Toggle methods correctly use singleton instance.All toggle methods have been updated to use the singleton instance instead of a local instance. The calls are consistent and maintain the same functionality.
202-232: Render methods correctly use singleton for display values.The render logic correctly uses the singleton instance to check current setting values for display. The event emission for dark mode toggle (
RefreshGraphicsEvent) is a good approach for handling UI updates without tight coupling.src/client/DarkModeButton.ts (1)
3-3: Good refactoring to use singleton pattern!The changes correctly replace the local UserSettings instance with the shared singleton. This improves performance by avoiding duplicate instances and enables centralized caching of settings.
Also applies to: 7-7, 14-15
src/client/InputHandler.ts (1)
3-3: Correct singleton usage!The change properly replaces the local UserSettings instance with the shared singleton, maintaining the same functionality while enabling performance improvements through caching.
Also applies to: 307-307
src/client/ClientGameRunner.ts (1)
27-27: Good singleton usage in game creation!The refactoring correctly passes the shared singleton to the game creation function, eliminating the need for local instantiation.
Also applies to: 94-96
src/client/UserSettingModal.ts (2)
4-4: Excellent singleton adoption for getters!The getter calls correctly use the shared singleton instance, ensuring consistent state access across the application.
Also applies to: 222-222, 232-232, 241-241, 250-250, 259-259
83-83: Correct implementation for dark mode toggle.This setter properly uses the
enabledparameter from the event, maintaining the expected toggle behavior.
src/core/game/UserSettings.ts
Outdated
| toggleDarkMode() { | ||
| this.set("settings.darkMode", !this.darkMode()); | ||
| if (this.darkMode()) { | ||
| this.cache.darkMode = !this.cache.darkMode; | ||
| this.setToStorage("settings.darkMode", this.cache.darkMode); | ||
| if (this.cache.darkMode) { | ||
| document.documentElement.classList.add("dark"); | ||
| } else { | ||
| document.documentElement.classList.remove("dark"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
DOM manipulation should be handled outside the settings class.
The toggleDarkMode method directly manipulates the DOM by adding/removing CSS classes. This creates tight coupling between settings logic and UI concerns.
Consider moving DOM manipulation to the caller or using an event system:
toggleDarkMode() {
this.cache.darkMode = !this.cache.darkMode;
this.setToStorage("settings.darkMode", this.cache.darkMode);
- if (this.cache.darkMode) {
- document.documentElement.classList.add("dark");
- } else {
- document.documentElement.classList.remove("dark");
- }
}Then handle DOM updates in the client code that calls this method.
📝 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.
| toggleDarkMode() { | |
| this.set("settings.darkMode", !this.darkMode()); | |
| if (this.darkMode()) { | |
| this.cache.darkMode = !this.cache.darkMode; | |
| this.setToStorage("settings.darkMode", this.cache.darkMode); | |
| if (this.cache.darkMode) { | |
| document.documentElement.classList.add("dark"); | |
| } else { | |
| document.documentElement.classList.remove("dark"); | |
| } | |
| } | |
| toggleDarkMode() { | |
| this.cache.darkMode = !this.cache.darkMode; | |
| this.setToStorage("settings.darkMode", this.cache.darkMode); | |
| } |
🤖 Prompt for AI Agents
In src/core/game/UserSettings.ts lines 76 to 84, the toggleDarkMode method
directly manipulates the DOM by adding or removing the "dark" class, which
couples settings logic with UI concerns. Refactor by removing the DOM
manipulation from this method so it only updates the darkMode setting and
storage. Instead, emit an event or return the new darkMode state so the caller
can handle updating the DOM accordingly.
src/core/game/UserSettings.ts
Outdated
| private getFromStorage(key: string, defaultValue: boolean): boolean { | ||
| const value = localStorage.getItem(key); | ||
| if (value === "true") return true; | ||
| if (value === "false") return false; | ||
|
|
||
| return defaultValue; | ||
| } | ||
|
|
||
| set(key: string, value: boolean) { | ||
| private setToStorage(key: string, value: boolean) { | ||
| localStorage.setItem(key, value ? "true" : "false"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add localStorage availability check to helper methods.
The private helper methods also need protection against localStorage being undefined in Node.js environments.
Apply this fix to both helper methods:
private getFromStorage(key: string, defaultValue: boolean): boolean {
+ if (typeof localStorage === 'undefined') {
+ return defaultValue;
+ }
const value = localStorage.getItem(key);
if (value === "true") return true;
if (value === "false") return false;
return defaultValue;
}
private setToStorage(key: string, value: boolean) {
+ if (typeof localStorage === 'undefined') {
+ return;
+ }
localStorage.setItem(key, value ? "true" : "false");
}📝 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 getFromStorage(key: string, defaultValue: boolean): boolean { | |
| const value = localStorage.getItem(key); | |
| if (value === "true") return true; | |
| if (value === "false") return false; | |
| return defaultValue; | |
| } | |
| set(key: string, value: boolean) { | |
| private setToStorage(key: string, value: boolean) { | |
| localStorage.setItem(key, value ? "true" : "false"); | |
| } | |
| private getFromStorage(key: string, defaultValue: boolean): boolean { | |
| if (typeof localStorage === 'undefined') { | |
| return defaultValue; | |
| } | |
| const value = localStorage.getItem(key); | |
| if (value === "true") return true; | |
| if (value === "false") return false; | |
| return defaultValue; | |
| } | |
| private setToStorage(key: string, value: boolean) { | |
| if (typeof localStorage === 'undefined') { | |
| return; | |
| } | |
| localStorage.setItem(key, value ? "true" : "false"); | |
| } |
🧰 Tools
🪛 GitHub Actions: 🧪 CI
[error] 19-19: ReferenceError: localStorage is not defined. The test suite failed to run due to usage of localStorage in a Node environment where it is not available.
🤖 Prompt for AI Agents
In src/core/game/UserSettings.ts lines 18 to 27, the private helper methods
getFromStorage and setToStorage access localStorage without checking if it is
available, which can cause errors in Node.js environments. Add a check at the
start of each method to verify if localStorage is defined; if not, return the
default value in getFromStorage and skip setting in setToStorage. This will
prevent runtime errors when localStorage is unavailable.
src/core/game/UserSettings.ts
Outdated
| setDarkMode(value: boolean) { | ||
| this.cache.darkMode = value; | ||
| this.setToStorage("settings.darkMode", value); | ||
| if (value) { | ||
| document.documentElement.classList.add("dark"); | ||
| } else { | ||
| document.documentElement.classList.remove("dark"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same DOM coupling issue in setDarkMode setter.
The explicit setter method has the same tight coupling to DOM manipulation as the toggle method.
Apply the same separation of concerns here:
setDarkMode(value: boolean) {
this.cache.darkMode = value;
this.setToStorage("settings.darkMode", value);
- if (value) {
- document.documentElement.classList.add("dark");
- } else {
- document.documentElement.classList.remove("dark");
- }
}📝 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.
| setDarkMode(value: boolean) { | |
| this.cache.darkMode = value; | |
| this.setToStorage("settings.darkMode", value); | |
| if (value) { | |
| document.documentElement.classList.add("dark"); | |
| } else { | |
| document.documentElement.classList.remove("dark"); | |
| } | |
| } | |
| setDarkMode(value: boolean) { | |
| this.cache.darkMode = value; | |
| this.setToStorage("settings.darkMode", value); | |
| } |
🤖 Prompt for AI Agents
In src/core/game/UserSettings.ts around lines 100 to 108, the setDarkMode method
directly manipulates the DOM by adding or removing the "dark" class on
document.documentElement, causing tight coupling. Refactor this method to remove
direct DOM manipulation and instead update only the internal state and storage.
Move the DOM update logic to a separate function or layer responsible for UI
changes, ensuring separation of concerns between state management and UI
rendering.
src/core/game/UserSettings.ts
Outdated
| constructor() { | ||
| this.cache = { | ||
| emojis: this.getFromStorage("settings.emojis", true), | ||
| anonymousNames: this.getFromStorage("settings.anonymousNames", false), | ||
| specialEffects: this.getFromStorage("settings.specialEffects", true), | ||
| darkMode: this.getFromStorage("settings.darkMode", false), | ||
| leftClickOpensMenu: this.getFromStorage( | ||
| "settings.leftClickOpensMenu", | ||
| false, | ||
| ), | ||
| focusLocked: this.getFromStorage("settings.focusLocked", true), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Fix localStorage access for Node.js test environment.
The pipeline failure shows localStorage is not defined in Node.js test environment. The constructor directly accesses localStorage during class instantiation, which breaks in server-side environments.
Consider adding a runtime check for localStorage availability:
constructor() {
+ if (typeof localStorage === 'undefined') {
+ // Fallback for Node.js or server environments
+ this.cache = {
+ emojis: true,
+ anonymousNames: false,
+ specialEffects: true,
+ darkMode: false,
+ leftClickOpensMenu: false,
+ focusLocked: true,
+ };
+ return;
+ }
this.cache = {
emojis: this.getFromStorage("settings.emojis", true),
anonymousNames: this.getFromStorage("settings.anonymousNames", false),
specialEffects: this.getFromStorage("settings.specialEffects", true),
darkMode: this.getFromStorage("settings.darkMode", false),
leftClickOpensMenu: this.getFromStorage(
"settings.leftClickOpensMenu",
false,
),
focusLocked: this.getFromStorage("settings.focusLocked", 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.
| constructor() { | |
| this.cache = { | |
| emojis: this.getFromStorage("settings.emojis", true), | |
| anonymousNames: this.getFromStorage("settings.anonymousNames", false), | |
| specialEffects: this.getFromStorage("settings.specialEffects", true), | |
| darkMode: this.getFromStorage("settings.darkMode", false), | |
| leftClickOpensMenu: this.getFromStorage( | |
| "settings.leftClickOpensMenu", | |
| false, | |
| ), | |
| focusLocked: this.getFromStorage("settings.focusLocked", true), | |
| }; | |
| } | |
| constructor() { | |
| if (typeof localStorage === 'undefined') { | |
| // Fallback for Node.js or server environments | |
| this.cache = { | |
| emojis: true, | |
| anonymousNames: false, | |
| specialEffects: true, | |
| darkMode: false, | |
| leftClickOpensMenu: false, | |
| focusLocked: true, | |
| }; | |
| return; | |
| } | |
| this.cache = { | |
| emojis: this.getFromStorage("settings.emojis", true), | |
| anonymousNames: this.getFromStorage("settings.anonymousNames", false), | |
| specialEffects: this.getFromStorage("settings.specialEffects", true), | |
| darkMode: this.getFromStorage("settings.darkMode", false), | |
| leftClickOpensMenu: this.getFromStorage( | |
| "settings.leftClickOpensMenu", | |
| false, | |
| ), | |
| focusLocked: this.getFromStorage("settings.focusLocked", true), | |
| }; | |
| } |
🤖 Prompt for AI Agents
In src/core/game/UserSettings.ts around lines 4 to 16, the constructor directly
accesses localStorage, causing errors in Node.js test environments where
localStorage is undefined. Modify the getFromStorage method or the constructor
to first check if localStorage is available (e.g., typeof localStorage !==
'undefined') before accessing it. If localStorage is not available, return
default values or handle gracefully to avoid runtime errors during server-side
execution.
| @@ -108,7 +107,7 @@ export async function createClientGame( | |||
| lobbyConfig: LobbyConfig, | |||
| eventBus: EventBus, | |||
| transport: Transport, | |||
| userSettings: UserSettings, | |||
| userSettings, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type annotation for better type safety.
The parameter type annotation was removed, which reduces type safety. Consider adding it back to maintain clear interfaces.
- userSettings,
+ userSettings: UserSettings,📝 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.
| userSettings, | |
| userSettings: UserSettings, |
🤖 Prompt for AI Agents
In src/client/ClientGameRunner.ts at line 110, the parameter userSettings is
missing a type annotation, reducing type safety. Add the appropriate type
annotation back to the userSettings parameter to ensure clear and safe
interfaces.
| @@ -97,7 +95,7 @@ export class UserSettingModal extends LitElement { | |||
| const enabled = e.detail?.checked; | |||
| if (typeof enabled !== "boolean") return; | |||
|
|
|||
| this.userSettings.set("settings.emojis", enabled); | |||
| userSettings.setEmojis(true); | |||
There was a problem hiding this comment.
Fix hardcoded values in toggle methods.
These setter calls hardcode true instead of using the enabled parameter from the event. This breaks the toggle functionality - users won't be able to turn these settings off.
- userSettings.setEmojis(true);
+ userSettings.setEmojis(enabled);
- userSettings.setFxLayer(true);
+ userSettings.setFxLayer(enabled);
- userSettings.setAnonymousNames(true);
+ userSettings.setAnonymousNames(enabled);
- userSettings.setLeftClickOpensMenu(true);
+ userSettings.setLeftClickOpensMenu(enabled);Also applies to: 107-107, 116-116, 125-125
🤖 Prompt for AI Agents
In src/client/UserSettingModal.ts at lines 98, 107, 116, and 125, the setter
methods like setEmojis are called with hardcoded true values, which disables
toggling off the settings. Replace the hardcoded true with the appropriate
boolean parameter (likely named enabled) from the event handler to correctly
reflect the user's toggle action and allow both enabling and disabling of these
settings.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
We have two threads: main & worker, so UserSettings actually has two instances, one in each thread. caching it locally will cause them to get out of sync.
What I recommend is to keep the local cache, and then have a invalidate() method that is called once per tick.
Instead of having a separate field for each value, just do:
cache = Map<string, string>(), and then on invalidate() call cache.clear()
|
Closing this PR as it doesn't look like this PR it its current form will be implemented anymore. |
github-project-automation
bot
moved this from Development
to Complete
in OpenFront Release Management
Description:
An alternative implementation of PR#760 by @robert-pitt-foodhub. Caches user settings which seem to be a heavy draw on main thread.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
1brucben