refactor: SOLID #347
Merged
Merged
Conversation
…ct interfaces with segregated base classes
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughRefactors audio backends to an IAudioBackend registry/factory, introduces IAudioEngine and sub-interfaces, extracts Metronome/AnalyzerCapture/AudioMetricsService, overhauls Effect core APIs and per-effect layout, adds WindowContext/UpdateChecker and AmplitronSession, converts PresetManager to IPresetManager with serializer/storage/migrator, updates recorder/MIDI, CMake, and tests. ChangesAmplitron refactor — single cohort
Estimated code review effort Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
… include metronome.cpp in APP_SOURCES
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/audio/engine/audio_engine.h (1)
318-330:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffClarify ownership and lifecycle for dual backend pointers.
AudioEnginenow holds bothbackend_(opaque PortAudio state) andpoly_backend_(polymorphic interface). Since lifecycle methods checkpoly_backend_first and early-return, whenpoly_backend_is set,backend_may remain allocated but unused.Verify that the destructor properly cleans up both pointers when both are set, and document which pointer takes precedence to avoid resource leaks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/engine/audio_engine.h` around lines 318 - 330, AudioEngine holds both backend_ (opaque PortAudio state) and poly_backend_ (polymorphic IAudioBackend) but lifecycle methods short-circuit on poly_backend_, risking leaked backend_ when both are set; update the destructor and shutdown/initialize paths (e.g., ~AudioEngine(), Shutdown(), Initialize()/Start()/Stop() equivalents) to explicitly detect both pointers and perform deterministic cleanup: if poly_backend_ is non-null call its cleanup then delete/reset it, and always also free backend_ (close/terminate and delete) regardless of poly_backend_ presence; document in the class comment which pointer takes precedence (poly_backend_ for operation control) and ensure ownership semantics (who deletes what) are clear and consistent across methods like backend_, poly_backend_, Initialize(), Shutdown(), and the destructor.
🧹 Nitpick comments (4)
src/audio/engine/metronome.h (1)
5-6: 💤 Low valueUnnecessary includes in header.
<cmath>and<algorithm>are not used in this header since there are no inline implementations. Move them to the.cppfile to minimize header dependencies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/engine/metronome.h` around lines 5 - 6, Remove the unnecessary header includes from the metronome.h file: delete `#include` <cmath> and `#include` <algorithm> from the header and instead add them to the corresponding implementation file (metronome.cpp) where any functions needing math or algorithm utilities are defined; keep metronome.h minimal (only forward declarations and necessary includes) and ensure any inline functions still present do not rely on those headers before removing them.src/audio/engine/metronome.cpp (1)
129-129: 💤 Low valueDuplicate constant
kTwoPi.This constant is also defined in
update_timing()(line 66). Extract to a file-level or class-level constant.♻️ Suggested extraction
At the top of the file:
namespace Amplitron { +namespace { +constexpr float kTwoPi = 6.28318530718f; +} // namespace + Metronome::Metronome() {Then remove the local definitions in both
update_timing()andnext_sample().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/engine/metronome.cpp` at line 129, The duplicate kTwoPi constant should be lifted out of the two local definitions into a single file-level or class-level constant so both update_timing() and next_sample() use the same symbol; add a single constexpr float kTwoPi = 6.28318530718f at the top of the metronome.cpp file or inside the Metronome class scope, then remove the local kTwoPi definitions from update_timing() and next_sample() and reference the shared kTwoPi instead.src/audio/effects/effect.h (2)
166-167: ⚡ Quick win
mix_is non-atomic here but atomic inEffectNode—potential data race.
Effect::mix_is a plainfloatwhileEffectNode::mix_isstd::atomic<float>. Ifset_mix()is called from the UI thread whileget_mix()orapply_mix()runs on the audio thread, this is a data race inEffectsubclasses that don't useEffectNode.Consider making
mix_atomic for consistency:- float mix_ = 1.0f; + std::atomic<float> mix_{1.0f};And update accessors accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/effects/effect.h` around lines 166 - 167, Effect::mix_ is a plain float while EffectNode uses std::atomic<float>, causing a potential data race when UI threads call set_mix() while the audio thread reads mix_ via get_mix() or apply_mix(); change Effect::mix_ to std::atomic<float> (matching EffectNode) and update/set accessors set_mix(), get_mix(), and any apply_mix() usages to load/store atomically (use .store()/.load() or the atomic assignment) while preserving memory ordering appropriate for real-time audio. Ensure enabled_ remains std::atomic<bool> and update any direct reads/writes of mix_ in Effect subclasses to use the atomic API.
140-150: ⚡ Quick win
const_castindicates missing const overload forparams().The
const_cast<Effect*>(this)->params()is a code smell. The root cause is thatIParameterizable::params()lacks a const overload, forcing this workaround in a const method.Consider adding a const accessor to the interface:
virtual const std::vector<EffectParam>& params() const = 0;Then derived classes provide both overloads, and
get_params()calls the const version without casting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/effects/effect.h` around lines 140 - 150, The get_params() const method uses const_cast because IParameterizable::params() lacks a const overload; add a const accessor to the interface (e.g. virtual const std::vector<EffectParam>& params() const = 0) and implement both non-const and const overloads in classes deriving from IParameterizable/Effect so callers can read parameters without casting, then update Effect::get_params() to call the const params() overload and remove the const_cast; keep existing usage of enabled_ and mix_ unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/audio/effects/effect_node.h`:
- Around line 34-42: The code currently allocates a temporary vector inside the
realtime audio path (std::vector<float> dry(num_samples)) which violates
audio-callback rules; replace this per-call heap allocation with a pre-allocated
member scratch buffer (e.g., a std::vector<float> dry_buffer_ or float*
dry_buffer_) owned by the EffectNode class, ensure you resize it only when block
size or sample rate changes (not per-process), and then use dry_buffer_.data()
in place of the local vector before calling processor_->process(...) and the
mixing loop that reads mix_.load(); ensure resizing and any potential
locking/updating of the scratch buffer happens outside the realtime process() /
process_stereo() path.
In `@src/audio/engine/audio_engine_api.cpp`:
- Around line 23-25: AudioEngine::toggle_metronome currently does a separate
read (metronome_.is_enabled()) then write (metronome_.set_enabled(...)) which
creates a TOCTOU race; add an atomic toggle on Metronome and call it instead:
implement Metronome::toggle() to perform an atomic read-modify-write on the
internal enabled_ atomic (e.g. fetch_xor(1) with appropriate memory_order),
expose it in metronome.h, and replace the body of AudioEngine::toggle_metronome
to call metronome_.toggle() so toggling is atomic and race-free.
In `@src/audio/engine/audio_engine.cpp`:
- Around line 199-206: replace_backend_for_test(IAudioBackend*) currently
assigns poly_backend_ without clarifying ownership and does not delete the old
or new backend, unlike the AudioBackendState* overload that calls
destroy_audio_backend(); update the implementation and/or class to make
ownership explicit: either (A) document that AudioEngine does NOT take ownership
and ensure destructor/cleanup does not delete poly_backend_ (add comment to
replace_backend_for_test and class-level doc), or (B) make AudioEngine own the
injected backend by calling destroy_audio_backend() before assigning and
ensuring the destructor deletes poly_backend_ (and update class destructor
accordingly); reference: replace_backend_for_test, poly_backend_,
destroy_audio_backend(), and the AudioEngine destructor to implement the chosen
ownership semantics consistently.
In `@src/audio/engine/metronome.cpp`:
- Around line 33-36: The member sample_rate_ is written in
Metronome::set_sample_rate and read in Metronome::next_sample, causing a data
race if set_sample_rate can be called while the audio thread runs; fix by making
sample_rate_ an std::atomic<int> (update the declaration in metronome.h) and
change all reads/writes in Metronome::set_sample_rate and Metronome::next_sample
(and any other accessors) to use atomic load/store semantics, or if you prefer a
simpler contract, document in Metronome (and set_sample_rate) that the stream
must be stopped before calling set_sample_rate and enforce it via an assertion;
ensure update_timing() behavior remains correct for the chosen approach.
In `@tests/unit/test_mock_backend.cpp`:
- Around line 63-91: The test TEST(AudioBackend_PolymorphicMockBackendInjection)
leaks MockAudioBackend because it is created with new and
replace_backend_for_test(IAudioBackend*) does not take ownership; fix by owning
the mock in the test (e.g., construct MockAudioBackend on the stack or manage it
with a std::unique_ptr and ensure it's deleted/released appropriately after
engine.shutdown()) so MockAudioBackend is destroyed at test end, referencing
MockAudioBackend, replace_backend_for_test, AudioEngine, and the test function
name to locate the code.
---
Outside diff comments:
In `@src/audio/engine/audio_engine.h`:
- Around line 318-330: AudioEngine holds both backend_ (opaque PortAudio state)
and poly_backend_ (polymorphic IAudioBackend) but lifecycle methods
short-circuit on poly_backend_, risking leaked backend_ when both are set;
update the destructor and shutdown/initialize paths (e.g., ~AudioEngine(),
Shutdown(), Initialize()/Start()/Stop() equivalents) to explicitly detect both
pointers and perform deterministic cleanup: if poly_backend_ is non-null call
its cleanup then delete/reset it, and always also free backend_ (close/terminate
and delete) regardless of poly_backend_ presence; document in the class comment
which pointer takes precedence (poly_backend_ for operation control) and ensure
ownership semantics (who deletes what) are clear and consistent across methods
like backend_, poly_backend_, Initialize(), Shutdown(), and the destructor.
---
Nitpick comments:
In `@src/audio/effects/effect.h`:
- Around line 166-167: Effect::mix_ is a plain float while EffectNode uses
std::atomic<float>, causing a potential data race when UI threads call set_mix()
while the audio thread reads mix_ via get_mix() or apply_mix(); change
Effect::mix_ to std::atomic<float> (matching EffectNode) and update/set
accessors set_mix(), get_mix(), and any apply_mix() usages to load/store
atomically (use .store()/.load() or the atomic assignment) while preserving
memory ordering appropriate for real-time audio. Ensure enabled_ remains
std::atomic<bool> and update any direct reads/writes of mix_ in Effect
subclasses to use the atomic API.
- Around line 140-150: The get_params() const method uses const_cast because
IParameterizable::params() lacks a const overload; add a const accessor to the
interface (e.g. virtual const std::vector<EffectParam>& params() const = 0) and
implement both non-const and const overloads in classes deriving from
IParameterizable/Effect so callers can read parameters without casting, then
update Effect::get_params() to call the const params() overload and remove the
const_cast; keep existing usage of enabled_ and mix_ unchanged.
In `@src/audio/engine/metronome.cpp`:
- Line 129: The duplicate kTwoPi constant should be lifted out of the two local
definitions into a single file-level or class-level constant so both
update_timing() and next_sample() use the same symbol; add a single constexpr
float kTwoPi = 6.28318530718f at the top of the metronome.cpp file or inside the
Metronome class scope, then remove the local kTwoPi definitions from
update_timing() and next_sample() and reference the shared kTwoPi instead.
In `@src/audio/engine/metronome.h`:
- Around line 5-6: Remove the unnecessary header includes from the metronome.h
file: delete `#include` <cmath> and `#include` <algorithm> from the header and
instead add them to the corresponding implementation file (metronome.cpp) where
any functions needing math or algorithm utilities are defined; keep metronome.h
minimal (only forward declarations and necessary includes) and ensure any inline
functions still present do not rely on those headers before removing them.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6cb0ef43-d6fd-4b7a-a1cf-470812cd720f
⛔ Files ignored due to path filters (2)
short.wavis excluded by!**/*.wavvery_long_ir_file_name_that_should_be_truncated.wavis excluded by!**/*.wav
📒 Files selected for processing (13)
CMakeLists.txtsrc/audio/backend/audio_backend.hsrc/audio/backend/audio_backend_portaudio_devices.cppsrc/audio/backend/audio_backend_portaudio_lifecycle.cppsrc/audio/effects/effect.hsrc/audio/effects/effect_node.hsrc/audio/engine/audio_engine.cppsrc/audio/engine/audio_engine.hsrc/audio/engine/audio_engine_api.cppsrc/audio/engine/audio_engine_process.cppsrc/audio/engine/metronome.cppsrc/audio/engine/metronome.htests/unit/test_mock_backend.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/audio/backend/audio_backend_jack.cpp`:
- Around line 132-137: When poly_backend_ is shutdown the code only clears
initialized_ but leaves running_ true; update the poly-backend shutdown branch
in audio_backend_jack.cpp so that after poly_backend_->shutdown() you also clear
running_ (or call stop()) to mirror the non-poly path and ensure
is_running()/start() behave correctly; apply the same change to the
corresponding poly-backend shutdown branches in the Oboe and SDL backend
implementations so running_ is consistently cleared when their poly backends are
shut down.
In `@src/audio/backend/audio_backend_oboe.cpp`:
- Around line 211-215: When poly_backend_ is present the shutdown path calls
poly_backend_->shutdown() and sets initialized_ = false but does not clear
running_; update the poly-backend branch to also clear running_ (same state
change performed by stop()) so engine state remains consistent — locate the
block where poly_backend_->shutdown() is called and add resetting running_
(e.g., running_ = false) after shutdown and before returning, keeping the
existing initialized_ handling unchanged.
In `@src/audio/backend/audio_backend_sdl.cpp`:
- Around line 97-101: In the poly-backend shutdown branch, ensure the running_
flag is cleared so it doesn't remain stale: after calling
poly_backend_->shutdown() and before returning (in the block guarded by
poly_backend_), set running_ = false (keeping initialized_ = false as currently
done) so behavior matches the non-poly path where stop() clears running_.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e57ef78-4f9e-4c4f-adca-5a8606574929
📒 Files selected for processing (4)
CMakeLists.txtsrc/audio/backend/audio_backend_jack.cppsrc/audio/backend/audio_backend_oboe.cppsrc/audio/backend/audio_backend_sdl.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
…nts, and memory leak concerns across DSP effects, audio engine, and test mocks
Contributor
🌍 Cross-Platform Coverage ReportPlatforms: Linux + macOS + Windows ✅ Coverage meets threshold: 76.7% >= 60% Full Merged Coverage Summary |
Contributor
PR Preview RemovedThe GitHub Pages preview for this PR has been removed because the PR was closed. |
…d classes and introduce a metrics service for audio analysis.
…audio backend interfaces and definitions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/audio/engine/audio_engine.h (1)
118-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid nullptr-ambiguous backend test overloads.
src/audio/engine/audio_engine.hdefines tworeplace_backend_for_testoverloads taking different pointer types; this makesreplace_backend_for_test(nullptr)(and similar null-like literals) ambiguous at compile time. Existing tests only pass non-null pointers, but add a disambiguating API (e.g., rename one overload and/or addclear_backend_for_test()for teardown).🛠️ One low-risk way to disambiguate
- void replace_backend_for_test(AudioBackendState* backend); + void replace_native_backend_for_test(AudioBackendState* backend); void replace_backend_for_test(IAudioBackend* backend); + void clear_backend_for_test();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/engine/audio_engine.h` around lines 118 - 119, The two overloads replace_backend_for_test(AudioBackendState*) and replace_backend_for_test(IAudioBackend*) make calls like replace_backend_for_test(nullptr) ambiguous; disambiguate by introducing a clear_backend_for_test() for teardown and keeping a single setter like replace_backend_for_test(IAudioBackend*). Concretely: remove or rename the AudioBackendState* overload (e.g., to replace_backend_state_for_test) and add a void clear_backend_for_test() declaration/implementation that explicitly clears the backend; update callers/tests to call replace_backend_for_test(...) with non-null IAudioBackend* to set and clear_backend_for_test() to teardown.
🧹 Nitpick comments (8)
src/audio/backend/audio_device_info.h (1)
7-14: ⚡ Quick winAdd default member initializers to avoid indeterminate values.
The scalar members are left uninitialized. A default-constructed
AudioDeviceInfo(e.g. aggregate-init that omits a field, or a value read before full population) would expose indeterminateindex, channel counts, or sample rate. Defaulting them keeps the struct safe by construction.🛡️ Proposed defaults
struct AudioDeviceInfo { - int index; + int index = -1; std::string name; - int max_input_channels; - int max_output_channels; - double default_sample_rate; - bool is_usb_device; + int max_input_channels = 0; + int max_output_channels = 0; + double default_sample_rate = 0.0; + bool is_usb_device = false; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/backend/audio_device_info.h` around lines 7 - 14, The struct AudioDeviceInfo leaves scalar members uninitialized; update the struct definition (AudioDeviceInfo) to add default member initializers for safety — e.g. set index = -1, max_input_channels = 0, max_output_channels = 0, default_sample_rate = 0.0, and is_usb_device = false (std::string name can remain default-initialized). Modify the declarations of index, max_input_channels, max_output_channels, default_sample_rate, and is_usb_device to include those = ... defaults so default-constructed instances have well-defined values.src/audio/effects/core/effect_param.h (1)
8-16: 💤 Low valueOptional: add default member initializers to numeric fields.
value,min_val,max_val, anddefault_valhave no in-class initializers. Aggregate construction (as used throughout) is fine, but a value-default-constructedEffectParam{}would carry indeterminate floats. Defaulting them (e.g.float value = 0.0f;) is cheap insurance.♻️ Proposed defaults
struct EffectParam { std::string name; - float value; - float min_val; - float max_val; - float default_val; + float value = 0.0f; + float min_val = 0.0f; + float max_val = 1.0f; + float default_val = 0.0f; std::string unit; std::string tooltip; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/effects/core/effect_param.h` around lines 8 - 16, The numeric members of struct EffectParam (value, min_val, max_val, default_val) lack in-class initializers and can be indeterminate for a value-initialized EffectParam; add default member initializers such as "= 0.0f" (or other sensible defaults) to each of these fields in EffectParam so a default-constructed instance is well-defined while leaving the string members as-is.src/audio/effects/core/effect_node.h (1)
38-41: 💤 Low valueOptional: potential allocation in the audio path for oversized blocks.
dry_buffer_is preallocated to 4096, so the resize only fires whennum_samples > 4096, which would allocate inside the processing call. If block sizes can exceed 4096, size the buffers from the real block size inset_sample_rateinstead of relying on the in-processgrow.Also applies to: 61-66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/effects/core/effect_node.h` around lines 38 - 41, dry_buffer_ is resized inside process which can allocate on the audio thread when num_samples exceeds the default 4096; move buffer sizing to set_sample_rate (or another initialization path) so buffers are preallocated to the real maximum block size. Update set_sample_rate (or the same initialization method) to accept/store the maximum expected block size and call dry_buffer_.resize(max_block_samples) (and do the same for the other buffers used in the process path referenced around lines 61-66) so process only uses memcpy into already-sized dry_buffer_.data() and never grows buffers at runtime.src/gui/window_context.cpp (1)
51-67: 💤 Low valueConsider cleaning up SDL state on the
initialize()failure paths.If
SDL_CreateWindowfails after a successfulSDL_Init, orSDL_GL_CreateContextfails after the window is created, the function returnsfalsewithout callingSDL_DestroyWindow/SDL_Quit. Since~WindowContextonly runsshutdown()wheninitialized_is true, these resources leak on a failed init. Worth tightening if init can be retried within a process; otherwise low priority since a failed startup usually aborts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gui/window_context.cpp` around lines 51 - 67, The initialize() function leaves SDL resources allocated on failure: if SDL_CreateWindow fails after SDL_Init, or SDL_GL_CreateContext fails after window_ is created, the function returns false without cleaning up; modify initialize() to call SDL_DestroyWindow(window_) and set window_ = nullptr before returning when SDL_GL_CreateContext fails, and call SDL_Quit() (and clear any initialized flags) when SDL_CreateWindow fails after SDL_Init so state is consistent; ensure shutdown() still safely handles partially initialized state and that initialized_ is only true after all resources are successfully created.src/audio/engine/audio_metrics_service.cpp (1)
14-21: ⚡ Quick winAvoid “seq==0” early-return driving spectrum updates with zero buffers (no safety issue)
- Memory-safety concern doesn’t apply:
analyzer_input_buf_/analyzer_output_buf_arestd::array<float, AudioEngine::ANALYZER_FFT_SIZE>value-initialized, so.data()is nevernullptrand reads are within bounds.- Behavioral concern: when
seq == analyzer_last_sequence_while both are0,AudioMetricsService::update()callsspectrum_analyzer_.update()with zero buffers instead ofcopy_analyzer_snapshot(). Butcopy_analyzer_snapshot()explicitly returnsfalseforseq == 0, so the first real spectrum update won’t happen untilanalyzer_sequence_increments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/engine/audio_metrics_service.cpp` around lines 14 - 21, The current early-return uses the analyzer sequence check and causes spectrum_analyzer_.update() to be called with zeroed buffers when both seq and analyzer_last_sequence_ are 0; change the logic in AudioMetricsService::update (the block using engine.get_analyzer_sequence(), analyzer_last_sequence_, analyzer_input_buf_.data(), analyzer_output_buf_.data(), and spectrum_analyzer_.update()) so that seq==0 does not trigger the “already up-to-date” early-return — e.g., only short-circuit when seq == analyzer_last_sequence_ && seq != 0, or explicitly call copy_analyzer_snapshot() when analyzer_last_sequence_ == 0 so the initial snapshot behavior (copy_analyzer_snapshot() returning false for seq==0) remains respected.src/gui/gui_manager_frame.cpp (1)
229-230: ⚡ Quick winLocal variables shouldn't use the trailing-underscore (member) convention.
window_width_/window_height_are function locals but use the trailing-underscore suffix reserved for class member variables, which is misleading at a glance.♻️ Rename locals to plain snake_case
- int window_width_ = window_context_.get_width(); - int window_height_ = window_context_.get_height(); + int window_width = window_context_.get_width(); + int window_height = window_context_.get_height(); ImGui::SetNextWindowPos(ImVec2(0, 20)); - ImGui::SetNextWindowSize(ImVec2(static_cast<float>(window_width_), - static_cast<float>(window_height_) - 20)); + ImGui::SetNextWindowSize(ImVec2(static_cast<float>(window_width), + static_cast<float>(window_height) - 20));As per coding guidelines: "Class Member Variables should use lowercase snake_case with a trailing underscore" and "Variables and Parameters should use lowercase snake_case".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gui/gui_manager_frame.cpp` around lines 229 - 230, Rename the local variables window_width_ and window_height_ (obtained from window_context_.get_width() / get_height()) to non-member style names (e.g., window_width and window_height) and update all usages within the function accordingly so they no longer use the trailing underscore reserved for class members; keep window_context_ as-is since it's a member.src/audio/engine/graph_link.h (1)
5-9: ⚡ Quick winDefault-initialize struct members.
id,source_pin_id, anddest_pin_idare left uninitialized. If aGraphLinkis ever created without explicitly setting every field, the indeterminate values could feed graph routing/lookup logic and surface as hard-to-trace bugs. Adding default member initializers is a cheap safeguard.♻️ Proposed change
struct GraphLink { - int id; - int source_pin_id; // Mapping from Output Pin - int dest_pin_id; // Mapping to Input Pin + int id = -1; + int source_pin_id = -1; // Mapping from Output Pin + int dest_pin_id = -1; // Mapping to Input Pin };Note: the Clang
unknown type name 'namespace'diagnostic on Line 3 appears to be a false positive — the header is valid C++17.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/engine/graph_link.h` around lines 5 - 9, The GraphLink struct leaves fields id, source_pin_id, and dest_pin_id uninitialized; update the GraphLink definition so each member has a safe default (e.g., = 0) to avoid indeterminate values feeding graph routing logic—modify the struct declaration for GraphLink to add default member initializers for id, source_pin_id, and dest_pin_id.src/audio/effects/utility/tuner.h (1)
8-9: ⚡ Quick winTuner doesn’t need a direct
<vector>include right now
src/audio/effects/utility/tuner.husesstd::vector, but it includesaudio/effects/core/effect.h, which already includes<vector>—so this isn’t relying on an unrelated transitive include. Adding<vector>directly would only be optional “extra insulation” against future changes toeffect.h.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/effects/utility/tuner.h` around lines 8 - 9, The header tuner.h currently relies on a transitive include of <vector> from audio/effects/core/effect.h; make tuner.h self-contained by adding an explicit `#include` <vector> at the top of tuner.h so uses of std::vector in that file are safe even if effect.h changes, and ensure any uses of std::vector in tuner.h (search for occurrences of "std::vector") compile after the change.
🤖 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 `@src/gui/update_checker.cpp`:
- Around line 55-58: The member shutdown_requested_ is a plain bool causing a
data race between the worker thread reading it in the loop and shutdown()
writing it; change its declaration to std::atomic<bool> in update_checker.h,
include <atomic> if missing, and keep the reads/writes via the same name (no
locking needed) so the loop in update_checker.cpp (the fgets loop) and the
shutdown() writer become atomic and race-free.
In `@src/gui/update_checker.h`:
- Line 29: Make shutdown_requested_ atomic to prevent a cross-thread data race:
change the member variable shutdown_requested_ to std::atomic<bool> and update
any reads/writes accordingly so shutdown() writes with simple assignment and the
worker loop in check_for_updates() reads the atomic (no extra locks needed).
Ensure the header includes <atomic> and that any existing initialization
(shutdown_requested_ = false) remains valid for the atomic type.
---
Outside diff comments:
In `@src/audio/engine/audio_engine.h`:
- Around line 118-119: The two overloads
replace_backend_for_test(AudioBackendState*) and
replace_backend_for_test(IAudioBackend*) make calls like
replace_backend_for_test(nullptr) ambiguous; disambiguate by introducing a
clear_backend_for_test() for teardown and keeping a single setter like
replace_backend_for_test(IAudioBackend*). Concretely: remove or rename the
AudioBackendState* overload (e.g., to replace_backend_state_for_test) and add a
void clear_backend_for_test() declaration/implementation that explicitly clears
the backend; update callers/tests to call replace_backend_for_test(...) with
non-null IAudioBackend* to set and clear_backend_for_test() to teardown.
---
Nitpick comments:
In `@src/audio/backend/audio_device_info.h`:
- Around line 7-14: The struct AudioDeviceInfo leaves scalar members
uninitialized; update the struct definition (AudioDeviceInfo) to add default
member initializers for safety — e.g. set index = -1, max_input_channels = 0,
max_output_channels = 0, default_sample_rate = 0.0, and is_usb_device = false
(std::string name can remain default-initialized). Modify the declarations of
index, max_input_channels, max_output_channels, default_sample_rate, and
is_usb_device to include those = ... defaults so default-constructed instances
have well-defined values.
In `@src/audio/effects/core/effect_node.h`:
- Around line 38-41: dry_buffer_ is resized inside process which can allocate on
the audio thread when num_samples exceeds the default 4096; move buffer sizing
to set_sample_rate (or another initialization path) so buffers are preallocated
to the real maximum block size. Update set_sample_rate (or the same
initialization method) to accept/store the maximum expected block size and call
dry_buffer_.resize(max_block_samples) (and do the same for the other buffers
used in the process path referenced around lines 61-66) so process only uses
memcpy into already-sized dry_buffer_.data() and never grows buffers at runtime.
In `@src/audio/effects/core/effect_param.h`:
- Around line 8-16: The numeric members of struct EffectParam (value, min_val,
max_val, default_val) lack in-class initializers and can be indeterminate for a
value-initialized EffectParam; add default member initializers such as "= 0.0f"
(or other sensible defaults) to each of these fields in EffectParam so a
default-constructed instance is well-defined while leaving the string members
as-is.
In `@src/audio/effects/utility/tuner.h`:
- Around line 8-9: The header tuner.h currently relies on a transitive include
of <vector> from audio/effects/core/effect.h; make tuner.h self-contained by
adding an explicit `#include` <vector> at the top of tuner.h so uses of
std::vector in that file are safe even if effect.h changes, and ensure any uses
of std::vector in tuner.h (search for occurrences of "std::vector") compile
after the change.
In `@src/audio/engine/audio_metrics_service.cpp`:
- Around line 14-21: The current early-return uses the analyzer sequence check
and causes spectrum_analyzer_.update() to be called with zeroed buffers when
both seq and analyzer_last_sequence_ are 0; change the logic in
AudioMetricsService::update (the block using engine.get_analyzer_sequence(),
analyzer_last_sequence_, analyzer_input_buf_.data(),
analyzer_output_buf_.data(), and spectrum_analyzer_.update()) so that seq==0
does not trigger the “already up-to-date” early-return — e.g., only
short-circuit when seq == analyzer_last_sequence_ && seq != 0, or explicitly
call copy_analyzer_snapshot() when analyzer_last_sequence_ == 0 so the initial
snapshot behavior (copy_analyzer_snapshot() returning false for seq==0) remains
respected.
In `@src/audio/engine/graph_link.h`:
- Around line 5-9: The GraphLink struct leaves fields id, source_pin_id, and
dest_pin_id uninitialized; update the GraphLink definition so each member has a
safe default (e.g., = 0) to avoid indeterminate values feeding graph routing
logic—modify the struct declaration for GraphLink to add default member
initializers for id, source_pin_id, and dest_pin_id.
In `@src/gui/gui_manager_frame.cpp`:
- Around line 229-230: Rename the local variables window_width_ and
window_height_ (obtained from window_context_.get_width() / get_height()) to
non-member style names (e.g., window_width and window_height) and update all
usages within the function accordingly so they no longer use the trailing
underscore reserved for class members; keep window_context_ as-is since it's a
member.
In `@src/gui/window_context.cpp`:
- Around line 51-67: The initialize() function leaves SDL resources allocated on
failure: if SDL_CreateWindow fails after SDL_Init, or SDL_GL_CreateContext fails
after window_ is created, the function returns false without cleaning up; modify
initialize() to call SDL_DestroyWindow(window_) and set window_ = nullptr before
returning when SDL_GL_CreateContext fails, and call SDL_Quit() (and clear any
initialized flags) when SDL_CreateWindow fails after SDL_Init so state is
consistent; ensure shutdown() still safely handles partially initialized state
and that initialized_ is only true after all resources are successfully created.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7571fd9a-e784-41bd-9aff-f05f69bb4958
📒 Files selected for processing (122)
CMakeLists.txtsrc/audio/backend/audio_backend.hsrc/audio/backend/audio_backend_jack.cppsrc/audio/backend/audio_backend_oboe.cppsrc/audio/backend/audio_backend_portaudio_lifecycle.cppsrc/audio/backend/audio_backend_sdl.cppsrc/audio/backend/audio_backend_state.hsrc/audio/backend/audio_device_info.hsrc/audio/backend/i_audio_backend.hsrc/audio/effects/amp_cab/amp_simulator.cppsrc/audio/effects/amp_cab/amp_simulator.hsrc/audio/effects/amp_cab/cabinet_sim.cppsrc/audio/effects/amp_cab/cabinet_sim.hsrc/audio/effects/core/effect.hsrc/audio/effects/core/effect_factory.hsrc/audio/effects/core/effect_node.hsrc/audio/effects/core/effect_param.hsrc/audio/effects/core/i_metadata.hsrc/audio/effects/core/i_parameterizable.hsrc/audio/effects/core/i_processor.hsrc/audio/effects/core/i_serializable.hsrc/audio/effects/delay_reverb/delay.cppsrc/audio/effects/delay_reverb/delay.hsrc/audio/effects/delay_reverb/reverb.cppsrc/audio/effects/delay_reverb/reverb.hsrc/audio/effects/distortion/distortion.cppsrc/audio/effects/distortion/distortion.hsrc/audio/effects/distortion/overdrive.cppsrc/audio/effects/distortion/overdrive.hsrc/audio/effects/dynamics/compressor.cppsrc/audio/effects/dynamics/compressor.hsrc/audio/effects/dynamics/multiband_compressor.cppsrc/audio/effects/dynamics/multiband_compressor.hsrc/audio/effects/dynamics/noise_gate.cppsrc/audio/effects/dynamics/noise_gate.hsrc/audio/effects/eq_filter/equalizer.cppsrc/audio/effects/eq_filter/equalizer.hsrc/audio/effects/eq_filter/wah.cppsrc/audio/effects/eq_filter/wah.hsrc/audio/effects/modulation/chorus.cppsrc/audio/effects/modulation/chorus.hsrc/audio/effects/modulation/flanger.cppsrc/audio/effects/modulation/flanger.hsrc/audio/effects/modulation/phaser.cppsrc/audio/effects/modulation/phaser.hsrc/audio/effects/pitch/octaver.cppsrc/audio/effects/pitch/octaver.hsrc/audio/effects/pitch/pitch_shifter.cppsrc/audio/effects/pitch/pitch_shifter.hsrc/audio/effects/utility/looper.cppsrc/audio/effects/utility/looper.hsrc/audio/effects/utility/tuner.cppsrc/audio/effects/utility/tuner.hsrc/audio/engine/audio_engine.cppsrc/audio/engine/audio_engine.hsrc/audio/engine/audio_engine_api.cppsrc/audio/engine/audio_graph.hsrc/audio/engine/audio_graph_executor.hsrc/audio/engine/audio_metrics_service.cppsrc/audio/engine/audio_metrics_service.hsrc/audio/engine/dsp_node.hsrc/audio/engine/graph_link.hsrc/audio/engine/metronome.cppsrc/audio/engine/metronome.hsrc/gui/commands/command.hsrc/gui/commands/command_chain.hsrc/gui/commands/command_clear.hsrc/gui/commands/command_param.hsrc/gui/commands/command_preset.hsrc/gui/commands/command_reset.hsrc/gui/components/screen.cppsrc/gui/gui_manager.cppsrc/gui/gui_manager.hsrc/gui/gui_manager_frame.cppsrc/gui/gui_manager_menu.cppsrc/gui/gui_manager_update.cppsrc/gui/pedalboard/pedal_board.cppsrc/gui/pedalboard/pedal_board_menu.cppsrc/gui/pedalboard/pedal_widget.cppsrc/gui/pedalboard/pedal_widget.hsrc/gui/pedalboard/pedal_widget_body.cppsrc/gui/state/snapshot_manager.hsrc/gui/update_checker.cppsrc/gui/update_checker.hsrc/gui/views/gui_presets.cppsrc/gui/views/gui_tuner.hsrc/gui/window_context.cppsrc/gui/window_context.hsrc/main.cppsrc/presets/preset_manager_io.cpptests/e2e/test_e2e_preset_workflow.cpptests/integration/test_audio_engine.cpptests/integration/test_audio_graph.cpptests/integration/test_headless.cpptests/integration/test_json_serialization.cpptests/integration/test_midi_manager.cpptests/integration/test_preset_manager.cpptests/integration/test_snapshot_manager.cpptests/integration/test_unsaved_preset.cpptests/test_mocks.htests/ui/test_gui_commands.cpptests/ui/test_gui_manager.cpptests/ui/test_gui_modules.cpptests/ui/test_gui_presets.cpptests/ui/test_gui_snapshots.cpptests/ui/test_pedal_board.cpptests/ui/test_pedal_board_chain.cpptests/ui/test_pedal_board_menu.cpptests/ui/test_pedal_widget_knobs.cpptests/ui/test_screen.cpptests/unit/test_cabinet_sim_ir.cpptests/unit/test_command_history.cpptests/unit/test_effects_dynamics.cpptests/unit/test_effects_filter.cpptests/unit/test_effects_modulation.cpptests/unit/test_effects_pitch.cpptests/unit/test_effects_saturation.cpptests/unit/test_effects_spatial.cpptests/unit/test_effects_utility.cpptests/unit/test_looper.cpptests/unit/test_mock_backend.cpptests/unit/test_pitch_shifter.cpp
💤 Files with no reviewable changes (1)
- src/gui/gui_manager_update.cpp
✅ Files skipped from review due to trivial changes (40)
- src/gui/pedalboard/pedal_widget.h
- src/gui/views/gui_tuner.h
- src/gui/commands/command_clear.h
- tests/ui/test_gui_snapshots.cpp
- tests/unit/test_effects_dynamics.cpp
- src/audio/effects/pitch/pitch_shifter.cpp
- src/audio/effects/delay_reverb/reverb.cpp
- src/main.cpp
- tests/unit/test_pitch_shifter.cpp
- src/gui/pedalboard/pedal_widget_body.cpp
- src/gui/commands/command_param.h
- tests/integration/test_unsaved_preset.cpp
- tests/unit/test_effects_pitch.cpp
- tests/unit/test_effects_modulation.cpp
- tests/integration/test_json_serialization.cpp
- tests/ui/test_screen.cpp
- src/gui/commands/command_chain.h
- src/gui/state/snapshot_manager.h
- src/gui/pedalboard/pedal_widget.cpp
- tests/unit/test_effects_saturation.cpp
- src/audio/effects/pitch/octaver.cpp
- tests/ui/test_pedal_board.cpp
- tests/ui/test_pedal_widget_knobs.cpp
- tests/ui/test_gui_modules.cpp
- tests/integration/test_audio_graph.cpp
- src/audio/effects/modulation/flanger.cpp
- src/presets/preset_manager_io.cpp
- src/audio/effects/modulation/chorus.cpp
- tests/unit/test_looper.cpp
- src/audio/effects/utility/looper.cpp
- tests/integration/test_audio_engine.cpp
- src/gui/components/screen.cpp
- tests/unit/test_command_history.cpp
- src/audio/effects/dynamics/compressor.cpp
- src/gui/commands/command_preset.h
- tests/ui/test_pedal_board_menu.cpp
- src/gui/pedalboard/pedal_board.cpp
- tests/e2e/test_e2e_preset_workflow.cpp
- tests/ui/test_gui_presets.cpp
- src/audio/effects/dynamics/multiband_compressor.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/audio/engine/audio_engine.cpp
- tests/unit/test_mock_backend.cpp
- src/audio/engine/metronome.cpp
…ovide interfaces for MidiManager, PresetManager, and AudioEngine.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/audio/engine/audio_engine_process.cpp (1)
179-218:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep
AudioCommand::effect_indexin one identifier space.Line 180 now resolves
cmd.effect_indexas a graphnode_id, but the public API still describes a chain index and the current headless caller insrc/main.cppLine 441 and Line 449 passes the printed pedal slot, not a node id. Once graph edits makenode.iddiverge from chain order, enable/bypass/mix/param commands will hit the wrong pedal or no pedal at all.Pick one identifier and migrate the queue payload and all producers/consumers together before merging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/engine/audio_engine_process.cpp` around lines 179 - 218, AudioCommand::effect_index is being interpreted as a node_id in get_effect_by_id but callers (e.g., the main thread producers) still pass a chain/slot index; pick one canonical identifier and migrate producers/consumers consistently. Either (A) treat effect_index as a chain/slot index everywhere: change get_effect_by_id to look up the node by chain order (use main_graph_.get_nodes() ordering or an explicit chain->node_id mapping) and update any executor/path lookups to map chain index to the correct node/pedal; or (B) make effect_index a node_id everywhere: update the AudioCommand definition and all producers (the callers that push SetEffectParam/SetEffectEnabled/SetEffectMix, e.g., the main thread code that prints/passes pedal slot) to send node ids instead of chain slots, and update any consumer logic that creates/reads these commands. Ensure dummy_effects_ fallback behavior matches the chosen space and update get_effect_by_id, the AudioCommand struct, and all call sites together so the identifier semantics remain consistent.
🧹 Nitpick comments (1)
src/gui/gui_manager.cpp (1)
38-51: ⚡ Quick winConstruct
PedalBoardin one place only.
pedal_board_is created in the constructor and then replaced again ininitialize(). That doubles the setup path and silently discards any state or side effects from the first instance.♻️ Simplify the ownership path
GuiManager::GuiManager(AmplitronSession& session) : session_(session), engine_(session.concrete_engine()), command_history_(session.command_history()), midi_manager_(session.concrete_midi()), snapshot_manager_(session.snapshot_manager()), tuner_pedal_(std::make_shared<TunerPedal>()), gui_presets_(engine_, command_history_), gui_midi_(midi_manager_) -{ - pedal_board_ = std::make_unique<PedalBoard>(engine_, command_history_, &gui_midi_); - gui_presets_.set_pedal_board(pedal_board_.get()); - gui_presets_.set_midi_manager(&midi_manager_); -} +{} bool GuiManager::initialize(int width, int height) { if (!window_context_.initialize(width, height, Theme::WINDOW_TITLE)) { return false; } pedal_board_ = std::make_unique<PedalBoard>(engine_, command_history_, &gui_midi_); gui_presets_.set_pedal_board(pedal_board_.get()); gui_presets_.set_midi_manager(&midi_manager_);Also applies to: 62-64
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gui/gui_manager.cpp` around lines 38 - 51, The constructor currently creates pedal_board_ (GuiManager::GuiManager) and then initialize() recreates it, losing state; pick a single construction site and remove the duplicate. Either remove the PedalBoard allocation from GuiManager::GuiManager and defer creating pedal_board_ to initialize(), or keep the constructor creation and delete the reallocation in initialize(); update uses of pedal_board_, gui_presets_.set_pedal_board and gui_midi_ accordingly so they point to the single instance (also remove the duplicate creation at the other occurrence referenced around lines 62-64).
🤖 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 `@src/audio/engine/audio_graph_executor.h`:
- Around line 20-33: The header declares inline process() methods (e.g., the
process(const float* input, float* output, int num_samples) override in the
class containing pedal_ and in PassthroughProcessor) that call std::memcpy but
does not include <cstring>; add a direct include of <cstring> at the top of
audio_graph_executor.h so these inline implementations have the required
declaration rather than relying on transitive includes.
In `@src/presets/i_preset_manager.h`:
- Around line 9-11: IPresetManager is leaking the concrete MidiManager type via
the load_preset signature; change the contract to accept the abstraction
(IMidiManager) instead of MidiManager by updating IPresetManager::load_preset to
take IMidiManager& (or IMidiManager*) and not MidiManager*, then update
PresetManagerService and all underlying implementations and callers to consume
IMidiManager (adjust any forward declarations from MidiManager to IMidiManager
and ensure PresetManagerService, PresetData and MidiMapping users call the new
signature). Ensure all implementations compile against the interface symbol
IMidiManager and remove direct references to MidiManager in the preset
interface.
---
Outside diff comments:
In `@src/audio/engine/audio_engine_process.cpp`:
- Around line 179-218: AudioCommand::effect_index is being interpreted as a
node_id in get_effect_by_id but callers (e.g., the main thread producers) still
pass a chain/slot index; pick one canonical identifier and migrate
producers/consumers consistently. Either (A) treat effect_index as a chain/slot
index everywhere: change get_effect_by_id to look up the node by chain order
(use main_graph_.get_nodes() ordering or an explicit chain->node_id mapping) and
update any executor/path lookups to map chain index to the correct node/pedal;
or (B) make effect_index a node_id everywhere: update the AudioCommand
definition and all producers (the callers that push
SetEffectParam/SetEffectEnabled/SetEffectMix, e.g., the main thread code that
prints/passes pedal slot) to send node ids instead of chain slots, and update
any consumer logic that creates/reads these commands. Ensure dummy_effects_
fallback behavior matches the chosen space and update get_effect_by_id, the
AudioCommand struct, and all call sites together so the identifier semantics
remain consistent.
---
Nitpick comments:
In `@src/gui/gui_manager.cpp`:
- Around line 38-51: The constructor currently creates pedal_board_
(GuiManager::GuiManager) and then initialize() recreates it, losing state; pick
a single construction site and remove the duplicate. Either remove the
PedalBoard allocation from GuiManager::GuiManager and defer creating
pedal_board_ to initialize(), or keep the constructor creation and delete the
reallocation in initialize(); update uses of pedal_board_,
gui_presets_.set_pedal_board and gui_midi_ accordingly so they point to the
single instance (also remove the duplicate creation at the other occurrence
referenced around lines 62-64).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 580e11ba-9378-4587-95a0-9ca700953f5e
📒 Files selected for processing (15)
src/amplitron_session.hsrc/audio/engine/audio_engine.hsrc/audio/engine/audio_engine_process.cppsrc/audio/engine/audio_graph_executor.cppsrc/audio/engine/audio_graph_executor.hsrc/audio/engine/i_audio_engine.hsrc/gui/gui_manager.cppsrc/gui/gui_manager.hsrc/gui/pedalboard/pedal_board.cppsrc/main.cppsrc/midi/i_midi_manager.hsrc/midi/midi_manager.hsrc/presets/i_preset_manager.hsrc/presets/preset_manager.htests/ui/test_gui_manager.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/ui/test_gui_manager.cpp
…nd update preset management interfaces
…y path specification on iOS
…atcher source files to build target
…dio mocks and guarding fallback calls under WITH_JACK
…on-tests and reverting mock stubs
…improve engine test stability
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/audio/engine/audio_engine_process.cpp (1)
15-18:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMemory allocation in audio callback violates real-time constraints.
resize()allocates memory whenframe_countexceeds the current buffer size. This can cause priority inversion, page faults, or unbounded latency in the real-time audio callback, leading to audible glitches.Pre-allocate these buffers to the maximum expected frame count during engine initialization (e.g., in
start()orinitialize()).🔧 Suggested approach
Replace dynamic resize with assertion that buffers are pre-allocated:
- if (frame_count > static_cast<int>(process_buffer_.size())) { - process_buffer_.resize(frame_count, 0.0f); - process_buffer_right_.resize(frame_count, 0.0f); - } + // Buffers must be pre-allocated in initialize() or start() + assert(frame_count <= static_cast<int>(process_buffer_.size()) && + "process_buffer_ not pre-allocated to sufficient size");And in the initialization code, allocate to max buffer size (e.g., 512 or configurable max).
As per coding guidelines: "Never allocate memory, perform disk/console I/O, or block on standard mutexes inside the audio callback loop"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/engine/audio_engine_process.cpp` around lines 15 - 18, The audio callback currently calls process_buffer_.resize(...) and process_buffer_right_.resize(...), which allocates memory inside the real-time callback; remove those resize calls and replace them with a runtime check/assert that the buffers were pre-allocated to at least frame_count (e.g., using an assertion or early return in the callback). Pre-allocate process_buffer_ and process_buffer_right_ to the configurable maximum frame count during engine initialization (in start() or initialize() where AudioEngineProcess is set up), ensuring the buffers are sized once and documented; keep the callback limited to bounds-checked access to avoid any allocation or blocking.
🧹 Nitpick comments (11)
src/gui/views/gui_presets.cpp (2)
287-293: ⚡ Quick win
delete_preset_by_indexdeletes viastd::removeinstead ofpresets_.Deletion bypasses the injected storage abstraction, so a non-filesystem/alternate
IPresetManager(e.g. the Emscripten path that writes viaFS/blobs) won't delete consistently. Consider adding adelete_preset(path)to the interface and routing throughpresets_.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gui/views/gui_presets.cpp` around lines 287 - 293, The deletion in delete_preset_by_index currently calls std::remove on preset_files_[index], bypassing the injected storage abstraction; change this to call into the preset manager (presets_) instead. Add a delete_preset(const std::string& path) method to the IPresetManager interface and implement it in concrete managers, then replace the std::remove(path.c_str()) invocation in delete_preset_by_index with presets_->delete_preset(path), preserve setting preset_status_msg_ (using preset_name_from_path(path)) and calling refresh_presets(false) on success. Ensure error handling mirrors the current behavior when deletion fails.
177-211: ⚡ Quick winDecouple GuiPresets error/dir reporting from PresetManager statics
IPresetManagerdoesn’t expose an error accessor (e.g.,last_error()) or a presets-directory accessor, butsrc/gui/views/gui_presets.cppstill builds error strings viaPresetManager::last_error()and constructs preset paths / UI messaging viaPresetManager::get_presets_dir(), bypassing the injectedpresets_abstraction. Add the needed accessors toIPresetManager(or route them through it) so alternateIPresetManagerimplementations can’t produce stale/empty messages; today it’s low-risk becausePresetManageris the only in-treeIPresetManagerimplementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gui/views/gui_presets.cpp` around lines 177 - 211, The GUI currently calls PresetManager::last_error() and PresetManager::get_presets_dir() directly instead of using the injected presets_ IPresetManager instance; update the interface and usage so errors and directory info are retrieved via the abstraction: add virtual accessors to IPresetManager (e.g., virtual std::string last_error() const and virtual std::string get_presets_dir() const), implement them in PresetManager, then replace direct calls to PresetManager::last_error() and any direct preset-dir constructions in src/gui/views/gui_presets.cpp with calls to presets_.last_error() and presets_.get_presets_dir() (or equivalent methods on the presets_ object) and ensure any UI messages and path selections use those methods and not the static PresetManager APIs.src/audio/backend/audio_backend_registry.h (1)
37-43: 💤 Low valueConsider reserving capacity in
available().The method allocates and grows the vector dynamically. Pre-reserving avoids reallocations.
♻️ Suggested optimization
std::vector<std::string> available() const { std::vector<std::string> keys; + keys.reserve(creators_.size()); for (const auto& pair : creators_) { keys.push_back(pair.first); } return keys; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/backend/audio_backend_registry.h` around lines 37 - 43, The available() method grows keys incrementally causing reallocations; pre-reserve capacity to creators_.size() before the loop to avoid reallocations. In the function available(), call keys.reserve(creators_.size()) (or construct keys with reserved capacity) and then push_back pair.first as currently done; this keeps behavior identical but eliminates repeated allocations when iterating creators_.tests/integration/test_audio_backend_portaudio.cpp (1)
1-6: 💤 Low valueWrap
portaudio_mock.cppinclude under theWITH_JACKconditional
tests/integration/test_audio_backend_portaudio.cppincludes../fixtures/portaudio_mock.cppunconditionally (before the#if !defined(WITH_JACK)), so the mock will be compiled even when the PortAudio tests are skipped in JACK builds.rgshowsportaudio_mock.cppis only referenced by this PortAudio test file (andjack_mock.cpponly by the JACK test), so cross-test mock symbol conflicts are unlikely—but move the mock include inside the same#if !defined(WITH_JACK)unless there’s a specific in-file compile-order requirement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/test_audio_backend_portaudio.cpp` around lines 1 - 6, The include for portaudio_mock.cpp is unconditional and should be moved inside the PortAudio-only guard; wrap or relocate the line including "portaudio_mock.cpp" so it is inside the existing `#if` !defined(WITH_JACK) / `#endif` block in test_audio_backend_portaudio.cpp to prevent compiling the PortAudio mock in JACK builds and avoid cross-test symbol leakage.src/presets/preset_manager.h (1)
89-98: 💤 Low valueRemove redundant default arguments from virtual overrides in
preset_manager.h.
src/presets/i_preset_manager.halready defines the defaults forsave_preset(midi_mappings = {}) andload_preset(midi_manager = nullptr).src/presets/preset_manager.hrepeats the same defaults on theoverridesignatures (lines 89-98), so keep the defaults only in the interface and omit them on the override declarations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/presets/preset_manager.h` around lines 89 - 98, Remove the redundant default parameter values from the override declarations in preset_manager.h: update the save_preset override to remove "= {}" from the midi_mappings parameter and update the load_preset override to remove "= nullptr" from the midi_manager parameter so their declarations match the interface (i_preset_manager.h); the function signatures to edit are save_preset(const std::string& filepath, const std::string& preset_name, const std::string& description, IAudioEngine& engine, const std::vector<MidiMapping>& midi_mappings) override and load_preset(const std::string& filepath, IAudioEngine& engine, IMidiManager* midi_manager) override.src/audio/backend/jack_backend.cpp (1)
40-44: 💤 Low value
statusis captured but never inspected.When
jack_client_openfails,statuscarries the failure reason but is discarded; only the generic "is jackd running?" message is printed. Including the status bits would make diagnostics actionable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/backend/jack_backend.cpp` around lines 40 - 44, The failure path after calling jack_client_open currently ignores the jack_status_t bits in status; update the client_ null branch to inspect and format status (the jack_status_t variable named status returned by jack_client_open) and include those status flags/details in the std::cerr message so diagnostics are actionable (e.g., mention which JACK status bits are set alongside the existing "could not open JACK server" text); ensure you reference the existing variables/function names (status, jack_client_open, client_) and produce a clear combined error string rather than discarding status.src/audio/backend/oboe_backend.cpp (1)
88-92: 💤 Low valueConsider making ring buffer position indices atomic for clarity and portability.
capture_read_pos_andcapture_write_pos_are plainintbut accessed from different Oboe callbacks (potentially different threads). While the current design relies oncapture_filled_atomic operations for synchronization and each position is only modified by one thread, making thesestd::atomic<int>would provide clearer guarantees and better portability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/backend/oboe_backend.cpp` around lines 88 - 92, Make capture_read_pos_ and capture_write_pos_ into std::atomic<int> (similar to capture_filled_) to clarify cross-thread access: change their declarations from int to std::atomic<int>, update every use sites in oboe_backend.cpp (increments, assignments and reads) to use atomic operations (e.g., .fetch_add(), .store(), .load()) or explicit ordering as appropriate, and ensure the modulo/wrap logic still uses the atomic value (load, compute new index, store or fetch_add with modulo handling). Keep capture_filled_ logic as-is for synchronization but rely on the new atomics for clear, portable position access.src/audio/backend/sdl_backend.h (1)
30-31: 💤 Low valueHardcoded device indices may cause confusion.
get_input_device()andget_output_device()always return0, but the class doesn't track which device index was selected (unlikePortAudioBackend). If device selection is ever implemented for SDL, these would need updating. Consider adding member variables to track selection or documenting that SDL backend doesn't support device switching.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/backend/sdl_backend.h` around lines 30 - 31, The SDL backend's get_input_device() and get_output_device() currently return hardcoded 0; update the SDL backend (sdl_backend.h / the SDL backend class) to either add and use member variables like selected_input_device and selected_output_device to store the chosen device index (initialize them appropriately and return them from get_input_device()/get_output_device()), or explicitly document in the class comment and API that SDL does not support device selection so returning 0 is intentional; reference the get_input_device, get_output_device methods and contrast with PortAudioBackend behavior when making the change.src/audio/backend/portaudio_backend.cpp (1)
314-323: 💤 Low valueEmpty conditional block is dead code.
The
if (si)block at lines 317-320 contains only a comment and no executable code. Either implement buffer size retrieval or remove the empty block.♻️ Remove empty block
int PortAudioBackend::get_buffer_size() const { - if (stream_) { - const PaStreamInfo* si = Pa_GetStreamInfo(stream_); - if (si) { - // PortAudio doesn't explicitly expose the negotiated buffer size - // in PaStreamInfo, so we return the engine's target buffer size. - } - } + // PortAudio doesn't expose the negotiated buffer size in PaStreamInfo, + // so we return the engine's target buffer size. return engine_ ? engine_->get_buffer_size() : 512; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/backend/portaudio_backend.cpp` around lines 314 - 323, The empty if (si) block inside PortAudioBackend::get_buffer_size() is dead code; remove the entire conditional (the if (si) { /* comment */ } block) so the function simply checks stream_ if needed and then returns engine_ ? engine_->get_buffer_size() : 512; ensure no other logic depends on Pa_GetStreamInfo(stream_) and leave Pa_GetStreamInfo call out or used only if you later implement actual negotiated-size retrieval.src/midi/midi_manager_mapping.cpp (1)
226-231: 💤 Low valueConsider extracting duplicated node_id lookup logic.
The same graph traversal pattern appears twice. A small helper would reduce duplication and make future changes easier.
♻️ Example helper
// In MidiManager or a utility header int find_node_id_for_effect(const IAudioEngine& engine, const std::shared_ptr<Effect>& effect, int fallback) { for (const auto& node : engine.graph().get_nodes()) { if (node.pedal == effect) return node.id; } return fallback; }Also applies to: 274-279
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/midi/midi_manager_mapping.cpp` around lines 226 - 231, Extract the duplicated node lookup into a helper (e.g., find_node_id_for_effect) that takes the engine (or IAudioEngine&), the effect shared_ptr (effects[i]) and a fallback id, iterates engine.graph().get_nodes() comparing node.pedal to the effect and returns node.id or the fallback; then replace both inline loops (the block using node_id and the similar block at lines ~274-279) with calls to that helper and use its result when calling engine.push_effect_enabled. Ensure the helper name and signature are referenced where you replace the original code paths to keep behavior identical.tests/integration/test_audio_backend_jack.cpp (1)
137-140: 💤 Low valueEmpty test body - consider removing or documenting intent.
AudioBackend_Jack_NullStateDestroynow only assertstruewithout testing anything. If the original purpose (testingdestroy_audio_backend(nullptr)) is no longer relevant with the new ownership model, consider removing this test or adding a comment explaining why it's retained.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/test_audio_backend_jack.cpp` around lines 137 - 140, The test AudioBackend_Jack_NullStateDestroy is empty and should be removed or given an explicit intent: either delete the TEST(AudioBackend_Jack_NullStateDestroy) block if null-destroy behavior is no longer relevant, or replace the body with an explicit call to destroy_audio_backend(nullptr) (or the current equivalent) and a clear assertion/comment (e.g., call without crash or EXPECT_NO_THROW) so the intent is documented; reference the test name AudioBackend_Jack_NullStateDestroy and the function destroy_audio_backend(nullptr) when making the change.
🤖 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 `@src/amplitron_session.h`:
- Around line 32-33: The concrete_engine() and concrete_midi() accessors use
unsafe static_casts to downcast unique_ptr-held interfaces (engine_, midi_) to
concrete types (AudioEngine, MidiManager); replace these static_casts with
dynamic_casts and add null checks (e.g., if dynamic_cast returns nullptr,
assert/throw or return an error) so injected mocks or alternative
implementations won't cause UB, or alternatively remove these accessors and
either store explicit concrete pointers alongside the interface pointers or
expand the interface to expose needed functionality; update concrete_engine()
and concrete_midi() to use dynamic_cast<...&> with a safe failure path or
refactor callers to use the interface instead.
In `@src/audio/backend/portaudio_backend.cpp`:
- Around line 192-209: The retry of Pa_OpenStream after a failure reuses the
same input_params, output_params, sample_rate and buffer_size (and the same
pa_audio_callback/engine_) so it will likely fail for the same reason; update
the retry logic in the PortAudio open path to adjust parameters before retrying:
on detecting an initial Pa_OpenStream error, clear/modify WASAPI-exclusive flags
and any custom latency settings in input_params/deviceInfo/output_params (e.g.
remove exclusive-mode flags and reset suggestedLatency to
Pa_GetDeviceInfo(...)->defaultLowInputLatency/defaultLowOutputLatency or 0),
then call Pa_OpenStream again with the adjusted
input_params/output_params/sample_rate/buffer_size and the same
pa_audio_callback and engine_ so the retry has a different configuration that
can succeed.
In `@src/audio/backend/sdl_backend.cpp`:
- Around line 19-22: The audio callback resizes the capture buffer (auto& cap =
be->get_capture_buffer(); cap.resize(...)) which may allocate on the audio
thread; move allocation out of the callback by pre-allocating the capture buffer
to the maximum needed size in the backend start() path (e.g., reserve/resize to
max_frame_count or 2*x buffer_size) and replace the runtime resize in the
callback with an assertion or a bound check that frame_count <= cap.size();
update get_capture_buffer()/start() initialization to ensure capacity and remove
any cap.resize() calls from the callback.
In `@src/audio/engine/audio_command_dispatcher.cpp`:
- Around line 97-99: The current fallback returns dummy_effects[node_id] using
node_id as a vector index (in the code block around dummy_effects and node_id),
which is unsafe because node IDs are not guaranteed to be zero-based or
contiguous; replace this fallback with a proper lookup or explicit guard: either
(A) remove the index-based fallback and return nullptr/error when the node_id is
not found, or (B) maintain a map/unordered_map from node_id to effect and
consult that (e.g., lookup by id before indexing dummy_effects), and in either
case add a clear log message if a node_id lookup fails; if the old index-based
behavior is intentionally required, add a comment on node_id semantics and keep
the bounds check but still log the fallback usage.
In `@src/midi/midi_manager_mapping.cpp`:
- Line 233: Remove the stray debug printf by deleting the printf("[DEBUG]
AmpSimulator BYPASS TOGGLED\n"); statement in midi_manager_mapping.cpp (or
replace it with the project's logging call if you need runtime visibility);
locate the printf by searching for the exact string "AmpSimulator BYPASS
TOGGLED" or the printf call in the function where amp bypass state changes and
either remove it or replace it with the appropriate logger (e.g.,
DEBUG/trace-level) used elsewhere in the codebase to ensure consistency.
In `@src/presets/preset_components.cpp`:
- Around line 33-48: The translation unit is missing the <filesystem> include
used by PresetStorage::list(): add the header so the function's use of
std::filesystem::exists, std::filesystem::is_directory,
std::filesystem::directory_iterator and std::filesystem::path is self-contained;
modify src/presets/preset_components.cpp to include <filesystem> (ensure it's
added alongside existing <fstream>) so the lambda append_files and
PresetStorage::list() compile without relying on transitive includes.
---
Outside diff comments:
In `@src/audio/engine/audio_engine_process.cpp`:
- Around line 15-18: The audio callback currently calls
process_buffer_.resize(...) and process_buffer_right_.resize(...), which
allocates memory inside the real-time callback; remove those resize calls and
replace them with a runtime check/assert that the buffers were pre-allocated to
at least frame_count (e.g., using an assertion or early return in the callback).
Pre-allocate process_buffer_ and process_buffer_right_ to the configurable
maximum frame count during engine initialization (in start() or initialize()
where AudioEngineProcess is set up), ensuring the buffers are sized once and
documented; keep the callback limited to bounds-checked access to avoid any
allocation or blocking.
---
Nitpick comments:
In `@src/audio/backend/audio_backend_registry.h`:
- Around line 37-43: The available() method grows keys incrementally causing
reallocations; pre-reserve capacity to creators_.size() before the loop to avoid
reallocations. In the function available(), call keys.reserve(creators_.size())
(or construct keys with reserved capacity) and then push_back pair.first as
currently done; this keeps behavior identical but eliminates repeated
allocations when iterating creators_.
In `@src/audio/backend/jack_backend.cpp`:
- Around line 40-44: The failure path after calling jack_client_open currently
ignores the jack_status_t bits in status; update the client_ null branch to
inspect and format status (the jack_status_t variable named status returned by
jack_client_open) and include those status flags/details in the std::cerr
message so diagnostics are actionable (e.g., mention which JACK status bits are
set alongside the existing "could not open JACK server" text); ensure you
reference the existing variables/function names (status, jack_client_open,
client_) and produce a clear combined error string rather than discarding
status.
In `@src/audio/backend/oboe_backend.cpp`:
- Around line 88-92: Make capture_read_pos_ and capture_write_pos_ into
std::atomic<int> (similar to capture_filled_) to clarify cross-thread access:
change their declarations from int to std::atomic<int>, update every use sites
in oboe_backend.cpp (increments, assignments and reads) to use atomic operations
(e.g., .fetch_add(), .store(), .load()) or explicit ordering as appropriate, and
ensure the modulo/wrap logic still uses the atomic value (load, compute new
index, store or fetch_add with modulo handling). Keep capture_filled_ logic
as-is for synchronization but rely on the new atomics for clear, portable
position access.
In `@src/audio/backend/portaudio_backend.cpp`:
- Around line 314-323: The empty if (si) block inside
PortAudioBackend::get_buffer_size() is dead code; remove the entire conditional
(the if (si) { /* comment */ } block) so the function simply checks stream_ if
needed and then returns engine_ ? engine_->get_buffer_size() : 512; ensure no
other logic depends on Pa_GetStreamInfo(stream_) and leave Pa_GetStreamInfo call
out or used only if you later implement actual negotiated-size retrieval.
In `@src/audio/backend/sdl_backend.h`:
- Around line 30-31: The SDL backend's get_input_device() and
get_output_device() currently return hardcoded 0; update the SDL backend
(sdl_backend.h / the SDL backend class) to either add and use member variables
like selected_input_device and selected_output_device to store the chosen device
index (initialize them appropriately and return them from
get_input_device()/get_output_device()), or explicitly document in the class
comment and API that SDL does not support device selection so returning 0 is
intentional; reference the get_input_device, get_output_device methods and
contrast with PortAudioBackend behavior when making the change.
In `@src/gui/views/gui_presets.cpp`:
- Around line 287-293: The deletion in delete_preset_by_index currently calls
std::remove on preset_files_[index], bypassing the injected storage abstraction;
change this to call into the preset manager (presets_) instead. Add a
delete_preset(const std::string& path) method to the IPresetManager interface
and implement it in concrete managers, then replace the
std::remove(path.c_str()) invocation in delete_preset_by_index with
presets_->delete_preset(path), preserve setting preset_status_msg_ (using
preset_name_from_path(path)) and calling refresh_presets(false) on success.
Ensure error handling mirrors the current behavior when deletion fails.
- Around line 177-211: The GUI currently calls PresetManager::last_error() and
PresetManager::get_presets_dir() directly instead of using the injected presets_
IPresetManager instance; update the interface and usage so errors and directory
info are retrieved via the abstraction: add virtual accessors to IPresetManager
(e.g., virtual std::string last_error() const and virtual std::string
get_presets_dir() const), implement them in PresetManager, then replace direct
calls to PresetManager::last_error() and any direct preset-dir constructions in
src/gui/views/gui_presets.cpp with calls to presets_.last_error() and
presets_.get_presets_dir() (or equivalent methods on the presets_ object) and
ensure any UI messages and path selections use those methods and not the static
PresetManager APIs.
In `@src/midi/midi_manager_mapping.cpp`:
- Around line 226-231: Extract the duplicated node lookup into a helper (e.g.,
find_node_id_for_effect) that takes the engine (or IAudioEngine&), the effect
shared_ptr (effects[i]) and a fallback id, iterates engine.graph().get_nodes()
comparing node.pedal to the effect and returns node.id or the fallback; then
replace both inline loops (the block using node_id and the similar block at
lines ~274-279) with calls to that helper and use its result when calling
engine.push_effect_enabled. Ensure the helper name and signature are referenced
where you replace the original code paths to keep behavior identical.
In `@src/presets/preset_manager.h`:
- Around line 89-98: Remove the redundant default parameter values from the
override declarations in preset_manager.h: update the save_preset override to
remove "= {}" from the midi_mappings parameter and update the load_preset
override to remove "= nullptr" from the midi_manager parameter so their
declarations match the interface (i_preset_manager.h); the function signatures
to edit are save_preset(const std::string& filepath, const std::string&
preset_name, const std::string& description, IAudioEngine& engine, const
std::vector<MidiMapping>& midi_mappings) override and load_preset(const
std::string& filepath, IAudioEngine& engine, IMidiManager* midi_manager)
override.
In `@tests/integration/test_audio_backend_jack.cpp`:
- Around line 137-140: The test AudioBackend_Jack_NullStateDestroy is empty and
should be removed or given an explicit intent: either delete the
TEST(AudioBackend_Jack_NullStateDestroy) block if null-destroy behavior is no
longer relevant, or replace the body with an explicit call to
destroy_audio_backend(nullptr) (or the current equivalent) and a clear
assertion/comment (e.g., call without crash or EXPECT_NO_THROW) so the intent is
documented; reference the test name AudioBackend_Jack_NullStateDestroy and the
function destroy_audio_backend(nullptr) when making the change.
In `@tests/integration/test_audio_backend_portaudio.cpp`:
- Around line 1-6: The include for portaudio_mock.cpp is unconditional and
should be moved inside the PortAudio-only guard; wrap or relocate the line
including "portaudio_mock.cpp" so it is inside the existing `#if`
!defined(WITH_JACK) / `#endif` block in test_audio_backend_portaudio.cpp to
prevent compiling the PortAudio mock in JACK builds and avoid cross-test symbol
leakage.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: de5be140-6ce1-4939-9c05-68e43b7226c6
📒 Files selected for processing (96)
.github/workflows/ci.ymlCMakeLists.txtsrc/amplitron_session.hsrc/audio/backend/audio_backend.hsrc/audio/backend/audio_backend_factory.cppsrc/audio/backend/audio_backend_jack.cppsrc/audio/backend/audio_backend_oboe.cppsrc/audio/backend/audio_backend_portaudio.cppsrc/audio/backend/audio_backend_portaudio_devices.cppsrc/audio/backend/audio_backend_portaudio_helpers.hsrc/audio/backend/audio_backend_portaudio_internal.hsrc/audio/backend/audio_backend_portaudio_lifecycle.cppsrc/audio/backend/audio_backend_registry.hsrc/audio/backend/audio_backend_sdl.cppsrc/audio/backend/audio_device_info.hsrc/audio/backend/i_audio_backend.hsrc/audio/backend/jack_backend.cppsrc/audio/backend/jack_backend.hsrc/audio/backend/oboe_backend.cppsrc/audio/backend/oboe_backend.hsrc/audio/backend/portaudio_backend.cppsrc/audio/backend/portaudio_backend.hsrc/audio/backend/sdl_backend.cppsrc/audio/backend/sdl_backend.hsrc/audio/effects/core/effect.cppsrc/audio/effects/core/effect_factory.hsrc/audio/effects/core/effect_node.hsrc/audio/effects/core/effect_param.hsrc/audio/effects/utility/tuner.hsrc/audio/engine/analyzer_capture.cppsrc/audio/engine/analyzer_capture.hsrc/audio/engine/audio_command_dispatcher.cppsrc/audio/engine/audio_command_dispatcher.hsrc/audio/engine/audio_engine.cppsrc/audio/engine/audio_engine.hsrc/audio/engine/audio_engine_api.cppsrc/audio/engine/audio_engine_process.cppsrc/audio/engine/audio_graph_executor.cppsrc/audio/engine/audio_graph_executor.hsrc/audio/engine/audio_metrics_service.cppsrc/audio/engine/audio_metrics_service.hsrc/audio/engine/graph_link.hsrc/audio/engine/i_audio_engine.hsrc/audio/engine/i_metronome.hsrc/audio/engine/metronome.hsrc/audio/recorder/i_recorder.hsrc/audio/recorder/recorder.hsrc/audio/recorder/recorder_wav.cppsrc/gui/commands/command_chain.hsrc/gui/commands/command_clear.hsrc/gui/commands/command_graph.hsrc/gui/commands/command_param.hsrc/gui/commands/command_preset.hsrc/gui/commands/command_reorder.hsrc/gui/commands/command_reset.hsrc/gui/components/screen.hsrc/gui/gui_manager.cppsrc/gui/gui_manager.hsrc/gui/gui_manager_frame.cppsrc/gui/pedalboard/pedal_board.cppsrc/gui/pedalboard/pedal_board.hsrc/gui/pedalboard/pedal_widget.cppsrc/gui/pedalboard/pedal_widget.hsrc/gui/state/snapshot_manager.hsrc/gui/update_checker.hsrc/gui/views/gui_midi.cppsrc/gui/views/gui_midi.hsrc/gui/views/gui_presets.cppsrc/gui/views/gui_presets.hsrc/gui/window_context.cppsrc/main.cppsrc/midi/i_midi_manager.hsrc/midi/midi_manager.hsrc/midi/midi_manager_mapping.cppsrc/presets/i_preset_manager.hsrc/presets/i_preset_migrator.hsrc/presets/i_preset_serializer.hsrc/presets/i_preset_storage.hsrc/presets/preset_components.cppsrc/presets/preset_components.hsrc/presets/preset_manager.cppsrc/presets/preset_manager.hsrc/presets/preset_manager_io.cpptests/e2e/test_e2e_preset_workflow.cpptests/integration/test_audio_backend_jack.cpptests/integration/test_audio_backend_portaudio.cpptests/integration/test_json_serialization.cpptests/integration/test_preset_manager.cpptests/test_fixtures.htests/ui/test_gui_manager.cpptests/ui/test_gui_presets.cpptests/ui/test_pedal_board.cpptests/ui/test_pedal_board_chain.cpptests/ui/test_pedal_board_menu.cpptests/ui/test_pedal_widget_knobs.cpptests/unit/test_mock_backend.cpp
💤 Files with no reviewable changes (7)
- src/audio/backend/audio_backend_portaudio_internal.h
- src/audio/backend/audio_backend_jack.cpp
- src/audio/backend/audio_backend_portaudio.cpp
- src/audio/backend/audio_backend_oboe.cpp
- src/audio/backend/audio_backend_sdl.cpp
- src/audio/backend/audio_backend_portaudio_lifecycle.cpp
- src/audio/backend/audio_backend_portaudio_devices.cpp
✅ Files skipped from review due to trivial changes (4)
- src/audio/backend/audio_backend_portaudio_helpers.h
- tests/ui/test_pedal_board.cpp
- src/audio/engine/audio_command_dispatcher.h
- src/gui/update_checker.h
🚧 Files skipped from review as they are similar to previous changes (21)
- src/audio/engine/graph_link.h
- src/audio/backend/audio_device_info.h
- src/audio/engine/audio_metrics_service.h
- tests/ui/test_pedal_board_chain.cpp
- src/audio/effects/utility/tuner.h
- tests/ui/test_pedal_widget_knobs.cpp
- src/audio/backend/i_audio_backend.h
- src/audio/effects/core/effect_param.h
- src/audio/engine/audio_graph_executor.h
- src/gui/pedalboard/pedal_board.cpp
- src/audio/effects/core/effect_node.h
- src/audio/engine/audio_metrics_service.cpp
- src/audio/engine/metronome.h
- src/gui/gui_manager_frame.cpp
- src/main.cpp
- tests/unit/test_mock_backend.cpp
- src/audio/engine/audio_graph_executor.cpp
- src/gui/gui_manager.h
- src/gui/gui_manager.cpp
- src/gui/window_context.cpp
- tests/ui/test_gui_manager.cpp
…ate preset manager access patterns
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/amplitron_session.h (1)
22-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce non-null subsystem dependencies in the constructor.
engine(),midi(), andpresets()all dereference these pointers later, so an injectednullptrcreates a session that fails far from the actual bug site. Reject null dependencies here, or replace them with defaults before storing them.Suggested fix
AmplitronSession(std::unique_ptr<IAudioEngine> engine = std::make_unique<AudioEngine>(), std::unique_ptr<IMidiManager> midi = std::make_unique<MidiManager>(), std::unique_ptr<IPresetManager> presets = std::make_unique<PresetManagerService>()) : engine_(std::move(engine)), midi_(std::move(midi)), - presets_(std::move(presets)) {} + presets_(std::move(presets)) { + if (!engine_ || !midi_ || !presets_) { + throw std::invalid_argument("AmplitronSession requires non-null subsystems"); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/amplitron_session.h` around lines 22 - 27, The AmplitronSession constructor currently stores the passed unique_ptrs directly which allows callers to pass nullptr and causes later dereferences to crash; update the AmplitronSession(std::unique_ptr<IAudioEngine> engine, std::unique_ptr<IMidiManager> midi, std::unique_ptr<IPresetManager> presets) to validate inputs and enforce non-null: for each of engine, midi, presets either replace a null argument with a default instance (e.g., std::make_unique<AudioEngine>(), std::make_unique<MidiManager>(), std::make_unique<PresetManagerService>()) before assigning to engine_, midi_, presets_ or throw a clear exception (e.g., std::invalid_argument) when a nullptr is passed so that AmplitronSession always stores non-null engine_, midi_, and presets_.
🤖 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 `@src/presets/preset_manager_io.cpp`:
- Around line 495-500: The delete_preset implementation bypasses the injected
storage backend; add a remove (e.g., remove_preset or delete_preset) method to
the IPresetStorage interface and implement it in the filesystem storage, then
modify PresetManager::delete_preset to call the IPresetStorage removal method
instead of std::remove so delete, save_preset_data and load_preset all use the
same backend; preserve error handling by setting last_error_ when the storage
remove call fails and return its success boolean.
In `@src/presets/preset_manager.h`:
- Around line 107-109: The PresetManager implementation still uses the legacy
static last_error_ buffer causing cross-instance state leakage; change
last_error_ from a static/global to an instance member on the PresetManager
class and have get_last_error(), delete_preset(), and other methods (e.g.
get_presets_directory()) read/write that instance member instead of the static;
update the IPresetManager-backed constructor/usage to ensure each PresetManager
holds its own error string and remove any global/static accessors so errors are
scoped per service instance.
---
Outside diff comments:
In `@src/amplitron_session.h`:
- Around line 22-27: The AmplitronSession constructor currently stores the
passed unique_ptrs directly which allows callers to pass nullptr and causes
later dereferences to crash; update the
AmplitronSession(std::unique_ptr<IAudioEngine> engine,
std::unique_ptr<IMidiManager> midi, std::unique_ptr<IPresetManager> presets) to
validate inputs and enforce non-null: for each of engine, midi, presets either
replace a null argument with a default instance (e.g.,
std::make_unique<AudioEngine>(), std::make_unique<MidiManager>(),
std::make_unique<PresetManagerService>()) before assigning to engine_, midi_,
presets_ or throw a clear exception (e.g., std::invalid_argument) when a nullptr
is passed so that AmplitronSession always stores non-null engine_, midi_, and
presets_.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c36c8386-098d-4d6e-a49b-73c30bee2d8f
📒 Files selected for processing (19)
src/amplitron_session.hsrc/audio/backend/audio_backend_registry.hsrc/audio/backend/jack_backend.cppsrc/audio/backend/oboe_backend.cppsrc/audio/backend/portaudio_backend.cppsrc/audio/backend/sdl_backend.cppsrc/audio/backend/sdl_backend.hsrc/audio/engine/audio_command_dispatcher.cppsrc/audio/engine/audio_engine.cppsrc/audio/engine/audio_engine_process.cppsrc/gui/views/gui_presets.cppsrc/main.cppsrc/midi/midi_manager_mapping.cppsrc/presets/i_preset_manager.hsrc/presets/preset_components.cppsrc/presets/preset_manager.hsrc/presets/preset_manager_io.cpptests/integration/test_audio_backend_jack.cpptests/integration/test_audio_backend_portaudio.cpp
💤 Files with no reviewable changes (1)
- tests/integration/test_audio_backend_jack.cpp
✅ Files skipped from review due to trivial changes (1)
- tests/integration/test_audio_backend_portaudio.cpp
🚧 Files skipped from review as they are similar to previous changes (13)
- src/presets/i_preset_manager.h
- src/audio/backend/audio_backend_registry.h
- src/midi/midi_manager_mapping.cpp
- src/audio/backend/sdl_backend.h
- src/presets/preset_components.cpp
- src/audio/backend/jack_backend.cpp
- src/audio/engine/audio_command_dispatcher.cpp
- src/audio/engine/audio_engine_process.cpp
- src/gui/views/gui_presets.cpp
- src/audio/engine/audio_engine.cpp
- src/audio/backend/sdl_backend.cpp
- src/audio/backend/portaudio_backend.cpp
- src/audio/backend/oboe_backend.cpp
… build configurations to include mock files
…nsions on startup and resize events
…esign documentation
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/gui/pedalboard/pedal_board_chain.cpp (1)
131-132: ⚡ Quick winRename the grid locals to lowercase snake_case.
GRID_SZandGRID_COLORare function-scope variables, so they should follow the local-variable naming convention instead of the global-constant/macros convention.♻️ Suggested cleanup
- float GRID_SZ = 32.0f * ui_state.zoom; - ImU32 GRID_COLOR = IM_COL32(36, 34, 30, 255); - for (float x = std::fmod(ui_state.scrolling.x, GRID_SZ); x < canvas_size.x; - x += GRID_SZ) { + const float grid_sz = 32.0f * ui_state.zoom; + const ImU32 grid_color = IM_COL32(36, 34, 30, 255); + for (float x = std::fmod(ui_state.scrolling.x, grid_sz); x < canvas_size.x; + x += grid_sz) { draw_list->AddLine(ImVec2(canvas_pos.x + x, canvas_pos.y), - ImVec2(canvas_pos.x + x, canvas_end.y), GRID_COLOR); + ImVec2(canvas_pos.x + x, canvas_end.y), grid_color); } - for (float y = std::fmod(ui_state.scrolling.y, GRID_SZ); y < canvas_size.y; - y += GRID_SZ) { + for (float y = std::fmod(ui_state.scrolling.y, grid_sz); y < canvas_size.y; + y += grid_sz) { draw_list->AddLine(ImVec2(canvas_pos.x, canvas_pos.y + y), - ImVec2(canvas_end.x, canvas_pos.y + y), GRID_COLOR); + ImVec2(canvas_end.x, canvas_pos.y + y), grid_color); }As per coding guidelines, "Variables and Parameters should use lowercase snake_case" and "Global Constants and Macros should use Uppercase snake_case."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gui/pedalboard/pedal_board_chain.cpp` around lines 131 - 132, Rename the function-local variables GRID_SZ and GRID_COLOR to lowercase snake_case (e.g., grid_sz and grid_color) to follow local-variable naming conventions; update every use of GRID_SZ and GRID_COLOR within the same function in src/gui/pedalboard/pedal_board_chain.cpp to the new names so all references compile and maintain style consistency.
🤖 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.
Nitpick comments:
In `@src/gui/pedalboard/pedal_board_chain.cpp`:
- Around line 131-132: Rename the function-local variables GRID_SZ and
GRID_COLOR to lowercase snake_case (e.g., grid_sz and grid_color) to follow
local-variable naming conventions; update every use of GRID_SZ and GRID_COLOR
within the same function in src/gui/pedalboard/pedal_board_chain.cpp to the new
names so all references compile and maintain style consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5dd3379-9ff4-4660-937b-8189e04a359e
⛔ Files ignored due to path filters (3)
assets/uml_audio_dsp.svgis excluded by!**/*.svgassets/uml_core_coordination.svgis excluded by!**/*.svgassets/uml_gui_components.svgis excluded by!**/*.svg
📒 Files selected for processing (5)
README.mdsrc/gui/gui_manager.cppsrc/gui/gui_manager_frame.cppsrc/gui/pedalboard/pedal_board_chain.cppsrc/gui/window_context.cpp
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/gui/gui_manager_frame.cpp
- src/gui/window_context.cpp
sudip-mondal-2002
added
type:bug
12 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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Architecture