Skip to content

refactor(app): carve the JUCE/desktop audio shims out of app.js (R3a)#893

Merged
byrongamatos merged 2 commits into
mainfrom
feat/r3-carve-juce
Jul 11, 2026
Merged

refactor(app): carve the JUCE/desktop audio shims out of app.js (R3a)#893
byrongamatos merged 2 commits into
mainfrom
feat/r3-carve-juce

Conversation

@byrongamatos

@byrongamatos byrongamatos commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Stacked on #892. 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:

lines what it does
_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, 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.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.

And note the safety property doing real work here: had I got this wrong, host.js throws loudly rather than silently misbehaving. That's the whole reason it has no no-op defaults.

Verification

A/B against origin/main in two browsers:

main carved
_juceMode / _juceOutputIsExclusive false / undefined identical
_reevaluateJuceRouting / _reevaluateRendererBus / _clearJuceRerouteMemo undefined (browser; desktop-only) identical
audio.play patched (not [native code]) true identical
audio.pause patched true identical
real song loads; togglePlay drives the public mirror identical
page errors none none

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.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added “Resume practice” support to restore recent songs, playback positions, arrangements, and speeds.
    • Improved desktop audio routing for exclusive and native audio playback.
    • Added renderer audio support for desktop playback scenarios.
  • Bug Fixes

    • Improved play, pause, and seek synchronization in desktop mode.
    • Added safer handling for failed or stale resume sessions.
  • Tests

    • Updated audio routing and runtime tests for the new playback behavior.

@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

JUCE routing, renderer audio feeding, and media-element shims move into juce-audio.js. Resume-session behavior moves into resume-session.js, with pending resume state added to shared player state and playSong exposed through the host seam. Tests are updated for the new module boundaries.

Changes

JUCE audio and resume extraction

Layer / File(s) Summary
Host seam, shared state, and resume flow
static/app.js, static/js/player-state.js, static/js/resume-session.js
Resume requests use shared S.pendingResume; snapshot persistence, restoration, and resume-pill rendering are provided by resume-session.js; playSong is added to the host contract.
JUCE audio runtime
static/app.js, static/js/juce-audio.js
JUCE routing, renderer-bus feeding, and audio-element shims are extracted from app.js into a dedicated side-effect module with resettable shim state.
Runtime extraction validation
tests/js/*.test.js, tests/test_plugin_runtime_idempotence.py
Tests extract JUCE runtime IIFEs from juce-audio.js, provide host seams, and verify relocated playback-state and event behavior.

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
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 23.08% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main refactor: moving JUCE/desktop audio shims out of app.js into a dedicated module.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/r3-carve-juce

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
tests/js/loop_api.test.js (1)

19-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consolidate duplicated extractFunction across test files.

This export-stripping variant of extractFunction is now copy-pasted (per graph evidence) into loop_api.test.js, song_credits_overlay.test.js, play_button_reroute_guard.test.js, and song_restart.test.js, while tests/js/test_utils.js already exports a base extractFunction (with an openBrace === -1 guard this copy lacks). Centralizing this in test_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

📥 Commits

Reviewing files that changed from the base of the PR and between dc429ec and 95c5bd0.

📒 Files selected for processing (23)
  • static/app.js
  • static/js/count-in.js
  • static/js/host.js
  • static/js/juce-audio.js
  • static/js/loops.js
  • static/js/player-controls.js
  • static/js/player-state.js
  • static/js/resume-session.js
  • static/js/section-practice.js
  • tests/js/autoplay_exit.test.js
  • tests/js/host_contract.test.js
  • tests/js/juce_engine_reroute.test.js
  • tests/js/loop_api.test.js
  • tests/js/loop_restart.test.js
  • tests/js/play_button_reroute_guard.test.js
  • tests/js/playback_app_adapter.test.js
  • tests/js/renderer_bus_feeder.test.js
  • tests/js/section_practice_dismiss.test.js
  • tests/js/song_credits_overlay.test.js
  • tests/js/song_restart.test.js
  • tests/js/song_seek.test.js
  • tests/js/speed_reset.test.js
  • tests/test_plugin_runtime_idempotence.py

Comment thread static/js/count-in.js
Comment on lines +273 to +275
function beginCount() {
const bpm = highway.getBPM(loopA);
const beatInterval = 60 / bpm;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
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.

Comment thread static/js/juce-audio.js
Comment on lines +869 to +909
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;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.js

Repository: got-feedBack/feedBack

Length of output: 6969


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '950,990p' static/js/juce-audio.js

Repository: 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.

Comment on lines +108 to +119
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');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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 -S

Repository: 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 -S

Repository: 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\(" . -S

Repository: 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);
  }
}
JS

Repository: 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.

byrongamatos and others added 2 commits July 11, 2026 22:36
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>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
static/js/juce-audio.js (1)

866-909: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Still missing: honor forUpcomingPlay in the pause-only batch branch.

This is the same gap flagged in a previous review and it's still present. The wantsPause && seekTime !== undefined branch (Lines 876-895) correctly skips pause side-effects when forUpcomingPlay is true, but the wantsPause-only branch (Lines 896-909) unconditionally pauses jucePlayer, flips S.isPlaying = false, and emits song:pause — even when audio.play() (Line 976) is about to immediately flush this batch with forUpcomingPlay: true and re-flip state to playing right after. Same-tick pause() + play() with no seek still produces a pause→play flicker for plugins listening to song: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

📥 Commits

Reviewing files that changed from the base of the PR and between 95c5bd0 and 2a048fe.

📒 Files selected for processing (7)
  • static/app.js
  • static/js/juce-audio.js
  • static/js/player-state.js
  • static/js/resume-session.js
  • tests/js/juce_engine_reroute.test.js
  • tests/js/renderer_bus_feeder.test.js
  • tests/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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant