Repository navigation
refactor(stems): lift streaming-path state into state.js ST container - #32
Conversation
Move the 9 reassigned streaming scalars (streaming, streamTracks, streamSampleRate, streamTotalSamples, jsWriteFrontier, pumpStop, lastWorkletPos, posWaiter, streamSeekToken) out of main.js's IIFE scope into an ST container in state.js — same read-only-binding rationale as S/SH. Read across the transport layer + onWorkletMessage + onSongReady (the pump, seek, and playhead all key off them), so they must be shared, not private to the future streaming module. Keystone #2 for extracting src/streaming.js next. Rewrite was context-audited: the 8 compound identifiers rewrote blind (every occurrence is code); 'streaming' was rewritten only in its 6 code contexts ((streaming / !streaming / streaming =), leaving its 10 English mentions in comments + console.warn strings untouched. Pure state-lift, no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughStreaming playback state in src/main.js is refactored to use a new exported ChangesStreaming state consolidation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Worklet
participant main_js as main.js
participant ST
participant Pump as runPump/appendRound
Worklet->>main_js: postMessage('pos', position)
main_js->>ST: update ST.lastWorkletPos
main_js->>ST: resolve ST.posWaiter
ST-->>Pump: waitPos() resolves
Pump->>ST: read ST.jsWriteFrontier, ST.streamTracks
Pump->>Worklet: postMessage('append', base=ST.jsWriteFrontier)
Pump->>ST: update ST.jsWriteFrontier
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Refactors the stems plugin’s streaming-playback path by moving the streaming-path mutable scalars out of main.js’s IIFE scope into a shared exported ST state container in state.js, aligning with the existing S / SH shared-container pattern and enabling the upcoming extraction of streaming logic into its own module.
Changes:
- Introduces
STinsrc/state.jsto hold streaming-path state that must be shared across transport,onWorkletMessage, and streaming setup/teardown. - Rewrites streaming-path reads/writes in
src/main.jsto useST.*instead of IIFE-local variables. - Removes the now-redundant local streaming-path scalar declarations from
main.js(pure state lift; behavior intended to remain unchanged).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/state.js | Adds the exported ST container to hold streaming-path mutable state alongside existing shared state containers. |
| src/main.js | Switches streaming-path logic to reference ST.* and deletes the previously IIFE-local streaming scalar variables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/main.js`:
- Around line 1271-1305: The pump loop in runPump(), along with its
appendRound() and waitPos() continuations, can keep running after
repositionStream() starts a new seek and clears ST.pumpStop, allowing stale
iterations to post outdated append data or leave an old ST.posWaiter hanging.
Capture the current ST.streamSeekToken when a pump cycle starts, and re-check
that token after each await and before any postMessage/resolve work so
superseded iterations exit early. Apply the same token guard to the
seek/reposition flow that resets pump state so only the latest stream seek can
continue.
🪄 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: a86c3b02-df40-4bd8-b03e-c547f34d161b
📒 Files selected for processing (2)
src/main.jssrc/state.js
A streaming seek (repositionStream) bumps streamSeekToken, sets pumpStop, cancels readers, then reopens them on the same track objects and clears pumpStop before older pump/appendRound continuations are guaranteed to have exited. The old pumpStop-only guard was insufficient (repositionStream clears it again), so a stale append could post misaligned data at the reset frontier — or, worse, consume PCM from the freshly reopened readers for later tracks, starving the new pump. Symptom: audio dropout / desync after scrubbing under slow-network timing. Fix (token = the seek generation, captured per pump run): - appendRound(token): re-check the token before EACH track read (dequeue reads t.reader fresh, and reopened readers live on the same track objects) and again before posting. The in-flight track resolves off its own cancelled reader, so it can't reach the fresh ones. - runPump: capture token at start; gate both loops + the post-prefill return on it. - repositionStream: resolve a stranded posWaiter on stop so an old pump parked in waitPos() exits promptly; and restart as runPump(!S.buffersReady) so a seek that supersedes the INITIAL prefill still establishes readiness (else buffersReady never flips and play() stays silent until reload — a pre-existing hole). Pre-existing race (identical before the ST lift); found by CodeRabbit on #32, deepened via Codex review. Unit coverage lands with the streaming.js extraction (these fns are still IIFE-private); verified here by on-device seek-stress smoke. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What
Step 9 — second keystone of the entangled-core split. Lifts the 9 reassigned streaming-path scalars out of
main.js's IIFE scope into anSTcontainer instate.js:streaming,streamTracks,streamSampleRate,streamTotalSamples,jsWriteFrontier,pumpStop,lastWorkletPos,posWaiter,streamSeekTokenSame read-only-binding rationale as
S/SH. These are read across the transport layer,onWorkletMessage, andonSongReady(the bounded-window pump, seek reposition, and playhead all key off them), so they must be shared — not private to the futurestreaming.js. Pure state-lift, no behavior change. Unblocks extractingsrc/streaming.jsnext.Rewrite safety
Context-audited (the big-file lesson from step 4b):
streamingcollides with the English word in 10 places (comments +console.warn('…not streaming')), so it was rewritten only in its 6 code contexts ((streaming,!streaming,streaming =). The 10 English mentions are left untouched.Tests
Full suite green (
npm test, 41 pass);tests/test_manifest.pygreen. Codex preflight: 0 issues.Summary by CodeRabbit
Bug Fixes
Refactor