refactor(app): carve the JUCE/desktop audio shims out of app.js (R3a)#893
Conversation
📝 WalkthroughWalkthroughJUCE routing, renderer audio feeding, and media-element shims move into ChangesJUCE audio and resume extraction
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant App
participant ResumeSession
participant PlayerState
participant JUCEAudio
participant DesktopAudio
App->>ResumeSession: import resume helpers
ResumeSession->>App: playSong(snapshot, resume options)
App->>PlayerState: consume pendingResume on song:ready
JUCEAudio->>DesktopAudio: inspect exclusive output
JUCEAudio->>DesktopAudio: push renderer audio or reroute transport
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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/js/loop_api.test.js (1)
19-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicated
extractFunctionacross test files.This export-stripping variant of
extractFunctionis now copy-pasted (per graph evidence) intoloop_api.test.js,song_credits_overlay.test.js,play_button_reroute_guard.test.js, andsong_restart.test.js, whiletests/js/test_utils.jsalready exports a baseextractFunction(with anopenBrace === -1guard this copy lacks). Centralizing this intest_utils.js(parameterizing export-stripping) would reduce drift risk across the R3a test suite as more modules get carved out.🤖 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_api.test.js` around lines 19 - 46, Consolidate the duplicated extractFunction implementations in the affected test files by extending the shared extractFunction in tests/js/test_utils.js with an option to strip ES-module exports before parsing. Preserve the existing brace validation, including the openBrace === -1 guard, and update loop_api.test.js, song_credits_overlay.test.js, play_button_reroute_guard.test.js, and song_restart.test.js to import and use the shared helper with export-stripping enabled.
🤖 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 `@static/js/count-in.js`:
- Around line 273-275: Update beginCount to validate the BPM returned by
highway.getBPM(loopA) using the same non-finite and non-positive fallback as
startSongCountIn. Ensure beatInterval is calculated from the fallback tempo
before scheduling count ticks, preserving normal behavior for valid BPM values.
In `@static/js/juce-audio.js`:
- Around line 869-909: Update the wantsPause-only branch in
flushJuceShimBatchNow to honor forUpcomingPlay before pausing and emitting pause
state/events. Preserve the existing pause behavior when forUpcomingPlay is
false, while skipping the pause-state reset and song:pause emission for upcoming
playback to avoid a pause→play flicker.
In `@static/js/resume-session.js`:
- Around line 108-119: Update _maybeShowResumePill so decoding snap.f cannot
throw when the saved filename contains malformed percent encoding; guard the
decodeURIComponent call and fall back to the raw filename (or the existing
default) before assigning label, while preserving the current title preference
and pill-rendering flow.
---
Nitpick comments:
In `@tests/js/loop_api.test.js`:
- Around line 19-46: Consolidate the duplicated extractFunction implementations
in the affected test files by extending the shared extractFunction in
tests/js/test_utils.js with an option to strip ES-module exports before parsing.
Preserve the existing brace validation, including the openBrace === -1 guard,
and update loop_api.test.js, song_credits_overlay.test.js,
play_button_reroute_guard.test.js, and song_restart.test.js to import and use
the shared helper with export-stripping enabled.
🪄 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: 6854d4ca-8c64-4ba1-b496-5699391bf84d
📒 Files selected for processing (23)
static/app.jsstatic/js/count-in.jsstatic/js/host.jsstatic/js/juce-audio.jsstatic/js/loops.jsstatic/js/player-controls.jsstatic/js/player-state.jsstatic/js/resume-session.jsstatic/js/section-practice.jstests/js/autoplay_exit.test.jstests/js/host_contract.test.jstests/js/juce_engine_reroute.test.jstests/js/loop_api.test.jstests/js/loop_restart.test.jstests/js/play_button_reroute_guard.test.jstests/js/playback_app_adapter.test.jstests/js/renderer_bus_feeder.test.jstests/js/section_practice_dismiss.test.jstests/js/song_credits_overlay.test.jstests/js/song_restart.test.jstests/js/song_seek.test.jstests/js/speed_reset.test.jstests/test_plugin_runtime_idempotence.py
| function beginCount() { | ||
| const bpm = highway.getBPM(loopA); | ||
| const beatInterval = 60 / bpm; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add the same BPM fallback used by startSongCountIn.
startSongCountIn guards against a non-finite / non-positive tempo (Line 344), but beginCount here does not. If highway.getBPM(loopA) returns 0, NaN, or a negative (pre-chart / malformed tempo), beatInterval becomes Infinity/NaN and setTimeout(tick, beatInterval * 1000) collapses to ~0 ms, firing all four count ticks back-to-back.
🐛 Proposed fix
function beginCount() {
- const bpm = highway.getBPM(loopA);
- const beatInterval = 60 / bpm;
+ let bpm = highway.getBPM(loopA);
+ if (!Number.isFinite(bpm) || bpm <= 0) bpm = 120;
+ const beatInterval = 60 / bpm;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function beginCount() { | |
| const bpm = highway.getBPM(loopA); | |
| const beatInterval = 60 / bpm; | |
| function beginCount() { | |
| let bpm = highway.getBPM(loopA); | |
| if (!Number.isFinite(bpm) || bpm <= 0) bpm = 120; | |
| const beatInterval = 60 / bpm; |
🤖 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 `@static/js/count-in.js` around lines 273 - 275, Update beginCount to validate
the BPM returned by highway.getBPM(loopA) using the same non-finite and
non-positive fallback as startSongCountIn. Ensure beatInterval is calculated
from the fallback tempo before scheduling count ticks, preserving normal
behavior for valid BPM values.
| function flushJuceShimBatchNow({ forUpcomingPlay = false } = {}) { | ||
| _juceShimBatchFlushScheduled = false; | ||
| const batch = _juceShimBatch; | ||
| _juceShimBatch = null; | ||
| if (!batch || !window._juceMode) return; | ||
| const wantsPause = !!batch.wantsPause; | ||
| const seekTime = batch.seekTime; | ||
| if (wantsPause && seekTime !== undefined) { | ||
| enqueue(async (gen) => { | ||
| const r = await host._audioSeek(seekTime, 'audio-element-shim'); | ||
| if (!r.completed) return; // seek cancelled by teardown | ||
| if (gen !== _juceShimGen) return; | ||
| if (!forUpcomingPlay) { | ||
| await host.jucePlayer().pause(); | ||
| if (gen !== _juceShimGen) return; | ||
| S.isPlaying = false; | ||
| host.setPlayButtonState(false); | ||
| const sm = window.feedBack; | ||
| if (sm) { | ||
| sm.isPlaying = false; | ||
| sm.emit('song:pause', host._songEventPayload()); | ||
| } | ||
| } | ||
| audio.dispatchEvent(new Event('seeked')); | ||
| }); | ||
| return; | ||
| } | ||
| if (wantsPause) { | ||
| enqueue(async (gen) => { | ||
| await host.jucePlayer().pause(); | ||
| if (gen !== _juceShimGen) return; | ||
| S.isPlaying = false; | ||
| host.setPlayButtonState(false); | ||
| const sm = window.feedBack; | ||
| if (sm) { | ||
| sm.isPlaying = false; | ||
| sm.emit('song:pause', host._songEventPayload()); | ||
| } | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and relevant symbols
git ls-files static/js/juce-audio.js
ast-grep outline static/js/juce-audio.js --view expanded
# Show the relevant section with line numbers
sed -n '820,950p' static/js/juce-audio.js
# Find all references to the batch flush helper and the flag
rg -n "flushJuceShimBatchNow|forUpcomingPlay|_juceShimBatch|song:pause|setPlayButtonState\\(false\\)" static/js/juce-audio.jsRepository: got-feedBack/feedBack
Length of output: 6969
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '950,990p' static/js/juce-audio.jsRepository: got-feedBack/feedBack
Length of output: 1507
Honor forUpcomingPlay in the pause-only batch branch. Same-tick audio.pause() + audio.play() with no seek still emits song:pause and clears S.isPlaying before the following play task, which creates a pause→play flicker for plugins. Guard this branch the same way as the pause+seek path.
🤖 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 `@static/js/juce-audio.js` around lines 869 - 909, Update the wantsPause-only
branch in flushJuceShimBatchNow to honor forUpcomingPlay before pausing and
emitting pause state/events. Preserve the existing pause behavior when
forUpcomingPlay is false, while skipping the pause-state reset and song:pause
emission for upcoming playback to avoid a pause→play flicker.
| export function _maybeShowResumePill() { | ||
| const active = document.querySelector('.screen.active'); | ||
| if (active && active.id === 'player') { _hideResumePill(); return; } | ||
| if (_resumePillDismissed) return; | ||
| const snap = _readResumeSession(); | ||
| if (!snap) { _hideResumePill(); return; } | ||
| if (document.getElementById('fb-resume-pill')) return; // already shown | ||
|
|
||
| const label = (snap.title || decodeURIComponent(snap.f || 'your last song')).toString(); | ||
| const pill = document.createElement('div'); | ||
| pill.id = 'fb-resume-pill'; | ||
| pill.setAttribute('role', 'status'); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== resume-session outline ==\n'
ast-grep outline static/js/resume-session.js --view expanded || true
printf '\n== resume-session lines 1-220 ==\n'
nl -ba static/js/resume-session.js | sed -n '1,220p'
printf '\n== search call sites ==\n'
rg -n "_maybeShowResumePill|screen:changed|decodeURIComponent\(snap\.f" static/js -SRepository: got-feedBack/feedBack
Length of output: 757
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== resume-session slice ==\n'
sed -n '1,220p' static/js/resume-session.js | cat -n
printf '\n== call sites ==\n'
rg -n "_maybeShowResumePill|screen:changed|DOMContentLoaded" static/js -SRepository: got-feedBack/feedBack
Length of output: 9077
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== search entire repo for call sites ==\n'
rg -n "_maybeShowResumePill|screen:changed|DOMContentLoaded|resume-session" . -S
printf '\n== search for try/catch around show/hide flow ==\n'
rg -n "try\s*\{|catch\s*\(|_maybeShowResumePill\(" . -SRepository: got-feedBack/feedBack
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== app.js resume pill call site ==\n'
sed -n '4438,4459p' static/app.js | cat -n
printf '\n== decodeURIComponent edge case probe ==\n'
node - <<'JS'
for (const s of ['50% Done.mp3', 'good%20name.mp3', 'plain.mp3']) {
try {
console.log(JSON.stringify(s), '=>', decodeURIComponent(s));
} catch (e) {
console.log(JSON.stringify(s), '=> THROW', e.name + ': ' + e.message);
}
}
JSRepository: got-feedBack/feedBack
Length of output: 1545
Guard decodeURIComponent before rendering the resume pill. screen:changed calls _maybeShowResumePill() without a surrounding try/catch, so a saved filename containing a bare % (for example 50% Done.mp3) will throw URIError and skip the pill render.
🤖 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 `@static/js/resume-session.js` around lines 108 - 119, Update
_maybeShowResumePill so decoding snap.f cannot throw when the saved filename
contains malformed percent encoding; guard the decodeURIComponent call and fall
back to the raw filename (or the existing default) before assigning label, while
preserving the current title preference and pill-rendering flow.
static/js/resume-session.js (157) — the snapshot taken when you leave a song and the
pill that offers it back. Bodies VERBATIM. app.js 7,727 → 7,601.
Fifth slice out of the strongly-connected core. ONE hook (playSong) + a
currentFilename getter.
S.pendingResume JOINS THE CONTAINER — on demand, exactly as intended. app.js WRITES
it (playSong({ resume }) arms it; the song:ready listener consumes it) while this
module reads it, so it cannot be a plain export: an imported binding is read-only.
Same reason isPlaying is there. The container grows one field per carve that needs
it, never speculatively.
THE CONTRACT TEST CAUGHT THE MISSING HOOK, again on a path nothing executes:
"playSong is read by a module but never wired by app.js — it would throw at runtime".
Second time it has caught a real wiring gap the moment it appeared.
A REAL TRAP, worth remembering: I first did the S.pendingResume rewrite by feeding
acorn's identifier RANGES from node into python, and it corrupted the file
(`_pS.pendingResume null;`). **Acorn's offsets are UTF-16 code units; Python's string
indices are code points.** static/app.js contains emoji, so every offset past one
drifts. Do an AST-driven rewrite in the SAME language that produced the offsets.
`node --check` caught it; a silent version of that bug is very easy to imagine.
VERIFIED. A/B against origin/main in two browsers, real song: the window API
(resumeLastSession / _snapshotResumeSession / _readResumeSession /
_clearResumeSession), snapshot, read-back, and clear — IDENTICAL, zero page errors.
HONEST LIMIT: my probe never got the snapshot to actually PERSIST (there is a guard
beyond the 3s minimum position that a scripted playSong does not satisfy), so that
path is verified only as identical-to-main, not as observed-working. The real
coverage is tests/browser/resume-session.spec.ts, which drives the flow properly.
Zero harnesses broke. 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>
static/js/juce-audio.js (994) — bodies VERBATIM. app.js 7,603 → 6,643.
THE LARGEST SINGLE SLICE of the whole carve phase: 960 lines, ~13% of what was left.
Three self-installing IIFEs:
_installJuceEngineRoutingWatcher (444) routes a song to the JUCE engine or HTML5 as
the desktop output enters/leaves exclusive/ASIO
_installRendererBusFeeder (337) feeds the highway renderer bus from whichever
transport is actually running
_installJuceAudioElementShim (156) patches audio.play/pause so the rest of the app
keeps talking to the <audio> element while JUCE
owns the transport
They EXPORT NOTHING — all three publish through `window.*` (_juceMode,
_reevaluateJuceRouting, _reevaluateRendererBus, …). So app.js needs only a
side-effect import plus the one binding it actually uses
(_resetJuceAudioShimChain, which the shim IIFE assigns).
THE ORDERING QUESTION, CHECKED RATHER THAN ASSUMED. Importing this module runs the
IIFEs EARLIER than before: imports evaluate ahead of app.js's body, and therefore
ahead of configureHost(). A hook read at IIFE-execution time would THROW. So I walked
the AST at IIFE-body depth to see what they actually touch when they run: nothing but
listener registration, and `audio.play`/`audio.pause` patching — and `audio` is itself
an imported module now. Verified in the browser: both are patched on the carved build
exactly as on main, which proves the shim installs correctly at its new, earlier point.
(Had I got this wrong, host.js throws loudly rather than silently misbehaving — which
is the whole reason it has no no-op defaults.)
VERIFIED. A/B against origin/main in two browsers: the entire window.* surface the
IIFEs publish (_juceMode, _juceOutputIsExclusive, _reevaluateJuceRouting,
_reevaluateRendererBus, _clearJuceRerouteMemo), audio.play/pause patched, a real song
loading and togglePlay driving the public mirror — IDENTICAL, zero page errors.
Harnesses: juce_engine_reroute (19 tests) + renderer_bus_feeder (13) slice the IIFEs by
signature — retargeted, and each sandbox gains a `host` object routed at its EXISTING
stubs so every assertion holds unchanged. test_plugin_runtime_idempotence is SPLIT: 3 of
its 4 source-asserts stayed in app.js, the sm.emit('song:resume') one moved.
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>
95c5bd0 to
2a048fe
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
static/js/juce-audio.js (1)
866-909: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStill missing: honor
forUpcomingPlayin the pause-only batch branch.This is the same gap flagged in a previous review and it's still present. The
wantsPause && seekTime !== undefinedbranch (Lines 876-895) correctly skips pause side-effects whenforUpcomingPlayis true, but thewantsPause-only branch (Lines 896-909) unconditionally pauses jucePlayer, flipsS.isPlaying = false, and emitssong:pause— even whenaudio.play()(Line 976) is about to immediately flush this batch withforUpcomingPlay: trueand re-flip state to playing right after. Same-tickpause()+play()with no seek still produces a pause→play flicker for plugins listening tosong:pause/song:play.🐛 Proposed fix
if (wantsPause) { - enqueue(async (gen) => { - await host.jucePlayer().pause(); - if (gen !== _juceShimGen) return; - S.isPlaying = false; - host.setPlayButtonState(false); - const sm = window.feedBack; - if (sm) { - sm.isPlaying = false; - sm.emit('song:pause', host._songEventPayload()); - } - }); + if (!forUpcomingPlay) { + enqueue(async (gen) => { + await host.jucePlayer().pause(); + if (gen !== _juceShimGen) return; + S.isPlaying = false; + host.setPlayButtonState(false); + const sm = window.feedBack; + if (sm) { + sm.isPlaying = false; + sm.emit('song:pause', host._songEventPayload()); + } + }); + } return; }🤖 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 `@static/js/juce-audio.js` around lines 866 - 909, Update the wantsPause-only branch in flushJuceShimBatchNow to honor forUpcomingPlay, matching the seek branch: when true, skip jucePlayer().pause(), playback-state mutations, and song:pause emission, while preserving the existing pause behavior when false.
🤖 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.
Duplicate comments:
In `@static/js/juce-audio.js`:
- Around line 866-909: Update the wantsPause-only branch in
flushJuceShimBatchNow to honor forUpcomingPlay, matching the seek branch: when
true, skip jucePlayer().pause(), playback-state mutations, and song:pause
emission, while preserving the existing pause behavior when false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e7f8440-f62b-42ad-bd8a-3c61453ebe78
📒 Files selected for processing (7)
static/app.jsstatic/js/juce-audio.jsstatic/js/player-state.jsstatic/js/resume-session.jstests/js/juce_engine_reroute.test.jstests/js/renderer_bus_feeder.test.jstests/test_plugin_runtime_idempotence.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_plugin_runtime_idempotence.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/js/renderer_bus_feeder.test.js
- tests/js/juce_engine_reroute.test.js
- static/js/resume-session.js
- static/app.js
The largest single slice of the whole carve phase: 960 lines, ~13% of what was left.
Three self-installing IIFEs:
_installJuceEngineRoutingWatcher_installRendererBusFeeder_installJuceAudioElementShimaudio.play/pauseso the rest of the app keeps talking to the<audio>element while JUCE owns the transportThey export nothing — all three publish through
window.*(_juceMode,_reevaluateJuceRouting,_reevaluateRendererBus, …). So app.js needs only a side-effect import, plus the one binding it actually uses (_resetJuceAudioShimChain, which the shim IIFE assigns).The ordering question — checked, not assumed
Importing this module runs the IIFEs earlier than before: imports evaluate ahead of app.js's body, and therefore ahead of
configureHost(). A hook read at IIFE-execution time would throw.So I walked the AST at IIFE-body depth to see what they actually touch when they run. The answer: nothing but listener registration, and
audio.play/audio.pausepatching — andaudiois itself an imported module now.Verified in the browser: both are patched on the carved build exactly as on main, which proves the shim installs correctly at its new, earlier point.
And note the safety property doing real work here: had I got this wrong,
host.jsthrows loudly rather than silently misbehaving. That's the whole reason it has no no-op defaults.Verification
A/B against
origin/mainin two browsers:_juceMode/_juceOutputIsExclusivefalse/undefined_reevaluateJuceRouting/_reevaluateRendererBus/_clearJuceRerouteMemoundefined(browser; desktop-only)audio.playpatched (not[native code])trueaudio.pausepatchedtruetogglePlaydrives the public mirrorHarnesses
juce_engine_reroute(19 tests) +renderer_bus_feeder(13) slice the IIFEs by signature — retargeted, and each sandbox gains ahostobject routed at its existing stubs, so every assertion holds unchanged.test_plugin_runtime_idempotenceis split: 3 of its 4 source-asserts stayed in app.js; thesm.emit('song:resume')one moved.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