Feature/graph undo redo#243
Merged
sudip-mondal-2002 merged 17 commits intoMay 28, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements undo/redo support for audio graph edits (adding/removing nodes and cables, moving nodes) and introduces PortAudio test mocking infrastructure. The command base interface is updated to return bool from ChangesGraph Undo/Redo and PortAudio Test Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Code Coverage Report 📊Line Coverage: 81.1% ✅ Coverage meets threshold: 81.1% >= 60% Full Coverage Summary |
Contributor
PR Preview RemovedThe GitHub Pages preview for this PR has been removed because the PR was closed. |
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/audio/audio_backend_portaudio_lifecycle.cpp (1)
156-164:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard device-info dereferences in auto-detect logging path.
MOCK_OR_REAL_GET_DEVICE_INFO(...)can returnnullptr;in_info->name,out_info->name,in_info->defaultSampleRate, and the direct...->namedereference on Line 188 can crash.Suggested fix
- std::cout << "\n>> Auto-selected (same API: " << c.api_name << "):" << std::endl; - std::cout << " INPUT: " << in_info->name << " [USB Guitar Cable]" << std::endl; - std::cout << " OUTPUT: " << out_info->name << " [Speakers]" << std::endl; + std::cout << "\n>> Auto-selected (same API: " << c.api_name << "):" << std::endl; + std::cout << " INPUT: " << (in_info ? in_info->name : "Unknown") << " [USB Guitar Cable]" << std::endl; + std::cout << " OUTPUT: " << (out_info ? out_info->name : "Unknown") << " [Speakers]" << std::endl; - if (in_info->defaultSampleRate > 0) { + if (in_info && in_info->defaultSampleRate > 0) { sample_rate = static_cast<int>(in_info->defaultSampleRate); std::cout << " Rate: " << sample_rate << " Hz (device native)" << std::endl; } ... - std::cout << " OUTPUT: " << MOCK_OR_REAL_GET_DEVICE_INFO(output_device)->name << std::endl; + const PaDeviceInfo* selected_out = MOCK_OR_REAL_GET_DEVICE_INFO(output_device); + std::cout << " OUTPUT: " << (selected_out ? selected_out->name : "Unknown") << std::endl;Also applies to: 188-188
🤖 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/audio_backend_portaudio_lifecycle.cpp` around lines 156 - 164, The logging and sample-rate assignment dereference pointers returned by MOCK_OR_REAL_GET_DEVICE_INFO without nullptr checks; guard the in_info and out_info pointers before accessing their members (in_info->name, out_info->name, in_info->defaultSampleRate) in the auto-detect path: if either pointer is null, use a safe fallback string like "<unknown device>" for names and skip or keep existing sample_rate if defaultSampleRate is unavailable; update the prints and the sample_rate assignment around the code that references in_info, out_info, and sample_rate to perform these null checks (symbols: MOCK_OR_REAL_GET_DEVICE_INFO, in_info, out_info, sample_rate).
🧹 Nitpick comments (4)
tests/test_audio_backend_portaudio.cpp (2)
48-71: ⚡ Quick winAvoid pass-without-verification lifecycle tests.
These tests currently pass even if
start()/restart()fails, which can hide regressions. Make the expected lifecycle outcome explicit (or convert to deterministic mock-based assertions) so failures are observable.🤖 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/test_audio_backend_portaudio.cpp` around lines 48 - 71, The tests silently allow start()/restart() to fail because they only conditionally check results; change them to assert the lifecycle calls succeed and produce the expected state: replace the conditional in TEST(PortAudioLifecycle_StartStopStartCycle) with ASSERT_TRUE(engine.start()) before checking engine.is_running(), and after stop use ASSERT_TRUE(engine.start()) and ASSERT_TRUE(engine.is_running()); in TEST(PortAudioLifecycle_Restart) assert the restart outcome (e.g., ASSERT_TRUE(engine.restart()) or ASSERT_TRUE(engine.is_running()) immediately after engine.restart()), ensuring you still call initialize() and shutdown() and use AudioEngine::initialize, start, stop, restart, is_running, and shutdown to locate the code to update.
101-143: ⚡ Quick winAvoid raw PortAudio global init/terminate in this unit test.
Mixing
Pa_Initialize()/Pa_Terminate()with liveAudioEngineobjects can create test-order coupling on global backend state. Prefer engine-managed lifecycle (or scoped mocks) and ensure engine shutdown completes before any manual termination.🤖 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/test_audio_backend_portaudio.cpp` around lines 101 - 143, This test currently calls Pa_Initialize() and Pa_Terminate() directly which conflicts with AudioEngine-managed lifecycle and can cause test-order coupling; remove the raw PortAudio calls and let AudioEngine manage init/terminate (use engine_valid.initialize() / engine_valid.shutdown() or create/destroy scoped AudioEngine instances), ensure any engine_uninit or engine_valid objects are fully shutdown before ending the test, and replace direct global init/terminate with engine-managed setup/teardown around the uses of AudioEngine methods (initialize(), set_input_device(), set_output_device()) so global PortAudio state is not mutated outside the engine.tests/test_command_graph.cpp (1)
97-117: ⚡ Quick winClean up singleton graph state after this test.
GuiGraphStateis global; leavingnode_positions[999]populated can leak into other tests. Remove the injected node state at test end to keep tests isolated.🤖 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/test_command_graph.cpp` around lines 97 - 117, The test leaves a global entry in the GuiGraphState singleton which can affect other tests; after exercising MoveGraphNodeCommand via history.execute/undo/redo, remove the injected node state by erasing state.node_positions[node_id] (or reset the singleton state) so the test cleans up its inserted node (reference symbols: GuiGraphState::get_instance(), state.node_positions, node_id, MoveGraphNodeCommand, CommandHistory).tests/web/amplitron.spec.ts (1)
623-668: ⚡ Quick winReplace fixed sleeps with condition-based waits.
The repeated
waitForTimeout(100)calls can make this scenario flaky on slower CI. Prefer waiting until node/link counts reach expected intermediate values after each action/undo.🤖 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/web/amplitron.spec.ts` around lines 623 - 668, Replace fixed waits after calling Module functions and after undo keystrokes with condition-based waits that poll the DOM/Module state until counts match expected values: after each trigger_add_splitter_node call assert get_node_count increases (use page.waitForFunction(() => Module.ccall('get_node_count', 'number', [], []) === expected))), after each trigger_add_link call assert get_link_count increases, and during the undo loop wait for node/link counts to decrement to their expected intermediate values instead of waitForTimeout; reference the Module functions get_node_count, get_link_count, trigger_add_splitter_node, trigger_add_link and the undo keystroke (Control+Z) to determine expected counts.
🤖 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/audio_backend_portaudio_devices.cpp`:
- Around line 74-76: The mismatch check mixes real and mocked device info;
update the logic so devices_share_host_api uses the same mock-aware access as
the call sites (or replace its usage by comparing the already-fetched
in_api/out_api). Specifically, either modify devices_share_host_api to call
g_mock_pa_get_device_info when g_mock_pa_get_device_info is set (instead of
calling Pa_GetDeviceInfo directly), or stop calling devices_share_host_api and
instead compare hostApi using the in_api and out_api pointers already obtained
in audio_backend_portaudio_devices.cpp (using in_api->hostApi and
out_api->hostApi when out_api is non-null); reference
g_mock_pa_get_host_api_info, g_mock_pa_get_device_info, devices_share_host_api,
in_api, out_api, and Pa_GetDeviceInfo to find and update the code.
In `@src/audio/audio_graph.cpp`:
- Around line 333-337: restore_link currently pushes link into links_ and
updates next_id_ before calling rebuild_topology(), leaving the graph corrupted
if rebuild_topology() fails; change it to perform validation and rollback on
failure by either (a) running the same validation/addition logic used by
add_link (prefer calling add_link or its validator) before mutating internal
state, or (b) make a local copy of links_ and next_id_, apply the push/update to
the copies, call rebuild_topology() on the tentative state, and only commit
copies to links_ and next_id_ if rebuild_topology() succeeds—if
rebuild_topology() fails, restore the originals (or avoid mutating them) and
propagate/handle the error so invalid payloads do not remain in links_.
In `@src/gui/command_graph.h`:
- Around line 103-112: The execute() implementation must signal when it actually
mutated the graph so history can skip no-ops: change the command API so
execute() returns a bool (true when mutation occurred) and update this execute()
(use engine_.graph().add_link(...), set was_successful = (link.id != -1), call
engine_.commit_graph_changes() only when was_successful, then return
was_successful); then update CommandHistory::execute() to call the command's
bool execute() and only queue/clear redo when that call returns true (i.e., skip
recording commands that return false).
In `@src/gui/command_history.h`:
- Line 19: Update the class comment that documents the default command history
depth so it matches the new constant value DEFAULT_MAX_DEPTH (now 100); locate
the class comment above the declaration in command_history.h (referenced near
DEFAULT_MAX_DEPTH) and change the documented default from 50 to 100 or state it
references DEFAULT_MAX_DEPTH to avoid future drift.
In `@src/gui/pedal_board_menu.cpp`:
- Around line 107-111: The menu currently constructs AddGraphNodeCommand with a
fixed ImVec2(40, 60) which writes into GuiGraphState::node_positions and
bypasses the auto-placement logic in render_signal_chain, causing new
Splitter/Mixer nodes to overlap; modify the two MenuItem handlers to call
AddGraphNodeCommand without a fixed position (pass nullptr or use the overload
that omits the position) so the command defers to
GuiGraphState/render_signal_chain auto-placement, e.g. replace the ImVec2(40,
60) argument in the AddGraphNodeCommand calls for "Splitter" and "Mixer" so node
positions are computed instead of hard-coded.
In `@src/main.cpp`:
- Around line 146-152: The functions trigger_add_splitter_node (and the similar
function that adds links) currently read IDs before or by peeking at
graph().get_nodes().back()/get_links().back(), which can be invalid or stale;
instead, after creating the command (e.g. the Amplitron::AddGraphNodeCommand /
AddGraphLinkCommand), call g_gui->command_history().execute(std::move(cmd))
first, then read the ID produced by the command object (use cmd->node_id or
cmd->link_id after execution) and return that, and if execution indicates
failure or the produced ID is invalid return -1; ensure you reference the same
symbols (g_gui, command_history().execute,
Amplitron::AddGraphNodeCommand/AddGraphLinkCommand, node_id/link_id) when making
the change.
- Around line 190-195: The remove path passes ImVec2(0,0) into
RemoveGraphNodeCommand so undo restores the node at top-left and then
immediately erases the stored position; fix by retrieving the node's actual
current position (e.g. use node.position if available or lookup
Amplitron::GuiGraphState::get_instance().node_positions.at(node.id)) and pass
that ImVec2 into the std::make_unique<Amplitron::RemoveGraphNodeCommand>(...)
call, and stop erasing the entry from
GuiGraphState::get_instance().node_positions here so the undo can restore the
original position.
---
Outside diff comments:
In `@src/audio/audio_backend_portaudio_lifecycle.cpp`:
- Around line 156-164: The logging and sample-rate assignment dereference
pointers returned by MOCK_OR_REAL_GET_DEVICE_INFO without nullptr checks; guard
the in_info and out_info pointers before accessing their members (in_info->name,
out_info->name, in_info->defaultSampleRate) in the auto-detect path: if either
pointer is null, use a safe fallback string like "<unknown device>" for names
and skip or keep existing sample_rate if defaultSampleRate is unavailable;
update the prints and the sample_rate assignment around the code that references
in_info, out_info, and sample_rate to perform these null checks (symbols:
MOCK_OR_REAL_GET_DEVICE_INFO, in_info, out_info, sample_rate).
---
Nitpick comments:
In `@tests/test_audio_backend_portaudio.cpp`:
- Around line 48-71: The tests silently allow start()/restart() to fail because
they only conditionally check results; change them to assert the lifecycle calls
succeed and produce the expected state: replace the conditional in
TEST(PortAudioLifecycle_StartStopStartCycle) with ASSERT_TRUE(engine.start())
before checking engine.is_running(), and after stop use
ASSERT_TRUE(engine.start()) and ASSERT_TRUE(engine.is_running()); in
TEST(PortAudioLifecycle_Restart) assert the restart outcome (e.g.,
ASSERT_TRUE(engine.restart()) or ASSERT_TRUE(engine.is_running()) immediately
after engine.restart()), ensuring you still call initialize() and shutdown() and
use AudioEngine::initialize, start, stop, restart, is_running, and shutdown to
locate the code to update.
- Around line 101-143: This test currently calls Pa_Initialize() and
Pa_Terminate() directly which conflicts with AudioEngine-managed lifecycle and
can cause test-order coupling; remove the raw PortAudio calls and let
AudioEngine manage init/terminate (use engine_valid.initialize() /
engine_valid.shutdown() or create/destroy scoped AudioEngine instances), ensure
any engine_uninit or engine_valid objects are fully shutdown before ending the
test, and replace direct global init/terminate with engine-managed
setup/teardown around the uses of AudioEngine methods (initialize(),
set_input_device(), set_output_device()) so global PortAudio state is not
mutated outside the engine.
In `@tests/test_command_graph.cpp`:
- Around line 97-117: The test leaves a global entry in the GuiGraphState
singleton which can affect other tests; after exercising MoveGraphNodeCommand
via history.execute/undo/redo, remove the injected node state by erasing
state.node_positions[node_id] (or reset the singleton state) so the test cleans
up its inserted node (reference symbols: GuiGraphState::get_instance(),
state.node_positions, node_id, MoveGraphNodeCommand, CommandHistory).
In `@tests/web/amplitron.spec.ts`:
- Around line 623-668: Replace fixed waits after calling Module functions and
after undo keystrokes with condition-based waits that poll the DOM/Module state
until counts match expected values: after each trigger_add_splitter_node call
assert get_node_count increases (use page.waitForFunction(() =>
Module.ccall('get_node_count', 'number', [], []) === expected))), after each
trigger_add_link call assert get_link_count increases, and during the undo loop
wait for node/link counts to decrement to their expected intermediate values
instead of waitForTimeout; reference the Module functions get_node_count,
get_link_count, trigger_add_splitter_node, trigger_add_link and the undo
keystroke (Control+Z) to determine expected counts.
🪄 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: cfa4adfc-345a-4dba-9392-187899583c4f
📒 Files selected for processing (19)
.gitignoreCMakeLists.txtsrc/audio/audio_backend_portaudio_devices.cppsrc/audio/audio_backend_portaudio_internal.hsrc/audio/audio_backend_portaudio_lifecycle.cppsrc/audio/audio_engine.hsrc/audio/audio_graph.cppsrc/audio/audio_graph.hsrc/gui/command.hsrc/gui/command_graph.hsrc/gui/command_history.hsrc/gui/gui_graph_state.hsrc/gui/gui_manager.hsrc/gui/pedal_board_chain.cppsrc/gui/pedal_board_menu.cppsrc/main.cpptests/test_audio_backend_portaudio.cpptests/test_command_graph.cpptests/web/amplitron.spec.ts
sudip-mondal-2002
requested changes
May 26, 2026
Owner
|
Please fix the conflicts and follow the latest test folder structures |
github-actions
Bot
added
Audio Processing
MohitBareja16
added
type:testing
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 (2)
src/audio/engine/audio_graph.cpp (1)
323-333:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
restore_nodetransactional (rollback on failed restore).
restore_nodepermanently mutatesnodes_andnext_id_before validation. If restore input is stale/corrupt, this can leave invalid graph state resident.Suggested fix
void AudioGraph::restore_node(const DSPNode& node) { + int prev_next_id = next_id_; nodes_.push_back(node); if (node.id >= next_id_) next_id_ = node.id + 1; for (int pin : node.input_pin_ids) { if (pin >= next_id_) next_id_ = pin + 1; } for (int pin : node.output_pin_ids) { if (pin >= next_id_) next_id_ = pin + 1; } - rebuild_topology(); + if (!rebuild_topology()) { + nodes_.pop_back(); + next_id_ = prev_next_id; + rebuild_topology(); + } }🤖 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_graph.cpp` around lines 323 - 333, The restore_node function mutates nodes_ and next_id_ before validating the node and calling rebuild_topology, risking corrupt state on failure; change it to be transactional by applying the insert to local temporaries (e.g., a temp vector copy of nodes_ and a temp_next_id computed from node.id and node.input_pin_ids/output_pin_ids), run the validation/rebuild_topology logic against the temp state (or call a non-mutating validate_topology(temp_nodes) helper), and only on success swap the temporaries into nodes_ and next_id_; on failure discard temporaries and return/error so nodes_ and next_id_ remain unchanged.src/gui/pedalboard/pedal_board_chain.cpp (1)
357-357:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove debug printf statement.
This debug logging should be removed before merge.
- printf("MANUAL DROP HOVER DETECTED! src: %d, dest: %d\n", ui_state.active_src_pin_id, pin_id);🤖 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` at line 357, Remove the debug printf call that prints "MANUAL DROP HOVER DETECTED!"—locate the printf("MANUAL DROP HOVER DETECTED! src: %d, dest: %d\n", ui_state.active_src_pin_id, pin_id) in pedal_board_chain.cpp (near the manual drop/hover handling code) and delete that line; if you need to retain a trace, replace it with the project's logging facility or a conditional debug macro, otherwise simply remove the printf invocation.
♻️ Duplicate comments (1)
src/audio/engine/audio_graph.cpp (1)
335-343:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
restore_linkstill bypasses link validity checks.This still restores links without verifying both pins exist and the edge is not already present.
rebuild_topology()rollback only protects cycle cases, not orphan/duplicate payloads.🤖 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_graph.cpp` around lines 335 - 343, The restore_link function currently appends to links_ and updates next_id_ without checking link validity; before mutating links_ or next_id_ in restore_link(const GraphLink& link) first verify both endpoint pins exist (use your pin lookup/exists helpers) and that no existing link in links_ has the same source/target (or identical id) to avoid duplicates/orphans; only if those checks pass then push_back(link), adjust next_id_ as you already do, call rebuild_topology(), and keep the existing rollback logic if rebuild_topology() fails—otherwise return immediately without modifying links_ or next_id_.
🧹 Nitpick comments (4)
src/gui/commands/command_history.cpp (1)
46-56: 💤 Low valueConsider checking
execute()return value during redo for consistency.In
redo(), ifcmd->execute()returnsfalse(indicating no mutation occurred), the command is still pushed onto the undo stack. This creates a minor inconsistency:execute()at line 7 skips recording for no-ops, but redo doesn't apply the same logic.In practice this is unlikely to cause issues since redo replays a previously successful command, but for defensive consistency you could check the return value.
♻️ Optional: Check return value in redo()
bool CommandHistory::redo() { if (redo_stack_.empty()) return false; auto cmd = std::move(redo_stack_.back()); redo_stack_.pop_back(); - cmd->execute(); - undo_stack_.push_back(std::move(cmd)); + if (cmd->execute()) { + undo_stack_.push_back(std::move(cmd)); + } return true; }🤖 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/commands/command_history.cpp` around lines 46 - 56, CommandHistory::redo currently calls cmd->execute() and unconditionally moves the command from redo_stack_ to undo_stack_; change it to check the bool return of cmd->execute() and only push the command onto undo_stack_ when execute() returns true (mirror the behavior used elsewhere when execute() indicates a no-op). Locate the redo method, evaluate the result of cmd->execute(), and conditionally push_back(std::move(cmd)) into undo_stack_ only on success; ensure the method still returns true/false appropriately when execute() fails.src/gui/commands/command_graph.h (1)
14-20: ⚡ Quick winAlign struct member naming with repo convention.
Member fields in these command structs should use trailing underscores for consistency with project C++ style.
As per coding guidelines "Class Member Variables should use lowercase snake_case with a trailing underscore (e.g., current_port_, midi_queue_)".
Also applies to: 55-60, 99-101, 135-136, 155-157
🤖 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/commands/command_graph.h` around lines 14 - 20, Rename the struct member fields to follow the project's trailing-underscore convention: change node_id, name, type, pedal, position, and cached_node to node_id_, name_, type_, pedal_, position_, and cached_node_ in the command struct shown (symbols to update: node_id, name, type, pedal, position, cached_node and the already-correct engine_). Do the same renaming for the other command structs referenced (the members in the blocks around lines 55-60, 99-101, 135-136, 155-157) and update all local and external references/usages in this header and any implementation files so builds compile with the new identifiers. Ensure symbol names in constructors, execute/undo methods, and any initializer lists are updated accordingly.src/gui/pedalboard/pedal_board_chain.cpp (1)
187-189: ⚡ Quick winUse epsilon comparison for floating-point position equality.
Comparing floats with
!=is unreliable due to floating-point precision issues. Small rounding errors during drag could cause false negatives (command not recorded) or false positives (redundant commands).- if (node_layout.position.x != node_layout.drag_start_pos.x || node_layout.position.y != node_layout.drag_start_pos.y) { + constexpr float EPSILON = 0.01f; + if (std::abs(node_layout.position.x - node_layout.drag_start_pos.x) > EPSILON || + std::abs(node_layout.position.y - node_layout.drag_start_pos.y) > EPSILON) {Apply the same fix at line 208.
Also applies to: 208-210
🤖 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 187 - 189, The float equality check on node_layout.position vs node_layout.drag_start_pos before pushing a MoveGraphNodeCommand is fragile; change the condition used where history_.push_executed(std::make_unique<MoveGraphNodeCommand>(...)) is called to use an epsilon comparison (e.g. fabs(position.x - drag_start_pos.x) > EPS || fabs(position.y - drag_start_pos.y) > EPS) instead of != so tiny rounding differences don't produce incorrect command records; make the identical change for the other occurrence that also constructs/pushes a MoveGraphNodeCommand in this file.src/main.cpp (1)
146-161: ⚡ Quick winRaw pointer after
std::moverelies on CommandHistory implementation details.The pattern of keeping
rawafterstd::move(cmd)works only becauseCommandHistory::execute()stores the unique_ptr without destroying it. This is fragile and could break if the history implementation changes.Consider using
push_executed()pattern (as done inpedal_board_chain.cppline 188) which explicitly separates execution from history recording:♻️ Safer alternative
extern "C" EMSCRIPTEN_KEEPALIVE int trigger_add_splitter_node() { if (!g_gui) return -1; auto cmd = std::make_unique<Amplitron::AddGraphNodeCommand>( g_gui->audio_engine(), "Splitter", Amplitron::NodeRoutingType::Splitter, nullptr, ImVec2(0, 0)); - auto* raw = cmd.get(); - g_gui->command_history().execute(std::move(cmd)); - return (raw->node_id != -1) ? raw->node_id : -1; + if (!cmd->execute()) return -1; + int id = cmd->node_id; + g_gui->command_history().push_executed(std::move(cmd)); + return id; } extern "C" EMSCRIPTEN_KEEPALIVE int trigger_add_link(int src_pin, int dst_pin) { if (!g_gui) return -1; auto cmd = std::make_unique<Amplitron::AddGraphLinkCommand>(g_gui->audio_engine(), src_pin, dst_pin); - auto* raw = cmd.get(); - g_gui->command_history().execute(std::move(cmd)); - return raw->was_successful ? raw->link.id : -1; + if (!cmd->execute()) return -1; + int id = cmd->was_successful ? cmd->link.id : -1; + if (cmd->was_successful) { + g_gui->command_history().push_executed(std::move(cmd)); + } + return id; }🤖 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/main.cpp` around lines 146 - 161, The code captures a raw pointer from cmd before calling command_history().execute(std::move(cmd)), which is fragile; instead create the command (AddGraphNodeCommand / AddGraphLinkCommand), hand it to the history via the safer push_executed pattern used in pedal_board_chain.cpp (i.e. call g_gui->command_history().push_executed(std::move(cmd)) and use the pointer/handle returned by push_executed to read node_id / was_successful / link.id), or alternatively call cmd->execute() before moving it into history so you don't dereference a moved-from pointer; update trigger_add_splitter_node and trigger_add_link to follow one of these approaches and stop relying on raw after std::move.
🤖 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/commands/command_graph.h`:
- Around line 162-168: The execute() method on the move command currently always
returns true, which records no-op moves when node_id is missing or the position
hasn't changed; change execute() (the override in the command class) to first
look up the node in GuiGraphState::get_instance().node_positions (use find to
avoid double lookup), return false if node_id is not present, compare the
existing position to new_pos and only assign positions[node_id].position =
new_pos and return true when they differ, otherwise return false so no-op moves
are not added to undo history.
- Around line 64-83: The execute() implementation must detect when the node
wasn't found and return false instead of proceeding; modify the function so that
after auto* node_to_remove = engine_.graph().find_node(node_id) you immediately
check if node_to_remove is null and return false (do not copy into cached_node
or populate severed_links), otherwise continue to assign cached_node =
*node_to_remove, compute severed_links, call
engine_.graph().remove_node(node_id), erase
GuiGraphState::get_instance().node_positions[node_id], and commit; this ensures
undo() only operates when a real removal occurred.
- Around line 140-144: The execute() method currently always returns true;
change it to call engine_.graph().remove_link(link.id), capture its boolean
result, and only call engine_.commit_graph_changes() when removal succeeded,
then return the actual remove_link result so no-op deletions are not recorded;
refer to the execute() method, engine_.graph().remove_link(link.id),
engine_.commit_graph_changes(), and link.id when making this change.
- Around line 110-121: The execute() path currently treats any non -1 id from
engine_.graph().add_link(...) as a success even when add_link returned an
existing link id; change execute() (and undo/restore logic) so the command only
records success for links actually created by this command: before calling
add_link or after its return, detect whether the link already existed (e.g.,
call a new or existing engine_.graph().has_link(link.source_pin_id,
link.dest_pin_id) or have add_link return a created flag) and set was_successful
(or a new created_by_command boolean) to true only when a new link is created;
ensure engine_.graph().restore_link(link) and undo/remove logic only act when
created_by_command is true and still call engine_.commit_graph_changes() only
when the command actually created or restored a link.
In `@src/gui/commands/command_preset.h`:
- Around line 6-8: Remove the duplicate include of <vector> in the header; in
src/gui/commands/command_preset.h only one `#include` <vector> should remain
(remove the repeated line so the includes are unique alongside <utility>).
Ensure the file contains a single `#include` <vector> and still keeps `#include`
<utility>.
---
Outside diff comments:
In `@src/audio/engine/audio_graph.cpp`:
- Around line 323-333: The restore_node function mutates nodes_ and next_id_
before validating the node and calling rebuild_topology, risking corrupt state
on failure; change it to be transactional by applying the insert to local
temporaries (e.g., a temp vector copy of nodes_ and a temp_next_id computed from
node.id and node.input_pin_ids/output_pin_ids), run the
validation/rebuild_topology logic against the temp state (or call a non-mutating
validate_topology(temp_nodes) helper), and only on success swap the temporaries
into nodes_ and next_id_; on failure discard temporaries and return/error so
nodes_ and next_id_ remain unchanged.
In `@src/gui/pedalboard/pedal_board_chain.cpp`:
- Line 357: Remove the debug printf call that prints "MANUAL DROP HOVER
DETECTED!"—locate the printf("MANUAL DROP HOVER DETECTED! src: %d, dest: %d\n",
ui_state.active_src_pin_id, pin_id) in pedal_board_chain.cpp (near the manual
drop/hover handling code) and delete that line; if you need to retain a trace,
replace it with the project's logging facility or a conditional debug macro,
otherwise simply remove the printf invocation.
---
Duplicate comments:
In `@src/audio/engine/audio_graph.cpp`:
- Around line 335-343: The restore_link function currently appends to links_ and
updates next_id_ without checking link validity; before mutating links_ or
next_id_ in restore_link(const GraphLink& link) first verify both endpoint pins
exist (use your pin lookup/exists helpers) and that no existing link in links_
has the same source/target (or identical id) to avoid duplicates/orphans; only
if those checks pass then push_back(link), adjust next_id_ as you already do,
call rebuild_topology(), and keep the existing rollback logic if
rebuild_topology() fails—otherwise return immediately without modifying links_
or next_id_.
---
Nitpick comments:
In `@src/gui/commands/command_graph.h`:
- Around line 14-20: Rename the struct member fields to follow the project's
trailing-underscore convention: change node_id, name, type, pedal, position, and
cached_node to node_id_, name_, type_, pedal_, position_, and cached_node_ in
the command struct shown (symbols to update: node_id, name, type, pedal,
position, cached_node and the already-correct engine_). Do the same renaming for
the other command structs referenced (the members in the blocks around lines
55-60, 99-101, 135-136, 155-157) and update all local and external
references/usages in this header and any implementation files so builds compile
with the new identifiers. Ensure symbol names in constructors, execute/undo
methods, and any initializer lists are updated accordingly.
In `@src/gui/commands/command_history.cpp`:
- Around line 46-56: CommandHistory::redo currently calls cmd->execute() and
unconditionally moves the command from redo_stack_ to undo_stack_; change it to
check the bool return of cmd->execute() and only push the command onto
undo_stack_ when execute() returns true (mirror the behavior used elsewhere when
execute() indicates a no-op). Locate the redo method, evaluate the result of
cmd->execute(), and conditionally push_back(std::move(cmd)) into undo_stack_
only on success; ensure the method still returns true/false appropriately when
execute() fails.
In `@src/gui/pedalboard/pedal_board_chain.cpp`:
- Around line 187-189: The float equality check on node_layout.position vs
node_layout.drag_start_pos before pushing a MoveGraphNodeCommand is fragile;
change the condition used where
history_.push_executed(std::make_unique<MoveGraphNodeCommand>(...)) is called to
use an epsilon comparison (e.g. fabs(position.x - drag_start_pos.x) > EPS ||
fabs(position.y - drag_start_pos.y) > EPS) instead of != so tiny rounding
differences don't produce incorrect command records; make the identical change
for the other occurrence that also constructs/pushes a MoveGraphNodeCommand in
this file.
In `@src/main.cpp`:
- Around line 146-161: The code captures a raw pointer from cmd before calling
command_history().execute(std::move(cmd)), which is fragile; instead create the
command (AddGraphNodeCommand / AddGraphLinkCommand), hand it to the history via
the safer push_executed pattern used in pedal_board_chain.cpp (i.e. call
g_gui->command_history().push_executed(std::move(cmd)) and use the
pointer/handle returned by push_executed to read node_id / was_successful /
link.id), or alternatively call cmd->execute() before moving it into history so
you don't dereference a moved-from pointer; update trigger_add_splitter_node and
trigger_add_link to follow one of these approaches and stop relying on raw after
std::move.
🪄 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: 7257bf33-8f8b-4573-8056-accbb31a8dea
📒 Files selected for processing (25)
CMakeLists.txtsrc/audio/backend/audio_backend_portaudio_devices.cppsrc/audio/backend/audio_backend_portaudio_internal.hsrc/audio/backend/audio_backend_portaudio_lifecycle.cppsrc/audio/engine/audio_graph.cppsrc/audio/engine/audio_graph.hsrc/gui/commands/command.hsrc/gui/commands/command_base.hsrc/gui/commands/command_chain.hsrc/gui/commands/command_clear.hsrc/gui/commands/command_graph.hsrc/gui/commands/command_history.cppsrc/gui/commands/command_history.hsrc/gui/commands/command_param.hsrc/gui/commands/command_preset.hsrc/gui/commands/command_reorder.hsrc/gui/commands/command_reset.hsrc/gui/gui_manager.hsrc/gui/pedalboard/pedal_board_chain.cppsrc/gui/pedalboard/pedal_board_menu.cppsrc/gui/state/gui_graph_state.hsrc/main.cpptests/fixtures/portaudio_mock.cpptests/fixtures/portaudio_mock.htests/integration/test_audio_backend_portaudio.cpp
💤 Files with no reviewable changes (3)
- src/gui/commands/command_base.h
- src/audio/engine/audio_graph.h
- src/gui/state/gui_graph_state.h
✅ Files skipped from review due to trivial changes (2)
- src/gui/commands/command.h
- src/audio/backend/audio_backend_portaudio_lifecycle.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- CMakeLists.txt
- src/gui/gui_manager.h
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/test_command_graph.cpp (1)
97-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd teardown for
GuiGraphStatesingleton mutation.
GuiGraphState::get_instance()is shared process-wide, and Line 102 insertsnode_id999 without cleanup. This leaks state across tests and can cause order-dependent failures.Suggested fix
TEST(MoveGraphNodeCommand_executes_and_undoes_correctly) { CommandHistory history(100); auto& state = GuiGraphState::get_instance(); @@ history.redo(); ASSERT_EQ(50.0f, state.node_positions[node_id].position.x); ASSERT_EQ(50.0f, state.node_positions[node_id].position.y); + + // teardown: avoid leaking singleton state into other tests + state.node_positions.erase(node_id); }🤖 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_command_graph.cpp` around lines 97 - 117, The test MoveGraphNodeCommand_executes_and_undoes_correctly mutates the process-wide singleton GuiGraphState (via GuiGraphState::get_instance() and its node_positions map) by inserting node_id 999 and never cleaning it up; remove or reset that mutation in a teardown step (e.g., erase node_id 999 from state.node_positions or restore the singleton to its prior state) at the end of the test (or use an RAII/scope-guard) so the shared state does not leak into other tests.
🤖 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.
Outside diff comments:
In `@tests/integration/test_command_graph.cpp`:
- Around line 97-117: The test
MoveGraphNodeCommand_executes_and_undoes_correctly mutates the process-wide
singleton GuiGraphState (via GuiGraphState::get_instance() and its
node_positions map) by inserting node_id 999 and never cleaning it up; remove or
reset that mutation in a teardown step (e.g., erase node_id 999 from
state.node_positions or restore the singleton to its prior state) at the end of
the test (or use an RAII/scope-guard) so the shared state does not leak into
other tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 416d9a59-f9bf-4c57-a8c3-6d6a7e218848
📒 Files selected for processing (2)
src/main.cpptests/integration/test_command_graph.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main.cpp
sudip-mondal-2002
previously approved these changes
May 28, 2026
sudip-mondal-2002
left a comment
sudip-mondal-2002
left a comment
Owner
There was a problem hiding this comment.
LGTM but test coverage didn't increase from base
Collaborator
Author
|
@sudip-mondal-2002 I will try to increase the test coverage |
…00% test coverage for undo/redo
Collaborator
Author
|
@sudip-mondal-2002 Increased test coverage, Check it again |
sudip-mondal-2002
approved these changes
May 28, 2026
This was referenced May 28, 2026
Merged
This was referenced Jun 10, 2026
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Implements a comprehensive, highly-robust Undo/Redo system for the 2D pedalboard audio graph. Users can now seamlessly use
Ctrl+Z/Ctrl+Shift+Zto reverse and reapply complex canvas interactions including adding/removing nodes, drawing/deleting cables, and dragging nodes across the screen.Key implementations:
AddGraphNodeCommand,RemoveGraphNodeCommand,AddGraphLinkCommand,RemoveGraphLinkCommand, andMoveGraphNodeCommand.AudioGraphAPI withrestore_node()andrestore_link()to ensure ID mappings and severed cables are perfectly reconstructed upon undo operations.CommandHistoryrouter to capture all UI events.GuiGraphStateto trackdrag_start_posfor precision floating-point coordinate restoration rather than interpolation approximations.Related Issue
Fixes #150
Type of Change
How Was This Tested?
make build && ./build/amplitron-testsandnpx playwright testControl+Z, and mathematically asserting the exact return to the default initial canvas state.Checklist
./amplitron-tests)std::mutex::lock()on the hot path)Screenshots / Demo
Screencast.from.2026-05-26.11-12-42.mp4
Summary by CodeRabbit
Release Notes
New Features
Tests