refactor(app): carve the A–B loop out of app.js (R3a)#888
Conversation
a56119e to
5afba10
Compare
|
Warning Review limit reached
Next review available in: 12 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)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
static/js/loops.js (261) — bodies VERBATIM. app.js 8,421 → 8,224.
The second slice out of the strongly-connected core.
It OWNS the loop state — loopA, loopB, _loopMutationGen. Nothing outside writes
them: restartCurrentSong() looked like it did, but it declares its own local `let
loopA/loopB` shadows, so the module-level bindings only ever change in setLoop /
setLoopStart / setLoopEnd / clearLoop. All four move here. No state container.
DIRECTION IS THE WHOLE DESIGN. loops and section-practice are mutually dependent —
the SCC in miniature. clearLoop() must drop section-practice's selection, and
practiceSection() must call setLoop(). Both edges cannot be imports or no-cycle
(rightly) rejects it. So:
section-practice -> reaches loops through the HOST SEAM (host.setLoop, …)
loops -> imports section-practice DIRECTLY
section-practice is the higher-level feature — a consumer of loops, not the reverse
— so it is the one that takes the indirection. app.js hands the loop module's
exports across into the seam for it. Graph stays acyclic; no-cycle passes.
THE CONTRACT TEST EARNED ITS KEEP IMMEDIATELY. It failed on the first build with
"these hooks are wired by app.js but no module reads them: playSong". My dependency
scan had counted a mention of playSong() inside a COMMENT in loops.js as a real
call. Wired but unused is precisely the "fossil of a rename" case the test exists
for — and it caught it on a path no test executes.
VERIFIED BY DRIVING BOTH SIDES OF THE SEAM. A/B against origin/main in two browsers,
real song loaded:
* setLoop(5,12) -> true; getLoop() -> 5,12 — IDENTICAL
* clearLoop() (loops -> section-practice, a direct import) -> getLoop() ->
null,null — IDENTICAL
* onPhraseNext() (section-practice -> loops, ACROSS THE SEAM) -> ok — IDENTICAL
* loadSavedLoop / saveCurrentLoop / deleteSelectedLoop on window — IDENTICAL
* zero page errors either side. An unwired hook throws, so a live app is itself
proof the seam is wired.
Harness: loop_api extracts the loop helpers by signature — retargeted to loops.js,
`export` stripped for the vm sandbox, and the sandbox's existing _audioSeek /
_audioTime / formatTime spies are now routed through a `host` object so every
assertion holds unchanged, just through the indirection the real code uses. It is
SPLIT: one test still reads app.js for the window.feedBack API surface, which stayed.
pytest 2396, node 1040/1040, ESLint 0 (no-cycle clean), tailwind clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5afba10 to
33de8b7
Compare
1 participant
Second slice out of the strongly-connected core.
It owns the loop state
loopA,loopB,_loopMutationGen. Nothing outside writes them —restartCurrentSong()looked like it did, but it declares its own locallet loopA/loopBshadows, so the module-level bindings only ever change insetLoop/setLoopStart/setLoopEnd/clearLoop. All four move here. No state container needed.Direction is the whole design
loopsandsection-practiceare mutually dependent — the SCC in miniature.clearLoop()must drop section-practice's selection;practiceSection()must callsetLoop(). Both edges cannot be imports, orno-cycle(rightly) rejects it. So:section-practice is the higher-level feature — a consumer of loops, not the reverse — so it's the one that takes the indirection. app.js hands the loop module's exports across into the seam for it. The graph stays acyclic.
The contract test earned its keep immediately
It failed on the very first build:
My dependency scan had counted a mention of
playSong()inside a comment in loops.js as a real call. "Wired but unused" is exactly the case the test exists for — and it caught it on a path no test executes. That's the whole point of the static layer.Verified by driving both sides of the seam
A/B against
origin/mainin two browsers, real song loaded:setLoop(5,12)→getLoop()5,12clearLoop()→getLoop()(loops → section-practice, direct import)null,nullonPhraseNext()(section-practice → loops, across the seam)loadSavedLoop/saveCurrentLoop/deleteSelectedLoopon windowfunctionSince an unwired hook throws, a live app is itself proof the seam is wired.
Harness
loop_apiextracts the loop helpers by signature — retargeted toloops.js,exportstripped for the vm sandbox, and its existing_audioSeek/_audioTime/formatTimespies are now routed through ahostobject so every assertion holds unchanged, just through the indirection the real code uses. It's split: one test still reads app.js for thewindow.feedBackAPI surface, which stayed.pytest 2396 · node 1040/1040 · ESLint 0 (no-cycle clean) · tailwind clean.
🤖 Generated with Claude Code