From 627bf06cedf5b60f7ea867ad3ab33c272cb02423 Mon Sep 17 00:00:00 2001
From: Patrick Buckley <eous@users.noreply.github.com>
Date: Fri, 8 May 2026 14:43:05 -0700
Subject: [PATCH] fix(skills-ui): show validation errors via .is-visible, not
 style.display
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 turnstone/console/static/governance.js | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/turnstone/console/static/governance.js b/turnstone/console/static/governance.js
index 0c5b0868..8fce545c 100644
--- a/turnstone/console/static/governance.js
+++ b/turnstone/console/static/governance.js
@@ -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;