feat(rig-builder): universal noise gate toggle, Advanced Save button, fix pan/level/phase persistence - #60
Conversation
… fix pan/level/phase persistence Adds two requested features and fixes a persistence bug they surfaced. Universal noise gate: - New "Universal noise gate" toggle in the Advanced tab's gate bar. Enabling it bulk-flips the per-tone noise gate on for every already-saved tone (default tone, saved tones, every song tone) in one pass, walks the whole library to create shell presets for any never-saved tone so it inherits the gate too, and makes gate-on the default for any tone saved from now on. Runs as a background job (POST/GET /universal_noise_gate) so a large library doesn't block the UI. Disabling reverses the bulk flip. - _persist_preset_chain's INSERT branch now defaults a brand-new preset's gate to the universal_noise_gate_enabled setting instead of a hardcoded off, without touching existing rows unless the caller passes an explicit gate_enabled (preserves the existing per-tone-gate COALESCE semantics). Advanced tab Save button: - Explicit "Save" button that flushes any pending edit (including a debounced Pan/Level slider drag) straight to the tone DB on demand. Root-cause fix for "tone db changes not applying": - The Advanced graph editor's per-piece Pan/Level/Phase-invert controls never round-tripped through the backend at all: preset_pieces had no columns for them, and THREE separate hand-duplicated piece-serializers (rbStudioChainToPayload, rbPersistMasterChain's inline map, rbPersistTone's inline map) all silently dropped gain_db/pan/phase_inv from the save payload. Added gain_db/pan/phase_inv columns to preset_pieces (migrated), wired them through _persist_preset_chain and _load_saved_chain (the single shared read path for every chain type), fixed all three serializers to send them, and added rbSeedPieceMix to seed the underscore-prefixed frontend fields on every chain load site. Also fixed the Advanced node builder (mk()) and the cached-graph-restore path (rbAdvRestore), which had the same "only pan was ever wired up" gap independently. - rbAdvOnLevelInput/rbAdvOnPanInput now debounce a rbStudioPersist() call (they previously only wrote to the localStorage-cached visual graph, never the tone DB) — the original, narrower half of this same bug.
📝 WalkthroughWalkthroughAdds universal noise-gate management with background bulk updates, persists per-piece gain/pan/phase settings, applies gain during NAM/VST chain construction, fixes VST leveler discovery, and adds Advanced editor save and gate controls. ChangesRig builder audio settings
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant AdvancedEditor
participant GateAPI
participant GateWorker
participant PresetDatabase
AdvancedEditor->>GateAPI: POST universal_noise_gate
GateAPI->>GateWorker: start bulk gate update
GateWorker->>PresetDatabase: update preset gates and create shell presets
AdvancedEditor->>GateAPI: GET universal_noise_gate
GateAPI-->>AdvancedEditor: return gate state and job status
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…eal song playback
Two follow-up fixes for the universal-noise-gate/persistence work, both found
by live-testing against a real running app rather than just reading the code.
Gate silently missing during real song playback:
The per-tone noise gate only applied via a live setNoiseGate() push from
rbLoadNativePresetPayload/rbApplyToneGate, which runs on every loadPreset -
but mega-chain (the path real songs actually use) switches tones by toggling
bypass on a pre-built shared chain, NEVER calling loadPreset again. Worse,
the mega_chain endpoint's response never even included a `gate` field per
tone to begin with (the SQL read gate_threshold into a variable literally
named `_gate` and never used it). Net effect: the gate worked inside Rig
Builder's own Studio/Advanced preview, but silently never applied - or went
stale on the previous tone - once you left Rig Builder and played the song
for real. Fixed: mega_chain's per-tone response now includes
`_preset_gate(preset_id)`, and RbMegaChain's _applyActiveTone pushes it via
rbApplyToneGate on every tone switch, mirroring the existing leveler-relock
poke right next to it.
Level (gain_db) never affecting playback:
The Advanced graph editor's Level slider persists to the DB now (previous
fix), but nothing ever fed that value into what actually plays - the only
"apply" path was a live setPostGain() JS call scoped to Rig Builder's own
screen state (rbState._adv), which the real song/mega-chain playback path
never touches at all. Fixed by baking gain_db directly into each stage's
`postGain` field in both native_preset_full and mega_chain's stage builders -
the SAME optional field _amp_trim_stage already relies on ("Engines with
per-slot postGain support (loadPreset reads this optional field)"), so it
now applies at loadPreset time on every consumer (Rig Builder's own preview
AND real song playback), with no separate live push required. A ~0 dB piece
is unaffected (byte-identical stage dict to before).
Confirmed live via the API: a piece saved with gain_db=6 shows
`"postGain": 1.9953` in native_preset_full's returned stage, and a real
song's mega_chain response now returns a populated `gate` object per tone
where before there was none.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
routes.py (1)
10814-10834: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPer-piece gain is lost for NAM stages shared between tones (mega-chain dedupe).
NAM/IR stages are deduped by file path only (
dkey = ("nam", stage.get("path"))), and the first occurrence wins. Two tones using the same.namwith differentgain_dbnow collapse into one stage, so the second tone silently plays the first tone'spostGain— a newly audible divergence, unlike the documentedbypassedcaveat. Fold the trim into the dedupe key.🐛 Proposed fix (dedupe key sites, lines ~10949 and ~11036)
- dkey = ("nam", stage.get("path")) + dkey = ("nam", stage.get("path"), stage.get("postGain"))(apply the same change to the pass-2 key so the per-tone slot lookup still resolves)
🤖 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 `@routes.py` around lines 10814 - 10834, Update the NAM/IR mega-chain deduplication keys in both pass-1 and pass-2 to include each stage’s per-piece postgain value (gain_db), alongside the file path. Ensure the corresponding per-tone slot lookup uses the same expanded key so shared NAM files with different gains remain distinct while identical path-and-gain stages continue deduplicating.
🧹 Nitpick comments (2)
routes.py (1)
561-580: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring describes the old bug in the present tense.
The second paragraph reads as a changelog of the previous (broken) implementation — "Only checked
_plugin_dir / vst… this always returned None" — which no longer describes this function. Consider stating current behaviour and moving the rationale to a short historical note.♻️ Suggested wording
- Only checked `_plugin_dir / vst` (the bundle shipped inside the plugin - directory itself), NOT `_downloaded_vst_root()` (where the opt-in - per-platform VST pack actually installs — see _vst_search_roots, which - every OTHER VST lookup in this file correctly checks). On any install that - got its bundled effects via the VST-pack download rather than a plugin.zip - that already contained vst/, this always returned None — the leveler was - silently missing with no error, no "missing" warning surfaced to the user - (native_preset_full's `missing` list DOES report it, but nothing in the - UI highlights that specific entry as "your loudness safety net is off"). + Searches every configured VST root (`_vst_search_roots`), like every other + VST lookup in this file. (Previously only `_plugin_dir / vst` was checked, + so installs that got their bundled effects via the VST-pack download had no + leveler at all — silently, with no UI warning.)🤖 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 `@routes.py` around lines 561 - 580, Update the final leveler function’s docstring to describe its current behavior: it searches all roots returned by _vst_search_roots() for the bundled rack and returns the resolved path when present, otherwise None. Remove the present-tense description of the old missing-search-root bug, or retain it only as a concise historical note.screen.js (1)
8182-8198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the mix-field derivation into one helper.
The
gainDb/pan/phaseInvderivation is now copy-pasted (with an identical comment) in three payload builders. A singlerbPieceMixPayload(p)returning{ gain_db, pan, phase_inv }keeps them from drifting.♻️ Suggested helper
function rbPieceMixPayload(p) { return { gain_db: typeof p._gain_db === 'number' ? p._gain_db : 0, pan: typeof p._pan === 'number' ? p._pan : 0, phase_inv: !!p._phase_inv, }; }then spread it:
return { slot, rs_gear_type: p.type, kind, file, params: {}, assigned_mode: 'manual', bypassed: !!p._bypassed, ...rbPieceMixPayload(p) };🤖 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 `@screen.js` around lines 8182 - 8198, Extract the duplicated gainDb, pan, and phaseInv derivation into a shared rbPieceMixPayload(p) helper returning gain_db, pan, and phase_inv with the existing defaults. Update all three payload builders, including the VST and non-VST branches shown here, to spread this helper result and remove the repeated derivation comments and locals.
🤖 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 `@routes.py`:
- Around line 7145-7151: Update the universal gate logic around the preset
UPDATE and _gate_batch_state so disabling the universal gate does not clear
individually configured per-tone gates. Record which rows the enable sweep
changed, or apply the existing _GATE_UNTOUCHED/default-parameter criterion used
by _auto_gate_for_high_gain, and restrict the off-path update to those rows
while preserving the batch update count.
- Around line 4841-4843: Define a module-level _safe_float helper that converts
valid values to float and returns the default for None, TypeError, or
ValueError. Update the piece-building logic around gain_db and pan to use
_safe_float instead of direct float conversion, preventing malformed client
payloads from aborting the save transaction.
In `@screen.js`:
- Around line 20759-20766: Update the active-tone change handling to flush any
pending debounced save before switching tones. Reuse _rbAdvSliderSaveTimer and
rbStudioPersist via a shared helper or the existing tone-switch path, clearing
the timer and persisting the previous tone before applying the new tone, while
preserving normal 500 ms debounce behavior for slider edits.
---
Outside diff comments:
In `@routes.py`:
- Around line 10814-10834: Update the NAM/IR mega-chain deduplication keys in
both pass-1 and pass-2 to include each stage’s per-piece postgain value
(gain_db), alongside the file path. Ensure the corresponding per-tone slot
lookup uses the same expanded key so shared NAM files with different gains
remain distinct while identical path-and-gain stages continue deduplicating.
---
Nitpick comments:
In `@routes.py`:
- Around line 561-580: Update the final leveler function’s docstring to describe
its current behavior: it searches all roots returned by _vst_search_roots() for
the bundled rack and returns the resolved path when present, otherwise None.
Remove the present-tense description of the old missing-search-root bug, or
retain it only as a concise historical note.
In `@screen.js`:
- Around line 8182-8198: Extract the duplicated gainDb, pan, and phaseInv
derivation into a shared rbPieceMixPayload(p) helper returning gain_db, pan, and
phase_inv with the existing defaults. Update all three payload builders,
including the VST and non-VST branches shown here, to spread this helper result
and remove the repeated derivation comments and locals.
🪄 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: 71192e68-95d0-48f4-9be1-8e8035bc933e
📒 Files selected for processing (3)
routes.pyscreen.htmlscreen.js
| float(p.get("gain_db") or 0.0), | ||
| float(p.get("pan") or 0.0), | ||
| 1 if p.get("phase_inv") else 0, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Non-numeric gain_db/pan in a client payload aborts the save mid-transaction.
pieces comes straight from the request body (/save_preset, /saved_tone/save, …). A non-numeric value raises ValueError after DELETE FROM preset_pieces has already run and before conn.commit(), so the chain is left dropped in an open transaction on the shared long-lived connection — the next unrelated commit() can persist that deletion. Coerce defensively instead.
🛡️ Proposed fix
- float(p.get("gain_db") or 0.0),
- float(p.get("pan") or 0.0),
+ _safe_float(p.get("gain_db")),
+ _safe_float(p.get("pan")),
1 if p.get("phase_inv") else 0,with a small module-level helper:
def _safe_float(v, default: float = 0.0) -> float:
try:
return float(v) if v is not None else default
except (TypeError, ValueError):
return default🤖 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 `@routes.py` around lines 4841 - 4843, Define a module-level _safe_float helper
that converts valid values to float and returns the default for None, TypeError,
or ValueError. Update the piece-building logic around gain_db and pan to use
_safe_float instead of direct float conversion, preventing malformed client
payloads from aborting the save transaction.
| with _lock: | ||
| updated = conn.execute( | ||
| "UPDATE presets SET gate_enabled = ?", (1 if enable else 0,) | ||
| ).rowcount | ||
| conn.commit() | ||
| with _gate_batch_lock: | ||
| _gate_batch_state["updated"] = updated |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Turning the universal gate OFF wipes every individually-configured per-tone gate.
UPDATE presets SET gate_enabled = 0 is unconditional, so disabling the toggle also clears gates the user enabled per-tone via /tone_gate — there's no record of which rows the sweep actually flipped on. Consider recording the pre-sweep state (or only clearing rows whose remaining gate params are still at defaults, mirroring _GATE_UNTOUCHED in _auto_gate_for_high_gain) so the off path is genuinely reversible.
🤖 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 `@routes.py` around lines 7145 - 7151, Update the universal gate logic around
the preset UPDATE and _gate_batch_state so disabling the universal gate does not
clear individually configured per-tone gates. Record which rows the enable sweep
changed, or apply the existing _GATE_UNTOUCHED/default-parameter criterion used
by _auto_gate_for_high_gain, and restrict the off-path update to those rows
while preserving the batch update count.
| let _rbAdvSliderSaveTimer = null; | ||
| function rbAdvDebouncedStudioPersist() { | ||
| if (_rbAdvSliderSaveTimer) clearTimeout(_rbAdvSliderSaveTimer); | ||
| _rbAdvSliderSaveTimer = setTimeout(() => { | ||
| _rbAdvSliderSaveTimer = null; | ||
| try { rbStudioPersist(); } catch (_) {} | ||
| }, 500); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
A pending debounced save can be dropped on a tone switch.
rbStudioPersist() saves whatever rbStudioCurrentChain() returns at fire time, so if the user drags a slider and switches tones within the 500 ms window, the edit to the previous tone is never persisted (and the new tone gets a redundant save). Flushing the timer wherever the active tone changes closes the gap.
🛡️ Suggested guard
+// Call from the tone-switch path (before the active chain changes).
+function rbAdvFlushPendingStudioPersist() {
+ if (!_rbAdvSliderSaveTimer) return;
+ clearTimeout(_rbAdvSliderSaveTimer);
+ _rbAdvSliderSaveTimer = null;
+ try { rbStudioPersist(); } catch (_) {}
+}🧰 Tools
🪛 ast-grep (0.44.1)
[error] 20761-20764: React's useState should not be directly called
Context: setTimeout(() => {
rbAdvSliderSaveTimer = null;
try { rbStudioPersist(); } catch () {}
}, 500)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🤖 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 `@screen.js` around lines 20759 - 20766, Update the active-tone change handling
to flush any pending debounced save before switching tones. Reuse
_rbAdvSliderSaveTimer and rbStudioPersist via a shared helper or the existing
tone-switch path, clearing the timer and persisting the previous tone before
applying the new tone, while preserving normal 500 ms debounce behavior for
slider edits.
1 participant
Adds two requested features and fixes a persistence bug they surfaced during live testing.
Universal noise gate
UPDATE.POST/GET /universal_noise_gate) so a large library doesn't block the UI; the toggle polls progress._persist_preset_chain's INSERT branch now defaults a brand-new preset's gate to theuniversal_noise_gate_enabledsetting instead of a hardcoded off, without touching existing rows unless the caller passes an explicitgate_enabled(preserves the existing per-tone-gate COALESCE-preserve semantics).Advanced tab Save button
Root-cause fix for "tone db changes not applying"
Live-testing the Save button surfaced a deeper, pre-existing bug: the Advanced graph editor's per-piece Pan/Level/Phase-invert controls never round-tripped through the backend at all.
preset_pieceshad no columns for gain/pan/phase - addedgain_db,pan,phase_inv(migrated, guarded ALTER like the existingvst_path/bypassedcolumns).rbStudioChainToPayload,rbPersistMasterChain's inline map,rbPersistTone's inline map) all silently droppedgain_db/pan/phase_invfrom the save payload - sincepreset_piecesis a full DELETE+INSERT on every save (no per-field COALESCE like the gate columns have), any field a serializer forgets is silently reset to 0/off on the very next save. Fixed all three._persist_preset_chain(write) and_load_saved_chain(the single shared read path for every chain type - song tones, saved tones, default tone, master pre/post).rbSeedPieceMixto seed the underscore-prefixed frontend fields (_gain_db/_pan/_phase_inv, matching the existing_bypassedconvention) on every chain-load site.mk()) and the cached-graph-restore path (rbAdvRestore), which had the same "only pan was ever wired up" gap independently of the backend fix - gain/phase were never mirrored from the durable piece onto the node, so even a correct backend value could still render as 0/off from a stale cached graph.rbAdvOnLevelInput/rbAdvOnPanInputnow debounce arbStudioPersist()call - they previously only wrote to the localStorage-cached visual graph, never the tone DB at all. This was the original, narrower half of the same bug.Test plan
Summary by CodeRabbit
New Features
Bug Fixes