feat: add UI size slider to settings#3524
Open
IgnazioDS wants to merge 3 commits intoopenfrontio:mainfrom
Open
feat: add UI size slider to settings#3524IgnazioDS wants to merge 3 commits intoopenfrontio:mainfrom
IgnazioDS wants to merge 3 commits intoopenfrontio:mainfrom
Conversation
Add a UI scale slider (50%-200%) to the in-game settings panel, allowing players to adjust the interface size regardless of their OS scaling, browser zoom, or screen type (mobile, ultrawide, etc.). Implementation: - UserSettings: uiScale()/setUiScale()/applyUiScale() with localStorage persistence and clamping to 50-200 range - SettingsModal: range input slider following the existing volume slider pattern, placed after the audio settings - Main.ts: apply saved scale on page load - en.json: translation key for the label Uses CSS zoom on the root element for proper layout reflow. Closes openfrontio#3067
|
|
Contributor
WalkthroughAdds a persistent UI scale feature: new localization key, UI slider in settings, UserSettings methods for reading/setting/applying a clamped 50–200% scale, document zoom application, startup application during client initialization, and tests for normalization/persistence/zoom. Changes
Sequence DiagramssequenceDiagram
participant Client
participant UserSettings
participant Browser
Client->>UserSettings: applyUiScale()
activate UserSettings
UserSettings->>UserSettings: read uiScale() (default 100, normalize)
UserSettings->>Browser: set document.documentElement.style.zoom = value/100
deactivate UserSettings
sequenceDiagram
participant User
participant SettingsModal
participant UserSettings
participant Storage
participant Browser
User->>SettingsModal: adjust range slider
SettingsModal->>SettingsModal: onUiScaleChange()
SettingsModal->>UserSettings: setUiScale(scale)
activate UserSettings
UserSettings->>UserSettings: normalize/snap/clamp scale
UserSettings->>Storage: persist settings.uiScale
UserSettings->>Browser: applyUiScale -> set zoom
deactivate UserSettings
SettingsModal->>User: update displayed percentage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🤖 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 269-284: Normalize UI scale in one place and ensure both setting
and applying use it: create or use a single normalization step (e.g.,
normalizeUiScale(value?: number)) that ensures the value is finite, defaults to
the stored uiScale(), clamps to [50,200], and snaps to 10% steps (round to
nearest 10), then use that normalized value in setUiScale (before
setFloat/applyUiScale) and in applyUiScale (call normalizeUiScale() instead of
using raw scale or uiScale()), and ensure getFloat/setFloat still receive only
the normalized number for "settings.uiScale".
🪄 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: dc210491-5b9a-462b-8791-f52c1212e273
📒 Files selected for processing (4)
resources/lang/en.jsonsrc/client/Main.tssrc/client/graphics/layers/SettingsModal.tssrc/core/game/UserSettings.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
|
This pull request is stale because it has been open for fourteen days with no activity. If you want to keep this pull request open, add a comment or update the branch. |
github-actions
bot
added
the
Stale
iiamlewis
added
UI/UX
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/core/game/UserSettings.ts (1)
390-405:⚠️ Potential issue | 🟡 MinorNormalize UI scale in one place before read/apply.
Right now only
setUiScaleclamps. Stored values read byuiScale()and values applied byapplyUiScale()can still bypass finite/step normalization, so startup can apply invalid/out-of-range zoom.Proposed fix
+ private normalizeUiScale(scale: number): number { + if (!Number.isFinite(scale)) return 100; + const stepped = Math.round(scale / 10) * 10; + return Math.max(50, Math.min(200, stepped)); + } + uiScale(): number { - return this.getFloat("settings.uiScale", 100); + return this.normalizeUiScale(this.getFloat("settings.uiScale", 100)); } setUiScale(scale: number): void { - const clamped = Math.max(50, Math.min(200, scale)); - this.setFloat("settings.uiScale", clamped); - this.applyUiScale(clamped); + const normalized = this.normalizeUiScale(scale); + this.setFloat("settings.uiScale", normalized); + this.applyUiScale(normalized); } applyUiScale(scale?: number): void { - const value = scale ?? this.uiScale(); + const value = this.normalizeUiScale(scale ?? this.uiScale()); if (typeof document !== "undefined") { document.documentElement.style.zoom = (value / 100).toString(); } }🤖 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 390 - 405, Stored and applied UI scale can be out-of-range or non-finite because only setUiScale clamps; add a single normalization step and use it everywhere: implement a helper (e.g., normalizeUiScale(value?: number): number) that ensures the value is a finite number, rounds/steps as needed, and clamps to the allowed range (50–200) with default 100; update uiScale() to return normalizeUiScale(this.getFloat("settings.uiScale", 100)), update setUiScale(scale) to store normalizeUiScale(scale) and pass that to applyUiScale, and update applyUiScale(scale?) to call normalizeUiScale(scale ?? this.uiScale()) before using document.documentElement.style.zoom; reference these symbols: uiScale, setUiScale, applyUiScale, normalizeUiScale.
🤖 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/client/graphics/layers/SettingsModal.ts`:
- Around line 288-315: Run Prettier on this file to fix formatting issues
reported by CI; specifically reformat the SettingsModal.ts block that contains
the UI scale range input (references: class SettingsModal, methods/properties
userSettings.uiScale(), onUiScaleChange, translateText) so indentation,
attribute spacing, and JSX/HTML template literal formatting comply with project
Prettier config, then re-stage the updated file and push the commit.
---
Duplicate comments:
In `@src/core/game/UserSettings.ts`:
- Around line 390-405: Stored and applied UI scale can be out-of-range or
non-finite because only setUiScale clamps; add a single normalization step and
use it everywhere: implement a helper (e.g., normalizeUiScale(value?: number):
number) that ensures the value is a finite number, rounds/steps as needed, and
clamps to the allowed range (50–200) with default 100; update uiScale() to
return normalizeUiScale(this.getFloat("settings.uiScale", 100)), update
setUiScale(scale) to store normalizeUiScale(scale) and pass that to
applyUiScale, and update applyUiScale(scale?) to call normalizeUiScale(scale ??
this.uiScale()) before using document.documentElement.style.zoom; reference
these symbols: uiScale, setUiScale, applyUiScale, normalizeUiScale.
🪄 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: 1b227b8c-7a48-47b4-8704-464a2e2d9284
📒 Files selected for processing (4)
resources/lang/en.jsonsrc/client/Main.tssrc/client/graphics/layers/SettingsModal.tssrc/core/game/UserSettings.ts
✅ Files skipped from review due to trivial changes (1)
- resources/lang/en.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/Main.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/core/game/UserSettings.test.ts (1)
61-71: LGTM — nice non-finite coverage.Good call covering
NaNand+Infinity. Consider adding-Infinityand a negative finite value (e.g.-50) too —-50is finite so it would bypass the finite guard and rely on the clamp; both paths together make the intent crystal clear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/game/UserSettings.test.ts` around lines 61 - 71, Add two more assertions to the test for non-finite and negative values: call userSettings.setUiScale(Number.NEGATIVE_INFINITY) and assert userSettings.uiScale() === 100, localStorage.getItem("settings.uiScale") === "100", and document.documentElement.style.zoom === "1"; then call userSettings.setUiScale(-50) and assert the clamped value (use userSettings.uiScale()), the stored string in localStorage.getItem("settings.uiScale"), and the document.documentElement.style.zoom reflect the expected clamped/normalized behavior based on the existing clamp logic.src/client/graphics/layers/SettingsModal.ts (2)
288-309: UI scale row looks good; minor a11y polish.The row mirrors the volume slider pattern, which is nice and consistent. Two small suggestions you can take or leave:
- The
<img src=${settingsIcon}>reuses the gear icon already used by the Performance Overlay row below, which can confuse users scanning the list. A dedicated icon (or a different existing one) would help discoverability — one of the stated goals in issue#3067.- The slider has no accessible label association. Screen readers will announce "slider, 50 to 200" with no name. Adding
aria-label=${translateText("user_setting.ui_scale_label")}on the<input type="range">would fix that.♿ Optional fix for a11y
<input type="range" min="50" max="200" step="10" + aria-label=${translateText("user_setting.ui_scale_label")} .value=${this.userSettings.uiScale()} `@input`=${this.onUiScaleChange} class="w-full border border-slate-500 rounded-lg" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/SettingsModal.ts` around lines 288 - 309, The UI scale row reuses the gear icon and lacks an accessible label; update the row in SettingsModal.ts to use a distinct icon (replace settingsIcon with a more appropriate icon symbol) so it doesn't duplicate the Performance Overlay icon, and add an aria-label to the range input (use translateText("user_setting.ui_scale_label") as the value) alongside the existing .value and `@input` handler (onUiScaleChange) so screen readers get a proper name while leaving userSettings.uiScale() logic intact.
196-200: Guard againstNaNfrom the input value.
parseFloaton an empty or malformedvaluereturnsNaN.setUiScalealready normalizesNaNto 100 on theUserSettingsside, so behavior is safe, but that means a stray parse failure silently resets the slider to 100% during a drag. Tiny hardening: fall back to the current value when parsing fails, so the user's slider keeps its position.♻️ Optional fix
private onUiScaleChange(event: Event) { - const scale = parseFloat((event.target as HTMLInputElement).value); - this.userSettings.setUiScale(scale); + const raw = parseFloat((event.target as HTMLInputElement).value); + const scale = Number.isFinite(raw) ? raw : this.userSettings.uiScale(); + this.userSettings.setUiScale(scale); this.requestUpdate(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/SettingsModal.ts` around lines 196 - 200, The onUiScaleChange handler parses the input value into scale with parseFloat and can produce NaN which will cause the UI to jump to the normalized default; instead validate the parsed value and if NaN use the current setting as a fallback before calling setUiScale: in onUiScaleChange, after computing scale = parseFloat(...), if Number.isNaN(scale) replace it with the existing value from this.userSettings (e.g. this.userSettings.uiScale or this.userSettings.getUiScale()) and then call this.userSettings.setUiScale(scale) followed by this.requestUpdate().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/game/UserSettings.test.ts`:
- Around line 1-37: Add explicit vitest imports at the top of the test file to
match project convention: import { vi, describe, it, expect, beforeEach,
afterEach } from "vitest"; this ensures the globals used in the file (vi,
describe, it, expect, beforeEach, afterEach) are explicitly imported. Leave the
existing createMockLocalStorage, the beforeEach/afterEach hooks calling
userSettings.removeCached("settings.uiScale") and
document.documentElement.style.zoom resets unchanged; just add the import line
so the test follows the project's import style and avoids implicit globals.
---
Nitpick comments:
In `@src/client/graphics/layers/SettingsModal.ts`:
- Around line 288-309: The UI scale row reuses the gear icon and lacks an
accessible label; update the row in SettingsModal.ts to use a distinct icon
(replace settingsIcon with a more appropriate icon symbol) so it doesn't
duplicate the Performance Overlay icon, and add an aria-label to the range input
(use translateText("user_setting.ui_scale_label") as the value) alongside the
existing .value and `@input` handler (onUiScaleChange) so screen readers get a
proper name while leaving userSettings.uiScale() logic intact.
- Around line 196-200: The onUiScaleChange handler parses the input value into
scale with parseFloat and can produce NaN which will cause the UI to jump to the
normalized default; instead validate the parsed value and if NaN use the current
setting as a fallback before calling setUiScale: in onUiScaleChange, after
computing scale = parseFloat(...), if Number.isNaN(scale) replace it with the
existing value from this.userSettings (e.g. this.userSettings.uiScale or
this.userSettings.getUiScale()) and then call
this.userSettings.setUiScale(scale) followed by this.requestUpdate().
In `@tests/core/game/UserSettings.test.ts`:
- Around line 61-71: Add two more assertions to the test for non-finite and
negative values: call userSettings.setUiScale(Number.NEGATIVE_INFINITY) and
assert userSettings.uiScale() === 100, localStorage.getItem("settings.uiScale")
=== "100", and document.documentElement.style.zoom === "1"; then call
userSettings.setUiScale(-50) and assert the clamped value (use
userSettings.uiScale()), the stored string in
localStorage.getItem("settings.uiScale"), and the
document.documentElement.style.zoom reflect the expected clamped/normalized
behavior based on the existing clamp logic.
🪄 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: 8432e858-fd3a-4a2c-8984-6a800ec33c8a
📒 Files selected for processing (3)
src/client/graphics/layers/SettingsModal.tssrc/core/game/UserSettings.tstests/core/game/UserSettings.test.ts
Comment on lines
+1
to
+37
| import { UserSettings } from "../../../src/core/game/UserSettings"; | ||
|
|
||
| function createMockLocalStorage(): Storage { | ||
| let store: Record<string, string> = {}; | ||
| return { | ||
| getItem: (key: string) => store[key] ?? null, | ||
| setItem: (key: string, value: string) => { | ||
| store[key] = String(value); | ||
| }, | ||
| removeItem: (key: string) => { | ||
| delete store[key]; | ||
| }, | ||
| clear: () => { | ||
| store = {}; | ||
| }, | ||
| get length() { | ||
| return Object.keys(store).length; | ||
| }, | ||
| key: (index: number) => Object.keys(store)[index] ?? null, | ||
| }; | ||
| } | ||
|
|
||
| describe("UserSettings UI scale", () => { | ||
| let userSettings: UserSettings; | ||
|
|
||
| beforeEach(() => { | ||
| vi.stubGlobal("localStorage", createMockLocalStorage()); | ||
| userSettings = new UserSettings(); | ||
| userSettings.removeCached("settings.uiScale"); | ||
| document.documentElement.style.zoom = ""; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| userSettings.removeCached("settings.uiScale"); | ||
| document.documentElement.style.zoom = ""; | ||
| vi.unstubAllGlobals(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether vitest globals are enabled and how other tests import helpers.
fd -a 'vitest.config*'
fd -a 'vite.config*'
rg -nP '^\s*import\s+\{[^}]*\b(vi|describe|it|expect)\b' --type=ts -g 'tests/**' | head -n 20
rg -nP 'globals\s*:\s*true' -g 'vitest.config*' -g 'vite.config*'Repository: openfrontio/OpenFrontIO
Length of output: 2110
🏁 Script executed:
cat -n /home/jailuser/git/vite.config.ts | grep -A 5 -B 5 "globals"Repository: openfrontio/OpenFrontIO
Length of output: 319
Add explicit vitest imports to match project convention.
The file uses vi, describe, it, expect, beforeEach, afterEach without importing them. While globals: true is enabled in vite.config.ts, every other test file in the project explicitly imports these from "vitest". Keep this test consistent with the rest of the codebase:
Suggested import
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import { UserSettings } from "../../../src/core/game/UserSettings";Also note: UserSettings.cache is a static Map shared across instances. removeCached("settings.uiScale") in beforeEach/afterEach correctly clears it for this key, but watch for the same pattern when testing other keys — easy footgun to miss.
📝 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.
Suggested change
| import { UserSettings } from "../../../src/core/game/UserSettings"; | |
| function createMockLocalStorage(): Storage { | |
| let store: Record<string, string> = {}; | |
| return { | |
| getItem: (key: string) => store[key] ?? null, | |
| setItem: (key: string, value: string) => { | |
| store[key] = String(value); | |
| }, | |
| removeItem: (key: string) => { | |
| delete store[key]; | |
| }, | |
| clear: () => { | |
| store = {}; | |
| }, | |
| get length() { | |
| return Object.keys(store).length; | |
| }, | |
| key: (index: number) => Object.keys(store)[index] ?? null, | |
| }; | |
| } | |
| describe("UserSettings UI scale", () => { | |
| let userSettings: UserSettings; | |
| beforeEach(() => { | |
| vi.stubGlobal("localStorage", createMockLocalStorage()); | |
| userSettings = new UserSettings(); | |
| userSettings.removeCached("settings.uiScale"); | |
| document.documentElement.style.zoom = ""; | |
| }); | |
| afterEach(() => { | |
| userSettings.removeCached("settings.uiScale"); | |
| document.documentElement.style.zoom = ""; | |
| vi.unstubAllGlobals(); | |
| }); | |
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | |
| import { UserSettings } from "../../../src/core/game/UserSettings"; | |
| function createMockLocalStorage(): Storage { | |
| let store: Record<string, string> = {}; | |
| return { | |
| getItem: (key: string) => store[key] ?? null, | |
| setItem: (key: string, value: string) => { | |
| store[key] = String(value); | |
| }, | |
| removeItem: (key: string) => { | |
| delete store[key]; | |
| }, | |
| clear: () => { | |
| store = {}; | |
| }, | |
| get length() { | |
| return Object.keys(store).length; | |
| }, | |
| key: (index: number) => Object.keys(store)[index] ?? null, | |
| }; | |
| } | |
| describe("UserSettings UI scale", () => { | |
| let userSettings: UserSettings; | |
| beforeEach(() => { | |
| vi.stubGlobal("localStorage", createMockLocalStorage()); | |
| userSettings = new UserSettings(); | |
| userSettings.removeCached("settings.uiScale"); | |
| document.documentElement.style.zoom = ""; | |
| }); | |
| afterEach(() => { | |
| userSettings.removeCached("settings.uiScale"); | |
| document.documentElement.style.zoom = ""; | |
| vi.unstubAllGlobals(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/game/UserSettings.test.ts` around lines 1 - 37, Add explicit
vitest imports at the top of the test file to match project convention: import {
vi, describe, it, expect, beforeEach, afterEach } from "vitest"; this ensures
the globals used in the file (vi, describe, it, expect, beforeEach, afterEach)
are explicitly imported. Leave the existing createMockLocalStorage, the
beforeEach/afterEach hooks calling userSettings.removeCached("settings.uiScale")
and document.documentElement.style.zoom resets unchanged; just add the import
line so the test follows the project's import style and avoids implicit globals.
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
3 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.
Resolves #3067
Description:
Adds a UI scale slider to the in-game settings panel so players can adjust interface size from 50% to 200% in 10% increments. The setting is persisted through UserSettings, normalized before storage/application, applied during startup, and exposed with translated settings text.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
IgnazioDS