Conversation
WalkthroughA new player name opacity control feature is implemented across the codebase. Users can adjust on-map player label visibility via a slider ranging from 0–100%. When opacity is 0, player names and flags become hidden. Changes span translations, UI components, game settings, and rendering layers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 Tip CodeRabbit can use your project's `biome` configuration to improve the quality of JS/TS/CSS/JSON code reviews.Add a configuration file to your project to customize how CodeRabbit runs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/client/graphics/layers/SettingsModal.ts (1)
462-493: Prefer reusing<setting-slider>here to avoid logic drift.This block duplicates slider rendering/value-label logic already implemented in
src/client/components/baseComponents/setting/SettingSlider.ts(including the zero-value translated label). Reusing the shared component will keep behavior consistent between both settings modals.🤖 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 462 - 493, This block duplicates the slider and zero-value label logic; replace it with the shared SettingSlider component (tag <setting-slider> / component implementation in SettingSlider.ts) to avoid drift: render <setting-slider> bound to this.userSettings.playerNameOpacity(), pass the same min/max (0/100), wire its input/change handler to onPlayerNameOpacityChange (or use the component's change event), and remove the duplicated percent/hidden label logic since SettingSlider already handles zero-value translation via translateText; ensure value source remains this.userSettings.playerNameOpacity() and event handler name matches the existing method.
🤖 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/NameLayer.ts`:
- Around line 342-345: Replace the unsafe innerHTML assignment for player names
with a safe text assignment: locate the code in NameLayer.ts where
nameDiv.querySelector(".player-name-span") is stored in span and currently calls
span.innerHTML = render.player.name(); and change it to set span.textContent (or
assign to (span as HTMLElement).textContent) while keeping the opacity logic
that uses nameOpacity intact; ensure you use render.player.name() only as text
(no HTML parsing) so player-controlled input cannot inject markup.
In `@src/core/game/UserSettings.ts`:
- Around line 249-252: setPlayerNameOpacity currently allows NaN through into
clampedOpacity and then setFloat which persists and emits "NaN"; guard against
non-finite inputs by validating the incoming opacity with
Number.isFinite(opacity) (or isFinite) before rounding and clamping, and if it's
not finite either return early (no save/emit) or default to a safe value (e.g.,
0), then compute const clampedOpacity = Math.max(0, Math.min(100,
Math.round(opacity))) and call this.setFloat("settings.playerNameOpacity",
clampedOpacity).
---
Nitpick comments:
In `@src/client/graphics/layers/SettingsModal.ts`:
- Around line 462-493: This block duplicates the slider and zero-value label
logic; replace it with the shared SettingSlider component (tag <setting-slider>
/ component implementation in SettingSlider.ts) to avoid drift: render
<setting-slider> bound to this.userSettings.playerNameOpacity(), pass the same
min/max (0/100), wire its input/change handler to onPlayerNameOpacityChange (or
use the component's change event), and remove the duplicated percent/hidden
label logic since SettingSlider already handles zero-value translation via
translateText; ensure value source remains this.userSettings.playerNameOpacity()
and event handler name matches the existing method.
🪄 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: 229ede9c-496a-4d1e-bee2-868e0c5e57d9
📒 Files selected for processing (6)
resources/lang/en.jsonsrc/client/UserSettingModal.tssrc/client/components/baseComponents/setting/SettingSlider.tssrc/client/graphics/layers/NameLayer.tssrc/client/graphics/layers/SettingsModal.tssrc/core/game/UserSettings.ts
| const span = nameDiv.querySelector(".player-name-span"); | ||
| if (span) { | ||
| span.innerHTML = render.player.name(); | ||
| (span as HTMLElement).style.opacity = `${nameOpacity}`; |
There was a problem hiding this comment.
Avoid innerHTML for player names (XSS risk).
Line [344] writes player-controlled text via innerHTML. Use textContent to prevent HTML/script injection.
Suggested fix
- span.innerHTML = render.player.name();
+ span.textContent = render.player.name();🧰 Tools
🪛 ast-grep (0.41.1)
[warning] 343-343: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: span.innerHTML = render.player.name()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 343-343: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: span.innerHTML = render.player.name()
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/NameLayer.ts` around lines 342 - 345, Replace the
unsafe innerHTML assignment for player names with a safe text assignment: locate
the code in NameLayer.ts where nameDiv.querySelector(".player-name-span") is
stored in span and currently calls span.innerHTML = render.player.name(); and
change it to set span.textContent (or assign to (span as
HTMLElement).textContent) while keeping the opacity logic that uses nameOpacity
intact; ensure you use render.player.name() only as text (no HTML parsing) so
player-controlled input cannot inject markup.
| setPlayerNameOpacity(opacity: number): void { | ||
| const clampedOpacity = Math.max(0, Math.min(100, Math.round(opacity))); | ||
| this.setFloat("settings.playerNameOpacity", clampedOpacity); | ||
| } |
There was a problem hiding this comment.
Guard non-finite opacity before saving.
Line [250] can produce NaN (for example if a malformed event passes NaN), and then setFloat stores "NaN" in storage and emits it.
Suggested fix
setPlayerNameOpacity(opacity: number): void {
+ if (!Number.isFinite(opacity)) return;
const clampedOpacity = Math.max(0, Math.min(100, Math.round(opacity)));
this.setFloat("settings.playerNameOpacity", clampedOpacity);
}📝 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.
| setPlayerNameOpacity(opacity: number): void { | |
| const clampedOpacity = Math.max(0, Math.min(100, Math.round(opacity))); | |
| this.setFloat("settings.playerNameOpacity", clampedOpacity); | |
| } | |
| setPlayerNameOpacity(opacity: number): void { | |
| if (!Number.isFinite(opacity)) return; | |
| const clampedOpacity = Math.max(0, Math.min(100, Math.round(opacity))); | |
| this.setFloat("settings.playerNameOpacity", clampedOpacity); | |
| } |
🤖 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 249 - 252, setPlayerNameOpacity
currently allows NaN through into clampedOpacity and then setFloat which
persists and emits "NaN"; guard against non-finite inputs by validating the
incoming opacity with Number.isFinite(opacity) (or isFinite) before rounding and
clamping, and if it's not finite either return early (no save/emit) or default
to a safe value (e.g., 0), then compute const clampedOpacity = Math.max(0,
Math.min(100, Math.round(opacity))) and call
this.setFloat("settings.playerNameOpacity", clampedOpacity).
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
1 participant
Description:
It's really hard sometimes to see what buildings or features are under those big text letters of people's nametags and troop counts on the map (also reported by users on the Discord). This PR adds a setting for opacity of the name and troop count in the NameLayer, allowing users to change the opacity via a slider in both lobby settings and in-game settings. Setting the opacity to 0% will also hide the flag, but opacity changes do not change the opacity of a nation/player's flag since it already has transparency applied to it. This does not affect gameplay critical icons such as alliance break, incoming alliance request, alliance notification, nukes, crowns, emojis, etc. (see below)
Video Demo
Recording.2026-03-17.020418.mp4
Setting on Lobby Menu
Screen.Recording.2026-03-17.020514.mp4
Emojis and other icons intact (when hidden)
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
bijx