Sound & Effects Settings + Notifications#3951
Open
sardidefcon wants to merge 3 commits into
Open
Conversation
Adds per-effect toggles for all in-game sound effects and visual effects, organized by category, in both the main menu Settings and the in-game Settings modal. Sound settings (new "Sounds" tab / submenu): - Volume sliders for background music and sound effects - Individual on/off toggles for all sound effects grouped by category: UI & Notifications, Weapons & Combat, Diplomacy, Construction Visual effects settings (new "Effects" tab / submenu): - Master toggle for Special Effects (gates all fx child toggles) - Master toggle for Alert Frame (gates betrayal/attack alert toggles) - Individual on/off toggles for all fx effects grouped by category: Environment, Combat, Nuclear, Screen Alerts - Child toggles are visually disabled when their master toggle is off Other improvements: - Per-effect persistence via UserSettings (localStorage) - SoundManager checks per-effect enabled state before playing - FxLayer, AlertFrame, DynamicUILayer, NukeFx all respect per-effect flags - Volume sliders use step="1" to avoid floating-point display artifacts - SettingToggle supports a disabled prop (opacity + pointer-events-none) - All new UI text goes through translateText() with en.json entries
Contributor
WalkthroughThis PR adds per-effect sound and FX toggles, expands settings into Sounds/Effects/Notifications pages, integrates a game-start sound and optional browser notification into lobby join, gates in-game FX by per-effect flags, and adds localization and tests for the new settings. ChangesSound and FX Settings System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/ClientGameRunner.ts`:
- Around line 138-148: The Notification constructor can throw and currently sits
inside the start-message handling path in ClientGameRunner.ts; wrap the new
Notification(...) call in a try-catch so exceptions don't abort the handler and
block resolveJoin() and createClientGame(); catch the error, log or report it
(e.g., via your existing logger or console.error) including context like
translateText("game_start_notification.*") and
userSettings.gameStartNotificationsEnabled(), but do not rethrow so the rest of
the start flow continues.
In `@src/client/graphics/layers/FxLayer.ts`:
- Line 163: In FxLayer.ts inside the onTrainEvent handler, the impact FX
conditional is inverted: change the check from using !unit.reachedTarget() to
unit.reachedTarget() so the "fx-shell-impact" effect is emitted when the unit
has reached its target; keep the existing fxEffectEnabled("fx-shell-impact")
guard and ensure the same branch emits the impact FX only when
unit.reachedTarget() is true.
In `@src/client/Main.ts`:
- Around line 821-826: Wrap the Notification access and request in a safe
try/catch inside handleJoinLobby: check for "Notification" in window, then try
to read Notification.permission and call Notification.requestPermission(),
awaiting the returned Promise and attaching a .catch to swallow/log errors; on
any thrown error, log it (or ignore) and continue so the function does not exit
early. Specifically modify the block that references Notification.permission and
Notification.requestPermission() to catch synchronous exceptions and promise
rejections so lobby join proceeds even if notification APIs are broken.
In `@src/core/game/UserSettings.ts`:
- Around line 401-427: Add unit tests that cover the new UserSettings
accessors/mutators: isSoundEffectEnabled, setSoundEffectEnabled, isFxEnabled,
setFxEnabled, gameStartNotificationsEnabled, and toggleGameStartNotifications.
Create tests that (1) assert default values returned by
isSoundEffectEnabled/isFxEnabled/gameStartNotificationsEnabled, (2) call
setSoundEffectEnabled/setFxEnabled and verify the corresponding getBool
behaviour changes, and (3) call toggleGameStartNotifications and verify the
boolean flips; use the same test harness/helpers/mocked storage used by other
UserSettings tests so settings persistence is exercised and cleaned up.
🪄 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: e6c04395-311a-4b7b-8393-1ef54650471e
⛔ Files ignored due to path filters (3)
resources/sounds/effects/add-ammo.mp3is excluded by!**/*.mp3resources/sounds/effects/start.mp3is excluded by!**/*.mp3resources/sounds/effects/upgrade.mp3is excluded by!**/*.mp3
📒 Files selected for processing (14)
resources/lang/en.jsonsrc/client/ClientGameRunner.tssrc/client/Main.tssrc/client/UserSettingModal.tssrc/client/components/baseComponents/setting/SettingSlider.tssrc/client/components/baseComponents/setting/SettingToggle.tssrc/client/graphics/fx/NukeFx.tssrc/client/graphics/layers/AlertFrame.tssrc/client/graphics/layers/DynamicUILayer.tssrc/client/graphics/layers/FxLayer.tssrc/client/graphics/layers/SettingsModal.tssrc/client/sound/SoundManager.tssrc/client/sound/Sounds.tssrc/core/game/UserSettings.ts
| onTrainEvent(unit: UnitView) { | ||
| if (!unit.isActive()) { | ||
| if (!unit.reachedTarget() && this.fxEnabled()) { | ||
| if (!unit.reachedTarget() && this.fxEffectEnabled("fx-shell-impact")) { |
Contributor
There was a problem hiding this comment.
Train impact FX condition looks inverted (Line 163).
onTrainEvent currently emits impact FX when !unit.reachedTarget(). For “impact” behavior, this should usually be unit.reachedTarget().
🔧 Proposed fix
- if (!unit.reachedTarget() && this.fxEffectEnabled("fx-shell-impact")) {
+ if (unit.reachedTarget() && this.fxEffectEnabled("fx-shell-impact")) {📝 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
| if (!unit.reachedTarget() && this.fxEffectEnabled("fx-shell-impact")) { | |
| if (unit.reachedTarget() && this.fxEffectEnabled("fx-shell-impact")) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/graphics/layers/FxLayer.ts` at line 163, In FxLayer.ts inside the
onTrainEvent handler, the impact FX conditional is inverted: change the check
from using !unit.reachedTarget() to unit.reachedTarget() so the
"fx-shell-impact" effect is emitted when the unit has reached its target; keep
the existing fxEffectEnabled("fx-shell-impact") guard and ensure the same branch
emits the impact FX only when unit.reachedTarget() is true.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
- Wrap game-start Notification creation in try-catch so a throwing constructor (quota, enterprise policy, browser bugs) cannot block resolveJoin()/createClientGame() and the game start flow - Guard notification permission request in handleJoinLobby with try-catch and a promise .catch() so a broken Notification API cannot abort lobby join - Add unit tests covering the new UserSettings accessors/mutators: isSoundEffectEnabled/setSoundEffectEnabled, isFxEnabled/setFxEnabled, gameStartNotificationsEnabled/toggleGameStartNotifications - Apply Prettier formatting
Contributor
|
can you add mobile screenshots too? |
Contributor
Author
Of course, I forgot. 
SOUNDSEFFECTSNOTIFICATIONS |
Contributor
Author
|
You think I'm an idiot, and you're right @ryanbarlow97 I don't know why, but my brain interpreted it as you asking for screenshots of the in-game menu, not the mobile menu hahahahha I attach them: 
|
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@resources/lang/en.json`:
- Around line 1352-1359: The JSON key "game_start_notification" is placed at the
top level but should be nested under the "fullscreen" object; move the entire
"game_start_notification" object so its keys become
fullscreen.game_start_notification.title and
fullscreen.game_start_notification.body to match the expected key path (update
the "fullscreen" object in resources/lang/en.json to include
"game_start_notification" rather than leaving it top-level).
In `@src/client/ClientGameRunner.ts`:
- Around line 103-104: SoundManager is instantiated at SoundManager(eventBus,
userSettings) but if the user leaves the lobby before a game starts the existing
stop() behavior (which checks currentGameRunner === null) doesn't dispose the
SoundManager, leaving audio/event resources alive; to fix, update the
early-lobby-exit path (the handler that runs when a user leaves before start) to
explicitly dispose the pre-start SoundManager instance—either call a disposal
method (e.g., soundManager.dispose() or soundManager.destroy()) or modify
SoundManager.stop() to perform full cleanup when currentGameRunner === null;
reference the SoundManager instance created at the top of ClientGameRunner and
ensure the same variable is cleaned up on the early-exit code path so eventBus
subscriptions and audio resources are released.
In `@src/client/graphics/layers/SettingsModal.ts`:
- Around line 244-267: Replace hardcoded alt attributes on the <img> elements in
SettingsModal.ts (e.g., alt="musicIcon", "soundEffectsIcon", "specialEffects",
"alertFrame", "notificationIcon", "permissionIcon") with calls to
translateText(...) using new localization keys (e.g.,
translateText("icon.music"), translateText("icon.soundEffects"), etc. Update the
corresponding entries in resources/lang/en.json with these keys and English
values, and ensure every img alt in the other mentioned ranges (around lines
434-452 and 553-574) is updated the same way so all user-visible alt text goes
through translateText().
In `@tests/UserSettings.test.ts`:
- Around line 24-114: The test file reimplements a test harness instead of using
the shared setup helper; update the suite to call the shared setup() helper to
create the test environment and replace manual stubbing/initialization.
Specifically, remove the manual vi.stubGlobal("localStorage",
createMockLocalStorage()) and the manual cache clear on UserSettings, and
instead call setup() at the top of the describe block to obtain the testing
context, then instantiate UserSettings via that context or normally if setup
provides globals; ensure all tests still reference the same UserSettings methods
(isSoundEffectEnabled, setSoundEffectEnabled, isFxEnabled, setFxEnabled,
gameStartNotificationsEnabled, toggleGameStartNotifications) and that
localStorage persistence assertions remain valid under the shared setup.
🪄 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: f949dad3-e389-40cc-83c9-5e5e07bcc12b
⛔ Files ignored due to path filters (3)
resources/sounds/effects/add-ammo.mp3is excluded by!**/*.mp3resources/sounds/effects/start.mp3is excluded by!**/*.mp3resources/sounds/effects/upgrade.mp3is excluded by!**/*.mp3
📒 Files selected for processing (15)
resources/lang/en.jsonsrc/client/ClientGameRunner.tssrc/client/Main.tssrc/client/UserSettingModal.tssrc/client/components/baseComponents/setting/SettingSlider.tssrc/client/components/baseComponents/setting/SettingToggle.tssrc/client/graphics/fx/NukeFx.tssrc/client/graphics/layers/AlertFrame.tssrc/client/graphics/layers/DynamicUILayer.tssrc/client/graphics/layers/FxLayer.tssrc/client/graphics/layers/SettingsModal.tssrc/client/sound/SoundManager.tssrc/client/sound/Sounds.tssrc/core/game/UserSettings.tstests/UserSettings.test.ts
Comment on lines
1352
to
1359
| "fullscreen": { | ||
| "enter": "Enter fullscreen", | ||
| "exit": "Exit fullscreen" | ||
| }, | ||
| "game_start_notification": { | ||
| "title": "The game is starting", | ||
| "body": "Choose your starting position" | ||
| } |
Contributor
There was a problem hiding this comment.
Move game_start_notification under fullscreen to match expected key path.
This block is currently top-level, but the stack contract for this PR expects fullscreen.game_start_notification.title/body. In the current shape, lookups on that path will fail.
Proposed fix
"fullscreen": {
- "enter": "Enter fullscreen",
- "exit": "Exit fullscreen"
- },
- "game_start_notification": {
- "title": "The game is starting",
- "body": "Choose your starting position"
+ "enter": "Enter fullscreen",
+ "exit": "Exit fullscreen",
+ "game_start_notification": {
+ "title": "The game is starting",
+ "body": "Choose your starting position"
+ }
}📝 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
| "fullscreen": { | |
| "enter": "Enter fullscreen", | |
| "exit": "Exit fullscreen" | |
| }, | |
| "game_start_notification": { | |
| "title": "The game is starting", | |
| "body": "Choose your starting position" | |
| } | |
| "fullscreen": { | |
| "enter": "Enter fullscreen", | |
| "exit": "Exit fullscreen", | |
| "game_start_notification": { | |
| "title": "The game is starting", | |
| "body": "Choose your starting position" | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@resources/lang/en.json` around lines 1352 - 1359, The JSON key
"game_start_notification" is placed at the top level but should be nested under
the "fullscreen" object; move the entire "game_start_notification" object so its
keys become fullscreen.game_start_notification.title and
fullscreen.game_start_notification.body to match the expected key path (update
the "fullscreen" object in resources/lang/en.json to include
"game_start_notification" rather than leaving it top-level).
Comment on lines
+103
to
104
| const soundManager = new SoundManager(eventBus, userSettings); | ||
|
|
Contributor
There was a problem hiding this comment.
Dispose pre-start SoundManager on early lobby exit.
SoundManager is created at Line 103, but if the user leaves before game start, stop() takes the currentGameRunner === null path and never disposes it. That leaves audio/event resources alive.
Suggested fix
return {
stop: (force: boolean = false) => {
if (!force && currentGameRunner?.shouldPreventWindowClose()) {
console.log("Player is active, prevent leaving game");
return false;
}
console.log("leaving game");
if (currentGameRunner) {
currentGameRunner.stop();
currentGameRunner = null;
} else {
+ soundManager.dispose();
transport.leaveGame();
}
return true;
},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/ClientGameRunner.ts` around lines 103 - 104, SoundManager is
instantiated at SoundManager(eventBus, userSettings) but if the user leaves the
lobby before a game starts the existing stop() behavior (which checks
currentGameRunner === null) doesn't dispose the SoundManager, leaving
audio/event resources alive; to fix, update the early-lobby-exit path (the
handler that runs when a user leaves before start) to explicitly dispose the
pre-start SoundManager instance—either call a disposal method (e.g.,
soundManager.dispose() or soundManager.destroy()) or modify SoundManager.stop()
to perform full cleanup when currentGameRunner === null; reference the
SoundManager instance created at the top of ClientGameRunner and ensure the same
variable is cleaned up on the early-exit code path so eventBus subscriptions and
audio resources are released.
Comment on lines
+24
to
+114
| describe("UserSettings sound/fx/notification accessors", () => { | ||
| let settings: UserSettings; | ||
|
|
||
| beforeEach(() => { | ||
| vi.stubGlobal("localStorage", createMockLocalStorage()); | ||
| // The cache is static and shared across instances, so it must be | ||
| // cleared between tests to avoid stale reads. | ||
| (UserSettings as unknown as { cache: Map<string, unknown> }).cache.clear(); | ||
| settings = new UserSettings(); | ||
| }); | ||
|
|
||
| describe("isSoundEffectEnabled / setSoundEffectEnabled", () => { | ||
| it("defaults to enabled when unset", () => { | ||
| expect(settings.isSoundEffectEnabled("atom-launch")).toBe(true); | ||
| }); | ||
|
|
||
| it("returns false after being disabled", () => { | ||
| settings.setSoundEffectEnabled("atom-launch", false); | ||
| expect(settings.isSoundEffectEnabled("atom-launch")).toBe(false); | ||
| }); | ||
|
|
||
| it("returns true after being re-enabled", () => { | ||
| settings.setSoundEffectEnabled("atom-launch", false); | ||
| settings.setSoundEffectEnabled("atom-launch", true); | ||
| expect(settings.isSoundEffectEnabled("atom-launch")).toBe(true); | ||
| }); | ||
|
|
||
| it("persists the value to localStorage", () => { | ||
| settings.setSoundEffectEnabled("ka-ching", false); | ||
| expect(localStorage.getItem("settings.sound.ka-ching")).toBe("false"); | ||
| }); | ||
|
|
||
| it("tracks each effect key independently", () => { | ||
| settings.setSoundEffectEnabled("atom-launch", false); | ||
| expect(settings.isSoundEffectEnabled("atom-launch")).toBe(false); | ||
| expect(settings.isSoundEffectEnabled("mirv-launch")).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe("isFxEnabled / setFxEnabled", () => { | ||
| it("defaults to enabled when unset", () => { | ||
| expect(settings.isFxEnabled("fx-conquest")).toBe(true); | ||
| }); | ||
|
|
||
| it("returns false after being disabled", () => { | ||
| settings.setFxEnabled("fx-conquest", false); | ||
| expect(settings.isFxEnabled("fx-conquest")).toBe(false); | ||
| }); | ||
|
|
||
| it("returns true after being re-enabled", () => { | ||
| settings.setFxEnabled("fx-conquest", false); | ||
| settings.setFxEnabled("fx-conquest", true); | ||
| expect(settings.isFxEnabled("fx-conquest")).toBe(true); | ||
| }); | ||
|
|
||
| it("persists the value to localStorage", () => { | ||
| settings.setFxEnabled("fx-nuke-debris", false); | ||
| expect(localStorage.getItem("settings.fx.fx-nuke-debris")).toBe("false"); | ||
| }); | ||
|
|
||
| it("tracks each effect key independently", () => { | ||
| settings.setFxEnabled("fx-conquest", false); | ||
| expect(settings.isFxEnabled("fx-conquest")).toBe(false); | ||
| expect(settings.isFxEnabled("fx-dust")).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe("gameStartNotificationsEnabled / toggleGameStartNotifications", () => { | ||
| it("defaults to enabled when unset", () => { | ||
| expect(settings.gameStartNotificationsEnabled()).toBe(true); | ||
| }); | ||
|
|
||
| it("flips to false on first toggle", () => { | ||
| settings.toggleGameStartNotifications(); | ||
| expect(settings.gameStartNotificationsEnabled()).toBe(false); | ||
| }); | ||
|
|
||
| it("flips back to true on second toggle", () => { | ||
| settings.toggleGameStartNotifications(); | ||
| settings.toggleGameStartNotifications(); | ||
| expect(settings.gameStartNotificationsEnabled()).toBe(true); | ||
| }); | ||
|
|
||
| it("persists the value to localStorage", () => { | ||
| settings.toggleGameStartNotifications(); | ||
| expect(localStorage.getItem("settings.notifications.gameStart")).toBe( | ||
| "false", | ||
| ); | ||
| }); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Use the shared setup() test helper in this test file.
This suite builds its own harness, but tests under tests/**/*.test.ts should use the shared setup path for consistency with repo testing conventions.
As per coding guidelines: "Use the setup() helper from tests/util/Setup.ts to create test game instances and exercise core simulation directly".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/UserSettings.test.ts` around lines 24 - 114, The test file reimplements
a test harness instead of using the shared setup helper; update the suite to
call the shared setup() helper to create the test environment and replace manual
stubbing/initialization. Specifically, remove the manual
vi.stubGlobal("localStorage", createMockLocalStorage()) and the manual cache
clear on UserSettings, and instead call setup() at the top of the describe block
to obtain the testing context, then instantiate UserSettings via that context or
normally if setup provides globals; ensure all tests still reference the same
UserSettings methods (isSoundEffectEnabled, setSoundEffectEnabled, isFxEnabled,
setFxEnabled, gameStartNotificationsEnabled, toggleGameStartNotifications) and
that localStorage persistence assertions remain valid under the shared setup.
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:
It adds specific submenus for sounds and effects, allowing you to adjust the overall volume of background music and sounds, and to toggle each sound effect individually. The effects submenu works similarly, featuring two master toggles to disable all special effects and alerts, but also allowing you to toggle them individually.
Sound settings (new "Sounds" tab / submenu):
In addition, 3 NEW SOUNDS have been added:
Visual effects settings (new "Effects" tab / submenu):
Other improvements:
NEW FEATURE: NOTIFICATIONS
A new feature has been added to send browser notifications to users. Currently, there is only one notification that alerts you when a game starts, and, due to standard browser notification logic, it is only sent if you are not in the window where the game is open. The game requests permission the first time a user joins a match. If permission is denied, the user can request it again from the browser's notification settings menu, where they can also disable this notification.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
sardidefcon