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.
This commit is contained in:
95
tests/test_speaker_edit_submit.py
Normal file
95
tests/test_speaker_edit_submit.py
Normal file
@@ -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"
|
||||
)
|
||||
@@ -171,7 +171,13 @@
|
||||
<form hx-put="/api/speakers/{{ speaker_idx }}"
|
||||
hx-target="#speaker-row-{{ speaker_idx }}"
|
||||
hx-swap="outerHTML"
|
||||
onsubmit="return queueVoiceRegenOnSave({{ speaker_idx }}, '{{ speaker.primary_name|e }}')">
|
||||
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. #}
|
||||
<div style="display:grid; grid-template-columns:1fr 1fr 1fr 1fr; gap:0.5rem; margin-bottom:0.5rem">
|
||||
<input type="text" name="primary_name" value="{{ speaker.primary_name }}" required
|
||||
style="padding:0.4rem; background:#1a1a2e; border:1px solid #333; border-radius:3px; color:#e0e0e0; font-size:0.85rem">
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user