refactor(app): carve the playback transport out of app.js — and retire 8 host hooks (R3a)#894
Merged
Merged
Conversation
Contributor
|
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 (10)
📝 WalkthroughWalkthroughPlayback and seek responsibilities moved from ChangesPlayback and audio module refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant Transport
participant JUCE
participant feedBack
App->>Transport: togglePlay or _audioSeek
Transport->>JUCE: play, pause, or seek
Transport->>feedBack: emit playback or seek event
Transport-->>App: return transport state
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 |
…E 8 host hooks (R3a)
static/js/transport.js (377) — bodies VERBATIM. app.js 6,643 → 6,316.
THIS IS THE FIRST CARVE THAT SUBTRACTS HOOKS INSTEAD OF ADDING THEM.
Every carve before this one added host hooks: a module pulled out of app.js still had
to call back into it. But four modules were all reaching through the seam for the SAME
handful of names — _audioSeek, _audioTime, setPlayButtonState, _songEventPayload,
jucePlayer. Those names have an owner, and it isn't app.js. Give them one, and the
consumers import them directly:
count-in.js 5 hooks -> 0 (host import deleted)
juce-audio.js 4 hooks -> 0 (host import deleted)
loops.js 6 hooks -> 4
section-practice.js 10 hooks -> 7
----------------------------------------------------------
configureHost() 20 hooks -> 12
A hook is a cycle you agreed to live with. An import is a dependency you actually have.
Prefer the import whenever the name has a real owner.
_audioSeekGen now stays PRIVATE. It has exactly one writer — _resetAudioSeekState(),
which moved with it — so readers get audioSeekGen() and nobody outside can desync it.
Strictly better than the hook it replaces, which handed out a getter and left the writer
behind in app.js.
THE SCAN HAD A HOLE, AND IT BIT. Picking the carve by dependency closure over app.js's
own top-level decls said this cluster was downward-closed. It wasn't:
_currentPlaybackSnapshot reads loopA/loopB — which live in ./js/loops.js, and loops.js
imports transport. The scan saw nothing, because loopA STOPPED BEING an app.js decl the
moment loops.js was carved out. Any dependency scan of a partly-carved monolith has to
resolve the imports too, or it will confidently hand you a cycle. Added that pass; it
found exactly one back-edge, and _currentPlaybackSnapshot stays in app.js (as does
restartCurrentSong, which calls _cancelCountIn). app.js is the root — it imports both
sides for free.
TESTS. Four harnesses retargeted (play_button_reroute_guard, song_event_payload,
song_seek -> transport.js; playback_app_adapter SPLIT, since
_installPlaybackTransportAdapter stayed behind).
The two CENSUS tests — "≥8 song:* emit sites", "every seek callsite passes a reason" —
now scan app.js AND every static/js/*.js, not one file. Pointed at a single file, their
count silently shrinks as code leaves, which reads as "someone deleted an emit" or, worse,
passes while genuinely missing sites. Both bite-tested: stripping a _songEventPayload()
from an emit and adding a reason-less _audioSeek() each fail the suite.
VERIFIED. A/B against origin/main, real song, real playback: song:play payload is exactly
{audioT, chartT, perfNow, time}; song:seek carries reason "seek-by" with finite from/to;
all five song:* events fire; seekBy advances the clock; restartCurrentSong returns to zero;
the play button's aria-pressed tracks state. IDENTICAL on all 21 probes, zero page errors.
pytest 2396, node 1040/1040, host contract 2/2, ESLint 0 (no-cycle clean), Codex 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
force-pushed
the
feat/r3-carve-transport
branch
from
July 11, 2026 20:44
4b677c5 to
b1a2d60
Compare
4 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
1 participant
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.
This is the first carve that subtracts hooks instead of adding them
Every carve before this one added host hooks: a module pulled out of app.js still had to call back into it. But four modules were all reaching through the seam for the same handful of names —
_audioSeek,_audioTime,setPlayButtonState,_songEventPayload,jucePlayer. Those names have an owner, and it isn't app.js.Give them one, and the consumers import them directly:
count-in.jshostimport deletedjuce-audio.jshostimport deletedloops.jssection-practice.jsconfigureHost()A hook is a cycle you agreed to live with. An import is a dependency you actually have. Prefer the import whenever the name has a real owner.
_audioSeekGennow stays private. It has exactly one writer —_resetAudioSeekState(), which moved with it — so readers getaudioSeekGen()and nobody outside can desync it. Strictly better than the hook it replaces, which handed out a getter and left the writer behind in app.js.The dependency scan had a hole, and it bit
Picking the carve by dependency closure over app.js's own top-level decls said this cluster was downward-closed. It wasn't.
_currentPlaybackSnapshotreadsloopA/loopB— which live in./js/loops.js, and loops.js imports transport.The scan saw nothing, because
loopAstopped being an app.js decl the moment loops.js was carved out. Any dependency scan of a partly-carved monolith has to resolve the imports too, or it will confidently hand you a cycle.I added that pass. It found exactly one back-edge. So
_currentPlaybackSnapshotstays in app.js — as doesrestartCurrentSong, which calls_cancelCountIn(). app.js is the root; it imports both sides for free.Tests
Four harnesses retargeted:
play_button_reroute_guard,song_event_payload,song_seek→transport.js;playback_app_adapteris split, since_installPlaybackTransportAdapterstayed behind.The two census tests — "≥8
song:*emit sites" and "every seek callsite passes a reason" — now scan app.js and everystatic/js/*.js, not one file. Pointed at a single file, their count silently shrinks as code leaves, which reads as "someone deleted an emit" — or, worse, passes while genuinely missing sites. Both bite-tested: stripping a_songEventPayload()from an emit, and adding a reason-less_audioSeek(), each fail the suite.Verification
A/B against
origin/main— real song, real playback, identical on all 21 probes, zero page errors:song:playpayload keysaudioT, chartT, perfNow, timesong:seekreason: "seek-by", finitefrom/tosong:*events firedplay,pause,seek,position-changed,ready)seekBy(5)restartCurrentSong()aria-pressedpytest 2396 · node 1040/1040 · host contract 2/2 · ESLint 0 (no-cycle clean) · Codex 0.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests