fix: read the full mix from the reserved full stem, not original_audio#39
Merged
Conversation
…udio` (feedback#933) The pristine mixdown we play at unity used to arrive on song_info as `original_audio_url` — named after a manifest key core invented and the feedpak spec never had. The mixdown is a stem: feedpak 1.15.0 RESERVES the id `full` for it, and core now surfaces it as `full_mix_url` / `has_full_mix`. Core still lifts it OUT of `info.stems` before sending, which is what keeps this plugin correct: `full` already contains every instrument, so if it ever appeared among the tracks we sum, unity playback would double the whole song and muting "guitar" would leave guitar audible inside the mix. Reads the new fields, falling back to the deprecated aliases so the plugin still works against a core that predates the rename (a user can have a newer bundled plugin than server). The fallback goes when the aliases do (feedback#945).
📝 WalkthroughWalkthroughThe plugin now detects full mixes through ChangesFull mix metadata compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
Ship the full-mix-is-a-stem change (read full_mix_url with an original_audio_url fallback) under a bumped plugin version. Compatible internal change, so patch. The CHANGELOG [Unreleased] entry is left unstamped — it accumulates other merged-but-unreleased changes and is stamped at release time. Signed-off-by: Kris Anderson <topkoa@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main.js (1)
806-819: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for both loading paths.
The new URL feeds
setupStreamingfor WAV andloadFullMixfor decode. Add current-only, legacy-only, absent-URL/false-flag, and non-worklet cases to protect this compatibility branch.🤖 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/main.js` around lines 806 - 819, Add regression tests covering both full-mix loading paths: verify full_mix_url is used by setupStreaming for WAV and by loadFullMix for decode, including current-only, legacy-only, absent-URL/false-flag, and non-worklet cases. Anchor the tests to the hasFullMix/fullMixUrl/fullUrl compatibility logic and confirm invalid or disabled configurations do not load a full mix.
🤖 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/main.js`:
- Around line 806-819: Add regression tests covering both full-mix loading
paths: verify full_mix_url is used by setupStreaming for WAV and by loadFullMix
for decode, including current-only, legacy-only, absent-URL/false-flag, and
non-worklet cases. Anchor the tests to the hasFullMix/fullMixUrl/fullUrl
compatibility logic and confirm invalid or disabled configurations do not load a
full mix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ecead97-532f-486f-8d6c-a9b70f250080
📒 Files selected for processing (4)
CHANGELOG.mdplugin.jsonsrc/main.jssrc/streaming.js
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
2 participants
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.
Part of got-feedBack/feedBack#933. Do not merge before got-feedBack/feedBack#933 and got-feedBack/feedpak-spec#53 — though this is safe to merge in any order in practice, because it falls back to the old field names.
What
The pristine mixdown this plugin plays while every fader sits at unity now arrives on
song_infoasfull_mix_url/has_full_mix. It used to arrive asoriginal_audio_url/has_original_audio— named after a manifest key core invented (feedback#583) and the feedpak spec never had.The mixdown is a stem. feedpak 1.15.0 RESERVES the id
fullfor it (got-feedBack/feedpak-spec#53), and core now reads it from there.Why this plugin stays correct
Core lifts
fullout ofinfo.stemsbefore sending the frame. That is what makes retaining the mixdown safe, and this plugin is the reason it matters:fullalready contains every instrument, so if it ever appeared among the tracks we sum, unity playback would double the whole song — and muting "guitar" would still leave guitar audible inside the mix. (That exact hazard is why the packer originally put the mixdown outsidestemsand invented a key to point at it. The bug was in the reader; core fixes the reader.)Compatibility
Reads the new fields and falls back to the deprecated aliases, so the plugin still works against a core that predates the rename — a real case, since a user can have a newer bundled plugin than server. The fallback goes when the aliases do (got-feedBack/feedBack#945).
Verification
npm test— 47/47 pass.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores