Implement a dedicated slider.wav sound effect that triggers only during manual slider adjustments

[ikostan]

---

name: Default Pull Request Template
about: Suggesting changes to SkyLockAssault
title: ''
labels: ''
assignees: ''

PR Summary: Audio Pipeline Performance, UI Safety, and Test Suite Hardening

Bug Fixes & UX Safety

• Invalid Bus Guard: Added defensive initialization to VolumeSlider. If an invalid or typoed bus_name is detected (bus_index == -1), the component safely aborts, disables the UI (editable = false), and drops mouse/keyboard focus (MOUSE_FILTER_IGNORE, FOCUS_NONE) to prevent silent runtime crashes.
• Stuck Drag State Resolution: Implemented a _notification listener in VolumeSlider to catch NOTIFICATION_FOCUS_EXIT and NOTIFICATION_WM_WINDOW_FOCUS_OUT. This guarantees the _is_dragging flag safely resets even if the system loses focus (e.g., Alt-Tabbing) before a mouse release event fires.
• Accurate Delta Tracking: Reordered the _previous_value assignment inside _handle_slider_sfx to execute after interaction guards. This ensures rogue programmatic updates don't artificially advance the delta tracker and swallow genuine manual user interactions.

Performance & Architecture Optimizations

• Audio Object Pooling: Refactored AudioManager to instantiate a reusable pool of AudioStreamPlayer nodes (default size 8) on _ready. This completely eliminates the severe CPU and memory fragmentation churn caused by constantly calling .new() and queue_free() on rapid slider drags.
• Missing File Caching: Implemented _missing_sfx_cache in AudioManager. If an audio file fails to load once, the system flags it and short-circuits future requests, preventing massive disk I/O spikes and log spam.
• Sub-Epsilon Jitter Gating: Added early-return is_equal_approx checks at the very top of both _on_value_changed and set_value_programmatically. This prevents redundant AudioServer backend calculations and stops the debounced disk-save timer from spinning up due to micro-jitters from controllers or float rounding.
• Component Encapsulation: Added public getters (get_previous_value(), get_last_sfx_time(), is_user_dragging()) to VolumeSlider to allow the test suite to validate logic without directly accessing private, underscored variables.

Test Suite Hardening (GUT & GdUnit4)

• Cross-Suite Leakage Prevention: Implemented a strict state snapshot-and-restore pattern for the AudioManager singleton across before_each and after_each lifecycle methods. This isolates test environments and prevents stray user:// disk writes during CI runs.
• Deterministic Rate-Limit Testing: Patched flaky CI test runs by forcing _last_sfx_time artificially into the future, guaranteeing the cooldown guard triggers mathematically regardless of thread pauses or garbage collection spikes.
• I/O Mocking: Replaced live audio playback in tests with a custom, duck-typed inline MockAudioManager class. This allows the suite to verify interaction logic and signal paths instantly without touching the disk or generating audio noise.
• GdUnit4 Registry Bypass: Explicitly preloaded script resources for test initialization to bypass known class_name registry parsing bugs in the GdUnit4 runner.

---

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

