R1 pilot: stems ES-module migration, step 4b (reassigned scalars → S object)

[byrongamatos]

R1 pilot — stems ES-module migration, step 4b (reassigned scalars → S object)

The hard part of the state extraction. The ~28 reassigned module scalars — the audio graph (audioCtx/masterGain/analyserNode), stemState, the worklet + decode flags, the current-song fields — move into a single exported S object in src/state.js.

Why an S object (not export let)

ES imports are read-only bindings, so main.js can't do x = … on an imported let. Grouping the reassigned scalars in one object means S.x = new AudioContext() works. Every reference becomes S.x (~230 sites).

How the rewrite was de-risked

• The one real collision — ctx used as a local param in the four capability-command functions — was renamed to cmdCtx first (verified they never touch the AudioContext), so ctx → S.audioCtx was unambiguous.
• Pre-checked for sed hazards (property keys, object shorthand, binding collisions): none.
• Post-verified: 0 bare references, 0 double-S. prefixes, parse clean.
• Codex preflight caught the one thing the static checks missed — a string-literal hazard: sed had rewritten Object.defineProperty(stemsApi, 'stemState', …) to 'S.stemState', which would have broken the public window.stems.stemState API. Fixed (property name restored; getter returns S.stemState). This is the only string-literal occurrence in the file.

Verification

• npm test: 31/31; pytest: 10/10; import-x/no-cycle clean on the 6-module graph.
• The graph boots + executes in-browser against core-with-R0 (window.stems installs, worklet resolves, no errors).

⚠️ Not yet exercised — real playback

These scalars drive playback, but a true playback test needs a seeded stems .sloppak (unavailable in the headless/empty-library dev setup). Boot/load/lint/tests are green; an on-device playback smoke is recommended before release (load a multi-stem song, toggle stems, drag volume, seek, let it end).

Move-only, no behaviour change intended.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Refactor
  • Centralized playback, worklet, decode, and UI lifecycle data into a single shared state container to keep behavior consistent across loading, start/seek/rate, and teardown.
  • Improved worklet gain routing and latency/timing synchronization by relying on the shared state for readiness and playback positioning.
  • Made pending playback and overlay/UI shutdown flows more reliable during setup/cleanup.
  • Change is move-only with no expected end-user behavior change.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 07548a8f-5950-4ec5-a8bb-0afcf3115511

📥 Commits

Reviewing files that changed from the base of the PR and between 8ae6c9b and fa2bc95.

📒 Files selected for processing (1)

• src/main.js

---

📝 Walkthrough

Walkthrough

This PR moves mutable runtime state in src/main.js into the exported S object from src/state.js, rewrites main playback and UI flows to use S.*, renames capability command parameters to cmdCtx, and adds a changelog entry for the migration.

Changes

State consolidation into S

Layer / File(s)	Summary
S state container definition src/state.js, src/main.js	Adds the exported S state object and imports it into main.js.
AudioContext and worklet routing src/main.js	Moves context setup, worklet readiness, latency tracking, mix routing, and worklet message handling onto S.
Transport and audio shim behavior src/main.js	Updates playhead logic, transport control, and #audio shims to use S playback state.
Overlay, teardown, and UI injection src/main.js	Moves overlay handling, teardown, and UI injection/mute persistence to S-backed fields.
Decode and streaming pipelines src/main.js	Reworks graph building, decode/stream setup, append/reposition handling, and readiness handoff around S.
Song readiness and capability API src/main.js	Updates onSongReady, readiness polling, stem state publication, capability handlers, and window.stems to use S.
Changelog documentation CHANGELOG.md	Documents the migration step and the capability parameter rename.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

• got-feedBack/feedBack-plugin-stems#8 — Updates the capability-command contract that this PR adapts by renaming ctx to cmdCtx in the capability handlers.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: migrating reassigned scalars to the shared S object during the stems ES-module migration.

✨ 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/es-module-split-state-scalars

---

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

[byrongamatos]

⚠️ Requires an on-device playback smoke before merge

4b moved the reassigned scalars that drive playback into S. The dev/CI checks (node tests, pytest, import-x/no-cycle, in-browser boot) are all green, but they don't play audio. The one validation the headless/empty-library setup can't do is real playback, so please run this on a build with the R0-enabled core + a multi-stem song before merging:

• Load a multi-stem sloppak → all stems audible, sample-locked (no drift vs the highway)
• Toggle stems on/off → mutes/unmutes correctly; the pristine full mix plays at unity when every stem is on
• Drag a stem's volume → smooth ramp, no hard switch; per-song volume persists across reload
• Seek mid-song → transport rebaselines, stays in sync
• Speed change (if applicable) → pitch-preserved (worklet path)
• Song end → fires once, clean teardown
• Leave & re-enter the player → re-inits via screen:changed, no stale state
• Karaoke default + startup-muted settings still apply
• iOS / streaming path (bounded-memory WAV) if reachable

Kept as draft until this is signed off — the code review (Codex/CodeRabbit/Copilot) can proceed in the meantime.

[Copilot]

Pull request overview

This PR continues the ES-module migration by moving the reassigned/mutable module-level scalars (audio graph nodes, per-song fields, worklet/decode flags, etc.) into a single exported S object in src/state.js, and updating src/main.js to read/write those values via S.* so reassignment works with ESM’s read-only import bindings.

Changes:

• Add exported S object to src/state.js to hold previously-reassigned mutable scalars.
• Update src/main.js to import S and replace former module-scoped scalars with S.* across the playback/worklet/transport logic.
• Document the migration step in CHANGELOG.md.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
src/state.js	Introduces exported S object to centralize reassigned mutable scalar state under ESM.
src/main.js	Converts prior module-scoped scalar references to S.* and imports S from state.js.
CHANGELOG.md	Adds an entry describing “step 4b” and the rationale/verification notes.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 2

🤖 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 1152-1161: The inline comment in the stem gain setup has been
accidentally rewritten to reference a field name, turning “wired” into a
spurious S.wired reference. Update the comment near S.stemState and the gain
initialization branch in the map callback so it reads as plain English again
(“wired into S.masterGain”) and does not look like a property access.
- Around line 1831-1836: Fix the corrupted comment text in the streaming path
near the content-type probe logic in main.js: the phrase describing desktop
audio should use the plain English noun “container,” not the accidental
field-like token “S.container.” Update the surrounding comment in the same block
that references audio/wav, audio/ogg, and the full-decode path so it reads
naturally and avoids misleading symbol-style wording.

🪄 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: eddc2b7a-e7c3-49f1-9eb0-8c0c371ada9b

📥 Commits

Reviewing files that changed from the base of the PR and between 76fc7c9 and 8ae6c9b.

📒 Files selected for processing (3)

• CHANGELOG.md
• src/main.js
• src/state.js

[byrongamatos]

✅ On-device playback smoke passed (Byron, local uvicorn: core@main + this branch, real multi-stem songs). Stems audible + sample-locked, toggle/volume/seek/teardown all correct — the S-object state migration is behaviour-preserving through real playback. Un-drafting; ready to merge on review sign-off.