fix(form): address PR review findings

Harden generated form behavior, accessibility, and CI dependency handling after review.

.github/workflows/ci.yml

@@ -16,9 +16,11 @@ jobs:
     runs-on: ubuntu-latest
     timeout-minutes: 15
     steps:
-      - uses: actions/checkout@v7
+      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
+        with:
+          persist-credentials: false
 
-      - uses: actions/setup-python@v7
+      - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7
         with:
           python-version: "3.12"
 

src/server/generate_form.py

@@ -58,6 +58,8 @@ def generate_form(
     use_google_auth = bool(google_client_id)
     auth_mode = "google" if use_google_auth else "local"
 
+    # apiKey is intentionally embedded in the generated HTML and must be treated
+    # as a shared client token, not a secret or user-authentication credential.
     form_config = {
         "authMode": auth_mode,
         "meals": menu["meals"],
@@ -112,7 +114,9 @@ def main():
         "--api-url", default="", help="API Gateway base URL (cloud mode)"
     )
     parser.add_argument(
-        "--api-key", default="", help="API key for order submission (cloud mode)"
+        "--api-key",
+        default="",
+        help="Shared client token embedded in the generated form (cloud mode)",
     )
     parser.add_argument(
         "--bulk-discount",

src/server/templates/admin.js

@@ -123,7 +123,21 @@ function buildViewActions(idx, email, name) {
   return wrap;
 }
 
+function validateAdminData(data) {
+  if (!data || !Array.isArray(data.orders)) {
+    throw new Error('Invalid orders response');
+  }
+  if (
+    data.orders.length > 0
+    && (!Number.isFinite(data.total_employees) || !Number.isFinite(data.grand_total))
+  ) {
+    throw new Error('Invalid order aggregates');
+  }
+  return data;
+}
+
 function renderAdminTable(data) {
+  validateAdminData(data);
   lastAdminData = data;
   const hasOrders = !!(data.orders && data.orders.length > 0);
   const dlBtn = document.getElementById('admin-download-btn');
@@ -186,7 +200,12 @@ function renderAdminTable(data) {
       MEALS.forEach(m => { menuPrices[m.name] = m.price; });
       const newTotal = editNames.reduce((sum, name) => {
         const price = menuPrices[name] || 0;
-        return sum + price * (editQuantities[name] || 0);
+        const employeePrice = window.FormUtils.employeePrice(
+          price,
+          window.CONFIG.bulkDiscount,
+          window.CONFIG.companySubsidy
+        );
+        return sum + employeePrice * (editQuantities[name] || 0);
       }, 0);
       totalHtml = '$' + newTotal.toFixed(2);
       tr.appendChild(empTd);
@@ -262,7 +281,8 @@ function downloadOrderList() {
   }));
   const names = Object.keys(totals).sort((a, b) => a.localeCompare(b));
   const esc = v => {
-    const s = String(v);
+    const value = String(v);
+    const s = /^[=+\-@]/.test(value) ? "'" + value : value;
     return /[",\n]/.test(s) ? '"' + s.replace(/"/g, '""') + '"' : s;
   };
   const rows = [['Item', 'Quantity']].concat(names.map(n => [n, totals[n]]));
@@ -305,15 +325,25 @@ function downloadSummaryPdf() {
 
 function adminStartEdit(idx, email) {
   fetch(ADMIN_URL + '?week=' + encodeURIComponent(currentAdminWeek), { headers: adminHeaders() })
-    .then(r => r.json())
+    .then(r => {
+      if (!r.ok) throw new Error('Request failed with status ' + r.status);
+      return r.json();
+    })
     .then(data => {
+      validateAdminData(data);
       lastAdminData = data;
       const order = data.orders.find(o => o.employee_email === email);
-      if (!order) return;
+      if (!order || !Array.isArray(order.items)) {
+        throw new Error('Order not found');
+      }
       editingOrder = email;
       editQuantities = {};
       order.items.forEach(i => { editQuantities[i.name] = i.quantity; });
       renderAdminTable(data);
+    })
+    .catch(e => {
+      if (lastAdminData) renderAdminTable(lastAdminData);
+      alert('Failed to load order: ' + e.message);
     });
 }
 

src/server/templates/form.css

@@ -6,6 +6,9 @@
   --radius-card: 12px;
   --radius-control: 8px;
   --shadow-card: 0 1px 3px rgba(0,0,0,0.08);
+  --user-avatar-size: 40px;
+  --user-bar-gap: 12px;
+  --user-bar-indent: calc(var(--user-avatar-size) + var(--user-bar-gap));
 }
 
 * { margin: 0; padding: 0; box-sizing: border-box; }
@@ -75,11 +78,11 @@ header p { font-size: 0.85rem; opacity: 0.8; }
   margin-bottom: 12px;
 }
 
-.user-bar { display: flex; align-items: center; gap: 12px; }
+.user-bar { display: flex; align-items: center; gap: var(--user-bar-gap); }
 
 .user-avatar {
-  width: 40px;
-  height: 40px;
+  width: var(--user-avatar-size);
+  height: var(--user-avatar-size);
   border-radius: 50%;
   object-fit: cover;
   display: none;
@@ -147,6 +150,7 @@ header p { font-size: 0.85rem; opacity: 0.8; }
   margin-bottom: 4px;
   display: -webkit-box;
   -webkit-box-orient: vertical;
+  line-clamp: 3;
   -webkit-line-clamp: 3;
   overflow: hidden;
 }
@@ -342,13 +346,12 @@ header p { font-size: 0.85rem; opacity: 0.8; }
   font-size: 1rem;
 }
 
-@media (max-width: 479px) {
-  .user-bar { flex-wrap: wrap; }
-  .user-meta { flex-basis: calc(100% - 52px); }
-  .user-bar #admin-btn { margin-left: 52px; }
-}
-
 @media (max-width: 480px) {
+  .user-bar { flex-wrap: wrap; }
+  .user-meta { flex-basis: calc(100% - var(--user-bar-indent)); }
+  .user-bar #admin-btn { margin-left: var(--user-bar-indent); }
+  .user-avatar.is-hidden + .user-meta { flex-basis: 100%; }
+  .user-avatar.is-hidden ~ #admin-btn { margin-left: 0; }
   .meal-card {
     display: grid;
     grid-template-columns: 80px minmax(0, 1fr);

src/server/templates/form.html.j2

@@ -14,22 +14,22 @@
 </head>
 <body>
 <!-- FORM_HTML_MARKER -->
-<div class="closed-overlay" id="closed-overlay">
-  <div class="closed-card">
+<div class="closed-overlay" id="closed-overlay" role="dialog" aria-modal="true" aria-labelledby="closed-title" aria-describedby="closed-description" aria-hidden="true">
+  <div class="closed-card" tabindex="-1">
     <div class="logo">&#127869;</div>
-    <h1>Orders Are Closed</h1>
-    <p>Orders open again Monday at 8:00 AM ET</p>
+    <h1 id="closed-title">Orders Are Closed</h1>
+    <p id="closed-description">Orders open again Monday at 8:00 AM ET</p>
     <div class="countdown" id="countdown"></div>
     <div class="countdown-label">until orders open</div>
   </div>
 </div>
 
 {% if config.authMode == 'google' %}
-<div class="auth-overlay" id="auth-overlay">
-  <div class="auth-card">
+<div class="auth-overlay" id="auth-overlay" role="dialog" aria-modal="true" aria-labelledby="auth-title" aria-describedby="auth-description" aria-hidden="false">
+  <div class="auth-card" tabindex="-1">
     <div class="logo">&#127869;</div>
-    <h1>Sea Haven Meal Order</h1>
-    <p>Sign in with your company Google account to place your order.</p>
+    <h1 id="auth-title">Sea Haven Meal Order</h1>
+    <p id="auth-description">Sign in with your company Google account to place your order.</p>
     <div id="g-signin-btn" class="g-signin-wrap"></div>
   </div>
 </div>
@@ -144,10 +144,14 @@
 window.CONFIG = {{ config_json|safe }};
 </script>
 <script>
+{
 {{ form_js|safe }}
+}
 </script>
 <script>
+{
 {{ admin_js|safe }}
+}
 </script>
 </body>
 </html>

src/server/templates/form.js

@@ -7,6 +7,8 @@ const STATUS_URL = CONFIG.statusUrl;
 const ROSTER_URL = CONFIG.rosterUrl;
 const ADMIN_URL = CONFIG.adminUrl;
 const ADMIN_PDF_URL = CONFIG.adminPdfUrl;
+// Embedded in the generated form by design; this is a shared client token,
+// not a secret or a user-authentication credential.
 const API_KEY = CONFIG.apiKey;
 const WEEK = CONFIG.week;
 const BULK_DISCOUNT = CONFIG.bulkDiscount;
@@ -27,6 +29,9 @@ let googleCredential = null;
 let googleUser = null;
 let reopenAtMs = null;
 let mealsListDelegationBound = false;
+let countdownInterval = null;
+let authOverlayReturnFocus = null;
+let closedOverlayReturnFocus = null;
 
 window.googleCredential = null;
 
@@ -40,10 +45,14 @@ function escapeHtml(str) {
     .replace(/'/g, ''');
 }
 
-function employeePrice(retail) {
+function employeePrice(
+  retail,
+  bulkDiscount = BULK_DISCOUNT,
+  companySubsidy = COMPANY_SUBSIDY
+) {
   if (!retail) return 0;
-  const bulkPrice = Math.round(retail * (1 - BULK_DISCOUNT / 100) * 100 + 1e-8) / 100;
-  const empPrice = Math.round(bulkPrice * (1 - COMPANY_SUBSIDY / 100) * 100 + 1e-8) / 100;
+  const bulkPrice = Math.round(retail * (1 - bulkDiscount / 100) * 100 + 1e-8) / 100;
+  const empPrice = Math.round(bulkPrice * (1 - companySubsidy / 100) * 100 + 1e-8) / 100;
   return empPrice;
 }
 
@@ -53,6 +62,41 @@ function syncGoogleCredential() {
   window.googleCredential = googleCredential;
 }
 
+function restoreOverlayFocus(previous, fallbackSelector) {
+  if (
+    previous
+    && previous !== document.body
+    && previous.isConnected
+    && !previous.closest('.is-hidden')
+  ) {
+    previous.focus();
+    return;
+  }
+  const fallback = document.querySelector(fallbackSelector);
+  if (fallback) fallback.focus();
+}
+
+function showAuthOverlay() {
+  const overlay = document.getElementById('auth-overlay');
+  if (!overlay) return;
+  if (overlay.classList.contains('is-hidden')) {
+    authOverlayReturnFocus = document.activeElement;
+  }
+  overlay.classList.remove('is-hidden');
+  overlay.setAttribute('aria-hidden', 'false');
+  overlay.querySelector('.auth-card')?.focus();
+}
+
+function hideAuthOverlay() {
+  const overlay = document.getElementById('auth-overlay');
+  if (!overlay) return;
+  const previous = authOverlayReturnFocus;
+  authOverlayReturnFocus = null;
+  restoreOverlayFocus(previous, '#search');
+  overlay.classList.add('is-hidden');
+  overlay.setAttribute('aria-hidden', 'true');
+}
+
 function buildItems() {
   return Object.entries(quantities).map(([i, qty]) => ({
     name: MEALS[i].name,
@@ -89,10 +133,10 @@ function handleCredentialResponse(response) {
   const payload = JSON.parse(new TextDecoder().decode(payloadBytes));
   googleUser = { name: payload.name, email: payload.email };
 
-  document.getElementById('auth-overlay').classList.add('is-hidden');
   document.getElementById('app').classList.remove('is-hidden');
   const footer = document.getElementById('sticky-footer');
   if (footer) footer.classList.remove('is-hidden');
+  hideAuthOverlay();
   document.getElementById('user-name').textContent = payload.name;
   document.getElementById('user-email').textContent = payload.email;
   if (payload.picture) {
@@ -118,10 +162,10 @@ function signOut() {
   if (CONFIG.authMode === 'google' && typeof google !== 'undefined' && google.accounts) {
     google.accounts.id.disableAutoSelect();
   }
-  document.getElementById('auth-overlay').classList.remove('is-hidden');
   document.getElementById('app').classList.add('is-hidden');
   const footer = document.getElementById('sticky-footer');
   if (footer) footer.classList.add('is-hidden');
+  showAuthOverlay();
   if (formClosed) showClosedOverlay();
 }
 
@@ -403,8 +447,12 @@ async function loadRoster() {
 
 function showClosedOverlay() {
   const overlay = document.getElementById('closed-overlay');
+  if (!overlay.classList.contains('is-visible')) {
+    closedOverlayReturnFocus = document.activeElement;
+  }
   overlay.classList.add('is-visible');
-  overlay.style.display = 'flex';
+  overlay.setAttribute('aria-hidden', 'false');
+  overlay.querySelector('.closed-card')?.focus();
   document.querySelectorAll('.qty-control button').forEach(b => { b.disabled = true; });
   const submitBtn = document.getElementById('submit-btn');
   if (submitBtn) submitBtn.disabled = true;
@@ -413,8 +461,15 @@ function showClosedOverlay() {
 
 function adminBypassClosed() {
   const overlay = document.getElementById('closed-overlay');
+  if (countdownInterval !== null) {
+    clearInterval(countdownInterval);
+    countdownInterval = null;
+  }
+  const previous = closedOverlayReturnFocus;
+  closedOverlayReturnFocus = null;
+  restoreOverlayFocus(previous, '#search');
   overlay.classList.remove('is-visible');
-  overlay.style.display = 'none';
+  overlay.setAttribute('aria-hidden', 'true');
   document.querySelectorAll('.qty-control button').forEach(b => { b.disabled = false; });
   const submitBtn = document.getElementById('submit-btn');
   if (submitBtn) submitBtn.disabled = Object.keys(quantities).length === 0;
@@ -477,8 +532,8 @@ function showAdmin() {
     opt.textContent = w.week + (w.form_status === 'open' ? ' (open)' : '');
     select.appendChild(opt);
   });
-  if (adminWeeks.length > 0 && typeof loadWeekOrders === 'function') {
-    loadWeekOrders(adminWeeks[0].week);
+  if (adminWeeks.length > 0 && typeof window.loadWeekOrders === 'function') {
+    window.loadWeekOrders(adminWeeks[0].week);
   }
 }
 
@@ -520,7 +575,8 @@ function getEasternOffsetMs(date) {
 function startCountdown(data) {
   if (data && data.reopen_at) reopenAtMs = data.reopen_at * 1000;
   updateCountdown();
-  setInterval(updateCountdown, 1000);
+  if (countdownInterval !== null) clearInterval(countdownInterval);
+  countdownInterval = setInterval(updateCountdown, 1000);
 }
 
 function updateCountdown() {
@@ -561,7 +617,6 @@ async function checkFormStatus() {
       formStatusData = data;
       if (isAdmin) {
         adminBypassClosed();
-      } else if (CONFIG.authMode === 'google') {
       } else {
         showClosedOverlay();
       }
@@ -572,6 +627,7 @@ async function checkFormStatus() {
 function init() {
   checkFormStatus();
   if (CONFIG.authMode === 'google') {
+    showAuthOverlay();
     waitForGoogleAuth();
   } else {
     loadRoster();
@@ -615,5 +671,6 @@ window.showAdmin = showAdmin;
 window.hideAdmin = hideAdmin;
 window.submitOrder = submitOrder;
 window.changeQty = changeQty;
+window.handleCredentialResponse = handleCredentialResponse;
 
 init();

tests/test_generate_form.py

@@ -146,6 +146,22 @@ class TestGenerateFormStructural:
         admin_pos = html.index("ADMIN_JS_MARKER")
         assert form_pos < admin_pos
 
+    def test_inline_scripts_have_independent_block_scopes(self):
+        html = _render()
+        assert "<script>\n{\n/* FORM_JS_MARKER */" in html
+        assert "<script>\n{\n/* ADMIN_JS_MARKER */" in html
+
+    def test_overlays_have_dialog_semantics(self):
+        html = _render(google=True)
+        assert (
+            'id="auth-overlay" role="dialog" aria-modal="true" '
+            'aria-labelledby="auth-title"'
+        ) in html
+        assert (
+            'id="closed-overlay" role="dialog" aria-modal="true" '
+            'aria-labelledby="closed-title"'
+        ) in html
+
     def test_single_submit_order_definition(self):
         html = _render(google=True)
         # One async function submitOrder in the inlined form.js
@@ -426,14 +442,16 @@ class TestGenerateFormGooglePlaywright:
         )
         assert dimensions["scrollWidth"] == dimensions["clientWidth"]
 
-    def test_user_bar_stays_single_row_at_480(self, signed_in_admin_page):
+    def test_user_bar_uses_shared_mobile_breakpoint_at_480(
+        self, signed_in_admin_page
+    ):
         page = signed_in_admin_page
         page.set_viewport_size({"width": 480, "height": 800})
         assert (
             page.locator(".user-bar").evaluate(
                 "(element) => getComputedStyle(element).flexWrap"
             )
-            == "nowrap"
+            == "wrap"
         )
         dimensions = page.evaluate(
             """() => ({