fix(editor): leaving the Song Editor stops its playback - #326
Conversation
|
Warning Review limit reached
Next review available in: 22 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 (3)
📝 WalkthroughWalkthroughThe Song Editor now stops playback and finalizes active MIDI recording when hidden. Its observer distinguishes real visibility transitions, preserves re-show behavior, and tests cover teardown errors and hide/show cycles. ChangesSong Editor teardown
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant SongEditor
participant MutationObserver
participant TeardownHandler
participant MIDIRecorder
participant AudioPlayback
SongEditor->>MutationObserver: active state changes
MutationObserver->>TeardownHandler: active to inactive transition
TeardownHandler->>MIDIRecorder: finalize MIDI take
TeardownHandler->>AudioPlayback: stop playback
MutationObserver->>SongEditor: inactive to active transition
SongEditor->>SongEditor: resize and schedule landing
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main.js (1)
2163-2165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the existing
screenreference instead of re-querying the DOM.Since the
MutationObserveris attached directly to thescreenelement, you can safely use thescreenreference captured in the closure. This avoids an unnecessary DOM query and ensures you are checking the exact element that triggered the mutation.♻️ Proposed refactor
- const el = document.getElementById('plugin-editor'); - if (!el) return; - const active = el.classList.contains('active'); + const active = screen.classList.contains('active');🤖 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 2163 - 2165, Update the MutationObserver callback around the plugin-editor handling to use the captured screen reference instead of calling document.getElementById('plugin-editor'). Preserve the existing null guard and active-class check while removing the redundant DOM query.
🤖 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 `@src/main.js`:
- Around line 2163-2165: Update the MutationObserver callback around the
plugin-editor handling to use the captured screen reference instead of calling
document.getElementById('plugin-editor'). Preserve the existing null guard and
active-class check while removing the redundant DOM query.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a566927-cca1-46de-8360-ffbdd9d8d104
📒 Files selected for processing (3)
CHANGELOG.mdsrc/main.jstests/leaving_stops_playback.test.mjs
Playback running when you navigated away just kept going. Leave the editor mid-play, start an actual song, and you get two mixes on top of each other. The editor already had a teardown that stops audio — but it only ran at the top of a NEW injection (main.js checks window.__editorScreenTeardown before registering its own globals). So it fired when you came BACK to the editor, never when you left. The host hides a screen by dropping its `active` class and does not re-run the module, so nothing was silencing anything. There was already a MutationObserver on #plugin-editor's class, handling the show (resize + entry landing). It just had no else. It does now. Deliberately NOT the full __editorScreenTeardown on hide: the host can re-show the same injection without re-running the module, and a full teardown would strip the global listeners and leave a dead editor behind. Stopping is what the Stop button does, so the session stays resumable. An in-flight MIDI take is finalized rather than discarded — held notes capped, device session released — via the existing editorStopRecordMidi, which is guarded and a no-op when idle. Both calls are wrapped so a failure in either can never block navigation. The observer only acts on a real active<->inactive transition: attributeFilter is `class`, not one specific class, so any class churn on #plugin-editor wakes it, and without the guard this would re-fire on every such mutation. Verified in a browser, same probe against both trees, by reading the transport clock while the (hidden) editor DOM is still mounted: main: after leaving 0:04.240 -> 0:06.259 still running fix: after leaving 0:02.720 -> 0:02.720 frozen Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix
0c03fb9 to
75c8007
Compare
1 participant
The bug
Start playback in the editor, navigate away without pausing, and it keeps playing — right over whatever you go to next. Reported by Christian: "the playback continues as I leave the editor menu, even following me into playing an actual in-game song."
Why
The editor already had a teardown that stops audio (
window.__editorScreenTeardown→teardownAudio()). It just ran at the wrong time:main.jsinvokes it at the top of a new injection, before registering its own globals. So it fired when you came back to the editor, never when you left.The host hides a screen by dropping its
activeclass (session.js:querySelectorAll('.screen').forEach(s => s.classList.remove('active'))) and does not re-run the plugin module. The Web Audio graph and the rAF transport both outlive the screen's DOM, so nothing was silencing anything.There was already a
MutationObserveron#plugin-editor's class, handling the show (resize + entry landing). It had noelse.The fix
An
_editorOnScreenHidden()on the hide transition:editorStopRecordMidi()(no-op when idle).stopPlayback()as the catch-all: main source, stems, reference media, rAF, guide voices.Deliberately not the full teardown. The host can re-show the same injection without re-running the module; a full teardown would strip the global listeners and leave a dead editor behind. Stopping is exactly what the Stop button does, so the session stays intact and resumable.
The observer only acts on a real
active↔inactivetransition.attributeFilterisclass, not one specific class, so any class change on#plugin-editorwakes it — without the guard this would re-firestopPlaybackon every such mutation.Verification
Browser-verified against both trees with the same probe, same song, by reading the transport clock while the hidden editor DOM is still mounted:
0:04.240→0:06.2590:02.720→0:02.720tests/leaving_stops_playback.test.mjs(7 cases) brace-extracts the real handler fromsrc/main.js— the file is the entry orchestrator and exports nothing, so that's the established way in, same astests/boot_teardown.test.js. It covers: both stops fire; a throwing take-stop still stops playback; a throwingstopPlaybackdoesn't escape; hide silences; show does not silence; repeated class churn doesn't re-fire; and a hide→show cycle still works.The wiring guard fails on main with "the screen observer must call
_editorOnScreenHiddenwhen the editor stops being active".226/226 JS tests pass; lint clean (3 pre-existing warnings in
create.js/main.js).Not covered here
The drum-pad strip's MIDI monitor tap stays armed while hidden, so a connected pad controller can still trigger voices from another screen. That's a narrower leak with a real regression risk if untapped on hide (coming back without re-injection would leave the strip dead), so it wants its own change.
🤖 Generated with Claude Code
https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix
Summary by CodeRabbit
Bug Fixes
Tests