refactor(editor): move the audio subsystem to src/audio.js (R2, step 27) - #179
Merged
Conversation
src/main.js 8,760 -> 7,715. Twenty-eighth module; the graph stays acyclic. main.js is now 64% below the 21,176 lines this refactor started from. 1,039 lines: the playback engine (startPlayback / stopPlayback / playbackTick), the waveform, the onset strip, follow-scroll and the WebAudio graph, plus the guide claps, the metronome, the A/B reference loop, the per-bus mixer and the edit blip. Stacks on #178 (the two loop-region pures it needs from transport.js). It owns the rAF loop: `rafId` is module-scope, set by playbackTick and cancelled by teardownAudio(), which main.js's screen teardown calls. The import-time button seeding folded into initAudio(), called from init(). The 8 window.editor* toolbar handlers are exported and re-attached. Five main.js symbols arrive as host hooks: draw, drawNow, the scroll-bounds math (editorClampScrollX / editorApplyScrollBounds), and the A/B loop-region selection (selectedLoopRegion / setLoopRegionEnabled). TWO REAL BUGS, both found on review, neither caught by the 90 unit tests: 1. main.js was REASSIGNING the now-imported _abPhase and _abOn bindings (illegal — an import binding is read-only). The A/B state's only writers outside the engine were two disarm sites; they now call the new export _abDisarm(), so all A/B state writes stay inside audio.js. Same live-binding rule as every prior step: the writer couldn't move, so it crosses as a function. 2. setHostHooks() was missing all 5 new hooks — my scripted edit's anchor didn't match and Python didn't complain (the exact trap from #176). So audio.js ran against host.js's inert defaults: playback advanced the cursor without repainting, follow-scroll was identity, A/B saw no region. Codex caught it. verify_audio.py now plays in compose mode (grid-driven, no audio buffer) and counts canvas repaints — with the drawNow hook unwired the cursor advances and the canvas never clears. It fails on exactly bug #2 while all 90 unit tests pass. And a third, latent: teardownAudio() completes what the old inline teardown skipped — it stops the guide/metronome setInterval (module-scope, so it outlived a re-injected screen). Codex flagged the gap; the fix is S.playing=false + _guideTimerSync() + _guideCancelVoices(). Tests: ~11 audio suites retarget their source read from main.js to audio.js and strip the `export` keyword before eval; loop_ab and boot_teardown gained host stubs for the new hooks. Several CJS -> .mjs. node --test 90/90, pytest 248/248, npm run lint 0 errors (6 warnings), Codex clean on three rounds, all 20 headless harnesses pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe audio subsystem is implemented in ChangesAudio subsystem refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant audiojs as audio.js
participant WebAudio
participant Host
participant GuideTimer
Editor->>audiojs: initAudio()
Editor->>audiojs: startPlayback()
audiojs->>WebAudio: start recording or compose source
audiojs->>Host: apply loop and scroll state
audiojs->>GuideTimer: synchronize guide scheduler
GuideTimer->>WebAudio: schedule clap or metronome voices
audiojs->>Editor: update playhead and repaint
Editor->>audiojs: teardownAudio()
audiojs->>GuideTimer: clear scheduler
audiojs->>WebAudio: stop sources and queued voices
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
This PR continues the editor refactor by extracting the playback/audio subsystem out of src/main.js into a new leaf module src/audio.js, wiring it back through src/host.js hooks, and retargeting unit tests that previously sliced audio-related source from main.js.
Changes:
- Introduces
src/audio.jscontaining playback (rAF loop), waveform/onset helpers, guide/metronome scheduler, mixer, and A/B loop logic, withteardownAudio()now responsible for stopping playback + intervals. - Updates
src/main.jsto import/re-attach audio toolbar handlers, delegate teardown toteardownAudio(), and provide the new host hooks needed by the audio module. - Updates multiple tests to read/slice source from
src/audio.jsinstead ofsrc/main.js, plus adds a changelog entry describing the extraction and teardown fix.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/waveform_peaks.test.js | Retargets waveform peak extraction source from main.js to audio.js. |
| tests/onset_strip.test.js | Retargets onset-strip pure-block extraction to audio.js and strips export before eval. |
| tests/onset_snap.test.js | Splits extraction across audio.js (onset pure) and main.js (snapTime) and strips export. |
| tests/metronome_click.test.js | Retargets guide-clap pure-block extraction to audio.js and strips export. |
| tests/loop_ab.test.js | Retargets loop-ab extraction to audio.js, adds host stubs, and adjusts runtime slicing. |
| tests/keyboard_gutter.test.mjs | Retargets extracted audition function source to audio.js and strips export. |
| tests/guide_clap.test.js | Retargets guide-clap pure-block extraction to audio.js and strips export. |
| tests/follow_toggle.test.js | Retargets follow-scroll pure-block extraction to audio.js and strips export. |
| tests/compose_transport.test.mjs | Retargets compose transport helper extraction to audio.js after the move. |
| tests/boot_teardown.test.js | Updates teardown harness to delegate playback/rAF cleanup to teardownAudio(). |
| tests/audio_mixer.test.js | Retargets mixer/bus pure-block extraction to audio.js and strips export. |
| src/main.js | Removes inlined audio subsystem, wires host hooks for audio.js, re-attaches window handlers, and delegates teardown via teardownAudio(). |
| src/host.js | Adds new host hook defaults needed by src/audio.js (drawNow/scroll/loop-region hooks). |
| src/audio.js | New module containing audio/playback/mixer/A/B/onset logic and teardownAudio(). |
| CHANGELOG.md | Documents the extraction and the teardown timer fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
src/main.js (1)
561-567: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winsrc/main.js:566 — Call
_guideTimerSync()after_abDisarm(). Clearing the region can leave the scheduler running when A/B was the only active guide source; syncing here matches theloadCDLC()path and stops it immediately.🤖 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.js` around lines 561 - 567, Update _updateLoopRegionControls() to call _guideTimerSync() immediately after _abDisarm() when clearing an active A/B guide, ensuring the scheduler stops when no guide source remains.
🤖 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.js`:
- Around line 917-924: Update _abDisarm() to call _guideTimerSync() after
disabling A/B and restoring the reference gain, ensuring the guide-clap
scheduler immediately reflects the inactive A/B state. Remove any redundant
caller-side synchronization only if appropriate, while preserving existing
behavior in loadCDLC() and _updateLoopRegionControls().
- Around line 54-70: Update loadAudio to propagate fetch or decode failures
instead of swallowing them: after logging in its catch block, return a failure
signal or rethrow the error. In the loadCDLC caller, check that result before
invoking draw() or setting the “Loaded” status, preserving the previous audio
state only as appropriate and stopping the success flow on failure.
In `@tests/metronome_click.test.js`:
- Line 20: Update the stale error message in the metronome test’s guide-clap
block lookup to reference src/audio.js instead of src/main.js, keeping the
failure output consistent with the file actually being read.
In `@tests/onset_snap.test.js`:
- Line 28: Update the stale error message in the onset-snap test harness to
reference src/audio.js instead of src/main.js, keeping the failure path
consistent with the file actually being read.
In `@tests/onset_strip.test.js`:
- Around line 18-24: Update the missing-marker error message in the onset-strip
test to reference src/audio.js instead of src/main.js, and apply the same
correction in the corresponding failure messages in follow_toggle.test.js,
audio_mixer.test.js, guide_clap.test.js, and loop_ab.test.js.
---
Outside diff comments:
In `@src/main.js`:
- Around line 561-567: Update _updateLoopRegionControls() to call
_guideTimerSync() immediately after _abDisarm() when clearing an active A/B
guide, ensuring the scheduler stops when no guide source remains.
🪄 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: 6443b1d0-db6e-4da9-bd32-46cabe9e905b
📒 Files selected for processing (15)
CHANGELOG.mdsrc/audio.jssrc/host.jssrc/main.jstests/audio_mixer.test.jstests/boot_teardown.test.jstests/compose_transport.test.mjstests/follow_toggle.test.jstests/guide_clap.test.jstests/keyboard_gutter.test.mjstests/loop_ab.test.jstests/metronome_click.test.jstests/onset_snap.test.jstests/onset_strip.test.jstests/waveform_peaks.test.js
…est paths Copilot + CodeRabbit, on #179. - CodeRabbit (Major, audio.js): _abDisarm() now calls _guideTimerSync() itself. Disarming A/B flips _guideTimerSync's "want" (which includes _abActive()), so leaving each caller to remember it invited exactly the kind of stranded-timer leak the teardown fix already chased. The one caller that did call it explicitly (the song-change reset) drops the now-redundant call. - Copilot (loop_ab.js): the A/B runtime slice regex matched a bare `let _abOn` but the code is now an exported binding. It worked by substring luck; made the `export` prefix optional-explicit so it can't silently mis-slice. - CodeRabbit (x3): metronome_click / onset_snap / onset_strip printed "not found in src/main.js" in their block-missing guard, but they read src/audio.js now. Corrected. NOT changed, with reasons: - loadAudio() swallowing decode errors (CodeRabbit Major, Copilot): real, but PRE-EXISTING and verbatim-moved. A decode failure leaving the prior song's buffer is a latent bug, and fixing it means changing loadCDLC's flow and deciding what the UI shows on failure — a behaviour change that belongs in its own PR, not a mechanical extraction. node --test 90/90, npm run lint 0 errors, verify_audio.py passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Addressed five of the seven; the other two are one pre-existing issue I'm deliberately deferring. Fixed
Deferred, with reason — |
byrongamatos
added a commit
that referenced
this pull request
Jul 10, 2026
…p.js (R2, step 28) src/main.js 7,714 -> 7,183. Twenty-ninth module; the graph stays acyclic. 66% below the 21,176 lines this refactor started from. 528 lines: the A/B loop strip and its drag / nudge / keyboard handling, bar-range selection, the scroll-bounds math, and snapTime — the one place a raw time becomes a snapped one (grid, or the nearest audio onset). Every module that places something on the timeline snaps through host.snapTime, which resolves here. Three main.js symbols arrive as host hooks: the seek, the snap-step query, and the Loop-in-3D button refresh. The loop-region + scroll functions and snapTime were already host hooks pointing at main.js; they resolve to the loop.js exports now. loop.js imports the A/B functions from audio.js; audio.js reaches loop's _selectedLoopRegion / _setLoopRegionEnabled through host, not import, so no cycle. Caught while wiring: updateLoopIn3DBtn was ALREADY a host hook from a prior step. My scripted add made a second one — removed the duplicate in both host.js and the setHostHooks call. (The 3-new-hook wiring itself landed correctly this time; verified by Codex before trusting it, given the same edit silently no-op'd in #176 and #179.) Also drops a dead _guideTimerSync import left in main.js when #179's _abDisarm consolidation removed its last caller. Tests: 9 loop/onset/group suites retarget their source read to src/loop.js and strip the `export` keyword before eval; loop_ab / loop_nudge_live gained host stubs for the new hooks; loop_undo_mode reads both loop.js and main.js (its @pure:pending-view block stayed behind). Several CJS -> .mjs. verify_loop.py drives editorSetLoopSnapMode end to end (persists the pref + reports through setStatus) and asserts the window.* re-attach; snapTime is covered by verify_drum. Comment out the re-attach and it throws where 90 unit tests pass. node --test 90/90, pytest 248/248, npm run lint 0 errors (6 warnings), Codex clean, all 21 headless harnesses pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
added a commit
that referenced
this pull request
Jul 10, 2026
…p.js (R2, step 28) (#180) * refactor(editor): move the loop region and scroll viewport to src/loop.js (R2, step 28) src/main.js 7,714 -> 7,183. Twenty-ninth module; the graph stays acyclic. 66% below the 21,176 lines this refactor started from. 528 lines: the A/B loop strip and its drag / nudge / keyboard handling, bar-range selection, the scroll-bounds math, and snapTime — the one place a raw time becomes a snapped one (grid, or the nearest audio onset). Every module that places something on the timeline snaps through host.snapTime, which resolves here. Three main.js symbols arrive as host hooks: the seek, the snap-step query, and the Loop-in-3D button refresh. The loop-region + scroll functions and snapTime were already host hooks pointing at main.js; they resolve to the loop.js exports now. loop.js imports the A/B functions from audio.js; audio.js reaches loop's _selectedLoopRegion / _setLoopRegionEnabled through host, not import, so no cycle. Caught while wiring: updateLoopIn3DBtn was ALREADY a host hook from a prior step. My scripted add made a second one — removed the duplicate in both host.js and the setHostHooks call. (The 3-new-hook wiring itself landed correctly this time; verified by Codex before trusting it, given the same edit silently no-op'd in #176 and #179.) Also drops a dead _guideTimerSync import left in main.js when #179's _abDisarm consolidation removed its last caller. Tests: 9 loop/onset/group suites retarget their source read to src/loop.js and strip the `export` keyword before eval; loop_ab / loop_nudge_live gained host stubs for the new hooks; loop_undo_mode reads both loop.js and main.js (its @pure:pending-view block stayed behind). Several CJS -> .mjs. verify_loop.py drives editorSetLoopSnapMode end to end (persists the pref + reports through setStatus) and asserts the window.* re-attach; snapTime is covered by verify_drum. Comment out the re-attach and it throws where 90 unit tests pass. node --test 90/90, pytest 248/248, npm run lint 0 errors (6 warnings), Codex clean, all 21 headless harnesses pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(test): correct stale src/main.js paths in the loop suites Copilot + CodeRabbit, on #180. Six stale references, all the same class: after retargeting the loop suites' source reads from main.js to loop.js (and audio.js for the @pure:loop-ab / onset-snap blocks), the comments and block-missing failure messages still named main.js. Corrected each to the actual source so a CI failure points at the right file. Verified each against where the sliced symbol now lives. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 10, 2026
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
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.
The biggest single lift of the split.
src/main.js8,760 → 7,715 — now 64% below the 21,176 lines this refactor started from.1,039 lines: the playback engine (
startPlayback/stopPlayback/playbackTick), the waveform, the onset strip, follow-scroll and the WebAudio graph, plus the guide claps, the metronome, the A/B reference loop, the per-bus mixer and the edit blip.It owns the rAF loop —
rafIdis module-scope, set byplaybackTickand cancelled byteardownAudio(). The import-time button seeding becameinitAudio(). The 8window.editor*toolbar handlers are exported and re-attached. Fivemain.jssymbols arrive as host hooks:draw/drawNow, the scroll-bounds math, and the A/B loop-region selection.Two real bugs, both found on review, neither caught by the 90 unit tests
1 — an illegal reassignment of an imported binding.
main.jswas writing_abPhaseand_abOn, which are nowimported fromaudio.jsand therefore read-only. That throws at runtime. The A/B state’s only writers outside the engine were two disarm sites; they now call a new_abDisarm()export, so every A/B state write stays insideaudio.js. Same live-binding rule as every prior step: the writer couldn’t move, so it crosses as a function.2 —
setHostHooks()was missing all 5 new hooks. My scripted edit’s anchor didn’t match and Python didn’t complain — the exact trap from #176. Soaudio.jsran againsthost.js’s inert defaults: playback advanced the cursor without repainting, follow-scroll was identity, A/B saw no region. Codex caught it.verify_audio.pynow plays in compose mode (grid-driven, no audio buffer needed) and counts canvas repaints:drawNowhook unwired (bug #2)All 90 unit tests pass in the broken state. The harness does not.
A third, latent bug
teardownAudio()completes what the old inline teardown skipped: it stops the guide/metronomesetInterval. That timer is module-scope, so it outlived a re-injected editor screen. Codex flagged the gap; fixed withS.playing = false+_guideTimerSync()+_guideCancelVoices(). (Broken out as a Fixed entry in the changelog.)Tests
~11 audio suites retarget their source read from
main.jstoaudio.jsand strip theexportkeyword before eval.loop_abandboot_teardowngained host stubs for the new hooks. Several CJS →.mjs.Verification
node --test90/90 ·pytest248/248 ·npm run lint0 errors (6 warnings) · Codex clean on three rounds · all 20 headless harnesses pass · graph acyclic (28 modules).Stacked on #178; rebased onto main after that merged.
🤖 Generated with Claude Code
Summary by CodeRabbit