refactor(app): carve count-in (and the song-credits overlay) out of app.js (R3a)#890
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughChangesCount-in and credits playback flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant CountIn
participant Host
participant Audio
App->>CountIn: startCountIn(options)
CountIn->>Host: _audioSeek(loopA)
Host->>Audio: Seek to loop A
CountIn->>Audio: Play four count-in clicks
CountIn->>Host: Update state and emit playback event
Host->>Audio: Start playback
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…pp.js (R3a) static/js/count-in.js (389) — bodies VERBATIM. app.js 8,223 → 7,913. The third slice out of the strongly-connected core, and the first that WRITES shared state rather than only reading it. #889's container is what makes it possible. imports: loops (setLoop/loopA/loopB — a count-in inside an A-B loop must begin at A), audio-el, player-state, host hooks : _audioSeek, setPlayButtonState, _songEventPayload, togglePlay + a jucePlayer getter Nothing imports count-in back — app.js and section-practice both reach it through the seam — so the graph stays acyclic. app.js's autoplay path used to reach IN and set this module's credits timers itself (_creditsTimer, _creditsHideOnPlay) and read _countingIn. It cannot now, and should not have to, so the module exports the OPERATIONS instead — armCreditsHideOnPlay(), scheduleCreditsHide(), holdCreditsThen(start), isCountingIn() — and owns its own timer invariants. Third time this has happened (section-practice's resetSelection, loops' state) and each time the constraint produced better code than was there before: the module keeps its own promises instead of trusting a caller 6,000 lines away to zero the right fields. THE no-undef GATE FOUND FIVE MISSED MEMBERS, one at a time: showSongCreditsOverlay and startSongCountIn (my name regex matched startCountIn, not startSongCountIn), then _creditLineLabel, _CREDITS_MAX_MS, and _CREDIT_ROLE_VERBS. A call-graph closure does not see a const table; only the undefined-symbol pass does. AND A REAL TRAP: I computed _CREDIT_ROLE_VERBS's span against the ALREADY-MODIFIED app.js and applied it to the clean one — the line numbers had drifted, so the slice would have cut somewhere else entirely. Recomputed every span from the clean file with acorn. Never carry line numbers across an edit. VERIFIED. A/B against origin/main in two browsers with a real song: playback state, the public feedBack.isPlaying mirror, audio position, cancel-count-in — IDENTICAL, zero page errors. Unit coverage moved with the code: loop_restart's count-in cancellation-token test and the 5 song_credits_overlay tests now read count-in.js; loop_restart's sandbox gains a `host` object routed at its EXISTING stubs, so every assertion is unchanged. HONEST LIMIT: I could not make the count-in OVERLAY actually render headlessly — its autoplay path needs a fresh-load _pendingAutostart that a scripted playSong() never arms. Behaviour is identical to main on every probe and the unit tests cover the logic, but the on-screen 1-2-3-4 and the credits card want a human look. pytest 2396, node 1040/1040, ESLint 0 (no-cycle clean), tailwind clean, Codex 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
force-pushed
the
feat/r3-countin-module
branch
from
July 11, 2026 19:53
7a4bb6d to
80abdd6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/js/loop_restart.test.js (1)
130-233: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHoist the repeated source-loading line into a single constant.
fs.readFileSync(APP_JS, 'utf8').replace(/^export /gm, '')is duplicated across four tests (lines 130, 182, 217, 233). The sibling file (tests/js/song_credits_overlay.test.js) already hoists this into a singleSRCmodule-level constant — apply the same pattern here to avoid redundant file I/O per test and keep the two files consistent.♻️ Proposed refactor
+const SRC = fs.readFileSync(APP_JS, 'utf8').replace(/^export /gm, ''); + test('loop:restart fires once when wrap path runs', async () => { - const src = fs.readFileSync(APP_JS, 'utf8').replace(/^export /gm, ''); + const src = SRC;(repeat for the other three occurrences at lines 182, 217, 233)
🤖 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 `@tests/js/loop_restart.test.js` around lines 130 - 233, Hoist the repeated APP_JS source-loading expression into a module-level SRC constant in tests/js/loop_restart.test.js, matching the pattern used by the sibling test file. Replace all four per-test fs.readFileSync(...).replace(...) assignments in the affected loop:restart tests with SRC, without changing test behavior.
🤖 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 `@tests/js/loop_restart.test.js`:
- Around line 130-233: Hoist the repeated APP_JS source-loading expression into
a module-level SRC constant in tests/js/loop_restart.test.js, matching the
pattern used by the sibling test file. Replace all four per-test
fs.readFileSync(...).replace(...) assignments in the affected loop:restart tests
with SRC, without changing test behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f14144d6-7730-4131-ae19-e7161ef29e8b
📒 Files selected for processing (4)
static/app.jsstatic/js/count-in.jstests/js/loop_restart.test.jstests/js/song_credits_overlay.test.js
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.
Third slice out of the strongly-connected core, and the first that writes shared state rather than only reading it. #889's container is what makes it possible.
Nothing imports count-in back — app.js and section-practice both reach it through the seam — so the graph stays acyclic.
The constraint produced better code again
app.js's autoplay path used to reach in and set this module's credits timers itself (
_creditsTimer,_creditsHideOnPlay) and read_countingIn. It can't now — and it shouldn't have to. So the module exports the operations:armCreditsHideOnPlay(),scheduleCreditsHide(),holdCreditsThen(start),isCountingIn().That's the third time this has happened (section-practice's
resetSelection, loops' state ownership), and each time the constraint produced something better than what was there: the module keeps its own promises, instead of trusting a caller 6,000 lines away to zero the right fields.The
no-undefgate found five missed members, one at a timeshowSongCreditsOverlayandstartSongCountIn(my name regex matchedstartCountIn, notstartSongCountIn), then_creditLineLabel,_CREDITS_MAX_MS,_CREDIT_ROLE_VERBS.A call-graph closure doesn't see a
consttable. Only the undefined-symbol pass does. This is why every carve ends with it.And a real trap worth recording
I computed
_CREDIT_ROLE_VERBS's span against the already-modified app.js and applied it to the clean one. The line numbers had drifted — the slice would have cut somewhere else entirely. Recomputed every span from the clean file with acorn.Never carry line numbers across an edit.
Verification
A/B against
origin/mainin two browsers with a real song: playback state, the publicfeedBack.isPlayingmirror, audio position, cancel-count-in — identical, zero page errors.Unit coverage moved with the code:
loop_restart's count-in cancellation-token test and the 5song_credits_overlaytests now readcount-in.js;loop_restart's sandbox gains ahostobject routed at its existing stubs, so every assertion is unchanged.Honest limit
I could not make the count-in overlay actually render headlessly — its autoplay path needs a fresh-load
_pendingAutostartthat a scriptedplaySong()never arms. Behaviour is identical to main on every probe and the unit tests cover the logic, but the on-screen 1-2-3-4 and the credits card want a human look.pytest 2396 · node 1040/1040 · ESLint 0 (no-cycle clean) · tailwind clean · Codex 0.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests