Conversation
Merge from master
Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 6.0.0 to 6.0.1. - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@57e3a13...e79a696) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-version: 6.0.1 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [actions/stale](https://github.com/actions/stale) from 10.2.0 to 10.3.0. - [Release notes](https://github.com/actions/stale/releases) - [Changelog](https://github.com/actions/stale/blob/main/CHANGELOG.md) - [Commits](actions/stale@v10.2.0...v10.3.0) --- updated-dependencies: - dependency-name: actions/stale dependency-version: 10.3.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [release-drafter/release-drafter](https://github.com/release-drafter/release-drafter) from 7.3.0 to 7.3.1. - [Release notes](https://github.com/release-drafter/release-drafter/releases) - [Commits](release-drafter/release-drafter@c2e2804...693d20e) --- updated-dependencies: - dependency-name: release-drafter/release-drafter dependency-version: 7.3.1 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
ikostan
added this to the
Milestone 19: UI Audio Infrastructure & Centralized SFX Systems milestone
Contributor
Reviewer's GuideImplements focus-aware auto-mute/unmute behavior with click SFX across all audio buses, centralizes UI mute toggle handling, hardens AudioManager SFX playback and asset integrity, and adapts tests and CI workflows to the new audio behavior and resource checks. Sequence diagram for auto-mute/unmute with click SFX on volume changesequenceDiagram
actor User
participant HSlider as VolumeSlider
participant AudioSettings as audio_settings_gd
participant AudioManager
participant AudioServer
User->>HSlider: drag()
HSlider->>AudioSettings: _on_global_volume_changed(bus_name, volume)
alt bus_name == BUS_MASTER
AudioSettings->>HSlider: master_slider.set_value_no_signal(volume)
else other buses
AudioSettings->>HSlider: _get_slider_for_bus(bus_name)
AudioSettings->>HSlider: slider.set_value_no_signal(volume)
end
alt volume <= AUTO_MUTE_VOLUME_THRESHOLD and not AudioManager.get_muted(bus_name)
AudioSettings->>AudioManager: set_muted(bus_name, true)
AudioSettings->>HSlider: _get_slider_for_bus(bus_name)
alt active_slider.has_focus()
alt bus_name == BUS_MASTER
AudioSettings->>AudioServer: get_bus_index(bus_name)
AudioSettings->>AudioServer: set_bus_volume_db(idx, linear_to_db(0.15))
AudioSettings->>AudioManager: play_sfx(check)
AudioSettings-->>AudioSettings: await create_timer(MUTE_HARDWARE_DELAY)
AudioSettings->>AudioServer: set_bus_volume_db(idx, linear_to_db(0.0))
else other buses
AudioSettings->>AudioManager: play_sfx(check)
end
end
else volume > AUTO_MUTE_VOLUME_THRESHOLD and AudioManager.get_muted(bus_name)
AudioSettings->>AudioManager: set_muted(bus_name, false)
AudioSettings->>HSlider: _get_slider_for_bus(bus_name)
alt active_slider.has_focus()
alt bus_name == BUS_MASTER
AudioSettings->>AudioServer: get_bus_index(bus_name)
AudioSettings->>AudioServer: set_bus_mute(idx, false)
AudioSettings->>AudioServer: set_bus_volume_db(idx, linear_to_db(volume))
end
AudioSettings->>AudioManager: play_sfx(check)
end
end
Sequence diagram for centralized mute toggle pipeline with deferred hardware mutesequenceDiagram
actor User
participant CheckButton as MuteButton
participant AudioSettings as audio_settings_gd
participant AudioManager
User->>CheckButton: toggle(toggled_on)
CheckButton->>AudioSettings: _on_*_mute_toggled(toggled_on)
AudioSettings->>AudioSettings: _execute_bus_mute_toggle(bus_name, toggled_on)
AudioSettings->>CheckButton: _get_mute_button_for_bus(bus_name)
AudioSettings->>AudioSettings: _get_slider_for_bus(bus_name)
AudioSettings-->>AudioSettings: has_user_focus = button.has_focus() or slider.has_focus()
alt has_user_focus
AudioSettings->>AudioManager: play_sfx(check)
end
AudioSettings->>AudioManager: set_muted(bus_name, not toggled_on)
AudioSettings->>AudioSettings: _update_ui_interactivity()
alt not toggled_on and has_user_focus
AudioSettings-->>AudioSettings: await create_timer(MUTE_HARDWARE_DELAY)
end
AudioSettings->>AudioManager: get_volume(bus_name)
AudioSettings->>AudioManager: get_muted(bus_name)
AudioSettings->>AudioManager: apply_volume_to_bus(bus_name, current_vol, is_muted)
AudioSettings->>AudioManager: save_volumes()
Sequence diagram for hardened SFX playback and missing asset handlingsequenceDiagram
participant Caller as AudioClient
participant AudioManager
participant ResourceLoader
Caller->>AudioManager: play_sfx(sfx_name, bus_name, max_instances, volume)
alt sfx_name in _missing_sfx_cache
AudioManager-->>Caller: return
else sfx_name not in _sfx_cache
AudioManager-->>AudioManager: full_path = SFX_DIR_PATH + sfx_name + .wav
AudioManager->>ResourceLoader: exists(full_path)
alt not exists(full_path)
AudioManager-->>AudioManager: _missing_sfx_cache[sfx_name] = true
AudioManager-->>Caller: return
else exists(full_path)
AudioManager-->>AudioManager: stream = load(full_path)
alt stream
AudioManager-->>AudioManager: _sfx_cache[sfx_name] = stream
AudioManager-->>Caller: play via pooled player
else no stream
AudioManager-->>AudioManager: _missing_sfx_cache[sfx_name] = true
AudioManager-->>Caller: return
end
end
else sfx_name in _sfx_cache
AudioManager-->>Caller: play cached stream via pooled player
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefines audio settings UI with auto-mute/unmute coupled to volume, focus-gated check SFX playback, deferred mute cutoff timing, finer slider granularity, registers check.wav import, updates CI workflow caching/pins, and adds milestone documentation describing new GUT tests. ChangesAudio Settings and Volume Control Refinements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the milestone doc’s file-level changes table, the path
files/docs/milestones/19/PART_1_test_plan_implementation_for_ui_bus_persistence .mdcontains an extra space before.mdthat likely doesn’t match the actual filename and could cause confusion when navigating to the file.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the milestone doc’s file-level changes table, the path `files/docs/milestones/19/PART_1_test_plan_implementation_for_ui_bus_persistence .md` contains an extra space before `.md` that likely doesn’t match the actual filename and could cause confusion when navigating to the file.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 |  |
May 29, 2026 12:37a.m. | Review ↗ |
| JavaScript | |
May 29, 2026 12:37a.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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@files/docs/milestones/19/PART_1_test_plan_implementation_for_ui_bus_persistence.md`:
- Around line 104-107: Replace the placeholder issue references "`#N/A`" and
"`#unknown`" in the milestones text for the tests by either inserting the correct
GitHub issue numbers or removing those list entries; specifically update the
lines referencing the test_ui_menu_mute_persistence and the UI/Menu volume
persistence test so they no longer contain placeholder issue tokens, ensuring
the entries point to a real issue number (e.g., `#1234`) or are deleted if there
is no tracked issue.
- Around line 111-141: The "Bots/AI Contributions Summary" block contains an
incorrect PR reference ("PR `#713`") that should match the current PR (`#719`);
update the header and any occurrences of "`#713`" within the "Bots/AI
Contributions Summary for PR `#713`" section to "`#719`" or remove/relocate that
entire "Bots/AI Contributions Summary" section if it pertains to another PR;
locate the section by the header text "Bots/AI Contributions Summary for PR
`#713`" and the documented contributors (`@sourcery-ai`, `@coderabbitai`,
`@deepsource-io`, `@ikostan`) and make the change consistently.
- Around line 67-142: The milestone doc currently documents tests from PR `#713`
but omits the actual changes in PR `#719` — specifically the added audio asset
check.wav and its integration with the mute button; update
files/docs/milestones/19/PART_1_test_plan_implementation_for_ui_bus_persistence.md
to either (A) add a short section describing the PR `#719` changes: the new asset
check.wav, where it lives, when it is played (on mute toggle), which UI control
or method triggers it (e.g., the mute button's toggle handler /
play_feedback_sound()), and any mute-state/AudioServer interaction details, or
(B) if this doc does not belong to PR `#719`, remove the file from the PR so the
commit only contains relevant test plan documentation.
- Around line 79-80: The markdown table contains inline HTML (<ul>, <li>, <br>)
that violates MD033 and a filename typo with an extra space before ".md"; edit
the table cells in PART_1_test_plan_implementation_for_ui_bus_persistence.md to
replace the HTML lists with plain markdown-friendly text or plain line breaks
(e.g., "• Set up and tear down AudioManager state; • Implement integration-style
test..." or separate lines), remove all <ul>/<li>/<br> tags from the two
affected cells, and correct the filename entry
`PART_1_test_plan_implementation_for_ui_bus_persistence .md` by removing the
stray space so it reads
`PART_1_test_plan_implementation_for_ui_bus_persistence.md`.
🪄 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: 24eabfab-ebca-4659-976f-424b6c1b03b9
⛔ Files ignored due to path filters (1)
files/sounds/sfx/check.wavis excluded by!**/*.wav
📒 Files selected for processing (2)
files/docs/milestones/19/PART_1_test_plan_implementation_for_ui_bus_persistence.mdfiles/sounds/sfx/check.wav.import
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: Pull Request Pipeline / 4_Markdown Lint _ lint (3.x).txt
files/docs/milestones/19/PART_1_test_plan_implementation_for_ui_bus_persistence.md
[error] 79-79: markdownlint-cli2 (MD033/no-inline-html): Inline HTML [Element: ul]. https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md033.md
🪛 GitHub Actions: Pull Request Pipeline / Markdown Lint _ lint (3.x)
files/docs/milestones/19/PART_1_test_plan_implementation_for_ui_bus_persistence.md
[error] 79-79: MD033/no-inline-html Inline HTML [Element: ul] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md033.md
🪛 GitHub Check: Markdown Lint / lint (3.x)
files/docs/milestones/19/PART_1_test_plan_implementation_for_ui_bus_persistence.md
[failure] 80-80: Inline HTML
files/docs/milestones/19/PART_1_test_plan_implementation_for_ui_bus_persistence.md:80:723 MD033/no-inline-html Inline HTML [Element: br] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md033.md
[failure] 80-80: Inline HTML
files/docs/milestones/19/PART_1_test_plan_implementation_for_ui_bus_persistence.md:80:514 MD033/no-inline-html Inline HTML [Element: li] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md033.md
[failure] 80-80: Inline HTML
files/docs/milestones/19/PART_1_test_plan_implementation_for_ui_bus_persistence.md:80:315 MD033/no-inline-html Inline HTML [Element: li] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md033.md
[failure] 80-80: Inline HTML
files/docs/milestones/19/PART_1_test_plan_implementation_for_ui_bus_persistence.md:80:171 MD033/no-inline-html Inline HTML [Element: li] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md033.md
[failure] 80-80: Inline HTML
files/docs/milestones/19/PART_1_test_plan_implementation_for_ui_bus_persistence.md:80:167 MD033/no-inline-html Inline HTML [Element: ul] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md033.md
[failure] 79-79: Inline HTML
files/docs/milestones/19/PART_1_test_plan_implementation_for_ui_bus_persistence.md:79:730 MD033/no-inline-html Inline HTML [Element: br] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md033.md
[failure] 79-79: Inline HTML
files/docs/milestones/19/PART_1_test_plan_implementation_for_ui_bus_persistence.md:79:526 MD033/no-inline-html Inline HTML [Element: li] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md033.md
[failure] 79-79: Inline HTML
files/docs/milestones/19/PART_1_test_plan_implementation_for_ui_bus_persistence.md:79:329 MD033/no-inline-html Inline HTML [Element: li] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md033.md
[failure] 79-79: Inline HTML
files/docs/milestones/19/PART_1_test_plan_implementation_for_ui_bus_persistence.md:79:171 MD033/no-inline-html Inline HTML [Element: li] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md033.md
[failure] 79-79: Inline HTML
files/docs/milestones/19/PART_1_test_plan_implementation_for_ui_bus_persistence.md:79:167 MD033/no-inline-html Inline HTML [Element: ul] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md033.md
🔇 Additional comments (1)
files/sounds/sfx/check.wav.import (1)
1-25: LGTM!
…v/codecov-action-6.0.1 Bump codecov/codecov-action from 6.0.0 to 6.0.1
…s/stale-10.3.0 Bump actions/stale from 10.2.0 to 10.3.0
…e-drafter/release-drafter-7.3.1 Bump release-drafter/release-drafter from 7.3.0 to 7.3.1
Owner
Author
All three points raised by Sourcery AI make perfect technical sense for a Godot 4 project running in an automated CI pipeline. These are not noisy static-analysis bugs; they are genuine structural recommendations designed to stop our test suite from becoming brittle. Here is an evaluation of how important each point is and why they matter for your codebase: 1.
|
To prevent false negatives or platform warnings inside strict headless CI environments, we should completely remove DirAccess.dir_exists_absolute() from your integrity test scripts. Calling DirAccess.open() directly acts as both the existence check and the initialization loop. If it returns null, the folder safely skips without a crash.
…set_button.gd Right now, test_audio_reset_button.gd uses exact float comparisons against snapped quantization steps (like 0.495, 0.792, and 0.99). If the layout slider's step value changes in the editor, your tests instantly break. To make the test suite completely resilient to layout or step changes, we change the exact equality assertions (assert_eq) to epsilon range assertions (assert_almost_eq).
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new auto-mute logic in
_on_global_volume_changedhardcodes behavior in the UI script (including master-specific AudioServer tweaks and token tracking); consider moving the threshold constants and master-specific handling intoAudioManagerso that bus state changes stay centralized and easier to reuse from non-UI callers. - Several new tests depend on internal
AudioManagerfields like_sfx_cache,_missing_sfx_cache, and_sfx_pool, which will make refactors of the SFX backend painful; it would be more robust to expose minimal public helpers (e.g. query methods or a test-only facade) instead of reaching into private state from tests. - A lot of tests now rely on hardcoded timing (e.g.
0.2seconds) and exact slider step-aligned values (0.495,0.693, etc.); it would be safer to derive these fromMUTE_HARDWARE_DELAYand the slider’sstepproperty at runtime so that UI tuning or timing changes don’t cascade into brittle test breakages.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new auto-mute logic in `_on_global_volume_changed` hardcodes behavior in the UI script (including master-specific AudioServer tweaks and token tracking); consider moving the threshold constants and master-specific handling into `AudioManager` so that bus state changes stay centralized and easier to reuse from non-UI callers.
- Several new tests depend on internal `AudioManager` fields like `_sfx_cache`, `_missing_sfx_cache`, and `_sfx_pool`, which will make refactors of the SFX backend painful; it would be more robust to expose minimal public helpers (e.g. query methods or a test-only facade) instead of reaching into private state from tests.
- A lot of tests now rely on hardcoded timing (e.g. `0.2` seconds) and exact slider step-aligned values (`0.495`, `0.693`, etc.); it would be safer to derive these from `MUTE_HARDWARE_DELAY` and the slider’s `step` property at runtime so that UI tuning or timing changes don’t cascade into brittle test breakages.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
Thanks for the detailed structural feedback. To ensure we maintain a stable milestone delivery path and avoid unnecessary scope creep on this specific PR, we have addressed the critiques as follows:
Merging this branch as-is to solidify the current green test baseline. |
Owner
Author
|
@sourcery-ai title |
sourcery-ai
Bot
changed the title
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new integrity test
test_sprite_integrity.gdtargetsres://files/ground_tilesest/, which looks like it might be a typo forground_tileset; if the actual directory name differs, the test will silently skip that subtree and provide a false sense of coverage. - Several new tests reach into
AudioManagerinternals (_sfx_pool,_sfx_cache,_missing_sfx_cache), which tightly couples tests to implementation details; consider adding small public helper/inspection APIs (or using signals) so tests can validate behavior without depending on private fields. - The auto-mute/auto-unmute logic embedded in
_on_global_volume_changedmixes UI concerns with backend audio state transitions; factoring the threshold logic intoAudioManager(with the UI just reacting to state) would simplify the menu script and centralize behavior for non-UI callers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new integrity test `test_sprite_integrity.gd` targets `res://files/ground_tilesest/`, which looks like it might be a typo for `ground_tileset`; if the actual directory name differs, the test will silently skip that subtree and provide a false sense of coverage.
- Several new tests reach into `AudioManager` internals (`_sfx_pool`, `_sfx_cache`, `_missing_sfx_cache`), which tightly couples tests to implementation details; consider adding small public helper/inspection APIs (or using signals) so tests can validate behavior without depending on private fields.
- The auto-mute/auto-unmute logic embedded in `_on_global_volume_changed` mixes UI concerns with backend audio state transitions; factoring the threshold logic into `AudioManager` (with the UI just reacting to state) would simplify the menu script and centralize behavior for non-UI callers.
## Individual Comments
### Comment 1
<location path="scripts/ui/menus/audio_settings.gd" line_range="284-293" />
<code_context>
+ if volume <= AUTO_MUTE_VOLUME_THRESHOLD and not AudioManager.get_muted(bus_name):
</code_context>
<issue_to_address>
**issue (bug_risk):** Slider-driven auto-mute may result in duplicate "check" SFX when mute toggles are also triggered.
Dragging the slider to or from zero with `has_focus()` causes `_on_global_volume_changed` to play "check", and the same state change then flows into `_on_global_mute_toggled` → `_execute_bus_mute_toggle`, which may play "check" again if focused. Consider passing a flag (or using a short-lived guard) along the auto-mute path so `_execute_bus_mute_toggle` skips SFX when feedback was already played for this transition.
</issue_to_address>
### Comment 2
<location path="scripts/ui/menus/audio_settings.gd" line_range="660-669" />
<code_context>
+## :param bus_name: The constant identifier name of the audio channel.
+## :type bus_name: String
+## :rtype: HSlider
+func _get_slider_for_bus(bus_name: String) -> HSlider:
+ var target_slider: HSlider = null
+
+ match bus_name:
+ AudioConstants.BUS_MASTER:
+ target_slider = master_slider
+ AudioConstants.BUS_MUSIC:
+ target_slider = music_slider
+ AudioConstants.BUS_SFX:
+ target_slider = sfx_slider
+ AudioConstants.BUS_SFX_WEAPON:
+ target_slider = weapon_slider
+ AudioConstants.BUS_SFX_ROTORS:
+ target_slider = rotor_slider
+ AudioConstants.BUS_SFX_MENU:
+ target_slider = menu_slider
+
+ return target_slider
+
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Unknown `bus_name` cases fall through silently to `null`, which can hide configuration mistakes.
Because callers silently receive `null`, focus/audio updates are skipped instead of clearly failing, which can obscure new or misconfigured buses during development. Consider adding an `assert(false, ...)` in the default branch (for debug builds) or at least logging a warning when `bus_name` is unknown, so missing mappings are caught early.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…o null, which can hide configuration mistakes. Because callers silently receive null, focus/audio updates are skipped instead of clearly failing, which can obscure new or misconfigured buses during development. Consider adding an assert(false, ...) in the default branch (for debug builds) or at least logging a warning when bus_name is unknown, so missing mappings are caught early.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
_get_slider_for_busand_get_mute_button_for_bushelpers assert on unknown bus names, which will hard-crash in production if a new bus is added but not wired; consider replacing theassert(false, ...)with a softer runtime check (e.g., logging and early-return) to avoid bringing down the UI in misconfigurations. - Several of the new tests (e.g., SFX centralization and auto-mute suites) reach into
AudioManagerinternals like_sfx_pool,_sfx_cache, and_missing_sfx_cache; you might want to introduce minimal public inspection/reset helpers for these concerns so tests don't depend on private fields and future refactors remain easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_get_slider_for_bus` and `_get_mute_button_for_bus` helpers assert on unknown bus names, which will hard-crash in production if a new bus is added but not wired; consider replacing the `assert(false, ...)` with a softer runtime check (e.g., logging and early-return) to avoid bringing down the UI in misconfigurations.
- Several of the new tests (e.g., SFX centralization and auto-mute suites) reach into `AudioManager` internals like `_sfx_pool`, `_sfx_cache`, and `_missing_sfx_cache`; you might want to introduce minimal public inspection/reset helpers for these concerns so tests don't depend on private fields and future refactors remain easier.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
|
@sourcery-ai review |
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
test_sprite_integrity.gd,_scan_directory_recursiveusesDirAccess.dir_exists_absolute(dir_path)withres://-style paths, which is meant for absolute OS paths; consider dropping that check and relying onDirAccess.open(as you already do in the audio integrity tests) to avoid silently skipping valid resource directories in CI. - The
_get_slider_for_busand_get_mute_button_for_bushelpers hard-assert(false)on unknown bus names, which will crash the game in production if a new bus is added but not wired; it may be safer to log a warning and returnnull, letting callers handle the missing mapping more gracefully while still surfacing the configuration error. - Several tests now depend on the 0.15s
MUTE_HARDWARE_DELAYbut hardcode0.2second waits; to reduce future flakiness if the delay changes, consider centralizing these waits through a helper that derives fromMUTE_HARDWARE_DELAY(similar to_get_safety_wait_time()intest_audio_settings_comprehensive.gd) and reuse it across the affected suites.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_sprite_integrity.gd`, `_scan_directory_recursive` uses `DirAccess.dir_exists_absolute(dir_path)` with `res://`-style paths, which is meant for absolute OS paths; consider dropping that check and relying on `DirAccess.open` (as you already do in the audio integrity tests) to avoid silently skipping valid resource directories in CI.
- The `_get_slider_for_bus` and `_get_mute_button_for_bus` helpers hard-`assert(false)` on unknown bus names, which will crash the game in production if a new bus is added but not wired; it may be safer to log a warning and return `null`, letting callers handle the missing mapping more gracefully while still surfacing the configuration error.
- Several tests now depend on the 0.15s `MUTE_HARDWARE_DELAY` but hardcode `0.2` second waits; to reduce future flakiness if the delay changes, consider centralizing these waits through a helper that derives from `MUTE_HARDWARE_DELAY` (similar to `_get_safety_wait_time()` in `test_audio_settings_comprehensive.gd`) and reuse it across the affected suites.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
Sourcery-AI is bringing up two completely valid points here that we should fix before pushing this branch across the final verification gate. The first point is a legitimate production stability risk. Since Godot automatically strips assert() calls out of exported release builds, our current fallback path would drop down to returning a null reference silently. The moment the calling function tries to use that null value, the UI will hard crash for the player. The second point is an encapsulation cleanup to keep our unit tests from breaking if we ever adjust how the sound effect pool behaves internally. |
The _get_slider_for_bus and _get_mute_button_for_bus helpers assert on unknown bus names, which will hard-crash in production if a new bus is added but not wired; consider replacing the assert(false, ...) with a softer runtime check (e.g., logging and early-return) to avoid bringing down the UI in misconfigurations. Several of the new tests (e.g., SFX centralization and auto-mute suites) reach into AudioManager internals like _sfx_pool, _sfx_cache, and _missing_sfx_cache; you might want to introduce minimal public inspection/reset helpers for these concerns so tests don't depend on private fields and future refactors remain easier. Sourcery-AI is bringing up two completely valid points here that we should fix before pushing this branch across the final verification gate. The first point is a legitimate production stability risk. Since Godot automatically strips assert() calls out of exported release builds, our current fallback path would drop down to returning a null reference silently. The moment the calling function tries to use that null value, the UI will hard crash for the player. The second point is an encapsulation cleanup to keep our unit tests from breaking if we ever adjust how the sound effect pool behaves internally.
Owner
Author
Acknowledged the linter feedback. Bypassing these suggestions for the current milestone:
|
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
7 tasks
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
name: Default Pull Request Template
about: Suggesting changes to SkyLockAssault
title: ''
labels: ''
assignees: ''
Description
What does this PR do? (e.g., "Fixes player jump physics in level 2" or "Adds
new enemy AI script")
Related Issue
Closes #ISSUE_NUMBER (if applicable)
Changes
system")
Testing
works on Win10 with 60 FPS")
Checklist
Additional Notes
Anything else? (e.g., "Tested on Win10 64-bit; needs Linux validation")
Summary by Sourcery
Add audio feedback and robust auto-mute/unmute behavior for UI mute controls, harden audio/sprite asset handling, and expand automated test coverage for audio settings and resources while tightening CI workflows.
New Features:
Enhancements:
CI:
Documentation:
Tests:
Chores:
Summary by Sourcery
Add focus-aware auto-mute/unmute coupling with click SFX across all audio buses, centralize UI mute handling and SFX playback safeguards, expand audio/UI and asset integrity tests, and tighten related CI workflows and documentation.
New Features:
Enhancements:
CI:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
New Features
Documentation
Chores
Bots/AI Contributions Summary for PR #719
This PR received substantial support from automated bots and AI-powered tools for dependency management, code review, static analysis, and PR summarization. These contributions improved security, CI reliability, documentation quality, and overall code health.
Automated Bots & AI Tools
@dependabot[bot]: Managed dependency updates for GitHub Actions workflows, including version bumps for
codecov/codecov-action,actions/stale, andrelease-drafter/release-drafter. This helped maintain supply-chain security and latest CI/CD features.@sourcery-ai: Provided a detailed PR summary covering new features (audio feedback, auto-mute/unmute), enhancements (centralized mute handling, SFX improvements), CI changes, expanded test coverage, and documentation updates.
@coderabbitai: Delivered a focused summary highlighting finer slider controls, auto-mute logic with audio feedback, documentation updates, and maintenance chores (CI pinning and asset configuration).
@deepsource-io: Performed automated static code analysis and code review. Evaluated changes across Security, Reliability, Complexity, and Hygiene categories, providing an overall grade, inline comments, and a comprehensive review report.
These automated contributions strengthened testing robustness, asset integrity checks, workflow pinning, and reviewer documentation.
Human Maintainers
@ikostan: Primary author and main contributor. Implemented core features including audio feedback for mute buttons, centralized mute handling with focus-aware SFX, auto-mute/unmute logic, AudioManager hardening (SFX pooling + LRU cache), extensive test suite updates (GUT/GDUnit4), CI refinements, and detailed milestone documentation with reviewer guides and sequence diagrams.
@espanakosta-jpg: Contributed custom audio assets, specifically creating/updating
check.wavand its.importfile for improved UI click feedback.