Centralize UI SFX mapping and tighten global input audio guards#784
Merged
ikostan merged 42 commits intoJun 30, 2026
Conversation
Add a focus gate check in Globals._input() to prevent navigation SFX from playing when no GUI element has focus. This ensures audio only triggers during active UI interaction. Adds comprehensive audio integration tests covering mute state propagation, focus-gated navigation audio, and hardware cutoff safety windows.
ikostan
added this to the
Milestone 21: Implement HUD FPS Counter UI Component milestone
20 tasks
Contributor
Reviewer's GuideCentralizes UI SFX routing via data-driven mappings and asset resolution, refactors global input handling to add device tracking and stricter UI audio guards, and adds/updates GUT integration suites around audio buses, mute hierarchy, sliders, button hooks, and ui_accept/ui_cancel behavior while removing a legacy test runner script. Sequence diagram for centralized UI SFX routing and input guardssequenceDiagram
actor Player
participant Globals
participant AudioManager
Player->>Globals: _input(event)
Globals->>Globals: _track_input_device(event)
Globals->>Globals: _check_menu_context(ui_has_focus)
alt [event is InputEventMouseMotion or not event.is_action_type()]
Globals-->>Player: return
else [valid menu context]
Globals->>Globals: _process_ui_navigation_sfx(event, focus_owner, ui_has_focus)
alt [event.is_action_pressed ui_cancel]
Globals->>Globals: _handle_ui_cancel_action(focus_owner, ui_cancel)
else [event.is_action_pressed ui_accept]
Globals->>Globals: _handle_ui_navigation_action(ui_accept, focus_owner, ui_has_focus)
else [directional or focus navigation]
Globals->>Globals: _handle_ui_navigation_action(action, focus_owner, ui_has_focus)
end
Globals->>AudioManager: play_sfx(logical_id, AudioConstants.BUS_SFX)
activate AudioManager
AudioManager->>AudioManager: play_sfx(sfx_name, bus_name, pitch_scale, volume_db)
alt [sfx_name in AudioConstants.SFX_ASSET_MAP]
AudioManager->>AudioManager: resolve file_name from SFX_ASSET_MAP
else [legacy .wav fallback]
AudioManager->>AudioManager: file_name = sfx_name + .wav
end
AudioManager-->>Player: SFX played
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This was
linked to
issues
Jun 28, 2026
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds data-driven UI SFX routing in ChangesUI audio controls and tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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 left some high level feedback:
- The strict
ui_has_focusgate inGlobals._inputwill block navigation sounds whenever that flag is not accurately maintained, so double-check that all UI focus changes consistently updateui_has_focusor consider deriving focus state directly fromget_focus_owner()instead of a separate boolean. - Several integration tests depend on concrete scene paths (
audio_settings.tscn) and real AudioServer bus names; to reduce brittleness across environments, consider adding guard checks or small helper wrappers that fail fast when the scene or bus is missing rather than relying on hardcoded paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The strict `ui_has_focus` gate in `Globals._input` will block navigation sounds whenever that flag is not accurately maintained, so double-check that all UI focus changes consistently update `ui_has_focus` or consider deriving focus state directly from `get_focus_owner()` instead of a separate boolean.
- Several integration tests depend on concrete scene paths (`audio_settings.tscn`) and real AudioServer bus names; to reduce brittleness across environments, consider adding guard checks or small helper wrappers that fail fast when the scene or bus is missing rather than relying on hardcoded paths.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Jun 29, 2026 4:19a.m. | Review ↗ |
| JavaScript | |
Jun 29, 2026 4:19a.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/core/globals.gd (1)
432-444: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winThis pushes
_input()over the repo’smax-returnslimit.CI is already failing on this block, so the PR cannot merge as-is. Because nothing runs after this loop, the loop-local
returns can bebreaks without changing behavior.Suggested refactor
for action: String in _nav_actions: if event.is_action_pressed(action, false): # STRICT FOCUS GATE: Navigation sounds strictly require an active GUI focus owner if not ui_has_focus: - return + break # Prevent double-audio when adjusting sliders. if focus_owner is Slider and (action == "ui_left" or action == "ui_right"): - return + break _play_ui_navigation_sfx() - return # Exit once sound is triggered to avoid double-plays + break # Exit once sound is triggered to avoid double-plays🤖 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 `@scripts/core/globals.gd` around lines 432 - 444, The _input navigation-sound block is using multiple early returns inside the _nav_actions loop, pushing the _input() function over the max-returns limit. In the loop over _nav_actions, replace the loop-local returns that exit after ui_has_focus, Slider handling, and _play_ui_navigation_sfx() with breaks where the function should stop processing this event, while preserving the same control flow and keeping the final sound-trigger path in _input() unchanged.Source: Pipeline failures
🤖 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 `@test/gut/test_audio_integration.gd`:
- Around line 95-131: Scenario C’s “no GUI focus” setup is flaky because
Globals._input() uses the viewport-wide focus owner, not just the local HSlider.
Update this test to explicitly clear and assert
get_viewport().gui_get_focus_owner() before firing the ui_down event, and keep
the focus-owner check near Globals._input() in test_audio_integration.gd. Also
move the Globals.options_open reset out of the test body into shared cleanup so
it cannot leak if an assertion fails.
---
Outside diff comments:
In `@scripts/core/globals.gd`:
- Around line 432-444: The _input navigation-sound block is using multiple early
returns inside the _nav_actions loop, pushing the _input() function over the
max-returns limit. In the loop over _nav_actions, replace the loop-local returns
that exit after ui_has_focus, Slider handling, and _play_ui_navigation_sfx()
with breaks where the function should stop processing this event, while
preserving the same control flow and keeping the final sound-trigger path in
_input() unchanged.
🪄 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: a23d7c26-67d7-4c89-a6ff-fe49eaccc8d0
📒 Files selected for processing (4)
scripts/core/globals.gdtest/gut/test_audio_integration.gdtest/gut/test_audio_integration.gd.uidworkspace/run_unit_tests.sh
💤 Files with no reviewable changes (1)
- workspace/run_unit_tests.sh
📜 Review details
⚠️ CI failures not shown inline (1)
GitHub Actions: Pull Request Pipeline / GDScript Lint and Format Check _ Lint and Format Check: UI audio logic unit tests gut volume mute integration
Conclusion: failure
##[group]Run gdlint ./scripts
�[36;1mgdlint ./scripts�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.14.6/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.14.6/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.14.6/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.14.6/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.14.6/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.14.6/x64/lib
##[endgroup]
./scripts/core/globals.gd:444: Error: Function "_input" has more than 6 return statements (max-returns)
Failure: 1 problem found
##[error]Process completed with exit code 1.
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-06-22T05:17:36.437Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 762
File: scripts/core/globals.gd:408-409
Timestamp: 2026-06-22T05:17:36.437Z
Learning: For Godot 4 GDScript, prefer the event-driven form inside `_input(event)` handlers: use `event.is_action_pressed(action: StringName, allow_echo: bool = false, exact_match: bool = false)` with the default `allow_echo` (i.e., pass `false`) to suppress echo/key-repeat at the engine level so the action behaves like “just pressed”. Prefer `event.is_action_pressed("action", false)` over polling `Input.is_action_just_pressed()` for architectural/performance reasons, and avoid review suggestions that recommend replacing one with the other.
Applied to files:
scripts/core/globals.gdtest/gut/test_audio_integration.gd
📚 Learning: 2026-06-26T03:15:29.611Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 782
File: scripts/ui/menus/main_menu.gd:335-335
Timestamp: 2026-06-26T03:15:29.611Z
Learning: In Godot 4 GDScript, when using a `ConfirmationDialog`, handle all user cancellation/abort paths (cancel button, Escape key, and title-bar close) via the `canceled` signal only. Avoid wiring both `canceled` and `close_requested` to the same cancel handler, as it can trigger the handler twice and cause duplicate side effects (e.g., playing `AudioManager.play_sfx("ui_cancel")` twice).
Applied to files:
scripts/core/globals.gdtest/gut/test_audio_integration.gd
📚 Learning: 2026-03-30T04:02:23.747Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 500
File: test/gut/test_audio_web_bridge.gd:131-145
Timestamp: 2026-03-30T04:02:23.747Z
Learning: In GUT (Godot Unit Test) for Godot 4, when using `assert_called` / `assert_called_count` with parameter matching, include *every* argument the mocked method accepts, including parameters with default values. GUT does not auto-fill default arguments during call matching. For example, if `JavaScriptBridgeWrapper.eval(script: String, global_exec: bool = false)` is invoked as `eval(js_string)`, the actual call recorded by GUT includes the default (`eval(js_string, false)`), so your assertion must match both arguments (e.g., `.bind(js_string, false)`, not `.bind(js_string)`). Apply this rule to GUT assertions in `test/gut` tests.
Applied to files:
test/gut/test_audio_integration.gd
🪛 GitHub Actions: Pull Request Pipeline / 5_GDScript Lint and Format Check _ Lint and Format Check.txt
scripts/core/globals.gd
[error] 444-444: gdlint failed: Function "_input" has more than 6 return statements (max-returns). Step: gdlint ./scripts
🪛 GitHub Actions: Pull Request Pipeline / GDScript Lint and Format Check _ Lint and Format Check
scripts/core/globals.gd
[error] 444-444: gdlint failed: Function "_input" has more than 6 return statements (max-returns).
🔇 Additional comments (1)
test/gut/test_audio_integration.gd.uid (1)
1-2: LGTM!
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new strict focus gate in
Globals._inputrelies onui_has_focus; consider adding coverage or safeguards for cases where this flag can be stale or out of sync with actualControlfocus to avoid unintentionally dropping navigation input. - Several tests depend on fixed
wait_frames/wait_secondsdelays for deferred connections and timers; you may want to assert on specific signals or states instead of hardcoded timing to reduce potential flakiness across different environments. - The
test_audio_integration.gd.uidmetadata file appears to be committed alongside the test script; consider removing this editor-specific asset from the repository if it is not required at runtime.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new strict focus gate in `Globals._input` relies on `ui_has_focus`; consider adding coverage or safeguards for cases where this flag can be stale or out of sync with actual `Control` focus to avoid unintentionally dropping navigation input.
- Several tests depend on fixed `wait_frames`/`wait_seconds` delays for deferred connections and timers; you may want to assert on specific signals or states instead of hardcoded timing to reduce potential flakiness across different environments.
- The `test_audio_integration.gd.uid` metadata file appears to be committed alongside the test script; consider removing this editor-specific asset from the repository if it is not required at runtime.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Implement Issue #490 compliance by moving UI sound effect mappings to a centralized data-driven dictionary in AudioConstants. The _input method now iterates through UI_SFX dictionary keys instead of hardcoding actions, and dynamically resolves sound file paths. This improves maintainability and centralizes audio configuration.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/core/globals.gd`:
- Around line 408-449: This hunk needs formatting cleanup to match gdformat
output and clear CI style checks. In the input-handling block in globals.gd
where the AudioConstants.UI_SFX loop, ui_cancel guard, and ui_has_focus branch
live, remove trailing whitespace and normalize the inline return comments/spaces
so the file is formatted exactly as gdformat would produce. Keep the existing
logic unchanged, but reformat the affected block consistently before merging.
- Around line 438-449: The input-handling branch in globals.gd is too broad and
can play the accept sound twice because it also runs for non-navigation actions
like ui_accept. Update the logic around the focused-input path in the navigation
handling flow to restrict this SFX trigger to navigation actions only, either by
replacing the generic else with an explicit nav-action guard or by excluding
ui_accept before looking up AudioConstants.UI_SFX. Keep the change localized to
the branch that checks ui_has_focus, is_horizontal_slider, and
AudioManager.play_sfx so _on_global_button_pressed() remains the sole source for
button accept audio.
🪄 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: 1e8d1890-21cf-4de8-af31-26dd99c8e426
📒 Files selected for processing (2)
scripts/core/globals.gdscripts/resources/audio_constants.gd
✅ Files skipped from review due to trivial changes (1)
- scripts/resources/audio_constants.gd
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: Pull Request Pipeline / GDScript Lint and Format Check _ Lint and Format Check: UI audio logic unit tests gut volume mute integration
Conclusion: failure
##[group]Run gdformat --diff --check ./scripts
�[36;1mgdformat --diff --check ./scripts�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.14.6/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.14.6/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.14.6/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.14.6/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.14.6/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.14.6/x64/lib
##[endgroup]
would reformat ./scripts/core/globals.gd
--- ./scripts/core/globals.gd
+++ ./scripts/core/globals.gd
@@ -408,7 +408,6 @@
# DATA-DRIVEN DICTIONARY LOOKUP (Issue `#490` Compliance)
for action: String in AudioConstants.UI_SFX.keys():
if event.is_action_pressed(action, false):
-
# Context Guard A: Handle Escape/Cancellation Safeguards
if action == "ui_cancel":
var is_editing_control: bool = (
@@ -427,26 +426,26 @@
or focus_owner.has_method("cancel_remap")
)
)
-
+
# If not typing or remapping controls, play cancel sound through SFX bus
if not is_editing_control and not is_remap_control:
var file_path: String = AudioConstants.UI_SFX[action]
var sfx_name: String = file_path.get_file().get_basename()
AudioManager.play_sfx(sfx_name, AudioConstants.BUS_SFX)
- return # Always break input cycle once action matches
-
+ return # Always break input cycle once action matches
+
# Context Guard B: Handle Directional & Focus Navigation Safeguards
else:
var is_horizontal_slider: bool = (
focus_owner is Slider and (action == "ui_left" or action == "ui_right")
)
-
+
# Gated on active UI focus ownership, excluding horizontal sliders
if ui_has_focus and not is_horizontal_slider:
var file_path: String = AudioConstants.UI_SFX[action]
var sfx_name: String = file_path.get_file().get_basename()
AudioManager.play_sfx(sfx_name, Au...
GitHub Actions: Pull Request Pipeline / 4_GDScript Lint and Format Check _ Lint and Format Check.txt: UI audio logic unit tests gut volume mute integration
Conclusion: failure
##[group]Run gdformat --diff --check ./scripts
�[36;1mgdformat --diff --check ./scripts�[0m
shell: /usr/bin/bash -e {0}
env:
pythonLocation: /opt/hostedtoolcache/Python/3.14.6/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.14.6/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.14.6/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.14.6/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.14.6/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.14.6/x64/lib
##[endgroup]
would reformat ./scripts/core/globals.gd
--- ./scripts/core/globals.gd
+++ ./scripts/core/globals.gd
@@ -408,7 +408,6 @@
# DATA-DRIVEN DICTIONARY LOOKUP (Issue `#490` Compliance)
for action: String in AudioConstants.UI_SFX.keys():
if event.is_action_pressed(action, false):
-
# Context Guard A: Handle Escape/Cancellation Safeguards
if action == "ui_cancel":
var is_editing_control: bool = (
@@ -427,26 +426,26 @@
or focus_owner.has_method("cancel_remap")
)
)
-
+
# If not typing or remapping controls, play cancel sound through SFX bus
if not is_editing_control and not is_remap_control:
var file_path: String = AudioConstants.UI_SFX[action]
var sfx_name: String = file_path.get_file().get_basename()
AudioManager.play_sfx(sfx_name, AudioConstants.BUS_SFX)
- return # Always break input cycle once action matches
-
+ return # Always break input cycle once action matches
+
# Context Guard B: Handle Directional & Focus Navigation Safeguards
else:
var is_horizontal_slider: bool = (
focus_owner is Slider and (action == "ui_left" or action == "ui_right")
)
-
+
# Gated on active UI focus ownership, excluding horizontal sliders
if ui_has_focus and not is_horizontal_slider:
var file_path: String = AudioConstants.UI_SFX[action]
var sfx_name: String = file_path.get_file().get_basename()
AudioManager.play_sfx(sfx_name, Au...
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-06-22T05:17:36.437Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 762
File: scripts/core/globals.gd:408-409
Timestamp: 2026-06-22T05:17:36.437Z
Learning: For Godot 4 GDScript, prefer the event-driven form inside `_input(event)` handlers: use `event.is_action_pressed(action: StringName, allow_echo: bool = false, exact_match: bool = false)` with the default `allow_echo` (i.e., pass `false`) to suppress echo/key-repeat at the engine level so the action behaves like “just pressed”. Prefer `event.is_action_pressed("action", false)` over polling `Input.is_action_just_pressed()` for architectural/performance reasons, and avoid review suggestions that recommend replacing one with the other.
Applied to files:
scripts/core/globals.gd
📚 Learning: 2026-06-26T03:15:29.611Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 782
File: scripts/ui/menus/main_menu.gd:335-335
Timestamp: 2026-06-26T03:15:29.611Z
Learning: In Godot 4 GDScript, when using a `ConfirmationDialog`, handle all user cancellation/abort paths (cancel button, Escape key, and title-bar close) via the `canceled` signal only. Avoid wiring both `canceled` and `close_requested` to the same cancel handler, as it can trigger the handler twice and cause duplicate side effects (e.g., playing `AudioManager.play_sfx("ui_cancel")` twice).
Applied to files:
scripts/core/globals.gd
🪛 GitHub Actions: Pull Request Pipeline / 4_GDScript Lint and Format Check _ Lint and Format Check.txt
scripts/core/globals.gd
[error] 408-427: gdformat reported whitespace/formatting changes in scripts/core/globals.gd: removed trailing blank line(s) and normalized spacing; also changed return # Always break input cycle once action matches to return # Always break input cycle once action matches.
🪛 GitHub Actions: Pull Request Pipeline / GDScript Lint and Format Check _ Lint and Format Check
scripts/core/globals.gd
[error] 408-427: gdformat formatting check failed (--diff --check). File would be reformatted; whitespace/spacing changes detected (e.g., removed trailing blank whitespace, normalized return # ... comments).
Add unique_id attributes to SFXMenu scene nodes and remove the load_steps attribute from the scene header (Godot scene metadata update). Refactor globals.gd input handling: pull the is_horizontal_slider computation out of the else branch and simplify the UI-focus gated conditional that plays navigation SFX. This is a behavioral-preserving cleanup to make the conditional logic clearer.
This was
linked to
issues
Jun 28, 2026
Introduce SFX_ASSET_MAP in AudioConstants to map logical identifiers to exact file paths, decoupling asset names from filenames. Update AudioManager.play_sfx() to resolve paths via the map with fallback support for legacy calls. Add early return optimization in Globals._input() to drop mouse motion and non-action events immediately, improving performance. Implement stale focus safeguard to ensure navigation audio plays in menu contexts even with transient focus transitions. Update tests with wait_process_frames and add comprehensive coverage for asset resolution and focus edge cases.
Owner
Author
Hey Sourcery, thanks for the sharp catch! Both architecture improvements have been fully implemented:1.
|
Owner
Author
Good catch! You're absolutely right—since this lifecycle tracking relies on a multi-frame deferral window, checking the presence of the connection alone leaves a blind spot for sync/async regressions. I will update the _get_global_connection_count() helper helper to strictly assert that the CONNECT_DEFERRED flag is present on the signal's connection entry. Thanks! |
Good catch! You're absolutely right—since this lifecycle tracking relies on a multi-frame deferral window, checking the presence of the connection alone leaves a blind spot for sync/async regressions. I will update the _get_global_connection_count() helper helper to strictly assert that the CONNECT_DEFERRED flag is present on the signal's connection entry. Thanks!
Owner
Author
🛠️ Code Review Updates AddedThanks for the excellent suggestions! Both items have been addressed in the latest commit to ensure better framework maintenance and design reliability:
|
### 🛠️ Code Review Updates Added Thanks for the excellent suggestions! Both items have been addressed in the latest commit to ensure better framework maintenance and design reliability: * **Extracted Single-Responsibility Helpers:** The massive `_input` function has been completely broken down into dedicated private helper tracking and processing operations (`_track_input_device`, `_check_menu_context`, and `_process_ui_navigation_sfx`). This untangles the input gating, device tracking, and audio execution pathways, making the logic much easier to audit and modify safely. * **Hardened Menu Context Detection:** Upgraded the brittle `"Menu" in scene.name` layout check by introducing explicit structural design markers. The system now evaluates whether the active scene belongs to the `"menu_context"` node group or carries an `is_menu_context` metadata flag. The original substring lookup has been retained purely as a safe, backward-compatible fallback to preserve stability across legacy configuration states and automated test suite execution paths.
Owner
Author
🛠️ Code Review Optimization CompleteAll three optimization targets listed here are fully implemented and verified in the current codebase:
|
### 🛠️ Fix Applied: Hardened Remap Control Detection
Good catch! Relying on the `in` operator to verify script properties directly on core `Control`/`Object` instances can definitely be brittle in GDScript.
I've refactored the remap control detection block to eliminate the `in` operator checks completely:
* **Explicit Capability Checking:** The check now primarily searches for the explicit `cancel_remap` method interface via `has_method()`.
* **Safe Property Inspection:** Fallback property lookups for legacy button implementations now safely use `.get("action")` and `.get("action_name")` checking against `null`, preventing unexpected evaluations or runtime engine errors.
Ready for review!
CodeRabbitAI is completely right here. In GUT (Grand Unified Tester), if an assertion fails (like the precondition check on line 204), the test execution halts immediately. Because the first button was attached using a regular add_child(), halting early means the test bypasses initial_btn.queue_free(), leaking an orphan node into the scene tree and potentially contaminating subsequent tests in your pipeline. Swapping to add_child_autofree() guarantees that GUT tracks and deallocates the node during cleanup, even if an assertion completely tanks the test execution midway through. GUT safely handles nodes that you manually queue_free() later in the same block.
### 🛠️ Fix Applied: Isolated Suite State & Prevented Leakage Excellent spot on the state leak! Leaving `Globals.current_input_device` sitting on `"keyboard"` permanently would absolutely break test runner isolation and create fragile, order-dependent test failures downstream. Changes introduced: * **State Caching:** Added a `_original_input_device` suite-level variable to catch the active device configuration right inside `before_all()`. * **State Restorations:** Added a restoration line within `after_all()` to guarantee the baseline environment state is completely put back together before handing execution back to the test runner. * **Maintained Intent:** Kept the `before_each()` safety sweep intact so each individual test context in this suite continues to run safely under a predictable keyboard configuration.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="scripts/core/globals.gd" line_range="417-426" />
<code_context>
+
+
+## Evaluates tree states and explicit markers to safeguard input contexts.
+func _check_menu_context(ui_has_focus: bool) -> bool:
var is_menu_context: bool = (
get_tree().paused or options_open or not hidden_menus.is_empty() or ui_has_focus
)
- # Test helper fallback: support menu context detection via current_scene name for GUT tests
- # Gated strictly behind debug/ci features to prevent leakage into production shipped builds.
+ # Explicit marker evaluation with a substring fallback for legacy compliance
+ if not is_menu_context and get_tree().current_scene:
+ var scene: Node = get_tree().current_scene
+ if (
+ scene.is_in_group("menu_context")
+ or scene.has_meta("is_menu_context")
+ or "Menu" in scene.name
+ ):
+ is_menu_context = true
+
+ # Test helper fallback: support menu context detection during automated test suite runs
</code_context>
<issue_to_address>
**suggestion:** Multiple `current_scene` reads inside menu context checks could race or add unnecessary branching.
Within `_check_menu_context`, `get_tree().current_scene` is read multiple times into different locals. If the scene changes mid-frame (e.g., during transitions or deferred frees), these separate reads could yield inconsistent results between the primary and debug/CI logic, and the extra branching is redundant. Cache `current_scene` once in a local and reuse it across all branches to simplify the logic and avoid potential edge-case inconsistencies.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
### 🛠️ Fix Applied: Consolidated Scene Lookups & Eliminated Race Risks Excellent catch! Evaluating `get_tree().current_scene` across multiple separate conditional branches definitely leaves the gate vulnerable to frame-boundary race conditions or unexpected volatility during engine layout transitions. Changes applied: * **Atomic Reference Caching:** Refactored `_check_menu_context` to fetch `get_tree().current_scene` exactly once at the entry point of the evaluation block. * **Guaranteed Frame-Boundary Consistency:** The rest of the logic now runs checks strictly against the local `active_scene` snapshot variable, protecting the input handler from state mutation if a scene transitions or defer-frees mid-frame. * **Redundancy Cleanup:** Stripped out duplicate `get_tree().current_scene` null-guard branches, cleaning up the logic pathways. Thanks for the precise feedback on this last stretch! 🚀
Owner
Author
|
@sourcery-ai title |
sourcery-ai
Bot
changed the title
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 `@scripts/core/globals.gd`:
- Around line 474-508: The UI SFX routing is inconsistent because
`_handle_ui_cancel_action()` and `_handle_ui_navigation_action()` use
`AudioConstants.BUS_SFX`, while `_on_global_button_pressed()` still plays accept
sounds on `AudioConstants.BUS_SFX_MENU`. Update the global UI audio path so all
UI interactions use the same bus, and align the button-accept flow with the new
helpers by changing `_on_global_button_pressed()` to the same shared bus as the
navigation/cancel actions.
In `@test/gut/test_audio_integration.gd`:
- Around line 207-214: The test is incorrectly binding to the first discovered
CheckButton, which makes it dependent on scene tree order rather than the
intended mute-control behavior. Update the audio integration test to locate the
specific mute control in test_audio_integration.gd using a stable identifier or
binding check instead of check_buttons[0], and only emit toggled after
confirming the found CheckButton is the expected one so the assertion targets
the correct contract.
In `@test/gut/test_audio_sfx_centralization.gd`:
- Around line 147-157: The test `test_play_sfx_unmapped_legacy_fallback` is
using a sound ID that is already mapped, so it never reaches the legacy fallback
path. Update the test to use a truly unmapped identifier or temporarily remove
the `AudioConstants.SFX_ASSET_MAP` entry before calling
`AudioManager.play_sfx()`, so the `file_name += ".wav"` branch is actually
exercised.
- Around line 163-181: The test body in test_input_ignores_mouse_motion leaves
Globals.options_open cleanup at the end, but a failed assert can skip that reset
and leak state into later GUT tests. Move the Globals.options_open = false
teardown into the suite’s shared after_each() cleanup for this test file, and
keep the test body focused on the assertion and event setup using Globals and
AudioManager.
🪄 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: 3a8ac4cd-959b-49cd-8e47-968aa3d4625d
📒 Files selected for processing (10)
scripts/core/globals.gdscripts/managers/audio_manager.gdscripts/resources/audio_constants.gdtest/gut/test_audio_integration.gdtest/gut/test_audio_sfx_centralization.gdtest/gut/test_globals_button_hooks.gdtest/gut/test_globals_input_guards.gdtest/gut/test_integration_key_mapping.gdtest/gut/test_nav_escape_sfx.gdtest/gut/test_quit_game_confirm_dialog_sfx.gd
✅ Files skipped from review due to trivial changes (1)
- test/gut/test_nav_escape_sfx.gd
🚧 Files skipped from review as they are similar to previous changes (2)
- test/gut/test_globals_input_guards.gd
- test/gut/test_globals_button_hooks.gd
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: CI/CD Infrastructure Tests / Test Godot Asset Infrastructure and Signature Verification
- GitHub Check: GUT Unit Tests / unit-test
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-03-30T04:02:23.747Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 500
File: test/gut/test_audio_web_bridge.gd:131-145
Timestamp: 2026-03-30T04:02:23.747Z
Learning: In GUT (Godot Unit Test) for Godot 4, when using `assert_called` / `assert_called_count` with parameter matching, include *every* argument the mocked method accepts, including parameters with default values. GUT does not auto-fill default arguments during call matching. For example, if `JavaScriptBridgeWrapper.eval(script: String, global_exec: bool = false)` is invoked as `eval(js_string)`, the actual call recorded by GUT includes the default (`eval(js_string, false)`), so your assertion must match both arguments (e.g., `.bind(js_string, false)`, not `.bind(js_string)`). Apply this rule to GUT assertions in `test/gut` tests.
Applied to files:
test/gut/test_quit_game_confirm_dialog_sfx.gdtest/gut/test_integration_key_mapping.gdtest/gut/test_audio_sfx_centralization.gdtest/gut/test_audio_integration.gd
📚 Learning: 2026-06-29T03:24:09.331Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 784
File: test/gut/test_globals_button_hooks.gd:206-209
Timestamp: 2026-06-29T03:24:09.331Z
Learning: In this repository’s GUT-based Godot 4 tests (files under `test/gut/`), note that a failed assertion aborts the remainder of the test body immediately, so cleanup code placed later in the test may not run. For nodes created during test setup in `test/gut` tests, prefer `add_child_autofree()` instead of plain `add_child()` when there could be a later assertion failure; this ensures the node is freed automatically even if the test exits early (manual freeing later is acceptable but should not be the only cleanup mechanism).
Applied to files:
test/gut/test_quit_game_confirm_dialog_sfx.gdtest/gut/test_integration_key_mapping.gdtest/gut/test_audio_sfx_centralization.gdtest/gut/test_audio_integration.gd
📚 Learning: 2026-06-22T05:17:36.437Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 762
File: scripts/core/globals.gd:408-409
Timestamp: 2026-06-22T05:17:36.437Z
Learning: For Godot 4 GDScript, prefer the event-driven form inside `_input(event)` handlers: use `event.is_action_pressed(action: StringName, allow_echo: bool = false, exact_match: bool = false)` with the default `allow_echo` (i.e., pass `false`) to suppress echo/key-repeat at the engine level so the action behaves like “just pressed”. Prefer `event.is_action_pressed("action", false)` over polling `Input.is_action_just_pressed()` for architectural/performance reasons, and avoid review suggestions that recommend replacing one with the other.
Applied to files:
test/gut/test_quit_game_confirm_dialog_sfx.gdtest/gut/test_integration_key_mapping.gdscripts/resources/audio_constants.gdscripts/managers/audio_manager.gdtest/gut/test_audio_sfx_centralization.gdtest/gut/test_audio_integration.gdscripts/core/globals.gd
📚 Learning: 2026-06-26T03:15:29.611Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 782
File: scripts/ui/menus/main_menu.gd:335-335
Timestamp: 2026-06-26T03:15:29.611Z
Learning: In Godot 4 GDScript, when using a `ConfirmationDialog`, handle all user cancellation/abort paths (cancel button, Escape key, and title-bar close) via the `canceled` signal only. Avoid wiring both `canceled` and `close_requested` to the same cancel handler, as it can trigger the handler twice and cause duplicate side effects (e.g., playing `AudioManager.play_sfx("ui_cancel")` twice).
Applied to files:
test/gut/test_quit_game_confirm_dialog_sfx.gdtest/gut/test_integration_key_mapping.gdscripts/resources/audio_constants.gdscripts/managers/audio_manager.gdtest/gut/test_audio_sfx_centralization.gdtest/gut/test_audio_integration.gdscripts/core/globals.gd
📚 Learning: 2026-06-26T01:41:48.842Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 782
File: test/gut/test_quit_game_confirm_dialog_sfx.gd:110-127
Timestamp: 2026-06-26T01:41:48.842Z
Learning: For Godot 4 GDScript GUT tests in `test/gut` that cover UI accept/click SFX behavior (files matching `*_sfx.gd`), verify the sound-triggering path through the real global listener pipeline: exercise the `ui_accept` SFX by routing via `Globals._on_node_added` → the global button-pressed listener and the control’s native `pressed` signal. Do not validate/trigger this behavior by directly calling `scripts/ui/menus/main_menu.gd` `_input()` or `_unhandled_input()`; for cases like flat-button gating (e.g., `test/gut/test_quit_game_confirm_dialog_sfx.gd`), confirm gating works through the same global listener flow.
Applied to files:
test/gut/test_quit_game_confirm_dialog_sfx.gd
Route all UI SFX through the same bus. These new helpers force nav/cancel through AudioConstants.BUS_SFX, but button accepts still come from _on_global_button_pressed() and default to AudioConstants.BUS_SFX_MENU. After this change, muting BUS_SFX silences keyboard nav/cancel while Button accepts remain audible, so UI mute behavior is no longer consistent and the new integration coverage cannot validate one coherent contract. Pick one UI bus and use it everywhere.
Sharp catch! Blindly grabbing check_buttons[0] was a layout-dependent trap waiting to happen. I've updated test_ui_mute_controls_sync_state_dynamically() to explicitly filter the collection for an active mute control by name, decoupling it from tree layout order. Fixed in the latest commit.
"slider" is already present in AudioConstants.SFX_ASSET_MAP, so this call resolves through the map instead of hitting file_name += ".wav". As written, the test can pass even if the unmapped fallback logic is broken. Use a real fixture key that is intentionally absent from the map, or remove the mapping inside the test before calling play_sfx().
If the assertion on Line 178 fails, Line 181 never runs and this suite leaks Globals.options_open = true into later tests. Reset that flag in shared teardown instead of at the bottom of the test body. Based on learnings, failed assertions in this repo's GUT tests abort the rest of the test body, so inline cleanup is not guaranteed.
Owner
Author
|
@sourcery-ai guide |
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
ikostan
deleted the
ui-audio-logic-unit-tests-gut-volume-mute-integration
branch
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
1 participant
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")
Summary by Sourcery
Refine global UI input handling and audio routing to support device tracking, menu context guards, and data-driven SFX lookup, while expanding automated coverage around audio buses, mute controls, sliders, and button hooks.
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Bots/AI Contributors to PR #784
AI/Bot-Assisted Contributions
These automated tools provided code summaries, reviews, suggestions, and analysis that influenced the development and refinement of the PR:
test_audio_integration.gdand_inputfunction complexity/return limits). Included context from prior learnings in the repo.No evidence of other common bots like @dependabot in the visible PR activity, commits, or reviews.
Human Contributors
AudioConstants, tightening global input guards inGlobals._input()(device tracking, menu context, echo/mouse filters, focus-based navigation/accept/cancel logic), updatingAudioManager, and adding/extending extensive GUT integration tests for audio buses, mute hierarchy, sliders, button hooks, and input guards. Also handled test infrastructure cleanup (e.g., removingrun_unit_tests.sh).Overall PR Context
The PR focuses on refactoring global UI audio handling for better consistency, robustness, and test coverage in a Godot project. AI tools significantly aided in summarizing changes, spotting potential issues (e.g., flakiness, code complexity), and ensuring comprehensive test expansion. The final output benefited from iterative human refinements addressing bot feedback.