mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-10-02 19:21:50 +00:00
fix(settings): partial-merge instrument_profiles; clamp tuning on string-count switch
Two partial-update follow-ups: - save_settings normalized a POSTed instrument_profiles by FILLING every omitted profile with defaults and replacing wholesale, so a one-profile update reset the others. Validate each PROVIDED profile individually and merge the partial over the persisted set inside the lock — /api/settings is partial-merge. - the string-count picker posted only string_count, so the backend silently reset a now-invalid tuning to Standard while the UI kept the old value (settings/tuner desync). Clamp + post the valid tuning too, mirroring the instrument-switch path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -45,7 +45,8 @@ from audio import find_wem_files, convert_wem
|
||||
from tunings import (
|
||||
DEFAULT_REFERENCE_PITCH, DEFAULT_TUNINGS, PROFILE_IDS, PROFILE_PATHWAYS,
|
||||
apply_flat_instrument_patch_to_profiles, apply_reference_pitch,
|
||||
normalize_instrument_profiles, settings_with_instrument_profiles, tuning_name,
|
||||
normalize_instrument_profile, normalize_instrument_profiles,
|
||||
settings_with_instrument_profiles, tuning_name,
|
||||
)
|
||||
import sloppak as sloppak_mod
|
||||
import drums as drums_mod
|
||||
@@ -9331,13 +9332,25 @@ def save_settings(data: dict):
|
||||
return {"error": "pathway must be one of songs, practice, learn, studio"}
|
||||
updates["pathway"] = raw
|
||||
|
||||
_profile_patch = None
|
||||
if "instrument_profiles" in data:
|
||||
raw = data["instrument_profiles"]
|
||||
if raw is not None:
|
||||
profiles, error = normalize_instrument_profiles(raw)
|
||||
if error:
|
||||
return {"error": error}
|
||||
updates["instrument_profiles"] = profiles
|
||||
if not isinstance(raw, dict):
|
||||
return {"error": "instrument_profiles must be an object"}
|
||||
# Validate each PROVIDED profile individually and keep the patch
|
||||
# PARTIAL — /api/settings is a partial-merge endpoint, so updating one
|
||||
# profile must NOT reset the others to defaults. Merged over the
|
||||
# persisted profiles inside the lock below (not via the wholesale
|
||||
# `updates` merge, which would clobber the unspecified ones).
|
||||
_profile_patch = {}
|
||||
for _pid, _praw in raw.items():
|
||||
if _pid not in PROFILE_IDS:
|
||||
return {"error": f"unknown instrument profile: {_pid}"}
|
||||
_prof, _perr = normalize_instrument_profile(_pid, _praw)
|
||||
if _perr:
|
||||
return {"error": _perr}
|
||||
_profile_patch[_pid] = _prof
|
||||
if "active_instrument_profile" in data:
|
||||
raw = data["active_instrument_profile"]
|
||||
if raw is not None:
|
||||
@@ -9360,6 +9373,15 @@ def save_settings(data: dict):
|
||||
if cfg is None:
|
||||
cfg = _default_settings()
|
||||
cfg.update(updates)
|
||||
if _profile_patch is not None:
|
||||
# Merge the validated partial over the persisted profiles so a
|
||||
# single-profile update leaves the others intact (a fresh config
|
||||
# falls back to the built-in defaults for the unspecified ones).
|
||||
_existing, _ = normalize_instrument_profiles(cfg.get("instrument_profiles"))
|
||||
if _existing is None:
|
||||
_existing = {}
|
||||
_existing.update(_profile_patch)
|
||||
cfg["instrument_profiles"] = _existing
|
||||
# Only canonicalize/persist the instrument profiles when this save
|
||||
# actually touches them (or the config already carries them). GET always
|
||||
# virtualizes profiles via settings_with_instrument_profiles, so a save
|
||||
|
||||
+11
-2
@@ -500,8 +500,17 @@
|
||||
renderInstrument(); keepOpen();
|
||||
}));
|
||||
menu.querySelectorAll('[data-pill="strings"]').forEach((b) => b.addEventListener('click', async () => {
|
||||
await saveSettings({ string_count: Number(b.getAttribute('data-val')) });
|
||||
setWorkingInstrument(settings.instrument, settings.string_count);
|
||||
const newSc = Number(b.getAttribute('data-val'));
|
||||
// Clamp the tuning to one valid for the new string count and post it
|
||||
// alongside string_count — otherwise the backend silently resets a
|
||||
// now-invalid tuning to Standard while this UI keeps showing the old
|
||||
// one (settings/tuner desync). Mirrors the instrument-switch clamp.
|
||||
const tunings = _tuningsForInstrument(settings.instrument, newSc);
|
||||
await saveSettings({
|
||||
string_count: newSc,
|
||||
tuning: tunings.includes(settings.tuning) ? settings.tuning : (tunings[0] || settings.tuning),
|
||||
});
|
||||
setWorkingInstrument(settings.instrument, newSc);
|
||||
renderInstrument(); keepOpen();
|
||||
}));
|
||||
menu.querySelector('[data-inst-tuning]').addEventListener('change', (e) => saveSettings({ tuning: e.target.value }));
|
||||
|
||||
@@ -834,6 +834,23 @@ def test_reset_clears_requested_keys(client, tmp_path):
|
||||
assert cfg["demucs_server_url"] == "http://demucs.example:9000"
|
||||
|
||||
|
||||
def test_partial_instrument_profiles_update_preserves_others(client, tmp_path):
|
||||
# /api/settings is a partial-merge endpoint, so a POST that carries only ONE
|
||||
# instrument profile must not reset the others to defaults.
|
||||
gl = client.get("/api/settings").json()["instrument_profiles"]["guitar-lead"]
|
||||
gl = dict(gl); gl["tuning"] = "Drop D"
|
||||
client.post("/api/settings", json={"instrument_profiles": {"guitar-lead": gl}})
|
||||
assert (client.get("/api/settings").json()["instrument_profiles"]
|
||||
["guitar-lead"]["tuning"] == "Drop D")
|
||||
# Now update ONLY bass (Drop D is valid for a 4-string bass).
|
||||
bass = client.get("/api/settings").json()["instrument_profiles"]["bass"]
|
||||
bass = dict(bass); bass["tuning"] = "Drop D"
|
||||
client.post("/api/settings", json={"instrument_profiles": {"bass": bass}})
|
||||
out = client.get("/api/settings").json()["instrument_profiles"]
|
||||
assert out["guitar-lead"]["tuning"] == "Drop D", "the untouched profile survived"
|
||||
assert out["bass"]["tuning"] == "Drop D"
|
||||
|
||||
|
||||
def test_active_profile_switch_on_fresh_config(client, tmp_path):
|
||||
# A fresh config has no instrument_profiles; an explicit active-profile
|
||||
# switch must be honored, not overwritten by the profile inferred from the
|
||||
|
||||
Reference in New Issue
Block a user