Repository navigation
refactor(stems): extract the streaming path to src/streaming.js - #34
Conversation
Move the bounded-memory iOS WAV streaming layer out of main.js into its own
module (~440 lines): the reader/WAV helpers (trackRead/ensureBytes/dropBytes/
readWavHeader/dequeueTrackFrames), the worklet-feed pump (appendRound/waitPos/
runPump) + seek path (cancelStreamReaders/openTrackStreams/repositionStream),
setupStreaming, resetStreamState, and streamOffsetBuffered/streamingSupported/
isWavResponse. main.js 2281 -> 1840 lines.
The layer would form a static import cycle (transport imports setupStreaming/
repositionStream; the pump calls transportPlay, setupStreaming needs
persistedSongGain + the shared onWorkletMessage — all in main.js). Broken with an
injected seam: configureStreaming({ startPendingPlay, songGain, onWorkletMessage })
wired once at boot. Everything else the layer calls is already an extracted module
(audio-ctx, mix, mix-gains, util, wav-pcm, prefs, state), imported by the same
names, so the bodies moved verbatim. Removed now-dead main.js imports
(parseWavHeader/pcm16ToFloat32, ensureCtxAtRate).
New tests/streaming.test.mjs covers the pure exports (isWavResponse,
streamingSupported, streamOffsetBuffered window math). The pump/seek internals
stay private, so the seek-token race guard remains on-device-verified.
CHANGELOG: backfilled steps 5-9.1 (missed) + added step 10.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBounded-memory streaming playback logic is extracted from ChangesStreaming module extraction
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant main.js
participant streaming.js
participant AudioWorklet
participant fetch
main.js->>streaming.js: configureStreaming(hooks)
main.js->>streaming.js: setupStreaming(stems, probeResp, fullUrl, gen)
streaming.js->>fetch: open Range requests for stems
fetch-->>streaming.js: WAV byte streams
streaming.js->>AudioWorklet: load worklet, initialize gains
streaming.js->>streaming.js: runPump append loop
streaming.js->>AudioWorklet: append PCM frames
AudioWorklet-->>streaming.js: pos message
main.js->>streaming.js: transportPlay uses streamOffsetBuffered(offset)
alt offset not buffered
main.js->>streaming.js: repositionStream(targetSec)
streaming.js->>fetch: reopen streams at target sample
streaming.js->>AudioWorklet: seek message
end
main.js->>streaming.js: resetStreamState()
streaming.js->>fetch: cancel readers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/streaming.test.mjs (1)
22-35: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the other two
streamingSupported()failure paths.This only proves the
AudioWorkletNodebranch. Add missing-ReadableStreamand missing-fetchcases so the&&contract is fully pinned down.♻️ Suggested test expansion
test('streamingSupported: true only when all three platform APIs exist', () => { @@ globalThis.ReadableStream = function () {}; globalThis.fetch = function () {}; globalThis.AudioWorkletNode = function () {}; assert.equal(streamingSupported(), true); - delete globalThis.AudioWorkletNode; // no worklet → unsupported + delete globalThis.ReadableStream; // no stream → unsupported + assert.equal(streamingSupported(), false); + globalThis.ReadableStream = function () {}; + delete globalThis.fetch; // no fetch → unsupported + assert.equal(streamingSupported(), false); + globalThis.fetch = function () {}; + delete globalThis.AudioWorkletNode; // no worklet → unsupported assert.equal(streamingSupported(), false);🤖 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/streaming.test.mjs` around lines 22 - 35, Expand the streamingSupported() test to cover the remaining false cases in addition to the existing AudioWorkletNode branch. In tests/streaming.test.mjs, keep using the same streamingSupported() helper and globalThis overrides, but add assertions for when ReadableStream is missing and when fetch is missing so the full && condition is verified; preserve and restore the original globals in the same test setup.src/streaming.js (1)
219-238: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard
resp.bodybeforegetReader()for consistency withsetupStreaming.
setupStreamingexplicitly guardsif (!resp || !resp.body) return false;(Line 306), butopenTrackStreamscallsresp.body.getReader()(Line 231) unconditionally. A 204/no-body response on a Range refetch would throw a TypeError; it's caught byrepositionStream's try/catch (Line 259) and reported as a generic seek failure, but a targeted guard yields cleaner handling and matches the sibling path.♻️ Proposed guard
const resp = await fetch(t.url, { signal: S.abortController.signal, headers }); if (gen !== S.loadGeneration || token !== ST.streamSeekToken) { try { resp.body && resp.body.cancel(); } catch (_) {} return; } + if (!resp.body) { t.reader = null; t.leftover = EMPTY_BYTES; t.done = true; return; } t.reader = resp.body.getReader();🤖 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 `@src/streaming.js` around lines 219 - 238, Guard the fetch response body in openTrackStreams before calling getReader, matching the existing setupStreaming check. In the openTrackStreams path, handle a missing resp or resp.body by returning early instead of unconditionally assigning t.reader from resp.body.getReader(), so 204/no-body Range responses don’t surface as a TypeError and are handled consistently with setupStreaming.
🤖 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 `@src/streaming.js`:
- Around line 219-238: Guard the fetch response body in openTrackStreams before
calling getReader, matching the existing setupStreaming check. In the
openTrackStreams path, handle a missing resp or resp.body by returning early
instead of unconditionally assigning t.reader from resp.body.getReader(), so
204/no-body Range responses don’t surface as a TypeError and are handled
consistently with setupStreaming.
In `@tests/streaming.test.mjs`:
- Around line 22-35: Expand the streamingSupported() test to cover the remaining
false cases in addition to the existing AudioWorkletNode branch. In
tests/streaming.test.mjs, keep using the same streamingSupported() helper and
globalThis overrides, but add assertions for when ReadableStream is missing and
when fetch is missing so the full && condition is verified; preserve and restore
the original globals in the same test setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f08fc4c9-a717-487b-92bb-0cce8f51116d
📒 Files selected for processing (4)
CHANGELOG.mdsrc/main.jssrc/streaming.jstests/streaming.test.mjs
There was a problem hiding this comment.
Pull request overview
Refactors the bounded-memory iOS WAV streaming playback path by extracting it from src/main.js into a dedicated src/streaming.js module, while preserving behavior and avoiding an ES-module import cycle via an injected seam (configureStreaming).
Changes:
- Extracts the entire streaming implementation (reader/WAV helpers, worklet pump, seek/reposition, setup/teardown, and small pure helpers) into
src/streaming.js. - Wires
src/main.jsto the new module (imports + one-timeconfigureStreaming({ startPendingPlay, songGain, onWorkletMessage })boot hook) and removes the now-dead local streaming code. - Adds
tests/streaming.test.mjsto cover the pure-ish exports (streamingSupported,isWavResponse,streamOffsetBuffered) and updatesCHANGELOG.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/streaming.test.mjs | Adds unit coverage for the extracted module’s pure exports using real ESM imports. |
| src/streaming.js | New module containing the extracted iOS WAV streaming path + injected hook seam to avoid import cycles. |
| src/main.js | Removes inlined streaming implementation; imports/wires the extracted streaming module and its seam. |
| CHANGELOG.md | Documents step 10 extraction (and backfills earlier step entries). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address CodeRabbit nitpick on #34 — pin the full && contract (missing ReadableStream / fetch / AudioWorkletNode), not just the worklet branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the two CodeRabbit nitpicks:
|
What
Step 10 — the first real module lift of the entangled core. Moves the bounded-memory iOS WAV streaming path out of
main.jsintosrc/streaming.js(~440 lines):trackRead,ensureBytes,dropBytes,readWavHeader,dequeueTrackFramesappendRound,waitPos,runPumpcancelStreamReaders,openTrackStreams,repositionStreamsetupStreaming,resetStreamState,streamOffsetBuffered,streamingSupported,isWavResponsemain.js2281 → 1840 lines.Breaking the cycle
The streaming layer would form a static import cycle: transport imports
setupStreaming/repositionStreamfrom here, while the pump resumes a deferred play viatransportPlay, andsetupStreamingneedspersistedSongGain+ the sharedonWorkletMessage— all inmain.js. Since ES cycles are banned by the R0 no-cycle gate, this is broken with a small injected seam wired once at boot:Everything else the layer calls is already an extracted module (audio-ctx, mix, mix-gains, util, wav-pcm, prefs, state), imported by the same names — so the function bodies moved verbatim. Removed the now-dead
main.jsimports (parseWavHeader/pcm16ToFloat32,ensureCtxAtRate).Tests
New
tests/streaming.test.mjs— real-import coverage for the pure exports:isWavResponse(content-type sniff),streamingSupported(platform-API gate), andstreamOffsetBuffered(the buffered-window boundary math). The pump/seek internals stay private, so the seek-token race guard (step 9.1) remains on-device-verified.Full suite green (
npm test, 45 pass);tests/test_manifest.pygreen. Codex preflight: 0 issues.CHANGELOG: backfilled the missed step 5–9.1 entries + added step 10.
Summary by CodeRabbit