• List key changes here (e.g., "Updated Jump.gd to use Godot 4.4's new Tween
  system")
• Any breaking changes? (e.g., "Deprecated old signal; migrate to new one")

Testing

• Ran the game in Godot v4.5 editor—describe what you tested (e.g., "Jump
  works on Win10 with 60 FPS")
• Any new unit tests added? (Link to test scene if yes)
• Screenshots/GIFs if UI-related: (Attach below)

Checklist

• Code follows Godot style guide (e.g., snake_case for variables)
• No console errors in editor/output
• Ready for review!

Additional Notes

Anything else? (e.g., "Tested on Win10 64-bit; needs Linux validation")

Summary by Sourcery

Add a dedicated, rate-limited slider sound effect that plays only for genuine user-driven volume adjustments, while hardening the audio/UI sync pipeline against feedback loops and noisy saves.

New Features:

• Introduce a dedicated slider sound effect wired through a centralized AudioManager.play_sfx API with cached SFX loading.
• Add a VolumeSlider API for programmatic value updates that explicitly suppresses SFX playback and debounced saves.
• Define audio constant IDs for common UI SFX such as slider, mute toggle, and navigation.

Bug Fixes:

• Prevent audio spam by rate-limiting slider SFX and ignoring non-interactive or redundant value changes.
• Avoid double UI sounds when using keyboard left/right to adjust focused sliders instead of navigating.
• Ensure global and manager-driven volume sync updates do not trigger slider value_changed signals, preventing audio feedback loops and unnecessary disk writes.

Enhancements:

• Document the refactored scripts/ directory layout and current milestones in the README for easier project navigation.

Tests:

• Add GUT test coverage for VolumeSlider initialization, programmatic guards, and SFX interaction/rate-limiting behavior.
• Add GUT tests validating that audio UI sync from global events and AudioManager updates bypasses slider signals and debounce timers.

Summary by Sourcery

Add a dedicated, rate-limited slider sound effect and central SFX playback API while hardening audio/UI synchronization to avoid feedback loops and noisy saves.

New Features:

• Introduce a dedicated slider UI sound routed through a centralized AudioManager.play_sfx API with caching and pooling.
• Add a programmatic VolumeSlider update API that suppresses sound effects and debounced saves for non-interactive changes.
• Define audio constant IDs for common UI sounds like slider, mute toggle, and navigation to standardize SFX usage.

Bug Fixes:

• Prevent double UI sounds when adjusting focused sliders with keyboard navigation inputs.
• Avoid audio spam by rate-limiting slider SFX, ignoring redundant value changes, and blocking interactions on invalid audio buses.
• Decouple programmatic volume updates from slider signals and DOM sync to prevent audio feedback loops and unnecessary disk writes.

Enhancements:

• Refine VolumeSlider behavior to track interaction state, guard invalid buses, and debounce settings persistence more safely.
• Add SFX caching and an AudioStreamPlayer pool in AudioManager to reduce disk I/O and node churn for UI sounds.
• Document the refactored scripts directory layout and current milestones in the README for easier project navigation.

Tests:

• Add GUT test coverage for VolumeSlider initialization, programmatic guards, SFX interaction, and rate-limiting behavior.
• Add GUT tests ensuring audio UI sync from global events and AudioManager updates bypasses slider signals and debounce timers.
• Add GUT tests validating one-way DOM sync in AudioWebBridge to prevent HTML-driven feedback into the engine.

Summary by CodeRabbit

• New Features

  • Added slider audio sample and audible feedback for slider interactions.

• Improvements

  • Centralized SFX playback with caching and validation; added SFX asset IDs.
  • Slider refined: programmatic updates avoid side effects; user changes are debounced, rate-limited, and suppress navigation sounds when adjusting.

• Documentation

  • README expanded with tooling, scripts architecture, and milestone entries.

• Tests

  • New unit tests covering audio/UI sync, web bridge DOM sync, and slider UX.

---

Here is a summary of @ikostan’s contributions as the author and primary developer of PR #578:

Core Feature Implementation

• Targeted Audio Feedback: Implemented a dedicated slider.wav sound effect that triggers exclusively during genuine, manual user interactions (e.g., active mouse dragging or keyboard focus), completely silencing redundant audio during programmatic UI syncs.

Architecture & Performance Optimization (AudioManager)

• Centralized SFX API: Built a robust play_sfx method to handle all non-positional UI audio routing safely.
• Asset Caching: Implemented an LRU (Least Recently Used) cache for audio streams to eliminate disk I/O stutter during gameplay, alongside a "missing SFX" cache to prevent the engine from repeatedly attempting to load broken file paths.
• Object Pooling: Refactored audio playback to utilize a pre-instantiated pool of AudioStreamPlayer nodes, drastically reducing node instantiation churn, CPU overhead, and memory fragmentation.

UI Component Hardening (VolumeSlider)

• Decoupled Syncing: Created the set_value_programmatically API to safely update slider visuals and backend audio servers without emitting the value_changed signal, breaking potential audio feedback loops.
• Audio Spam Prevention: Engineered a strict 3-tier guard system to block audio spam:
  1. Verifying a float-safe delta change.
  2. Validating active user interaction (preventing rogue scripts from triggering sounds).
  3. Enforcing a strict 60ms rate limit to protect the user's ears during rapid slider movement.
• Fail-Safes: Added defensive initialization checks to instantly disable the UI component, push editor warnings, and drop focus if an invalid audio bus name is detected, preventing silent runtime crashes.

Test-Driven Reliability (GUT Framework)

• Comprehensive Coverage: Wrote extensive unit tests verifying initialization, programmatic update guards, and the interaction/rate-limiting logic of the VolumeSlider.
• Deterministic CI Strategies: Ensured time-based tests (like the 60ms rate limiter) are completely deterministic by pushing evaluation timestamps into the future, preventing flaky failures on slow CI runners.
• State Isolation: Implemented strict snapshot-and-restore patterns for the AudioManager singleton during before_each and after_each test phases to guarantee zero cross-suite state leakage.
• I/O Mocking: Designed custom inline mock classes (MockAudioManager) to intercept and assert audio calls during testing, verifying complex state logic without triggering actual sound playback or disk reads.

---

Summary of the automated and AI-driven contributions to PR #578:

@sourcery-ai Generated the primary PR summary, detailing the new features (dedicated slider sound effect, AudioManager.play_sfx API), bug fixes (audio spam rate-limiting, fixing feedback loops), and added GUT test coverage.

• Provided architectural feedback, reminding the team to verify if .import files should be source-controlled and to standardize the SFX folder structure (files/sounds/sfx/).

• Identified a copy-paste error in the test_volume_slider.gd suite and suggested adding an early-return short-circuit in VolumeSlider.set_value_programmatically to prevent redundant backend calls.

@coderabbitai

• Conducted a detailed code review, specifically identifying a latent state-machine bug in scripts/ui/components/volume_slider.gd. It noted that committing _previous_value before the interaction and cooldown guards could allow non-interactive programmatic changes to mask real user interactions, and provided a refactoring suggestion to fix it.

@deepsource-io

• Participated in the automated review pipeline to scan for code quality, potential anti-patterns, and static analysis issues across the new Godot GDScript additions.

---

[sourcery-ai]

Reviewer's Guide

Implements a dedicated, rate-limited slider SFX and central SFX playback API, refines VolumeSlider to distinguish manual vs programmatic updates and handle invalid buses, suppresses duplicate navigation sounds when keyboard-adjusting sliders, documents the scripts/ layout and milestones, and adds GUT tests to ensure audio/UI/web sync without feedback loops.

Sequence diagram for manual vs programmatic volume slider updates with SFX and debounced save

sequenceDiagram
    actor User
    participant VolumeSlider
    participant AudioServer
    participant AudioManager
    participant Timer_save_debounce

    rect rgb(235, 250, 255)
    Note over User,AudioManager: Manual slider drag or keyboard left/right on focused slider
    User->>VolumeSlider: Drag slider / press ui_left or ui_right
    VolumeSlider->>VolumeSlider: _on_gui_input(event) sets _is_dragging = true
    VolumeSlider->>VolumeSlider: _on_value_changed(new_value)
    VolumeSlider->>VolumeSlider: is_equal_approx(new_value, _previous_value)?
    alt Value changed
        VolumeSlider->>VolumeSlider: _previous_value = new_value
        VolumeSlider->>AudioServer: set_bus_volume_db(bus_index, linear_to_db(new_value))
        VolumeSlider->>AudioManager: set_volume(bus_name, new_value)
        VolumeSlider->>VolumeSlider: _handle_slider_sfx(new_value)
        VolumeSlider->>Timer_save_debounce: start()
        Note right of Timer_save_debounce: Debounces disk saves
        Timer_save_debounce-->>VolumeSlider: timeout
        VolumeSlider->>AudioManager: _on_debounce_timeout() calls save_volumes()
    else Value jitter (no real change)
        VolumeSlider-->>VolumeSlider: Return early, no SFX, no save restart
    end
    VolumeSlider->>VolumeSlider: _notification(NOTIFICATION_FOCUS_EXIT) clears _is_dragging
    end

    rect rgb(245, 235, 255)
    Note over AudioManager,VolumeSlider: Programmatic volume sync (no SFX, no debounce)
    AudioManager->>VolumeSlider: set_value_programmatically(new_value)
    VolumeSlider->>VolumeSlider: Guard bus_index != -1
    VolumeSlider->>VolumeSlider: clamped_value = clamp(new_value, min_value, max_value)
    VolumeSlider->>VolumeSlider: set_value_no_signal(clamped_value)
    VolumeSlider->>AudioServer: set_bus_volume_db(bus_index, linear_to_db(clamped_value))
    VolumeSlider->>AudioManager: set_volume(bus_name, clamped_value)
    VolumeSlider->>VolumeSlider: _previous_value = clamped_value (no SFX, no timer)
    end

Loading

Sequence diagram for centralized AudioManager.play_sfx with caching and pooling

sequenceDiagram
    participant Caller as Any_UI_or_System
    participant AudioManager
    participant SFXCache as _sfx_cache
    participant MissingCache as _missing_sfx_cache
    participant Pool as _sfx_pool
    participant AudioServer

    Caller->>AudioManager: play_sfx(sfx_name, bus_name, pitch_scale, volume_db)
    alt Empty sfx_name
        AudioManager-->>Caller: Return
    else Nonempty sfx_name
        alt sfx_name in _missing_sfx_cache
            AudioManager-->>Caller: Return (known missing)
        else sfx_name not in _missing_sfx_cache
            alt sfx_name not in _sfx_cache
                AudioManager->>AudioManager: full_path = SFX_DIR_PATH + sfx_name + .wav
                AudioManager->>AudioManager: stream = load(full_path)
                alt stream loaded
                    AudioManager->>SFXCache: Check size against MAX_SFX_CACHE_SIZE
                    alt Cache full
                        SFXCache->>SFXCache: Evict oldest_key
                        AudioManager->>Caller: Log eviction
                    else Cache not full
                    end
                    SFXCache->>SFXCache: Store stream under sfx_name
                else load failed
                    AudioManager->>Caller: Log warning (missing SFX)
                    MissingCache->>MissingCache: Mark sfx_name as missing
                    AudioManager-->>Caller: Return
                end
            else sfx_name in _sfx_cache
                SFXCache->>AudioManager: stream = _sfx_cache[sfx_name]
                SFXCache->>SFXCache: Erase sfx_name
                SFXCache->>SFXCache: Reinsert stream as newest (LRU update)
            end

            AudioManager->>Pool: Find first AudioStreamPlayer where playing == false
            alt Free player found
                Pool-->>AudioManager: player
            else All players busy
                Pool-->>AudioManager: Fallback to _sfx_pool[0]
            end

            AudioManager->>AudioManager: Configure player.stream, pitch_scale, volume_db
            AudioManager->>AudioServer: get_bus_index(bus_name)
            alt Invalid bus
                AudioManager->>Caller: Log warning, fallback to BUS_SFX_MENU
                AudioManager->>Pool: player.bus = AudioConstants.BUS_SFX_MENU
            else Valid bus
                AudioManager->>Pool: player.bus = bus_name
            end

            AudioManager->>Pool: player.play()
        end
    end

Loading

Updated class diagram for VolumeSlider, AudioManager, AudioConstants, and Globals input gating

classDiagram
    class VolumeSlider {
        <<HSlider>>
        +int SFX_COOLDOWN_MS
        +String bus_name
        +int bus_index
        +Timer save_debounce_timer
        -int _last_sfx_time
        -float _previous_value
        -bool _is_dragging
        +_ready() void
        +set_value_programmatically(new_value float) void
        +_on_gui_input(event InputEvent) void
        +_notification(what int) void
        +_on_value_changed(new_value float) void
        +_handle_slider_sfx(_new_value float) void
        +_on_debounce_timeout() void
        +get_previous_value() float
        +get_last_sfx_time() int
        +is_user_dragging() bool
    }

    class AudioManager {
        +String SFX_DIR_PATH
        +int MAX_SFX_CACHE_SIZE
        +int SFX_POOL_SIZE
        +float master_volume
        +bool master_muted
        +String current_config_path
        -Dictionary _sfx_cache
        -Dictionary _missing_sfx_cache
        -Array~AudioStreamPlayer~ _sfx_pool
        +_ready() void
        +_init_to_defaults() void
        +load_volumes() void
        +apply_all_volumes() void
        +set_volume(bus_name String, volume float) void
        +save_volumes() void
        +reset_volumes() void
        +play_sfx(sfx_name String, bus_name String, pitch_scale float, volume_db float) void
        +signal volume_changed(bus_name String, volume float)
        +signal mute_toggled(bus_name String, is_muted bool)
    }

    class AudioConstants {
        +String BUS_MASTER
        +String BUS_MUSIC
        +String BUS_SFX
        +String BUS_SFX_ROTORS
        +String BUS_SFX_WEAPON
        +String BUS_SFX_MENU
        +String SFX_SLIDER
        +String SFX_MUTE_TOGGLE
        +String SFX_UI_NAVIGATION
        +Dictionary BUS_CONFIG
    }

    class Globals {
        +_input(_event InputEvent) void
        +_play_ui_navigation_sfx() void
    }

    VolumeSlider --> AudioManager : uses set_volume
    VolumeSlider --> AudioManager : uses play_sfx
    VolumeSlider --> AudioManager : uses save_volumes
    VolumeSlider --> AudioConstants : uses SFX_SLIDER
    VolumeSlider --> AudioServer : uses buses

    AudioManager --> AudioServer : applies bus volumes
    AudioManager --> AudioConstants : uses BUS_SFX_MENU

    Globals --> AudioManager : calls _play_ui_navigation_sfx
    Globals --> Slider : suppresses nav SFX when Slider focused

Loading

File-Level Changes

Change	Details	Files
Refine VolumeSlider to gate SFX to genuine user interactions, debounce saves, support safe programmatic updates, and handle invalid audio buses.	Add SFX cooldown tracking, previous-value tracking, and drag-state tracking to the slider. Connect gui_input and notification handlers to track mouse/touch drag and reset state on focus loss. Introduce set_value_programmatically to update value and backend without emitting signals or triggering SFX/saves. Ensure _on_value_changed ignores redundant float jitter, updates AudioServer/AudioManager once, plays gated SFX, and restarts a debounce timer. Guard initialization against invalid bus names by disabling the slider and skipping signal connections. Expose getter helpers for tests to inspect previous value, last SFX time, and drag state.	scripts/ui/components/volume_slider.gd
Introduce centralized, cached SFX playback in AudioManager for UI sounds.	Define constants for SFX directory path, cache size, and AudioStreamPlayer pool size. Initialize an AudioStreamPlayer pool in _ready for UI SFX reuse. Add dictionaries to cache loaded SFX streams and remember missing SFX to avoid repeated load attempts. Implement play_sfx API that loads or retrieves cached streams, applies LRU-style eviction, selects a pooled player, validates the target bus, and plays the sound.	scripts/managers/audio_manager.gd
Add audio constants for common UI SFX IDs and clarify bus naming.	Add SFX_SLIDER, SFX_MUTE_TOGGLE, and SFX_UI_NAVIGATION constants for SFX asset IDs. Group existing audio bus name constants under a commented section header for clarity.	scripts/resources/audio_constants.gd
Prevent double UI sounds when using keyboard to adjust sliders by suppressing navigation SFX in that context.	Capture the focused control in Globals._input and reuse it for context checks. Before playing navigation SFX on ui_left/ui_right, early-return when the focused control is a Slider so adjustments don’t also trigger navigation sounds.	scripts/core/globals.gd
Document scripts/ directory structure and update milestones in the README.	Add DeepSource to the tooling list. Document the refactored scripts/ subdirectory layout in a table for easier navigation. Add Milestone 14 and Milestone 16 status descriptions, including version and focus. Clarify current architecture and testing status in the milestones section.	README.md
Update existing GDUnit4 tests for VolumeSlider to use constants and correct slider configuration.	Preload the VolumeSlider script resource explicitly to work around class_name registry issues. Use AudioConstants bus names instead of hardcoded strings. Configure max_value and step on the slider in tests to avoid float snapping and reflect inspector settings. Tighten assertions on the debounce timer started/stopped state.	test/gdunit4/test_volume_slider.gd
Add GUT tests covering VolumeSlider logic, including programmatic guards, SFX gating, rate limiting, and invalid-bus behavior.	Create a gut VolumeSlider suite that snapshots AudioManager state and isolates config-path side effects. Verify initialization, programmatic updates (no debounce), manual changes (debounce starts), and drag state behavior. Use a mock AudioManager to assert that SFX plays only on valid interactions and is blocked for identical values, no-interaction cases, and within the cooldown window. Add tests ensuring sliders with invalid bus names disable themselves, block programmatic updates, and avoid connecting signals.	test/gut/test_volume_slider.gd test/gut/test_volume_slider.gd.uid
Add GUT tests to ensure AudioWebBridge DOM sync is one-way and does not create feedback loops.	Introduce MockOSWrapper and MockJSBridgeWrapper to force web feature detection and capture JS eval calls. Instantiate AudioWebBridge with injected mocks before _ready runs. Test that volume and mute state changes from Godot produce single, direct DOM property assignments via eval without triggering browser events back into Godot.	test/gut/test_audio_web_bridge_dom_sync.gd test/gut/test_audio_web_bridge_dom_sync.gd.uid
Add GUT tests to verify audio/UI sync decoupling between AudioManager, sliders, and settings scene.	Instantiate the audio settings scene from GamePaths, snapshot AudioManager state, and use an isolated test config file. Assert that global volume change callbacks update slider values via set_value_no_signal so debounce timers remain stopped. Assert that _sync_ui_from_manager updates slider values for all buses without starting debounce timers, preventing initialization feedback loops.	test/gut/test_audio_sync_decoupling.gd test/gut/test_audio_sync_decoupling.gd.uid
Add import metadata for the new slider SFX asset.	Add a .import file for files/sounds/sfx/slider.wav so Godot imports the new slider sound asset for use by AudioManager.play_sfx and VolumeSlider.	files/sounds/sfx/slider.wav.import

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#456	Update README.md to document the refactored scripts/ directory structure introduced in the Milestone 16 work, improving project navigation and onboarding.	✅
#456	Update README.md milestone documentation to reflect the current status and focus of Milestone 16 (and related recent milestones), so the README serves as an up-to-date hub for tracking project progress.	✅
#563	Add a dedicated slider.wav SFX asset and hook it into the audio system via AudioManager and AudioConstants so it can be requested explicitly.	✅
#563	Update VolumeSlider so that it plays the dedicated slider SFX only during genuine manual user interactions (mouse drag or focused keyboard adjustments), with rate limiting, while programmatic updates do not trigger SFX or immediate saves.	✅
#563	Ensure UI/web-bridge-driven volume sync uses programmatic slider updates that bypass value_changed signals, preventing slider SFX and debounce saves during automated changes.	✅
#564	Add the slider.wav audio file to the project’s SFX sound library so it is imported by Godot and available via the filesystem.	✅
#564	Integrate the new slider.wav sound into the UI so that volume slider interactions use this dedicated sound (instead of the generic navigation SFX), proving the asset is correctly accessible and wired into the audio system.	✅
#565	Add an AudioManager.play_sfx API that plays non-positional SFX with caching, defaulting to the SFX_Menu bus, validating/falling back on invalid buses, and safely handling missing files without crashes.	✅
#565	Integrate VolumeSlider to play the dedicated slider SFX via AudioManager.play_sfx only on genuine user interactions (mouse drag / keyboard focus), not on programmatic changes, while routing through the SFX_Menu bus and avoiding performance or feedback-loop issues.	✅
#566	Connect VolumeSlider to centralized AudioManager.play_sfx("slider") so that manual slider adjustments (mouse drag/click, including when cursor leaves bounds, and keyboard/gamepad when focused) play slider.wav, while programmatic changes do not trigger any SFX.	✅
#566	Add robust guards around SFX playback in VolumeSlider: a delta guard using _previous_value and is_equal_approx() so no sound plays when the effective value does not change, and rate limiting using Time.get_ticks_msec() with a ~60ms cooldown to throttle sounds during rapid sliding.	✅
#566	Ensure slider feedback respects the SFX_Menu audio path and mute hierarchy by routing through the new AudioManager.play_sfx API (using the SFX_SLIDER ID on the SFX_Menu bus) and by providing a programmatic update path that bypasses value_changed signals to avoid initialization/sync sound storms and unnecessary saves.	✅
#567	Ensure that programmatic volume sync in audio_settings.gd (e.g., in _on_global_volume_changed and _sync_ui_from_manager) updates HSliders via a no-signal path (set_value_no_signal or equivalent) so that slider SFX and debounced saves are not triggered during web/config-driven sync.	✅
#567	Ensure AudioWebBridge updates the HTML DOM directly for volume/mute sync (pure JS property assignment) without causing events/signals that feed back into Godot sliders.	✅
#567	Add automated GUT tests that verify audio sync decoupling: calling the audio settings scene’s _on_global_volume_changed and _sync_ui_from_manager updates the relevant HSlider.value while the save_debounce_timer remains stopped, and add tests validating that AudioWebBridge DOM sync is one-way (JS-only) and does not re-emit signals.	✅

Possibly linked issues

• [FEATURE] Connect VolumeSlider to Contextual Audio Feedback #566: They match: PR wires VolumeSlider to AudioManager.play_sfx with interaction gating, rate-limiting, and programmatic-change guards as specified.
• #FEATURE Verify Signal Decoupling for Web and UI Sync: They match exactly: PR adds set_value_no_signal-based sync, DOM-only web updates, and the specified GUT decoupling tests.
• [FEATURE] Implement play_sfx Method in AudioManager #565: They both implement a dedicated slider.wav sound for manual slider changes, excluding automated web-bridge or programmatic updates.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds a Godot import remap for files/sounds/sfx/slider.wav; implements SFX caching and a centralized play_sfx(...) in AudioManager; updates VolumeSlider to debounce, rate-limit, and suppress duplicate SFX during manual adjustments; adds audio constants; introduces GUT tests and README/config updates.

Changes

Cohort / File(s)	Summary
Audio Asset Import files/sounds/sfx/slider.wav.import	New Godot import remap for slider.wav: importer/stream type, pinned UID/destination, sample mapping, and import parameters (8-bit/mono/max-rate, trimming/normalization, loop bounds, compression).
Audio Manager & Constants scripts/managers/audio_manager.gd, scripts/resources/audio_constants.gd	Adds _sfx_cache and play_sfx(sfx_name, bus_name, pitch_scale, volume_db) to load/cache AudioStreams, validate bus, spawn temporary AudioStreamPlayer and cleanup; introduces SFX ID constants (SFX_SLIDER, SFX_MUTE_TOGGLE, SFX_UI_NAVIGATION).
UI Slider Component scripts/ui/components/volume_slider.gd	Tracks _previous_value, adds SFX_COOLDOWN_MS and _last_sfx_time, rate-limits and gates SFX to manual interactions, debounced saves, set_value_programmatically() to update without emitting value_changed, and handles invalid bus by disabling itself.
Core Input Handling scripts/core/globals.gd	Caches gui_get_focus_owner() into focus_owner, derives ui_has_focus, and suppresses navigation SFX for left/right when a Slider has focus.
Documentation & Repo Config README.md, .all-contributorsrc	README: adds DeepSource, documents scripts/ architecture, and Milestones 14 & 16; .all-contributorsrc: initializes explicit empty contributors array.
Tests (GUT) test/gut/test_audio_sync_decoupling.gd, test/gut/test_audio_sync_decoupling.gd.uid, test/gut/test_audio_web_bridge_dom_sync.gd, test/gut/test_audio_web_bridge_dom_sync.gd.uid, test/gut/test_volume_slider.gd, test/gut/test_volume_slider.gd.uid	Adds test suites for UI↔backend decoupling, AudioWebBridge DOM sync with JS mocks, and VolumeSlider behaviors (debounce, programmatic set, SFX gating/cooldown); includes UID files for new tests.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant VolumeSlider as VolumeSlider
    participant AudioServer as AudioServer
    participant AudioManager as AudioManager
    participant FS as FileSystem
    participant Player as AudioStreamPlayer

    User->>VolumeSlider: interact (drag / keyboard)
    VolumeSlider->>AudioServer: set bus volume
    VolumeSlider->>AudioManager: play_sfx("slider", bus, pitch, vol)
    AudioManager->>AudioManager: check _sfx_cache
    alt cache miss
        AudioManager->>FS: load AudioStream (`files/sounds/sfx/slider.wav`)
        FS-->>AudioManager: AudioStream
        AudioManager->>AudioManager: cache stream (evict if needed)
    end
    AudioManager->>Player: create AudioStreamPlayer(stream, bus, pitch, vol)
    Player->>Player: play()
    Player-->>AudioManager: finished -> queue_free()

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

• [FEATURE] Implement play_sfx Method in AudioManager #565 — Implements play_sfx(...) with SFX caching and bus validation matching AudioManager changes.
• [FEATURE] Implement a dedicated slider.wav sound effect that triggers only during manual slider adjustments #563 — Adds slider.wav import and integrates slider-driven SFX playback and rate-limiting consistent with VolumeSlider changes.
• [FEATURE] Connect VolumeSlider to Contextual Audio Feedback #566 — Supplies the slider.wav import plus AudioManager/VolumeSlider integration for rate-limited slider SFX.
• [FEATURE] Verify Signal Decoupling for Web and UI Sync #567 — Adds GUT tests and APIs to verify programmatic UI syncs don't trigger slider SFX/save debounces, matching the issue objective.

Possibly related PRs

• Refactor Phase 4: Migrate Resources, Path Centralization, Test Suite Stabilization, and Docs Alignment #582 — Touches audio constants and relates to the added SFX constants in this PR.
• Refactor Phase 2: Migrate UI scripts #525 #580 — Updates UI scripts and aligns with the migrated volume_slider implementation used here.
• Migrate Core and Manager Scripts. Align CI pipeline and tests with new script layout, add GUT. #576 — Modifies manager and core script layout related to the audio/manager and globals changes in this PR.

Suggested labels

testing, GUI, documentation

Poem

🐰 I nudged a slider, soft and small,
A tiny ping that echoed the hall,
Streams cached and imports neat,
Tests green, docs complete—hop, code replete! 🎶

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: implementing a dedicated slider sound effect that triggers only during manual adjustments.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	PR description substantially exceeds template structure with extensive technical detail, multiple summaries, contributor attribution, and architecture documentation.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch SFX

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[sourcery-ai]

Hey - I've left some high level feedback:

• Double-check whether .import files like slider.wav.import are meant to be source-controlled in this project, as some teams rely on per-developer generated import data instead.
• Consider confirming the folder structure and naming convention for SFX assets (e.g., files/sounds/sfx/) matches existing patterns so future audio assets stay organized consistently.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Double-check whether `.import` files like `slider.wav.import` are meant to be source-controlled in this project, as some teams rely on per-developer generated import data instead.
- Consider confirming the folder structure and naming convention for SFX assets (e.g., `files/sounds/sfx/`) matches existing patterns so future audio assets stay organized consistently.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[deepsource-io]

DeepSource Code Review

We reviewed changes in 7eb7e8c...12c21c3 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade

Security   Reliability   Complexity   Hygiene

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
Python		Apr 26, 2026 8:18p.m.	Review ↗
JavaScript		Apr 26, 2026 8:18p.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]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@files/sounds/sfx/slider.wav.import`:
- Around line 1-24: The slider.wav asset is imported but never played; add an
AudioStreamPlayer instance (e.g., slider_player) and wire it into the existing
audio code so sliding produces feedback: in volume_slider.gd create or preload
an AudioStream for "res://files/sounds/sfx/slider.wav", instantiate an
AudioStreamPlayer (slider_player), set its bus to the menu volume bus used by
the project (same bus name used elsewhere, e.g., "Menu" or the bus constant),
and call slider_player.play() from the slider value change handler only for
genuine user interactions; implement the user-interaction gating by listening
for gui_input on the slider (handle InputEventMouseButton/Touch events) or by
setting/clearing a local flag like user_is_interacting that is set on gui_input
and checked in _on_value_changed to avoid playing on programmatic changes, and
ensure the new player is freed or reused consistently (attach to the same node
as other audio players and follow existing naming conventions).

🪄 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: 0273c5e7-8d6e-4784-80e1-2fe5fb56e3a0

📥 Commits

Reviewing files that changed from the base of the PR and between 7eb7e8c and 0a50250.

⛔ Files ignored due to path filters (1)

• files/sounds/sfx/slider.wav is excluded by !**/*.wav

📒 Files selected for processing (1)

• files/sounds/sfx/slider.wav.import

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: GUT Unit Tests / unit-test
• GitHub Check: GDUnit4 Unit Tests / unit-test

[ikostan]

@all-contributors please add @sourcery-ai for review
@all-contributors please add @coderabbitai for review
@all-contributors please add @deepsource for review

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@ikostan, thanks for the mention! The @all-contributors bot should pick up all three requests and add @sourcery-ai, @coderabbitai, and @deepsource as contributors with the review contribution type to your project's .all-contributorsrc and README.md. 🎉

If the bot doesn't respond automatically, make sure it's installed on the repository (via the All Contributors GitHub App) or that the @all-contributors bot has the necessary permissions. You can also use the all-contributors CLI as an alternative.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

README.md (1)

157-157: Spelling variant inconsistency: use "centralized" for consistency.

The project predominantly uses American English spelling. Line 157 uses "centralised" (British), which is inconsistent with other documentation.

🔤 Proposed fix

-| `scripts/core/`      | Foundational systems: `game_paths.gd` (centralised path registry), `globals.gd`, `main_scene.gd`, `settings.gd`                                                                              |
+| `scripts/core/`      | Foundational systems: `game_paths.gd` (centralized path registry), `globals.gd`, `main_scene.gd`, `settings.gd`                                                                              |

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@README.md` at line 157, Replace the British spelling "centralised" with the
American spelling "centralized" in the README table entry that reads
"centralised path registry" (the table row listing `scripts/core/` and
`game_paths.gd`) so the documentation consistently uses American English.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@README.md`:
- Around line 154-161: The markdown linter is failing because the table rows
containing entries like `scripts/core/`, `scripts/resources/`,
`scripts/entities/`, `scripts/system/`, and `scripts/managers/` exceed the
80-character limit; fix it by surrounding this table with the markdown-lint
disable/enable comments (disable MD013 before the table and re-enable after) so
the long cells are ignored, or alternatively wrap the long cell content across
multiple lines within the table cells (e.g., split descriptions into stacked
lines) if you prefer not to change linter directives—apply the change near the
table block that lists the `scripts/*/` directories.
- Line 80: The slider.wav asset was added but not used; update the
volume_slider.gd component to play the sound in the _on_value_changed() handler:
load or preload the files/sounds/sfx/slider.wav asset (or reference an
AudioStreamPlayer node) and trigger a short playback inside _on_value_changed()
(respecting the existing debounce/save logic), ensuring you do not block save
debounce; also remove or correct unrelated README edits (the Deepsource and
unrelated scripts/ reorganization notes) so the README reflects only this PR's
slider-audio change.

---

Nitpick comments:
In `@README.md`:
- Line 157: Replace the British spelling "centralised" with the American
spelling "centralized" in the README table entry that reads "centralised path
registry" (the table row listing `scripts/core/` and `game_paths.gd`) so the
documentation consistently uses American English.

🪄 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: c9f3011e-06f4-4ce9-988c-a4ad52d05e62

📥 Commits

Reviewing files that changed from the base of the PR and between 0a50250 and 6f83b6f.

📒 Files selected for processing (1)

• README.md

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Sourcery review

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2026-04-10T00:07:55.427Z

Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 534
File: scripts/player.gd:146-149
Timestamp: 2026-04-10T00:07:55.427Z
Learning: In `scripts/globals.gd` and `scripts/player.gd` (GDScript, Godot 4), `current_fuel` is intentionally treated as volatile session data and should NOT be saved to or loaded from `settings.cfg`. Only `max_fuel` (tank capacity) is a persistent setting. `current_fuel` is always reset to `max_fuel` unconditionally in `player._ready()`. Persisting `current_fuel` is considered an architectural mistake by the project maintainer (ikostan). Mid-run fuel state persistence is planned via a separate `SaveGameResource` in PR `#535`.

Applied to files:

• README.md

🪛 GitHub Actions: Pull Request Pipeline

README.md

[error] 154-154: markdownlint-cli2 (markdownlint v0.40.0) reported MD013/line-length: Line length expected 80, actual 215.

🪛 GitHub Check: Markdown Lint / lint (3.x)

README.md

[failure] 161-161: Line length
README.md:161:81 MD013/line-length Line length [Expected: 80; Actual: 215] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md013.md

---

[failure] 160-160: Line length
README.md:160:81 MD013/line-length Line length [Expected: 80; Actual: 215] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md013.md

---

[failure] 159-159: Line length
README.md:159:81 MD013/line-length Line length [Expected: 80; Actual: 215] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md013.md

---

[failure] 158-158: Line length
README.md:158:81 MD013/line-length Line length [Expected: 80; Actual: 215] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md013.md

---

[failure] 157-157: Line length
README.md:157:81 MD013/line-length Line length [Expected: 80; Actual: 215] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md013.md

---

[failure] 156-156: Line length
README.md:156:81 MD013/line-length Line length [Expected: 80; Actual: 215] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md013.md

---

[failure] 154-154: Line length
README.md:154:81 MD013/line-length Line length [Expected: 80; Actual: 215] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md013.md

🪛 LanguageTool

README.md

[uncategorized] ~157-~157: Do not mix variants of the same word (‘centralise’ and ‘centralize’) within a single text.
Context: ... Foundational systems: game_paths.gd (centralised path registry), globals.gd, `main_sce...

(EN_WORD_COHERENCY)

🔇 Additional comments (2)

README.md (2)

308-320: Incorrect PR reference and mischaracterization of commit scope.

This is PR #456 (not #578), a documentation update for Milestone #16. The commit message explicitly references "PR #582" and the purpose is to update the README to align with Release v0.9.18. This is a large infrastructure commit that adds slider.wav and many other assets to the repository—not a feature PR that requires slider.wav to be integrated.

The milestone documentation (lines 308-320) is appropriate for this update: both Milestone 14 and Milestone 16 correctly document the project's status and active focus areas at their respective release points. No changes to the milestone sections are necessary.

			> Likely an incorrect or invalid review comment.

---

152-152: No action needed. The reference to PR #582 is accurate and documented correctly. There is no PR #578 in the repository history, so no mismatch exists between the documented PR number and the actual PR this change belongs to.

			> Likely an incorrect or invalid review comment.

[sourcery-ai]

Hey - I've found 2 issues, and left some high level feedback:

• The new VolumeSlider tests reach into several internal members and helper methods (e.g., _is_dragging, _previous_value, _handle_slider_sfx); consider adding small public/query helpers or refactoring the logic behind a non-underscored method so tests rely less on private implementation details and are more resilient to refactors.
• In AudioManager.play_sfx, you create a new AudioStreamPlayer node on every call and only rely on the finished signal for cleanup; if slider or UI sounds can be triggered frequently, consider pooling or reusing a small number of players to avoid excessive node churn and signal connections at runtime.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new `VolumeSlider` tests reach into several internal members and helper methods (e.g., `_is_dragging`, `_previous_value`, `_handle_slider_sfx`); consider adding small public/query helpers or refactoring the logic behind a non-underscored method so tests rely less on private implementation details and are more resilient to refactors.
- In `AudioManager.play_sfx`, you create a new `AudioStreamPlayer` node on every call and only rely on the `finished` signal for cleanup; if slider or UI sounds can be triggered frequently, consider pooling or reusing a small number of players to avoid excessive node churn and signal connections at runtime.

## Individual Comments

### Comment 1
<location path="scripts/ui/components/volume_slider.gd" line_range="81-94" />
<code_context>
+## :param new_value: The target volume (0.0 to 1.0).
+## :type new_value: float
+## :rtype: void
+func set_value_programmatically(new_value: float) -> void:
+	# Guard against external updates if the bus is invalid
+	if bus_index == -1:
+		return
+
+	# Godot 4 native method: updates visual value without emitting 'value_changed'
+	set_value_no_signal(new_value)
+
+	# Explicitly sync the audio backend, since the signal was bypassed
+	AudioServer.set_bus_volume_db(bus_index, linear_to_db(new_value))
+	AudioManager.set_volume(bus_name, new_value)
+
+	# Sync the delta tracker so the next manual interaction calculates correctly
+	_previous_value = new_value
+
+
</code_context>
<issue_to_address>
**suggestion:** Consider clamping programmatic values to the slider’s range before applying them.

Currently `set_value_programmatically` forwards `new_value` directly to `set_value_no_signal`, `AudioServer`, and `AudioManager`. If callers pass values outside the configured range, you can get UI/backend divergence or invalid dB conversions. Clamping `new_value` to the slider’s `[min_value, max_value]` (and using the clamped value for `_previous_value`) would make this safer against incorrect callers.

```suggestion
func set_value_programmatically(new_value: float) -> void:
	# Guard against external updates if the bus is invalid
	if bus_index == -1:
		return

	# Clamp to the slider's configured range to avoid UI/backend divergence
	var clamped_value := clamp(new_value, min_value, max_value)

	# Godot 4 native method: updates visual value without emitting 'value_changed'
	set_value_no_signal(clamped_value)

	# Explicitly sync the audio backend, since the signal was bypassed
	AudioServer.set_bus_volume_db(bus_index, linear_to_db(clamped_value))
	AudioManager.set_volume(bus_name, clamped_value)

	# Sync the delta tracker so the next manual interaction calculates correctly
	_previous_value = clamped_value
```
</issue_to_address>

### Comment 2
<location path="scripts/ui/components/volume_slider.gd" line_range="102-104" />
<code_context>
+## :param event: The input event passed by the UI system.
+## :type event: InputEvent
+## :rtype: void
+func _on_gui_input(event: InputEvent) -> void:
+	if event is InputEventMouseButton and event.button_index == MOUSE_BUTTON_LEFT:
+		_is_dragging = event.pressed
+
+
</code_context>
<issue_to_address>
**suggestion:** Touch and non-mouse interactions won’t ever flip `_is_dragging`, which could unintentionally suppress SFX.

`_is_dragging` is only updated for `InputEventMouseButton` with `MOUSE_BUTTON_LEFT`, so for touch and other pointer events (`InputEventScreenTouch` / `InputEventScreenDrag`) it remains `false`. Because `_handle_slider_sfx` depends on `_is_dragging` or keyboard focus, pointer interactions that don’t grant focus may never play SFX. To keep behavior consistent across input methods, also update `_is_dragging` for touch/drag events or base the decision on a more general condition (e.g., hover + pressed).

```suggestion
func _on_gui_input(event: InputEvent) -> void:
	if event is InputEventMouseButton and event.button_index == MOUSE_BUTTON_LEFT:
		_is_dragging = event.pressed
	elif event is InputEventScreenTouch:
		# Touch down/up should mirror mouse press/release behavior.
		_is_dragging = event.pressed
	elif event is InputEventScreenDrag:
		# Any active drag implies the pointer is currently dragging the slider.
		_is_dragging = true
```
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (2)

scripts/ui/components/volume_slider.gd (2)

127-150: Guard ordering: _previous_value is committed before interaction/rate guards.

The early commit at Line 133 (before the interaction and cooldown checks) is intentional per the comment, but it has a subtle consequence: if a non-interactive value_changed slips through (e.g., a future code path writes to value directly without set_value_no_signal), _previous_value silently advances and the next genuine user interaction may see no delta and skip SFX. Since the current upstream paths all use set_value_programmatically, this is latent rather than active — worth a brief inline note or moving the commit after the interaction guard so user-driven deltas remain authoritative.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/ui/components/volume_slider.gd` around lines 127 - 150, The handler
_handle_slider_sfx commits _previous_value too early (before checking
interaction and cooldown), which lets non-interactive programmatic changes
advance the previous-value and mask the next real user delta; fix by keeping the
initial is_equal_approx guard, but move the assignment _previous_value =
new_value to after the interaction checks (is_mouse_active / has_focus()) and
the SFX cooldown check so you only update _previous_value when you will actually
play a sound; leave the _last_sfx_time update and
AudioManager.play_sfx(AudioConstants.SFX_SLIDER) as-is after that assignment.

---

111-119: Debounce timer restarts even when the new value is effectively identical.

_on_value_changed unconditionally calls save_debounce_timer.start() and re-applies the dB write. While value_changed typically only fires on real changes, sub-epsilon float jitter (especially from keyboard step rounding or external value writes that bypassed set_value_no_signal) can still trigger this path and schedule a redundant disk save 0.5 s later. Consider gating both the audio write and the debounce on the is_equal_approx check that's currently only used inside _handle_slider_sfx:

♻️ Suggested gating

 func _on_value_changed(new_value: float) -> void:
+	if is_equal_approx(new_value, _previous_value):
+		return
+
 	AudioServer.set_bus_volume_db(bus_index, linear_to_db(new_value))
 	AudioManager.set_volume(bus_name, new_value)
 
 	# Attempt to play interaction feedback
 	_handle_slider_sfx(new_value)
 
 	# Godot automatically restarts an active timer when start() is called
 	save_debounce_timer.start()

Note: _handle_slider_sfx would then no longer need its own is_equal_approx guard.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/ui/components/volume_slider.gd` around lines 111 - 119, The handler
_on_value_changed currently always applies AudioServer.set_bus_volume_db and
AudioManager.set_volume and always starts save_debounce_timer even for
sub-epsilon jitter; add a stored last_applied_value (e.g., a class-level float)
and at the top of _on_value_changed use is_equal_approx(new_value,
last_applied_value) to early-return when effectively identical, otherwise update
AudioServer.set_bus_volume_db(bus_index, linear_to_db(new_value)) and
AudioManager.set_volume(bus_name, new_value), call _handle_slider_sfx(new_value)
(remove its internal is_equal_approx guard), start save_debounce_timer, and set
last_applied_value = new_value so redundant disk saves and audio writes are
avoided.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@scripts/ui/components/volume_slider.gd`:
- Around line 76-80: The docstring for set_value_safely(new_value) incorrectly
claims the input is 0.0–1.0 while the function actually uses the slider's
min_value/max_value; fix by clamping new_value to the slider's configured range
before applying it (use min_value and max_value) and update the docstring to
reflect that callers should pass a value in the slider's configured range (or
document you will clamp to min_value/max_value). Ensure you reference new_value,
min_value, max_value and the set_value_safely method in the change.
- Around line 81-94: set_value_programmatically currently uses the raw new_value
for set_value_no_signal, AudioServer.set_bus_volume_db, AudioManager.set_volume,
and _previous_value, so out-of-range inputs can desync UI and audio; clamp
new_value to the slider bounds (use the control's min_value and max_value) into
a local clamped_value before calling set_value_no_signal(clamped_value),
AudioServer.set_bus_volume_db(bus_index, linear_to_db(clamped_value)),
AudioManager.set_volume(bus_name, clamped_value), and assigning _previous_value
= clamped_value; keep the early guard for bus_index == -1 unchanged.
- Around line 102-104: The _is_dragging flag set in _on_gui_input can get stuck
if the release event is missed; update the control to also clear _is_dragging
when the control loses focus or the window loses focus by handling
NOTIFICATION_FOCUS_EXIT and NOTIFICATION_WM_WINDOW_FOCUS_OUT (implement or
extend _notification to set _is_dragging = false for those notifications), and
avoid using NOTIFICATION_MOUSE_EXIT so out-of-bounds dragging still works; keep
_on_gui_input logic unchanged except relying on these additional notifications
to defensively reset dragging state.

---

Nitpick comments:
In `@scripts/ui/components/volume_slider.gd`:
- Around line 127-150: The handler _handle_slider_sfx commits _previous_value
too early (before checking interaction and cooldown), which lets non-interactive
programmatic changes advance the previous-value and mask the next real user
delta; fix by keeping the initial is_equal_approx guard, but move the assignment
_previous_value = new_value to after the interaction checks (is_mouse_active /
has_focus()) and the SFX cooldown check so you only update _previous_value when
you will actually play a sound; leave the _last_sfx_time update and
AudioManager.play_sfx(AudioConstants.SFX_SLIDER) as-is after that assignment.
- Around line 111-119: The handler _on_value_changed currently always applies
AudioServer.set_bus_volume_db and AudioManager.set_volume and always starts
save_debounce_timer even for sub-epsilon jitter; add a stored last_applied_value
(e.g., a class-level float) and at the top of _on_value_changed use
is_equal_approx(new_value, last_applied_value) to early-return when effectively
identical, otherwise update AudioServer.set_bus_volume_db(bus_index,
linear_to_db(new_value)) and AudioManager.set_volume(bus_name, new_value), call
_handle_slider_sfx(new_value) (remove its internal is_equal_approx guard), start
save_debounce_timer, and set last_applied_value = new_value so redundant disk
saves and audio writes are avoided.

🪄 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: d19ea11d-e0ab-432a-90e9-485b865ebe97

📥 Commits

Reviewing files that changed from the base of the PR and between 1caf738 and 2c92711.

📒 Files selected for processing (2)

• scripts/ui/components/volume_slider.gd
• test/gut/test_volume_slider.gd

🚧 Files skipped from review as they are similar to previous changes (1)

• test/gut/test_volume_slider.gd

📜 Review details

🔇 Additional comments (1)

scripts/ui/components/volume_slider.gd (1)

40-73: LGTM on _ready() hardening.

Bus validation, early disablement (editable/mouse_filter/focus_mode), seeding _previous_value with the linearized initial value, and using the gui_input signal instead of overriding _gui_input together close the previously-flagged issues cleanly.

[ikostan]

• The new VolumeSlider tests reach into several internal members and helper methods (e.g., _is_dragging, _previous_value, _handle_slider_sfx); consider adding small public/query helpers or refactoring the logic behind a non-underscored method so tests rely less on private implementation details and are more resilient to refactors.

Working on it...

[ikostan]

• In AudioManager.play_sfx, you create a new AudioStreamPlayer node on every call and only rely on the finished signal for cleanup; if slider or UI sounds can be triggered frequently, consider pooling or reusing a small number of players to avoid excessive node churn and signal connections at runtime.

Working on this one now...

[ikostan]

@sourcery-ai review

[sourcery-ai]

Hey - I've left some high level feedback:

• In test_volume_slider.gd (GUT suite), test_sfx_guard_allows_valid_interaction contains a second, unrelated block after the cleanup that reinitializes _slider state and calls _handle_slider_sfx; this looks like a copy-paste from the identical-value test and should either be split into a separate test or removed to keep the test’s intent clear and focused.
• In VolumeSlider.set_value_programmatically, you always propagate the clamped value to AudioServer and AudioManager even if it hasn’t changed; adding a short-circuit when is_equal_approx(clamped_value, _previous_value) is true would avoid redundant backend calls and keep behavior consistent with _on_value_changed’s duplicate-guard.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `test_volume_slider.gd` (GUT suite), `test_sfx_guard_allows_valid_interaction` contains a second, unrelated block after the cleanup that reinitializes `_slider` state and calls `_handle_slider_sfx`; this looks like a copy-paste from the identical-value test and should either be split into a separate test or removed to keep the test’s intent clear and focused.
- In `VolumeSlider.set_value_programmatically`, you always propagate the clamped value to `AudioServer` and `AudioManager` even if it hasn’t changed; adding a short-circuit when `is_equal_approx(clamped_value, _previous_value)` is true would avoid redundant backend calls and keep behavior consistent with `_on_value_changed`’s duplicate-guard.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[ikostan]

• In test_volume_slider.gd (GUT suite), test_sfx_guard_allows_valid_interaction contains a second, unrelated block after the cleanup that reinitializes _slider state and calls _handle_slider_sfx; this looks like a copy-paste from the identical-value test and should either be split into a separate test or removed to keep the test’s intent clear and focused.
• In VolumeSlider.set_value_programmatically, you always propagate the clamped value to AudioServer and AudioManager even if it hasn’t changed; adding a short-circuit when is_equal_approx(clamped_value, _previous_value) is true would avoid redundant backend calls and keep behavior consistent with _on_value_changed’s duplicate-guard.

All addressed.