fix(editor): route live MIDI record through the host midi-input capability domain - #121
Conversation
EDITOR-VIEW-MODALITY-DESIGN P4 (VD.4, decision V7) — closes the compat drift flagged in the DAW-workspace doc (open question #3): the editor called private navigator.requestMIDIAccess while the org converged on the core midi-input capability domain (window.feedBack.midiInput v1). - Backend adapter (@pure:midi-adapter): domain preferred whenever the host ships v1; older hosts fall back to the private Web-MIDI path unchanged; 'none' disables Record with an honest title. - ONE routing function: the domain handle delivers raw bytes (e.data); the private path unwraps its MIDIMessageEvent to the same bytes — _recMidiOnMessage(e) became _recMidiOnData(bytes), body unchanged. - Start stays SYNCHRONOUS (the user-gesture constraint that keeps the transport anchor honest): the domain session pre-opens at modal-open and on device change; _recMidiConnect only attaches the listener and returns ok|pending|fail ('pending' = pre-open in flight, retry lands). - Sessions are the domain's SHARED refcounted kind: Stop detaches the listener but keeps the session for follow-up takes; modal close releases our ref (never yanks the device from drums/input wizard). - Device picker normalizes both source shapes to one {id, label} row; editor.recordMidiDeviceId persistence unchanged. Tests: tests/midi_domain.test.js (8) — backend selection (unknown future domain versions NOT assumed compatible), picker normalization, and behavioral-equivalence routing over raw bytes: on/off pairing, vel-0-off, channel filter, CC64 pedal deferral, cross-channel pedal isolation, idle gating. Full suite green except pre-existing CRLF section_coverage (#116). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds a host ChangesMIDI domain backend integration
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/midi_domain.test.js (1)
33-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
extractFnname lookup can collide with prefix-matching function names.
src.indexOf('function ' + name)matches a raw substring, so a function whose name hasnameas a prefix (e.g._recMidiOnDataExtradefined earlier inscreen.js) would be picked up as a false match before the intended target. Not currently triggered, but worth hardening since this harness will keep being reused for future@pure/extracted-function tests.♻️ Suggested word-boundary fix
function extractFn(name) { - const start = src.indexOf('function ' + name); + const re = new RegExp('function\\s+' + name + '\\s*\\('); + const m = src.match(re); + assert.ok(m, `function ${name} must exist`); + const start = m.index; - assert.ok(start >= 0, `function ${name} must exist`); const open = src.indexOf('{', start);🤖 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/midi_domain.test.js` around lines 33 - 43, The extractFn helper currently uses a raw substring search that can accidentally match functions whose names only start with the requested name. Update extractFn to locate the exact function declaration for the target name in the test harness, using a name boundary-aware match instead of src.indexOf('function ' + name), so it reliably extracts the intended function from src even when similarly named helpers like _recMidiOnDataExtra exist.
🤖 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 `@tests/midi_domain.test.js`:
- Around line 33-43: The extractFn helper currently uses a raw substring search
that can accidentally match functions whose names only start with the requested
name. Update extractFn to locate the exact function declaration for the target
name in the test harness, using a name boundary-aware match instead of
src.indexOf('function ' + name), so it reliably extracts the intended function
from src even when similarly named helpers like _recMidiOnDataExtra exist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ef8061da-9dc5-481a-9339-064a5a3b12c4
📒 Files selected for processing (4)
CHANGELOG.mdscreen.htmlscreen.jstests/midi_domain.test.js
- guard _recMidiEnsureOpen against stale/superseded async opens via a generation counter; a resolution that lands after a newer open or after teardown self-closes its orphaned session ref instead of leaking it or resurrecting a handle onto a torn-down session. - release the domain MIDI session in editorStopRecordMidi (Stop hides the modal, so the modal-close teardown never ran, holding the refcounted session open indefinitely). - add tests/midi_domain_leak.test.js pinning both leaks (fail pre-fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/midi_domain_leak.test.js (1)
138-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only pattern-matches source text, doesn't exercise the behavior.
This asserts
_recMidiDisconnectDomain()appears as a substring in the extractededitorStopRecordMidibody — it never actually invokes the function with a fake domain/handle. It would still pass if the call were dead, misplaced, or conditionally unreachable, giving a false sense of regression coverage for the leak this PR is meant to fix.Consider extending the existing harness pattern (as used for
makeEnv/makeDomain) to actually calleditorStopRecordMidiagainst a stubbed DOM/panel and a fake domain with an open handle, then assertdom._calls.closefired — mirroring how tests 1 and 2 verify behavior rather than source text.🤖 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/midi_domain_leak.test.js` around lines 138 - 142, The current test only checks that editorStopRecordMidi contains a source-text substring for _recMidiDisconnectDomain(), so it can pass without proving the leak fix works. Update the test to follow the existing makeEnv/makeDomain harness style: create a stubbed DOM/panel and a fake open domain/handle, invoke editorStopRecordMidi for real, and assert the expected close/disconnect behavior occurs (for example, dom._calls.close is recorded). Keep the verification behavioral, not based on matching the function body text.
🤖 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 `@tests/midi_domain_leak.test.js`:
- Around line 138-142: The current test only checks that editorStopRecordMidi
contains a source-text substring for _recMidiDisconnectDomain(), so it can pass
without proving the leak fix works. Update the test to follow the existing
makeEnv/makeDomain harness style: create a stubbed DOM/panel and a fake open
domain/handle, invoke editorStopRecordMidi for real, and assert the expected
close/disconnect behavior occurs (for example, dom._calls.close is recorded).
Keep the verification behavioral, not based on matching the function body text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 505d8dc4-a6ff-47ba-b182-388a3f87a78c
📒 Files selected for processing (2)
screen.jstests/midi_domain_leak.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- screen.js
Address CodeRabbit nitpick: invoke editorStopRecordMidi against a stubbed DOM/transport and a fake open domain handle, asserting the session is actually closed and the handle released — instead of pattern-matching the function body. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the nitpick on tests/midi_domain_leak.test.js in 12f41af: the Stop-teardown test now invokes editorStopRecordMidi for real against a stubbed DOM/transport + fake open domain handle and asserts the session is closed (dom._calls.close) and the handle released — no longer source-text matching. @coderabbitai review |
|
✅ Action performedReview finished.
|
…-domain # Conflicts: # CHANGELOG.md
2 participants
P4 of the view-modality train (
EDITOR-VIEW-MODALITY-DESIGN.mdVD.4, decision V7: input routing is orthogonal to view choice) — and it closes the compat drift the DAW-workspace doc flagged as open question #3: the editor''s record path called privatenavigator.requestMIDIAccesswhile the rest of the org (drums plugin, input wizard) converged on the coremidi-inputcapability domain — one permission prompt, one shared source list, refcounted sessions, PII-redacted diagnostics.What
@pure:midi-adapter):window.feedBack.midiInputv1 preferred whenever the host ships it; older hosts fall back to the private Web-MIDI path unchanged; neither → Record disabled with an honest tooltip. An unknown future domain version is deliberately NOT assumed compatible (falls back)._recMidiOnMessage(e)became_recMidiOnData(bytes)with the body untouched.AudioContext.resume()grace period — an await would put every captured note at t=0). So the domain session pre-opens at modal-open and on device change (newonchange), and_recMidiConnectonly attaches the listener, returningok|pending|fail—pendingcovers a still-in-flight pre-open with a "press Start again" status.{id, label}row;editor.recordMidiDeviceIdpersistence unchanged. One small behavior delta, stated honestly: under the domain, the device list refreshes on modal open rather than live onstatechange(the private path keeps its live refresh).Tests
tests/midi_domain.test.js(8): backend selection, picker normalization for both shapes, and behavioral-equivalence routing over raw bytes through the real router — note on/off pairing with sustain, velocity-0-as-off, the channel filter, CC64 pedal deferral, cross-channel pedal isolation, and idle-state gating. Full suite green except the pre-existing CRLFsection_coveragefailure (#116 fixes it).Fresh region vs the open queue — the record-MIDI plumbing only.
🤖 Generated with Claude Code
https://claude.ai/code/session_01JEoFeTPSnz4NpwwCG52hnu
Summary by CodeRabbit
New Features
Bug Fixes
Tests