Port tree node observation loop into audiomanager - #817
Merged
ikostan merged 23 commits intoJul 14, 2026
Conversation
Port automatic Button pressed hooking from Globals to AudioManager (Issue #800). Globals no longer connects node_added and its handler is commented out. AudioManager now registers node_added in _ready and provides _on_node_added/_on_global_button_pressed (preserving flat/dialog protections and CONNECT_DEFERRED). Tests updated to call AudioManager hooks and include a stub _on_node_added in the audio mock used by the suite. This centralizes UI SFX routing and addresses lifecycle/connection concerns referenced in #763/#800.
Clean up .github/workflows/browser_test.yml: add yamllint disable/enable around long lines, update Playwright cache comment, standardize quoting for if expressions and string fields (node-version, artifact names, file paths, codecov params), and re-indent the embedded Python server block for readability. These are formatting/linting improvements only; no functional changes intended.
ikostan
added this to the
Milestone 21: Implement Global UI SFX Layer & Decouple Settings milestone
ikostan
added
CI/CD
github actions
Contributor
Reviewer's GuidePorts global UI button audio wiring and SceneTree observation from Globals into AudioManager, adds retroactive scene scanning and strict listener guards, and updates tests and CI workflows to validate the new behavior. Sequence diagram for AudioManager-driven global button SFX wiringsequenceDiagram
participant SceneTree
participant AudioManager
participant Button
participant AudioConstants
Note over AudioManager: Initialization
AudioManager->>SceneTree: get_tree()
AudioManager->>SceneTree: node_added.is_connected(_on_node_added)
alt [listener not connected]
AudioManager->>SceneTree: node_added.connect(_on_node_added)
end
AudioManager->>SceneTree: _retroactive_ui_scan(root)
loop Retroactive scan
AudioManager->>SceneTree: _retroactive_ui_scan(node)
alt [node.get_class() == Button]
AudioManager->>Button: _on_node_added(node)
end
end
Note over SceneTree,AudioManager: Runtime node addition
SceneTree-->>AudioManager: node_added(node)
AudioManager->>AudioManager: _on_node_added(node)
alt [node.get_class() == Button]
AudioManager->>Button: cast to Button
alt [btn.flat or btn.has_meta(no_global_sound)]
AudioManager-->>Button: return (no hook)
else [btn inside AcceptDialog]
AudioManager-->>Button: return (no hook)
else [eligible button]
AudioManager->>Button: btn.pressed.is_connected(_on_global_button_pressed)
alt [not connected]
AudioManager->>Button: btn.pressed.connect(_on_global_button_pressed, CONNECT_DEFERRED)
end
end
end
Note over Button,AudioManager: User presses button
Button-->>AudioManager: pressed -> _on_global_button_pressed()
AudioManager->>AudioManager: _on_global_button_pressed()
AudioManager->>AudioConstants: use BUS_SFX_MENU
AudioManager->>AudioManager: play_sfx(ui_accept, AudioConstants.BUS_SFX_MENU)
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAudioManager now owns automatic sound wiring for eligible buttons, including retroactive scanning and duplicate-connection protection. Globals’ previous handlers are removed, tests follow the new ownership, milestone documentation is added, and CI workflows receive formatting and GDToolkit installation updates. ChangesAudio button wiring
CI workflow updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SceneTree
participant AudioManager
participant Button
participant MenuSFX
SceneTree->>AudioManager: node_added(Button)
AudioManager->>Button: connect pressed signal
Button->>AudioManager: pressed
AudioManager->>MenuSFX: play ui_accept on BUS_SFX_MENU
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 |
This was
linked to
issues
Jul 12, 2026
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The old button hook implementation in
globals.gdis now fully commented out but still present; consider either removing it or replacing it with a brief comment pointing to the newAudioManagerimplementation to avoid future divergence and confusion. - With
get_tree().node_addednow connected inAudioManager._ready, this implicitly relies on a single AudioManager being instantiated early and only once; if there’s any chance of multiple instances or alternative boot paths, it may be worth adding a guard or assertion to ensure only one listener is attached.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The old button hook implementation in `globals.gd` is now fully commented out but still present; consider either removing it or replacing it with a brief comment pointing to the new `AudioManager` implementation to avoid future divergence and confusion.
- With `get_tree().node_added` now connected in `AudioManager._ready`, this implicitly relies on a single AudioManager being instantiated early and only once; if there’s any chance of multiple instances or alternative boot paths, it may be worth adding a guard or assertion to ensure only one listener is attached.
## Individual Comments
### Comment 1
<location path="scripts/managers/audio_manager.gd" line_range="65-66" />
<code_context>
# Initialize the SFX object pool
_initialize_sfx_pool()
+ # NEW: Connect global listener to monitor all runtime UI instantiation tracks (Issue #800)
+ get_tree().node_added.connect(_on_node_added)
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Buttons instantiated before AudioManager._ready() will never be wired up to global SFX.
By moving this listener from `Globals` to `AudioManager`, any `Button` nodes created before `AudioManager._ready()` fires will never trigger the `node_added` handler and thus won’t get their `pressed` signal hooked up to global SFX. If that’s a problem for your UI flow, either guarantee `AudioManager` is initialized very early in the scene tree, or add a one-time scan in `_ready()` to connect existing `Button` instances retroactively.
</issue_to_address>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 |  |
Jul 13, 2026 5:11a.m. | Review ↗ |
| JavaScript | |
Jul 13, 2026 5:11a.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
🧹 Nitpick comments (2)
scripts/core/globals.gd (1)
683-710: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove commented-out dead code.
The old
_on_node_addedand_on_global_button_pressedmethods are fully commented out and now live inAudioManager. Keeping 28 lines of stale code creates confusion and risks accidental reactivation. Remove them entirely.♻️ Proposed cleanup
- -## Automatically hooks up base Button elements for confirmation sfx -#func _on_node_added(node: Node) -> void: -# # FIXED: Use strict string evaluation to satisfy the Issue `#763` contract -# if node.get_class() == "Button": -# var btn := node as Button -# if is_instance_valid(btn): -# # Flat Button Protection: Avoid superimposing global audio over theme audio -# if btn.flat or btn.has_meta("no_global_sound"): -# return -# -# # Dialog Protection: Exclude internal buttons of Accept/ConfirmationDialogs -# var parent := btn.get_parent() -# while parent: -# if parent is AcceptDialog: -# return -# parent = parent.get_parent() -# -# # Guard against duplicate connections using the explicit named callable. -# # NOTE: CONNECT_DEFERRED is strictly required here to pass the Issue `#763` -# # verification contract and guarantee thread-safe scene tree execution. -# if not btn.pressed.is_connected(_on_global_button_pressed): -# btn.pressed.connect(_on_global_button_pressed, CONNECT_DEFERRED) - -## Centralized button audio execution target to prevent lambda churn -#func _on_global_button_pressed() -> void: -# # Explicitly route button accepts through the shared UI SFX bus to guarantee consistent -# # muting behavior -# AudioManager.play_sfx("ui_accept", AudioConstants.BUS_SFX_MENU)🤖 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 683 - 710, Remove the fully commented-out _on_node_added and _on_global_button_pressed method blocks from globals.gd, including their associated comments, while leaving the active globals implementation unchanged.test/gut/test_quit_game_confirm_dialog_sfx.gd (1)
26-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock's
_on_node_addedstub makestest_flat_button_anti_trigger_protectionineffective.The mock script replaces AudioManager's entire script with a
_on_node_addedthat doespass. Whentest_flat_button_anti_trigger_protectioncallsAudioManager._on_node_added(start_button), it invokes the mock's no-op stub — no connection is made regardless of whether the button is flat. The test then emitspressedand assertsui_acceptwas not called, which passes trivially because no handler was ever connected, not because the flat-flag guard worked.This means the test cannot detect a regression in the flat-button protection logic. The real coverage exists in
test_globals_button_hooks.gd::test_flat_button_is_ignored_by_hook, so consider either removing this redundant test or updating the mock to replicate the real_on_node_addedgating logic.Based on learnings from PR 782: for
*_sfx.gdGUT tests, verify the sound-triggering path through the real global listener pipeline rather than a stub that bypasses it.♻️ Proposed option: remove redundant test
- -## Assert that standard menu buttons do not trigger global confirmation requests on ui_accept. -## :rtype: void -func test_flat_button_anti_trigger_protection() -> void: - var start_button: Button = Button.new() - start_button.flat = true - main_menu_instance.add_child(start_button) - - # FIX: Explicitly drive the button through the global connection hook to mimic tree entry - # OLD: Globals._on_node_added(start_button) - AudioManager._on_node_added(start_button) - await get_tree().process_frame - - # FIX: Directly emit the pressed signal to verify the global hook was successfully blocked - start_button.pressed.emit() - await get_tree().process_frame - - # Global accept confirmation must remain untouched to respect native inspector themes - _assert_sfx_not_called("ui_accept")Also applies to: 126-127
🤖 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 26 - 29, Remove the redundant test_flat_button_anti_trigger_protection coverage from test_quit_game_confirm_dialog_sfx.gd, since it invokes the no-op _on_node_added stub and cannot validate flat-button protection. Keep the existing real pipeline coverage in test_globals_button_hooks.gd::test_flat_button_is_ignored_by_hook, and remove the mock stub only if no remaining tests require it.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 @.github/workflows/browser_test.yml:
- Around line 256-265: Quote the Codecov token expression in the “Upload
Coverage to Codecov” step by updating the token field to a quoted string, while
leaving the surrounding action configuration unchanged.
---
Nitpick comments:
In `@scripts/core/globals.gd`:
- Around line 683-710: Remove the fully commented-out _on_node_added and
_on_global_button_pressed method blocks from globals.gd, including their
associated comments, while leaving the active globals implementation unchanged.
In `@test/gut/test_quit_game_confirm_dialog_sfx.gd`:
- Around line 26-29: Remove the redundant
test_flat_button_anti_trigger_protection coverage from
test_quit_game_confirm_dialog_sfx.gd, since it invokes the no-op _on_node_added
stub and cannot validate flat-button protection. Keep the existing real pipeline
coverage in test_globals_button_hooks.gd::test_flat_button_is_ignored_by_hook,
and remove the mock stub only if no remaining tests require it.
🪄 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: 0667d936-c0d9-4a44-a8bb-e0b121c652de
📒 Files selected for processing (5)
.github/workflows/browser_test.ymlscripts/core/globals.gdscripts/managers/audio_manager.gdtest/gut/test_globals_button_hooks.gdtest/gut/test_quit_game_confirm_dialog_sfx.gd
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: GUT Unit Tests / unit-test
- GitHub Check: GDUnit4 Unit Tests / unit-test
- GitHub Check: CI/CD Infrastructure Tests / Test Godot Asset Infrastructure and Signature Verification
- GitHub Check: CodeRabbit
- GitHub Check: Sourcery review
🧰 Additional context used
🧠 Learnings (7)
📚 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_globals_button_hooks.gdtest/gut/test_quit_game_confirm_dialog_sfx.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_globals_button_hooks.gdtest/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_globals_button_hooks.gdscripts/core/globals.gdscripts/managers/audio_manager.gdtest/gut/test_quit_game_confirm_dialog_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:
test/gut/test_globals_button_hooks.gdscripts/core/globals.gdscripts/managers/audio_manager.gdtest/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
📚 Learning: 2026-04-28T02:11:45.806Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 588
File: .github/workflows/deploy_to_itch.yml:44-56
Timestamp: 2026-04-28T02:11:45.806Z
Learning: When a CI workflow edits Godot's `project.godot` (INI) to inject custom ProjectSettings values, insert the setting key under the correct section header that matches the `game/` (or other) root in the ProjectSettings path. For example, `ProjectSettings.get_setting("game/security/save_salt", ...)` expects the INI entry under `[game]` with key `security/save_salt` (i.e., `[game]` then `security/save_salt=...`), not under `[application]`. Otherwise the lookup will fall back to the default value at runtime.
Applied to files:
.github/workflows/browser_test.yml
📚 Learning: 2026-05-20T00:01:27.632Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 654
File: .github/workflows/browser_test.yml:99-101
Timestamp: 2026-05-20T00:01:27.632Z
Learning: In this repository’s GitHub Actions workflows, treat supply-chain pinning as follows:
- **Do not flag** steps that use **first-party** GitHub-owned actions under `actions/*` (e.g., `actions/checkout`, `actions/cache`) when they use a **major version tag** like `v6` / `v5`.
- **Do flag** **third-party** actions (anything not under `actions/*`, e.g., `firebelley/godot-export`, `codecov/codecov-action`) when they use an unpinned ref such as `vX` or `main` instead of being pinned to a **commit SHA** (i.e., `@<commit-sha>`).
Applied to files:
.github/workflows/browser_test.yml
🪛 GitHub Check: YAML Lint / build (3.x)
.github/workflows/browser_test.yml
[warning] 264-264:
264:18 [quoted-strings] string value is not quoted with double quotes
🔇 Additional comments (6)
.github/workflows/browser_test.yml (1)
109-138: LGTM!Also applies to: 164-164, 215-218, 241-255, 267-275
scripts/managers/audio_manager.gd (2)
62-67: LGTM!
556-583: LGTM!scripts/core/globals.gd (1)
55-55: LGTM!test/gut/test_globals_button_hooks.gd (1)
41-42: LGTM!Also applies to: 63-64, 234-235
test/gut/test_quit_game_confirm_dialog_sfx.gd (1)
14-14: LGTM!
Connects get_tree().node_added in _ready and adds a recursive _retroactive_ui_scan(node) to walk the existing scene tree and hook UI nodes instantiated before the autoload was ready (addresses Issue #800). Also adds two GUT tests to ensure pre-existing buttons are captured and that the scan is idempotent and does not double-bind connections.
Owner
Author
Fixed |
Owner
Author
Excellent point. Relying on an Autoload structure usually guarantees a single initialization, but alternate boot paths, script doubling during unit tests, or manual re-initialization workflows could easily cause duplicate listener registrations. I have added an explicit .is_connected() defensive guard directly inside _ready() before binding the scene tree signal to guarantee that only a single instance of the listener is ever attached. Additionally, I've appended a new GUT architectural integration test to verify that this guard successfully prevents double-binding if secondary setup loops run. Resolving |
Add an is_connected() guard in AudioManager._ready to prevent duplicate connections to SceneTree.node_added (addresses Issue #800). Also add a GUT test (test_audiomanager_listener_registration_is_strictly_singular) that asserts the listener is registered exactly once and that subsequent connect attempts do not create duplicate bindings.
Owner
Author
|
@sourcery-ai review |
Owner
Author
|
@sourcery-ai review |
Owner
Author
Agreed. While I recently updated the mock to inherit from the production script (which fixed the immediate blind spot), you are right that this specific test is redundant here. test_globals_button_hooks.gd already covers flat-button isolation comprehensively. I have removed the redundant test to keep this suite strictly focused on the quit dialog logic. Resolving! |
Mock's _on_node_added stub makes test_flat_button_anti_trigger_protection ineffective. The mock script replaces AudioManager's entire script with a _on_node_added that does pass. When test_flat_button_anti_trigger_protection calls AudioManager._on_node_added(start_button), it invokes the mock's no-op stub — no connection is made regardless of whether the button is flat. The test then emits pressed and asserts ui_accept was not called, which passes trivially because no handler was ever connected, not because the flat-flag guard worked. This means the test cannot detect a regression in the flat-button protection logic. The real coverage exists in test_globals_button_hooks.gd::test_flat_button_is_ignored_by_hook, so consider either removing this redundant test or updating the mock to replicate the real _on_node_added gating logic. Based on learnings from PR 782: for *_sfx.gd GUT tests, verify the sound-triggering path through the real global listener pipeline rather than a stub that bypasses it.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the GitHub Actions workflows, several
if:conditions and inputs are now wrapped in quotes (e.g.if: "steps.playwright-cache.outputs.cache-hit != 'true'",if: "always()",token: "${{ secrets.CODECOV_TOKEN }}"), which may cause GitHub to treat them as literal strings rather than expressions; consider reverting these to the unquoted expression form to avoid breaking conditional execution and interpolation. - The
_retroactive_ui_scanhelper currently calls_on_node_addedon every node in the tree, even non-UI nodes; you might want to early-return based onnode.get_class()or a type check before calling_on_node_addedto reduce unnecessary work on large scene trees. - In
test_audiomanager_listener_registration_is_strictly_singular, the secondconnectis guarded byif not get_tree().node_added.is_connected(AudioManager._on_node_added), so the test never actually attempts a duplicate registration; consider forcing a secondconnectunconditionally to verify that the guard in_ready()truly prevents multiple listener attachments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the GitHub Actions workflows, several `if:` conditions and inputs are now wrapped in quotes (e.g. `if: "steps.playwright-cache.outputs.cache-hit != 'true'"`, `if: "always()"`, `token: "${{ secrets.CODECOV_TOKEN }}"`), which may cause GitHub to treat them as literal strings rather than expressions; consider reverting these to the unquoted expression form to avoid breaking conditional execution and interpolation.
- The `_retroactive_ui_scan` helper currently calls `_on_node_added` on every node in the tree, even non-UI nodes; you might want to early-return based on `node.get_class()` or a type check before calling `_on_node_added` to reduce unnecessary work on large scene trees.
- In `test_audiomanager_listener_registration_is_strictly_singular`, the second `connect` is guarded by `if not get_tree().node_added.is_connected(AudioManager._on_node_added)`, so the test never actually attempts a duplicate registration; consider forcing a second `connect` unconditionally to verify that the guard in `_ready()` truly prevents multiple listener attachments.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/gdlint.yml (1)
31-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin GDToolkit to the repository’s declared version.
The workflow installs any future
4.xrelease, whilerequirements.txtpinsgdtoolkit==4.5.0. This can make lint results and CI reliability change without a repository update. Use the same exact version, or update the dependency pin and workflow together.Proposed fix
- pip install "gdtoolkit==4.*" + pip install "gdtoolkit==4.5.0"🤖 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 @.github/workflows/gdlint.yml around lines 31 - 33, Update the workflow’s gdtoolkit installation command to use the exact version declared in requirements.txt, gdtoolkit==4.5.0, instead of the floating 4.* constraint; keep the repository dependency pin and CI installation synchronized.
🤖 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_globals_button_hooks.gd`:
- Around line 336-376: Update the shared after_each() cleanup to verify that
AudioManager._on_node_added is connected to get_tree().node_added and reconnect
it when missing. Keep this guard idempotent so it restores listener state after
test_retroactive_scan_captures_pre_existing_buttons without creating duplicate
connections.
---
Nitpick comments:
In @.github/workflows/gdlint.yml:
- Around line 31-33: Update the workflow’s gdtoolkit installation command to use
the exact version declared in requirements.txt, gdtoolkit==4.5.0, instead of the
floating 4.* constraint; keep the repository dependency pin and CI installation
synchronized.
🪄 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: 941a5a1e-703f-49f9-9af9-00c265053ce2
📒 Files selected for processing (6)
.github/workflows/browser_test.yml.github/workflows/gdlint.ymlscripts/core/globals.gdscripts/managers/audio_manager.gdtest/gut/test_globals_button_hooks.gdtest/gut/test_quit_game_confirm_dialog_sfx.gd
💤 Files with no reviewable changes (1)
- scripts/core/globals.gd
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/managers/audio_manager.gd
- .github/workflows/browser_test.yml
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Sourcery review
- GitHub Check: GUT Unit Tests / unit-test
- GitHub Check: CI/CD Infrastructure Tests / Test Godot Asset Infrastructure and Signature Verification
- GitHub Check: Sourcery review
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2026-04-28T02:11:45.806Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 588
File: .github/workflows/deploy_to_itch.yml:44-56
Timestamp: 2026-04-28T02:11:45.806Z
Learning: When a CI workflow edits Godot's `project.godot` (INI) to inject custom ProjectSettings values, insert the setting key under the correct section header that matches the `game/` (or other) root in the ProjectSettings path. For example, `ProjectSettings.get_setting("game/security/save_salt", ...)` expects the INI entry under `[game]` with key `security/save_salt` (i.e., `[game]` then `security/save_salt=...`), not under `[application]`. Otherwise the lookup will fall back to the default value at runtime.
Applied to files:
.github/workflows/gdlint.yml
📚 Learning: 2026-05-20T00:01:27.632Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 654
File: .github/workflows/browser_test.yml:99-101
Timestamp: 2026-05-20T00:01:27.632Z
Learning: In this repository’s GitHub Actions workflows, treat supply-chain pinning as follows:
- **Do not flag** steps that use **first-party** GitHub-owned actions under `actions/*` (e.g., `actions/checkout`, `actions/cache`) when they use a **major version tag** like `v6` / `v5`.
- **Do flag** **third-party** actions (anything not under `actions/*`, e.g., `firebelley/godot-export`, `codecov/codecov-action`) when they use an unpinned ref such as `vX` or `main` instead of being pinned to a **commit SHA** (i.e., `@<commit-sha>`).
Applied to files:
.github/workflows/gdlint.yml
📚 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_globals_button_hooks.gdtest/gut/test_quit_game_confirm_dialog_sfx.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_globals_button_hooks.gdtest/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_globals_button_hooks.gdtest/gut/test_quit_game_confirm_dialog_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:
test/gut/test_globals_button_hooks.gdtest/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
🔇 Additional comments (2)
test/gut/test_globals_button_hooks.gd (1)
41-42: LGTM!Also applies to: 63-64, 234-235, 378-403, 405-438
test/gut/test_quit_game_confirm_dialog_sfx.gd (1)
14-34: LGTM!
Owner
Author
This configuration is intentional. Our repository's strict yamllint configuration enforces the quoted-strings rule, which flags any unquoted or bare expressions in YAML properties. We are intentionally wrapping these in double quotes to prevent linter violations and keep the CI green." |
Spot-on catch regarding the early abort behavior in GUT. If the precondition assertion fails, the engine listener would remain detached, polluting the state of all subsequent test sequences. I have implemented the proposed fix by moving the idempotent reconnection guard directly into the shared after_each() lifecycle cleanup routine. This guarantees the listener is always restored cleanly between runs, avoiding runtime state leakage. Resolving!
Owner
Author
Excellent point regarding scene tree scale efficiency. Passing thousands of non-UI spatial, structural, or timing nodes into the full validation function creates unnecessary call stack overhead during scene initialization. I have updated _retroactive_ui_scan to perform a lightweight inline class filter check before invoking _on_node_added(). This eliminates function call churn on non-button elements while ensuring the recursive loop still safely crawls through nested container hierarchies. Resolving! |
Skip unnecessary function calls on non-Button components in the retroactive scan by checking node class before processing. This reduces overhead while maintaining full tree traversal to discover nested buttons. Adds comprehensive test verifying the optimization correctly handles mixed hierarchies with structural nodes, containers, and leaf buttons.
Updated test to directly call _ready() instead of manually connecting the signal, providing a more accurate simulation of how duplicate listeners could occur in production code when _ready() is invoked multiple times.
Owner
Author
"Great catch. Replicating the defensive if guard inside the test block turned it into a false positive since the test code was short-circuiting before doing any real work[cite: 11]. I have updated the test to invoke AudioManager._ready() unconditionally. This accurately simulates a secondary initialization/alternate boot sequence, verifying that the production guard inside _ready() successfully drops duplicate connection requests. Resolving! |
Owner
Author
|
@sourcery-ai guide |
Owner
Author
|
@sourcery-ai guide |
Owner
Author
|
@sourcery-ai guide |
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In both
_on_node_addedand_retroactive_ui_scan, usingnode.get_class() == "Button"means subclasses ofButtonwill not be wired for SFX; if you expect custom button types to behave like standard buttons, consider switching tonode is Buttonor a more flexible type check. - The retroactive scan currently calls
_on_node_addedfor everyButtonit encounters, which re-runs all filtering and connection logic; consider extracting the core wiring into a dedicated helper so that the node-added handler and retroactive scan share a single, easier-to-maintain implementation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In both `_on_node_added` and `_retroactive_ui_scan`, using `node.get_class() == "Button"` means subclasses of `Button` will not be wired for SFX; if you expect custom button types to behave like standard buttons, consider switching to `node is Button` or a more flexible type check.
- The retroactive scan currently calls `_on_node_added` for every `Button` it encounters, which re-runs all filtering and connection logic; consider extracting the core wiring into a dedicated helper so that the node-added handler and retroactive scan share a single, easier-to-maintain implementation.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
files/docs/milestones/21/Part_8_Port_tree_node_observation_loop_into_audio_manager.md (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse consistent "AudioManager" casing in the title.
The heading uses "audiomanager" (lowercase, single word) while the rest of the document consistently uses "AudioManager" (PascalCase, two words). Align the title for consistency.
✏️ Proposed fix
-# 🏁 Port tree node observation loop into audiomanager +# 🏁 Port tree node observation loop into AudioManager🤖 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 `@files/docs/milestones/21/Part_8_Port_tree_node_observation_loop_into_audio_manager.md` at line 1, Update the document’s heading to use the consistent “AudioManager” casing and spacing instead of “audiomanager,” leaving the rest of the title unchanged.
🤖 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_8_Port_tree_node_observation_loop_into_audio_manager.md`:
- Line 15: Update the documentation statements at the “Globals” obsolete-code
summary and “commenting out obsolete hooks” entry to consistently state that the
button-hook implementation and previous handlers were removed from globals.gd,
not commented out. Preserve the surrounding milestone content.
---
Nitpick comments:
In
`@files/docs/milestones/21/Part_8_Port_tree_node_observation_loop_into_audio_manager.md`:
- Line 1: Update the document’s heading to use the consistent “AudioManager”
casing and spacing instead of “audiomanager,” leaving the rest of the title
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: 8ef6cafa-d239-46d6-a2d2-deba5eb03134
📒 Files selected for processing (4)
.github/workflows/browser_test.ymlfiles/docs/milestones/21/Part_8_Port_tree_node_observation_loop_into_audio_manager.mdscripts/managers/audio_manager.gdtest/gut/test_globals_button_hooks.gd
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/managers/audio_manager.gd
- test/gut/test_globals_button_hooks.gd
- .github/workflows/browser_test.yml
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: CI/CD Infrastructure Tests / Test Godot Asset Infrastructure and Signature Verification
🧰 Additional context used
🪛 LanguageTool
files/docs/milestones/21/Part_8_Port_tree_node_observation_loop_into_audio_manager.md
[uncategorized] ~22-~22: The official name of this software platform is spelled with a capital “H”.
Context: ... and spying. - CI/CD Improvements (.github/workflows/): - Normalized YAML quoti...
(GITHUB)
[uncategorized] ~50-~50: The official name of this software platform is spelled with a capital “H”.
Context: ... | .github/workflows/browser_test.yml
`.githu...
(GITHUB)
[uncategorized] ~60-~60: The official name of this software platform is spelled with a capital “H”.
Context: ...es and conditional if: expressions in .github/workflows/browser_test.yml so they con...
(GITHUB)
[uncategorized] ~61-~61: The official name of this software platform is spelled with a capital “H”.
Context: ...amllint [quoted-strings] warnings for .github/workflows/browser_test.yml in the CI Y...
(GITHUB)
[uncategorized] ~62-~62: The official name of this software platform is spelled with a capital “H”.
Context: ...lve yamllint line-length violations in .github/workflows/browser_test.yml (specificall...
(GITHUB)
[uncategorized] ~93-~93: The official name of this software platform is spelled with a capital “H”.
Context: ...wright cache, gdtoolkit installation in .github/workflows/). - Addressing linked issue...
(GITHUB)
Owner
Author
Fixed |
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
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: ''
PR #817 Summary: Port Tree Node Observation Loop into AudioManager
Overview
This pull request by @ikostan refactors the global UI button sound effects (SFX) handling in the SkyLockAssault Godot project. It migrates the automatic
Buttonpressedsignal hooking logic (previously inGlobals) into the centralizedAudioManagersingleton. This improves architecture, lifecycle management, and maintainability while addressing related issues around UI audio consistency.Key Changes
Core Refactoring (
scripts/managers/audio_manager.gd,scripts/core/globals.gd):AudioManagernow registers a guarded listener toSceneTree.node_addedin_ready()._retroactive_ui_scan()for recursive traversal to hook pre-existing buttons created before the manager initializes.no_global_soundmetadata, buttons insideAcceptDialogs) and duplicate-connection guards usingCONNECT_DEFERRED._on_global_button_pressed()to routeui_acceptSFX via the configured menu bus.Globalsis commented out/removed.Testing Updates (
test/gut/):AudioManager._on_global_button_pressed).CI/CD Improvements (
.github/workflows/):gdtoolkitpip installation.Benefits & Implications
AudioManager, reducing Globals bloat and lifecycle issues.AI/Bot Support
Status: Ready for review/merge. Includes sequence diagrams and thorough test coverage. No breaking changes to existing UI audio behavior.
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
Move global UI button observation and audio hook logic from Globals into AudioManager, adding retroactive tree scanning and stronger listener guards, while updating tests and CI workflows accordingly.
New Features:
Bug Fixes:
Enhancements:
CI:
Summary by Sourcery
Centralize global UI button SFX wiring in AudioManager, adding retroactive scene-tree scanning and guarded listener registration while cleaning up Globals and aligning workflows and docs with the new architecture.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
ui_acceptsound hooking for buttons created during gameplay by using scene-tree monitoring plus a retroactive scan for pre-existing buttons.Quality Improvements
Chores
Bots/AI Contributions Summary for PR #817
AI/Bot-Assisted Review and Automation
This PR benefited significantly from automated code review tools and AI assistants, which provided summaries, suggestions, walkthroughs, nitpicks, and quality checks. These contributions helped refine the refactoring (porting UI button observation logic to
AudioManager), improve tests, clean up CI workflows, and address potential issues like duplicate connections and retroactive scanning.Key bots/AI contributors (in GitHub-mentionable format):
globals.gdand guarding thenode_addedlistener). It also flagged the need for retroactive UI scanning for pre-existing buttons.These tools enhanced code quality, ensured consistency with project standards (e.g., GDScript/Godot practices), and streamlined the review process without direct code commits (all commits were authored by the human contributor).
@ikostan Contributions (Human Maintainer)
@ikostan drove the entire implementation through multiple targeted commits, handling the core refactoring, test updates, and CI fixes. Key efforts include:
ButtonSFX hooking logic fromGlobalstoAudioManager(including_on_node_added,_on_global_button_pressed, andCONNECT_DEFERREDguards)._retroactive_ui_scan) to handle buttons created beforeAudioManager._ready().test_globals_button_hooks.gd,test_quit_game_confirm_dialog_sfx.gd) for new behavior, idempotency, and lifecycle coverage.globals.gd(commenting out obsolete hooks) and refining CI workflows (YAML quoting, yamllint fixes, Playwright cache, gdtoolkit installation in.github/workflows/).This PR centralizes UI audio management, improves maintainability, and resolves several CI/lint issues while preserving existing behavior for flat buttons, dialog buttons, and metadata exclusions.
Overall Impact: Strong collaboration between human engineering and AI tooling resulted in a polished, well-tested change ready for merge. No dependency updates or external bot commits (e.g., no @dependabot activity here).