refactor(app): carve the player controls out of app.js (R3a)#891
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughPlayback preference, speed, and mastery controls are extracted into ChangesPlayback controls
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
static/app.js (1)
1090-1113: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClear the public playback mirror during teardown.
During an active reroute, the audio pause listener intentionally suppresses pause events. If
showScreen()orplaySong()tears down the HTML5 route in that window, these paths clearS.isPlayingbut can leavewindow.feedBack.isPlaying === true. Explicitly clear the mirror in the teardown path after pausing.Proposed fix
audio.pause(); audio.src = ''; +if (window.feedBack) window.feedBack.isPlaying = false; S.isPlaying = false;Also applies to: 5680-5696
🤖 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/app.js` around lines 1090 - 1113, Update the shared teardown path around audio.pause() to explicitly set window.feedBack.isPlaying to false after pausing, alongside the existing S.isPlaying reset. Apply the same mirror reset in the corresponding teardown path around the referenced alternate location, while preserving existing event suppression and cleanup behavior.
🧹 Nitpick comments (1)
static/js/loops.js (1)
47-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
loop-settransport-event payload construction.
setLoopEnd(Line 56) andsetLoop(Line 160) build byte-identical{requesterId, loopA, loopB, loop:{...}}payloads for the'loop-set'event. Worth extracting a small payload helper so the two call sites can't drift when the shape changes.♻️ Proposed refactor
+function _loopSetEventPayload() { + return { requesterId: 'core.loop', loopA, loopB, loop: { startTime: loopA, endTime: loopB, enabled: true, state: 'active' } }; +} + export function setLoopEnd() { ... - window.feedBack?.playback?.transportEvent?.('loop-set', { requesterId: 'core.loop', loopA, loopB, loop: { startTime: loopA, endTime: loopB, enabled: true, state: 'active' } }); + window.feedBack?.playback?.transportEvent?.('loop-set', _loopSetEventPayload()); } ... if (emitTransportEvent && typeof window !== 'undefined') { - window.feedBack?.playback?.transportEvent?.('loop-set', { requesterId: 'core.loop', loopA, loopB, loop: { startTime: loopA, endTime: loopB, enabled: true, state: 'active' } }); + window.feedBack?.playback?.transportEvent?.('loop-set', _loopSetEventPayload()); }Also applies to: 118-163
🤖 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/loops.js` around lines 47 - 57, Extract the shared loop-set event payload construction from setLoopEnd and setLoop into a small helper in the loop module. Update both call sites to use that helper while preserving the existing requesterId, loopA, loopB, and nested loop fields exactly.
🤖 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/app.js`:
- Around line 3779-3781: Update the playback-start flows around audio.play() and
the corresponding jucePlayer.play() calls to re-check S.isPlaying after each
asynchronous transport start completes. If playback is no longer intended, stop
the newly started transport and avoid committing the route as actively playing;
apply the same guard to all three referenced start paths.
In `@static/js/count-in.js`:
- Around line 284-294: Update the JUCE playback promise handling in the
_juceMode branch so both a false play() result and a rejected play() promise
reset S.isPlaying to false, matching the existing HTML5 failure behavior while
preserving the current success flow and error logging.
- Around line 184-189: Update both count-in pause paths around the JUCE and
non-JUCE branches to detect pause failure, abort the count-in, and reset
_countingIn before returning. Ensure JUCE pause rejections are not merely
logged, while preserving the existing generation teardown check after a
successful pause.
- Around line 228-268: The _audioSeek() promise handling in the loop restart
callback must also process rejections. Update the chain around _audioSeek,
identified by the callback containing the !r.completed check, so rejected seeks
enter the same abort/reset path that clears _countingIn and synchronizes paused
transport state, while preserving the existing successful-seek validation and
beginCount flow.
In `@static/js/loops.js`:
- Around line 41-57: Update setLoopStart() to clear loopB and reset the
end-button UI when the newly assigned loopA is after the existing endpoint,
preventing inverted ranges from remaining active. Update setLoopEnd() to also
clear/reset the B-button styling when loopB <= loopA before returning, while
preserving valid loop setup and transport-event behavior.
In `@static/js/player-controls.js`:
- Around line 170-175: Update the debounced settings persistence fetch in
setMastery() to inspect the returned response and reject when response.ok is
false, ensuring HTTP 4xx/5xx responses reach the existing catch handler while
preserving the current best-effort retry behavior.
In `@tests/js/host_contract.test.js`:
- Around line 56-60: Update the host-import filter in the JS directory scan to
recognize both single- and double-quoted `./host.js` imports, while preserving
the existing file exclusions and hook scanning via `HOOK_RE`.
---
Outside diff comments:
In `@static/app.js`:
- Around line 1090-1113: Update the shared teardown path around audio.pause() to
explicitly set window.feedBack.isPlaying to false after pausing, alongside the
existing S.isPlaying reset. Apply the same mirror reset in the corresponding
teardown path around the referenced alternate location, while preserving
existing event suppression and cleanup behavior.
---
Nitpick comments:
In `@static/js/loops.js`:
- Around line 47-57: Extract the shared loop-set event payload construction from
setLoopEnd and setLoop into a small helper in the loop module. Update both call
sites to use that helper while preserving the existing requesterId, loopA,
loopB, and nested loop fields exactly.
🪄 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: 09d34fca-bd97-402c-ae41-4de11350042a
📒 Files selected for processing (20)
static/app.jsstatic/js/count-in.jsstatic/js/host.jsstatic/js/loops.jsstatic/js/player-controls.jsstatic/js/player-state.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/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
Comment on lines
+3779
to
3781
| if (S.isPlaying && !_isStale(songAudio)) { | ||
| if (!audio.src) { audio.src = url; audio.load(); } | ||
| try { await audio.play(); } catch (_) { /* ignore */ } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Re-check playback intent after the asynchronous transport start.
S.isPlaying can change while audio.play() or jucePlayer.play() is pending. If the user presses Pause during that await, the start may still succeed and the route can be committed with audio running while S.isPlaying is false. Add a post-start intent check and stop the newly started transport when playback is no longer wanted.
Proposed guard
const started = await jucePlayer.play();
if (started === false) {
...
}
+if (!S.isPlaying) {
+ await jucePlayer.pause().catch(() => {});
+}Also applies to: 3808-3814, 3923-3928
🤖 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/app.js` around lines 3779 - 3781, Update the playback-start flows
around audio.play() and the corresponding jucePlayer.play() calls to re-check
S.isPlaying after each asynchronous transport start completes. If playback is no
longer intended, stop the newly started transport and avoid committing the route
as actively playing; apply the same guard to all three referenced start paths.
Comment on lines
+184
to
+189
| if (window._juceMode) { | ||
| await host.jucePlayer().pause().catch((err) => console.error('[app] host.jucePlayer().pause error in count-in:', err)); | ||
| } else { | ||
| audio.pause(); | ||
| } | ||
| if (gen !== _countInGen) return; // teardown during pause |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Abort count-in when JUCE pause fails.
The rejection is only logged, then count-in proceeds while the engine may still be playing. Abort and reset _countingIn in both paths instead of continuing after a failed pause.
Also applies to: 335-340
🤖 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 184 - 189, Update both count-in pause
paths around the JUCE and non-JUCE branches to detect pause failure, abort the
count-in, and reset _countingIn before returning. Ensure JUCE pause rejections
are not merely logged, while preserving the existing generation teardown check
after a successful pause.
Comment on lines
+228
to
+268
| host._audioSeek(loopA, 'loop-wrap').then((r) => { | ||
| if (gen !== _countInGen) return; // teardown during seek | ||
| // Abort the loop restart in two cases: | ||
| // 1. Cancelled (player torn down): don't beginCount on a | ||
| // new session. | ||
| // 2. Off-target landing (JUCE rollback / clamp far from | ||
| // loopA): proceeding would emit loop:restart and start | ||
| // a count-in from the wrong position. Audio is at | ||
| // r.from / r.to, which is not where the loop wants to | ||
| // resume — better to drop this iteration than play out | ||
| // of sync. | ||
| // 50 ms tolerance: well within JUCE's normal seek precision | ||
| // but tight enough to catch a real rollback or no-op. | ||
| if (!r.completed || Math.abs(r.to - loopA) > 0.05) { | ||
| // startCountIn paused audio at entry but left isPlaying | ||
| // alone — beginCount would have set it on resume. On | ||
| // abort, sync the transport: audio is paused, so | ||
| // isPlaying must reflect that and the button + plugin | ||
| // host must agree. | ||
| _countingIn = false; | ||
| if (S.isPlaying) { | ||
| S.isPlaying = false; | ||
| host.setPlayButtonState(false); | ||
| if (window.feedBack) { | ||
| window.feedBack.isPlaying = false; | ||
| window.feedBack.emit('song:pause', host._songEventPayload()); | ||
| } | ||
| } | ||
| return; | ||
| } | ||
| // Use the verified post-seek clock for the chart so audio | ||
| // and chart stay in sync if JUCE clamped to slightly | ||
| // before/after loopA. The loop:restart event keeps `time: | ||
| // loopA` because subscribers treat that as the semantic | ||
| // marker for "new iteration starts at A", not the actual | ||
| // audio position. | ||
| S.lastAudioTime = r.to; | ||
| highway.setTime(r.to); | ||
| window.feedBack.emit('loop:restart', { loopA, loopB, time: loopA }); | ||
| beginCount(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the reported section with line numbers.
sed -n '180,320p' static/js/count-in.js | cat -n
# Find all references to the latch and seek call.
rg -n "_countingIn|_audioSeek|beginCount|loop-wrap" static/js/count-in.jsRepository: got-feedBack/feedBack
Length of output: 8548
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the startCountIn path around the reported branch.
sed -n '160,210p' static/js/count-in.js | cat -n
# Read the other count-in path and any reset logic near the bottom.
sed -n '320,390p' static/js/count-in.js | cat -n
# Search for promise rejection handling around _audioSeek and count-in state.
rg -n "\.catch\(|try \{|finally|_countingIn = false|_audioSeek\(" static/js/count-in.jsRepository: got-feedBack/feedBack
Length of output: 6966
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the count-in helpers and cancellation/reset path.
sed -n '1,120p' static/js/count-in.js | cat -nRepository: got-feedBack/feedBack
Length of output: 6647
Handle _audioSeek() rejection here. If the seek promise rejects, this branch never clears _countingIn or syncs transport state, so later count-ins short-circuit and playback can get stuck. Route the rejection through the same abort/reset path as an unsuccessful seek.
🤖 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 228 - 268, The _audioSeek() promise
handling in the loop restart callback must also process rejections. Update the
chain around _audioSeek, identified by the callback containing the !r.completed
check, so rejected seeks enter the same abort/reset path that clears _countingIn
and synchronizes paused transport state, while preserving the existing
successful-seek validation and beginCount flow.
Comment on lines
+284
to
+294
| if (window._juceMode) { | ||
| host.jucePlayer().play().then((started) => { | ||
| if (gen !== _countInGen) return; // teardown during play start | ||
| if (!started) return; | ||
| S.isPlaying = true; | ||
| host.setPlayButtonState(true); | ||
| window.feedBack.isPlaying = true; | ||
| const payload = host._songEventPayload(); | ||
| window.feedBack.emit('song:play', payload); | ||
| window.feedBack.emit('song:resume', payload); | ||
| }).catch((err) => console.error('[app] host.jucePlayer().play error:', err)); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Synchronize state when JUCE playback fails.
When play() resolves false or rejects, the code hides the overlay but leaves S.isPlaying unchanged. During loop restart this can leave the UI and plugin believing playback is active while JUCE is paused; mirror the HTML5 failure path for both outcomes.
🤖 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 284 - 294, Update the JUCE playback
promise handling in the _juceMode branch so both a false play() result and a
rejected play() promise reset S.isPlaying to false, matching the existing HTML5
failure behavior while preserving the current success flow and error logging.
Comment on lines
+41
to
+57
| export function setLoopStart() { | ||
| loopA = host._audioTime(); | ||
| document.getElementById('btn-loop-a').className = 'px-3 py-1.5 bg-green-900/50 rounded-lg text-xs text-green-300 transition'; | ||
| updateLoopUI(); | ||
| } | ||
|
|
||
| export function setLoopEnd() { | ||
| if (loopA === null) return; | ||
| loopB = host._audioTime(); | ||
| if (loopB <= loopA) { loopB = null; return; } | ||
| document.getElementById('btn-loop-b').className = 'px-3 py-1.5 bg-green-900/50 rounded-lg text-xs text-green-300 transition'; | ||
| updateLoopUI(); | ||
| // Manual A/B arming is a loop mutation like setLoop()'s — emit the same | ||
| // transport event so event-driven consumers (note_detect drill sync) see | ||
| // button-armed loops without having to poll getLoop(). | ||
| window.feedBack?.playback?.transportEvent?.('loop-set', { requesterId: 'core.loop', loopA, loopB, loop: { startTime: loopA, endTime: loopB, enabled: true, state: 'active' } }); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg '(^|/)(static/js/loops\.js|static/js/app\.js|tests/js/loop_api\.test\.js|lib/routers/loops\.py)$'Repository: got-feedBack/feedBack
Length of output: 225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in static/js/loops.js static/js/app.js tests/js/loop_api.test.js lib/routers/loops.py; do
if [ -f "$f" ]; then
echo "### $f"
wc -l "$f"
fi
doneRepository: got-feedBack/feedBack
Length of output: 314
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## loops.js"
ast-grep outline static/js/loops.js --view expanded || true
echo
echo "## loops.js excerpt"
sed -n '1,320p' static/js/loops.js | cat -n
echo
echo "## app.js search"
rg -n "loopA|loopB|setLoopStart|setLoopEnd|updateLoopUI|feedBack|transportEvent|saveCurrentLoop|getLoop|hasLoop" static/js/app.js static/js/loops.js
echo
echo "## tests"
sed -n '1,260p' tests/js/loop_api.test.js | cat -n
echo
echo "## backend loops"
sed -n '1,260p' lib/routers/loops.py | cat -nRepository: got-feedBack/feedBack
Length of output: 18354
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### static/js/loops.js"
ast-grep outline static/js/loops.js --view expanded || true
echo
sed -n '1,320p' static/js/loops.js | cat -n
echo
echo "### static/js/app.js search"
rg -n "loopA|loopB|setLoopStart|setLoopEnd|updateLoopUI|feedBack|transportEvent|saveCurrentLoop|getLoop|hasLoop" static/js/app.js static/js/loops.js
echo
echo "### tests/js/loop_api.test.js"
sed -n '1,260p' tests/js/loop_api.test.js | cat -n
echo
echo "### lib/routers/loops.py"
sed -n '1,260p' lib/routers/loops.py | cat -nRepository: got-feedBack/feedBack
Length of output: 18356
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### lib/routers/loops.py"
sed -n '1,220p' lib/routers/loops.py | cat -n
echo
echo "### tests/js/loop_api.test.js search"
rg -n "setLoopStart|setLoopEnd|setLoop\\(|clearLoop|updateLoopUI|saveCurrentLoop|loopA|loopB" tests/js/loop_api.test.jsRepository: got-feedBack/feedBack
Length of output: 6071
Manual Set A can leave a stale end point and persist an inverted loop
setLoopStart()should clear a previousloopBwhen the new A lands after it; otherwise the UI keeps treating the backward range as active, andsaveCurrentLoop()will POSTstart > end.setLoopEnd()also returns early onloopB <= loopAwithout resetting the B button’s armed styling, so a prior valid loop can leave stale green state behind.
🤖 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/loops.js` around lines 41 - 57, Update setLoopStart() to clear
loopB and reset the end-button UI when the newly assigned loopA is after the
existing endpoint, preventing inverted ranges from remaining active. Update
setLoopEnd() to also clear/reset the B-button styling when loopB <= loopA before
returning, while preserving valid loop setup and transport-event behavior.
Comment on lines
+170
to
+175
| fetch('/api/settings', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ master_difficulty: pct }), | ||
| }).catch(() => { /* best-effort — next setMastery() will retry */ }); | ||
| }, 300); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Handle non-2xx settings responses as failures.
fetch() resolves for HTTP 4xx/5xx responses, so the current .catch() is skipped and mastery persistence can silently fail. Check response.ok and reject before the catch handler.
Proposed fix
fetch('/api/settings', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ master_difficulty: pct }),
})
+ .then(response => {
+ if (!response.ok) {
+ throw new Error(`settings POST failed: ${response.status}`);
+ }
+ })
.catch(() => { /* best-effort — next setMastery() will retry */ });📝 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.
Suggested change
| fetch('/api/settings', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ master_difficulty: pct }), | |
| }).catch(() => { /* best-effort — next setMastery() will retry */ }); | |
| }, 300); | |
| fetch('/api/settings', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ master_difficulty: pct }), | |
| }) | |
| .then(response => { | |
| if (!response.ok) { | |
| throw new Error(`settings POST failed: ${response.status}`); | |
| } | |
| }) | |
| .catch(() => { /* best-effort — next setMastery() will retry */ }); | |
| }, 300); |
🤖 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/player-controls.js` around lines 170 - 175, Update the debounced
settings persistence fetch in setMastery() to inspect the returned response and
reject when response.ok is false, ensuring HTTP 4xx/5xx responses reach the
existing catch handler while preserving the current best-effort retry behavior.
Comment on lines
+56
to
+60
| for (const file of fs.readdirSync(JS_DIR)) { | ||
| if (!file.endsWith('.js') || file === 'host.js') continue; | ||
| const raw = fs.readFileSync(path.join(JS_DIR, file), 'utf8'); | ||
| if (!/from\s+'\.\/host\.js'/.test(raw)) continue; | ||
| for (const m of scrub(raw).matchAll(HOOK_RE)) { |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make host-import detection quote-agnostic.
This condition only recognizes from './host.js'. A valid module using from "./host.js" is skipped entirely, so its hook usage cannot be checked for missing wiring. Match both quote styles, or parse imports instead of filtering with this single regex.
Proposed fix
- if (!/from\s+'\.\/host\.js'/.test(raw)) continue;
+ if (!/from\s+(['"])\.\/host\.js\1/.test(raw)) continue;📝 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.
Suggested change
| for (const file of fs.readdirSync(JS_DIR)) { | |
| if (!file.endsWith('.js') || file === 'host.js') continue; | |
| const raw = fs.readFileSync(path.join(JS_DIR, file), 'utf8'); | |
| if (!/from\s+'\.\/host\.js'/.test(raw)) continue; | |
| for (const m of scrub(raw).matchAll(HOOK_RE)) { | |
| for (const file of fs.readdirSync(JS_DIR)) { | |
| if (!file.endsWith('.js') || file === 'host.js') continue; | |
| const raw = fs.readFileSync(path.join(JS_DIR, file), 'utf8'); | |
| if (!/from\s+(['"])\.\/host\.js\1/.test(raw)) continue; | |
| for (const m of scrub(raw).matchAll(HOOK_RE)) { |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 57-57: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.join(JS_DIR, file), 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🤖 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/host_contract.test.js` around lines 56 - 60, Update the host-import
filter in the JS directory scan to recognize both single- and double-quoted
`./host.js` imports, while preserving the existing file exclusions and hook
scanning via `HOOK_RE`.
static/js/player-controls.js (229) — the speed + mastery sliders and the four
playback-preference reads (autoplay-exit, up-next, countdown-before-song,
confirm-exit). Bodies VERBATIM. app.js 7,914 → 7,727.
The fourth slice out of the strongly-connected core, and by far the easiest: ONE
hook (handleSliderInput) and NO shared mutable state. The three groups are the same
surface — the controls under the highway — and the preference reads are the
one-line localStorage lookups half of app.js consults before deciding whether to
auto-start, show the Up Next pill, run a count-in, or confirm on exit. They travel
with the controls that set them.
Zero missed members on the first build (the no-undef pass was clean), which is the
first time that has happened in this phase.
TWO HARNESSES ARE SPLIT, and both taught something:
* speed_reset spans BOTH files — playSong (app.js) resets the speed controls
(module). Its presence GUARDS still read `src.includes('function setSpeed')`
against app.js, so once the code moved they silently evaluated FALSE and the
helpers were quietly dropped from the sandbox. A guard that disables itself is
worse than no guard. Repointed at the file the code actually lives in.
* Its `host.handleSliderInput` stub had to route at the sandbox's EXISTING spy,
not a fresh `() => {}`. The test asserts the slider was actually refreshed
(`deepEqual(__sliderInputs, ['speed-slider'])`); a fresh stub swallows the call
and the assertion passes VACUOUSLY. Same failure mode as a no-op host default —
the thing this whole seam design exists to prevent.
* autoplay_exit is split too: _autoplayExitEnabled moved, but the auto-exit
machinery around it (_clearAutoExit, holdAutoExit, _resolvePlayerOrigin) stayed.
VERIFIED. A/B against origin/main in two browsers, real song: setSpeed(0.75) ->
playbackRate 0.75; applySpeedPreset(100) -> 1; the speed slider; setMastery;
setAutoplayExit / setCountdownBeforeSong / setShowUpNext — IDENTICAL, zero page errors.
pytest 2396, node 1040/1040, ESLint 0 (no-cycle clean), tailwind clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
byrongamatos
force-pushed
the
feat/r3-carve-player-controls
branch
from
July 11, 2026 20:06
cdf0829 to
60b4b09
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/js/speed_reset.test.js (1)
136-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale "not found in app.js" error messages now also fire against
player-controls.jsextractions.
extractFunction(Line 14) andextractConstLine(Line 130) hardcode"not found in app.js", but Lines 143-146, 179 now call them againstcontrols(player-controls.js). A failed extraction from the new source will report a misleading file name, slowing debugging.Proposed fix
- if (start === -1) throw new Error(`extractFunction: '${signature}' not found in app.js`); + if (start === -1) throw new Error(`extractFunction: '${signature}' not found`);- if (!match) throw new Error(`extractConstLine: '${name}' not found in app.js`); + if (!match) throw new Error(`extractConstLine: '${name}' not found`);Also applies to: 179-179
🤖 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/speed_reset.test.js` around lines 136 - 148, Update extractFunction and extractConstLine to accept a source-file label or derive it from the provided content, then pass the appropriate label when extracting from controls so failures identify player-controls.js instead of hardcoding app.js.
🤖 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/player-controls.js`:
- Around line 14-15: Update the imports in player-controls.js to include the
highway module, or explicitly access it through window.highway wherever
_applyMastery() calls highway.setMastery(). Ensure the call uses a defined
binding while preserving the existing mastery application behavior.
---
Nitpick comments:
In `@tests/js/speed_reset.test.js`:
- Around line 136-148: Update extractFunction and extractConstLine to accept a
source-file label or derive it from the provided content, then pass the
appropriate label when extracting from controls so failures identify
player-controls.js instead of hardcoding app.js.
🪄 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: 9df4911b-67cd-484c-8139-ef3a020286dd
📒 Files selected for processing (4)
static/app.jsstatic/js/player-controls.jstests/js/autoplay_exit.test.jstests/js/speed_reset.test.js
Comment on lines
+14
to
+15
| import { audio } from './audio-el.js'; | ||
| import { host } from './host.js'; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how `highway` is declared/exposed in app.js and whether it's a bare global
grep -n "highway" static/app.js | head -50
grep -n "^var highway\|^let highway\|^const highway\|window.highway" static/app.js static/js/*.jsRepository: got-feedBack/feedBack
Length of output: 4737
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- player-controls.js around the referenced call ---'
nl -ba static/js/player-controls.js | sed -n '180,235p'
echo
echo '--- search for highway declarations/exports/imports ---'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
'window\.highway\b|globalThis\.highway\b|^\s*(var|let|const)\s+highway\b|export\s+(default\s+)?highway\b|import\s+\{[^}]*\bhighway\b[^}]*\}' \
static
echo
echo '--- app.js: places where highway is assigned or used as a global ---'
rg -n --hidden --glob '!**/node_modules/**' 'window\.highway\s*=|globalThis\.highway\s*=|highway\s*=' static/app.js static/js/*.js
echo
echo '--- nearby context around any window.highway assignment in app.js ---'
python3 - <<'PY'
from pathlib import Path
text = Path('static/app.js').read_text()
for needle in ['window.highway =', 'globalThis.highway =']:
idx = text.find(needle)
if idx != -1:
start = max(0, idx - 300)
end = min(len(text), idx + 500)
print(f'\n### {needle}\n')
print(text[start:end])
PYRepository: got-feedBack/feedBack
Length of output: 253
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- player-controls.js around the referenced call ---'
sed -n '180,235p' static/js/player-controls.js | cat -n
echo
echo '--- search for highway declarations/exports/imports ---'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
'window\.highway\b|globalThis\.highway\b|^\s*(var|let|const)\s+highway\b|export\s+(default\s+)?highway\b|import\s+\{[^}]*\bhighway\b[^}]*\}' \
static
echo
echo '--- app.js: places where highway is assigned or used as a global ---'
rg -n --hidden --glob '!**/node_modules/**' 'window\.highway\s*=|globalThis\.highway\s*=|highway\s*=' static/app.js static/js/*.js
echo
echo '--- nearby context around any window.highway/globalThis.highway assignment in app.js ---'
python3 - <<'PY'
from pathlib import Path
text = Path('static/app.js').read_text()
for needle in ['window.highway =', 'globalThis.highway =']:
idx = text.find(needle)
if idx != -1:
start = max(0, idx - 300)
end = min(len(text), idx + 500)
print(f'\n### {needle}\n')
print(text[start:end])
PYRepository: got-feedBack/feedBack
Length of output: 5344
Import highway here or reference window.highway. static/highway.js only assigns window.highway; it does not create a module binding, so highway.setMastery(...) can throw ReferenceError when _applyMastery() runs.
🤖 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/player-controls.js` around lines 14 - 15, Update the imports in
player-controls.js to include the highway module, or explicitly access it
through window.highway wherever _applyMastery() calls highway.setMastery().
Ensure the call uses a defined binding while preserving the existing mastery
application behavior.
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.
Fourth slice out of the strongly-connected core, and by far the easiest: one hook (
handleSliderInput) and no shared mutable state.The three groups are the same surface — the controls under the highway — and the preference reads (
_autoplayExitEnabled,_showUpNextEnabled,_countdownBeforeSongEnabled,_exitConfirmEnabled) are the one-line localStorage lookups half of app.js consults before deciding whether to auto-start, show the Up Next pill, run a count-in, or confirm on exit. They travel with the controls that set them.Zero missed members on the first build — the
no-undefpass was clean first time, which hasn't happened before in this phase.Two split harnesses, and both taught something
speed_resetspans both files —playSong(app.js) resets the speed controls (module). Its presence guards still readsrc.includes('function setSpeed')against app.js, so once the code moved they silently evaluated false and the helpers were quietly dropped from the sandbox.Repointed at the file the code actually lives in.
Its
host.handleSliderInputstub had to route at the sandbox's existing spy, not a fresh() => {}. The test asserts the slider was actually refreshed (deepEqual(__sliderInputs, ['speed-slider'])) — a fresh stub swallows the call and the assertion passes vacuously. That's the same failure mode as a no-op host default: the exact thing this seam design exists to prevent, showing up in the test layer instead.autoplay_exitis split too —_autoplayExitEnabledmoved, but the auto-exit machinery around it (_clearAutoExit,holdAutoExit,_resolvePlayerOrigin) stayed.Verification
A/B against
origin/mainin two browsers, real song loaded:setSpeed(0.75)→audio.playbackRate0.75applySpeedPreset(100)→ rate1100setMastery(0.5)setAutoplayExit/setCountdownBeforeSong/setShowUpNextpytest 2396 · node 1040/1040 · ESLint 0 (no-cycle clean) · tailwind clean.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes