fix(skills-ui): show validation errors via .is-visible, not style.display

Smoke-testing the unlock flow surfaced a latent bug: clicking Save
on the edit-skill modal silently no-op'd whenever the
notify-on-complete field had non-JSON content. The error div was
DOM-correct (text content set, role=alert, aria-live=assertive),
but invisible — because the project's modal-error CSS contract is:

  .admin-modal [role="alert"]              { display: none; }
  .admin-modal [role="alert"].is-visible   { display: block; }

…and the JS in submitEditTemplate / submitCreateTemplate was
clearing the inline `display: none` via `el.style.display = ""`.
That falls back to the CSS rule, which still says `display: none`,
so the error never rendered. The user saw no error and the click
felt unresponsive (compounded by the early-return before the
disabled-state reset, which also made Save look broken).

Fixed both skill-modal flows (create + edit) by toggling the
canonical `.is-visible` class instead. Six sites in governance.js:
the two early-return show paths, the two .catch show paths, and
the two modal-open hide-resets.

Scope note: this same bug pattern exists in ~20 other modal error
sites across governance.js and admin.js (create-role, edit-role,
create-policy, edit-policy, github-import, cpp, epp, create-hr,
edit-hr, create-ogp, edit-ogp, mcp-create, mcp-import, mcp-install,
plus admin.js sites that don't go through _showModalError). All
pre-existing, broken silently for who knows how long. Out of scope
for this PR — recommend a follow-up sweep that also normalises
_showModalError's `style.display = "block"` to the same convention.
This commit is contained in:
Patrick Buckley
2026-05-08 14:43:05 -07:00
parent b67da0f48a
commit 627bf06ced
+11 -7
View File
@@ -1072,7 +1072,9 @@ function showCreateTemplateModal() {
document.getElementById("csk-auto-approve").onchange = function () {
document.getElementById("csk-allowed-tools").disabled = this.checked;
};
document.getElementById("create-template-error").style.display = "none";
document
.getElementById("create-template-error")
.classList.remove("is-visible");
// Clear resource list
_pendingResources = [];
_renderPendingResources();
@@ -1115,7 +1117,7 @@ function submitCreateTemplate() {
if (!name || !content) {
var e = document.getElementById("create-template-error");
e.textContent = "Name and content are required";
e.style.display = "";
e.classList.add("is-visible");
return;
}
var varList = _detectTemplateVars(content);
@@ -1157,7 +1159,7 @@ function submitCreateTemplate() {
} catch (ne) {
var ne2 = document.getElementById("create-template-error");
ne2.textContent = "Notify on completion: " + ne.message;
ne2.style.display = "";
ne2.classList.add("is-visible");
return;
}
}
@@ -1240,7 +1242,7 @@ function submitCreateTemplate() {
.catch(function (e) {
var el = document.getElementById("create-template-error");
el.textContent = e.message;
el.style.display = "";
el.classList.add("is-visible");
})
.finally(function () {
document.getElementById("ctm-submit").disabled = false;
@@ -1322,7 +1324,9 @@ function showEditTemplateModal(tmplId) {
document.getElementById("esk-auto-approve").onchange = function () {
document.getElementById("esk-allowed-tools").disabled = this.checked;
};
document.getElementById("edit-template-error").style.display = "none";
// The CSS contract for .admin-modal [role="alert"] is hide-by-default,
// .is-visible to show — so toggling style.display does nothing here.
document.getElementById("edit-template-error").classList.remove("is-visible");
// Scan report section
var scanSection = document.getElementById("etm-scan-section");
if (scanSection) {
@@ -1883,7 +1887,7 @@ function submitEditTemplate() {
} catch (ne) {
var ne3 = document.getElementById("edit-template-error");
ne3.textContent = "Notify on completion: " + ne.message;
ne3.style.display = "";
ne3.classList.add("is-visible");
return;
}
}
@@ -1937,7 +1941,7 @@ function submitEditTemplate() {
.catch(function (e) {
var el = document.getElementById("edit-template-error");
el.textContent = e.message;
el.style.display = "";
el.classList.add("is-visible");
})
.finally(function () {
document.getElementById("etm-submit").disabled = false;