Prevent duplicate ui_accept SFX and expand input guard tests#803
Conversation
Add unit tests to improve coverage of global audio hooks and input-guard behavior. - test/gut/test_globals_button_hooks.gd: add test_standard_button_pressed_executes_audio_output to verify deferred connection established on Button instantiation routes playback through AudioManager when pressed (functional coverage). - test/gut/test_globals_input_guards.gd: add tests test_line_edit_focus_skips_global_audio and test_text_edit_focus_skips_global_audio to ensure ui_accept events are bypassed when LineEdit/TextEdit are focused (branch coverage). Tests use existing helpers (_wait_for_registration, _setup_focused_control, _create_ui_accept_event) and AudioManager cleanup to isolate audio state.
ikostan
added this to the
Milestone 21: Implement HUD FPS Counter UI Component milestone
|
Warning Review limit reached
Next review available in: 34 seconds 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 (5)
📝 WalkthroughWalkthroughAdds a CI maintenance documentation file summarizing GitHub Actions workflow dependency updates, and introduces new GUT test cases validating global button-press audio routing and focus-based bypass of the ui_accept input interceptor for LineEdit and TextEdit controls. ChangesCI Maintenance Documentation
GUT Test Additions for Globals Audio Behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Jul 3, 2026 6:20a.m. | Review ↗ |
| JavaScript | |
Jul 3, 2026 6:20a.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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/gut/test_globals_input_guards.gd (1)
188-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating the six near-identical focus-skip tests into a parameterized helper.
With these two additions, the file now has six tests (
CheckButton,HSlider,Button,TextureButton,LineEdit,TextEdit) that all follow the identical arrange/act/assert shape — only the control type and failure message differ. A small helper (e.g._assert_focus_blocks_ui_accept(control, msg)) driven by a control-factory array would reduce duplication and make it trivial to add future control types.Not blocking; the current tests are correct and readable as-is.
🤖 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_globals_input_guards.gd` around lines 188 - 221, The new focus-skip tests in test_line_edit_focus_skips_global_audio and test_text_edit_focus_skips_global_audio duplicate the same arrange/act/assert pattern already used by the other ui_accept focus tests. Refactor these into a shared helper such as _assert_focus_blocks_ui_accept(control, message) and drive it with per-control factory data so LineEdit and TextEdit are covered alongside the existing CheckButton, HSlider, Button, and TextureButton cases without repeating the same setup and assertions.
🤖 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_4_Maintenance.md`:
- Line 37: The maintenance summary has a workflow count mismatch: it says seven
workflow files are touched, but the documented file list contains eight. Update
the count in the summary to match the file list, and use the surrounding
maintenance text in Part_4_Maintenance.md to keep the wording consistent.
In `@test/gut/test_globals_button_hooks.gd`:
- Around line 227-254: Remove the stray citation markers from the comments in
test_standard_button_pressed_executes_audio_output so the test reads cleanly.
Update the affected comment lines around add_child_autofree,
AudioManager.stop_all_sfx/cleanup_for_test, and the await _wait_for_registration
call to keep the intended test explanation without the leftover [cite: ###]
artifacts.
In `@test/gut/test_globals_input_guards.gd`:
- Around line 186-221: Remove the leftover citation markers from the comments in
test_line_edit_focus_skips_global_audio and
test_text_edit_focus_skips_global_audio. Clean up the inline “[cite: 788]”,
“[cite: 791]”, and “[cite: 789]” artifacts in both the LineEdit and TextEdit
blocks so the test names and
_setup_focused_control/_create_ui_accept_event/_input assertions remain
unchanged, just without the stray annotations.
---
Nitpick comments:
In `@test/gut/test_globals_input_guards.gd`:
- Around line 188-221: The new focus-skip tests in
test_line_edit_focus_skips_global_audio and
test_text_edit_focus_skips_global_audio duplicate the same arrange/act/assert
pattern already used by the other ui_accept focus tests. Refactor these into a
shared helper such as _assert_focus_blocks_ui_accept(control, message) and drive
it with per-control factory data so LineEdit and TextEdit are covered alongside
the existing CheckButton, HSlider, Button, and TextureButton cases without
repeating the same setup and assertions.
🪄 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: e0778db1-0703-45d9-9a1d-30b3166c9730
📒 Files selected for processing (3)
files/docs/milestones/21/Part_4_Maintenance.mdtest/gut/test_globals_button_hooks.gdtest/gut/test_globals_input_guards.gd
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: CI/CD Infrastructure Tests / Test Godot Asset Infrastructure and Signature Verification
- GitHub Check: GUT Unit Tests / unit-test
🧰 Additional context used
🧠 Learnings (4)
📚 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_input_guards.gdtest/gut/test_globals_button_hooks.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_input_guards.gdtest/gut/test_globals_button_hooks.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_input_guards.gdtest/gut/test_globals_button_hooks.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_input_guards.gdtest/gut/test_globals_button_hooks.gd
🪛 LanguageTool
files/docs/milestones/21/Part_4_Maintenance.md
[uncategorized] ~13-~13: The official name of this software platform is spelled with a capital “H”.
Context: ...ywright, and Butler artifacts. - Update github/codeql-action/upload-sarif to a newer p...
(GITHUB)
[uncategorized] ~25-~25: The official name of this software platform is spelled with a capital “H”.
Context: ... of actions/cache, release-drafter, and github/codeql upload-sarif across several GitH...
(GITHUB)
[uncategorized] ~31-~31: The official name of this software platform is spelled with a capital “H”.
Context: ...or cache implementation.
(GITHUB)
[uncategorized] ~31-~31: The official name of this software platform is spelled with a capital “H”.
Context: ...inning for reproducibility.
.github/workflows/browser_test.yml`.githu...
(GITHUB)
[uncategorized] ~37-~37: The official name of this software platform is spelled with a capital “H”.
Context: ...r release-drafter/release-drafter and github/codeql-action/upload-sarif). The chang...
(GITHUB)
[uncategorized] ~48-~48: The official name of this software platform is spelled with a capital “H”.
Context: ...s, applied labels (CI/CD, dependencies, github_actions), self-assigned, and managed pr...
(GITHUB)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This says the changes touch seven workflow files, but the table lists eight. Please update the count to match the documented file list.
|
@sourcery-ai review |
Reviewer's GuideStrengthens the global ui_accept focus guard to prevent duplicate confirmation SFX, refactors and extends GUT input-guard tests (including new coverage for text inputs and button hooks), and documents CI workflow maintenance and automation contributions in milestone docs. File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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:
- The new input guard tests for LineEdit and TextEdit duplicate setup/teardown logic; consider extracting a shared helper to reduce repetition and keep future branch-coverage additions simpler.
- In
test_standard_button_pressed_executes_audio_output, the additional_wait_for_registration()after emittingpressedcould be clarified or tightened if only signal delivery is needed, to avoid depending on potentially unrelated deferred work in the registration pipeline. - Some of the test comments are very verbose and narrative; trimming them to focus on the specific behavior under test would make the tests easier to scan and maintain over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new input guard tests for LineEdit and TextEdit duplicate setup/teardown logic; consider extracting a shared helper to reduce repetition and keep future branch-coverage additions simpler.
- In `test_standard_button_pressed_executes_audio_output`, the additional `_wait_for_registration()` after emitting `pressed` could be clarified or tightened if only signal delivery is needed, to avoid depending on potentially unrelated deferred work in the registration pipeline.
- Some of the test comments are very verbose and narrative; trimming them to focus on the specific behavior under test would make the tests easier to scan and maintain over time.
## Individual Comments
### Comment 1
<location path="files/docs/milestones/21/Part_4_Maintenance.md" line_range="25" />
<code_context>
+
+## Reviewer's Guide
+
+This PR performs maintenance on CI workflows by updating pinned versions of actions/cache, release-drafter, and github/codeql upload-sarif across several GitHub Actions workflows to newer, presumably patched SHAs and major versions.
+
+### File-Level Changes
</code_context>
<issue_to_address>
**issue (typo):** Consider correcting `github/codeql upload-sarif` to `github/codeql-action/upload-sarif` for accuracy and consistency.
Elsewhere in this doc you already use `github/codeql-action/upload-sarif`; update this occurrence to match so the action name stays accurate throughout.
```suggestion
This PR performs maintenance on CI workflows by updating pinned versions of actions/cache, release-drafter, and github/codeql-action/upload-sarif across several GitHub Actions workflows to newer, presumably patched SHAs and major versions.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Thanks for the suggestion! To eliminate the internal code duplication while keeping the test suite highly readable, I refactored the common setup and assertions into a shared helper function: _assert_focus_blocks_ui_accept(control, failure_message). I chose to keep the six test scenarios as separate, explicitly named methods rather than compressing them into a single loop array. This ensures that if a specific control regressions in the future, the exact failing scenario shows up instantly and clearly by name in the GUT test runner logs. |
Introduces `_assert_focus_blocks_ui_accept()` helper to consolidate the Arrange/Act/Assert pattern used across multiple test functions. Refactors 6 test functions to use the helper, eliminating code duplication. Removes verbose comments and reorders test functions for better organization.
Apprised and updated. Pushed fixes for points 1 and 2—extracted the text input duplication into a shared helper and optimized the deferred signal test yield to a single frame. |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
_assert_focus_blocks_ui_accept, you’re stopping SFX and releasing focus inside the helper even thoughafter_each()already handles cleanup; consider centralizing teardown inafter_each()so the helper is purely about assertion logic and doesn’t double-handle lifecycle concerns. - The new
_assert_focus_blocks_ui_accepthelper currently hardcodes theui_acceptpath; if you expect to guard other actions in the future, it might be worth parameterizing the action name or event factory to keep this reusable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_assert_focus_blocks_ui_accept`, you’re stopping SFX and releasing focus inside the helper even though `after_each()` already handles cleanup; consider centralizing teardown in `after_each()` so the helper is purely about assertion logic and doesn’t double-handle lifecycle concerns.
- The new `_assert_focus_blocks_ui_accept` helper currently hardcodes the `ui_accept` path; if you expect to guard other actions in the future, it might be worth parameterizing the action name or event factory to keep this reusable.
## Individual Comments
### Comment 1
<location path="files/docs/milestones/21/Part_4_Maintenance.md" line_range="31" />
<code_context>
+| Update GitHub Actions workflow dependencies to newer pinned versions for caching, release drafting, and SARIF uploads. | <ul><li>Bump actions/cache from v5 to v6 for PIP and Playwright caches in browser_test.yml, gdlint.yml, deploy_to_itch.yml, and yamllint.yml to use the latest major cache implementation.</li><li>Update github/codeql-action/upload-sarif pinned SHA in snyk.yml and trivy.yml to a newer commit while keeping version alignment and security pinning.</li><li>Update release-drafter/release-drafter pinned SHA in release_drafter.yml and release_drafter_pr.yml to a newer commit, maintaining SHA pinning for reproducibility.</li></ul> | `.github/workflows/browser_test.yml`<br/>`.github/workflows/snyk.yml`<br/>`.github/workflows/deploy_to_itch.yml`<br/>`.github/workflows/gdlint.yml`<br/>`.github/workflows/release_drafter.yml`<br/>`.github/workflows/release_drafter_pr.yml`<br/>`.github/workflows/trivy.yml`<br/>`.github/workflows/yamllint.yml` |
</code_context>
<issue_to_address>
**suggestion (typo):** Consider changing "PIP" to "pip" for consistency with common usage and earlier references.
This keeps terminology aligned with the rest of the document and with the usual lowercase spelling of the Python package manager.
```suggestion
| Update GitHub Actions workflow dependencies to newer pinned versions for caching, release drafting, and SARIF uploads. | <ul><li>Bump actions/cache from v5 to v6 for pip and Playwright caches in browser_test.yml, gdlint.yml, deploy_to_itch.yml, and yamllint.yml to use the latest major cache implementation.</li><li>Update github/codeql-action/upload-sarif pinned SHA in snyk.yml and trivy.yml to a newer commit while keeping version alignment and security pinning.</li><li>Update release-drafter/release-drafter pinned SHA in release_drafter.yml and release_drafter_pr.yml to a newer commit, maintaining SHA pinning for reproducibility.</li></ul> | `.github/workflows/browser_test.yml`<br/>`.github/workflows/snyk.yml`<br/>`.github/workflows/deploy_to_itch.yml`<br/>`.github/workflows/gdlint.yml`<br/>`.github/workflows/release_drafter.yml`<br/>`.github/workflows/release_drafter_pr.yml`<br/>`.github/workflows/trivy.yml`<br/>`.github/workflows/yamllint.yml` |
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Great catch on both fronts. I've updated the test suite with these exact optimizations: Teardown Separation: Purged the manual focus release and sound termination from the assertion block, leaving execution safety completely to the centralized after_each() lifecycle sweep. Generalization: Renamed the helper to _assert_focus_blocks_action and added an optional action_name string parameter (defaulting to "ui_accept"), decoupling it from hardcoded events for easier future expansion. Pushed and ready for a green light!
Great catch on both fronts. I've updated the test suite with these exact optimizations:
Pushed and ready for a green light! |
|
@sourcery-ai title |
sourcery-ai
Bot
changed the title
|
@sourcery-ai guide |
1 participant
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
Strengthen UI input audio guarding and documentation around CI maintenance workflows to prevent duplicate confirmation sounds and capture recent automation updates.
Bug Fixes:
Enhancements:
Documentation:
Summary by CodeRabbit
Documentation
Tests
Bots/AI Contributions to PR #803
This PR primarily addresses a UI confirmation audio double-trigger bug for
ui_acceptevents (linked to issue #787), adds/enhances GUT unit tests for global audio hooks and input guards, and includes maintenance documentation for CI workflow updates. While human oversight integrated the changes, bots and AI tools handled the bulk of dependency updates, code reviews, summaries, refactoring suggestions, and quality checks.AI/Bot Contributors
actions/cachefrom v5 to v6, refreshing pinned SHAs forrelease-drafter/release-drafterandgithub/codeql-action/upload-sarifacross multiple workflows likebrowser_test.yml,deploy_to_itch.yml, security scans, etc.). Automated commits were merged into this PR.Part_4_Maintenance.md). Provided structured analysis of changes, related PR context, and estimated review effort.These tools collectively managed most technical updates, validation, documentation polishing, and review processes, showcasing effective automation for CI hygiene and test coverage.
Human Contributors
globals.gd, added/expanded tests (test_globals_button_hooks.gd,test_globals_input_guards.gd), created and refined the maintenance doc (Part_4_Maintenance.md), merged bot contributions, applied labels (bug, testing, CI/CD, etc.), self-assigned, and handled milestone/project tracking.Overall Impact: The PR strengthens audio input handling to prevent duplicates, expands test coverage for UI controls (buttons, sliders, LineEdit, TextEdit, etc.), and documents bot/AI-driven CI maintenance. Automation significantly reduced manual effort while maintaining quality.