feat:Validate and fix modular graph canvas for Emscripten web demo #208
Merged
MohitBareja16 merged 11 commits intoMay 25, 2026
Merged
Conversation
Contributor
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughAdds DPI-aware canvas state and smooth time-based zoom/pan, hover-gated input handling, Emscripten-exported gesture and graph APIs, browser touch/wheel bridging, and Playwright E2E tests verifying pan/zoom, node/link creation, and deletion. ChangesWeb Canvas Interactions
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser/User
participant ShellHTML as shell.html (Event Listeners)
participant EmscriptenAPI as Emscripten API (main.cpp)
participant PedalBoard as PedalBoard (pedal_board_chain.cpp)
participant GuiGraphState as GuiGraphState
participant AudioGraph as Audio Graph (GuiManager)
Browser->>ShellHTML: two-finger pinch/drag or wheel/right-drag
ShellHTML->>EmscriptenAPI: on_canvas_touch_gesture(dx, dy, scale, localX, localY)
EmscriptenAPI->>GuiGraphState: set target_zoom, adjust target_scrolling or set target_scrolling
PedalBoard->>GuiGraphState: read zoom, target_zoom, scrolling, target_scrolling
PedalBoard->>PedalBoard: lerp(zoom, target_zoom, DeltaTime) and lerp(scrolling, target_scrolling)
PedalBoard->>AudioGraph: render nodes/wires with DPI-scaled fonts
Browser->>ShellHTML: triggers add splitter node
ShellHTML->>EmscriptenAPI: trigger_add_splitter_node()
EmscriptenAPI->>AudioGraph: create splitter node (node_count++)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
bhavyanjain3004
changed the title
Contributor
Code Coverage Report 📊Line Coverage: 79.1% ✅ Coverage meets threshold: 79.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: 4
🧹 Nitpick comments (1)
tests/web/amplitron.spec.ts (1)
579-583: ⚡ Quick winAvoid hard-coded node indices in the cable test.
Using fixed indices (
2,3) couples this test to initial graph shape and can cause brittle failures after unrelated graph-default changes.Suggested fix
- const result: number = await page.evaluate(() => { - const srcPin = Module.ccall('get_node_output_pin_by_index', 'number', ['number', 'number'], [2, 0]); - const dstPin = Module.ccall('get_node_input_pin_by_index', 'number', ['number', 'number'], [3, 0]); - return Module.ccall('trigger_add_link', 'number', ['number', 'number'], [srcPin, dstPin]); - }); + const result: number = await page.evaluate(() => { + const nodeCount = Module.ccall('get_node_count', 'number', [], []); + if (nodeCount < 2) return -1; + const srcNodeIndex = nodeCount - 2; + const dstNodeIndex = nodeCount - 1; + const srcPin = Module.ccall('get_node_output_pin_by_index', 'number', ['number', 'number'], [srcNodeIndex, 0]); + const dstPin = Module.ccall('get_node_input_pin_by_index', 'number', ['number', 'number'], [dstNodeIndex, 0]); + return Module.ccall('trigger_add_link', 'number', ['number', 'number'], [srcPin, dstPin]); + });🤖 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 579 - 583, The test currently hard-codes node indices 2 and 3 when creating srcPin/dstPin which makes it brittle; replace those literals by looking up the node indices dynamically (e.g., call whatever existing runtime helpers or use Module.ccall to iterate nodes: get_node_count/get_node_label_by_index or a findNodeIndexByLabel helper) to find the source and destination node indices, then pass those computed indices into get_node_output_pin_by_index and get_node_input_pin_by_index before calling trigger_add_link; update the variables srcPin and dstPin to be derived from that lookup rather than fixed numbers.
🤖 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/pedal_board_chain.cpp`:
- Around line 27-30: The zoom anchoring is using ImGui::GetMousePos() (screen
coords) against ui_state.scrolling (canvas coords), causing drift; compute the
mouse position in canvas-local coordinates first (mouse_local =
ImGui::GetMousePos() - canvas_screen_origin or
ImGui::GetCursorScreenPos()/window origin used for this canvas), then apply the
scaling: use actual_factor = ui_state.zoom / old_zoom and update
ui_state.scrolling.x and .y with mouse_local (not raw screen pos) so the scroll
transform anchors correctly during zoom. Ensure you reference the same canvas
origin used elsewhere for rendering so the subtraction matches
ui_state.scrolling space.
- Line 15: ui_state.canvas_hovered is being set using
ImGui::IsWindowHovered(ImGuiHoveredFlags_ChildWindows) which reports hover for
the whole window, causing non-canvas UI to mark the canvas as hovered; replace
that assignment so it tests hover for the actual canvas item/child instead
(e.g., call ImGui::IsItemHovered(...) immediately after the canvas
BeginChild/Canvas widget, or use ImGui::IsWindowHovered with a handle limited to
the canvas child window if you create the canvas via BeginChild) so
ui_state.canvas_hovered reflects only pointer over the canvas itself.
In `@src/main.cpp`:
- Around line 178-189: The trigger_delete_last_node function currently deletes
the last node unconditionally; instead, check the last node's identity or
protection flag before calling graph.remove_node to match the GUI rules. Locate
trigger_delete_last_node and, after obtaining nodes.back(), inspect the node's
name/type or a protection accessor (e.g., node.name, node.type, or a
is_protected/is_deletable method on the node or graph) and return false if it is
a protected node (e.g., "Input" or "Amp Sim") or if the graph’s deletion policy
forbids it; if deletion is allowed, proceed with graph.remove_node(last_id) and
erase node_positions and commit changes as before. If the project has an
existing UI-safe deletion helper, call that instead of graph.remove_node to
preserve invariants.
In `@web/shell.html`:
- Around line 349-353: The empty catch around the
Module.ccall('is_canvas_hovered', 'boolean') call swallows bridge errors; update
the try/catch that sets canvasHovered to catch (err) and log the error
(including err and context like "is_canvas_hovered failed") via console.error or
an existing logger, and optionally set canvasHovered to a safe default; ensure
the code still guards Module && Module.ccall but does not silently ignore
exceptions from the WASM bridge.
---
Nitpick comments:
In `@tests/web/amplitron.spec.ts`:
- Around line 579-583: The test currently hard-codes node indices 2 and 3 when
creating srcPin/dstPin which makes it brittle; replace those literals by looking
up the node indices dynamically (e.g., call whatever existing runtime helpers or
use Module.ccall to iterate nodes: get_node_count/get_node_label_by_index or a
findNodeIndexByLabel helper) to find the source and destination node indices,
then pass those computed indices into get_node_output_pin_by_index and
get_node_input_pin_by_index before calling trigger_add_link; update the
variables srcPin and dstPin to be derived from that lookup rather than fixed
numbers.
🪄 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: 910e1830-62a4-4269-827b-1ab72a60c73f
📒 Files selected for processing (10)
.gitignoresrc/gui/gui_graph_state.hsrc/gui/gui_manager.cppsrc/gui/gui_manager.hsrc/gui/pedal_board_chain.cppsrc/main.cpptests/web/amplitron.spec.tstests/web/playwright-report/index.htmltests/web/playwright.config.tsweb/shell.html
sudip-mondal-2002
added
type:feature
…/bhavyanjain3004/Amplitron into feature/modular-graph-canvas-web
MohitBareja16
requested changes
May 25, 2026
MohitBareja16
left a comment
MohitBareja16
left a comment
Collaborator
There was a problem hiding this comment.
Code looks fine and web demo works smoothly as well. It only works on the web right? Would be better if you can test it once on your linux/macos/windows and implement the same feature there too.
Contributor
Author
|
@MohitBareja16 The screenrecording I shared is actually from the native macOS desktop app itself and not the web browser. so it works fine. |
Collaborator
LGTM then, Nice work! |
MohitBareja16
approved these changes
May 25, 2026
MohitBareja16
left a comment
MohitBareja16
left a comment
Collaborator
There was a problem hiding this comment.
Zoom in, zoom out working! LGTM!
Merged
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
3 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.
Tasks
fixes #149
Acceptance Criteria
Native unit test
Playwright E2E tests
Zoom and pan behaviour
Screen.Recording.2026-05-23.at.10.40.42.PM.mov
Summary by CodeRabbit
New Features
Tests
Chores