Feature/add web midi api#112
Merged
sudip-mondal-2002 merged 19 commits intoMay 22, 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 adds Web MIDI API support to the Emscripten web demo. CMake enables MIDI compilation for Emscripten builds, C++ exports Emscripten callbacks to inject MIDI events, JavaScript bridges Web MIDI API messages, Web-specific CC mappings are registered with toggle-press behavior, and end-to-end tests verify the integration. ChangesWeb MIDI Support for Emscripten
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Owner
|
Why are there 1 lakh line changes? looks spammy Did you mistakenly push something @Rakshak05 ? |
Rakshak05
force-pushed
the
feature/Add-Web-MIDI-API
branch
from
a331f90 to
0677839
Compare
Contributor
Author
|
my bad @sudip-mondal-2002. |
![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: 6
🧹 Nitpick comments (1)
tests/web/amplitron.spec.ts (1)
364-437: ⚡ Quick winTest intent and assertion scope are mismatched.
The test title claims CC11 controls output gain, but it never asserts that
on_midi_ccwas called or that gain changed. Add an assertion on bridge invocation (or observable parameter effect) so this test covers the contract.🤖 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 364 - 437, The test currently sends a mock MIDI CC11 message but never asserts its effect; update the test "MIDI CC11 controls output gain" to verify the bridge handler (on_midi_cc) was invoked or that the output gain actually changed after the MIDI message. Locate the MIDI injection via midiMessageListener and the unlock trigger ('`#audio-unlock`') and add an assertion that the bridge method on_midi_cc was called (or observe a visible/inspectable change such as a gain value in the DOM or an exposed JS variable) after the mock message is delivered, failing the test if no change or invocation occurs.
🤖 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/nanosvgrast.h`:
- Around line 1235-1254: The defringe pass currently skips the immediate
left/top neighbor when x==1 or y==1 because it uses the checks x-1 > 0 and y-1 >
0; update those conditions to test for a valid neighbor (use x > 0 or x-1 >= 0,
and y > 0 or y-1 >= 0) so the left/top pixels at index 1 are included, leaving
the other boundary checks (x+1 < w, y+1 < h) unchanged; locate the block in
nanosvgrast.h where variables x, y, w, h, row and stride are used in the
defringe pass and replace the two offending comparisons accordingly.
- Around line 223-232: The nsvg__alloc function and several realloc sites risk
corrupting rasterizer state on OOM; change the nsvg__nextPage() call in
nsvg__alloc to assign its return to a temporary (e.g., NSVGrasterizerPage* next
= nsvg__nextPage(r, r->curpage)), check for NULL and only then set r->curpage =
next before dereferencing, and for every realloc site referenced (places
updating capacity/pointer such as the blocks around realloc calls that currently
update capacity before checking the realloc result), assign realloc() to a
temporary pointer, check for NULL, and only then update the original buffer
pointer and its capacity field (update pointer and capacity together after
success) to avoid desynchronization and state corruption.
In `@src/main.cpp`:
- Around line 50-89: Clear the global pointer g_gui during application shutdown
so MIDI callbacks can't dereference a destroyed GuiManager: in your
shutdown/teardown routine (the code that destroys the GuiManager instance),
explicitly set g_gui = nullptr after destroying or resetting the GuiManager
object; keep the existing guards in on_midi_cc and on_midi_device_connected that
check if (!g_gui) to drop events, and apply the same nulling change wherever
GuiManager is torn down (also ensure any other places that stop MIDI callbacks
happen-before you null g_gui).
In `@src/midi/midi_manager_mapping.cpp`:
- Around line 76-93: The Web-specific MIDI defaults block is missing the CC1
mapping required by the feature contract; add a MidiMapping for CC1 (modulation)
in the same block using MidiMapping cc1_mod; set cc_number = 1, midi_channel =
-1 (any channel), target_type = MidiTargetType::Modulation (or the enum value
used for modulation), mode = MidiMappingMode::Continuous, and call
add_mapping(cc1_mod) alongside the existing cc11_output and cc7_output mappings
so CC1/CC7/CC11 defaults are all present.
- Around line 101-108: The OutputGain handling currently ignores
MidiMapping.mode and effect_name so a mapping like the cc64_bypass MidiMapping
(cc_number=64, midi_channel=-1, target_type=MidiTargetType::OutputGain,
mode=MidiMappingMode::Toggle, effect_name="AmpSimulator") behaves as continuous
gain instead of a bypass toggle; update the code in the OutputGain branch to
check mapping.mode and mapping.effect_name and, when
mode==MidiMappingMode::Toggle and effect_name is non-empty, route the action to
the existing bypass-toggle logic (or call the EffectBypass handler) rather than
applying continuous scaling—use the MidiMapping struct (cc64_bypass) and
add_mapping flow to locate and modify the OutputGain branch to respect Toggle
semantics and effect_name.
In `@web/shell.html`:
- Around line 217-222: When inputCount > 0, include the active MIDI device name
in the status message instead of only the count: retrieve the device name from
the connected MIDI input object (e.g., inputs[0].name or the first element of
the array/Map of MIDIInput objects) and call updateMIDIStatus with a string like
"MIDI Active: [Device Name]" (or "✓ [n] MIDI device(s) — Active: [Device Name]"
if you prefer to keep the count). Update the console.log similarly to include
input.name so the UI and logs reflect the active device.
---
Nitpick comments:
In `@tests/web/amplitron.spec.ts`:
- Around line 364-437: The test currently sends a mock MIDI CC11 message but
never asserts its effect; update the test "MIDI CC11 controls output gain" to
verify the bridge handler (on_midi_cc) was invoked or that the output gain
actually changed after the MIDI message. Locate the MIDI injection via
midiMessageListener and the unlock trigger ('`#audio-unlock`') and add an
assertion that the bridge method on_midi_cc was called (or observe a
visible/inspectable change such as a gain value in the DOM or an exposed JS
variable) after the mock message is delivered, failing the test if no change or
invocation occurs.
🪄 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: c6d2dba8-9d4b-4dbd-bd35-c0d74861db6e
📒 Files selected for processing (10)
CMakeLists.txtsrc/audio/dsp/dr_wav.hsrc/gui/gui_manager.hsrc/gui/nanosvg.hsrc/gui/nanosvgrast.hsrc/main.cppsrc/midi/midi_manager.cppsrc/midi/midi_manager_mapping.cpptests/web/amplitron.spec.tsweb/shell.html
Contributor
Author
|
Just a quick note. Now the diff size is ~14k added lines are primarily from including required header-only dependencies for the web build:
These are lightweight, single-header libraries needed for WAV loading and SVG rendering and are compiled directly into the project (no external linking or submodules involved). Aside from these, the remaining changes are limited to the Web MIDI integration and related source updates. Sorry for the inconvenience. |
Contributor
Author
|
@sudip-mondal-2002 One small suggestion: it might be worth adding |
Owner
it looks like it's already there but sure you may add it explicitly
|
sudip-mondal-2002
requested changes
May 17, 2026
sudip-mondal-2002
left a comment
sudip-mondal-2002
left a comment
Owner
There was a problem hiding this comment.
Do not add external header files like this
please add it inside external and update setup_dependencies, CI and all the scripts
the same way imggui is used
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/midi/midi_manager_mapping.cpp (1)
222-231: ⚡ Quick winRemove or gate the hard-coded bypass debug print.
Line 231 logs
"AmpSimulator"on every press edge even though this branch is keyed bymapping.effect_name. That will pollute the browser console and becomes misleading as soon as another effect gets a bypass mapping.🤖 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 222 - 231, The hard-coded debug print "AmpSimulator BYPASS TOGGLED" should be removed or replaced with a gated/logging call that uses the actual mapping.effect_name so it doesn't mislead; locate the press-edge branch where mapping.last_state is set and effects[i]->set_enabled()/engine.push_effect_enabled() are called and either delete the printf or replace it with a formatted logger (or conditional debug flag) that prints mapping.effect_name (or i) and the new enable state instead of the fixed "AmpSimulator" string.src/midi/midi_manager.h (1)
41-44: ⚡ Quick winAlign the public
Togglecontract with the new edge-triggered state.Adding
last_statemeans bypass mappings now behave as press-edge toggles, butMidiMappingMode::Toggleis still documented asCC >= 64 -> on, CC < 64 -> off. Please update that public comment or rename the mode so callers are not told the old semantics.🤖 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.h` around lines 41 - 44, The public contract for MidiMappingMode::Toggle no longer matches its implementation because last_state makes mappings behave as an edge-triggered press toggle; update the API by either (A) changing the MidiMappingMode::Toggle doc-comment to explicitly state "edge/press-triggered toggle (toggled on CC press edge, not level: rising CC >=64 crossing toggles state tracked via last_state)" or (B) rename the enum value to a clearer name (e.g., MidiMappingMode::ToggleOnPress or MidiMappingMode::EdgeToggle) and update all usages/tests and the public comment accordingly so callers are not misled by the old "CC >= 64 -> on, CC < 64 -> off" semantics; ensure you update any public docs/comments that describe the enum and adjust consumers that relied on level semantics.
🤖 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/nanosvgrast.h`:
- Around line 796-810: The dash loop can hang when strokeDashArray contains zero
or negative entries: before computing allDashLen and entering the segmentation
loop in nanosvgrast (use variables allDashLen, shape->strokeDashCount,
shape->strokeDashArray, shape->strokeDashOffset, dashOffset, idash, dashLen,
scale), validate and filter out any dash entries <= 0 (or collapse them) into a
sanitized dash list and adjust strokeDashCount/allDashLen accordingly; use that
sanitized array for the fmodf/dashOffset computation and for the while loops
(and similarly for the later 817–839 code paths) so dashOffset is always reduced
and dashLen never becomes zero, preventing infinite loops.
- Around line 284-295: nsvg__duplicatePoints currently returns void and may
return early on realloc failure leaving r->points2/r->npoints2 in a stale state;
change nsvg__duplicatePoints to return a success/failure value (e.g., int or
bool), and on realloc failure explicitly set r->points2 = NULL and r->npoints2 =
0 (or otherwise leave a consistent state) before returning failure so callers
(dash code paths) can skip the dashed stroke; update callers to check the return
value and abort the dashed-stroke handling when duplication fails.
---
Nitpick comments:
In `@src/midi/midi_manager_mapping.cpp`:
- Around line 222-231: The hard-coded debug print "AmpSimulator BYPASS TOGGLED"
should be removed or replaced with a gated/logging call that uses the actual
mapping.effect_name so it doesn't mislead; locate the press-edge branch where
mapping.last_state is set and
effects[i]->set_enabled()/engine.push_effect_enabled() are called and either
delete the printf or replace it with a formatted logger (or conditional debug
flag) that prints mapping.effect_name (or i) and the new enable state instead of
the fixed "AmpSimulator" string.
In `@src/midi/midi_manager.h`:
- Around line 41-44: The public contract for MidiMappingMode::Toggle no longer
matches its implementation because last_state makes mappings behave as an
edge-triggered press toggle; update the API by either (A) changing the
MidiMappingMode::Toggle doc-comment to explicitly state "edge/press-triggered
toggle (toggled on CC press edge, not level: rising CC >=64 crossing toggles
state tracked via last_state)" or (B) rename the enum value to a clearer name
(e.g., MidiMappingMode::ToggleOnPress or MidiMappingMode::EdgeToggle) and update
all usages/tests and the public comment accordingly so callers are not misled by
the old "CC >= 64 -> on, CC < 64 -> off" semantics; ensure you update any public
docs/comments that describe the enum and adjust consumers that relied on level
semantics.
🪄 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: 89bec57a-4043-40a6-a743-d22ab551947f
📒 Files selected for processing (7)
.gitignoreCMakeLists.txtsrc/gui/nanosvgrast.hsrc/main.cppsrc/midi/midi_manager.hsrc/midi/midi_manager_mapping.cppweb/shell.html
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main.cpp
- CMakeLists.txt
- web/shell.html
Rakshak05
force-pushed
the
feature/Add-Web-MIDI-API
branch
from
6e64393 to
1d9cde4
Compare
Owner
|
@Rakshak05 please don't force push |
sudip-mondal-2002
added
gssoc:approved
sudip-mondal-2002
requested changes
May 18, 2026
sudip-mondal-2002
left a comment
sudip-mondal-2002
left a comment
Owner
There was a problem hiding this comment.
Do not push nanosvg, dr_wav etc to codebase, please add it in dependencies
As far as i remember, nanosvg is already in the dependencies
Owner
|
@Rakshak05 Are you still working on it? |
Contributor
Author
|
yess sir, actually i had an exam today, so was busy with that. I'll work on this tonight! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@CMakeLists.txt`:
- Around line 51-71: The download steps for the three headers (variables
_nanosvg_url, _nanosvgrast_url, _dr_wav_url and their file(DOWNLOAD ...)
invocations) must be hardened: pin each download with EXPECTED_HASH set to the
known SHA256 for the file, enable TLS_VERIFY to ON, capture the STATUS variable
already used (_dl_status, _dl_status2, _dl_status3) and check its value after
each file(DOWNLOAD); if the STATUS indicates failure or the downloaded file's
hash doesn't match EXPECTED_HASH, abort configuration with message(FATAL_ERROR)
and a clear error message. Apply this pattern to all three file(DOWNLOAD ...)
calls so configure-time downloads are verified and fail fast on mismatch.
🪄 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: 94bb1237-4eda-4908-a875-9f333510d1b7
📒 Files selected for processing (8)
.gitignoreCMakeLists.txtsrc/main.cppsrc/midi/midi_manager.hsrc/midi/midi_manager_mapping.cpptests/web/amplitron.spec.tstests/web/playwright-report/index.htmlweb/shell.html
💤 Files with no reviewable changes (5)
- src/midi/midi_manager.h
- src/main.cpp
- tests/web/amplitron.spec.ts
- web/shell.html
- src/midi/midi_manager_mapping.cpp
Contributor
PR Preview RemovedThe GitHub Pages preview for this PR has been removed because the PR was closed. |
sudip-mondal-2002
requested changes
May 21, 2026
sudip-mondal-2002
left a comment
sudip-mondal-2002
left a comment
Owner
There was a problem hiding this comment.
Hi As requested before please remove dr_wav and nanosvg from the codebase and add them in the setup_dependencies script
Nanosvg is already a part of it.
Contributor
Author
|
Done Sir. |
Owner
|
@Rakshak05 the files are still there, please remove |
Owner
|
@Rakshak05 now builds are failing, take a look |
Rakshak05
force-pushed
the
feature/Add-Web-MIDI-API
branch
from
a4b7e85 to
f63d7b2
Compare
Contributor
Code Coverage Report 📊Line Coverage: 66.2% ✅ Coverage meets threshold: 66.2% >= 60% Full Coverage Summary |
Contributor
Author
|
finally! 😅 |
12 tasks
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds Web MIDI API support to the browser demo, allowing real-time parameter control via USB MIDI controllers without installation.
Details:
on_midi_ccviaEMSCRIPTEN_KEEPALIVEMidiManagermapping system.Related Issue
Fixes #70
Type of Change
How Was This Tested?
Manual Testing
Limitations
Checklist
Summary by CodeRabbit
New Features
Tests
Chores