mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-07-22 12:52:29 +00:00
fix(settings): don't let a stale DLC path block saving the Demucs server address (#795)
Some checks are pending
ship-ci / ci (push) Waiting to run
Some checks are pending
ship-ci / ci (push) Waiting to run
The v3 Settings "Save" button posts dlc_dir together with demucs_server_url, default_arrangement and av_offset_ms in one request. POST /api/settings validated dlc_dir first and early-returned "DLC directory not found" before it ever processed demucs_server_url, so on a machine whose DLC path doesn't resolve (fresh install, unplugged/network drive, a path carried over from another machine) setting the Demucs server address silently failed — reported in got-feedBack/feedBack-demucs-server#3 (macOS 07-05 nightly). - server: a non-resolving dlc_dir is now recorded as a warning and skipped rather than aborting the whole POST, so the co-submitted keys still persist. The bad path is surfaced via a new additive `warnings` field and folded into `message` so the settings status line still shows it. - client (v3): the Demucs input now autosaves on blur/enter via a single-key persistSetting POST, like every other v3 setting, so it never depends on the coupled Save button. - tests: cover the decoupling and the unchanged happy path. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
a20dca21bb
commit
69c8ad4e0c
24
server.py
24
server.py
@ -10765,6 +10765,9 @@ def save_settings(data: dict):
|
||||
config_file = CONFIG_DIR / "config.json"
|
||||
updates: dict = {}
|
||||
messages: list[str] = []
|
||||
# Named dlc_warnings (not `warnings`) so it can't shadow the module-level
|
||||
# `import warnings` used elsewhere in this file.
|
||||
dlc_warnings: list[str] = []
|
||||
|
||||
if "dlc_dir" in data:
|
||||
dlc_path = data["dlc_dir"]
|
||||
@ -10784,7 +10787,16 @@ def save_settings(data: dict):
|
||||
if f.suffix.lower() in sloppak_mod.SONG_EXTS)
|
||||
messages.append(f"DLC folder: {count} song files found")
|
||||
else:
|
||||
return {"error": f"DLC directory not found: {dlc_path}"}
|
||||
# A non-resolving DLC path (a stale value, an unplugged
|
||||
# external/network drive, or a path carried over from another
|
||||
# machine) must NOT abort the whole POST. saveSettings() bundles
|
||||
# dlc_dir together with demucs_server_url / default_arrangement /
|
||||
# av_offset_ms in a single request, so an early `return` here
|
||||
# silently dropped every co-submitted key — this is the "can't
|
||||
# set the Demucs server address" report (feedBack-demucs-server
|
||||
# #3). Record it as a warning, skip persisting dlc_dir, and keep
|
||||
# validating the rest so the other settings still save.
|
||||
dlc_warnings.append(f"DLC directory not found: {dlc_path}")
|
||||
|
||||
# Both of these are consumed downstream as strings (e.g.
|
||||
# demucs_server_url.rstrip('/')), so reject non-string shapes
|
||||
@ -11041,7 +11053,15 @@ def save_settings(data: dict):
|
||||
return {"error": str(exc)}
|
||||
cfg = settings_with_instrument_profiles(cfg)
|
||||
_atomic_write_file(config_file, json.dumps(cfg, indent=2).encode("utf-8"))
|
||||
return {"message": ". ".join(messages) if messages else "Settings saved"}
|
||||
resp = {"message": ". ".join(messages) if messages else "Settings saved"}
|
||||
if dlc_warnings:
|
||||
# `warnings` is an additive response field (existing clients read
|
||||
# `message || error`); fold the text into `message` too so the current
|
||||
# settings status line still surfaces the bad DLC path even though the
|
||||
# rest of the save succeeded.
|
||||
resp["warnings"] = dlc_warnings
|
||||
resp["message"] = resp["message"] + " — " + "; ".join(dlc_warnings)
|
||||
return resp
|
||||
|
||||
|
||||
# Keys a client "Reset {category}" action may clear. Resetting removes the key
|
||||
|
||||
@ -641,7 +641,12 @@
|
||||
</div>
|
||||
</div>
|
||||
<div class="fb-srow-control">
|
||||
<!-- Autosave on blur/enter via a single-key POST (like every other v3
|
||||
setting), so setting the address never depends on the shared Save
|
||||
button — whose bundled dlc_dir could otherwise block it. Save button
|
||||
kept for discoverability. -->
|
||||
<input type="text" id="demucs-server-url" placeholder="http://192.168.1.100:7865"
|
||||
onchange="persistSetting('demucs_server_url', this.value.trim())"
|
||||
class="bg-dark-700 border border-gray-800 rounded-xl px-4 py-2.5 text-sm text-gray-300 placeholder-gray-600 focus:border-accent/50 outline-none">
|
||||
<button onclick="saveSettings()" class="bg-accent hover:bg-accent-light px-6 py-2.5 rounded-xl text-sm font-semibold text-white transition">Save</button>
|
||||
</div>
|
||||
|
||||
@ -219,6 +219,47 @@ def test_dlc_dir_empty_string_clears(client, tmp_path):
|
||||
assert _read_cfg(tmp_path)["dlc_dir"] == ""
|
||||
|
||||
|
||||
def test_unresolvable_dlc_dir_does_not_block_other_keys(client, tmp_path):
|
||||
# Regression (feedBack-demucs-server#3): the v3 "Save" button next to the
|
||||
# Demucs field bundles dlc_dir with demucs_server_url in one POST. A DLC
|
||||
# path that doesn't resolve on THIS machine (stale value / unplugged drive)
|
||||
# must not abort the whole request — the co-submitted demucs_server_url has
|
||||
# to persist, and the bad path is surfaced as a warning rather than a hard
|
||||
# error that drops every other key.
|
||||
missing = str(tmp_path / "does-not-exist")
|
||||
r = client.post("/api/settings", json={
|
||||
"dlc_dir": missing,
|
||||
"demucs_server_url": "http://demucs.example:7865",
|
||||
"default_arrangement": "Lead",
|
||||
})
|
||||
assert r.status_code == 200
|
||||
body = r.json()
|
||||
# No hard error; the bad path is reported as a warning.
|
||||
assert "error" not in body
|
||||
assert any("does-not-exist" in w for w in body.get("warnings", []))
|
||||
assert "does-not-exist" in body["message"]
|
||||
cfg = _read_cfg(tmp_path)
|
||||
# The valid keys persisted...
|
||||
assert cfg["demucs_server_url"] == "http://demucs.example:7865"
|
||||
assert cfg["default_arrangement"] == "Lead"
|
||||
# ...and the unresolvable path was NOT written.
|
||||
assert cfg.get("dlc_dir", "") != missing
|
||||
|
||||
|
||||
def test_valid_dlc_dir_still_reports_song_count(client, tmp_path):
|
||||
# The happy path is unchanged: a resolvable DLC dir persists and the
|
||||
# response message still carries the "N song files found" summary (no
|
||||
# warnings key when nothing went wrong).
|
||||
dlc = tmp_path / "dlc"
|
||||
dlc.mkdir()
|
||||
r = client.post("/api/settings", json={"dlc_dir": str(dlc)})
|
||||
assert r.status_code == 200
|
||||
body = r.json()
|
||||
assert "warnings" not in body
|
||||
assert "song files found" in body["message"]
|
||||
assert _read_cfg(tmp_path)["dlc_dir"] == str(dlc)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("key", ["default_arrangement", "demucs_server_url"])
|
||||
def test_string_key_null_is_noop(client, tmp_path, key):
|
||||
# Match the dlc_dir contract: null preserves the on-disk value.
|
||||
|
||||
Loading…
Reference in New Issue
Block a user