refactor(editor): move the MIDI recorder to src/midi-record.js (R2, step 23) - #175
Merged
Merged
Conversation
…tep 23)
src/main.js 10,380 -> 9,779. Under ten thousand for the first time; 25 modules,
graph still acyclic.
ZERO new host hooks. Two of the recorder's eight main.js dependencies were not
hooks waiting to happen, they were symbols in the wrong file:
_transportChartTimePure pure time math, and the ONE formula the playback tick,
the guide scheduler and the recorder must agree on.
It is now the whole of src/transport.js (with
_composeSongDurationPure, which its test already pairs
it with). No imports; the seed of the audio lift.
_uniqueKeysName names a Keys arrangement. That is what src/keys.js is.
The other six were already on `host`. This is the rule host.js's header states,
applied: a symbol only belongs there if it genuinely cannot leave main.js.
_recState is exported as a live `export let`. Every writer is inside the
recorder; main.js reads it at 14 sites and wires host.isRecording() for the
modules that only need the predicate. An import binding is read-only for
reassignment and live for reads, which is precisely the guarantee wanted — no
container, no accessor.
Tests: compose_transport imports the two pures instead of slicing them.
midi_domain and midi_domain_leak still slice module internals, now from
midi-record.js — CJS -> .mjs, they strip the `export` keyword before eval (it is
a SyntaxError inside `new Function`), and midi_domain_leak's sandbox preamble
gained a `host` stub.
verify_midi_record.py opens the record modal on a loaded session and asserts the
five window.* handlers are bound, that the backend is queried, and that with no
device attached Start stays disabled rather than the modal throwing. Recording
itself needs hardware, so the state machine stays with the unit suites; the
harness deliberately asserts nothing it cannot observe.
node --test 89/89, pytest 248/248, npm run lint 0 errors (6 warnings), Codex
clean, all 17 headless harnesses pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe MIDI recorder and transport helpers were moved from ChangesRecorder and transport modularization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant MidiRecord
participant MIDIInput
participant Playback
participant Arrangement
Editor->>MidiRecord: Start recording
MidiRecord->>MIDIInput: Attach MIDI listener
MidiRecord->>Playback: Start transport playback
MIDIInput->>MidiRecord: Deliver MIDI events
Editor->>MidiRecord: Stop recording
MidiRecord->>Arrangement: Store finalized notes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Refactors the editor’s MIDI recording feature by extracting the recorder logic out of src/main.js into a dedicated module and centralizing transport time math into a new src/transport.js, while updating tests/harness references to match the new module boundaries.
Changes:
- Extract MIDI recorder implementation into
src/midi-record.jsand re-attach itswindow.editor*handlers frommain.js. - Move transport pure functions into new
src/transport.jsand update the compose transport tests to import them directly. - Relocate
_uniqueKeysNameintosrc/keys.jsand update unit tests that slice recorder internals to handle ESM exports.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/midi_domain.test.mjs | Updated to slice recorder internals from src/midi-record.js and strip export keywords for new Function evaluation. |
| tests/midi_domain_leak.test.mjs | Updated leak regression harness to slice from src/midi-record.js, adjust handler extraction, and stub host. |
| tests/compose_transport.test.mjs | Imports transport pures from src/transport.js instead of slicing from main.js. |
| src/transport.js | New module for _transportChartTimePure and _composeSongDurationPure. |
| src/midi-record.js | New module containing MIDI record modal, backend selection, and recording state machine. |
| src/main.js | Wires new module exports, re-attaches window.editor* functions, and imports transport pures. |
| src/keys.js | Adds _uniqueKeysName export for shared use. |
| CHANGELOG.md | Documents the refactor and the new module locations. |
Comments suppressed due to low confidence (1)
tests/midi_domain.test.mjs:35
- The failure message in extractBlock still says it’s searching
src/main.js, but this test now slices fromsrc/midi-record.js, which will mislead debugging when the regex fails.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| const rows = _recMidiDeviceRowsPure(backend, raw); | ||
|
|
||
| const saved = localStorage.getItem('editor.recordMidiDeviceId') || ''; |
| // handle calls listeners with e.data, so the private path | ||
| // unwraps the event here to keep one routing function. | ||
| _recMidiInput.onmidimessage = (e) => _recMidiOnData(e.data); | ||
| localStorage.setItem('editor.recordMidiDeviceId', id); |
| // before audioSource.stop() runs. | ||
| if (S.audioSource) { | ||
| S.audioSource.onended = () => { | ||
| if (_recState === 'recording') window.editorStopRecordMidi(); |
Comment on lines
+29
to
+31
| export const COMPOSE_CONTENT_TAIL = 0.25; // seconds past the last authored onset, so its | ||
| // guide clap (a ~60 ms voice) rings out before | ||
| // playbackTick hits duration and cancels voices. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/midi-record.js (1)
472-476: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelf-reference via
window.editorStopRecordMidi()instead of the local function.
editorStopRecordMidiis defined in this same module and exported; calling it directly avoids depending onmain.jshaving already attached it towindowbefore thisonendedcallback fires. This contradicts the module's own stated self-containment intent ("Every writer is in this file... main.js wires it... for the modules that only need the predicate") and leaves this path untestable without a fullwindowstub (the leak test only exerciseseditorStopRecordMidi()via direct extraction, not through this callback).♻️ Proposed fix
if (S.audioSource) { S.audioSource.onended = () => { - if (_recState === 'recording') window.editorStopRecordMidi(); + if (_recState === 'recording') editorStopRecordMidi(); }; }🤖 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-record.js` around lines 472 - 476, Update the audio source onended callback in the module’s recording setup to invoke the local editorStopRecordMidi function directly instead of window.editorStopRecordMidi(). Preserve the existing recording-state guard and ensure the callback remains self-contained and independently testable.
🤖 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/midi-record.js`:
- Around line 227-237: Guard the localStorage write in the private-backend
branch of _recMidiConnect by wrapping
localStorage.setItem('editor.recordMidiDeviceId', id) in the same try/catch
pattern used elsewhere in the file, allowing storage failures to be ignored
while preserving MIDI connection startup.
---
Nitpick comments:
In `@src/midi-record.js`:
- Around line 472-476: Update the audio source onended callback in the module’s
recording setup to invoke the local editorStopRecordMidi function directly
instead of window.editorStopRecordMidi(). Preserve the existing recording-state
guard and ensure the callback remains self-contained and independently testable.
🪄 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 Plus
Run ID: 062629d2-6149-47d9-9bd0-7757c98ab5f0
📒 Files selected for processing (8)
CHANGELOG.mdsrc/keys.jssrc/main.jssrc/midi-record.jssrc/transport.jstests/compose_transport.test.mjstests/midi_domain.test.mjstests/midi_domain_leak.test.mjs
Comment on lines
+227
to
+237
| _recMidiAccess.inputs.forEach(inp => { | ||
| if (inp.id === id) { | ||
| _recMidiInput = inp; | ||
| // Both paths deliver RAW BYTES to _recMidiOnData — the domain | ||
| // handle calls listeners with e.data, so the private path | ||
| // unwraps the event here to keep one routing function. | ||
| _recMidiInput.onmidimessage = (e) => _recMidiOnData(e.data); | ||
| localStorage.setItem('editor.recordMidiDeviceId', id); | ||
| } | ||
| }); | ||
| return _recMidiInput ? 'ok' : 'fail'; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unguarded localStorage.setItem in the private-backend connect path.
Line 234 calls localStorage.setItem directly, while the same call is wrapped in try/catch everywhere else in this file (lines 142, 220). If storage access throws (e.g. private browsing / quota exceeded), the exception propagates out of this forEach callback, up through _recMidiConnect, and out of the unguarded editorStartRecordMidi() call site — breaking the entire recording start flow instead of just skipping the persisted device-id.
🛡️ Proposed fix
_recMidiAccess.inputs.forEach(inp => {
if (inp.id === id) {
_recMidiInput = inp;
// Both paths deliver RAW BYTES to _recMidiOnData — the domain
// handle calls listeners with e.data, so the private path
// unwraps the event here to keep one routing function.
_recMidiInput.onmidimessage = (e) => _recMidiOnData(e.data);
- localStorage.setItem('editor.recordMidiDeviceId', id);
+ try { localStorage.setItem('editor.recordMidiDeviceId', id); } catch (_) {}
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _recMidiAccess.inputs.forEach(inp => { | |
| if (inp.id === id) { | |
| _recMidiInput = inp; | |
| // Both paths deliver RAW BYTES to _recMidiOnData — the domain | |
| // handle calls listeners with e.data, so the private path | |
| // unwraps the event here to keep one routing function. | |
| _recMidiInput.onmidimessage = (e) => _recMidiOnData(e.data); | |
| localStorage.setItem('editor.recordMidiDeviceId', id); | |
| } | |
| }); | |
| return _recMidiInput ? 'ok' : 'fail'; | |
| _recMidiAccess.inputs.forEach(inp => { | |
| if (inp.id === id) { | |
| _recMidiInput = inp; | |
| // Both paths deliver RAW BYTES to _recMidiOnData — the domain | |
| // handle calls listeners with e.data, so the private path | |
| // unwraps the event here to keep one routing function. | |
| _recMidiInput.onmidimessage = (e) => _recMidiOnData(e.data); | |
| try { localStorage.setItem('editor.recordMidiDeviceId', id); } catch (_) {} | |
| } | |
| }); | |
| return _recMidiInput ? 'ok' : 'fail'; |
🤖 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-record.js` around lines 227 - 237, Guard the localStorage write in
the private-backend branch of _recMidiConnect by wrapping
localStorage.setItem('editor.recordMidiDeviceId', id) in the same try/catch
pattern used elsewhere in the file, allowing storage failures to be ignored
while preserving MIDI connection startup.
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
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.
src/main.js10,380 → 9,779 — under ten thousand for the first time. 25 modules, graph still acyclic.Zero new host hooks
The recorder reached eight
main.jssymbols. Six were already onhost. The other two were not hooks waiting to happen — they were symbols in the wrong file:_transportChartTimePuresrc/transport.js(new)_composeSongDurationPure, which its own test already pairs it with. No imports — the seed of the eventual audio lift._uniqueKeysNamesrc/keys.jskeys.jsis.This is the rule
host.js’s own header states, applied: a symbol only belongs there if it genuinely cannot leavemain.js. A pure function always can._recStateas a live binding_recStateis nowexport let. Every writer is inside the recorder;main.jsreads it at 14 sites and wireshost.isRecording()for the modules that only need the predicate.An import binding is read-only for reassignment and live for reads — precisely the guarantee wanted. No container, no accessor. (This is the same live-binding rule that decided
lanes.js’sLCthe other way: can the writer move?)Tests
compose_transportimports the two pures instead of slicing them.midi_domain/midi_domain_leakstill slice module internals, now frommidi-record.js. Both CJS →.mjs. They strip theexportkeyword before eval (it is aSyntaxErrorinsidenew Function), andmidi_domain_leak’s sandbox preamble gained ahoststub.Harness
verify_midi_record.pyopens the record modal on a loaded session and asserts the fivewindow.*handlers are bound, that the backend is queried, and that with no device attached Start stays disabled rather than the modal throwing.Recording itself needs hardware, so the state machine stays with the unit suites. The harness deliberately asserts nothing it cannot observe — an earlier draft had a hardcoded
Trueand a "canvas still exists" check; both were padding and were cut.Comment out one
window.*re-attach and the harness throws where 89 unit tests still pass.Verification
node --test89/89 ·pytest248/248 ·npm run lint0 errors (6 warnings) · Codex clean · all 17 headless harnesses pass.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes