From 305f6a3912fddf2ca488f553226dff145d21888d Mon Sep 17 00:00:00 2001 From: "B.D. Murphy" <161863146+bdmurphy73@users.noreply.github.com> Date: Sat, 10 Oct 2026 20:04:41 -0700 Subject: [PATCH] Fix Save & Re-voice silently discarding every speaker edit Reported: editing a speaker, then "Save & Re-voice", did nothing. This is a regression from e810b49 (the voice-queue commit), which deleted the body of queueVoiceRegenOnSave but left the edit form calling it as onsubmit="return queueVoiceRegenOnSave(...)". A JS handler that falls off the end returns undefined, which is falsy, so the browser cancelled the submit. The PUT never fired: no request, no error, no change. It affected every field on the speaker edit form - name, gender, traits, accent - not just re-voicing. - Re-voicing is now hooked to hx-on::after-request, so the new profile is on disk before the voice is rebuilt from it, and a failed save does not queue work. The handler re-reads the name from the swapped row, so a rename regenerates under the new name instead of the old one. - The backend was never at fault: a full round-trip through PUT /api/speakers/0 with the record's own values leaves every field byte-identical. - New tests/test_speaker_edit_submit.py guards the class of bug, not just this instance: no onsubmit="return ..." in the row template, and any onsubmit that returns a value anywhere must return a literal true/false. Comments are stripped before scanning, since the hazard is documented in one. Verified by reintroducing the bug - 3 of the 4 tests fail. Suite: 688 passed, 8 pre-existing. --- tests/test_speaker_edit_submit.py | 95 ++++++++++++++++++++++ umbraaudio/web/templates/_speaker_row.html | 8 +- umbraaudio/web/templates/speakers.html | 13 ++- 3 files changed, 114 insertions(+), 2 deletions(-) create mode 100644 tests/test_speaker_edit_submit.py diff --git a/tests/test_speaker_edit_submit.py b/tests/test_speaker_edit_submit.py new file mode 100644 index 0000000..70dfb7e --- /dev/null +++ b/tests/test_speaker_edit_submit.py @@ -0,0 +1,95 @@ +"""Guards on the speaker edit form's submit behaviour. + +Regression: the voice-queue work emptied ``queueVoiceRegenOnSave`` but left +``onsubmit="return queueVoiceRegenOnSave(...)"`` in the edit form. A handler +that falls off the end returns ``undefined``, which is falsy, so the browser +cancelled the submit and every speaker edit was silently discarded - the save +never reached the server, so nothing looked broken and nothing errored. + +An ``onsubmit`` that returns a value must return a boolean deliberately. +""" +from __future__ import annotations + +import re +from pathlib import Path + +TEMPLATES = Path(__file__).resolve().parents[1] / "umbraaudio" / "web" / "templates" + +# onsubmit="return someFn(...)" - the cancelling shape +ONSUBMIT_RETURN = re.compile(r'onsubmit="return\s+([A-Za-z_$][\w$]*)\s*\(') +FUNC_DEF = re.compile( + r"function\s+([A-Za-z_$][\w$]*)\s*\([^)]*\)\s*\{", re.MULTILINE +) +# Comments quote these patterns when documenting the hazard, so strip them +# first or the scanner reports the documentation as if it were live markup. +COMMENTS = re.compile(r"\{#.*?#\}|", re.DOTALL) + + +def _strip_comments(text: str) -> str: + return COMMENTS.sub("", text) + + +def _all_template_text() -> str: + return _strip_comments( + "\n".join(p.read_text() for p in TEMPLATES.rglob("*.html")) + ) + + +def _function_bodies(text: str) -> dict[str, str]: + """Map function name -> body text, brace-matched.""" + bodies: dict[str, str] = {} + for m in FUNC_DEF.finditer(text): + start = m.end() + depth = 1 + i = start + while i < len(text) and depth: + if text[i] == "{": + depth += 1 + elif text[i] == "}": + depth -= 1 + i += 1 + bodies[m.group(1)] = text[start : i - 1] + return bodies + + +def test_no_onsubmit_handler_can_cancel_the_speaker_edit() -> None: + """The edit form must never gate its own submission on a JS return value.""" + row = _strip_comments((TEMPLATES / "_speaker_row.html").read_text()) + assert 'onsubmit="return' not in row, ( + "_speaker_row.html uses onsubmit=\"return ...\"; if that handler ever " + "returns a falsy value the browser drops the edit without any error" + ) + + +def test_every_onsubmit_return_handler_returns_a_boolean() -> None: + """Repo-wide: any onsubmit returning a value must decide explicitly.""" + text = _all_template_text() + bodies = _function_bodies(text) + for name in sorted(set(ONSUBMIT_RETURN.findall(text))): + assert name in bodies, f"onsubmit returns {name}() but it is not defined" + body = bodies[name] + returns_bool = re.search(r"return\s+(true|false)\b", body) + assert returns_bool, ( + f"onsubmit=\"return {name}(...)\" but {name}() never returns an " + "explicit true/false - falling off the end yields undefined, which " + "cancels the submit and silently discards the user's edit" + ) + + +def test_voice_regen_on_save_is_not_empty() -> None: + """The specific function that was gutted.""" + bodies = _function_bodies((TEMPLATES / "speakers.html").read_text()) + assert "queueVoiceRegenOnSave" in bodies, "handler is missing entirely" + body = bodies["queueVoiceRegenOnSave"].strip() + assert body, "queueVoiceRegenOnSave has an empty body" + assert "enqueueVoice" in body, "it must actually queue a regeneration" + + +def test_edit_form_queues_regen_after_a_successful_save() -> None: + """Re-voice must run after the new profile is persisted, not before.""" + row = _strip_comments((TEMPLATES / "_speaker_row.html").read_text()) + assert "queueVoiceRegenOnSave" in row + assert "hx-on::after-request" in row, ( + "regeneration should be hooked to after-request so the saved profile " + "is on disk before the voice is rebuilt from it" + ) diff --git a/umbraaudio/web/templates/_speaker_row.html b/umbraaudio/web/templates/_speaker_row.html index 30bca91..8001ed3 100644 --- a/umbraaudio/web/templates/_speaker_row.html +++ b/umbraaudio/web/templates/_speaker_row.html @@ -171,7 +171,13 @@
+ hx-on::after-request="if(event.detail.successful){ queueVoiceRegenOnSave({{ speaker_idx }}); }"> + {# Re-voice is hooked to after-request, NOT onsubmit. An + onsubmit="return fn(...)" whose fn falls through returns + undefined, which is falsy, so the browser cancels the whole + submit and silently discards every edit. Queueing here also + guarantees the saved profile is on disk before the voice is + regenerated from it. #}
diff --git a/umbraaudio/web/templates/speakers.html b/umbraaudio/web/templates/speakers.html index b194c8d..d05778c 100644 --- a/umbraaudio/web/templates/speakers.html +++ b/umbraaudio/web/templates/speakers.html @@ -33,7 +33,18 @@ var _regenPoll = null; var _regenIdx = null; -function queueVoiceRegenOnSave(idx, name) { +// Runs after a successful PUT, so speakers.json already holds the new profile. +// The name is re-read from the swapped row because the reviewer may have just +// renamed the speaker, and regenerating under the old name would voice a +// speaker that no longer exists. +function queueVoiceRegenOnSave(idx) { + var row = document.getElementById('speaker-row-' + idx); + var label = row ? row.querySelector('strong') : null; + var name = label ? label.textContent.trim() : ''; + if (!name) { + return; + } + enqueueVoice(name, idx); } // Voice regeneration is queue-driven (see enqueueVoice below); the old