Conversation
Merge from master
Merge from master
ikostan
added this to the
Milestone 14: Resource-Driven Mechanics & Persistence Refactor milestone
ikostan
added
enhancement
Contributor
Reviewer's GuideIntroduces a dedicated Menu SFX bus and global UI navigation sound, and refactors web audio integration into an AudioWebBridge autoload while updating audio settings UI, Globals, and tests to support the new bus and DOM-sync signaling. Sequence diagram for bidirectional audio settings sync between web DOM, AudioWebBridge, AudioManager, and UIsequenceDiagram
actor WebUser
participant DOM as AudioDOM_elements
participant JSWin as window_JS
participant Bridge as AudioWebBridge
participant AM as AudioManager
participant UI as AudioSettings
rect rgb(240,240,255)
WebUser->>DOM: Drag menu-slider (HTML range)
DOM-->>JSWin: onchange calls changeMenuVolume([value])
JSWin->>Bridge: changeMenuVolume([value])
Bridge->>Bridge: _on_change_menu_volume_js(args)
Bridge->>Bridge: _on_change_volume_js(args BUS_SFX_MENU)
Bridge->>AM: set_volume(BUS_SFX_MENU value)
Bridge->>AM: apply_volume_to_bus(BUS_SFX_MENU value menu_muted)
Bridge->>AM: save_volumes()
end
rect rgb(240,255,240)
AM-->>Bridge: volume_changed(BUS_SFX_MENU value)
Bridge->>DOM: set menu-slider.value = value
end
rect rgb(255,240,240)
AM-->>UI: volume_changed(BUS_SFX_MENU value)
UI->>UI: _on_global_volume_changed(BUS_SFX_MENU value)
UI->>UI: menu_slider.set_value_no_signal(value)
UI->>UI: _update_ui_interactivity()
end
rect rgb(240,240,255)
WebUser->>DOM: Click mute-menu checkbox
DOM-->>JSWin: onchange calls toggleMuteMenu([checked])
JSWin->>Bridge: toggleMuteMenu([checked])
Bridge->>Bridge: _on_toggle_mute_menu_js(args)
Bridge->>Bridge: _on_toggle_mute_js(args BUS_SFX_MENU)
Bridge->>AM: set_muted(BUS_SFX_MENU is_muted)
Bridge->>AM: apply_volume_to_bus(BUS_SFX_MENU menu_volume is_muted)
Bridge->>AM: save_volumes()
end
rect rgb(240,255,240)
AM-->>Bridge: mute_toggled(BUS_SFX_MENU is_muted)
Bridge->>DOM: set mute-menu.checked = not is_muted
end
rect rgb(255,240,240)
AM-->>UI: mute_toggled(BUS_SFX_MENU is_muted)
UI->>UI: _on_global_mute_toggled(BUS_SFX_MENU is_muted)
UI->>UI: mute_menu.set_pressed_no_signal(not is_muted)
UI->>UI: _update_ui_interactivity()
end
Class diagram for AudioManager, AudioWebBridge, Globals, AudioSettings, and AudioConstantsclassDiagram
class AudioManager {
<<Node>>
+signal volume_changed(bus_name String volume float)
+signal mute_toggled(bus_name String is_muted bool)
+master_volume float
+master_muted bool
+music_volume float
+music_muted bool
+sfx_volume float
+sfx_muted bool
+weapon_volume float
+weapon_muted bool
+rotors_volume float
+rotors_muted bool
+menu_volume float
+menu_muted bool
+current_config_path String
+set_bus_state(bus_name String volume float muted bool) void
+get_volume(bus_name String) float
+set_volume(bus_name String vol float) void
+get_muted(bus_name String) bool
+set_muted(bus_name String muted bool) void
+get_bus_state(bus_name String) Dictionary
+apply_volume_to_bus(bus_name String vol float muted bool) void
+load_volumes() void
+save_volumes() void
+reset_volumes() void
}
class AudioConstants {
<<Node or Script>>
+BUS_MASTER String
+BUS_MUSIC String
+BUS_SFX String
+BUS_SFX_ROTORS String
+BUS_SFX_WEAPON String
+BUS_SFX_MENU String
+BUS_CONFIG Dictionary
}
class AudioWebBridge {
<<Node(Auotload)>>
+signal web_back_requested
+signal web_reset_requested
+os_wrapper OSWrapper
+js_bridge_wrapper JavaScriptBridgeWrapper
+js_window Variant
+toggle_dom_visibility(show bool) void
+sync_all_to_dom() void
+restore_options_menu_dom() void
+_ready() void
+_on_godot_volume_changed(bus_name String value float) void
+_on_godot_mute_toggled(bus_name String is_muted bool) void
+_on_change_volume_js(args Array bus_name String) void
+_on_toggle_mute_js(args Array bus_name String) void
+_on_change_master_volume_js(args Array) void
+_on_change_music_volume_js(args Array) void
+_on_change_sfx_volume_js(args Array) void
+_on_change_weapon_volume_js(args Array) void
+_on_change_rotors_volume_js(args Array) void
+_on_change_menu_volume_js(args Array) void
+_on_toggle_mute_master_js(args Array) void
+_on_toggle_mute_music_js(args Array) void
+_on_toggle_mute_sfx_js(args Array) void
+_on_toggle_mute_weapon_js(args Array) void
+_on_toggle_mute_rotors_js(args Array) void
+_on_toggle_mute_menu_js(args Array) void
+_on_audio_back_button_pressed_js(args Array) void
+_on_audio_reset_js(args Array) void
+_register_all_callbacks() void
+_register_js_callback(callback_method String window_property String) Variant
+_validate_volume_args(args Array) float
+_validate_mute_args(args Array) Variant
+_get_slider_id_for_bus(bus_name String) String
+_get_mute_id_for_bus(bus_name String) String
+_sync_all_dom_values() void
}
class AudioSettings {
<<Control>>
+master_warning_shown bool
+sfx_warning_shown bool
+_intentional_exit bool
+master_slider HSlider
+mute_master CheckButton
+music_slider HSlider
+mute_music CheckButton
+sfx_slider HSlider
+mute_sfx CheckButton
+weapon_slider HSlider
+mute_weapon CheckButton
+rotor_slider HSlider
+mute_rotor CheckButton
+menu_slider HSlider
+mute_menu CheckButton
+audio_back_button Button
+audio_reset_button Button
+_ready() void
+_on_back_button_pressed() void
+_on_audio_reset_button_pressed() void
+_sync_ui_from_manager() void
+_sync_all_sliders_and_mutes() void
+_on_global_volume_changed(bus_name String volume float) void
+_on_global_mute_toggled(bus_name String is_muted bool) void
+_update_ui_interactivity() void
+_connect_bus_ui(mute_btn CheckButton slider HSlider mute_callback Callable gui_callback Callable is_muted bool) void
+_on_master_volume_control_gui_input(event InputEvent) void
+_on_music_volume_control_gui_input(event InputEvent) void
+_on_sfx_volume_control_gui_input(event InputEvent) void
+_on_weapon_volume_control_gui_input(event InputEvent) void
+_on_rotor_volume_control_gui_input(event InputEvent) void
+_on_menu_volume_control_gui_input(event InputEvent) void
+_on_master_mute_toggled(toggled_on bool) void
+_on_music_mute_toggled(toggled_on bool) void
+_on_sfx_mute_toggled(toggled_on bool) void
+_on_weapon_mute_toggled(toggled_on bool) void
+_on_rotor_mute_toggled(toggled_on bool) void
+_on_menu_mute_toggled(toggled_on bool) void
+_on_music_mute_gui_input(event InputEvent) void
+_on_sfx_mute_gui_input(event InputEvent) void
+_on_weapon_mute_gui_input(event InputEvent) void
+_on_rotor_mute_gui_input(event InputEvent) void
+_on_menu_mute_gui_input(event InputEvent) void
+_handle_slider_gui_input(event InputEvent master_muted bool sfx_muted bool bus_muted bool mute_button CheckButton master_dialog AcceptDialog sfx_dialog AcceptDialog) void
+_handle_mute_gui_input(event InputEvent master_muted bool sfx_muted bool master_dialog AcceptDialog sfx_dialog AcceptDialog) void
+_update_label_colors() void
+_reset_master_warning_shown() void
+_reset_sfx_warning_shown() void
+_on_tree_exited() void
}
class Globals {
<<Node(Auotload)>>
+UI_NAV_SOUND_PATH String
+settings GameSettingsResource
+previous_scene String
+next_scene String
+options_open bool
+hidden_menus Array
+_ui_nav_stream AudioStream
+_nav_sfx_player AudioStreamPlayer
+_nav_actions Array~String~
+_ready() void
+_on_setting_changed(setting_name String new_value Variant) void
+_load_settings() void
+_save_settings() void
+log_message(message String level LogLevel) void
+_input(event InputEvent) void
+_play_ui_navigation_sfx() void
+ensure_initial_focus(first Control controls Array~Control~ context String) void
+get_game_version() String
+set_game_version_for_tests(value String) void
}
AudioSettings ..> AudioManager : uses
AudioSettings ..> AudioConstants : uses
AudioSettings ..> Globals : uses
AudioSettings ..> AudioWebBridge : optional reference via /root
AudioWebBridge ..> AudioManager : uses signals and setters
AudioWebBridge ..> AudioConstants : uses bus names
AudioManager ..> AudioConstants : uses BUS_* and BUS_CONFIG
Globals ..> AudioConstants : sets AudioStreamPlayer bus
AudioWebBridge ..> OSWrapper : composition
AudioWebBridge ..> JavaScriptBridgeWrapper : composition
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 13 minutes and 54 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds an AudioWebBridge autoload for browser DOM ⇄ Godot audio sync, introduces a dedicated SFX_Menu audio bus and UI controls (slider + mute), moves JS bridge logic out of audio_settings.gd, adds UI navigation SFX playback, and includes tests for the web bridge and menu volume/mute behavior. Changes
Sequence DiagramsequenceDiagram
participant Web as JavaScript (Browser)
participant Bridge as AudioWebBridge
participant Manager as AudioManager
participant Audio as AudioServer
participant UI as AudioSettingsUI
Web->>Bridge: window.changeMenuVolume([0.9])
activate Bridge
Bridge->>Manager: set_volume("SFX_Menu", 0.9)
activate Manager
Manager->>Manager: menu_volume = 0.9
Manager->>Audio: apply volume to "SFX_Menu"
Manager->>Manager: emit volume_changed("SFX_Menu", 0.9)
Manager-->>Bridge: (signal)
deactivate Manager
Bridge->>Web: eval(...) to update DOM slider
deactivate Bridge
Web->>Bridge: window.toggleMuteMenu([true])
activate Bridge
Bridge->>Manager: set_muted("SFX_Menu", true)
activate Manager
Manager->>Manager: menu_muted = true
Manager->>Audio: apply mute to "SFX_Menu"
Manager->>Manager: emit mute_toggled("SFX_Menu", true)
Manager-->>Bridge: (signal)
deactivate Manager
Bridge->>Web: eval(...) to update DOM checkbox
deactivate Bridge
Manager-->>UI: volume_changed / mute_toggled
UI->>UI: update slider/checkbox visuals
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Mar 31, 2026 4:50a.m. | Review ↗ |
| JavaScript | |
Mar 31, 2026 4:50a.m. | Review ↗ |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
_on_menu_volume_control_gui_inputyou passAudioManager.rotors_mutedinto_handle_slider_gui_inputwhile controlling the Menu slider; this looks like a copy‑paste issue and probably should use the menu mute state instead to avoid incorrect lock/enable behavior. - In
globals.gd,_ui_nav_streamis preloaded usingUI_NAV_SOUND_PATHbefore that constant is defined, which can cause a load-time error in GDScript; consider moving theconst UI_NAV_SOUND_PATHdefinition above the preload or moving the preload into_ready(). - The global
_unhandled_inputhandler inglobals.gdwill play the navigation SFX for anyui_*movement anywhere in the game; if this is intended only for menus, consider guarding it based on the active scene or focused UI to avoid SFX triggering during gameplay.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_on_menu_volume_control_gui_input` you pass `AudioManager.rotors_muted` into `_handle_slider_gui_input` while controlling the Menu slider; this looks like a copy‑paste issue and probably should use the menu mute state instead to avoid incorrect lock/enable behavior.
- In `globals.gd`, `_ui_nav_stream` is preloaded using `UI_NAV_SOUND_PATH` before that constant is defined, which can cause a load-time error in GDScript; consider moving the `const UI_NAV_SOUND_PATH` definition above the preload or moving the preload into `_ready()`.
- The global `_unhandled_input` handler in `globals.gd` will play the navigation SFX for any `ui_*` movement anywhere in the game; if this is intended only for menus, consider guarding it based on the active scene or focused UI to avoid SFX triggering during gameplay.
## Individual Comments
### Comment 1
<location path="scripts/audio_settings.gd" line_range="726" />
<code_context>
+ event,
+ AudioManager.master_muted,
+ AudioManager.sfx_muted,
+ AudioManager.rotors_muted,
+ mute_menu,
+ master_warning_dialog,
</code_context>
<issue_to_address>
**issue (bug_risk):** Menu slider mute state should likely use `menu_muted` instead of `rotors_muted`.
In `_on_menu_volume_control_gui_input`, the 4th argument to `_handle_slider_gui_input` is `AudioManager.rotors_muted`. To keep the menu slider independent of the rotors bus and consistent with the other sub-buses, this should be `AudioManager.menu_muted` instead.
</issue_to_address>
### Comment 2
<location path="scripts/globals.gd" line_range="39-42" />
<code_context>
var _is_loading_settings: bool = false # Guard flag
+## Preloaded stream to prevent disk I/O lag during fast menu navigation.
+var _ui_nav_stream: AudioStream = preload(UI_NAV_SOUND_PATH)
+
+## Path to the navigation sound file
+const UI_NAV_SOUND_PATH: String = "res://files/sounds/sfx/ui_navigation.wav"
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Preload uses `UI_NAV_SOUND_PATH` before the constant is defined, which will break in GDScript.
Because GDScript evaluates top-to-bottom, `preload(UI_NAV_SOUND_PATH)` runs before `UI_NAV_SOUND_PATH` exists and will error. Fix by moving `const UI_NAV_SOUND_PATH` above `_ui_nav_stream`, or by inlining the path string directly in `preload()`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This was
linked to
issues
Mar 22, 2026
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/audio_manager.gd (1)
72-88:⚠️ Potential issue | 🟠 MajorRefactor the bus getters before adding another bus case.
Adding
SFX_Menupushes both functions past gdlint’smax-returnslimit, and CI is already red.AudioConstants.BUS_CONFIGalready carries the backing property names, so both lookups can be reduced to a single guarded return path.♻️ One way to collapse both functions
func get_volume(bus_name: String) -> float: - match bus_name: - AudioConstants.BUS_MASTER: - return master_volume - AudioConstants.BUS_MUSIC: - return music_volume - AudioConstants.BUS_SFX: - return sfx_volume - AudioConstants.BUS_SFX_WEAPON: - return weapon_volume - AudioConstants.BUS_SFX_ROTORS: - return rotors_volume - AudioConstants.BUS_SFX_MENU: - return menu_volume - _: - Globals.log_message("Unknown bus for get_volume: " + bus_name, Globals.LogLevel.WARNING) - return 0.0 + if not AudioConstants.BUS_CONFIG.has(bus_name): + Globals.log_message("Unknown bus for get_volume: " + bus_name, Globals.LogLevel.WARNING) + return 0.0 + return float(get(AudioConstants.BUS_CONFIG[bus_name]["volume_var"])) @@ func get_muted(bus_name: String) -> bool: - match bus_name: - AudioConstants.BUS_MASTER: - return master_muted - AudioConstants.BUS_MUSIC: - return music_muted - AudioConstants.BUS_SFX: - return sfx_muted - AudioConstants.BUS_SFX_WEAPON: - return weapon_muted - AudioConstants.BUS_SFX_ROTORS: - return rotors_muted - AudioConstants.BUS_SFX_MENU: - return menu_muted - _: - Globals.log_message("Unknown bus for get_muted: " + bus_name, Globals.LogLevel.WARNING) - return false + if not AudioConstants.BUS_CONFIG.has(bus_name): + Globals.log_message("Unknown bus for get_muted: " + bus_name, Globals.LogLevel.WARNING) + return false + return bool(get(AudioConstants.BUS_CONFIG[bus_name]["muted_var"]))Also applies to: 143-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/audio_manager.gd` around lines 72 - 88, Refactor get_volume (and the sibling setter at lines 143-159) to avoid multiple return points by using AudioConstants.BUS_CONFIG (which maps bus names to backing property names) to resolve the target property, then perform a single guarded return: look up AudioConstants.BUS_CONFIG[bus_name], if missing log the warning via Globals.log_message and return 0.0 (or appropriate default), otherwise read the resolved property (master_volume, music_volume, etc.) dynamically and return it; apply the same collapse pattern to the corresponding setter so both functions have a single exit and comply with gdlint max-returns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scenes/audio_settings.tscn`:
- Around line 156-158: The focus chain is skipping the new SFXMenu row; update
focus properties so keyboard/gamepad can reach it: in the SFXRotors node change
focus_neighbor_bottom and focus_next to "SFXMenu" instead of "AudioBackButton";
add focus_neighbor_top="SFXRotors", focus_neighbor_bottom="AudioBackButton",
focus_next="AudioBackButton" and focus_previous="SFXRotors" to the SFXMenu node;
and update AudioBackButton to set focus_neighbor_top="SFXMenu" (was
"SFXRotors"). Ensure the node names match exactly: SFXRotors, SFXMenu,
AudioBackButton.
In `@scenes/volume_controls/sfx_menu_volume_control.tscn`:
- Line 21: Two tooltip_text properties in sfx_menu_volume_control.tscn still say
"SFX Rotors Volume Control"; update both occurrences of tooltip_text (the ones
currently set to "SFX Rotors Volume Control") to the correct label, e.g. "SFX
Menu Volume Control" (or simply "SFX Volume Control") so the menu row
tooltip/accessibility text matches this control.
In `@scripts/audio_settings.gd`:
- Line 80: The _update_label_colors() function currently updates labels up to
rotor_label but omits the new menu_label; update that helper so it also sets
menu_label's modulate/color based on the same focus logic (same branch used for
rotor_label), referencing the onready var menu_label and the
_update_label_colors() function to locate and add the assignment so the Menu row
gets the same highlight behavior as the other volume rows.
- Around line 719-796: The file exceeds max-file-lines due to duplicated per-bus
blocks (see _on_menu_volume_control_gui_input, _on_menu_mute_toggled,
_on_menu_mute_gui_input, _on_change_menu_volume_js, _on_toggle_mute_menu_js and
variables mute_menu/menu_slider), so refactor by extracting a shared helper and
a small bus-config data structure: create a reusable
register_bus_handlers(bus_id, ui_nodes, warning_dialogs, slider_props) that
wraps calls to _handle_slider_gui_input/_handle_mute_gui_input, sets/get
toggles, calls AudioManager.apply_volume_to_bus, AudioManager.save_volumes,
menu_slider.set_value_no_signal, and uses
_validate_volume_args/_validate_mute_args; then replace the menu-specific blocks
with a single invocation of register_bus_handlers using
AudioConstants.BUS_SFX_MENU and the existing UI node references to remove the
near-duplicate code.
- Around line 721-730: In _on_menu_volume_control_gui_input replace the
incorrect rotor mute flag with the menu mute flag: call
_handle_slider_gui_input(event, AudioManager.master_muted,
AudioManager.sfx_muted, AudioManager.menu_muted, mute_menu,
master_warning_dialog, sfx_warning_dialog) so the Menu slider uses
AudioManager.menu_muted instead of AudioManager.rotors_muted to determine its
mute state.
In `@scripts/globals.gd`:
- Around line 346-358: The _unhandled_input handler currently plays navigation
SFX for ui_* actions even during gameplay; modify Globals._unhandled_input to
first check whether there is a visible GUI focus owner before calling
_play_ui_navigation_sfx (e.g., verify get_tree().get_gui_focus_owner() is
non-null and is_visible_in_tree or otherwise confirm a control has
focus/visibility). Update the guard logic in _unhandled_input so
_play_ui_navigation_sfx() is only invoked when a focused/visible UI control
exists.
---
Outside diff comments:
In `@scripts/audio_manager.gd`:
- Around line 72-88: Refactor get_volume (and the sibling setter at lines
143-159) to avoid multiple return points by using AudioConstants.BUS_CONFIG
(which maps bus names to backing property names) to resolve the target property,
then perform a single guarded return: look up
AudioConstants.BUS_CONFIG[bus_name], if missing log the warning via
Globals.log_message and return 0.0 (or appropriate default), otherwise read the
resolved property (master_volume, music_volume, etc.) dynamically and return it;
apply the same collapse pattern to the corresponding setter so both functions
have a single exit and comply with gdlint max-returns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6b461682-4a6f-442b-99f5-f87ce39dab0f
📒 Files selected for processing (8)
custom_shell.htmldefault_bus_layout.tresscenes/audio_settings.tscnscenes/volume_controls/sfx_menu_volume_control.tscnscripts/audio_constants.gdscripts/audio_manager.gdscripts/audio_settings.gdscripts/globals.gd
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: Pull Request Pipeline
scripts/audio_manager.gd
[error] 88-88: gdlint: Function "get_volume" has more than 6 return statements (max-returns)
[error] 159-159: gdlint: Function "get_muted" has more than 6 return statements (max-returns)
scripts/globals.gd
[error] 42-42: gdlint: Definition out of order in global scope (class-definitions-order)
scripts/audio_settings.gd
[error] 1123-1123: gdlint: Max allowed file lines num (1000) exceeded (max-file-lines)
🔇 Additional comments (1)
scripts/globals.gd (1)
38-42: This review comment is incorrect. The code is already in the correct order.The
UI_NAV_SOUND_PATHconstant is declared on line 12, and the_ui_nav_streamvariable that uses it is declared on line 32. The const-before-var ordering is already correct, which is why thepreload(UI_NAV_SOUND_PATH)call on line 32 works without issue. There is noclass-definitions-orderlint failure in the current code.> Likely an incorrect or invalid review comment.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/audio_settings.gd (1)
768-769: Fix misleading comment.The comment on line 768 says "sets rotors_volume" but this function sets menu volume.
Proposed fix
- # Update AudioManager (sets rotors_volume) + # Update AudioManager (sets menu_volume) AudioManager.set_volume(AudioConstants.BUS_SFX_MENU, value)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/audio_settings.gd` around lines 768 - 769, The inline comment above the call to AudioManager.set_volume with AudioConstants.BUS_SFX_MENU is incorrect ("sets rotors_volume"); update that comment to accurately describe the call (e.g., "sets menu volume" or "set SFX menu bus volume") or remove it if redundant so the comment matches the function's behavior. Ensure the modified comment sits immediately above the AudioManager.set_volume(AudioConstants.BUS_SFX_MENU, value) call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/audio_settings.gd`:
- Around line 768-769: The inline comment above the call to
AudioManager.set_volume with AudioConstants.BUS_SFX_MENU is incorrect ("sets
rotors_volume"); update that comment to accurately describe the call (e.g.,
"sets menu volume" or "set SFX menu bus volume") or remove it if redundant so
the comment matches the function's behavior. Ensure the modified comment sits
immediately above the AudioManager.set_volume(AudioConstants.BUS_SFX_MENU,
value) call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d21d138a-2116-405c-99b7-86d6c6c9bdeb
📒 Files selected for processing (3)
scripts/audio_manager.gdscripts/audio_settings.gdscripts/globals.gd
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/audio_manager.gd
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: Pull Request Pipeline
scripts/audio_settings.gd
[error] 1118-1118: gdlint error: Max allowed file lines num (1000) exceeded (max-file-lines). Failure: 1 problem found.
🔇 Additional comments (10)
scripts/globals.gd (3)
11-12: LGTM!Preloading the navigation sound at startup is the correct approach to avoid disk I/O lag during fast menu navigation.
Also applies to: 31-33
336-348: Guard SFX playback to prevent menu navigation sounds during gameplay.The
_unhandled_inputhandler will receiveui_*actions during gameplay because neitherplayer.gdnormain_scene.gdconsume these actions. Navigation SFX will leak outside menus when no control has focus.Proposed fix
func _unhandled_input(event: InputEvent) -> void: + var focus_owner: Control = get_viewport().gui_get_focus_owner() + if not is_instance_valid(focus_owner) or not focus_owner.is_visible_in_tree(): + return + # Target Actions: ui_up, ui_down, ui_left, ui_right, ui_focus_next, ui_focus_prev. if ( event.is_action_pressed("ui_up")[raise_major_issue, duplicate_comment]
351-365: LGTM!The one-shot sound pattern with automatic cleanup via
finished.connect(sfx_player.queue_free)is appropriate. Routing throughBUS_SFX_MENUcorrectly respects user's menu volume settings.scripts/audio_settings.gd (7)
35-35: LGTM!Menu control declarations follow the established pattern for other SFX sub-buses.
Also applies to: 42-42, 61-63, 75-75
150-158: LGTM!Menu control initialization follows the established pattern with proper signal connection guards and JS callback registration.
Also applies to: 197-199, 215-217, 234-235
246-259: Includemenu_labelin_update_label_colors().The
menu_labelis declared at line 75 but is not included in this function, so the Menu row will never receive the same focus highlight as other volume rows.Proposed fix
rotor_label.modulate = yellow if (rotor_slider.has_focus() or mute_rotor.has_focus()) else white + menu_label.modulate = yellow if (menu_slider.has_focus() or mute_menu.has_focus()) else white[raise_minor_issue, duplicate_comment]
280-282: LGTM!DOM synchronization for menu slider and mute checkbox follows the established pattern.
Also applies to: 310-312
716-725: Usemenu_mutedhere, notrotors_muted.Line 721 passes
AudioManager.rotors_mutedas thebus_mutedparameter. This causes the Menu slider to auto-unmute based on the Rotor mute state instead of its own mute flag.Proposed fix
func _on_menu_volume_control_gui_input(event: InputEvent) -> void: _handle_slider_gui_input( event, AudioManager.master_muted, AudioManager.sfx_muted, - AudioManager.rotors_muted, + AudioManager.menu_muted, mute_menu, master_warning_dialog, sfx_warning_dialog )[raise_major_issue, duplicate_comment]
831-833: LGTM!Menu controls correctly follow the SFX-dependent enable/disable pattern, matching weapon and rotor behavior.
711-795: File exceeds max-file-lines lint limit (1000 lines).The pipeline is failing due to this file having 1118 lines. The menu-specific handlers add another near-identical block that increases code duplication.
Consider extracting shared per-bus registration/handler logic into a reusable helper and a small bus-config data structure. This would consolidate the repetitive patterns for master, music, SFX, weapon, rotors, and menu into a single parameterized implementation.
As a short-term fix to unblock CI, you could extract the new menu handlers into a separate partial class or helper script.
[raise_major_issue, duplicate_comment]
… instead of rotors_muted. In _on_menu_volume_control_gui_input, the 4th argument to _handle_slider_gui_input is AudioManager.rotors_muted. To keep the menu slider independent of the rotors bus and consistent with the other sub-buses, this should be AudioManager.menu_muted instead.
These still announce “Rotors”, so the new control exposes the wrong label in tooltip/accessibility text.
The new row is wired here, but the color-update helper still stops at rotor_label, so Menu never gets the same focus highlight as the other volume rows.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/audio_settings.gd (1)
150-157: 🛠️ Refactor suggestion | 🟠 MajorPlease extract the new Menu bus plumbing behind shared helpers before merge.
This adds another full copy of the init/sync/JS-handler/cleanup path, and CI is still failing
gdlintonmax-file-linesfor this file. A small per-bus config plus shared helpers would remove most of these new hunks and clear the lint gate.Also applies to: 197-216, 281-313, 715-792, 1045-1047, 1075-1081, 1111-1117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/audio_settings.gd` around lines 150 - 157, The Menu bus plumbing duplicates init/sync/JS-handler/cleanup logic (connecting toggled/gui_input and setting button_pressed) and should be refactored into shared helpers; create a generic helper (e.g., _init_bus_controls or _setup_bus_plumbing) that takes the bus identifier and relevant nodes/handlers (mute_menu, menu_slider, _on_menu_mute_toggled, _on_menu_volume_control_gui_input, _on_menu_mute_gui_input) and performs the connect checks and initial state assignment (using AudioManager.menu_muted for initial button_pressed), then replace the repeated blocks (including the other ranges noted) to call that helper and implement corresponding teardown/sync helpers for cleanup and JS-handler wiring.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/audio_settings.gd`:
- Around line 759-792: The web Menu callbacks don't mirror the native
parent-mute guard: in _on_change_menu_volume_js, when returning early due to
Master or SFX being muted, call _sync_dom_ui() (and leave UI slider unchanged)
before returning so the DOM stays in sync; in _on_toggle_mute_menu_js, check
AudioManager.get_muted(AudioConstants.BUS_MASTER) and
AudioManager.get_muted(AudioConstants.BUS_SFX) and if either is true, reset
mute_menu.button_pressed to the actual Menu bus state (do not apply the toggle),
call _sync_dom_ui(), and return early so the web overlay cannot drift out of
sync with the native gui_input path.
---
Duplicate comments:
In `@scripts/audio_settings.gd`:
- Around line 150-157: The Menu bus plumbing duplicates
init/sync/JS-handler/cleanup logic (connecting toggled/gui_input and setting
button_pressed) and should be refactored into shared helpers; create a generic
helper (e.g., _init_bus_controls or _setup_bus_plumbing) that takes the bus
identifier and relevant nodes/handlers (mute_menu, menu_slider,
_on_menu_mute_toggled, _on_menu_volume_control_gui_input,
_on_menu_mute_gui_input) and performs the connect checks and initial state
assignment (using AudioManager.menu_muted for initial button_pressed), then
replace the repeated blocks (including the other ranges noted) to call that
helper and implement corresponding teardown/sync helpers for cleanup and
JS-handler wiring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 99402d20-f116-466a-bcb4-6681a1daa1db
📒 Files selected for processing (2)
scenes/volume_controls/sfx_menu_volume_control.tscnscripts/audio_settings.gd
✅ Files skipped from review due to trivial changes (1)
- scenes/volume_controls/sfx_menu_volume_control.tscn
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: Pull Request Pipeline
scripts/audio_settings.gd
[error] 1119-1119: gdlint failed: Max allowed file lines num (1000) exceeded (max-file-lines). Failure: 1 problem found.
In `@scripts/audio_settings.gd` around lines 190 - 193, The code is incorrectly
chaining three call_deferred calls on start_button which delays grab_focus by
three frames; replace the triple-nested call_deferred with a single deferred
invocation that calls grab_focus once (e.g., call_deferred("grab_focus") on the
start_button) while keeping the is_instance_valid(start_button) check and the
prev_menu/Panel logic intact so focus is set on the StartButton in the next idle
frame.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Mute toggle handler lacks parent-mute guard. Unlike volume changes which block when parent buses are muted, _on_toggle_mute_js allows mute toggles on child buses even when Master or SFX is muted. This creates an inconsistency with the native UI path in audio_settings.gd where _handle_mute_gui_input blocks such interactions and shows a warning dialog.
Owner
Author
|
@sourcery-ai resolve |
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
Globals._play_ui_navigation_sfx()you log every navigation sound play; given this is triggered on high‑frequency UI input, consider downgrading or gating that log (e.g., behind a debug flag) to avoid noisy logs in normal gameplay. - In
audio_settings.gdyou repeatedly callget_node_or_null("/root/AudioWebBridge")in several methods (_ready,_on_back_button_pressed,_on_audio_reset_button_pressed); caching this reference once in_ready(with a nullable member) would simplify the code and avoid repeated tree lookups.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Globals._play_ui_navigation_sfx()` you log every navigation sound play; given this is triggered on high‑frequency UI input, consider downgrading or gating that log (e.g., behind a debug flag) to avoid noisy logs in normal gameplay.
- In `audio_settings.gd` you repeatedly call `get_node_or_null("/root/AudioWebBridge")` in several methods (`_ready`, `_on_back_button_pressed`, `_on_audio_reset_button_pressed`); caching this reference once in `_ready` (with a nullable member) would simplify the code and avoid repeated tree lookups.
## Individual Comments
### Comment 1
<location path="scripts/audio_settings.gd" line_range="111-116" />
<code_context>
+ )
+
+ # Connect specific mute button warning interceptors
+ mute_music.gui_input.connect(_on_music_mute_gui_input)
+ mute_sfx.gui_input.connect(_on_sfx_mute_gui_input)
+ mute_weapon.gui_input.connect(_on_weapon_mute_gui_input)
+ mute_rotor.gui_input.connect(_on_rotor_mute_gui_input)
+ mute_menu.gui_input.connect(_on_menu_mute_gui_input)
+
+ # Buttons
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Guard the new gui_input connections to avoid duplicate signal hookups on re-entrance.
The new `gui_input` connections for the mute buttons aren’t guarded with `if not X.is_connected(...)` like the other `_ready` connections. If the scene is removed/re-added or `_ready` runs multiple times, these handlers may be connected repeatedly and fire multiple times. Please wrap these `gui_input.connect(...)` calls in the same `is_connected` guard for consistency and to avoid duplicate signal handling.
```suggestion
# Connect specific mute button warning interceptors
if not mute_music.gui_input.is_connected(_on_music_mute_gui_input):
mute_music.gui_input.connect(_on_music_mute_gui_input)
if not mute_sfx.gui_input.is_connected(_on_sfx_mute_gui_input):
mute_sfx.gui_input.connect(_on_sfx_mute_gui_input)
if not mute_weapon.gui_input.is_connected(_on_weapon_mute_gui_input):
mute_weapon.gui_input.connect(_on_weapon_mute_gui_input)
if not mute_rotor.gui_input.is_connected(_on_rotor_mute_gui_input):
mute_rotor.gui_input.connect(_on_rotor_mute_gui_input)
if not mute_menu.gui_input.is_connected(_on_menu_mute_gui_input):
mute_menu.gui_input.connect(_on_menu_mute_gui_input)
```
</issue_to_address>
### Comment 2
<location path="scripts/audio_settings.gd" line_range="151-152" />
<code_context>
Globals.ensure_initial_focus(master_slider, menu_controls, "Audio Settings")
+ # 1. Listen for changes coming from Playwright/Web
+ AudioManager.volume_changed.connect(_on_global_volume_changed)
+ AudioManager.mute_toggled.connect(_on_global_mute_toggled)
+
+ # Apply the hierarchy locks immediately when the menu opens
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider disconnecting AudioManager signals on exit to avoid callbacks into freed nodes.
These connections in `_ready` have no matching disconnect in `_on_tree_exited` or before `queue_free()`. If this node is freed while `AudioManager` remains, later signal emissions will still try to call into it (Godot will warn and drop the call). Please explicitly `disconnect` (guarded with `is_connected`) during teardown to avoid noisy callbacks into freed objects.
</issue_to_address>
### Comment 3
<location path="scripts/globals.gd" line_range="382-383" />
<code_context>
+
+ # If the sound is already playing (e.g., from rapid button presses),
+ # restart it from the beginning to feel responsive.
+ _nav_sfx_player.play()
+ log_message("Playing UI nav sound...", LogLevel.DEBUG)
</code_context>
<issue_to_address>
**suggestion (performance):** The per-navigation DEBUG log can be very noisy; consider gating or removing it.
This now logs on every navigation event, which will flood DEBUG output in fast keyboard/controller navigation and can slow CI logs. If you still need this visibility, consider logging only on error/exception paths (e.g., invalid player) or adding a simple rate limit/boolean flag so it doesn’t emit on every single nav event.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
… to avoid callbacks into freed nodes. issue (bug_risk): Consider disconnecting AudioManager signals on exit to avoid callbacks into freed nodes. These connections in _ready have no matching disconnect in _on_tree_exited or before queue_free(). If this node is freed while AudioManager remains, later signal emissions will still try to call into it (Godot will warn and drop the call). Please explicitly disconnect (guarded with is_connected) during teardown to avoid noisy callbacks into freed objects.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/audio_settings.gd (1)
513-517: Reset path does redundant full-state sync.
_sync_ui_from_manager()already updates sliders/mutes + interactivity. Calling_sync_all_sliders_and_mutes()immediately after duplicates the same work and repeatedly re-runs interactivity updates.Also applies to: 525-550
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/audio_settings.gd` around lines 513 - 517, The reset routine calls _sync_ui_from_manager() which already updates sliders, mutes, and interactivity, so remove the redundant _sync_all_sliders_and_mutes() and any duplicate _update_ui_interactivity() calls after it; update the reset handler(s) that currently call the sequence (_sync_ui_from_manager(), _sync_all_sliders_and_mutes(), _update_ui_interactivity())—including the similar block around the 525-550 region—to only call _sync_ui_from_manager(), ensuring no duplicate UI-sync or interactivity updates remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/audio_settings.gd`:
- Around line 296-299: In _connect_bus_ui(), avoid assigning
mute_btn.button_pressed directly because it emits the toggled signal and invokes
mute_callback during initialization; instead call
mute_btn.set_pressed_no_signal(not is_muted) to set the button state without
emitting signals, and keep the existing toggled.connect(mute_callback) logic;
ensure this change is made near the current
mute_btn.toggled.connect(mute_callback) line and leave the slider.gui_input/
gui_callback connections unchanged.
---
Nitpick comments:
In `@scripts/audio_settings.gd`:
- Around line 513-517: The reset routine calls _sync_ui_from_manager() which
already updates sliders, mutes, and interactivity, so remove the redundant
_sync_all_sliders_and_mutes() and any duplicate _update_ui_interactivity() calls
after it; update the reset handler(s) that currently call the sequence
(_sync_ui_from_manager(), _sync_all_sliders_and_mutes(),
_update_ui_interactivity())—including the similar block around the 525-550
region—to only call _sync_ui_from_manager(), ensuring no duplicate UI-sync or
interactivity updates remain.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ab9863c7-f4a3-4e1d-a7a8-7a71f9c730a8
📒 Files selected for processing (3)
scripts/audio_settings.gdscripts/audio_web_bridge.gdscripts/globals.gd
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Browser Functional Tests / test
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-02-25T05:50:14.510Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 429
File: scripts/input_remap_button.gd:158-162
Timestamp: 2026-02-25T05:50:14.510Z
Learning: In `scripts/input_remap_button.gd`, the keyboard event copying logic (around lines 158-162) intentionally omits `meta_pressed` when transferring modifier states to new_event. Only `shift_pressed`, `ctrl_pressed`, and `alt_pressed` are copied. This is by design.
Applied to files:
scripts/globals.gd
📚 Learning: 2026-02-25T05:46:55.056Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 429
File: scripts/settings.gd:593-601
Timestamp: 2026-02-25T05:46:55.056Z
Learning: In `scripts/settings.gd`, the `events_match` function intentionally omits `meta_pressed` comparison for InputEventKey events. It only compares `physical_keycode`, `shift_pressed`, `ctrl_pressed`, and `alt_pressed`. This is by design.
Applied to files:
scripts/globals.gd
🔇 Additional comments (8)
scripts/globals.gd (5)
11-12: LGTM!Clean constant definition for the navigation sound resource path.
31-40: LGTM!Preloading the audio stream and using a persistent player are good practices that avoid disk I/O lag and node churn during rapid menu navigation.
43-51: LGTM!The
PROCESS_MODE_ALWAYScorrectly addresses the autoload sleep bug mentioned in the PR objectives, ensuring input handling continues during paused states. The SFX player initialization is clean and properly routes audio through the dedicatedSFX_Menubus.
353-372: LGTM!The implementation correctly:
- Uses
_inputto catch navigation events before UI control focus traversal consumes them- Guards playback with a comprehensive menu context check covering pause state, options visibility, hidden menu stack, and focus ownership
- Applies
Input.is_action_just_pressed()for proper debouncing across keyboard and joypad inputsThis addresses the prior feedback about gameplay sound leakage and joypad echo handling.
375-383: LGTM!The helper method has appropriate defensive null-checking and the restart behavior on rapid presses provides responsive feedback. DEBUG-level logging is suitable for this high-frequency event.
scripts/audio_web_bridge.gd (2)
37-58: Web-only autoload bootstrap is correctly guarded.Good use of early
queue_free()on non-web and explicitwindowinterface validation before callback wiring.
185-214: Parent-mute guards now keep DOM and engine state consistent.The early-return branches correctly revert slider/checkbox DOM state to current
AudioManagervalues when changes are blocked.Also applies to: 228-260
scripts/audio_settings.gd (1)
177-216: Unified back-navigation flow looks consistent now.Nice consolidation: DOM hide, previous menu restore, focus restoration, and scene fallback are handled in one path.
…isy; consider gating or removing it. suggestion (performance): The per-navigation DEBUG log can be very noisy; consider gating or removing it. This now logs on every navigation event, which will flood DEBUG output in fast keyboard/controller navigation and can slow CI logs. If you still need this visibility, consider logging only on error/exception paths (e.g., invalid player) or adding a simple rate limit/boolean flag so it doesn’t emit on every single nav event.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Owner
Author
|
@sourcery-ai guide |
Owner
Author
|
@coderabbitai resolve |
Contributor
✅ Actions performedComments resolved. Approval is disabled; enable |
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
This was referenced May 22, 2026
This was referenced Jun 27, 2026
8 tasks
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.
name: Default Pull Request Template
about: Suggesting changes to SkyLockAssault
title: ''
labels: ''
assignees: ''
Description
What does this PR do? (e.g., "Fixes player jump physics in level 2" or "Adds
new enemy AI script")
Related Issue
Closes #ISSUE_NUMBER (if applicable)
Changes
system")
Testing
works on Win10 with 60 FPS")
Checklist
Additional Notes
Anything else? (e.g., "Tested on Win10 64-bit; needs Linux validation")
🤖 Bot & AI Contributions
This pull request utilized several AI-driven tools to ensure code quality, automate documentation, and provide actionable feedback during the development of the audio system refactor.
Summary of Activity
audio_settings.gdandtest_audio_settings.gdfiles.AudioWebBridge), and testing infrastructure.AI Contributors
The following automated entities contributed to the successful completion of this PR:
call_deferredchains), and suggested specific GDScript fixes.Summary by Sourcery
Add a dedicated Menu SFX audio bus and UI controls, and hook up global UI navigation sounds to that bus.
New Features:
Enhancements:
Summary by Sourcery
Add a dedicated menu SFX bus and global UI navigation sound, while decoupling web audio bridging into a new autoload and updating audio settings UI and tests accordingly.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Audio
UI
Tests