Skip to content

fix(audio-effects): accept pre-rebrand chain plan schema as alias - #816

Merged
byrongamatos merged 3 commits into
mainfrom
fix/effects-plan-schema-legacy-alias
Jul 9, 2026
Merged

byrongamatos merged 3 commits into
mainfrom
fix/effects-plan-schema-legacy-alias

Conversation

@OmikronApex

@OmikronApex OmikronApex commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

The Slopsmith→fee[dB]ack rebrand renamed PLAN_SCHEMA to feedBack.audio_effects.chain_plan.v1, but shipped plugin bundles (rig_builder ≤ 3.0.6) still send the slopsmith-era id. _validatePlan therefore rejected every plan ("Provider returned an invalid audio-effects chain plan") and providers fell back to their heavyweight legacy load paths — a full multi-VST chain rebuild per poll cycle, heard as continuous distortion during songs (root cause of the tester distortion reports).

Accept the old id as an explicit alias so existing plugin installs keep working. Companion fixes:

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Reduced repeated bridge-hit reporting during frequent playback polling, preventing diagnostics from being flooded.
    • Manual A/B loop actions now emit the same loop change event as programmatic loop updates, improving plugin and automation reactions.
    • Added support for an additional legacy audio-effects chain-plan format, improving compatibility with older clients.
  • Documentation

    • Updated the changelog with the latest playback and compatibility fixes.

OmikronApex and others added 3 commits July 7, 2026 23:37
…r manual A/B

window.feedBack.getLoop() is a read surface plugins legitimately poll
(note_detect HUD ticked it at ~30 Hz), but every call recorded a
playback.loop-api bridge hit: compat-shim bookkeeping, a
playback:bridge-hit event, and a diagnostics snapshot rebuild +
stringify per call — real main-thread cost and a saturated hitCount in
the capability inspector, even with no song playing.

- _recordPlaybackBridge now throttles per bridgeId|surface (5 s window).
  Bridge hits are a 'surface still in use' signal, not a call counter.
- setLoopEnd() (manual A/B buttons) now emits the same loop-set
  transport event as setLoop(), so event-driven consumers no longer
  need to poll getLoop() to see button-armed loops.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The rebrand renamed PLAN_SCHEMA to 'feedBack.audio_effects.chain_plan.v1'
but shipped plugin bundles (rig_builder <= 2.9.x) still send the
slopsmith-era id, so _validatePlan rejected every plan and providers fell
back to their heavyweight legacy load paths (full chain rebuild per poll
cycle — audible as continuous distortion during songs). Accept the old id
as an explicit alias.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR throttles playback bridge hit recording to once per 5 seconds per bridge/surface pair, dispatches a loop-set transport event when manually arming an A/B loop, and extends chain-plan validation to accept a new legacy schema identifier alongside existing checks. A changelog entry documents the fixes.

Changes

Playback Loop and Schema Fixes

Layer / File(s) Summary
Bridge hit throttling and loop-set event emission
static/app.js, CHANGELOG.md
Throttles _recordPlaybackBridge calls per (bridgeId, legacySurface) to once per 5 seconds, dispatches a loop-set transport event with loopA/loopB/active when arming a manual A/B loop, and documents both fixes in the changelog.
Legacy chain-plan schema support
static/capabilities/audio-effects.js
Adds a LEGACY_PLAN_SCHEMA constant and updates _validatePlan to accept it alongside PLAN_SCHEMA and version "1".

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant AppJS as app.js
  participant Bridge as playback bridge
  participant Transport as transportEvent

  User->>AppJS: arm A/B loop end point
  AppJS->>Transport: transportEvent('loop-set', loopA, loopB, active)
  User->>AppJS: poll getLoop()
  AppJS->>AppJS: check (bridgeId, legacySurface) last-call timestamp
  alt within 5s interval
    AppJS-->>Bridge: skip recordBridgeHit()
  else interval elapsed
    AppJS->>Bridge: recordBridgeHit()
  end
Loading

Possibly related PRs

  • got-feedBack/feedBack#811: Modifies the same _recordPlaybackBridge() throttling logic and loop-set transport event emission in static/app.js.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main audio-effects validation fix: accepting the pre-rebrand chain plan schema as an alias.
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.
✨ 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 fix/effects-plan-schema-legacy-alias

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: 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 `@static/capabilities/audio-effects.js`:
- Around line 12-13: The inline comment above LEGACY_PLAN_SCHEMA in
audio-effects.js has the wrong affected-version range and conflicts with the PR
intent. Update that comment to match the actual legacy plugin scope described
for the schema alias, keeping the code behavior unchanged, so future maintainers
can correctly identify which rig_builder versions still send the old schema id.
🪄 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: a46c5512-1970-4abe-8e52-703502626f95

📥 Commits

Reviewing files that changed from the base of the PR and between 950e348 and 65a3c25.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • static/app.js
  • static/capabilities/audio-effects.js

Comment on lines +12 to +13
// Pre-rebrand plugins (rig_builder <= 2.9.x) still send the old schema id — accept it as an alias.
const LEGACY_PLAN_SCHEMA = 'slopsmith.audio_effects.chain_plan.v1';

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.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Version number in comment conflicts with PR description.

Comment says rig_builder <= 2.9.x but the PR objectives state the affected legacy plugin version is rig_builder ≤ 3.0.6. Fix the comment to avoid confusing future maintainers about which plugin versions require this alias.

📝 Proposed fix
-// Pre-rebrand plugins (rig_builder <= 2.9.x) still send the old schema id — accept it as an alias.
+// Pre-rebrand plugins (rig_builder <= 3.0.6) still send the old schema id — accept it as an alias.
📝 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
// Pre-rebrand plugins (rig_builder <= 2.9.x) still send the old schema id — accept it as an alias.
const LEGACY_PLAN_SCHEMA = 'slopsmith.audio_effects.chain_plan.v1';
// Pre-rebrand plugins (rig_builder <= 3.0.6) still send the old schema id — accept it as an alias.
const LEGACY_PLAN_SCHEMA = 'slopsmith.audio_effects.chain_plan.v1';
🤖 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/capabilities/audio-effects.js` around lines 12 - 13, The inline
comment above LEGACY_PLAN_SCHEMA in audio-effects.js has the wrong
affected-version range and conflicts with the PR intent. Update that comment to
match the actual legacy plugin scope described for the schema alias, keeping the
code behavior unchanged, so future maintainers can correctly identify which
rig_builder versions still send the old schema id.

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.

2 participants