Route menu UI SFX through AudioManager and add GUT tests#782
Conversation
Use AudioManager.play_sfx for menu navigation/cancel sounds and add test helpers to support GUT. globals.gd: add a test-fallback menu detection via current_scene.name and route _play_ui_navigation_sfx through AudioManager. main_menu.gd: remove redundant cancel-button connection and trigger ui_cancel via AudioManager in _on_quit_dialog_canceled. Add comprehensive GUT tests (test_nav_escape_sfx.gd, test_quit_game_confirm_dialog_sfx.gd) with mock AudioManager scripts and UID files to validate navigation, cancel, and confirmation audio pathways.
ikostan
added this to the
Milestone 21: Implement HUD FPS Counter UI Component milestone
Reviewer's GuideRoute menu navigation and cancel UI sound effects through the centralized AudioManager, add GUT test suites to validate global menu/quit-dialog audio behavior, and tighten CI/Godot integrity checks and milestone docs formatting. Sequence diagram for routed menu UI SFX through AudioManagersequenceDiagram
actor User
participant Globals as Globals
participant MainMenu as MainMenu
participant QuitDialog as ConfirmationDialog
participant AudioManager as AudioManager
%% Navigation SFX via Globals
User->>Globals: _input(event: InputEvent)
alt [event is ui_nav action]
alt [debug/ci menu detection via current_scene.name]
Globals-->>Globals: is_menu_context = true
end
Globals->>Globals: _play_ui_navigation_sfx()
Globals->>AudioManager: play_sfx(ui_navigation)
end
%% Cancel SFX via quit dialog
User->>QuitDialog: cancel (button/Esc/close)
QuitDialog->>MainMenu: canceled
MainMenu->>MainMenu: _on_quit_dialog_canceled()
MainMenu->>AudioManager: play_sfx(ui_cancel)
%% Confirm SFX and guarded quit
User->>QuitDialog: confirm
QuitDialog->>MainMenu: confirmed
MainMenu->>MainMenu: _on_quit_dialog_confirmed()
MainMenu->>QuitDialog: hide()
alt [bypass_quit_for_testing]
MainMenu-->>MainMenu: log_message("Bypassing game quit execution for unit testing.")
else [normal runtime]
alt [OS.get_name() == Web]
MainMenu->>JavaScriptBridge: eval(setTimeout(...))
else [native]
MainMenu->>MainMenu: await get_tree().create_timer(audio_flush_delay).timeout
MainMenu->>MainMenu: get_tree().quit()
end
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (8)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used🪛 LanguageToolfiles/docs/milestones/18/PART_2_CI_CD_Infrastructure_Updates_&_Caching_Hardening.md[uncategorized] ~69-~69: The official name of this software platform is spelled with a capital “H”. (GITHUB) files/docs/milestones/20/Part_2_Upgrade_project_environment_to_Godot_4_6_3.md[uncategorized] ~88-~88: The official name of this software platform is spelled with a capital “H”. (GITHUB) [uncategorized] ~89-~89: The official name of this software platform is spelled with a capital “H”. (GITHUB) files/docs/milestones/21/Part_2_Route_menu_UI_SFX_through_AudioManager_&_add_GUT_tests.md[style] ~73-~73: ‘prior to’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_PRIOR_TO) [uncategorized] ~114-~114: The official name of this software platform is spelled with a capital “H”. (GITHUB) 📝 WalkthroughWalkthroughThe PR routes menu UI sound effects through ChangesUI audio routing and verification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Jun 27, 2026 3:38a.m. | Review ↗ |
| JavaScript | |
Jun 27, 2026 3:38a.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.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
globals.gd,_play_ui_navigation_sfx()now callsAudioManager.play_sfx()but still retains the_nav_sfx_playervalidity check and related logic that is no longer used; consider either delegating everything toAudioManagerand removing the dead_nav_sfx_playercode, or keeping_nav_sfx_playeras the single source of playback to avoid split responsibility. - The test helper fallback that infers
is_menu_contextfromget_tree().current_scene.namecontaining "Menu" affects runtime behavior globally; consider narrowing this (e.g., behind a test flag, checkingEngine.is_editor_hint(), or using a dedicated test hook) so production scenes are not misclassified as menus based solely on naming. - After removing the cancel button connection in
main_menu.gd,_on_cancel_button_clicked()appears to be unused; consider removing it or documenting why it must remain to avoid confusion about which path is responsible for playing the cancel SFX.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `globals.gd`, `_play_ui_navigation_sfx()` now calls `AudioManager.play_sfx()` but still retains the `_nav_sfx_player` validity check and related logic that is no longer used; consider either delegating everything to `AudioManager` and removing the dead `_nav_sfx_player` code, or keeping `_nav_sfx_player` as the single source of playback to avoid split responsibility.
- The test helper fallback that infers `is_menu_context` from `get_tree().current_scene.name` containing "Menu" affects runtime behavior globally; consider narrowing this (e.g., behind a test flag, checking `Engine.is_editor_hint()`, or using a dedicated test hook) so production scenes are not misclassified as menus based solely on naming.
- After removing the cancel button connection in `main_menu.gd`, `_on_cancel_button_clicked()` appears to be unused; consider removing it or documenting why it must remain to avoid confusion about which path is responsible for playing the cancel SFX.
## Individual Comments
### Comment 1
<location path="scripts/core/globals.gd" line_range="449-450" />
<code_context>
+## Internal helper to play the navigation sound through the dedicated Menu SFX bus.
## Internal helper to play the navigation sound through the dedicated Menu SFX bus.
func _play_ui_navigation_sfx() -> void:
+ # Route through AudioManager for centralization and test suite assertion tracking
+ AudioManager.play_sfx("ui_nav_tick")
+
if not is_instance_valid(_nav_sfx_player):
</code_context>
<issue_to_address>
**issue (bug_risk):** Navigation SFX is likely being played twice (AudioManager and `_nav_sfx_player`).
The added `AudioManager.play_sfx("ui_nav_tick")` call now runs in addition to the existing `_nav_sfx_player` logic, so the sound may play twice when `_nav_sfx_player` is valid, which also conflicts with the stated centralization goal. If you want to centralize via `AudioManager`, consider removing or conditioning the `_nav_sfx_player` path, or choose a single playback path (e.g., via a feature flag).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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)
448-457: 🩺 Stability & Availability | 🟡 MinorAlign the navigation SFX name with the existing asset
AudioManager.play_sfx("ui_nav_tick")points at a missing file; onlyui_navigation.wavexists. That makes the new call a no-op while_nav_sfx_player.play()still drives the sound. Rename theAudioManagercall to the existing asset name or addui_nav_tick.wavbefore removing the legacy player.🤖 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 448 - 457, The navigation SFX identifier in _play_ui_navigation_sfx() points to a missing asset, so the AudioManager call becomes a no-op while the legacy _nav_sfx_player still plays the sound. Update the AudioManager.play_sfx(...) argument to match the existing ui_navigation.wav asset, or add the missing ui_nav_tick.wav asset before removing the fallback player path. Use the _play_ui_navigation_sfx and _nav_sfx_player symbols to locate the affected logic.
🧹 Nitpick comments (2)
scripts/ui/menus/main_menu.gd (1)
327-330: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
_on_cancel_button_clickedis now dead code.Its only wiring (the cancel-button
pressedconnection in_setup_quit_dialog) was removed in this PR, and the cancel SFX now lives in_on_quit_dialog_canceled. This handler is no longer referenced and can be deleted.🧹 Proposed removal
-func _on_cancel_button_clicked() -> void: - ## Triggers the cancel audio stream exclusively on manual mouse clicks. - AudioManager.play_sfx("ui_cancel") - - func _on_quit_dialog_canceled() -> void:🤖 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/ui/menus/main_menu.gd` around lines 327 - 330, Delete the now-unused _on_cancel_button_clicked handler in main_menu.gd because its only connection was removed and the cancel sound logic has moved to _on_quit_dialog_canceled; make sure no remaining calls or signal bindings reference _on_cancel_button_clicked after removing it.test/gut/test_nav_escape_sfx.gd (1)
40-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSnapshot fields never match Globals, so
_set_menu_contextonly flips the scene name.
is_menu_contextis a local variable insideGlobals._input, not a member field — none of thepossible_fieldsexist on the singleton, sooriginal_fieldsstays empty and thefor field ... set(field, value)loop in_set_menu_contextis a no-op. The tests therefore exercise only the new scene-name fallback (current_scene.name = "MainMenu"/"GameLevel"). That is fine for what's under test, but the field-snapshot machinery is dead weight and is misleading about what's being asserted. Worth simplifying to just the scene-name toggle.Also applies to: 88-92
🤖 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 `@test/gut/test_nav_escape_sfx.gd` around lines 40 - 47, The field snapshot logic in the menu-context test is dead code because Globals does not expose any of the listed members, so _set_menu_context only affects the scene-name fallback. Simplify the test helper by removing the possible_fields/original_fields probing from test_nav_escape_sfx.gd and rely on the current_scene.name toggle used by _set_menu_context. Keep the test focused on the behavior actually under test and apply the same cleanup to the duplicate block referenced by the related comment.
🤖 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 404-406: The scene-name fallback in the menu context check is
leaking test-only behavior into runtime. Update the `_input`/menu detection
logic in `globals.gd` so the `current_scene.name` contains-"Menu" fallback only
runs under a test or debug feature gate, while normal shipped builds rely on the
existing menu detection path. Keep the change localized to the menu-context
helper near the `is_menu_context` assignment so real gameplay scenes like
`MenuLevel` do not trigger UI SFX.
In `@scripts/ui/menus/main_menu.gd`:
- Line 335: The quit dialog dismissal path in _setup_quit_dialog() is wired so
both canceled and close_requested can reach _on_quit_dialog_canceled(), which
causes AudioManager.play_sfx("ui_cancel") to fire twice. Update the signal
connections on the quit dialog so only one of those signals uses
_on_quit_dialog_canceled(), or add a guard in _on_quit_dialog_canceled() to
prevent duplicate handling when the title-bar close path is used.
In `@test/gut/test_nav_escape_sfx.gd`:
- Around line 91-92: The _set_menu_context helper is mutating
get_tree().current_scene.name on the shared test-runner scene, and that state is
not being restored between tests. Update the test setup/teardown around
_set_menu_context and after_each so the original current_scene name is saved
before renaming and restored afterward, preventing cross-test contamination.
In `@test/gut/test_quit_game_confirm_dialog_sfx.gd`:
- Around line 107-124: The test is exercising the wrong path:
main_menu_instance._input does not trigger ui_accept SFX, so the assertion is
vacuously passing. Update test_flat_button_anti_trigger_protection to drive the
real global accept flow by going through Globals._on_node_added and the button
pressed path (or the relevant global button handler), then verify the
flat-button gating against the accept sound there.
---
Outside diff comments:
In `@scripts/core/globals.gd`:
- Around line 448-457: The navigation SFX identifier in
_play_ui_navigation_sfx() points to a missing asset, so the AudioManager call
becomes a no-op while the legacy _nav_sfx_player still plays the sound. Update
the AudioManager.play_sfx(...) argument to match the existing ui_navigation.wav
asset, or add the missing ui_nav_tick.wav asset before removing the fallback
player path. Use the _play_ui_navigation_sfx and _nav_sfx_player symbols to
locate the affected logic.
---
Nitpick comments:
In `@scripts/ui/menus/main_menu.gd`:
- Around line 327-330: Delete the now-unused _on_cancel_button_clicked handler
in main_menu.gd because its only connection was removed and the cancel sound
logic has moved to _on_quit_dialog_canceled; make sure no remaining calls or
signal bindings reference _on_cancel_button_clicked after removing it.
In `@test/gut/test_nav_escape_sfx.gd`:
- Around line 40-47: The field snapshot logic in the menu-context test is dead
code because Globals does not expose any of the listed members, so
_set_menu_context only affects the scene-name fallback. Simplify the test helper
by removing the possible_fields/original_fields probing from
test_nav_escape_sfx.gd and rely on the current_scene.name toggle used by
_set_menu_context. Keep the test focused on the behavior actually under test and
apply the same cleanup to the duplicate block referenced by the related comment.
🪄 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: db3eae54-4b29-4f78-b90b-1a2a29a84429
📒 Files selected for processing (6)
scripts/core/globals.gdscripts/ui/menus/main_menu.gdtest/gut/test_nav_escape_sfx.gdtest/gut/test_nav_escape_sfx.gd.uidtest/gut/test_quit_game_confirm_dialog_sfx.gdtest/gut/test_quit_game_confirm_dialog_sfx.gd.uid
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: CI/CD Infrastructure Tests / Test Deployment Scripts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-05-27T04:07:45.235Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 719
File: scripts/ui/menus/audio_settings.gd:300-313
Timestamp: 2026-05-27T04:07:45.235Z
Learning: In Godot 4 GDScript UI code (e.g., slider/drag signal handlers), treat high-frequency callbacks as potentially re-entrant: if you have a conditional state-guard (the “circuit breaker”) inside the handler, put the guard at the very top of the conditional block and perform it before any bus-specific calls, async/yield operations, or other side effects (like `play_sfx("check")` or token increments). This prevents rapid drag frames from re-entering before the first call finishes, which can cause duplicate SFX triggers and redundant state updates.
Applied to files:
scripts/ui/menus/main_menu.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:
scripts/ui/menus/main_menu.gdtest/gut/test_quit_game_confirm_dialog_sfx.gdscripts/core/globals.gdtest/gut/test_nav_escape_sfx.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_quit_game_confirm_dialog_sfx.gdtest/gut/test_nav_escape_sfx.gd
🔇 Additional comments (3)
test/gut/test_nav_escape_sfx.gd.uid (1)
1-1: LGTM!test/gut/test_quit_game_confirm_dialog_sfx.gd.uid (1)
1-1: LGTM!test/gut/test_quit_game_confirm_dialog_sfx.gd (1)
87-92: 🩺 Stability & AvailabilityNo runner-quit risk here.
main_menu_instanceis auto-freed at teardown, so this coroutine stops before the 0.2s timer can resume and reachget_tree().quit().> Likely an incorrect or invalid review comment.
Update Dockerfile to download Godot v4.6.3 binary and export templates from the TuxFamily mirror instead of GitHub releases. The existing SHA256 verification, extraction, and installation steps are preserved (grep/sha256sum checks, unzip, move to /usr/local/bin, and clean up). This change improves download sourcing while keeping integrity checks and installation behavior unchanged.
Switch Godot v4.6.3 download and verification from the TuxFamily mirror/SHA256 to the official GitHub release SHA512-SUMS. Update wget URLs for the Linux binary and export templates, use sha512sum to verify SHA512-SUMS.txt entries, and adjust cleanup to remove SHA512-SUMS.txt filenames. Installation and extraction steps remain unchanged.
Ensure AudioManager restores internal state after set_script() in test teardowns. Adds a has_method check and calls cleanup_for_test() in test/gut/test_nav_escape_sfx.gd and test/gut/test_quit_game_confirm_dialog_sfx.gd to repopulate/rebuild variables wiped by set_script(), preventing cross-test state leakage.
The issue causing test_global_cancellation_in_menu_context to fail specifically on the GitHub Actions CI/CD environment comes down to a difference in how the Godot engine initializes the SceneTree in headless test runner configurations compared to your local editor viewports. We can fix this seamlessly by adding "options_open" to the property snapshot tracking list. Because options_open is an actual variable on the Globals autoload singleton, the context helper will directly toggle this boolean flag. This forces the production check to register as a valid menu context on any CI server or container, completely independent of scene or window focus initialization.
Remove the previously added persistent AudioStreamPlayer for UI navigation sounds from globals.gd. Deleted the _nav_sfx_player variable, its initialization and child addition in _ready(), and the instance validity/play restart logic in _play_ui_navigation_sfx(). Playback is now centralized through AudioManager.play_sfx("ui_nav_tick"). This simplifies node management and removes the extra node churn introduced by the persistent player.
Rename the existing 'test-scripts' job to 'test-python' (name changed to "Run CI Injection Tests") and add a new 'verify-godot' job to run Godot asset infrastructure and signature verification. The new job runs on ubuntu-latest with a 10 minute timeout (to allow downloading a ~1.2 GB template), checks out the repository, and prepares the verify_godot.sh script (chmod +x) before running verification.
main_menu.gd's _input only handles Web background-music unlock; it doesn't route accept SFX. The global accept path lives in Globals._on_node_added/_on_global_button_pressed, which isn't exercised by manually calling main_menu_instance._input(event). So the assertion passes trivially regardless of flat-button gating. If the intent is to verify flat-button protection, drive Globals._on_node_added / the button pressed signal instead.
_set_menu_context overwrites get_tree().current_scene.name. Since current_scene is the shared test-runner scene, this rename persists across tests and could affect any later suite that inspects the scene name. Consider restoring the original name in after_each.
This branch treats any scene whose name contains "Menu" as menu context. Since _input is an autoload handler invoked at runtime, a real gameplay scene named e.g. MenuLevel/SubMenuArea would start emitting UI navigation/cancel SFX. Consider gating it behind a test feature (e.g. OS.has_feature("ci")/"debug") so the helper does not leak into shipped builds.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/gut/test_quit_game_confirm_dialog_sfx.gd (1)
90-95: 🩺 Stability & Availability | 🟠 MajorDon't call the full quit handler in this test.
test_dialog_confirmation_audio()invokesscripts/ui/menus/main_menu.gd::_on_quit_dialog_confirmed(), which playsui_acceptand then, on native platforms, awaitsaudio_flush_delaybefore callingget_tree().quit(). Isolate the SFX assertion from the quit path so the test can't shut down the rest of the suite.🤖 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 `@test/gut/test_quit_game_confirm_dialog_sfx.gd` around lines 90 - 95, The test is invoking the full quit-confirmation handler, which can reach the quit path and disrupt the suite. Update test_dialog_confirmation_audio() to isolate only the SFX behavior by avoiding the direct call to main_menu_instance._on_quit_dialog_confirmed() and instead trigger or mock the audio side effect through a narrower path in main_menu.gd. Keep the assertions focused on _assert_sfx_called("ui_accept") and _assert_sfx_call_count(1), and ensure no code path can call get_tree().quit() during this test.
🤖 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.
Outside diff comments:
In `@test/gut/test_quit_game_confirm_dialog_sfx.gd`:
- Around line 90-95: The test is invoking the full quit-confirmation handler,
which can reach the quit path and disrupt the suite. Update
test_dialog_confirmation_audio() to isolate only the SFX behavior by avoiding
the direct call to main_menu_instance._on_quit_dialog_confirmed() and instead
trigger or mock the audio side effect through a narrower path in main_menu.gd.
Keep the assertions focused on _assert_sfx_called("ui_accept") and
_assert_sfx_call_count(1), and ensure no code path can call get_tree().quit()
during this test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 746f137e-a0f8-40c4-97e4-22ead793e970
📒 Files selected for processing (3)
scripts/core/globals.gdtest/gut/test_nav_escape_sfx.gdtest/gut/test_quit_game_confirm_dialog_sfx.gd
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/core/globals.gd
- test/gut/test_nav_escape_sfx.gd
📜 Review details
🧰 Additional context used
🧠 Learnings (3)
📚 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.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.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
Avoid wiring both dismiss signals to the same handler — _on_quit_dialog_canceled() can run twice on the title-bar close path, so ui_cancel may play twice. Connect only one of canceled / close_requested, or guard against duplicate handling in _setup_quit_dialog().
After removing the cancel button connection in main_menu.gd, _on_cancel_button_clicked() appears to be unused; consider removing it or documenting why it must remain to avoid confusion about which path is responsible for playing the cancel SFX.
🚀 PR Review Status Update: All Issues ResolvedEvery piece of feedback from the code reviews has been refactored, tested, and verified against the headless CI/CD pipeline. Below is the final status of each item: 1. Audio Centralization & Dead Code Removal (
|
Brilliant catch! I completely overlooked that running _on_quit_dialog_confirmed() on native platforms fires get_tree().quit() directly. Locally, this kills the editor, and in the CI/CD workflow, it tricks the process into exiting early with a 0 status, masking downstream test skipping. To resolve this cleanly, I have introduced a bypass_quit_for_testing safety flag to main_menu.gd which early-returns right after the ui_accept sound effect is queued. The unit test suite toggles this guard on automatically inside before_each(). This completely isolates our audio verification step and ensures the test runner finishes safely. The clean changes are pushed! |
Add a safety flag bypass_quit_for_testing to MainMenu to prevent actual engine quitting when the quit confirmation is confirmed during automated tests. The quit handler now logs and returns early when the flag is set. Update the GUT test to instantiate the scene (so @onready children initialize) and enable the bypass flag on the instance to avoid get_tree().quit() terminating the test runner.
Fix incorrect SFX key used for UI navigation: change AudioManager.play_sfx call from "ui_nav_tick" to "ui_navigation" to match the actual asset filename (ui_navigation.wav). Update the unit test assertion to expect "ui_navigation" so tests align with production behavior and verify the correct audio request.
Amazing catch! I completely crossed my wires during the centralization refactor by passing "ui_nav_tick" instead of referencing the actual file asset ui_navigation.wav. This would have left menu navigation completely silent in production. I have renamed the string mapping inside _play_ui_navigation_sfx() to "ui_navigation" so it resolves seamlessly. I also updated our custom GUT unit test suites to track the correct asset string name accordingly. The clean changes are pushed! |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
_inputfallback that infersis_menu_contextfromget_tree().current_scene.namewhendebug/cifeatures are present couples behavior to scene naming; consider making this an explicit test hook (e.g., a dedicated test-only flag/method) to avoid brittle reliance on scene names and feature flags. - The
bypass_quit_for_testingflag inmain_menu.gdadds test-specific logic into production code; you might want to route this behavior through an injectable dependency orOS.has_featurecheck so tests can control quitting without adding a general-purpose toggle to the runtime script. - Both new GUT suites duplicate the inline mock
AudioManagerscript definition; consider extracting a small shared test helper or base class to centralize this setup and reduce maintenance overhead if the mocking strategy changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_input` fallback that infers `is_menu_context` from `get_tree().current_scene.name` when `debug/ci` features are present couples behavior to scene naming; consider making this an explicit test hook (e.g., a dedicated test-only flag/method) to avoid brittle reliance on scene names and feature flags.
- The `bypass_quit_for_testing` flag in `main_menu.gd` adds test-specific logic into production code; you might want to route this behavior through an injectable dependency or `OS.has_feature` check so tests can control quitting without adding a general-purpose toggle to the runtime script.
- Both new GUT suites duplicate the inline mock `AudioManager` script definition; consider extracting a small shared test helper or base class to centralize this setup and reduce maintenance overhead if the mocking strategy changes.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai title |
sourcery-ai
Bot
changed the title
This has been completely addressed! The unused _on_cancel_button_clicked() handler has been deleted from main_menu.gd, and all cancellation vectors are now safely routed through the single-dispatch _on_quit_dialog_canceled() callback. No orphaned code or signal bindings remain. Thank you! |
Completely agree! That property-probing array loop was legacy scaffolding from an early iteration where we experimented with injecting mock properties directly onto the singleton. Because is_menu_context is evaluated as a localized variable, that block was dead weight. I have completely stripped out the possible_fields mapping loops from before_each() and after_each(). The _set_menu_context() helper now exclusively drives the environment setup via the targeted get_tree().current_scene.name fallback channel, keeping the suite concise and focused. Clean changes pushed! |
Snapshot fields never match Globals, so _set_menu_context only flips the scene name. is_menu_context is a local variable inside Globals._input, not a member field — none of the possible_fields exist on the singleton, so original_fields stays empty and the for field ... set(field, value) loop in _set_menu_context is a no-op. The tests therefore exercise only the new scene-name fallback (current_scene.name = "MainMenu"/"GameLevel"). That is fine for what's under test, but the field-snapshot machinery is dead weight and is misleading about what's being asserted. Worth simplifying to just the scene-name toggle.
Thanks for the high-level architectural feedback! While extracting a shared test base class and refactoring to an injectable dependency model would be cleaner long-term, doing so right now expands the scope and footprint of this specific bug-fix PR. Since the current scene-name fallback is strictly encapsulated behind production-safe feature flags (OS.has_feature("debug") or OS.has_feature("ci")) and the code cleanly passes all 9/9 unit tests, we are going to merge this localized fix as-is. We've noted the test-base centralization as a candidate for a future standalone tech-debt cleanup task. Thanks! |
The failure in test_global_cancellation_in_menu_context when executing on GitHub CI or headless modes happens due to how Godot initializes the scene tree in headless testing environments. Instead of increasing the PR footprint by introducing more tracking flags or refactoring production singletons, we can apply a pure testing-side fix to test_nav_escape_sfx.gd. By updating before_each() to dynamically create, mount, and attach a temporary dummy node to get_tree().current_scene whenever it is missing, we ensure that both local viewports and remote server environments possess an identical, predictable context state.
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)
test/gut/test_nav_escape_sfx.gd (1)
221-226: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
ui_cancelhere;ui_acceptnever hits the LineEdit bypass.
Globals._input()only checksLineEdit/TextEditfocus inside theui_cancelbranch. Withui_accept, this test stays green even if that bypass regresses.Suggested change
var event: InputEventAction = InputEventAction.new() - event.action = "ui_accept" + event.action = "ui_cancel" event.pressed = trueBased on
scripts/core/globals.gd:384-447, the focused-text-entry suppression being asserted here is implemented only forevent.is_action_pressed("ui_cancel", false).🤖 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 `@test/gut/test_nav_escape_sfx.gd` around lines 221 - 226, The test is using the wrong input action for the LineEdit/TextEdit bypass, so it does not exercise the code path guarded in Globals._input(). Update the event in the nav escape SFX test to use ui_cancel instead of ui_accept, and keep the assertion on _assert_sfx_not_called() so it verifies the focused-text-entry suppression in the ui_cancel branch.
🧹 Nitpick comments (1)
test/gut/test_nav_escape_sfx.gd (1)
107-112: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise only
Globals._input()in this suite.These tests are validating the routing in
scripts/core/globals.gd:384-447, but_simulate_input()also calls_unhandled_input(). That can let the suite pass if the SFX path drifts to_unhandled_input()or starts firing from both handlers.Suggested change
func _simulate_input(event: InputEvent) -> void: if is_instance_valid(globals_instance): if globals_instance.has_method("_input"): globals_instance._input(event) - if globals_instance.has_method("_unhandled_input"): - globals_instance._unhandled_input(event)Based on learnings, prefer the event-driven
_input(event)path for these actions;scripts/core/globals.gd:384-447implements this SFX contract there.🤖 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 `@test/gut/test_nav_escape_sfx.gd` around lines 107 - 112, The test helper `_simulate_input()` is exercising both `Globals._input()` and `Globals._unhandled_input()`, which weakens the routing assertion for the SFX contract. Update this helper to invoke only the `_input(event)` path on `globals_instance` and remove the `_unhandled_input()` call so the suite validates the behavior implemented in `Globals._input()` consistently.Source: Learnings
🤖 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
`@files/docs/milestones/21/Part_2_Route_menu_UI_SFX_through_AudioManager_`&_add_GUT_tests.md:
- Around line 113-114: The issue is that the linked issue assessment lists
incorrect GUT test file paths, which makes cross-referencing the requirements
inconsistent. Update the two entries that reference the nav escape and quit
confirmation test suites so they match the actual locations used elsewhere in
the PR: the test files associated with Globals and MainMenu should point to the
test/gut paths, not res://test/unit. Keep the issue references and test
descriptions unchanged, only correct the path strings for those two test suite
entries.
---
Outside diff comments:
In `@test/gut/test_nav_escape_sfx.gd`:
- Around line 221-226: The test is using the wrong input action for the
LineEdit/TextEdit bypass, so it does not exercise the code path guarded in
Globals._input(). Update the event in the nav escape SFX test to use ui_cancel
instead of ui_accept, and keep the assertion on _assert_sfx_not_called() so it
verifies the focused-text-entry suppression in the ui_cancel branch.
---
Nitpick comments:
In `@test/gut/test_nav_escape_sfx.gd`:
- Around line 107-112: The test helper `_simulate_input()` is exercising both
`Globals._input()` and `Globals._unhandled_input()`, which weakens the routing
assertion for the SFX contract. Update this helper to invoke only the
`_input(event)` path on `globals_instance` and remove the `_unhandled_input()`
call so the suite validates the behavior implemented in `Globals._input()`
consistently.
🪄 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: 9db4365b-30f4-4052-8f8f-2d3d0296f8aa
📒 Files selected for processing (5)
files/docs/milestones/21/Part_2_Route_menu_UI_SFX_through_AudioManager_&_add_GUT_tests.mdscripts/core/globals.gdscripts/ui/menus/main_menu.gdtest/gut/test_nav_escape_sfx.gdtest/gut/test_quit_game_confirm_dialog_sfx.gd
🚧 Files skipped from review as they are similar to previous changes (2)
- test/gut/test_quit_game_confirm_dialog_sfx.gd
- scripts/core/globals.gd
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: GUT Unit Tests / unit-test
- GitHub Check: CI/CD Infrastructure Tests / Test Godot Asset Infrastructure and Signature Verification
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-05-27T04:07:45.235Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 719
File: scripts/ui/menus/audio_settings.gd:300-313
Timestamp: 2026-05-27T04:07:45.235Z
Learning: In Godot 4 GDScript UI code (e.g., slider/drag signal handlers), treat high-frequency callbacks as potentially re-entrant: if you have a conditional state-guard (the “circuit breaker”) inside the handler, put the guard at the very top of the conditional block and perform it before any bus-specific calls, async/yield operations, or other side effects (like `play_sfx("check")` or token increments). This prevents rapid drag frames from re-entering before the first call finishes, which can cause duplicate SFX triggers and redundant state updates.
Applied to files:
scripts/ui/menus/main_menu.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:
scripts/ui/menus/main_menu.gdtest/gut/test_nav_escape_sfx.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/ui/menus/main_menu.gdtest/gut/test_nav_escape_sfx.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_nav_escape_sfx.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_nav_escape_sfx.gd
🪛 LanguageTool
files/docs/milestones/21/Part_2_Route_menu_UI_SFX_through_AudioManager_&_add_GUT_tests.md
[style] ~72-~72: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...r_testing = true` on the scene instance prior to executing test frames. #### 4. Infrast...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[uncategorized] ~106-~106: The official name of this software platform is spelled with a capital “H”.
Context: ... | Dockerfile.github/workflows/test_ci_scripts.yml ...
(GITHUB)
🔇 Additional comments (4)
scripts/ui/menus/main_menu.gd (4)
208-213: Single-path dismissal confirmed. Routing onlycanceled(covering Cancel button, Escape, and title-bar close) avoids the doubleui_canceltrigger flagged previously.
40-41: LGTM!
310-316: LGTM!
336-341: LGTM!
Apply editorial and markdown formatting fixes across README.md and milestone docs (milestones 18–20). Standardize bullet/list styles, table/link formatting, line-wrapping and spacing; add/remove blank lines for readability; fix minor typos and identifier/name inconsistencies in documentation (no code logic changes). Affected files: README.md and multiple files under files/docs/milestones/{18,19,20}.
|
@sourcery-ai guide |
Sharp observation! Since our UI navigation and cancellation SFX processing contract lives strictly within the _input() step, calling both input handlers in the simulation layer adds unnecessary noise and risks masking potential regressions or double-triggering drifts down the line. I have stripped the redundant _unhandled_input() execution path from the test suite's _simulate_input() helper. It now hooks directly into Globals._input() exclusively, ensuring maximum testing precision. The clean update has been pushed! |
Exercise only Globals._input() in this suite. These tests are validating the routing in scripts/core/globals.gd:384-447, but _simulate_input() also calls _unhandled_input(). That can let the suite pass if the SFX path drifts to _unhandled_input() or starts firing from both handlers. Sharp observation! Since our UI navigation and cancellation SFX processing contract lives strictly within the _input() step, calling both input handlers in the simulation layer adds unnecessary noise and risks masking potential regressions or double-triggering drifts down the line. I have stripped the redundant _unhandled_input() execution path from the test suite's _simulate_input() helper. It now hooks directly into Globals._input() exclusively, ensuring maximum testing precision. The clean update has been pushed!
Outstanding catch! This test was passing trivially because "ui_accept" is completely unhandled by our global input context manager, meaning it never would have fired audio anyway. I have updated test_value_editing_text_gate() to fire a "ui_cancel" event instead. This ensures we are genuinely asserting that active focus on a text element blocks the layout's cancellation chime. The fix is pushed! |
Use ui_cancel here; ui_accept never hits the LineEdit bypass.
Globals._input() only checks LineEdit/TextEdit focus inside the ui_cancel branch. With ui_accept, this test stays green even if that bypass regresses.
Based on scripts/core/globals.gd:384-447, the focused-text-entry suppression being asserted here is implemented only for event.is_action_pressed("ui_cancel", false).
Outstanding catch! This test was passing trivially because "ui_accept" is completely unhandled by our global input context manager, meaning it never would have fired audio anyway. I have updated test_value_editing_text_gate() to fire a "ui_cancel" event instead. This ensures we are genuinely asserting that active focus on a text element blocks the layout's cancellation chime. The fix is pushed!
We deliberately decided not to perform a major architectural overhaul or introduce new base files to avoid scope creep in a focused bug-fix Pull Request. Instead, we used production-safe environment guards to achieve 100% safety with minimal code footprint. Here is the exact status of those three points on our branch right now: 📋 Final Architectural Checklist1. Brittle Scene Name Reliance (
|
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-confirmation-cancellation-triggering
branch
1 participant
Use AudioManager.play_sfx for menu navigation/cancel sounds and add test helpers to support GUT. globals.gd: add a test-fallback menu detection via current_scene.name and route _play_ui_navigation_sfx through AudioManager. main_menu.gd: remove redundant cancel-button connection and trigger ui_cancel via AudioManager in _on_quit_dialog_canceled. Add comprehensive GUT tests (test_nav_escape_sfx.gd, test_quit_game_confirm_dialog_sfx.gd) with mock AudioManager scripts and UID files to validate navigation, cancel, and confirmation audio pathways.
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
Route menu UI navigation and cancel sounds through the central AudioManager and add automated coverage for menu audio behavior and Godot asset verification.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Tests:
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
ui_navigation/ui_cancelbehavior.Tests
Documentation
CI / Build
Bots/AI Contributions to PR #782
AI/Code Review Bots
These automated tools provided summaries, reviews, suggestions, and feedback that contributed to the PR's quality, structure, and testing improvements:
globals.gd, test fallback concerns, and dead code inmain_menu.gd. Provided high-level feedback and actionable prompts._on_cancel_button_clickedhandler, test snapshot logic improvements), and maintainability suggestions. Included poem and pre-merge checks.@deepsourcebot.No evidence of @dependabot or other dependency bots in this PR (all commits by human author; changes were manual code/test/CI updates).
Human Contributors
ui_navigation/ui_cancel) throughAudioManageringlobals.gdandmain_menu.gd; added comprehensive GUT tests (test_nav_escape_sfx.gd,test_quit_game_confirm_dialog_sfx.gdwith mocks/UIDs); introduced test helpers (e.g.,bypass_quit_for_testing, scene-name fallback for CI/debug); updated Dockerfile (SHA512 verification) and CI workflow (split jobs). Addressed review feedback iteratively across multiple commits.This PR focused on audio centralization, test coverage for UI interactions, and infrastructure hardening, with AI bots enhancing review depth and maintainability. The final output benefits from iterative human + bot collaboration.